[PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS

[PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS

[Romain Perier]

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

Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS

[Florian Fainelli]

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

Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS

[Romain Perier]

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

Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS

[Florian Fainelli]

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

Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS

[Romain Perier]

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

Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS

[Florian Fainelli]

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

Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS

[Phil Reid]

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