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

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

[Michael Walle]

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

-- 
2.30.2

[PATCH 1/8] of: base: add of_parse_phandle_with_optional_args()

[Michael Walle]

Add a new variant of the of_parse_phandle_with_args() which treats the
cells name as optional. If it's missing, it is assumed that the phandle
has no arguments.

Up until now, a nvmem node didn't have any arguments, so all the device
trees haven't any '#*-cells' property. But there is a need for an
additional argument for the phandle, for which we need a '#*-cells'
property. Therefore, we need to support nvmem nodes with and without
this property.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/of/base.c  | 23 +++++++++++++++++++++++
 include/linux/of.h | 12 ++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5b907600f5b0..fb28bb26276e 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1543,6 +1543,29 @@ int of_parse_phandle_with_args(const struct device_node *np, const char *list_na
 }
 EXPORT_SYMBOL(of_parse_phandle_with_args);
 
+/**
+ * of_parse_phandle_with_optional_args() - Find a node pointed by phandle in a list
+ *
+ * Same as of_parse_phandle_args() except that if the cells_name property is
+ * not found, cell_count of 0 is assumed.
+ *
+ * This is used to useful, if you have a phandle which didn't have arguments
+ * before and thus doesn't have a '#*-cells' property but is now migrated to
+ * having arguments while retaining backwards compatibility.
+ */
+int of_parse_phandle_with_optional_args(const struct device_node *np,
+					const char *list_name,
+					const char *cells_name, int index,
+					struct of_phandle_args *out_args)
+{
+	if (index < 0)
+		return -EINVAL;
+
+	return __of_parse_phandle_with_args(np, list_name, cells_name,
+					    0, index, out_args);
+}
+EXPORT_SYMBOL(of_parse_phandle_with_optional_args);
+
 /**
  * of_parse_phandle_with_args_map() - Find a node pointed by phandle in a list and remap it
  * @np:		pointer to a device tree node containing a list
diff --git a/include/linux/of.h b/include/linux/of.h
index ff143a027abc..ccace453d25f 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -376,6 +376,9 @@ extern int of_parse_phandle_with_args_map(const struct device_node *np,
 extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
 	const char *list_name, int cells_count, int index,
 	struct of_phandle_args *out_args);
+extern int of_parse_phandle_with_optional_args(const struct device_node *np,
+	const char *list_name, const char *cells_name, int index,
+	struct of_phandle_args *out_args);
 extern int of_count_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name);
 
@@ -897,6 +900,15 @@ static inline int of_parse_phandle_with_fixed_args(const struct device_node *np,
 	return -ENOSYS;
 }
 
+static inline int of_parse_phandle_with_optional_args(const struct device_node *np,
+						      char *list_name,
+						      int cells_count,
+						      int index,
+						      struct of_phandle_args *out_args)
+{
+	return -ENOSYS;
+}
+
 static inline int of_count_phandle_with_args(const struct device_node *np,
 					     const char *list_name,
 					     const char *cells_name)
-- 
2.30.2

Re: [PATCH 1/8] of: base: add of_parse_phandle_with_optional_args()

[Rob Herring]

On Tue, Dec 28, 2021 at 03:25:42PM +0100, Michael Walle wrote:
> Add a new variant of the of_parse_phandle_with_args() which treats the
> cells name as optional. If it's missing, it is assumed that the phandle
> has no arguments.
> 
> Up until now, a nvmem node didn't have any arguments, so all the device
> trees haven't any '#*-cells' property. But there is a need for an
> additional argument for the phandle, for which we need a '#*-cells'
> property. Therefore, we need to support nvmem nodes with and without
> this property.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/of/base.c  | 23 +++++++++++++++++++++++
>  include/linux/of.h | 12 ++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 5b907600f5b0..fb28bb26276e 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1543,6 +1543,29 @@ int of_parse_phandle_with_args(const struct device_node *np, const char *list_na
>  }
>  EXPORT_SYMBOL(of_parse_phandle_with_args);
>  
> +/**
> + * of_parse_phandle_with_optional_args() - Find a node pointed by phandle in a list
> + *
> + * Same as of_parse_phandle_args() except that if the cells_name property is
> + * not found, cell_count of 0 is assumed.
> + *
> + * This is used to useful, if you have a phandle which didn't have arguments
> + * before and thus doesn't have a '#*-cells' property but is now migrated to
> + * having arguments while retaining backwards compatibility.
> + */
> +int of_parse_phandle_with_optional_args(const struct device_node *np,
> +					const char *list_name,
> +					const char *cells_name, int index,
> +					struct of_phandle_args *out_args)
> +{
> +	if (index < 0)
> +		return -EINVAL;

I'm not sure why we didn't do this from the start, but just make index 
unsigned and then this check is not needed.

> +
> +	return __of_parse_phandle_with_args(np, list_name, cells_name,
> +					    0, index, out_args);
> +}
> +EXPORT_SYMBOL(of_parse_phandle_with_optional_args);


