Re: [PATCH] ARM: dts: nuvoton: Add Fii Mori system

From: Krzysztof Kozlowski
Date: Wed Jul 26 2023 - 14:56:24 EST

Next message: Dragos Tatulea: "Re: [PATCH 0/2] vdpa: Enable strict validation for netlink ops"
Previous message: Mathieu Desnoyers: "Re: [RFC PATCH 1/1] sched: Extend cpu idle state for 1ms"
In reply to: Charles Boyer: "[PATCH] ARM: dts: nuvoton: Add Fii Mori system"
Next in thread: Konrad Dybcio: "Re: [PATCH] ARM: dts: nuvoton: Add Fii Mori system"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 26/07/2023 20:46, Charles Boyer wrote:
> Add the device tree for Mori BMC, which is an Ampere server platform
> manufactured by Fii. The device tree is based on Nuvoton NPCM730 SoC.
>
> Signed-off-by: Charles Boyer <Charles.Boyer@xxxxxxxxxxx>
> ---
> arch/arm/boot/dts/nuvoton/Makefile | 1 +
> .../boot/dts/nuvoton/nuvoton-npcm730-mori.dts | 1491 +++++++++++++++++

Thank you for your patch. There is something to discuss/improve.

> 2 files changed, 1492 insertions(+)
> create mode 100644 arch/arm/boot/dts/nuvoton/nuvoton-npcm730-mori.dts
>
> diff --git a/arch/arm/boot/dts/nuvoton/Makefile b/arch/arm/boot/dts/nuvoton/Makefile
> index 89c157dad312..ab74184b0283 100644
> --- a/arch/arm/boot/dts/nuvoton/Makefile
> +++ b/arch/arm/boot/dts/nuvoton/Makefile
> @@ -3,6 +3,7 @@ dtb-$(CONFIG_ARCH_NPCM7XX) += \
> nuvoton-npcm730-gsj.dtb \
> nuvoton-npcm730-gbs.dtb \
> nuvoton-npcm730-kudo.dtb \
> + nuvoton-npcm730-mori.dtb \
> nuvoton-npcm750-evb.dtb \
> nuvoton-npcm750-runbmc-olympus.dtb
> dtb-$(CONFIG_ARCH_WPCM450) += \
> diff --git a/arch/arm/boot/dts/nuvoton/nuvoton-npcm730-mori.dts b/arch/arm/boot/dts/nuvoton/nuvoton-npcm730-mori.dts
> new file mode 100644
> index 000000000000..45ef0b29d6ba
> --- /dev/null
> +++ b/arch/arm/boot/dts/nuvoton/nuvoton-npcm730-mori.dts
> @@ -0,0 +1,1491 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2023 Fii USA Inc.
> +
> +/dts-v1/;
> +#include "nuvoton-npcm730.dtsi"
> +
> +#include <dt-bindings/gpio/gpio.h>
> +
> +/ {
> + model = "Fii mori Board";
> + compatible = "fii,mori", "nuvoton,npcm730";

Missing bindings patch (don't forget about running checkpatch).

> +
> + aliases {
> + serial0 = &serial0;

...

> +
> + iio-hwmon {
> + compatible = "iio-hwmon";
> + io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
> + <&adc 4>, <&adc 5>, <&adc 6>, <&adc 7>;
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> + BMC_ALIVE {

Eh... so many things to say...

1. Please base your work on already upstreamed code, so you will not
repeat the same errors we already fixed. It can happen though that this
Nuvoton boards are in terrible shape...

2. Underscores and capital letters are not allowed.

3. It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

> + gpios = <&gpio9 4 0>;

Use appropriate defines for flags.

> + };
> + boot_status_led {

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

> + gpios = <&gpio10 15 0>;

Define

> + };
> + };
> +
> + pinctrl: pinctrl@f0800000 {

Weird placement... but okay...

> +
> + ahb {
> + udc5: udc@f0835000 {
> + compatible = "nuvoton,npcm750-udc";
> + reg = <0xf0835000 0x1000
> + 0xfffd2800 0x800>;

Aren't these two items?

> + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
> + status = "okay";

Are you adding new node or overriding? Why board defines compatible for
SoC component? This is confusing...

> + clocks = <&clk NPCM7XX_CLK_SU>;
> + clock-names = "clk_usb_bridge";
> + };
> + };
> +
> + pcie-slot {
> + pcie0: pcie-slot@0 {
> + label = "slot0";
> + };
> + pcie1: pcie-slot@1 {
> + label = "slot1";
> + };
> + pcie2: pcie-slot@2 {
> + label = "slot2";
> + };
> + pcie3: pcie-slot@3 {
> + label = "slot3";
> + };
> + };
> +};

Missing blank line.

> +&gmac0 {
> + phy-mode = "rgmii-id";
> + snps,eee-force-disable;
> + status = "okay";
> +};
> +
> +&emc0 {
> + phy-mode = "rmii";
> + status = "okay";
> + fixed-link {
> + speed = <100>;
> + full-duplex;
> + };
> +};
> +
> +&ehci1 {
> + status = "okay";
> +};
> +
> +&ohci1 {
> + status = "okay";
> +};
> +
> +&aes {
> + status = "okay";
> +};
> +
> +&sha {
> + status = "okay";
> +};
> +
> +&spi1 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&gpio177_pins &gpio176o_pins
> + &gpio175ol_pins>;
> + status = "okay";
> + jtag_master {

Nope, nope. Don't use reviewers as tools. Use tools for this:

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

`make dtbs W=1`

> + compatible = "nuvoton,npcm750-jtag-master";
> + spi-max-frequency = <25000000>;
> + reg = <0>;
> +
> + pinctrl-names = "pspi", "gpio";
> + pinctrl-0 = <&pspi1_pins>;
> + pinctrl-1 = <&gpio177_pins &gpio176o_pins
> + &gpio175ol_pins>;
> +
> + tck-gpios = <&gpio5 15 GPIO_ACTIVE_HIGH>;
> + tdi-gpios = <&gpio5 16 GPIO_ACTIVE_HIGH>;
> + tdo-gpios = <&gpio5 17 GPIO_ACTIVE_HIGH>;
> + tms-gpios = <&gpio6 11 GPIO_ACTIVE_HIGH>;
> + status = "okay";

okay is by default, why do you need it?

> + };
> +};
> +
> +&fiu0 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&spi0cs1_pins>;
> + status = "okay";
> + spi-nor@0 {
> + compatible = "jedec,spi-nor";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0>;
> + spi-max-frequency = <5000000>;
> + spi-rx-bus-width = <2>;
> + label = "bmc";
> + partitions@80000000 {
> + compatible = "fixed-partitions";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + u-boot@0 {
> + label = "u-boot";
> + reg = <0x0000000 0xC0000>;
> + read-only;
> + };
> + image-descriptor@f0000 {
> + label = "image-descriptor";
> + reg = <0xf0000 0x10000>;
> + };
> + hoth-update@100000 {
> + label = "hoth-update";
> + reg = <0x100000 0x100000>;
> + };
> + kernel@200000 {
> + label = "kernel";
> + reg = <0x200000 0x500000>;
> + };
> + rofs@700000 {
> + label = "rofs";
> + reg = <0x700000 0x35f0000>;
> + };
> + rwfs@3cf0000 {
> + label = "rwfs";
> + reg = <0x3cf0000 0x300000>;
> + };
> + hoth-mailbox@3ff0000 {
> + label = "hoth-mailbox";
> + reg = <0x3ff0000 0x10000>;
> + };
> + };
> + };
> +};
> +
> +&fiu3 {
> + pinctrl-0 = <&spi3_pins>;
> + status = "okay";
> +
> + spi-nor@0 {
> + compatible = "jedec,spi-nor";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + spi-rx-bus-width = <2>;
> + reg = <0>;
> + label = "hnor";
> + };
> +};
> +
> +// emmc
> +&sdhci0 {
> + status = "okay";
> +};
> +// USB
> +&ehci1 {
> + status = "okay";
> +};
> +
> +&ohci1 {
> + status = "okay";
> +};
> +
> +&vdma {
> + status = "okay";
> +};
> +
> +&pcimbox {
> + status = "okay";
> +};
> +
> +&vcd {
> + status = "okay";
> +};
> +
> +&ece {
> + status = "okay";
> +};
> +
> +&watchdog1 {
> + status = "okay";
> +};
> +
> +&rng {
> + status = "okay";
> +};
> +
> +&serial0 {
> + status = "okay";
> +};
> +
> +&serial1 {
> + status = "okay";
> +};
> +
> +&serial2 {
> + status = "okay";
> +};
> +
> +&serial3 {
> + status = "okay";
> +};
> +
> +&adc {
> + #io-channel-cells = <1>;
> + status = "okay";
> +};
> +
> +&otp {
> + status = "okay";
> +};
> +
> +&i2c0 {
> + status = "okay";
> + clock-frequency = <400000>;
> +};
> +
> +&i2c1 {
> + status = "okay";
> + clock-frequency = <400000>;
> + i2c-switch@75 {
> + compatible = "nxp,pca9548";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x75>;
> + i2c-mux-idle-disconnect;
> +
> + i2c16: i2c@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
> + };
> + i2c17: i2c@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> + };
> + i2c18: i2c@2 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <2>;
> + };
> + i2c19: i2c@3 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <3>;
> +
> + cpu@50 {

cpu? Is it really a CPU on I2C bus?

> + compatible = "atmel,24c64";
> + reg = <0x50>;
> + };
> + };
> + i2c20: i2c@4 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <4>;
> + };
> + i2c21: i2c@5 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <5>;
> + };
> + i2c22: i2c@6 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <6>;
> + };
> + i2c23: i2c@7 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <7>;
> + };
> + };

