RE: [PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux
>-Original Message- >From: U-Boot On Behalf Of Alex Marginean >Sent: Tuesday, December 10, 2019 8:26 PM >To: u-boot@lists.denx.de >Cc: Joe Hershberger ; Claudiu Manoil >; Vladimir Oltean >Subject: [PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to >Linux > >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. > >Signed-off-by: Alex Marginean >--- Patch applied (after removing extra spaces in description) in u-boot-fsl-qoriq/master -priyankajain
Re: [PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux
On 12/12/2019 1:12 PM, Michael Walle wrote:
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 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 = <0x00 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.
Between ethaddr from eeprom and e1000 ROM address? Yes, it will complain.
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
Re: [PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux
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
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 = <0x00 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 th
Re: [PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux
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 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 = <0x00 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.
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.
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 |-- pc
Re: [PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux
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 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 = <0x00 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.
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).
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@1f000
eth 0 [ + ] enetc_eth | |-- enetc-0
mdio 0 [ + ] enetc_mdio| |-- emdio-3
pci_generi0 [ ] pci_generic_drv | |-- pci_4:0.4
dsa 0 [ ] felix-switch | |-- felix-switch
pci_generi1 [ ] pci_generic_drv | `-- pci_4:1f.0
[snip]
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 cod
Re: [PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux
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 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. 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. 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. Alex
Re: [PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux
Am 2019-12-11 14:04, schrieb Alexandru Marginean: On 12/10/2019 11:47 PM, Michael Walle 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. By proprietary do you mean IERB? I meant the fdt fixups for mac-address/local-mac-address which is not handled in the generic u-boot code, see my previous mail. Because if it would, then there would be no need for the board specific code below (just the one function call actually). Network cards normally have a ROM which comes preset from the factory with default MAC addresses. At run-time drivers can use the default address or replace it. The MAC address in IERB is the default MAC address for ENETC interfaces at run-time, this address is available to the driver/stack after issuing an FLR and in the absence of a DT node for the PCI function. We can pass MAC addresses to Linux through DT alone, but that's more of a hassle if Linux decides to assign the PCI function to a guest or user-space app. Using the IERB values doesn't require any fix-up for guest DTs. Understood. IERB is not actually a ROM though and we need U-Boot to set factory MAC addresses into IERB some time before Linux boots. Also, shouldn't write_hwaddr callback be implemented instead of the enetc_set_ierb_primary_mac()? OK, makes sense, I will move the code there. -michael
Re: [PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux
Hi Vladimir, Am 2019-12-11 13:46, schrieb Vladimir Oltean: Hi Michael, On Wed, 11 Dec 2019 at 00:48, Michael Walle 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(). 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
Re: [PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux
On 12/10/2019 11:47 PM, Michael Walle 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.
By proprietary do you mean IERB?
Network cards normally have a ROM which comes preset from the factory
with default MAC addresses. At run-time drivers can use the default
address or replace it. The MAC address in IERB is the default MAC
address for ENETC interfaces at run-time, this address is available to
the driver/stack after issuing an FLR and in the absence of a DT node
for the PCI function.
We can pass MAC addresses to Linux through DT alone, but that's more of
a hassle if Linux decides to assign the PCI function to a guest or
user-space app. Using the IERB values doesn't require any fix-up for
guest DTs.
IERB is not actually a ROM though and we need U-Boot to set factory MAC
addresses into IERB some time before Linux boots.
Also, shouldn't write_hwaddr callback be implemented instead of the
enetc_set_ierb_primary_mac()?
OK, makes sense, I will move the code there.
I'll send a v3,
Thanks!
Alex
-michael
Signed-off-by: Alex Marginean
---
The code is called fom ft_board_setup in
board/freescale/ls1028a/ls1028a.c
mostly for consistency with other LS parts. I'm open to suggestions
though.
Changes in v2:
- fixed warning for fdt_fixup_enetc_mac being implicitly declared
board/freescale/ls1028a/ls1028a.c | 5 +++
drivers/net/fsl_enetc.c | 65 ++-
drivers/net/fsl_enetc.h | 3 ++
3 files changed, 72 insertions(+), 1 deletion(-)
diff --git a/board/freescale/ls1028a/ls1028a.c
b/board/freescale/ls1028a/ls1028a.c
index a9606b8865..1a82c95402 100644
--- a/board/freescale/ls1028a/ls1028a.c
+++ b/board/freescale/ls1028a/ls1028a.c
@@ -25,6 +25,7 @@
#include
#include
#include "../common/qixis.h"
+#include "../drivers/net/fsl_enetc.h"
DECLARE_GLOBAL_DATA_PTR;
@@ -150,6 +151,10 @@ int ft_board_setup(void *blob, bd_t *bd)
fdt_fixup_icid(blob);
+#ifdef CONFIG_FSL_ENETC
+ fdt_fixup_enetc_mac(blob);
+#endif
+
return 0;
}
#endif
diff --git a/drivers/net/fsl_enetc.c b/drivers/net/fsl_enetc.c
index 0ca7e838a8..3c043888db 100644
--- a/drivers/net/fsl_enetc.c
+++ b/drivers/net/fsl_enetc.c
@@ -14,6 +14,69 @@
#include "fsl_enetc.h"
+#define ENETC_DRIVER_NAME "enetc_eth"
+
+/*
+ * sets the MAC address in IERB registers, this setting is persistent
and
+ * carried over to Linux.
+ */
+static void enetc_set_ierb_primary_mac(struct udevice *dev, int devfn,
+ const u8 *enetaddr)
+{
+#ifdef CONFIG_ARCH_LS1028A
+/*
+ * LS1028A is the only part with IERB at this time and there are
plans to change
+ * its structure, keep this LS1028A specific for now
+ */
+#define IERB_BASE 0x1f080ULL
+#define IERB_PFMAC(pf, vf, n) (IERB_BASE + 0x8000 + (pf) * 0x100 +
(vf) * 8 \
+ + (n) * 4)
+
+static int ierb_fn_to_pf[] = {0, 1, 2, -1, -1, -1, 3};
wrong indendation
+
+ u16 lower = *(const u16 *)(enetaddr + 4);
+ u32 upper = *(const u32 *)enetaddr;
+
+ if (ierb_fn_to_pf[devfn] < 0)
+ return;
it would be easier to read if ierb_fn_to_pf[devfn] would be assigned to
a local variable.
+
+ out_le32(IERB_PFMAC(ierb_fn_to_pf[devfn], 0, 0), upper);
+ out_le32(IERB_PFMAC(ierb_fn_to_pf[devfn], 0, 1), (u32)lower);
+#endif
+}
+
+/* sets up primary MAC addresses in DT/IERB */
+void fdt_fixup_enetc_mac(void *blob)
+{
+ struct pci_child_platdata *ppdata;
+ struct eth_pdata *pdata;
+ struct udevice *dev;
+ struct uclass *uc;
+ char path[256];
+ int offset;
+ int devfn;
+
+ uclass_get(UCLASS_ETH, &uc);
+ uclass_foreach_dev(dev, uc) {
+ if (!dev->driver || !dev->driver->name ||
+ strcmp(dev->driver->name, ENETC_DRIVER_NAME))
+ continue;
+
+ pdata = dev_get_platdata(dev);
+ ppdata = dev_get_parent_platdata(dev);
+ devfn = PCI_FUNC(ppdata->devfn);
+
+ enetc_set_ierb_primary_mac(dev, devfn, pdata->enetaddr);
+
+ snprintf(path, 256, "/soc/pcie@1f000/ethernet@%x,%x",
+ PCI_DEV(ppdata->devfn), PCI_FUNC(ppdata->devfn));
+ offset = fdt_path_offset(blob, path);
+ if (offset < 0)
+ continue;
+ fdt_setprop(blob, offset, "mac-address", pdata->enetaddr, 6);
+ }
+}
+
/*
* Bind the device:
* - set a more explicit name on the interface
@@ -583,7 +646,7 @@ static const struct eth_ops enetc_ops = {
};
U_BOOT_DRIVER(eth_enetc) = {
- .name = "enetc_eth",
+ .name = ENETC_DRIVER_NAME,
.id = UCLASS_ETH,
.bind = enetc_bind,
.probe = enetc_probe
Re: [PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux
Hi Michael,
On Wed, 11 Dec 2019 at 00:48, Michael Walle 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.
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.
> -michael
>
> > Signed-off-by: Alex Marginean
> > ---
> >
> > The code is called fom ft_board_setup in
> > board/freescale/ls1028a/ls1028a.c
> > mostly for consistency with other LS parts. I'm open to suggestions
> > though.
> >
> > Changes in v2:
> > - fixed warning for fdt_fixup_enetc_mac being implicitly declared
> >
> > board/freescale/ls1028a/ls1028a.c | 5 +++
> > drivers/net/fsl_enetc.c | 65 ++-
> > drivers/net/fsl_enetc.h | 3 ++
> > 3 files changed, 72 insertions(+), 1 deletion(-)
> >
> > diff --git a/board/freescale/ls1028a/ls1028a.c
> > b/board/freescale/ls1028a/ls1028a.c
> > index a9606b8865..1a82c95402 100644
> > --- a/board/freescale/ls1028a/ls1028a.c
> > +++ b/board/freescale/ls1028a/ls1028a.c
> > @@ -25,6 +25,7 @@
> > #include
> > #include
> > #include "../common/qixis.h"
> > +#include "../drivers/net/fsl_enetc.h"
> >
> > DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -150,6 +151,10 @@ int ft_board_setup(void *blob, bd_t *bd)
> >
> > fdt_fixup_icid(blob);
> >
> > +#ifdef CONFIG_FSL_ENETC
> > + fdt_fixup_enetc_mac(blob);
> > +#endif
> > +
> > return 0;
> > }
> > #endif
> > diff --git a/drivers/net/fsl_enetc.c b/drivers/net/fsl_enetc.c
> > index 0ca7e838a8..3c043888db 100644
> > --- a/drivers/net/fsl_enetc.c
> > +++ b/drivers/net/fsl_enetc.c
> > @@ -14,6 +14,69 @@
> >
> > #include "fsl_enetc.h"
> >
> > +#define ENETC_DRIVER_NAME"enetc_eth"
> > +
> > +/*
> > + * sets the MAC address in IERB registers, this setting is persistent
> > and
> > + * carried over to Linux.
> > + */
> > +static void enetc_set_ierb_primary_mac(struct udevice *dev, int devfn,
> > +const u8 *enetaddr)
> > +{
> > +#ifdef CONFIG_ARCH_LS1028A
> > +/*
> > + * LS1028A is the only part with IERB at this time and there are
> > plans to change
> > + * its structure, keep this LS1028A specific for now
> > + */
> > +#define IERB_BASE0x1f080ULL
> > +#define IERB_PFMAC(pf, vf, n)(IERB_BASE + 0x8000 + (pf) * 0x100 +
> > (vf) * 8 \
> > + + (n) * 4)
> > +
> > +static int ierb_fn_to_pf[] = {0, 1, 2, -1, -1, -1, 3};
> wrong indendation
>
> > +
> > + u16 lower = *(const u16 *)(enetaddr + 4);
> > + u32 upper = *(const u32 *)enetaddr;
> > +
> > + if (ierb_fn_to_pf[devfn] < 0)
> > + return;
> it would be easier to read if ierb_fn_to_pf[devfn] would be assigned to
> a local variable.
>
Alex, after you address this feedback from Michael, you can add my:
Reviewed-by: Vladimir Oltean
> > +
> > + out_le32(IERB_PFMAC(ierb_fn_to_pf[devfn], 0, 0), upper);
> > + out_le32(IERB_PFMAC(ierb_fn_to_pf[devfn], 0, 1), (u32)lower);
> > +#endif
> > +}
> > +
> > +/* sets up primary MAC addresses in DT/IERB */
> > +void fdt_fixup_enetc_mac(void *blob)
> > +{
> > + struct pci_child_platdata *ppdata;
> > + struct eth_pdata *pdata;
> > + struct udevice *dev;
> > + struct uclass *uc;
> > + char path[256];
> > + int offset;
> > + int devfn;
> > +
> > + uclass_get(UCLASS_ETH, &uc);
> > + uclass_foreach_dev(dev, uc) {
> > + if (!dev->driver || !dev->driver->name ||
> > + strcmp(dev->driver->name, ENETC_DRIVER_NAME))
> > + continue;
> > +
> > + pdata = dev_get_platdata(dev);
> > + ppdata = dev_get_parent_platdata(dev);
> > + devfn = PCI_FUNC(ppdata->devfn);
> > +
> > + enetc_set_ierb_primary_mac(dev, devfn, pdata->enetaddr);
> > +
> > + snprintf(path, 256, "/soc/pcie@1f000/ethernet@%x,%x",
> > + PCI_DEV(ppdata->devfn), PCI_FUNC(
Re: [PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux
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()?
-michael
Signed-off-by: Alex Marginean
---
The code is called fom ft_board_setup in
board/freescale/ls1028a/ls1028a.c
mostly for consistency with other LS parts. I'm open to suggestions
though.
Changes in v2:
- fixed warning for fdt_fixup_enetc_mac being implicitly declared
board/freescale/ls1028a/ls1028a.c | 5 +++
drivers/net/fsl_enetc.c | 65 ++-
drivers/net/fsl_enetc.h | 3 ++
3 files changed, 72 insertions(+), 1 deletion(-)
diff --git a/board/freescale/ls1028a/ls1028a.c
b/board/freescale/ls1028a/ls1028a.c
index a9606b8865..1a82c95402 100644
--- a/board/freescale/ls1028a/ls1028a.c
+++ b/board/freescale/ls1028a/ls1028a.c
@@ -25,6 +25,7 @@
#include
#include
#include "../common/qixis.h"
+#include "../drivers/net/fsl_enetc.h"
DECLARE_GLOBAL_DATA_PTR;
@@ -150,6 +151,10 @@ int ft_board_setup(void *blob, bd_t *bd)
fdt_fixup_icid(blob);
+#ifdef CONFIG_FSL_ENETC
+ fdt_fixup_enetc_mac(blob);
+#endif
+
return 0;
}
#endif
diff --git a/drivers/net/fsl_enetc.c b/drivers/net/fsl_enetc.c
index 0ca7e838a8..3c043888db 100644
--- a/drivers/net/fsl_enetc.c
+++ b/drivers/net/fsl_enetc.c
@@ -14,6 +14,69 @@
#include "fsl_enetc.h"
+#define ENETC_DRIVER_NAME "enetc_eth"
+
+/*
+ * sets the MAC address in IERB registers, this setting is persistent
and
+ * carried over to Linux.
+ */
+static void enetc_set_ierb_primary_mac(struct udevice *dev, int devfn,
+ const u8 *enetaddr)
+{
+#ifdef CONFIG_ARCH_LS1028A
+/*
+ * LS1028A is the only part with IERB at this time and there are
plans to change
+ * its structure, keep this LS1028A specific for now
+ */
+#define IERB_BASE 0x1f080ULL
+#define IERB_PFMAC(pf, vf, n) (IERB_BASE + 0x8000 + (pf) * 0x100 +
(vf) * 8 \
++ (n) * 4)
+
+static int ierb_fn_to_pf[] = {0, 1, 2, -1, -1, -1, 3};
wrong indendation
+
+ u16 lower = *(const u16 *)(enetaddr + 4);
+ u32 upper = *(const u32 *)enetaddr;
+
+ if (ierb_fn_to_pf[devfn] < 0)
+ return;
it would be easier to read if ierb_fn_to_pf[devfn] would be assigned to
a local variable.
+
+ out_le32(IERB_PFMAC(ierb_fn_to_pf[devfn], 0, 0), upper);
+ out_le32(IERB_PFMAC(ierb_fn_to_pf[devfn], 0, 1), (u32)lower);
+#endif
+}
+
+/* sets up primary MAC addresses in DT/IERB */
+void fdt_fixup_enetc_mac(void *blob)
+{
+ struct pci_child_platdata *ppdata;
+ struct eth_pdata *pdata;
+ struct udevice *dev;
+ struct uclass *uc;
+ char path[256];
+ int offset;
+ int devfn;
+
+ uclass_get(UCLASS_ETH, &uc);
+ uclass_foreach_dev(dev, uc) {
+ if (!dev->driver || !dev->driver->name ||
+ strcmp(dev->driver->name, ENETC_DRIVER_NAME))
+ continue;
+
+ pdata = dev_get_platdata(dev);
+ ppdata = dev_get_parent_platdata(dev);
+ devfn = PCI_FUNC(ppdata->devfn);
+
+ enetc_set_ierb_primary_mac(dev, devfn, pdata->enetaddr);
+
+ snprintf(path, 256, "/soc/pcie@1f000/ethernet@%x,%x",
+PCI_DEV(ppdata->devfn), PCI_FUNC(ppdata->devfn));
+ offset = fdt_path_offset(blob, path);
+ if (offset < 0)
+ continue;
+ fdt_setprop(blob, offset, "mac-address", pdata->enetaddr, 6);
+ }
+}
+
/*
* Bind the device:
* - set a more explicit name on the interface
@@ -583,7 +646,7 @@ static const struct eth_ops enetc_ops = {
};
U_BOOT_DRIVER(eth_enetc) = {
- .name = "enetc_eth",
+ .name = ENETC_DRIVER_NAME,
.id = UCLASS_ETH,
.bind = enetc_bind,
.probe = enetc_probe,
diff --git a/drivers/net/fsl_enetc.h b/drivers/net/fsl_enetc.h
index 0bb4cdff47..e441891468 100644
--- a/drivers/net/fsl_enetc.h
+++ b/drivers/net/fsl_enetc.h
@@ -226,4 +226,7 @@ int enetc_mdio_read_priv(struct enetc_mdio_priv
*priv, int addr, int devad,
int enetc_mdio_write_priv(struct enetc_mdio_priv *priv, int addr, int
devad,
int reg, u16 val);
+/* sets up primary MAC addresses in DT/IERB */
+void fdt_fixup_enetc_mac(void *blob);
+
#endif /* _ENETC_H */
