Re: [PATCH v2 5/8] riscv: dts: renesas: Add initial devicetree for Renesas RZ/Five SoC

[Conor.Dooley]

Hey Prabhakar,
(btw should I use Lad or Prabhakar?)

On 15/08/2022 16:14, Lad Prabhakar wrote:
> Add initial device tree for Renesas RZ/Five RISC-V CPU Core (AX45MP
> Single).
> 
> Below is the list of IP blocks added in the initial SoC DTSI which can be
> used to boot via initramfs on RZ/Five SMARC EVK:
> - AX45MP CPU
> - CPG
> - PINCTRL
> - PLIC
> - SCIF0
> - SYSC
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
> v1->v2
> * Dropped including makefile change
> * Updated ndev count
> ---
>  arch/riscv/boot/dts/renesas/r9a07g043.dtsi | 121 +++++++++++++++++++++
>  1 file changed, 121 insertions(+)
>  create mode 100644 arch/riscv/boot/dts/renesas/r9a07g043.dtsi
> 
> diff --git a/arch/riscv/boot/dts/renesas/r9a07g043.dtsi b/arch/riscv/boot/dts/renesas/r9a07g043.dtsi
> new file mode 100644
> index 000000000000..b288d2607796
> --- /dev/null
> +++ b/arch/riscv/boot/dts/renesas/r9a07g043.dtsi
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * Device Tree Source for the RZ/Five SoC
> + *
> + * Copyright (C) 2022 Renesas Electronics Corp.
> + */
> +
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/clock/r9a07g043-cpg.h>
> +
> +/ {
> +	compatible = "renesas,r9a07g043";
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	/* clock can be either from exclk or crystal oscillator (XIN/XOUT) */
> +	extal_clk: extal-clk {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		/* This value must be overridden by the board */
> +		clock-frequency = <0>;

What's the value in having the clock-frequency here if the board .dtsi
overwrites it? dtbs_check will complain if someone forgets to fill it
IIUC & what the missing frequency means is also kinda obvious, no?

That aside, by convention so far we have put things like extals or
reference clocks below the /cpus node. Could you do the same here too
please?

> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		timebase-frequency = <24000000>;
> +
> +		ax45mp: cpu@0 {
> +			compatible = "andestech,ax45mp", "riscv";
> +			device_type = "cpu";
> +			reg = <0x0>;
> +			status = "okay";
> +			riscv,isa = "rv64imafdc";
> +			mmu-type = "riscv,sv39";
> +			i-cache-size = <0x8000>;
> +			i-cache-line-size = <0x40>;
> +			d-cache-size = <0x8000>;
> +			d-cache-line-size = <0x40>;
> +			clocks = <&cpg CPG_CORE R9A07G043_AX45MP_CORE0_CLK>,
> +				 <&cpg CPG_CORE R9A07G043_AX45MP_ACLK>;

I've been on a bit of a topology-fixing binge lately, so I noticed
that you are missing a link to the l2 cache here. FWIW this does show
up in userspace with things like "lstopo" so it might be nice to add
that in from the start. You don't need to have a driver for it at all,
just the entry itself & a "next-level-cache" entry for the CPU.

Other than those two things, and this l2 one is in the "nice to have"
category:
Reviewed-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>

> +
> +			cpu0_intc: interrupt-controller {
> +				#interrupt-cells = <1>;
> +				compatible = "riscv,cpu-intc";
> +				interrupt-controller;
> +			};
> +		};
> +	};
> +
> +	soc: soc {
> +		compatible = "simple-bus";
> +		interrupt-parent = <&plic>;
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		scif0: serial@1004b800 {
> +			compatible = "renesas,scif-r9a07g043",
> +				     "renesas,scif-r9a07g044";
> +			reg = <0 0x1004b800 0 0x400>;
> +			interrupts = <412 IRQ_TYPE_LEVEL_HIGH>,
> +				     <414 IRQ_TYPE_LEVEL_HIGH>,
> +				     <415 IRQ_TYPE_LEVEL_HIGH>,
> +				     <413 IRQ_TYPE_LEVEL_HIGH>,
> +				     <416 IRQ_TYPE_LEVEL_HIGH>,
> +				     <416 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "eri", "rxi", "txi",
> +					  "bri", "dri", "tei";
> +			clocks = <&cpg CPG_MOD R9A07G043_SCIF0_CLK_PCK>;
> +			clock-names = "fck";
> +			power-domains = <&cpg>;
> +			resets = <&cpg R9A07G043_SCIF0_RST_SYSTEM_N>;
> +			status = "disabled";
> +		};
> +
> +		cpg: clock-controller@11010000 {
> +			compatible = "renesas,r9a07g043-cpg";
> +			reg = <0 0x11010000 0 0x10000>;
> +			clocks = <&extal_clk>;
> +			clock-names = "extal";
> +			#clock-cells = <2>;
> +			#reset-cells = <1>;
> +			#power-domain-cells = <0>;
> +		};
> +
> +		sysc: system-controller@11020000 {
> +			compatible = "renesas,r9a07g043-sysc";
> +			reg = <0 0x11020000 0 0x10000>;
> +			status = "disabled";
> +		};
> +
> +		pinctrl: pinctrl@11030000 {
> +			compatible = "renesas,r9a07g043-pinctrl";
> +			reg = <0 0x11030000 0 0x10000>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			#interrupt-cells = <2>;
> +			interrupt-controller;
> +			gpio-ranges = <&pinctrl 0 0 152>;
> +			clocks = <&cpg CPG_MOD R9A07G043_GPIO_HCLK>;
> +			power-domains = <&cpg>;
> +			resets = <&cpg R9A07G043_GPIO_RSTN>,
> +				 <&cpg R9A07G043_GPIO_PORT_RESETN>,
> +				 <&cpg R9A07G043_GPIO_SPARE_RESETN>;
> +		};
> +
> +		plic: interrupt-controller@12c00000 {
> +			compatible = "renesas,r9a07g043-plic", "andestech,nceplic100";
> +			#interrupt-cells = <2>;
> +			#address-cells = <0>;
> +			riscv,ndev = <512>;
> +			interrupt-controller;
> +			reg = <0x0 0x12c00000 0 0x400000>;
> +			clocks = <&cpg CPG_MOD R9A07G043_NCEPLIC_ACLK>;
> +			power-domains = <&cpg>;
> +			resets = <&cpg R9A07G043_NCEPLIC_ARESETN>;
> +			interrupts-extended = <&cpu0_intc 11 &cpu0_intc 9>;
> +		};
> +	};
> +};