Re: [PATCH 01/11] dt-bindings: clock: Add RaspberryPi RP1 clock bindings

[Andrea della Porta <andrea.porta@suse.com>]

Hi Conor and Krzysztof,

On 17:23 Thu 22 Aug     , Conor Dooley wrote:
> On Thu, Aug 22, 2024 at 11:52:27AM +0200, Krzysztof Kozlowski wrote:
> 
> > >>>>> +examples:
> > >>>>> +  - |
> > >>>>> +    #include <dt-bindings/clock/rp1.h>
> > >>>>> +
> > >>>>> +    rp1 {
> > >>>>> +        #address-cells = <2>;
> > >>>>> +        #size-cells = <2>;
> > >>>>> +
> > >>>>> +        rp1_clocks: clocks@18000 {
> > >>>>
> > >>>> The unit address does not match the reg property. I'm surprised that
> > >>>> dtc doesn't complain about that.
> > >>>
> > >>> Agreed. I'll update the address with the reg value in the next release
> > >>>
> > >>>>
> > >>>>> +            compatible = "raspberrypi,rp1-clocks";
> > >>>>> +            reg = <0xc0 0x40018000 0x0 0x10038>;
> > >>>>
> > >>>> This is a rather oddly specific size. It leads me to wonder if this
> > >>>> region is inside some sort of syscon area?
> > >>>
> > >>> >From downstream source code and RP1 datasheet it seems that the last addressable
> > >>> register is at 0xc040028014 while the range exposed through teh devicetree ends
> > >>> up at 0xc040028038, so it seems more of a little safe margin. I wouldn't say it
> > >>> is a syscon area since those register are quite specific for video clock
> > >>> generation and not to be intended to be shared among different peripherals.
> > >>> Anyway, the next register aperture is at 0xc040030000 so I would say we can 
> > >>> extend the clock mapped register like the following:
> > >>>
> > >>> reg = <0xc0 0x40018000 0x0 0x18000>;
> > >>>
> > >>> if you think it is more readable.
> > >>
> > >> I don't care
> > > 
> > > Ack.
> > > 
> > >>>>> +            #clock-cells = <1>;
> > >>>>> +            clocks = <&clk_xosc>;
> > >>>>> +
> > >>>>> +            assigned-clocks = <&rp1_clocks RP1_PLL_SYS_CORE>,
> > >>>
> > >>>> FWIW, I don't think any of these assigned clocks are helpful for the
> > >>>> example. That said, why do you need to configure all of these assigned
> > >>>> clocks via devicetree when this node is the provider of them?
> > >>>
> > >>> Not sure to understand what you mean here, the example is there just to
> > >>> show how to compile the dt node, maybe you're referring to the fact that
> > >>> the consumer should setup the clock freq?
> > >>
> > >> I suppose, yeah. I don't think a particular configuration is relevant
> > >> for the example binding, but simultaneously don't get why you are
> > >> assigning the rate for clocks used by audio devices or ethernet in the
> > >> clock provider node.
> > >>
> > > 
> > > Honestly I don't have a strong preference here, I can manage to do some tests
> > > moving the clock rate settings inside the consumer nodes but I kinda like
> > > the curernt idea of a centralized node where clocks are setup beforehand.
> > > In RP1 the clock generator and peripherals such as ethernet are all on-board
> > > and cannot be rewired in any other way so the devices are not standalone
> > > consumer in their own right (such it would be an ethernet chip wired to an
> > > external CPU). But of course this is debatable, on the other hand the current
> > > approach of provider/consumer is of course very clean. I'm just wondering
> > > wthether you think I should take action on this or we can leave it as it is.
> > > Please see also below.
> > > 
> > >>> Consider that the rp1-clocks
> > >>> is coupled to the peripherals contained in the same RP1 chip so there is
> > >>> not much point in letting the peripherals set the clock to their leisure.
> > >>
> > >> How is that any different to the many other SoCs in the kernel?
> > > 
> > > In fact, it isn't. Please take a look at:
> > >  
> > > arch/arm/boot/dts/st/stm32mp15xx-dhcom-som.dtsi
> > > arch/arm/boot/dts/ti/omap/omap44xx-clocks.dtsi
> > > arch/arm/boot/dts/ti/omap/dra7xx-clocks.dtsi
> > > arch/arm/boot/dts/nxp/imx/imx7d-zii-rpu2.dts
> > > 
> > > and probably many others... they use the same approach, so I assumed it is at
> > > least reasonable to assign the clock rate this way.
> > 
> > Please do not bring some ancient DTS, not really worked on, as example.
> > stm32 could is moderately recent but dra and omap are not.
> 
> Right, there may be some examples like this, but there are many many
> other SoCs where clocks are also not re-wireable, that do not. To me
> this line of argument is akin to the clock driver calling enable on all
> of the clocks because "all of the peripherals are always on the SoC".
> The peripheral is the actual consumer of the clock that quote-unquote
> wants the particular rate, not the clock provider, so having the rate
> assignments in the consumers is the only thing that makes sense to me.
> 
> 

I'll try to cook something that move the rate definition to the consumer
side, then.

Many thanks,
Andrea