| Message ID | 302d982c5550f10d589735fc2e46cf27386c39f4.1683756691.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Improvements for RealTek 2.5G Ethernet PHYs | expand |
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
> +#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
> +#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
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
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.
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 --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,
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(+)