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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <x43v3wn5rp2mkhmmmyjvdo7aov4l7hnus34wjw7snd2zbtzrbh@r5wrvn3kxxwv>
Date: Thu, 13 Mar 2025 07:29:43 +0800
From: Inochi Amaoto <inochiama@...il.com>
To: Stephen Boyd <sboyd@...nel.org>, Chen Wang <unicorn_wang@...look.com>, 
	Conor Dooley <conor+dt@...nel.org>, Inochi Amaoto <inochiama@...il.com>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Michael Turquette <mturquette@...libre.com>, 
	Richard Cochran <richardcochran@...il.com>, Rob Herring <robh@...nel.org>
Cc: linux-clk@...r.kernel.org, devicetree@...r.kernel.org, 
	sophgo@...ts.linux.dev, linux-kernel@...r.kernel.org, netdev@...r.kernel.org, 
	Yixun Lan <dlan@...too.org>, Longbin Li <looong.bin@...il.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Subject: Re: [PATCH v3 1/2] dt-bindings: clock: sophgo: add clock controller
 for SG2044

On Wed, Mar 12, 2025 at 04:14:37PM -0700, Stephen Boyd wrote:
> 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@...il.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?
> 

Good, I will add it.

> > > > +  - |
> > > > +    clock-controller@...02000 {
> > > > +      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? 

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.

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

Regards,
Inochi