Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY

[Philippe Schenker]

On Tue, 2020-04-28 at 17:47 +0200, Andrew Lunn wrote:
> On Tue, Apr 28, 2020 at 05:28:30PM +0200, Geert Uytterhoeven wrote:
> > This triggers on Renesas Salvator-X(S):
> > 
> >     Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00:
> > *-skew-ps values should be used only with phy-mode = "rgmii"
> > 
> > which uses:
> > 
> >         phy-mode = "rgmii-txid";
> > 
> > and:
> > 
> >         rxc-skew-ps = <1500>;
> > 
> > If I understand Documentation/devicetree/bindings/net/ethernet-
> > controller.yaml
> > correctly:
> 
> Hi Geert
> 
> Checking for skews which might contradict the PHY-mode is new. I think
> this is the first PHY driver to do it. So i'm not too surprised it has
> triggered a warning, or there is contradictory documentation.
> 
> Your use cases is reasonable. Have the normal transmit delay, and a
> bit shorted receive delay. So we should allow it. It just makes the
> validation code more complex :-(
> 
> 	   Andrew

Hello Geert and Andrew

I reviewed Oleksij's patch that introduced this warning. I just want to
explain our thinking why this is a good thing, but yes maybe we change
that warning a little bit until it lands in mainline.

The KSZ9031 driver didn't support for proper phy-modes until now as it
don't have dedicated registers to control tx and rx delays. With
Oleksij's patch this delay is now done accordingly in skew registers as
best as possible. If you now also set the rxc-skew-ps registers those
values you previously set with rgmii-txid or rxid get overwritten.

We chose the warning to occur on phy-modes 'rgmii-id', 'rgmii-rxid' and
'rgmii-txid' as on those, with the 'rxc-skew-ps' value present,
overwriting skew values could occur and you end up with values you do
not wanted. We thought, that most of the boards have just 'rgmii' set in
phy-mode with specific skew-values present.

@Geert if you actually want the PHY to apply RXC and TXC delays just
insert 'rgmii-id' in your DT and remove those *-skew-ps values. If you
need custom timing due to PCB routing it was thought out to use the phy-
mode 'rgmii' and do the whole required timing with the *-skew-ps values.

@Andrew This warning might be not the best solution but we should
definitely warn that values might get overwritten from what was intended
with e.g. 'rgmii-txid'. The out-of-reset behaviour of the PHY actually
is 'rgmii-txid' so we may also throw now warning if this mode is set.
What is your oppinion?

Regards,
Philippe