| Message ID | 20230119131821.3832456-3-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: add EEE support for KSZ9477 switch series | expand |
> +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
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).
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
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
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?
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
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);
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(+)