Re: [PATCH 3/3] arm64: dts: freescale: add Nodebox v3 CPU Module

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] System-critical reset GPIOs (such as the ethernet switch reset) are incorrectly tied to the `fspi` peripheral's pinctrl lifecycle.
- [Low] Active-low GPIO signals are semantically defined with incorrect polarity (GPIO_ACTIVE_HIGH).
- [Low] The new board compatible string 'freemobile,nbxv3' is missing from the dt-bindings documentation.
- [Low] The .dtb entry in the Makefile is inserted out of alphabetical order.
--

commit 8ec6719880bbc095a9e9249535f912cf432fd8e8
Author: Vincent Jardin <vjardin@free.fr>

arm64: dts: freescale: add Nodebox v3 CPU Module

This commit introduces the device tree and include files for the Nodebox v3,
a Free Mobile CPU module built around the NXP LX2160A. The same device
tree is intended to be shared between the Linux kernel and U-Boot.

> diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
> index 95ae85ab4adf5..f67d0a53cdc34 100644
> --- a/arch/arm64/boot/dts/freescale/Makefile
> +++ b/arch/arm64/boot/dts/freescale/Makefile
> @@ -59,6 +59,8 @@ DTC_FLAGS_fsl-lx2160a-qds := -Wno-interrupt_map
>  dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-qds.dtb
>  DTC_FLAGS_fsl-lx2160a-rdb := -Wno-interrupt_map
>  dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-rdb.dtb
> +DTC_FLAGS_fsl-lx2160a-nbxv3 := -Wno-interrupt_map
> +dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-nbxv3.dtb
>  DTC_FLAGS_fsl-lx2162a-clearfog := -Wno-interrupt_map

[Severity: Low]
Does this break the alphabetical ordering of the dtbs? It looks like
fsl-lx2160a-nbxv3.dtb is being inserted after fsl-lx2160a-rdb.dtb instead of
before fsl-lx2160a-qds.dtb.

> diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a-nbxv3.dts b/arch/arm64/boot/dts/freescale/fsl-lx2160a-nbxv3.dts
> new file mode 100644
> index 0000000000000..ccc5855208a63
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-nbxv3.dts
[ ... ]
> +/ {
> +	model = "Free Mobile Nodebox v3 CPU Module (LX2160A)";
> +	compatible = "freemobile,nbxv3", "fsl,lx2160a";
> +};

[Severity: Low]
Is the "freemobile,nbxv3" compatible string documented in the device tree
bindings? Missing documentation here might trigger warnings during a
make dtbs_check run.

> +
> +&fspi {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&fspi_dqs_sck_cs10_pins>, <&fspi_data30_pins>,
> +		    <&gpio1_31_28_pins>;
> +};

[Severity: Medium]
Does assigning the pinctrl group for GPIOs 28-31 to the fspi device create
a conflict with the gpio-hogs defined in the gpio1 controller?

If the pinmux controller enforces strict muxing, the gpio-hog nodes for
the ethernet switch and flash resets will fail to claim the pins because
they have already been claimed by fspi's pinctrl here. Furthermore, tying
unrelated system resets to the fspi peripheral's pinctrl lifecycle means
that if the fspi driver fails to probe, or is unbound, these components
will lose their configuration.

> diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a-nbxv3.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2160a-nbxv3.dtsi
> new file mode 100644
> index 0000000000000..bd10eccac4fb4
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-nbxv3.dtsi
[ ... ]
> +&gpio1 {
> +	status = "okay";
[ ... ]
> +	/* Ethernet switch reset, released: electrical high */
> +	ethsw-reset-hog {
> +		gpio-hog;
> +		gpios = <31 GPIO_ACTIVE_HIGH>;
> +		output-high;
> +		line-name = "ethsw-reset#";
> +	};
> +};

[Severity: Low]
Is this active-low hardware reset signal defined with the correct semantic
polarity? Defining an active-low reset as GPIO_ACTIVE_HIGH and manually
forcing the electrical state with output-high bypasses the device tree
coding standards, which expect the hardware's active state to be accurately
reflected in the bindings.

> +&gpio3 {
> +	status = "okay";
[ ... ]
> +	/* Low enables the carrier-side EMDIO2 level translator */
> +	ethsw-mdio-en-hog {
> +		gpio-hog;
> +		gpios = <25 GPIO_ACTIVE_HIGH>;
> +		output-low;
> +		line-name = "ethsw-mdio-en";
> +	};
> +};

[Severity: Low]
Similarly, this active-low enable signal is defined as GPIO_ACTIVE_HIGH
while using output-low to force the electrical low. Should this be defined
as GPIO_ACTIVE_LOW instead to reflect its true semantic polarity?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-for-upstream-nbxv3-dts-v1-0-f43fd0028b78@free.fr?part=3