[PATCH net-next v2 0/4] net: add EEE support for KSZ9477 switch series

[PATCH net-next v2 0/4] net: add EEE support for KSZ9477 switch series

[Oleksij Rempel]

changes v2:
- use phydev->supported instead of reading MII_BMSR regiaster
- fix @get_eee > @set_eee

With this patch series we provide EEE support for KSZ9477 family of switches.
According to my tests, on a system with KSZ8563 switch and 100Mbit idle link,
we consume 0,192W less power per port if EEE is enabled.

Oleksij Rempel (4):
  net: phy: add driver specific get/set_eee support
  net: phy: micrel: add EEE configuration support for KSZ9477 variants
    of PHYs
  net: phy: micrel: disable 1000Mbit EEE support if 1000Mbit is not
    supported
  net: dsa: microchip: enable EEE support

 drivers/net/dsa/microchip/ksz_common.c |  35 +++++++++
 drivers/net/phy/micrel.c               | 102 ++++++++++++++++++++++++-
 drivers/net/phy/phy.c                  |   6 ++
 include/linux/phy.h                    |   5 ++
 4 files changed, 147 insertions(+), 1 deletion(-)

-- 
2.30.2

[PATCH net-next v2 1/4] net: phy: add driver specific get/set_eee support

[Oleksij Rempel]

Not all PHYs can be handled by generic phy_ethtool_get/set_eee()
functions. So, add driver specific get/set_eee support.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/phy.c | 6 ++++++
 include/linux/phy.h   | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 3378ca4f49b6..57e125a99414 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1583,6 +1583,9 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_eee *data)
 	if (!phydev->drv)
 		return -EIO;
 
+	if (phydev->drv->get_eee)
+		return phydev->drv->get_eee(phydev, data);
+
 	/* Get Supported EEE */
 	val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE);
 	if (val < 0)
@@ -1622,6 +1625,9 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
 	if (!phydev->drv)
 		return -EIO;
 
+	if (phydev->drv->set_eee)
+		return phydev->drv->set_eee(phydev, data);
+
 	/* Get Supported EEE */
 	cap = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE);
 	if (cap < 0)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index b3cf1e08e880..9ad6c3f5c865 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1055,6 +1055,11 @@ struct phy_driver {
 	/** @get_plca_status: Return the current PLCA status info */
 	int (*get_plca_status)(struct phy_device *dev,
 			       struct phy_plca_status *plca_st);
+
+	/** @get_eee: Return the current EEE configuration */
+	int (*get_eee)(struct phy_device *phydev, struct ethtool_eee *e);
+	/** @set_eee: Set the EEE configuration */
+	int (*set_eee)(struct phy_device *phydev, struct ethtool_eee *e);
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
-- 
2.30.2

[PATCH net-next v2 2/4] net: phy: micrel: add EEE configuration support for KSZ9477 variants of PHYs

[Oleksij Rempel]

KSZ9477 variants of PHYs are not completely compatible with generic
phy_ethtool_get/set_eee() handlers. For example MDIO_PCS_EEE_ABLE acts
like a mirror of MDIO_AN_EEE_ADV register. If MDIO_AN_EEE_ADV set to 0,
MDIO_PCS_EEE_ABLE will be 0 too. It means, if we do
"ethtool --set-eee lan2 eee off", we won't be able to enable it again.

With this patch, instead of reading MDIO_PCS_EEE_ABLE register, the
driver will provide proper abilities.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/micrel.c | 81 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index d5b80c31ab91..dca61a73c144 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1370,6 +1370,85 @@ static int ksz9131_config_aneg(struct phy_device *phydev)
 	return genphy_config_aneg(phydev);
 }
 
