[0/8] nvmem: add ethernet address offset support

This is my second attempt to solve the use case where there is only the
base MAC address stored in an EEPROM or similar storage provider. This
is the case for the Kontron sl28 board and multiple openwrt supported
boards.

Introduce an NVMEM transformation op. This can then be used to parse or
swap bytes of the NVMEM cell value. A transformation might also have
multiple output values, like in the base mac address case. It reads the mac
address from the nvmem storage and generates multiple individual addresses,
i.e. on our board we reserve 8 consecutive addresses. These addresses then
can be assigned to different network interfaces. To make it possible to
reference different values we need to introduce an argument to the phandle.
This additional argument is then an index which is can be used by the
transformation op.

Previous discussion can be found here:
https://lore.kernel.org/linux-devicetree/20211123134425.3875656-1-michael@walle.cc/

Michael Walle (8):
  of: base: add of_parse_phandle_with_optional_args()
  dt-bindings: nvmem: add transformation bindings
  nvmem: core: add an index parameter to the cell
  nvmem: core: add transformations support
  net: add helper eth_addr_add()
  nvmem: transformations: ethernet address offset support
  arm64: dts: ls1028a: sl28: get MAC addresses from VPD
  arm64: defconfig: enable NVMEM transformations

 .../devicetree/bindings/mtd/mtd.yaml          |  7 +-
 .../bindings/nvmem/nvmem-transformations.yaml | 46 ++++++++++++
 .../fsl-ls1028a-kontron-kbox-a-230-ls.dts     |  8 ++
 .../fsl-ls1028a-kontron-sl28-var1.dts         |  2 +
 .../fsl-ls1028a-kontron-sl28-var2.dts         |  4 +
 .../fsl-ls1028a-kontron-sl28-var4.dts         |  2 +
 .../freescale/fsl-ls1028a-kontron-sl28.dts    | 17 +++++
 arch/arm64/configs/defconfig                  |  1 +
 drivers/nvmem/Kconfig                         |  7 ++
 drivers/nvmem/Makefile                        |  1 +
 drivers/nvmem/core.c                          | 44 ++++++++---
 drivers/nvmem/imx-ocotp.c                     |  4 +-
 drivers/nvmem/transformations.c               | 73 +++++++++++++++++++
 drivers/of/base.c                             | 23 ++++++
 include/linux/etherdevice.h                   | 14 ++++
 include/linux/nvmem-provider.h                | 13 +++-
 include/linux/of.h                            | 12 +++
 17 files changed, 260 insertions(+), 18 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml
 create mode 100644 drivers/nvmem/transformations.c

On 28.12.2021 15:25, Michael Walle wrote:
> Add a helper to add an offset to a ethernet address. This comes in handy
> if you have a base ethernet address for multiple interfaces.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>   include/linux/etherdevice.h | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> index 2ad71cc90b37..9d621dc85290 100644
> --- a/include/linux/etherdevice.h
> +++ b/include/linux/etherdevice.h
> @@ -486,6 +486,20 @@ static inline void eth_addr_inc(u8 *addr)
>   	u64_to_ether_addr(u, addr);
>   }
>   
> +/**
> + * eth_addr_add() - Add (or subtract) and offset to/from the given MAC address.
> + *
> + * @offset: Offset to add.
> + * @addr: Pointer to a six-byte array containing Ethernet address to increment.
> + */
> +static inline void eth_addr_add(u8 *addr, long offset)
> +{
> +	u64 u = ether_addr_to_u64(addr);
> +
> +	u += offset;
> +	u64_to_ether_addr(u, addr);
> +}

Please check eth_hw_addr_gen() which contains identical code +
eth_hw_addr_set().

You should probably make eth_hw_addr_gen() use your new function as a
helper.

