Date:   Wed, 29 Jan 2020 10:53:03 +0000
From:   "Madalin Bucur (OSS)" <madalin.bucur@....nxp.com>
To:     Vladimir Oltean <olteanv@...il.com>,
        "Madalin Bucur (OSS)" <madalin.bucur@....nxp.com>
CC:     Florian Fainelli <f.fainelli@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "hkallweit1@...il.com" <hkallweit1@...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 1/2] net: phy: aquantia: add rate_adaptation
 indication

> -----Original Message-----
> From: Vladimir Oltean <olteanv@...il.com>
> Sent: Wednesday, January 29, 2020 12:15 PM
> To: Madalin Bucur (OSS) <madalin.bucur@....nxp.com>
> Cc: Florian Fainelli <f.fainelli@...il.com>; davem@...emloft.net;
> andrew@...n.ch; hkallweit1@...il.com; netdev@...r.kernel.org;
> ykaukab@...e.de; Russell King - ARM Linux admin <linux@...linux.org.uk>
> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
> indication
> 
> Hi Madalin,
> 
> On Wed, 29 Jan 2020 at 11:38, Madalin Bucur (OSS)
> <madalin.bucur@....nxp.com> wrote:
> >
> > > -----Original Message-----
> > > From: Vladimir Oltean <olteanv@...il.com>
> > > Sent: Tuesday, January 28, 2020 5:55 PM
> > > To: Florian Fainelli <f.fainelli@...il.com>
> > > Cc: Madalin Bucur (OSS) <madalin.bucur@....nxp.com>;
> davem@...emloft.net;
> > > andrew@...n.ch; hkallweit1@...il.com; netdev@...r.kernel.org;
> > > ykaukab@...e.de
> > > Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add
> rate_adaptation
> > > indication
> > >
> > > Hi Florian,
> > >
> > > On Mon, 27 Jan 2020 at 19:44, Florian Fainelli <f.fainelli@...il.com>
> > > wrote:
> > > >
> > > > On 1/22/20 11:38 PM, Madalin Bucur (OSS) wrote:
> > > > >> -----Original Message-----
> > > > >> From: Florian Fainelli <f.fainelli@...il.com>
> > > > >> Sent: Wednesday, January 22, 2020 7:58 PM
> > > > >> To: Madalin Bucur (OSS) <madalin.bucur@....nxp.com>;
> > > davem@...emloft.net
> > > > >> Cc: andrew@...n.ch; hkallweit1@...il.com;
> netdev@...r.kernel.org;
> > > > >> ykaukab@...e.de
> > > > >> Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add
> > > rate_adaptation
> > > > >> indication
> > > > >>
> > > > >> On 1/22/20 5:59 AM, Madalin Bucur wrote:
> > > > >>> The AQR PHYs are able to perform rate adaptation between
> > > > >>> the system interface and the line interfaces. When such
> > > > >>> a PHY is deployed, the ethernet driver should not limit
> > > > >>> the modes supported or advertised by the PHY. This patch
> > > > >>> introduces the bit that allows checking for this feature
> > > > >>> in the phy_device structure and its use for the Aquantia
> > > > >>> PHYs.
> > > > >>>
> > > > >>> Signed-off-by: Madalin Bucur <madalin.bucur@....nxp.com>
> > > > >>> ---
> > > > >>>  drivers/net/phy/aquantia_main.c | 3 +++
> > > > >>>  include/linux/phy.h             | 3 +++
> > > > >>>  2 files changed, 6 insertions(+)
> > > > >>>
> > > > >>> diff --git a/drivers/net/phy/aquantia_main.c
> > > > >> b/drivers/net/phy/aquantia_main.c
> > > > >>> index 975789d9349d..36fdd523b758 100644
> > > > >>> --- a/drivers/net/phy/aquantia_main.c
> > > > >>> +++ b/drivers/net/phy/aquantia_main.c
> > > > >>> @@ -209,6 +209,9 @@ static int aqr_config_aneg(struct
> phy_device
> > > > >> *phydev)
> > > > >>>     u16 reg;
> > > > >>>     int ret;
> > > > >>>
> > > > >>> +   /* add here as this is called for all devices */
> > > > >>> +   phydev->rate_adaptation = 1;
> > > > >>
> > > > >> How about introducing a new PHY_SUPPORTS_RATE_ADAPTATION flag
> and
> > > you
> > > > >> set that directly from the phy_driver entry? using the "flags"
> > > bitmask
> > > > >> instead of adding another structure member to phy_device?
> > > > >
> > > > > I've looked at the phydev->dev_flags use, it seemed to me that
> mostly
> > > it
> > > > > is used to convey configuration options towards the PHY.
> > > >
> > > > You read me incorrectly, I am suggesting using the
> phy_driver::flags
> > > > member, not the phy_device::dev_flags entry, please re-consider
> your
> > > > position.
> > > >
> > >
> > > Whether the PHY performs rate adaptation is a dynamic property.
> > > It will perform it at wire speeds lower than 2500Mbps (1000/100) when
> > > system side is 2500Base-X, but not at wire speed 2500 & 2500Base-X,
> > > and not at wire speed 1000 & USXGMII.
> > > You can't really encode that in phydev->flags.
> >
> > Vladimir, the patch adds a bit that indicates the PHY ability to
> perform
> > rate adaptation, not whether it is actually in use in a certain
> combination
> > of system interface and line interface modes. Please review the
> submission
> 
> As far as I understand, for Aquantia devices this is a 3-way switch for:
> - No rate adaptation
> - USX rate adaptation
> - Pause rate adaptation.
> So what does your "phydev->rate_adaptation = 1" assignment mean?

