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