* [PATCH] net: mdio: of_mdio: check for already registered phy before creating new instances
@ 2014-05-06 15:21 Daniel Mack
2014-05-07 0:55 ` Florian Fainelli
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Mack @ 2014-05-06 15:21 UTC (permalink / raw)
To: f.fainelli; +Cc: davem, netdev, grant.likely, robh+dt, Daniel Mack
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(-)
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net: mdio: of_mdio: check for already registered phy before creating new instances
2014-05-06 15:21 [PATCH] net: mdio: of_mdio: check for already registered phy before creating new instances Daniel Mack
@ 2014-05-07 0:55 ` Florian Fainelli
2014-05-07 16:01 ` Daniel Mack
0 siblings, 1 reply; 5+ messages in thread
From: Florian Fainelli @ 2014-05-07 0:55 UTC (permalink / raw)
To: Daniel Mack; +Cc: David Miller, netdev, Grant Likely, Rob Herring
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
>
--
Florian
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: mdio: of_mdio: check for already registered phy before creating new instances
2014-05-07 0:55 ` Florian Fainelli
@ 2014-05-07 16:01 ` Daniel Mack
2014-05-07 17:26 ` Florian Fainelli
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Mack @ 2014-05-07 16:01 UTC (permalink / raw)
To: Florian Fainelli
Cc: David Miller, netdev, Grant Likely, Rob Herring, Mugunthan V N
(+ 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: mdio: of_mdio: check for already registered phy before creating new instances
2014-05-07 16:01 ` Daniel Mack
@ 2014-05-07 17:26 ` Florian Fainelli
2014-05-08 7:40 ` Daniel Mack
0 siblings, 1 reply; 5+ messages in thread
From: Florian Fainelli @ 2014-05-07 17:26 UTC (permalink / raw)
To: Daniel Mack
Cc: David Miller, netdev, Grant Likely, Rob Herring, Mugunthan V N
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?
--
Florian
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: mdio: of_mdio: check for already registered phy before creating new instances
2014-05-07 17:26 ` Florian Fainelli
@ 2014-05-08 7:40 ` Daniel Mack
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Mack @ 2014-05-08 7:40 UTC (permalink / raw)
To: Florian Fainelli
Cc: David Miller, netdev, Grant Likely, Rob Herring, Mugunthan V N
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-05-08 7:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-06 15:21 [PATCH] net: mdio: of_mdio: check for already registered phy before creating new instances Daniel Mack
2014-05-07 0:55 ` Florian Fainelli
2014-05-07 16:01 ` Daniel Mack
2014-05-07 17:26 ` Florian Fainelli
2014-05-08 7:40 ` Daniel Mack
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.