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

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

[Priyanka Jain]

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>
---
 drivers/net/phy/phy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index ae37dd6c1e..419e6869c0 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -952,7 +952,7 @@ struct phy_device *phy_connect(struct mii_dev *bus, int addr,
 #endif
 {
 	struct phy_device *phydev = NULL;
-	uint mask = (addr > 0) ? (1 << addr) : 0xffffffff;
+	uint mask = (addr >= 0) ? (1 << addr) : 0xffffffff;
 
 #ifdef CONFIG_PHY_FIXED
 	phydev = phy_connect_fixed(bus, dev, interface);
-- 
2.17.1

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

[Tom Rini]

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191107/d1300644/attachment.sig>

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

[Joe Hershberger]

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>

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

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut

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

[Tom Rini]

On Tue, Dec 17, 2019 at 08:38:48AM +0100, Marek Vasut 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.

Well, a simple revert breaks the other boards that this fixes.  So we
need to think about how to fix all of the cases involved here, thanks!

-- 
Tom

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

[Marek Vasut]

On 12/17/19 1:52 PM, Tom Rini wrote:
> On Tue, Dec 17, 2019 at 08:38:48AM +0100, Marek Vasut 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.
> 
> Well, a simple revert breaks the other boards that this fixes.  So we
> need to think about how to fix all of the cases involved here, thanks!

That's probably task for the author of the patch.

btw -rc4 worked fine, -rc5 does not work.

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

[Joe Hershberger]

Hi Marek,

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. 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.

Cheers,
-Joe

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

[Marek Vasut]

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.

> 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.

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

[Joe Hershberger]

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?

> > 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? The code as is should compute a mask of
"0x01" which should match the offset for address 0. If it really is at
0 in hardware, maybe there is a different bug. Otherwise I don't see
how this patch would work for the author.

Thanks,
-Joe

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

[Marek Vasut]

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.

>>> 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.

> The code as is should compute a mask of
> "0x01" which should match the offset for address 0. If it really is at
> 0 in hardware, maybe there is a different bug. Otherwise I don't see
> how this patch would work for the author.

Reverting this patch makes things work again for me.

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

[Tom Rini]

On Tue, Dec 17, 2019, 2: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.
>
> >>> 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.
>
> > The code as is should compute a mask of
> > "0x01" which should match the offset for address 0. If it really is at
> > 0 in hardware, maybe there is a different bug. Otherwise I don't see
> > how this patch would work for the author.
>
> Reverting this patch makes things work again for me.
>

Ok but breaking other boards again to fix your board is also unacceptable.
It's not a theoretical case that something else failed previously.

>

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

[Marek Vasut]

On 12/18/19 12:35 AM, Tom Rini wrote:
> On Tue, Dec 17, 2019, 2: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.
>>
>>>>> 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.
>>
>>> The code as is should compute a mask of
>>> "0x01" which should match the offset for address 0. If it really is at
>>> 0 in hardware, maybe there is a different bug. Otherwise I don't see
>>> how this patch would work for the author.
>>
>> Reverting this patch makes things work again for me.
>>
> 
> Ok but breaking other boards again to fix your board is also unacceptable.
> It's not a theoretical case that something else failed previously.

At least it restores the behavior in -rc4 , so that is the last option
if there is no better patch.

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

[Joe Hershberger]

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.

> >>> 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.

> > The code as is should compute a mask of
> > "0x01" which should match the offset for address 0. If it really is at
> > 0 in hardware, maybe there is a different bug. Otherwise I don't see
> > how this patch would work for the author.
>
> Reverting this patch makes things work again for me.

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

[Marek Vasut]

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.

http://ww1.microchip.com/downloads/en/DeviceDoc/00002164B.pdf

>>>>> 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 ...

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

[Joe Hershberger]

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.
>
> http://ww1.microchip.com/downloads/en/DeviceDoc/00002164B.pdf

I see. What's an example of a phy that treats 0 as broadcast?

> >>>>> 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?

[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@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.
>>
>> 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.

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

[Priyanka Jain]

>-----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 

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

[Alexandru Marginean]

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