Re: [PATCH v1 1/2] arm64: dts: nuvoton: npcm845: Add peripheral nodes

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

On 17/07/2025 15:53, Tomer Maimon wrote:
> Enable peripheral support for the Nuvoton NPCM845 SoC by adding device
> nodes for Ethernet controllers, MMC controller, SPI controllers, USB
> device controllers, random number generator, ADC, PWM-FAN controller,
> and I2C controllers. Include pinmux configurations for relevant
> peripherals to support hardware operation. Add an OP-TEE firmware node
> for secure services.
> This patch enhances functionality for NPCM845-based platforms.

Drop this sentence, redundant and not in style (see submitting patches).
> 
> Depends-on: ARM: dts: nuvoton: npcm845: Add pinctrl groups

There is no such tag.

Use changelog for this purpose or b4 dependencies.


> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>  .../dts/nuvoton/nuvoton-common-npcm8xx.dtsi   | 695 ++++++++++++++++++
>  .../boot/dts/nuvoton/nuvoton-npcm845.dtsi     |   7 +
>  2 files changed, 702 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> index 5cd877e6c20a..3564e0e30791 100644
> --- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> @@ -4,6 +4,7 @@
>  #include <dt-bindings/clock/nuvoton,npcm845-clk.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/gpio/gpio.h>
>  
>  / {
>  	#address-cells = <2>;
> @@ -40,6 +41,11 @@ ppi_cluster0: interrupt-partition-0 {
>  		};
>  	};
>  
> +	udc0_phy: usb-phy {
> +		#phy-cells = <0>;
> +		compatible = "usb-nop-xceiv";

Please follow DTS coding style.

> +	};
> +
>  	ahb {
>  		#address-cells = <2>;
>  		#size-cells = <2>;
> @@ -56,6 +62,259 @@ clk: rstc: reset-controller@f0801000 {
>  			#clock-cells = <1>;
>  		};
>  
> +		gmac1: eth@f0804000 {

Please follow established naming for node names. I'll fix whatever old
stuff you took it from.


> +			device_type = "network";
> +			compatible = "snps,dwmac";

I don't think we want the generic one, even if it is allowed by bindings.

Also... You CC-ed an address, which suggests you do not work on mainline
kernel or you do not use get_maintainers.pl/b4/patman. Please rebase and
always work on mainline or start using mentioned tools, so correct
addresses will be used.


> +			reg = <0x0 0xf0804000 0x0 0x2000>;


> +			interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "macirq";
> +			clocks	= <&clk NPCM8XX_CLK_AHB>;
> +			clock-names = "stmmaceth";
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&rg2_pins
> +					&rg2mdio_pins>;
> +			status = "disabled";
> +		};


...

> +
> +		mc: memory-controller@f0824000 {
> +			compatible = "nuvoton,npcm845-memory-controller";
> +			reg = <0x0 0xf0824000 0x0 0x1000>;
> +			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		udc0:usb@f0830000 {

DTS coding style...

> +			compatible = "nuvoton,npcm845-udc";
> +			reg = <0x0 0xf0830000 0x0 0x1000
> +			       0x0 0xfffeb000 0x0 0x800>;
> +			interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&clk NPCM8XX_CLK_SU>;
> +			clock-names = "clk_usb_bridge";
> +
> +			phys = <&udc0_phy>;
> +			phy_type = "utmi_wide";
> +			dr_mode = "peripheral";
> +			status = "disabled";
> +		};
> +

...


Best regards,
Krzysztof