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

[Oleksij Rempel <o.rempel@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 4481 bytes --]

Hi Geert,

On Wed, Apr 29, 2020 at 10:45:35AM +0200, Geert Uytterhoeven wrote:
> Hi Philippe,
> 
> On Tue, Apr 28, 2020 at 6:16 PM Philippe Schenker
> <philippe.schenker@toradex.com> wrote:
> > 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:
> > >
> > > 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 :-(
> >
> > 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
> 
> That seems to work for me, but of course doesn't take into account PCB
> routing.

On boards with simple design, the clock lines have nearly same length as data
lines. To provide needed clock delay, you should make clock line ~17
centimeter longer than data lines. Or configure PHY or MAC side to
provide needed delay.
Since "phy-mode = "rgmii-txid"" was ignored till my patch. And the
"rxc-skew-ps = <1500>" will add a delay of 0.6 nano seconds. Your
configuration was:
TX delay = 1.2ns
RX delay = 0.6ns

Is it really reflects the configuration of you PCB?

> > 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.
> 
> That mean we do have to provide all values again?

No. Using proper phy-mode should be enough. If you using default TX dealy and
configuring RX delay manually, the phy-mode = "rgmii-id" is
the right choice for you.

> Using "rgmii" without any skew values makes DHCP fail on R-Car H3 ES2.0,
> M3-W (ES1.0), and M3-N (ES1.0). Interestingly, DHCP still works on R-Car
> H3 ES1.0.

The TX delay affects MAC to PHY path. The RX delay affects PHY to MAC
path. On my HW, disabling TX delays, didn't affected the communication
in any measurable way. Even with clock line length is equal to the data
lines length. So, it may work just on the edge of the spec.

> Note that I'm not too-familiar with the actual skew values needed
> (CC Mizuguchi-san).
> 
> Related commits:
>   - 0e45da1c6ea6b186 ("arm64: dts: r8a7795: salvator-x: Fix
> EthernetAVB PHY timing")
>   - dda3887907d74338 ("arm64: dts: r8a7795: Use rgmii-txid phy-mode
> for EthernetAVB")
>   - 7eda14afb8843a0d ("arm64: dts: renesas: r8a77990: ebisu: Fix
> EthernetAVB phy mode to rgmii")

Regards,
Oleksij.
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]