Message-ID: <ZsjTLhgubiMN5BXm@apocalypse>
Date: Fri, 23 Aug 2024 20:21:34 +0200
From: Andrea della Porta <andrea.porta@...e.com>
To: Conor Dooley <conor@...nel.org>
Cc: Krzysztof Kozlowski <krzk@...nel.org>,
	Andrea della Porta <andrea.porta@...e.com>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Derek Kiernan <derek.kiernan@....com>,
	Dragan Cvetic <dragan.cvetic@....com>,
	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Nicolas Ferre <nicolas.ferre@...rochip.com>,
	Claudiu Beznea <claudiu.beznea@...on.dev>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Saravana Kannan <saravanak@...gle.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>, linux-clk@...r.kernel.org,
	devicetree@...r.kernel.org, linux-rpi-kernel@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-gpio@...r.kernel.org, netdev@...r.kernel.org,
	linux-pci@...r.kernel.org, linux-arch@...r.kernel.org,
	Lee Jones <lee@...nel.org>, Andrew Lunn <andrew@...n.ch>,
	Stefan Wahren <wahrenst@....net>
Subject: Re: [PATCH 01/11] dt-bindings: clock: Add RaspberryPi RP1 clock
 bindings

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@...00 {
> > >>>>
> > >>>> 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

Powered by blists - more mailing lists