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 <oli...@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 <oli...@schinagl.nl>
Acked-by: Joe Hershberger <joe.hershber...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