With the above, just make this static inline. Bonus points if you want 
to do the same changes on the other variants.

Rob

[PATCH 2/8] dt-bindings: nvmem: add transformation bindings

[Michael Walle]

Just add a simple list of the supported devices which need a nvmem
transformations.

Also, since the compatible string is prepended to the actual nvmem
compatible string, we need to match using "contains" instead of an exact
match.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../devicetree/bindings/mtd/mtd.yaml          |  7 +--
 .../bindings/nvmem/nvmem-transformations.yaml | 46 +++++++++++++++++++
 2 files changed, 50 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml

diff --git a/Documentation/devicetree/bindings/mtd/mtd.yaml b/Documentation/devicetree/bindings/mtd/mtd.yaml
index 376b679cfc70..0291e439b6a6 100644
--- a/Documentation/devicetree/bindings/mtd/mtd.yaml
+++ b/Documentation/devicetree/bindings/mtd/mtd.yaml
@@ -33,9 +33,10 @@ patternProperties:
 
     properties:
       compatible:
-        enum:
-          - user-otp
-          - factory-otp
+        contains:
+          enum:
+            - user-otp
+            - factory-otp
 
     required:
       - compatible
diff --git a/Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml b/Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml
new file mode 100644
index 000000000000..8c8d85fd6d27
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/nvmem-transformations.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NVMEM transformations Device Tree Bindings
+
+maintainers:
+  - Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
+
+description: |
+  This is a list NVMEM devices which need transformations.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+        - enum:
+          - kontron,sl28-vpd
+        - const: user-otp
+      - const: user-otp
+
+required:
+  - compatible
+
+additionalProperties: true
+
+examples:
+  - |
+    otp-1 {
+            compatible = "kontron,sl28-vpd", "user-otp";
+            #address-cells = <1>;
+            #size-cells = <1>;
+
+            serial@2 {
+                    reg = <2 15>;
+            };
+
+            base_mac_address: base-mac-address@17 {
+                    #nvmem-cell-cells = <1>;
+                    reg = <17 6>;
+            };
+    };
+
+...
-- 
2.30.2

Re: [PATCH 2/8] dt-bindings: nvmem: add transformation bindings

[Rob Herring]

On Tue, 28 Dec 2021 15:25:43 +0100, Michael Walle wrote:
> Just add a simple list of the supported devices which need a nvmem
> transformations.
> 
> Also, since the compatible string is prepended to the actual nvmem
> compatible string, we need to match using "contains" instead of an exact
> match.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  .../devicetree/bindings/mtd/mtd.yaml          |  7 +--
>  .../bindings/nvmem/nvmem-transformations.yaml | 46 +++++++++++++++++++
>  2 files changed, 50 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml:19:9: [warning] wrong indentation: expected 10 but found 8 (indentation)
./Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml:20:11: [warning] wrong indentation: expected 12 but found 10 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1573687

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 2/8] dt-bindings: nvmem: add transformation bindings

[Rob Herring]

On Tue, Dec 28, 2021 at 03:25:43PM +0100, Michael Walle wrote:
> Just add a simple list of the supported devices which need a nvmem
> transformations.
> 
> Also, since the compatible string is prepended to the actual nvmem
> compatible string, we need to match using "contains" instead of an exact
> match.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  .../devicetree/bindings/mtd/mtd.yaml          |  7 +--
>  .../bindings/nvmem/nvmem-transformations.yaml | 46 +++++++++++++++++++
>  2 files changed, 50 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mtd/mtd.yaml b/Documentation/devicetree/bindings/mtd/mtd.yaml
> index 376b679cfc70..0291e439b6a6 100644
> --- a/Documentation/devicetree/bindings/mtd/mtd.yaml
> +++ b/Documentation/devicetree/bindings/mtd/mtd.yaml
> @@ -33,9 +33,10 @@ patternProperties:
>  
>      properties:
>        compatible:
> -        enum:
> -          - user-otp
> -          - factory-otp
> +        contains:
> +          enum:
> +            - user-otp
> +            - factory-otp

