| Message ID | 1574759380-102986-1-git-send-email-mparab@cadence.com |
|---|---|
| State | Deferred |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: macb: cover letter | expand |
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
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
>-----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
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; } }
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(-)