[PATCH V2] net: smc911x: Automatically Update ethaddr with MAC

[Tom Rini <trini@konsulko.com>]

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 <trini@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 <trini@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 <aford173@gmail.com>
> >>>>>>> Acked-by: Joe Hershberger <joe.hershberger@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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201001/dcd42f59/attachment.sig>