diff mbox series

[v1,09/14] dt-bindings: nvmem: add YAML schema for the sl28 vpd layout

Message ID 20220825214423.903672-10-michael@walle.cc (mailing list archive)
State Not Applicable
Headers show
Series nvmem: core: introduce NVMEM layouts | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Michael Walle Aug. 25, 2022, 9:44 p.m. UTC
Add a schema for the NVMEM layout on Kontron's sl28 boards.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../nvmem/layouts/kontron,sl28-vpd.yaml       | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml

Comments

Rob Herring (Arm) Aug. 26, 2022, 4:05 p.m. UTC | #1
On Thu, 25 Aug 2022 23:44:18 +0200, Michael Walle wrote:
> Add a schema for the NVMEM layout on Kontron's sl28 boards.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  .../nvmem/layouts/kontron,sl28-vpd.yaml       | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.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:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mtd/mtd.example.dtb: otp-2: compatible:0: 'kontron,sl28-vpd' was expected
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mtd/mtd.example.dtb: otp-2: compatible: ['user-otp'] is too short
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mtd/mtd.example.dtb: otp-2: '#address-cells', '#size-cells', 'mac-address@0' do not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml

doc reference errors (make refcheckdocs):

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

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.
Krzysztof Kozlowski Aug. 31, 2022, 7:45 a.m. UTC | #2
On 26/08/2022 00:44, Michael Walle wrote:
> Add a schema for the NVMEM layout on Kontron's sl28 boards.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  .../nvmem/layouts/kontron,sl28-vpd.yaml       | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml b/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
> new file mode 100644
> index 000000000000..e4bc2d9182db
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/nvmem/layouts/kontron,sl28-vpd.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NVMEM layout of the Kontron SMARC-sAL28 vital product data
> +
> +maintainers:
> +  - Michael Walle <michael@walle.cc>
> +
> +description:
> +  The vital product data (VPD) of the sl28 boards contains a serial
> +  number and a base MAC address. The actual MAC addresses for the
> +  on-board ethernet devices are derived from this base MAC address by
> +  adding an offset.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: kontron,sl28-vpd
> +      - const: user-otp
> +
> +  serial-number:
> +    type: object

You should define the contents of this object. I would expect this to be
uint32 or string. I think you also need description, as this is not
really standard field.

> +
> +  base-mac-address:

Fields should be rather described here, not in top-level description.

> +    type: object

On this level:
    additionalProperties: false

> +
> +    properties:
> +      "#nvmem-cell-cells":
> +        const: 1
> +

I also wonder why you do not have unit addresses. What if you want to
have two base MAC addresses?

> +required:
> +  - compatible

Other fields are I guess required? At least serial-number should be always?

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +      otp-1 {

Messed up indentation (use 4 spaces). Generic node name "otp".

> +          compatible = "kontron,sl28-vpd", "user-otp";
> +
> +          serial_number: serial-number {

What's the point of the empty node?

> +          };
> +
> +          base_mac_address: base-mac-address {
> +              #nvmem-cell-cells = <1>;
> +          };
> +      };
> +
> +...


Best regards,
Krzysztof
Michael Walle Aug. 31, 2022, 8:17 a.m. UTC | #3
Am 2022-08-31 09:45, schrieb Krzysztof Kozlowski:
> On 26/08/2022 00:44, Michael Walle wrote:
>> Add a schema for the NVMEM layout on Kontron's sl28 boards.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  .../nvmem/layouts/kontron,sl28-vpd.yaml       | 52 
>> +++++++++++++++++++
>>  1 file changed, 52 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml 
>> b/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
>> new file mode 100644
>> index 000000000000..e4bc2d9182db
>> --- /dev/null
>> +++ 
>> b/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
>> @@ -0,0 +1,52 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: 
>> http://devicetree.org/schemas/nvmem/layouts/kontron,sl28-vpd.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: NVMEM layout of the Kontron SMARC-sAL28 vital product data
>> +
>> +maintainers:
>> +  - Michael Walle <michael@walle.cc>
>> +
>> +description:
>> +  The vital product data (VPD) of the sl28 boards contains a serial
>> +  number and a base MAC address. The actual MAC addresses for the
>> +  on-board ethernet devices are derived from this base MAC address by
>> +  adding an offset.
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - const: kontron,sl28-vpd
>> +      - const: user-otp
>> +
>> +  serial-number:
>> +    type: object
> 
> You should define the contents of this object. I would expect this to 
> be
> uint32 or string. I think you also need description, as this is not
> really standard field.

