diff mbox series

[net-next,1/8] net: phy: realtek: rtl8221: allow to configure SERDES mode

Message ID 302d982c5550f10d589735fc2e46cf27386c39f4.1683756691.git.daniel@makrotopia.org
State New
Headers
Series Improvements for RealTek 2.5G Ethernet PHYs |

Commit Message

Alexander Couzens May 10, 2023, 10:53 p.m. UTC 
  The rtl8221 supports multiple SERDES modes:
- SGMII
- 2500base-x
- HiSGMII

Further it supports rate adaption on SERDES links to allow
slow ethernet speeds (10/100/1000mbit) to work on 2500base-x/HiSGMII
links without reducing the SERDES speed.

When operating without rate adapters the SERDES link will follow the
ethernet speed.

Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
---
 drivers/net/phy/realtek.c | 55 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Comments

Andrew Lunn May 11, 2023, 12:34 a.m. UTC | #1 
On Thu, May 11, 2023 at 12:56:12AM +0200, Daniel Golle wrote:
> From: Alexander Couzens <lynxis@fe80.eu>

Hi Daniel

Why are we getting two copies of this patch from different people?

I would expect to see just this version, sent by Daniel Golle and the
first line being From: Alexander Couzens <lynxis@fe80.eu> to indicate
the primary author.

> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>

Your Signed-off-by: should be here as well, since you are submitting
the patch.

    Andrew
Andrew Lunn May 11, 2023, 12:38 a.m. UTC | #2 
> +#define RTL8221B_SERDES_OPTION_MODE_2500BASEX_SGMII	0
> +#define RTL8221B_SERDES_OPTION_MODE_HISGMII_SGMII	1
> +#define RTL8221B_SERDES_OPTION_MODE_2500BASEX		2

So what is 2500BASEX_SGMII? You cannot run SGMII at 2.5G, because
there is no way to repeat a symbol 2.5 times so that a 1G stream takes
up 2.5G bandwidth. The SGMII signalling also does not work at 2.5G.

Please add an explanation what this actually is.

       Andrew
Andrew Lunn May 11, 2023, 12:41 a.m. UTC | #3 
> +#define RTL8221B_MMD_SERDES_CTRL		MDIO_MMD_VEND1
> +#define RTL8221B_MMD_PHY_CTRL			MDIO_MMD_VEND2

I suggest you don't do this. Use MDIO_MMD_VEND[1|2] to make it clear
these are vendor registers.

> +	case RTL8221B_SERDES_OPTION_MODE_2500BASEX_SGMII:
> +	case RTL8221B_SERDES_OPTION_MODE_2500BASEX:
> +		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6a04, 0x0503);
> +		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f10, 0xd455);
> +		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f11, 0x8020);
> +		break;
> +	case RTL8221B_SERDES_OPTION_MODE_HISGMII_SGMII:
> +	case RTL8221B_SERDES_OPTION_MODE_HISGMII:
> +		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6a04, 0x0503);
> +		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f10, 0xd433);
> +		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f11, 0x8020);
> +		break;
> +	}

Is there anything in the datasheet to indicate register names and what
the values mean? It would be good to replace these magic values with
#defines.

	Andrew
Steen Hegelund May 11, 2023, 7:53 a.m. UTC | #4 
Hi Alexander,

On Thu, 2023-05-11 at 00:53 +0200, Alexander Couzens wrote:
> [You don't often get email from lynxis@fe80.eu. Learn why this is important
> athttps://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> The rtl8221 supports multiple SERDES modes:
> - SGMII
> - 2500base-x
> - HiSGMII
> 
> Further it supports rate adaption on SERDES links to allow
> slow ethernet speeds (10/100/1000mbit) to work on 2500base-x/HiSGMII
> links without reducing the SERDES speed.
> 
> When operating without rate adapters the SERDES link will follow the
> ethernet speed.
> 
> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
> ---
>  drivers/net/phy/realtek.c | 55 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 3d99fd6664d7..a7dd5a075135 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -53,6 +53,15 @@
>                                                  RTL8201F_ISR_LINK)
>  #define RTL8201F_IER                           0x13
> 
> +#define RTL8221B_MMD_SERDES_CTRL               MDIO_MMD_VEND1
> +#define RTL8221B_MMD_PHY_CTRL                  MDIO_MMD_VEND2
> +#define RTL8221B_SERDES_OPTION                 0x697a
> +#define RTL8221B_SERDES_OPTION_MODE_MASK       GENMASK(5, 0)
> +#define RTL8221B_SERDES_OPTION_MODE_2500BASEX_SGMII    0
> +#define RTL8221B_SERDES_OPTION_MODE_HISGMII_SGMII      1
> +#define RTL8221B_SERDES_OPTION_MODE_2500BASEX          2
> +#define RTL8221B_SERDES_OPTION_MODE_HISGMII            3
> +
>  #define RTL8366RB_POWER_SAVE                   0x15
>  #define RTL8366RB_POWER_SAVE_ON                        BIT(12)
> 
> @@ -849,6 +858,48 @@ static irqreturn_t rtl9000a_handle_interrupt(struct
> phy_device *phydev)
>         return IRQ_HANDLED;
>  }
> 
> +static int rtl8221b_config_init(struct phy_device *phydev)
> +{
> +       u16 option_mode;
> +
> +       switch (phydev->interface) {
> +       case PHY_INTERFACE_MODE_2500BASEX:
> +               if (!phydev->is_c45) {
> +                       option_mode = RTL8221B_SERDES_OPTION_MODE_2500BASEX;
> +                       break;
> +               }
> +               fallthrough;
> +       case PHY_INTERFACE_MODE_SGMII:
> +               option_mode = RTL8221B_SERDES_OPTION_MODE_2500BASEX_SGMII;
> +               break;
> +       default:
> +               return 0;
> +       }
> +
> +       phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL,
> +                     0x75f3, 0);

