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

Haylen Chu
2025-03-07T06:28:37Z

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