On 10/1/20 8:51 PM, Tom Rini wrote: > On Thu, Oct 01, 2020 at 08:46:56PM +0200, Marek Vasut wrote: >> On 10/1/20 8:42 PM, Adam Ford wrote: >>> On Thu, Oct 1, 2020 at 1:28 PM Tom Rini <tr...@konsulko.com> wrote: >>> >>>> On Thu, Oct 01, 2020 at 01:22:37PM -0500, Adam Ford wrote: >>>>> On Thu, Oct 1, 2020 at 1:17 PM Tom Rini <tr...@konsulko.com> wrote: >>>>> >>>>>> On Thu, Oct 01, 2020 at 07:48:32PM +0200, Marek Vasut wrote: >>>>>>> On 10/1/20 4:09 PM, Tom Rini wrote: >>>>>>>> On Tue, Aug 18, 2020 at 08:19:02AM -0500, Adam Ford wrote: >>>>>>>> >>>>>>>>> The ethernet controller can read the MAC from EEPROM and display >>>> it, >>>>>>>>> but if ethaddr is not set, the ethernet is still unavailable. >>>>>>>>> >>>>>>>>> This patch checks will automatically set the MAC address if it has >>>>>>>>> not already been set. >>>>>>>>> >>>>>>>>> Signed-off-by: Adam Ford <aford...@gmail.com> >>>>>>>>> Acked-by: Joe Hershberger <joe.hershber...@ni.com> >>>>>>>> >>>>>>>> Applied to u-boot/next, thanks! >>>>>>> >>>>>>> Note that this breaks every single setup where smc911x is not primary >>>>>>> ethernet. On systems where smc911x is secondary ethernet, you need to >>>>>>> set eth1addr and so on, so please do fix that. >>>>>>> >>>>>>> Also, this kind of ethXaddr update should happen in the ethernet core >>>>>>> instead, drivers shouldn't really modify environment, no ? >>>>>> >>>>>> Interesting points. So, if smc911x is not the primary ether device, >>>>>> something else will have already set "ethaddr", most likely. We do >>>> have >>>>>> both the common case where "ethaddr" (and "eth1addr" and so forth) are >>>>>> set. >>>>>> >>>>>> Adam, when exactly did you run in to the case where ethaddr wasn't set >>>>>> correctly? Was it on a non-DM_ETH case? To Marek's last point, we do >>>>>> have drivers that set ethaddr/ethXaddr, but that's in the non-DM_ETH >>>>>> case. >>>>>> >>>>> >>>>> The only situation I tested was with DM_ETH since I thought it was going >>>> to >>>>> be a requirement by 2020.07. If ethaddr is already set, it shouldn't >>>>> override it, but I can see an issue where using the SMC911x as a >>>> secondary >>>>> controller may cause an issue because the driver at this level doesn't >>>> know. >>>>> >>>>> It seems like there should be a way to determine if the MAC address is >>>>> readable so the user doesn't need to enter it manually, but it's probably >>>>> not at the driver level based on the feedback. >>>>> >>>>> If you want to revert this patch, I won't object. I don't really have >>>> time >>>>> to develop a better one right now. >>>> >>>> Well, wait. Did you encounter a case where "ethaddr" was not >>>> automatically correctly set? A quick skim of the driver and it looks >>>> like it's doing everything needed for the common code to set "ethaddr" >>>> correctly from enetaddr the driver probed. Thanks! >>>> >>> >>> I haven't tried lately, but when booting the Logic PD OMAP3 boards, I was >>> seeing a message displaying the MAC address while simultaneously showing >>> the message that it didn't have an address, so DHCP calls would fail. I >>> could confirm that ethaddr was not set. However, if I manually set the >>> ethaddr to the value dumped by the SMC911x driver, save the environmental >>> variables then reset the board, the ethernet would work. It seemed like >>> the area where the SMC911x displayed the MAC address made sense to update >>> it since it just finished fetching it. so that's why I set up the patch the >>> way it was. I hadn't considered a use case where it wasn't the primary >>> ethernet controller. >> >> Unless there is something else broken, the driver does provide a >> callback to pull ethernet address from the EEPROM already, and that >> should permit the driver core to set ethXaddr. So can you please debug >> that, why it is not happening on your board, instead of adding the >> current workaround ? > > It does sound like something more fundamental is broken in that > particular setup if the generic code path in net/net-uclass.c to set > "ethaddr"/etc as appropriate is not called. I will revert this for now, > as you said. Thanks!
That's fine, but it would be good to figure out what was broken in that other setup too.
