Message-ID: <DB8PR04MB698538EB0ED388D39771E159EC0F0@DB8PR04MB6985.eurprd04.prod.outlook.com>
Date:   Thu, 23 Jan 2020 07:20:58 +0000
From:   "Madalin Bucur (OSS)" <madalin.bucur@....nxp.com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        "Madalin Bucur (OSS)" <madalin.bucur@....nxp.com>,
        "davem@...emloft.net" <davem@...emloft.net>
CC:     "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ykaukab@...e.de" <ykaukab@...e.de>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>
Subject: RE: [PATCH net-next 2/2] dpaa_eth: support all modes with rate
 adapting PHYs

> -----Original Message-----
> From: Heiner Kallweit <hkallweit1@...il.com>
> Sent: Wednesday, January 22, 2020 10:06 PM
> To: Madalin Bucur (OSS) <madalin.bucur@....nxp.com>; davem@...emloft.net
> Cc: andrew@...n.ch; f.fainelli@...il.com; netdev@...r.kernel.org;
> ykaukab@...e.de
> Subject: Re: [PATCH net-next 2/2] dpaa_eth: support all modes with rate
> adapting PHYs
> 
> On 22.01.2020 14:59, Madalin Bucur wrote:
> > Stop removing modes that are not supported on the system interface
> > when the connected PHY is capable of rate adaptation. This addresses
> > an issue with the LS1046ARDB board 10G interface no longer working
> > with an 1G link partner after autonegotiation support was added
> > for the Aquantia PHY on board in
> >
> > commit 09c4c57f7bc4 ("net: phy: aquantia: add support for auto-
> negotiation configuration")
> >
> > As it only worked in other modes besides 10G because the PHY
> > was not configured by its driver to remove them, this is not
> > really a bug fix but more of a feature add.
> >
> 
> I understand the issue, however the description may be a little
> misleading.
> mac_dev->if_support doesn't include 1Gbps mode, therefore this mode is
> removed from phydev->supported. What happens:
> - before referenced commit: aqr_config_aneg() basically is a
>   no-op and doesn't touch the advertised modes in the chip.
>   Therefore 1Gbps is advertised and aneg succeeds.
> - after referenced commit: 1Gbps is removed from modes advertised by the
>   PHY, therefore aneg doesn't succeed.

Hi, I can include this description in the commit log, it's a more detailed
explanation of the actual mechanism behind the above issue. 
 
> Maybe in the context of this change the interface mode should be fixed.
> These Aquantia PHY's don't support XGMII, they support USXGMII.
> USXGMII support was added to phylib not too long ago, therefore older
> drivers use value PHY_INTERFACE_MODE_XGMII. For the same compatibility
> reason the Aquantia PHY driver still accepts PHY_INTERFACE_MODE_XGMII.
> 
> Heiner

There has been a long email thread [1] related to this particular issue.
Please note USXGMII is one of the supported modes of the AQR PHYs but
unfortunately not a mode our SoC is capable to use. The DPAA 1 platforms
do not support USXGMII, they use a serial interface to the PHY, called
anywhere in the DPAA SoC and PHY datasheets XFI. We're left with "xgmii"
compatible in the device tree because that's what was available at the
time in the kernel for 10G and because in the SoC XGMII is the actual
MII that connects the MAC to the SoC internal blocks that are part of
the physical layer. Because we have an internal PCS, the DPAA SoC to
external PHY connection is a PHY-layer internal connection, that has
no official denomination derived from a standard. In the industry, this
is called XFI. We are using the XFI electrical specification to connect
to the external Aquantia PHY and I intended to describe this in the device
tree, and add it to the known PHY connection types. The issue is, the
phy-connection-type device tree parameter was aliased at a certain
moment with phy-mode, which is something else, a PHY operation mode
configuration parameter (not sure this should belong in a device tree)
and now it is considered that only PHY modes should be allowed there.

I see a way forward by splitting the two device tree elements, as they
should have been in the first place. Now we're having issues with things
like RGMII internal delay, making the code overcomplicated when one
just wants to check for RGMII. Maybe we should have a parameter for the
system interface electrical connection (phy-connection-type), another
one for the operation mode (phy-mode) and a tolerant parsing function
that adapts for backward compatibility.

The existing device trees must be supported going forward, even if we
change them for the better. In this regard, the rejection of XGMII in
the PHY driver is forcing this backward compatibility to be moved
somewhere else (in the MAC driver), which is probably a better place
for it. I'm assuming this is the reason for the change in the PHY
driver, as otherwise I do not see any real issue with it treating
XGMII as XFI in the AQR code going further.

[1] https://patchwork.ozlabs.org/patch/1213515/

Regards,
Madalin

> > Reported-by: Mian Yousaf Kaukab <ykaukab@...e.de>
> > Signed-off-by: Madalin Bucur <madalin.bucur@....nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > index a301f0095223..d3eb235450e5 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > @@ -2471,9 +2471,13 @@ static int dpaa_phy_init(struct net_device
> *net_dev)
> >  		return -ENODEV;
> >  	}
> >
> > -	/* Remove any features not supported by the controller */
> > -	ethtool_convert_legacy_u32_to_link_mode(mask, mac_dev->if_support);
> > -	linkmode_and(phy_dev->supported, phy_dev->supported, mask);
> > +	if (mac_dev->phy_if != PHY_INTERFACE_MODE_XGMII ||
> > +	    !phy_dev->rate_adaptation) {
> > +		/* Remove any features not supported by the controller */
> > +		ethtool_convert_legacy_u32_to_link_mode(mask,
> > +							mac_dev->if_support);
> > +		linkmode_and(phy_dev->supported, phy_dev->supported, mask);
> > +	}
> >
> >  	phy_support_asym_pause(phy_dev);
> >
> >

Powered by blists - more mailing lists