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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 03.12.2024 13:54, Zhiyuan Wan wrote:
> This feature is automatically enabled after a reset of this
> transceiver. When this feature is enabled, the phy not only
> responds to the configured 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 rtl8211f stops working.
> 
> This patch disables broadcast address feature of rtl8211f, and
> returns -ENODEV if using broadcast address (0) as phy address.
> 
> Hardware design hint:
> This PHY only support address 1-7, and DO NOT tie all PHYAD pins
> ground when you connect more than one PHY on a MDIO bus.
> If you do that, this PHY will automatically take the first address
> appeared on the MDIO bus as it's address, causing address conflict.
> 
> 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..0ef636d7b 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(phydev, 0xa43, RTL8211F_PHYCR1,
> +				       RTL8211F_PHYAD0_EN, 0);
> +	if (ret < 0) {
> +		return dev_err_probe(dev, PTR_ERR(ret),

Why PTR_ERR()? Did you even compile-test?

> +				     "disabling MDIO address 0 failed\n");
> +	}
> +	/* Deny 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 more formal hints:
- If you send a new version of a patch, annotate it accordingly.
- Allow 24h before sending a new version
- Include a change log

https://docs.kernel.org/process/submitting-patches.html
https://www.kernel.org/doc/html/v6.1/process/maintainer-netdev.html