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

["Hawkins, Nick" <nick.hawkins@hpe.com>]

-----Original Message-----
From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@canonical.com]
Sent: Friday, March 11, 2022 4:30 AM
To: Hawkins, Nick <nick.hawkins@hpe.com>>; Verdun, Jean-Marie <verdun@hpe.com>>
Cc: Arnd Bergmann <arnd@arndb.de>>; Olof Johansson <olof@lixom.net>>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@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@hpe.com wrote:
>> From: Nick Hawkins <nick.hawkins@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@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@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@0 {
>> +			compatible = "arm,cortex-a9";
>> +			device_type = "cpu";
>> +			reg = <0>>;
>> +		};
>> +	};
>> +
>> +	gxp-init@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@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@ceff0000 {
>> +			compatible = "arm,pl192-vic";
>> +			interrupt-controller;
>> +			reg = <0xceff0000 0x1000>>;

> Please put reg after compatible, everywhere.

Done

>> +			#interrupt-cells = <1>>;
>> +		};
>> +
>> +		vic1: interrupt-controller@80f00000 {
>> +			compatible = "arm,pl192-vic";
>> +			interrupt-controller;
>> +			reg = <0x80f00000 0x1000>>;
>> +			#interrupt-cells = <1>>;
>> +		};
>> +
>> +		timer0: timer@c0000080 {
>> +			compatible = "hpe,gxp-timer";
>> +			reg = <0xc0000080 0x1>>, <0xc0000094 0x01>>, <0xc0000088 0x08>>;
>> +			interrupts = <0>>;
>> +			interrupt-parent = <&vic0>>;
>> +			clock-frequency = <400000000>>;
>> +		};
>> +
>> +		uarta: serial@c00000e0 {
>> +			compatible = "ns16550a";
>> +			reg = <0xc00000e0 0x8>>;
>> +			interrupts = <17>>;
>> +			interrupt-parent = <&vic0>>;
>> +			clock-frequency = <1846153>>;
>> +			reg-shift = <0>>;
>> +		};
>> +
>> +		uartb: serial@c00000e8 {
>> +			compatible = "ns16550a";
>> +			reg = <0xc00000e8 0x8>>;
>> +			interrupts = <18>>;
>> +			interrupt-parent = <&vic0>>;
>> +			clock-frequency = <1846153>>;
>> +			reg-shift = <0>>;
>> +		};
>> +
>> +		uartc: serial@c00000f0 {
>> +			compatible = "ns16550a";
>> +			reg = <0xc00000f0 0x8>>;
>> +			interrupts = <19>>;
>> +			interrupt-parent = <&vic0>>;
>> +			clock-frequency = <1846153>>;
>> +			reg-shift = <0>>;
>> +		};
>> +
>> +		usb0: usb@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@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@cefe0100 {
>> +			compatible = "generic-ohci";
>> +			reg = <0xcefe0100 0x110>>;
>> +			interrupts = <6>>;
>> +			interrupt-parent = <&vic0>>;
>> +		};
>> +
>> +		vrom@58000000 {
>> +			compatible = "mtd-ram";
>> +			bank-width = <4>>;
>> +			reg = <0x58000000 0x4000000>>;
>> +			#address-cells = <1>>;
>> +			#size-cells = <1>>;
>> +			partition@0 {
>> +				label = "vrom-prime";
>> +				reg = <0x0 0x2000000>>;
>> +			};
>> +			partition@2000000 {
>> +				label = "vrom-second";
>> +				reg = <0x2000000 0x2000000>>;
>> +			};
>> +		};
>> +
>> +		i2cg: syscon@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