From: "Madalin Bucur (OSS)" <madalin.bucur@oss.nxp.com>
To: Vladimir Oltean <olteanv@gmail.com>,
Florian Fainelli <f.fainelli@gmail.com>
Cc: "Madalin Bucur (OSS)" <madalin.bucur@oss.nxp.com>,
"davem@davemloft.net" <davem@davemloft.net>,
"andrew@lunn.ch" <andrew@lunn.ch>,
"hkallweit1@gmail.com" <hkallweit1@gmail.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"ykaukab@suse.de" <ykaukab@suse.de>
Subject: RE: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication
Date: Wed, 29 Jan 2020 09:38:06 +0000 [thread overview]
Message-ID: <DB8PR04MB6985139D4ABED85B701445A9EC050@DB8PR04MB6985.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <CA+h21hq7U_EtetuLZN5rjXcq+cRUoD0y_76LxuHpoC53J70CEQ@mail.gmail.com>
> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Tuesday, January 28, 2020 5:55 PM
> To: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>; davem@davemloft.net;
> andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
> ykaukab@suse.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@gmail.com>
> wrote:
> >
> > On 1/22/20 11:38 PM, Madalin Bucur (OSS) wrote:
> > >> -----Original Message-----
> > >> From: Florian Fainelli <f.fainelli@gmail.com>
> > >> Sent: Wednesday, January 22, 2020 7:58 PM
> > >> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>;
> davem@davemloft.net
> > >> Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
> > >> ykaukab@suse.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@oss.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
next prev parent reply other threads:[~2020-01-29 9:39 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-22 13:59 [PATCH net-next 0/2] net: phy: aquantia: indicate rate adaptation Madalin Bucur
2020-01-22 13:59 ` [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation indication Madalin Bucur
2020-01-22 17:58 ` Florian Fainelli
2020-01-23 7:38 ` Madalin Bucur (OSS)
2020-01-27 17:42 ` Florian Fainelli
2020-01-28 7:02 ` Madalin Bucur (OSS)
2020-01-28 15:55 ` Vladimir Oltean
2020-01-29 9:38 ` Madalin Bucur (OSS) [this message]
2020-01-29 10:15 ` Vladimir Oltean
2020-01-29 10:53 ` Madalin Bucur (OSS)
2020-01-29 10:57 ` Russell King - ARM Linux admin
2020-01-29 10:59 ` Madalin Bucur (OSS)
2020-01-29 11:57 ` Vladimir Oltean
2020-01-29 12:47 ` Madalin Bucur
2020-01-29 13:44 ` Andrew Lunn
2020-01-29 14:36 ` Madalin Bucur (OSS)
2020-01-29 15:33 ` Andrew Lunn
2020-01-30 9:40 ` Madalin Bucur (OSS)
2020-01-22 13:59 ` [PATCH net-next 2/2] dpaa_eth: support all modes with rate adapting PHYs Madalin Bucur
2020-01-22 20:05 ` Heiner Kallweit
2020-01-23 7:20 ` Madalin Bucur (OSS)
2020-01-29 13:34 ` Russell King - ARM Linux admin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DB8PR04MB6985139D4ABED85B701445A9EC050@DB8PR04MB6985.eurprd04.prod.outlook.com \
--to=madalin.bucur@oss.nxp.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=hkallweit1@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=ykaukab@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).