* [PATCH net-next 0/3] net: phy: realtek: complete 5Gbps support and replace private constants
@ 2024-02-04 14:15 Heiner Kallweit
2024-02-04 14:16 ` [PATCH net-next 1/3] net: mdio: add 2.5g and 5g related PMA speed constants Heiner Kallweit
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Heiner Kallweit @ 2024-02-04 14:15 UTC (permalink / raw)
To: Marek Behún, Andrew Lunn, Russell King - ARM Linux,
Jakub Kicinski, David Miller, Paolo Abeni, Eric Dumazet
Cc: netdev@vger•kernel.org
Realtek maps standard C45 registers to vendor-specific registers which
can be accessed via C22 w/o MMD. For an unknown reason C22 MMD access
to C45 registers isn't supported for integrated PHY's.
However the vendor-specific registers preserve the format of the C45
registers, so we can use standard constants. First two patches are
cherry-picked from a series posted by Marek some time ago.
RTL8126 supports 5Gbps, therefore add the missing 5Gbps support to
rtl822x_config_aneg().
Heiner Kallweit (1):
net: phy: realtek: add 5Gbps support to rtl822x_config_aneg()
Marek Behún (2):
net: mdio: add 2.5g and 5g related PMA speed constants
net: phy: realtek: use generic MDIO constants
drivers/net/phy/realtek.c | 36 ++++++++++++++++++------------------
include/uapi/linux/mdio.h | 2 ++
2 files changed, 20 insertions(+), 18 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next 1/3] net: mdio: add 2.5g and 5g related PMA speed constants
2024-02-04 14:15 [PATCH net-next 0/3] net: phy: realtek: complete 5Gbps support and replace private constants Heiner Kallweit
@ 2024-02-04 14:16 ` Heiner Kallweit
2024-02-04 14:17 ` [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants Heiner Kallweit
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Heiner Kallweit @ 2024-02-04 14:16 UTC (permalink / raw)
To: Marek Behún, Andrew Lunn, Russell King - ARM Linux,
Jakub Kicinski, David Miller, Paolo Abeni, Eric Dumazet
Cc: netdev@vger•kernel.org
From: Marek Behún <kabel@kernel•org>
Add constants indicating 2.5g and 5g ability in the MMD PMA speed
register.
Signed-off-by: Marek Behún <kabel@kernel•org>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail•com>
---
include/uapi/linux/mdio.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index d03863da1..3c9097502 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h
@@ -138,6 +138,8 @@
#define MDIO_PMA_SPEED_1000 0x0010 /* 1000M capable */
#define MDIO_PMA_SPEED_100 0x0020 /* 100M capable */
#define MDIO_PMA_SPEED_10 0x0040 /* 10M capable */
+#define MDIO_PMA_SPEED_2_5G 0x2000 /* 2.5G capable */
+#define MDIO_PMA_SPEED_5G 0x4000 /* 5G capable */
#define MDIO_PCS_SPEED_10P2B 0x0002 /* 10PASS-TS/2BASE-TL capable */
#define MDIO_PCS_SPEED_2_5G 0x0040 /* 2.5G capable */
#define MDIO_PCS_SPEED_5G 0x0080 /* 5G capable */
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants
2024-02-04 14:15 [PATCH net-next 0/3] net: phy: realtek: complete 5Gbps support and replace private constants Heiner Kallweit
2024-02-04 14:16 ` [PATCH net-next 1/3] net: mdio: add 2.5g and 5g related PMA speed constants Heiner Kallweit
@ 2024-02-04 14:17 ` Heiner Kallweit
2024-02-04 16:00 ` Andrew Lunn
2024-02-04 14:18 ` [PATCH net-next 3/3] net: phy: realtek: add 5Gbps support to rtl822x_config_aneg() Heiner Kallweit
2024-02-08 2:30 ` [PATCH net-next 0/3] net: phy: realtek: complete 5Gbps support and replace private constants patchwork-bot+netdevbpf
3 siblings, 1 reply; 13+ messages in thread
From: Heiner Kallweit @ 2024-02-04 14:17 UTC (permalink / raw)
To: Marek Behún, Andrew Lunn, Russell King - ARM Linux,
Jakub Kicinski, David Miller, Paolo Abeni, Eric Dumazet
Cc: netdev@vger•kernel.org
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);
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)
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net-next 3/3] net: phy: realtek: add 5Gbps support to rtl822x_config_aneg()
2024-02-04 14:15 [PATCH net-next 0/3] net: phy: realtek: complete 5Gbps support and replace private constants Heiner Kallweit
2024-02-04 14:16 ` [PATCH net-next 1/3] net: mdio: add 2.5g and 5g related PMA speed constants Heiner Kallweit
2024-02-04 14:17 ` [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants Heiner Kallweit
@ 2024-02-04 14:18 ` Heiner Kallweit
2024-02-08 2:30 ` [PATCH net-next 0/3] net: phy: realtek: complete 5Gbps support and replace private constants patchwork-bot+netdevbpf
3 siblings, 0 replies; 13+ messages in thread
From: Heiner Kallweit @ 2024-02-04 14:18 UTC (permalink / raw)
To: Marek Behún, Andrew Lunn, Russell King - ARM Linux,
Jakub Kicinski, David Miller, Paolo Abeni, Eric Dumazet
Cc: netdev@vger•kernel.org
RTL8126 as an evolution of RTL8125 supports 5Gbps. rtl822x_config_aneg()
is used by the PHY driver for the integrated PHY, therefore add 5Gbps
support to it.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail•com>
---
drivers/net/phy/realtek.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index ffc13c495..06f6672a4 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -680,15 +680,19 @@ static int rtl822x_config_aneg(struct phy_device *phydev)
int ret = 0;
if (phydev->autoneg == AUTONEG_ENABLE) {
- u16 adv2500 = 0;
+ u16 adv = 0;
if (linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
phydev->advertising))
- adv2500 = MDIO_AN_10GBT_CTRL_ADV2_5G;
+ adv |= MDIO_AN_10GBT_CTRL_ADV2_5G;
+ if (linkmode_test_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+ phydev->advertising))
+ adv |= MDIO_AN_10GBT_CTRL_ADV5G;
ret = phy_modify_paged_changed(phydev, 0xa5d, 0x12,
- MDIO_AN_10GBT_CTRL_ADV2_5G,
- adv2500);
+ MDIO_AN_10GBT_CTRL_ADV2_5G |
+ MDIO_AN_10GBT_CTRL_ADV5G,
+ adv);
if (ret < 0)
return ret;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants
2024-02-04 14:17 ` [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants Heiner Kallweit
@ 2024-02-04 16:00 ` Andrew Lunn
2024-02-04 16:26 ` Heiner Kallweit
0 siblings, 1 reply; 13+ messages in thread
From: Andrew Lunn @ 2024-02-04 16:00 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Marek Behún, Russell King - ARM Linux, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, netdev@vger•kernel.org
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants
2024-02-04 16:00 ` Andrew Lunn
@ 2024-02-04 16:26 ` Heiner Kallweit
2024-02-04 16:35 ` Heiner Kallweit
0 siblings, 1 reply; 13+ messages in thread
From: Heiner Kallweit @ 2024-02-04 16:26 UTC (permalink / raw)
To: Andrew Lunn
Cc: Marek Behún, Russell King - ARM Linux, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, netdev@vger•kernel.org
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants
2024-02-04 16:26 ` Heiner Kallweit
@ 2024-02-04 16:35 ` Heiner Kallweit
2024-02-07 19:51 ` Heiner Kallweit
0 siblings, 1 reply; 13+ messages in thread
From: Heiner Kallweit @ 2024-02-04 16:35 UTC (permalink / raw)
To: Andrew Lunn
Cc: Marek Behún, Russell King - ARM Linux, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, netdev@vger•kernel.org
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants
2024-02-04 16:35 ` Heiner Kallweit
@ 2024-02-07 19:51 ` Heiner Kallweit
2024-02-07 20:28 ` Andrew Lunn
2024-02-07 21:19 ` Heiner Kallweit
0 siblings, 2 replies; 13+ messages in thread
From: Heiner Kallweit @ 2024-02-07 19:51 UTC (permalink / raw)
To: Andrew Lunn
Cc: Marek Behún, Russell King - ARM Linux, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, netdev@vger•kernel.org
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants
2024-02-07 19:51 ` Heiner Kallweit
@ 2024-02-07 20:28 ` Andrew Lunn
2024-02-07 21:19 ` Heiner Kallweit
1 sibling, 0 replies; 13+ messages in thread
From: Andrew Lunn @ 2024-02-07 20:28 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Marek Behún, Russell King - ARM Linux, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, netdev@vger•kernel.org
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants
2024-02-07 19:51 ` Heiner Kallweit
2024-02-07 20:28 ` Andrew Lunn
@ 2024-02-07 21:19 ` Heiner Kallweit
2024-02-07 22:31 ` Andrew Lunn
1 sibling, 1 reply; 13+ messages in thread
From: Heiner Kallweit @ 2024-02-07 21:19 UTC (permalink / raw)
To: Andrew Lunn
Cc: Marek Behún, Russell King - ARM Linux, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, netdev@vger•kernel.org
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants
2024-02-07 21:19 ` Heiner Kallweit
@ 2024-02-07 22:31 ` Andrew Lunn
2024-02-08 7:18 ` Heiner Kallweit
0 siblings, 1 reply; 13+ messages in thread
From: Andrew Lunn @ 2024-02-07 22:31 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Marek Behún, Russell King - ARM Linux, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, netdev@vger•kernel.org
> >>>>> @@ -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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 0/3] net: phy: realtek: complete 5Gbps support and replace private constants
2024-02-04 14:15 [PATCH net-next 0/3] net: phy: realtek: complete 5Gbps support and replace private constants Heiner Kallweit
` (2 preceding siblings ...)
2024-02-04 14:18 ` [PATCH net-next 3/3] net: phy: realtek: add 5Gbps support to rtl822x_config_aneg() Heiner Kallweit
@ 2024-02-08 2:30 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 13+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-02-08 2:30 UTC (permalink / raw)
To: Heiner Kallweit
Cc: kabel, andrew, linux, kuba, davem, pabeni, edumazet, netdev
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel•org>:
On Sun, 4 Feb 2024 15:15:00 +0100 you wrote:
> Realtek maps standard C45 registers to vendor-specific registers which
> can be accessed via C22 w/o MMD. For an unknown reason C22 MMD access
> to C45 registers isn't supported for integrated PHY's.
> However the vendor-specific registers preserve the format of the C45
> registers, so we can use standard constants. First two patches are
> cherry-picked from a series posted by Marek some time ago.
>
> [...]
Here is the summary with links:
- [net-next,1/3] net: mdio: add 2.5g and 5g related PMA speed constants
https://git.kernel.org/netdev/net-next/c/6c06c88fa838
- [net-next,2/3] net: phy: realtek: use generic MDIO constants
https://git.kernel.org/netdev/net-next/c/2b9ec5dfb825
- [net-next,3/3] net: phy: realtek: add 5Gbps support to rtl822x_config_aneg()
https://git.kernel.org/netdev/net-next/c/db1bb7741ff2
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants
2024-02-07 22:31 ` Andrew Lunn
@ 2024-02-08 7:18 ` Heiner Kallweit
0 siblings, 0 replies; 13+ messages in thread
From: Heiner Kallweit @ 2024-02-08 7:18 UTC (permalink / raw)
To: Andrew Lunn
Cc: Marek Behún, Russell King - ARM Linux, Jakub Kicinski,
David Miller, Paolo Abeni, Eric Dumazet, netdev@vger•kernel.org
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-02-08 7:18 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-04 14:15 [PATCH net-next 0/3] net: phy: realtek: complete 5Gbps support and replace private constants Heiner Kallweit
2024-02-04 14:16 ` [PATCH net-next 1/3] net: mdio: add 2.5g and 5g related PMA speed constants Heiner Kallweit
2024-02-04 14:17 ` [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants Heiner Kallweit
2024-02-04 16:00 ` Andrew Lunn
2024-02-04 16:26 ` Heiner Kallweit
2024-02-04 16:35 ` Heiner Kallweit
2024-02-07 19:51 ` Heiner Kallweit
2024-02-07 20:28 ` Andrew Lunn
2024-02-07 21:19 ` Heiner Kallweit
2024-02-07 22:31 ` Andrew Lunn
2024-02-08 7:18 ` Heiner Kallweit
2024-02-04 14:18 ` [PATCH net-next 3/3] net: phy: realtek: add 5Gbps support to rtl822x_config_aneg() Heiner Kallweit
2024-02-08 2:30 ` [PATCH net-next 0/3] net: phy: realtek: complete 5Gbps support and replace private constants patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox