Re: [PATCH v2 2/3] arm64: dts: qcom: Add base QDU1000/QRU1000 DTSIs

[Stephan Gerhold]

On Thu, Oct 20, 2022 at 12:08:36PM -0400, Krzysztof Kozlowski wrote:
> On 20/10/2022 09:54, Stephan Gerhold wrote:
> > On Thu, Oct 20, 2022 at 08:41:15AM -0400, Krzysztof Kozlowski wrote:
> >> On 19/10/2022 16:21, Melody Olvera wrote:
> >>> On 10/15/2022 6:28 AM, Krzysztof Kozlowski wrote:
> >>>> On 14/10/2022 18:11, Melody Olvera wrote:
> >>>> [...]
> >>>>> +	clocks {
> >>>>> +		xo_board: xo-board {
> >>>>> +			compatible = "fixed-clock";
> >>>>> +			clock-frequency = <19200000>;
> >>>> Both clocks are not a property of a SoC. They are provided by the board,
> >>>> so they should either be defined by board DTS or at least their
> >>>> frequency must be provided by the board.
> >>> That doesn't seem in keeping with precedent.... the sm8* series all have the clocks in
> >>> the dtsi. These are common to the boards anyways.
> >>
> >> Because people do not pay attention what is part of SoC, what is part of
> >> board. DTSI is for the SoC and these are inputs to the SoC.
> >>
> > 
> > (Just chiming in because I had this thought already a few times when you
> >  suggested moving the XO "clock-frequency" to the board DTS:)
> > 
> > I understand your reasoning for moving components of the board to the
> > board DTS, but IMHO adding just the clock-frequency to the board DTS is
> > even more misleading: It suggests that there are functional board
> > designs where you would use a XO clock with a different clock-frequency.
> > Is that really realistic though?
> 
> Keeping it in DTSI also suggests you could have some different frequency.
> 

If the fixed-clock was listed in the SoC DTSI I would personally
consider it as fixed and would not try to modify it in the board DTS.
The way I see it is that the board DTS mostly _extends_ the SoC DTSI
(add peripherals/supplies/...) instead of _modifying_ it.

Anyway this is likely subjective and was not my main point/question. :)

> > 
> > There are assumptions about the XO clock frequency in a lot of places:
> > You would need to fully rewrite the gcc-<SoC>.c driver because it has
> > fixed multipliers/dividers for one specific XO frequency. All firmware
> > binaries would likely need changes. And does the hardware even support a
> > different XO clock frequency? The APQ8016E datasheet for example
> > strictly documents a XO clock input of 19.2 MHz and a sleep clock of
> > 32.768 kHz.
> 
> I know, the same with most of other platforms. Qualcomm is not special
> here. Maybe the difference is that some other platforms have few
> external clocks and not all of them are required.
> 
> > IMHO the only realistic variation of the XO clock setup would be to have
> > a physical "fixed-clock" with a higher frequency, followed by a
> > "fixed-factor-clock" that brings it back to the expected frequency. To
> > model that properly it is not enough to have just the "clock-frequency"
> > in the board DTS. In this case you need two clock nodes, and the
> > xo_board would be the "fixed-factor-clock".
> 
> It's not about whether you can change it or not. It's about describing
> hardware - SoC DTSI describes SoC. DTS describes the board (assuming
> there is no SoM or other DTSI files). This clock is not in DTSI.
> 
> > Therefore it should be all or nothing IMO: Either we move the full
> > xo-board node to the board DTS (which allows alternatively defining the
> > "fixed-factor-clock" or whatever).
> 
> You can move entire clock to boards.
> 
> > Or we assume that there will be
> > always an input clock signal with the fixed frequency and keep it fully
> > in the SoC .dtsi.
> > 
> > Having just the "clock-frequency" in the board DTS puts the attention on
> > the wrong detail, IMO. :)
> 
> No, it puts attention to the board designer that he needs to provide the
> clock in his design.
> 
> We had such talks about other platforms, although I do not have any
> recent bookmarks. Something older:
> 
> https://lore.kernel.org/all/3382034.5ADO0F7naY@wuerfel/
> 
> https://lore.kernel.org/linux-samsung-soc/53DAB0A6.9030700@xxxxxxxxx/
> 

If I understand you correctly your argument for having the clock in the
board DTS instead of the SoC DTSI is:

The SoC DTSI describes the components of the SoC, while the board DTS
describes the components of the board (built around the SoC). The clock
is part of the board (and not the SoC) and therefore belongs into the
board DTS and not the SoC DTSI. Having the SoC/board components clearly
separated ensures people writing new board DTS pay attention to
everything board-specific.

Correct? This sounds reasonable to me.

However, the main question of my previous mail was: Why do you
alternatively recommend to keep the clock defined in the SoC DTSI and to
just put the clock-frequency into the board DTS? This sounds like a
contradiction of the above to me: the clock is still (partially)
described as part of the SoC, even though it belongs to the board.

Someone writing a board DTS should not just put attention to the
clock-frequency, but also if they have a single fixed-clock or
maybe some kind of clock-fixed-factor setup, as I wrote.

Thanks,
Stephan