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

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 16/03/2022 16:41, Hawkins, Nick wrote:
> -----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.

Yes, except hpe,gxp goes at the end.

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

(...)

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

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.

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

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.

Best regards,
Krzysztof