diff mbox series

[v3,6/7] riscv: dts: starfive: Add initial StarFive JH7110 device tree

Message ID 20221220011247.35560-7-hal.feng@starfivetech.com
State New
Headers
Series Basic device tree support for StarFive JH7110 RISC-V SoC |

Commit Message

Hal Feng Dec. 20, 2022, 1:12 a.m. UTC 
  From: Emil Renner Berthing <kernel@esmil.dk>

Add initial device tree for the JH7110 RISC-V SoC by StarFive
Technology Ltd.

Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
---
 arch/riscv/boot/dts/starfive/jh7110.dtsi | 411 +++++++++++++++++++++++
 1 file changed, 411 insertions(+)
 create mode 100644 arch/riscv/boot/dts/starfive/jh7110.dtsi

Comments

Krzysztof Kozlowski Dec. 20, 2022, 10:10 a.m. UTC | #1 
On 20/12/2022 02:12, Hal Feng wrote:
> From: Emil Renner Berthing <kernel@esmil.dk>
> 
> Add initial device tree for the JH7110 RISC-V SoC by StarFive
> Technology Ltd.
> 
> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 411 +++++++++++++++++++++++
>  1 file changed, 411 insertions(+)
>  create mode 100644 arch/riscv/boot/dts/starfive/jh7110.dtsi
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> new file mode 100644
> index 000000000000..64d260ea1f29
> --- /dev/null
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -0,0 +1,411 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + * Copyright (C) 2022 Emil Renner Berthing <kernel@esmil.dk>
> + */
> +
> +/dts-v1/;
> +#include <dt-bindings/clock/starfive,jh7110-crg.h>
> +#include <dt-bindings/reset/starfive,jh7110-crg.h>
> +
> +/ {
> +	compatible = "starfive,jh7110";
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		S76_0: cpu@0 {
> +			compatible = "sifive,u74-mc", "riscv";
> +			reg = <0>;
> +			d-cache-block-size = <64>;
> +			d-cache-sets = <64>;
> +			d-cache-size = <8192>;
> +			d-tlb-sets = <1>;
> +			d-tlb-size = <40>;
> +			device_type = "cpu";
> +			i-cache-block-size = <64>;
> +			i-cache-sets = <64>;
> +			i-cache-size = <16384>;
> +			i-tlb-sets = <1>;
> +			i-tlb-size = <40>;
> +			mmu-type = "riscv,sv39";
> +			next-level-cache = <&ccache>;
> +			riscv,isa = "rv64imac";
> +			tlb-split;
> +			status = "disabled";
> +
> +			cpu0_intc: interrupt-controller {
> +				compatible = "riscv,cpu-intc";
> +				interrupt-controller;
> +				#interrupt-cells = <1>;
> +			};
> +		};
> +
> +		U74_1: cpu@1 {
> +			compatible = "sifive,u74-mc", "riscv";
> +			reg = <1>;
> +			d-cache-block-size = <64>;
> +			d-cache-sets = <64>;
> +			d-cache-size = <32768>;
> +			d-tlb-sets = <1>;
> +			d-tlb-size = <40>;
> +			device_type = "cpu";
> +			i-cache-block-size = <64>;
> +			i-cache-sets = <64>;
> +			i-cache-size = <32768>;
> +			i-tlb-sets = <1>;
> +			i-tlb-size = <40>;
> +			mmu-type = "riscv,sv39";
> +			next-level-cache = <&ccache>;
> +			riscv,isa = "rv64imafdc";
> +			tlb-split;
> +
> +			cpu1_intc: interrupt-controller {
> +				compatible = "riscv,cpu-intc";
> +				interrupt-controller;
> +				#interrupt-cells = <1>;
> +			};
> +		};
> +
> +		U74_2: cpu@2 {
> +			compatible = "sifive,u74-mc", "riscv";
> +			reg = <2>;
> +			d-cache-block-size = <64>;
> +			d-cache-sets = <64>;
> +			d-cache-size = <32768>;
> +			d-tlb-sets = <1>;
> +			d-tlb-size = <40>;
> +			device_type = "cpu";
> +			i-cache-block-size = <64>;
> +			i-cache-sets = <64>;
> +			i-cache-size = <32768>;
> +			i-tlb-sets = <1>;
> +			i-tlb-size = <40>;
> +			mmu-type = "riscv,sv39";
> +			next-level-cache = <&ccache>;
> +			riscv,isa = "rv64imafdc";
> +			tlb-split;
> +
> +			cpu2_intc: interrupt-controller {
> +				compatible = "riscv,cpu-intc";
> +				interrupt-controller;
> +				#interrupt-cells = <1>;
> +			};
> +		};
> +
> +		U74_3: cpu@3 {
> +			compatible = "sifive,u74-mc", "riscv";
> +			reg = <3>;
> +			d-cache-block-size = <64>;
> +			d-cache-sets = <64>;
> +			d-cache-size = <32768>;
> +			d-tlb-sets = <1>;
> +			d-tlb-size = <40>;
> +			device_type = "cpu";
> +			i-cache-block-size = <64>;
> +			i-cache-sets = <64>;
> +			i-cache-size = <32768>;
> +			i-tlb-sets = <1>;
> +			i-tlb-size = <40>;
> +			mmu-type = "riscv,sv39";
> +			next-level-cache = <&ccache>;
> +			riscv,isa = "rv64imafdc";
> +			tlb-split;
> +
> +			cpu3_intc: interrupt-controller {
> +				compatible = "riscv,cpu-intc";
> +				interrupt-controller;
> +				#interrupt-cells = <1>;
> +			};
> +		};
> +
> +		U74_4: cpu@4 {
> +			compatible = "sifive,u74-mc", "riscv";
> +			reg = <4>;
> +			d-cache-block-size = <64>;
> +			d-cache-sets = <64>;
> +			d-cache-size = <32768>;
> +			d-tlb-sets = <1>;
> +			d-tlb-size = <40>;
> +			device_type = "cpu";
> +			i-cache-block-size = <64>;
> +			i-cache-sets = <64>;
> +			i-cache-size = <32768>;
> +			i-tlb-sets = <1>;
> +			i-tlb-size = <40>;
> +			mmu-type = "riscv,sv39";
> +			next-level-cache = <&ccache>;
> +			riscv,isa = "rv64imafdc";
> +			tlb-split;
> +
> +			cpu4_intc: interrupt-controller {
> +				compatible = "riscv,cpu-intc";
> +				interrupt-controller;
> +				#interrupt-cells = <1>;
> +			};
> +		};
> +
> +		cpu-map {
> +			cluster0 {
> +				core0 {
> +					cpu = <&S76_0>;
> +				};
> +
> +				core1 {
> +					cpu = <&U74_1>;
> +				};
> +
> +				core2 {
> +					cpu = <&U74_2>;
> +				};
> +
> +				core3 {
> +					cpu = <&U74_3>;
> +				};
> +
> +				core4 {
> +					cpu = <&U74_4>;
> +				};
> +			};
> +		};
> +	};
> +
> +	osc: osc {

Node names should be generic, so why this is "osc" and other oscillators
are not "osc"?


> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +	};
> +
> +	rtc_osc: rtc_osc {

No underscores in node names. Generic node names (so each of them
starting or ending with clock).

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +	};
> +
> +	gmac0_rmii_refin: gmac0_rmii_refin {

Same problem... and actually you have way too many fixed clocks which do
nothing. It looks like you avoid to define proper clock controller.
What's the point for all these clocks? These are no-op.

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +	};
> +
> +	gmac0_rgmii_rxin: gmac0_rgmii_rxin {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +	};
> +
> +	gmac1_rmii_refin: gmac1_rmii_refin {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +	};
> +
> +	gmac1_rgmii_rxin: gmac1_rgmii_rxin {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +	};
> +
> +	i2stx_bclk_ext: i2stx_bclk_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +	};
> +
> +	i2stx_lrck_ext: i2stx_lrck_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +	};
> +
> +	i2srx_bclk_ext: i2srx_bclk_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +	};
> +
> +	i2srx_lrck_ext: i2srx_lrck_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +	};
> +
> +	tdm_ext: tdm_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +	};
> +
> +	mclk_ext: mclk_ext {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +	};
> +
> +	soc {
> +		compatible = "simple-bus";
> +		interrupt-parent = <&plic>;
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		clint: clint@2000000 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +			compatible = "starfive,jh7110-clint", "sifive,clint0";
> +			reg = <0x0 0x2000000 0x0 0x10000>;
> +			interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>,
> +					      <&cpu1_intc 3>, <&cpu1_intc 7>,
> +					      <&cpu2_intc 3>, <&cpu2_intc 7>,
> +					      <&cpu3_intc 3>, <&cpu3_intc 7>,
> +					      <&cpu4_intc 3>, <&cpu4_intc 7>;
> +		};
> +
> +		plic: plic@c000000 {

Node names should be generic, so interrupt-controller
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +			compatible = "starfive,jh7110-plic", "sifive,plic-1.0.0";
> +			reg = <0x0 0xc000000 0x0 0x4000000>;
> +			interrupts-extended = <&cpu0_intc 11>,
> +					      <&cpu1_intc 11>, <&cpu1_intc 9>,
> +					      <&cpu2_intc 11>, <&cpu2_intc 9>,
> +					      <&cpu3_intc 11>, <&cpu3_intc 9>,
> +					      <&cpu4_intc 11>, <&cpu4_intc 9>;
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +			#address-cells = <0>;
> +			riscv,ndev = <136>;
> +		};
> +
> +		ccache: cache-controller@2010000 {
> +			compatible = "starfive,jh7110-ccache", "sifive,ccache0", "cache";
> +			reg = <0x0 0x2010000 0x0 0x4000>;
> +			interrupts = <1>, <3>, <4>, <2>;
> +			cache-block-size = <64>;
> +			cache-level = <2>;
> +			cache-sets = <2048>;
> +			cache-size = <2097152>;
> +			cache-unified;
> +		};
> +
> +		uart0: serial@10000000 {
> +			compatible = "snps,dw-apb-uart";
> +			reg = <0x0 0x10000000 0x0 0x10000>;
> +			clocks = <&syscrg JH7110_SYSCLK_UART0_CORE>,
> +				 <&syscrg JH7110_SYSCLK_UART0_APB>;
> +			clock-names = "baudclk", "apb_pclk";
> +			resets = <&syscrg JH7110_SYSRST_UART0_APB>;
> +			interrupts = <32>;
> +			reg-io-width = <4>;
> +			reg-shift = <2>;
> +			status = "disabled";
> +		};
> +

Best regards,
Krzysztof
Conor Dooley Dec. 20, 2022, 9:31 p.m. UTC | #2 
On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
> From: Emil Renner Berthing <kernel@esmil.dk>
> 
> Add initial device tree for the JH7110 RISC-V SoC by StarFive
> Technology Ltd.
> 
> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---

FWIW, this cpu-map is now the default in linux, so you no longer *need*
to add it for that purpose - but there's obviously no harm in being
explicit for other operating systems etc. (IOW, don't remove it!)

> +		cpu-map {
> +			cluster0 {
> +				core0 {
> +					cpu = <&S76_0>;
> +				};
> +
> +				core1 {
> +					cpu = <&U74_1>;
> +				};
> +
> +				core2 {
> +					cpu = <&U74_2>;
> +				};
> +
> +				core3 {
> +					cpu = <&U74_3>;
> +				};
> +
> +				core4 {
> +					cpu = <&U74_4>;
> +				};
> +			};
> +		};
> +	};

