On Tue, Dec 17, 2019 at 11:55 PM Marek Vasut <ma...@denx.de> wrote: > > On 12/18/19 3:06 AM, Joe Hershberger wrote: > > On Tue, Dec 17, 2019 at 1:04 PM Marek Vasut <ma...@denx.de> wrote: > >> > >> On 12/17/19 7:47 PM, Joe Hershberger wrote: > >>> On Tue, Dec 17, 2019 at 11:46 AM Marek Vasut <ma...@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 <ma...@denx.de> wrote: > >>>>>> > >>>>>> On 11/7/19 9:04 PM, Joe Hershberger wrote: > >>>>>>> On Thu, Nov 7, 2019 at 1:16 PM Tom Rini <tr...@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.j...@nxp.com> > >>>>>>>> > >>>>>>>> Reported-by: tetsu-aoki via github > >>>>>>> > >>>>>>> Acked-by: Joe Hershberger <joe.hershber...@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?
