net: mdio: of_mdio: check for already registered phy before creating new instances

In of_mdiobus_register_phy(), check if the phy with the given address is
already registered within the mii bus before calling phy_device_create()
or get_phy_device().

This allows us to augment auto-probed phy devices with extra information
via DT. Without this patch, a second instance of the phydev is created
unnecessarily.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/of/of_mdio.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Hi Daniel,

2014-05-06 8:21 GMT-07:00 Daniel Mack <zonque@gmail.com>:
> In of_mdiobus_register_phy(), check if the phy with the given address is
> already registered within the mii bus before calling phy_device_create()
> or get_phy_device().

I am not exactly sure how you could be in that sort of situation.
of_mdiobus_register() handles two different cases at the moment:

1) PHY child nodes have a valid 'reg' property, and this property is
used to register the PHY at the given address
2) if a PHY child node does not have a valid 'reg' property, which
will trigger an auto-scan and then we iterate through all 32 addresses
of the bus, we skip over PHYs that are already registered

>
> This allows us to augment auto-probed phy devices with extra information
> via DT. Without this patch, a second instance of the phydev is created
> unnecessarily.

Which piece of code is doing the auto-probing in your case? One of the
very first things that of_mdiobus_register() does is set the PHY mask
to 0xffffffff to prevent the default PHY probing method to trigger,
since we are using information from the Device Tree right after that.

>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  drivers/of/of_mdio.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 9a95831..264ea3f 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -72,10 +72,15 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
>         is_c45 = of_device_is_compatible(child,
>                                          "ethernet-phy-ieee802.3-c45");
>
> -       if (!is_c45 && !of_get_phy_id(child, &phy_id))
> -               phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
> -       else
> -               phy = get_phy_device(mdio, addr, is_c45);
> +       /* Check if the phy we're looking for is already registered */
> +       phy = mdio->phy_map[addr];
> +       if (!phy) {
> +               if (!is_c45 && !of_get_phy_id(child, &phy_id))
> +                       phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
> +               else
> +                       phy = get_phy_device(mdio, addr, is_c45);
> +       }
> +
>         if (!phy || IS_ERR(phy))
>                 return 1;
>
> --
> 1.9.0
>

(+ Mugunthan V N)

Hi Florian,

On 05/07/2014 02:55 AM, Florian Fainelli wrote:
> 2014-05-06 8:21 GMT-07:00 Daniel Mack <zonque@gmail.com>:
>> In of_mdiobus_register_phy(), check if the phy with the given address is
>> already registered within the mii bus before calling phy_device_create()
>> or get_phy_device().
> 
> I am not exactly sure how you could be in that sort of situation.
> of_mdiobus_register() handles two different cases at the moment:
> 
> 1) PHY child nodes have a valid 'reg' property, and this property is
> used to register the PHY at the given address
> 2) if a PHY child node does not have a valid 'reg' property, which
> will trigger an auto-scan and then we iterate through all 32 addresses
> of the bus, we skip over PHYs that are already registered
> 
>>
>> This allows us to augment auto-probed phy devices with extra information
>> via DT. Without this patch, a second instance of the phydev is created
>> unnecessarily.
> 
> Which piece of code is doing the auto-probing in your case? One of the
> very first things that of_mdiobus_register() does is set the PHY mask
> to 0xffffffff to prevent the default PHY probing method to trigger,
> since we are using information from the Device Tree right after that.

Ah, ok. So what happens here is that of_mdiobus_register() sets the
phy_mask to ~0, but mdiobus_register() calls bus->reset(), which resets
the mask to ffffffef in my case. bus->reset is davinci_mdio_reset() in
my case.

Is the davinci mdio driver doing anything wrong by touching
bus->phy_mask? Another solution would be to split mdiobus_register() and
create a mdiobus_register_noscan() or something, and then call that from
of_mdiobus_register().


Thanks,
Daniel

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Daniel,

