While the MDIO address of the internal PHY on Allwinner sun8i chips is
generally 1, of_mdio_parse_addr is used to cleanly parse the address
from the device-tree instead of hardcoding it.

A commit reworking the code ditched the parsed value and hardcoded the
value 1 instead, which didn't really break anything but is more fragile
and not future-proof.

Restore the initial behavior using the parsed address returned from the
helper.

Fixes: 634db83b8265 ("net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs")
Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 85723a78793a..6c7e8655a7eb 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -964,7 +964,7 @@ static int sun8i_dwmac_set_syscon(struct device *dev,
 		/* of_mdio_parse_addr returns a valid (0 ~ 31) PHY
 		 * address. No need to mask it again.
 		 */
-		reg |= 1 << H3_EPHY_ADDR_SHIFT;
+		reg |= ret << H3_EPHY_ADDR_SHIFT;
 	} else {
 		/* For SoCs without internal PHY the PHY selection bit should be
 		 * set to 0 (external PHY).
-- 
2.49.0

Le Mon, May 19, 2025 at 06:49:36PM +0200, Paul Kocialkowski a écrit :
> While the MDIO address of the internal PHY on Allwinner sun8i chips is
> generally 1, of_mdio_parse_addr is used to cleanly parse the address
> from the device-tree instead of hardcoding it.
> 
> A commit reworking the code ditched the parsed value and hardcoded the
> value 1 instead, which didn't really break anything but is more fragile
> and not future-proof.
> 
> Restore the initial behavior using the parsed address returned from the
> helper.
> 
> Fixes: 634db83b8265 ("net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs")
> Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> index 85723a78793a..6c7e8655a7eb 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> @@ -964,7 +964,7 @@ static int sun8i_dwmac_set_syscon(struct device *dev,
>  		/* of_mdio_parse_addr returns a valid (0 ~ 31) PHY
>  		 * address. No need to mask it again.
>  		 */
> -		reg |= 1 << H3_EPHY_ADDR_SHIFT;
> +		reg |= ret << H3_EPHY_ADDR_SHIFT;
>  	} else {
>  		/* For SoCs without internal PHY the PHY selection bit should be
>  		 * set to 0 (external PHY).
> -- 
> 2.49.0
> 

Acked-by: Corentin LABBE <clabbe.montjoie@gmail.com>
Tested-by: Corentin LABBE <clabbe.montjoie@gmail.com>
Tested-on: sun50i-h6-orangepi-one-plus
Tested-on: sun8i-h3-orangepi-pc

Thanks
Regards

On Mon, May 19, 2025 at 06:49:36PM +0200, Paul Kocialkowski wrote:
> While the MDIO address of the internal PHY on Allwinner sun8i chips is
> generally 1, of_mdio_parse_addr is used to cleanly parse the address
> from the device-tree instead of hardcoding it.
> 
> A commit reworking the code ditched the parsed value and hardcoded the
> value 1 instead, which didn't really break anything but is more fragile
> and not future-proof.
> 
> Restore the initial behavior using the parsed address returned from the
> helper.
> 
> Fixes: 634db83b8265 ("net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs")
> Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>

Have you validated all .dts files using this binding? Any using
anything other than 1? The problem with silly bugs like this is that
there might be DT blobs with the value 42 which currently work, but
will break as soon as this patch lands.

Just stating in the commit message you have checked will help get the
patch merged.

      Andrew

Hi Andrew,

Thanks for your reply.

Le Mon 19 May 25, 21:25, Andrew Lunn a écrit :
> On Mon, May 19, 2025 at 06:49:36PM +0200, Paul Kocialkowski wrote:
> > While the MDIO address of the internal PHY on Allwinner sun8i chips is
> > generally 1, of_mdio_parse_addr is used to cleanly parse the address
> > from the device-tree instead of hardcoding it.
> > 
> > A commit reworking the code ditched the parsed value and hardcoded the
> > value 1 instead, which didn't really break anything but is more fragile
> > and not future-proof.
> > 
> > Restore the initial behavior using the parsed address returned from the
> > helper.
> > 
> > Fixes: 634db83b8265 ("net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs")
> > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> 
> Have you validated all .dts files using this binding? Any using
> anything other than 1? The problem with silly bugs like this is that
> there might be DT blobs with the value 42 which currently work, but
> will break as soon as this patch lands.

There's actually only two users of this (this code path for internal PHYs is
only used by two chips), which are platform dtsi files that have the address
already set to 1.

I have actually tested both with hardware and they work fine.

> Just stating in the commit message you have checked will help get the
> patch merged.

Do you think it's worth resending? I'd be fine with adding this when picking
up the patch:

The internal PHY is only found on the H3/H5 and V3s/V3/S3 chips which already
have the same address properly described in their platform device-tree and
were verified to work after this patch.

Cheers,

Paul

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

> There's actually only two users of this (this code path for internal PHYs is
> only used by two chips), which are platform dtsi files that have the address
> already set to 1.
> 
> I have actually tested both with hardware and they work fine.

O.K.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

This is something to think about for patches in general. What
questions are reviewers likely to ask, and can you answer them in the
commit message?

    Andrew