Re: [PATCH v3 3/3] arm64: dts: arm: add corstone1000 device tree

[Rui Miguel Silva <rui.silva@linaro.org>]

Hi Sudeep,
Thanks for the review.

On Thu, Mar 31, 2022 at 11:48:54AM +0100, Sudeep Holla wrote:
> On Wed, Mar 30, 2022 at 02:10:53PM +0100, Rui Miguel Silva wrote:
> > Corstone1000 is a platform from arm, which includes pre
> > verified Corstone SSE710 sub-system that combines Cortex-A and
> > Cortex-M processors [0].
> >
> > These device trees contains the necessary bits to support the
> > Corstone 1000 FVP (Fixed Virtual Platform) [1] and the
> > FPGA MPS3 board Cortex-A35 implementation at Cortex-A35 host
> > side of this platform. [2]
> >
> 
> I prefer not to have these static URLs in the commit log or in the files
> as they tend to get stale soon.
> 
> > 0: https://documentation-service.arm.com/static/619e02b1f45f0b1fbf3a8f16
> 
> https://developer.arm.com/documentation/102360/0000
> 
> > 1: https://developer.arm.com/tools-and-software/open-source-software/arm-platforms-software/arm-ecosystem-fvps
> > 2: https://documentation-service.arm.com/static/61f3f4d7fa8173727a1b71bf
> 
> https://developer.arm.com/documentation/dai0550/c/
> 
> Please use the above alternatives instead.

yeah, makes sense thanks.

