| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 16 Feb 2022 16:29:38 +0000
From: "Hawkins, Nick" <nick.hawkins@....com>
To: Arnd Bergmann <arnd@...db.de>
CC: "Verdun, Jean-Marie" <verdun@....com>,
Olof Johansson <olof@...om.net>, SoC Team <soc@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
DTML <devicetree@...r.kernel.org>
Subject: RE: [PATCH] [v3] arch: arm: boot: dts: Create HPE GXP Device Tree
> +GXP ARM ARCHITECTURE
> +M: Nick Hawkins <nick.hawkins@....com>
> +M: Jean-Marie <verdun@....com>
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@...00090 {
> + 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@...00000 {
> + 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:arnd@...db.de]
Sent: Wednesday, February 16, 2022 10:01 AM
To: Hawkins, Nick <nick.hawkins@....com>
Cc: Verdun, Jean-Marie <verdun@....com>; Arnd Bergmann <arnd@...db.de>; Olof Johansson <olof@...om.net>; SoC Team <soc@...nel.org>; Rob Herring <robh+dt@...nel.org>; Linux Kernel Mailing List <linux-kernel@...r.kernel.org>; Linux ARM <linux-arm-kernel@...ts.infradead.org>; DTML <devicetree@...r.kernel.org>
Subject: Re: [PATCH] [v3] arch: arm: boot: dts: Create HPE GXP Device Tree
On Wed, Feb 16, 2022 at 4:36 PM <nick.hawkins@....com> wrote:
>
> From: Nick Hawkins <nick.hawkins@....com>
>
> 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 <nick.hawkins@....com>
> +M: Jean-Marie <verdun@....com>
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@...00080 {
> + compatible = "hpe,gxp-timer";
> + reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;
> + interrupts = <0>;
> + interrupt-parent = <&vic0>;
> + clock-frequency = <400000000>;
> + };
> +
> + watchdog: watchdog@...00090 {
> + 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@...000f0 {
> + compatible = "ns16550a";
> + reg = <0xc00000f0 0x8>;
> + interrupts = <19>;
> + interrupt-parent = <&vic0>;
> + clock-frequency = <1846153>;
> + reg-shift = <0>;
> + };
> +
> + usb0: ehci@...e0000 {
> + 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@...00000 {
> + 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@...00 {
> + 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@...00000 {
> + compatible = "mtd-ram";
Same thing here, "vrom" is clearly not a standard name.
Arnd
Powered by blists - more mailing lists