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

[Marek Vasut]

On 06/19/2017 05:55 PM, Joe Hershberger wrote:
> On Mon, Jun 19, 2017 at 4:37 AM, Marek Vasut <marex at denx.de> wrote:
>> 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.
> 
> OK.
> 
>>>>>               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 ?
> 
> That's how I read it.

Sigh, well ... if you could split the patch in two, that'd be nice. I
will try to test it as soon as I have a chance. If it's broken, I'll try
to bisect it then.

-- 
Best regards,
Marek Vasut