[v1,16/16] riscv: dts: starfive: jh8100: Add clocks and resets nodes

Commit Message

Sia Jee Heng Dec. 6, 2023, 11:50 a.m. UTC

Add SYSCRG/SYSCRG-NE/SYSCRG-NW/SYSCRG-SW/AONCRG clock and reset
nodes for JH8100 RISC-V SoC.

Signed-off-by: Sia Jee Heng <[e-mail]>
Reviewed-by: Ley Foon Tan <[e-mail]>
---
 arch/riscv/boot/dts/starfive/jh8100-clk.dtsi | 180 +++++++++++++++++++
 arch/riscv/boot/dts/starfive/jh8100.dtsi     | 115 ++++++++++++
 2 files changed, 295 insertions(+)
 create mode 100644 arch/riscv/boot/dts/starfive/jh8100-clk.dtsi

Comments

Emil Renner Berthing Dec. 8, 2023, 4:39 p.m. UTC | #1

Sia Jee Heng wrote:
> Add SYSCRG/SYSCRG-NE/SYSCRG-NW/SYSCRG-SW/AONCRG clock and reset
> nodes for JH8100 RISC-V SoC.
>
> Signed-off-by: Sia Jee Heng <[e-mail]>
> Reviewed-by: Ley Foon Tan <[e-mail]>
> ---
>  arch/riscv/boot/dts/starfive/jh8100-clk.dtsi | 180 +++++++++++++++++++
>  arch/riscv/boot/dts/starfive/jh8100.dtsi     | 115 ++++++++++++

Why the split here? I mean why can't the clocks just be in the jh8100.dtsi?

