Re: [PATCH net-next v6 5/5] ARM: dts: aspeed: ast2600-evb: Configure RGMII delay for MAC

[Andrew Lunn]

On Thu, Mar 05, 2026 at 05:05:03AM +0000, Jacky Chou wrote:
> Hi Andrew,
> 
> Thank you for yor reply.
> 
> > On Mon, Mar 02, 2026 at 06:24:32PM +0800, Jacky Chou wrote:
> > > This change sets the rx-internal-delay-ps and tx-internal-delay-ps
> > > properties to control the RGMII signal delay.
> > > The phy-mode for MAC0–MAC3 is updated to "rgmii-id" to enable TX/RX
> > > internal delay on the PHY and disable the corresponding delay on the
> > > MAC.
> > >
> > > Signed-off-by: Jacky Chou <jacky_chou@xxxxxxxxxxxxxx>
> > > ---
> > >  arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts | 20
> > > ++++++++++++++++----
> > >  1 file changed, 16 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
> > > b/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
> > > index 3f2ca9da0be2..a2a1c1dbb830 100644
> > > --- a/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
> > > +++ b/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts
> > > @@ -123,42 +123,54 @@ ethphy3: ethernet-phy@0 {
> > >  &mac0 {
> > >  	status = "okay";
> > >
> > > -	phy-mode = "rgmii-rxid";
> > > +	phy-mode = "rgmii-id";
> > >  	phy-handle = <&ethphy0>;
> > >
> > >  	pinctrl-names = "default";
> > >  	pinctrl-0 = <&pinctrl_rgmii1_default>;
> > > +
> > > +	rx-internal-delay-ps = <0>;
> > > +	tx-internal-delay-ps = <0>;
> > 
> > In the binding, you said these default to 0. So you don't need them.
> > 
> > It is also odd that rgmii-rxid becomes rmgii-id, yet both delays are 0?
> > 
> > What was the bootloader doing? This is worth a comment in the commit
> > messages.
> > 
> 
> Before this patch, aspeed-ast2600-evb.dts is an existed old dts in mainline kernel.
> In this series, ftgmac100 for AST2600 will configure the MAC RGMII internal delay
> via SCU register, so this patch is changing this dts as a NEW dts for driver to configure
> RGMII delay from the properties of MAC nodes.
> 
> Old dts: generally, leak tx/rx-internal-delay-ps -> Calculate the RGMII delay that is configured
> from bootloader and decide whether keep the original value
> 
> New dts: In AST2600, we expect the MAC node includes the rx/tx-internal-delay-ns properties
> and the driver directly uses these properties to configure RGMII delay.

You did not answer my question...

> It is also odd that rgmii-rxid becomes rmgii-id, yet both delays are 0?

I could understand "rgmii" becoming "rgmii-id" with both delays being
0. But that is not what you have here. It is starting as
"rgmii-rxid". So the PHY is being asked to insert the RX delay, but
not the TX delay. Where is the TX delay coming from?

I assume the MAC? Has the bootloader configured the MAC to insert the
TX delay? But look at all the board vendors who have been submitting
DT patches, and i've been rejecting them. They all seem to use rgmii,
not rgmii-rxid.

Why is this different to all the other boards? This is what i would
like to see in the commit message, an explanation of this oddness.

> We hope this series can keep the old dts works fine

Hope is not sufficient. Backwards compatibility is required. You need
to convince the reviewer the code changes are backwards
compatible. You can use the commit messages and comments in the code
to explain how backwards compatibility is maintained, while adding
this new functionality.

And once you have convinced me, i will probably ask you to post the
changes to the BMC mailing list, and ask for a few board vendors to
test the patches with old DT blobs, current DT blobs and updated DT
blobs.

And this is why i think fixing the issue in the bootloader is
better. It is much easier to convince a reviewer the changes are
backwards compatible.

	Andrew