Please provide a symbol for the magic value.

> +
> +       phy_modify_mmd_changed(phydev, RTL8221B_MMD_SERDES_CTRL,
> +                              RTL8221B_SERDES_OPTION,
> +                              RTL8221B_SERDES_OPTION_MODE_MASK, option_mode);
> +       switch (option_mode) {
> +       case RTL8221B_SERDES_OPTION_MODE_2500BASEX_SGMII:
> +       case RTL8221B_SERDES_OPTION_MODE_2500BASEX:

This next section also uses a number of magic values.  Please convert to
symbols.

> +               phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6a04,
> 0x0503);
> +               phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f10,
> 0xd455);
> +               phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f11,
> 0x8020);
> +               break;
> +       case RTL8221B_SERDES_OPTION_MODE_HISGMII_SGMII:
> +       case RTL8221B_SERDES_OPTION_MODE_HISGMII:
> +               phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6a04,
> 0x0503);
> +               phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f10,
> 0xd433);
> +               phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f11,
> 0x8020);
> +               break;
> +       }
> +
> +       return 0;
> +}
> +
>  static struct phy_driver realtek_drvs[] = {
>         {
>                 PHY_ID_MATCH_EXACT(0x00008201),
> @@ -970,6 +1021,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    = rtl8221b_config_init,
>                 .read_status    = rtl822x_read_status,
>                 .suspend        = genphy_suspend,
>                 .resume         = rtlgen_resume,
> @@ -992,6 +1044,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    = rtl8221b_config_init,
>                 .read_status    = rtl822x_read_status,
>                 .suspend        = genphy_suspend,
>                 .resume         = rtlgen_resume,
> @@ -1002,6 +1055,7 @@ static struct phy_driver realtek_drvs[] = {
>                 .name           = "RTL8221B-VB-CG 2.5Gbps PHY",
>                 .get_features   = rtl822x_get_features,
>                 .config_aneg    = rtl822x_config_aneg,
> +               .config_init    = rtl8221b_config_init,
>                 .read_status    = rtl822x_read_status,
>                 .suspend        = genphy_suspend,
>                 .resume         = rtlgen_resume,
> @@ -1012,6 +1066,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    = rtl8221b_config_init,
>                 .read_status    = rtl822x_read_status,
>                 .suspend        = genphy_suspend,
>                 .resume         = rtlgen_resume,
> --
> 2.40.0
> 
> 

Otherwise

Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>

Best Regards
Steen
Daniel Golle May 11, 2023, 11:50 a.m. UTC | #5 
On Thu, May 11, 2023 at 02:38:19AM +0200, Andrew Lunn wrote:
> > +#define RTL8221B_SERDES_OPTION_MODE_2500BASEX_SGMII	0
> > +#define RTL8221B_SERDES_OPTION_MODE_HISGMII_SGMII	1
> > +#define RTL8221B_SERDES_OPTION_MODE_2500BASEX		2
> 
> So what is 2500BASEX_SGMII? You cannot run SGMII at 2.5G, because
> there is no way to repeat a symbol 2.5 times so that a 1G stream takes
> up 2.5G bandwidth. The SGMII signalling also does not work at 2.5G.

*_MODE_2500BASEX_SGMII means that the PHY will dynamically switch
interface mode between 2500Base-X for 2500M links and SGMII for
everything else.

Setting  *_MODE_2500BASEX in contrast to that enabled rate-adapter mode
and always uses 2500Base-X no matter what the speed of the link on the
TP interface is.

I will add a comment explaining that.
Daniel Golle May 11, 2023, 11:53 a.m. UTC | #6 
On Thu, May 11, 2023 at 02:41:07AM +0200, Andrew Lunn wrote:
> > +#define RTL8221B_MMD_SERDES_CTRL		MDIO_MMD_VEND1
> > +#define RTL8221B_MMD_PHY_CTRL			MDIO_MMD_VEND2
> 
> I suggest you don't do this. Use MDIO_MMD_VEND[1|2] to make it clear
> these are vendor registers.

Ack, I will not introduce new macros to label them, but just use
MDIO_MMD_VEND1 and MDIO_MMD_VEND2 then.

> 
> > +	case RTL8221B_SERDES_OPTION_MODE_2500BASEX_SGMII:
> > +	case RTL8221B_SERDES_OPTION_MODE_2500BASEX:
> > +		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6a04, 0x0503);
> > +		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f10, 0xd455);
> > +		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f11, 0x8020);
> > +		break;
> > +	case RTL8221B_SERDES_OPTION_MODE_HISGMII_SGMII:
> > +	case RTL8221B_SERDES_OPTION_MODE_HISGMII:
> > +		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6a04, 0x0503);
> > +		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f10, 0xd433);
> > +		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f11, 0x8020);
> > +		break;
> > +	}
> 
> Is there anything in the datasheet to indicate register names and what
> the values mean? It would be good to replace these magic values with
> #defines.