On 28.12.2021 15:25, Michael Walle wrote:
> An nvmem cell might just contain a base MAC address. To generate a
> address of a specific interface, add a transformation to add an offset
> to this base address.
> 
> Add a generic implementation and the first user of it, namely the sl28
> vpd storage.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>   drivers/nvmem/transformations.c | 45 +++++++++++++++++++++++++++++++++
>   1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/nvmem/transformations.c b/drivers/nvmem/transformations.c
> index 61642a9feefb..15cd26da1f83 100644
> --- a/drivers/nvmem/transformations.c
> +++ b/drivers/nvmem/transformations.c
> @@ -12,7 +12,52 @@ struct nvmem_transformations {
>   	nvmem_cell_post_process_t pp;
>   };
>   
> +/**
> + * nvmem_transform_mac_address_offset() - Add an offset to a mac address cell
> + *
> + * A simple transformation which treats the index argument as an offset and add
> + * it to a mac address. This is useful, if the nvmem cell stores a base
> + * ethernet address.
> + *
> + * @index: nvmem cell index
> + * @data: nvmem data
> + * @bytes: length of the data
> + *
> + * Return: 0 or negative error code on failure.
> + */
> +static int nvmem_transform_mac_address_offset(int index, unsigned int offset,
> +					      void *data, size_t bytes)
> +{
> +	if (bytes != ETH_ALEN)
> +		return -EINVAL;
> +
> +	if (index < 0)
> +		return -EINVAL;
> +
> +	if (!is_valid_ether_addr(data))
> +		return -EINVAL;
> +
> +	eth_addr_add(data, index);
> +
> +	return 0;
> +}
> +
> +static int nvmem_kontron_sl28_vpd_pp(void *priv, const char *id, int index,
> +				     unsigned int offset, void *data,
> +				     size_t bytes)
> +{
> +	if (!id)
> +		return 0;
> +
> +	if (!strcmp(id, "mac-address"))
> +		return nvmem_transform_mac_address_offset(index, offset, data,
> +							  bytes);
> +
> +	return 0;
> +}
> +
>   static const struct nvmem_transformations nvmem_transformations[] = {
> +	{ .compatible = "kontron,sl28-vpd", .pp = nvmem_kontron_sl28_vpd_pp },
>   	{}
>   };

I think it's a rather bad solution that won't scale well at all.

You'll end up with a lot of NVMEM device specific strings and code in a
NVMEM core.

You'll have a lot of duplicated code (many device specific functions
calling e.g. nvmem_transform_mac_address_offset()).

I think it also ignores fact that one NVMEM device can be reused in
multiple platforms / device models using different (e.g. vendor / device
specific) cells.


What if we have:
1. Foo company using "kontron,sl28-vpd" with NVMEM cells:
    a. "mac-address"
    b. "mac-address-2"
    c. "mac-address-3"
2. Bar company using "kontron,sl28-vpd" with NVMEM cell:
    a. "mac-address"

In the first case you don't want any transformation.


If you consider using transformations for ASCII formats too then it
causes another conflict issue. Consider two devices:

1. Foo company device with BIN format of MAC
2. Bar company device with ASCII format of MAC

Both may use exactly the same binding:

partition@0 {
         compatible = "nvmem-cells";
         reg = <0x0 0x100000>;
         label = "bootloader";

         #address-cells = <1>;
         #size-cells = <1>;

         mac-address@100 {
                 reg = <0x100 0x6>;
         };
};

how are you going to handle them with proposed implementation? You can't
support both if you share "nvmem-cells" compatible string.


I think that what can solve those problems is assing "compatible" to
NVMEM cells.

Let me think about details of that possible solution.

Hi,

Am 2022-01-25 13:08, schrieb Rafał Miłecki:
> On 28.12.2021 15:25, Michael Walle wrote:
>> An nvmem cell might just contain a base MAC address. To generate a
>> address of a specific interface, add a transformation to add an offset
>> to this base address.
>> 
>> Add a generic implementation and the first user of it, namely the sl28
>> vpd storage.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>   drivers/nvmem/transformations.c | 45 
>> +++++++++++++++++++++++++++++++++
>>   1 file changed, 45 insertions(+)
>> 
>> diff --git a/drivers/nvmem/transformations.c 
>> b/drivers/nvmem/transformations.c
>> index 61642a9feefb..15cd26da1f83 100644
>> --- a/drivers/nvmem/transformations.c
>> +++ b/drivers/nvmem/transformations.c
>> @@ -12,7 +12,52 @@ struct nvmem_transformations {
>>   	nvmem_cell_post_process_t pp;
>>   };
>>   +/**
>> + * nvmem_transform_mac_address_offset() - Add an offset to a mac 
>> address cell
>> + *
>> + * A simple transformation which treats the index argument as an 
>> offset and add
>> + * it to a mac address. This is useful, if the nvmem cell stores a 
>> base
>> + * ethernet address.
>> + *
>> + * @index: nvmem cell index
>> + * @data: nvmem data
>> + * @bytes: length of the data
>> + *
>> + * Return: 0 or negative error code on failure.
>> + */
>> +static int nvmem_transform_mac_address_offset(int index, unsigned int 
>> offset,
>> +					      void *data, size_t bytes)
>> +{
>> +	if (bytes != ETH_ALEN)
>> +		return -EINVAL;
>> +
>> +	if (index < 0)
>> +		return -EINVAL;
>> +
>> +	if (!is_valid_ether_addr(data))
>> +		return -EINVAL;
>> +
>> +	eth_addr_add(data, index);
>> +
>> +	return 0;
>> +}
>> +
>> +static int nvmem_kontron_sl28_vpd_pp(void *priv, const char *id, int 
>> index,
>> +				     unsigned int offset, void *data,
>> +				     size_t bytes)
>> +{
>> +	if (!id)
>> +		return 0;
>> +
>> +	if (!strcmp(id, "mac-address"))
>> +		return nvmem_transform_mac_address_offset(index, offset, data,
>> +							  bytes);
>> +
>> +	return 0;
>> +}
>> +
>>   static const struct nvmem_transformations nvmem_transformations[] = 
>> {
>> +	{ .compatible = "kontron,sl28-vpd", .pp = nvmem_kontron_sl28_vpd_pp 
>> },
>>   	{}
>>   };
> 
> I think it's a rather bad solution that won't scale well at all.
> 
> You'll end up with a lot of NVMEM device specific strings and code in a
> NVMEM core.