If the addition is only compatible strings, then I would just add them 
here. Otherwise this needs to be structured a bit differently. More on 
that below.

>  
>      required:
>        - compatible
> diff --git a/Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml b/Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml
> new file mode 100644
> index 000000000000..8c8d85fd6d27
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/nvmem/nvmem-transformations.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NVMEM transformations Device Tree Bindings
> +
> +maintainers:
> +  - Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> +
> +description: |
> +  This is a list NVMEM devices which need transformations.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +        - enum:
> +          - kontron,sl28-vpd
> +        - const: user-otp
> +      - const: user-otp

This will be applied to any node containing 'user-otp'. You need a 
custom 'select' to avoid that.

> +
> +required:
> +  - compatible
> +
> +additionalProperties: true

True is only allowed for common schema intended to be included (i.e. a 
$ref) by other schemas. IOW, ones that are incomplete on their own. So 
you need to reference mtd.yaml and make this 'unevaluatedProperties: false'.

> +
> +examples:
> +  - |
> +    otp-1 {
> +            compatible = "kontron,sl28-vpd", "user-otp";
> +            #address-cells = <1>;
> +            #size-cells = <1>;
> +
> +            serial@2 {
> +                    reg = <2 15>;
> +            };
> +
> +            base_mac_address: base-mac-address@17 {
> +                    #nvmem-cell-cells = <1>;
> +                    reg = <17 6>;
> +            };
> +    };
> +
> +...
> -- 
> 2.30.2
> 
> 

Re: [PATCH 2/8] dt-bindings: nvmem: add transformation bindings

[Michael Walle]

Am 2022-01-04 16:03, schrieb Rob Herring:
> On Tue, Dec 28, 2021 at 03:25:43PM +0100, Michael Walle wrote:
>> Just add a simple list of the supported devices which need a nvmem
>> transformations.
>> 
>> Also, since the compatible string is prepended to the actual nvmem
>> compatible string, we need to match using "contains" instead of an 
>> exact
>> match.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  .../devicetree/bindings/mtd/mtd.yaml          |  7 +--
>>  .../bindings/nvmem/nvmem-transformations.yaml | 46 
>> +++++++++++++++++++
>>  2 files changed, 50 insertions(+), 3 deletions(-)
>>  create mode 100644 
>> Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/mtd/mtd.yaml 
>> b/Documentation/devicetree/bindings/mtd/mtd.yaml
>> index 376b679cfc70..0291e439b6a6 100644
>> --- a/Documentation/devicetree/bindings/mtd/mtd.yaml
>> +++ b/Documentation/devicetree/bindings/mtd/mtd.yaml
>> @@ -33,9 +33,10 @@ patternProperties:
>> 
>>      properties:
>>        compatible:
>> -        enum:
>> -          - user-otp
>> -          - factory-otp
>> +        contains:
>> +          enum:
>> +            - user-otp
>> +            - factory-otp
> 
> If the addition is only compatible strings, then I would just add them
> here. Otherwise this needs to be structured a bit differently. More on
> that below.

I wanted to avoid having these compatible strings "cluttered" all around
the various files. Esp. having a specific compatible string in a generic
mtd.yaml. But if everyone is fine with that, I'll just move it here.

>> 
>>      required:
>>        - compatible
>> diff --git 
>> a/Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml 
>> b/Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml
>> new file mode 100644
>> index 000000000000..8c8d85fd6d27
>> --- /dev/null
>> +++ 
>> b/Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml
>> @@ -0,0 +1,46 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/nvmem/nvmem-transformations.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: NVMEM transformations Device Tree Bindings
>> +
>> +maintainers:
>> +  - Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> +
>> +description: |
>> +  This is a list NVMEM devices which need transformations.
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - items:
>> +        - enum:
>> +          - kontron,sl28-vpd
>> +        - const: user-otp
>> +      - const: user-otp
> 
> This will be applied to any node containing 'user-otp'. You need a
> custom 'select' to avoid that.

