diff mbox series

[RFC,net-next,1/6] net: phy: realtek: configure SerDes mode for rtl822x/8251b PHYs

Message ID 20240227075151.793496-2-ericwouds@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series rtl8221b/8251b add C45 instances and SerDes switching | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 940 this patch: 940
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 957 this patch: 957
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 957 this patch: 957
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 165 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Eric Woudstra Feb. 27, 2024, 7:51 a.m. UTC 
From: Alexander Couzens <lynxis@fe80.eu>

The rtl822x series and rtl8251b support switching SerDes mode between
2500base-x and sgmii based on the negotiated copper speed.

Configure this switching mode according to SerDes modes supported by
host.

Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
[ refactored, dropped HiSGMII mode and changed commit message ]
Signed-off-by: Marek Behún <kabel@kernel.org>
[ changed rtl822x_update_interface() to use vendor register ]
Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
---
 drivers/net/phy/realtek.c | 96 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 2 deletions(-)

Comments

Russell King (Oracle) Feb. 27, 2024, 10:36 a.m. UTC | #1 
On Tue, Feb 27, 2024 at 08:51:46AM +0100, Eric Woudstra wrote:
> From: Alexander Couzens <lynxis@fe80.eu>
> 
> The rtl822x series and rtl8251b support switching SerDes mode between
> 2500base-x and sgmii based on the negotiated copper speed.
> 
> Configure this switching mode according to SerDes modes supported by
> host.
> 
> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
> [ refactored, dropped HiSGMII mode and changed commit message ]
> Signed-off-by: Marek Behún <kabel@kernel.org>
> [ changed rtl822x_update_interface() to use vendor register ]
> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
> ---
>  drivers/net/phy/realtek.c | 96 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 1fa70427b2a2..67cffe9b7d5d 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -54,6 +54,16 @@
>  						 RTL8201F_ISR_LINK)
>  #define RTL8201F_IER				0x13
>  
> +#define RTL822X_VND1_SERDES_OPTION			0x697a
> +#define RTL822X_VND1_SERDES_OPTION_MODE_MASK		GENMASK(5, 0)
> +#define RTL822X_VND1_SERDES_OPTION_MODE_2500BASEX_SGMII		0
> +#define RTL822X_VND1_SERDES_OPTION_MODE_2500BASEX		2
> +
> +#define RTL822X_VND1_SERDES_CTRL3			0x7580
> +#define RTL822X_VND1_SERDES_CTRL3_MODE_MASK		GENMASK(5, 0)
> +#define RTL822X_VND1_SERDES_CTRL3_MODE_SGMII			0x02
> +#define RTL822X_VND1_SERDES_CTRL3_MODE_2500BASEX		0x16
> +
>  #define RTL8366RB_POWER_SAVE			0x15
>  #define RTL8366RB_POWER_SAVE_ON			BIT(12)
>  
> @@ -659,6 +669,60 @@ static int rtl822x_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
>  	return ret;
>  }
>  
> +static int rtl822x_config_init(struct phy_device *phydev)
> +{
> +	bool has_2500, has_sgmii;
> +	u16 mode;
> +	int ret;
> +
> +	has_2500 = test_bit(PHY_INTERFACE_MODE_2500BASEX,
> +			    phydev->host_interfaces) ||
> +		   phydev->interface == PHY_INTERFACE_MODE_2500BASEX;
> +
> +	has_sgmii = test_bit(PHY_INTERFACE_MODE_SGMII,
> +			     phydev->host_interfaces) ||
> +		    phydev->interface == PHY_INTERFACE_MODE_SGMII;
> +
> +	if (!has_2500 && !has_sgmii)
> +		return 0;
> +
> +	/* fill in possible interfaces */
> +	__assign_bit(PHY_INTERFACE_MODE_2500BASEX, phydev->possible_interfaces,
> +		     has_2500);
> +	__assign_bit(PHY_INTERFACE_MODE_SGMII, phydev->possible_interfaces,
> +		     has_sgmii);

It would be nice to fill phydev->possible_interfaces even if
phydev->host_interfaces has not been populated. That means that the
"newer" paths in phylink can be always used during validation.

In other words, move the if() test just above this to below it.

> +
> +	/* determine SerDes option mode */
> +	if (has_2500 && !has_sgmii)
> +		mode = RTL822X_VND1_SERDES_OPTION_MODE_2500BASEX;
> +	else
> +		mode = RTL822X_VND1_SERDES_OPTION_MODE_2500BASEX_SGMII;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x75f3, 0);
> +	if (ret < 0)
> +		return ret;

