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

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