From mboxrd@z Thu Jan 1 00:00:00 1970 From: Priyanka Jain Date: Thu, 19 Dec 2019 06:30:47 +0000 Subject: [U-Boot] [PATCH] net/phy: Fix phy_connect() for phy addr 0 In-Reply-To: <789fab0b-6c5d-3860-3e1e-68cf02df2007@denx.de> References: <20191105040439.23450-1-priyanka.jain@nxp.com> <20191107191543.GK19317@bill-the-cat> <423b0427-c8b3-8e2e-d64c-ae06d79214b6@denx.de> <4cecdc1d-cf11-e26f-0bb9-deeeb2bd4d95@denx.de> <789fab0b-6c5d-3860-3e1e-68cf02df2007@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable To: u-boot@lists.denx.de >-----Original Message----- >From: U-Boot 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 ; Joseph >Hershberger >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 wrote: >>> >>> On 12/18/19 3:06 AM, Joe Hershberger wrote: >>>> On Tue, Dec 17, 2019 at 1:04 PM Marek Vasut wrote: >>>>> >>>>> On 12/17/19 7:47 PM, Joe Hershberger wrote: >>>>>> On Tue, Dec 17, 2019 at 11:46 AM Marek Vasut >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 >wrote: >>>>>>>>> >>>>>>>>> On 11/7/19 9:04 PM, Joe Hershberger wrote: >>>>>>>>>> On Thu, Nov 7, 2019 at 1:16 PM Tom Rini >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 >>>>>>>>>>> >>>>>>>>>>> Reported-by: tetsu-aoki via github >>>>>>>>>> >>>>>>>>>> Acked-by: Joe Hershberger >>>>>>>>> >>>>>>>>> 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=3Dhttp%3A%2F%2Fww1. >>> >microchip.com%2Fdownloads%2Fen%2FDeviceDoc%2F00002164B.pdf&da >ta=3D0 >>> >2%7C01%7Cpriyanka.jain%40nxp.com%7C5270d34d955647ee66ea08d783d5ab >c8%7 >>> >C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637122826047859376&a >mp;sd >>> >ata=3Ds22V5eU1kUe0030lbvWazQpooiM2OutlJbTxrPjbxs0%3D&reserved=3D0 >> >> 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 addre= ss 0. >>> But that's not right either, the mask should be BIT(0) =3D 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://gith= ub.com/u-boot/u-boot/commit/afbc31948a007e03d6a1282677aafc2208f45819#commit= comment-35747179 introduced by commit afbc31948a007e03d6a1282677aafc2208f45819 (net: phy: im= plement 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=E2= =80=99t have the board which is broken because of this current patch. So, n= eed help in analysis and proper fix. Thanks Priyanka=20