> 
> > 
> > Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
> > ---
> >  arch/arm64/boot/dts/arm/Makefile              |   1 +
> >  arch/arm64/boot/dts/arm/corstone1000-fvp.dts  |  27 +++
> >  arch/arm64/boot/dts/arm/corstone1000-mps3.dts |  36 ++++
> >  arch/arm64/boot/dts/arm/corstone1000.dtsi     | 161 ++++++++++++++++++
> >  4 files changed, 225 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/arm/corstone1000-fvp.dts
> >  create mode 100644 arch/arm64/boot/dts/arm/corstone1000-mps3.dts
> >  create mode 100644 arch/arm64/boot/dts/arm/corstone1000.dtsi
> > 
> > diff --git a/arch/arm64/boot/dts/arm/Makefile b/arch/arm64/boot/dts/arm/Makefile
> > index 4382b73baef5..d908e96d7ddc 100644
> > --- a/arch/arm64/boot/dts/arm/Makefile
> > +++ b/arch/arm64/boot/dts/arm/Makefile
> > @@ -6,3 +6,4 @@ dtb-$(CONFIG_ARCH_VEXPRESS) += juno.dtb juno-r1.dtb juno-r2.dtb juno-scmi.dtb ju
> >  dtb-$(CONFIG_ARCH_VEXPRESS) += rtsm_ve-aemv8a.dtb
> >  dtb-$(CONFIG_ARCH_VEXPRESS) += vexpress-v2f-1xv7-ca53x2.dtb
> >  dtb-$(CONFIG_ARCH_VEXPRESS) += fvp-base-revc.dtb
> > +dtb-$(CONFIG_ARCH_VEXPRESS) += corstone1000-fvp.dtb corstone1000-mps3.dtb
> > diff --git a/arch/arm64/boot/dts/arm/corstone1000-fvp.dts b/arch/arm64/boot/dts/arm/corstone1000-fvp.dts
> > new file mode 100644
> > index 000000000000..dea8b5f4d68a
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/arm/corstone1000-fvp.dts
> > @@ -0,0 +1,27 @@
> > +// SPDX-License-Identifier: GPL-2.0 or MIT
> > +/*
> > + * Copyright (c) 2022, Arm Limited. All rights reserved.
> > + * Copyright (c) 2022, Linaro Limited. All rights reserved.
> > + *
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "corstone1000.dtsi"
> > +
> > +/ {
> > +	model = "ARM Corstone1000 FVP (Fixed Virtual Platform)";
> > +	compatible = "arm,corstone1000-fvp";
> > +
> > +	smsc: ethernet@4010000 {
> > +		compatible = "smsc,lan91c111";
> > +		reg = <0x40100000 0x10000>;
> > +		phy-mode = "mii";
> > +		interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>;
> > +		reg-io-width = <2>;
> > +	};
> > +};
> > +
> > +&cpu {
> > +	compatible = "arm,armv8";
> 
> I see the publicly available model contains Cortex-A35, looks like FVP
> does model the core and is not same as AEMs. So you can move this to dtsi IMO.

yeah, indeed.

> 
> > +};
> > diff --git a/arch/arm64/boot/dts/arm/corstone1000-mps3.dts b/arch/arm64/boot/dts/arm/corstone1000-mps3.dts
> > new file mode 100644
> > index 000000000000..9989586db70e
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/arm/corstone1000-mps3.dts
> > @@ -0,0 +1,36 @@
> > +// SPDX-License-Identifier: GPL-2.0 or MIT
> > +/*
> > + * Copyright (c) 2022, Arm Limited. All rights reserved.
> > + * Copyright (c) 2022, Linaro Limited. All rights reserved.
> > + *
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "corstone1000.dtsi"
> > +
> > +/ {
> > +	model = "ARM Corstone1000 FPGA MPS3 board";
> > +	compatible = "arm,corstone1000-mps3";
> > +
> > +	smsc: ethernet@4010000 {
> > +		compatible = "smsc,lan9220", "smsc,lan9115";
> > +		reg = <0x40100000 0x10000>;
> > +		phy-mode = "mii";
> > +		interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>;
> > +		reg-io-width = <2>;
> > +		smsc,irq-push-pull;
> > +	};
> > +
> > +	usb_host: usb@40200000 {
> > +		compatible = "nxp,usb-isp1763";
> > +		reg = <0x40200000 0x100000>;
> > +		interrupts = <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>;
> > +		bus-width = <16>;
> > +		dr_mode = "host";
> > +	};
> > +};
> > +
> > +&cpu {
> > +	compatible = "arm,cortex-a35";
> > +};
> > diff --git a/arch/arm64/boot/dts/arm/corstone1000.dtsi b/arch/arm64/boot/dts/arm/corstone1000.dtsi
> > new file mode 100644
> > index 000000000000..194d959de828
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/arm/corstone1000.dtsi
> > @@ -0,0 +1,161 @@
> > +// SPDX-License-Identifier: GPL-2.0 or MIT
> > +/*
> > + * Copyright (c) 2022, Arm Limited. All rights reserved.
> > + * Copyright (c) 2022, Linaro Limited. All rights reserved.
> > + *
> > + */
> > +
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +/ {
> > +	interrupt-parent = <&gic>;
> > +	#address-cells = <1>;
> > +	#size-cells = <1>;
> > +
> > +	aliases {
> > +		serial0 = &uart0;
> > +		serial1 = &uart1;
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = "serial0:115200n8";
> > +	};
> > +
> > +	cpus {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		cpu: cpu@0 {
> > +			device_type = "cpu";
> > +			reg = <0>;
> > +			next-level-cache = <&L2_0>;
> > +		};
> > +	};
> > +
> > +	memory@88200000 {
> > +		device_type = "memory";
> > +		reg = <0x88200000 0x77e00000>;
> > +	};
> > +
> > +	gic: interrupt-controller@1c000000 {
> > +		compatible = "arm,gic-400";
> > +		#interrupt-cells = <3>;
> > +		#address-cells = <0>;
> > +		interrupt-controller;
> > +		reg =	<0x1c010000 0x1000>,
> > +			<0x1c02f000 0x2000>,
> > +			<0x1c04f000 0x1000>,
> > +			<0x1c06f000 0x2000>;
> > +		interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(1) |
> > +			      IRQ_TYPE_LEVEL_LOW)>;
> > +	};
> > +
> > +	L2_0: l2-cache0 {
> > +		compatible = "cache";
> 
> Any other properties ?
> 
> > +	};
> > +
> > +	refclk100mhz: refclk100mhz {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <100000000>;
> > +		clock-output-names = "apb_pclk";
> > +	};
> > +
> > +	smbclk: refclk24mhzx2 {
> > +		/* Reference 24MHz clock x 2 */
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <48000000>;
> > +		clock-output-names = "smclk";
> > +	};
> > +
> > +	timer {
> > +		compatible = "arm,armv8-timer";
> > +		interrupts =	<GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(1) |
> > +				 IRQ_TYPE_LEVEL_LOW)>,
> > +				<GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(1) |
> > +				 IRQ_TYPE_LEVEL_LOW)>,
> > +				<GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(1) |
> > +				 IRQ_TYPE_LEVEL_LOW)>,
> > +				<GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(1) |
> > +				 IRQ_TYPE_LEVEL_LOW)>;
> > +	};
> > +
> > +	uartclk: uartclk {
> > +		/* UART clock - 50MHz */
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <50000000>;
> > +		clock-output-names = "uartclk";
> > +	};
> > +
> > +	psci {
> > +		compatible = "arm,psci-1.0", "arm,psci-0.2";
> > +		method = "smc";
> > +	};
> > +
> > +	soc {
> > +		compatible = "simple-bus";
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		interrupt-parent = <&gic>;
> > +		ranges;
> > +
> > +		timer@1a220000 {
> > +			compatible = "arm,armv7-timer-mem";
> > +			reg = <0x1a220000 0x1000>;
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			clock-frequency = <50000000>;
> > +			ranges;
> > +
> > +			frame@1a230000 {
> > +				frame-number = <0>;
> > +				interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> > +				reg = <0x1a230000 0x1000>;
> > +			};
> > +		};
> > +
> > +		uart0: serial@1a510000 {
> > +			compatible = "arm,pl011", "arm,primecell";
> > +			reg = <0x1a510000 0x1000>;
> > +			interrupt-parent = <&gic>;
> 
> Are these really needed even if there is only one interrupt controller
> in the system ?

correct, can be dropped.

> 
> > +			interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&uartclk>, <&refclk100mhz>;
> > +			clock-names = "uartclk", "apb_pclk";
> > +		};
> > +
> > +		uart1: serial@1a520000 {
> > +			compatible = "arm,pl011", "arm,primecell";
> > +			reg = <0x1a520000 0x1000>;
> > +			interrupt-parent = <&gic>;
> > +			interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&uartclk>, <&refclk100mhz>;
> > +			clock-names = "uartclk", "apb_pclk";
> > +		};
> > +
> > +		mhu_hse1: mailbox@1b820000 {
> > +			compatible = "arm,mhuv2-tx", "arm,primecell";
> > +			reg = <0x1b820000 0x1000>;
> > +			clocks = <&refclk100mhz>;
> > +			clock-names = "apb_pclk";
> > +			interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
> > +			#mbox-cells = <2>;
> > +			arm,mhuv2-protocols = <0 0>;
> > +			secure-status = "okay";     /* secure-world-only */
> 
> Please drop the above. Though I see it is in the binding, no one uses
> it in the kernel and I prefer not to have this.

the secure partitions in secure world use this mailbox to
doorbell the secure enclave so, after talking with Rob he suggested to
use this bindings to make this clear. So, I will keep this ones.

Cheers,
   Rui
> 
> -- 
> Regards,
> Sudeep

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