First thing, this binding isn't like the usual ones, so it might be
totally wrong.

What I'd like to achieve here is the following:

We have the nvmem-consumer dt binding where you can reference a
nvmem cell in a consumer node. Example:
   nvmem-cells = <&base_mac_address 5>;
   nvmem-cell-names = "mac-address";

On the other end of the link we have the nvmem-provider. The dt
bindings works well if that one has individual cell nodes, like
it is described in the nvmem.yaml binding. I.e. you can give the
cell a label and make a reference to it in the consumer just like
in the example above.

Now comes the catch: what if there is no actual description of the
cell in the device tree, but is is generated during runtime. How
can I get a label to it. Therefore, in this case, there is just
an empty node and the driver will associate it with the cell
created during runtime (see patch 10). It is not expected, that
is has any properties.

>> +
>> +  base-mac-address:
> 
> Fields should be rather described here, not in top-level description.
> 
>> +    type: object
> 
> On this level:
>     additionalProperties: false
> 
>> +
>> +    properties:
>> +      "#nvmem-cell-cells":
>> +        const: 1
>> +
> 
> I also wonder why you do not have unit addresses. What if you want to
> have two base MAC addresses?

That would describe an offset within the nvmem device. But the offset
might not be constant, depending on the content. My understanding
so far was that in that case, you use the "-N" suffix.

base-mac-address-1
base-mac-address-2

(or maybe completely different names).

>> +required:
>> +  - compatible
> 
> Other fields are I guess required? At least serial-number should be 
> always?

Yes I could add them to the required list, but they are only
"required" if you need to reference them within the device tree, in 
which
case there have to be there anyway. IOW, the driver doesn't care if
there is a node. If there is none, it doesn't set the "struct 
device_node*"
in the nvmem cell.

>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +      otp-1 {
> 
> Messed up indentation (use 4 spaces). Generic node name "otp".
> 
>> +          compatible = "kontron,sl28-vpd", "user-otp";
>> +
>> +          serial_number: serial-number {
> 
> What's the point of the empty node?

See above.

-michael

>> +          };
>> +
>> +          base_mac_address: base-mac-address {
>> +              #nvmem-cell-cells = <1>;
>> +          };
>> +      };
>> +
>> +...
Krzysztof Kozlowski Aug. 31, 2022, 9:24 a.m. UTC | #4
On 31/08/2022 11:17, Michael Walle wrote:
> Am 2022-08-31 09:45, schrieb Krzysztof Kozlowski:
>> On 26/08/2022 00:44, Michael Walle wrote:
>>> Add a schema for the NVMEM layout on Kontron's sl28 boards.
>>>
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> ---
>>>  .../nvmem/layouts/kontron,sl28-vpd.yaml       | 52 
>>> +++++++++++++++++++
>>>  1 file changed, 52 insertions(+)
>>>  create mode 100644 
>>> Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml 
>>> b/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
>>> new file mode 100644
>>> index 000000000000..e4bc2d9182db
>>> --- /dev/null
>>> +++ 
>>> b/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
>>> @@ -0,0 +1,52 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: 
>>> http://devicetree.org/schemas/nvmem/layouts/kontron,sl28-vpd.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: NVMEM layout of the Kontron SMARC-sAL28 vital product data
>>> +
>>> +maintainers:
>>> +  - Michael Walle <michael@walle.cc>
>>> +
>>> +description:
>>> +  The vital product data (VPD) of the sl28 boards contains a serial
>>> +  number and a base MAC address. The actual MAC addresses for the
>>> +  on-board ethernet devices are derived from this base MAC address by
>>> +  adding an offset.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - const: kontron,sl28-vpd
>>> +      - const: user-otp
>>> +
>>> +  serial-number:
>>> +    type: object
>>
>> You should define the contents of this object. I would expect this to 
>> be
>> uint32 or string. I think you also need description, as this is not
>> really standard field.
> 
> First thing, this binding isn't like the usual ones, so it might be
> totally wrong.
> 
> What I'd like to achieve here is the following:
> 
> We have the nvmem-consumer dt binding where you can reference a
> nvmem cell in a consumer node. Example:
>    nvmem-cells = <&base_mac_address 5>;
>    nvmem-cell-names = "mac-address";
> 
> On the other end of the link we have the nvmem-provider. The dt
> bindings works well if that one has individual cell nodes, like
> it is described in the nvmem.yaml binding. I.e. you can give the
> cell a label and make a reference to it in the consumer just like
> in the example above.

