* [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS
@ 2015-12-29 14:05 Romain Perier
[not found] ` <1451397935-23643-1-git-send-email-romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 8+ messages in thread
From: Romain Perier @ 2015-12-29 14:05 UTC (permalink / raw)
To: peppe.cavallaro; +Cc: netdev, linux-rockchip
Originally, most of the platforms using this driver did not define an mdio subnode
in the devicetree. Commit e34d65 ("stmmac: create of compatible mdio bus for stmmac driver")
introduced a backward compatibily issue by using of_mdiobus_register explicitly
with an mdio subnode. This patch fixes the issue by calling the function
mdiobus_register, when mdio subnode is not found. The driver is now compatible
with both modes.
Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index 16c85cc..0034de44 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -218,8 +218,7 @@ int stmmac_mdio_register(struct net_device *ndev)
if (mdio_node) {
netdev_dbg(ndev, "FOUND MDIO subnode\n");
} else {
- netdev_err(ndev, "NO MDIO subnode\n");
- return 0;
+ netdev_warn(ndev, "No MDIO subnode found\n");
}
}
@@ -251,7 +250,10 @@ int stmmac_mdio_register(struct net_device *ndev)
new_bus->phy_mask = mdio_bus_data->phy_mask;
new_bus->parent = priv->device;
- err = of_mdiobus_register(new_bus, mdio_node);
+ if (IS_ENABLED(CONFIG_OF) && mdio_node)
+ err = of_mdiobus_register(new_bus, mdio_node);
+ else
+ err = mdiobus_register(new_bus);
if (err != 0) {
pr_err("%s: Cannot register as MDIO bus\n", new_bus->name);
goto bus_register_fail;
--
2.5.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS
[not found] ` <1451397935-23643-1-git-send-email-romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2016-01-02 5:51 ` Florian Fainelli
0 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2016-01-02 5:51 UTC (permalink / raw)
To: Romain Perier, peppe.cavallaro-qxv4g6HH51o
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On December 29, 2015 6:05:35 AM PST, Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>Originally, most of the platforms using this driver did not define an
>mdio subnode
>in the devicetree. Commit e34d65 ("stmmac: create of compatible mdio
>bus for stmmac driver")
>introduced a backward compatibily issue by using of_mdiobus_register
>explicitly
>with an mdio subnode. This patch fixes the issue by calling the
>function
>mdiobus_register, when mdio subnode is not found. The driver is now
>compatible
>with both modes.
Looks reasonable to me, though you will want to make sure the different DTSes get updated to include the proper compatible node for the MDIO bus.
>
>Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Please include a Fixes tag to help keep track of changes.
>---
> drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>index 16c85cc..0034de44 100644
>--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>@@ -218,8 +218,7 @@ int stmmac_mdio_register(struct net_device *ndev)
> if (mdio_node) {
> netdev_dbg(ndev, "FOUND MDIO subnode\n");
> } else {
>- netdev_err(ndev, "NO MDIO subnode\n");
>- return 0;
>+ netdev_warn(ndev, "No MDIO subnode found\n");
> }
> }
>
>@@ -251,7 +250,10 @@ int stmmac_mdio_register(struct net_device *ndev)
> new_bus->phy_mask = mdio_bus_data->phy_mask;
> new_bus->parent = priv->device;
>
>- err = of_mdiobus_register(new_bus, mdio_node);
>+ if (IS_ENABLED(CONFIG_OF) && mdio_node)
You should be able to drop the IS_ENABLED part since there is an inline provided in the non-OF case which does a fallback to mdiobus_register().
>+ err = of_mdiobus_register(new_bus, mdio_node);
>+ else
>+ err = mdiobus_register(new_bus);
> if (err != 0) {
This looks reasonable, does it work if we just assign mdio_node to the pdev->dev.of_node to be backwards compatible with DTSes which have not yet been updated?
> pr_err("%s: Cannot register as MDIO bus\n", new_bus->name);
> goto bus_register_fail;
--
Florian
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS
@ 2016-01-02 5:51 ` Florian Fainelli
0 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2016-01-02 5:51 UTC (permalink / raw)
To: Romain Perier, peppe.cavallaro-qxv4g6HH51o
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On December 29, 2015 6:05:35 AM PST, Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>Originally, most of the platforms using this driver did not define an
>mdio subnode
>in the devicetree. Commit e34d65 ("stmmac: create of compatible mdio
>bus for stmmac driver")
>introduced a backward compatibily issue by using of_mdiobus_register
>explicitly
>with an mdio subnode. This patch fixes the issue by calling the
>function
>mdiobus_register, when mdio subnode is not found. The driver is now
>compatible
>with both modes.
Looks reasonable to me, though you will want to make sure the different DTSes get updated to include the proper compatible node for the MDIO bus.
>
>Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Please include a Fixes tag to help keep track of changes.
>---
> drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>index 16c85cc..0034de44 100644
>--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>@@ -218,8 +218,7 @@ int stmmac_mdio_register(struct net_device *ndev)
> if (mdio_node) {
> netdev_dbg(ndev, "FOUND MDIO subnode\n");
> } else {
>- netdev_err(ndev, "NO MDIO subnode\n");
>- return 0;
>+ netdev_warn(ndev, "No MDIO subnode found\n");
> }
> }
>
>@@ -251,7 +250,10 @@ int stmmac_mdio_register(struct net_device *ndev)
> new_bus->phy_mask = mdio_bus_data->phy_mask;
> new_bus->parent = priv->device;
>
>- err = of_mdiobus_register(new_bus, mdio_node);
>+ if (IS_ENABLED(CONFIG_OF) && mdio_node)
You should be able to drop the IS_ENABLED part since there is an inline provided in the non-OF case which does a fallback to mdiobus_register().
>+ err = of_mdiobus_register(new_bus, mdio_node);
>+ else
>+ err = mdiobus_register(new_bus);
> if (err != 0) {
This looks reasonable, does it work if we just assign mdio_node to the pdev->dev.of_node to be backwards compatible with DTSes which have not yet been updated?
> pr_err("%s: Cannot register as MDIO bus\n", new_bus->name);
> goto bus_register_fail;
--
Florian
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS
2016-01-02 5:51 ` Florian Fainelli
(?)
@ 2016-01-02 15:11 ` Romain Perier
2016-01-02 17:52 ` Florian Fainelli
-1 siblings, 1 reply; 8+ messages in thread
From: Romain Perier @ 2016-01-02 15:11 UTC (permalink / raw)
To: Florian Fainelli; +Cc: peppe.cavallaro, netdev, open list:ARM/Rockchip SoC...
Hi all,
2016-01-02 6:51 GMT+01:00 Florian Fainelli <f.fainelli@gmail.com>:
> On December 29, 2015 6:05:35 AM PST, Romain Perier <romain.perier@gmail.com> wrote:
>>Originally, most of the platforms using this driver did not define an
>>mdio subnode
>>in the devicetree. Commit e34d65 ("stmmac: create of compatible mdio
>>bus for stmmac driver")
>>introduced a backward compatibily issue by using of_mdiobus_register
>>explicitly
>>with an mdio subnode. This patch fixes the issue by calling the
>>function
>>mdiobus_register, when mdio subnode is not found. The driver is now
>>compatible
>>with both modes.
>
> Looks reasonable to me, though you will want to make sure the different DTSes get updated to include the proper compatible node for the MDIO bus.
>
>>
>>Signed-off-by: Romain Perier <romain.perier@gmail.com>
>
> Please include a Fixes tag to help keep track of changes.
Sure, sorry for my ignorance, but I did not know that we're supposed
to add a Fixes tag in this situation.
>
>>---
>> drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>>diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>>b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>>index 16c85cc..0034de44 100644
>>--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>>+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>>@@ -218,8 +218,7 @@ int stmmac_mdio_register(struct net_device *ndev)
>> if (mdio_node) {
>> netdev_dbg(ndev, "FOUND MDIO subnode\n");
>> } else {
>>- netdev_err(ndev, "NO MDIO subnode\n");
>>- return 0;
>>+ netdev_warn(ndev, "No MDIO subnode found\n");
>> }
>> }
>>
>>@@ -251,7 +250,10 @@ int stmmac_mdio_register(struct net_device *ndev)
>> new_bus->phy_mask = mdio_bus_data->phy_mask;
>> new_bus->parent = priv->device;
>>
>>- err = of_mdiobus_register(new_bus, mdio_node);
>>+ if (IS_ENABLED(CONFIG_OF) && mdio_node)
>
> You should be able to drop the IS_ENABLED part since there is an inline provided in the non-OF case which does a fallback to mdiobus_register().
Good point
>
>>+ err = of_mdiobus_register(new_bus, mdio_node);
>>+ else
>>+ err = mdiobus_register(new_bus);
>> if (err != 0) {
>
> This looks reasonable, does it work if we just assign mdio_node to the pdev->dev.of_node to be backwards compatible with DTSes which have not yet been updated?
>
I don't understand what you mean. Instead of defining mdio_node with
NULL, you propose to assign it to pdev->dev.of_node as its default
value... then it would be reassigned with "child_nod" if a subnode
with the compatible string "snps,dwmac-mdio" is found, or kept with
its default value otherwise... and in this case we could call
of_mdiobus_register(new_bus, mdio_node) all the time ? (it removes the
conditonnal branch)
Regards,
Romain
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS
2016-01-02 15:11 ` Romain Perier
@ 2016-01-02 17:52 ` Florian Fainelli
[not found] ` <56880E46.4050206-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 8+ messages in thread
From: Florian Fainelli @ 2016-01-02 17:52 UTC (permalink / raw)
To: Romain Perier; +Cc: peppe.cavallaro, netdev, open list:ARM/Rockchip SoC...
Le 02/01/2016 07:11, Romain Perier a écrit :
>>> Signed-off-by: Romain Perier <romain.perier@gmail.com>
>>
>> Please include a Fixes tag to help keep track of changes.
>
> Sure, sorry for my ignorance, but I did not know that we're supposed
> to add a Fixes tag in this situation.
It is a good practice and definitively helps making sure that if
regression entered a merge window, and a fix came in another merge
window, people can easily backport the fix, moving on..
[snip]
>> This looks reasonable, does it work if we just assign mdio_node to the pdev->dev.of_node to be backwards compatible with DTSes which have not yet been updated?
>>
>
> I don't understand what you mean. Instead of defining mdio_node with
> NULL, you propose to assign it to pdev->dev.of_node as its default
> value... then it would be reassigned with "child_nod" if a subnode
> with the compatible string "snps,dwmac-mdio" is found, or kept with
> its default value otherwise... and in this case we could call
> of_mdiobus_register(new_bus, mdio_node) all the time ? (it removes the
> conditonnal branch)
Your understanding is correct, this is what I was suggesting, thanks
--
Florian
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS
[not found] ` <56880E46.4050206-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2016-01-03 14:36 ` Romain Perier
2016-01-07 19:45 ` Florian Fainelli
0 siblings, 1 reply; 8+ messages in thread
From: Romain Perier @ 2016-01-03 14:36 UTC (permalink / raw)
To: Florian Fainelli
Cc: peppe.cavallaro-qxv4g6HH51o, open list:ARM/Rockchip SoC..., netdev
Hi,
2016-01-02 18:52 GMT+01:00 Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>
>>> This looks reasonable, does it work if we just assign mdio_node to the pdev->dev.of_node to be backwards compatible with DTSes which have not yet been updated?
>>>
>>
>> I don't understand what you mean. Instead of defining mdio_node with
>> NULL, you propose to assign it to pdev->dev.of_node as its default
>> value... then it would be reassigned with "child_nod" if a subnode
>> with the compatible string "snps,dwmac-mdio" is found, or kept with
>> its default value otherwise... and in this case we could call
>> of_mdiobus_register(new_bus, mdio_node) all the time ? (it removes the
>> conditonnal branch)
>
> Your understanding is correct, this is what I was suggesting, thanks
So in this case, I already tested this yesterday, it does not work,
basically because the for loop just after of_mdiobus_register is never
executed, probably because new_bus->phy_map is not populated, which is
not the case by calling mdiobus_register for example.
of_mdiobus_register seems to populate this array of phys only for
subnodes found in the devicetree...
Regards,
Romain
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS
2016-01-03 14:36 ` Romain Perier
@ 2016-01-07 19:45 ` Florian Fainelli
2016-01-08 1:00 ` Phil Reid
0 siblings, 1 reply; 8+ messages in thread
From: Florian Fainelli @ 2016-01-07 19:45 UTC (permalink / raw)
To: Romain Perier; +Cc: peppe.cavallaro, netdev, open list:ARM/Rockchip SoC...
On 03/01/16 06:36, Romain Perier wrote:
> Hi,
>
> 2016-01-02 18:52 GMT+01:00 Florian Fainelli <f.fainelli@gmail.com>:
>>
>>>> This looks reasonable, does it work if we just assign mdio_node to the pdev->dev.of_node to be backwards compatible with DTSes which have not yet been updated?
>>>>
>>>
>>> I don't understand what you mean. Instead of defining mdio_node with
>>> NULL, you propose to assign it to pdev->dev.of_node as its default
>>> value... then it would be reassigned with "child_nod" if a subnode
>>> with the compatible string "snps,dwmac-mdio" is found, or kept with
>>> its default value otherwise... and in this case we could call
>>> of_mdiobus_register(new_bus, mdio_node) all the time ? (it removes the
>>> conditonnal branch)
>>
>> Your understanding is correct, this is what I was suggesting, thanks
>
> So in this case, I already tested this yesterday, it does not work,
> basically because the for loop just after of_mdiobus_register is never
> executed, probably because new_bus->phy_map is not populated, which is
> not the case by calling mdiobus_register for example.
> of_mdiobus_register seems to populate this array of phys only for
> subnodes found in the devicetree...
Fair enough, then you initial patch with dropping IS_ENABLED() and
adding a "Fixes" tag should be resubmitted.
Thank you
--
Florian
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS
2016-01-07 19:45 ` Florian Fainelli
@ 2016-01-08 1:00 ` Phil Reid
0 siblings, 0 replies; 8+ messages in thread
From: Phil Reid @ 2016-01-08 1:00 UTC (permalink / raw)
To: Florian Fainelli, Romain Perier
Cc: peppe.cavallaro, netdev, open list:ARM/Rockchip SoC...
On 8/01/2016 3:45 AM, Florian Fainelli wrote:
> On 03/01/16 06:36, Romain Perier wrote:
>> Hi,
>>
>> 2016-01-02 18:52 GMT+01:00 Florian Fainelli <f.fainelli@gmail.com>:
>>>
>>>>> This looks reasonable, does it work if we just assign mdio_node to the pdev->dev.of_node to be backwards compatible with DTSes which have not yet been updated?
>>>>>
>>>>
>>>> I don't understand what you mean. Instead of defining mdio_node with
>>>> NULL, you propose to assign it to pdev->dev.of_node as its default
>>>> value... then it would be reassigned with "child_nod" if a subnode
>>>> with the compatible string "snps,dwmac-mdio" is found, or kept with
>>>> its default value otherwise... and in this case we could call
>>>> of_mdiobus_register(new_bus, mdio_node) all the time ? (it removes the
>>>> conditonnal branch)
>>>
>>> Your understanding is correct, this is what I was suggesting, thanks
>>
>> So in this case, I already tested this yesterday, it does not work,
>> basically because the for loop just after of_mdiobus_register is never
>> executed, probably because new_bus->phy_map is not populated, which is
>> not the case by calling mdiobus_register for example.
>> of_mdiobus_register seems to populate this array of phys only for
>> subnodes found in the devicetree...
>
> Fair enough, then you initial patch with dropping IS_ENABLED() and
> adding a "Fixes" tag should be resubmitted.
>
Looks reasonable.
--
Regards
Phil Reid
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-01-08 1:00 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-29 14:05 [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS Romain Perier
[not found] ` <1451397935-23643-1-git-send-email-romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2016-01-02 5:51 ` Florian Fainelli
2016-01-02 5:51 ` Florian Fainelli
2016-01-02 15:11 ` Romain Perier
2016-01-02 17:52 ` Florian Fainelli
[not found] ` <56880E46.4050206-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2016-01-03 14:36 ` Romain Perier
2016-01-07 19:45 ` Florian Fainelli
2016-01-08 1:00 ` Phil Reid
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.