RE: [PATCH 1/3] net: macb: fix for fixed-link mode

[Milind Parab]

>-----Original Message-----
>From: Nicolas.Ferre@xxxxxxxxxxxxx <Nicolas.Ferre@xxxxxxxxxxxxx>
>Sent: Wednesday, November 27, 2019 11:32 PM
>To: Milind Parab <mparab@xxxxxxxxxxx>; andrew@xxxxxxx;
>antoine.tenart@xxxxxxxxxxx; f.fainelli@xxxxxxxxx
>Cc: davem@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; hkallweit1@xxxxxxxxx;
>linux-kernel@xxxxxxxxxxxxxxx; Dhananjay Vilasrao Kangude
><dkangude@xxxxxxxxxxx>; Parshuram Raju Thombare
><pthombar@xxxxxxxxxxx>; a.fatoum@xxxxxxxxxxxxxx;
>brad.mouring@xxxxxx; rmk+kernel@xxxxxxxxxxxxxxx
>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@xxxxxxxxxxx>
>> ---
>>   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