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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The TI DP83822I PHY is incorrectly declared as a Clause 45 PHY, which will cause MDIO probe failures.
- [Medium] Contradictory RGMII internal delay configuration on gem1 forces TX delay on a mode that explicitly requests it disabled.
- [Medium] USB0 interface is not enabled despite commit message claiming 'USB host interfaces' (plural).
- [Low] Magic number '1' used instead of the standard 'GPIO_ACTIVE_LOW' macro.
- [Low] Non-generic node name for LM75 temperature sensor.
--

commit 9efb3b575525e794cefe9379fc0090ce8f78e409
Author: Marek Vasut <marex@nabladev.com>

arm64: zynqmp: Add ifm ZynqMP VHIP6 EvalBoard

This commit adds support for the ifm ZynqMP VHIP6 EvalBoard reference
design. The board configuration includes 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
> --- /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;

[Severity: Medium]
The commit message mentions USB host interfaces in the plural, and there is an
alias for usb0 here, but is the usb0 node actually enabled in this file?

The zynqmp.dtsi base disables both usb0 and usb1 by default. While usb1
is enabled later in this file, there is no corresponding override for usb0
or dwc3_0. Does this inadvertently leave the first USB interface disabled?

> +		usb1 = &usb1;
> +	};
[ ... ]
> +&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]
Will this compatible string cause the MDIO probe to fail? 

The TI DP83822I is a Clause 22 PHY, but the ethernet-phy-ieee802.3-c45 string
sets the is_c45 flag in the kernel MDIO core. This forces macb_mdio to emit
C45 frames, which the DP83822I will ignore, causing the read to return 0xffff
and the probe to fail with -ENODEV.

Should this just be ethernet-phy-ieee802.3-c22 or rely solely on the ID?
This same C45 compatibility declaration appears to be applied to gem1, gem2,
and gem3 below as well.

> +			reg = <1>;
> +			reset-gpios = <&gpio 78 GPIO_ACTIVE_LOW>;
> +			reset-assert-us = <10>;
> +			reset-deassert-us = <10>;
> +		};
> +	};
> +};
> +
> +&gem1 {
> +	phy-handle = <&phy1>;
> +	phy-mode = "rgmii-rxid";

[Severity: Medium]
Does this PHY mode contradict the internal delay property defined below? 
The rgmii-rxid mode specifies that only RX delay should be added by the PHY.

> +	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]
The DP83822 driver interprets tx-internal-delay-ps as a request to enable the
PHY internal TX delay, overriding the rgmii-rxid contract. 

If the MAC or PCB also applies a delay, this double-delay on the TX clock
could cause connection instability. Should this property be removed, or should
the mode be changed to rgmii-id?

> +		};
> +	};
> +};
[ ... ]
> +&i2c0 {
> +	clock-frequency = <400000>;
> +	status = "okay";
> +
> +	lm75@48 {
> +		compatible = "national,lm75";

[Severity: Low]
Does this node name align with the Devicetree specification? 

The guidelines require node names to be generic and describe the function of
the device rather than the specific model. Would a name like
temperature-sensor@48 or sensor@48 be more appropriate here?

> +		reg = <0x48>;
> +	};
[ ... ]
> +&usb1 {
> +	reset-gpios = <&gpio 77 1>;

[Severity: Low]
Is there a reason to use the magic number 1 here instead of the standard
GPIO_ACTIVE_LOW macro? 

The dt-bindings header is included at the top of the file, and GPIO_ACTIVE_LOW
is used correctly for all other reset-gpios definitions in this board setup.

> +	status = "okay";
> +};

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