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

[Jacky Chou]

> > > > -	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.

The current ftgmac100 driver does not support to configure the MAC RGMII delay and 
these tx/rx internal delay on MAC is configured in bootloader stage.
So, the phy-mode of MAC node in current aspeed-ast2600-evb.dts will use the same
from U-boot dts.

Could you please help confirm how we should proceed with this project?

1. Discard this series.
2. Fix the bootloader that we provide to our customers.
3. Send a patch to correct the phy-mode settings in aspeed-ast2600-evb.dts so that they 
match the bootloader after the fix.

mac0: "rgmii-rxid" -> "rgmii-id"
mac1: "rgmii-rxid" -> "rgmii-id"
mac2: "rgmii" -> "rgmii-id"
mac3: "rgmii" -> "rgmii-id"

We will also add a detailed comment in the patch to clarify why these phy-mode settings 
are changed and when the MAC RGMII internal delay should be configured.

Thanks,

Jacky