Here and in all other places - missing blank line after node.

> + i2c-switch@77 {
> + compatible = "nxp,pca9548";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x77>;
> + i2c-mux-idle-disconnect;
> +
> + i2c24: i2c@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
> + };
> + i2c25: i2c@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> +
> + adm1272@1f {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> + compatible = "adi,adm1272";
> + reg = <0x1f>;
> + shunt-resistor-micro-ohms = <333>;
> + };
> +
> + };
> + i2c26: i2c@2 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <2>;
> + };
> + i2c27: i2c@3 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <3>;
> + };
> + i2c28: i2c@4 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <4>;
> + };
> + i2c29: i2c@5 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <5>;
> + };
> + i2c30: i2c@6 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <6>;
> + };
> + i2c31: i2c@7 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <7>;
> + };
> + };
> +};
> +
> +&i2c2 {
> + status = "okay";
> + clock-frequency = <400000>;
> + i2c-switch@77 {
> + compatible = "nxp,pca9548";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x77>;
> + i2c-mux-idle-disconnect;
> +
> + i2c32: i2c@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
> + };
> + i2c33: i2c@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> + };
> + i2c34: i2c@2 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <2>;
> + };
> + i2c35: i2c@3 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <3>;
> +
> + //REAR FAN

Fix the comments to match Linux coding style.

