Messages in this thread | | | From | Milind Parab <> | Subject | RE: [PATCH 1/3] net: macb: fix for fixed-link mode | Date | Thu, 28 Nov 2019 06:50:10 +0000 |
| |
>-----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
| |