+static void ksz9477_get_eee_caps(struct phy_device *phydev,
+				 struct ethtool_eee *data)
+{
+	/* At least on KSZ8563 (which has same PHY_ID as KSZ9477), the
+	 * MDIO_PCS_EEE_ABLE register is a mirror of MDIO_AN_EEE_ADV register.
+	 * So, we need to provide this information by driver.
+	 */
+	data->supported = SUPPORTED_100baseT_Full;
+
+	/* KSZ8563 is able to advertise not supported MDIO_EEE_1000T.
+	 * We need to test if the PHY is 1Gbit capable.
+	 */
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+			      phydev->supported))
+		data->supported |= SUPPORTED_1000baseT_Full;
+}
+
+static int ksz9477_get_eee(struct phy_device *phydev, struct ethtool_eee *data)
+{
+	int val;
+
+	ksz9477_get_eee_caps(phydev, data);
+
+	/* Get advertisement EEE */
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV);
+	if (val < 0)
+		return val;
+	data->advertised = mmd_eee_adv_to_ethtool_adv_t(val);
+	data->eee_enabled = !!data->advertised;
+
+	/* Get LP advertisement EEE */
+	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_LPABLE);
+	if (val < 0)
+		return val;
+	data->lp_advertised = mmd_eee_adv_to_ethtool_adv_t(val);
+
+	data->eee_active = !!(data->advertised & data->lp_advertised);
+
+	return 0;
+}
+
+static int ksz9477_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
+{
+	int old_adv, adv = 0, ret;
+
+	ksz9477_get_eee_caps(phydev, data);
+
+	old_adv = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV);
+	if (old_adv < 0)
+		return old_adv;
+
+	if (data->eee_enabled) {
+		if (!data->advertised)
+			adv = ethtool_adv_to_mmd_eee_adv_t(data->supported);
+		else
+			adv = ethtool_adv_to_mmd_eee_adv_t(data->advertised &
+							   data->supported);
+		/* Mask prohibited EEE modes */
+		adv &= ~phydev->eee_broken_modes;
+	}
+
+	if (old_adv != adv) {
+		ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, adv);
+		if (ret < 0)
+			return ret;
+
+		/* Restart autonegotiation so the new modes get sent to the
+		 * link partner.
+		 */
+		if (phydev->autoneg == AUTONEG_ENABLE) {
+			ret = phy_restart_aneg(phydev);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
 #define KSZ8873MLL_GLOBAL_CONTROL_4	0x06
 #define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX	BIT(6)
 #define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED	BIT(4)
@@ -3422,6 +3501,8 @@ static struct phy_driver ksphy_driver[] = {
 	.handle_interrupt = kszphy_handle_interrupt,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
+	.get_eee	= ksz9477_get_eee,
+	.set_eee	= ksz9477_set_eee,
 } };
 
 module_phy_driver(ksphy_driver);
-- 
2.30.2

Re: [PATCH net-next v2 2/4] net: phy: micrel: add EEE configuration support for KSZ9477 variants of PHYs

[Arun.Ramadoss]

Hi Oleksij,

