Re: [PATCH v1 2/2] ARM64: dts: nuvoton: Add initial yosemitev4 device tree

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 12/01/2024 02:36, Delphine CC Chiu wrote:
> Add linux device tree entry related to
> Yosemite 4 specific devices connected to BMC SoC.
> 

Prefix is arm64, not ARM64.

> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>
> ---
>  arch/arm64/boot/dts/nuvoton/Makefile          |    1 +
>  .../dts/nuvoton/nuvoton-npcm845-yosemite4.dts | 1493 +++++++++++++++++
>  2 files changed, 1494 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-yosemite4.dts
> 
> diff --git a/arch/arm64/boot/dts/nuvoton/Makefile b/arch/arm64/boot/dts/nuvoton/Makefile
> index 3bc9787801a5..2b3c03083dc0 100644
> --- a/arch/arm64/boot/dts/nuvoton/Makefile
> +++ b/arch/arm64/boot/dts/nuvoton/Makefile
> @@ -2,3 +2,4 @@
>  dtb-$(CONFIG_ARCH_MA35) += ma35d1-iot-512m.dtb
>  dtb-$(CONFIG_ARCH_MA35) += ma35d1-som-256m.dtb
>  dtb-$(CONFIG_ARCH_NPCM) += nuvoton-npcm845-evb.dtb
> +dtb-$(CONFIG_ARCH_NPCM) += nuvoton-npcm845-yosemite4.dtb
> diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-yosemite4.dts b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-yosemite4.dts
> new file mode 100644
> index 000000000000..f6a6a47b1397
> --- /dev/null
> +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-yosemite4.dts
> @@ -0,0 +1,1493 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright 2023 Facebook Inc.
> +
> +/dts-v1/;
> +#include "nuvoton-npcm845.dtsi"
> +#include "nuvoton-npcm845-pincfg-evb.dtsi"
> +#include <dt-bindings/i2c/i2c.h>
> +
> +/ {
> +	model = "Facebook Yosemite 4 BMC";
> +	compatible = "facebook,yosemite4-n-bmc", "nuvoton,npcm845";
> +
> +	aliases {
> +		serial4 = &serial0;
> +		serial0 = &serial1;
> +		serial1 = &serial3;
> +		serial2 = &serial4;
> +		serial3 = &serial5;
> +		serial5 = &cpld_serial0;
> +		serial6 = &cpld_serial1;
> +		serial7 = &cpld_serial2;
> +		serial8 = &cpld_serial3;
> +		fiu0 = &fiu0;
> +
> +		i2c16 = &imux16;
> +		i2c17 = &imux17;
> +		i2c18 = &imux18;
> +		i2c19 = &imux19;
> +		i2c20 = &imux20;
> +		i2c21 = &imux21;
> +		i2c22 = &imux22;
> +		i2c23 = &imux23;
> +		i2c24 = &imux24;
> +		i2c25 = &imux25;
> +		i2c26 = &imux26;
> +		i2c27 = &imux27;
> +		i2c28 = &imux28;
> +		i2c29 = &imux29;
> +		i2c30 = &imux30;
> +		i2c31 = &imux31;
> +		i2c32 = &imux32;
> +		i2c33 = &imux33;
> +		i2c34 = &imux34;
> +		i2c35 = &imux35;
> +		i2c36 = &imux36;
> +		i2c37 = &imux37;
> +	};
> +
> +	chosen {
> +		stdout-path = &serial0;
> +	};
> +
> +	memory {
> +		device_type = "memory";
> +		reg = <0x0 0x0 0x0 0x40000000>;
> +	};
> +
> +	iio-hwmon {
> +		compatible = "iio-hwmon";
> +		io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>,
> +			<&adc 4>, <&adc 5>, <&adc 6>, <&adc 7>;
> +	};
> +
> +	firmware {
> +		optee {
> +			compatible = "linaro,optee-tz";
> +			method = "smc";
> +		};
> +	};
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		tip_reserved: tip@0x0 {
> +			reg = <0x0 0x0 0x0 0x6200000>;
> +		};
> +	};
> +
> +	spi_gpio: spi-gpio {
> +		compatible = "spi-gpio";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		gpio-sck = <&gpio0 19 GPIO_ACTIVE_HIGH>; // GPIO19
> +		gpio-mosi = <&gpio0 18 GPIO_ACTIVE_HIGH>; // GPIO18
> +		gpio-miso = <&gpio0 17 GPIO_ACTIVE_HIGH>; // GPIO17
> +		num-chipselects = <1>;
> +		cs-gpios = <&gpio6 11 GPIO_ACTIVE_LOW>; // GPIO203
> +
> +		tpmdev@0 {

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 = "tcg,tpm_tis-spi";

Please base on ongoing work adding specific compatibles.

> +			spi-max-frequency = <33000000>;
> +			reg = <0>;

reg is always after compatible.

> +		};
> +	};
> +
> +	cpld_serial0: cpld_uart@f8000800 {

Eh... so again you make the same mistakes and send the same downstream
poor code with the same bad patterns we asked to fix.

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


> +		device_type = "serial";

compatible is always first, reg is second. Why do you need this property
anyway?

> +		compatible = "ns16450";
> +		reg = <0x0 0xf8000800 0x0 0x200>;
> +		reg-shift = <0>;
> +		clocks = <&clk NPCM8XX_CLK_UART>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
> +	};
> +
> +	cpld_serial1: cpld_uart@f8000a00 {
> +		device_type = "serial";
> +		compatible = "ns16450";
> +		reg = <0x0 0xf8000a00 0x0 0x200>;
> +		reg-shift = <0>;
> +		clocks = <&clk NPCM8XX_CLK_UART>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
> +	};
> +
> +	cpld_serial2: cpld_uart@f8000c00 {
> +		device_type = "serial";
> +		compatible = "ns16450";
> +		reg = <0x0 0xf8000c00 0x0 0x200>;
> +		reg-shift = <0>;
> +		clocks = <&clk NPCM8XX_CLK_UART>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
> +	};
> +
> +	cpld_serial3: cpld_uart@f8000e00 {
> +		device_type = "serial";
> +		compatible = "ns16450";
> +		reg = <0x0 0xf8000e00 0x0 0x200>;
> +		reg-shift = <0>;
> +		clocks = <&clk NPCM8XX_CLK_UART>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
> +	};
> +};
> +
> +&serial0 {
> +	status = "okay";
> +};
> +
> +&serial1 {
> +	status = "okay";
> +};
> +
> +&serial3 {
> +	status = "okay";
> +};
> +
> +&serial4 {
> +	status = "okay";
> +};
> +
> +&serial5 {
> +	status = "okay";
> +};
> +
> +&watchdog1 {
> +	status = "okay";
> +};
> +
> +&watchdog2 {
> +	status = "okay";
> +};
> +
> +&gmac2 {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&r1_pins
> +			&r1oen_pins>;
> +	use-ncsi;
> +};
> +
> +&gmac3 {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&r2_pins
> +			&r2oen_pins>;
> +	use-ncsi;
> +};
> +
> +&fiu0 {
> +	status = "okay";
> +	spi-nor@0 {

NAK

I am not going review further. You keep repeating the same mistakes.

Best regards,
Krzysztof