>  2 files changed, 295 insertions(+)
>  create mode 100644 arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
>
> diff --git a/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi b/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
> new file mode 100644
> index 000000000000..27ba249f523e
> --- /dev/null
> +++ b/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Copyright (C) 2023 StarFive Technology Co., Ltd.
> + */
> +
> +/ {
> +	clk_osc: clk_osc {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <24000000>;
> +	};
> +
> +	clk_i2srx_bclk_ext: clk_i2srx_bclk_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <12288000>;
> +	};
> +
> +	clk_i2srx_lrck_ext: clk_i2srx_lrck_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <192000>;
> +	};
> +
> +	clk_mclk_ext: clk_mclk_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <49152000>;
> +	};
> +	/* sys-ne */
> +	clk_usb3_tap_tck_ext: clk_usb3_tap_tck_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <100000000>;
> +	};
> +
> +	clk_glb_ext_clk: clk_glb_ext_clk {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <30000000>;
> +	};
> +
> +	clk_usb1_tap_tck_ext: clk_usb1_tap_tck_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <100000000>;
> +	};
> +
> +	clk_usb2_tap_tck_ext: clk_usb2_tap_tck_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <100000000>;
> +	};
> +
> +	clk_i2s_tscko: clk_i2s_tscko {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <12800000>;
> +	};
> +
> +	clk_typec_tap_tck_ext: clk_typec_tap_tck_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <100000000>;
> +	};
> +
> +	clk_spi_in0_ext: clk_spi_in0_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <100000000>;
> +	};
> +
> +	clk_spi_in1_ext: clk_spi_in1_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <100000000>;
> +	};
> +
> +	clk_spi_in2_ext: clk_spi_in2_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <100000000>;
> +	};
> +
> +	clk_i2stx_bclk_ext: clk_i2stx_bclk_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <12288000>;
> +	};
> +
> +	clk_i2stx_lrck_ext: clk_i2stx_lrck_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <192000>;
> +	};
> +	/* sys-nw */
> +	clk_dvp_ext: clk_dvp_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <150000000>;
> +	};
> +
> +	clk_isp_dphy_tap_tck_ext: clk_isp_dphy_tap_tck_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <100000000>;
> +	};
> +
> +	clk_vout_mipi_dphy_tap_tck_ext: clk_vout_mipi_dphy_tap_tck_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <100000000>;
> +	};
> +
> +	clk_vout_edp_tap_tck_ext: clk_vout_edp_tap_tck_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <100000000>;
> +	};
> +
> +	clk_rtc: clk_rtc {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <32768>;
> +	};
> +	/* aon */
> +	clk_gmac0_rmii_func: clk_gmac0_rmii_func {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <50000000>;
> +	};
> +
> +	clk_gmac0_rgmii_func: clk_gmac0_rgmii_func {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <125000000>;
> +	};
> +
> +	clk_aon50: clk_aon50 {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <50000000>;
> +	};
> +
> +	clk_aon125: clk_aon125 {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <125000000>;
> +	};
> +
> +	clk_aon2000: clk_aon2000 {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <2000000000>;
> +	};
> +
> +	clk_aon200: clk_aon200 {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <200000000>;
> +	};
> +
> +	clk_aon667: clk_isp_aon667 {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <667000000>;
> +	};
> +
> +	clk_i3c_ext: clk_i3c_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <12500000>;
> +	};
> +
> +	clk_espi_ext: clk_espi_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <60000000>;
> +	};
> +};
> diff --git a/arch/riscv/boot/dts/starfive/jh8100.dtsi b/arch/riscv/boot/dts/starfive/jh8100.dtsi
> index f26aff5c1ddf..9863c61324a0 100644
> --- a/arch/riscv/boot/dts/starfive/jh8100.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh8100.dtsi
> @@ -4,6 +4,9 @@
>   */
>
>  /dts-v1/;
> +#include <dt-bindings/clock/starfive,jh8100-crg.h>
> +#include <dt-bindings/reset/starfive,jh8100-crg.h>
> +#include "jh8100-clk.dtsi"
>
>  / {
>  	compatible = "starfive,jh8100";
> @@ -357,6 +360,104 @@ uart4: serial@121a0000  {
>  			status = "disabled";
>  		};
>
> +		syscrg_ne: syscrg_ne@12320000 {
> +			compatible = "starfive,jh8100-syscrg-ne";
> +			reg = <0x0 0x12320000 0x0 0x10000>;
> +			clocks = <&clk_osc>, <&syscrg SYSCRG_CLK_AXI_400>,
> +				 <&syscrg SYSCRG_CLK_VOUT_ROOT0>,
> +				 <&syscrg SYSCRG_CLK_VOUT_ROOT1>,
> +				 <&syscrg SYSCRG_CLK_USB_WRAP_480>,
> +				 <&syscrg SYSCRG_CLK_USB_WRAP_625>,
> +				 <&syscrg SYSCRG_CLK_USB_WRAP_240>,
> +				 <&syscrg SYSCRG_CLK_USB_WRAP_60>,
> +				 <&syscrg SYSCRG_CLK_USB_WRAP_156P25>,
> +				 <&syscrg SYSCRG_CLK_USB_WRAP_312P5>,
> +				 <&syscrg SYSCRG_CLK_USB_125M>,
> +				 <&syscrg_nw SYSCRG_NW_CLK_GPIO_100>,
> +				 <&syscrg SYSCRG_CLK_PERH_ROOT>, <&syscrg SYSCRG_CLK_MCLK>,
> +				 <&syscrg SYSCRG_CLK_PERH_ROOT_PREOSC>,
> +				 <&syscrg SYSCRG_CLK_AHB0>,
> +				 <&syscrg SYSCRG_CLK_APB_BUS_PER1>,
> +				 <&syscrg SYSCRG_CLK_APB_BUS_PER2>,
> +				 <&syscrg SYSCRG_CLK_APB_BUS_PER3>,
> +				 <&syscrg SYSCRG_CLK_APB_BUS_PER5>,
> +				 <&syscrg SYSCRG_CLK_VENC_ROOT>,
> +				 <&syscrg SYSCRG_CLK_SPI_CORE_100>,
> +				 <&clk_glb_ext_clk>, <&clk_usb3_tap_tck_ext>,
> +				 <&clk_usb1_tap_tck_ext>, <&clk_usb2_tap_tck_ext>,
> +				 <&clk_typec_tap_tck_ext>, <&clk_spi_in0_ext>,
> +				 <&clk_spi_in1_ext>, <&clk_i2stx_bclk_ext>, <&clk_i2stx_lrck_ext>;
> +			clock-names = "clk_osc", "sys_clk_axi_400",
> +				      "sys_clk_vout_root0", "sys_clk_vout_root1",
> +				      "sys_clk_usb_wrap_480", "sys_clk_usb_wrap_625",
> +				      "sys_clk_usb_wrap_240", "sys_clk_usb_wrap_60",
> +				      "sys_clk_usb_wrap_156p25", "sys_clk_usb_wrap_312p5",
> +				      "sys_clk_usb_125m", "sys_nw_clk_gpio_100",
> +				      "sys_clk_perh_root", "sys_clk_mclk",
> +				      "sys_clk_perh_root_preosc", "sys_clk_ahb0",
> +				      "sys_clk_apb_bus_per1", "sys_clk_apb_bus_per2",
> +				      "sys_clk_apb_bus_per3", "sys_clk_apb_bus_per5",
> +				      "sys_clk_venc_root", "sys_clk_spi_core_100",
> +				      "clk_glb_ext_clk", "clk_usb3_tap_tck_ext",
> +				      "clk_usb1_tap_tck_ext", "clk_usb2_tap_tck_ext",
> +				      "clk_typec_tap_tck_ext", "clk_spi_in0_ext",
> +				      "clk_spi_in1_ext", "clk_i2stx_bclk_ext",
> +				      "clk_i2stx_lrck_ext";
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +		};
> +
> +		syscrg_nw: syscrg_nw@123c0000 {
> +			compatible = "starfive,jh8100-syscrg-nw";
> +			reg = <0x0 0x123c0000 0x0 0x10000>;
> +			clocks = <&clk_osc>, <&syscrg SYSCRG_CLK_APB_BUS>,
> +				 <&syscrg SYSCRG_CLK_ISP_2X>, <&syscrg SYSCRG_CLK_ISP_AXI>,
> +				 <&syscrg SYSCRG_CLK_VOUT_ROOT0>, <&syscrg SYSCRG_CLK_VOUT_ROOT1>,
> +				 <&syscrg SYSCRG_CLK_VOUT_SCAN_ATS>,
> +				 <&syscrg SYSCRG_CLK_VOUT_DC_CORE>, <&syscrg SYSCRG_CLK_VOUT_AXI>,
> +				 <&syscrg SYSCRG_CLK_AXI_400>, <&syscrg SYSCRG_CLK_AXI_200>,
> +				 <&syscrg SYSCRG_CLK_PERH_ROOT_PREOSC>,
> +				 <&clk_dvp_ext>, <&clk_isp_dphy_tap_tck_ext>,
> +				 <&clk_glb_ext_clk>, <&clk_i2s_tscko>,
> +				 <&clk_vout_mipi_dphy_tap_tck_ext>, <&clk_vout_edp_tap_tck_ext>,
> +				 <&clk_spi_in2_ext>;
> +			clock-names = "clk_osc", "sys_clk_apb_bus",
> +				      "sys_clk_isp_2x", "sys_clk_isp_axi",
> +				      "sys_clk_vout_root0", "sys_clk_vout_root1",
> +				      "sys_clk_vout_scan_ats", "sys_clk_vout_dc_core",
> +				      "sys_clk_vout_axi", "sys_clk_axi_400",
> +				      "sys_clk_axi_200", "sys_clk_perh_root_preosc", "clk_dvp_ext",
> +				      "clk_isp_dphy_tap_tck_ext", "clk_glb_ext_clk",
> +				      "clk_i2s_tscko", "clk_vout_mipi_dphy_tap_tck_ext",
> +				      "clk_vout_edp_tap_tck_ext", "clk_spi_in2_ext";
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +		};
> +
> +		syscrg: syscrg@126d0000 {
> +			compatible = "starfive,jh8100-syscrg";
> +			reg = <0x0 0x126d0000 0x0 0x10000>;
> +			clocks = <&clk_osc>, <&clk_i2srx_bclk_ext>,
> +				 <&clk_i2srx_lrck_ext>, <&clk_mclk_ext>;
> +			clock-names = "clk_osc", "clk_i2srx_bclk_ext",
> +				      "clk_i2srx_lrck_ext", "clk_mclk_ext";
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +		};
> +
> +		syscrg_sw: syscrg_sw@12720000 {
> +			compatible = "starfive,jh8100-syscrg-sw";
> +			reg = <0x0 0x12720000 0x0 0x10000>;
> +			clocks = <&syscrg SYSCRG_CLK_APB_BUS>,
> +				 <&syscrg SYSCRG_CLK_VDEC_ROOT>,
> +				 <&syscrg SYSCRG_CLK_FLEXNOC1>;
> +			clock-names = "sys_clk_apb_bus",
> +				      "sys_clk_vdec_root",
> +				      "sys_clk_flexnoc1";
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +		};
> +
>  		uart5: serial@127d0000  {
>  			compatible = "starfive,jh8100-uart", "cdns,uart-r1p8";
>  			reg = <0x0 0x127d0000 0x0 0x10000>;
> @@ -374,5 +475,19 @@ uart6: serial@127e0000  {
>  			interrupts = <73>;
>  			status = "disabled";
>  		};
> +
> +		aoncrg: aoncrg@1f310000 {
> +			compatible = "starfive,jh8100-aoncrg";
> +			reg = <0x0 0x1f310000 0x0 0x10000>;
> +			clocks = <&clk_osc>, <&clk_gmac0_rmii_func>,
> +				 <&clk_gmac0_rgmii_func>, <&clk_aon125>,
> +				 <&clk_aon2000>, <&clk_aon200>,
> +				 <&clk_aon667>, <&clk_rtc>;
> +			clock-names = "clk_osc", "clk_gmac0_rmii_func", "clk_gmac0_rgmii_func",
> +				      "clk_aon125", "clk_aon2000", "clk_aon200",
> +				      "clk_aon667", "clk_rtc";
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +		};
>  	};
>  };
> --
> 2.34.1
>

