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

[Vincent Jardin <vjardin@free.fr>]

Hi M. Bot,

I'll submit a v3 update tomorrow.

See below for the few other topics,

> > +&fspi {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&fspi_dqs_sck_cs10_pins>, <&fspi_data30_pins>;
> > +};
> 
> [Severity: High]
> Does this pinmux configuration conflict with the thermtrip-hog defined on gpio1
> pin 23?
> 
> During boot, the pin controller configures the RCWPMUXCR0 register (via
> fspi_dqs_sck_cs10_pins) to route pins 20-23 to the FSPI (XSPI1) controller en
> masse. Concurrently, the GPIO driver applies the thermtrip-hog to pin 23 in
> fsl-lx2160a-nbxv3.dtsi, attempting to drive it low as a GPIO.

Correct, it is something I should have cleaned sooner. I'll be removed into the
next update.

> > +&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]
> Will this force the kernel into an inefficient polling mode for the Ethernet
> PHYs?
> 
> The hardware possesses dedicated interrupt lines for the PHYs, as the gpio2
> node explicitly defines the line names 'lan8023-mdint' and
> 'u1300-rtl8211fd-int#'. Without interrupts and interrupt-parent properties in
> the PHY nodes (lan8023_phy_s0, lan8023_phy_s1, and rtl8211fd), the PHY driver
> will fall back to periodically polling the MDIO bus to check for link state
> changes.

The lines do exist and gpio2 is an interrupt-controller, so this can be
added. I'd prefer to keep it for a future update.

> [Severity: Medium]
> Should this node also include a reset-gpios property to ensure the PHY is
> properly initialized?
> 
> The u800 I2C GPIO expander explicitly defines line 15 as 'lan8023-rst'.
> Without claiming this GPIO (e.g., reset-gpios = <&u800 15 GPIO_ACTIVE_LOW>),
> the Ethernet driver will not execute a hardware reset sequence during probing,
> leaving the hardware dependent on the state the bootloader left it in and
> potentially failing to recover from a suspended state.

It is a single reset shared by both slices of the one LAN8023, so it cannot be
claimed by both.
The reset is owned by the bring-up userland application of the lan8023.

> > +&fspi {
> > +	status = "okay";
> > +
> > +	/* GigaDevice GD55LB02GF, 256 MiB boot NOR */
> > +	flash@0 {
> > +		compatible = "jedec,spi-nor";
> > +		reg = <0>;
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		spi-max-frequency = <150000000>;
> > +		spi-rx-bus-width = <1>;
> > +		spi-tx-bus-width = <1>;
> > +	};
> > +};
> 
> [Severity: Medium]
> Does this artificially restrict the FSPI flash to a 1-bit bus width despite the
> hardware being wired for 4-bit Quad SPI?
> 
> The pinctrl-0 property in the .dts file includes <&fspi_data30_pins>, which
> configures four data lines (DATA[3:0]) for the FSPI controller. However, this
> flash@0 node sets the spi-rx-bus-width and spi-tx-bus-width to 1. This mismatch
> forces the SPI NOR flash driver to operate in single-bit mode rather than
> quad mode, needlessly degrading I/O performance.

Deliberately on purpose for this board ! for the time being.

Thanks,
  Vincent
 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260907-for-upstream-nbxv3-dts-v2-0-6b4afd36180b@free.fr?part=3