Message-ID: <f39edf3d-aa9e-43a0-8997-762d76c9c248@kernel.org>
Date: Thu, 5 Sep 2024 18:52:10 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Andrea della Porta <andrea.porta@...e.com>
Cc: 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 08/11] misc: rp1: RaspberryPi RP1 misc driver

On 05/09/2024 18:33, Andrea della Porta wrote:
> Hi Krzysztof,
> 
> On 20:27 Tue 03 Sep     , Krzysztof Kozlowski wrote:
>> On 03/09/2024 17:15, Andrea della Porta wrote:
>>>>>>> +
>>>>>>> +				rp1_clocks: clocks@...0018000 {
>>>>>>
>>>>>> Why do you mix MMIO with non-MMIO nodes? This really does not look
>>>>>> correct.
>>>>>>
>>>>>
>>>>> Right. This is already under discussion here:
>>>>> https://lore.kernel.org/all/ZtBzis5CzQMm8loh@apocalypse/
>>>>>
>>>>> IIUC you proposed to instantiate the non-MMIO nodes (the three clocks) by
>>>>> using CLK_OF_DECLARE.
>>>>
>>>> Depends. Where are these clocks? Naming suggests they might not be even
>>>> part of this device. But if these are part of the device, then why this
>>>> is not a clock controller (if they are controllable) or even removed
>>>> (because we do not represent internal clock tree in DTS).
>>>
>>> xosc is a crystal connected to the oscillator input of the RP1, so I would
>>> consider it an external fixed-clock. If we were in the entire dts, I would have
>>> put it in root under /clocks node, but here we're in the dtbo so I'm not sure
>>> where else should I put it.
>>
>> But physically, on which PCB, where is this clock located?
> 
> xosc is a crystal, feeding the reference clock oscillator input pins of the RP1,
> please see page 12 of the following document:
> https://datasheets.raspberrypi.com/rp1/rp1-peripherals.pdf

That's not the answer. Where is it physically located?

> On Rpi5, the PCB is the very same as the one on which the BCM2712 (SoC) and RP1
> are soldered. Would you consider it external (since the crystal is outside the RP1)
> or internal (since the oscillator feeded by the crystal is inside the RP1)?

So it is on RPi 5 board? Just like every other SoC and every other
vendor? Then just like every other SoC and every other vendor it is in
board DTS file.

> 
>>
>>>
>>> Regarding pclk and hclk, I'm still trying to understand where they come from.
>>> If they are external clocks (since they are fixed-clock too), they should be
>>> in the same node as xosc. CLK_OF_DECLARE does not seem to fit here because
>>
>> There is no such node as "/clocks" so do not focus on that. That's just
>> placeholder but useless and it is inconsistent with other cases (e.g.
>> regulators).
> 
> Fine, I beleve that the root node would be okay then, or some other carefully named
> node in root, if the clock is not internal to any chip.
> 
>>
>> If this is external oscillator then it is not part of RP1 and you cannot
>> put it inside just to satisfy your drivers.
> 
> Ack.
> 
>>
>>> there's no special management of these clocks, so no new clock definition is
>>> needed.
>>
>>> If they are internal tree, I cannot simply get rid of them because rp1_eth node
>>> references these two clocks (see clocks property), so they must be decalred 
>>> somewhere. Any hint about this?.
>>>
>>
>> Describe the hardware. Show the diagram or schematics where is which device.
> 
> Unfortunately I don't have the documentation (schematics or other info) about
> how these two clocks (pclk and hclk) are arranged, but I'm trying to get
> some insight about that from various sources. While we're waiting for some
> (hopefully) more certain info, I'd like to speculate a bit. I would say that
> they both probably be either external (just like xosc), or generated internally
> to the RP1:
> 
> If externals, I would place them in the same position as xosc, so root node
> or some other node under root (eg.: /rp1-clocks)

Just like /clocks, /rp1-clocks is not better. Neither /rp1-foo-clocks.

I think there is some sort of big misunderstanding here. Is this RP1
co-processor on the RP board, connected over PCI to Broadcom SoC?

> 
> If internals, I would leave them just where they are, i.e. inside the rp1 node
> 
> Does it make sense?

No, because you do not have xosc there, according to my knowledge.

Best regards,
Krzysztof

Powered by blists - more mailing lists