It would be nice to know what this is doing.
> +
> +	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_VEND1,
> +				     RTL822X_VND1_SERDES_OPTION,
> +				     RTL822X_VND1_SERDES_OPTION_MODE_MASK,
> +				     mode);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* the following 3 writes into SerDes control are needed for 2500base-x
> +	 * mode to work properly
> +	 */
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x6a04, 0x0503);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x6f10, 0xd455);
> +	if (ret < 0)
> +		return ret;
> +
> +	return phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x6f11, 0x8020);

Also for these. "to work properly" is too vague - is it to do with the
inband signalling?

> +}
> +
>  static int rtl822x_get_features(struct phy_device *phydev)
>  {
>  	int val;
> @@ -695,6 +759,25 @@ static int rtl822x_config_aneg(struct phy_device *phydev)
>  	return __genphy_config_aneg(phydev, ret);
>  }
>  
> +static void rtl822x_update_interface(struct phy_device *phydev)
> +{
> +	int val;
> +
> +	/* Change interface according to serdes mode */
> +	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, RTL822X_VND1_SERDES_CTRL3);
> +	if (val < 0)
> +		return;
> +
> +	switch (val & RTL822X_VND1_SERDES_CTRL3_MODE_MASK) {
> +	case RTL822X_VND1_SERDES_CTRL3_MODE_2500BASEX:
> +		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
> +		break;
> +	case RTL822X_VND1_SERDES_CTRL3_MODE_SGMII:
> +		phydev->interface = PHY_INTERFACE_MODE_SGMII;
> +		break;
> +	}

Just to confirm that this doesn't change existing device behaviour?

Thanks.
Eric Woudstra Feb. 27, 2024, 1:34 p.m. UTC | #2 
On 2/27/24 11:36, Russell King (Oracle) wrote:

> It would be nice to fill phydev->possible_interfaces even if
> phydev->host_interfaces has not been populated. That means that the
> "newer" paths in phylink can be always used during validation.
> 
> In other words, move the if() test just above this to below it.
> 

I will change it accordingly.

>> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x75f3, 0);
>> +	if (ret < 0)
>> +		return ret;
> 
> It would be nice to know what this is doing.
>> +
>> +	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_VEND1,
>> +				     RTL822X_VND1_SERDES_OPTION,
>> +				     RTL822X_VND1_SERDES_OPTION_MODE_MASK,
>> +				     mode);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* the following 3 writes into SerDes control are needed for 2500base-x
>> +	 * mode to work properly
>> +	 */
>> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x6a04, 0x0503);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x6f10, 0xd455);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x6f11, 0x8020);
> 
> Also for these. "to work properly" is too vague - is it to do with the
> inband signalling?
> 
> 
> Just to confirm that this doesn't change existing device behaviour?

This is a magic sequence all needed to change serdes-option-mode and only
that. This patch (series) does not change inband negotiation behavior of
the phy (there is another sequence of magic numbers to disable it).

I will change the comment to describe it better.

Regards,

Eric
Marek Behún Feb. 29, 2024, 12:50 p.m. UTC | #3 
On Tue, 27 Feb 2024 10:36:05 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> > +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x75f3, 0);
> > +	if (ret < 0)
> > +		return ret;  
> 
> It would be nice to know what this is doing.

No documentation for this from Realtek, I guess this was just taken
from SDK originally.

Marek
Daniel Golle Feb. 29, 2024, 4:34 p.m. UTC | #4 
On Thu, Feb 29, 2024 at 01:50:10PM +0100, Marek Behún wrote:
> On Tue, 27 Feb 2024 10:36:05 +0000
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > > +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x75f3, 0);
> > > +	if (ret < 0)
> > > +		return ret;  
> > 
> > It would be nice to know what this is doing.
> 
> No documentation for this from Realtek, I guess this was just taken
> from SDK originally.

There is an additional datasheet for RTL8226B/RTL8221B called
"SERDES MODE SETTING FLOW APPLICATION NOTE" where this sequence to
setup interface and rate adapter mode, and also the sequence to
disable (H)SGMII in-band-status are described.

