[PATCH v2] drivers: net: fsl_enetc: Pass on primary MAC address to Linux

[Michael Walle]

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 <michael at walle.cc> 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 at 0,0 {
                                 compatible = "fsl,enetc";
                                 reg = <0x000000 0 0 0 0>;
+                               local-mac-address = [ 00 00 00 00 00 00 
];
                         };
                         enetc_port1: ethernet at 0,1 {
                                 compatible = "fsl,enetc";
                                 reg = <0x000100 0 0 0 0>;
+                               local-mac-address = [ 00 00 00 00 00 00 
];
                         };
                         enetc_mdio_pf3: mdio at 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 at 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]

> 
> 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