You can also achieve it with phandle argument to the nvmwm controller,
right? Just like most of providers are doing (clocks, resets). Having
fake (empty) nodes just for that seems like overkill.

> 
> Now comes the catch: what if there is no actual description of the
> cell in the device tree, but is is generated during runtime. How
> can I get a label to it.

Same as clocks, resets, power-domains and everyone else.


> Therefore, in this case, there is just
> an empty node and the driver will associate it with the cell
> created during runtime (see patch 10). It is not expected, that
> is has any properties.

It cannot be even referenced as it does not have #cells property...

> 
>>> +
>>> +  base-mac-address:
>>
>> Fields should be rather described here, not in top-level description.
>>
>>> +    type: object
>>
>> On this level:
>>     additionalProperties: false
>>
>>> +
>>> +    properties:
>>> +      "#nvmem-cell-cells":
>>> +        const: 1
>>> +
>>
>> I also wonder why you do not have unit addresses. What if you want to
>> have two base MAC addresses?
> 
> That would describe an offset within the nvmem device. But the offset
> might not be constant, depending on the content. My understanding
> so far was that in that case, you use the "-N" suffix.
> 
> base-mac-address-1
> base-mac-address-2
> 
> (or maybe completely different names).

You do not allow "base-mac-address-1". Your binding explicitly accepts
only "base-mac-address".

Best regards,
Krzysztof
Michael Walle Aug. 31, 2022, 9:51 a.m. UTC | #5
Am 2022-08-31 11:24, schrieb Krzysztof Kozlowski:
> On 31/08/2022 11:17, Michael Walle wrote:

>> First thing, this binding isn't like the usual ones, so it might be
>> totally wrong.
>> 
>> What I'd like to achieve here is the following:
>> 
>> We have the nvmem-consumer dt binding where you can reference a
>> nvmem cell in a consumer node. Example:
>>    nvmem-cells = <&base_mac_address 5>;
>>    nvmem-cell-names = "mac-address";
>> 
>> On the other end of the link we have the nvmem-provider. The dt
>> bindings works well if that one has individual cell nodes, like
>> it is described in the nvmem.yaml binding. I.e. you can give the
>> cell a label and make a reference to it in the consumer just like
>> in the example above.
> 
> You can also achieve it with phandle argument to the nvmwm controller,
> right? Just like most of providers are doing (clocks, resets). Having
> fake (empty) nodes just for that seems like overkill.

You mean like
  nvmem-cells = <&nvmem_device SERIAL_NUMBER>;

I'm not sure about the implications for now, because one is
referencing the device and not individal cells. Putting that
aside for now, there seems to be a problem with the index for
the base mac address: You will have different number of arguments
for the phandle. That doesn't work, right?

