On 31/08/2022 12:51, Michael Walle wrote:

Am 2022-08-31 11:24, schrieb Krzysztof Kozlowski:

quoted

On 31/08/2022 11:17, Michael Walle wrote:

quoted

quoted

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.

quoted

quoted

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/ (local)

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

quoted

quoted

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()?

quoted

quoted

quoted

quoted

+
+  base-mac-address:

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

quoted

+    type: object

On this level:
    additionalProperties: false

quoted

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