On Fri, 2023-01-20 at 10:20 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> KSZ9477 variants of PHYs are not completely compatible with generic
> phy_ethtool_get/set_eee() handlers. For example MDIO_PCS_EEE_ABLE
> acts
> like a mirror of MDIO_AN_EEE_ADV register. If MDIO_AN_EEE_ADV set to
> 0,
> MDIO_PCS_EEE_ABLE will be 0 too. It means, if we do
> "ethtool --set-eee lan2 eee off", we won't be able to enable it
> again.
> 
> With this patch, instead of reading MDIO_PCS_EEE_ABLE register, the
> driver will provide proper abilities.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/phy/micrel.c | 81
> ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index d5b80c31ab91..dca61a73c144 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -1370,6 +1370,85 @@ static int ksz9131_config_aneg(struct
> phy_device *phydev)
>         return genphy_config_aneg(phydev);
>  }
> 
> +static int ksz9477_set_eee(struct phy_device *phydev, struct
> ethtool_eee *data)
> +{
> +       int old_adv, adv = 0, ret;

nit: you can consider declaring variable in two lines, one with
initialized and other with uninitialized.
int old_adv, ret;
int adv = 0;

> +
> +       ksz9477_get_eee_caps(phydev, data);
> +
> +       old_adv = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV);
> +       if (old_adv < 0)
> +               return old_adv;
> +
> +       if (data->eee_enabled) {
> +               if (!data->advertised)
> +                       adv = ethtool_adv_to_mmd_eee_adv_t(data-
> >supported);
> +               else
> +                       adv = ethtool_adv_to_mmd_eee_adv_t(data-
> >advertised &
> +                                                          data-
> >supported);
> +               /* Mask prohibited EEE modes */
> +               adv &= ~phydev->eee_broken_modes;
> +       }
> +
> +       if (old_adv != adv) {
> +               ret = phy_write_mmd(phydev, MDIO_MMD_AN,
> MDIO_AN_EEE_ADV, adv);
> +               if (ret < 0)
> +                       return ret;
> +
> +               /* Restart autonegotiation so the new modes get sent
> to the
> +                * link partner.
> +                */
> +               if (phydev->autoneg == AUTONEG_ENABLE) {
> +                       ret = phy_restart_aneg(phydev);
> +                       if (ret < 0)
> +                               return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> 
> 

[PATCH net-next v2 3/4] net: phy: micrel: disable 1000Mbit EEE support if 1000Mbit is not supported

[Oleksij Rempel]

KSZ8563 is announcing by default 1000Mbit EEE support, but at same time
do not supporting 1000Mbit speed.

This patch will disable 1000Mbit EEE advertisement if the PHY is not
1000Mbit capable.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/micrel.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index dca61a73c144..30fed309250e 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1449,6 +1449,25 @@ static int ksz9477_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
 	return 0;
 }
 
+static int ksz9477_config_init(struct phy_device *phydev)
+{
+	int ret;
+
+	/* KSZ8563 is able to advertise not supported MDIO_EEE_1000T.
+	 * We need to test if the PHY is 1Gbit capable and
+	 * clear MDIO_EEE_1000T if needed.
+	 */
+	if (!linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+			       phydev->supported)) {
+		ret = phy_clear_bits_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV,
+					 MDIO_EEE_1000T);
+		if (ret)
+			return ret;
+	}
+
+	return kszphy_config_init(phydev);
+}
+
 #define KSZ8873MLL_GLOBAL_CONTROL_4	0x06
 #define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX	BIT(6)
 #define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED	BIT(4)
@@ -3496,7 +3515,7 @@ static struct phy_driver ksphy_driver[] = {
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Microchip KSZ9477",
 	/* PHY_GBIT_FEATURES */
-	.config_init	= kszphy_config_init,
+	.config_init	= ksz9477_config_init,
 	.config_intr	= kszphy_config_intr,
 	.handle_interrupt = kszphy_handle_interrupt,
 	.suspend	= genphy_suspend,
-- 
2.30.2

[PATCH net-next v2 4/4] net: dsa: microchip: enable EEE support

[Oleksij Rempel]

Some of KSZ9477 family switches provides EEE support. To enable it, we
just need to register set_mac_eee/set_mac_eee handlers and validate
supported chip version and port.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz_common.c | 35 ++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 5e1e5bd555d2..2f1f71b3be86 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2645,6 +2645,39 @@ static int ksz_max_mtu(struct dsa_switch *ds, int port)
 	return -EOPNOTSUPP;
 }
 
