Hi Alex,
Am 2019-12-11 22:01, schrieb Alexandru Marginean:
Hi Michael,
On 12/11/2019 6:03 PM, Michael Walle wrote:
Hi Alex,
Am 2019-12-11 16:37, schrieb Alexandru Marginean:
On 12/11/2019 2:16 PM, Michael Walle wrote:
Hi Vladimir,
Am 2019-12-11 13:46, schrieb Vladimir Oltean:
Hi Michael,
On Wed, 11 Dec 2019 at 00:48, Michael Walle <[email protected]>
wrote:
Am 2019-12-10 15:55, schrieb Alex Marginean:
> Passes on the primary address used by u-boot to Linux. The code does a
> DT
> fix-up for ENETC PFs and sets the primary MAC address in IERB. The
> address
> in IERB is restored on ENETC PCI functions at FLR.
I don't get the reason why this is done in a proprietary way. What
is
the
difference between any other network interface whose hardware
address is
set up in the generic u-boot code.
Also, shouldn't write_hwaddr callback be implemented instead of
the
enetc_set_ierb_primary_mac()?
At the moment, the Linux driver ignored the device tree and only
reads
the POR values of the SIPMAR registers. The reset value of those
comes
from the IERB space, which U-Boot is configuring. So it would be
good
if that behavior keeps working.
It would also be good if the Linux driver called
of_get_mac_address,
so it needs the device tree binding for that. That's why both
fixups
are performed, and why the generic function is not used.
yes, but u-boot already sets the mac-address/local-mac-address
property
in the device tree already in a generic way, see
fdt_fixup_ethernet().
I think fdt_fixup_ethernet is not a good choice for us.
The issue is that it ties Linux DT to device indexes in U-Boot.
That's a problem if we plug an Eth PCI card in, we would need to
change Linux DT, which is definitely not desirable.
We actually do use PCI Eth cards with some of our boards and U-Boot
does pick those up and indexes shift.
are you sure? afaik it works by reading the /alias/ethernetN phandles
which gets ethNaddr assigned if you set the FDT_SEQ_MACADDR_FROM_ENV
config option. I've just tried it, here is my linux dts diff
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
@@ -17,6 +17,11 @@
#address-cells = <2>;
#size-cells = <2>;
+ aliases {
+ ethernet0 = &enetc_port0;
+ ethernet1 = &enetc_port1;
+ };
+
cpus {
#address-cells = <1>;
#size-cells = <0>;
@@ -761,10 +766,12 @@
enetc_port0: ethernet@0,0 {
compatible = "fsl,enetc";
reg = <0x000000 0 0 0 0>;
+ local-mac-address = [ 00 00 00 00 00
00 ];
};
enetc_port1: ethernet@0,1 {
compatible = "fsl,enetc";
reg = <0x000100 0 0 0 0>;
+ local-mac-address = [ 00 00 00 00 00
00 ];
};
enetc_mdio_pf3: mdio@0,3 {
compatible = "fsl,enetc-mdio";
That way the mapping between ethNaddr and the network device can also
be changed by the user by changing the linux device tree.
also, uboot should respect the /aliases/ethernetN, too.
I don't disagree with any of that, but that's not the issue I
mentioned. I meant actual PCI cards being plugged in, I didn't mean
having disabled ECAM functions.
In your example DT enetc port 0 is tied to /aliases/ethernet0 and to
ethaddr, enetc port 1 is tied to /aliases/ethernet1 and to eth1addr.
A LS1028 board with a PCI Eth card plugged in shows this:
Net: e1000: 68:05:ca:66:bf:bd
eth0: e1000#0 [PRIME], eth1: enetc-0, eth2: enetc-2, eth3:
swp0, eth4: swp1, eth5: swp2, eth6: swp3
In this case eth0 is the e1000 card and it uses ethaddr, enetc port 0
is eth1 and uses eth1addr. The fix-up to /aliases/ethernet0 in your
example makes enetc port 0 get the MAC address of the PCI card. If
the PCI card is removed then enetc port 0 ends up being eth0 in U-Boot
and and actually use ethaddr, not eth1addr.
Mh, I've just tried this and it seems that this is even worse. Because
if you set the ethaddr, uboot will complain about mismatching ethernet
addresses.
To make this work with a PCI card plugged in one would need to change
the aliases in Linux DT, which is not a fun thing to do.
BTW what will be the source of the network addresses, the u-boot
environment variables? (which might be set either by the user or some
kind of board specific code).
Yes, the environment variables. As far as I know these come preset
from the factory. The reference boards usually come with stickers
too, listing the preset MAC addresses.
so here is already the first problem, see above. Just to be sure, I
don't argue against having the environment as the source (actually
we do import the mac addresses from our eeprom into the environment),
just to make you aware that there is an underlying problem.
I've tried to use the aliases in u-boot, too. Unfortunately, the
e1000 will get the first slot, regardless that /aliases/ethernet0 is
a phandle to enetc-0. I guess that is the actual problem here. Because
if that would work then everything else would fall into place. (except
the write_hwaddr()).
Also U-Boot and Linux DTs have to be in sync all the time, if we
disable one port in U-Boot but not in Linux we would mix up MAC
addresses.
I see. But that shouldn't happen with the code above. But are you sure
that this
+ uclass_get(UCLASS_ETH, &uc);
+ uclass_foreach_dev(dev, uc) {
will work then? in my config (just enetc-0) there is only one eth
device
# dm tree
[snip]
pci 2 [ + ] pci_generic_ecam |-- pcie@1f0000000
eth 0 [ + ] enetc_eth | |-- enetc-0
mdio 0 [ + ] enetc_mdio | |-- emdio-3
pci_generi 0 [ ] pci_generic_drv | |-- pci_4:0.4
dsa 0 [ ] felix-switch | |-- felix-switch
pci_generi 1 [ ] pci_generic_drv | `-- pci_4:1f.0
[snip]
Yeah, I guess that's no better either way. U-Boot wouldn't be able to
fix-up addresses for interfaces it didn't know about, yes. It works
if Linux doesn't use some interfaces that U-Boot does, the MAC
addresses per port would match between U-Boot and Linux. That case
isn't too useful though, if they differ it's probably the other way
around, and U-Boot has fewer interfaces.
There is no simple solution to satisfy all cases really.
mhh just to let you know some cases we have:
(1) enetc-0, enetc-1 in u-boot
enetc-0, enetc-1, enetc-2 (and switch) in linux
(2) enetc-1 in u-boot
all enetc's and switch in linux
(3a) well the "standard" ones where linux == uboot
(3b) "standard one" but just enetc-1, skipping enetc-0
Using an index encoded in the DT to identify a given interface isn't
OK, in my example there's a PCI card that gets probed first and then
all IDs shift up.
correct, and i guess that is the actual problem here. If it would be
appended to a pre-defined list (aka /aliases in u-boot) then there
wouldn't
be any shift.
Fixing up addresses for interfaces U-Boot doesn't know about is also a
problem, as the fix-up code should stay clear of eth addresses used by
live interfaces, on the assumption that those should continue using
them. Assume the device has two interfaces, first one is disabled in
U-Boot but enabled in Linux. Should it use ethaddr in Linux, or
eth1addr? Ethaddr is associated with 2nd interface in U-Boot and it
makes sense to maintain that association in Linux.
If you say ethNaddr always maps to /alias/ethernetN, you won't have a
problem, will you? Other than ethXaddr might be unused.
Also consider the following:
- you have consecutive mac addresses
- u-boot uses enetc-1, enetc-0 is disabled, it would use ethaddr
lets say (its ..:00)
- linux uses enetc-0 and enetc-1, then if it is not shifted,
enetc-0 will have (..:01) and enetc-1 stays with (..:00).
Although it doesn't impact the function, it still looks bad.
So as far as using generic fix-up code I'm all for it, but in this
case we would need some platform specific rules to match Linux DT
nodes to U-Boot eth addresses.
As for the write_hwaddr callback instead of
enetc_set_primary_mac_addr, that is valid but I suppose it is
outside
the scope of this patch. That function is related to the runtime
MAC
address and not to the MAC passed to Linux.
according to the comment in eth-uclass.c this is not for (u-boot)
runtime:
/*
* Devices need to write the hwaddr even if not started so that
Linux
* will have access to the hwaddr that u-boot stored for the
device.
* This is accomplished by attempting to probe each device and
calling
* their write_hwaddr() operation.
*/
and the runtime mac address for u-boot is already set enetc_start().
-michael
This is fine, I'll move the IERB code to write_hwaddr.
This will also only be called if the device is not disabled in u-boot,
from what I see. So it might be better to "fix" this.
I bet you are not the first/only one which needs this kind of "how do
we
propagate the hardware address to linux" where the devices might be
unused
in u-boot. There must be a better way to do this ;)
-michael
Yeah, I'm sure others bumped into this before.
For sure fdt_fixup_ethernet as defined now can't cover all cases. It
can only rely on the U-Boot interface index which is volatile with
plug-in PCI cards. We could introduce some new index for integrated
interfaces that's set by the driver, maybe improve on existing
req_seq. In current code req_seq is not too useful, as dev->seq is
allocated in order of discovery, so it doesn't help if 2nd found
device requests seq 0, that's already in use. But that could probably
be improved.
For now I think I'll stick to current code, proprietary as it is and
with the limitation that U-Boot has to enable interfaces that Linux
uses.
Please don't take that shortcut because it will be a pain to get it
cleaned-up and retain backwards compatibility :( Also like explained
above, it already falls short if you have ethaddr defined.
I am open to any better suggestions though.
see above and like you've described it. append any plugable interfaces
at the end of the predefined list (with probably empty slots in used
ethNaddr).
-michael