Krzysztof Kozlowski Dec. 8, 2023, 5:57 p.m. UTC | #2

On 06/12/2023 12:50, Sia Jee Heng wrote:
> Add SYSCRG/SYSCRG-NE/SYSCRG-NW/SYSCRG-SW/AONCRG clock and reset
> nodes for JH8100 RISC-V SoC.
> 
> Signed-off-by: Sia Jee Heng <[e-mail]>
> Reviewed-by: Ley Foon Tan <[e-mail]>

Really? Looks automated... Care to provide any links to effects of
internal review?

> ---
>  arch/riscv/boot/dts/starfive/jh8100-clk.dtsi | 180 +++++++++++++++++++
>  arch/riscv/boot/dts/starfive/jh8100.dtsi     | 115 ++++++++++++
>  2 files changed, 295 insertions(+)
>  create mode 100644 arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi b/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
> new file mode 100644
> index 000000000000..27ba249f523e
> --- /dev/null
> +++ b/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Copyright (C) 2023 StarFive Technology Co., Ltd.
> + */
> +
> +/ {
> +	clk_osc: clk_osc {

No underscores in node names.

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <24000000>;
> +	};
> +

...

> diff --git a/arch/riscv/boot/dts/starfive/jh8100.dtsi b/arch/riscv/boot/dts/starfive/jh8100.dtsi
> index f26aff5c1ddf..9863c61324a0 100644
> --- a/arch/riscv/boot/dts/starfive/jh8100.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh8100.dtsi
> @@ -4,6 +4,9 @@
>   */
>  
>  /dts-v1/;
> +#include <dt-bindings/clock/starfive,jh8100-crg.h>
> +#include <dt-bindings/reset/starfive,jh8100-crg.h>
> +#include "jh8100-clk.dtsi"
>  
>  / {
>  	compatible = "starfive,jh8100";
> @@ -357,6 +360,104 @@ uart4: serial@121a0000  {
>  			status = "disabled";
>  		};
>  
> +		syscrg_ne: syscrg_ne@12320000 {

clock-controller@

Just open your bindings and take a look how it is done there...

This applies everywhere

Best regards,
Krzysztof

Krzysztof Kozlowski Dec. 8, 2023, 5:57 p.m. UTC | #3

On 08/12/2023 17:39, Emil Renner Berthing wrote:
> Sia Jee Heng wrote:
>> Add SYSCRG/SYSCRG-NE/SYSCRG-NW/SYSCRG-SW/AONCRG clock and reset
>> nodes for JH8100 RISC-V SoC.
>>
>> Signed-off-by: Sia Jee Heng <[e-mail]>
>> Reviewed-by: Ley Foon Tan <[e-mail]>
>> ---
>>  arch/riscv/boot/dts/starfive/jh8100-clk.dtsi | 180 +++++++++++++++++++
>>  arch/riscv/boot/dts/starfive/jh8100.dtsi     | 115 ++++++++++++
> 
> Why the split here? I mean why can't the clocks just be in the jh8100.dtsi?

There should be. What's the point? Clocks are internal part of SoC and
not really re-usable piece of hardware.

Best regards,
Krzysztof

Sia Jee Heng Dec. 12, 2023, 1:07 a.m. UTC | #4

> -----Original Message-----
> From: Emil Renner Berthing <[e-mail]>
> Sent: Saturday, December 9, 2023 12:40 AM
> To: JeeHeng Sia <[e-mail]>; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]; Hal Feng
> <[e-mail]>; Xingyu Wu <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail]; [e-mail]; Leyfoon Tan
> <[e-mail]>
> Subject: Re: [PATCH v1 16/16] riscv: dts: starfive: jh8100: Add clocks and resets nodes
> 
> Sia Jee Heng wrote:
> > Add SYSCRG/SYSCRG-NE/SYSCRG-NW/SYSCRG-SW/AONCRG clock and reset
> > nodes for JH8100 RISC-V SoC.
> >
> > Signed-off-by: Sia Jee Heng <[e-mail]>
> > Reviewed-by: Ley Foon Tan <[e-mail]>
> > ---
> >  arch/riscv/boot/dts/starfive/jh8100-clk.dtsi | 180 +++++++++++++++++++
> >  arch/riscv/boot/dts/starfive/jh8100.dtsi     | 115 ++++++++++++
> 
> Why the split here? I mean why can't the clocks just be in the jh8100.dtsi?
The reason is that the number of fixed clocks increases when more and more
domain clocks are added. So, that is why we split out the clock file.
> 
> >  2 files changed, 295 insertions(+)
> >  create mode 100644 arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
> >
> > diff --git a/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi b/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
> > new file mode 100644
> > index 000000000000..27ba249f523e
> > --- /dev/null
> > +++ b/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
> > @@ -0,0 +1,180 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/*
> > + * Copyright (C) 2023 StarFive Technology Co., Ltd.
> > + */
> > +
> > +/ {
> > +	clk_osc: clk_osc {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <24000000>;
> > +	};
> > +
> > +	clk_i2srx_bclk_ext: clk_i2srx_bclk_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <12288000>;
> > +	};
> > +
> > +	clk_i2srx_lrck_ext: clk_i2srx_lrck_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <192000>;
> > +	};
> > +
> > +	clk_mclk_ext: clk_mclk_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <49152000>;
> > +	};
> > +	/* sys-ne */
> > +	clk_usb3_tap_tck_ext: clk_usb3_tap_tck_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <100000000>;
> > +	};
> > +
> > +	clk_glb_ext_clk: clk_glb_ext_clk {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <30000000>;
> > +	};
> > +
> > +	clk_usb1_tap_tck_ext: clk_usb1_tap_tck_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <100000000>;
> > +	};
> > +
> > +	clk_usb2_tap_tck_ext: clk_usb2_tap_tck_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <100000000>;
> > +	};
> > +
> > +	clk_i2s_tscko: clk_i2s_tscko {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <12800000>;
> > +	};
> > +
> > +	clk_typec_tap_tck_ext: clk_typec_tap_tck_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <100000000>;
> > +	};
> > +
> > +	clk_spi_in0_ext: clk_spi_in0_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <100000000>;
> > +	};
> > +
> > +	clk_spi_in1_ext: clk_spi_in1_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <100000000>;
> > +	};
> > +
> > +	clk_spi_in2_ext: clk_spi_in2_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <100000000>;
> > +	};
> > +
> > +	clk_i2stx_bclk_ext: clk_i2stx_bclk_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <12288000>;
> > +	};
> > +
> > +	clk_i2stx_lrck_ext: clk_i2stx_lrck_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <192000>;
> > +	};
> > +	/* sys-nw */
> > +	clk_dvp_ext: clk_dvp_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <150000000>;
> > +	};
> > +
> > +	clk_isp_dphy_tap_tck_ext: clk_isp_dphy_tap_tck_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <100000000>;
> > +	};
> > +
> > +	clk_vout_mipi_dphy_tap_tck_ext: clk_vout_mipi_dphy_tap_tck_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <100000000>;
> > +	};
> > +
> > +	clk_vout_edp_tap_tck_ext: clk_vout_edp_tap_tck_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <100000000>;
> > +	};
> > +
> > +	clk_rtc: clk_rtc {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <32768>;
> > +	};
> > +	/* aon */
> > +	clk_gmac0_rmii_func: clk_gmac0_rmii_func {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <50000000>;
> > +	};
> > +
> > +	clk_gmac0_rgmii_func: clk_gmac0_rgmii_func {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <125000000>;
> > +	};
> > +
> > +	clk_aon50: clk_aon50 {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <50000000>;
> > +	};
> > +
> > +	clk_aon125: clk_aon125 {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <125000000>;
> > +	};
> > +
> > +	clk_aon2000: clk_aon2000 {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <2000000000>;
> > +	};
> > +
> > +	clk_aon200: clk_aon200 {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <200000000>;
> > +	};
> > +
> > +	clk_aon667: clk_isp_aon667 {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <667000000>;
> > +	};
> > +
> > +	clk_i3c_ext: clk_i3c_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <12500000>;
> > +	};
> > +
> > +	clk_espi_ext: clk_espi_ext {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <60000000>;
> > +	};
> > +};
> > diff --git a/arch/riscv/boot/dts/starfive/jh8100.dtsi b/arch/riscv/boot/dts/starfive/jh8100.dtsi
> > index f26aff5c1ddf..9863c61324a0 100644
> > --- a/arch/riscv/boot/dts/starfive/jh8100.dtsi
> > +++ b/arch/riscv/boot/dts/starfive/jh8100.dtsi
> > @@ -4,6 +4,9 @@
> >   */
> >
> >  /dts-v1/;
> > +#include <dt-bindings/clock/starfive,jh8100-crg.h>
> > +#include <dt-bindings/reset/starfive,jh8100-crg.h>
> > +#include "jh8100-clk.dtsi"
> >
> >  / {
> >  	compatible = "starfive,jh8100";
> > @@ -357,6 +360,104 @@ uart4: serial@121a0000  {
> >  			status = "disabled";
> >  		};
> >
> > +		syscrg_ne: syscrg_ne@12320000 {
> > +			compatible = "starfive,jh8100-syscrg-ne";
> > +			reg = <0x0 0x12320000 0x0 0x10000>;
> > +			clocks = <&clk_osc>, <&syscrg SYSCRG_CLK_AXI_400>,
> > +				 <&syscrg SYSCRG_CLK_VOUT_ROOT0>,
> > +				 <&syscrg SYSCRG_CLK_VOUT_ROOT1>,
> > +				 <&syscrg SYSCRG_CLK_USB_WRAP_480>,
> > +				 <&syscrg SYSCRG_CLK_USB_WRAP_625>,
> > +				 <&syscrg SYSCRG_CLK_USB_WRAP_240>,
> > +				 <&syscrg SYSCRG_CLK_USB_WRAP_60>,
> > +				 <&syscrg SYSCRG_CLK_USB_WRAP_156P25>,
> > +				 <&syscrg SYSCRG_CLK_USB_WRAP_312P5>,
> > +				 <&syscrg SYSCRG_CLK_USB_125M>,
> > +				 <&syscrg_nw SYSCRG_NW_CLK_GPIO_100>,
> > +				 <&syscrg SYSCRG_CLK_PERH_ROOT>, <&syscrg SYSCRG_CLK_MCLK>,
> > +				 <&syscrg SYSCRG_CLK_PERH_ROOT_PREOSC>,
> > +				 <&syscrg SYSCRG_CLK_AHB0>,
> > +				 <&syscrg SYSCRG_CLK_APB_BUS_PER1>,
> > +				 <&syscrg SYSCRG_CLK_APB_BUS_PER2>,
> > +				 <&syscrg SYSCRG_CLK_APB_BUS_PER3>,
> > +				 <&syscrg SYSCRG_CLK_APB_BUS_PER5>,
> > +				 <&syscrg SYSCRG_CLK_VENC_ROOT>,
> > +				 <&syscrg SYSCRG_CLK_SPI_CORE_100>,
> > +				 <&clk_glb_ext_clk>, <&clk_usb3_tap_tck_ext>,
> > +				 <&clk_usb1_tap_tck_ext>, <&clk_usb2_tap_tck_ext>,
> > +				 <&clk_typec_tap_tck_ext>, <&clk_spi_in0_ext>,
> > +				 <&clk_spi_in1_ext>, <&clk_i2stx_bclk_ext>, <&clk_i2stx_lrck_ext>;
> > +			clock-names = "clk_osc", "sys_clk_axi_400",
> > +				      "sys_clk_vout_root0", "sys_clk_vout_root1",
> > +				      "sys_clk_usb_wrap_480", "sys_clk_usb_wrap_625",
> > +				      "sys_clk_usb_wrap_240", "sys_clk_usb_wrap_60",
> > +				      "sys_clk_usb_wrap_156p25", "sys_clk_usb_wrap_312p5",
> > +				      "sys_clk_usb_125m", "sys_nw_clk_gpio_100",
> > +				      "sys_clk_perh_root", "sys_clk_mclk",
> > +				      "sys_clk_perh_root_preosc", "sys_clk_ahb0",
> > +				      "sys_clk_apb_bus_per1", "sys_clk_apb_bus_per2",
> > +				      "sys_clk_apb_bus_per3", "sys_clk_apb_bus_per5",
> > +				      "sys_clk_venc_root", "sys_clk_spi_core_100",
> > +				      "clk_glb_ext_clk", "clk_usb3_tap_tck_ext",
> > +				      "clk_usb1_tap_tck_ext", "clk_usb2_tap_tck_ext",
> > +				      "clk_typec_tap_tck_ext", "clk_spi_in0_ext",
> > +				      "clk_spi_in1_ext", "clk_i2stx_bclk_ext",
> > +				      "clk_i2stx_lrck_ext";
> > +			#clock-cells = <1>;
> > +			#reset-cells = <1>;
> > +		};
> > +
> > +		syscrg_nw: syscrg_nw@123c0000 {
> > +			compatible = "starfive,jh8100-syscrg-nw";
> > +			reg = <0x0 0x123c0000 0x0 0x10000>;
> > +			clocks = <&clk_osc>, <&syscrg SYSCRG_CLK_APB_BUS>,
> > +				 <&syscrg SYSCRG_CLK_ISP_2X>, <&syscrg SYSCRG_CLK_ISP_AXI>,
> > +				 <&syscrg SYSCRG_CLK_VOUT_ROOT0>, <&syscrg SYSCRG_CLK_VOUT_ROOT1>,
> > +				 <&syscrg SYSCRG_CLK_VOUT_SCAN_ATS>,
> > +				 <&syscrg SYSCRG_CLK_VOUT_DC_CORE>, <&syscrg SYSCRG_CLK_VOUT_AXI>,
> > +				 <&syscrg SYSCRG_CLK_AXI_400>, <&syscrg SYSCRG_CLK_AXI_200>,
> > +				 <&syscrg SYSCRG_CLK_PERH_ROOT_PREOSC>,
> > +				 <&clk_dvp_ext>, <&clk_isp_dphy_tap_tck_ext>,
> > +				 <&clk_glb_ext_clk>, <&clk_i2s_tscko>,
> > +				 <&clk_vout_mipi_dphy_tap_tck_ext>, <&clk_vout_edp_tap_tck_ext>,
> > +				 <&clk_spi_in2_ext>;
> > +			clock-names = "clk_osc", "sys_clk_apb_bus",
> > +				      "sys_clk_isp_2x", "sys_clk_isp_axi",
> > +				      "sys_clk_vout_root0", "sys_clk_vout_root1",
> > +				      "sys_clk_vout_scan_ats", "sys_clk_vout_dc_core",
> > +				      "sys_clk_vout_axi", "sys_clk_axi_400",
> > +				      "sys_clk_axi_200", "sys_clk_perh_root_preosc", "clk_dvp_ext",
> > +				      "clk_isp_dphy_tap_tck_ext", "clk_glb_ext_clk",
> > +				      "clk_i2s_tscko", "clk_vout_mipi_dphy_tap_tck_ext",
> > +				      "clk_vout_edp_tap_tck_ext", "clk_spi_in2_ext";
> > +			#clock-cells = <1>;
> > +			#reset-cells = <1>;
> > +		};
> > +
> > +		syscrg: syscrg@126d0000 {
> > +			compatible = "starfive,jh8100-syscrg";
> > +			reg = <0x0 0x126d0000 0x0 0x10000>;
> > +			clocks = <&clk_osc>, <&clk_i2srx_bclk_ext>,
> > +				 <&clk_i2srx_lrck_ext>, <&clk_mclk_ext>;
> > +			clock-names = "clk_osc", "clk_i2srx_bclk_ext",
> > +				      "clk_i2srx_lrck_ext", "clk_mclk_ext";
> > +			#clock-cells = <1>;
> > +			#reset-cells = <1>;
> > +		};
> > +
> > +		syscrg_sw: syscrg_sw@12720000 {
> > +			compatible = "starfive,jh8100-syscrg-sw";
> > +			reg = <0x0 0x12720000 0x0 0x10000>;
> > +			clocks = <&syscrg SYSCRG_CLK_APB_BUS>,
> > +				 <&syscrg SYSCRG_CLK_VDEC_ROOT>,
> > +				 <&syscrg SYSCRG_CLK_FLEXNOC1>;
> > +			clock-names = "sys_clk_apb_bus",
> > +				      "sys_clk_vdec_root",
> > +				      "sys_clk_flexnoc1";
> > +			#clock-cells = <1>;
> > +			#reset-cells = <1>;
> > +		};
> > +
> >  		uart5: serial@127d0000  {
> >  			compatible = "starfive,jh8100-uart", "cdns,uart-r1p8";
> >  			reg = <0x0 0x127d0000 0x0 0x10000>;
> > @@ -374,5 +475,19 @@ uart6: serial@127e0000  {
> >  			interrupts = <73>;
> >  			status = "disabled";
> >  		};
> > +
> > +		aoncrg: aoncrg@1f310000 {
> > +			compatible = "starfive,jh8100-aoncrg";
> > +			reg = <0x0 0x1f310000 0x0 0x10000>;
> > +			clocks = <&clk_osc>, <&clk_gmac0_rmii_func>,
> > +				 <&clk_gmac0_rgmii_func>, <&clk_aon125>,
> > +				 <&clk_aon2000>, <&clk_aon200>,
> > +				 <&clk_aon667>, <&clk_rtc>;
> > +			clock-names = "clk_osc", "clk_gmac0_rmii_func", "clk_gmac0_rgmii_func",
> > +				      "clk_aon125", "clk_aon2000", "clk_aon200",
> > +				      "clk_aon667", "clk_rtc";
> > +			#clock-cells = <1>;
> > +			#reset-cells = <1>;
> > +		};
> >  	};
> >  };
> > --
> > 2.34.1
> >