phydev->rate_adaptation = 1 means the PHY is able to perform rate adaptation

There is not such thing as USX, if you refer to USXGMII, that's something you
can check for in the interface mode.

> From context and datasheet I deduced that you mean "the PHY is
> configured to send PAUSE frames for 10GBase-R and 2500Base-X modes",
> which are probably the modes in which you're using it.
> 
> But how do you _know_ that the PHY has this switch set correctly? It
> is either set by firmware or by MDIO, but I don't see any of that
> being checked for.

I know it is set because someone does put some work in designing a system,
in provisioning a correct firmware image.

> > again, I understand you have something slightly different in mind, but this
> > is just addressing a basic need of knowing whether there is a chance the
> > line side could work at other speeds than the system interface and allow it
> > to do so.
> 
> I do understand this works, when it works, _for your board_, but is it
> generic enough that other systems can work with this simple setting?
> What if somebody else reads "phydev->rate_adaptation" to mean "USX
> adaptation"?

That person would be wrong, as there is a dedicated interface mode for USXGMII
(I assume that's what you're trying to refer to here).

> >
> > > I was actually searching for a way to encode some more PHY
> capabilities:
> > > - Does it work with in-band autoneg enabled?
> > > - Does it work with in-band autoneg bypassed?
> > > - Does it emit pause frames? <- Madalin got ahead of me on this one.
> > >
> > > For the first 2, I want a mechanism for the PHY library to figure out
> > > whether the MAC and the PHY should use in-band autoneg or not. If
> both
> > > support in-band autoneg, that would be preferred. Otherwise,
> > > out-of-band (MDIO) is preferred, and in-band autoneg is explicitly
> > > disabled in both, if that is claimed to be supported. Otherwise,
> > > report error to user. Yes, this deprecates "managed =
> > > 'in-band-status'" in the device tree, and that's for (what I think
> is)
> > > the better. We can make "managed = 'in-band-status'" to just clear
> the
> > > MAC's ability to support the "in-band autoneg bypassed" mode.
> > >
> > > So I thought a function pointer in the phy driver would be better,
> and
> > > more extensible.
> > > Thoughts?
> >
> > This is where you get when you try to implement anti-stupid devices, it
> gets
> > complicated fast and, most often, it gets in the way. Should someone
> change
> > a setting (pause settings) and experience adverse effects (excessive
> frame
> > loss), we should rely on his analytical abilities to connect the dots,
> imho.
> >
> 
> So you think that having a PHY firmware with rate adaptation disabled
> is "stupid user"?
> What if the rate adaptation feature is not enabled in firmware, but is
> being enabled by U-Boot, but the user had the generic PHY driver
> instead of the Aquantia one, or used a different or old bootloader?
> "Stupid user"?

Disabling rate adaptation is one of so many ways one could cripple a
system interface, there are many that require polarity inversion, lane
switching, etc. and still there is no support for that in the kernel.

> The PHY and the Ethernet driver are strongly decoupled, so they need
> to agree on an operating mode that will work for both of them.
> Ideally the PHY would really know how it's configured, not just
> hardcode "yeah, I can do rate adaptation, try me".
> The fact that you can build sanity checks on top (like in this case
> not allowing the user to disable pause frames in the Ethernet driver
> on RX), that don't stand in the way, is just proof that the system is
> well designed. If you can't build sanity checks, or if they stand in
> the way, it isn't.

If you don't need them, it's even better.

> If anything, why haven't you considered the opposite logic here:
> - Ethernet driver (dpaa_eth) supports all speeds 10G and below.
> - PHY driver (or PHY core) removes the supported and advertised speeds
> for anything other than 2.5G and 10G if it can't do this rate
> adaptation thing, and its system side isn't USXGMII. It's not like
> this is dpaa_eth specific in any way.

"what about...", indeed. There are many situations one could imagine
when things would not work, we can do some brain storming on improving
this list. Meanwhile, a real issue is that in the current design, the
ethernet driver needs to remove modes that are invalid from the PHY
advertising, but needs to refrain from doing that when coupled with
a PHY that does rate adaptation. This bit provides just an indication
of that ability.

> > Madalin
> >
> >
> > > > --
> > > > Florian
> > >
> > > Regards,
> > > -Vladimir
> 
> -Vladimir

Powered by blists - more mailing lists