| Message ID | E1tKzVS-006c67-IJ@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] net: fec: use phydev->eee_cfg.tx_lpi_timer | expand |
On Tue, Dec 10, 2024 at 12:38:26PM +0000, Russell King (Oracle) wrote: > Rather than maintaining a private copy of the LPI timer, make use of > the LPI timer maintained by phylib. In any case, phylib overwrites the > value of tx_lpi_timer set by the driver in phy_ethtool_get_eee(). > > Note that feb->eee.tx_lpi_timer is initialised to zero, which is just > the same with phylib's copy, so there should be no functional change. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Note that this need testing on compatible hardware - I only have iMX6 which doesn't have EEE support in FEC. I'm particularly interested in any change of output from # ethtool --show-eee $if with/without this patch. Also testing that it doesn't cause any regression. Thanks. > --- > drivers/net/ethernet/freescale/fec.h | 2 -- > drivers/net/ethernet/freescale/fec_main.c | 16 ++++++---------- > 2 files changed, 6 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h > index 1cca0425d493..c81f2ea588f2 100644 > --- a/drivers/net/ethernet/freescale/fec.h > +++ b/drivers/net/ethernet/freescale/fec.h > @@ -671,8 +671,6 @@ struct fec_enet_private { > unsigned int tx_time_itr; > unsigned int itr_clk_rate; > > - /* tx lpi eee mode */ > - struct ethtool_keee eee; > unsigned int clk_ref_rate; > > /* ptp clock period in ns*/ > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 1b55047c0237..b2daed55bf6c 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -2045,14 +2045,14 @@ static int fec_enet_us_to_tx_cycle(struct net_device *ndev, int us) > return us * (fep->clk_ref_rate / 1000) / 1000; > } > > -static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable) > +static int fec_enet_eee_mode_set(struct net_device *ndev, u32 lpi_timer, > + bool enable) > { > struct fec_enet_private *fep = netdev_priv(ndev); > - struct ethtool_keee *p = &fep->eee; > unsigned int sleep_cycle, wake_cycle; > > if (enable) { > - sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer); > + sleep_cycle = fec_enet_us_to_tx_cycle(ndev, lpi_timer); > wake_cycle = sleep_cycle; > } else { > sleep_cycle = 0; > @@ -2105,7 +2105,9 @@ static void fec_enet_adjust_link(struct net_device *ndev) > napi_enable(&fep->napi); > } > if (fep->quirks & FEC_QUIRK_HAS_EEE) > - fec_enet_eee_mode_set(ndev, phy_dev->enable_tx_lpi); > + fec_enet_eee_mode_set(ndev, > + phy_dev->eee_cfg.tx_lpi_timer, > + phy_dev->enable_tx_lpi); > } else { > if (fep->link) { > netif_stop_queue(ndev); > @@ -3181,7 +3183,6 @@ static int > fec_enet_get_eee(struct net_device *ndev, struct ethtool_keee *edata) > { > struct fec_enet_private *fep = netdev_priv(ndev); > - struct ethtool_keee *p = &fep->eee; > > if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) > return -EOPNOTSUPP; > @@ -3189,8 +3190,6 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_keee *edata) > if (!netif_running(ndev)) > return -ENETDOWN; > > - edata->tx_lpi_timer = p->tx_lpi_timer; > - > return phy_ethtool_get_eee(ndev->phydev, edata); > } > > @@ -3198,7 +3197,6 @@ static int > fec_enet_set_eee(struct net_device *ndev, struct ethtool_keee *edata) > { > struct fec_enet_private *fep = netdev_priv(ndev); > - struct ethtool_keee *p = &fep->eee; > > if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) > return -EOPNOTSUPP; > @@ -3206,8 +3204,6 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_keee *edata) > if (!netif_running(ndev)) > return -ENETDOWN; > > - p->tx_lpi_timer = edata->tx_lpi_timer; > - > return phy_ethtool_set_eee(ndev->phydev, edata); > } > > -- > 2.30.2 > >
On Tue, Dec 10, 2024 at 12:39:53PM +0000, Russell King (Oracle) wrote: > On Tue, Dec 10, 2024 at 12:38:26PM +0000, Russell King (Oracle) wrote: > > Rather than maintaining a private copy of the LPI timer, make use of > > the LPI timer maintained by phylib. In any case, phylib overwrites the > > value of tx_lpi_timer set by the driver in phy_ethtool_get_eee(). > > > > Note that feb->eee.tx_lpi_timer is initialised to zero, which is just > > the same with phylib's copy, so there should be no functional change. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > Note that this need testing on compatible hardware - I only have iMX6 > which doesn't have EEE support in FEC. The FEC hardware i have does not have EEE either. Sorry. Andrew
> On Tue, Dec 10, 2024 at 12:38:26PM +0000, Russell King (Oracle) wrote: > > Rather than maintaining a private copy of the LPI timer, make use of > > the LPI timer maintained by phylib. In any case, phylib overwrites the > > value of tx_lpi_timer set by the driver in phy_ethtool_get_eee(). > > > > Note that feb->eee.tx_lpi_timer is initialised to zero, which is just > > the same with phylib's copy, so there should be no functional change. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > Note that this need testing on compatible hardware - I only have iMX6 which > doesn't have EEE support in FEC. > > I'm particularly interested in any change of output from > > # ethtool --show-eee $if > > with/without this patch. Also testing that it doesn't cause any regression. > > Thanks. > Hi Russell, There are no changes after applying this patch. Before: root@imx8mqevk:~# ethtool --show-eee eth0 EEE settings for eth0: EEE status: enabled - active Tx LPI: 0 (us) Supported EEE link modes: 100baseT/Full 1000baseT/Full Advertised EEE link modes: 100baseT/Full 1000baseT/Full Link partner advertised EEE link modes: 100baseT/Full 1000baseT/Full root@imx8mqevk:~# ethtool --set-eee eth0 eee on tx-lpi on tx-timer 5000 root@imx8mqevk:~# ethtool --show-eee eth0 EEE settings for eth0: EEE status: enabled - active Tx LPI: 5000 (us) Supported EEE link modes: 100baseT/Full 1000baseT/Full Advertised EEE link modes: 100baseT/Full 1000baseT/Full Link partner advertised EEE link modes: 100baseT/Full 1000baseT/Full After applying the patch: root@imx8mqevk:~# ethtool --show-eee eth0 EEE settings for eth0: EEE status: enabled - active Tx LPI: 0 (us) Supported EEE link modes: 100baseT/Full 1000baseT/Full Advertised EEE link modes: 100baseT/Full 1000baseT/Full Link partner advertised EEE link modes: 100baseT/Full 1000baseT/Full root@imx8mqevk:~# ethtool --set-eee eth0 eee on tx-lpi on tx-timer 5000 root@imx8mqevk:~# ethtool --show-eee eth0 EEE settings for eth0: EEE status: enabled - active Tx LPI: 5000 (us) Supported EEE link modes: 100baseT/Full 1000baseT/Full Advertised EEE link modes: 100baseT/Full 1000baseT/Full Link partner advertised EEE link modes: 100baseT/Full 1000baseT/Full So, Tested-by: Wei Fang <wei.fang@nxp.com>
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 10 Dec 2024 12:38:26 +0000 you wrote: > Rather than maintaining a private copy of the LPI timer, make use of > the LPI timer maintained by phylib. In any case, phylib overwrites the > value of tx_lpi_timer set by the driver in phy_ethtool_get_eee(). > > Note that feb->eee.tx_lpi_timer is initialised to zero, which is just > the same with phylib's copy, so there should be no functional change. > > [...] Here is the summary with links: - [net-next] net: fec: use phydev->eee_cfg.tx_lpi_timer https://git.kernel.org/netdev/net-next/c/3fa2540d93d8 You are awesome, thank you!
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h index 1cca0425d493..c81f2ea588f2 100644 --- a/drivers/net/ethernet/freescale/fec.h +++ b/drivers/net/ethernet/freescale/fec.h @@ -671,8 +671,6 @@ struct fec_enet_private { unsigned int tx_time_itr; unsigned int itr_clk_rate; - /* tx lpi eee mode */ - struct ethtool_keee eee; unsigned int clk_ref_rate; /* ptp clock period in ns*/ diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 1b55047c0237..b2daed55bf6c 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -2045,14 +2045,14 @@ static int fec_enet_us_to_tx_cycle(struct net_device *ndev, int us) return us * (fep->clk_ref_rate / 1000) / 1000; } -static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable) +static int fec_enet_eee_mode_set(struct net_device *ndev, u32 lpi_timer, + bool enable) { struct fec_enet_private *fep = netdev_priv(ndev); - struct ethtool_keee *p = &fep->eee; unsigned int sleep_cycle, wake_cycle; if (enable) { - sleep_cycle = fec_enet_us_to_tx_cycle(ndev, p->tx_lpi_timer); + sleep_cycle = fec_enet_us_to_tx_cycle(ndev, lpi_timer); wake_cycle = sleep_cycle; } else { sleep_cycle = 0; @@ -2105,7 +2105,9 @@ static void fec_enet_adjust_link(struct net_device *ndev) napi_enable(&fep->napi); } if (fep->quirks & FEC_QUIRK_HAS_EEE) - fec_enet_eee_mode_set(ndev, phy_dev->enable_tx_lpi); + fec_enet_eee_mode_set(ndev, + phy_dev->eee_cfg.tx_lpi_timer, + phy_dev->enable_tx_lpi); } else { if (fep->link) { netif_stop_queue(ndev); @@ -3181,7 +3183,6 @@ static int fec_enet_get_eee(struct net_device *ndev, struct ethtool_keee *edata) { struct fec_enet_private *fep = netdev_priv(ndev); - struct ethtool_keee *p = &fep->eee; if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) return -EOPNOTSUPP; @@ -3189,8 +3190,6 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_keee *edata) if (!netif_running(ndev)) return -ENETDOWN; - edata->tx_lpi_timer = p->tx_lpi_timer; - return phy_ethtool_get_eee(ndev->phydev, edata); } @@ -3198,7 +3197,6 @@ static int fec_enet_set_eee(struct net_device *ndev, struct ethtool_keee *edata) { struct fec_enet_private *fep = netdev_priv(ndev); - struct ethtool_keee *p = &fep->eee; if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) return -EOPNOTSUPP; @@ -3206,8 +3204,6 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_keee *edata) if (!netif_running(ndev)) return -ENETDOWN; - p->tx_lpi_timer = edata->tx_lpi_timer; - return phy_ethtool_set_eee(ndev->phydev, edata); }
Rather than maintaining a private copy of the LPI timer, make use of the LPI timer maintained by phylib. In any case, phylib overwrites the value of tx_lpi_timer set by the driver in phy_ethtool_get_eee(). Note that feb->eee.tx_lpi_timer is initialised to zero, which is just the same with phylib's copy, so there should be no functional change. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/ethernet/freescale/fec.h | 2 -- drivers/net/ethernet/freescale/fec_main.c | 16 ++++++---------- 2 files changed, 6 insertions(+), 12 deletions(-)