| Message ID | Z4gdtOaGsBhQCZXn@shell.armlinux.org.uk (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: add phylink managed EEE support | expand |
On 1/15/2025 12:42 PM, Russell King (Oracle) wrote: > Hi, > > Adding managed EEE support to phylink has been on the cards ever since > the idea in phylib was mooted. This overly large series attempts to do > so. I've included all the patches as it's important to get the driver > patches out there. > > Patch 1 adds a definition for the clock stop capable bit in the PCS > MMD status register. > > Patch 2 adds a phylib API to query whether the PHY allows the transmit > xMII clock to be stopped while in LPI mode. This capability is for MAC > drivers to save power when LPI is active, to allow them to stop their > transmit clock. > > Patch 3 extracts a phylink internal helper for determining whether the > link is up. > > Patch 4 adds basic phylink managed EEE support. Two new MAC APIs are > added, to enable and disable LPI. The enable method is passed the LPI > timer setting which it is expected to program into the hardware, and > also a flag ehther the transmit clock should be stopped. > > I have taken the decision to make enable_tx_lpi() to return an error > code, but not do much with it other than report it - the intention > being that we can later use it to extend functionality if needed > without reworking loads of drivers. > > I have also dropped the validation/limitation of the LPI timer, and > left that in the driver code prior to calling phylink_ethtool_set_eee(). > > The remainder of the patches convert mvneta, lan743x and stmmac, and > add support for mvneta. > > Since yesterday's RFC: > - fixed the mvpp2 GENMASK() > - dropped the DSA patch > - changed how phylink restricts EEE advertisement, and the EEE support > reported to userspace which fixes a bug. > Everything in this series looks good to me. Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Hello: This series was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 15 Jan 2025 20:42:28 +0000 you wrote: > Hi, > > Adding managed EEE support to phylink has been on the cards ever since > the idea in phylib was mooted. This overly large series attempts to do > so. I've included all the patches as it's important to get the driver > patches out there. > > [...] Here is the summary with links: - [net-next,1/9] net: mdio: add definition for clock stop capable bit https://git.kernel.org/netdev/net-next/c/3ba0262a8fed - [net-next,2/9] net: phy: add support for querying PHY clock stop capability https://git.kernel.org/netdev/net-next/c/a00e0d34c036 - [net-next,3/9] net: phylink: add phylink_link_is_up() helper https://git.kernel.org/netdev/net-next/c/a17ceec62f81 - [net-next,4/9] net: phylink: add EEE management https://git.kernel.org/netdev/net-next/c/03abf2a7c654 - [net-next,5/9] net: mvneta: convert to phylink EEE implementation https://git.kernel.org/netdev/net-next/c/ac79927dc84f - [net-next,6/9] net: mvpp2: add EEE implementation https://git.kernel.org/netdev/net-next/c/b53b14786ed8 - [net-next,7/9] net: lan743x: use netdev in lan743x_phylink_mac_link_down() https://git.kernel.org/netdev/net-next/c/a66447966f03 - [net-next,8/9] net: lan743x: convert to phylink managed EEE https://git.kernel.org/netdev/net-next/c/bd691d5ca918 - [net-next,9/9] net: stmmac: convert to phylink managed EEE support https://git.kernel.org/netdev/net-next/c/4218647d4556 You are awesome, thank you!
> Hi, > > Adding managed EEE support to phylink has been on the cards ever since > the idea in phylib was mooted. This overly large series attempts to do > so. I've included all the patches as it's important to get the driver > patches out there. > > Patch 1 adds a definition for the clock stop capable bit in the PCS > MMD status register. > > Patch 2 adds a phylib API to query whether the PHY allows the transmit > xMII clock to be stopped while in LPI mode. This capability is for MAC > drivers to save power when LPI is active, to allow them to stop their > transmit clock. > > Patch 3 extracts a phylink internal helper for determining whether the > link is up. > > Patch 4 adds basic phylink managed EEE support. Two new MAC APIs are > added, to enable and disable LPI. The enable method is passed the LPI > timer setting which it is expected to program into the hardware, and > also a flag ehther the transmit clock should be stopped. > > I have taken the decision to make enable_tx_lpi() to return an error > code, but not do much with it other than report it - the intention > being that we can later use it to extend functionality if needed > without reworking loads of drivers. > > I have also dropped the validation/limitation of the LPI timer, and > left that in the driver code prior to calling phylink_ethtool_set_eee(). > > The remainder of the patches convert mvneta, lan743x and stmmac, and > add support for mvneta. > > Since yesterday's RFC: > - fixed the mvpp2 GENMASK() > - dropped the DSA patch > - changed how phylink restricts EEE advertisement, and the EEE support > reported to userspace which fixes a bug. > > drivers/net/ethernet/marvell/mvneta.c | 107 ++++++++++------ > drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 5 + > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 86 +++++++++++++ > drivers/net/ethernet/microchip/lan743x_ethtool.c | 21 --- > drivers/net/ethernet/microchip/lan743x_main.c | 46 ++++++- > drivers/net/ethernet/microchip/lan743x_main.h | 1 - > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 57 +++++++-- > drivers/net/phy/phy.c | 20 +++ > drivers/net/phy/phylink.c | 149 ++++++++++++++++++++-- > include/linux/phy.h | 1 + > include/linux/phylink.h | 45 +++++++ > include/uapi/linux/mdio.h | 1 + > 12 files changed, 446 insertions(+), 93 deletions(-) Hi Russell, Since merging these patches, phylink_connect_phy() can no longer be invoked correctly in ngbe_open(). The error is returned from the function phy_eee_rx_clock_stop(). Since EEE is not supported on our NGBE hardware. How should I modify the ngbe driver to meet this change? Thanks.
On Fri, Jan 17, 2025 at 04:56:34PM +0800, Jiawen Wu wrote: > > Hi, > > > > Adding managed EEE support to phylink has been on the cards ever since > > the idea in phylib was mooted. This overly large series attempts to do > > so. I've included all the patches as it's important to get the driver > > patches out there. > > > > Patch 1 adds a definition for the clock stop capable bit in the PCS > > MMD status register. > > > > Patch 2 adds a phylib API to query whether the PHY allows the transmit > > xMII clock to be stopped while in LPI mode. This capability is for MAC > > drivers to save power when LPI is active, to allow them to stop their > > transmit clock. > > > > Patch 3 extracts a phylink internal helper for determining whether the > > link is up. > > > > Patch 4 adds basic phylink managed EEE support. Two new MAC APIs are > > added, to enable and disable LPI. The enable method is passed the LPI > > timer setting which it is expected to program into the hardware, and > > also a flag ehther the transmit clock should be stopped. > > > > I have taken the decision to make enable_tx_lpi() to return an error > > code, but not do much with it other than report it - the intention > > being that we can later use it to extend functionality if needed > > without reworking loads of drivers. > > > > I have also dropped the validation/limitation of the LPI timer, and > > left that in the driver code prior to calling phylink_ethtool_set_eee(). > > > > The remainder of the patches convert mvneta, lan743x and stmmac, and > > add support for mvneta. > > > > Since yesterday's RFC: > > - fixed the mvpp2 GENMASK() > > - dropped the DSA patch > > - changed how phylink restricts EEE advertisement, and the EEE support > > reported to userspace which fixes a bug. > > > > drivers/net/ethernet/marvell/mvneta.c | 107 ++++++++++------ > > drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 5 + > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 86 +++++++++++++ > > drivers/net/ethernet/microchip/lan743x_ethtool.c | 21 --- > > drivers/net/ethernet/microchip/lan743x_main.c | 46 ++++++- > > drivers/net/ethernet/microchip/lan743x_main.h | 1 - > > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 57 +++++++-- > > drivers/net/phy/phy.c | 20 +++ > > drivers/net/phy/phylink.c | 149 ++++++++++++++++++++-- > > include/linux/phy.h | 1 + > > include/linux/phylink.h | 45 +++++++ > > include/uapi/linux/mdio.h | 1 + > > 12 files changed, 446 insertions(+), 93 deletions(-) > > Since merging these patches, phylink_connect_phy() can no longer be > invoked correctly in ngbe_open(). The error is returned from the function > phy_eee_rx_clock_stop(). Since EEE is not supported on our NGBE hardware. That would mean phy_modify_mmd() is failing, but the question is why that is. Please investigate. Thanks.
> > Since merging these patches, phylink_connect_phy() can no longer be > > invoked correctly in ngbe_open(). The error is returned from the function > > phy_eee_rx_clock_stop(). Since EEE is not supported on our NGBE hardware. > > That would mean phy_modify_mmd() is failing, but the question is why > that is. Please investigate. Thanks. Yes, phy_modify_mmd() returns -EOPNOTSUPP. Since .read/write_mmd are implemented in the PHY driver, but it's not supported to read/write the register field (devnum=MDIO_MMD_PCS, regnum= MDIO_CTRL1). So the error occurs on __phy_read_mmd(): if (phydev->drv && phydev->drv->read_mmd) return phydev->drv->read_mmd(phydev, devad, regnum);
On Fri, Jan 17, 2025 at 06:17:05PM +0800, Jiawen Wu wrote: > > > Since merging these patches, phylink_connect_phy() can no longer be > > > invoked correctly in ngbe_open(). The error is returned from the function > > > phy_eee_rx_clock_stop(). Since EEE is not supported on our NGBE hardware. > > > > That would mean phy_modify_mmd() is failing, but the question is why > > that is. Please investigate. Thanks. > > Yes, phy_modify_mmd() returns -EOPNOTSUPP. Since .read/write_mmd are > implemented in the PHY driver, but it's not supported to read/write the > register field (devnum=MDIO_MMD_PCS, regnum= MDIO_CTRL1). > > So the error occurs on __phy_read_mmd(): > if (phydev->drv && phydev->drv->read_mmd) > return phydev->drv->read_mmd(phydev, devad, regnum); Thanks. The patch below should fix it. Please test, meanwhile I'll prepare a proper patch. diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 66eea3f963d3..56d411bb2547 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -2268,7 +2268,11 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy, /* Explicitly configure whether the PHY is allowed to stop it's * receive clock. */ - return phy_eee_rx_clock_stop(phy, pl->config->eee_rx_clk_stop_enable); + ret = phy_eee_rx_clock_stop(phy, pl->config->eee_rx_clk_stop_enable); + if (ret == -EOPNOTSUPP) + ret = 0; + + return ret; } static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
On Fri, Jan 17, 2025 8:24 PM, Russell King (Oracle) wrote: > On Fri, Jan 17, 2025 at 06:17:05PM +0800, Jiawen Wu wrote: > > > > Since merging these patches, phylink_connect_phy() can no longer be > > > > invoked correctly in ngbe_open(). The error is returned from the function > > > > phy_eee_rx_clock_stop(). Since EEE is not supported on our NGBE hardware. > > > > > > That would mean phy_modify_mmd() is failing, but the question is why > > > that is. Please investigate. Thanks. > > > > Yes, phy_modify_mmd() returns -EOPNOTSUPP. Since .read/write_mmd are > > implemented in the PHY driver, but it's not supported to read/write the > > register field (devnum=MDIO_MMD_PCS, regnum= MDIO_CTRL1). > > > > So the error occurs on __phy_read_mmd(): > > if (phydev->drv && phydev->drv->read_mmd) > > return phydev->drv->read_mmd(phydev, devad, regnum); > > Thanks. The patch below should fix it. Please test, meanwhile I'll > prepare a proper patch. > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index 66eea3f963d3..56d411bb2547 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -2268,7 +2268,11 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy, > /* Explicitly configure whether the PHY is allowed to stop it's > * receive clock. > */ > - return phy_eee_rx_clock_stop(phy, pl->config->eee_rx_clk_stop_enable); > + ret = phy_eee_rx_clock_stop(phy, pl->config->eee_rx_clk_stop_enable); > + if (ret == -EOPNOTSUPP) > + ret = 0; > + > + return ret; > } > > static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy, Test pass. Thanks.
On Mon, Jan 20, 2025 at 09:51:29AM +0800, Jiawen Wu wrote: > On Fri, Jan 17, 2025 8:24 PM, Russell King (Oracle) wrote: > > On Fri, Jan 17, 2025 at 06:17:05PM +0800, Jiawen Wu wrote: > > > > > Since merging these patches, phylink_connect_phy() can no longer be > > > > > invoked correctly in ngbe_open(). The error is returned from the function > > > > > phy_eee_rx_clock_stop(). Since EEE is not supported on our NGBE hardware. > > > > > > > > That would mean phy_modify_mmd() is failing, but the question is why > > > > that is. Please investigate. Thanks. > > > > > > Yes, phy_modify_mmd() returns -EOPNOTSUPP. Since .read/write_mmd are > > > implemented in the PHY driver, but it's not supported to read/write the > > > register field (devnum=MDIO_MMD_PCS, regnum= MDIO_CTRL1). > > > > > > So the error occurs on __phy_read_mmd(): > > > if (phydev->drv && phydev->drv->read_mmd) > > > return phydev->drv->read_mmd(phydev, devad, regnum); > > > > Thanks. The patch below should fix it. Please test, meanwhile I'll > > prepare a proper patch. > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > > index 66eea3f963d3..56d411bb2547 100644 > > --- a/drivers/net/phy/phylink.c > > +++ b/drivers/net/phy/phylink.c > > @@ -2268,7 +2268,11 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy, > > /* Explicitly configure whether the PHY is allowed to stop it's > > * receive clock. > > */ > > - return phy_eee_rx_clock_stop(phy, pl->config->eee_rx_clk_stop_enable); > > + ret = phy_eee_rx_clock_stop(phy, pl->config->eee_rx_clk_stop_enable); > > + if (ret == -EOPNOTSUPP) > > + ret = 0; > > + > > + return ret; > > } > > > > static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy, > > Test pass. > Thanks. Thanks, I guess that's a tested-by then?
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > > > index 66eea3f963d3..56d411bb2547 100644 > > > --- a/drivers/net/phy/phylink.c > > > +++ b/drivers/net/phy/phylink.c > > > @@ -2268,7 +2268,11 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy, > > > /* Explicitly configure whether the PHY is allowed to stop it's > > > * receive clock. > > > */ > > > - return phy_eee_rx_clock_stop(phy, pl->config->eee_rx_clk_stop_enable); > > > + ret = phy_eee_rx_clock_stop(phy, pl->config->eee_rx_clk_stop_enable); > > > + if (ret == -EOPNOTSUPP) > > > + ret = 0; > > > + > > > + return ret; > > > } > > > > > > static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy, > > > > Test pass. > > Thanks. > > Thanks, I guess that's a tested-by then? Yes, for this patch, Tested-by: Jiawen Wu <jiawenwu@trustnetic.com>