On 30.03.2020 20:22, Florian Fainelli wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On 3/30/2020 9:30 AM, Andrew Lunn wrote:

quoted

On Mon, Mar 30, 2020 at 07:01:36PM +0300, Codrin Ciubotariu wrote:

quoted

Some ethernet controllers, such as cadence's macb, have an embedded MDIO.
For this reason, the ethernet PHY nodes are not under an MDIO bus, but
directly under the ethernet node. Since these drivers might use
of_mdiobus_child_is_phy(), we should fix this function by returning false
if a fixed-link is found.

So i assume the problem occurs here:

static int macb_mdiobus_register(struct macb *bp)
{
         struct device_node *child, *np = bp->pdev->dev.of_node;

         /* Only create the PHY from the device tree if at least one PHY is
          * described. Otherwise scan the entire MDIO bus. We do this to support
          * old device tree that did not follow the best practices and did not
          * describe their network PHYs.
          */
         for_each_available_child_of_node(np, child)
                 if (of_mdiobus_child_is_phy(child)) {
                         /* The loop increments the child refcount,
                          * decrement it before returning.
                          */
                         of_node_put(child);

                         return of_mdiobus_register(bp->mii_bus, np);
                 }

         return mdiobus_register(bp->mii_bus);
}

I think a better solution is

         for_each_available_child_of_node(np, child)
+             if (of_phy_is_fixed_link(child)
+                continue;
                 if (of_mdiobus_child_is_phy(child)) {
                         /* The loop increments the child refcount,
                          * decrement it before returning.
                          */
                         of_node_put(child);

                         return of_mdiobus_register(bp->mii_bus, np);
                 }

         return mdiobus_register(bp->mii_bus);
}

This problem is only an issue for macb, so keep the fix local to macb.

Agree, there is no reason for of_mdiobus_child_is_phy() to be checking
for a fixed-link. If you submit this formally:

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks guys. I thought there might be other controllers that have the 
PHY nodes inside the ethernet node. If not, I guess that 
of_mdiobus_child_is_phy() can be restricted to be called only if the 
passed device_node points to an MDIO node.
Moving the fix to macb, I think that of_phy_is_fixed_link() needs a 
device_node to the ethernet node, since it also has to deal with the 
legacy case in which fixed-link is a property, so it would look like 
something like this:
         struct device_node *child, *np = bp->pdev->dev.of_node;

+       if (of_phy_is_fixed_link(np))
+               return mdiobus_register(bp->mii_bus);
+
         /* Only create the PHY from the device tree if at least one PHY is

I will send another patch shortly.

Thanks and best regards,
Codrin