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

[Frank Li <Frank.li@oss.nxp.com>]

On Sun, Sep 13, 2026 at 12:45:48PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [Medium] Ethernet PHY nodes lack 'interrupts' and 'reset-gpios' properties despite physical lines being explicitly defined on the board.
> - [Medium] Regulator polarity mismatch for `ethsw-mdio-en`.
> - [Low] Duplicate GPIO line name `usb01-oc#` in `gpio3` controller.
> - [Low] Missing Devicetree binding documentation for the new board compatible.
> --
>
> commit 8433b3c3d1e5258b1e8b5fc1b405c85d777065d4
> Author: Vincent Jardin <vjardin@free.fr>
>
> arm64: dts: freescale: add Nodebox v3 CPU Module
>
> The Nodebox v3 is a Free Mobile CPU module built around the NXP
> LX2160A (16x Cortex-A72, DPAA2).
>
> > 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..cc02204e9d1b4
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-nbxv3.dts
> [ ... ]
> > +/ {
> > +	compatible = "freemobile,nbxv3", "fsl,lx2160a";
>
> [Severity: Low]
> Are the devicetree bindings for this new board compatible string documented?
>
> New compatible strings must be documented to pass make dt_binding_check
> validation, but this patch does not appear to include the required updates
> to the Devicetree bindings documentation.
>
> > +	model = "Free Mobile Nodebox v3 CPU Module (LX2160A)";
> > +};
> > +
> [ ... ]
> > 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..ca4fb5329c0b2
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-nbxv3.dtsi
> [ ... ]
> > +	gpio-restart {
> > +		compatible = "gpio-restart";
> > +		gpios = <&gpio1 8 GPIO_ACTIVE_LOW>;
> > +		priority = <128>;
> > +	};
> > +
> > +	reg_ethsw_mdio: regulator-ethsw-mdio {
> > +		compatible = "regulator-fixed";
> > +		gpio = <&gpio3 25 GPIO_ACTIVE_LOW>;
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +		regulator-name = "ethsw-mdio-en";
>
> [Severity: Medium]
> Is ethsw-mdio-en truly an active-low signal?
>
> The reg_ethsw_mdio regulator specifies GPIO_ACTIVE_LOW and regulator-always-on.
> The kernel gpiolib will apply a logical-to-physical inversion, driving the
> physical pin to 0V.
>
> However, the board's strict naming convention uses a '#' or '-b' suffix for
> active-low signals (e.g., ethsw-reset#, usb01-oc#). The absence of this
> suffix on ethsw-mdio-en strongly implies it is an active-high signal. If it
> is active-high, driving it low will erroneously disable the MDIO bus to the
> Ethernet switch.

Is it true?

>
> > +	};
> > +};
> > +
> [ ... ]
> > +&emdio1 {
> > +	status = "okay";
> > +
> > +	/* Microchip LAN8023 dual 10G PCS/retimer, Clause 45 only, one address per slice */
> > +	lan8023_phy_s0: ethernet-phy@2 {
> > +		compatible = "ethernet-phy-ieee802.3-c45";
> > +		reg = <2>;
> > +	};
>
> [Severity: Medium]
> Should this node include the interrupts and reset-gpios mappings?
>
> The lan8023_phy_s0 node omits the interrupts (lan8023-mdint) and reset-gpios
> (lan8023-rst) mappings, even though these physical lines are explicitly
> defined on the board.
>
> Without the interrupts property, the kernel's PHY state machine is forced to
> use inefficient polling mode instead of hardware interrupts, increasing CPU
> overhead and latency.
>
> Missing the reset-gpios property prevents the kernel from managing the PHY
> reset lifecycle. If the I2C expander (u800) initializes to a state that holds
> the PHYs in reset, the PHY probe will fail completely.
>
> > +
> > +	lan8023_phy_s1: ethernet-phy@3 {
> > +		compatible = "ethernet-phy-ieee802.3-c45";
> > +		reg = <3>;
> > +	};
> > +
> > +	rtl8211fd: ethernet-phy@6 {
> > +		compatible = "ethernet-phy-id001c.c916";
> > +		reg = <6>;
> > +	};
>
> [Severity: Medium]
> Similar to the lan8023_phy_s0 node, should this node include the interrupts
> mapping?
>
> The rtl8211fd node omits the interrupts (u1300-rtl8211fd-int#) mapping.
> Missing this also forces the kernel to use polling mode for the PHY state
> machine.
>
> > +};
> [ ... ]
> > +&gpio3 {
> > +	gpio-line-names =
> > +		"altfn:ec1-txd3", "altfn:ec1-txd2",
> > +		"altfn:ec1-txd1", "altfn:ec1-txd0",
> > +		"altfn:ec1-tx-en", "altfn:ec1-gtx-clk",
> > +		"altfn:ec1-rxd3", "altfn:ec1-rxd2",
> > +		"altfn:ec1-rxd1", "altfn:ec1-rxd0",
> > +		"altfn:ec1-rx-clk", "altfn:ec1-rx-dv",
> > +		"", "",
> > +		"ptp-refclk-in",
> > +		"", "", "", "", "", "", "", "",
> > +		"u1700-zl30733-pps-out",
> > +		"",
> > +		"ethsw-mdio-en",
> > +		"usb01-oc#",	/* USB1_PWRFAULT and USB2_PWRFAULT share one net */
> > +		"",
> > +		"usb01-oc#",
>
> [Severity: Low]
> Are these duplicate line names intentional?

Please fix this small problem.

Frank

>
> Device tree GPIO line names must be unique within a controller. Defining
> usb01-oc# twice causes ambiguity; any in-kernel consumer or userspace tool
> (via libgpiod) attempting to lookup the GPIO by name will only ever match
> the first occurrence.
>
> This violates DT schema rules and makes the second pin inaccessible by name.
>
> > +		"altfn:spi3-sck",
> > +		"", "";
> > +
> > +	status = "okay";
> > +};
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260913-for-upstream-nbxv3-dts-v6-0-058566b93ba1@free.fr?part=3