Out of curiosity, you mean something like:

select:
   compatible:
     contains:
       enum:
         - kontron,sl28-vpd

Re: [PATCH 2/8] dt-bindings: nvmem: add transformation bindings

[Rob Herring]

On Wed, Jan 5, 2022 at 2:25 AM Michael Walle <michael@walle.cc> wrote:
>
> Am 2022-01-04 16:03, schrieb Rob Herring:
> > On Tue, Dec 28, 2021 at 03:25:43PM +0100, Michael Walle wrote:
> >> Just add a simple list of the supported devices which need a nvmem
> >> transformations.
> >>
> >> Also, since the compatible string is prepended to the actual nvmem
> >> compatible string, we need to match using "contains" instead of an
> >> exact
> >> match.
> >>
> >> Signed-off-by: Michael Walle <michael@walle.cc>
> >> ---
> >>  .../devicetree/bindings/mtd/mtd.yaml          |  7 +--
> >>  .../bindings/nvmem/nvmem-transformations.yaml | 46
> >> +++++++++++++++++++
> >>  2 files changed, 50 insertions(+), 3 deletions(-)
> >>  create mode 100644
> >> Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/mtd/mtd.yaml
> >> b/Documentation/devicetree/bindings/mtd/mtd.yaml
> >> index 376b679cfc70..0291e439b6a6 100644
> >> --- a/Documentation/devicetree/bindings/mtd/mtd.yaml
> >> +++ b/Documentation/devicetree/bindings/mtd/mtd.yaml
> >> @@ -33,9 +33,10 @@ patternProperties:
> >>
> >>      properties:
> >>        compatible:
> >> -        enum:
> >> -          - user-otp
> >> -          - factory-otp
> >> +        contains:
> >> +          enum:
> >> +            - user-otp
> >> +            - factory-otp
> >
> > If the addition is only compatible strings, then I would just add them
> > here. Otherwise this needs to be structured a bit differently. More on
> > that below.
>
> I wanted to avoid having these compatible strings "cluttered" all around
> the various files. Esp. having a specific compatible string in a generic
> mtd.yaml. But if everyone is fine with that, I'll just move it here.
>
> >>
> >>      required:
> >>        - compatible
> >> diff --git
> >> a/Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml
> >> b/Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml
> >> new file mode 100644
> >> index 000000000000..8c8d85fd6d27
> >> --- /dev/null
> >> +++
> >> b/Documentation/devicetree/bindings/nvmem/nvmem-transformations.yaml
> >> @@ -0,0 +1,46 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/nvmem/nvmem-transformations.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: NVMEM transformations Device Tree Bindings
> >> +
> >> +maintainers:
> >> +  - Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> >> +
> >> +description: |
> >> +  This is a list NVMEM devices which need transformations.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    oneOf:
> >> +      - items:
> >> +        - enum:
> >> +          - kontron,sl28-vpd
> >> +        - const: user-otp
> >> +      - const: user-otp
> >
> > This will be applied to any node containing 'user-otp'. You need a
> > custom 'select' to avoid that.
>
> Out of curiosity, you mean something like:
>
> select:
>    compatible:
>      contains:
>        enum:
>          - kontron,sl28-vpd

Yes, that's correct.

Rob

[PATCH 3/8] nvmem: core: add an index parameter to the cell

[Michael Walle]

Sometimes a cell can represend multiple values. For example, a base
ethernet address stored in the NVMEM can be expanded into multiple
discreet ones by adding an offset.

For this use case, introduce an index parameter which is then used to
distiguish between values. This parameter will then be passed to the
post process hook which can then use it to create different values
during reading.

