[U-Boot] [RFC PATCH] net: ag7xxx: Clean up some issues with phy access

[Marek Vasut]

On 06/13/2017 06:28 PM, Joe Hershberger wrote:
> On Tue, Jun 13, 2017 at 4:24 AM, Marek Vasut <marex at denx.de> wrote:
>> On 06/12/2017 10:20 PM, Joe Hershberger wrote:
>>> Don't wait forever, Pass errors back, etc.
>>>
>>> Signed-off-by: Joe Hershberger <joe.hershberger at ni.com>
>>>
>>> ---
>>> This is a pass at improving the code quality.
>>> This has not been tested in any way.
>>>
>>>  drivers/net/ag7xxx.c | 63 +++++++++++++++++++++++++++++++++++++++++-----------
>>>  1 file changed, 50 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c
>>> index cf60d11..c8352d1 100644
>>> --- a/drivers/net/ag7xxx.c
>>> +++ b/drivers/net/ag7xxx.c
> 
> [...] SNIP
> 
>>> @@ -723,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev)
>>>                       return ret;
>>>
>>>               /* Read out link status */
>>> -             ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR);
>>> +             ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR);
>>>               if (ret < 0)
>>>                       return ret;
>>>
>>> +             if (!(ret & AG7XXX_PHY_PSSR_LINK_UP))
>>> +                     return -ENOLINK;
>>
>> Are you sure about this ?
> 
> It seems reasonable to me, but I don't have the HW to test against as
> noted above.

CCing Wills . I wouldn't be surprised if the hardware was somehow
magically screwed up, so I'd prefer to stick with equivalent changes and
maybe changes of the logic in a separate patch.

>>>               return 0;
>>>       }
>>>
>>> @@ -743,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev)
>>>                       return ret;
>>>       }
>>>
>>> -     for (i = 0; i < phymax; i++) {
>>> -             /* Read out link status */
>>> -             ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR);
>>> -             if (ret < 0)
>>> -                     return ret;
>>> -     }
>>
>> And this ?
> 
> This was based on your comment: "Actually, I think this is only for
> the switch ports, so we don't care about the link status."

Just so I understand it correctly, the code reads link status and does
nothing with it ?

-- 
Best regards,
Marek Vasut