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