However, there is no documentation about the meaning of registers
and bits, it's literally just magic numbers and pseudo-code.
Marek Behún March 1, 2024, 9:24 a.m. UTC | #5 
On Thu, 29 Feb 2024 16:34:08 +0000
Daniel Golle <daniel@makrotopia.org> wrote:

> On Thu, Feb 29, 2024 at 01:50:10PM +0100, Marek Behún wrote:
> > On Tue, 27 Feb 2024 10:36:05 +0000
> > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> >   
> > > > +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x75f3, 0);
> > > > +	if (ret < 0)
> > > > +		return ret;    
> > > 
> > > It would be nice to know what this is doing.  
> > 
> > No documentation for this from Realtek, I guess this was just taken
> > from SDK originally.  
> 
> There is an additional datasheet for RTL8226B/RTL8221B called
> "SERDES MODE SETTING FLOW APPLICATION NOTE" where this sequence to
> setup interface and rate adapter mode, and also the sequence to
> disable (H)SGMII in-band-status are described.
> 
> However, there is no documentation about the meaning of registers
> and bits, it's literally just magic numbers and pseudo-code.

Thanks, Daniel.

Eric, can you mention this in the code in a comment?

Marek
diff mbox series

Patch

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 1fa70427b2a2..67cffe9b7d5d 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -54,6 +54,16 @@ 
 						 RTL8201F_ISR_LINK)
 #define RTL8201F_IER				0x13
 
+#define RTL822X_VND1_SERDES_OPTION			0x697a
+#define RTL822X_VND1_SERDES_OPTION_MODE_MASK		GENMASK(5, 0)
+#define RTL822X_VND1_SERDES_OPTION_MODE_2500BASEX_SGMII		0
+#define RTL822X_VND1_SERDES_OPTION_MODE_2500BASEX		2
+
+#define RTL822X_VND1_SERDES_CTRL3			0x7580
+#define RTL822X_VND1_SERDES_CTRL3_MODE_MASK		GENMASK(5, 0)
+#define RTL822X_VND1_SERDES_CTRL3_MODE_SGMII			0x02
+#define RTL822X_VND1_SERDES_CTRL3_MODE_2500BASEX		0x16
+
 #define RTL8366RB_POWER_SAVE			0x15
 #define RTL8366RB_POWER_SAVE_ON			BIT(12)
 
@@ -659,6 +669,60 @@  static int rtl822x_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
 	return ret;
 }
 
+static int rtl822x_config_init(struct phy_device *phydev)
+{
+	bool has_2500, has_sgmii;
+	u16 mode;
+	int ret;
+
+	has_2500 = test_bit(PHY_INTERFACE_MODE_2500BASEX,
+			    phydev->host_interfaces) ||
+		   phydev->interface == PHY_INTERFACE_MODE_2500BASEX;
+
+	has_sgmii = test_bit(PHY_INTERFACE_MODE_SGMII,
+			     phydev->host_interfaces) ||
+		    phydev->interface == PHY_INTERFACE_MODE_SGMII;
+
+	if (!has_2500 && !has_sgmii)
+		return 0;
+
+	/* fill in possible interfaces */
+	__assign_bit(PHY_INTERFACE_MODE_2500BASEX, phydev->possible_interfaces,
+		     has_2500);
+	__assign_bit(PHY_INTERFACE_MODE_SGMII, phydev->possible_interfaces,
+		     has_sgmii);
+
+	/* determine SerDes option mode */
+	if (has_2500 && !has_sgmii)
+		mode = RTL822X_VND1_SERDES_OPTION_MODE_2500BASEX;
+	else
+		mode = RTL822X_VND1_SERDES_OPTION_MODE_2500BASEX_SGMII;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x75f3, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_VEND1,
+				     RTL822X_VND1_SERDES_OPTION,
+				     RTL822X_VND1_SERDES_OPTION_MODE_MASK,
+				     mode);
+	if (ret < 0)
+		return ret;
+
+	/* the following 3 writes into SerDes control are needed for 2500base-x
+	 * mode to work properly
+	 */
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x6a04, 0x0503);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x6f10, 0xd455);
+	if (ret < 0)
+		return ret;
+
+	return phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x6f11, 0x8020);
+}
+
 static int rtl822x_get_features(struct phy_device *phydev)
 {
 	int val;
@@ -695,6 +759,25 @@  static int rtl822x_config_aneg(struct phy_device *phydev)
 	return __genphy_config_aneg(phydev, ret);
 }
 
