| Message ID | 4677d3c4-60e2-4094-81a8-adae42ca46bb@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] r8169: copy vendor driver 2.5G/5G EEE advertisement constraints | expand |
On Mon, Nov 04, 2024 at 11:07:20PM +0100, Heiner Kallweit wrote: > Vendor driver r8125 doesn't advertise 2.5G EEE on RTL8125A, and r8126 > doesn't advertise 5G EEE. Likely there are compatibility issues, > therefore do the same in r8169. > With this change we don't have to disable 2.5G EEE advertisement in > rtl8125a_config_eee_phy() any longer. > Note: We don't remove the potentially problematic modes from the > supported modes, so users can re-enable advertisement of these modes > if they work fine in their setup. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/net/ethernet/realtek/r8169_main.c | 7 +++++++ > drivers/net/ethernet/realtek/r8169_phy_config.c | 16 ++++------------ > 2 files changed, 11 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index e83c4841b..4f37d25e0 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -5318,6 +5318,13 @@ static int r8169_mdio_register(struct rtl8169_private *tp) > phy_support_eee(tp->phydev); > phy_support_asym_pause(tp->phydev); > > + /* mimic behavior of r8125/r8126 vendor drivers */ > + if (tp->mac_version == RTL_GIGA_MAC_VER_61) > + linkmode_clear_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, > + tp->phydev->advertising_eee); > + linkmode_clear_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, > + tp->phydev->advertising_eee); Hi Heiner phy_device.c has: /** * phy_remove_link_mode - Remove a supported link mode * @phydev: phy_device structure to remove link mode from * @link_mode: Link mode to be removed * * Description: Some MACs don't support all link modes which the PHY * does. e.g. a 1G MAC often does not support 1000Half. Add a helper * to remove a link mode. */ void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode) { linkmode_clear_bit(link_mode, phydev->supported); phy_advertise_supported(phydev); } EXPORT_SYMBOL(phy_remove_link_mode); Maybe we need a phy_remove_eee_link_mode()? That could also remove it from supported? At minimum, it would stop MAC drivers poking around the insides of phylib. Andrew
On 05.11.2024 22:06, Andrew Lunn wrote: > On Mon, Nov 04, 2024 at 11:07:20PM +0100, Heiner Kallweit wrote: >> Vendor driver r8125 doesn't advertise 2.5G EEE on RTL8125A, and r8126 >> doesn't advertise 5G EEE. Likely there are compatibility issues, >> therefore do the same in r8169. >> With this change we don't have to disable 2.5G EEE advertisement in >> rtl8125a_config_eee_phy() any longer. >> Note: We don't remove the potentially problematic modes from the >> supported modes, so users can re-enable advertisement of these modes >> if they work fine in their setup. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/net/ethernet/realtek/r8169_main.c | 7 +++++++ >> drivers/net/ethernet/realtek/r8169_phy_config.c | 16 ++++------------ >> 2 files changed, 11 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c >> index e83c4841b..4f37d25e0 100644 >> --- a/drivers/net/ethernet/realtek/r8169_main.c >> +++ b/drivers/net/ethernet/realtek/r8169_main.c >> @@ -5318,6 +5318,13 @@ static int r8169_mdio_register(struct rtl8169_private *tp) >> phy_support_eee(tp->phydev); >> phy_support_asym_pause(tp->phydev); >> >> + /* mimic behavior of r8125/r8126 vendor drivers */ >> + if (tp->mac_version == RTL_GIGA_MAC_VER_61) >> + linkmode_clear_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >> + tp->phydev->advertising_eee); >> + linkmode_clear_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, >> + tp->phydev->advertising_eee); > > Hi Heiner > > phy_device.c has: > > /** > * phy_remove_link_mode - Remove a supported link mode > * @phydev: phy_device structure to remove link mode from > * @link_mode: Link mode to be removed > * > * Description: Some MACs don't support all link modes which the PHY > * does. e.g. a 1G MAC often does not support 1000Half. Add a helper > * to remove a link mode. > */ > void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode) > { > linkmode_clear_bit(link_mode, phydev->supported); > phy_advertise_supported(phydev); > } > EXPORT_SYMBOL(phy_remove_link_mode); > > Maybe we need a phy_remove_eee_link_mode()? That could also remove it > from supported? At minimum, it would stop MAC drivers poking around > the insides of phylib. > I just do it in the MAC driver because PHY drivers don't have a good place to initially disable a certain (EEE) mode. Modifying the advertisement register in get_features(), so that genphy_c45_read_eee_adv() reads the desired advertisement, would be somewhat hacky. My use case "remove a mode from advertisement initially, but keep it in supported modes, so that user can re-enable it" isn't really supported as of today. Of course we could add a simple setter like void phy_remove_eee_mode_from_advertising(struct phy_device *phydev, u32 link_mode) { linkmode_clear_bit(link_mode, phydev->advertising_eee); } But would this be worth it? > Andrew Heiner
On 05.11.2024 22:06, Andrew Lunn wrote: > On Mon, Nov 04, 2024 at 11:07:20PM +0100, Heiner Kallweit wrote: >> Vendor driver r8125 doesn't advertise 2.5G EEE on RTL8125A, and r8126 >> doesn't advertise 5G EEE. Likely there are compatibility issues, >> therefore do the same in r8169. >> With this change we don't have to disable 2.5G EEE advertisement in >> rtl8125a_config_eee_phy() any longer. >> Note: We don't remove the potentially problematic modes from the >> supported modes, so users can re-enable advertisement of these modes >> if they work fine in their setup. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/net/ethernet/realtek/r8169_main.c | 7 +++++++ >> drivers/net/ethernet/realtek/r8169_phy_config.c | 16 ++++------------ >> 2 files changed, 11 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c >> index e83c4841b..4f37d25e0 100644 >> --- a/drivers/net/ethernet/realtek/r8169_main.c >> +++ b/drivers/net/ethernet/realtek/r8169_main.c >> @@ -5318,6 +5318,13 @@ static int r8169_mdio_register(struct rtl8169_private *tp) >> phy_support_eee(tp->phydev); >> phy_support_asym_pause(tp->phydev); >> >> + /* mimic behavior of r8125/r8126 vendor drivers */ >> + if (tp->mac_version == RTL_GIGA_MAC_VER_61) >> + linkmode_clear_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >> + tp->phydev->advertising_eee); >> + linkmode_clear_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, >> + tp->phydev->advertising_eee); > > Hi Heiner > > phy_device.c has: > > /** > * phy_remove_link_mode - Remove a supported link mode > * @phydev: phy_device structure to remove link mode from > * @link_mode: Link mode to be removed > * > * Description: Some MACs don't support all link modes which the PHY > * does. e.g. a 1G MAC often does not support 1000Half. Add a helper > * to remove a link mode. > */ > void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode) > { > linkmode_clear_bit(link_mode, phydev->supported); > phy_advertise_supported(phydev); > } > EXPORT_SYMBOL(phy_remove_link_mode); > > Maybe we need a phy_remove_eee_link_mode()? That could also remove it > from supported? At minimum, it would stop MAC drivers poking around > the insides of phylib. > After checking eee_broken_modes in more detail: In general it does what I need, it prevents broken EEE modes from being advertised. Some challenges: - eee_broken_modes currently represents eee_cap1 register bits. So it's not possible to flag 2.5G or 5G EEE as broken. - We would have to change eee_broken_modes to a linkmode bitmap. - eee_broken_modes can be populated via DT only. of_set_phy_eee_broken() would have to be changed to operate on fwnodes. Then I may be able to "inject" the broken modes in my case as swnode's. Not the easiest solution, but maybe the cleanest. Feedback welcome. > Andrew Heiner
On Wed, Nov 06, 2024 at 02:28:54PM +0100, Heiner Kallweit wrote: > On 05.11.2024 22:06, Andrew Lunn wrote: > > On Mon, Nov 04, 2024 at 11:07:20PM +0100, Heiner Kallweit wrote: > >> Vendor driver r8125 doesn't advertise 2.5G EEE on RTL8125A, and r8126 > >> doesn't advertise 5G EEE. Likely there are compatibility issues, > >> therefore do the same in r8169. > >> With this change we don't have to disable 2.5G EEE advertisement in > >> rtl8125a_config_eee_phy() any longer. > >> Note: We don't remove the potentially problematic modes from the > >> supported modes, so users can re-enable advertisement of these modes > >> if they work fine in their setup. > >> > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > >> --- > >> drivers/net/ethernet/realtek/r8169_main.c | 7 +++++++ > >> drivers/net/ethernet/realtek/r8169_phy_config.c | 16 ++++------------ > >> 2 files changed, 11 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > >> index e83c4841b..4f37d25e0 100644 > >> --- a/drivers/net/ethernet/realtek/r8169_main.c > >> +++ b/drivers/net/ethernet/realtek/r8169_main.c > >> @@ -5318,6 +5318,13 @@ static int r8169_mdio_register(struct rtl8169_private *tp) > >> phy_support_eee(tp->phydev); > >> phy_support_asym_pause(tp->phydev); > >> > >> + /* mimic behavior of r8125/r8126 vendor drivers */ > >> + if (tp->mac_version == RTL_GIGA_MAC_VER_61) > >> + linkmode_clear_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, > >> + tp->phydev->advertising_eee); > >> + linkmode_clear_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, > >> + tp->phydev->advertising_eee); > > > > Hi Heiner > > > > phy_device.c has: > > > > /** > > * phy_remove_link_mode - Remove a supported link mode > > * @phydev: phy_device structure to remove link mode from > > * @link_mode: Link mode to be removed > > * > > * Description: Some MACs don't support all link modes which the PHY > > * does. e.g. a 1G MAC often does not support 1000Half. Add a helper > > * to remove a link mode. > > */ > > void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode) > > { > > linkmode_clear_bit(link_mode, phydev->supported); > > phy_advertise_supported(phydev); > > } > > EXPORT_SYMBOL(phy_remove_link_mode); > > > > Maybe we need a phy_remove_eee_link_mode()? That could also remove it > > from supported? At minimum, it would stop MAC drivers poking around > > the insides of phylib. > > > After checking eee_broken_modes in more detail: > In general it does what I need, it prevents broken EEE modes from being > advertised. Some challenges: > > - eee_broken_modes currently represents eee_cap1 register bits. > So it's not possible to flag 2.5G or 5G EEE as broken. > - We would have to change eee_broken_modes to a linkmode bitmap. > > - eee_broken_modes can be populated via DT only. > of_set_phy_eee_broken() would have to be changed to operate on > fwnodes. Then I may be able to "inject" the broken modes in my > case as swnode's. > Not the easiest solution, but maybe the cleanest. Feedback welcome. I probably would not go as far as adding swnode support for something so simple. Just add phy_remove_eee_link_mode() or maybe phy_set_eee_broken() as i suggested to set bits in phydev->eee_broken_modes. And converting it to a linkmode bitmap is probably needed in the long term anyway. the change does not look too invasive, eee_broken_mode is not used in many places. I say go for it. Andrew
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index e83c4841b..4f37d25e0 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -5318,6 +5318,13 @@ static int r8169_mdio_register(struct rtl8169_private *tp) phy_support_eee(tp->phydev); phy_support_asym_pause(tp->phydev); + /* mimic behavior of r8125/r8126 vendor drivers */ + if (tp->mac_version == RTL_GIGA_MAC_VER_61) + linkmode_clear_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, + tp->phydev->advertising_eee); + linkmode_clear_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, + tp->phydev->advertising_eee); + /* PHY will be woken up in rtl_open() */ phy_suspend(tp->phydev); diff --git a/drivers/net/ethernet/realtek/r8169_phy_config.c b/drivers/net/ethernet/realtek/r8169_phy_config.c index 1d5b33f6c..5307c6ff4 100644 --- a/drivers/net/ethernet/realtek/r8169_phy_config.c +++ b/drivers/net/ethernet/realtek/r8169_phy_config.c @@ -96,15 +96,7 @@ static void rtl8125_common_config_eee_phy(struct phy_device *phydev) phy_modify_paged(phydev, 0xa4a, 0x11, 0x0200, 0x0000); } -static void rtl8125a_config_eee_phy(struct phy_device *phydev) -{ - rtl8168g_config_eee_phy(phydev); - /* disable EEE at 2.5Gbps */ - phy_modify_paged(phydev, 0xa6d, 0x12, 0x0001, 0x0000); - rtl8125_common_config_eee_phy(phydev); -} - -static void rtl8125b_config_eee_phy(struct phy_device *phydev) +static void rtl8125_config_eee_phy(struct phy_device *phydev) { rtl8168g_config_eee_phy(phydev); rtl8125_common_config_eee_phy(phydev); @@ -1066,7 +1058,7 @@ static void rtl8125a_2_hw_phy_config(struct rtl8169_private *tp, rtl8168g_enable_gphy_10m(phydev); rtl8168g_disable_aldps(phydev); - rtl8125a_config_eee_phy(phydev); + rtl8125_config_eee_phy(phydev); } static void rtl8125b_hw_phy_config(struct rtl8169_private *tp, @@ -1106,7 +1098,7 @@ static void rtl8125b_hw_phy_config(struct rtl8169_private *tp, rtl8125_legacy_force_mode(phydev); rtl8168g_disable_aldps(phydev); - rtl8125b_config_eee_phy(phydev); + rtl8125_config_eee_phy(phydev); } static void rtl8125d_hw_phy_config(struct rtl8169_private *tp, @@ -1116,7 +1108,7 @@ static void rtl8125d_hw_phy_config(struct rtl8169_private *tp, rtl8168g_enable_gphy_10m(phydev); rtl8125_legacy_force_mode(phydev); rtl8168g_disable_aldps(phydev); - rtl8125b_config_eee_phy(phydev); + rtl8125_config_eee_phy(phydev); } static void rtl8126a_hw_phy_config(struct rtl8169_private *tp,
Vendor driver r8125 doesn't advertise 2.5G EEE on RTL8125A, and r8126 doesn't advertise 5G EEE. Likely there are compatibility issues, therefore do the same in r8169. With this change we don't have to disable 2.5G EEE advertisement in rtl8125a_config_eee_phy() any longer. Note: We don't remove the potentially problematic modes from the supported modes, so users can re-enable advertisement of these modes if they work fine in their setup. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169_main.c | 7 +++++++ drivers/net/ethernet/realtek/r8169_phy_config.c | 16 ++++------------ 2 files changed, 11 insertions(+), 12 deletions(-)