Re: [PATCH 08/11] misc: rp1: RaspberryPi RP1 misc driver

[Krzysztof Kozlowski <krzk@kernel.org>]

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@c040018000 {
>>>>>>
>>>>>> 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