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

[Li Chen]

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