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

[Stephen Boyd <sboyd@kernel.org>]

Quoting Inochi Amaoto (2025-03-13 15:46:22)
> On Thu, Mar 13, 2025 at 01:22:28PM -0700, Stephen Boyd wrote:
> > Quoting Inochi Amaoto (2025-03-12 18:08:11)
> > > On Wed, Mar 12, 2025 at 04:43:51PM -0700, Stephen Boyd wrote:
> > > > 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?
> > > > 
> > > 
> > > The PLL clk_ops does nothing, but there is an implicit dependency:
> > > When the PLL change rate, the mux attached to it must switch to 
> > > another source to keep the output clock stable. This is the only
> > > thing it needed.
> > 
> > I haven't looked at the clk_ops in detail (surprise! :) but that sounds
> > a lot like the parent of the mux is the PLL and there's some "safe"
> > source that is needed temporarily while the PLL is reprogrammed for a
> > new rate. Is that right? I recall the notifier is in the driver so this
> > sounds like that sort of design.
> 
> You are right, this design is like what you say. And this design is 
> the reason that I prefer to just reference the syscon node but not
> setting the syscon with "#clock-cell".
> 

I don't see why a syscon phandle is preferred over #clock-cells. This
temporary parent is still a clk, right? In my opinion syscon should
never be used. It signals that we lack a proper framework in the kernel
to handle something. Even in the "miscellaneous" register range sort of
design, we can say that this grab bag of registers is exposing resources
like clks or gpios, etc. as a one off sort of thing because it was too
late to change other hardware blocks.