lkml.org 
[lkml]   [2019]   [Nov]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 1/3] net: macb: fix for fixed-link mode
Date

>-----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

\
 
 \ /
  Last update: 2019-11-28 07:51    [W:0.066 / U:1.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site