[1/3] net: macb: fix for fixed-link mode
diff mbox series

Message ID 1574759380-102986-1-git-send-email-mparab@cadence.com
State Deferred
Delegated to: David Miller
Headers show
Series
  • net: macb: cover letter
Related show

Commit Message

Milind Parab Nov. 26, 2019, 9:09 a.m. UTC
This patch fix the issue with fixed link mode in macb.

Signed-off-by: Milind Parab <mparab@cadence.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

Andrew Lunn Nov. 26, 2019, 2:30 p.m. UTC | #1
On Tue, Nov 26, 2019 at 09:09:40AM +0000, Milind Parab wrote:
> This patch fix the issue with fixed link mode in macb.

Hi Milind.

What issue is it fixing? Please give a good description here, so we
can understand what/why the patch exists.

    Andrew
Nicolas Ferre Nov. 27, 2019, 6:02 p.m. UTC | #2
On 26/11/2019 at 10:09, Milind Parab wrote:
> This patch fix the issue with fixed link mode in macb.

I would need more context here. What needs to be fixed?

I think we had several attempts, at the phylib days, to have this part 
of the driver behave correctly, so providing us more insight will help 
understand what is going wrong now.
For instance, is it related to the patch that converts the driver to the 
phylink interface done by this patch in net-next "net: macb: convert to 
phylink"?


