[PATCH net-next 0/3] net: phy: realtek: complete 5Gbps support and replace private constants

[PATCH net-next 0/3] net: phy: realtek: complete 5Gbps support and replace private constants

[Heiner Kallweit]

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

[PATCH net-next 1/3] net: mdio: add 2.5g and 5g related PMA speed constants

[Heiner Kallweit]

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

[PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants

[Heiner Kallweit]

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

Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants

[Andrew Lunn]

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

Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants

[Heiner Kallweit]

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

Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants

[Heiner Kallweit]

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

Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants

[Heiner Kallweit]

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

Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants

[Andrew Lunn]

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

Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants

[Heiner Kallweit]

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

Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants

[Andrew Lunn]

> >>>>> @@ -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

Re: [PATCH net-next 2/3] net: phy: realtek: use generic MDIO constants

[Heiner Kallweit]

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

[PATCH net-next 3/3] net: phy: realtek: add 5Gbps support to rtl822x_config_aneg()

[Heiner Kallweit]

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

Re: [PATCH net-next 0/3] net: phy: realtek: complete 5Gbps support and replace private constants

[patchwork-bot+netdevbpf]

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