[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.

> Yes, except hpe,gxp goes at the end.

Done

>> 
>>>>>> +	model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
>>>>>> +

(...)

>>>>>> +
>>>>>> +		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?

> Yes,, see other cases in generic-ehci.yaml bindings. Your current choice would be pointed out by dtbs_check, that it's invalid according to current bindings.

For some reason when I compile I am not seeing a warning for that file. I have been using "make dtbs_check" and "make dtbs W=1". Perhaps I am missing an important flag?

In the case of creating a hpe,gxp-ehci binding would I need to add that to the generic-ehci.yaml?


>> 
>>>>>> +			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.

> You should rather have a clock controller driver which defines this (and many others). They can stay as a temporary work-around, if you really need them for some other nodes.

I am trying to picture what you are saying but I am unsure, I know that on a separate review you mentioned that the gxp-timer needed to have clocks, and clock-names inside the node. Would it be improper for the gxp-timer to reference iopclk?

Thanks!

-Nick Hawkins