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

[Alexandru Marginean <alexm.osslist@gmail.com>]

On 12/19/2019 7:30 AM, Priyanka Jain wrote:
>> -----Original Message-----
>> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Marek Vasut
>> Sent: Wednesday, December 18, 2019 9:47 PM
>> To: joe.hershberger at ni.com
>> Cc: u-boot at lists.denx.de; Tom Rini <trini@konsulko.com>; Joseph
>> Hershberger <joseph.hershberger@ni.com>
>> Subject: Re: [U-Boot] [PATCH] net/phy: Fix phy_connect() for phy addr 0
>>
>> On 12/18/19 5:15 PM, Joe Hershberger wrote:
>>> On Tue, Dec 17, 2019 at 11:55 PM Marek Vasut <marex@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@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@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@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@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@nxp.com>
>>>>>>>>>>>>
>>>>>>>>>>>> Reported-by: tetsu-aoki via github
>>>>>>>>>>>
>>>>>>>>>>> Acked-by: Joe Hershberger <joe.hershberger@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.
>>>>
>>>> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww1.
>>>>
>> microchip.com%2Fdownloads%2Fen%2FDeviceDoc%2F00002164B.pdf&amp;da
>> ta=0
>>>>
>> 2%7C01%7Cpriyanka.jain%40nxp.com%7C5270d34d955647ee66ea08d783d5ab
>> c8%7
>>>>
>> C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637122826047859376&a
>> mp;sd
>>>>
>> ata=s22V5eU1kUe0030lbvWazQpooiM2OutlJbTxrPjbxs0%3D&amp;reserved=0
>>>
>>> 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.
> I also realized a mistake in commit message . The value of mask  will be 1.
> 
> This current patch was basically a fix to an issue reported at https://github.com/u-boot/u-boot/commit/afbc31948a007e03d6a1282677aafc2208f45819#commitcomment-35747179
> introduced by commit afbc31948a007e03d6a1282677aafc2208f45819 (net: phy: implement fallback mechanism for negative phy addresses)
> Before this commit, the argument value passed to  phy_find_by_mask() in phy_connect() for phy addr '0' was '1' , so this current patch tries to revert to same value '1'. So, not sure how has this current patch has introduced the issue .
> Via git log, I can see many other commits related to phy_id '0'. I don’t have the board which is broken because of this current patch. So, need help in analysis and proper fix.
> 
> Thanks
> Priyanka
> 

The fix is OK, the issue is that some of the drivers were using addr 0 
in phy_connect() to trigger a MDIO bus scan, not to connect to PHY @ addr 0.
I am using a board with a PHY @ addr 0 and it works fine.

Grepping though the code it seems a couple of drivers hardcode addr 0:
- dwc_eth_qos already solved by Marek in a separate patch,
- bcm-sf2-eth.

Other drivers may use 0 but passed though a variable, those are harder 
to spot.

Converting bcm-sf2-eth to phy_connect(-1) may be OK, it would work like 
it did before this patch.

Alex