nvmem-cells = <&nvmem_device SERIAL_NUMBER>;
nvmem-cells = <&nvmem_device BASE_MAC_ADDRESS 1>;

>> Now comes the catch: what if there is no actual description of the
>> cell in the device tree, but is is generated during runtime. How
>> can I get a label to it.
> 
> Same as clocks, resets, power-domains and everyone else.

See
https://git.kernel.org/torvalds/c/084973e944bec21804f8afb0515b25434438699a

And I guess this discussion is relevant here:
https://lore.kernel.org/linux-devicetree/20220124160300.25131-1-zajec5@gmail.com/

>> Therefore, in this case, there is just
>> an empty node and the driver will associate it with the cell
>> created during runtime (see patch 10). It is not expected, that
>> is has any properties.
> 
> It cannot be even referenced as it does not have #cells property...

You mean "#nvmem-cell-cells"? See patch #2. None of the nvmem
cells had such a property for now.

>>>> +
>>>> +  base-mac-address:
>>> 
>>> Fields should be rather described here, not in top-level description.
>>> 
>>>> +    type: object
>>> 
>>> On this level:
>>>     additionalProperties: false
>>> 
>>>> +
>>>> +    properties:
>>>> +      "#nvmem-cell-cells":
>>>> +        const: 1
>>>> +
>>> 
>>> I also wonder why you do not have unit addresses. What if you want to
>>> have two base MAC addresses?
>> 
>> That would describe an offset within the nvmem device. But the offset
>> might not be constant, depending on the content. My understanding
>> so far was that in that case, you use the "-N" suffix.
>> 
>> base-mac-address-1
>> base-mac-address-2
>> 
>> (or maybe completely different names).
> 
> You do not allow "base-mac-address-1". Your binding explicitly accepts
> only "base-mac-address".

Because the binding matches the driver, which matches the driver
which matches the VPD data and there is only one base mac address.
Thus, no need for different ones.

-michael
Krzysztof Kozlowski Aug. 31, 2022, 1:07 p.m. UTC | #6
On 31/08/2022 12:51, Michael Walle wrote:
> Am 2022-08-31 11:24, schrieb Krzysztof Kozlowski:
>> On 31/08/2022 11:17, Michael Walle wrote:
> 
>>> First thing, this binding isn't like the usual ones, so it might be
>>> totally wrong.
>>>
>>> What I'd like to achieve here is the following:
>>>
>>> We have the nvmem-consumer dt binding where you can reference a
>>> nvmem cell in a consumer node. Example:
>>>    nvmem-cells = <&base_mac_address 5>;
>>>    nvmem-cell-names = "mac-address";
>>>
>>> On the other end of the link we have the nvmem-provider. The dt
>>> bindings works well if that one has individual cell nodes, like
>>> it is described in the nvmem.yaml binding. I.e. you can give the
>>> cell a label and make a reference to it in the consumer just like
>>> in the example above.
>>
>> You can also achieve it with phandle argument to the nvmwm controller,
>> right? Just like most of providers are doing (clocks, resets). Having
>> fake (empty) nodes just for that seems like overkill.
> 
> You mean like
>   nvmem-cells = <&nvmem_device SERIAL_NUMBER>;

Yes.

> 
> I'm not sure about the implications for now, because one is
> referencing the device and not individal cells. Putting that
> aside for now, there seems to be a problem with the index for
> the base mac address: You will have different number of arguments
> for the phandle. That doesn't work, right?
> 
> nvmem-cells = <&nvmem_device SERIAL_NUMBER>;
> nvmem-cells = <&nvmem_device BASE_MAC_ADDRESS 1>;

It could work, but looks poor, however it could be still nicely extended
with new defines and renames later:

Once:
nvmem-cells = <&nvmem_device BASE_MAC_ADDRESS>;

Later renamed to (with some ABI impact, but in theory names are not part
of ABI, but numbers are):
nvmem-cells = <&nvmem_device BASE_MAC_ADDRESS_1>;
nvmem-cells = <&nvmem_device BASE_MAC_ADDRESS_2>;

