Re: [PATCH v5 5/5] riscv: dts: spacemit: Add clock tree for Spacemit K1

From: Haylen Chu <heylenay@4d2.org>
To: Inochi Amaoto <inochiama@gmail.com>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Haylen Chu <heylenay@outlook.com>, Yixun Lan <dlan@gentoo.org>
Cc: linux-riscv@lists.infradead.org, linux-clk@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	spacemit@lists.linux.dev, Inochi Amaoto <inochiama@outlook.com>,
	Chen Wang <unicornxdotw@foxmail.com>,
	Jisheng Zhang <jszhang@kernel.org>,
	Meng Zhang <zhangmeng.kevin@linux.spacemit.com>
Subject: Re: [PATCH v5 5/5] riscv: dts: spacemit: Add clock tree for Spacemit K1
Date: Fri, 7 Mar 2025 06:28:16 +0000	[thread overview]
Message-ID: <Z8qSAKXQEfnyVef6@ketchup> (raw)
In-Reply-To: <2isaev6ys2fn2u6lnyudvnjam34fggr5tqh7afajwdjbdp5rvr@ajokoevhqq4p>

On Fri, Mar 07, 2025 at 09:55:15AM +0800, Inochi Amaoto wrote:
> On Thu, Mar 06, 2025 at 05:57:51PM +0000, Haylen Chu wrote:
> > Describe the PLL and system controllers that're capable of generating
> > clock signals in the devicetree.
> > 
> > Signed-off-by: Haylen Chu <heylenay@4d2.org>
> > ---
> >  arch/riscv/boot/dts/spacemit/k1.dtsi | 79 ++++++++++++++++++++++++++++
> >  1 file changed, 79 insertions(+)
> > 
> > diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
> > index c670ebf8fa12..09a9100986b1 100644
> > --- a/arch/riscv/boot/dts/spacemit/k1.dtsi
> > +++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
> > @@ -3,6 +3,8 @@
> >   * Copyright (C) 2024 Yangyu Chen <cyy@cyyself.name>
> >   */
> >  
> > +#include <dt-bindings/clock/spacemit,k1-ccu.h>
> > +
> >  /dts-v1/;
> >  / {
> >  	#address-cells = <2>;
> > @@ -306,6 +308,40 @@ cluster1_l2_cache: l2-cache1 {
> >  		};
> >  	};
> >  
> > +	clocks {
> 
> > +		#address-cells = <0x2>;
> > +		#size-cells = <0x2>;
> > +		ranges;
> 
> why setting this?

I just noticed these unnecessary properties after sending the series,
will drop them, thanks.

> > +
> > +		vctcxo_1m: clock-1m {
> > +			compatible = "fixed-clock";
> 
> > +			clock-frequency = <1000000>;
> 
> Should the frequency this move to the board file?
> I do not think these clock are in the soc.
> This applys to all clock below.

This should be the special case described in the dts-coding-style
documentation,

> A partial exception is a common external reference SoC input clock,
> which could be coded as a fixed-clock in the SoC DTSI with its
> frequency provided by each board DTS.[1]

These four external clocks are essential for K1 SoC to operate as shown
in the clock tree diagram[2]. I think they fit the exception.

Furthermore, the SoC expects these four clocks in fixed rates, thus I
don't see a reason not to include the clock-frequency property here.
Please correct me if I get something wrong.

> > +			clock-output-names = "vctcxo_1m";
> > +			#clock-cells = <0>;
> > +		};
> > +
> > +		vctcxo_24m: clock-24m {
> > +			compatible = "fixed-clock";
> > +			clock-frequency = <24000000>;
> > +			clock-output-names = "vctcxo_24m";
> > +			#clock-cells = <0>;
> > +		};
> > +
> > +		vctcxo_3m: clock-3m {
> > +			compatible = "fixed-clock";
> > +			clock-frequency = <3000000>;
> > +			clock-output-names = "vctcxo_3m";
> > +			#clock-cells = <0>;
> > +		};
> > +
> > +		osc_32k: clock-32k {
> > +			compatible = "fixed-clock";
> > +			clock-frequency = <32000>;
> > +			clock-output-names = "osc_32k";
> > +			#clock-cells = <0>;
> > +		};
> > +	};
> > +
> >  	soc {
> >  		compatible = "simple-bus";
> >  		interrupt-parent = <&plic>;

Thanks,
Haylen Chu

[1]: https://elixir.bootlin.com/linux/v6.11-rc1/source/Documentation/devicetree/bindings/dts-coding-style.rst#L196
[2]: https://developer.spacemit.com/resource/file/images?fileName=E9VPb63n8o1heox2KXHcgWiVnWh.png

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv