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

Krzysztof Kozlowski
2022-03-16T15:51:01Z

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