Re: [PATCH 06/12] riscv: dts: allwinner: Add the D1 SoC base devicetree

[Krzysztof Kozlowski]

On 16/08/2022 14:00, Andre Przywara wrote:
> On Tue, 16 Aug 2022 12:42:39 +0300
> Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> 
> Hi,
> 
>> On 16/08/2022 12:25, Jernej Škrabec wrote:
>>> Dne torek, 16. avgust 2022 ob 11:12:05 CEST je Heiko Stübner napisal(a):  
>>>> Am Dienstag, 16. August 2022, 09:49:58 CEST schrieb Jernej Škrabec:  
>>>>> Dne torek, 16. avgust 2022 ob 09:41:45 CEST je Krzysztof Kozlowski   
>>> napisal(a):  
>>>>>> On 15/08/2022 08:08, Samuel Holland wrote:  
>>>>>>> +
>>>>>>> +	de: display-engine {
>>>>>>> +		compatible = "allwinner,sun20i-d1-display-engine";
>>>>>>> +		allwinner,pipelines = <&mixer0>, <&mixer1>;
>>>>>>> +		status = "disabled";
>>>>>>> +	};
>>>>>>> +
>>>>>>> +	osc24M: osc24M-clk {  
>>>>>>
>>>>>> lowercase
>>>>>>  
>>>>>>> +		compatible = "fixed-clock";
>>>>>>> +		clock-frequency = <24000000>;  
>>>>>>
>>>>>> This is a property of the board, not SoC.  
>>>>>
>>>>> SoC needs 24 MHz oscillator for correct operation, so each and every board
>>>>> has it. Having it here simplifies board DT files.  
>>>>
>>>> I guess the oscillator is a separate component on each board, right?  
>>>
>>> Correct.
>>>   
>>>> And DT obvious is meant to describe the hardware - independently from
>>>> implementation-specific choices.  
>>>
>>> There is no choice in this case. 24 MHz crystal has to be present.
>>>
>>> FWIW, including crystal node in SoC specific DTSI is already common pattern in 
>>> Allwinner ARM SoC DTSI files.
>>>   
>>>>
>>>> Starting to discuss which exceptions to allow then might lead to even more
>>>> exceptions.
>>>>
>>>> Also having to look for a board-component in the soc dtsi also is surprising
>>>> if one gets to the party later on :-) .  
>>>
>>> As I said, if one is accustomed to Allwinner ARM DT development, it would be 
>>> more surprising to include 24 MHz crystal node in each and every board DT.  
>>
>> It's same everywhere. Allwinner, Exynos, iMX, Qualcomm. Everywhere this
>> is a part of the board, so even if oscillator frequency is fixed (as in
>> 99% of cases although some SoCs I think might just allow to implement
>> one of few), still this is a property of the board. Because:
>> 1. DTSI describes the SoC part, not board.
>> 2. So the DTS developer is a bit more conscious about his design.
> 
> 1) is certainly true, but indeed most platforms put the base
> crystal oscillator in the SoC .dtsi:

Yes. And once per week when I look at new DTS I need to repeat the same
arguments. :)

> I just sampled Rockchip (rk3399.dtsi,
> rk356x.dtsi, rk3328.dtsi), Amlogic (meson-g12-common.dtsi), ActionSemi (s[79]00.dtsi),
> Qualcomm (msm8916.dtsi, sm8450.dtsi, sc7180.dtsi), Freescale (imx8mm.dtsi,
> imx8qxp.dtsi), Realtek (rtd129x.dtsi), Broadcom (bcm283x.dtsi), Mediatek
> (mt8183.dtsi, mt8516.dtsi). The list probably goes on (I just stopped
> here).
> 
> I think one reason might be that this is so central to the whole SoC
> operation, that it's already referenced multiple times in the base .dtsi.
> And having a yet unresolved reference in the .dtsi looks dodgy.
> 
> NVidia seems to omit a base oscillator (maybe it's implicit in their
> binding design), Marvell doesn't use a fixed-clock (but still puts their
> base clock in armada-37xx.dtsi).
> 
> Exynos and Renesas put a *stub* fixed-clock in the .dtsi, and set the
> frequency in the board .dts files. Would this be a compromise?

This is exactly what I said before. The clock frequency is a property of
the board. Feel free to keep the rest of the clock in the SoC DTSI to
reduce duplication, but at minimum the clock should go to the board.

Best regards,
Krzysztof