[PATCH 0/2] PHY of_init breakage with non-OF

[PATCH 0/2] PHY of_init breakage with non-OF

[Vladimir Oltean]

Investigating an issue with the NXP LS1012A-FRWY board and the AR8031
PHY not negotiating, I found that the introduction of the

 	node = phy_get_ofnode(phydev);
 	if (!ofnode_valid(node))
 		return -EINVAL;

pattern in PHY drivers causes silent breakage for users of phy_connect().

I've fixed this in the Atheros driver, and also saw the same issue in
the TI PHY driver, so I fixed it there as well.

Vladimir Oltean (2):
  net: phy: dp83867: avoid error in dp83867_of_init() when PHY has no OF
    node
  net: phy: atheros: avoid error in ar803x_of_init() when PHY has no OF
    node

 drivers/net/phy/atheros.c | 2 +-
 drivers/net/phy/dp83867.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH 1/2] net: phy: dp83867: avoid error in dp83867_of_init() when PHY has no OF node

[Vladimir Oltean]

A DM_ETH driver may use phy_connect() towards a PHY address on an MDIO
bus which is not specified in the device tree, as evidenced by:

pfe_eth_probe
-> pfe_phy_configure
   -> phy_connect

When this happens, the PHY will have an invalid OF node.

The dp83867_config() method has extra initialization steps which are
bypassed when the PHY lacks an OF node, which is undesirable because it
will lead to broken networking. Allow the rest of the code to run.

Fixes: 085445ca4104 ("net: phy: ti: Allow the driver to be more configurable")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/dp83867.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index eada4541c9c3..49978d0f25f3 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -158,7 +158,7 @@ static int dp83867_of_init(struct phy_device *phydev)
 
 	node = phy_get_ofnode(phydev);
 	if (!ofnode_valid(node))
