| Message ID | 20161108155437.1085-8-oliver@schinagl.nl |
|---|---|
| State | Superseded |
| Delegated to: | Joe Hershberger |
| Headers | show |
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl <oliver@schinagl.nl> wrote: > Currently we inject 5 ethernet addresses into the environment, just in > case we may need them. We do this because some boards have no eeprom > (programmed) with a proper ethernet address. With the recent addition of > reading actual ethernet addresses from the eeprom via the net_op we > should not inject environment variables any more. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
Hi, On 15-11-16 04:25, Joe Hershberger wrote: > On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl <oliver@schinagl.nl> wrote: >> Currently we inject 5 ethernet addresses into the environment, just in >> case we may need them. We do this because some boards have no eeprom >> (programmed) with a proper ethernet address. With the recent addition of >> reading actual ethernet addresses from the eeprom via the net_op we >> should not inject environment variables any more. >> >> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > > Acked-by: Joe Hershberger <joe.hershberger@ni.com> Erm, this patch seems wrong to me: NACK, let me explain: 1) It does not do what its commit message says, it only removes the second step for setting ethernet addresses from the env, but it keeps the existing code to set them AFAICT, it only does it once now. 2) "Currently we inject 5 ethernet addresses into the environment", this is not true, we only inject ethernet addresses into the environment for devices which have an ethernet alias in dt, so maximum 2 for devices with both wired ethernet and wifi 3) The second attempt at setting ethernet addresses in the environment after loading the kernel dt is necessary because the kernel dt may be newer and contain more ethernet aliases, e.g. the u-boot dt may only contain the nodes + alias for the wired, while the (newer) kernel dt may also contain a dt-node + alias for the wireless 4) We cannot solely rely on the ethernet driver to set mac-addresses, because the ethernet driver may not be enabled while the kernel does have the ethernet driver enabled; and the kernel relies on u-boot to generate fixed mac-addresses based on the SID independent whether or not u-boot has ethernet enabled, this is especially relevant for wifi chips where the kernel also relies on u-boot generated fixed mac-addresses on e.g. the recent orange-pi boards, which come with a realtek rtl8189etv chip which does not have a mac address programmed. 5) AFAIK the dt code for passing mac-addresses to the kernel relies on the environment variables, so even if we get the mac-address from a ROM we should still store it in the environment variable. Regards, Hans
Hey Hans, I was hopeing and expecting this :) As you will be able to tell below, I need to learn a bit more as to why we do things and discuss this proper I guess. On 15-11-16 10:26, Hans de Goede wrote: > Hi, > > On 15-11-16 04:25, Joe Hershberger wrote: >> On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl <oliver@schinagl.nl> >> wrote: >>> Currently we inject 5 ethernet addresses into the environment, just in >>> case we may need them. We do this because some boards have no eeprom >>> (programmed) with a proper ethernet address. With the recent >>> addition of >>> reading actual ethernet addresses from the eeprom via the net_op we >>> should not inject environment variables any more. >>> >>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >> >> Acked-by: Joe Hershberger <joe.hershberger@ni.com> > > > Erm, this patch seems wrong to me: NACK, let me > explain: > > 1) It does not do what its commit message says, it only > removes the second step for setting ethernet addresses > from the env, but it keeps the existing code to set them > AFAICT, it only does it once now. > > 2) "Currently we inject 5 ethernet addresses into the environment", > this is not true, we only inject ethernet addresses into the > environment for devices which have an ethernet alias in dt, > so maximum 2 for devices with both wired ethernet and wifi If we want the fdt to do mac things, shouldn't that be done at a higher level? This is not really board specific is it? > > 3) The second attempt at setting ethernet addresses in > the environment after loading the kernel dt is necessary > because the kernel dt may be newer and contain more > ethernet aliases, e.g. the u-boot dt may only contain the > nodes + alias for the wired, while the (newer) kernel dt > may also contain a dt-node + alias for the wireless I agree with you here, but I still don't think this should be board specific > > 4) We cannot solely rely on the ethernet driver to set > mac-addresses, because the ethernet driver may not be enabled > while the kernel does have the ethernet driver enabled; and > the kernel relies on u-boot to generate fixed mac-addresses > based on the SID independent whether or not u-boot has > ethernet enabled, this is especially relevant for wifi > chips where the kernel also relies on u-boot generated > fixed mac-addresses on e.g. the recent orange-pi boards, > which come with a realtek rtl8189etv chip which does not > have a mac address programmed. I agree, and I'll fix that in my new patch series proper by making rtl8189etv also read rom hook which IS board specific > > 5) AFAIK the dt code for passing mac-addresses to the kernel > relies on the environment variables, so even if we get the > mac-address from a ROM we should still store it in the > environment variable. The new patch series does that, as the net core layer does that. What happens is (note code is mangled and might not be 100% accurate, i reduced it the bares): eth_read_eeprom_hwaddr(dev); first read from the eeprom, which may return all zero's if it is unconfigured/missconfigured or should not be used from the eeprom. if (is_zero_ethaddr(pdata->enetaddr)) if (eth_get_ops(dev)->read_rom_hwaddr) eth_get_ops(dev)->read_rom_hwaddr(dev); if the eeprom failed to produce a mac, we check the read_rom_hwaddr callback, which hooks into the driver. The driver can be overridden by a board (such as sunxi) where the MAC is generated from the SID. so at this point we may have a hwaddress actually obtained from the hardware, via the eeprom (or some fixed rom even) or from the hardware itself next we allow 'software' overrides. e.g. u-boot env (and i think this is where the fdt method should live as well eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr); if (!is_zero_ethaddr(env_enetaddr)) { if (!is_zero_ethaddr(pdata->enetaddr) && memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN); // <snip> we compare the HW addr and the ENV addr. if the env is unset, we use whatever the hardware supplied us with. if the env is set, it overrides the HW addr. I think next would be to check the fdt to override the env? } else if (is_valid_ethaddr(pdata->enetaddr)) { eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr); Finally, we set whatever mac has come from the above probing into the environment (if the address is actually a valid MAC). } else if (is_zero_ethaddr(pdata->enetaddr)) { #ifdef CONFIG_NET_RANDOM_ETHADDR net_random_ethaddr(pdata->enetaddr); otherwise (if configured) let u-boot generate it. So I think here is where the fdt override should live, as it applies to everyone, not just sunxi. I'll post my split-up new series for review after testing it, so we can discuss it in more detail? Olliver > > Regards, > > Hans >
Hi, On 15-11-16 11:17, Olliver Schinagl wrote: > Hey Hans, > > I was hopeing and expecting this :) > > > As you will be able to tell below, I need to learn a bit more as to why we do things and discuss this proper I guess. > > > On 15-11-16 10:26, Hans de Goede wrote: >> Hi, >> >> On 15-11-16 04:25, Joe Hershberger wrote: >>> On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl <oliver@schinagl.nl> wrote: >>>> Currently we inject 5 ethernet addresses into the environment, just in >>>> case we may need them. We do this because some boards have no eeprom >>>> (programmed) with a proper ethernet address. With the recent addition of >>>> reading actual ethernet addresses from the eeprom via the net_op we >>>> should not inject environment variables any more. >>>> >>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>> >>> Acked-by: Joe Hershberger <joe.hershberger@ni.com> >> >> >> Erm, this patch seems wrong to me: NACK, let me >> explain: >> >> 1) It does not do what its commit message says, it only >> removes the second step for setting ethernet addresses >> from the env, but it keeps the existing code to set them >> AFAICT, it only does it once now. >> >> 2) "Currently we inject 5 ethernet addresses into the environment", >> this is not true, we only inject ethernet addresses into the >> environment for devices which have an ethernet alias in dt, >> so maximum 2 for devices with both wired ethernet and wifi > If we want the fdt to do mac things, shouldn't that be done at a higher level? This is not really board specific is it? We want to put mac addresses into the fdt, and this is done at a higher level, by existing dt code, which looks at ethernet aliases in dt and then for any which are present, checks the corresponding ethaddr env setting and if set, injects that mac address into the dt-node the alias points to. What is sunxi specific is setting the environment variables based on the SID. The sunxi specific code also checks the aliases, exactly to avoid the "inject 5 ethernet addresses" thing you are describing, as we don't want to inject ethernet addresses for non existent NICs as that may confuse the user. >> 3) The second attempt at setting ethernet addresses in >> the environment after loading the kernel dt is necessary >> because the kernel dt may be newer and contain more >> ethernet aliases, e.g. the u-boot dt may only contain the >> nodes + alias for the wired, while the (newer) kernel dt >> may also contain a dt-node + alias for the wireless > I agree with you here, but I still don't think this should be board specific Instead of doing this through the environment I guess you could have the u-boot dt code which is injecting the MACs into the dt call some board specific callback when there is no MAC set in the environment, and implement a weak stub for this. Then all the sunxi environment mangling code can go away, and sunxi can simply: 1) Try eeprom if present 2) Do the current SID based thing >> 4) We cannot solely rely on the ethernet driver to set >> mac-addresses, because the ethernet driver may not be enabled >> while the kernel does have the ethernet driver enabled; and >> the kernel relies on u-boot to generate fixed mac-addresses >> based on the SID independent whether or not u-boot has >> ethernet enabled, this is especially relevant for wifi >> chips where the kernel also relies on u-boot generated >> fixed mac-addresses on e.g. the recent orange-pi boards, >> which come with a realtek rtl8189etv chip which does not >> have a mac address programmed. > I agree, and I'll fix that in my new patch series proper by making rtl8189etv also read rom hook which IS board specific The problem is that u-boot may not have a driver for one of the NICs at all, so no place to call the rom hook at all. Anyways I believe this is solved by my suggestion for making the u-boot dt code which injects the MAC call a board specific callback when no ethaddr is set in the env. >> 5) AFAIK the dt code for passing mac-addresses to the kernel >> relies on the environment variables, so even if we get the >> mac-address from a ROM we should still store it in the >> environment variable. > The new patch series does that, as the net core layer does that. > > What happens is (note code is mangled and might not be 100% accurate, i reduced it the bares): > > eth_read_eeprom_hwaddr(dev); > first read from the eeprom, which may return all zero's if it is unconfigured/missconfigured or should not be used from the eeprom. > if (is_zero_ethaddr(pdata->enetaddr)) > if (eth_get_ops(dev)->read_rom_hwaddr) > eth_get_ops(dev)->read_rom_hwaddr(dev); > if the eeprom failed to produce a mac, we check the read_rom_hwaddr callback, which hooks into the driver. The driver can be overridden by a board (such as sunxi) where the MAC is generated from the SID. > > so at this point we may have a hwaddress actually obtained from the hardware, via the eeprom (or some fixed rom even) or from the hardware itself > next we allow 'software' overrides. e.g. u-boot env (and i think this is where the fdt method should live as well > > eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr); > if (!is_zero_ethaddr(env_enetaddr)) { > if (!is_zero_ethaddr(pdata->enetaddr) && > memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) > memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN); > > // <snip> we compare the HW addr and the ENV addr. if the env is unset, we use whatever the hardware supplied us with. > if the env is set, it overrides the HW addr. > I think next would be to check the fdt to override the env? > > } else if (is_valid_ethaddr(pdata->enetaddr)) { > eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr); > Finally, we set whatever mac has come from the above probing into the environment (if the address is actually a valid MAC). Ok, good I just wanted to make sure that that would still happen. > > } else if (is_zero_ethaddr(pdata->enetaddr)) { > #ifdef CONFIG_NET_RANDOM_ETHADDR > net_random_ethaddr(pdata->enetaddr); > otherwise (if configured) let u-boot generate it. > > > So I think here is where the fdt override should live, as it applies to everyone, not just sunxi. I think you're thinking too much about the case where u-boot has an actual driver for the NIC (and that driver gets initialized, what if it gets skipped to speed up the boot?) and not enough about the case where there is no driver but we still want to use u-boot's board specific MAC generation code to provide a fixed MAC address to the kernel. Regards, Hans
Hey Hans, I guess I have to contradict something here ... On 15-11-16 11:17, Olliver Schinagl wrote: > Hey Hans, > > I was hopeing and expecting this :) > > > As you will be able to tell below, I need to learn a bit more as to why > we do things and discuss this proper I guess. > > > On 15-11-16 10:26, Hans de Goede wrote: >> Hi, >> >> On 15-11-16 04:25, Joe Hershberger wrote: >>> On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl <oliver@schinagl.nl> >>> wrote: >>>> Currently we inject 5 ethernet addresses into the environment, just in >>>> case we may need them. We do this because some boards have no eeprom >>>> (programmed) with a proper ethernet address. With the recent >>>> addition of >>>> reading actual ethernet addresses from the eeprom via the net_op we >>>> should not inject environment variables any more. >>>> >>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>> >>> Acked-by: Joe Hershberger <joe.hershberger@ni.com> >> >> >> Erm, this patch seems wrong to me: NACK, let me >> explain: >> >> 1) It does not do what its commit message says, it only >> removes the second step for setting ethernet addresses >> from the env, but it keeps the existing code to set them >> AFAICT, it only does it once now. >> >> 2) "Currently we inject 5 ethernet addresses into the environment", >> this is not true, we only inject ethernet addresses into the >> environment for devices which have an ethernet alias in dt, >> so maximum 2 for devices with both wired ethernet and wifi > If we want the fdt to do mac things, shouldn't that be done at a higher > level? This is not really board specific is it? >> >> 3) The second attempt at setting ethernet addresses in >> the environment after loading the kernel dt is necessary >> because the kernel dt may be newer and contain more >> ethernet aliases, e.g. the u-boot dt may only contain the >> nodes + alias for the wired, while the (newer) kernel dt >> may also contain a dt-node + alias for the wireless > I agree with you here, but I still don't think this should be board > specific >> >> 4) We cannot solely rely on the ethernet driver to set >> mac-addresses, because the ethernet driver may not be enabled >> while the kernel does have the ethernet driver enabled; and >> the kernel relies on u-boot to generate fixed mac-addresses >> based on the SID independent whether or not u-boot has >> ethernet enabled, this is especially relevant for wifi >> chips where the kernel also relies on u-boot generated >> fixed mac-addresses on e.g. the recent orange-pi boards, >> which come with a realtek rtl8189etv chip which does not >> have a mac address programmed. > I agree, and I'll fix that in my new patch series proper by making > rtl8189etv also read rom hook which IS board specific Of course I didn't realize that the rtl8189etv does not have a u-boot driver, and thus does not get to call its hook and thus nothing sunxi specific will ever be invoked. So I guess in the case of the rtl8189 we have to figure out something (probably near the same as you did) to make it work. It does feel somewhat nasty/hackish of course. I would expect that the linux driver sorts this out for itself and not simply assume u-boot supplies this info on non-existing hardware (to u-boot). I need some time to think this over, so I'm hoping smarter people then me come with great suggestions here :) But for now I'm leaning to think that, the rtl8189 is special. So is this a board specific hack, or a fdt net specific hack. I does feel like the fdt bit I described earlier injects extra mac addresses into the environment for these unknown hardware pieces ... But that will need to come from board specific pieces, as the net stack never gets invoked for these ... I'll stop thinking outloud now and get back to work ;) olliver >> >> 5) AFAIK the dt code for passing mac-addresses to the kernel >> relies on the environment variables, so even if we get the >> mac-address from a ROM we should still store it in the >> environment variable. > The new patch series does that, as the net core layer does that. > > What happens is (note code is mangled and might not be 100% accurate, i > reduced it the bares): > > eth_read_eeprom_hwaddr(dev); > first read from the eeprom, which may return all zero's if it is > unconfigured/missconfigured or should not be used from the eeprom. > if (is_zero_ethaddr(pdata->enetaddr)) > if (eth_get_ops(dev)->read_rom_hwaddr) > eth_get_ops(dev)->read_rom_hwaddr(dev); > if the eeprom failed to produce a mac, we check the read_rom_hwaddr > callback, which hooks into the driver. The driver can be overridden by a > board (such as sunxi) where the MAC is generated from the SID. > > so at this point we may have a hwaddress actually obtained from the > hardware, via the eeprom (or some fixed rom even) or from the hardware > itself > next we allow 'software' overrides. e.g. u-boot env (and i think this is > where the fdt method should live as well > > eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr); > if (!is_zero_ethaddr(env_enetaddr)) { > if (!is_zero_ethaddr(pdata->enetaddr) && > memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) > memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN); > > // <snip> we compare the HW addr and the ENV addr. if the env is unset, > we use whatever the hardware supplied us with. > if the env is set, it overrides the HW addr. > I think next would be to check the fdt to override the env? > > } else if (is_valid_ethaddr(pdata->enetaddr)) { > eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr); > Finally, we set whatever mac has come from the above probing into the > environment (if the address is actually a valid MAC). > > } else if (is_zero_ethaddr(pdata->enetaddr)) { > #ifdef CONFIG_NET_RANDOM_ETHADDR > net_random_ethaddr(pdata->enetaddr); > otherwise (if configured) let u-boot generate it. > > > So I think here is where the fdt override should live, as it applies to > everyone, not just sunxi. > > I'll post my split-up new series for review after testing it, so we can > discuss it in more detail? > > Olliver >> >> Regards, >> >> Hans >> >
Hey, On 15-11-16 11:27, Hans de Goede wrote: > Hi, > > On 15-11-16 11:17, Olliver Schinagl wrote: >> Hey Hans, >> >> I was hopeing and expecting this :) >> >> >> As you will be able to tell below, I need to learn a bit more as to >> why we do things and discuss this proper I guess. >> >> >> On 15-11-16 10:26, Hans de Goede wrote: >>> Hi, >>> >>> On 15-11-16 04:25, Joe Hershberger wrote: >>>> On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl >>>> <oliver@schinagl.nl> wrote: >>>>> Currently we inject 5 ethernet addresses into the environment, >>>>> just in >>>>> case we may need them. We do this because some boards have no eeprom >>>>> (programmed) with a proper ethernet address. With the recent >>>>> addition of >>>>> reading actual ethernet addresses from the eeprom via the net_op we >>>>> should not inject environment variables any more. >>>>> >>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>>> >>>> Acked-by: Joe Hershberger <joe.hershberger@ni.com> >>> >>> >>> Erm, this patch seems wrong to me: NACK, let me >>> explain: >>> >>> 1) It does not do what its commit message says, it only >>> removes the second step for setting ethernet addresses >>> from the env, but it keeps the existing code to set them >>> AFAICT, it only does it once now. >>> >>> 2) "Currently we inject 5 ethernet addresses into the environment", >>> this is not true, we only inject ethernet addresses into the >>> environment for devices which have an ethernet alias in dt, >>> so maximum 2 for devices with both wired ethernet and wifi >> If we want the fdt to do mac things, shouldn't that be done at a >> higher level? This is not really board specific is it? > > We want to put mac addresses into the fdt, and this is done at > a higher level, by existing dt code, which looks at ethernet > aliases in dt and then for any which are present, checks > the corresponding ethaddr env setting and if set, injects > that mac address into the dt-node the alias points to. > > What is sunxi specific is setting the environment variables > based on the SID. The sunxi specific code also checks the > aliases, exactly to avoid the "inject 5 ethernet addresses" > thing you are describing, as we don't want to inject > ethernet addresses for non existent NICs as that may confuse > the user. > >>> 3) The second attempt at setting ethernet addresses in >>> the environment after loading the kernel dt is necessary >>> because the kernel dt may be newer and contain more >>> ethernet aliases, e.g. the u-boot dt may only contain the >>> nodes + alias for the wired, while the (newer) kernel dt >>> may also contain a dt-node + alias for the wireless >> I agree with you here, but I still don't think this should be board >> specific > > Instead of doing this through the environment I guess you > could have the u-boot dt code which is injecting the MACs > into the dt call some board specific callback when there > is no MAC set in the environment, and implement a weak > stub for this. Then all the sunxi environment mangling > code can go away, and sunxi can simply: > 1) Try eeprom if present > 2) Do the current SID based thing > >>> 4) We cannot solely rely on the ethernet driver to set >>> mac-addresses, because the ethernet driver may not be enabled >>> while the kernel does have the ethernet driver enabled; and >>> the kernel relies on u-boot to generate fixed mac-addresses >>> based on the SID independent whether or not u-boot has >>> ethernet enabled, this is especially relevant for wifi >>> chips where the kernel also relies on u-boot generated >>> fixed mac-addresses on e.g. the recent orange-pi boards, >>> which come with a realtek rtl8189etv chip which does not >>> have a mac address programmed. >> I agree, and I'll fix that in my new patch series proper by making >> rtl8189etv also read rom hook which IS board specific > > The problem is that u-boot may not have a driver for one > of the NICs at all, so no place to call the rom hook at all. > > Anyways I believe this is solved by my suggestion for making > the u-boot dt code which injects the MAC call a board specific > callback when no ethaddr is set in the env. > >>> 5) AFAIK the dt code for passing mac-addresses to the kernel >>> relies on the environment variables, so even if we get the >>> mac-address from a ROM we should still store it in the >>> environment variable. >> The new patch series does that, as the net core layer does that. >> >> What happens is (note code is mangled and might not be 100% accurate, >> i reduced it the bares): >> >> eth_read_eeprom_hwaddr(dev); >> first read from the eeprom, which may return all zero's if it is >> unconfigured/missconfigured or should not be used from the eeprom. >> if (is_zero_ethaddr(pdata->enetaddr)) >> if (eth_get_ops(dev)->read_rom_hwaddr) >> eth_get_ops(dev)->read_rom_hwaddr(dev); >> if the eeprom failed to produce a mac, we check the read_rom_hwaddr >> callback, which hooks into the driver. The driver can be overridden >> by a board (such as sunxi) where the MAC is generated from the SID. >> >> so at this point we may have a hwaddress actually obtained from the >> hardware, via the eeprom (or some fixed rom even) or from the >> hardware itself >> next we allow 'software' overrides. e.g. u-boot env (and i think this >> is where the fdt method should live as well >> >> eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr); >> if (!is_zero_ethaddr(env_enetaddr)) { >> if (!is_zero_ethaddr(pdata->enetaddr) && >> memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) >> memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN); >> >> // <snip> we compare the HW addr and the ENV addr. if the env is >> unset, we use whatever the hardware supplied us with. >> if the env is set, it overrides the HW addr. >> I think next would be to check the fdt to override the env? >> >> } else if (is_valid_ethaddr(pdata->enetaddr)) { >> eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr); >> Finally, we set whatever mac has come from the above probing into the >> environment (if the address is actually a valid MAC). > > Ok, good I just wanted to make sure that that would still happen. > > >> >> } else if (is_zero_ethaddr(pdata->enetaddr)) { >> #ifdef CONFIG_NET_RANDOM_ETHADDR >> net_random_ethaddr(pdata->enetaddr); >> otherwise (if configured) let u-boot generate it. >> >> >> So I think here is where the fdt override should live, as it applies >> to everyone, not just sunxi. > > I think you're thinking too much about the case where u-boot > has an actual driver for the NIC (and that driver gets > initialized, what if it gets skipped to speed up the boot?) > and not enough about the case where there is no driver but > we still want to use u-boot's board specific MAC generation code > to provide a fixed MAC address to the kernel. In my other e-mail contradiciting myself I just found this somewhat out, and that indeed is another usecase worth thinking about yeah. So I'll need to re-think that part too. And then thinks come to mind 'if there are 5 address in the eeprom, but only 2 drivers, do the drivers get the first two?' ... I guess there needs to be a general agreement on strange cases as such. How are non-driver devices handled. The dts obviously is one method, but I'm sure board manufactures will hate us for forcing board specific dts. They want to just produce boards en-masse and may be kind enough to supply eeprom or MAC-prom's with fixed mac addresses stored there. I think this is an architectural based decision which deserves its own thread? Olliver > > Regards, > > Hans
Hi, On 15-11-16 11:29, Olliver Schinagl wrote: > Hey Hans, > > I guess I have to contradict something here ... > > On 15-11-16 11:17, Olliver Schinagl wrote: >> Hey Hans, >> >> I was hopeing and expecting this :) >> >> >> As you will be able to tell below, I need to learn a bit more as to why >> we do things and discuss this proper I guess. >> >> >> On 15-11-16 10:26, Hans de Goede wrote: >>> Hi, >>> >>> On 15-11-16 04:25, Joe Hershberger wrote: >>>> On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl <oliver@schinagl.nl> >>>> wrote: >>>>> Currently we inject 5 ethernet addresses into the environment, just in >>>>> case we may need them. We do this because some boards have no eeprom >>>>> (programmed) with a proper ethernet address. With the recent >>>>> addition of >>>>> reading actual ethernet addresses from the eeprom via the net_op we >>>>> should not inject environment variables any more. >>>>> >>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>>> >>>> Acked-by: Joe Hershberger <joe.hershberger@ni.com> >>> >>> >>> Erm, this patch seems wrong to me: NACK, let me >>> explain: >>> >>> 1) It does not do what its commit message says, it only >>> removes the second step for setting ethernet addresses >>> from the env, but it keeps the existing code to set them >>> AFAICT, it only does it once now. >>> >>> 2) "Currently we inject 5 ethernet addresses into the environment", >>> this is not true, we only inject ethernet addresses into the >>> environment for devices which have an ethernet alias in dt, >>> so maximum 2 for devices with both wired ethernet and wifi >> If we want the fdt to do mac things, shouldn't that be done at a higher >> level? This is not really board specific is it? >>> >>> 3) The second attempt at setting ethernet addresses in >>> the environment after loading the kernel dt is necessary >>> because the kernel dt may be newer and contain more >>> ethernet aliases, e.g. the u-boot dt may only contain the >>> nodes + alias for the wired, while the (newer) kernel dt >>> may also contain a dt-node + alias for the wireless >> I agree with you here, but I still don't think this should be board >> specific >>> >>> 4) We cannot solely rely on the ethernet driver to set >>> mac-addresses, because the ethernet driver may not be enabled >>> while the kernel does have the ethernet driver enabled; and >>> the kernel relies on u-boot to generate fixed mac-addresses >>> based on the SID independent whether or not u-boot has >>> ethernet enabled, this is especially relevant for wifi >>> chips where the kernel also relies on u-boot generated >>> fixed mac-addresses on e.g. the recent orange-pi boards, >>> which come with a realtek rtl8189etv chip which does not >>> have a mac address programmed. >> I agree, and I'll fix that in my new patch series proper by making >> rtl8189etv also read rom hook which IS board specific > > Of course I didn't realize that the rtl8189etv does not have a u-boot driver, and thus does not get to call its hook and thus nothing sunxi specific will ever be invoked. > > So I guess in the case of the rtl8189 we have to figure out something (probably near the same as you did) to make it work. > > It does feel somewhat nasty/hackish of course. I would expect that the linux driver sorts this out for itself and not simply assume u-boot supplies this info on non-existing hardware (to u-boot). We've the same with e.g. the wobo-i5 which has its emac routed to an other set of pins then which are usually used which u-boot does not support. Yet the kernel emac driver relies on u-boot to set a MAC, or it will fall back to a random-MAC (which is undesirable). Likewise if someone configures u-boot without network support to make it boot faster, we get the same problem with the emac / gmac driver. The logic here really goes like this: 1) When u-boot does have network support, and picks a MAC-address that MAC-address should be propagated to the kernel so that it uses the same MAC (this is esp. important with switches which allow only 1 MAC per port as a security setting). 2) 1. means that u-boot has some algorithm to pick a fixed MAC in a SoC specific manner. Since we do not want booting with an u-boot with networking enabled resulting in a different fixed MAC then booting on a u-boot without networking support, this means that this algorithm should be used even when networking support in u-boot is disabled (e.g. the wobo-i5 case). 3) Given 2. it makes sense to simply have u-boot generate MACs for all NICs in the system. > I need some time to think this over, so I'm hoping smarter people then me come with great suggestions here :) I still think that my suggestion for having fdt_fixup_ethernet() from common/fdt_support.c call a board specific function instead of the "continue" here: tmp = getenv(mac); if (!tmp) continue; for (j = 0; j < 6; j++) { mac_addr[j] = tmp ? simple_strtoul(tmp, &end, 16) : 0; if (tmp) tmp = (*end) ? end + 1 : end; } E.g: tmp = getenv(mac); if (!tmp) { if (board_get_ethaddr(i, mac_addr) != 0) continue; } else { for (j = 0; j < 6; j++) { mac_addr[j] = tmp ? simple_strtoul(tmp, &end, 16) : 0; if (tmp) tmp = (*end) ? end + 1 : end; } } Would be a good idea, with a weak default for int board_get_ethaddr(int index, unsigned char *mac_addr); Which simply returns -EINVAL; This will neatly solve all the problems discussed, you could probably even use the same board_get_ethaddr() in both the generic ethernet setup code you've been working in, as well as in fdt_fixup_ethernet() Regards, Hans > > But for now I'm leaning to think that, the rtl8189 is special. > > So is this a board specific hack, or a fdt net specific hack. I does feel like the fdt bit I described earlier injects extra mac addresses into the environment for these unknown hardware pieces ... But that will need to come from board specific pieces, as the net stack never gets invoked for these ... > > I'll stop thinking outloud now and get back to work ;) > > olliver > >>> >>> 5) AFAIK the dt code for passing mac-addresses to the kernel >>> relies on the environment variables, so even if we get the >>> mac-address from a ROM we should still store it in the >>> environment variable. >> The new patch series does that, as the net core layer does that. >> >> What happens is (note code is mangled and might not be 100% accurate, i >> reduced it the bares): >> >> eth_read_eeprom_hwaddr(dev); >> first read from the eeprom, which may return all zero's if it is >> unconfigured/missconfigured or should not be used from the eeprom. >> if (is_zero_ethaddr(pdata->enetaddr)) >> if (eth_get_ops(dev)->read_rom_hwaddr) >> eth_get_ops(dev)->read_rom_hwaddr(dev); >> if the eeprom failed to produce a mac, we check the read_rom_hwaddr >> callback, which hooks into the driver. The driver can be overridden by a >> board (such as sunxi) where the MAC is generated from the SID. >> >> so at this point we may have a hwaddress actually obtained from the >> hardware, via the eeprom (or some fixed rom even) or from the hardware >> itself >> next we allow 'software' overrides. e.g. u-boot env (and i think this is >> where the fdt method should live as well >> >> eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr); >> if (!is_zero_ethaddr(env_enetaddr)) { >> if (!is_zero_ethaddr(pdata->enetaddr) && >> memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) >> memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN); >> >> // <snip> we compare the HW addr and the ENV addr. if the env is unset, >> we use whatever the hardware supplied us with. >> if the env is set, it overrides the HW addr. >> I think next would be to check the fdt to override the env? >> >> } else if (is_valid_ethaddr(pdata->enetaddr)) { >> eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr); >> Finally, we set whatever mac has come from the above probing into the >> environment (if the address is actually a valid MAC). >> >> } else if (is_zero_ethaddr(pdata->enetaddr)) { >> #ifdef CONFIG_NET_RANDOM_ETHADDR >> net_random_ethaddr(pdata->enetaddr); >> otherwise (if configured) let u-boot generate it. >> >> >> So I think here is where the fdt override should live, as it applies to >> everyone, not just sunxi. >> >> I'll post my split-up new series for review after testing it, so we can >> discuss it in more detail? >> >> Olliver >>> >>> Regards, >>> >>> Hans >>> >> >
Hey Hans, On 15-11-16 11:49, Hans de Goede wrote: > Hi, > > On 15-11-16 11:29, Olliver Schinagl wrote: >> Hey Hans, >> >> I guess I have to contradict something here ... >> >> On 15-11-16 11:17, Olliver Schinagl wrote: >>> Hey Hans, >>> >>> I was hopeing and expecting this :) >>> >>> >>> As you will be able to tell below, I need to learn a bit more as to why >>> we do things and discuss this proper I guess. >>> >>> >>> On 15-11-16 10:26, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 15-11-16 04:25, Joe Hershberger wrote: >>>>> On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl <oliver@schinagl.nl> >>>>> wrote: >>>>>> Currently we inject 5 ethernet addresses into the environment, >>>>>> just in >>>>>> case we may need them. We do this because some boards have no eeprom >>>>>> (programmed) with a proper ethernet address. With the recent >>>>>> addition of >>>>>> reading actual ethernet addresses from the eeprom via the net_op we >>>>>> should not inject environment variables any more. >>>>>> >>>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >>>>> >>>>> Acked-by: Joe Hershberger <joe.hershberger@ni.com> >>>> >>>> >>>> Erm, this patch seems wrong to me: NACK, let me >>>> explain: >>>> >>>> 1) It does not do what its commit message says, it only >>>> removes the second step for setting ethernet addresses >>>> from the env, but it keeps the existing code to set them >>>> AFAICT, it only does it once now. >>>> >>>> 2) "Currently we inject 5 ethernet addresses into the environment", >>>> this is not true, we only inject ethernet addresses into the >>>> environment for devices which have an ethernet alias in dt, >>>> so maximum 2 for devices with both wired ethernet and wifi >>> If we want the fdt to do mac things, shouldn't that be done at a higher >>> level? This is not really board specific is it? >>>> >>>> 3) The second attempt at setting ethernet addresses in >>>> the environment after loading the kernel dt is necessary >>>> because the kernel dt may be newer and contain more >>>> ethernet aliases, e.g. the u-boot dt may only contain the >>>> nodes + alias for the wired, while the (newer) kernel dt >>>> may also contain a dt-node + alias for the wireless >>> I agree with you here, but I still don't think this should be board >>> specific >>>> >>>> 4) We cannot solely rely on the ethernet driver to set >>>> mac-addresses, because the ethernet driver may not be enabled >>>> while the kernel does have the ethernet driver enabled; and >>>> the kernel relies on u-boot to generate fixed mac-addresses >>>> based on the SID independent whether or not u-boot has >>>> ethernet enabled, this is especially relevant for wifi >>>> chips where the kernel also relies on u-boot generated >>>> fixed mac-addresses on e.g. the recent orange-pi boards, >>>> which come with a realtek rtl8189etv chip which does not >>>> have a mac address programmed. >>> I agree, and I'll fix that in my new patch series proper by making >>> rtl8189etv also read rom hook which IS board specific >> >> Of course I didn't realize that the rtl8189etv does not have a u-boot >> driver, and thus does not get to call its hook and thus nothing sunxi >> specific will ever be invoked. >> >> So I guess in the case of the rtl8189 we have to figure out something >> (probably near the same as you did) to make it work. >> >> It does feel somewhat nasty/hackish of course. I would expect that the >> linux driver sorts this out for itself and not simply assume u-boot >> supplies this info on non-existing hardware (to u-boot). > > We've the same with e.g. the wobo-i5 which has its emac routed to > an other set of pins then which are usually used which u-boot does > not support. Yet the kernel emac driver relies on u-boot to set a > MAC, or it will fall back to a random-MAC (which is undesirable). > > Likewise if someone configures u-boot without network support > to make it boot faster, we get the same problem with the emac / > gmac driver. > > The logic here really goes like this: > > 1) When u-boot does have network support, and picks a MAC-address > that MAC-address should be propagated to the kernel so that it > uses the same MAC (this is esp. important with switches which > allow only 1 MAC per port as a security setting). > > 2) 1. means that u-boot has some algorithm to pick a fixed MAC > in a SoC specific manner. Since we do not want booting with an > u-boot with networking enabled resulting in a different fixed > MAC then booting on a u-boot without networking support, this > means that this algorithm should be used even when networking > support in u-boot is disabled (e.g. the wobo-i5 case). > > 3) Given 2. it makes sense to simply have u-boot generate > MACs for all NICs in the system. I have to digest all you said so far, but 3) makes sense, expect for the 'generate' part :) But I think we mean the same. From a manufacturing PoV. we have 2 problems. A product with ethernet hardware needs to use registered mac addresses. So you have fixed mac addresses you can use for your product. This needs to be facilitated in some manner. The MAC needs to be available to the board uniformly, regardless of the used bootloader or OS. E.g. lets say raspberry pi 3 (without going into actual factual details), which can boot windows or linux, both OSes need to get the same MAC address from the board somehow. It is not uncommon to have an EEPROM from this configuration data (even NIC's have eeproms for this). In any case, a manufacturer of boards wants to program a unique MAC into every board, without having to modify each env/fdt. The cases you describe are all valid, but or really for hobbist use in small scales. They are likley also not handing out valid MAC addresses, but use the 'for internal use' range. Of course we want to facilitate both. So I do agree that u-boot should be responsible for setting the mac of all devices (configured or unconfigured), or 'generate' if you will. The problem however is 'what are all NIC's in the system'. And again, if there are MAC's in the eeprom, which device gets what eeprom. Normally, u-boot probes the hardware, based on that you get a fixed order of eth devices and that fixed order decides which MAC to use. Unless we make the FDT leading here, so that the FDT decides on the probing order. You may have noticed however, I am in somewhat unfamiliary territory here as to what does u-boot do now, how does the fdt fit in here. > >> I need some time to think this over, so I'm hoping smarter people then >> me come with great suggestions here :) > > I still think that my suggestion for having fdt_fixup_ethernet() > from common/fdt_support.c call a board specific function instead > of the "continue" here: > > tmp = getenv(mac); > if (!tmp) > continue; > > for (j = 0; j < 6; j++) { > mac_addr[j] = tmp ? > simple_strtoul(tmp, &end, > 16) : 0; > if (tmp) > tmp = (*end) ? end + 1 : end; > } > > > E.g: > > tmp = getenv(mac); > if (!tmp) { > if (board_get_ethaddr(i, mac_addr) != 0) > continue; > } else { > for (j = 0; j < 6; j++) { > mac_addr[j] = tmp ? > simple_strtoul(tmp, > &end, 16) : 0; > if (tmp) > tmp = (*end) ? end + 1 : end; > } > } > > Would be a good idea, with a weak default for > > int board_get_ethaddr(int index, unsigned char *mac_addr); > > Which simply returns -EINVAL; > > > This will neatly solve all the problems discussed, you > could probably even use the same board_get_ethaddr() > in both the generic ethernet setup code you've been > working in, as well as in fdt_fixup_ethernet() Let me digest this all some more and mull it over. But to summarize, ignoring the ordering of devices for now (which runs havoc in the 'unconfigured' case to speed up booting), check eeprom for mac if not, call device read rom hook which may be overriden by the board set to env let env override hw generated code check fdt and let the fdt override the env if fdt has more ethernet devices, add those to the env as well. Ideally this is where re-ording would take place to match what the fdt says, but we don't know of course what eth0, eth1 and eth2 are bound too... but again, i need to mull and look at the fdt_support bit. olliver > > Regards, > > Hans > > > > > >> >> But for now I'm leaning to think that, the rtl8189 is special. >> >> So is this a board specific hack, or a fdt net specific hack. I does >> feel like the fdt bit I described earlier injects extra mac addresses >> into the environment for these unknown hardware pieces ... But that >> will need to come from board specific pieces, as the net stack never >> gets invoked for these ... >> >> I'll stop thinking outloud now and get back to work ;) >> >> olliver >> >>>> >>>> 5) AFAIK the dt code for passing mac-addresses to the kernel >>>> relies on the environment variables, so even if we get the >>>> mac-address from a ROM we should still store it in the >>>> environment variable. >>> The new patch series does that, as the net core layer does that. >>> >>> What happens is (note code is mangled and might not be 100% accurate, i >>> reduced it the bares): >>> >>> eth_read_eeprom_hwaddr(dev); >>> first read from the eeprom, which may return all zero's if it is >>> unconfigured/missconfigured or should not be used from the eeprom. >>> if (is_zero_ethaddr(pdata->enetaddr)) >>> if (eth_get_ops(dev)->read_rom_hwaddr) >>> eth_get_ops(dev)->read_rom_hwaddr(dev); >>> if the eeprom failed to produce a mac, we check the read_rom_hwaddr >>> callback, which hooks into the driver. The driver can be overridden by a >>> board (such as sunxi) where the MAC is generated from the SID. >>> >>> so at this point we may have a hwaddress actually obtained from the >>> hardware, via the eeprom (or some fixed rom even) or from the hardware >>> itself >>> next we allow 'software' overrides. e.g. u-boot env (and i think this is >>> where the fdt method should live as well >>> >>> eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr); >>> if (!is_zero_ethaddr(env_enetaddr)) { >>> if (!is_zero_ethaddr(pdata->enetaddr) && >>> memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) >>> memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN); >>> >>> // <snip> we compare the HW addr and the ENV addr. if the env is unset, >>> we use whatever the hardware supplied us with. >>> if the env is set, it overrides the HW addr. >>> I think next would be to check the fdt to override the env? >>> >>> } else if (is_valid_ethaddr(pdata->enetaddr)) { >>> eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr); >>> Finally, we set whatever mac has come from the above probing into the >>> environment (if the address is actually a valid MAC). >>> >>> } else if (is_zero_ethaddr(pdata->enetaddr)) { >>> #ifdef CONFIG_NET_RANDOM_ETHADDR >>> net_random_ethaddr(pdata->enetaddr); >>> otherwise (if configured) let u-boot generate it. >>> >>> >>> So I think here is where the fdt override should live, as it applies to >>> everyone, not just sunxi. >>> >>> I'll post my split-up new series for review after testing it, so we can >>> discuss it in more detail? >>> >>> Olliver >>>> >>>> Regards, >>>> >>>> Hans >>>> >>> >>
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 9b56a89..71124f4 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -688,15 +688,19 @@ static void parse_spl_header(const uint32_t spl_addr) setenv_hex("fel_scriptaddr", spl->fel_script_address); } -/* - * Note this function gets called multiple times. - * It must not make any changes to env variables which already exist. - */ -static void setup_environment(const void *fdt) +int misc_init_r(void) { char serial_string[17] = { 0 }; unsigned int sid[4]; - int ret; + __maybe_unused int ret; + + setenv("fel_booted", NULL); + setenv("fel_scriptaddr", NULL); + /* determine if we are running in FEL mode */ + if (!is_boot0_magic(SPL_ADDR + 4)) { /* eGON.BT0 */ + setenv("fel_booted", "1"); + parse_spl_header(SPL_ADDR); + } ret = sunxi_get_sid(sid); if (ret == 0 && sid[0] != 0) { @@ -717,28 +721,11 @@ static void setup_environment(const void *fdt) sid[3] = crc32(0, (unsigned char *)&sid[1], 12); #endif - if (!getenv("serial#")) { - snprintf(serial_string, sizeof(serial_string), - "%08x%08x", sid[0], sid[3]); + snprintf(serial_string, sizeof(serial_string), + "%08x%08x", sid[0], sid[3]); - setenv("serial#", serial_string); - } + setenv("serial#", serial_string); } -} - -int misc_init_r(void) -{ - __maybe_unused int ret; - - setenv("fel_booted", NULL); - setenv("fel_scriptaddr", NULL); - /* determine if we are running in FEL mode */ - if (!is_boot0_magic(SPL_ADDR + 4)) { /* eGON.BT0 */ - setenv("fel_booted", "1"); - parse_spl_header(SPL_ADDR); - } - - setup_environment(gd->fdt_blob); #ifndef CONFIG_MACH_SUN9I ret = sunxi_usb_phy_probe(); @@ -754,12 +741,6 @@ int ft_board_setup(void *blob, bd_t *bd) { int __maybe_unused r; - /* - * Call setup_environment again in case the boot fdt has - * ethernet aliases the u-boot copy does not have. - */ - setup_environment(blob); - #ifdef CONFIG_VIDEO_DT_SIMPLEFB r = sunxi_simplefb_setup(blob); if (r)
Currently we inject 5 ethernet addresses into the environment, just in case we may need them. We do this because some boards have no eeprom (programmed) with a proper ethernet address. With the recent addition of reading actual ethernet addresses from the eeprom via the net_op we should not inject environment variables any more. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> --- board/sunxi/board.c | 45 +++++++++++++-------------------------------- 1 file changed, 13 insertions(+), 32 deletions(-)