Unfortunately they are only mentioned as magic values which have to be
written to magic registers also in the datasheet :(
diff mbox series

Patch

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 3d99fd6664d7..a7dd5a075135 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -53,6 +53,15 @@ 
 						 RTL8201F_ISR_LINK)
 #define RTL8201F_IER				0x13
 
+#define RTL8221B_MMD_SERDES_CTRL		MDIO_MMD_VEND1
+#define RTL8221B_MMD_PHY_CTRL			MDIO_MMD_VEND2
+#define RTL8221B_SERDES_OPTION			0x697a
+#define RTL8221B_SERDES_OPTION_MODE_MASK	GENMASK(5, 0)
+#define RTL8221B_SERDES_OPTION_MODE_2500BASEX_SGMII	0
+#define RTL8221B_SERDES_OPTION_MODE_HISGMII_SGMII	1
+#define RTL8221B_SERDES_OPTION_MODE_2500BASEX		2
+#define RTL8221B_SERDES_OPTION_MODE_HISGMII		3
+
 #define RTL8366RB_POWER_SAVE			0x15
 #define RTL8366RB_POWER_SAVE_ON			BIT(12)
 
@@ -849,6 +858,48 @@  static irqreturn_t rtl9000a_handle_interrupt(struct phy_device *phydev)
 	return IRQ_HANDLED;
 }
 
+static int rtl8221b_config_init(struct phy_device *phydev)
+{
+	u16 option_mode;
+
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_2500BASEX:
+		if (!phydev->is_c45) {
+			option_mode = RTL8221B_SERDES_OPTION_MODE_2500BASEX;
+			break;
+		}
+		fallthrough;
+	case PHY_INTERFACE_MODE_SGMII:
+		option_mode = RTL8221B_SERDES_OPTION_MODE_2500BASEX_SGMII;
+		break;
+	default:
+		return 0;
+	}
+
+	phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL,
+		      0x75f3, 0);
+
+	phy_modify_mmd_changed(phydev, RTL8221B_MMD_SERDES_CTRL,
+			       RTL8221B_SERDES_OPTION,
+			       RTL8221B_SERDES_OPTION_MODE_MASK, option_mode);
+	switch (option_mode) {
+	case RTL8221B_SERDES_OPTION_MODE_2500BASEX_SGMII:
+	case RTL8221B_SERDES_OPTION_MODE_2500BASEX:
+		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6a04, 0x0503);
+		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f10, 0xd455);
+		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f11, 0x8020);
+		break;
+	case RTL8221B_SERDES_OPTION_MODE_HISGMII_SGMII:
+	case RTL8221B_SERDES_OPTION_MODE_HISGMII:
+		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6a04, 0x0503);
+		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f10, 0xd433);
+		phy_write_mmd(phydev, RTL8221B_MMD_SERDES_CTRL, 0x6f11, 0x8020);
+		break;
+	}
+
+	return 0;
+}
+
 static struct phy_driver realtek_drvs[] = {
 	{
 		PHY_ID_MATCH_EXACT(0x00008201),
@@ -970,6 +1021,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	= rtl8221b_config_init,
 		.read_status	= rtl822x_read_status,
 		.suspend	= genphy_suspend,
 		.resume		= rtlgen_resume,
@@ -992,6 +1044,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    = rtl8221b_config_init,
 		.read_status    = rtl822x_read_status,
 		.suspend        = genphy_suspend,
 		.resume         = rtlgen_resume,
@@ -1002,6 +1055,7 @@  static struct phy_driver realtek_drvs[] = {
 		.name           = "RTL8221B-VB-CG 2.5Gbps PHY",
 		.get_features   = rtl822x_get_features,
 		.config_aneg    = rtl822x_config_aneg,
+		.config_init    = rtl8221b_config_init,
 		.read_status    = rtl822x_read_status,
 		.suspend        = genphy_suspend,
 		.resume         = rtlgen_resume,
@@ -1012,6 +1066,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    = rtl8221b_config_init,
 		.read_status    = rtl822x_read_status,
 		.suspend        = genphy_suspend,
 		.resume         = rtlgen_resume,