> + max31790@2c {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> + compatible = "maxim,max31790";
> + reg = <0x2c>;
> + };
> + };
> + i2c36: i2c@4 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <4>;
> + };
> + i2c37: i2c@5 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <5>;
> +
> + //OUTLET1_T
> + lm75@5c {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> + compatible = "ti,lm75";
> + reg = <0x5c>;
> + };
> + };
> + i2c38: i2c@6 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <6>;
> +
> + //INLET1_T
> + lm75@5c {
> + compatible = "ti,lm75";
> + reg = <0x5c>;
> + };
> + };
> + i2c39: i2c@7 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <7>;
> + };
> + };
> +};
> +
> +&i2c3 {
> + status = "okay";
> + clock-frequency = <400000>;
> + pcie-slot = &pcie0;
> +};
> +
> +&i2c4 {
> + status = "okay";
> + clock-frequency = <400000>;
> + mbfru@50 {
> + compatible = "atmel,24c64";
> + reg = <0x50>;
> + };
> +
> + i2c-switch@77 {
> + compatible = "nxp,pca9548";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x77>;
> + i2c-mux-idle-disconnect;
> +
> + i2c40: i2c@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
> +
> + //Power Sequencer
> + adm1266@40 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> + compatible = "adi,adm1266";
> + reg = <0x40>;
> + };
> + };
> + i2c41: i2c@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> + };
> + i2c42: i2c@2 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <2>;
> +
> + //SKM EEPROM
> + skm@55 {

Come on... eeprom

> + compatible = "atmel,24c64";
> + reg = <0x55>;
> + pagesize = <0x20>;
> + };
> + };
> + i2c43: i2c@3 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <3>;
> +
> + gpio10: pca6416@20 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> + compatible = "nxp,pca6416";
> + reg = <0x20>;
> + pinctrl-names = "default";
> + gpio-controller;
> + #gpio-cells = <2>;
> + gpio-line-names =
> + "P12V_PCIE_PE0_ON", "P12V_PCIE_PE1_ON", "P12V_PCIE_PE2_ON", "P12V_PCIE_PE3_ON", "P13V5_NBM_S0_PGD",
> + "P12V_EFUSE_ABCD_EN", "P12V_EFUSE_EFGH_EN", "NC_P0_7", "MB_JTAG_MUX_SEL", "JTAG_SCM_MUX_OE_N",
> + "FM_BMC_IO_CPLD_PROGRAM_N", "FM_BMC_IO_CPLD_HITLESS_N", "FM_IOEXP_DRAM_P12V_EN", "MB_CPLD_INIT",
> + "NCSI_CX_PRSNT_N", "SYS_BOOT_STATUS_LED";
> + };
> + };
> + i2c44: i2c@4 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <4>;
> +
> + gpio12: pca6416@21 {

Enough. This DTS should have basic things first cleaned up.

Best regards,
Krzysztof

Next message: Dragos Tatulea: "Re: [PATCH 0/2] vdpa: Enable strict validation for netlink ops"
Previous message: Mathieu Desnoyers: "Re: [RFC PATCH 1/1] sched: Extend cpu idle state for 1ms"
In reply to: Charles Boyer: "[PATCH] ARM: dts: nuvoton: Add Fii Mori system"
Next in thread: Konrad Dybcio: "Re: [PATCH] ARM: dts: nuvoton: Add Fii Mori system"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]