(or even skip renaming and just add suffix _2)

You cannot rename device nodes, without deprecating them.

> 
>>> Now comes the catch: what if there is no actual description of the
>>> cell in the device tree, but is is generated during runtime. How
>>> can I get a label to it.
>>
>> Same as clocks, resets, power-domains and everyone else.
> 
> See
> https://git.kernel.org/torvalds/c/084973e944bec21804f8afb0515b25434438699a
> 
> And I guess this discussion is relevant here:
> https://lore.kernel.org/linux-devicetree/20220124160300.25131-1-zajec5@gmail.com/

Eh, ok, I jumped in the middle of something and seems Rob was fine with
these empty nodes. Looks weird and overkill too me (imagine defining 500
clocks in clock-controller like that), but I am here still learning. :)

I guess it makes sense for the cases when OTP/nvmem cells are not
controller specific, not fixed for given OTP controller but rather board
specific and having defined them in header would not make sense.

But then if they are strictly/statically defined as children of a
device, means they are fixed for given OTP and effort is the same as
having them in header...

> 
>>> Therefore, in this case, there is just
>>> an empty node and the driver will associate it with the cell
>>> created during runtime (see patch 10). It is not expected, that
>>> is has any properties.
>>
>> It cannot be even referenced as it does not have #cells property...
> 
> You mean "#nvmem-cell-cells"? See patch #2. None of the nvmem
> cells had such a property for now.

Oh, so so how do you reference them? Users of this seems to be missing,
so I am guessing that directly via phandle to label and nvmem maps them
 with nvmem_find_cell_of_node()?

> 
>>>>> +
>>>>> +  base-mac-address:
>>>>
>>>> Fields should be rather described here, not in top-level description.
>>>>
>>>>> +    type: object
>>>>
>>>> On this level:
>>>>     additionalProperties: false
>>>>
>>>>> +
>>>>> +    properties:
>>>>> +      "#nvmem-cell-cells":
>>>>> +        const: 1
>>>>> +
>>>>
>>>> I also wonder why you do not have unit addresses. What if you want to
>>>> have two base MAC addresses?
>>>
>>> That would describe an offset within the nvmem device. But the offset
>>> might not be constant, depending on the content. My understanding
>>> so far was that in that case, you use the "-N" suffix.
>>>
>>> base-mac-address-1
>>> base-mac-address-2
>>>
>>> (or maybe completely different names).
>>
>> You do not allow "base-mac-address-1". Your binding explicitly accepts
>> only "base-mac-address".
> 
> Because the binding matches the driver, which matches the driver
> which matches the VPD data and there is only one base mac address.
> Thus, no need for different ones.

True, but it is also not extensible, so you have to be sure you covered
100% of this device. And then if you have a new, slightly different
device, you need entirely new schema, because this one is not reusable
at all.

It's ok, it just has some drawbacks/limitations.

