Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

Li Chen
2023-01-27T14:48:55Z

Hi Krzysztof,
---- On Thu, 26 Jan 2023 19:29:05 +0800 Krzysztof Kozlowski wrote ---
> On 25/01/2023 14:40, Li Chen wrote:
> > On Wed, 25 Jan 2023 20:14:16 +0800,
> >
> > Hi Krzysztof,
> >
> > Krzysztof Kozlowski wrote:
> >>
> >> On 25/01/2023 13:06, Li Chen wrote:
> >>>>> Feel free to correct me if you think this
> >>>>> is not a good idea.
> >>>>
> >>>> This is bad idea. Compatibles should be specific. Devices should not use
> >>>> syscons to poke other registers, unless strictly necessary, but have
> >>>> strictly defined MMIO address space and use it.
> >>>
> >>> Ok, I will convert syscon-based regmaps to SoC-specific compatibles and of_device_id->data.
> >>>
> >>> But I have three questions:
> >>>
> >>> 0. why syscon + offsets is a bad idea copared to specific compatibles?
> >>
> >> Specific compatibles are a requirement. They are needed to match device
> >> in exact way, not some generic and unspecific. The same with every other
> >> interface, it must be specific to allow only correct usage.
> >>
> >> It's of course different with generic fallbacks, but we do not talk
> >> about them here...
> >>
> >>> 1. when would it be a good idea to use syscon in device tree?
> >>
> >> When your device needs to poke one or few registers from some
> >> system-controller block.
> >>
> >>> 2. syscon VS reg, which is preferred in device tree?
> >>
> >> There is no such choice. Your DTS *must* describe the hardware. The
> >> hardware description is for example clock controller which has its own
> >> address space. If you now do not add clock controller's address space to
> >> the clock controller, it is not a proper hardware description. The same
> >> with every other property. If your device has interrupts, but you do not
> >> add them, it is not correct description.
> >
> > Got it. But Ambarella hardware design is kind of strange. I want to add mroe
> > expalaination about why Ambarella's downstream kernel
> > use so much syscon in device trees:
> >
> > For most SoCs from other vendors, they have seperate address space regions
> > for different peripherals, like
> > axi address space A: ENET
> > axi address space B: PCIe
> > axi address space B: USB
> > ...
> >
> > Ambarella is somewhat **different**, its SoCs have two system controllers regions:
> > RCT and scratchpad, take RCT for example:
> > "The S6LM system software
> > interacts with PLLs, PHYs and several other low-level hardware blocks using APB reset clock and test (RCT)
> > registers with a system-layer application programming interface (API).
> > This includes the setting of clock frequencies."
> >
> > There are so many peripherals registers located inside RCT and scratchpad
> > (like usb/phy, gpio, sd, dac, enet, rng), and some peripherals even have no their
> > own modules for register definitions.
>
> Then the syscon is the parent device of these peripherals and clocks.
> You did not represent them as children but as siblings which does not
> look correct.

Ok, I will these syscon(RCT and scratchpad) as the parent node of our clocks and related peripherals.

> >
> > So most time(for a peripheral driver), the only differences between different
> > Ambarella SoCs are just the syscon(rct or scratchpad) offsets get changed.
> >
> > I don't think such lazy hardware design is common in vendors other than ambarella.
> >
> > If I switch to SoC-specific compatibles,
>
> This is independent topic. SoC-specific compatibles are a requirement
> but it does not affect your device hierarchy.

Thanks, "requirement" makes things much more clear. So I will always use SoC-specific compatibles even
if different Amarella SoCs may share the same reg offset and setting.

> > and remove these syscon from device tree,
> > of_device_id->data may only contain system controller(rct or scratchpad) offset for many Ambarella drivers,
> > and ioremap/devm_ioremap carefully.
>
> I don't understand the problem. Neither the solution.
>
> >
> > The question is: can upstream kernel accept such codes?
> >
> > If yes, I will switch to SoC-specific compatibles and remove syscon without hesitation.
>
> Sorry, none of your explanations here match your DTS. Your DTS clearly
> models (for some reason there is no soc which makes even bigger confusion):
>
> rct_syscon
> clocks
> |-gclk-core
> |-gclk-ddr
>
> but what you are saying is that there is no separate clock controller
> device with its own IO address but these clocks are part of rct_syscon.
> Then model it that way in DTS. The rct_syscon is then your clock
> controller and all these fake gclk-core and gclk-ddr nodes should be gone.

Ok, I will remove these fake nodes, and model the hardware as:

rct_syscon node
| clock node(pll, div, mux, composite clocks live in the same driver)
| other periphal nodes

Regards,
Li