Re: [net-next v2 0/4] Add AST2600 RGMII delay into ftgmac100

[Andrew Lunn <andrew@lunn.ch>]

> I would like to discuss with you how we fix the RGMII of AST2600 in this thread.
> And thank you for your patience in reviewing our code.
> 
> Currently, the RGMII delay in AST2600 is configured in U-boot stage, not in Linux.
> The ftgmac driver will not use the phy-mode to configure the RGMII delay on MAC side.
> 
> I list the parts that I think need to be modified.
> 1. Change the phy-mode to "rgmii-id" in aspeed-ast2600-evb.dts.
> 2. Add the tx/rx-internal-delay-ps in dts.
> 3. Add RGMII delay configuration in ftgmac driver. If the tx/rx-internal-delay-ps has not existed,
>   according to the phy-mode to configure default value.
> 
> These are the fix items I can think of.
> Could you point out what I miss or a clear direction to correct the RGMII mode on AST2600?

We have to be careful with the assumption u-boot is configuring
delays. I've seen DT blobs which use 'rgmii-id', which suggests
something is disabling the MAC delays, maybe because they are using a
different version of u-boot, or barebox etc.

You should be able to read what the MAC is doing with delays. You can
compare this with what the phy-mode is.

* If the MAC is adding delays, and the phy-mode is rgmii-id, disable
  the MAC delays, pass rgmii-id to the PHY.

* If the MAC is adding delays, and the phy-mode is rgmii, issue a
  warning the DT blob is out of date, disable the MAC delays, and pass
  rgmii-id to the PHY.

* If the MAC is not adding delays, and the phy-mode is rgmii-id, pass
  rmgii-id to the PHY.

* If the MAC is not adding delays, and the phy-mode is rgmii, that
  suggests the PCB has extra long clock lines, and the board is using
  a U-boot which has been modified to not enable MAC delays. Pass
  rgmii to the PHY.

I would also suggest you review all DT blobs in mainline and see if
any are using rgmii-rxid or rgmii-txid. If not, i would go with KISS
and return -EINVAL for these, with a comment. Support for these can be
added when somebody actually needs them.

Most boards today will be the second bullet point. With time, they can
transition to the first. New boards should hopefully be directly the
first bullet point.

Backwards compatibility is however an issue. Anybody with a new DT
blob won't have working Ethernet with an old kernel if they are using
the first bullet point. So we should not edit every DT blob in
mainline and change rgmii to rgmii-id. We want to leave it to the
board owner to decide it is time to make the warning go away.

tx/rx-internal-delay-ps are optional. I would not add them yet,
because it makes the logic more complex. As far as i understand, no
board has required them so far, so there does not appear to be a need
for them.

You need good commit messages with these changes. Make it clear there
could be backwards compatibility issues, but that is the cost of
fixing up a broken implementation.

	Andrew