> Signed-off-by: Milind Parab <mparab@cadence.com>
> ---
>   drivers/net/ethernet/cadence/macb_main.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index d5ae2e1e0b0e..5e6d27d33d43 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -617,15 +617,11 @@ static int macb_phylink_connect(struct macb *bp)
>          struct phy_device *phydev;
>          int ret;
> 
> -       if (bp->pdev->dev.of_node &&
> -           of_parse_phandle(bp->pdev->dev.of_node, "phy-handle", 0)) {

You mean we don't need to parse this phandle anymore because it's better 
handled by phylink_of_phy_connect() below that takes care of the 
fixed-link case?
If yes, then telling it in commit message is worth it...

> +       if (bp->pdev->dev.of_node)
>                  ret = phylink_of_phy_connect(bp->phylink, bp->pdev->dev.of_node,
>                                               0);
> -               if (ret) {
> -                       netdev_err(dev, "Could not attach PHY (%d)\n", ret);
> -                       return ret;
> -               }
> -       } else {
> +
> +       if ((!bp->pdev->dev.of_node || ret == -ENODEV) && bp->mii_bus) {
>                  phydev = phy_find_first(bp->mii_bus);
>                  if (!phydev) {
>                          netdev_err(dev, "no PHY found\n");
> @@ -635,7 +631,7 @@ static int macb_phylink_connect(struct macb *bp)
>                  /* attach the mac to the phy */
>                  ret = phylink_connect_phy(bp->phylink, phydev);
>                  if (ret) {
> -                       netdev_err(dev, "Could not attach to PHY (%d)\n", ret);
> +                       netdev_err(dev, "Could not attach to PHY\n");

Why modifying this?

>                          return ret;
>                  }
>          }
> --
> 2.17.1
> 

Best regards,
   Nicolas
Milind Parab Nov. 28, 2019, 6:50 a.m. UTC | #3
>-----Original Message-----
>From: Nicolas.Ferre@microchip.com <Nicolas.Ferre@microchip.com>
>Sent: Wednesday, November 27, 2019 11:32 PM
>To: Milind Parab <mparab@cadence.com>; andrew@lunn.ch;
>antoine.tenart@bootlin.com; f.fainelli@gmail.com
>Cc: davem@davemloft.net; netdev@vger.kernel.org; hkallweit1@gmail.com;
>linux-kernel@vger.kernel.org; Dhananjay Vilasrao Kangude
><dkangude@cadence.com>; Parshuram Raju Thombare
><pthombar@cadence.com>; a.fatoum@pengutronix.de;
>brad.mouring@ni.com; rmk+kernel@armlinux.org.uk
>Subject: Re: [PATCH 1/3] net: macb: fix for fixed-link mode
>
>EXTERNAL MAIL
>
>
>On 26/11/2019 at 10:09, Milind Parab wrote:
>> This patch fix the issue with fixed link mode in macb.
>
>I would need more context here. What needs to be fixed?
>
>I think we had several attempts, at the phylib days, to have this part of the
>driver behave correctly, so providing us more insight will help understand
>what is going wrong now.
>For instance, is it related to the patch that converts the driver to the phylink
>interface done by this patch in net-next "net: macb: convert to phylink"?
>
Yes, this is related to the patch that converts the driver to phylink
With phylink patch, in fixed-link the device open is failing. The reason for failure is because
here an attempt is made to search and connect to PHY even for the  fixed-link.
phylink_of_phy_connect() handles this case well, and for the fixed-link it just returns 0. 
So, further steps to search and connect to PHY are not needed. 
This patch solves this problem by allowing phylink_of_phy_connect() to take this decision

>
>> Signed-off-by: Milind Parab <mparab@cadence.com>
>> ---
>>   drivers/net/ethernet/cadence/macb_main.c | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index d5ae2e1e0b0e..5e6d27d33d43 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -617,15 +617,11 @@ static int macb_phylink_connect(struct macb *bp)
>>          struct phy_device *phydev;
>>          int ret;
>>
>> -       if (bp->pdev->dev.of_node &&
>> -           of_parse_phandle(bp->pdev->dev.of_node, "phy-handle", 0)) {
>
>You mean we don't need to parse this phandle anymore because it's better
>handled by phylink_of_phy_connect() below that takes care of the fixed-link
>case?
>If yes, then telling it in commit message is worth it...

Yes, this we will explain in the commit message

>
>> +       if (bp->pdev->dev.of_node)
>>                  ret = phylink_of_phy_connect(bp->phylink, bp->pdev-
>>dev.of_node,
>>                                               0);
>> -               if (ret) {
>> -                       netdev_err(dev, "Could not attach PHY (%d)\n", ret);
>> -                       return ret;
>> -               }
>> -       } else {
>> +
>> +       if ((!bp->pdev->dev.of_node || ret == -ENODEV) && bp->mii_bus)
>> + {
>>                  phydev = phy_find_first(bp->mii_bus);
>>                  if (!phydev) {
>>                          netdev_err(dev, "no PHY found\n"); @@ -635,7
>> +631,7 @@ static int macb_phylink_connect(struct macb *bp)
>>                  /* attach the mac to the phy */
>>                  ret = phylink_connect_phy(bp->phylink, phydev);
>>                  if (ret) {
>> -                       netdev_err(dev, "Could not attach to PHY (%d)\n", ret);
>> +                       netdev_err(dev, "Could not attach to PHY\n");
>
>Why modifying this?

This is by mistake. This will be corrected in the revision patch

>
>>                          return ret;
>>                  }
>>          }
>> --
>> 2.17.1
>>
>
>Best regards,
>   Nicolas
>
>--
>Nicolas Ferre

Patch
diff mbox series

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index d5ae2e1e0b0e..5e6d27d33d43 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -617,15 +617,11 @@  static int macb_phylink_connect(struct macb *bp)
 	struct phy_device *phydev;
 	int ret;
 
-	if (bp->pdev->dev.of_node &&
-	    of_parse_phandle(bp->pdev->dev.of_node, "phy-handle", 0)) {
+	if (bp->pdev->dev.of_node)
 		ret = phylink_of_phy_connect(bp->phylink, bp->pdev->dev.of_node,
 					     0);
-		if (ret) {
-			netdev_err(dev, "Could not attach PHY (%d)\n", ret);
-			return ret;
-		}
-	} else {
+
+	if ((!bp->pdev->dev.of_node || ret == -ENODEV) && bp->mii_bus) {
 		phydev = phy_find_first(bp->mii_bus);
 		if (!phydev) {
 			netdev_err(dev, "no PHY found\n");
@@ -635,7 +631,7 @@  static int macb_phylink_connect(struct macb *bp)
 		/* attach the mac to the phy */
 		ret = phylink_connect_phy(bp->phylink, phydev);
 		if (ret) {
-			netdev_err(dev, "Could not attach to PHY (%d)\n", ret);
+			netdev_err(dev, "Could not attach to PHY\n");
 			return ret;
 		}
 	}