[PATCH 1/2] net: phy: realtek: add combo mode support for RTL8211FS

[PATCH 1/2] net: phy: realtek: add combo mode support for RTL8211FS

[Zhiyuan Wan]

The RTL8211FS chip is an ethernet transceiver with both copper MDIX and
optical (SGMII) port, and it has ability to switch between copper and
optical mode (combo mode).

On Linux kernel v6.12.1, the driver doesn't support negotiation port mode,
which causes optical mode unusable, and copper mode works fine.

This patch solved the issue above by add negotiation phase for this
transceiver chip, allows this transceiver works in combo mode.

Signed-off-by: Yuki Lee <febrieac@outlook.com>
Signed-off-by: Zhiyuan Wan <kmlinuxm@gmail.com>
---
 drivers/net/phy/realtek.c | 67 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

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,
-- 
2.30.2

[PATCH 2/2] net: phy: realtek: add dt property to disable broadcast PHY address

[Zhiyuan Wan]

This patch add support to disable 'broadcast PHY address' feature of
RTL8211F.

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 not every transceiver supports this feature, when RTL8211
shares one MDIO bus with other transceivers which doesn't support
this feature, like mt7530 switch chip (integrated in mt7621 SoC),
it usually causes address conflict, leads to the
port of RTL8211FS stops working.

This patch adds dt property `realtek,phyad0-disable` to disable
broadcast PHY address feature of this transceiver.

This patch did not change the default behavior of this driver.

Signed-off-by: Yuki Lee <febrieac@outlook.com>
Signed-off-by: Zhiyuan Wan <kmlinuxm@gmail.com>
---
 drivers/net/phy/realtek.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 10a87d58c..014dd2da1 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 RTL8211FS_FIBER_ESR			0x0F
 #define RTL8211FS_MODE_MASK			0xC000
@@ -421,12 +422,18 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	struct device *dev = &phydev->mdio.dev;
 	u16 val_txdly, val_rxdly;
 	int ret;
+	u16 phyad0_disable = 0;
 
+	if (of_property_read_bool(dev->of_node, "realtek,phyad0-disable")) {
+		phyad0_disable = RTL8211F_PHYAD0_EN;
+		dev_dbg(dev, "disabling MDIO address 0 for this phy");
+	}
 	ret = phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1,
-				       RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF,
+				       RTL8211F_ALDPS_PLL_OFF  | RTL8211F_ALDPS_ENABLE |
+				       RTL8211F_ALDPS_XTAL_OFF | phyad0_disable,
 				       priv->phycr1);
 	if (ret < 0) {
-		dev_err(dev, "aldps mode  configuration failed: %pe\n",
+		dev_err(dev, "mode configuration failed: %pe\n",
 			ERR_PTR(ret));
 		return ret;
 	}
-- 
2.30.2

Re: [PATCH 2/2] net: phy: realtek: add dt property to disable broadcast PHY address

[Andrew Lunn]

On Tue, Dec 03, 2024 at 03:50:29AM +0800, Zhiyuan Wan wrote:
> This patch add support to disable 'broadcast PHY address' feature of
> RTL8211F.
> 
> 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 not every transceiver supports this feature, when RTL8211
> shares one MDIO bus with other transceivers which doesn't support
> this feature, like mt7530 switch chip (integrated in mt7621 SoC),
> it usually causes address conflict, leads to the
> port of RTL8211FS stops working.

I think you can do this without needing a new property. The DT binding
has:

            reg = <4>;

This is the address the PHY should respond on. If reg is not 0, then
broadcast is not wanted.

If reg is 0, it means one of two things:

The DT author did not know about this broadcast feature, the PHY
appeared at address 0, so they wrote that. It might actually be
strapped to another address, but it does not matter.

The DT author wants it to use the broadcast address, it might even be
strapped to address 0.

Am i missing anything?

	Andrew

[PATCH v2 1/2] net: phy: realtek: disable broadcast address feature of rtl8211f

[Zhiyuan Wan]

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 when
phy_addr is not 0. Solved address conflict with other devices
on MDIO bus.

Reviewed-by: Yuki Lee <febrieac@outlook.com>
Signed-off-by: Zhiyuan Wan <kmlinuxm@gmail.com>
---
 drivers/net/phy/realtek.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index f65d7f1f3..9824718af 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)
@@ -377,12 +378,18 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	struct device *dev = &phydev->mdio.dev;
 	u16 val_txdly, val_rxdly;
 	int ret;
+	u16 phyad0_disable = 0;
 