+static int ksz_validate_eee(struct dsa_switch *ds, int port)
+{
+	struct ksz_device *dev = ds->priv;
+
+	if (!dev->info->internal_phy[port])
+		return -EOPNOTSUPP;
+
+	switch (dev->chip_id) {
+	case KSZ8563_CHIP_ID:
+	case KSZ9477_CHIP_ID:
+	case KSZ9563_CHIP_ID:
+	case KSZ9567_CHIP_ID:
+	case KSZ9893_CHIP_ID:
+	case KSZ9896_CHIP_ID:
+	case KSZ9897_CHIP_ID:
+		return 0;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int ksz_get_mac_eee(struct dsa_switch *ds, int port,
+			   struct ethtool_eee *e)
+{
+	return ksz_validate_eee(ds, port);
+}
+
+static int ksz_set_mac_eee(struct dsa_switch *ds, int port,
+			   struct ethtool_eee *e)
+{
+	return ksz_validate_eee(ds, port);
+}
+
 static void ksz_set_xmii(struct ksz_device *dev, int port,
 			 phy_interface_t interface)
 {
@@ -3006,6 +3039,8 @@ static const struct dsa_switch_ops ksz_switch_ops = {
 	.port_hwtstamp_set	= ksz_hwtstamp_set,
 	.port_txtstamp		= ksz_port_txtstamp,
 	.port_rxtstamp		= ksz_port_rxtstamp,
+	.get_mac_eee		= ksz_get_mac_eee,
+	.set_mac_eee		= ksz_set_mac_eee,
 };
 
 struct ksz_device *ksz_switch_alloc(struct device *base, void *priv)
-- 
2.30.2

Re: [PATCH net-next v2 4/4] net: dsa: microchip: enable EEE support

[Arun.Ramadoss]

Hi Oleksij,

On Fri, 2023-01-20 at 10:20 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Some of KSZ9477 family switches provides EEE support. To enable it,
> we
> just need to register set_mac_eee/set_mac_eee handlers and validate
> supported chip version and port.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 35
> ++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c
> index 5e1e5bd555d2..2f1f71b3be86 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -2645,6 +2645,39 @@ static int ksz_max_mtu(struct dsa_switch *ds,
> int port)
>         return -EOPNOTSUPP;
>  }
> 
> +static int ksz_validate_eee(struct dsa_switch *ds, int port)
> +{
> +       struct ksz_device *dev = ds->priv;
> +
> +       if (!dev->info->internal_phy[port])
> +               return -EOPNOTSUPP;
> +
> +       switch (dev->chip_id) {
> +       case KSZ8563_CHIP_ID:
> +       case KSZ9477_CHIP_ID:
> +       case KSZ9563_CHIP_ID:
> +       case KSZ9567_CHIP_ID:
> +       case KSZ9893_CHIP_ID:
> +       case KSZ9896_CHIP_ID:
> +       case KSZ9897_CHIP_ID:
> +               return 0;
> +       }
> +
> +       return -EOPNOTSUPP;
> +}
> +
> +static int ksz_get_mac_eee(struct dsa_switch *ds, int port,
> +                          struct ethtool_eee *e)
> +{
> +       return ksz_validate_eee(ds, port);
> +}
> +
> +static int ksz_set_mac_eee(struct dsa_switch *ds, int port,
> +                          struct ethtool_eee *e)
> +{
> +       return ksz_validate_eee(ds, port);
> +}
> +

nit: As both set and get function perform the same operation, we can
assign .get_mac_eee and .set_mac_eee to ksz_validate_eee function by
changing its prototype. In future, if any things to be added specific
to get or set, we can separate it to two function.

>  static void ksz_set_xmii(struct ksz_device *dev, int port,
>                          phy_interface_t interface)
>  {
> @@ -3006,6 +3039,8 @@ static const struct dsa_switch_ops
> ksz_switch_ops = {
>         .port_hwtstamp_set      = ksz_hwtstamp_set,
>         .port_txtstamp          = ksz_port_txtstamp,
>         .port_rxtstamp          = ksz_port_rxtstamp,
> +       .get_mac_eee            = ksz_get_mac_eee,
> +       .set_mac_eee            = ksz_set_mac_eee,
>  };
> 
>  struct ksz_device *ksz_switch_alloc(struct device *base, void *priv)
> --
> 2.30.2
>