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

[李志 <lizhi2@eswincomputing.com>]

Thank you for the review. Please see my responses inline below.


> -----Original Messages-----
> From: sashiko-bot@kernel.org
> Send time:Wednesday, 01/07/2026 14:35:12
> To: lizhi2@eswincomputing.com
> Cc: conor+dt@kernel.org, robh@kernel.org, devicetree@vger.kernel.org
> Subject: Re: [PATCH net-next v9 6/6] riscv: dts: eswin: eic7700-hifive-premier-p550: enable Ethernet controller
> 
> 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.
> 

There are a few cases where spaces are used after tabs for alignment. I will
remove the alignment spaces in the next revision and keep the indentation
consistent with the kernel coding style.

> > +		 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?
> 

As described in the upstream device tree binding documentation:
https://elixir.bootlin.com/linux/v6.15/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287

The MAC internal delay (tx-internal-delay-ps / rx-internal-delay-ps) is
intended for fine-grained timing adjustment, while the PHY RGMII mode(phy-mode)
defines the standard RGMII timing behavior.

In this model, the PHY provides the RGMII timing mode (including the typical
~2ns delay for rgmii-id), while the MAC delay is used only for additional
silicon-specific timing tuning and is not equivalent to an additional full
RGMII delay.

Therefore, this configuration does not result in a duplicated TX delay condition.

> > +	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?
> 

The rationale for the eth1 timing model is described in the binding and driver
commit messages of this series.

As discussed previously during the review, the eth1 MAC has a fixed
silicon-inherent TX delay that cannot be disabled. Andrew also pointed this out
during the binding review:

https://lore.kernel.org/lkml/6ecfeac7-b900-4861-b0b8-5c5d2e0754b2@lunn.ch/

The current binding and driver implementation follow the outcome of that
discussion.

> > +	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?
> 

I will update the node name to bus@50400000 to match the address space
described by the ranges property and avoid dtc 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.
> 

This compatible was introduced to distinguish the eth1 hardware variant, which
has silicon-specific RX sampling behavior and TX timing characteristics.
The rationale was discussed previously here:

https://lore.kernel.org/lkml/32a1f814.2c79.19bfe173225.Coremail.linmin@eswincomputing.com/

The current implementation follows the approach from that discussion.

> > +				reg = <0x0 0x50410000 0x0 0x10000>;
> > +				interrupts = <70>;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260630063123.1118-1-lizhi2@eswincomputing.com?part=6

Hi Andrew,

One question before I prepare the next revision.

As discussed previously, the DTS patch was included only to provide the overall
Ethernet design context during the review of the binding and driver patches:

https://lore.kernel.org/lkml/64bf6b40-b947-4ffa-8d48-4d6341931327@lunn.ch/

For the next revision, would it be acceptable to drop the DTS patch from this
series and post only the binding and driver patches? The complete DTS
enablement is planned to be submitted later as a separate series after the
binding and driver have been merged.

Also, if you have any further comments or suggestions on the binding or driver
patches, I'd be happy to address them in the next revision.

Best regards,
Zhi Li