[PATCH] net: mdio: of: Do not treat fixed-link as PHY

[PATCH] net: mdio: of: Do not treat fixed-link as PHY

[Codrin Ciubotariu]

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.

Fixes: 801a8ef54e8b ("of: phy: Only register a phy device for phys")
Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
---
 drivers/of/of_mdio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 9f982c0627a0..7cf8aad645a4 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -195,12 +195,17 @@ static const struct of_device_id whitelist_phys[] = {
  * o Compatible string of "ethernet-phy-ieee802.3-c22"
  * o In the white list above (and issue a warning)
  * o No compatibility string
+ * o Not a fixed-link
  *
  * A device which is not a phy is expected to have a compatible string
  * indicating what sort of device it is.
  */
 bool of_mdiobus_child_is_phy(struct device_node *child)
 {
+	const struct of_device_id fixed_link[] = {
+		{ .name = "fixed-link" },
+		{}
+	};
 	u32 phy_id;
 
 	if (of_get_phy_id(child, &phy_id) != -EINVAL)
@@ -219,7 +224,8 @@ bool of_mdiobus_child_is_phy(struct device_node *child)
 		return true;
 	}
 
-	if (!of_find_property(child, "compatible", NULL))
+	if (!of_find_property(child, "compatible", NULL) &&
+	    !of_match_node(fixed_link, child))
 		return true;
 
 	return false;
-- 
2.20.1

Re: [PATCH] net: mdio: of: Do not treat fixed-link as PHY

[Andrew Lunn]

On Mon, Mar 30, 2020 at 07:01:36PM +0300, Codrin Ciubotariu wrote:
> 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.

Hi Codrin

That is deprecated. It causes all sorts of problems putting PHY nodes
in the MAC without a container.

Please fix macb to look for an mdio node, and place your fixed link
inside it.

       Andrew

Re: [PATCH] net: mdio: of: Do not treat fixed-link as PHY

[Russell King - ARM Linux admin]

On Mon, Mar 30, 2020 at 06:17:40PM +0200, Andrew Lunn wrote:
> On Mon, Mar 30, 2020 at 07:01:36PM +0300, Codrin Ciubotariu wrote:
> > 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.
> 
> Hi Codrin
> 
> That is deprecated. It causes all sorts of problems putting PHY nodes
> in the MAC without a container.
> 
> Please fix macb to look for an mdio node, and place your fixed link
> inside it.

Seems wrong.

fixed links have never needed to be under a mdio node - see
Documentation/devicetree/bindings/net/ethernet-controller.yaml

fixed-link is a child of the MAC controller, not of a mdio node.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH] net: mdio: of: Do not treat fixed-link as PHY

[Andrew Lunn]

On Mon, Mar 30, 2020 at 05:21:30PM +0100, Russell King - ARM Linux admin wrote:
> On Mon, Mar 30, 2020 at 06:17:40PM +0200, Andrew Lunn wrote:
> > On Mon, Mar 30, 2020 at 07:01:36PM +0300, Codrin Ciubotariu wrote:
> > > 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.
> > 
> > Hi Codrin
> > 
> > That is deprecated. It causes all sorts of problems putting PHY nodes
> > in the MAC without a container.
> > 
> > Please fix macb to look for an mdio node, and place your fixed link
> > inside it.
> 
> Seems wrong.

Hi Russell

Gerr. You are right.

> fixed links have never needed to be under a mdio node - see
> Documentation/devicetree/bindings/net/ethernet-controller.yaml

macb does crazy stuff. I will take another look.

     Andrew

Re: [PATCH] net: mdio: of: Do not treat fixed-link as PHY

[Andrew Lunn]

On Mon, Mar 30, 2020 at 07:01:36PM +0300, Codrin Ciubotariu wrote:
> 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.

	Andrew

Re: [PATCH] net: mdio: of: Do not treat fixed-link as PHY

[Florian Fainelli]

On 3/30/2020 9:30 AM, Andrew Lunn wrote:
> On Mon, Mar 30, 2020 at 07:01:36PM +0300, Codrin Ciubotariu wrote:
>> 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>
-- 
Florian

Re: [PATCH] net: mdio: of: Do not treat fixed-link as PHY

[Codrin.Ciubotariu]

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:
>> On Mon, Mar 30, 2020 at 07:01:36PM +0300, Codrin Ciubotariu wrote:
>>> 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

Re: [PATCH] net: mdio: of: Do not treat fixed-link as PHY

[Andrew Lunn]

> Thanks guys. I thought there might be other controllers that have the 
> PHY nodes inside the ethernet node.

Hi Codrin

There are some still using this deprecated feature. But macb is the
only one doing this odd looping over child nodes. It is this looping
which is breaking things, not the use of the deprecated feature
itself.

	Andrew

Re: [PATCH] net: mdio: of: Do not treat fixed-link as PHY

[Codrin.Ciubotariu]

On 31.03.2020 15:59, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> Thanks guys. I thought there might be other controllers that have the
>> PHY nodes inside the ethernet node.
> 
> Hi Codrin

Hi Andrew

> 
> There are some still using this deprecated feature. But macb is the
> only one doing this odd looping over child nodes. It is this looping
> which is breaking things, not the use of the deprecated feature
> itself.

Yes, its due to the fact that the MDIO node is missing. Should we have 
in mind to add an MDIO node under the macb node, where we could add the 
PHY nodes? The macb bindings don't seem to require the PHY nodes 
directly under the macb node, but the compatibility with the deprecated 
feature needs to be maintained.

Best regards,
Codrin

Re: [PATCH] net: mdio: of: Do not treat fixed-link as PHY

[Andrew Lunn]

> Hi Andrew
> 
> > 
> > There are some still using this deprecated feature. But macb is the
> > only one doing this odd looping over child nodes. It is this looping
> > which is breaking things, not the use of the deprecated feature
> > itself.
> 
> Yes, its due to the fact that the MDIO node is missing. Should we have 
> in mind to add an MDIO node under the macb node, where we could add the 
> PHY nodes?

Yes, you can make the driver complient any time you want. But as you
said, you need to keep with backwards compatibility. But net-next is
closed now, for the merge window. So you probably want to wait two
weeks before posting code.

      Andrew