-		return -EINVAL;
+		return 0;
 
 	/* Optional configuration */
 	ret = ofnode_read_u32(node, "ti,clk-output-sel",
-- 
2.25.1

[PATCH 2/2] net: phy: atheros: avoid error in ar803x_of_init() when PHY has no OF node

[Vladimir Oltean]

A DM_ETH driver may use phy_connect() towards a PHY address on an MDIO
bus which is not specified in the device tree, as evidenced by:

pfe_eth_probe
-> pfe_phy_configure
   -> phy_connect

When this happens, the PHY will have an invalid OF node.

When ar803x_config() runs, it silently fails at ar803x_of_init(), and
therefore, fails to run the rest of the initialization.

This makes MII_BMCR contain what it had after BMCR_RESET (0x8000) has
been written into it by phy_reset(). Since BMCR_RESET is volatile and
self-clearing, the MII_BMCR ends up having a value of 0x0. The further
configuration of this register, which is supposed to be handled by
genphy_config_aneg() lower in ar803x_config(), never gets a chance to
run due to this early error from ar803x_of_init().

As a result of having MII_BMCR as 0, the following symptom appears:

=> setenv ethact pfe_eth0
=> setenv ipaddr 10.0.0.1
=> ping 10.0.0.2
pfe_eth0 Waiting for PHY auto negotiation to complete......... TIMEOUT !
Could not initialize PHY pfe_eth0

Manually writing 0x1140 into register 0 of the PHY makes the connection
work, but it is rather desirable that the port works without any manual
intervention.

Fixes: fe6293a80959 ("phy: atheros: add device tree bindings and config")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/atheros.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/atheros.c b/drivers/net/phy/atheros.c
index f922fecd6b5d..fa1fe08518f4 100644
--- a/drivers/net/phy/atheros.c
+++ b/drivers/net/phy/atheros.c
@@ -199,7 +199,7 @@ static int ar803x_of_init(struct phy_device *phydev)
 
 	node = phy_get_ofnode(phydev);
 	if (!ofnode_valid(node))
-		return -EINVAL;
+		return 0;
 
 	priv = malloc(sizeof(*priv));
 	if (!priv)
-- 
2.25.1

Re: [PATCH 1/2] net: phy: dp83867: avoid error in dp83867_of_init() when PHY has no OF node

[Ramon Fried]

On Wed, Feb 23, 2022 at 3:21 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> A DM_ETH driver may use phy_connect() towards a PHY address on an MDIO
> bus which is not specified in the device tree, as evidenced by:
>
> pfe_eth_probe
> -> pfe_phy_configure
>    -> phy_connect
>
> When this happens, the PHY will have an invalid OF node.
>
> The dp83867_config() method has extra initialization steps which are
> bypassed when the PHY lacks an OF node, which is undesirable because it
> will lead to broken networking. Allow the rest of the code to run.
>
> Fixes: 085445ca4104 ("net: phy: ti: Allow the driver to be more configurable")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/phy/dp83867.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index eada4541c9c3..49978d0f25f3 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -158,7 +158,7 @@ static int dp83867_of_init(struct phy_device *phydev)
>
>         node = phy_get_ofnode(phydev);
>         if (!ofnode_valid(node))
> -               return -EINVAL;
> +               return 0;
>
>         /* Optional configuration */
>         ret = ofnode_read_u32(node, "ti,clk-output-sel",
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 2/2] net: phy: atheros: avoid error in ar803x_of_init() when PHY has no OF node

[Ramon Fried]

On Wed, Feb 23, 2022 at 3:21 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> A DM_ETH driver may use phy_connect() towards a PHY address on an MDIO
> bus which is not specified in the device tree, as evidenced by:
>
> pfe_eth_probe
> -> pfe_phy_configure
>    -> phy_connect
>
> When this happens, the PHY will have an invalid OF node.
>
> When ar803x_config() runs, it silently fails at ar803x_of_init(), and
> therefore, fails to run the rest of the initialization.
>
> This makes MII_BMCR contain what it had after BMCR_RESET (0x8000) has
> been written into it by phy_reset(). Since BMCR_RESET is volatile and
> self-clearing, the MII_BMCR ends up having a value of 0x0. The further
> configuration of this register, which is supposed to be handled by
> genphy_config_aneg() lower in ar803x_config(), never gets a chance to
> run due to this early error from ar803x_of_init().
>
> As a result of having MII_BMCR as 0, the following symptom appears:
>
> => setenv ethact pfe_eth0
> => setenv ipaddr 10.0.0.1
> => ping 10.0.0.2
> pfe_eth0 Waiting for PHY auto negotiation to complete......... TIMEOUT !
> Could not initialize PHY pfe_eth0
>
> Manually writing 0x1140 into register 0 of the PHY makes the connection
> work, but it is rather desirable that the port works without any manual
> intervention.
>
> Fixes: fe6293a80959 ("phy: atheros: add device tree bindings and config")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/phy/atheros.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/atheros.c b/drivers/net/phy/atheros.c
> index f922fecd6b5d..fa1fe08518f4 100644
> --- a/drivers/net/phy/atheros.c
> +++ b/drivers/net/phy/atheros.c
> @@ -199,7 +199,7 @@ static int ar803x_of_init(struct phy_device *phydev)
>
>         node = phy_get_ofnode(phydev);
>         if (!ofnode_valid(node))
> -               return -EINVAL;
> +               return 0;
>
>         priv = malloc(sizeof(*priv));
>         if (!priv)
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 2/2] net: phy: atheros: avoid error in ar803x_of_init() when PHY has no OF node

[Ramon Fried]