2014-05-07 9:01 GMT-07:00 Daniel Mack <zonque@gmail.com>:
> (+ Mugunthan V N)
>
> Hi Florian,
>
> On 05/07/2014 02:55 AM, Florian Fainelli wrote:
>> 2014-05-06 8:21 GMT-07:00 Daniel Mack <zonque@gmail.com>:
>>> In of_mdiobus_register_phy(), check if the phy with the given address is
>>> already registered within the mii bus before calling phy_device_create()
>>> or get_phy_device().
>>
>> I am not exactly sure how you could be in that sort of situation.
>> of_mdiobus_register() handles two different cases at the moment:
>>
>> 1) PHY child nodes have a valid 'reg' property, and this property is
>> used to register the PHY at the given address
>> 2) if a PHY child node does not have a valid 'reg' property, which
>> will trigger an auto-scan and then we iterate through all 32 addresses
>> of the bus, we skip over PHYs that are already registered
>>
>>>
>>> This allows us to augment auto-probed phy devices with extra information
>>> via DT. Without this patch, a second instance of the phydev is created
>>> unnecessarily.
>>
>> Which piece of code is doing the auto-probing in your case? One of the
>> very first things that of_mdiobus_register() does is set the PHY mask
>> to 0xffffffff to prevent the default PHY probing method to trigger,
>> since we are using information from the Device Tree right after that.
>
> Ah, ok. So what happens here is that of_mdiobus_register() sets the
> phy_mask to ~0, but mdiobus_register() calls bus->reset(), which resets
> the mask to ffffffef in my case. bus->reset is davinci_mdio_reset() in
> my case.

I see, something similar exists with TI's CPMAC driver too, where the
MDIO bus can tell you which MDIO slave is alive.

>
> Is the davinci mdio driver doing anything wrong by touching
> bus->phy_mask?

Not really, I think it is valid to do this as part of the
mdiobus_reset() callback. This worked much better with non-DT
configurations because nothing would override the phy_mask.

> Another solution would be to split mdiobus_register() and
> create a mdiobus_register_noscan() or something, and then call that from
> of_mdiobus_register().

That, or have a specific MDIO bus controller node boolean property
such as  "mdio-bus-autoscan" or something similar which will tell
of_mdiobus_register() not to override the phy_mask since the MDIO bus
controller is capable of auto-detecting the PHYs present.

This should not be too controversial as we should really be describing
a feature of the hardware here.

What do you think?

Hi Florian,

On 05/07/2014 07:26 PM, Florian Fainelli wrote:
> 2014-05-07 9:01 GMT-07:00 Daniel Mack <zonque@gmail.com>:

>> Another solution would be to split mdiobus_register() and
>> create a mdiobus_register_noscan() or something, and then call that from
>> of_mdiobus_register().
> 
> That, or have a specific MDIO bus controller node boolean property
> such as  "mdio-bus-autoscan" or something similar which will tell
> of_mdiobus_register() not to override the phy_mask since the MDIO bus
> controller is capable of auto-detecting the PHYs present.
> 
> This should not be too controversial as we should really be describing
> a feature of the hardware here.

Hmm. Actually, we can't easily disable autoscanning for DT boards, with
or without the opt-in via "mdio-bus-autoscan", because that would force
all DT users to at least add this property, or add the sub-nodes
explicitly. Also, with "mdio-bus-autoscan", sub-nodes of the bus would
not be linked to the drivers' of_node pointer, which is confusing.

From a DT user point of view, I believe that the behaviour with my patch
applied is most convenient: if sub-nodes are given, and their 'reg'
properties match the addresses of auto-probed phys, they are linked to
the existing instances, so their properties can be used by the drivers.
The only nasty detail is that, as the code stands, dev->of_node is not
available at the phy driver's .probe() callback but earliest in
.config_init().

I'll see if I find a nicer implementation.


Thanks,
Daniel

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html