| Message ID | 20241202195029.2045633-1-kmlinuxm@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/2] net: phy: realtek: add combo mode support for RTL8211FS | expand |
> +static int rtl8211f_config_aneg(struct phy_device *phydev) > +{ > + int ret; > + > + struct rtl821x_priv *priv = phydev->priv; > + > + ret = genphy_read_abilities(phydev); > + if (ret < 0) > + return ret; > + > + linkmode_copy(phydev->advertising, phydev->supported); This is all very unusual for config_aneg(). genphy_read_abilities() will have been done very early on during phy_probe(). So why do it now? And why overwrite how the user might of configured what is to be advertised? > +static int rtl8211f_read_status(struct phy_device *phydev) > +{ > + int ret; > + struct rtl821x_priv *priv = phydev->priv; > + bool changed = false; > + > + if (rtl8211f_mode(phydev) != priv->lastmode) { > + changed = true; > + ret = rtl8211f_config_aneg(phydev); > + if (ret < 0) > + return ret; > + > + ret = genphy_restart_aneg(phydev); > + if (ret < 0) > + return ret; > + } > + > + return genphy_c37_read_status(phydev, &changed); > +} So you are assuming read_status() is called once per second? But what about when interrupts are used? You might want to look at how the marvell driver does this. It is not great, but better than this. Andrew --- pw-bot: cr
On 2024/12/3 7:52, Andrew Lunn wrote: >> +static int rtl8211f_config_aneg(struct phy_device *phydev) >> +{ >> + int ret; >> + >> + struct rtl821x_priv *priv = phydev->priv; >> + >> + ret = genphy_read_abilities(phydev); >> + if (ret < 0) >> + return ret; >> + >> + linkmode_copy(phydev->advertising, phydev->supported); > > This is all very unusual for config_aneg(). genphy_read_abilities() > will have been done very early on during phy_probe(). So why do it > now? And why overwrite how the user might of configured what is to be > advertised? > These codes are migrated from Rockchip SDK and I'm not familiar with this part. I will use `linkmode_and` instead of `linkmode_copy` in my next version of patch like Marvell does. >> +static int rtl8211f_read_status(struct phy_device *phydev) >> +{ >> + int ret; >> + struct rtl821x_priv *priv = phydev->priv; >> + bool changed = false; >> + >> + if (rtl8211f_mode(phydev) != priv->lastmode) { >> + changed = true; >> + ret = rtl8211f_config_aneg(phydev); >> + if (ret < 0) >> + return ret; >> + >> + ret = genphy_restart_aneg(phydev); >> + if (ret < 0) >> + return ret; >> + } >> + >> + return genphy_c37_read_status(phydev, &changed); >> +} > > So you are assuming read_status() is called once per second? But what > about when interrupts are used? > > You might want to look at how the marvell driver does this. It is not > great, but better than this. I'm not familiar with that either, could you please tell me how to do it properly to automatically switch between copper and fiber mode? > > Andrew > > --- > pw-bot: cr Sincerely, Zhiyuan Wan
On Tue, Dec 03, 2024 at 11:08:22AM +0800, 万致远 wrote: > On 2024/12/3 7:52, Andrew Lunn wrote: > >> +static int rtl8211f_config_aneg(struct phy_device *phydev) > >> +{ > >> + int ret; > >> + > >> + struct rtl821x_priv *priv = phydev->priv; > >> + > >> + ret = genphy_read_abilities(phydev); > >> + if (ret < 0) > >> + return ret; > >> + > >> + linkmode_copy(phydev->advertising, phydev->supported); > > > > This is all very unusual for config_aneg(). genphy_read_abilities() > > will have been done very early on during phy_probe(). So why do it > > now? And why overwrite how the user might of configured what is to be > > advertised? > > > > These codes are migrated from Rockchip SDK and I'm not familiar with this part. > > I will use `linkmode_and` instead of `linkmode_copy` in my next > version of patch like Marvell does. No, it needs a lot more work than just that. Spend some time to really understand how the marvell driver handles either copper or fibre, and assume the Rockchip SDK is poor quality code. It might also be that the marvell scheme does not work. It will depend on how the PHY actually works. Andrew
On 2024/12/3 11:53, Andrew Lunn wrote: > No, it needs a lot more work than just that. Spend some time to really > understand how the marvell driver handles either copper or fibre, and > assume the Rockchip SDK is poor quality code. > I'm currently busy for a new IC project so I might not have much time working on this patch, would you like to accept the another patch about broadcasting PHY address? > It might also be that the marvell scheme does not work. It will depend > on how the PHY actually works. In fact I don't familiar with how thisworks and which function will handle link-state change callback, yeh, Rockchip SDK is indeed filled with low-quality code but it Sincerely, Zhiyuan Wan
On 2024/12/3 11:53, Andrew Lunn wrote: > No, it needs a lot more work than just that. Spend some time to really > understand how the marvell driver handles either copper or fibre, and > assume the Rockchip SDK is poor quality code. > I'm currently busy in a new IC project so I might not have much time working on this patch, would you like to accept another patch about broadcast PHY address? > It might also be that the marvell scheme does not work. It will depend > on how the PHY actually works. In fact I don't familiar with how this code works and which function will handle link-state change callback. But yeh, Rockchip SDK is indeed filled with low-quality code,, although it works. Maybe I'll get back to this patch several weeks later. I'm sorry for that. Sincerely, Zhiyuan Wan
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c index f65d7f1f3..10a87d58c 100644 --- a/drivers/net/phy/realtek.c +++ b/drivers/net/phy/realtek.c @@ -32,6 +32,11 @@ #define RTL8211F_PHYCR2 0x19 #define RTL8211F_INSR 0x1d +#define RTL8211FS_FIBER_ESR 0x0F +#define RTL8211FS_MODE_MASK 0xC000 +#define RTL8211FS_MODE_COPPER 0 +#define RTL8211FS_MODE_FIBER 1 + #define RTL8211F_LEDCR 0x10 #define RTL8211F_LEDCR_MODE BIT(15) #define RTL8211F_LEDCR_ACT_TXRX BIT(4) @@ -110,6 +115,7 @@ struct rtl821x_priv { u16 phycr1; u16 phycr2; bool has_phycr2; + int lastmode; struct clk *clk; }; @@ -163,6 +169,44 @@ static int rtl821x_probe(struct phy_device *phydev) return 0; } +static int rtl8211f_mode(struct phy_device *phydev) +{ + u16 val; + + val = __phy_read(phydev, RTL8211FS_FIBER_ESR); + val &= RTL8211FS_MODE_MASK; + + if (val) + return RTL8211FS_MODE_FIBER; + else + return RTL8211FS_MODE_COPPER; +} + +static int rtl8211f_config_aneg(struct phy_device *phydev) +{ + int ret; + + struct rtl821x_priv *priv = phydev->priv; + + ret = genphy_read_abilities(phydev); + if (ret < 0) + return ret; + + linkmode_copy(phydev->advertising, phydev->supported); + + if (rtl8211f_mode(phydev) == RTL8211FS_MODE_FIBER) { + dev_dbg(&phydev->mdio.dev, "fiber link up"); + priv->lastmode = RTL8211FS_MODE_FIBER; + return genphy_c37_config_aneg(phydev); + } + + dev_dbg(&phydev->mdio.dev, "copper link up"); + + priv->lastmode = RTL8211FS_MODE_COPPER; + + return genphy_config_aneg(phydev); +} + static int rtl8201_ack_interrupt(struct phy_device *phydev) { int err; @@ -732,6 +776,26 @@ static int rtlgen_read_status(struct phy_device *phydev) return 0; } +static int rtl8211f_read_status(struct phy_device *phydev) +{ + int ret; + struct rtl821x_priv *priv = phydev->priv; + bool changed = false; + + if (rtl8211f_mode(phydev) != priv->lastmode) { + changed = true; + ret = rtl8211f_config_aneg(phydev); + if (ret < 0) + return ret; + + ret = genphy_restart_aneg(phydev); + if (ret < 0) + return ret; + } + + return genphy_c37_read_status(phydev, &changed); +} + static int rtlgen_read_mmd(struct phy_device *phydev, int devnum, u16 regnum) { int ret; @@ -1375,7 +1439,8 @@ static struct phy_driver realtek_drvs[] = { .name = "RTL8211F Gigabit Ethernet", .probe = rtl821x_probe, .config_init = &rtl8211f_config_init, - .read_status = rtlgen_read_status, + .config_aneg = rtl8211f_config_aneg, + .read_status = rtl8211f_read_status, .config_intr = &rtl8211f_config_intr, .handle_interrupt = rtl8211f_handle_interrupt, .suspend = rtl821x_suspend,