Re: [PATCH v3 1/2] dt-bindings: clock: sophgo: add clock controller for SG2044

[Stephen Boyd]

Quoting Inochi Amaoto (2025-03-12 16:29:43)
> On Wed, Mar 12, 2025 at 04:14:37PM -0700, Stephen Boyd wrote:
> > Quoting Inochi Amaoto (2025-03-11 16:31:29)
> > > 
> > > > or if that syscon node should just have the #clock-cells property as
> > > > part of the node instead.
> > > 
> > > This is not match the hardware I think. The pll area is on the middle
> > > of the syscon and is hard to be separated as a subdevice of the syscon
> > > or just add  "#clock-cells" to the syscon device. It is better to handle
> > > them in one device/driver. So let the clock device reference it.
> > 
> > This happens all the time. We don't need a syscon for that unless the
> > registers for the pll are both inside the syscon and in the register
> > space 0x50002000. Is that the case? 
> 
> Yes, the clock has two areas, one in the clk controller and one in
> the syscon, the vendor said this design is a heritage from other SoC.

My question is more if the PLL clk_ops need to access both the syscon
register range and the clk controller register range. What part of the
PLL clk_ops needs to access the clk controller at 0x50002000?

> 
> > This looks like you want there to be  one node for clks on the system
> > because logically that is clean, when the reality is that there is a
> > PLL block exposed in the syscon (someone forgot to put it in the clk
> > controller?) and a non-PLL block for the other clks.
> 
> That is true, I prefer to keep clean and make less mistakes. Although
> the PLL is exposed in the syscon, the pll need to be tight with other
> clocks in the space 0x50002000 (especially between the PLL and mux).
> In this view, it is more like a mistake made by the hardware design.
> And I prefer not to add a subnode for the syscon.

Ok. You wouldn't add a subnode for the syscon. You would just have
#clock-cells in that syscon node and register an auxiliary device to
provide the PLL(s) from there. Then in drivers/clk we would have an
auxiliary driver that uses a regmap or gets an iomem pointer from the
parent device somehow so that we can logically put the PLL code in
drivers/clk while having one node in DT for the "miscellaneous register
area" where the hardware engineer had to expose the PLL control to
software.