Re: [PATCH net-next v9 6/6] riscv: dts: eswin: eic7700-hifive-premier-p550: enable Ethernet controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Contradictory RGMII delay configuration using `phy-mode = "rgmii-id"` causes double TX delay and violates RGMII timing specifications.
- [Medium] Hardware behavioral workaround or instance-specific configuration encoded into a compatible string.
- [Low] Invalid unit-address for the `hsp` simple-pm-bus node.
- [Low] Indentation style violation (mixing tabs and spaces).
--

commit bfe37dac7b1890539f4db41b461affd87be42ea4
Author: Zhi Li <lizhi2@eswincomputing.com>

riscv: dts: eswin: eic7700-hifive-premier-p550: enable Ethernet controller

Enable the on-board Gigabit Ethernet controller on the HiFive Premier P550
development board. This patch is included only to provide the DTS context for
reviewing the binding and driver changes.

> diff --git a/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts b/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts
> index 131ed1fc6b2ed..9d37bafdd1b2e 100644
> --- a/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts
> +++ b/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts
[ ... ]
> +	vcc_1v8: regulator-1v8 {
> +		 compatible = "regulator-fixed";
> +		 regulator-name = "vcc1v8";

[Severity: Low]
Are there space characters being used for indentation here after the tabs?
The kernel standard requires strict 8-character tab indentation.

> +		 regulator-always-on;
> +		 regulator-boot-on;
> +		 regulator-min-microvolt = <1800000>;
> +		 regulator-max-microvolt = <1800000>;
> +	 };
> +};
[ ... ]
> +&gmac0 {
> +	phy-handle = <&gmac0_phy0>;
> +	phy-mode = "rgmii-id";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_gpio106>;
> +	rx-internal-delay-ps = <20>;
> +	tx-internal-delay-ps = <100>;

[Severity: High]
Does this configuration cause a double TX delay?

The phy-mode is set to rgmii-id, which tells the PHY to add both TX and RX
delays. However, the MAC is additionally configured with a
tx-internal-delay-ps. At Gigabit speeds, this excessive TX skew could
corrupt the data stream.

Could the phy-mode be set to rgmii-rxid if the MAC already provides the
TX delay?

> +	status = "okay";
> +};
[ ... ]
> +&gmac1 {
> +	phy-handle = <&gmac1_phy0>;
> +	/*
> +	 * The MAC silicon unconditionally introduces an ~2 ns TX clock-to-data
> +	 * skew (MAC-side TX internal delay). The PHY provides the standard
> +	 * ~2 ns RX internal delay. The driver additionally inverts the RX
> +	 * clock at 1000 Mb/s to correct a silicon RX sampling timing issue.
> +	 * phy-mode is "rgmii-id": TX delay from the MAC silicon, RX delay
> +	 * from the PHY.
> +	 */
> +	phy-mode = "rgmii-id";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_gpio111>;
> +	rx-internal-delay-ps = <200>;
> +	tx-internal-delay-ps = <2200>;

[Severity: High]
Is the comment regarding rgmii-id accurate for Linux?

The comment mentions that rgmii-id means TX delay from the MAC and RX delay
from the PHY. In Linux, rgmii-id tells the PHY to add both delays, which will
lead to double TX delays when combined with the MAC-side delay.

Would this result in packet loss or connection failures?

> +	status = "okay";
> +};
[ ... ]
> diff --git a/arch/riscv/boot/dts/eswin/eic7700.dtsi b/arch/riscv/boot/dts/eswin/eic7700.dtsi
> index c3ed93008bca1..041ecc5cb0077 100644
> --- a/arch/riscv/boot/dts/eswin/eic7700.dtsi
> +++ b/arch/riscv/boot/dts/eswin/eic7700.dtsi
[ ... ]
> +		hsp: bus@0 {
> +			compatible = "simple-pm-bus";
> +			ranges = <0x0 0x50400000 0x0 0x50400000 0x0 0xa0000>;

[Severity: Low]
Does the unit address bus@0 match the ranges property?

According to the Devicetree Specification, when a node has a ranges property
but no reg property, the unit-address should match the first child address.
Could this be changed to bus@50400000 to prevent dtc compiler warnings?

> +			clocks = <&clk 171>;
> +			#address-cells = <2>;
> +			#size-cells = <2>;
[ ... ]
> +			gmac1: ethernet@50410000 {
> +				compatible = "eswin,eic7700-qos-eth-clk-inversion",
> +					     "snps,dwmac-5.20";

[Severity: Medium]
Will this compatible string be rejected by the Devicetree maintainers?

It appears to encode a software workaround for clock inversion rather than
describing the hardware block itself. Instance-specific configurations usually
need to be handled via dedicated boolean properties.

> +				reg = <0x0 0x50410000 0x0 0x10000>;
> +				interrupts = <70>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630063123.1118-1-lizhi2@eswincomputing.com?part=6