* [PATCH net-next v1 0/4] net: add EEE support for KSZ9477 switch series
@ 2023-01-19 13:18 Oleksij Rempel
2023-01-19 13:18 ` [PATCH net-next v1 1/4] net: phy: add driver specific get/set_eee support Oleksij Rempel
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Oleksij Rempel @ 2023-01-19 13:18 UTC (permalink / raw)
To: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, Arun.Ramadoss
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 | 115 ++++++++++++++++++++++++-
drivers/net/phy/phy.c | 6 ++
include/linux/phy.h | 5 ++
4 files changed, 160 insertions(+), 1 deletion(-)
--
2.30.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next v1 1/4] net: phy: add driver specific get/set_eee support
2023-01-19 13:18 [PATCH net-next v1 0/4] net: add EEE support for KSZ9477 switch series Oleksij Rempel
@ 2023-01-19 13:18 ` Oleksij Rempel
2023-01-20 3:53 ` Jakub Kicinski
2023-01-19 13:18 ` [PATCH net-next v1 2/4] net: phy: micrel: add EEE configuration support for KSZ9477 variants of PHYs Oleksij Rempel
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Oleksij Rempel @ 2023-01-19 13:18 UTC (permalink / raw)
To: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, Arun.Ramadoss
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..39ee2628643c 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);
+ /** @get_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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net-next v1 2/4] net: phy: micrel: add EEE configuration support for KSZ9477 variants of PHYs
2023-01-19 13:18 [PATCH net-next v1 0/4] net: add EEE support for KSZ9477 switch series Oleksij Rempel
2023-01-19 13:18 ` [PATCH net-next v1 1/4] net: phy: add driver specific get/set_eee support Oleksij Rempel
@ 2023-01-19 13:18 ` Oleksij Rempel
2023-01-19 14:08 ` Andrew Lunn
2023-01-19 19:25 ` Florian Fainelli
2023-01-19 13:18 ` [PATCH net-next v1 3/4] net: phy: micrel: disable 1000Mbit EEE support if 1000Mbit is not supported Oleksij Rempel
2023-01-19 13:18 ` [PATCH net-next v1 4/4] net: dsa: microchip: enable EEE support Oleksij Rempel
3 siblings, 2 replies; 12+ messages in thread
From: Oleksij Rempel @ 2023-01-19 13:18 UTC (permalink / raw)
To: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, Arun.Ramadoss
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 | 92 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 92 insertions(+)
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index d5b80c31ab91..099f1e83c08c 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1370,6 +1370,96 @@ static int ksz9131_config_aneg(struct phy_device *phydev)
return genphy_config_aneg(phydev);
}
+static int ksz9477_get_eee_caps(struct phy_device *phydev,
+ struct ethtool_eee *data)
+{
+ int val;
+
+ /* 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.
+ */
+ val = phy_read(phydev, MII_BMSR);
+ if (val < 0)
+ return val;
+
+ if (val & BMSR_ERCAP)
+ data->supported |= SUPPORTED_1000baseT_Full;
+
+ return 0;
+}
+
+static int ksz9477_get_eee(struct phy_device *phydev, struct ethtool_eee *data)
+{
+ int val;
+
+ val = ksz9477_get_eee_caps(phydev, data);
+ if (val)
+ return val;
+
+ /* 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;
+
+ ret = ksz9477_get_eee_caps(phydev, data);
+ if (ret)
+ return ret;
+
+ 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 +3512,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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net-next v1 3/4] net: phy: micrel: disable 1000Mbit EEE support if 1000Mbit is not supported
2023-01-19 13:18 [PATCH net-next v1 0/4] net: add EEE support for KSZ9477 switch series Oleksij Rempel
2023-01-19 13:18 ` [PATCH net-next v1 1/4] net: phy: add driver specific get/set_eee support Oleksij Rempel
2023-01-19 13:18 ` [PATCH net-next v1 2/4] net: phy: micrel: add EEE configuration support for KSZ9477 variants of PHYs Oleksij Rempel
@ 2023-01-19 13:18 ` Oleksij Rempel
2023-01-19 13:18 ` [PATCH net-next v1 4/4] net: dsa: microchip: enable EEE support Oleksij Rempel
3 siblings, 0 replies; 12+ messages in thread
From: Oleksij Rempel @ 2023-01-19 13:18 UTC (permalink / raw)
To: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, Arun.Ramadoss
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 | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 099f1e83c08c..11bef217b45a 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1460,6 +1460,27 @@ 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.
+ */
+ ret = phy_read(phydev, MII_BMSR);
+ if (ret < 0)
+ return ret;
+
+ if (!(ret & BMSR_ERCAP)) {
+ 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)
@@ -3507,7 +3528,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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH net-next v1 4/4] net: dsa: microchip: enable EEE support
2023-01-19 13:18 [PATCH net-next v1 0/4] net: add EEE support for KSZ9477 switch series Oleksij Rempel
` (2 preceding siblings ...)
2023-01-19 13:18 ` [PATCH net-next v1 3/4] net: phy: micrel: disable 1000Mbit EEE support if 1000Mbit is not supported Oleksij Rempel
@ 2023-01-19 13:18 ` Oleksij Rempel
3 siblings, 0 replies; 12+ messages in thread
From: Oleksij Rempel @ 2023-01-19 13:18 UTC (permalink / raw)
To: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, Arun.Ramadoss
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v1 2/4] net: phy: micrel: add EEE configuration support for KSZ9477 variants of PHYs
2023-01-19 13:18 ` [PATCH net-next v1 2/4] net: phy: micrel: add EEE configuration support for KSZ9477 variants of PHYs Oleksij Rempel
@ 2023-01-19 14:08 ` Andrew Lunn
2023-01-20 5:50 ` Oleksij Rempel
2023-01-19 19:25 ` Florian Fainelli
1 sibling, 1 reply; 12+ messages in thread
From: Andrew Lunn @ 2023-01-19 14:08 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Woojung Huh, UNGLinuxDriver, Vivien Didelot, Florian Fainelli,
Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, kernel, linux-kernel, netdev, Arun.Ramadoss
> +static int ksz9477_get_eee_caps(struct phy_device *phydev,
> + struct ethtool_eee *data)
> +{
> + int val;
> +
> + /* 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.
> + */
> + val = phy_read(phydev, MII_BMSR);
> + if (val < 0)
> + return val;
> +
> + if (val & BMSR_ERCAP)
> + data->supported |= SUPPORTED_1000baseT_Full;
This works, but you could also look at phydev->supported and see if
one of the 1G modes is listed. That should be faster, since there is
no MDIO transaction involved. Not that this is on any sort of hot
path.
Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v1 2/4] net: phy: micrel: add EEE configuration support for KSZ9477 variants of PHYs
2023-01-19 13:18 ` [PATCH net-next v1 2/4] net: phy: micrel: add EEE configuration support for KSZ9477 variants of PHYs Oleksij Rempel
2023-01-19 14:08 ` Andrew Lunn
@ 2023-01-19 19:25 ` Florian Fainelli
2023-01-20 5:55 ` Oleksij Rempel
1 sibling, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2023-01-19 19:25 UTC (permalink / raw)
To: Oleksij Rempel, Woojung Huh, UNGLinuxDriver, Andrew Lunn,
Vivien Didelot, Vladimir Oltean, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: kernel, linux-kernel, netdev, Arun.Ramadoss
On 1/19/23 05:18, Oleksij Rempel wrote:
> 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.
We have hooks in place already for PHY drivers with the form of the
read_mmd and write_mmd callbacks, did this somehow not work for you?
Below is an example:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d88fd1b546ff19c8040cfaea76bf16aed1c5a0bb
(here the register location is non-standard but the bit definitions
within that register are following the standard).
--
Florian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v1 1/4] net: phy: add driver specific get/set_eee support
2023-01-19 13:18 ` [PATCH net-next v1 1/4] net: phy: add driver specific get/set_eee support Oleksij Rempel
@ 2023-01-20 3:53 ` Jakub Kicinski
0 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2023-01-20 3:53 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
Paolo Abeni, kernel, linux-kernel, netdev, Arun.Ramadoss
On Thu, 19 Jan 2023 14:18:18 +0100 Oleksij Rempel wrote:
> + /** @get_eee: Set the EEE configuration */
set
> + int (*set_eee)(struct phy_device *phydev, struct ethtool_eee *e);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v1 2/4] net: phy: micrel: add EEE configuration support for KSZ9477 variants of PHYs
2023-01-19 14:08 ` Andrew Lunn
@ 2023-01-20 5:50 ` Oleksij Rempel
0 siblings, 0 replies; 12+ messages in thread
From: Oleksij Rempel @ 2023-01-20 5:50 UTC (permalink / raw)
To: Andrew Lunn
Cc: Woojung Huh, Arun.Ramadoss, Florian Fainelli, netdev,
linux-kernel, Vivien Didelot, UNGLinuxDriver, Eric Dumazet,
Paolo Abeni, kernel, Jakub Kicinski, Vladimir Oltean,
David S. Miller
On Thu, Jan 19, 2023 at 03:08:59PM +0100, Andrew Lunn wrote:
> > +static int ksz9477_get_eee_caps(struct phy_device *phydev,
> > + struct ethtool_eee *data)
> > +{
> > + int val;
> > +
> > + /* 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.
> > + */
> > + val = phy_read(phydev, MII_BMSR);
> > + if (val < 0)
> > + return val;
> > +
> > + if (val & BMSR_ERCAP)
> > + data->supported |= SUPPORTED_1000baseT_Full;
>
> This works, but you could also look at phydev->supported and see if
> one of the 1G modes is listed. That should be faster, since there is
> no MDIO transaction involved. Not that this is on any sort of hot
> path.
ack. Sounds good.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v1 2/4] net: phy: micrel: add EEE configuration support for KSZ9477 variants of PHYs
2023-01-19 19:25 ` Florian Fainelli
@ 2023-01-20 5:55 ` Oleksij Rempel
2023-01-20 17:58 ` Florian Fainelli
0 siblings, 1 reply; 12+ messages in thread
From: Oleksij Rempel @ 2023-01-20 5:55 UTC (permalink / raw)
To: Florian Fainelli
Cc: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vivien Didelot,
Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, kernel, linux-kernel, netdev, Arun.Ramadoss
On Thu, Jan 19, 2023 at 11:25:42AM -0800, Florian Fainelli wrote:
> On 1/19/23 05:18, Oleksij Rempel wrote:
> > 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.
>
> We have hooks in place already for PHY drivers with the form of the read_mmd
> and write_mmd callbacks, did this somehow not work for you?
>
> Below is an example:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d88fd1b546ff19c8040cfaea76bf16aed1c5a0bb
>
> (here the register location is non-standard but the bit definitions within
> that register are following the standard).
It will work for this PHY, but not allow to complete support for AR8035.
AR8035 provides support for "SmartEEE" where tx_lpi_enabled and
tx_lpi_timer are optionally handled by the PHY, not by MAC.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v1 2/4] net: phy: micrel: add EEE configuration support for KSZ9477 variants of PHYs
2023-01-20 5:55 ` Oleksij Rempel
@ 2023-01-20 17:58 ` Florian Fainelli
2023-01-21 6:34 ` Oleksij Rempel
0 siblings, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2023-01-20 17:58 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vivien Didelot,
Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, kernel, linux-kernel, netdev, Arun.Ramadoss
On 1/19/2023 9:55 PM, Oleksij Rempel wrote:
> On Thu, Jan 19, 2023 at 11:25:42AM -0800, Florian Fainelli wrote:
>> On 1/19/23 05:18, Oleksij Rempel wrote:
>>> 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.
>>
>> We have hooks in place already for PHY drivers with the form of the read_mmd
>> and write_mmd callbacks, did this somehow not work for you?
>>
>> Below is an example:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d88fd1b546ff19c8040cfaea76bf16aed1c5a0bb
>>
>> (here the register location is non-standard but the bit definitions within
>> that register are following the standard).
>
> It will work for this PHY, but not allow to complete support for AR8035.
> AR8035 provides support for "SmartEEE" where tx_lpi_enabled and
> tx_lpi_timer are optionally handled by the PHY, not by MAC.
Not sure I understand your reply here, this would appear to be a
limitation that exists regardless of the current API defined, does that
mean that you can make use of the phy_driver::{read,write}_mmd function
calls and you will make a v2 that uses them, or something else entirely?
--
Florian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v1 2/4] net: phy: micrel: add EEE configuration support for KSZ9477 variants of PHYs
2023-01-20 17:58 ` Florian Fainelli
@ 2023-01-21 6:34 ` Oleksij Rempel
0 siblings, 0 replies; 12+ messages in thread
From: Oleksij Rempel @ 2023-01-21 6:34 UTC (permalink / raw)
To: Florian Fainelli
Cc: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vivien Didelot,
Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, kernel, linux-kernel, netdev, Arun.Ramadoss
On Fri, Jan 20, 2023 at 09:58:05AM -0800, Florian Fainelli wrote:
>
>
> On 1/19/2023 9:55 PM, Oleksij Rempel wrote:
> > On Thu, Jan 19, 2023 at 11:25:42AM -0800, Florian Fainelli wrote:
> > > On 1/19/23 05:18, Oleksij Rempel wrote:
> > > > 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.
> > >
> > > We have hooks in place already for PHY drivers with the form of the read_mmd
> > > and write_mmd callbacks, did this somehow not work for you?
> > >
> > > Below is an example:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d88fd1b546ff19c8040cfaea76bf16aed1c5a0bb
> > >
> > > (here the register location is non-standard but the bit definitions within
> > > that register are following the standard).
> >
> > It will work for this PHY, but not allow to complete support for AR8035.
> > AR8035 provides support for "SmartEEE" where tx_lpi_enabled and
> > tx_lpi_timer are optionally handled by the PHY, not by MAC.
>
> Not sure I understand your reply here, this would appear to be a limitation
> that exists regardless of the current API defined, does that mean that you
> can make use of the phy_driver::{read,write}_mmd function calls and you will
> make a v2 that uses them, or something else entirely?
There are two ways to solve this problem:
- indirect way. Add read/write_mdd filter to catch requests and patch
them as needed.
- direct way. Introduce PHY specific EEE API and allow drivers to use
it.
What's with indirect way?
1. Hard to find common pattern within other drivers.
2. It is not obvious for some one, what is going on, without deep diving
in to documentation.
3. We provide different levels of abstractions not really compatible with
each other. One part of code thinks we are doing right thing other
part is trying to fake the answers. It looks and feels wrong.
4. I already tried to mainline driver with for a HW not 100% compatible
with 802.3 specification, which was faking read/write_mdd answers to
not supported or broken registers. It was not accepted and it was
good decision to not doing this.
Direct way:
- clean understandable API.
- It is possible to find common patterns.
- It is possible to support more exotic variants not reflected in the
802.3 spec.
Now we have a direct way. Yes, it is possible to implement in exact this
driver a read/write_mdd filter, but it is also possible to implement
get/set_eee as well. Why should I implement in this driver the filter
if I already know that next driver will need get/set_eee any way?
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-01-21 6:35 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-19 13:18 [PATCH net-next v1 0/4] net: add EEE support for KSZ9477 switch series Oleksij Rempel
2023-01-19 13:18 ` [PATCH net-next v1 1/4] net: phy: add driver specific get/set_eee support Oleksij Rempel
2023-01-20 3:53 ` Jakub Kicinski
2023-01-19 13:18 ` [PATCH net-next v1 2/4] net: phy: micrel: add EEE configuration support for KSZ9477 variants of PHYs Oleksij Rempel
2023-01-19 14:08 ` Andrew Lunn
2023-01-20 5:50 ` Oleksij Rempel
2023-01-19 19:25 ` Florian Fainelli
2023-01-20 5:55 ` Oleksij Rempel
2023-01-20 17:58 ` Florian Fainelli
2023-01-21 6:34 ` Oleksij Rempel
2023-01-19 13:18 ` [PATCH net-next v1 3/4] net: phy: micrel: disable 1000Mbit EEE support if 1000Mbit is not supported Oleksij Rempel
2023-01-19 13:18 ` [PATCH net-next v1 4/4] net: dsa: microchip: enable EEE support Oleksij Rempel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).