From: Nick Hawkins <[email protected]>
Description: Originally this was of a larger patch
HPE BMC GXP SUPPORT that was rejected for being to big.
This is why the label v3 is being used.
This patch Create a basic device tree layout that
will allow the HPE GXP to boot. The undocumented
bindings hpe,gxp-wdt and hpe,gxp-timer are being
documented in patches:
[v1] dt-bindings: timer: Add HPE GXP Timer binding
[v1] dt-bindings: watchdog: Add HPE GXP Watchdog timer binding
[v1]dt-bindings: vendor-prefixes: add HPE Prefix
that are currently out for review.
The dts file is largely empty for this initial patch but
follow up patches will add more content.
Information: GXP is the name of the HPE SoC.
This SoC is used to implement BMC features of HPE servers
(all ProLiant, Synergy, and many Apollo, and Superdome machines)
It does support many features including:
ARMv7 architecture, and it is based on a Cortex A9 core
Use an AXI bus to which
a memory controller is attached, as well as
multiple SPI interfaces to connect boot flash,
and ROM flash, a 10/100/1000 Mac engine which
supports SGMII (2 ports) and RMII
Multiple I2C engines to drive connectivity with a
host infrastructure
A video engine which support VGA and DP, as well as
an hardware video encoder
Multiple PCIe ports
A PECI interface, and LPC eSPI
Multiple UART for debug purpose, and Virtual UART for
host connectivity
A GPIO engine.
Signed-off-by: Nick Hawkins <[email protected]>
---
MAINTAINERS | 7 +
arch/arm/boot/dts/Makefile | 2 +
arch/arm/boot/dts/hpe-bmc-dl360gen10.dts | 27 ++++
arch/arm/boot/dts/hpe-gxp.dtsi | 169 +++++++++++++++++++++++
4 files changed, 205 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/MAINTAINERS b/MAINTAINERS
index f41088418aae..ca9fcc611b1b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8385,6 +8385,13 @@ L: [email protected]
S: Maintained
F: block/partitions/efi.*
+GXP ARM ARCHITECTURE
+M: Nick Hawkins <[email protected]>
+M: Jean-Marie <[email protected]>
+S: Maintained
+F: arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
+F: arch/arm/boot/dts/hpe-gxp.dtsi
+
H8/300 ARCHITECTURE
M: Yoshinori Sato <[email protected]>
L: [email protected] (moderated for non-subscribers)
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 235ad559acb2..a96b4d5b7f68 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -1549,3 +1549,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
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..179de6945e3f
--- /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";
+ model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
+
+ chosen {
+ bootargs = "earlyprintk console=ttyS0,115200 user_debug=31";
+ };
+
+ memory@40000000 {
+ device_type = "memory";
+ reg = <0x40000000 0x20000000>;
+ };
+
+ ahb {
+
+ };
+
+};
diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi b/arch/arm/boot/dts/hpe-gxp.dtsi
new file mode 100644
index 000000000000..f62109abf685
--- /dev/null
+++ b/arch/arm/boot/dts/hpe-gxp.dtsi
@@ -0,0 +1,169 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree file for HPE GXP
+ */
+
+/dts-v1/;
+/ {
+ model = "Hewlett Packard Enterprise GXP BMC";
+ compatible = "hpe,gxp";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ cpus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ cpu@0 {
+ compatible = "arm,armv7";
+ device_type = "cpu";
+ reg = <0>;
+ };
+ };
+
+ memory@40000000 {
+ device_type = "memory";
+ reg = <0x40000000 0x20000000>;
+ };
+
+ ahb {
+ compatible = "simple-bus";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ device_type = "soc";
+ ranges;
+
+ vic0: interrupt-controller@ceff0000 {
+ compatible = "arm,pl192-vic";
+ #address-cells = <1>;
+ interrupt-controller;
+ reg = <0xceff0000 0x1000>;
+ #interrupt-cells = <1>;
+ };
+
+ vic1: vic@80f00000 {
+ compatible = "arm,pl192-vic";
+ #address-cells = <1>;
+ interrupt-controller;
+ reg = <0x80f00000 0x1000>;
+ #interrupt-cells = <1>;
+ };
+
+ timer0: timer@c0000080 {
+ compatible = "hpe,gxp-timer";
+ reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;
+ interrupts = <0>;
+ interrupt-parent = <&vic0>;
+ clock-frequency = <400000000>;
+ };
+
+ watchdog: watchdog@c0000090 {
+ compatible = "hpe,gxp-wdt";
+ reg = <0xc0000090 0x02>, <0xc0000096 0x01>;
+ };
+
+ uarta: serial@c00000e0 {
+ compatible = "ns16550a";
+ reg = <0xc00000e0 0x8>;
+ interrupts = <17>;
+ interrupt-parent = <&vic0>;
+ clock-frequency = <1846153>;
+ reg-shift = <0>;
+ };
+
+ uartb: serial@c00000e8 {
+ compatible = "ns16550a";
+ reg = <0xc00000e8 0x8>;
+ interrupts = <18>;
+ interrupt-parent = <&vic0>;
+ clock-frequency = <1846153>;
+ reg-shift = <0>;
+ };
+
+ uartc: serial@c00000f0 {
+ compatible = "ns16550a";
+ reg = <0xc00000f0 0x8>;
+ interrupts = <19>;
+ interrupt-parent = <&vic0>;
+ clock-frequency = <1846153>;
+ reg-shift = <0>;
+ };
+
+ usb0: ehci@cefe0000 {
+ compatible = "generic-ehci";
+ reg = <0xcefe0000 0x100>;
+ interrupts = <7>;
+ interrupt-parent = <&vic0>;
+ };
+
+ usb1: ohci@cefe0100 {
+ compatible = "generic-ohci";
+ reg = <0xcefe0100 0x110>;
+ interrupts = <6>;
+ interrupt-parent = <&vic0>;
+ };
+
+ sram@d0000000 {
+ compatible = "mtd-ram";
+ reg = <0xd0000000 0x80000>;
+ bank-width = <1>;
+ erase-size =<1>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ partition@0 {
+ label = "host-reserved";
+ reg = <0x0 0x10000>;
+ };
+ partition@10000 {
+ label = "nvram";
+ reg = <0x10000 0x70000>;
+ };
+ };
+
+ 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: i2cg@c00000f8 {
+ compatible = "syscon";
+ reg = <0xc00000f8 0x08>;
+ };
+ };
+
+ clocks {
+ osc: osc {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-output-names = "osc";
+ clock-frequency = <33333333>;
+ };
+
+ iopclk: iopclk {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clocks = <&osc>;
+ clock-out-put-names = "iopclk";
+ clock-frequency = <400000000>;
+ };
+
+ memclk: memclk {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clocks = <&osc>;
+ clock-out-put-names = "memclk";
+ clock-frequency = <800000000>;
+ };
+ };
+};
--
2.17.1
> +GXP ARM ARCHITECTURE
> +M: Nick Hawkins <[email protected]>
> +M: Jean-Marie <[email protected]>
It looks like you are missing the family name here.
NH: Do you mean I need to have an HPE in front of it?
...
> +S: Maintained
> +F: arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> +F: arch/arm/boot/dts/hpe-gxp.dtsi
> +
I would make a single entry for the platform with all the drivers here. Please keep the MAINTAINERS file patch separate from the devicetree patch though.
NH: I will remove maintainers from this, the checkpatch warned me about adding files and not modifying Maintainers.
...
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 235ad559acb2..a96b4d5b7f68 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1549,3 +1549,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
This Kconfig symbol does not yet exist
NH: Should I include the definition of this Kconfig symbol in this patch or just not modify the makefile when doing device tree changes?
...
> +
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "hpe,gxp";
No board specific compatible string? Also, this value is not documented anywhere.
NH: I was uncertain if this needed to be documented or not, checkpatch did not notice it. Where would you recommend I should put this in documentation? I was considering Documentation/devicetree/bindings/soc/hpe/
...
> +
> + watchdog: watchdog@c0000090 {
> + compatible = "hpe,gxp-wdt";
> + reg = <0xc0000090 0x02>, <0xc0000096 0x01>;
> + };
As mentioned in a previous review, it would be helpful to have the drivers and bindings together in the same series for easier review.
NH: Just to confirm, I need the driver source and the new DTS file in one review? I also need to have a review to add files: arch/arm/configs/gxp_defconfig, arch/arm/mach-hpe/gxp.c should those be included as well? I am trying to not create too massive of a patch. I could also create completely separate patches for these items so that reviewers could refer to it.
...
To a lesser degree, the same is true for the uart.
Please check the devicetree files in order to validate the bindings. In this case, you should get a warning about the 'ehci@' name being non-standard. The normal name is usb@
NH: Which tool are you using to validate bindings? I did not receive any warnings but I did receive several errors which I corrected.
> +
> + vrom@58000000 {
> + compatible = "mtd-ram";
Same thing here, "vrom" is clearly not a standard name.
NH: VROM is a Virtual ROM device that we use to create a memory mapping of a SPI flash content copy that is served to a host system. How should I rename it?
Thanks for all the feedback!
-Nick
-----Original Message-----
From: Arnd Bergmann [mailto:[email protected]]
Sent: Wednesday, February 16, 2022 10:01 AM
To: Hawkins, Nick <[email protected]>
Cc: Verdun, Jean-Marie <[email protected]>; Arnd Bergmann <[email protected]>; Olof Johansson <[email protected]>; SoC Team <[email protected]>; Rob Herring <[email protected]>; Linux Kernel Mailing List <[email protected]>; Linux ARM <[email protected]>; DTML <[email protected]>
Subject: Re: [PATCH] [v3] arch: arm: boot: dts: Create HPE GXP Device Tree
On Wed, Feb 16, 2022 at 4:36 PM <[email protected]> wrote:
>
> From: Nick Hawkins <[email protected]>
>
> Description: Originally this was of a larger patch HPE BMC GXP SUPPORT
> that was rejected for being to big.
> This is why the label v3 is being used.
> This patch Create a basic device tree layout that will allow the HPE
> GXP to boot. The undocumented bindings hpe,gxp-wdt and hpe,gxp-timer
> are being documented in patches:
> [v1] dt-bindings: timer: Add HPE GXP Timer binding
> [v1] dt-bindings: watchdog: Add HPE GXP Watchdog timer binding
> [v1]dt-bindings: vendor-prefixes: add HPE Prefix that are currently
> out for review.
> The dts file is largely empty for this initial patch but follow up
> patches will add more content.
>
> Information: GXP is the name of the HPE SoC.
> This SoC is used to implement BMC features of HPE servers
> (all ProLiant, Synergy, and many Apollo, and Superdome machines)
> It does support many features including:
> ARMv7 architecture, and it is based on a Cortex A9 core
> Use an AXI bus to which
> a memory controller is attached, as well as
> multiple SPI interfaces to connect boot flash,
> and ROM flash, a 10/100/1000 Mac engine which
> supports SGMII (2 ports) and RMII
> Multiple I2C engines to drive connectivity with a
> host infrastructure
> A video engine which support VGA and DP, as well as
> an hardware video encoder
> Multiple PCIe ports
> A PECI interface, and LPC eSPI
> Multiple UART for debug purpose, and Virtual UART for
> host connectivity
> A GPIO engine.
Something happened to the linewrapping here.
>
> +GXP ARM ARCHITECTURE
> +M: Nick Hawkins <[email protected]>
> +M: Jean-Marie <[email protected]>
It looks like you are missing the family name here.
> +S: Maintained
> +F: arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> +F: arch/arm/boot/dts/hpe-gxp.dtsi
> +
I would make a single entry for the platform with all the drivers here. Please keep the MAINTAINERS file patch separate from the devicetree patch though.
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 235ad559acb2..a96b4d5b7f68 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1549,3 +1549,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
This Kconfig symbol does not yet exist
> 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..179de6945e3f
> --- /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";
No board specific compatible string? Also, this value is not documented anywhere.
> + model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
> + bootargs = "earlyprintk console=ttyS0,115200
> + user_debug=31";
'earlyprintk' and 'user_debug' should not go into the .dts, set these from the boot loader when you are debugging.
You probably want to add some aliases, to assign fixed numbers to the uart and mmc controller among other things.
> + timer0: timer@c0000080 {
> + compatible = "hpe,gxp-timer";
> + reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;
> + interrupts = <0>;
> + interrupt-parent = <&vic0>;
> + clock-frequency = <400000000>;
> + };
> +
> + watchdog: watchdog@c0000090 {
> + compatible = "hpe,gxp-wdt";
> + reg = <0xc0000090 0x02>, <0xc0000096 0x01>;
> + };
As mentioned in a previous review, it would be helpful to have the drivers and bindings together in the same series for easier review.
> + uartc: serial@c00000f0 {
> + compatible = "ns16550a";
> + reg = <0xc00000f0 0x8>;
> + interrupts = <19>;
> + interrupt-parent = <&vic0>;
> + clock-frequency = <1846153>;
> + reg-shift = <0>;
> + };
> +
> + usb0: ehci@cefe0000 {
> + compatible = "generic-ehci";
> + reg = <0xcefe0000 0x100>;
> + interrupts = <7>;
> + interrupt-parent = <&vic0>;
> + };
The ehci is almost never completely generic, I would recommend defining a SoC specific compatible string as well, so you can add quirks later if you find a problem. Having the generic string as a fallback is a good idea though, as that means you can just use the normal driver as long as there are no special requirements.
To a lesser degree, the same is true for the uart.
Please check the devicetree files in order to validate the bindings. In this case, you should get a warning about the 'ehci@' name being non-standard. The normal name is usb@
> + sram@d0000000 {
> + compatible = "mtd-ram";
> + reg = <0xd0000000 0x80000>;
> + bank-width = <1>;
> + erase-size =<1>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + partition@0 {
> + label = "host-reserved";
> + reg = <0x0 0x10000>;
> + };
> + partition@10000 {
> + label = "nvram";
> + reg = <0x10000 0x70000>;
> + };
> + };
What device is this exactly? The name indicates an sram, which would use the compatible="mmio-sram" binding instead of "mtd-ram", but then the partition has an "nvram" label that indicates that this is an nvmem type device. "mtd-ram" is probably not what you want though.
> +
> + vrom@58000000 {
> + compatible = "mtd-ram";
Same thing here, "vrom" is clearly not a standard name.
Arnd
On Wed, Feb 16, 2022 at 5:29 PM Hawkins, Nick <[email protected]> wrote:
> > +GXP ARM ARCHITECTURE
> > +M: Nick Hawkins <[email protected]>
> > +M: Jean-Marie <[email protected]>
>
> It looks like you are missing the family name here.
>
> NH: Do you mean I need to have an HPE in front of it?
No, I meant a 'Verdun' after Jean-Marie ;-)
> ...
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index 235ad559acb2..a96b4d5b7f68 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -1549,3 +1549,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
>
> This Kconfig symbol does not yet exist
>
> NH: Should I include the definition of this Kconfig symbol in this patch or just not modify the makefile when doing device tree changes?
I would start with a patch for the bindings, or one per binding, then
the Kconfig
patch, then the dts file and finally the MAINTAINERS entry.
The order is not super important, but it helps to do it in a way that one patch
builds on the previous one in the series, rather than having a single big patch,
or separate small patches that are not in a series.
>
> > +
> > +/ {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + compatible = "hpe,gxp";
>
> No board specific compatible string? Also, this value is not documented anywhere.
>
> NH: I was uncertain if this needed to be documented or not, checkpatch did not notice it. Where would you recommend I should put this in documentation? I was considering Documentation/devicetree/bindings/soc/hpe/
Documentation/devicetree/bindings/arm/hpe.yaml would be best.
The bindings/soc/ directory is not for the SoC as a whole but more
for components on the SoC that are either for identifying the chip
(as in struct soc_device in Linux) or for things that don't fit anywhere
else but are specific to one soc.
> ...
>
> > +
> > + watchdog: watchdog@c0000090 {
> > + compatible = "hpe,gxp-wdt";
> > + reg = <0xc0000090 0x02>, <0xc0000096 0x01>;
> > + };
>
> As mentioned in a previous review, it would be helpful to have the drivers and bindings together in the same series for easier review.
>
> NH: Just to confirm, I need the driver source and the new DTS file in one review? I also need to have a review to add files: arch/arm/configs/gxp_defconfig, arch/arm/mach-hpe/gxp.c should those be included as well? I am trying to not create too massive of a patch. I could also create completely separate patches for these items so that reviewers could refer to it.
Yes, I think it's best to have all patches in a larger series for the
initial review. The defconfig
patch is usually separate, but the gxp.c file can be in the same patch
as maintainers entry
and the Kconfig and Makefile changes (outside of dts).
Try to keep each patch in the series self-contained, e.g. don't
separate a .c file from the Makefile
and Kconfig change, but don't group things into one patch that are not
directly related, e.g.
the dts file.
> ...
>
> To a lesser degree, the same is true for the uart.
>
> Please check the devicetree files in order to validate the bindings. In this case, you should get a warning about the 'ehci@' name being non-standard. The normal name is usb@
>
> NH: Which tool are you using to validate bindings? I did not receive any warnings but I did receive several errors which I corrected.
You can run "make dtbs W=1" to get most of the useful checks enabled.
> > +
> > + vrom@58000000 {
> > + compatible = "mtd-ram";
>
> Same thing here, "vrom" is clearly not a standard name.
>
> NH: VROM is a Virtual ROM device that we use to create a memory mapping of a SPI flash content copy that is served to a host system. How should I rename it?
Not sure, I don't remember seeing this case so far. Adding Joel to Cc,
maybe he knows of
another BMC that does the same thing.
Arnd
On Wed, Feb 16, 2022 at 4:36 PM <[email protected]> wrote:
>
> From: Nick Hawkins <[email protected]>
>
> Description: Originally this was of a larger patch
> HPE BMC GXP SUPPORT that was rejected for being to big.
> This is why the label v3 is being used.
> This patch Create a basic device tree layout that
> will allow the HPE GXP to boot. The undocumented
> bindings hpe,gxp-wdt and hpe,gxp-timer are being
> documented in patches:
> [v1] dt-bindings: timer: Add HPE GXP Timer binding
> [v1] dt-bindings: watchdog: Add HPE GXP Watchdog timer binding
> [v1]dt-bindings: vendor-prefixes: add HPE Prefix
> that are currently out for review.
> The dts file is largely empty for this initial patch but
> follow up patches will add more content.
>
> Information: GXP is the name of the HPE SoC.
> This SoC is used to implement BMC features of HPE servers
> (all ProLiant, Synergy, and many Apollo, and Superdome machines)
> It does support many features including:
> ARMv7 architecture, and it is based on a Cortex A9 core
> Use an AXI bus to which
> a memory controller is attached, as well as
> multiple SPI interfaces to connect boot flash,
> and ROM flash, a 10/100/1000 Mac engine which
> supports SGMII (2 ports) and RMII
> Multiple I2C engines to drive connectivity with a
> host infrastructure
> A video engine which support VGA and DP, as well as
> an hardware video encoder
> Multiple PCIe ports
> A PECI interface, and LPC eSPI
> Multiple UART for debug purpose, and Virtual UART for
> host connectivity
> A GPIO engine.
Something happened to the linewrapping here.
>
> +GXP ARM ARCHITECTURE
> +M: Nick Hawkins <[email protected]>
> +M: Jean-Marie <[email protected]>
It looks like you are missing the family name here.
> +S: Maintained
> +F: arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> +F: arch/arm/boot/dts/hpe-gxp.dtsi
> +
I would make a single entry for the platform with all the drivers here. Please
keep the MAINTAINERS file patch separate from the devicetree patch though.
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 235ad559acb2..a96b4d5b7f68 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1549,3 +1549,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
This Kconfig symbol does not yet exist
> 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..179de6945e3f
> --- /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";
No board specific compatible string? Also, this value is not
documented anywhere.
> + model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
> + bootargs = "earlyprintk console=ttyS0,115200 user_debug=31";
'earlyprintk' and 'user_debug' should not go into the .dts, set these from
the boot loader when you are debugging.
You probably want to add some aliases, to assign fixed numbers to
the uart and mmc controller among other things.
> + timer0: timer@c0000080 {
> + compatible = "hpe,gxp-timer";
> + reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;
> + interrupts = <0>;
> + interrupt-parent = <&vic0>;
> + clock-frequency = <400000000>;
> + };
> +
> + watchdog: watchdog@c0000090 {
> + compatible = "hpe,gxp-wdt";
> + reg = <0xc0000090 0x02>, <0xc0000096 0x01>;
> + };
As mentioned in a previous review, it would be helpful to have the drivers
and bindings together in the same series for easier review.
> + uartc: serial@c00000f0 {
> + compatible = "ns16550a";
> + reg = <0xc00000f0 0x8>;
> + interrupts = <19>;
> + interrupt-parent = <&vic0>;
> + clock-frequency = <1846153>;
> + reg-shift = <0>;
> + };
> +
> + usb0: ehci@cefe0000 {
> + compatible = "generic-ehci";
> + reg = <0xcefe0000 0x100>;
> + interrupts = <7>;
> + interrupt-parent = <&vic0>;
> + };
The ehci is almost never completely generic, I would recommend defining a
SoC specific compatible string as well, so you can add quirks later if you
find a problem. Having the generic string as a fallback is a good idea though,
as that means you can just use the normal driver as long as there are no
special requirements.
To a lesser degree, the same is true for the uart.
Please check the devicetree files in order to validate the bindings. In
this case, you should get a warning about the 'ehci@' name being
non-standard. The normal name is usb@
> + sram@d0000000 {
> + compatible = "mtd-ram";
> + reg = <0xd0000000 0x80000>;
> + bank-width = <1>;
> + erase-size =<1>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + partition@0 {
> + label = "host-reserved";
> + reg = <0x0 0x10000>;
> + };
> + partition@10000 {
> + label = "nvram";
> + reg = <0x10000 0x70000>;
> + };
> + };
What device is this exactly? The name indicates an sram, which would
use the compatible="mmio-sram" binding instead of "mtd-ram", but then
the partition has an "nvram" label that indicates that this is an nvmem
type device. "mtd-ram" is probably not what you want though.
> +
> + vrom@58000000 {
> + compatible = "mtd-ram";
Same thing here, "vrom" is clearly not a standard name.
Arnd
On Wed, 16 Feb 2022 at 16:51, Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Feb 16, 2022 at 5:29 PM Hawkins, Nick <[email protected]> wrote:
> > > +
> > > + vrom@58000000 {
> > > + compatible = "mtd-ram";
> >
> > Same thing here, "vrom" is clearly not a standard name.
> >
> > NH: VROM is a Virtual ROM device that we use to create a memory mapping of a SPI flash content copy that is served to a host system. How should I rename it?
>
> Not sure, I don't remember seeing this case so far. Adding Joel to Cc,
> maybe he knows of
> another BMC that does the same thing.
We have something similar. The aspeed systems map this over LPC, using
FW cycles (aka memory accesses) over the LPC bus to read from the
BMC's SPI NOR devices, or DRAM.
The device tree bindings we have are in the LPC document:
Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml
It's the aspeed,ast2400-lpc-ctrl specifically. It takes a phandle to a
reserved-memory node.
The kernel driver is at drivers/soc/aspeed/aspeed-lpc-ctrl.c. It
exposes a character device that allows userspace to read and write to
the memory window.
We have this very aspeed specific design because it was the first
implementation in the kernel. Where possible, we should create a new
unified API for doing this type of access, where each BMC can
implement a backend. Now that we have GXP, Nuvoton and ASPEED it
should be easier to pick out common functionality.
Cheers,
Joel