Sia Jee Heng Dec. 12, 2023, 2:51 a.m. UTC | #5

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Saturday, December 9, 2023 1:58 AM
> To: Emil Renner Berthing <[e-mail]>; JeeHeng Sia <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]; Hal Feng
> <[e-mail]>; Xingyu Wu <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail]; [e-mail]; Leyfoon Tan
> <[e-mail]>
> Subject: Re: [PATCH v1 16/16] riscv: dts: starfive: jh8100: Add clocks and resets nodes
> 
> On 08/12/2023 17:39, Emil Renner Berthing wrote:
> > Sia Jee Heng wrote:
> >> Add SYSCRG/SYSCRG-NE/SYSCRG-NW/SYSCRG-SW/AONCRG clock and reset
> >> nodes for JH8100 RISC-V SoC.
> >>
> >> Signed-off-by: Sia Jee Heng <[e-mail]>
> >> Reviewed-by: Ley Foon Tan <[e-mail]>
> >> ---
> >>  arch/riscv/boot/dts/starfive/jh8100-clk.dtsi | 180 +++++++++++++++++++
> >>  arch/riscv/boot/dts/starfive/jh8100.dtsi     | 115 ++++++++++++
> >
> > Why the split here? I mean why can't the clocks just be in the jh8100.dtsi?
> 
> There should be. What's the point? Clocks are internal part of SoC and
> not really re-usable piece of hardware.
Can move it back to the SoC.dtsi
> 
> Best regards,
> Krzysztof