+	if (phydev->mdio.addr != 0) {
+		phyad0_disable = RTL8211F_PHYAD0_EN;
+		dev_dbg(dev, "disabling MDIO address 0 for this phy");
+	}
 	ret = phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1,
-				       RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF,
+				       RTL8211F_ALDPS_PLL_OFF  | RTL8211F_ALDPS_ENABLE |
+				       RTL8211F_ALDPS_XTAL_OFF | phyad0_disable,
 				       priv->phycr1);
 	if (ret < 0) {
-		dev_err(dev, "aldps mode  configuration failed: %pe\n",
+		dev_err(dev, "mode configuration failed: %pe\n",
 			ERR_PTR(ret));
 		return ret;
 	}
-- 
2.30.2

Re: [PATCH v2 1/2] net: phy: realtek: disable broadcast address feature of rtl8211f

[Andrew Lunn]

On Tue, Dec 03, 2024 at 11:46:35AM +0800, 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 when
> phy_addr is not 0. Solved address conflict with other devices
> on MDIO bus.
> 
> Reviewed-by: Yuki Lee <febrieac@outlook.com>
> Signed-off-by: Zhiyuan Wan <kmlinuxm@gmail.com>
> ---
>  drivers/net/phy/realtek.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index f65d7f1f3..9824718af 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)
> @@ -377,12 +378,18 @@ static int rtl8211f_config_init(struct phy_device *phydev)

config_init is too late. You should be doing it in probe. It could be
the scan of the bus has found the PHY on the broadcast address, as
well as its proper address. When probe is called on the broadcast
address you want to return -ENODEV and disable broadcast. It will then
get probed again on its proper address.

	Andrew

[PATCH v3 1/2] net: phy: realtek: disable broadcast address feature of rtl8211f

[Zhiyuan Wan]

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>
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);
+	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;
-- 
2.30.2

Re: [PATCH v3 1/2] net: phy: realtek: disable broadcast address feature of rtl8211f

[Heiner Kallweit]

On 03.12.2024 05:26, 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>
> 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);

Why do you use the _changed version if you don't use the related feature?

And formal aspects:
- patch should be annotated net-next
- you missed to address all maintainers, use the get_maintainers.pl
  script

> +	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;

[PATCH net-next 1/2] net: phy: realtek: disable broadcast address feature of rtl8211f

[Zhiyuan Wan]

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>
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);
+	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;
-- 
2.30.2

Re: [PATCH net-next 1/2] net: phy: realtek: disable broadcast address feature of rtl8211f

[Heiner Kallweit]

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.

Re: [PATCH net-next 1/2] net: phy: realtek: disable broadcast address feature of rtl8211f

[Zhiyuan Wan]

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

Re: [PATCH net-next 1/2] net: phy: realtek: disable broadcast address feature of rtl8211f

[Heiner Kallweit]

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

Re: [PATCH 1/2] net: phy: realtek: add combo mode support for RTL8211FS

[Andrew Lunn]

> +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

Re: [PATCH 1/2] net: phy: realtek: add combo mode support for RTL8211FS

[万致远]

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

Re: [PATCH 1/2] net: phy: realtek: add combo mode support for RTL8211FS

[Andrew Lunn]

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

Re: [PATCH 1/2] net: phy: realtek: add combo mode support for RTL8211FS

[万致远]

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

Re: [PATCH 1/2] net: phy: realtek: add combo mode support for RTL8211FS

[万致远]

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

Re: [PATCH 2/2] net: phy: realtek: add dt property to disable broadcast PHY address

[万致远]

On 2024/12/3 8:04, Andrew Lunn wrote:
> On Tue, Dec 03, 2024 at 03:50:29AM +0800, Zhiyuan Wan wrote:
>> This patch add support to disable 'broadcast PHY address' feature of
>> RTL8211F.
>>
>> 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 not every transceiver supports this feature, when RTL8211
>> shares one MDIO bus with other transceivers which doesn't support
>> this feature, like mt7530 switch chip (integrated in mt7621 SoC),
>> it usually causes address conflict, leads to the
>> port of RTL8211FS stops working.
>
> I think you can do this without needing a new property. The DT binding
> has:
>
>             reg = <4>;
>
> This is the address the PHY should respond on. If reg is not 0, then
> broadcast is not wanted.
>
First, broadcast has no relationship with PHY address, it allows MAC
broadcast command to all supported PHY on the MDIO bus.

