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 <redacted>>; 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:

quoted

quoted

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

quoted

Alphabetically, also in respect to other architectures, so before CONFIG_ARCH_INTEGRATOR.

Done

quoted

quoted

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

quoted

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.

quoted

quoted

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

(...)

quoted

quoted

+
+		usb0: usb@cefe0000 {
+			compatible = "generic-ehci";

quoted

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.

quoted

quoted

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

quoted

quoted

+			compatible = "simple-mfd", "syscon";
+			reg = <0xc00000f8 0x08>>;
+		};
+	};
+
+	clocks {
+		osc: osc {

quoted

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.

quoted

quoted

+			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>>;
+		};

quoted

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