On Thu, Mar 3, 2022 at 9:53 AM Ramon Fried <rfried.dev@gmail.com> wrote:
>
> On Wed, Feb 23, 2022 at 3:21 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> >
> > A DM_ETH driver may use phy_connect() towards a PHY address on an MDIO
> > bus which is not specified in the device tree, as evidenced by:
> >
> > pfe_eth_probe
> > -> pfe_phy_configure
> >    -> phy_connect
> >
> > When this happens, the PHY will have an invalid OF node.
> >
> > When ar803x_config() runs, it silently fails at ar803x_of_init(), and
> > therefore, fails to run the rest of the initialization.
> >
> > This makes MII_BMCR contain what it had after BMCR_RESET (0x8000) has
> > been written into it by phy_reset(). Since BMCR_RESET is volatile and
> > self-clearing, the MII_BMCR ends up having a value of 0x0. The further
> > configuration of this register, which is supposed to be handled by
> > genphy_config_aneg() lower in ar803x_config(), never gets a chance to
> > run due to this early error from ar803x_of_init().
> >
> > As a result of having MII_BMCR as 0, the following symptom appears:
> >
> > => setenv ethact pfe_eth0
> > => setenv ipaddr 10.0.0.1
> > => ping 10.0.0.2
> > pfe_eth0 Waiting for PHY auto negotiation to complete......... TIMEOUT !
> > Could not initialize PHY pfe_eth0
> >
> > Manually writing 0x1140 into register 0 of the PHY makes the connection
> > work, but it is rather desirable that the port works without any manual
> > intervention.
> >
> > Fixes: fe6293a80959 ("phy: atheros: add device tree bindings and config")
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  drivers/net/phy/atheros.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/atheros.c b/drivers/net/phy/atheros.c
> > index f922fecd6b5d..fa1fe08518f4 100644
> > --- a/drivers/net/phy/atheros.c
> > +++ b/drivers/net/phy/atheros.c
> > @@ -199,7 +199,7 @@ static int ar803x_of_init(struct phy_device *phydev)
> >
> >         node = phy_get_ofnode(phydev);
> >         if (!ofnode_valid(node))
> > -               return -EINVAL;
> > +               return 0;
> >
> >         priv = malloc(sizeof(*priv));
> >         if (!priv)
> > --
> > 2.25.1
> >
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
Applied to u-boot-net/next
Thanks,
Ramon

Re: [PATCH 1/2] net: phy: dp83867: avoid error in dp83867_of_init() when PHY has no OF node

[Ramon Fried]

On Thu, Mar 3, 2022 at 9:52 AM Ramon Fried <rfried.dev@gmail.com> wrote:
>
> On Wed, Feb 23, 2022 at 3:21 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> >
> > A DM_ETH driver may use phy_connect() towards a PHY address on an MDIO
> > bus which is not specified in the device tree, as evidenced by:
> >
> > pfe_eth_probe
> > -> pfe_phy_configure
> >    -> phy_connect
> >
> > When this happens, the PHY will have an invalid OF node.
> >
> > The dp83867_config() method has extra initialization steps which are
> > bypassed when the PHY lacks an OF node, which is undesirable because it
> > will lead to broken networking. Allow the rest of the code to run.
> >
> > Fixes: 085445ca4104 ("net: phy: ti: Allow the driver to be more configurable")
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  drivers/net/phy/dp83867.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> > index eada4541c9c3..49978d0f25f3 100644
> > --- a/drivers/net/phy/dp83867.c
> > +++ b/drivers/net/phy/dp83867.c
> > @@ -158,7 +158,7 @@ static int dp83867_of_init(struct phy_device *phydev)
> >
> >         node = phy_get_ofnode(phydev);
> >         if (!ofnode_valid(node))
> > -               return -EINVAL;
> > +               return 0;
> >
> >         /* Optional configuration */
> >         ret = ofnode_read_u32(node, "ti,clk-output-sel",
> > --
> > 2.25.1
> >
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
Applied to u-boot-net/next
Thanks,
Ramon

Re: [PATCH 2/2] net: phy: atheros: avoid error in ar803x_of_init() when PHY has no OF node

[Vladimir Oltean]

Hi Ramon,

On Fri, Apr 01, 2022 at 10:11:13PM +0300, Ramon Fried wrote:
> On Thu, Mar 3, 2022 at 9:53 AM Ramon Fried <rfried.dev@gmail.com> wrote:
> > Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
> Applied to u-boot-net/next
> Thanks,
> Ramon

Did you push this commit anywhere? I don't see it here
https://source.denx.de/u-boot/custodians/u-boot-net/-/commits/next/