| Message ID | 732a70d6-4191-4aae-8862-3716b062aa9e@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2b9ec5dfb8255656ca731ab9d9bf59d94566d377 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: realtek: complete 5Gbps support and replace private constants | expand |
On Sun, Feb 04, 2024 at 03:17:53PM +0100, Heiner Kallweit wrote: > From: Marek Behún <kabel@kernel.org> > > Drop the ad-hoc MDIO constants used in the driver and use generic > constants instead. > > Signed-off-by: Marek Behún <kabel@kernel.org> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/net/phy/realtek.c | 30 +++++++++++++----------------- > 1 file changed, 13 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c > index 894172a3e..ffc13c495 100644 > --- a/drivers/net/phy/realtek.c > +++ b/drivers/net/phy/realtek.c > @@ -57,14 +57,6 @@ > #define RTL8366RB_POWER_SAVE 0x15 > #define RTL8366RB_POWER_SAVE_ON BIT(12) > > -#define RTL_SUPPORTS_5000FULL BIT(14) > -#define RTL_SUPPORTS_2500FULL BIT(13) > -#define RTL_SUPPORTS_10000FULL BIT(0) > -#define RTL_ADV_2500FULL BIT(7) > -#define RTL_LPADV_10000FULL BIT(11) > -#define RTL_LPADV_5000FULL BIT(6) > -#define RTL_LPADV_2500FULL BIT(5) > - > #define RTL9000A_GINMR 0x14 > #define RTL9000A_GINMR_LINK_STATUS BIT(4) > > @@ -674,11 +666,11 @@ static int rtl822x_get_features(struct phy_device *phydev) > return val; > > linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, > - phydev->supported, val & RTL_SUPPORTS_2500FULL); > + phydev->supported, val & MDIO_PMA_SPEED_2_5G); > linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, > - phydev->supported, val & RTL_SUPPORTS_5000FULL); > + phydev->supported, val & MDIO_PMA_SPEED_5G); > linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, > - phydev->supported, val & RTL_SUPPORTS_10000FULL); > + phydev->supported, val & MDIO_SPEED_10G); Now that this only using generic constants, should it move into mdio.h as a shared helper? Is this a standard register defined in 802.3, just at a different address? > > return genphy_read_abilities(phydev); > } > @@ -692,10 +684,11 @@ static int rtl822x_config_aneg(struct phy_device *phydev) > > if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, > phydev->advertising)) > - adv2500 = RTL_ADV_2500FULL; > + adv2500 = MDIO_AN_10GBT_CTRL_ADV2_5G; > > ret = phy_modify_paged_changed(phydev, 0xa5d, 0x12, > - RTL_ADV_2500FULL, adv2500); > + MDIO_AN_10GBT_CTRL_ADV2_5G, > + adv2500); > if (ret < 0) > return ret; > } > @@ -714,11 +707,14 @@ static int rtl822x_read_status(struct phy_device *phydev) > return lpadv; > > linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, > - phydev->lp_advertising, lpadv & RTL_LPADV_10000FULL); > + phydev->lp_advertising, > + lpadv & MDIO_AN_10GBT_STAT_LP10G); > linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, > - phydev->lp_advertising, lpadv & RTL_LPADV_5000FULL); > + phydev->lp_advertising, > + lpadv & MDIO_AN_10GBT_STAT_LP5G); > linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, > - phydev->lp_advertising, lpadv & RTL_LPADV_2500FULL); > + phydev->lp_advertising, > + lpadv & MDIO_AN_10GBT_STAT_LP2_5G); Is this mii_10gbt_stat_mod_linkmode_lpa_t() ? Something i've done in the past is to do this sort of conversion to standard macros, and the followed up with a patch which says that function X is now clearly the same as helper Y, so delete the function and use the helper... Andrew
On 04.02.2024 17:00, Andrew Lunn wrote: > On Sun, Feb 04, 2024 at 03:17:53PM +0100, Heiner Kallweit wrote: >> From: Marek Behún <kabel@kernel.org> >> >> Drop the ad-hoc MDIO constants used in the driver and use generic >> constants instead. >> >> Signed-off-by: Marek Behún <kabel@kernel.org> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/net/phy/realtek.c | 30 +++++++++++++----------------- >> 1 file changed, 13 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c >> index 894172a3e..ffc13c495 100644 >> --- a/drivers/net/phy/realtek.c >> +++ b/drivers/net/phy/realtek.c >> @@ -57,14 +57,6 @@ >> #define RTL8366RB_POWER_SAVE 0x15 >> #define RTL8366RB_POWER_SAVE_ON BIT(12) >> >> -#define RTL_SUPPORTS_5000FULL BIT(14) >> -#define RTL_SUPPORTS_2500FULL BIT(13) >> -#define RTL_SUPPORTS_10000FULL BIT(0) >> -#define RTL_ADV_2500FULL BIT(7) >> -#define RTL_LPADV_10000FULL BIT(11) >> -#define RTL_LPADV_5000FULL BIT(6) >> -#define RTL_LPADV_2500FULL BIT(5) >> - >> #define RTL9000A_GINMR 0x14 >> #define RTL9000A_GINMR_LINK_STATUS BIT(4) >> >> @@ -674,11 +666,11 @@ static int rtl822x_get_features(struct phy_device *phydev) >> return val; >> >> linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >> - phydev->supported, val & RTL_SUPPORTS_2500FULL); >> + phydev->supported, val & MDIO_PMA_SPEED_2_5G); >> linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, >> - phydev->supported, val & RTL_SUPPORTS_5000FULL); >> + phydev->supported, val & MDIO_PMA_SPEED_5G); >> linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, >> - phydev->supported, val & RTL_SUPPORTS_10000FULL); >> + phydev->supported, val & MDIO_SPEED_10G); > > Now that this only using generic constants, should it move into mdio.h > as a shared helper? Is this a standard register defined in 802.3, just > at a different address? > This is register 1.4 (PMA/PMD speed ability), mapped to a vendor-specific register. There's very few users of this register, and nothing where such a helper could be reused. >> >> return genphy_read_abilities(phydev); >> } >> @@ -692,10 +684,11 @@ static int rtl822x_config_aneg(struct phy_device *phydev) >> >> if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >> phydev->advertising)) >> - adv2500 = RTL_ADV_2500FULL; >> + adv2500 = MDIO_AN_10GBT_CTRL_ADV2_5G; >> >> ret = phy_modify_paged_changed(phydev, 0xa5d, 0x12, >> - RTL_ADV_2500FULL, adv2500); >> + MDIO_AN_10GBT_CTRL_ADV2_5G, >> + adv2500); >> if (ret < 0) >> return ret; >> } >> @@ -714,11 +707,14 @@ static int rtl822x_read_status(struct phy_device *phydev) >> return lpadv; >> >> linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, >> - phydev->lp_advertising, lpadv & RTL_LPADV_10000FULL); >> + phydev->lp_advertising, >> + lpadv & MDIO_AN_10GBT_STAT_LP10G); >> linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, >> - phydev->lp_advertising, lpadv & RTL_LPADV_5000FULL); >> + phydev->lp_advertising, >> + lpadv & MDIO_AN_10GBT_STAT_LP5G); >> linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >> - phydev->lp_advertising, lpadv & RTL_LPADV_2500FULL); >> + phydev->lp_advertising, >> + lpadv & MDIO_AN_10GBT_STAT_LP2_5G); > > Is this mii_10gbt_stat_mod_linkmode_lpa_t() ? > Indeed, it is. Thanks for the hint. I'd prefer to submit the patch making use of this helper as a follow-up patch. Then it's obvious that the helper is the same as the replaced code. > Something i've done in the past is to do this sort of conversion to > standard macros, and the followed up with a patch which says that > function X is now clearly the same as helper Y, so delete the function > and use the helper... > > Andrew Heiner
On 04.02.2024 17:26, Heiner Kallweit wrote: > On 04.02.2024 17:00, Andrew Lunn wrote: >> On Sun, Feb 04, 2024 at 03:17:53PM +0100, Heiner Kallweit wrote: >>> From: Marek Behún <kabel@kernel.org> >>> >>> Drop the ad-hoc MDIO constants used in the driver and use generic >>> constants instead. >>> >>> Signed-off-by: Marek Behún <kabel@kernel.org> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> --- >>> drivers/net/phy/realtek.c | 30 +++++++++++++----------------- >>> 1 file changed, 13 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c >>> index 894172a3e..ffc13c495 100644 >>> --- a/drivers/net/phy/realtek.c >>> +++ b/drivers/net/phy/realtek.c >>> @@ -57,14 +57,6 @@ >>> #define RTL8366RB_POWER_SAVE 0x15 >>> #define RTL8366RB_POWER_SAVE_ON BIT(12) >>> >>> -#define RTL_SUPPORTS_5000FULL BIT(14) >>> -#define RTL_SUPPORTS_2500FULL BIT(13) >>> -#define RTL_SUPPORTS_10000FULL BIT(0) >>> -#define RTL_ADV_2500FULL BIT(7) >>> -#define RTL_LPADV_10000FULL BIT(11) >>> -#define RTL_LPADV_5000FULL BIT(6) >>> -#define RTL_LPADV_2500FULL BIT(5) >>> - >>> #define RTL9000A_GINMR 0x14 >>> #define RTL9000A_GINMR_LINK_STATUS BIT(4) >>> >>> @@ -674,11 +666,11 @@ static int rtl822x_get_features(struct phy_device *phydev) >>> return val; >>> >>> linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >>> - phydev->supported, val & RTL_SUPPORTS_2500FULL); >>> + phydev->supported, val & MDIO_PMA_SPEED_2_5G); >>> linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, >>> - phydev->supported, val & RTL_SUPPORTS_5000FULL); >>> + phydev->supported, val & MDIO_PMA_SPEED_5G); >>> linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, >>> - phydev->supported, val & RTL_SUPPORTS_10000FULL); >>> + phydev->supported, val & MDIO_SPEED_10G); >> >> Now that this only using generic constants, should it move into mdio.h >> as a shared helper? Is this a standard register defined in 802.3, just >> at a different address? >> > This is register 1.4 (PMA/PMD speed ability), mapped to a vendor-specific > register. There's very few users of this register, and nothing where such > a helper could be reused. > >>> >>> return genphy_read_abilities(phydev); >>> } >>> @@ -692,10 +684,11 @@ static int rtl822x_config_aneg(struct phy_device *phydev) >>> >>> if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >>> phydev->advertising)) >>> - adv2500 = RTL_ADV_2500FULL; >>> + adv2500 = MDIO_AN_10GBT_CTRL_ADV2_5G; >>> Similarly linkmode_adv_to_mii_10gbt_adv_t() can be used here. >>> ret = phy_modify_paged_changed(phydev, 0xa5d, 0x12, >>> - RTL_ADV_2500FULL, adv2500); >>> + MDIO_AN_10GBT_CTRL_ADV2_5G, >>> + adv2500); >>> if (ret < 0) >>> return ret; >>> } >>> @@ -714,11 +707,14 @@ static int rtl822x_read_status(struct phy_device *phydev) >>> return lpadv; >>> >>> linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, >>> - phydev->lp_advertising, lpadv & RTL_LPADV_10000FULL); >>> + phydev->lp_advertising, >>> + lpadv & MDIO_AN_10GBT_STAT_LP10G); >>> linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, >>> - phydev->lp_advertising, lpadv & RTL_LPADV_5000FULL); >>> + phydev->lp_advertising, >>> + lpadv & MDIO_AN_10GBT_STAT_LP5G); >>> linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >>> - phydev->lp_advertising, lpadv & RTL_LPADV_2500FULL); >>> + phydev->lp_advertising, >>> + lpadv & MDIO_AN_10GBT_STAT_LP2_5G); >> >> Is this mii_10gbt_stat_mod_linkmode_lpa_t() ? >> > Indeed, it is. Thanks for the hint. I'd prefer to submit the patch making use > of this helper as a follow-up patch. Then it's obvious that the helper is > the same as the replaced code. > >> Something i've done in the past is to do this sort of conversion to >> standard macros, and the followed up with a patch which says that >> function X is now clearly the same as helper Y, so delete the function >> and use the helper... >> >> Andrew > Heiner
Hi Andrew, On 04.02.2024 17:35, Heiner Kallweit wrote: > On 04.02.2024 17:26, Heiner Kallweit wrote: >> On 04.02.2024 17:00, Andrew Lunn wrote: >>> On Sun, Feb 04, 2024 at 03:17:53PM +0100, Heiner Kallweit wrote: >>>> From: Marek Behún <kabel@kernel.org> >>>> >>>> Drop the ad-hoc MDIO constants used in the driver and use generic >>>> constants instead. >>>> >>>> Signed-off-by: Marek Behún <kabel@kernel.org> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>>> --- >>>> drivers/net/phy/realtek.c | 30 +++++++++++++----------------- >>>> 1 file changed, 13 insertions(+), 17 deletions(-) >>>> >>>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c >>>> index 894172a3e..ffc13c495 100644 >>>> --- a/drivers/net/phy/realtek.c >>>> +++ b/drivers/net/phy/realtek.c >>>> @@ -57,14 +57,6 @@ >>>> #define RTL8366RB_POWER_SAVE 0x15 >>>> #define RTL8366RB_POWER_SAVE_ON BIT(12) >>>> >>>> -#define RTL_SUPPORTS_5000FULL BIT(14) >>>> -#define RTL_SUPPORTS_2500FULL BIT(13) >>>> -#define RTL_SUPPORTS_10000FULL BIT(0) >>>> -#define RTL_ADV_2500FULL BIT(7) >>>> -#define RTL_LPADV_10000FULL BIT(11) >>>> -#define RTL_LPADV_5000FULL BIT(6) >>>> -#define RTL_LPADV_2500FULL BIT(5) >>>> - >>>> #define RTL9000A_GINMR 0x14 >>>> #define RTL9000A_GINMR_LINK_STATUS BIT(4) >>>> >>>> @@ -674,11 +666,11 @@ static int rtl822x_get_features(struct phy_device *phydev) >>>> return val; >>>> >>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >>>> - phydev->supported, val & RTL_SUPPORTS_2500FULL); >>>> + phydev->supported, val & MDIO_PMA_SPEED_2_5G); >>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, >>>> - phydev->supported, val & RTL_SUPPORTS_5000FULL); >>>> + phydev->supported, val & MDIO_PMA_SPEED_5G); >>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, >>>> - phydev->supported, val & RTL_SUPPORTS_10000FULL); >>>> + phydev->supported, val & MDIO_SPEED_10G); >>> >>> Now that this only using generic constants, should it move into mdio.h >>> as a shared helper? Is this a standard register defined in 802.3, just >>> at a different address? >>> >> This is register 1.4 (PMA/PMD speed ability), mapped to a vendor-specific >> register. There's very few users of this register, and nothing where such >> a helper could be reused. >> >>>> >>>> return genphy_read_abilities(phydev); >>>> } >>>> @@ -692,10 +684,11 @@ static int rtl822x_config_aneg(struct phy_device *phydev) >>>> >>>> if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >>>> phydev->advertising)) >>>> - adv2500 = RTL_ADV_2500FULL; >>>> + adv2500 = MDIO_AN_10GBT_CTRL_ADV2_5G; >>>> > > Similarly linkmode_adv_to_mii_10gbt_adv_t() can be used here. > >>>> ret = phy_modify_paged_changed(phydev, 0xa5d, 0x12, >>>> - RTL_ADV_2500FULL, adv2500); >>>> + MDIO_AN_10GBT_CTRL_ADV2_5G, >>>> + adv2500); >>>> if (ret < 0) >>>> return ret; >>>> } >>>> @@ -714,11 +707,14 @@ static int rtl822x_read_status(struct phy_device *phydev) >>>> return lpadv; >>>> >>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, >>>> - phydev->lp_advertising, lpadv & RTL_LPADV_10000FULL); >>>> + phydev->lp_advertising, >>>> + lpadv & MDIO_AN_10GBT_STAT_LP10G); >>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, >>>> - phydev->lp_advertising, lpadv & RTL_LPADV_5000FULL); >>>> + phydev->lp_advertising, >>>> + lpadv & MDIO_AN_10GBT_STAT_LP5G); >>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >>>> - phydev->lp_advertising, lpadv & RTL_LPADV_2500FULL); >>>> + phydev->lp_advertising, >>>> + lpadv & MDIO_AN_10GBT_STAT_LP2_5G); >>> >>> Is this mii_10gbt_stat_mod_linkmode_lpa_t() ? >>> >> Indeed, it is. Thanks for the hint. I'd prefer to submit the patch making use >> of this helper as a follow-up patch. Then it's obvious that the helper is >> the same as the replaced code. >> Is it fine with you to do this in a follow-up patch? The series is marked "under review", so Jakub seems to wait for an outcome of our discussion. >>> Something i've done in the past is to do this sort of conversion to >>> standard macros, and the followed up with a patch which says that >>> function X is now clearly the same as helper Y, so delete the function >>> and use the helper... >>> >>> Andrew >> Heiner > Heiner
On Wed, Feb 07, 2024 at 08:51:29PM +0100, Heiner Kallweit wrote: > Hi Andrew, > > On 04.02.2024 17:35, Heiner Kallweit wrote: > > On 04.02.2024 17:26, Heiner Kallweit wrote: > >> On 04.02.2024 17:00, Andrew Lunn wrote: > >>> On Sun, Feb 04, 2024 at 03:17:53PM +0100, Heiner Kallweit wrote: > >>>> From: Marek Behún <kabel@kernel.org> > >>>> > >>>> Drop the ad-hoc MDIO constants used in the driver and use generic > >>>> constants instead. > >>>> > >>>> Signed-off-by: Marek Behún <kabel@kernel.org> > >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > >>>> --- > >>>> drivers/net/phy/realtek.c | 30 +++++++++++++----------------- > >>>> 1 file changed, 13 insertions(+), 17 deletions(-) > >>>> > >>>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c > >>>> index 894172a3e..ffc13c495 100644 > >>>> --- a/drivers/net/phy/realtek.c > >>>> +++ b/drivers/net/phy/realtek.c > >>>> @@ -57,14 +57,6 @@ > >>>> #define RTL8366RB_POWER_SAVE 0x15 > >>>> #define RTL8366RB_POWER_SAVE_ON BIT(12) > >>>> > >>>> -#define RTL_SUPPORTS_5000FULL BIT(14) > >>>> -#define RTL_SUPPORTS_2500FULL BIT(13) > >>>> -#define RTL_SUPPORTS_10000FULL BIT(0) > >>>> -#define RTL_ADV_2500FULL BIT(7) > >>>> -#define RTL_LPADV_10000FULL BIT(11) > >>>> -#define RTL_LPADV_5000FULL BIT(6) > >>>> -#define RTL_LPADV_2500FULL BIT(5) > >>>> - > >>>> #define RTL9000A_GINMR 0x14 > >>>> #define RTL9000A_GINMR_LINK_STATUS BIT(4) > >>>> > >>>> @@ -674,11 +666,11 @@ static int rtl822x_get_features(struct phy_device *phydev) > >>>> return val; > >>>> > >>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, > >>>> - phydev->supported, val & RTL_SUPPORTS_2500FULL); > >>>> + phydev->supported, val & MDIO_PMA_SPEED_2_5G); > >>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, > >>>> - phydev->supported, val & RTL_SUPPORTS_5000FULL); > >>>> + phydev->supported, val & MDIO_PMA_SPEED_5G); > >>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, > >>>> - phydev->supported, val & RTL_SUPPORTS_10000FULL); > >>>> + phydev->supported, val & MDIO_SPEED_10G); > >>> > >>> Now that this only using generic constants, should it move into mdio.h > >>> as a shared helper? Is this a standard register defined in 802.3, just > >>> at a different address? > >>> > >> This is register 1.4 (PMA/PMD speed ability), mapped to a vendor-specific > >> register. There's very few users of this register, and nothing where such > >> a helper could be reused. Nothing at the moment. If ixgbe ever gets converted to phylib, or at least converted to link modes, it could use it. But i think it should be in mdio.h. That is where people will look for such a helper, and might overlook it here. We want to encourage such helpers and there use. > >>> Is this mii_10gbt_stat_mod_linkmode_lpa_t() ? > >>> > >> Indeed, it is. Thanks for the hint. I'd prefer to submit the patch making use > >> of this helper as a follow-up patch. Then it's obvious that the helper is > >> the same as the replaced code. > >> > Is it fine with you to do this in a follow-up patch? > The series is marked "under review", so Jakub seems to wait for an outcome > of our discussion. Converting to use the standard helps can be a follow-up patch. And moving the helper into mdio.h as well. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 07.02.2024 20:51, Heiner Kallweit wrote: > Hi Andrew, > > On 04.02.2024 17:35, Heiner Kallweit wrote: >> On 04.02.2024 17:26, Heiner Kallweit wrote: >>> On 04.02.2024 17:00, Andrew Lunn wrote: >>>> On Sun, Feb 04, 2024 at 03:17:53PM +0100, Heiner Kallweit wrote: >>>>> From: Marek Behún <kabel@kernel.org> >>>>> >>>>> Drop the ad-hoc MDIO constants used in the driver and use generic >>>>> constants instead. >>>>> >>>>> Signed-off-by: Marek Behún <kabel@kernel.org> >>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>>>> --- >>>>> drivers/net/phy/realtek.c | 30 +++++++++++++----------------- >>>>> 1 file changed, 13 insertions(+), 17 deletions(-) >>>>> >>>>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c >>>>> index 894172a3e..ffc13c495 100644 >>>>> --- a/drivers/net/phy/realtek.c >>>>> +++ b/drivers/net/phy/realtek.c >>>>> @@ -57,14 +57,6 @@ >>>>> #define RTL8366RB_POWER_SAVE 0x15 >>>>> #define RTL8366RB_POWER_SAVE_ON BIT(12) >>>>> >>>>> -#define RTL_SUPPORTS_5000FULL BIT(14) >>>>> -#define RTL_SUPPORTS_2500FULL BIT(13) >>>>> -#define RTL_SUPPORTS_10000FULL BIT(0) >>>>> -#define RTL_ADV_2500FULL BIT(7) >>>>> -#define RTL_LPADV_10000FULL BIT(11) >>>>> -#define RTL_LPADV_5000FULL BIT(6) >>>>> -#define RTL_LPADV_2500FULL BIT(5) >>>>> - >>>>> #define RTL9000A_GINMR 0x14 >>>>> #define RTL9000A_GINMR_LINK_STATUS BIT(4) >>>>> >>>>> @@ -674,11 +666,11 @@ static int rtl822x_get_features(struct phy_device *phydev) >>>>> return val; >>>>> >>>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >>>>> - phydev->supported, val & RTL_SUPPORTS_2500FULL); >>>>> + phydev->supported, val & MDIO_PMA_SPEED_2_5G); >>>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, >>>>> - phydev->supported, val & RTL_SUPPORTS_5000FULL); >>>>> + phydev->supported, val & MDIO_PMA_SPEED_5G); >>>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, >>>>> - phydev->supported, val & RTL_SUPPORTS_10000FULL); >>>>> + phydev->supported, val & MDIO_SPEED_10G); >>>> >>>> Now that this only using generic constants, should it move into mdio.h >>>> as a shared helper? Is this a standard register defined in 802.3, just >>>> at a different address? >>>> >>> This is register 1.4 (PMA/PMD speed ability), mapped to a vendor-specific >>> register. There's very few users of this register, and nothing where such >>> a helper could be reused. >>> When looking a little closer at creating a helper for it, I stumbled across the following. This register just states that the PHY can operate at a certain speed, it leaves open which mode(s) are supported at that speed. I see e.g. 10 different modes for 200Gbps. So it may not be possible to create a generic helper. A translation to linkmodes is only possible if the PHY supports just one mode per speed. >>>>> >>>>> return genphy_read_abilities(phydev); >>>>> } >>>>> @@ -692,10 +684,11 @@ static int rtl822x_config_aneg(struct phy_device *phydev) >>>>> >>>>> if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >>>>> phydev->advertising)) >>>>> - adv2500 = RTL_ADV_2500FULL; >>>>> + adv2500 = MDIO_AN_10GBT_CTRL_ADV2_5G; >>>>> >> >> Similarly linkmode_adv_to_mii_10gbt_adv_t() can be used here. >> >>>>> ret = phy_modify_paged_changed(phydev, 0xa5d, 0x12, >>>>> - RTL_ADV_2500FULL, adv2500); >>>>> + MDIO_AN_10GBT_CTRL_ADV2_5G, >>>>> + adv2500); >>>>> if (ret < 0) >>>>> return ret; >>>>> } >>>>> @@ -714,11 +707,14 @@ static int rtl822x_read_status(struct phy_device *phydev) >>>>> return lpadv; >>>>> >>>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, >>>>> - phydev->lp_advertising, lpadv & RTL_LPADV_10000FULL); >>>>> + phydev->lp_advertising, >>>>> + lpadv & MDIO_AN_10GBT_STAT_LP10G); >>>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, >>>>> - phydev->lp_advertising, lpadv & RTL_LPADV_5000FULL); >>>>> + phydev->lp_advertising, >>>>> + lpadv & MDIO_AN_10GBT_STAT_LP5G); >>>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >>>>> - phydev->lp_advertising, lpadv & RTL_LPADV_2500FULL); >>>>> + phydev->lp_advertising, >>>>> + lpadv & MDIO_AN_10GBT_STAT_LP2_5G); >>>> >>>> Is this mii_10gbt_stat_mod_linkmode_lpa_t() ? >>>> >>> Indeed, it is. Thanks for the hint. I'd prefer to submit the patch making use >>> of this helper as a follow-up patch. Then it's obvious that the helper is >>> the same as the replaced code. >>> > Is it fine with you to do this in a follow-up patch? > The series is marked "under review", so Jakub seems to wait for an outcome > of our discussion. > >>>> Something i've done in the past is to do this sort of conversion to >>>> standard macros, and the followed up with a patch which says that >>>> function X is now clearly the same as helper Y, so delete the function >>>> and use the helper... >>>> >>>> Andrew >>> Heiner >> > Heiner
> >>>>> @@ -674,11 +666,11 @@ static int rtl822x_get_features(struct phy_device *phydev) > >>>>> return val; > >>>>> > >>>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, > >>>>> - phydev->supported, val & RTL_SUPPORTS_2500FULL); > >>>>> + phydev->supported, val & MDIO_PMA_SPEED_2_5G); > >>>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, > >>>>> - phydev->supported, val & RTL_SUPPORTS_5000FULL); > >>>>> + phydev->supported, val & MDIO_PMA_SPEED_5G); > >>>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, > >>>>> - phydev->supported, val & RTL_SUPPORTS_10000FULL); > >>>>> + phydev->supported, val & MDIO_SPEED_10G); > >>>> > >>>> Now that this only using generic constants, should it move into mdio.h > >>>> as a shared helper? Is this a standard register defined in 802.3, just > >>>> at a different address? > >>>> > >>> This is register 1.4 (PMA/PMD speed ability), mapped to a vendor-specific > >>> register. There's very few users of this register, and nothing where such > >>> a helper could be reused. > >>> > > When looking a little closer at creating a helper for it, I stumbled across > the following. This register just states that the PHY can operate at a certain > speed, it leaves open which mode(s) are supported at that speed. O.K, yes, i agree. All it says is speed, nothing more. But that also means this driver should not really be doing this. Is there a full list of registers which are implemented? Is there a way to implement genphy_c45_pma_read_abilities(), or at least the subset needed for this device? That appears to be MDIO_MMD_PMAPMD:MDIO_PMA_NG_EXTABLE and MDIO_MMD_PMAPMD:MDIO_PMA_EXTABLE? Andrew
On 07.02.2024 23:31, Andrew Lunn wrote: >>>>>>> @@ -674,11 +666,11 @@ static int rtl822x_get_features(struct phy_device *phydev) >>>>>>> return val; >>>>>>> >>>>>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >>>>>>> - phydev->supported, val & RTL_SUPPORTS_2500FULL); >>>>>>> + phydev->supported, val & MDIO_PMA_SPEED_2_5G); >>>>>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, >>>>>>> - phydev->supported, val & RTL_SUPPORTS_5000FULL); >>>>>>> + phydev->supported, val & MDIO_PMA_SPEED_5G); >>>>>>> linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, >>>>>>> - phydev->supported, val & RTL_SUPPORTS_10000FULL); >>>>>>> + phydev->supported, val & MDIO_SPEED_10G); >>>>>> >>>>>> Now that this only using generic constants, should it move into mdio.h >>>>>> as a shared helper? Is this a standard register defined in 802.3, just >>>>>> at a different address? >>>>>> >>>>> This is register 1.4 (PMA/PMD speed ability), mapped to a vendor-specific >>>>> register. There's very few users of this register, and nothing where such >>>>> a helper could be reused. >>>>> >> >> When looking a little closer at creating a helper for it, I stumbled across >> the following. This register just states that the PHY can operate at a certain >> speed, it leaves open which mode(s) are supported at that speed. > > O.K, yes, i agree. All it says is speed, nothing more. > > But that also means this driver should not really be doing this. Is > there a full list of registers which are implemented? Is there a way > to implement genphy_c45_pma_read_abilities(), or at least the subset > needed for this device? That appears to be MDIO_MMD_PMAPMD:MDIO_PMA_NG_EXTABLE and > MDIO_MMD_PMAPMD:MDIO_PMA_EXTABLE? > I don't have access to Realtek datasheets, and AFAIK the datasheets don't document the mapping of c45 standard registers to vendor-specific registers. But let me check with my contact at Realtek. > Andrew Heiner
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c index 894172a3e..ffc13c495 100644 --- a/drivers/net/phy/realtek.c +++ b/drivers/net/phy/realtek.c @@ -57,14 +57,6 @@ #define RTL8366RB_POWER_SAVE 0x15 #define RTL8366RB_POWER_SAVE_ON BIT(12) -#define RTL_SUPPORTS_5000FULL BIT(14) -#define RTL_SUPPORTS_2500FULL BIT(13) -#define RTL_SUPPORTS_10000FULL BIT(0) -#define RTL_ADV_2500FULL BIT(7) -#define RTL_LPADV_10000FULL BIT(11) -#define RTL_LPADV_5000FULL BIT(6) -#define RTL_LPADV_2500FULL BIT(5) - #define RTL9000A_GINMR 0x14 #define RTL9000A_GINMR_LINK_STATUS BIT(4) @@ -674,11 +666,11 @@ static int rtl822x_get_features(struct phy_device *phydev) return val; linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, - phydev->supported, val & RTL_SUPPORTS_2500FULL); + phydev->supported, val & MDIO_PMA_SPEED_2_5G); linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, - phydev->supported, val & RTL_SUPPORTS_5000FULL); + phydev->supported, val & MDIO_PMA_SPEED_5G); linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, - phydev->supported, val & RTL_SUPPORTS_10000FULL); + phydev->supported, val & MDIO_SPEED_10G); return genphy_read_abilities(phydev); } @@ -692,10 +684,11 @@ static int rtl822x_config_aneg(struct phy_device *phydev) if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->advertising)) - adv2500 = RTL_ADV_2500FULL; + adv2500 = MDIO_AN_10GBT_CTRL_ADV2_5G; ret = phy_modify_paged_changed(phydev, 0xa5d, 0x12, - RTL_ADV_2500FULL, adv2500); + MDIO_AN_10GBT_CTRL_ADV2_5G, + adv2500); if (ret < 0) return ret; } @@ -714,11 +707,14 @@ static int rtl822x_read_status(struct phy_device *phydev) return lpadv; linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, - phydev->lp_advertising, lpadv & RTL_LPADV_10000FULL); + phydev->lp_advertising, + lpadv & MDIO_AN_10GBT_STAT_LP10G); linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, - phydev->lp_advertising, lpadv & RTL_LPADV_5000FULL); + phydev->lp_advertising, + lpadv & MDIO_AN_10GBT_STAT_LP5G); linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, - phydev->lp_advertising, lpadv & RTL_LPADV_2500FULL); + phydev->lp_advertising, + lpadv & MDIO_AN_10GBT_STAT_LP2_5G); } ret = genphy_read_status(phydev); @@ -736,7 +732,7 @@ static bool rtlgen_supports_2_5gbps(struct phy_device *phydev) val = phy_read(phydev, 0x13); phy_write(phydev, RTL821x_PAGE_SELECT, 0); - return val >= 0 && val & RTL_SUPPORTS_2500FULL; + return val >= 0 && val & MDIO_PMA_SPEED_2_5G; } static int rtlgen_match_phy_device(struct phy_device *phydev)