From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Wed, 18 Dec 2019 17:16:30 +0100 Subject: [U-Boot] [PATCH] net/phy: Fix phy_connect() for phy addr 0 In-Reply-To: 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> Message-ID: <789fab0b-6c5d-3860-3e1e-68cf02df2007@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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. >> >> 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.