Sia Jee Heng Dec. 12, 2023, 2:58 a.m. UTC | #6

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Saturday, December 9, 2023 1:57 AM
> To: JeeHeng Sia <[e-mail]>; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]; Hal Feng
> <[e-mail]>; Xingyu Wu <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail]; [e-mail]; Leyfoon Tan
> <[e-mail]>
> Subject: Re: [PATCH v1 16/16] riscv: dts: starfive: jh8100: Add clocks and resets nodes
> 
> On 06/12/2023 12:50, Sia Jee Heng wrote:
> > Add SYSCRG/SYSCRG-NE/SYSCRG-NW/SYSCRG-SW/AONCRG clock and reset
> > nodes for JH8100 RISC-V SoC.
> >
> > Signed-off-by: Sia Jee Heng <[e-mail]>
> > Reviewed-by: Ley Foon Tan <[e-mail]>
> 
> Really? Looks automated... Care to provide any links to effects of
> internal review?
https://gitlab.starfivetech.com/jeeheng.sia/linux/-/commits/JH8100_Upstream/
> 
> > ---
> >  arch/riscv/boot/dts/starfive/jh8100-clk.dtsi | 180 +++++++++++++++++++
> >  arch/riscv/boot/dts/starfive/jh8100.dtsi     | 115 ++++++++++++
> >  2 files changed, 295 insertions(+)
> >  create mode 100644 arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
> >
> > diff --git a/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi b/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
> > new file mode 100644
> > index 000000000000..27ba249f523e
> > --- /dev/null
> > +++ b/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
> > @@ -0,0 +1,180 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/*
> > + * Copyright (C) 2023 StarFive Technology Co., Ltd.
> > + */
> > +
> > +/ {
> > +	clk_osc: clk_osc {
> 
> No underscores in node names.
Noted.
> 
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <24000000>;
> > +	};
> > +
> 
> ...
> 
> > diff --git a/arch/riscv/boot/dts/starfive/jh8100.dtsi b/arch/riscv/boot/dts/starfive/jh8100.dtsi
> > index f26aff5c1ddf..9863c61324a0 100644
> > --- a/arch/riscv/boot/dts/starfive/jh8100.dtsi
> > +++ b/arch/riscv/boot/dts/starfive/jh8100.dtsi
> > @@ -4,6 +4,9 @@
> >   */
> >
> >  /dts-v1/;
> > +#include <dt-bindings/clock/starfive,jh8100-crg.h>
> > +#include <dt-bindings/reset/starfive,jh8100-crg.h>
> > +#include "jh8100-clk.dtsi"
> >
> >  / {
> >  	compatible = "starfive,jh8100";
> > @@ -357,6 +360,104 @@ uart4: serial@121a0000  {
> >  			status = "disabled";
> >  		};
> >
> > +		syscrg_ne: syscrg_ne@12320000 {
> 
> clock-controller@
> 
> Just open your bindings and take a look how it is done there...
> 
> This applies everywhere
> 
> Best regards,
> Krzysztof

Krzysztof Kozlowski Dec. 12, 2023, 8:43 a.m. UTC | #7

On 12/12/2023 03:58, JeeHeng Sia wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Saturday, December 9, 2023 1:57 AM
>> To: JeeHeng Sia <[e-mail]>; [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]; [e-mail]; Hal Feng
>> <[e-mail]>; Xingyu Wu <[e-mail]>
>> Cc: [e-mail]; [e-mail]; [e-mail]; [e-mail]; Leyfoon Tan
>> <[e-mail]>
>> Subject: Re: [PATCH v1 16/16] riscv: dts: starfive: jh8100: Add clocks and resets nodes
>>
>> On 06/12/2023 12:50, Sia Jee Heng wrote:
>>> Add SYSCRG/SYSCRG-NE/SYSCRG-NW/SYSCRG-SW/AONCRG clock and reset
>>> nodes for JH8100 RISC-V SoC.
>>>
>>> Signed-off-by: Sia Jee Heng <[e-mail]>
>>> Reviewed-by: Ley Foon Tan <[e-mail]>
>>
>> Really? Looks automated... Care to provide any links to effects of
>> internal review?
> https://gitlab.starfivetech.com/jeeheng.sia/linux/-/commits/JH8100_Upstream/
>>
>>> ---
>>>  arch/riscv/boot/dts/starfive/jh8100-clk.dtsi | 180 +++++++++++++++++++
>>>  arch/riscv/boot/dts/starfive/jh8100.dtsi     | 115 ++++++++++++
>>>  2 files changed, 295 insertions(+)
>>>  create mode 100644 arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
>>>
>>> diff --git a/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi b/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
>>> new file mode 100644
>>> index 000000000000..27ba249f523e
>>> --- /dev/null
>>> +++ b/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
>>> @@ -0,0 +1,180 @@
>>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>>> +/*
>>> + * Copyright (C) 2023 StarFive Technology Co., Ltd.
>>> + */
>>> +
>>> +/ {
>>> +	clk_osc: clk_osc {
>>
>> No underscores in node names.
> Noted.
>>
>>> +		compatible = "fixed-clock";
>>> +		#clock-cells = <0>;
>>> +		clock-frequency = <24000000>;
>>> +	};
>>> +
>>
>> ...
>>
>>> diff --git a/arch/riscv/boot/dts/starfive/jh8100.dtsi b/arch/riscv/boot/dts/starfive/jh8100.dtsi
>>> index f26aff5c1ddf..9863c61324a0 100644
>>> --- a/arch/riscv/boot/dts/starfive/jh8100.dtsi
>>> +++ b/arch/riscv/boot/dts/starfive/jh8100.dtsi
>>> @@ -4,6 +4,9 @@
>>>   */
>>>
>>>  /dts-v1/;
>>> +#include <dt-bindings/clock/starfive,jh8100-crg.h>
>>> +#include <dt-bindings/reset/starfive,jh8100-crg.h>
>>> +#include "jh8100-clk.dtsi"
>>>
>>>  / {
>>>  	compatible = "starfive,jh8100";
>>> @@ -357,6 +360,104 @@ uart4: serial@121a0000  {
>>>  			status = "disabled";
>>>  		};
>>>
>>> +		syscrg_ne: syscrg_ne@12320000 {
>>
>> clock-controller@
>>
>> Just open your bindings and take a look how it is done there...
>>
>> This applies everywhere

I assume you did not ignore all the other comments you did not respond to.

Best regards,
Krzysztof

Sia Jee Heng Dec. 12, 2023, 10:03 a.m. UTC | #8

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Tuesday, December 12, 2023 4:44 PM
> To: JeeHeng Sia <[e-mail]>; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]; Hal Feng
> <[e-mail]>; Xingyu Wu <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail]; [e-mail]; Leyfoon Tan
> <[e-mail]>
> Subject: Re: [PATCH v1 16/16] riscv: dts: starfive: jh8100: Add clocks and resets nodes
> 
> On 12/12/2023 03:58, JeeHeng Sia wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: Saturday, December 9, 2023 1:57 AM
> >> To: JeeHeng Sia <[e-mail]>; [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail]; [e-mail]; Hal Feng
> >> <[e-mail]>; Xingyu Wu <[e-mail]>
> >> Cc: [e-mail]; [e-mail]; [e-mail]; [e-mail]; Leyfoon
> Tan
> >> <[e-mail]>
> >> Subject: Re: [PATCH v1 16/16] riscv: dts: starfive: jh8100: Add clocks and resets nodes
> >>
> >> On 06/12/2023 12:50, Sia Jee Heng wrote:
> >>> Add SYSCRG/SYSCRG-NE/SYSCRG-NW/SYSCRG-SW/AONCRG clock and reset
> >>> nodes for JH8100 RISC-V SoC.
> >>>
> >>> Signed-off-by: Sia Jee Heng <[e-mail]>
> >>> Reviewed-by: Ley Foon Tan <[e-mail]>
> >>
> >> Really? Looks automated... Care to provide any links to effects of
> >> internal review?
> > https://gitlab.starfivetech.com/jeeheng.sia/linux/-/commits/JH8100_Upstream/
> >>
> >>> ---
> >>>  arch/riscv/boot/dts/starfive/jh8100-clk.dtsi | 180 +++++++++++++++++++
> >>>  arch/riscv/boot/dts/starfive/jh8100.dtsi     | 115 ++++++++++++
> >>>  2 files changed, 295 insertions(+)
> >>>  create mode 100644 arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
> >>>
> >>> diff --git a/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi b/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
> >>> new file mode 100644
> >>> index 000000000000..27ba249f523e
> >>> --- /dev/null
> >>> +++ b/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
> >>> @@ -0,0 +1,180 @@
> >>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> >>> +/*
> >>> + * Copyright (C) 2023 StarFive Technology Co., Ltd.
> >>> + */
> >>> +
> >>> +/ {
> >>> +	clk_osc: clk_osc {
> >>
> >> No underscores in node names.
> > Noted.
> >>
> >>> +		compatible = "fixed-clock";
> >>> +		#clock-cells = <0>;
> >>> +		clock-frequency = <24000000>;
> >>> +	};
> >>> +
> >>
> >> ...
> >>
> >>> diff --git a/arch/riscv/boot/dts/starfive/jh8100.dtsi b/arch/riscv/boot/dts/starfive/jh8100.dtsi
> >>> index f26aff5c1ddf..9863c61324a0 100644
> >>> --- a/arch/riscv/boot/dts/starfive/jh8100.dtsi
> >>> +++ b/arch/riscv/boot/dts/starfive/jh8100.dtsi
> >>> @@ -4,6 +4,9 @@
> >>>   */
> >>>
> >>>  /dts-v1/;
> >>> +#include <dt-bindings/clock/starfive,jh8100-crg.h>
> >>> +#include <dt-bindings/reset/starfive,jh8100-crg.h>
> >>> +#include "jh8100-clk.dtsi"
> >>>
> >>>  / {
> >>>  	compatible = "starfive,jh8100";
> >>> @@ -357,6 +360,104 @@ uart4: serial@121a0000  {
> >>>  			status = "disabled";
> >>>  		};
> >>>
> >>> +		syscrg_ne: syscrg_ne@12320000 {
> >>
> >> clock-controller@
> >>
> >> Just open your bindings and take a look how it is done there...
> >>
> >> This applies everywhere
> 
> I assume you did not ignore all the other comments you did not respond to.
Arr, my bad. Will fix it.
> 
> Best regards,
> Krzysztof

Patch

diff --git a/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi b/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
new file mode 100644
index 000000000000..27ba249f523e
--- /dev/null
+++ b/arch/riscv/boot/dts/starfive/jh8100-clk.dtsi
@@ -0,0 +1,180 @@ 
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * Copyright (C) 2023 StarFive Technology Co., Ltd.
+ */
+
+/ {
+	clk_osc: clk_osc {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <24000000>;
+	};
+
+	clk_i2srx_bclk_ext: clk_i2srx_bclk_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <12288000>;
+	};
+
+	clk_i2srx_lrck_ext: clk_i2srx_lrck_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <192000>;
+	};
+
+	clk_mclk_ext: clk_mclk_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <49152000>;
+	};
+	/* sys-ne */
+	clk_usb3_tap_tck_ext: clk_usb3_tap_tck_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <100000000>;
+	};
+
+	clk_glb_ext_clk: clk_glb_ext_clk {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <30000000>;
+	};
+
+	clk_usb1_tap_tck_ext: clk_usb1_tap_tck_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <100000000>;
+	};
+
+	clk_usb2_tap_tck_ext: clk_usb2_tap_tck_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <100000000>;
+	};
+
+	clk_i2s_tscko: clk_i2s_tscko {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <12800000>;
+	};
+
+	clk_typec_tap_tck_ext: clk_typec_tap_tck_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <100000000>;
+	};
+
+	clk_spi_in0_ext: clk_spi_in0_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <100000000>;
+	};
+
+	clk_spi_in1_ext: clk_spi_in1_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <100000000>;
+	};
+
+	clk_spi_in2_ext: clk_spi_in2_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <100000000>;
+	};
+
+	clk_i2stx_bclk_ext: clk_i2stx_bclk_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <12288000>;
+	};
+
+	clk_i2stx_lrck_ext: clk_i2stx_lrck_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <192000>;
+	};
+	/* sys-nw */
+	clk_dvp_ext: clk_dvp_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <150000000>;
+	};
+
+	clk_isp_dphy_tap_tck_ext: clk_isp_dphy_tap_tck_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <100000000>;
+	};
+
+	clk_vout_mipi_dphy_tap_tck_ext: clk_vout_mipi_dphy_tap_tck_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <100000000>;
+	};
+
+	clk_vout_edp_tap_tck_ext: clk_vout_edp_tap_tck_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <100000000>;
+	};
+
+	clk_rtc: clk_rtc {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <32768>;
+	};
+	/* aon */
+	clk_gmac0_rmii_func: clk_gmac0_rmii_func {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <50000000>;
+	};
+
+	clk_gmac0_rgmii_func: clk_gmac0_rgmii_func {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <125000000>;
+	};
+
+	clk_aon50: clk_aon50 {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <50000000>;
+	};
+
+	clk_aon125: clk_aon125 {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <125000000>;
+	};
+
+	clk_aon2000: clk_aon2000 {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <2000000000>;
+	};
+
+	clk_aon200: clk_aon200 {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <200000000>;
+	};
+
+	clk_aon667: clk_isp_aon667 {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <667000000>;
+	};
+
+	clk_i3c_ext: clk_i3c_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <12500000>;
+	};
+
+	clk_espi_ext: clk_espi_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <60000000>;
+	};
+};
diff --git a/arch/riscv/boot/dts/starfive/jh8100.dtsi b/arch/riscv/boot/dts/starfive/jh8100.dtsi
index f26aff5c1ddf..9863c61324a0 100644
--- a/arch/riscv/boot/dts/starfive/jh8100.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh8100.dtsi
@@ -4,6 +4,9 @@ 
  */
 
 /dts-v1/;
