Re: [PATCH v3 2/2] arm64: zynqmp: Add ifm ZynqMP VHIP6 EvalBoard

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] net: dt-bindings: Incorrect Clause 45 compatible string forces incompatible MDIO probe on C22 PHY
- [High] net: dt-bindings: Insufficient reset deassert delay for DP83822I PHY
- [Medium] net: dt-bindings: Contradictory RGMII internal delay configuration on gem1
- [Medium] mmc: dt-bindings: eMMC node lacks 'non-removable' and 'bus-width' properties
- [Medium] nvmem: dt-bindings: 8-byte NVMEM MAC address cells will be rejected by the kernel
- [Low] dt-bindings: Dangling alias to disabled usb0 node
--

commit 2e2a25c7bccf2c438229b39299b352c4fb573831
Author: Marek Vasut <marex@nabladev.com>

arm64: zynqmp: Add ifm ZynqMP VHIP6 EvalBoard

Add support for ifm ZynqMP VHIP6 EvalBoard reference design.
The board is populated with four ethernet ports, QSPI NOR,
eMMC, UART and USB host interfaces.

> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-vhip6-evalboard.dts b/arch/arm64/boot/dts/xilinx/zynqmp-vhip6-evalboard.dts
> new file mode 100644
> index 0000000000000..ed2c6d170426d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/xilinx/zynqmp-vhip6-evalboard.dts

[ ... ]

> +	aliases {
> +		ethernet0 = &gem0;
> +		ethernet1 = &gem1;
> +		ethernet2 = &gem2;
> +		ethernet3 = &gem3;
> +		gpio0 = &gpio;
> +		i2c0 = &i2c0;
> +		i2c1 = &i2c1;
> +		rtc0 = &i2crtc;
> +		serial0 = &uart0;
> +		spi0 = &qspi;
> +		usb0 = &usb0;
> +		usb1 = &usb1;

[Severity: Low]
Does this map usb0 to a disabled node? In the base zynqmp.dtsi, usb0 defaults
to disabled and it is never enabled here, which might cause confusion for
bootloaders or userspace indexing.

> +	};

[ ... ]

> +&gem0 {
> +	phy-handle = <&phy0>;
> +	phy-mode = "mii";
> +	status = "okay";
> +
> +	mdio0: mdio {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		phy0: phy@1 {	/* TI DP83822I */
> +			compatible = "ethernet-phy-id2000.a240",
> +				     "ethernet-phy-ieee802.3-c45";

[Severity: High]
Does declaring "ethernet-phy-ieee802.3-c45" force an incompatible MDIO probe
on this C22 PHY? The fwnode_mdiobus_register_phy() function sets is_c45 = true
when it sees this compatible string. This forces the Cadence MACB MDIO
controller to send Clause 45 MDIO frames (Start Code 00). 

Since the TI DP83822I is a Clause 22 PHY (expecting Start Code 01), will it
ignore the frames and cause get_phy_device() to fail with -ENODEV?

This same compatible string is used for phy1, phy2, and phy3 below.

> +			reg = <1>;
> +			reset-gpios = <&gpio 78 GPIO_ACTIVE_LOW>;
> +			reset-assert-us = <10>;
> +			reset-deassert-us = <10>;

[Severity: High]
Is a 10 us deassert delay sufficient for the DP83822I PHY? Section 8.4.1.2 of
the TI DP83822I datasheet strictly requires a minimum of 150 us before MDIO
access. Accessing the PHY before its internal state machine has initialized
could cause the MDIO probe to fail or read garbage.

This same deassert delay is also used for phy1, phy2, and phy3 below.

> +		};
> +	};
> +};
> +
> +&gem1 {
> +	phy-handle = <&phy1>;
> +	phy-mode = "rgmii-rxid";
> +	status = "okay";
> +
> +	mdio1: mdio {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		phy1: phy@1 {	/* TI DP83822I */
> +			compatible = "ethernet-phy-id2000.a240",
> +				     "ethernet-phy-ieee802.3-c45";
> +			reg = <1>;
> +			max-speed = <100>;
> +			reset-gpios = <&gpio 8 GPIO_ACTIVE_LOW>;
> +			reset-assert-us = <10>;
> +			reset-deassert-us = <10>;
> +			tx-internal-delay-ps = <1>;

[Severity: Medium]
Does specifying tx-internal-delay-ps = <1> contradict the "rgmii-rxid"
phy-mode requested by gem1?

The dp83822 driver's DP83822_TX_CLK_SHIFT logic disables the TX delay only if
tx_int_delay <= 0. Because 1 > 0, the TX delay appears to be erroneously
enabled here, violating the RGMII-RXID contract and potentially causing packet
loss or link instability.

> +		};
> +	};
> +};

[ ... ]

> +&i2c1 {
> +	clock-frequency = <400000>;
> +	status = "okay";
> +
> +	eeprom@51 {
> +		compatible = "atmel,24c128";
> +		reg = <0x51>;
> +		label = "ifm-deviceinfo";
> +		wp-gpios = <&gpio 37 GPIO_ACTIVE_HIGH>;
> +
> +		nvmem-layout {
> +			compatible = "fixed-layout";
> +			#address-cells = <1>;
> +			#size-cells = <1>;

[ ... ]

> +			/* Each MAC address is padded to 8 Bytes */
> +			cfg_eeprom_mac1: mac-address-1@c8 {
> +				reg = <0xc8 0x8>;

[Severity: Medium]
Will this 8-byte NVMEM cell be rejected by the kernel? nvmem_get_mac_address()
in net/ethernet/eth.c strictly verifies that the read cell length is exactly
ETH_ALEN (6). Because the reg length is defined as 8 bytes, the
if (len != ETH_ALEN) check will fail and return -EINVAL, causing the
programmed MAC addresses to be ignored.

This also applies to mac-address-2 through mac-address-8 below.

> +			};

[ ... ]

> +&qspi {
> +	status = "okay";
> +
> +	flash@0 {
> +		compatible = "jedec,spi-nor";
> +		reg = <0x0>;
> +		spi-max-frequency = <80000000>;
> +		spi-tx-bus-width = <1>;
> +		spi-rx-bus-width = <4>;
> +	};
> +};
> +
> +&sdhci0 {
> +	xlnx,mio-bank = <0x0>;
> +	status = "okay";
> +};

[Severity: Medium]
Since this is an eMMC controller, is it missing the mandatory 'non-removable'
and 'bus-width = <8>' properties?

Without these, the sdhci driver will initialize the device using default
properties meant for hot-pluggable SD cards, which might cause the eMMC to
fail probing, operate at degraded speeds, or erroneously poll for a card
detect signal.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916204509.166633-1-marex@nabladev.com?part=2