> +		syscrg: clock-controller@13020000 {

For obvious reasons, I cannot apply this until both the clock & pinctrl
bindings are in my tree - but you know that already.

> +			compatible = "starfive,jh7110-syscrg";
> +			reg = <0x0 0x13020000 0x0 0x10000>;
> +			clocks = <&osc>, <&gmac1_rmii_refin>,
> +				 <&gmac1_rgmii_rxin>,
> +				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
> +				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
> +				 <&tdm_ext>, <&mclk_ext>;

As Krzk asked - are these clocks really all inputs to the SoC?

> +			clock-names = "osc", "gmac1_rmii_refin",
> +				      "gmac1_rgmii_rxin",
> +				      "i2stx_bclk_ext", "i2stx_lrck_ext",
> +				      "i2srx_bclk_ext", "i2srx_lrck_ext",
> +				      "tdm_ext", "mclk_ext";
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +		};
> +
> +		gpio: gpio@13040000 {

> +		gpioa: gpio@17020000 {

Out of curiousity, why gpio & gpioa?

Thanks,
Conor.
Hal Feng Dec. 25, 2022, 10:31 a.m. UTC | #3 
On Tue, 20 Dec 2022 11:10:11 +0100, Krzysztof Kozlowski wrote:
> On 20/12/2022 02:12, Hal Feng wrote:
>> From: Emil Renner Berthing <kernel@esmil.dk>
>> 
>> Add initial device tree for the JH7110 RISC-V SoC by StarFive
>> Technology Ltd.
>> 
>> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
>> Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
>> Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
>> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>> ---
>>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 411 +++++++++++++++++++++++
>>  1 file changed, 411 insertions(+)
>>  create mode 100644 arch/riscv/boot/dts/starfive/jh7110.dtsi
>> 
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> new file mode 100644
>> index 000000000000..64d260ea1f29
>> --- /dev/null
>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> @@ -0,0 +1,411 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +/*
>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> + * Copyright (C) 2022 Emil Renner Berthing <kernel@esmil.dk>
>> + */
>> +
>> +/dts-v1/;
>> +#include <dt-bindings/clock/starfive,jh7110-crg.h>
>> +#include <dt-bindings/reset/starfive,jh7110-crg.h>
>> +
>> +/ {
>> +	compatible = "starfive,jh7110";
>> +	#address-cells = <2>;
>> +	#size-cells = <2>;
>> +
>> +	cpus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		S76_0: cpu@0 {
>> +			compatible = "sifive,u74-mc", "riscv";
>> +			reg = <0>;
>> +			d-cache-block-size = <64>;
>> +			d-cache-sets = <64>;
>> +			d-cache-size = <8192>;
>> +			d-tlb-sets = <1>;
>> +			d-tlb-size = <40>;
>> +			device_type = "cpu";
>> +			i-cache-block-size = <64>;
>> +			i-cache-sets = <64>;
>> +			i-cache-size = <16384>;
>> +			i-tlb-sets = <1>;
>> +			i-tlb-size = <40>;
>> +			mmu-type = "riscv,sv39";
>> +			next-level-cache = <&ccache>;
>> +			riscv,isa = "rv64imac";
>> +			tlb-split;
>> +			status = "disabled";
>> +
>> +			cpu0_intc: interrupt-controller {
>> +				compatible = "riscv,cpu-intc";
>> +				interrupt-controller;
>> +				#interrupt-cells = <1>;
>> +			};
>> +		};
>> +
>> +		U74_1: cpu@1 {
>> +			compatible = "sifive,u74-mc", "riscv";
>> +			reg = <1>;
>> +			d-cache-block-size = <64>;
>> +			d-cache-sets = <64>;
>> +			d-cache-size = <32768>;
>> +			d-tlb-sets = <1>;
>> +			d-tlb-size = <40>;
>> +			device_type = "cpu";
>> +			i-cache-block-size = <64>;
>> +			i-cache-sets = <64>;
>> +			i-cache-size = <32768>;
>> +			i-tlb-sets = <1>;
>> +			i-tlb-size = <40>;
>> +			mmu-type = "riscv,sv39";
>> +			next-level-cache = <&ccache>;
>> +			riscv,isa = "rv64imafdc";
>> +			tlb-split;
>> +
>> +			cpu1_intc: interrupt-controller {
>> +				compatible = "riscv,cpu-intc";
>> +				interrupt-controller;
>> +				#interrupt-cells = <1>;
>> +			};
>> +		};
>> +
>> +		U74_2: cpu@2 {
>> +			compatible = "sifive,u74-mc", "riscv";
>> +			reg = <2>;
>> +			d-cache-block-size = <64>;
>> +			d-cache-sets = <64>;
>> +			d-cache-size = <32768>;
>> +			d-tlb-sets = <1>;
>> +			d-tlb-size = <40>;
>> +			device_type = "cpu";
>> +			i-cache-block-size = <64>;
>> +			i-cache-sets = <64>;
>> +			i-cache-size = <32768>;
>> +			i-tlb-sets = <1>;
>> +			i-tlb-size = <40>;
>> +			mmu-type = "riscv,sv39";
>> +			next-level-cache = <&ccache>;
>> +			riscv,isa = "rv64imafdc";
>> +			tlb-split;
>> +
>> +			cpu2_intc: interrupt-controller {
>> +				compatible = "riscv,cpu-intc";
>> +				interrupt-controller;
>> +				#interrupt-cells = <1>;
>> +			};
>> +		};
>> +
>> +		U74_3: cpu@3 {
>> +			compatible = "sifive,u74-mc", "riscv";
>> +			reg = <3>;
>> +			d-cache-block-size = <64>;
>> +			d-cache-sets = <64>;
>> +			d-cache-size = <32768>;
>> +			d-tlb-sets = <1>;
>> +			d-tlb-size = <40>;
>> +			device_type = "cpu";
>> +			i-cache-block-size = <64>;
>> +			i-cache-sets = <64>;
>> +			i-cache-size = <32768>;
>> +			i-tlb-sets = <1>;
>> +			i-tlb-size = <40>;
>> +			mmu-type = "riscv,sv39";
>> +			next-level-cache = <&ccache>;
>> +			riscv,isa = "rv64imafdc";
>> +			tlb-split;
>> +
>> +			cpu3_intc: interrupt-controller {
>> +				compatible = "riscv,cpu-intc";
>> +				interrupt-controller;
>> +				#interrupt-cells = <1>;
>> +			};
>> +		};
>> +
>> +		U74_4: cpu@4 {
>> +			compatible = "sifive,u74-mc", "riscv";
>> +			reg = <4>;
>> +			d-cache-block-size = <64>;
>> +			d-cache-sets = <64>;
>> +			d-cache-size = <32768>;
>> +			d-tlb-sets = <1>;
>> +			d-tlb-size = <40>;
>> +			device_type = "cpu";
>> +			i-cache-block-size = <64>;
>> +			i-cache-sets = <64>;
>> +			i-cache-size = <32768>;
>> +			i-tlb-sets = <1>;
>> +			i-tlb-size = <40>;
>> +			mmu-type = "riscv,sv39";
>> +			next-level-cache = <&ccache>;
>> +			riscv,isa = "rv64imafdc";
>> +			tlb-split;
>> +
>> +			cpu4_intc: interrupt-controller {
>> +				compatible = "riscv,cpu-intc";
>> +				interrupt-controller;
>> +				#interrupt-cells = <1>;
>> +			};
>> +		};
>> +
>> +		cpu-map {
>> +			cluster0 {
>> +				core0 {
>> +					cpu = <&S76_0>;
>> +				};
>> +
>> +				core1 {
>> +					cpu = <&U74_1>;
>> +				};
>> +
>> +				core2 {
>> +					cpu = <&U74_2>;
>> +				};
>> +
>> +				core3 {
>> +					cpu = <&U74_3>;
>> +				};
>> +
>> +				core4 {
>> +					cpu = <&U74_4>;
>> +				};
>> +			};
>> +		};
>> +	};
>> +
>> +	osc: osc {
> 
> Node names should be generic, so why this is "osc" and other oscillators
> are not "osc"?

Only "osc" and "rtc_osc" are oscillators, the rest are clock sources provided
through gpio. I will modify the node names according to your link below. So

	osc: oscillator {

> 
> 
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	rtc_osc: rtc_osc {
> 
> No underscores in node names. Generic node names (so each of them
> starting or ending with clock).

Will change this line to

	rtc_osc: oscillator {

> 
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	gmac0_rmii_refin: gmac0_rmii_refin {
> 
> Same problem... and actually you have way too many fixed clocks which do

Will change the node names as follows.

	gmac0_rmii_refin: clock {
		...
	};

	gmac0_rgmii_rxin: clock {
		...
	};
	...

> nothing. It looks like you avoid to define proper clock controller.
> What's the point for all these clocks? These are no-op.

These are all external fixed rate clocks inputted to the SoC. They are the root
clocks of the clock tree made by clock drivers. Their ops are provided in
drivers/clk/clk-fixed-rate.c.

> 
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	gmac0_rgmii_rxin: gmac0_rgmii_rxin {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	gmac1_rmii_refin: gmac1_rmii_refin {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	gmac1_rgmii_rxin: gmac1_rgmii_rxin {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	i2stx_bclk_ext: i2stx_bclk_ext {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	i2stx_lrck_ext: i2stx_lrck_ext {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	i2srx_bclk_ext: i2srx_bclk_ext {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	i2srx_lrck_ext: i2srx_lrck_ext {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	tdm_ext: tdm_ext {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	mclk_ext: mclk_ext {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +	};
>> +
>> +	soc {
>> +		compatible = "simple-bus";
>> +		interrupt-parent = <&plic>;
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		ranges;
>> +
>> +		clint: clint@2000000 {
> 
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Change it to 

		clint: timer@2000000 {

> 
>> +			compatible = "starfive,jh7110-clint", "sifive,clint0";
>> +			reg = <0x0 0x2000000 0x0 0x10000>;
>> +			interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>,
>> +					      <&cpu1_intc 3>, <&cpu1_intc 7>,
>> +					      <&cpu2_intc 3>, <&cpu2_intc 7>,
>> +					      <&cpu3_intc 3>, <&cpu3_intc 7>,
>> +					      <&cpu4_intc 3>, <&cpu4_intc 7>;
>> +		};
>> +
>> +		plic: plic@c000000 {
> 
> Node names should be generic, so interrupt-controller
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Change it to 

		plic: interrupt-controller@c000000 {

Best regards,
Hal

> 
>> +			compatible = "starfive,jh7110-plic", "sifive,plic-1.0.0";
>> +			reg = <0x0 0xc000000 0x0 0x4000000>;
>> +			interrupts-extended = <&cpu0_intc 11>,
>> +					      <&cpu1_intc 11>, <&cpu1_intc 9>,
>> +					      <&cpu2_intc 11>, <&cpu2_intc 9>,
>> +					      <&cpu3_intc 11>, <&cpu3_intc 9>,
>> +					      <&cpu4_intc 11>, <&cpu4_intc 9>;
>> +			interrupt-controller;
>> +			#interrupt-cells = <1>;
>> +			#address-cells = <0>;
>> +			riscv,ndev = <136>;
>> +		};
Krzysztof Kozlowski Dec. 25, 2022, 11:56 a.m. UTC | #4 
On 25/12/2022 11:31, Hal Feng wrote:

> 
> 	gmac0_rmii_refin: clock {
> 		...
> 	};
> 
> 	gmac0_rgmii_rxin: clock {
> 		...
> 	};
> 	...
> 
>> nothing. It looks like you avoid to define proper clock controller.
>> What's the point for all these clocks? These are no-op.
> 
> These are all external fixed rate clocks inputted to the SoC. They are the root

If they are all external, then it is fine.

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

Best regards,
Krzysztof
Hal Feng Dec. 25, 2022, 2:31 p.m. UTC | #5 
On Tue, 20 Dec 2022 21:31:43 +0000, Conor Dooley wrote:
> On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
> > From: Emil Renner Berthing <kernel@esmil.dk>
> > 
> > Add initial device tree for the JH7110 RISC-V SoC by StarFive
> > Technology Ltd.
> > 
> > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> > Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> > Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> > Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > ---
> 
> FWIW, this cpu-map is now the default in linux, so you no longer *need*
> to add it for that purpose - but there's obviously no harm in being
> explicit for other operating systems etc. (IOW, don't remove it!)
> 
> > +		cpu-map {
> > +			cluster0 {
> > +				core0 {
> > +					cpu = <&S76_0>;
> > +				};
> > +
> > +				core1 {
> > +					cpu = <&U74_1>;
> > +				};
> > +
> > +				core2 {
> > +					cpu = <&U74_2>;
> > +				};
> > +
> > +				core3 {
> > +					cpu = <&U74_3>;
> > +				};
> > +
> > +				core4 {
> > +					cpu = <&U74_4>;
> > +				};
> > +			};
> > +		};
> > +	};
> 
> > +		syscrg: clock-controller@13020000 {
> 
> For obvious reasons, I cannot apply this until both the clock & pinctrl
> bindings are in my tree - but you know that already.
> 
> > +			compatible = "starfive,jh7110-syscrg";
> > +			reg = <0x0 0x13020000 0x0 0x10000>;
> > +			clocks = <&osc>, <&gmac1_rmii_refin>,
> > +				 <&gmac1_rgmii_rxin>,
> > +				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
> > +				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
> > +				 <&tdm_ext>, <&mclk_ext>;
> 
> As Krzk asked - are these clocks really all inputs to the SoC?

Yes, they are all external clock sources inputted to the SoC. They are
used as root clocks or optional parent clocks in clock tree.

> 
> > +			clock-names = "osc", "gmac1_rmii_refin",
> > +				      "gmac1_rgmii_rxin",
> > +				      "i2stx_bclk_ext", "i2stx_lrck_ext",
> > +				      "i2srx_bclk_ext", "i2srx_lrck_ext",
> > +				      "tdm_ext", "mclk_ext";
> > +			#clock-cells = <1>;
> > +			#reset-cells = <1>;
> > +		};
> > +
> > +		gpio: gpio@13040000 {
> 
> > +		gpioa: gpio@17020000 {
> 
> Out of curiousity, why gpio & gpioa?

Oh, is it easier to read if I change "gpio" and "gpioa"
to "sysgpio" and "aongpio"? Thanks.

Best regards,
Hal
Conor Dooley Dec. 27, 2022, 8:58 p.m. UTC | #6 
On Sun, Dec 25, 2022 at 10:31:41PM +0800, Hal Feng wrote:
> On Tue, 20 Dec 2022 21:31:43 +0000, Conor Dooley wrote:
> > On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
> > > From: Emil Renner Berthing <kernel@esmil.dk>
> > > 
> > > Add initial device tree for the JH7110 RISC-V SoC by StarFive
> > > Technology Ltd.
> > > 
> > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> > > Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> > > Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> > > Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> > > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > > ---

> > > +		gpio: gpio@13040000 {
> > 
> > > +		gpioa: gpio@17020000 {
> > 
> > Out of curiousity, why gpio & gpioa?
> 
> Oh, is it easier to read if I change "gpio" and "gpioa"
> to "sysgpio" and "aongpio"? Thanks.

I think those would be more reader friendly, that's for sure.
Conor Dooley Dec. 28, 2022, 10:48 p.m. UTC | #7 
Hey,

On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
> From: Emil Renner Berthing <kernel@esmil.dk>
> 
> Add initial device tree for the JH7110 RISC-V SoC by StarFive
> Technology Ltd.
> 
> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 411 +++++++++++++++++++++++
>  1 file changed, 411 insertions(+)
>  create mode 100644 arch/riscv/boot/dts/starfive/jh7110.dtsi
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> new file mode 100644
> index 000000000000..64d260ea1f29
> --- /dev/null
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -0,0 +1,411 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + * Copyright (C) 2022 Emil Renner Berthing <kernel@esmil.dk>
> + */
> +
> +/dts-v1/;
> +#include <dt-bindings/clock/starfive,jh7110-crg.h>
> +#include <dt-bindings/reset/starfive,jh7110-crg.h>
> +
> +/ {
> +	compatible = "starfive,jh7110";
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		S76_0: cpu@0 {
> +			compatible = "sifive,u74-mc", "riscv";

The label here says S76 but the compatible says u74-mc.
Which is correct? Your docs say S7 and S76, so I would imagine that it
is actually an S76?

> +			reg = <0>;
> +			d-cache-block-size = <64>;
> +			d-cache-sets = <64>;
> +			d-cache-size = <8192>;
> +			d-tlb-sets = <1>;
> +			d-tlb-size = <40>;
> +			device_type = "cpu";
> +			i-cache-block-size = <64>;
> +			i-cache-sets = <64>;
> +			i-cache-size = <16384>;
> +			i-tlb-sets = <1>;
> +			i-tlb-size = <40>;
> +			mmu-type = "riscv,sv39";
> +			next-level-cache = <&ccache>;
> +			riscv,isa = "rv64imac";

While I was poking around trying to see if there was some logic behind
that compatible, I noticed that SiFive's docs for the S76 say it is
RV64GBC *but* the docs for the u74-mc say "4xRV64GBC and 1xRV64IMAC".
I assume that rv64imac is the correct one here?

> +			tlb-split;
> +			status = "disabled";
> +
> +			cpu0_intc: interrupt-controller {
> +				compatible = "riscv,cpu-intc";
> +				interrupt-controller;
> +				#interrupt-cells = <1>;
> +			};
> +		};
> +
> +		U74_1: cpu@1 {
> +			compatible = "sifive,u74-mc", "riscv";
> +			reg = <1>;
> +			d-cache-block-size = <64>;
> +			d-cache-sets = <64>;
> +			d-cache-size = <32768>;
> +			d-tlb-sets = <1>;
> +			d-tlb-size = <40>;
> +			device_type = "cpu";
> +			i-cache-block-size = <64>;
> +			i-cache-sets = <64>;
> +			i-cache-size = <32768>;
> +			i-tlb-sets = <1>;
> +			i-tlb-size = <40>;
> +			mmu-type = "riscv,sv39";
> +			next-level-cache = <&ccache>;
> +			riscv,isa = "rv64imafdc";

That also begs the question:
Do your u74s support RV64GBC, as the (current) SiFive documentation
suggests?

Thanks,
Conor.
Icenowy Zheng Dec. 29, 2022, 5:25 a.m. UTC | #8 
在 2022-12-28星期三的 22:48 +0000，Conor Dooley写道：
> Hey,
> 
> On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
> > From: Emil Renner Berthing <kernel@esmil.dk>
> > 
> > Add initial device tree for the JH7110 RISC-V SoC by StarFive
> > Technology Ltd.
> > 
> > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> > Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> > Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> > Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> > ---
> >  arch/riscv/boot/dts/starfive/jh7110.dtsi | 411
> > +++++++++++++++++++++++
> >  1 file changed, 411 insertions(+)
> >  create mode 100644 arch/riscv/boot/dts/starfive/jh7110.dtsi
> > 
> > diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > new file mode 100644
> > index 000000000000..64d260ea1f29
> > --- /dev/null
> > +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > @@ -0,0 +1,411 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/*
> > + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> > + * Copyright (C) 2022 Emil Renner Berthing <kernel@esmil.dk>
> > + */
> > +
> > +/dts-v1/;
> > +#include <dt-bindings/clock/starfive,jh7110-crg.h>
> > +#include <dt-bindings/reset/starfive,jh7110-crg.h>
> > +
> > +/ {
> > +       compatible = "starfive,jh7110";
> > +       #address-cells = <2>;
> > +       #size-cells = <2>;
> > +
> > +       cpus {
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               S76_0: cpu@0 {
> > +                       compatible = "sifive,u74-mc", "riscv";
> 
> The label here says S76 but the compatible says u74-mc.
> Which is correct? Your docs say S7 and S76, so I would imagine that
> it
> is actually an S76?
> 
> > +                       reg = <0>;
> > +                       d-cache-block-size = <64>;
> > +                       d-cache-sets = <64>;
> > +                       d-cache-size = <8192>;
> > +                       d-tlb-sets = <1>;
> > +                       d-tlb-size = <40>;
> > +                       device_type = "cpu";
> > +                       i-cache-block-size = <64>;
> > +                       i-cache-sets = <64>;
> > +                       i-cache-size = <16384>;
> > +                       i-tlb-sets = <1>;
> > +                       i-tlb-size = <40>;
> > +                       mmu-type = "riscv,sv39";
> > +                       next-level-cache = <&ccache>;
> > +                       riscv,isa = "rv64imac";
> 
> While I was poking around trying to see if there was some logic
> behind
> that compatible, I noticed that SiFive's docs for the S76 say it is
> RV64GBC *but* the docs for the u74-mc say "4xRV64GBC and 1xRV64IMAC".
> I assume that rv64imac is the correct one here?
> 
> > +                       tlb-split;
> > +                       status = "disabled";
> > +
> > +                       cpu0_intc: interrupt-controller {
> > +                               compatible = "riscv,cpu-intc";
> > +                               interrupt-controller;
> > +                               #interrupt-cells = <1>;
> > +                       };
> > +               };
> > +
> > +               U74_1: cpu@1 {
> > +                       compatible = "sifive,u74-mc", "riscv";
> > +                       reg = <1>;
> > +                       d-cache-block-size = <64>;
> > +                       d-cache-sets = <64>;
> > +                       d-cache-size = <32768>;
> > +                       d-tlb-sets = <1>;
> > +                       d-tlb-size = <40>;
> > +                       device_type = "cpu";
> > +                       i-cache-block-size = <64>;
> > +                       i-cache-sets = <64>;
> > +                       i-cache-size = <32768>;
> > +                       i-tlb-sets = <1>;
> > +                       i-tlb-size = <40>;
> > +                       mmu-type = "riscv,sv39";
> > +                       next-level-cache = <&ccache>;
> > +                       riscv,isa = "rv64imafdc";
> 
> That also begs the question:
> Do your u74s support RV64GBC, as the (current) SiFive documentation
> suggests?

It supports RV64GCZbaZbb.

B is not a well-defined thing by specifications, so it should be
prevented here.

> 
> Thanks,
> Conor.
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Conor Dooley Dec. 29, 2022, 9:02 a.m. UTC | #9 
Hey Icenowy, Hal

On 29 December 2022 05:25:00 GMT, Icenowy Zheng <uwu@icenowy.me> wrote:
>在 2022-12-28星期三的 22:48 +0000，Conor Dooley写道：
>> Hey,
>> 
>> On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
>> > From: Emil Renner Berthing <kernel@esmil.dk>
>> > 
>> > Add initial device tree for the JH7110 RISC-V SoC by StarFive
>> > Technology Ltd.
>> > 
>> > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
>> > Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
>> > Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
>> > Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
>> > Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>> > ---
>> >  arch/riscv/boot/dts/starfive/jh7110.dtsi | 411
>> > +++++++++++++++++++++++
>> >  1 file changed, 411 insertions(+)
>> >  create mode 100644 arch/riscv/boot/dts/starfive/jh7110.dtsi
>> > 
>> > diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> > b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> > new file mode 100644
>> > index 000000000000..64d260ea1f29
>> > --- /dev/null
>> > +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> > @@ -0,0 +1,411 @@
>> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> > +/*
>> > + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> > + * Copyright (C) 2022 Emil Renner Berthing <kernel@esmil.dk>
>> > + */
>> > +
>> > +/dts-v1/;
>> > +#include <dt-bindings/clock/starfive,jh7110-crg.h>
>> > +#include <dt-bindings/reset/starfive,jh7110-crg.h>
>> > +
>> > +/ {
>> > +       compatible = "starfive,jh7110";
>> > +       #address-cells = <2>;
>> > +       #size-cells = <2>;
>> > +
>> > +       cpus {
>> > +               #address-cells = <1>;
>> > +               #size-cells = <0>;
>> > +
>> > +               S76_0: cpu@0 {
>> > +                       compatible = "sifive,u74-mc", "riscv";
>> 
>> The label here says S76 but the compatible says u74-mc.
>> Which is correct? Your docs say S7 and S76, so I would imagine that
>> it
>> is actually an S76?
>> 
>> > +                       reg = <0>;
>> > +                       d-cache-block-size = <64>;
>> > +                       d-cache-sets = <64>;
>> > +                       d-cache-size = <8192>;
>> > +                       d-tlb-sets = <1>;
>> > +                       d-tlb-size = <40>;
>> > +                       device_type = "cpu";
>> > +                       i-cache-block-size = <64>;
>> > +                       i-cache-sets = <64>;
>> > +                       i-cache-size = <16384>;
>> > +                       i-tlb-sets = <1>;
>> > +                       i-tlb-size = <40>;
>> > +                       mmu-type = "riscv,sv39";
>> > +                       next-level-cache = <&ccache>;
>> > +                       riscv,isa = "rv64imac";
>> 
>> While I was poking around trying to see if there was some logic
>> behind
>> that compatible, I noticed that SiFive's docs for the S76 say it is
>> RV64GBC *but* the docs for the u74-mc say "4xRV64GBC and 1xRV64IMAC".
>> I assume that rv64imac is the correct one here?
>> 
>> > +                       tlb-split;
>> > +                       status = "disabled";
>> > +
>> > +                       cpu0_intc: interrupt-controller {
>> > +                               compatible = "riscv,cpu-intc";
>> > +                               interrupt-controller;
>> > +                               #interrupt-cells = <1>;
>> > +                       };
>> > +               };
>> > +
>> > +               U74_1: cpu@1 {
>> > +                       compatible = "sifive,u74-mc", "riscv";
>> > +                       reg = <1>;
>> > +                       d-cache-block-size = <64>;
>> > +                       d-cache-sets = <64>;
>> > +                       d-cache-size = <32768>;
>> > +                       d-tlb-sets = <1>;
>> > +                       d-tlb-size = <40>;
>> > +                       device_type = "cpu";
>> > +                       i-cache-block-size = <64>;
>> > +                       i-cache-sets = <64>;
>> > +                       i-cache-size = <32768>;
>> > +                       i-tlb-sets = <1>;
>> > +                       i-tlb-size = <40>;
>> > +                       mmu-type = "riscv,sv39";
>> > +                       next-level-cache = <&ccache>;
>> > +                       riscv,isa = "rv64imafdc";
>> 
>> That also begs the question:
>> Do your u74s support RV64GBC, as the (current) SiFive documentation
>> suggests?
>
>It supports RV64GCZbaZbb.

Sweet, thanks.

>B is not a well-defined thing by specifications, so it should be
>prevented here.

Yah, don't worry - my next question was going to be which bits were supported :)

Hal, can you update the isa string in the next version please?

Thanks,
Conor.
Hal Feng Jan. 31, 2023, 2 a.m. UTC | #10 
On Tue, 20 Dec 2022 09:12:46 +0800, Hal Feng wrote:
> From: Emil Renner Berthing <kernel@esmil.dk>
> 
> Add initial device tree for the JH7110 RISC-V SoC by StarFive
> Technology Ltd.
> 
> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 411 +++++++++++++++++++++++
>  1 file changed, 411 insertions(+)
>  create mode 100644 arch/riscv/boot/dts/starfive/jh7110.dtsi

I wanna add i2c nodes (i2c0-6) in the next version, so someone else
can use them when they submit i2c driver patches.

Best regards,
Hal
Conor Dooley Jan. 31, 2023, 6:17 a.m. UTC | #11 
On 31 January 2023 02:00:26 GMT, Hal Feng <hal.feng@starfivetech.com> wrote:
>On Tue, 20 Dec 2022 09:12:46 +0800, Hal Feng wrote:
>> From: Emil Renner Berthing <kernel@esmil.dk>
>> 
>> Add initial device tree for the JH7110 RISC-V SoC by StarFive
>> Technology Ltd.
>> 
>> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
>> Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
>> Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
>> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>> ---
>>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 411 +++++++++++++++++++++++
>>  1 file changed, 411 insertions(+)
>>  create mode 100644 arch/riscv/boot/dts/starfive/jh7110.dtsi
>
>I wanna add i2c nodes (i2c0-6) in the next version, so someone else
>can use them when they submit i2c driver patches.

All of the other series depend on this one for enablement,
so unless the binding for i2c is already upstream I'd advise keeping it separate.

Cheers,
Conor.
Hal Feng Feb. 1, 2023, 7:21 a.m. UTC | #12 
On Wed, 28 Dec 2022 22:48:43 +0000, Conor Dooley wrote:
> Hey,
> 
> On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
>> From: Emil Renner Berthing <kernel@esmil.dk>
>> 
>> Add initial device tree for the JH7110 RISC-V SoC by StarFive
>> Technology Ltd.
>> 
>> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
>> Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
>> Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
>> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>> ---
>>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 411 +++++++++++++++++++++++
>>  1 file changed, 411 insertions(+)
>>  create mode 100644 arch/riscv/boot/dts/starfive/jh7110.dtsi
>> 
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> new file mode 100644
>> index 000000000000..64d260ea1f29
>> --- /dev/null
>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> @@ -0,0 +1,411 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +/*
>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> + * Copyright (C) 2022 Emil Renner Berthing <kernel@esmil.dk>
>> + */
>> +
>> +/dts-v1/;
>> +#include <dt-bindings/clock/starfive,jh7110-crg.h>
>> +#include <dt-bindings/reset/starfive,jh7110-crg.h>
>> +
>> +/ {
>> +	compatible = "starfive,jh7110";
>> +	#address-cells = <2>;
>> +	#size-cells = <2>;
>> +
>> +	cpus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		S76_0: cpu@0 {
>> +			compatible = "sifive,u74-mc", "riscv";
> 
> The label here says S76 but the compatible says u74-mc.

U74-MC has 5 cores including 1 * S7 core and 4 * U74 cores.

> Which is correct? Your docs say S7 and S76, so I would imagine that it
> is actually an S76?

I found SiFive website [1] call it S76, but call it S7 in other places.
So I misunderstood this. Considering the ISA difference you described
as below, I think it's proper to change the label to "S7_0".

[1] https://www.sifive.com/cores/essential

> 
>> +			reg = <0>;
>> +			d-cache-block-size = <64>;
>> +			d-cache-sets = <64>;
>> +			d-cache-size = <8192>;
>> +			d-tlb-sets = <1>;
>> +			d-tlb-size = <40>;
>> +			device_type = "cpu";
>> +			i-cache-block-size = <64>;
>> +			i-cache-sets = <64>;
>> +			i-cache-size = <16384>;
>> +			i-tlb-sets = <1>;
>> +			i-tlb-size = <40>;
>> +			mmu-type = "riscv,sv39";
>> +			next-level-cache = <&ccache>;
>> +			riscv,isa = "rv64imac";
> 
> While I was poking around trying to see if there was some logic behind
> that compatible, I noticed that SiFive's docs for the S76 say it is
> RV64GBC *but* the docs for the u74-mc say "4xRV64GBC and 1xRV64IMAC".
> I assume that rv64imac is the correct one here?

Yes, "RV64IMAC" is correct. The monitor core in U74-MC is a
S7-series core, not S76.

> 
>> +			tlb-split;
>> +			status = "disabled";
>> +
>> +			cpu0_intc: interrupt-controller {
>> +				compatible = "riscv,cpu-intc";
>> +				interrupt-controller;
>> +				#interrupt-cells = <1>;
>> +			};
>> +		};
>> +
>> +		U74_1: cpu@1 {
>> +			compatible = "sifive,u74-mc", "riscv";
>> +			reg = <1>;
>> +			d-cache-block-size = <64>;
>> +			d-cache-sets = <64>;
>> +			d-cache-size = <32768>;
>> +			d-tlb-sets = <1>;
>> +			d-tlb-size = <40>;
>> +			device_type = "cpu";
>> +			i-cache-block-size = <64>;
>> +			i-cache-sets = <64>;
>> +			i-cache-size = <32768>;
>> +			i-tlb-sets = <1>;
>> +			i-tlb-size = <40>;
>> +			mmu-type = "riscv,sv39";
>> +			next-level-cache = <&ccache>;
>> +			riscv,isa = "rv64imafdc";
> 
> That also begs the question:
> Do your u74s support RV64GBC, as the (current) SiFive documentation
> suggests?

Actually, U74 doesn't support the full B extension, and the SiFive doc [1]
describes the ISA of U74 is "RV64GC_Zba_Zbb_Sscofpmf" which "G" includes
"IMAFD". "_Zba_Zbb_Sscofpmf" is not shown in other device trees such as
jh7100.dtsi and fu740-c000.dtsi, so I didn't show them here.

[1] https://sifive.cdn.prismic.io/sifive/2dd11994-693c-4360-8aea-5453d8642c42_u74mc_core_complex_manual_21G3.pdf

Best regards,
Hal
Hal Feng Feb. 1, 2023, 7:31 a.m. UTC | #13 
On Thu, 29 Dec 2022 13:25:00 +0800, Icenowy Zheng wrote:
> 在 2022-12-28星期三的 22:48 +0000，Conor Dooley写道：
>> Hey,
>> 
>> On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
[...]
>> > +               U74_1: cpu@1 {
>> > +                       compatible = "sifive,u74-mc", "riscv";
>> > +                       reg = <1>;
>> > +                       d-cache-block-size = <64>;
>> > +                       d-cache-sets = <64>;
>> > +                       d-cache-size = <32768>;
>> > +                       d-tlb-sets = <1>;
>> > +                       d-tlb-size = <40>;
>> > +                       device_type = "cpu";
>> > +                       i-cache-block-size = <64>;
>> > +                       i-cache-sets = <64>;
>> > +                       i-cache-size = <32768>;
>> > +                       i-tlb-sets = <1>;
>> > +                       i-tlb-size = <40>;
>> > +                       mmu-type = "riscv,sv39";
>> > +                       next-level-cache = <&ccache>;
>> > +                       riscv,isa = "rv64imafdc";
>> 
>> That also begs the question:
>> Do your u74s support RV64GBC, as the (current) SiFive documentation
>> suggests?
> 
> It supports RV64GCZbaZbb.
> 
> B is not a well-defined thing by specifications, so it should be
> prevented here.

Thank you for your kindly reply.

Best regards,
Hal
Hal Feng Feb. 1, 2023, 7:53 a.m. UTC | #14 
On Thu, 29 Dec 2022 09:02:15 +0000, Conor Dooley wrote:
> Hey Icenowy, Hal
> 
> On 29 December 2022 05:25:00 GMT, Icenowy Zheng <uwu@icenowy.me> wrote:
>>在 2022-12-28星期三的 22:48 +0000，Conor Dooley写道：
>>> Hey,
>>> 
>>> On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
[...]
>>> > +               U74_1: cpu@1 {
>>> > +                       compatible = "sifive,u74-mc", "riscv";
>>> > +                       reg = <1>;
>>> > +                       d-cache-block-size = <64>;
>>> > +                       d-cache-sets = <64>;
>>> > +                       d-cache-size = <32768>;
>>> > +                       d-tlb-sets = <1>;
>>> > +                       d-tlb-size = <40>;
>>> > +                       device_type = "cpu";
>>> > +                       i-cache-block-size = <64>;
>>> > +                       i-cache-sets = <64>;
>>> > +                       i-cache-size = <32768>;
>>> > +                       i-tlb-sets = <1>;
>>> > +                       i-tlb-size = <40>;
>>> > +                       mmu-type = "riscv,sv39";
>>> > +                       next-level-cache = <&ccache>;
>>> > +                       riscv,isa = "rv64imafdc";
>>> 
>>> That also begs the question:
>>> Do your u74s support RV64GBC, as the (current) SiFive documentation
>>> suggests?
>>
>>It supports RV64GCZbaZbb.
> 
> Sweet, thanks.
> 
>>B is not a well-defined thing by specifications, so it should be
>>prevented here.
> 
> Yah, don't worry - my next question was going to be which bits were supported :)
> 
> Hal, can you update the isa string in the next version please?

The current isa description is correct. Please see my reply [1].
Thank you.

[1] https://lore.kernel.org/all/c507e0b2-5ca3-cffe-55d2-873ed8c24e3d@starfivetech.com/

Best regards,
Hal
Conor Dooley Feb. 1, 2023, 8:21 a.m. UTC | #15 
On Wed, Feb 01, 2023 at 03:21:48PM +0800, Hal Feng wrote:
> On Wed, 28 Dec 2022 22:48:43 +0000, Conor Dooley wrote:
> > On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:

> >> +/ {
> >> +	compatible = "starfive,jh7110";
> >> +	#address-cells = <2>;
> >> +	#size-cells = <2>;
> >> +
> >> +	cpus {
> >> +		#address-cells = <1>;
> >> +		#size-cells = <0>;
> >> +
> >> +		S76_0: cpu@0 {
> >> +			compatible = "sifive,u74-mc", "riscv";
> > 
> > The label here says S76 but the compatible says u74-mc.
> 
> U74-MC has 5 cores including 1 * S7 core and 4 * U74 cores.
> 
> > Which is correct? Your docs say S7 and S76, so I would imagine that it
> > is actually an S76?
> 
> I found SiFive website [1] call it S76, but call it S7 in other places.
> So I misunderstood this. Considering the ISA difference you described
> as below, I think it's proper to change the label to "S7_0".

I'm less worried about the label & more interested in the compatible.
hart0 is, as you say, not a u74. Should we not be adding a "sifive,s7"
compatible string to Documentation/devicetree/bindings/riscv/cpus.yaml
and using that here instead?

> 
> [1] https://www.sifive.com/cores/essential
> 
> > 
> >> +			reg = <0>;
> >> +			d-cache-block-size = <64>;
> >> +			d-cache-sets = <64>;
> >> +			d-cache-size = <8192>;
> >> +			d-tlb-sets = <1>;
> >> +			d-tlb-size = <40>;
> >> +			device_type = "cpu";
> >> +			i-cache-block-size = <64>;
> >> +			i-cache-sets = <64>;
> >> +			i-cache-size = <16384>;
> >> +			i-tlb-sets = <1>;
> >> +			i-tlb-size = <40>;
> >> +			mmu-type = "riscv,sv39";
> >> +			next-level-cache = <&ccache>;
> >> +			riscv,isa = "rv64imac";
> > 
> > While I was poking around trying to see if there was some logic behind
> > that compatible, I noticed that SiFive's docs for the S76 say it is
> > RV64GBC *but* the docs for the u74-mc say "4xRV64GBC and 1xRV64IMAC".
> > I assume that rv64imac is the correct one here?
> 
> Yes, "RV64IMAC" is correct. The monitor core in U74-MC is a
> S7-series core, not S76.

Cool, thanks.

> >> +			tlb-split;
> >> +			status = "disabled";
> >> +
> >> +			cpu0_intc: interrupt-controller {
> >> +				compatible = "riscv,cpu-intc";
> >> +				interrupt-controller;
> >> +				#interrupt-cells = <1>;
> >> +			};
> >> +		};
> >> +
> >> +		U74_1: cpu@1 {
> >> +			compatible = "sifive,u74-mc", "riscv";
> >> +			reg = <1>;
> >> +			d-cache-block-size = <64>;
> >> +			d-cache-sets = <64>;
> >> +			d-cache-size = <32768>;
> >> +			d-tlb-sets = <1>;
> >> +			d-tlb-size = <40>;
> >> +			device_type = "cpu";
> >> +			i-cache-block-size = <64>;
> >> +			i-cache-sets = <64>;
> >> +			i-cache-size = <32768>;
> >> +			i-tlb-sets = <1>;
> >> +			i-tlb-size = <40>;
> >> +			mmu-type = "riscv,sv39";
> >> +			next-level-cache = <&ccache>;
> >> +			riscv,isa = "rv64imafdc";
> > 
> > That also begs the question:
> > Do your u74s support RV64GBC, as the (current) SiFive documentation
> > suggests?
> 
> Actually, U74 doesn't support the full B extension, and the SiFive doc [1]

Yeah, I knew asking that question that the "RV64GBC" in SiFive's online
documentation was using outdated terminology. Also, that is not the doc
for your core complex as far as I can tell. That is the document for
impid 0x0621_1222, whereas (IIRC) your core is 0x0421_0427.
Jess and I had a look one evening but could not find the 21G1.02.00
revision of this document, which is the one corresponding to 0x421_0427.
See Table 92 for more details.

> describes the ISA of U74 is "RV64GC_Zba_Zbb_Sscofpmf" which "G" includes
> "IMAFD".

I could not find the 21G1.02.00 version of this document online, but I
was able to find the 21G1.01.00 version of it & that version does not
support the Sscofpmf extension (but does have Zba/Zbb support).

> "_Zba_Zbb_Sscofpmf" is not shown in other device trees such as
> jh7100.dtsi and fu740-c000.dtsi, so I didn't show them here.

Just because other devicetrees omit them, doesn't mean that you should
too!
This compatible should be an accurate description of your hardware, so
you should add what you actually have.
If you have Zba and Zbb, then add them.
I would double check against your internal documentation for 21G2.02.00
as to whether you do have Sscofpmf, and if you do, add that too!

That way, whenever support for those extensions lands, the jh7110 will
automatically pick it up, rather than needing to have them retrofitted.

> [1] https://sifive.cdn.prismic.io/sifive/2dd11994-693c-4360-8aea-5453d8642c42_u74mc_core_complex_manual_21G3.pdf

Thanks,
Conor.
Hal Feng Feb. 2, 2023, 2:42 a.m. UTC | #16 
On Tue, 31 Jan 2023 06:17:17 +0000, Conor Dooley wrote:
> On 31 January 2023 02:00:26 GMT, Hal Feng <hal.feng@starfivetech.com> wrote:
>>On Tue, 20 Dec 2022 09:12:46 +0800, Hal Feng wrote:
>>> From: Emil Renner Berthing <kernel@esmil.dk>
>>> 
>>> Add initial device tree for the JH7110 RISC-V SoC by StarFive
>>> Technology Ltd.
>>> 
>>> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
>>> Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
>>> Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
>>> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
>>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>>> ---
>>>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 411 +++++++++++++++++++++++
>>>  1 file changed, 411 insertions(+)
>>>  create mode 100644 arch/riscv/boot/dts/starfive/jh7110.dtsi
>>
>>I wanna add i2c nodes (i2c0-6) in the next version, so someone else
>>can use them when they submit i2c driver patches.
> 
> All of the other series depend on this one for enablement,
> so unless the binding for i2c is already upstream I'd advise keeping it separate.

The i2c IP of JH7110 is from Synopsys and the same as the i2c IP in JH7100.
The binding and driver for i2c are already upstream. It works as long as we
add the i2c nodes and configure pins for i2c in device tree. It will simplify
the dependency if we do that.

By the way, I am checking the ISA of U74-MC on JH7110 with someone else.
I will reply you today.

Best regards,
Hal
Conor Dooley Feb. 2, 2023, 6:19 a.m. UTC | #17 
On 2 February 2023 02:42:32 GMT, Hal Feng <hal.feng@starfivetech.com> wrote:
>On Tue, 31 Jan 2023 06:17:17 +0000, Conor Dooley wrote:
>> On 31 January 2023 02:00:26 GMT, Hal Feng <hal.feng@starfivetech.com> wrote:
>>>On Tue, 20 Dec 2022 09:12:46 +0800, Hal Feng wrote:
>>>> From: Emil Renner Berthing <kernel@esmil.dk>
>>>> 
>>>> Add initial device tree for the JH7110 RISC-V SoC by StarFive
>>>> Technology Ltd.
>>>> 
>>>> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
>>>> Co-developed-by: Jianlong Huang <jianlong.huang@starfivetech.com>
>>>> Signed-off-by: Jianlong Huang <jianlong.huang@starfivetech.com>
>>>> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
>>>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>>>> ---
>>>>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 411 +++++++++++++++++++++++
>>>>  1 file changed, 411 insertions(+)
>>>>  create mode 100644 arch/riscv/boot/dts/starfive/jh7110.dtsi
>>>
>>>I wanna add i2c nodes (i2c0-6) in the next version, so someone else
>>>can use them when they submit i2c driver patches.
>> 
>> All of the other series depend on this one for enablement,
>> so unless the binding for i2c is already upstream I'd advise keeping it separate.
>
>The i2c IP of JH7110 is from Synopsys and the same as the i2c IP in JH7100.
>The binding and driver for i2c are already upstream. It works as long as we
>add the i2c nodes and configure pins for i2c in device tree. It will simplify
>the dependency if we do that.

Please make sure that you add a device specific compatible for jh7110 then, thanks.

>By the way, I am checking the ISA of U74-MC on JH7110 with someone else.
>I will reply you today.

Cool!
Hal Feng Feb. 2, 2023, 6:56 p.m. UTC | #18 
On Wed, 1 Feb 2023 08:21:05 +0000, Conor Dooley wrote:
> On Wed, Feb 01, 2023 at 03:21:48PM +0800, Hal Feng wrote:
>> On Wed, 28 Dec 2022 22:48:43 +0000, Conor Dooley wrote:
>> > On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
> 
>> >> +/ {
>> >> +	compatible = "starfive,jh7110";
>> >> +	#address-cells = <2>;
>> >> +	#size-cells = <2>;
>> >> +
>> >> +	cpus {
>> >> +		#address-cells = <1>;
>> >> +		#size-cells = <0>;
>> >> +
>> >> +		S76_0: cpu@0 {
>> >> +			compatible = "sifive,u74-mc", "riscv";
>> > 
>> > The label here says S76 but the compatible says u74-mc.
>> 
>> U74-MC has 5 cores including 1 * S7 core and 4 * U74 cores.
>> 
>> > Which is correct? Your docs say S7 and S76, so I would imagine that it
>> > is actually an S76?
>> 
>> I found SiFive website [1] call it S76, but call it S7 in other places.
>> So I misunderstood this. Considering the ISA difference you described
>> as below, I think it's proper to change the label to "S7_0".
> 
> I'm less worried about the label & more interested in the compatible.
> hart0 is, as you say, not a u74. Should we not be adding a "sifive,s7"
> compatible string to Documentation/devicetree/bindings/riscv/cpus.yaml
> and using that here instead?

First of all, it's my fault that I didn't check the revision of U74-MC
manual, so most of my previous replies might not make sense.

If we add a new compatible string for S7, should we change the compatibles
of hart1~3 to "sifive,u74" also? And then, there may be no point keeping some
compatible strings of core complex like "sifive,u74-mc" and "sifive,u54-mc".
I'm not sure about this.

> 
>> 
>> [1] https://www.sifive.com/cores/essential
>> 
>> > 
>> >> +			reg = <0>;
>> >> +			d-cache-block-size = <64>;
>> >> +			d-cache-sets = <64>;
>> >> +			d-cache-size = <8192>;
>> >> +			d-tlb-sets = <1>;
>> >> +			d-tlb-size = <40>;
>> >> +			device_type = "cpu";
>> >> +			i-cache-block-size = <64>;
>> >> +			i-cache-sets = <64>;
>> >> +			i-cache-size = <16384>;
>> >> +			i-tlb-sets = <1>;
>> >> +			i-tlb-size = <40>;
>> >> +			mmu-type = "riscv,sv39";
>> >> +			next-level-cache = <&ccache>;
>> >> +			riscv,isa = "rv64imac";
>> > 
>> > While I was poking around trying to see if there was some logic behind
>> > that compatible, I noticed that SiFive's docs for the S76 say it is
>> > RV64GBC *but* the docs for the u74-mc say "4xRV64GBC and 1xRV64IMAC".
>> > I assume that rv64imac is the correct one here?
>> 
>> Yes, "RV64IMAC" is correct. The monitor core in U74-MC is a
>> S7-series core, not S76.
> 
> Cool, thanks.

Now I think it might be another version of S76.

> 
>> >> +			tlb-split;
>> >> +			status = "disabled";
>> >> +
>> >> +			cpu0_intc: interrupt-controller {
>> >> +				compatible = "riscv,cpu-intc";
>> >> +				interrupt-controller;
>> >> +				#interrupt-cells = <1>;
>> >> +			};
>> >> +		};
>> >> +
>> >> +		U74_1: cpu@1 {
>> >> +			compatible = "sifive,u74-mc", "riscv";
>> >> +			reg = <1>;
>> >> +			d-cache-block-size = <64>;
>> >> +			d-cache-sets = <64>;
>> >> +			d-cache-size = <32768>;
>> >> +			d-tlb-sets = <1>;
>> >> +			d-tlb-size = <40>;
>> >> +			device_type = "cpu";
>> >> +			i-cache-block-size = <64>;
>> >> +			i-cache-sets = <64>;
>> >> +			i-cache-size = <32768>;
>> >> +			i-tlb-sets = <1>;
>> >> +			i-tlb-size = <40>;
>> >> +			mmu-type = "riscv,sv39";
>> >> +			next-level-cache = <&ccache>;
>> >> +			riscv,isa = "rv64imafdc";
>> > 
>> > That also begs the question:
>> > Do your u74s support RV64GBC, as the (current) SiFive documentation
>> > suggests?
>> 
>> Actually, U74 doesn't support the full B extension, and the SiFive doc [1]
> 
> Yeah, I knew asking that question that the "RV64GBC" in SiFive's online
> documentation was using outdated terminology. Also, that is not the doc
> for your core complex as far as I can tell. That is the document for
> impid 0x0621_1222, whereas (IIRC) your core is 0x0421_0427.
> Jess and I had a look one evening but could not find the 21G1.02.00
> revision of this document, which is the one corresponding to 0x421_0427.
> See Table 92 for more details.

I found the 21G1.02.00 revision on StarFive internal net, but I'm not sure
whether I can make it public and I am checking this. This revision records
that the ISA of 21G1.02.00 U74 is "RV64GCB" and ISA of 21G1.02.00 S7 is
"RV64IMACB". I am asking someone to check with SiFive whether both 21G1.02.00
U74 and S7 support the full B extension.

> 
>> describes the ISA of U74 is "RV64GC_Zba_Zbb_Sscofpmf" which "G" includes
>> "IMAFD".
> 
> I could not find the 21G1.02.00 version of this document online, but I
> was able to find the 21G1.01.00 version of it & that version does not
> support the Sscofpmf extension (but does have Zba/Zbb support).
> 
>> "_Zba_Zbb_Sscofpmf" is not shown in other device trees such as
>> jh7100.dtsi and fu740-c000.dtsi, so I didn't show them here.
> 
> Just because other devicetrees omit them, doesn't mean that you should
> too!
> This compatible should be an accurate description of your hardware, so
> you should add what you actually have.

Will keep it in mind. Thank you.

Best regards,
Hal

> If you have Zba and Zbb, then add them.
> I would double check against your internal documentation for 21G2.02.00
> as to whether you do have Sscofpmf, and if you do, add that too!
> 
> That way, whenever support for those extensions lands, the jh7110 will
> automatically pick it up, rather than needing to have them retrofitted.
> 
>> [1] https://sifive.cdn.prismic.io/sifive/2dd11994-693c-4360-8aea-5453d8642c42_u74mc_core_complex_manual_21G3.pdf
Conor Dooley Feb. 2, 2023, 7:41 p.m. UTC | #19 
On Fri, Feb 03, 2023 at 02:56:41AM +0800, Hal Feng wrote:
> On Wed, 1 Feb 2023 08:21:05 +0000, Conor Dooley wrote:
> > On Wed, Feb 01, 2023 at 03:21:48PM +0800, Hal Feng wrote:
> >> On Wed, 28 Dec 2022 22:48:43 +0000, Conor Dooley wrote:
> >> > On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
> > 
> >> >> +/ {
> >> >> +	compatible = "starfive,jh7110";
> >> >> +	#address-cells = <2>;
> >> >> +	#size-cells = <2>;
> >> >> +
> >> >> +	cpus {
> >> >> +		#address-cells = <1>;
> >> >> +		#size-cells = <0>;
> >> >> +
> >> >> +		S76_0: cpu@0 {
> >> >> +			compatible = "sifive,u74-mc", "riscv";
> >> > 
> >> > The label here says S76 but the compatible says u74-mc.
> >> 
> >> U74-MC has 5 cores including 1 * S7 core and 4 * U74 cores.
> >> 
> >> > Which is correct? Your docs say S7 and S76, so I would imagine that it
> >> > is actually an S76?
> >> 
> >> I found SiFive website [1] call it S76, but call it S7 in other places.
> >> So I misunderstood this. Considering the ISA difference you described
> >> as below, I think it's proper to change the label to "S7_0".
> > 
> > I'm less worried about the label & more interested in the compatible.
> > hart0 is, as you say, not a u74. Should we not be adding a "sifive,s7"
> > compatible string to Documentation/devicetree/bindings/riscv/cpus.yaml
> > and using that here instead?
> 
> First of all, it's my fault that I didn't check the revision of U74-MC
> manual, so most of my previous replies might not make sense.

No that's fine. The manual stuff confused me too when I went looking
initially, and I still get get mixed up by the fact that there are
core-complex manuals but not core manuals.

> If we add a new compatible string for S7, should we change the compatibles
> of hart1~3 to "sifive,u74" also? And then, there may be no point keeping some
> compatible strings of core complex like "sifive,u74-mc" and "sifive,u54-mc".
> I'm not sure about this.

[...]

> >> Yes, "RV64IMAC" is correct. The monitor core in U74-MC is a
> >> S7-series core, not S76.
> > 
> > Cool, thanks.
> 
> Now I think it might be another version of S76.

The SiFive docs describe the u74-mc core complex, which AFAIU you have,
as being 1x S7 & 4x U7.

I'd be happy with new binding for "sifive,s7" & then we use that here.
If you're sure it's S76, we can also use that. S76 is described, in what
docs I can see, as a core complex containing an S7, so S7 seems likely
to be correct?

u7, u74 & u74-mc are valid compatibles, added by SiFive, in commit
75e6d7248efc ("dt-bindings: riscv: Update DT binding docs to support
SiFive FU740 SoC"). Unfortunately, they never actually *used* those
compatibles for anything, and just used "sifive,bullet0" for the fu740.

I'll accept any of u7, u74 or u74-mc for those harts.

> >> >> +			tlb-split;
> >> >> +			status = "disabled";
> >> >> +
> >> >> +			cpu0_intc: interrupt-controller {
> >> >> +				compatible = "riscv,cpu-intc";
> >> >> +				interrupt-controller;
> >> >> +				#interrupt-cells = <1>;
> >> >> +			};
> >> >> +		};
> >> >> +
> >> >> +		U74_1: cpu@1 {
> >> >> +			compatible = "sifive,u74-mc", "riscv";
> >> >> +			reg = <1>;
> >> >> +			d-cache-block-size = <64>;
> >> >> +			d-cache-sets = <64>;
> >> >> +			d-cache-size = <32768>;
> >> >> +			d-tlb-sets = <1>;
> >> >> +			d-tlb-size = <40>;
> >> >> +			device_type = "cpu";
> >> >> +			i-cache-block-size = <64>;
> >> >> +			i-cache-sets = <64>;
> >> >> +			i-cache-size = <32768>;
> >> >> +			i-tlb-sets = <1>;
> >> >> +			i-tlb-size = <40>;
> >> >> +			mmu-type = "riscv,sv39";
> >> >> +			next-level-cache = <&ccache>;
> >> >> +			riscv,isa = "rv64imafdc";
> >> > 
> >> > That also begs the question:
> >> > Do your u74s support RV64GBC, as the (current) SiFive documentation
> >> > suggests?
> >> 
> >> Actually, U74 doesn't support the full B extension, and the SiFive doc [1]
> > 
> > Yeah, I knew asking that question that the "RV64GBC" in SiFive's online
> > documentation was using outdated terminology. Also, that is not the doc
> > for your core complex as far as I can tell. That is the document for
> > impid 0x0621_1222, whereas (IIRC) your core is 0x0421_0427.
> > Jess and I had a look one evening but could not find the 21G1.02.00
> > revision of this document, which is the one corresponding to 0x421_0427.
> > See Table 92 for more details.
> 
> I found the 21G1.02.00 revision on StarFive internal net, but I'm not sure
> whether I can make it public and I am checking this.

Yeah, certainly don't do anything without cross-checking!

> This revision records
> that the ISA of 21G1.02.00 U74 is "RV64GCB" and ISA of 21G1.02.00 S7 is
> "RV64IMACB". I am asking someone to check with SiFive whether both 21G1.02.00
> U74 and S7 support the full B extension.

Having cross-checked a 21G1.01.00 document against a 21G2.01.00 one, I'm
99% sure that you have _Zba_Zbb.
The G2.01 document says _Zba_Zbb & has the same instructions listed as
supported as the G1.02 one.
I've also tried the Zbb support patches posted by Heiko [2] on a
VisionFive V2 and had them work - which is why a definitive statement on
the version of Zbb supported would be really great to have!

[2] - https://lore.kernel.org/linux-riscv/20230113212301.3534711-1-heiko@sntech.de/

> >> describes the ISA of U74 is "RV64GC_Zba_Zbb_Sscofpmf" which "G" includes
> >> "IMAFD".
> > 
> > I could not find the 21G1.02.00 version of this document online, but I
> > was able to find the 21G1.01.00 version of it & that version does not
> > support the Sscofpmf extension (but does have Zba/Zbb support).
> > 
> >> "_Zba_Zbb_Sscofpmf" is not shown in other device trees such as
> >> jh7100.dtsi and fu740-c000.dtsi, so I didn't show them here.
> > 
> > Just because other devicetrees omit them, doesn't mean that you should
> > too!
> > This compatible should be an accurate description of your hardware, so
> > you should add what you actually have.
> 
> Will keep it in mind. Thank you.

FWIW, the deadline for getting material in for v6.3 has already passed,
so you can send the next version of this series without waiting for
clarification on the compatibles & ISA string. We should have plenty of
time to get those fixed up before the series gets applied.

Thanks,
Conor.

> > If you have Zba and Zbb, then add them.
> > I would double check against your internal documentation for 21G2.02.00
> > as to whether you do have Sscofpmf, and if you do, add that too!
> > 
> > That way, whenever support for those extensions lands, the jh7110 will
> > automatically pick it up, rather than needing to have them retrofitted.
> > 
> >> [1] https://sifive.cdn.prismic.io/sifive/2dd11994-693c-4360-8aea-5453d8642c42_u74mc_core_complex_manual_21G3.pdf
Conor Dooley Feb. 9, 2023, 11:11 a.m. UTC | #20 
On Thu, Feb 02, 2023 at 07:41:33PM +0000, Conor Dooley wrote:
> On Fri, Feb 03, 2023 at 02:56:41AM +0800, Hal Feng wrote:
> > On Wed, 1 Feb 2023 08:21:05 +0000, Conor Dooley wrote:
> > > On Wed, Feb 01, 2023 at 03:21:48PM +0800, Hal Feng wrote:
> > >> On Wed, 28 Dec 2022 22:48:43 +0000, Conor Dooley wrote:
> > >> > On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:

> FWIW, the deadline for getting material in for v6.3 has already passed,
> so you can send the next version of this series without waiting for
> clarification on the compatibles & ISA string. We should have plenty of
> time to get those fixed up before the series gets applied.

Also, as it looks like the pinctrl driver is going to land in time for
v6.3, that leaves just this series and the clock driver required for
base support.

In the original submission, you sent the clock driver and dt in the same
series & I think it might make the process a bit faster if you sent them
both together for the next version again.

That way, both the drivers and dts can go together as their have an
inter dependence.

That's my opinion anyway, will make trying to sequence things between
trees easier.

Cheers,
Conor.
Hal Feng Feb. 13, 2023, 9:41 a.m. UTC | #21 
On Thu, 9 Feb 2023 11:11:51 +0000, Conor Dooley wrote:
> On Thu, Feb 02, 2023 at 07:41:33PM +0000, Conor Dooley wrote:
>> On Fri, Feb 03, 2023 at 02:56:41AM +0800, Hal Feng wrote:
>> > On Wed, 1 Feb 2023 08:21:05 +0000, Conor Dooley wrote:
>> > > On Wed, Feb 01, 2023 at 03:21:48PM +0800, Hal Feng wrote:
>> > >> On Wed, 28 Dec 2022 22:48:43 +0000, Conor Dooley wrote:
>> > >> > On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
> 
>> FWIW, the deadline for getting material in for v6.3 has already passed,
>> so you can send the next version of this series without waiting for
>> clarification on the compatibles & ISA string. We should have plenty of
>> time to get those fixed up before the series gets applied.
> 
> Also, as it looks like the pinctrl driver is going to land in time for
> v6.3, that leaves just this series and the clock driver required for
> base support.
> 
> In the original submission, you sent the clock driver and dt in the same
> series & I think it might make the process a bit faster if you sent them
> both together for the next version again.
> 
> That way, both the drivers and dts can go together as their have an
> inter dependence.
> 
> That's my opinion anyway, will make trying to sequence things between
> trees easier.

Good idea. But how can I write the change log if we do so? Will it make
the history confused? Thanks.

Best regards,
Hal
Conor Dooley Feb. 13, 2023, 10:07 a.m. UTC | #22 
On Mon, Feb 13, 2023 at 05:41:02PM +0800, Hal Feng wrote:
> On Thu, 9 Feb 2023 11:11:51 +0000, Conor Dooley wrote:
> > On Thu, Feb 02, 2023 at 07:41:33PM +0000, Conor Dooley wrote:
> >> On Fri, Feb 03, 2023 at 02:56:41AM +0800, Hal Feng wrote:
> >> > On Wed, 1 Feb 2023 08:21:05 +0000, Conor Dooley wrote:
> >> > > On Wed, Feb 01, 2023 at 03:21:48PM +0800, Hal Feng wrote:
> >> > >> On Wed, 28 Dec 2022 22:48:43 +0000, Conor Dooley wrote:
> >> > >> > On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
> > 
> >> FWIW, the deadline for getting material in for v6.3 has already passed,
> >> so you can send the next version of this series without waiting for
> >> clarification on the compatibles & ISA string. We should have plenty of
> >> time to get those fixed up before the series gets applied.
> > 
> > Also, as it looks like the pinctrl driver is going to land in time for
> > v6.3, that leaves just this series and the clock driver required for
> > base support.
> > 
> > In the original submission, you sent the clock driver and dt in the same
> > series & I think it might make the process a bit faster if you sent them
> > both together for the next version again.
> > 
> > That way, both the drivers and dts can go together as their have an
> > inter dependence.
> > 
> > That's my opinion anyway, will make trying to sequence things between
> > trees easier.
> 
> Good idea. But how can I write the change log if we do so? Will it make
> the history confused? Thanks.

I'm not sure what you mean w.r.t. history.
Both series are on V3 I think, so just make the next version v4 title it
something like "Basic clock, reset & dt support..."
For the changelogs, just mention you merged the two series again in
the cover letter & add the changelogs that you would have made for each
series to the cover as a single changelog.
Say somewhere in the cover that I suggested merging the series together
so that they could go via the same tree as the dt-binding headers are
required by both driver & devicetree.

Cheers,
Conor.
Hal Feng Feb. 14, 2023, 2:37 a.m. UTC | #23 
On Mon, 13 Feb 2023 10:07:38 +0000, Conor Dooley wrote:
> On Mon, Feb 13, 2023 at 05:41:02PM +0800, Hal Feng wrote:
>> On Thu, 9 Feb 2023 11:11:51 +0000, Conor Dooley wrote:
>> > On Thu, Feb 02, 2023 at 07:41:33PM +0000, Conor Dooley wrote:
>> >> On Fri, Feb 03, 2023 at 02:56:41AM +0800, Hal Feng wrote:
>> >> > On Wed, 1 Feb 2023 08:21:05 +0000, Conor Dooley wrote:
>> >> > > On Wed, Feb 01, 2023 at 03:21:48PM +0800, Hal Feng wrote:
>> >> > >> On Wed, 28 Dec 2022 22:48:43 +0000, Conor Dooley wrote:
>> >> > >> > On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
>> > 
>> >> FWIW, the deadline for getting material in for v6.3 has already passed,
>> >> so you can send the next version of this series without waiting for
>> >> clarification on the compatibles & ISA string. We should have plenty of
>> >> time to get those fixed up before the series gets applied.
>> > 
>> > Also, as it looks like the pinctrl driver is going to land in time for
>> > v6.3, that leaves just this series and the clock driver required for
>> > base support.
>> > 
>> > In the original submission, you sent the clock driver and dt in the same
>> > series & I think it might make the process a bit faster if you sent them
>> > both together for the next version again.
>> > 
>> > That way, both the drivers and dts can go together as their have an
>> > inter dependence.
>> > 
>> > That's my opinion anyway, will make trying to sequence things between
>> > trees easier.
>> 
>> Good idea. But how can I write the change log if we do so? Will it make
>> the history confused? Thanks.
> 
> I'm not sure what you mean w.r.t. history.
> Both series are on V3 I think, so just make the next version v4 title it
> something like "Basic clock, reset & dt support..."
> For the changelogs, just mention you merged the two series again in
> the cover letter & add the changelogs that you would have made for each
> series to the cover as a single changelog.
> Say somewhere in the cover that I suggested merging the series together
> so that they could go via the same tree as the dt-binding headers are
> required by both driver & devicetree.

OK, I see. I will merge the clock patch series [1] and this DT patch
series in v4. Thanks for your suggestions.

[1] https://lore.kernel.org/all/20221220005054.34518-1-hal.feng@starfivetech.com/

Best regards,
Hal
Hal Feng Feb. 15, 2023, 3:07 a.m. UTC | #24 
On Thu, 2 Feb 2023 19:41:33 +0000, Conor Dooley wrote:
> On Fri, Feb 03, 2023 at 02:56:41AM +0800, Hal Feng wrote:
>> On Wed, 1 Feb 2023 08:21:05 +0000, Conor Dooley wrote:
>> > On Wed, Feb 01, 2023 at 03:21:48PM +0800, Hal Feng wrote:
>> >> On Wed, 28 Dec 2022 22:48:43 +0000, Conor Dooley wrote:
>> >> > On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
>> > 
>> >> >> +/ {
>> >> >> +	compatible = "starfive,jh7110";
>> >> >> +	#address-cells = <2>;
>> >> >> +	#size-cells = <2>;
>> >> >> +
>> >> >> +	cpus {
>> >> >> +		#address-cells = <1>;
>> >> >> +		#size-cells = <0>;
>> >> >> +
>> >> >> +		S76_0: cpu@0 {
>> >> >> +			compatible = "sifive,u74-mc", "riscv";
>> >> > 
>> >> > The label here says S76 but the compatible says u74-mc.
>> >> 
>> >> U74-MC has 5 cores including 1 * S7 core and 4 * U74 cores.
>> >> 
>> >> > Which is correct? Your docs say S7 and S76, so I would imagine that it
>> >> > is actually an S76?
>> >> 
>> >> I found SiFive website [1] call it S76, but call it S7 in other places.
>> >> So I misunderstood this. Considering the ISA difference you described
>> >> as below, I think it's proper to change the label to "S7_0".
>> > 
>> > I'm less worried about the label & more interested in the compatible.
>> > hart0 is, as you say, not a u74. Should we not be adding a "sifive,s7"
>> > compatible string to Documentation/devicetree/bindings/riscv/cpus.yaml
>> > and using that here instead?
>> 
>> First of all, it's my fault that I didn't check the revision of U74-MC
>> manual, so most of my previous replies might not make sense.
> 
> No that's fine. The manual stuff confused me too when I went looking
> initially, and I still get get mixed up by the fact that there are
> core-complex manuals but not core manuals.
> 
>> If we add a new compatible string for S7, should we change the compatibles
>> of hart1~3 to "sifive,u74" also? And then, there may be no point keeping some
>> compatible strings of core complex like "sifive,u74-mc" and "sifive,u54-mc".
>> I'm not sure about this.
> 
> [...]
> 
>> >> Yes, "RV64IMAC" is correct. The monitor core in U74-MC is a
>> >> S7-series core, not S76.
>> > 
>> > Cool, thanks.
>> 
>> Now I think it might be another version of S76.
> 
> The SiFive docs describe the u74-mc core complex, which AFAIU you have,
> as being 1x S7 & 4x U7.
> 
> I'd be happy with new binding for "sifive,s7" & then we use that here.
> If you're sure it's S76, we can also use that. S76 is described, in what
> docs I can see, as a core complex containing an S7, so S7 seems likely
> to be correct?

I will add a new binding for "sifive,s7" and modify the code as follows.

	S7_0: cpu@0 {
		compatible = "sifive,s7", "riscv";
		...
		riscv,isa = "rv64imac_zicsr_zba_zbb";
		...
	};

> 
> u7, u74 & u74-mc are valid compatibles, added by SiFive, in commit
> 75e6d7248efc ("dt-bindings: riscv: Update DT binding docs to support
> SiFive FU740 SoC"). Unfortunately, they never actually *used* those
> compatibles for anything, and just used "sifive,bullet0" for the fu740.
> 
> I'll accept any of u7, u74 or u74-mc for those harts.
> 
>> >> >> +			tlb-split;
>> >> >> +			status = "disabled";
>> >> >> +
>> >> >> +			cpu0_intc: interrupt-controller {
>> >> >> +				compatible = "riscv,cpu-intc";
>> >> >> +				interrupt-controller;
>> >> >> +				#interrupt-cells = <1>;
>> >> >> +			};
>> >> >> +		};
>> >> >> +
>> >> >> +		U74_1: cpu@1 {
>> >> >> +			compatible = "sifive,u74-mc", "riscv";
>> >> >> +			reg = <1>;
>> >> >> +			d-cache-block-size = <64>;
>> >> >> +			d-cache-sets = <64>;
>> >> >> +			d-cache-size = <32768>;
>> >> >> +			d-tlb-sets = <1>;
>> >> >> +			d-tlb-size = <40>;
>> >> >> +			device_type = "cpu";
>> >> >> +			i-cache-block-size = <64>;
>> >> >> +			i-cache-sets = <64>;
>> >> >> +			i-cache-size = <32768>;
>> >> >> +			i-tlb-sets = <1>;
>> >> >> +			i-tlb-size = <40>;
>> >> >> +			mmu-type = "riscv,sv39";
>> >> >> +			next-level-cache = <&ccache>;
>> >> >> +			riscv,isa = "rv64imafdc";
>> >> > 
>> >> > That also begs the question:
>> >> > Do your u74s support RV64GBC, as the (current) SiFive documentation
>> >> > suggests?
>> >> 
>> >> Actually, U74 doesn't support the full B extension, and the SiFive doc [1]
>> > 
>> > Yeah, I knew asking that question that the "RV64GBC" in SiFive's online
>> > documentation was using outdated terminology. Also, that is not the doc
>> > for your core complex as far as I can tell. That is the document for
>> > impid 0x0621_1222, whereas (IIRC) your core is 0x0421_0427.
>> > Jess and I had a look one evening but could not find the 21G1.02.00
>> > revision of this document, which is the one corresponding to 0x421_0427.
>> > See Table 92 for more details.
>> 
>> I found the 21G1.02.00 revision on StarFive internal net, but I'm not sure
>> whether I can make it public and I am checking this.
> 
> Yeah, certainly don't do anything without cross-checking!
> 
>> This revision records
>> that the ISA of 21G1.02.00 U74 is "RV64GCB" and ISA of 21G1.02.00 S7 is
>> "RV64IMACB". I am asking someone to check with SiFive whether both 21G1.02.00
>> U74 and S7 support the full B extension.
> 
> Having cross-checked a 21G1.01.00 document against a 21G2.01.00 one, I'm
> 99% sure that you have _Zba_Zbb.
> The G2.01 document says _Zba_Zbb & has the same instructions listed as
> supported as the G1.02 one.
> I've also tried the Zbb support patches posted by Heiko [2] on a
> VisionFive V2 and had them work - which is why a definitive statement on
> the version of Zbb supported would be really great to have!
> 
> [2] - https://lore.kernel.org/linux-riscv/20230113212301.3534711-1-heiko@sntech.de/

The 21G1.02.00 document is still not allowed to be public so far. By
comparing with instructions included in b extensions [1], I can confirm
that the 21G1.02.00 only supports Zba and Zbb.

[1] https://github.com/riscv/riscv-bitmanip/blob/main/bitmanip/overview.adoc#

Zicsr is also supported as described in 21G1.02.00 document. So I will
modify as follows.

	U74_1: cpu@1 {
		compatible = "sifive,u74-mc", "riscv";
		...
		riscv,isa = "rv64imafdc_zicsr_zba_zbb";
		...
	};

Best regards,
Hal

> 
>> >> describes the ISA of U74 is "RV64GC_Zba_Zbb_Sscofpmf" which "G" includes
>> >> "IMAFD".
>> > 
>> > I could not find the 21G1.02.00 version of this document online, but I
>> > was able to find the 21G1.01.00 version of it & that version does not
>> > support the Sscofpmf extension (but does have Zba/Zbb support).
>> > 
>> >> "_Zba_Zbb_Sscofpmf" is not shown in other device trees such as
>> >> jh7100.dtsi and fu740-c000.dtsi, so I didn't show them here.
>> > 
>> > Just because other devicetrees omit them, doesn't mean that you should
>> > too!
>> > This compatible should be an accurate description of your hardware, so
>> > you should add what you actually have.
>> 
>> Will keep it in mind. Thank you.
> 
> FWIW, the deadline for getting material in for v6.3 has already passed,
> so you can send the next version of this series without waiting for
> clarification on the compatibles & ISA string. We should have plenty of
> time to get those fixed up before the series gets applied.
> 
> Thanks,
> Conor.
> 
>> > If you have Zba and Zbb, then add them.
>> > I would double check against your internal documentation for 21G2.02.00
>> > as to whether you do have Sscofpmf, and if you do, add that too!
>> > 
>> > That way, whenever support for those extensions lands, the jh7110 will
>> > automatically pick it up, rather than needing to have them retrofitted.
>> > 
>> >> [1] https://sifive.cdn.prismic.io/sifive/2dd11994-693c-4360-8aea-5453d8642c42_u74mc_core_complex_manual_21G3.pdf
Conor Dooley Feb. 15, 2023, 7:42 a.m. UTC | #25 
Hey Hal!

On Wed, Feb 15, 2023 at 11:07:15AM +0800, Hal Feng wrote:
> On Thu, 2 Feb 2023 19:41:33 +0000, Conor Dooley wrote:
> > On Fri, Feb 03, 2023 at 02:56:41AM +0800, Hal Feng wrote:
> >> On Wed, 1 Feb 2023 08:21:05 +0000, Conor Dooley wrote:
> >> > On Wed, Feb 01, 2023 at 03:21:48PM +0800, Hal Feng wrote:
> >> >> On Wed, 28 Dec 2022 22:48:43 +0000, Conor Dooley wrote:
> >> >> > On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
> >> > 
> >> >> >> +/ {
> >> >> >> +	compatible = "starfive,jh7110";
> >> >> >> +	#address-cells = <2>;
> >> >> >> +	#size-cells = <2>;
> >> >> >> +
> >> >> >> +	cpus {
> >> >> >> +		#address-cells = <1>;
> >> >> >> +		#size-cells = <0>;
> >> >> >> +
> >> >> >> +		S76_0: cpu@0 {
> >> >> >> +			compatible = "sifive,u74-mc", "riscv";
> >> >> > 
> >> >> > The label here says S76 but the compatible says u74-mc.
> >> >> 
> >> >> U74-MC has 5 cores including 1 * S7 core and 4 * U74 cores.
> >> >> 
> >> >> > Which is correct? Your docs say S7 and S76, so I would imagine that it
> >> >> > is actually an S76?
> >> >> 
> >> >> I found SiFive website [1] call it S76, but call it S7 in other places.
> >> >> So I misunderstood this. Considering the ISA difference you described
> >> >> as below, I think it's proper to change the label to "S7_0".
> >> > 
> >> > I'm less worried about the label & more interested in the compatible.
> >> > hart0 is, as you say, not a u74. Should we not be adding a "sifive,s7"
> >> > compatible string to Documentation/devicetree/bindings/riscv/cpus.yaml
> >> > and using that here instead?
> >> 
> >> First of all, it's my fault that I didn't check the revision of U74-MC
> >> manual, so most of my previous replies might not make sense.
> > 
> > No that's fine. The manual stuff confused me too when I went looking
> > initially, and I still get get mixed up by the fact that there are
> > core-complex manuals but not core manuals.
> > 
> >> If we add a new compatible string for S7, should we change the compatibles
> >> of hart1~3 to "sifive,u74" also? And then, there may be no point keeping some
> >> compatible strings of core complex like "sifive,u74-mc" and "sifive,u54-mc".
> >> I'm not sure about this.
> > 
> > [...]
> > 
> >> >> Yes, "RV64IMAC" is correct. The monitor core in U74-MC is a
> >> >> S7-series core, not S76.
> >> > 
> >> > Cool, thanks.
> >> 
> >> Now I think it might be another version of S76.
> > 
> > The SiFive docs describe the u74-mc core complex, which AFAIU you have,
> > as being 1x S7 & 4x U7.
> > 
> > I'd be happy with new binding for "sifive,s7" & then we use that here.
> > If you're sure it's S76, we can also use that. S76 is described, in what
> > docs I can see, as a core complex containing an S7, so S7 seems likely
> > to be correct?
> 
> I will add a new binding for "sifive,s7" and modify the code as follows.
> 
> 	S7_0: cpu@0 {
> 		compatible = "sifive,s7", "riscv";
> 		...
> 		riscv,isa = "rv64imac_zicsr_zba_zbb";

I'm not sure that I'd bother with the zicsr, it gets added automagically
by the Makefile if needed:
| # Newer binutils versions default to ISA spec version 20191213 which moves some
| # instructions from the I extension to the Zicsr and Zifencei extensions.
| toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
| riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei

Otherwise, thanks for the actual confirmation of zba/zbb!

Thanks,
Conor.
Conor Dooley Feb. 15, 2023, 7:59 a.m. UTC | #26 
On Wed, Feb 15, 2023 at 07:42:32AM +0000, Conor Dooley wrote:
> Hey Hal!
> 
> On Wed, Feb 15, 2023 at 11:07:15AM +0800, Hal Feng wrote:
> > On Thu, 2 Feb 2023 19:41:33 +0000, Conor Dooley wrote:
> > > On Fri, Feb 03, 2023 at 02:56:41AM +0800, Hal Feng wrote:
> > >> On Wed, 1 Feb 2023 08:21:05 +0000, Conor Dooley wrote:
> > >> > On Wed, Feb 01, 2023 at 03:21:48PM +0800, Hal Feng wrote:
> > >> >> On Wed, 28 Dec 2022 22:48:43 +0000, Conor Dooley wrote:
> > >> >> > On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:
> > >> > 
> > >> >> >> +/ {
> > >> >> >> +	compatible = "starfive,jh7110";
> > >> >> >> +	#address-cells = <2>;
> > >> >> >> +	#size-cells = <2>;
> > >> >> >> +
> > >> >> >> +	cpus {
> > >> >> >> +		#address-cells = <1>;
> > >> >> >> +		#size-cells = <0>;
> > >> >> >> +
> > >> >> >> +		S76_0: cpu@0 {
> > >> >> >> +			compatible = "sifive,u74-mc", "riscv";
> > >> >> > 
> > >> >> > The label here says S76 but the compatible says u74-mc.
> > >> >> 
> > >> >> U74-MC has 5 cores including 1 * S7 core and 4 * U74 cores.
> > >> >> 
> > >> >> > Which is correct? Your docs say S7 and S76, so I would imagine that it
> > >> >> > is actually an S76?
> > >> >> 
> > >> >> I found SiFive website [1] call it S76, but call it S7 in other places.
> > >> >> So I misunderstood this. Considering the ISA difference you described
> > >> >> as below, I think it's proper to change the label to "S7_0".
> > >> > 
> > >> > I'm less worried about the label & more interested in the compatible.
> > >> > hart0 is, as you say, not a u74. Should we not be adding a "sifive,s7"
> > >> > compatible string to Documentation/devicetree/bindings/riscv/cpus.yaml
> > >> > and using that here instead?
> > >> 
> > >> First of all, it's my fault that I didn't check the revision of U74-MC
> > >> manual, so most of my previous replies might not make sense.
> > > 
> > > No that's fine. The manual stuff confused me too when I went looking
> > > initially, and I still get get mixed up by the fact that there are
> > > core-complex manuals but not core manuals.
> > > 
> > >> If we add a new compatible string for S7, should we change the compatibles
> > >> of hart1~3 to "sifive,u74" also? And then, there may be no point keeping some
> > >> compatible strings of core complex like "sifive,u74-mc" and "sifive,u54-mc".
> > >> I'm not sure about this.
> > > 
> > > [...]
> > > 
> > >> >> Yes, "RV64IMAC" is correct. The monitor core in U74-MC is a
> > >> >> S7-series core, not S76.
> > >> > 
> > >> > Cool, thanks.
> > >> 
> > >> Now I think it might be another version of S76.
> > > 
> > > The SiFive docs describe the u74-mc core complex, which AFAIU you have,
> > > as being 1x S7 & 4x U7.
> > > 
> > > I'd be happy with new binding for "sifive,s7" & then we use that here.
> > > If you're sure it's S76, we can also use that. S76 is described, in what
> > > docs I can see, as a core complex containing an S7, so S7 seems likely
> > > to be correct?
> > 
> > I will add a new binding for "sifive,s7" and modify the code as follows.
> > 
> > 	S7_0: cpu@0 {
> > 		compatible = "sifive,s7", "riscv";
> > 		...
> > 		riscv,isa = "rv64imac_zicsr_zba_zbb";
> 
> I'm not sure that I'd bother with the zicsr, it gets added automagically
> by the Makefile if needed:

Meh, I probably shouldn't have replied to this first thing in the
morning as this comment of mine doesn't really make sense.
I skipped the middle part of my point here...
What I meant was that you can avoid zicsr & zifencei because when the
binding was defined they were included in i. I meant to use the
following as a kinda explanation of it depending on the version of the
ISA spec & that we just assume that zicsr & zifencei are present.
I suppose you can add them to the isa string if you like, dtbs_check
shouldn't complain!

> | # Newer binutils versions default to ISA spec version 20191213 which moves some
> | # instructions from the I extension to the Zicsr and Zifencei extensions.
> | toolchain-need-zicsr-zifencei := $(call cc-option-yn, -march=$(riscv-march-y)_zicsr_zifencei)
> | riscv-march-$(toolchain-need-zicsr-zifencei) := $(riscv-march-y)_zicsr_zifencei
> 
> Otherwise, thanks for the actual confirmation of zba/zbb!
> 
> Thanks,
> Conor.
>
diff mbox series

Patch

diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
new file mode 100644
index 000000000000..64d260ea1f29
--- /dev/null
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -0,0 +1,411 @@ 
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * Copyright (C) 2022 StarFive Technology Co., Ltd.
+ * Copyright (C) 2022 Emil Renner Berthing <kernel@esmil.dk>
+ */
+
+/dts-v1/;
+#include <dt-bindings/clock/starfive,jh7110-crg.h>
+#include <dt-bindings/reset/starfive,jh7110-crg.h>
+
+/ {
+	compatible = "starfive,jh7110";
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		S76_0: cpu@0 {
+			compatible = "sifive,u74-mc", "riscv";
+			reg = <0>;
+			d-cache-block-size = <64>;
+			d-cache-sets = <64>;
+			d-cache-size = <8192>;
+			d-tlb-sets = <1>;
+			d-tlb-size = <40>;
+			device_type = "cpu";
+			i-cache-block-size = <64>;
+			i-cache-sets = <64>;
+			i-cache-size = <16384>;
+			i-tlb-sets = <1>;
+			i-tlb-size = <40>;
+			mmu-type = "riscv,sv39";
+			next-level-cache = <&ccache>;
+			riscv,isa = "rv64imac";
+			tlb-split;
+			status = "disabled";
+
+			cpu0_intc: interrupt-controller {
+				compatible = "riscv,cpu-intc";
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+		};
+
+		U74_1: cpu@1 {
+			compatible = "sifive,u74-mc", "riscv";
+			reg = <1>;
+			d-cache-block-size = <64>;
+			d-cache-sets = <64>;
+			d-cache-size = <32768>;
+			d-tlb-sets = <1>;
+			d-tlb-size = <40>;
+			device_type = "cpu";
+			i-cache-block-size = <64>;
+			i-cache-sets = <64>;
+			i-cache-size = <32768>;
+			i-tlb-sets = <1>;
+			i-tlb-size = <40>;
+			mmu-type = "riscv,sv39";
+			next-level-cache = <&ccache>;
+			riscv,isa = "rv64imafdc";
+			tlb-split;
+
+			cpu1_intc: interrupt-controller {
+				compatible = "riscv,cpu-intc";
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+		};
+
+		U74_2: cpu@2 {
+			compatible = "sifive,u74-mc", "riscv";
+			reg = <2>;
+			d-cache-block-size = <64>;
+			d-cache-sets = <64>;
+			d-cache-size = <32768>;
+			d-tlb-sets = <1>;
+			d-tlb-size = <40>;
+			device_type = "cpu";
+			i-cache-block-size = <64>;
+			i-cache-sets = <64>;
+			i-cache-size = <32768>;
+			i-tlb-sets = <1>;
+			i-tlb-size = <40>;
+			mmu-type = "riscv,sv39";
+			next-level-cache = <&ccache>;
+			riscv,isa = "rv64imafdc";
+			tlb-split;
+
+			cpu2_intc: interrupt-controller {
+				compatible = "riscv,cpu-intc";
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+		};
+
+		U74_3: cpu@3 {
+			compatible = "sifive,u74-mc", "riscv";
+			reg = <3>;
+			d-cache-block-size = <64>;
+			d-cache-sets = <64>;
+			d-cache-size = <32768>;
+			d-tlb-sets = <1>;
+			d-tlb-size = <40>;
+			device_type = "cpu";
+			i-cache-block-size = <64>;
+			i-cache-sets = <64>;
+			i-cache-size = <32768>;
+			i-tlb-sets = <1>;
+			i-tlb-size = <40>;
+			mmu-type = "riscv,sv39";
+			next-level-cache = <&ccache>;
+			riscv,isa = "rv64imafdc";
+			tlb-split;
+
+			cpu3_intc: interrupt-controller {
+				compatible = "riscv,cpu-intc";
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+		};
+
+		U74_4: cpu@4 {
+			compatible = "sifive,u74-mc", "riscv";
+			reg = <4>;
+			d-cache-block-size = <64>;
+			d-cache-sets = <64>;
+			d-cache-size = <32768>;
+			d-tlb-sets = <1>;
+			d-tlb-size = <40>;
+			device_type = "cpu";
+			i-cache-block-size = <64>;
+			i-cache-sets = <64>;
+			i-cache-size = <32768>;
+			i-tlb-sets = <1>;
+			i-tlb-size = <40>;
+			mmu-type = "riscv,sv39";
+			next-level-cache = <&ccache>;
+			riscv,isa = "rv64imafdc";
+			tlb-split;
+
+			cpu4_intc: interrupt-controller {
+				compatible = "riscv,cpu-intc";
+				interrupt-controller;
+				#interrupt-cells = <1>;
+			};
+		};
+
+		cpu-map {
+			cluster0 {
+				core0 {
+					cpu = <&S76_0>;
+				};
+
+				core1 {
+					cpu = <&U74_1>;
+				};
+
+				core2 {
+					cpu = <&U74_2>;
+				};
+
+				core3 {
+					cpu = <&U74_3>;
+				};
+
+				core4 {
+					cpu = <&U74_4>;
+				};
+			};
+		};
+	};
+
+	osc: osc {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	rtc_osc: rtc_osc {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	gmac0_rmii_refin: gmac0_rmii_refin {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	gmac0_rgmii_rxin: gmac0_rgmii_rxin {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	gmac1_rmii_refin: gmac1_rmii_refin {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	gmac1_rgmii_rxin: gmac1_rgmii_rxin {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	i2stx_bclk_ext: i2stx_bclk_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	i2stx_lrck_ext: i2stx_lrck_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	i2srx_bclk_ext: i2srx_bclk_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	i2srx_lrck_ext: i2srx_lrck_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	tdm_ext: tdm_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	mclk_ext: mclk_ext {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+	};
+
+	soc {
+		compatible = "simple-bus";
+		interrupt-parent = <&plic>;
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		clint: clint@2000000 {
+			compatible = "starfive,jh7110-clint", "sifive,clint0";
+			reg = <0x0 0x2000000 0x0 0x10000>;
+			interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>,
+					      <&cpu1_intc 3>, <&cpu1_intc 7>,
+					      <&cpu2_intc 3>, <&cpu2_intc 7>,
+					      <&cpu3_intc 3>, <&cpu3_intc 7>,
+					      <&cpu4_intc 3>, <&cpu4_intc 7>;
+		};
+
+		plic: plic@c000000 {
+			compatible = "starfive,jh7110-plic", "sifive,plic-1.0.0";
+			reg = <0x0 0xc000000 0x0 0x4000000>;
+			interrupts-extended = <&cpu0_intc 11>,
+					      <&cpu1_intc 11>, <&cpu1_intc 9>,
+					      <&cpu2_intc 11>, <&cpu2_intc 9>,
+					      <&cpu3_intc 11>, <&cpu3_intc 9>,
+					      <&cpu4_intc 11>, <&cpu4_intc 9>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+			#address-cells = <0>;
+			riscv,ndev = <136>;
+		};
+
+		ccache: cache-controller@2010000 {
+			compatible = "starfive,jh7110-ccache", "sifive,ccache0", "cache";
+			reg = <0x0 0x2010000 0x0 0x4000>;
+			interrupts = <1>, <3>, <4>, <2>;
+			cache-block-size = <64>;
+			cache-level = <2>;
+			cache-sets = <2048>;
+			cache-size = <2097152>;
+			cache-unified;
+		};
+
+		uart0: serial@10000000 {
+			compatible = "snps,dw-apb-uart";
+			reg = <0x0 0x10000000 0x0 0x10000>;
+			clocks = <&syscrg JH7110_SYSCLK_UART0_CORE>,
+				 <&syscrg JH7110_SYSCLK_UART0_APB>;
+			clock-names = "baudclk", "apb_pclk";
+			resets = <&syscrg JH7110_SYSRST_UART0_APB>;
+			interrupts = <32>;
+			reg-io-width = <4>;
+			reg-shift = <2>;
+			status = "disabled";
+		};
+
+		uart1: serial@10010000 {
+			compatible = "snps,dw-apb-uart";
+			reg = <0x0 0x10010000 0x0 0x10000>;
+			clocks = <&syscrg JH7110_SYSCLK_UART1_CORE>,
+				 <&syscrg JH7110_SYSCLK_UART1_APB>;
+			clock-names = "baudclk", "apb_pclk";
+			resets = <&syscrg JH7110_SYSRST_UART1_APB>;
+			interrupts = <33>;
+			reg-io-width = <4>;
+			reg-shift = <2>;
+			status = "disabled";
+		};
+
+		uart2: serial@10020000 {
+			compatible = "snps,dw-apb-uart";
+			reg = <0x0 0x10020000 0x0 0x10000>;
+			clocks = <&syscrg JH7110_SYSCLK_UART2_CORE>,
+				 <&syscrg JH7110_SYSCLK_UART2_APB>;
+			clock-names = "baudclk", "apb_pclk";
+			resets = <&syscrg JH7110_SYSRST_UART2_APB>;
+			interrupts = <34>;
+			reg-io-width = <4>;
+			reg-shift = <2>;
+			status = "disabled";
+		};
+
+		uart3: serial@12000000 {
+			compatible = "snps,dw-apb-uart";
+			reg = <0x0 0x12000000 0x0 0x10000>;
+			clocks = <&syscrg JH7110_SYSCLK_UART3_CORE>,
+				 <&syscrg JH7110_SYSCLK_UART3_APB>;
+			clock-names = "baudclk", "apb_pclk";
+			resets = <&syscrg JH7110_SYSRST_UART3_APB>;
+			interrupts = <45>;
+			reg-io-width = <4>;
+			reg-shift = <2>;
+			status = "disabled";
+		};
+
+		uart4: serial@12010000 {
+			compatible = "snps,dw-apb-uart";
+			reg = <0x0 0x12010000 0x0 0x10000>;
+			clocks = <&syscrg JH7110_SYSCLK_UART4_CORE>,
+				 <&syscrg JH7110_SYSCLK_UART4_APB>;
+			clock-names = "baudclk", "apb_pclk";
+			resets = <&syscrg JH7110_SYSRST_UART4_APB>;
+			interrupts = <46>;
+			reg-io-width = <4>;
+			reg-shift = <2>;
+			status = "disabled";
+		};
+
+		uart5: serial@12020000 {
+			compatible = "snps,dw-apb-uart";
+			reg = <0x0 0x12020000 0x0 0x10000>;
+			clocks = <&syscrg JH7110_SYSCLK_UART5_CORE>,
+				 <&syscrg JH7110_SYSCLK_UART5_APB>;
+			clock-names = "baudclk", "apb_pclk";
+			resets = <&syscrg JH7110_SYSRST_UART5_APB>;
+			interrupts = <47>;
+			reg-io-width = <4>;
+			reg-shift = <2>;
+			status = "disabled";
+		};
+
+		syscrg: clock-controller@13020000 {
+			compatible = "starfive,jh7110-syscrg";
+			reg = <0x0 0x13020000 0x0 0x10000>;
+			clocks = <&osc>, <&gmac1_rmii_refin>,
+				 <&gmac1_rgmii_rxin>,
+				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
+				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
+				 <&tdm_ext>, <&mclk_ext>;
+			clock-names = "osc", "gmac1_rmii_refin",
+				      "gmac1_rgmii_rxin",
+				      "i2stx_bclk_ext", "i2stx_lrck_ext",
+				      "i2srx_bclk_ext", "i2srx_lrck_ext",
+				      "tdm_ext", "mclk_ext";
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+		};
+
+		gpio: gpio@13040000 {
+			compatible = "starfive,jh7110-sys-pinctrl";
+			reg = <0x0 0x13040000 0x0 0x10000>;
+			clocks = <&syscrg JH7110_SYSCLK_IOMUX_APB>;
+			resets = <&syscrg JH7110_SYSRST_IOMUX_APB>;
+			interrupts = <86>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+
+		aoncrg: clock-controller@17000000 {
+			compatible = "starfive,jh7110-aoncrg";
+			reg = <0x0 0x17000000 0x0 0x10000>;
+			clocks = <&osc>, <&rtc_osc>,
+				 <&gmac0_rmii_refin>, <&gmac0_rgmii_rxin>,
+				 <&syscrg JH7110_SYSCLK_STG_AXIAHB>,
+				 <&syscrg JH7110_SYSCLK_APB_BUS>,
+				 <&syscrg JH7110_SYSCLK_GMAC0_GTXCLK>;
+			clock-names = "osc", "rtc_osc", "gmac0_rmii_refin",
+				      "gmac0_rgmii_rxin", "stg_axiahb",
+				      "apb_bus", "gmac0_gtxclk";
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+		};
+
+		gpioa: gpio@17020000 {
+			compatible = "starfive,jh7110-aon-pinctrl";
+			reg = <0x0 0x17020000 0x0 0x10000>;
+			resets = <&aoncrg JH7110_AONRST_IOMUX>;
+			interrupts = <85>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+	};
+};