[PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----Original Message-----
From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski at canonical.com]
Sent: Friday, March 11, 2022 4:30 AM
To: Hawkins, Nick <nick.hawkins at hpe.com>>; Verdun, Jean-Marie <verdun at hpe.com>>
Cc: Arnd Bergmann <arnd at arndb.de>>; Olof Johansson <olof at lixom.net>>; soc at kernel.org; Rob Herring <robh+dt at kernel.org>>; linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

On 10/03/2022 20:52, nick.hawkins at hpe.com wrote:
>> From: Nick Hawkins <nick.hawkins at hpe.com>>
>> 
>> The HPE SoC is new to linux. This patch creates the basic device tree 
>> layout with minimum required for linux to boot. This includes timer 
>> and watchdog support.
>> 
>> Signed-off-by: Nick Hawkins <nick.hawkins at hpe.com>>
>> ---
>>  arch/arm/boot/dts/Makefile               |   2 +
>>  arch/arm/boot/dts/hpe-bmc-dl360gen10.dts |  27 +++++
>>  arch/arm/boot/dts/hpe-gxp.dtsi           | 148 +++++++++++++++++++++++
>>  3 files changed, 177 insertions(+)
>>  create mode 100644 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>  create mode 100644 arch/arm/boot/dts/hpe-gxp.dtsi
>> 
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile 
>> index e41eca79c950..2823b359d373 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -1550,3 +1550,5 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>>  	aspeed-bmc-vegman-n110.dtb \
>>  	aspeed-bmc-vegman-rx20.dtb \
>>  	aspeed-bmc-vegman-sx20.dtb
>> +dtb-$(CONFIG_ARCH_HPE_GXP) += \
>> +	hpe-bmc-dl360gen10.dtb

> Alphabetically, also in respect to other architectures, so before CONFIG_ARCH_INTEGRATOR.

Done

>> diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>> b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>> new file mode 100644
>> index 000000000000..da5eac1213a8
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>> @@ -0,0 +1,27 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Device Tree file for HPE DL360Gen10  */
>> +
>> +/include/ "hpe-gxp.dtsi"
>> +
>> +/ {
>> +	#address-cells = <1>>;
>> +	#size-cells = <1>>;
>> +	compatible = "hpe,gxp";

> Missing board compatible.

Will become compatible =  "hpe,gxp","hpe,bmc-dl360gen10"; If that seems okay to you.

>> +	model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
>> +
>> +	chosen {
>> +		bootargs = "earlyprintk console=ttyS2,115200";

> I have impression we talked about it...

Removed!

>> +	};
>> +
>> +	memory at 40000000 {
>> +		device_type = "memory";
>> +		reg = <0x40000000 0x20000000>>;
>> +	};
>> +
>> +	ahb {

> Why do you need empty node?

I do not, this was a placeholder for after the initial checkin where I would put all the board specific stuff. This has been removed.

>> +
>> +	};
>> +
>> +};
>> diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi 
>> b/arch/arm/boot/dts/hpe-gxp.dtsi new file mode 100644 index 
>> 000000000000..dfaf8df829fe
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/hpe-gxp.dtsi
>> @@ -0,0 +1,148 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Device Tree file for HPE GXP
>> + */
>> +
>> +/dts-v1/;
>> +/ {
>> +	model = "Hewlett Packard Enterprise GXP BMC";
>> +	compatible = "hpe,gxp";
>> +	#address-cells = <1>>;
>> +	#size-cells = <1>>;
>> +
>> +	cpus {
>> +		#address-cells = <1>>;
>> +		#size-cells = <0>>;
>> +
>> +		cpu at 0 {
>> +			compatible = "arm,cortex-a9";
>> +			device_type = "cpu";
>> +			reg = <0>>;
>> +		};
>> +	};
>> +
>> +	gxp-init at cefe0010 {

> Need a generic node name. gpx-init is specific.

Will do. But more than likely this is going to get removed as I try to push this option into the bootloader.

>> +		compatible = "hpe,gxp-cpu-init";
>> +		reg = <0xcefe0010 0x04>>;
>> +	};
>> +
>> +	memory at 40000000 {
>> +		device_type = "memory";
>> +		reg = <0x40000000 0x20000000>>;
>> +	};
>> +
>> +	ahb {

> By convention we call it soc.

>> +		compatible = "simple-bus";
>> +		#address-cells = <1>>;
>> +		#size-cells = <1>>;
>> +		device_type = "soc";
>> +		ranges;
>> +
>> +		vic0: interrupt-controller at ceff0000 {
>> +			compatible = "arm,pl192-vic";
>> +			interrupt-controller;
>> +			reg = <0xceff0000 0x1000>>;

> Please put reg after compatible, everywhere.

Done

>> +			#interrupt-cells = <1>>;
>> +		};
>> +
>> +		vic1: interrupt-controller at 80f00000 {
>> +			compatible = "arm,pl192-vic";
>> +			interrupt-controller;
>> +			reg = <0x80f00000 0x1000>>;
>> +			#interrupt-cells = <1>>;
>> +		};
>> +
>> +		timer0: timer at c0000080 {
>> +			compatible = "hpe,gxp-timer";
>> +			reg = <0xc0000080 0x1>>, <0xc0000094 0x01>>, <0xc0000088 0x08>>;
>> +			interrupts = <0>>;
>> +			interrupt-parent = <&vic0>>;
>> +			clock-frequency = <400000000>>;
>> +		};
>> +
>> +		uarta: serial at c00000e0 {
>> +			compatible = "ns16550a";
>> +			reg = <0xc00000e0 0x8>>;
>> +			interrupts = <17>>;
>> +			interrupt-parent = <&vic0>>;
>> +			clock-frequency = <1846153>>;
>> +			reg-shift = <0>>;
>> +		};
>> +
>> +		uartb: serial at c00000e8 {
>> +			compatible = "ns16550a";
>> +			reg = <0xc00000e8 0x8>>;
>> +			interrupts = <18>>;
>> +			interrupt-parent = <&vic0>>;
>> +			clock-frequency = <1846153>>;
>> +			reg-shift = <0>>;
>> +		};
>> +
>> +		uartc: serial at c00000f0 {
>> +			compatible = "ns16550a";
>> +			reg = <0xc00000f0 0x8>>;
>> +			interrupts = <19>>;
>> +			interrupt-parent = <&vic0>>;
>> +			clock-frequency = <1846153>>;
>> +			reg-shift = <0>>;
>> +		};
>> +
>> +		usb0: usb at cefe0000 {
>> +			compatible = "generic-ehci";

> I think one of previous comments was that you cannot have "generic-ehci"
> only, right?

Yes there was, I removed the usb0: ehci at cefe0000. I see now that this is in reference to the compatible. This is our ehci controller. What would be a more appropriate compatible? Do we need hpe,gxp-ehci perhaps?

>> +			reg = <0xcefe0000 0x100>>;
>> +			interrupts = <7>>;
>> +			interrupt-parent = <&vic0>>;
>> +		};
>> +
>> +		usb1: usb at cefe0100 {
>> +			compatible = "generic-ohci";
>> +			reg = <0xcefe0100 0x110>>;
>> +			interrupts = <6>>;
>> +			interrupt-parent = <&vic0>>;
>> +		};
>> +
>> +		vrom at 58000000 {
>> +			compatible = "mtd-ram";
>> +			bank-width = <4>>;
>> +			reg = <0x58000000 0x4000000>>;
>> +			#address-cells = <1>>;
>> +			#size-cells = <1>>;
>> +			partition at 0 {
>> +				label = "vrom-prime";
>> +				reg = <0x0 0x2000000>>;
>> +			};
>> +			partition at 2000000 {
>> +				label = "vrom-second";
>> +				reg = <0x2000000 0x2000000>>;
>> +			};
>> +		};
>> +
>> +		i2cg: syscon at c00000f8 {


>> +			compatible = "simple-mfd", "syscon";
>> +			reg = <0xc00000f8 0x08>>;
>> +		};
>> +	};
>> +
>> +	clocks {
>> +		osc: osc {

> Keep node naming consistent, so just "clk"... but it's also very generic comparing to others, so I wonder what is this clock?

We are in the process of redoing the clocks. This was the oscillator but no longer needed for the minimum boot config.

>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>>;
>> +			clock-output-names = "osc";
>> +			clock-frequency = <33333333>>;
>> +		};
>> +
>> +		iopclk: iopclk {
>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>>;
>> +			clock-output-names = "iopclk";
>> +			clock-frequency = <400000000>>;
>> +		};
>> +
>> +		memclk: memclk {
>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>>;
>> +			clock-output-names = "memclk";
>> +			clock-frequency = <800000000>>;
>> +		};

> What are these clocks? If external to the SoC, then where are they? On the board?

This was the internal iopclk and memclk they were both internal to the chip.
For now I am removing osc and memclk and will just have an iopclk that Gxp-timer will refer to.

>> +	};
>> +};

Thanks,

-Nick