| Message ID | 20230628124326.55732-1-ruppala@nvidia.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [1/4] net: phy: aquantia: Enable Tx/Rx pause frame support in aquantia PHY | expand |
On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote: > From: Narayan Reddy <narayanr@nvidia.com> > > Enable flow control support using pause frames in aquantia phy driver. > > Signed-off-by: Narayan Reddy <narayanr@nvidia.com> > Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com> I think this is over-complex. > #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK GENMASK(7, 3) > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR 0 > @@ -583,6 +585,17 @@ static int aqr107_config_init(struct phy_device *phydev) > if (!ret) > aqr107_chip_info(phydev); > > + /* Advertize flow control */ > + linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported); > + linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported); > + linkmode_copy(phydev->advertising, phydev->supported); This is the wrong place to be doing this, since pause support depends not only on the PHY but also on the MAC. There are phylib interfaces that MACs should call so that phylib knows that the MAC supports pause frames. Secondly, the PHY driver needs to tell phylib that the PHY supports pause frames, and that's done through either setting the .features member in the PHY driver, or by providing a .get_features implementation. Configuration of the pause advertisement should already be happening through the core phylib code. Thanks.
Final comments on the series, posted here because there's no cover message. 1. The series should include a cover message summarising the overall purpose of the series is, giving an overview of the changes, and a diffstat for the whole series. 2. The subject line should contain the tree that the patches are targetting, e.g. "[PATCH net-next 0/4] Add support for WoL to Aquantia PHY driver". 3. The tree should be identified in each patch mailed out. 4. The patches should be threaded to the cover message. Thanks.
On Wed, Jun 28, 2023 at 02:30:48PM +0100, Russell King (Oracle) wrote: > On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote: > > From: Narayan Reddy <narayanr@nvidia.com> > > > > Enable flow control support using pause frames in aquantia phy driver. > > > > Signed-off-by: Narayan Reddy <narayanr@nvidia.com> > > Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com> > > I think this is over-complex. > > > #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 > > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK GENMASK(7, 3) > > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR 0 > > @@ -583,6 +585,17 @@ static int aqr107_config_init(struct phy_device *phydev) > > if (!ret) > > aqr107_chip_info(phydev); > > > > + /* Advertize flow control */ > > + linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported); > > + linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported); > > + linkmode_copy(phydev->advertising, phydev->supported); > > This is the wrong place to be doing this, since pause support depends > not only on the PHY but also on the MAC. There are phylib interfaces > that MACs should call so that phylib knows that the MAC supports pause > frames. > > Secondly, the PHY driver needs to tell phylib that the PHY supports > pause frames, and that's done through either setting the .features > member in the PHY driver, or by providing a .get_features > implementation. > > Configuration of the pause advertisement should already be happening > through the core phylib code. I really should do a LPC netdev talk "Everybody gets pause wrong..." genphy_c45_an_config_aneg() will configure pause advertisement. The PHY driver does not need to configure it, if the PHY follows the standard and has the configuration in the correct place. As Russell said, please check the PHYs ability to advertise pause is being reported correctly, by .get_features, of the default implementation of .get_features if that is being used. And then check your MAC driver is also indicating it supports pause. Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Wednesday, June 28, 2023 7:17 PM > To: Russell King (Oracle) <linux@armlinux.org.uk> > Cc: Revanth Kumar Uppala <ruppala@nvidia.com>; hkallweit1@gmail.com; > netdev@vger.kernel.org; linux-tegra@vger.kernel.org; Narayan Reddy > <narayanr@nvidia.com> > Subject: Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support > in aquantia PHY > > External email: Use caution opening links or attachments > > > On Wed, Jun 28, 2023 at 02:30:48PM +0100, Russell King (Oracle) wrote: > > On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote: > > > From: Narayan Reddy <narayanr@nvidia.com> > > > > > > Enable flow control support using pause frames in aquantia phy driver. > > > > > > Signed-off-by: Narayan Reddy <narayanr@nvidia.com> > > > Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com> > > > > I think this is over-complex. > > > > > #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 > > > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK GENMASK(7, 3) > > > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR 0 @@ -583,6 +585,17 > @@ > > > static int aqr107_config_init(struct phy_device *phydev) > > > if (!ret) > > > aqr107_chip_info(phydev); > > > > > > + /* Advertize flow control */ > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev- > >supported); > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev- > >supported); > > > + linkmode_copy(phydev->advertising, phydev->supported); > > > > This is the wrong place to be doing this, since pause support depends > > not only on the PHY but also on the MAC. There are phylib interfaces > > that MACs should call so that phylib knows that the MAC supports pause > > frames. > > > > Secondly, the PHY driver needs to tell phylib that the PHY supports > > pause frames, and that's done through either setting the .features > > member in the PHY driver, or by providing a .get_features > > implementation. > > > > Configuration of the pause advertisement should already be happening > > through the core phylib code. > > I really should do a LPC netdev talk "Everybody gets pause wrong..." > > genphy_c45_an_config_aneg() will configure pause advertisement. The PHY > driver does not need to configure it, if the PHY follows the standard and has the > configuration in the correct place. As Russell said, please check the PHYs ability > to advertise pause is being reported correctly, by .get_features, of the default > implementation of .get_features if that is being used. And then check your MAC > driver is also indicating it supports pause. From .get_features, it is not possible to check PHY's ability to advertise pause is being reported as there is no such register present for AQR PHY to check capabilities in its datasheet. Hence, we are directly configuring the pause frames from aqr107_config_init(). Thanks, Revanth Uppala > > Andrew
On Mon, Jul 24, 2023 at 11:29:18AM +0000, Revanth Kumar Uppala wrote: > > > > -----Original Message----- > > From: Andrew Lunn <andrew@lunn.ch> > > Sent: Wednesday, June 28, 2023 7:17 PM > > To: Russell King (Oracle) <linux@armlinux.org.uk> > > Cc: Revanth Kumar Uppala <ruppala@nvidia.com>; hkallweit1@gmail.com; > > netdev@vger.kernel.org; linux-tegra@vger.kernel.org; Narayan Reddy > > <narayanr@nvidia.com> > > Subject: Re: [PATCH 1/4] net: phy: aquantia: Enable Tx/Rx pause frame support > > in aquantia PHY > > > > External email: Use caution opening links or attachments > > > > > > On Wed, Jun 28, 2023 at 02:30:48PM +0100, Russell King (Oracle) wrote: > > > On Wed, Jun 28, 2023 at 06:13:23PM +0530, Revanth Kumar Uppala wrote: > > > > From: Narayan Reddy <narayanr@nvidia.com> > > > > > > > > Enable flow control support using pause frames in aquantia phy driver. > > > > > > > > Signed-off-by: Narayan Reddy <narayanr@nvidia.com> > > > > Signed-off-by: Revanth Kumar Uppala <ruppala@nvidia.com> > > > > > > I think this is over-complex. > > > > > > > #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 > > > > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK GENMASK(7, 3) > > > > #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR 0 @@ -583,6 +585,17 > > @@ > > > > static int aqr107_config_init(struct phy_device *phydev) > > > > if (!ret) > > > > aqr107_chip_info(phydev); > > > > > > > > + /* Advertize flow control */ > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev- > > >supported); > > > > + linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev- > > >supported); > > > > + linkmode_copy(phydev->advertising, phydev->supported); > > > > > > This is the wrong place to be doing this, since pause support depends > > > not only on the PHY but also on the MAC. There are phylib interfaces > > > that MACs should call so that phylib knows that the MAC supports pause > > > frames. > > > > > > Secondly, the PHY driver needs to tell phylib that the PHY supports > > > pause frames, and that's done through either setting the .features > > > member in the PHY driver, or by providing a .get_features > > > implementation. > > > > > > Configuration of the pause advertisement should already be happening > > > through the core phylib code. > > > > I really should do a LPC netdev talk "Everybody gets pause wrong..." > > > > genphy_c45_an_config_aneg() will configure pause advertisement. The PHY > > driver does not need to configure it, if the PHY follows the standard and has the > > configuration in the correct place. As Russell said, please check the PHYs ability > > to advertise pause is being reported correctly, by .get_features, of the default > > implementation of .get_features if that is being used. And then check your MAC > > driver is also indicating it supports pause. > From .get_features, it is not possible to check PHY's ability to advertise pause is being reported as there is no such register present for AQR PHY to check capabilities in its datasheet. > Hence, we are directly configuring the pause frames from aqr107_config_init(). ... and thus creating a trashy implementation... so NAK. The first thing to get straight is that in "normal" non-rate adapting setups, pause frames are no different from normal Ethernet frames as far as a PHY is concerned. The PHY should be doing absolutely nothing special with pause frames - it should merely forward them to the MAC, and it's the MAC that deals with pause frames. The only thing that a non-rate adapting baseT PHY has to deal with is the media advertisement and nothing else. So, whether pause frames are supported has more to do with the MAC than the PHY. The way phylib works is that when a PHY is probed, phy_probe() will set the Pause and Asym_Pause bits in the ->supported bitmap. It is then up to the MAC driver (or phylink) to call phy_support_asym_pause() or phy_support_sym_pause() to tell phylib what the MAC supports. PHY drivers must *not* override this in their config_init() function, and most certainly must not copy the supported bitmap to the advertising bitmap there either. If you need pause-mode rate adaption, this has nothing to do with the media side, and ->supported and ->advertising are not relevant for that. Non-phylink based MAC drivers have to use phy_get_rate_matching() to find out whether the PHY will be using rate adaption and act accordingly. phylink based MAC drivers have this dealt with for them and as long as they do what phylink tells them to do, everything should be fine. So, basically, do not mess with setting bits in the ->supported bitmap nor the ->advertising bitmap in config_init. It is wrong, and we will NAK it. Thanks.
diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c index 334a6904ca5a..c99b9d066463 100644 --- a/drivers/net/phy/aquantia_main.c +++ b/drivers/net/phy/aquantia_main.c @@ -26,6 +26,8 @@ #define PHY_ID_AQR412 0x03a1b712 #define PHY_ID_AQR113C 0x31c31c12 +#define MDIO_AN_ADVT 0x0010 + #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_MASK GENMASK(7, 3) #define MDIO_PHYXS_VEND_IF_STATUS_TYPE_KR 0 @@ -583,6 +585,17 @@ static int aqr107_config_init(struct phy_device *phydev) if (!ret) aqr107_chip_info(phydev); + /* Advertize flow control */ + linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported); + linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported); + linkmode_copy(phydev->advertising, phydev->supported); + + /* Configure flow control */ + ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_ADVT, ADVERTISE_PAUSE_CAP | + ADVERTISE_PAUSE_ASYM); + if (ret < 0) + return ret; + return aqr107_set_downshift(phydev, MDIO_AN_VEND_PROV_DOWNSHIFT_DFLT); }