At the moment, there is only support for the device tree path. You can
add the index to the phandle, e.g.

  &net {
          nvmem-cells = <&base_mac_address 2>;
          nvmem-cell-names = "mac-address";
  };

  &nvmem_provider {
          base_mac_address: base-mac-address@0 {
                  #nvmem-cell-cells = <1>;
                  reg = <0 6>;
          };
  };

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/nvmem/core.c           | 37 ++++++++++++++++++++++++----------
 drivers/nvmem/imx-ocotp.c      |  4 ++--
 include/linux/nvmem-provider.h |  4 ++--
 3 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 23a38dcf0fc4..31d77c51dbe3 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -60,6 +60,7 @@ struct nvmem_cell_entry {
 struct nvmem_cell {
 	struct nvmem_cell_entry *entry;
 	const char		*id;
+	int			index;
 };
 
 static DEFINE_MUTEX(nvmem_mutex);
@@ -1148,7 +1149,8 @@ struct nvmem_device *devm_nvmem_device_get(struct device *dev, const char *id)
 }
 EXPORT_SYMBOL_GPL(devm_nvmem_device_get);
 
-static struct nvmem_cell *nvmem_create_cell(struct nvmem_cell_entry *entry, const char *id)
+static struct nvmem_cell *nvmem_create_cell(struct nvmem_cell_entry *entry,
+					    const char *id, int index)
 {
 	struct nvmem_cell *cell;
 	const char *name = NULL;
@@ -1167,6 +1169,7 @@ static struct nvmem_cell *nvmem_create_cell(struct nvmem_cell_entry *entry, cons
 
 	cell->id = name;
 	cell->entry = entry;
+	cell->index = index;
 
 	return cell;
 }
@@ -1205,7 +1208,7 @@ nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
 				__nvmem_device_put(nvmem);
 				cell = ERR_PTR(-ENOENT);
 			} else {
-				cell = nvmem_create_cell(cell_entry, con_id);
+				cell = nvmem_create_cell(cell_entry, con_id, 0);
 				if (IS_ERR(cell))
 					__nvmem_device_put(nvmem);
 			}
@@ -1253,15 +1256,27 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, const char *id)
 	struct nvmem_device *nvmem;
 	struct nvmem_cell_entry *cell_entry;
 	struct nvmem_cell *cell;
+	struct of_phandle_args cell_spec;
 	int index = 0;
+	int cell_index = 0;
+	int ret;
 
 	/* if cell name exists, find index to the name */
 	if (id)
 		index = of_property_match_string(np, "nvmem-cell-names", id);
 
-	cell_np = of_parse_phandle(np, "nvmem-cells", index);
-	if (!cell_np)
-		return ERR_PTR(-ENOENT);
+	ret = of_parse_phandle_with_optional_args(np, "nvmem-cells",
+						  "#nvmem-cell-cells",
+						  index, &cell_spec);
+	if (ret)
+		return ERR_PTR(ret);
+
+	if (cell_spec.args_count > 1)
+		return ERR_PTR(-EINVAL);
+
+	cell_np = cell_spec.np;
+	if (cell_spec.args_count)
+		cell_index = cell_spec.args[0];
 
 	nvmem_np = of_get_next_parent(cell_np);
 	if (!nvmem_np)
@@ -1278,7 +1293,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, const char *id)
 		return ERR_PTR(-ENOENT);
 	}
 
-	cell = nvmem_create_cell(cell_entry, id);
+	cell = nvmem_create_cell(cell_entry, id, cell_index);
 	if (IS_ERR(cell))
 		__nvmem_device_put(nvmem);
 
