Date:   Wed, 29 Jan 2020 09:38:06 +0000
From:   "Madalin Bucur (OSS)" <madalin.bucur@....nxp.com>
To:     Vladimir Oltean <olteanv@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>
CC:     "Madalin Bucur (OSS)" <madalin.bucur@....nxp.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>
Subject: RE: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
 indication

> -----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
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 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.

Madalin

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

Powered by blists - more mailing lists