+#include <dt-bindings/clock/starfive,jh8100-crg.h>
+#include <dt-bindings/reset/starfive,jh8100-crg.h>
+#include "jh8100-clk.dtsi"
 
 / {
 	compatible = "starfive,jh8100";
@@ -357,6 +360,104 @@  uart4: serial@121a0000  {
 			status = "disabled";
 		};
 
+		syscrg_ne: syscrg_ne@12320000 {
+			compatible = "starfive,jh8100-syscrg-ne";
+			reg = <0x0 0x12320000 0x0 0x10000>;
+			clocks = <&clk_osc>, <&syscrg SYSCRG_CLK_AXI_400>,
+				 <&syscrg SYSCRG_CLK_VOUT_ROOT0>,
+				 <&syscrg SYSCRG_CLK_VOUT_ROOT1>,
+				 <&syscrg SYSCRG_CLK_USB_WRAP_480>,
+				 <&syscrg SYSCRG_CLK_USB_WRAP_625>,
+				 <&syscrg SYSCRG_CLK_USB_WRAP_240>,
+				 <&syscrg SYSCRG_CLK_USB_WRAP_60>,
+				 <&syscrg SYSCRG_CLK_USB_WRAP_156P25>,
+				 <&syscrg SYSCRG_CLK_USB_WRAP_312P5>,
+				 <&syscrg SYSCRG_CLK_USB_125M>,
+				 <&syscrg_nw SYSCRG_NW_CLK_GPIO_100>,
+				 <&syscrg SYSCRG_CLK_PERH_ROOT>, <&syscrg SYSCRG_CLK_MCLK>,
+				 <&syscrg SYSCRG_CLK_PERH_ROOT_PREOSC>,
+				 <&syscrg SYSCRG_CLK_AHB0>,
+				 <&syscrg SYSCRG_CLK_APB_BUS_PER1>,
+				 <&syscrg SYSCRG_CLK_APB_BUS_PER2>,
+				 <&syscrg SYSCRG_CLK_APB_BUS_PER3>,
+				 <&syscrg SYSCRG_CLK_APB_BUS_PER5>,
+				 <&syscrg SYSCRG_CLK_VENC_ROOT>,
+				 <&syscrg SYSCRG_CLK_SPI_CORE_100>,
+				 <&clk_glb_ext_clk>, <&clk_usb3_tap_tck_ext>,
+				 <&clk_usb1_tap_tck_ext>, <&clk_usb2_tap_tck_ext>,
+				 <&clk_typec_tap_tck_ext>, <&clk_spi_in0_ext>,
+				 <&clk_spi_in1_ext>, <&clk_i2stx_bclk_ext>, <&clk_i2stx_lrck_ext>;
+			clock-names = "clk_osc", "sys_clk_axi_400",
+				      "sys_clk_vout_root0", "sys_clk_vout_root1",
+				      "sys_clk_usb_wrap_480", "sys_clk_usb_wrap_625",
+				      "sys_clk_usb_wrap_240", "sys_clk_usb_wrap_60",
+				      "sys_clk_usb_wrap_156p25", "sys_clk_usb_wrap_312p5",
+				      "sys_clk_usb_125m", "sys_nw_clk_gpio_100",
+				      "sys_clk_perh_root", "sys_clk_mclk",
+				      "sys_clk_perh_root_preosc", "sys_clk_ahb0",
+				      "sys_clk_apb_bus_per1", "sys_clk_apb_bus_per2",
+				      "sys_clk_apb_bus_per3", "sys_clk_apb_bus_per5",
+				      "sys_clk_venc_root", "sys_clk_spi_core_100",
+				      "clk_glb_ext_clk", "clk_usb3_tap_tck_ext",
+				      "clk_usb1_tap_tck_ext", "clk_usb2_tap_tck_ext",
+				      "clk_typec_tap_tck_ext", "clk_spi_in0_ext",
+				      "clk_spi_in1_ext", "clk_i2stx_bclk_ext",
+				      "clk_i2stx_lrck_ext";
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+		};
+
+		syscrg_nw: syscrg_nw@123c0000 {
+			compatible = "starfive,jh8100-syscrg-nw";
+			reg = <0x0 0x123c0000 0x0 0x10000>;
+			clocks = <&clk_osc>, <&syscrg SYSCRG_CLK_APB_BUS>,
+				 <&syscrg SYSCRG_CLK_ISP_2X>, <&syscrg SYSCRG_CLK_ISP_AXI>,
+				 <&syscrg SYSCRG_CLK_VOUT_ROOT0>, <&syscrg SYSCRG_CLK_VOUT_ROOT1>,
+				 <&syscrg SYSCRG_CLK_VOUT_SCAN_ATS>,
+				 <&syscrg SYSCRG_CLK_VOUT_DC_CORE>, <&syscrg SYSCRG_CLK_VOUT_AXI>,
+				 <&syscrg SYSCRG_CLK_AXI_400>, <&syscrg SYSCRG_CLK_AXI_200>,
+				 <&syscrg SYSCRG_CLK_PERH_ROOT_PREOSC>,
+				 <&clk_dvp_ext>, <&clk_isp_dphy_tap_tck_ext>,
+				 <&clk_glb_ext_clk>, <&clk_i2s_tscko>,
+				 <&clk_vout_mipi_dphy_tap_tck_ext>, <&clk_vout_edp_tap_tck_ext>,
+				 <&clk_spi_in2_ext>;
+			clock-names = "clk_osc", "sys_clk_apb_bus",
+				      "sys_clk_isp_2x", "sys_clk_isp_axi",
+				      "sys_clk_vout_root0", "sys_clk_vout_root1",
+				      "sys_clk_vout_scan_ats", "sys_clk_vout_dc_core",
+				      "sys_clk_vout_axi", "sys_clk_axi_400",
+				      "sys_clk_axi_200", "sys_clk_perh_root_preosc", "clk_dvp_ext",
+				      "clk_isp_dphy_tap_tck_ext", "clk_glb_ext_clk",
+				      "clk_i2s_tscko", "clk_vout_mipi_dphy_tap_tck_ext",
+				      "clk_vout_edp_tap_tck_ext", "clk_spi_in2_ext";
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+		};
+
+		syscrg: syscrg@126d0000 {
+			compatible = "starfive,jh8100-syscrg";
+			reg = <0x0 0x126d0000 0x0 0x10000>;
+			clocks = <&clk_osc>, <&clk_i2srx_bclk_ext>,
+				 <&clk_i2srx_lrck_ext>, <&clk_mclk_ext>;
+			clock-names = "clk_osc", "clk_i2srx_bclk_ext",
+				      "clk_i2srx_lrck_ext", "clk_mclk_ext";
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+		};
+
+		syscrg_sw: syscrg_sw@12720000 {
+			compatible = "starfive,jh8100-syscrg-sw";
+			reg = <0x0 0x12720000 0x0 0x10000>;
+			clocks = <&syscrg SYSCRG_CLK_APB_BUS>,
+				 <&syscrg SYSCRG_CLK_VDEC_ROOT>,
+				 <&syscrg SYSCRG_CLK_FLEXNOC1>;
+			clock-names = "sys_clk_apb_bus",
+				      "sys_clk_vdec_root",
+				      "sys_clk_flexnoc1";
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+		};
+
 		uart5: serial@127d0000  {
 			compatible = "starfive,jh8100-uart", "cdns,uart-r1p8";
 			reg = <0x0 0x127d0000 0x0 0x10000>;
@@ -374,5 +475,19 @@  uart6: serial@127e0000  {
 			interrupts = <73>;
 			status = "disabled";
 		};
+
+		aoncrg: aoncrg@1f310000 {
+			compatible = "starfive,jh8100-aoncrg";
+			reg = <0x0 0x1f310000 0x0 0x10000>;
+			clocks = <&clk_osc>, <&clk_gmac0_rmii_func>,
+				 <&clk_gmac0_rgmii_func>, <&clk_aon125>,
+				 <&clk_aon2000>, <&clk_aon200>,
+				 <&clk_aon667>, <&clk_rtc>;
+			clock-names = "clk_osc", "clk_gmac0_rmii_func", "clk_gmac0_rgmii_func",
+				      "clk_aon125", "clk_aon2000", "clk_aon200",
+				      "clk_aon667", "clk_rtc";
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+		};
 	};
 };