@@ -1431,8 +1446,8 @@ static void nvmem_shift_read_buffer_in_place(struct nvmem_cell_entry *cell, void
 }
 
 static int __nvmem_cell_read(struct nvmem_device *nvmem,
-		      struct nvmem_cell_entry *cell,
-		      void *buf, size_t *len, const char *id)
+			     struct nvmem_cell_entry *cell,
+			     void *buf, size_t *len, const char *id, int index)
 {
 	int rc;
 
@@ -1446,7 +1461,7 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem,
 		nvmem_shift_read_buffer_in_place(cell, buf);
 
 	if (nvmem->cell_post_process) {
-		rc = nvmem->cell_post_process(nvmem->priv, id,
+		rc = nvmem->cell_post_process(nvmem->priv, id, index,
 					      cell->offset, buf, cell->bytes);
 		if (rc)
 			return rc;
@@ -1481,7 +1496,7 @@ void *nvmem_cell_read(struct nvmem_cell *cell, size_t *len)
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
 
-	rc = __nvmem_cell_read(nvmem, cell->entry, buf, len, cell->id);
+	rc = __nvmem_cell_read(nvmem, cell->entry, buf, len, cell->id, cell->index);
 	if (rc) {
 		kfree(buf);
 		return ERR_PTR(rc);
@@ -1794,7 +1809,7 @@ ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
 	if (rc)
 		return rc;
 
-	rc = __nvmem_cell_read(nvmem, &cell, buf, &len, NULL);
+	rc = __nvmem_cell_read(nvmem, &cell, buf, &len, NULL, 0);
 	if (rc)
 		return rc;
 
diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c
index 14284e866f26..e9b52ecb3f72 100644
--- a/drivers/nvmem/imx-ocotp.c
+++ b/drivers/nvmem/imx-ocotp.c
@@ -222,8 +222,8 @@ static int imx_ocotp_read(void *context, unsigned int offset,
 	return ret;
 }
 
-static int imx_ocotp_cell_pp(void *context, const char *id, unsigned int offset,
-			     void *data, size_t bytes)
+static int imx_ocotp_cell_pp(void *context, const char *id, int index,
+			     unsigned int offset, void *data, size_t bytes)
 {
 	struct ocotp_priv *priv = context;
 
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 98efb7b5660d..8cf8593c8ae7 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -20,8 +20,8 @@ typedef int (*nvmem_reg_read_t)(void *priv, unsigned int offset,
 typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset,
 				 void *val, size_t bytes);
 /* used for vendor specific post processing of cell data */
-typedef int (*nvmem_cell_post_process_t)(void *priv, const char *id, unsigned int offset,
-					  void *buf, size_t bytes);
+typedef int (*nvmem_cell_post_process_t)(void *priv, const char *id, int index,
+					 unsigned int offset, void *buf, size_t bytes);
 
 enum nvmem_type {
 	NVMEM_TYPE_UNKNOWN = 0,
-- 
2.30.2

[PATCH 4/8] nvmem: core: add transformations support

[Michael Walle]

Sometimes the value of an nvmem cell must be transformed. For example, a
MAC address might be stored in ASCII representation or we might need to
swap bytes. We might also want to expand one value to mutliple ones, for
example, the nvmem cell might just store a base MAC address to which we
need to add an offset to get an address for different network
interfaces.

Add initial support to add such transformations based on the device tree
compatible string of the NVMEM device. It will reuse the nvmem post
process hook. Both are mutually exclusive.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/nvmem/Kconfig           |  7 +++++++
 drivers/nvmem/Makefile          |  1 +
 drivers/nvmem/core.c            |  7 +++++++
 drivers/nvmem/transformations.c | 28 ++++++++++++++++++++++++++++
 include/linux/nvmem-provider.h  |  9 +++++++++
 5 files changed, 52 insertions(+)
 create mode 100644 drivers/nvmem/transformations.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index da414617a54d..94dd60b2654e 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -21,6 +21,13 @@ config NVMEM_SYSFS
 	 This interface is mostly used by userspace applications to
 	 read/write directly into nvmem.
 
+config NVMEM_TRANSFORMATIONS
+	bool "Support cell transformations"
+	help
+	  Say Y to support to expand one NVMEM cell into multiple values. For
+	  example, when the NVMEM cell stores a base MAC address and it should
+	  be expanded to be used for multiple network adapters.
+
 config NVMEM_IMX_IIM
 	tristate "i.MX IC Identification Module support"
 	depends on ARCH_MXC || COMPILE_TEST
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index dcbbde35b6a8..4e6d877fdfaf 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -5,6 +5,7 @@
 
 obj-$(CONFIG_NVMEM)		+= nvmem_core.o
 nvmem_core-y			:= core.o
+nvmem_core-$(CONFIG_NVMEM_TRANSFORMATIONS)	+= transformations.o
 
 # Devices
 obj-$(CONFIG_NVMEM_BCM_OCOTP)	+= nvmem-bcm-ocotp.o
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 31d77c51dbe3..30e4b58a6dca 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -838,6 +838,13 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		}
 	}
 
+	/*
+	 * Transformations are using the same post process hook, therefore they
+	 * are mutually exclusive.
+	 */
+	if (!nvmem->cell_post_process)
+		nvmem->cell_post_process = nvmem_get_transformations(nvmem->dev.of_node);
+
 	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
 
 	rval = device_register(&nvmem->dev);
diff --git a/drivers/nvmem/transformations.c b/drivers/nvmem/transformations.c
new file mode 100644
index 000000000000..61642a9feefb
--- /dev/null
+++ b/drivers/nvmem/transformations.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * nvmem cell transformations
+ */
+
+#include <linux/etherdevice.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of.h>
+
+struct nvmem_transformations {
+	const char *compatible;
+	nvmem_cell_post_process_t pp;
+};
+
+static const struct nvmem_transformations nvmem_transformations[] = {
+	{}
+};
+
+nvmem_cell_post_process_t nvmem_get_transformations(struct device_node *np)
+{
+	const struct nvmem_transformations *transform = nvmem_transformations;
+
+	for (; transform->compatible; transform++)
+		if (of_device_is_compatible(np, transform->compatible))
+			return transform->pp;
+
+	return NULL;
+}
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 8cf8593c8ae7..efc6ffdcb7e0 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -138,6 +138,15 @@ int devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem);
 void nvmem_add_cell_table(struct nvmem_cell_table *table);
 void nvmem_del_cell_table(struct nvmem_cell_table *table);
 
+#if IS_ENABLED(CONFIG_NVMEM_TRANSFORMATIONS)
+nvmem_cell_post_process_t nvmem_get_transformations(struct device_node *np);
+#else
+static inline nvmem_cell_post_process_t nvmem_get_transformations(struct device_node *np)
+{
+	return NULL;
+}
+#endif
+
 #else
 
 static inline struct nvmem_device *nvmem_register(const struct nvmem_config *c)
-- 
2.30.2

[PATCH 5/8] net: add helper eth_addr_add()

[Michael Walle]

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);
+}
+
 /**
  * is_etherdev_addr - Tell if given Ethernet address belongs to the device.
  * @dev: Pointer to a device structure
-- 
2.30.2

Re: [PATCH 5/8] net: add helper eth_addr_add()

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

Re: [PATCH 5/8] net: add helper eth_addr_add()

[Michael Walle]

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

[PATCH 6/8] nvmem: transformations: ethernet address offset support

[Michael Walle]

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 },
 	{}
 };
 
-- 
2.30.2

Re: [PATCH 6/8] nvmem: transformations: ethernet address offset support

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

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.

Re: [PATCH 6/8] nvmem: transformations: ethernet address offset support

[Michael Walle]

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/

[PATCH 7/8] arm64: dts: ls1028a: sl28: get MAC addresses from VPD

[Michael Walle]

Now that it is finally possible to get the MAC addresses from the OTP
memory, use it to set the addresses of the network devices.

There are 8 reserved MAC addresses in total per board. Distribute them
as follows:

+----------+------+------+------+------+------+
|          | var1 | var2 | var3 | var4 | kbox |
+----------+------+------+------+------+------+
| enetc #0 |   +0 |      |      |   +0 |   +0 |
| enetc #1 |      |      |   +0 |   +1 |   +1 |
| enetc #2 | rand | rand | rand | rand | rand |
| enetc #3 |      |      |      |      |      |
| felix p0 |      |   +0 |      |      |   +4 |
| felix p1 |      |   +1 |      |      |   +5 |
| felix p2 |      |      |      |      |   +6 |
| felix p3 |      |      |      |      |   +7 |
| felix p4 |      |      |      |      |      |
| felix p5 |      |      |      |      |      |
+----------+------+------+------+------+------+

An empty cell means, the port is not available and thus doesn't need an
ethernet address.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../fsl-ls1028a-kontron-kbox-a-230-ls.dts       |  8 ++++++++
 .../freescale/fsl-ls1028a-kontron-sl28-var1.dts |  2 ++
 .../freescale/fsl-ls1028a-kontron-sl28-var2.dts |  4 ++++
 .../freescale/fsl-ls1028a-kontron-sl28-var4.dts |  2 ++
 .../dts/freescale/fsl-ls1028a-kontron-sl28.dts  | 17 +++++++++++++++++
 5 files changed, 33 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
index 0bcf97772995..6f08d3442309 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
@@ -76,6 +76,8 @@ &mscc_felix_port0 {
 	managed = "in-band-status";
 	phy-handle = <&qsgmii_phy0>;
 	phy-mode = "qsgmii";
+	nvmem-cells = <&base_mac_address 4>;
+	nvmem-cell-names = "mac-address";
 	status = "okay";
 };
 
@@ -84,6 +86,8 @@ &mscc_felix_port1 {
 	managed = "in-band-status";
 	phy-handle = <&qsgmii_phy1>;
 	phy-mode = "qsgmii";
+	nvmem-cells = <&base_mac_address 5>;
+	nvmem-cell-names = "mac-address";
 	status = "okay";
 };
 
@@ -92,6 +96,8 @@ &mscc_felix_port2 {
 	managed = "in-band-status";
 	phy-handle = <&qsgmii_phy2>;
 	phy-mode = "qsgmii";
+	nvmem-cells = <&base_mac_address 6>;
+	nvmem-cell-names = "mac-address";
 	status = "okay";
 };
 
@@ -100,6 +106,8 @@ &mscc_felix_port3 {
 	managed = "in-band-status";
 	phy-handle = <&qsgmii_phy3>;
 	phy-mode = "qsgmii";
+	nvmem-cells = <&base_mac_address 7>;
+	nvmem-cell-names = "mac-address";
 	status = "okay";
 };
 
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var1.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var1.dts
index 7cd29ab970d9..1f34c7553459 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var1.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var1.dts
@@ -55,5 +55,7 @@ &enetc_port0 {
 &enetc_port1 {
 	phy-handle = <&phy0>;
 	phy-mode = "rgmii-id";
+	nvmem-cells = <&base_mac_address 0>;
+	nvmem-cell-names = "mac-address";
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts
index 330e34f933a3..0ed0d2545922 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts
@@ -48,6 +48,8 @@ &mscc_felix_port0 {
 	managed = "in-band-status";
 	phy-handle = <&phy0>;
 	phy-mode = "sgmii";
+	nvmem-cells = <&base_mac_address 0>;
+	nvmem-cell-names = "mac-address";
 	status = "okay";
 };
 
@@ -56,6 +58,8 @@ &mscc_felix_port1 {
 	managed = "in-band-status";
 	phy-handle = <&phy1>;
 	phy-mode = "sgmii";
+	nvmem-cells = <&base_mac_address 1>;
+	nvmem-cell-names = "mac-address";
 	status = "okay";
 };
 
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var4.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var4.dts
index 9b5e92fb753e..a4421db3784e 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var4.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var4.dts
@@ -43,5 +43,7 @@ vddh: vddh-regulator {
 &enetc_port1 {
 	phy-handle = <&phy1>;
 	phy-mode = "rgmii-id";
+	nvmem-cells = <&base_mac_address 1>;
+	nvmem-cell-names = "mac-address";
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
index 63e005dd5aae..81b5f735d22e 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
@@ -108,6 +108,8 @@ &enetc_port0 {
 	phy-handle = <&phy0>;
 	phy-mode = "sgmii";
 	managed = "in-band-status";
+	nvmem-cells = <&base_mac_address 0>;
+	nvmem-cell-names = "mac-address";
 	status = "okay";
 };
 
@@ -170,6 +172,21 @@ partition@3e0000 {
 				label = "bootloader environment";
 			};
 		};
+
+		otp-1 {
+			compatible = "kontron,sl28-vpd", "user-otp";
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			serial@2 {
+				reg = <2 15>;
+			};
+
+			base_mac_address: base-mac-address@17 {
+				#nvmem-cell-cells = <1>;
+				reg = <17 6>;
+			};
+		};
 	};
 };
 
-- 
2.30.2

[PATCH 8/8] arm64: defconfig: enable NVMEM transformations

[Michael Walle]

The Kontron sl28 needs the transformation to get the correct ethernet
addresses for its onboard NICs.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index ee4bd7710a0d..117423e81d15 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1178,6 +1178,7 @@ CONFIG_FSL_IMX8_DDR_PMU=m
 CONFIG_HISI_PMU=y
 CONFIG_QCOM_L2_PMU=y
 CONFIG_QCOM_L3_PMU=y
+CONFIG_NVMEM_TRANSFORMATIONS=y
 CONFIG_NVMEM_IMX_OCOTP=y
 CONFIG_NVMEM_IMX_OCOTP_SCU=y
 CONFIG_QCOM_QFPROM=y
-- 
2.30.2