| Message ID | E1q7Y9W-00DI8m-Jo@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | phylink EEE support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | fail | Errors and warnings before: 10 this patch: 11 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | fail | Errors and warnings before: 8 this patch: 10 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 10 this patch: 11 |
| netdev/checkpatch | fail | ERROR: Macros with complex values should be enclosed in parentheses |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, Jun 09, 2023 at 10:11:26AM +0100, Russell King (Oracle) wrote: > Convert mvneta to use phylink's EEE implementation, which means we just > need to implement the two methods for LPI control, and adding the > initial configuration. > > Disabling LPI requires clearing a single bit. Enabling LPI needs a full > configuration of several values, as the timer values are dependent on > the MAC operating speed. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/net/ethernet/marvell/mvneta.c | 95 +++++++++++++++++---------- > 1 file changed, 61 insertions(+), 34 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > index e2abc00d0472..c634ec5d3f9a 100644 > --- a/drivers/net/ethernet/marvell/mvneta.c > +++ b/drivers/net/ethernet/marvell/mvneta.c > @@ -284,8 +284,10 @@ > MVNETA_TXQ_BUCKET_REFILL_PERIOD)) > > #define MVNETA_LPI_CTRL_0 0x2cc0 > +#define MVNETA_LPI_CTRL_0_TS 0xff << 8 Hi Russell, maybe GENMASK would be useful here. If not, perhaps (0xffUL << 8) > #define MVNETA_LPI_CTRL_1 0x2cc4 > #define MVNETA_LPI_REQUEST_ENABLE BIT(0) > +#define MVNETA_LPI_CTRL_1_TW 0xfff << 4 Likewise here. > #define MVNETA_LPI_CTRL_2 0x2cc8 > #define MVNETA_LPI_STATUS 0x2ccc > > @@ -541,10 +543,6 @@ struct mvneta_port { > struct mvneta_bm_pool *pool_short; > int bm_win_id; > > - bool eee_enabled; > - bool eee_active; > - bool tx_lpi_enabled; > - > u64 ethtool_stats[ARRAY_SIZE(mvneta_statistics)]; > > u32 indir[MVNETA_RSS_LU_TABLE_SIZE]; > @@ -4232,9 +4230,6 @@ static void mvneta_mac_link_down(struct phylink_config *config, > val |= MVNETA_GMAC_FORCE_LINK_DOWN; > mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val); > } > - > - pp->eee_active = false; > - mvneta_set_eee(pp, false); > } mvneta_set_eee() seems unused after this patch. Perhaps it can be removed? ...
On Fri, Jun 09, 2023 at 04:02:09PM +0200, Simon Horman wrote: > On Fri, Jun 09, 2023 at 10:11:26AM +0100, Russell King (Oracle) wrote: > > Convert mvneta to use phylink's EEE implementation, which means we just > > need to implement the two methods for LPI control, and adding the > > initial configuration. > > > > Disabling LPI requires clearing a single bit. Enabling LPI needs a full > > configuration of several values, as the timer values are dependent on > > the MAC operating speed. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > drivers/net/ethernet/marvell/mvneta.c | 95 +++++++++++++++++---------- > > 1 file changed, 61 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > > index e2abc00d0472..c634ec5d3f9a 100644 > > --- a/drivers/net/ethernet/marvell/mvneta.c > > +++ b/drivers/net/ethernet/marvell/mvneta.c > > @@ -284,8 +284,10 @@ > > MVNETA_TXQ_BUCKET_REFILL_PERIOD)) > > > > #define MVNETA_LPI_CTRL_0 0x2cc0 > > +#define MVNETA_LPI_CTRL_0_TS 0xff << 8 > > Hi Russell, > > maybe GENMASK would be useful here. If not, perhaps (0xffUL << 8) Why "unsigned long" when the variable we use it with is u32, which is defined as "unsigned int" ?
On Fri, Jun 09, 2023 at 03:31:28PM +0100, Russell King (Oracle) wrote: > On Fri, Jun 09, 2023 at 04:02:09PM +0200, Simon Horman wrote: > > On Fri, Jun 09, 2023 at 10:11:26AM +0100, Russell King (Oracle) wrote: > > > Convert mvneta to use phylink's EEE implementation, which means we just > > > need to implement the two methods for LPI control, and adding the > > > initial configuration. > > > > > > Disabling LPI requires clearing a single bit. Enabling LPI needs a full > > > configuration of several values, as the timer values are dependent on > > > the MAC operating speed. > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > --- > > > drivers/net/ethernet/marvell/mvneta.c | 95 +++++++++++++++++---------- > > > 1 file changed, 61 insertions(+), 34 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > > > index e2abc00d0472..c634ec5d3f9a 100644 > > > --- a/drivers/net/ethernet/marvell/mvneta.c > > > +++ b/drivers/net/ethernet/marvell/mvneta.c > > > @@ -284,8 +284,10 @@ > > > MVNETA_TXQ_BUCKET_REFILL_PERIOD)) > > > > > > #define MVNETA_LPI_CTRL_0 0x2cc0 > > > +#define MVNETA_LPI_CTRL_0_TS 0xff << 8 > > > > Hi Russell, > > > > maybe GENMASK would be useful here. If not, perhaps (0xffUL << 8) > > Why "unsigned long" when the variable we use it with is u32, which is > defined as "unsigned int" ? Fair point. What I actually wanted to ask is if the logic that uses this works without parentheses. Probably I'm missing something obvious and it does. But it looked odd to me.
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index e2abc00d0472..c634ec5d3f9a 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -284,8 +284,10 @@ MVNETA_TXQ_BUCKET_REFILL_PERIOD)) #define MVNETA_LPI_CTRL_0 0x2cc0 +#define MVNETA_LPI_CTRL_0_TS 0xff << 8 #define MVNETA_LPI_CTRL_1 0x2cc4 #define MVNETA_LPI_REQUEST_ENABLE BIT(0) +#define MVNETA_LPI_CTRL_1_TW 0xfff << 4 #define MVNETA_LPI_CTRL_2 0x2cc8 #define MVNETA_LPI_STATUS 0x2ccc @@ -541,10 +543,6 @@ struct mvneta_port { struct mvneta_bm_pool *pool_short; int bm_win_id; - bool eee_enabled; - bool eee_active; - bool tx_lpi_enabled; - u64 ethtool_stats[ARRAY_SIZE(mvneta_statistics)]; u32 indir[MVNETA_RSS_LU_TABLE_SIZE]; @@ -4232,9 +4230,6 @@ static void mvneta_mac_link_down(struct phylink_config *config, val |= MVNETA_GMAC_FORCE_LINK_DOWN; mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val); } - - pp->eee_active = false; - mvneta_set_eee(pp, false); } static void mvneta_mac_link_up(struct phylink_config *config, @@ -4283,11 +4278,57 @@ static void mvneta_mac_link_up(struct phylink_config *config, } mvneta_port_up(pp); +} + +static void mvneta_mac_disable_tx_lpi(struct phylink_config *config) +{ + struct mvneta_port *pp = netdev_priv(to_net_dev(config->dev)); + u32 lpi1; + + lpi1 = mvreg_read(pp, MVNETA_LPI_CTRL_1); + lpi1 &= ~MVNETA_LPI_REQUEST_ENABLE; + mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi1); +} - if (phy && pp->eee_enabled) { - pp->eee_active = phy_init_eee(phy, false) >= 0; - mvneta_set_eee(pp, pp->eee_active && pp->tx_lpi_enabled); +static void mvneta_mac_enable_tx_lpi(struct phylink_config *config, u32 timer) +{ + struct mvneta_port *pp = netdev_priv(to_net_dev(config->dev)); + u32 ts, tw, lpi0, lpi1, status; + + status = mvreg_read(pp, MVNETA_GMAC_STATUS); + + if (status & MVNETA_GMAC_SPEED_1000) { + /* At 1G speeds, the timer resolution are 1us, and + * 802.3 says tw is 16.5us. Round up to 17us. + */ + tw = 17; + ts = timer; + } else { + /* At 100M speeds, the timer resolutions are 10us, and + * 802.3 says tw is 30us. + */ + tw = 3; + ts = DIV_ROUND_UP(timer, 10); } + + if (ts > 255) + ts = 255; + + /* Ensure LPI generation is disabled */ + lpi1 = mvreg_read(pp, MVNETA_LPI_CTRL_1); + mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi1 & ~MVNETA_LPI_REQUEST_ENABLE); + + /* Configure ts */ + lpi0 = mvreg_read(pp, MVNETA_LPI_CTRL_0) & ~MVNETA_LPI_CTRL_0_TS; + lpi0 |= FIELD_PREP(MVNETA_LPI_CTRL_0_TS, ts); + mvreg_write(pp, MVNETA_LPI_CTRL_0, lpi0); + + /* Configure tw */ + lpi1 &= ~MVNETA_LPI_CTRL_1_TW; + lpi1 |= FIELD_PREP(MVNETA_LPI_CTRL_1_TW, tw); + + /* Enable LPI generation */ + mvreg_write(pp, MVNETA_LPI_CTRL_1, lpi1 | MVNETA_LPI_REQUEST_ENABLE); } static const struct phylink_mac_ops mvneta_phylink_ops = { @@ -4297,6 +4338,8 @@ static const struct phylink_mac_ops mvneta_phylink_ops = { .mac_finish = mvneta_mac_finish, .mac_link_down = mvneta_mac_link_down, .mac_link_up = mvneta_mac_link_up, + .mac_disable_tx_lpi = mvneta_mac_disable_tx_lpi, + .mac_enable_tx_lpi = mvneta_mac_enable_tx_lpi, }; static int mvneta_mdio_probe(struct mvneta_port *pp) @@ -5087,14 +5130,6 @@ static int mvneta_ethtool_get_eee(struct net_device *dev, struct ethtool_eee *eee) { struct mvneta_port *pp = netdev_priv(dev); - u32 lpi_ctl0; - - lpi_ctl0 = mvreg_read(pp, MVNETA_LPI_CTRL_0); - - eee->eee_enabled = pp->eee_enabled; - eee->eee_active = pp->eee_active; - eee->tx_lpi_enabled = pp->tx_lpi_enabled; - eee->tx_lpi_timer = (lpi_ctl0) >> 8; // * scale; return phylink_ethtool_get_eee(pp->phylink, eee); } @@ -5103,23 +5138,10 @@ static int mvneta_ethtool_set_eee(struct net_device *dev, struct ethtool_eee *eee) { struct mvneta_port *pp = netdev_priv(dev); - u32 lpi_ctl0; - /* The Armada 37x documents do not give limits for this other than - * it being an 8-bit register. - */ - if (eee->tx_lpi_enabled && eee->tx_lpi_timer > 255) - return -EINVAL; - - lpi_ctl0 = mvreg_read(pp, MVNETA_LPI_CTRL_0); - lpi_ctl0 &= ~(0xff << 8); - lpi_ctl0 |= eee->tx_lpi_timer << 8; - mvreg_write(pp, MVNETA_LPI_CTRL_0, lpi_ctl0); - - pp->eee_enabled = eee->eee_enabled; - pp->tx_lpi_enabled = eee->tx_lpi_enabled; - - mvneta_set_eee(pp, eee->tx_lpi_enabled && eee->eee_enabled); + /* clamp tx_lpi_timer */ + if (eee->tx_lpi_timer > 255) + eee->tx_lpi_timer = 255; return phylink_ethtool_set_eee(pp->phylink, eee); } @@ -5550,6 +5572,11 @@ static int mvneta_probe(struct platform_device *pdev) pp->phylink_config.supported_interfaces); } + /* Setup EEE. Choose 250us idle. */ + pp->phylink_config.eee.eee_enabled = true; + pp->phylink_config.eee.tx_lpi_enabled = true; + pp->phylink_config.eee.tx_lpi_timer = 250; + phylink = phylink_create(&pp->phylink_config, pdev->dev.fwnode, phy_mode, &mvneta_phylink_ops); if (IS_ERR(phylink)) {
Convert mvneta to use phylink's EEE implementation, which means we just need to implement the two methods for LPI control, and adding the initial configuration. Disabling LPI requires clearing a single bit. Enabling LPI needs a full configuration of several values, as the timer values are dependent on the MAC operating speed. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/ethernet/marvell/mvneta.c | 95 +++++++++++++++++---------- 1 file changed, 61 insertions(+), 34 deletions(-)