+static void rtl822x_update_interface(struct phy_device *phydev)
+{
+	int val;
+
+	/* Change interface according to serdes mode */
+	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, RTL822X_VND1_SERDES_CTRL3);
+	if (val < 0)
+		return;
+
+	switch (val & RTL822X_VND1_SERDES_CTRL3_MODE_MASK) {
+	case RTL822X_VND1_SERDES_CTRL3_MODE_2500BASEX:
+		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
+		break;
+	case RTL822X_VND1_SERDES_CTRL3_MODE_SGMII:
+		phydev->interface = PHY_INTERFACE_MODE_SGMII;
+		break;
+	}
+}
+
 static int rtl822x_read_status(struct phy_device *phydev)
 {
 	int ret;
@@ -709,11 +792,13 @@  static int rtl822x_read_status(struct phy_device *phydev)
 						  lpadv);
 	}
 
-	ret = genphy_read_status(phydev);
+	ret = rtlgen_read_status(phydev);
 	if (ret < 0)
 		return ret;
 
-	return rtlgen_get_speed(phydev);
+	rtl822x_update_interface(phydev);
+
+	return 0;
 }
 
 static bool rtlgen_supports_2_5gbps(struct phy_device *phydev)
@@ -976,6 +1061,7 @@  static struct phy_driver realtek_drvs[] = {
 		.match_phy_device = rtl8226_match_phy_device,
 		.get_features	= rtl822x_get_features,
 		.config_aneg	= rtl822x_config_aneg,
+		.config_init    = rtl822x_config_init,
 		.read_status	= rtl822x_read_status,
 		.suspend	= genphy_suspend,
 		.resume		= rtlgen_resume,
@@ -988,6 +1074,7 @@  static struct phy_driver realtek_drvs[] = {
 		.name		= "RTL8226B_RTL8221B 2.5Gbps PHY",
 		.get_features	= rtl822x_get_features,
 		.config_aneg	= rtl822x_config_aneg,
+		.config_init    = rtl822x_config_init,
 		.read_status	= rtl822x_read_status,
 		.suspend	= genphy_suspend,
 		.resume		= rtlgen_resume,
@@ -1000,6 +1087,7 @@  static struct phy_driver realtek_drvs[] = {
 		.name           = "RTL8226-CG 2.5Gbps PHY",
 		.get_features   = rtl822x_get_features,
 		.config_aneg    = rtl822x_config_aneg,
+		.config_init    = rtl822x_config_init,
 		.read_status    = rtl822x_read_status,
 		.suspend        = genphy_suspend,
 		.resume         = rtlgen_resume,
@@ -1010,6 +1098,7 @@  static struct phy_driver realtek_drvs[] = {
 		.name           = "RTL8226B-CG_RTL8221B-CG 2.5Gbps PHY",
 		.get_features   = rtl822x_get_features,
 		.config_aneg    = rtl822x_config_aneg,
+		.config_init    = rtl822x_config_init,
 		.read_status    = rtl822x_read_status,
 		.suspend        = genphy_suspend,
 		.resume         = rtlgen_resume,
@@ -1019,6 +1108,7 @@  static struct phy_driver realtek_drvs[] = {
 		PHY_ID_MATCH_EXACT(0x001cc849),
 		.name           = "RTL8221B-VB-CG 2.5Gbps PHY",
 		.get_features   = rtl822x_get_features,
+		.config_init    = rtl822x_config_init,
 		.config_aneg    = rtl822x_config_aneg,
 		.read_status    = rtl822x_read_status,
 		.suspend        = genphy_suspend,
@@ -1030,6 +1120,7 @@  static struct phy_driver realtek_drvs[] = {
 		.name           = "RTL8221B-VM-CG 2.5Gbps PHY",
 		.get_features   = rtl822x_get_features,
 		.config_aneg    = rtl822x_config_aneg,
+		.config_init    = rtl822x_config_init,
 		.read_status    = rtl822x_read_status,
 		.suspend        = genphy_suspend,
 		.resume         = rtlgen_resume,
@@ -1040,6 +1131,7 @@  static struct phy_driver realtek_drvs[] = {
 		.name           = "RTL8251B 5Gbps PHY",
 		.get_features   = rtl822x_get_features,
 		.config_aneg    = rtl822x_config_aneg,
+		.config_init    = rtl822x_config_init,
 		.read_status    = rtl822x_read_status,
 		.suspend        = genphy_suspend,
 		.resume         = rtlgen_resume,