Re: [PATCH v2 05/10] boards: enclustra-sa2: read MAC address from EEPROM

[David Picard <david.picard@clermont.in2p3.fr>]

Hello,

Did you make up your mind, in the light of the manual statement below?

David

Le 26/09/2025 à 14:57, David Picard a écrit :
> The SoM manual states, p46, bottom:
> "The Ethernet MAC address is stored using big-endian byte order (MSB 
> on the lowest address). Each module
> is assigned two sequential MAC addresses; only the lower one is stored 
> in the EEPROM."
> https://filesender.renater.fr/?s=download&token=c553d226-7c90-41e3-8c74-96393f1f1edd 
>
>
> What's the problem with fallback addresses? At least, the board will 
> look like an Enclustra product.
>
>
> Le 26/09/2025 à 14:40, Sascha Hauer a écrit :
>> On Thu, Sep 25, 2025 at 01:59:12PM +0200, David Picard wrote:
>>> Signed-off-by: David Picard <david.picard@clermont.in2p3.fr>
>>>
>>> ---
>>> v1 --> v2:
>>>    - Use the atsha204a driver to read the MAC address instead of
>>>      board-specific code.
>>> ---
>>> ---
>>>   arch/arm/boards/enclustra-sa2/board.c          | 74 
>>> +++++++++++++++++++++++++-
>>>   arch/arm/dts/socfpga_cyclone5_mercury_sa2.dtsi | 16 +++++-
>>>   2 files changed, 87 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm/boards/enclustra-sa2/board.c 
>>> b/arch/arm/boards/enclustra-sa2/board.c
>>> index 
>>> d3117e9a1058738ea541f45b28c6a95184331554..4c2b44252d84c78ed8de5754051d0bf194ce8d02 
>>> 100644
>>> --- a/arch/arm/boards/enclustra-sa2/board.c
>>> +++ b/arch/arm/boards/enclustra-sa2/board.c
>>> @@ -12,6 +12,12 @@
>>>   #include <fcntl.h>
>>>   #include <fs.h>
>>>   #include <mach/socfpga/cyclone5-regs.h>
>>> +#include <net.h>
>>> +#include <linux/nvmem-consumer.h>
>>> +
>>> +/** Enclustra's MAC address vendor prefix is 20:B0:F7 */
>>> +#define ENCLUSTRA_PREFIX            (0x20b0f7)
>>> +#define MAC_ADDR_NUM_BYTES          (6)
>>>     /*
>>>    * Ethernet PHY: Microchip/Micrel KSZ9031RNX
>>> @@ -21,14 +27,78 @@ static int phy_fixup(struct phy_device *dev)
>>>       return 0;
>>>   }
>>>   +/*
>>> + * Read the MAC address via the atsha204a driver.
>>> + *
>>> + * Set two consecutive MAC addresses, as specified by the 
>>> manufacturer.
>>> + */
>>> +static void set_mac_addr(void)
>>> +{
>>> +    uint8_t hwaddr[MAC_ADDR_NUM_BYTES] = { 0, 0, 0, 0, 0, 0 };
>>> +    uint32_t hwaddr_prefix;
>>> +    u8 *data = NULL;
>>> +    static const char * const aliases[] = { "ethernet0" };
>>> +    struct device_node *np, *root;
>>> +    /* Fallback MAC addresses, used if we can't read from EEPROM: */
>>> +    const uint8_t enclustra_ethaddr_fallback1[] = { 0x20, 0xB0, 
>>> 0xF7, 0x01,
>>> +                                                    0x02, 0x03 };
>>> +    const uint8_t enclustra_ethaddr_fallback2[] = { 0x20, 0xB0, 
>>> 0xF7, 0x01,
>>> +                                                    0x02, 0x04 };
>> Please no fallback addresses. Just skip setting these and barebox will
>> use a random MAC.
>>
>>> +
>>> +    root = of_get_root_node();
>>> +
>>> +    for (int i = 0; i < ARRAY_SIZE(aliases); i++) {
>>> +        const char *alias = aliases[i];
>>> +        np = of_find_node_by_alias(root, alias);
>>> +        if (!np) {
>>> +            pr_warn("%s() >> ERROR: can't find alias %s\n", 
>>> __func__, alias);
>>> +            continue;
>>> +        }
>>> +        data = nvmem_cell_get_and_read(np, "mac-address", 
>>> MAC_ADDR_NUM_BYTES);
>>> +        if (IS_ERR(data)) {
>>> +            pr_warn("%s() >> ERROR: can't read NVMEM cell\n", 
>>> __func__);
>>> +            data = NULL;
>>> +        }
>>> +    }
>> You shouldn't need this. of_get_mac_addr_nvmem() called by the net layer
>> during startup should handle this for you.
>>
>>> +    if (!data)
>>> +        goto fallback_addr;
>>> +
>>> +    memcpy(hwaddr, data, MAC_ADDR_NUM_BYTES);
>>> +
>>> +    debug("MAC address: %02x:%02x:%02x:%02x:%02x:%02x\n",
>>> +          hwaddr[0], hwaddr[1], hwaddr[2],
>>> +          hwaddr[3], hwaddr[4], hwaddr[5]);
>>> +
>>> +    /* check vendor prefix and set the environment variable */
>>> +    hwaddr_prefix = (hwaddr[0] << 16) | (hwaddr[1] << 8) | 
>>> (hwaddr[2]);
>>> +    if (hwaddr_prefix == ENCLUSTRA_PREFIX) {
>>> +        eth_register_ethaddr(0, hwaddr);
>>> +        hwaddr[5]++;    /* calculate 2nd, consecutive MAC address */
>>> +        eth_register_ethaddr(1, hwaddr);
>>> +    } else {
>>> +        printf("%s() >> ERROR: invalid MAC address vendor prefix,"
>>> +               "using fallback addresses\n", __func__);
>>> +        goto fallback_addr;
>>> +    }
>> Not sure what to do about the consecutive MAC address. There seems to be
>> beginning support for this in fixed-cell.yaml, but I can't find an
>> implementation for it.
>>
>>> diff --git a/arch/arm/dts/socfpga_cyclone5_mercury_sa2.dtsi 
>>> b/arch/arm/dts/socfpga_cyclone5_mercury_sa2.dtsi
>>> index 
>>> fa80b7c63d28d17a24d63ac3ee87531ad320ddb1..55d54d289c81fa4a6d46123bcb93c5fc483485e4 
>>> 100644
>>> --- a/arch/arm/dts/socfpga_cyclone5_mercury_sa2.dtsi
>>> +++ b/arch/arm/dts/socfpga_cyclone5_mercury_sa2.dtsi
>>> @@ -76,6 +76,19 @@ atsha204a: atsha204a@64 {
>>>           status = "okay";
>>>           compatible = "atmel,atsha204a";
>>>           reg = <0x64>;
>>> +
>>> +        nvmem-layout {
>>> +        compatible = "fixed-layout";
>>> +        #address-cells = <1>;
>>> +        #size-cells = <1>;
>> Indentation wrong here.
>>
>>> +
>>> +            mac_address_0: mac@10 {
>>> +                compatible = "mac-base";
>>> +                reg = <0x10 0x6>;
>>> +                nvmem-cell-cells = <1>;
>> must be #nvmem-cell-cells#
>>
>> Sascha
>>
>
>