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

[Stephen Boyd <sboyd@kernel.org>]

Quoting Inochi Amaoto (2025-03-11 16:31:29)
> On Tue, Mar 11, 2025 at 12:26:21PM -0700, Stephen Boyd wrote:
> > Quoting Inochi Amaoto (2025-02-26 15:23:18)
> > > diff --git a/Documentation/devicetree/bindings/clock/sophgo,sg2044-clk.yaml b/Documentation/devicetree/bindings/clock/sophgo,sg2044-clk.yaml
> > > new file mode 100644
> > > index 000000000000..d55c5d32e206
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/sophgo,sg2044-clk.yaml
> > > @@ -0,0 +1,40 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/clock/sophgo,sg2044-clk.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Sophgo SG2044 Clock Controller
> > > +
> > > +maintainers:
> > > +  - Inochi Amaoto <inochiama@gmail.com>
> > 
> > No description?
> > 
> 
> I am not sure the things to be described. Maybe just tell the
> clock required and providing?

Sure and point to the header file with the binding numbers?

> > > +  - |
> > > +    clock-controller@50002000 {
> > > +      compatible = "sophgo,sg2044-clk";
> > > +      reg = <0x50002000 0x1000>;
> > > +      #clock-cells = <1>;
> > > +      clocks = <&osc>;
> > 
> > I think you want the syscon phandle here as another property. Doing that
> > will cause the DT parsing logic to wait for the syscon to be probed
> > before trying to probe this driver. It's also useful so we can see if
> > the clock controller is overlapping withe whatever the syscon node is,
> 
> It sounds like a good idea. At now, it does not seem like a good idea
> to hidden the device dependency detail. I will add a syscon property
> like "sophgo,pll-syscon" to identify its pll needs a syscon handle.

Cool.

> 
> > 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? 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.