| Message ID | e7ae2409f68a2f953ba7c823e248de7d67dfd4e9.1716973237.git.siyanteng@loongson.cn (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | stmmac: Add Loongson platform support | expand |
Hi, Yanteng, The title should be "Fix ....." rather than "Fixed .....", and it is better to move this patch earlier since it is a preparation for later Loongson-related patches. Huacai On Wed, May 29, 2024 at 6:20 PM Yanteng Si <siyanteng@loongson.cn> wrote: > > Loongson GNET devices with dev revision 0x00 do not > support manually setting the speed to 1000, When the > bug is triggered, let's return -EOPNOTSUPP, which > will be flag in later gnet support patches. > > Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn> > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn> > Signed-off-by: Yanteng Si <siyanteng@loongson.cn> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 6 ++++++ > include/linux/stmmac.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > index 542e2633a6f5..eb4b3eaf9e17 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > @@ -422,6 +422,12 @@ stmmac_ethtool_set_link_ksettings(struct net_device *dev, > return 0; > } > > + if (priv->plat->flags & STMMAC_FLAG_DISABLE_FORCE_1000) { > + if (cmd->base.speed == SPEED_1000 && > + cmd->base.autoneg != AUTONEG_ENABLE) > + return -EOPNOTSUPP; > + } > + > return phylink_ethtool_ksettings_set(priv->phylink, cmd); > } > > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h > index 1536455f9052..3e4f7e8d73fb 100644 > --- a/include/linux/stmmac.h > +++ b/include/linux/stmmac.h > @@ -208,6 +208,7 @@ struct dwmac4_addrs { > #define STMMAC_FLAG_RX_CLK_RUNS_IN_LPI BIT(10) > #define STMMAC_FLAG_EN_TX_LPI_CLOCKGATING BIT(11) > #define STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY BIT(12) > +#define STMMAC_FLAG_DISABLE_FORCE_1000 BIT(13) > > struct plat_stmmacenet_data { > int bus_id; > -- > 2.31.4 >
On Thu, May 30, 2024 at 10:25:01AM +0800, Huacai Chen wrote: > Hi, Yanteng, > > The title should be "Fix ....." rather than "Fixed .....", and it is I would avoid the ambiguous "Fix" which for stable folk imply that this is a bug fix - but it isn't. It's adding support for requiring 1G speeds to always be negotiated. I would like this patch to be held off until more thought can be put into how to handle this without having a hack in the driver (stmmac has too many hacks and we're going to have to start saying no to these.) However, I'm completely overloaded right now to have any bandwidth to think about this.
2024年5月30日 15:22, "Russell King (Oracle)" <linux@armlinux.org.uk> 写到: Hi, Russell, Serge, > > On Thu, May 30, 2024 at 10:25:01AM +0800, Huacai Chen wrote: > > > > > Hi, Yanteng, > > > > > > > > The title should be "Fix ....." rather than "Fixed .....", and it is > > > > I would avoid the ambiguous "Fix" which for stable folk imply that this > > is a bug fix - but it isn't. It's adding support for requiring 1G > > speeds to always be negotiated. Oh, I get it now. Thanks! > > I would like this patch to be held off until more thought can be put > > into how to handle this without having a hack in the driver (stmmac > > has too many hacks and we're going to have to start saying no to > > these.) Yeah, you have a point there, but I would also like to hear Serge's opinion. Thanks, Yanteng
On Tue, Jun 04, 2024 at 11:29:43AM +0000, si.yanteng@linux.dev wrote: > 2024年5月30日 15:22, "Russell King (Oracle)" <linux@armlinux.org.uk> 写到: > > Hi, Russell, Serge, > > > > > On Thu, May 30, 2024 at 10:25:01AM +0800, Huacai Chen wrote: > > > > > > > > Hi, Yanteng, > > > > > > > > > > > > The title should be "Fix ....." rather than "Fixed .....", and it is > > > > > > > I would avoid the ambiguous "Fix" which for stable folk imply that this > > > > is a bug fix - but it isn't. It's adding support for requiring 1G > > > > speeds to always be negotiated. > Oh, I get it now. Thanks! > > > > > I would like this patch to be held off until more thought can be put > > > > into how to handle this without having a hack in the driver (stmmac > > > > has too many hacks and we're going to have to start saying no to > > > > these.) > Yeah, you have a point there, but I would also like to hear Serge's opinion. I would be really glad not to have so many hacks in the STMMAC driver. It would have been awesome if we are able to find a solution without introducing one more quirk in the common driver code. I started digging into this sometime ago, but failed to come up with any decent hypothesis about the problem nature. One of the glimpse idea was that the loongson_gnet_fix_speed() method code might be somehow connected with the problem. But I didn't have much time to go further than that. Anyway I guess we'll need to have at least two more patchset re-spins to settle all the found issues. Until then we can keep discussing the problem Yanteng experience on his device. Russell, do you have any suggestion of what info Yanteng should provide to better understand the problem and get closer to it' cause? -Serge(y) > > > > Thanks, > Yanteng
On Tue, Jul 02, 2024 at 01:31:44PM +0300, Serge Semin wrote: > On Tue, Jun 04, 2024 at 11:29:43AM +0000, si.yanteng@linux.dev wrote: > > 2024年5月30日 15:22, "Russell King (Oracle)" <linux@armlinux.org.uk> 写到: > > > > Hi, Russell, Serge, > > > > > I would like this patch to be held off until more thought can be put > > > > > > into how to handle this without having a hack in the driver (stmmac > > > > > > has too many hacks and we're going to have to start saying no to > > > > > > these.) > > Yeah, you have a point there, but I would also like to hear Serge's opinion. > > I would be really glad not to have so many hacks in the STMMAC driver. > It would have been awesome if we are able to find a solution without > introducing one more quirk in the common driver code. > > I started digging into this sometime ago, but failed to come up with > any decent hypothesis about the problem nature. One of the glimpse > idea was that the loongson_gnet_fix_speed() method code might be somehow > connected with the problem. But I didn't have much time to go further > than that. > > Anyway I guess we'll need to have at least two more patchset > re-spins to settle all the found issues. Until then we can keep > discussing the problem Yanteng experience on his device. Russell, do > you have any suggestion of what info Yanteng should provide to better > understand the problem and get closer to it' cause? First question: is auto-negotiation required by 802.3 for 1000base-T? By "required" I mean "required to be used" not "required to be implemented". This is an important distinction, and 802.3 tends to be a bit wooly about the exact meaning. However, I think on balance the conclusion is that AN is mandatory _to be used_ for 1000base-T links. Annex 40C's state diagrams seems to imply that mr_autoneg_enable (BMCR AN ENABLE) doesn't affect whether or not the AN state machines work for 1000base-T, and some PHY datasheets (e.g. Marvell Alaska) state that disabling mr_autoneg_enable leaves AN enabled but forced to 1G full duplex. So, I'm thinking is that the ethtool interface should reject any request where we have a PHY supporting *only* base-T for gigabit speeds, where the user is requesting !AN && SPEED_1000 on the basis that AN is part of the link setup of 1000base-T links. Maybe this should be a property of the struct phy_device so we can transition phylib and phylink to return an appropriate error to userspace? Alternatively, maybe just implement the Marvell Alaska solution to this problem (if the user attempts to disable AN on a PHY supporting only base-T at gigabit speeds, then we silently force AN with SPEED_1000 and DUPLEX_FULL. Andrew - seems to be an IEEE 802.3 requirement that's been missed in phylib, any thoughts?
On Tue, Jul 02, 2024 at 04:08:05PM +0100, Russell King (Oracle) wrote: > On Tue, Jul 02, 2024 at 01:31:44PM +0300, Serge Semin wrote: > > On Tue, Jun 04, 2024 at 11:29:43AM +0000, si.yanteng@linux.dev wrote: > > > 2024年5月30日 15:22, "Russell King (Oracle)" <linux@armlinux.org.uk> 写到: > > > > > > Hi, Russell, Serge, > > > > > > > I would like this patch to be held off until more thought can be put > > > > > > > > into how to handle this without having a hack in the driver (stmmac > > > > > > > > has too many hacks and we're going to have to start saying no to > > > > > > > > these.) > > > Yeah, you have a point there, but I would also like to hear Serge's opinion. > > > > I would be really glad not to have so many hacks in the STMMAC driver. > > It would have been awesome if we are able to find a solution without > > introducing one more quirk in the common driver code. > > > > I started digging into this sometime ago, but failed to come up with > > any decent hypothesis about the problem nature. One of the glimpse > > idea was that the loongson_gnet_fix_speed() method code might be somehow > > connected with the problem. But I didn't have much time to go further > > than that. > > > > Anyway I guess we'll need to have at least two more patchset > > re-spins to settle all the found issues. Until then we can keep > > discussing the problem Yanteng experience on his device. Russell, do > > you have any suggestion of what info Yanteng should provide to better > > understand the problem and get closer to it' cause? > > First question: is auto-negotiation required by 802.3 for 1000base-T? > By "required" I mean "required to be used" not "required to be > implemented". This is an important distinction, and 802.3 tends to be > a bit wooly about the exact meaning. However, I think on balance the > conclusion is that AN is mandatory _to be used_ for 1000base-T links. One another statement in IEEE 802.3 C40 that implies the AN being mandatory is that 1000BASE-T PHYs determine their MASTER or SLAVE part during the Auto-Negotiation process. The part determines the clock source utilized by the PHYs: "The MASTER PHY uses a local clock to determine the timing of transmitter operations. The SLAVE PHY recovers the clock from the received signal and uses it to determine the timing of transmitter operations, i.e.," (40.1.3 Operation of 1000BASE-T) So I guess that without Auto-negotiation the link just won't be established due to the clocks missconfiguration. > > Annex 40C's state diagrams seems to imply that mr_autoneg_enable > (BMCR AN ENABLE) doesn't affect whether or not the AN state machines > work for 1000base-T, and some PHY datasheets (e.g. Marvell Alaska) > state that disabling mr_autoneg_enable leaves AN enabled but forced > to 1G full duplex. > > So, I'm thinking is that the ethtool interface should reject any > request where we have a PHY supporting *only* base-T for gigabit > speeds, where the user is requesting !AN && SPEED_1000 on the basis > that AN is part of the link setup of 1000base-T links. > > Maybe this should be a property of the struct phy_device so we can > transition phylib and phylink to return an appropriate error to > userspace? > > Alternatively, maybe just implement the Marvell Alaska solution > to this problem (if the user attempts to disable AN on a PHY > supporting only base-T at gigabit speeds, then we silently force > AN with SPEED_1000 and DUPLEX_FULL. I am not that much knowledgable about the PHY-lib and PHY-link internals, but if we get to establish that the standard indeed implies the AN being mandatory, then this sounds like the least harmful solution from the user-space point of view. -SergeY > > Andrew - seems to be an IEEE 802.3 requirement that's been missed > in phylib, any thoughts? > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Jul 03, 2024 at 07:56:31PM +0300, Serge Semin wrote: > One another statement in IEEE 802.3 C40 that implies the AN being > mandatory is that 1000BASE-T PHYs determine their MASTER or SLAVE part > during the Auto-Negotiation process. The part determines the clock > source utilized by the PHYs: "The MASTER PHY uses a local clock to > determine the timing of transmitter operations. The SLAVE PHY recovers > the clock from the received signal and uses it to determine the timing > of transmitter operations, i.e.," (40.1.3 Operation of 1000BASE-T) > > So I guess that without Auto-negotiation the link just won't be > established due to the clocks missconfiguration. Oh damn, I did a reply, then cocked up sending it (lost it instead!) So, this is going to be a brief response now. It seems AN is basically required for 1000base-T. > > Alternatively, maybe just implement the Marvell Alaska solution > > to this problem (if the user attempts to disable AN on a PHY > > supporting only base-T at gigabit speeds, then we silently force > > AN with SPEED_1000 and DUPLEX_FULL. > > I am not that much knowledgable about the PHY-lib and PHY-link > internals, but if we get to establish that the standard indeed > implies the AN being mandatory, then this sounds like the least > harmful solution from the user-space point of view. The Atheros PHYs are another PHY where we should not be disabling AN when wishing to use 1000base-T (so says the datasheet - I did quote it in my original reply but lost that...) As has already been mentioned, Marvell Alaska takes an interesting approach - when BMCR AN enable is cleared but speed is forced to 1000, it internally keeps AN enabled and advertises the appropriate 1G speed + duplex capability bit depending on the BMCR duplex bit. Rather than erroring out, I think it may be better to just adopt the Marvell solution to this problem to give consistent behaviour across all PHYs.
> Rather than erroring out, I think it may be better to just adopt > the Marvell solution to this problem to give consistent behaviour > across all PHYs. Yes, expand phy_config_aneg() to look for this condition and enable autoneg. But should we disable it when the condition is reverted? The user swaps to 100Mbps forced? What about other speeds? Is this limited to 1G? Since we have devices without auto-neg for 2500BaseX i assume it is not an issue there. Andrew
On Wed, Jul 03, 2024 at 09:09:53PM +0200, Andrew Lunn wrote: > > Rather than erroring out, I think it may be better to just adopt > > the Marvell solution to this problem to give consistent behaviour > > across all PHYs. > > Yes, expand phy_config_aneg() to look for this condition and enable > autoneg. But should we disable it when the condition is reverted? The > user swaps to 100Mbps forced? I think we should "lie" to userspace rather than report how the hardware was actually programmed - again, because that's what would happen with Marvell Alaska. > What about other speeds? Is this limited to 1G? Since we have devices > without auto-neg for 2500BaseX i assume it is not an issue there. 1000base-X can have AN disabled - that's not an issue. Yes, there's the ongoing issues with 2500base-X. 10Gbase-T wording is similar to 1000base-T, so we probably need to do similar there. Likely also the case for 2500base-T and 5000base-T as well. So I'm thinking of something like this (untested): diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 6c6ec9475709..197c4d5ab55b 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -2094,22 +2094,20 @@ EXPORT_SYMBOL(phy_reset_after_clk_enable); /** * genphy_config_advert - sanitize and advertise auto-negotiation parameters * @phydev: target phy_device struct + * @advert: auto-negotiation parameters to advertise * * Description: Writes MII_ADVERTISE with the appropriate values, * after sanitizing the values to make sure we only advertise * what is supported. Returns < 0 on error, 0 if the PHY's advertisement * hasn't changed, and > 0 if it has changed. */ -static int genphy_config_advert(struct phy_device *phydev) +static int genphy_config_advert(struct phy_device *phydev, + const unsigned long *advert) { int err, bmsr, changed = 0; u32 adv; - /* Only allow advertising what this PHY supports */ - linkmode_and(phydev->advertising, phydev->advertising, - phydev->supported); - - adv = linkmode_adv_to_mii_adv_t(phydev->advertising); + adv = linkmode_adv_to_mii_adv_t(advert); /* Setup standard advertisement */ err = phy_modify_changed(phydev, MII_ADVERTISE, @@ -2132,7 +2130,7 @@ static int genphy_config_advert(struct phy_device *phydev) if (!(bmsr & BMSR_ESTATEN)) return changed; - adv = linkmode_adv_to_mii_ctrl1000_t(phydev->advertising); + adv = linkmode_adv_to_mii_ctrl1000_t(advert); err = phy_modify_changed(phydev, MII_CTRL1000, ADVERTISE_1000FULL | ADVERTISE_1000HALF, @@ -2356,6 +2354,9 @@ EXPORT_SYMBOL(genphy_check_and_restart_aneg); */ int __genphy_config_aneg(struct phy_device *phydev, bool changed) { + __ETHTOOL_DECLARE_LINK_MODE_MASK(fixed_advert); + const struct phy_setting *set; + unsigned long *advert; int err; err = genphy_c45_an_config_eee_aneg(phydev); @@ -2370,10 +2371,25 @@ int __genphy_config_aneg(struct phy_device *phydev, bool changed) else if (err) changed = true; - if (AUTONEG_ENABLE != phydev->autoneg) + if (phydev->autoneg == AUTONEG_ENABLE) { + /* Only allow advertising what this PHY supports */ + linkmode_and(phydev->advertising, phydev->advertising, + phydev->supported); + advert = phydev->advertising; + } else if (phydev->speed < SPEED_1000) { return genphy_setup_forced(phydev); + } else { + linkmode_zero(fixed_advert); + + set = phy_lookup_setting(phydev->speed, phydev->duplex, + phydev->supported, true); + if (set) + linkmode_set(set->bit, fixed_advert); + + advert = fixed_advert; + } - err = genphy_config_advert(phydev); + err = genphy_config_advert(phydev, advert); if (err < 0) /* error */ return err; else if (err)
Hi all, Thanks for your comments! 在 2024/7/4 04:33, Russell King (Oracle) 写道: > On Wed, Jul 03, 2024 at 09:09:53PM +0200, Andrew Lunn wrote: >>> Rather than erroring out, I think it may be better to just adopt >>> the Marvell solution to this problem to give consistent behaviour >>> across all PHYs. >> Yes, expand phy_config_aneg() to look for this condition and enable >> autoneg. But should we disable it when the condition is reverted? The >> user swaps to 100Mbps forced? > I think we should "lie" to userspace rather than report how the > hardware was actually programmed - again, because that's what would > happen with Marvell Alaska. > >> What about other speeds? Is this limited to 1G? Since we have devices >> without auto-neg for 2500BaseX i assume it is not an issue there. > 1000base-X can have AN disabled - that's not an issue. Yes, there's > the ongoing issues with 2500base-X. 10Gbase-T wording is similar to > 1000base-T, so we probably need to do similar there. Likely also the > case for 2500base-T and 5000base-T as well. > > So I'm thinking of something like this (untested): > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 6c6ec9475709..197c4d5ab55b 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -2094,22 +2094,20 @@ EXPORT_SYMBOL(phy_reset_after_clk_enable); > /** > * genphy_config_advert - sanitize and advertise auto-negotiation parameters > * @phydev: target phy_device struct > + * @advert: auto-negotiation parameters to advertise > * > * Description: Writes MII_ADVERTISE with the appropriate values, > * after sanitizing the values to make sure we only advertise > * what is supported. Returns < 0 on error, 0 if the PHY's advertisement > * hasn't changed, and > 0 if it has changed. > */ > -static int genphy_config_advert(struct phy_device *phydev) > +static int genphy_config_advert(struct phy_device *phydev, > + const unsigned long *advert) > { > int err, bmsr, changed = 0; > u32 adv; > > - /* Only allow advertising what this PHY supports */ > - linkmode_and(phydev->advertising, phydev->advertising, > - phydev->supported); > - > - adv = linkmode_adv_to_mii_adv_t(phydev->advertising); > + adv = linkmode_adv_to_mii_adv_t(advert); > > /* Setup standard advertisement */ > err = phy_modify_changed(phydev, MII_ADVERTISE, > @@ -2132,7 +2130,7 @@ static int genphy_config_advert(struct phy_device *phydev) > if (!(bmsr & BMSR_ESTATEN)) > return changed; > > - adv = linkmode_adv_to_mii_ctrl1000_t(phydev->advertising); > + adv = linkmode_adv_to_mii_ctrl1000_t(advert); > > err = phy_modify_changed(phydev, MII_CTRL1000, > ADVERTISE_1000FULL | ADVERTISE_1000HALF, > @@ -2356,6 +2354,9 @@ EXPORT_SYMBOL(genphy_check_and_restart_aneg); > */ > int __genphy_config_aneg(struct phy_device *phydev, bool changed) > { > + __ETHTOOL_DECLARE_LINK_MODE_MASK(fixed_advert); > + const struct phy_setting *set; > + unsigned long *advert; > int err; > > err = genphy_c45_an_config_eee_aneg(phydev); > @@ -2370,10 +2371,25 @@ int __genphy_config_aneg(struct phy_device *phydev, bool changed) > else if (err) > changed = true; > > - if (AUTONEG_ENABLE != phydev->autoneg) > + if (phydev->autoneg == AUTONEG_ENABLE) { > + /* Only allow advertising what this PHY supports */ > + linkmode_and(phydev->advertising, phydev->advertising, > + phydev->supported); > + advert = phydev->advertising; > + } else if (phydev->speed < SPEED_1000) { > return genphy_setup_forced(phydev); > + } else { > + linkmode_zero(fixed_advert); > + > + set = phy_lookup_setting(phydev->speed, phydev->duplex, > + phydev->supported, true); > + if (set) > + linkmode_set(set->bit, fixed_advert); > + > + advert = fixed_advert; > + } > > - err = genphy_config_advert(phydev); > + err = genphy_config_advert(phydev, advert); > if (err < 0) /* error */ > return err; > else if (err) It looks great, but I still want to follow Russell's earlier advice and drop this patch from v14, then submit it separately with the above code. Thanks, Yanteng
On Fri, Jul 05, 2024 at 07:17:01PM +0800, Yanteng Si wrote: > 在 2024/7/4 04:33, Russell King (Oracle) 写道: > > I think we should "lie" to userspace rather than report how the > > hardware was actually programmed - again, because that's what would > > happen with Marvell Alaska. > > > > > What about other speeds? Is this limited to 1G? Since we have devices > > > without auto-neg for 2500BaseX i assume it is not an issue there. > > 1000base-X can have AN disabled - that's not an issue. Yes, there's > > the ongoing issues with 2500base-X. 10Gbase-T wording is similar to > > 1000base-T, so we probably need to do similar there. Likely also the > > case for 2500base-T and 5000base-T as well. > > > > So I'm thinking of something like this (untested): > > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > > index 6c6ec9475709..197c4d5ab55b 100644 > > --- a/drivers/net/phy/phy_device.c > > +++ b/drivers/net/phy/phy_device.c > > @@ -2094,22 +2094,20 @@ EXPORT_SYMBOL(phy_reset_after_clk_enable); > > /** > > * genphy_config_advert - sanitize and advertise auto-negotiation parameters > > * @phydev: target phy_device struct > > + * @advert: auto-negotiation parameters to advertise > > * > > * Description: Writes MII_ADVERTISE with the appropriate values, > > * after sanitizing the values to make sure we only advertise > > * what is supported. Returns < 0 on error, 0 if the PHY's advertisement > > * hasn't changed, and > 0 if it has changed. > > */ > > -static int genphy_config_advert(struct phy_device *phydev) > > +static int genphy_config_advert(struct phy_device *phydev, > > + const unsigned long *advert) > > { > > int err, bmsr, changed = 0; > > u32 adv; > > - /* Only allow advertising what this PHY supports */ > > - linkmode_and(phydev->advertising, phydev->advertising, > > - phydev->supported); > > - > > - adv = linkmode_adv_to_mii_adv_t(phydev->advertising); > > + adv = linkmode_adv_to_mii_adv_t(advert); > > /* Setup standard advertisement */ > > err = phy_modify_changed(phydev, MII_ADVERTISE, > > @@ -2132,7 +2130,7 @@ static int genphy_config_advert(struct phy_device *phydev) > > if (!(bmsr & BMSR_ESTATEN)) > > return changed; > > - adv = linkmode_adv_to_mii_ctrl1000_t(phydev->advertising); > > + adv = linkmode_adv_to_mii_ctrl1000_t(advert); > > err = phy_modify_changed(phydev, MII_CTRL1000, > > ADVERTISE_1000FULL | ADVERTISE_1000HALF, > > @@ -2356,6 +2354,9 @@ EXPORT_SYMBOL(genphy_check_and_restart_aneg); > > */ > > int __genphy_config_aneg(struct phy_device *phydev, bool changed) > > { > > + __ETHTOOL_DECLARE_LINK_MODE_MASK(fixed_advert); > > + const struct phy_setting *set; > > + unsigned long *advert; > > int err; > > err = genphy_c45_an_config_eee_aneg(phydev); > > @@ -2370,10 +2371,25 @@ int __genphy_config_aneg(struct phy_device *phydev, bool changed) > > else if (err) > > changed = true; > > - if (AUTONEG_ENABLE != phydev->autoneg) > > + if (phydev->autoneg == AUTONEG_ENABLE) { > > + /* Only allow advertising what this PHY supports */ > > + linkmode_and(phydev->advertising, phydev->advertising, > > + phydev->supported); > > + advert = phydev->advertising; > > + } else if (phydev->speed < SPEED_1000) { > > return genphy_setup_forced(phydev); > > + } else { > > + linkmode_zero(fixed_advert); > > + > > + set = phy_lookup_setting(phydev->speed, phydev->duplex, > > + phydev->supported, true); > > + if (set) > > + linkmode_set(set->bit, fixed_advert); > > + > > + advert = fixed_advert; > > + } > > - err = genphy_config_advert(phydev); > > + err = genphy_config_advert(phydev, advert); > > if (err < 0) /* error */ > > return err; > > else if (err) > > It looks great, but I still want to follow Russell's earlier advice and drop > this patch > > from v14, then submit it separately with the above code. If the above change is made to phylib, then drivers do not need any changes other than removing such workarounds detecting !AN with speed = 1G. The point of the above change is that drivers shouldn't be doing anything and the issue should be handled entirely within phylib.
在 2024/7/5 19:31, Russell King (Oracle) 写道: > On Fri, Jul 05, 2024 at 07:17:01PM +0800, Yanteng Si wrote: >> 在 2024/7/4 04:33, Russell King (Oracle) 写道: >>> I think we should "lie" to userspace rather than report how the >>> hardware was actually programmed - again, because that's what would >>> happen with Marvell Alaska. >>> >>>> What about other speeds? Is this limited to 1G? Since we have devices >>>> without auto-neg for 2500BaseX i assume it is not an issue there. >>> 1000base-X can have AN disabled - that's not an issue. Yes, there's >>> the ongoing issues with 2500base-X. 10Gbase-T wording is similar to >>> 1000base-T, so we probably need to do similar there. Likely also the >>> case for 2500base-T and 5000base-T as well. >>> >>> So I'm thinking of something like this (untested): >>> >>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >>> index 6c6ec9475709..197c4d5ab55b 100644 >>> --- a/drivers/net/phy/phy_device.c >>> +++ b/drivers/net/phy/phy_device.c >>> @@ -2094,22 +2094,20 @@ EXPORT_SYMBOL(phy_reset_after_clk_enable); >>> /** >>> * genphy_config_advert - sanitize and advertise auto-negotiation parameters >>> * @phydev: target phy_device struct >>> + * @advert: auto-negotiation parameters to advertise >>> * >>> * Description: Writes MII_ADVERTISE with the appropriate values, >>> * after sanitizing the values to make sure we only advertise >>> * what is supported. Returns < 0 on error, 0 if the PHY's advertisement >>> * hasn't changed, and > 0 if it has changed. >>> */ >>> -static int genphy_config_advert(struct phy_device *phydev) >>> +static int genphy_config_advert(struct phy_device *phydev, >>> + const unsigned long *advert) >>> { >>> int err, bmsr, changed = 0; >>> u32 adv; >>> - /* Only allow advertising what this PHY supports */ >>> - linkmode_and(phydev->advertising, phydev->advertising, >>> - phydev->supported); >>> - >>> - adv = linkmode_adv_to_mii_adv_t(phydev->advertising); >>> + adv = linkmode_adv_to_mii_adv_t(advert); >>> /* Setup standard advertisement */ >>> err = phy_modify_changed(phydev, MII_ADVERTISE, >>> @@ -2132,7 +2130,7 @@ static int genphy_config_advert(struct phy_device *phydev) >>> if (!(bmsr & BMSR_ESTATEN)) >>> return changed; >>> - adv = linkmode_adv_to_mii_ctrl1000_t(phydev->advertising); >>> + adv = linkmode_adv_to_mii_ctrl1000_t(advert); >>> err = phy_modify_changed(phydev, MII_CTRL1000, >>> ADVERTISE_1000FULL | ADVERTISE_1000HALF, >>> @@ -2356,6 +2354,9 @@ EXPORT_SYMBOL(genphy_check_and_restart_aneg); >>> */ >>> int __genphy_config_aneg(struct phy_device *phydev, bool changed) >>> { >>> + __ETHTOOL_DECLARE_LINK_MODE_MASK(fixed_advert); >>> + const struct phy_setting *set; >>> + unsigned long *advert; >>> int err; >>> err = genphy_c45_an_config_eee_aneg(phydev); >>> @@ -2370,10 +2371,25 @@ int __genphy_config_aneg(struct phy_device *phydev, bool changed) >>> else if (err) >>> changed = true; >>> - if (AUTONEG_ENABLE != phydev->autoneg) >>> + if (phydev->autoneg == AUTONEG_ENABLE) { >>> + /* Only allow advertising what this PHY supports */ >>> + linkmode_and(phydev->advertising, phydev->advertising, >>> + phydev->supported); >>> + advert = phydev->advertising; >>> + } else if (phydev->speed < SPEED_1000) { >>> return genphy_setup_forced(phydev); >>> + } else { >>> + linkmode_zero(fixed_advert); >>> + >>> + set = phy_lookup_setting(phydev->speed, phydev->duplex, >>> + phydev->supported, true); >>> + if (set) >>> + linkmode_set(set->bit, fixed_advert); >>> + >>> + advert = fixed_advert; >>> + } >>> - err = genphy_config_advert(phydev); >>> + err = genphy_config_advert(phydev, advert); >>> if (err < 0) /* error */ >>> return err; >>> else if (err) >> It looks great, but I still want to follow Russell's earlier advice and drop >> this patch >> >> from v14, then submit it separately with the above code. > If the above change is made to phylib, then drivers do not need any > changes other than removing such workarounds detecting !AN with > speed = 1G. > > The point of the above change is that drivers shouldn't be doing > anything and the issue should be handled entirely within phylib. > OK, I see. I will try this after send v14(Maybe tomorrow, or next Monday). Thanks, Yanteng
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c index 542e2633a6f5..eb4b3eaf9e17 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c @@ -422,6 +422,12 @@ stmmac_ethtool_set_link_ksettings(struct net_device *dev, return 0; } + if (priv->plat->flags & STMMAC_FLAG_DISABLE_FORCE_1000) { + if (cmd->base.speed == SPEED_1000 && + cmd->base.autoneg != AUTONEG_ENABLE) + return -EOPNOTSUPP; + } + return phylink_ethtool_ksettings_set(priv->phylink, cmd); } diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index 1536455f9052..3e4f7e8d73fb 100644 --- a/include/linux/stmmac.h +++ b/include/linux/stmmac.h @@ -208,6 +208,7 @@ struct dwmac4_addrs { #define STMMAC_FLAG_RX_CLK_RUNS_IN_LPI BIT(10) #define STMMAC_FLAG_EN_TX_LPI_CLOCKGATING BIT(11) #define STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY BIT(12) +#define STMMAC_FLAG_DISABLE_FORCE_1000 BIT(13) struct plat_stmmacenet_data { int bus_id;