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

From: Hawkins, Nick
Date: Wed Mar 16 2022 - 16:11:51 EST

>> -----Original Message-----
>> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@xxxxxxxxxxxxx]
>> Sent: Friday, March 11, 2022 4:30 AM
>> To: Hawkins, Nick <nick.hawkins@xxxxxxx>>>>; Verdun, Jean-Marie
>> <verdun@xxxxxxx>>>>
>> Cc: Arnd Bergmann <arnd@xxxxxxxx>>>>; Olof Johansson <olof@xxxxxxxxx>>>>;
>> soc@xxxxxxxxxx; Rob Herring <robh+dt@xxxxxxxxxx>>>>;
>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
>> linux-kernel@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP
>> Device tree
>>
>> On 10/03/2022 20:52, nick.hawkins@xxxxxxx wrote:
>>>>>> From: Nick Hawkins <nick.hawkins@xxxxxxx>>>>
>>>>>>
>>>>>> 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@xxxxxxx>>>>
>>>>>> ---
>>>>>> 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@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.

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

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