I can't assume that there's no user use this feature to configure multiple
PHY devices (e.g. there's like 3 or more PHYs on board, their address
represented as 1, 2, 3. When this feature is enabled (default behavior),
users can send commands to address 0 to configure common parameters shared
by these PHYs) at the same time. And they can configure each PHY by it's
own address without influencing other PHY too.

> If reg is 0, it means one of two things:
>
> The DT author did not know about this broadcast feature, the PHY
> appeared at address 0, so they wrote that. It might actually be
> strapped to another address, but it does not matter.
>
Well, that's possible. A misconfiguration on DT with only ONE PHY may
just works, if we disable this feature by default, may cause some device
stops working.

> The DT author wants it to use the broadcast address, it might even be
> strapped to address 0.
>
Again, the broadcast address is shared by all PHYs on MDIO which
support this feature, it's handy for MAC to change multiple PHYs
setting at the same time. But each PHY must have their own address,
and the address usually can't be broadcast address (0).

> Am i missing anything?
>
>       Andrew

I would recommend to add this feature, because it doesn't change the
behavior of this driver, and allows this PHY works together with
other PHY or switch chip which don't support this feature, like mt7530 or
Marvell ones.

Sincerely,

Zhiyuan Wan

Re: [PATCH 2/2] net: phy: realtek: add dt property to disable broadcast PHY address

[Andrew Lunn]

> > I think you can do this without needing a new property. The DT binding
> > has:
> >
> >             reg = <4>;
> >
> > This is the address the PHY should respond on. If reg is not 0, then
> > broadcast is not wanted.
> >
> First, broadcast has no relationship with PHY address, it allows MAC
> broadcast command to all supported PHY on the MDIO bus.
> 
> I can't assume that there's no user use this feature to configure multiple
> PHY devices (e.g. there's like 3 or more PHYs on board, their address
> represented as 1, 2, 3. When this feature is enabled (default behavior),
> users can send commands to address 0 to configure common parameters shared
> by these PHYs) at the same time.

phylib does not do that. Each PHY is considered a single entity. User
space could in theory do it via phy_do_ioctl(), but that is a very
risky thing to do, there is no locking, and you are likely to confuse
phylib and/or the PHY driver.

So we don't actually need the broadcast feature.

> Again, the broadcast address is shared by all PHYs on MDIO which
> support this feature, it's handy for MAC to change multiple PHYs
> setting at the same time.

Please point me at a MAC driver doing this.

> I would recommend to add this feature, because it doesn't change the
> behavior of this driver, and allows this PHY works together with
> other PHY or switch chip which don't support this feature, like mt7530 or
> Marvell ones.

I agree we should be disabling this when it is safe to disable, but i
don't agree we need a new property, reg is sufficient.

	Andrew

Re: [PATCH 2/2] net: phy: realtek: add dt property to disable broadcast PHY address

[万致远]

At 2024/12/3 10:54, Andrew Lunn wrote:
>>> I think you can do this without needing a new property. The DT binding
>>> has:
>>>
>>>             reg = <4>;
>>>
>>> This is the address the PHY should respond on. If reg is not 0, then
>>> broadcast is not wanted.
>>>
>> First, broadcast has no relationship with PHY address, it allows MAC
>> broadcast command to all supported PHY on the MDIO bus.
>>
>> I can't assume that there's no user use this feature to configure multiple
>> PHY devices (e.g. there's like 3 or more PHYs on board, their address
>> represented as 1, 2, 3. When this feature is enabled (default behavior),
>> users can send commands to address 0 to configure common parameters shared
>> by these PHYs) at the same time.
>
> phylib does not do that. Each PHY is considered a single entity. User
> space could in theory do it via phy_do_ioctl(), but that is a very
> risky thing to do, there is no locking, and you are likely to confuse
> phylib and/or the PHY driver.
>
> So we don't actually need the broadcast feature.
>
>> Again, the broadcast address is shared by all PHYs on MDIO which
>> support this feature, it's handy for MAC to change multiple PHYs
>> setting at the same time.
>
> Please point me at a MAC driver doing this.
>

I know some cursed user-mode program do that but seems no kernel driver
doing this.

>> I would recommend to add this feature, because it doesn't change the
>> behavior of this driver, and allows this PHY works together with
>> other PHY or switch chip which don't support this feature, like mt7530 or
>> Marvell ones.
>
> I agree we should be disabling this when it is safe to disable, but i
> don't agree we need a new property, reg is sufficient.
>
Okay, so I will left PHYAD == 0 stay untouched, and PHYAD != 0 disables
broadcast feature.
>       Andrew
Sincerely,

Zhiyuan Wan