[U-Boot] [PATCH] net/phy: Fix phy_connect() for phy addr 0

[Marek Vasut]

On 12/18/19 5:15 PM, Joe Hershberger wrote:
> On Tue, Dec 17, 2019 at 11:55 PM Marek Vasut <marex at denx.de> wrote:
>>
>> On 12/18/19 3:06 AM, Joe Hershberger wrote:
>>> On Tue, Dec 17, 2019 at 1:04 PM Marek Vasut <marex at denx.de> wrote:
>>>>
>>>> On 12/17/19 7:47 PM, Joe Hershberger wrote:
>>>>> On Tue, Dec 17, 2019 at 11:46 AM Marek Vasut <marex at denx.de> wrote:
>>>>>>
>>>>>> On 12/17/19 5:25 PM, Joe Hershberger wrote:
>>>>>>> Hi Marek,
>>>>>>
>>>>>> Hi Joe,
>>>>>>
>>>>>>> On Tue, Dec 17, 2019 at 1:39 AM Marek Vasut <marex at denx.de> wrote:
>>>>>>>>
>>>>>>>> On 11/7/19 9:04 PM, Joe Hershberger wrote:
>>>>>>>>> On Thu, Nov 7, 2019 at 1:16 PM Tom Rini <trini at konsulko.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Tue, Nov 05, 2019 at 04:05:11AM +0000, Priyanka Jain wrote:
>>>>>>>>>>
>>>>>>>>>>> Fix 'mask' calculation in phy_connect() for phy addr '0'.
>>>>>>>>>>> 'mask' is getting set to '0xffffffff' for phy addr '0'
>>>>>>>>>>> in phy_connect() whereas expected value is '0'.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Priyanka Jain <priyanka.jain at nxp.com>
>>>>>>>>>>
>>>>>>>>>> Reported-by: tetsu-aoki via github
>>>>>>>>>
>>>>>>>>> Acked-by: Joe Hershberger <joe.hershberger at ni.com>
>>>>>>>>
>>>>>>>> Sadly, this breaks systems where a PHY is at address 0.
>>>>>>>> I have such an STM32MP1 system with LAN8720 PHY and since this patch, I
>>>>>>>> cannot use ethernet. Please revert.
>>>>>>>
>>>>>>> It seems like a case that shouldn't have worked before.
>>>>>>
>>>>>> Eh? PHY at address 0 definitely did work before and must work now.
>>>>>
>>>>> Agreed that a phy at address 0 should work. Not agreed that because
>>>>> the value "0" used to work due to a bug that it must still. Which of
>>>>> these is the statement you are making? Do we already agree or
>>>>> disagree?
>>>>
>>>> I am saying that because a board worked on rc4 and does not work on rc5,
>>>> this is a bug introduced by this patch in rc5 and must be fixed before
>>>> the release.
>>>>
>>>> The address 0 is a PHY broadcast address for some PHYs, it's a fixed
>>>> address for other PHYs. Thus, a PHY at address 0 must work. If this is
>>>> broken now, it's a bug.
>>>
>>> The only thing this patch should change is to not access addresses
>>> other than 0. I read the data sheet for the LAN8720 and it doesn't
>>> mention anything about any broadcast behavior, so I'm not sure what
>>> you're trying to state here.
>>
>> Read [1] section 3.7.1 PHYAD[2:0]: PHY ADDRESS CONFIGURATION
>>
>> What I am saying is that there are two types of PHYs, ones which treat
>> PHY address 0 as broadcast and ones which treat it as regular address.
>> This one is the later and is configured as such in my case.
>>
>> http://ww1.microchip.com/downloads/en/DeviceDoc/00002164B.pdf
> 
> I see. What's an example of a phy that treats 0 as broadcast?

IIRC KSZ9031 does.

>>>>>>> What about
>>>>>>> this board requires the mask to be all 'f's, other than specifying the
>>>>>>> wrong phy address? It seems that in your case the phy address is not
>>>>>>> actually 0 (or the computed mask would find it), but your board dts
>>>>>>> may be setting it to 0 as an "unknown" value, but the correct unknown
>>>>>>> value should be "-1". It seems the issue is with these boards.
>>>>>>
>>>>>> Nope, the address is actually configured to 0 in hardware.
>>>>>
>>>>> Can you double check that?
>>>>
>>>> No, sorry, I know the hardware is fixed to 0. Checking it again will not
>>>> change this fact.
>>>
>>> It seems there is no phy driver for this in U-Boot so the generic
>>> behavior is being used. I'm at a disadvantage of not having this board
>>> to try. Can you revert this patch and run with debug enabled for
>>> drivers/net/phy/phy.c to determine what is happening for this board? I
>>> would appreciate you helping with this.
>>
>> It only says "connected to Generic PHY" .
>>
>> So looking at the commit message, I am not really sure which board or
>> issue does this patch fix. But if I understand the commit message right,
>> then the aim is to set mask to 0 instead of 0xffffffff for address 0.
>> But that's not right either, the mask should be BIT(0) = 1 for address
>> 0, and that's what the patch actually does. I guess this then fails
>> somewhere further down the road ...
> 
> Yes, the commit message is wrong... the expected value is 1, not 0.  I
> missed that in the review.
> 
> Is the patch you sent earlier a solution for your board or something
> unrelated you found as a result of this discussion?

It works for my board, but I wonder how many other boards got broken here.