-----Original Message-----
From: Nicolas.Ferre@microchip.com <Nicolas.Ferre@microchip.com>
Sent: Wednesday, November 27, 2019 11:32 PM
To: Milind Parab <redacted>; 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
[off-list ref]; Parshuram Raju Thombare
[off-list ref]; 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:

quoted

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

quoted

Signed-off-by: Milind Parab <redacted>
---
  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

quoted

+       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

quoted

                         return ret;
                 }
         }
--
2.17.1

Best regards,
  Nicolas

--
Nicolas Ferre