* [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC @ 2020-08-18 13:19 Adam Ford 2020-09-26 11:06 ` Adam Ford ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Adam Ford @ 2020-08-18 13:19 UTC (permalink / raw) To: u-boot 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> --- V2: Fix typo diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c index 09372d7f6b..1fa3667b77 100644 --- a/drivers/net/smc911x.c +++ b/drivers/net/smc911x.c @@ -6,6 +6,7 @@ */ #include <common.h> +#include <env.h> #include <command.h> #include <malloc.h> #include <net.h> @@ -185,6 +186,8 @@ static void smc911x_handle_mac_address(struct smc911x_priv *priv) smc911x_set_mac_csr(priv, ADDRH, addrh); printf(DRIVERNAME ": MAC %pM\n", m); + if (!env_get("ethaddr")) + env_set("ethaddr", (const char *)m); } static bool smc911x_read_mac_address(struct smc911x_priv *priv) -- 2.17.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC 2020-08-18 13:19 [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC Adam Ford @ 2020-09-26 11:06 ` Adam Ford 2020-09-28 18:03 ` Joe Hershberger 2020-10-01 14:09 ` Tom Rini 2 siblings, 0 replies; 12+ messages in thread From: Adam Ford @ 2020-09-26 11:06 UTC (permalink / raw) To: u-boot On Tue, Aug 18, 2020 at 8:19 AM Adam Ford <aford173@gmail.com> 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> > --- > V2: Fix typo > > Ping. Any chance this can get reviewed? thank you, adam > diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c > index 09372d7f6b..1fa3667b77 100644 > --- a/drivers/net/smc911x.c > +++ b/drivers/net/smc911x.c > @@ -6,6 +6,7 @@ > */ > > #include <common.h> > +#include <env.h> > #include <command.h> > #include <malloc.h> > #include <net.h> > @@ -185,6 +186,8 @@ static void smc911x_handle_mac_address(struct > smc911x_priv *priv) > smc911x_set_mac_csr(priv, ADDRH, addrh); > > printf(DRIVERNAME ": MAC %pM\n", m); > + if (!env_get("ethaddr")) > + env_set("ethaddr", (const char *)m); > } > > static bool smc911x_read_mac_address(struct smc911x_priv *priv) > -- > 2.17.1 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC 2020-08-18 13:19 [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC Adam Ford 2020-09-26 11:06 ` Adam Ford @ 2020-09-28 18:03 ` Joe Hershberger 2020-10-01 14:09 ` Tom Rini 2 siblings, 0 replies; 12+ messages in thread From: Joe Hershberger @ 2020-09-28 18:03 UTC (permalink / raw) To: u-boot On Tue, Aug 18, 2020 at 8:19 AM Adam Ford <aford173@gmail.com> 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC 2020-08-18 13:19 [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC Adam Ford 2020-09-26 11:06 ` Adam Ford 2020-09-28 18:03 ` Joe Hershberger @ 2020-10-01 14:09 ` Tom Rini 2020-10-01 17:48 ` Marek Vasut 2 siblings, 1 reply; 12+ messages in thread From: Tom Rini @ 2020-10-01 14:09 UTC (permalink / raw) To: u-boot 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! -- 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/9781a987/attachment.sig> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC 2020-10-01 14:09 ` Tom Rini @ 2020-10-01 17:48 ` Marek Vasut 2020-10-01 18:17 ` Tom Rini 0 siblings, 1 reply; 12+ messages in thread From: Marek Vasut @ 2020-10-01 17:48 UTC (permalink / raw) To: u-boot 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 ? ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC 2020-10-01 17:48 ` Marek Vasut @ 2020-10-01 18:17 ` Tom Rini 2020-10-01 18:22 ` Adam Ford 0 siblings, 1 reply; 12+ messages in thread From: Tom Rini @ 2020-10-01 18:17 UTC (permalink / raw) To: u-boot 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. 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/f94a6b7d/attachment.sig> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC 2020-10-01 18:17 ` Tom Rini @ 2020-10-01 18:22 ` Adam Ford 2020-10-01 18:28 ` Tom Rini 0 siblings, 1 reply; 12+ messages in thread From: Adam Ford @ 2020-10-01 18:22 UTC (permalink / raw) To: u-boot 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. adam > > Thanks! > > -- > Tom > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC 2020-10-01 18:22 ` Adam Ford @ 2020-10-01 18:28 ` Tom Rini 2020-10-01 18:42 ` Adam Ford 0 siblings, 1 reply; 12+ messages in thread From: Tom Rini @ 2020-10-01 18:28 UTC (permalink / raw) To: u-boot 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! -- 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/52c6e60d/attachment.sig> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC 2020-10-01 18:28 ` Tom Rini @ 2020-10-01 18:42 ` Adam Ford 2020-10-01 18:46 ` Marek Vasut 0 siblings, 1 reply; 12+ messages in thread From: Adam Ford @ 2020-10-01 18:42 UTC (permalink / raw) To: u-boot 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. adam > > -- > Tom > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC 2020-10-01 18:42 ` Adam Ford @ 2020-10-01 18:46 ` Marek Vasut 2020-10-01 18:51 ` Tom Rini 0 siblings, 1 reply; 12+ messages in thread From: Marek Vasut @ 2020-10-01 18:46 UTC (permalink / raw) To: u-boot 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 ? ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC 2020-10-01 18:46 ` Marek Vasut @ 2020-10-01 18:51 ` Tom Rini 2020-10-01 19:49 ` Marek Vasut 0 siblings, 1 reply; 12+ messages in thread From: Tom Rini @ 2020-10-01 18:51 UTC (permalink / raw) To: u-boot 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC 2020-10-01 18:51 ` Tom Rini @ 2020-10-01 19:49 ` Marek Vasut 0 siblings, 0 replies; 12+ messages in thread From: Marek Vasut @ 2020-10-01 19:49 UTC (permalink / raw) To: u-boot 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 <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! That's fine, but it would be good to figure out what was broken in that other setup too. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-10-01 19:49 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-18 13:19 [PATCH V2] net: smc911x: Automatically Update ethaddr with MAC Adam Ford 2020-09-26 11:06 ` Adam Ford 2020-09-28 18:03 ` Joe Hershberger 2020-10-01 14:09 ` Tom Rini 2020-10-01 17:48 ` Marek Vasut 2020-10-01 18:17 ` Tom Rini 2020-10-01 18:22 ` Adam Ford 2020-10-01 18:28 ` Tom Rini 2020-10-01 18:42 ` Adam Ford 2020-10-01 18:46 ` Marek Vasut 2020-10-01 18:51 ` Tom Rini 2020-10-01 19:49 ` Marek Vasut
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.