Best regards,
Krzysztof
Michael Walle Aug. 31, 2022, 3:29 p.m. UTC | #7
Am 2022-08-31 15:07, schrieb Krzysztof Kozlowski:
> On 31/08/2022 12:51, Michael Walle wrote:
>> Am 2022-08-31 11:24, schrieb Krzysztof Kozlowski:
>>> On 31/08/2022 11:17, Michael Walle wrote:
>> 
>>>> First thing, this binding isn't like the usual ones, so it might be
>>>> totally wrong.
>>>> 
>>>> What I'd like to achieve here is the following:
>>>> 
>>>> We have the nvmem-consumer dt binding where you can reference a
>>>> nvmem cell in a consumer node. Example:
>>>>    nvmem-cells = <&base_mac_address 5>;
>>>>    nvmem-cell-names = "mac-address";
>>>> 
>>>> On the other end of the link we have the nvmem-provider. The dt
>>>> bindings works well if that one has individual cell nodes, like
>>>> it is described in the nvmem.yaml binding. I.e. you can give the
>>>> cell a label and make a reference to it in the consumer just like
>>>> in the example above.
>>> 
>>> You can also achieve it with phandle argument to the nvmwm 
>>> controller,
>>> right? Just like most of providers are doing (clocks, resets). Having
>>> fake (empty) nodes just for that seems like overkill.
>> 
>> You mean like
>>   nvmem-cells = <&nvmem_device SERIAL_NUMBER>;
> 
> Yes.
> 
>> 
>> I'm not sure about the implications for now, because one is
>> referencing the device and not individal cells. Putting that
>> aside for now, there seems to be a problem with the index for
>> the base mac address: You will have different number of arguments
>> for the phandle. That doesn't work, right?
>> 
>> nvmem-cells = <&nvmem_device SERIAL_NUMBER>;
>> nvmem-cells = <&nvmem_device BASE_MAC_ADDRESS 1>;
> 
> It could work, but looks poor, however it could be still nicely 
> extended
> with new defines and renames later:
> 
> Once:
> nvmem-cells = <&nvmem_device BASE_MAC_ADDRESS>;
> 
> Later renamed to (with some ABI impact, but in theory names are not 
> part
> of ABI, but numbers are):
> nvmem-cells = <&nvmem_device BASE_MAC_ADDRESS_1>;
> nvmem-cells = <&nvmem_device BASE_MAC_ADDRESS_2>;

Ah I think I didn't express correctly what I want to achieve:

nvmem-cells = <&nvmem_device BASE_MAC_ADDRESS 1>;

means "take base mac address and add one to it". the first
argument is treated as the offset. i.e.
   nvmem-cells = <&nvmem_device BASE_MAC_ADDRESS 5>;
would be "base_mac + 5". It is not another base mac address.

whereas, the serial number doesn't take an argument.

I think it could be achieved with some preprocessor magic
like:
#define SL28VPD_SERIAL_NUMBER   (0 << 8)
#define SL28VPD_MAC_ADDRESS(x)  (1 << 8) + (x & 0xff)
#define SL28VPD_ANOTHER_THING   (2 << 8)

I'm not sure if that is any better.

Also the whole binding around nvmem is that it references the cells,
not the device. So if you insist on using the phandle to the
nvmem device, I'll really have to check if that is something which
could be done. I.e. have a look at
   
https://elixir.bootlin.com/linux/latest/source/drivers/nvmem/core.c#L1242

It assumes the reference phandle points to a cell and fetches
the parent to get the nvmem device. We could try to get a device
by the current handle and if that succeeds use that one. But I
*think* it doesn't work with the EPROBE_DEFERRED. I.e. the device
might not be there, because it wasn't probed yet.

> (or even skip renaming and just add suffix _2)
> 
> You cannot rename device nodes, without deprecating them.
> 
>> 
>>>> Now comes the catch: what if there is no actual description of the
>>>> cell in the device tree, but is is generated during runtime. How
>>>> can I get a label to it.
>>> 
>>> Same as clocks, resets, power-domains and everyone else.
>> 
>> See
>> https://git.kernel.org/torvalds/c/084973e944bec21804f8afb0515b25434438699a
>> 
>> And I guess this discussion is relevant here:
>> https://lore.kernel.org/linux-devicetree/20220124160300.25131-1-zajec5@gmail.com/
> 
> Eh, ok, I jumped in the middle of something and seems Rob was fine with
> these empty nodes. Looks weird and overkill too me (imagine defining 
> 500
> clocks in clock-controller like that), but I am here still learning. :)
> 
> I guess it makes sense for the cases when OTP/nvmem cells are not
> controller specific, not fixed for given OTP controller but rather 
> board
> specific and having defined them in header would not make sense.
> 
> But then if they are strictly/statically defined as children of a
> device, means they are fixed for given OTP and effort is the same as
> having them in header...
> 
>>>> Therefore, in this case, there is just
>>>> an empty node and the driver will associate it with the cell
>>>> created during runtime (see patch 10). It is not expected, that
>>>> is has any properties.
>>> 
>>> It cannot be even referenced as it does not have #cells property...
>> 
>> You mean "#nvmem-cell-cells"? See patch #2. None of the nvmem
>> cells had such a property for now.
> 
> Oh, so so how do you reference them? Users of this seems to be missing,
> so I am guessing that directly via phandle to label and nvmem maps them
>  with nvmem_find_cell_of_node()?

