| Message ID | 20241203071853.2067014-1-kmlinuxm@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,1/2] net: phy: realtek: disable broadcast address feature of rtl8211f | expand |
On 03.12.2024 08:18, Zhiyuan Wan wrote: > This feature is enabled defaultly after a reset of this transceiver. > When this feature is enabled, the phy not only responds to the > configuration PHY address by pin states on board, but also responds > to address 0, the optional broadcast address of the MDIO bus. > > But some MDIO device like mt7530 switch chip (integrated in mt7621 > SoC), also use address 0 to configure a specific port, when use > mt7530 and rtl8211f together, it usually causes address conflict, > leads to the port of RTL8211FS stops working. > > This patch disables broadcast address feature of rtl8211f, and > returns -ENODEV if using broadcast address (0) as phy address. > > Reviewed-by: Yuki Lee <febrieac@outlook.com> Take care to remove the Rb tag if you make changes to the patch. > Signed-off-by: Zhiyuan Wan <kmlinuxm@gmail.com> > --- > drivers/net/phy/realtek.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c > index f65d7f1f3..8a38b02ad 100644 > --- a/drivers/net/phy/realtek.c > +++ b/drivers/net/phy/realtek.c > @@ -31,6 +31,7 @@ > #define RTL8211F_PHYCR1 0x18 > #define RTL8211F_PHYCR2 0x19 > #define RTL8211F_INSR 0x1d > +#define RTL8211F_PHYAD0_EN BIT(13) > > #define RTL8211F_LEDCR 0x10 > #define RTL8211F_LEDCR_MODE BIT(15) > @@ -139,6 +140,17 @@ static int rtl821x_probe(struct phy_device *phydev) > return dev_err_probe(dev, PTR_ERR(priv->clk), > "failed to get phy clock\n"); > > + dev_dbg(dev, "disabling MDIO address 0 for this phy"); > + ret = phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, > + RTL8211F_PHYAD0_EN, 0); This still uses the _changed version even if not needed. > + if (ret < 0) { > + dev_err(dev, "disabling MDIO address 0 failed: %pe\n", > + ERR_PTR(ret)); You may want to use dev_err_probe() here. And is it by intent that the error is ignored and you go on? > + } > + /* Don't allow using broadcast address as PHY address */ > + if (phydev->mdio.addr == 0) > + return -ENODEV; > + > ret = phy_read_paged(phydev, 0xa43, RTL8211F_PHYCR1); > if (ret < 0) > return ret; And one more formal issue: You annotated the patch as 1/2, but submit it as single patch.
On 2024/12/3 15:38, Heiner Kallweit wrote: > > Take care to remove the Rb tag if you make changes to the patch. > Roger that. > > This still uses the _changed version even if not needed. > I'm not sure is it okay to directly write PHYCR1 register, because not only it controls ALDPS and broadcast PHY address, but also controls MDI mode/Jabber detection. I'm afraid that maybe it causes problem if I don't use RMW to clear the PHYAD_EN bit. Because the following code in `rtl8211f_config_init` also utilizes `phy_modify_paged_changed` to change ALDPS setting without touching anything else. But if you insist, I can modify code to this if you like: ret = phy_read_paged(phydev, 0xa43, RTL8211F_PHYCR1); if (ret < 0) return ret; dev_dbg(dev, "disabling MDIO address 0 for this phy"); priv->phycr1 = ret & (u16)~RTL8211F_PHYAD0_EN; ret = phy_write_paged(phydev, 0xa43, RTL8211F_PHYCR1, priv->phycr1); if (ret < 0) { return dev_err_probe(dev, ret, "disabling MDIO address 0 failed\n"); } /* Don't allow using broadcast address as PHY address */ if (phydev->mdio.addr == 0) return -ENODEV; priv->phycr1 &= (RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF); ... >> + if (ret < 0) { >> + dev_err(dev, "disabling MDIO address 0 failed: %pe\n", >> + ERR_PTR(ret)); > > You may want to use dev_err_probe() here. And is it by intent that > the error is ignored and you go on? > I'm sorry that I made a mistake. > > And one more formal issue: > You annotated the patch as 1/2, but submit it as single patch. > I have another patch to enable support optical/copper combo support of RTL8211FS PHY in this mail thread, but since Andrew said that patch (migrated from Rockchip SDK) is low quality and I'm too busy with my job, don't have much time to read and improve it, so I decided to suspend that patch's submission and I'll resume to submit that patch when I'm free. Could you please give me some advice or recommends on it? Sincerely, Zhiyuan Wan
On 03.12.2024 09:35, Zhiyuan Wan wrote: > On 2024/12/3 15:38, Heiner Kallweit wrote: >> >> Take care to remove the Rb tag if you make changes to the patch. >> > Roger that. >> >> This still uses the _changed version even if not needed. >> > I'm not sure is it okay to directly write PHYCR1 register, because not > only it controls ALDPS and broadcast PHY address, but also controls > MDI mode/Jabber detection. > This was not my point. Just use phy_modify_paged(). > I'm afraid that maybe it causes problem if I don't use RMW to clear > the PHYAD_EN bit. Because the following code in `rtl8211f_config_init` > also utilizes `phy_modify_paged_changed` to change ALDPS setting > without touching anything else. > > But if you insist, I can modify code to this if you like: > > > ret = phy_read_paged(phydev, 0xa43, RTL8211F_PHYCR1); > if (ret < 0) > return ret; > > dev_dbg(dev, "disabling MDIO address 0 for this phy"); > priv->phycr1 = ret & (u16)~RTL8211F_PHYAD0_EN; > ret = phy_write_paged(phydev, 0xa43, RTL8211F_PHYCR1, > priv->phycr1); > if (ret < 0) { > return dev_err_probe(dev, ret, > "disabling MDIO address 0 failed\n"); > } > /* Don't allow using broadcast address as PHY address */ > if (phydev->mdio.addr == 0) > return -ENODEV; > > priv->phycr1 &= (RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF); > ... > > > >>> + if (ret < 0) { >>> + dev_err(dev, "disabling MDIO address 0 failed: %pe\n", >>> + ERR_PTR(ret)); >> >> You may want to use dev_err_probe() here. And is it by intent that >> the error is ignored and you go on? >> > > I'm sorry that I made a mistake. > >> >> And one more formal issue: >> You annotated the patch as 1/2, but submit it as single patch. >> > I have another patch to enable support optical/copper combo support > of RTL8211FS PHY in this mail thread, but since Andrew said that patch > (migrated from Rockchip SDK) is low quality and I'm too busy with my > job, don't have much time to read and improve it, so I decided to > suspend that patch's submission and I'll resume to submit that patch > when I'm free. Could you please give me some advice or recommends on it? > The patch here seems to be independent and should be properly submitted as single patch. Regarding the other patch I have nothing to add to what Andrew stated already. > Sincerely, > > Zhiyuan Wan
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c index f65d7f1f3..8a38b02ad 100644 --- a/drivers/net/phy/realtek.c +++ b/drivers/net/phy/realtek.c @@ -31,6 +31,7 @@ #define RTL8211F_PHYCR1 0x18 #define RTL8211F_PHYCR2 0x19 #define RTL8211F_INSR 0x1d +#define RTL8211F_PHYAD0_EN BIT(13) #define RTL8211F_LEDCR 0x10 #define RTL8211F_LEDCR_MODE BIT(15) @@ -139,6 +140,17 @@ static int rtl821x_probe(struct phy_device *phydev) return dev_err_probe(dev, PTR_ERR(priv->clk), "failed to get phy clock\n"); + dev_dbg(dev, "disabling MDIO address 0 for this phy"); + ret = phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, + RTL8211F_PHYAD0_EN, 0); + if (ret < 0) { + dev_err(dev, "disabling MDIO address 0 failed: %pe\n", + ERR_PTR(ret)); + } + /* Don't allow using broadcast address as PHY address */ + if (phydev->mdio.addr == 0) + return -ENODEV; + ret = phy_read_paged(phydev, 0xa43, RTL8211F_PHYCR1); if (ret < 0) return ret;