They must not be in the core, but they have to be somewhere. That is
because Rob isn't fond of describing the actual transformation in
the device tree but to have it a specific compatible [1]. Thus you have
to have these strings somewhere in the driver code.

> You'll have a lot of duplicated code (many device specific functions
> calling e.g. nvmem_transform_mac_address_offset()).

If there will be multiple formats using the same transformation for
different compatible strings, you could just use the same function
for all compatibles.

But we have to first agree on the device tree representation because
that is then fixed. The driver code can change over time.

> I think it also ignores fact that one NVMEM device can be reused in
> multiple platforms / device models using different (e.g. vendor / 
> device
> specific) cells.
> 
> 
> What if we have:
> 1. Foo company using "kontron,sl28-vpd" with NVMEM cells:
>    a. "mac-address"
>    b. "mac-address-2"
>    c. "mac-address-3"
> 2. Bar company using "kontron,sl28-vpd" with NVMEM cell:
>    a. "mac-address"
> 
> In the first case you don't want any transformation.

I can't follow you here. The "kontron,sl28-vpd" specifies one
particular format, namely, that there is a base address
rather than individual ones.

> If you consider using transformations for ASCII formats too then it
> causes another conflict issue. Consider two devices:
> 
> 1. Foo company device with BIN format of MAC
> 2. Bar company device with ASCII format of MAC
> 
> Both may use exactly the same binding:
> 
> partition@0 {
>         compatible = "nvmem-cells";
>         reg = <0x0 0x100000>;
>         label = "bootloader";
> 
>         #address-cells = <1>;
>         #size-cells = <1>;
> 
>         mac-address@100 {
>                 reg = <0x100 0x6>;
>         };
> };
> 
> how are you going to handle them with proposed implementation? You 
> can't
> support both if you share "nvmem-cells" compatible string.

No, you'd need two different compatible strings. Again, that all boils
down to what the device tree should describe and what not.

But if you have the u-boot environment as an nvmem provider, you already
know that the mac address is in ascii representation and you'd need
to transform it to the kernel representation.

> I think that what can solve those problems is assing "compatible" to
> NVMEM cells.
> 
> Let me think about details of that possible solution.

See [2].

-michael

[1] 
https://lore.kernel.org/linux-devicetree/YaZ5JNCFeKcdIfu8@robh.at.kernel.org/
[2] 
https://lore.kernel.org/linux-devicetree/CAL_JsqL55mZJ6jUyQACer2pKMNDV08-FgwBREsJVgitnuF18Cg@mail.gmail.com/

Am 2022-01-25 11:24, schrieb Rafał Miłecki:
> On 28.12.2021 15:25, Michael Walle wrote:
>> Add a helper to add an offset to a ethernet address. This comes in 
>> handy
>> if you have a base ethernet address for multiple interfaces.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>   include/linux/etherdevice.h | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>> 
>> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
>> index 2ad71cc90b37..9d621dc85290 100644
>> --- a/include/linux/etherdevice.h
>> +++ b/include/linux/etherdevice.h
>> @@ -486,6 +486,20 @@ static inline void eth_addr_inc(u8 *addr)
>>   	u64_to_ether_addr(u, addr);
>>   }
>>   +/**
>> + * eth_addr_add() - Add (or subtract) and offset to/from the given 
>> MAC address.
>> + *
>> + * @offset: Offset to add.
>> + * @addr: Pointer to a six-byte array containing Ethernet address to 
>> increment.
>> + */
>> +static inline void eth_addr_add(u8 *addr, long offset)
>> +{
>> +	u64 u = ether_addr_to_u64(addr);
>> +
>> +	u += offset;
>> +	u64_to_ether_addr(u, addr);
>> +}
> 
> Please check eth_hw_addr_gen() which contains identical code +
> eth_hw_addr_set().
> 
> You should probably make eth_hw_addr_gen() use your new function as a
> helper.

You'd need to copy the mac address first because eth_addr_add()
modifies the mac address in place. I don't see that this is
improving anything.

-michael