Mh? there are plenty of users which references a nvmem cell. E.g.
https://elixir.bootlin.com/linux/v5.19.5/source/arch/arm64/boot/dts/freescale/imx8mm.dtsi#L76
https://elixir.bootlin.com/linux/v5.19.5/source/arch/arm64/boot/dts/freescale/imx8mm.dtsi#L1080

The entry point for the "fetch mac address" is here:
https://elixir.bootlin.com/linux/v5.19.5/source/net/ethernet/eth.c#L550

And the relevant functions is:
https://elixir.bootlin.com/linux/v5.19.5/source/drivers/nvmem/core.c#L1226
which uses of_parse_phandle(), which don't need the "#-cells" property, 
but
is limited to no arguments. Thus the new function in patch #2.

>>>>>> +
>>>>>> +  base-mac-address:
>>>>> 
>>>>> Fields should be rather described here, not in top-level 
>>>>> description.
>>>>> 
>>>>>> +    type: object
>>>>> 
>>>>> On this level:
>>>>>     additionalProperties: false
>>>>> 
>>>>>> +
>>>>>> +    properties:
>>>>>> +      "#nvmem-cell-cells":
>>>>>> +        const: 1
>>>>>> +
>>>>> 
>>>>> I also wonder why you do not have unit addresses. What if you want 
>>>>> to
>>>>> have two base MAC addresses?
>>>> 
>>>> That would describe an offset within the nvmem device. But the 
>>>> offset
>>>> might not be constant, depending on the content. My understanding
>>>> so far was that in that case, you use the "-N" suffix.
>>>> 
>>>> base-mac-address-1
>>>> base-mac-address-2
>>>> 
>>>> (or maybe completely different names).
>>> 
>>> You do not allow "base-mac-address-1". Your binding explicitly 
>>> accepts
>>> only "base-mac-address".
>> 
>> Because the binding matches the driver, which matches the driver
>> which matches the VPD data and there is only one base mac address.
>> Thus, no need for different ones.
> 
> True, but it is also not extensible, so you have to be sure you covered
> 100% of this device. And then if you have a new, slightly different
> device, you need entirely new schema, because this one is not reusable
> at all.
> 
> It's ok, it just has some drawbacks/limitations.

But isn't that the whole thing around the specific compatible?
And you can still add subnodes if there is a new version of
the vpd. The driver will figure that out and add the cells.

-michael
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml b/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
new file mode 100644
index 000000000000..e4bc2d9182db
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
@@ -0,0 +1,52 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/layouts/kontron,sl28-vpd.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NVMEM layout of the Kontron SMARC-sAL28 vital product data
+
+maintainers:
+  - Michael Walle <michael@walle.cc>
+
+description:
+  The vital product data (VPD) of the sl28 boards contains a serial
+  number and a base MAC address. The actual MAC addresses for the
+  on-board ethernet devices are derived from this base MAC address by
+  adding an offset.
+
+properties:
+  compatible:
+    items:
+      - const: kontron,sl28-vpd
+      - const: user-otp
+
+  serial-number:
+    type: object
+
+  base-mac-address:
+    type: object
+
+    properties:
+      "#nvmem-cell-cells":
+        const: 1
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+      otp-1 {
+          compatible = "kontron,sl28-vpd", "user-otp";
+
+          serial_number: serial-number {
+          };
+
+          base_mac_address: base-mac-address {
+              #nvmem-cell-cells = <1>;
+          };
+      };
+
+...