Message ID | 20211123134425.3875656-1-michael@walle.cc |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | [RFC] dt-bindings: nvmem: add transformation support | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | warning | total: 0 errors, 1 warnings, 46 lines checked |
robh/dt-meta-schema | fail | build log |
On Tue, 23 Nov 2021 14:44:25 +0100, Michael Walle wrote: > 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. > > The first proposal [1] didn't find much appreciation and there wasn't > any reply to my question or new proposal [2]. So here we are with my new > proposal, that is more flexible and doesn't fix the ethernet mac only. > This is just an RFC for the device tree representation for now to see if > this is the correct way to tackle this. > > I'm also aware of the latest post process hook support [3]. This doesn't > fix the base mac address issue, but I think it also doesn't solve the > case with swapped ethernet addresses in the general case. That hook will > involve the driver to do the swapping, but how would the driver know > if that swapping is actually required. Usually the interpretation of the > content is opaque to the driver, after all it is the user/board > manufacturer who does program the storage device. We might be lucky in > the imx-ocotp case because the IMX reference manual actually states > where and in which format the mac address is programmed. > > Introduce a transformation property. This is intended to be just an > enumeration of operations. If there will be a new operation, support for > it has to be added to the nvmem core. > > A transformation might 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 into a list of values. > > Example: > mac_addresses: base-mac-address@10 { > #nvmem-cell-cells = <1>; > reg = <10 6>; > transformation = <NVMEM_T_ETH_OFFSET 0 1 7>; > } > > ð0 { > nvmem-cells = <&mac_addresses 0>; > nvmem-cell-names = "mac-address"; > }; > > ð1 { > nvmem-cells = <&mac_addresses 2>; > nvmem-cell-names = "mac-address"; > }; > > The NVMEM_T_ETH_OFFSET transformation takes N additional (dt) cells and > will generate N values. In this example BASE_MAC+0, BASE_MAC+1, BASE_MAC+7. > An nvmem consumer can then reference the nvmem cell with an index. So eth0 > will get BASE_MAC+0 and eth1 will get BASE_MAC+7. > > This should be sufficient flexible for many different transformations > without having to touch the bindings except for adding documentation and > checks for new transformations. > > I do have one question regarding "#nvmem-cell-cells" (aside from the > awkward naming): is it allowed to have that property optional if there > is no additional argument to the phandle? > > [1] https://lore.kernel.org/all/20210414152657.12097-2-michael@walle.cc/ > [2] https://lore.kernel.org/linux-devicetree/362f1c6a8b0ec191b285ac6a604500da@walle.cc/ > [3] https://lore.kernel.org/lkml/20211013131957.30271-1-srinivas.kandagatla@linaro.org/ > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > .../devicetree/bindings/nvmem/nvmem.yaml | 29 +++++++++++++++++++ > 1 file changed, 29 insertions(+) > 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: Error: Documentation/devicetree/bindings/nvmem/nvmem.example.dts:53.35-36 syntax error FATAL ERROR: Unable to parse input tree make[1]: *** [scripts/Makefile.lib:373: Documentation/devicetree/bindings/nvmem/nvmem.example.dt.yaml] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1413: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1558545 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.
Hi Michael, On 23/11/2021 13:44, Michael Walle wrote: > 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. > > The first proposal [1] didn't find much appreciation and there wasn't > any reply to my question or new proposal [2]. So here we are with my new > proposal, that is more flexible and doesn't fix the ethernet mac only. > This is just an RFC for the device tree representation for now to see if > this is the correct way to tackle this. > > I'm also aware of the latest post process hook support [3]. This doesn't > fix the base mac address issue, but I think it also doesn't solve the > case with swapped ethernet addresses in the general case. That hook will > involve the driver to do the swapping, but how would the driver know > if that swapping is actually required. Usually the interpretation of the > content is opaque to the driver, after all it is the user/board But this is the path for any post processing hook, which ever direction we endup with(using core helpers or provider specific post-processing). > manufacturer who does program the storage device. We might be lucky in > the imx-ocotp case because the IMX reference manual actually states > where and in which format the mac address is programmed. > > Introduce a transformation property. This is intended to be just an > enumeration of operations. If there will be a new operation, support for > it has to be added to the nvmem core. > > A transformation might 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 into a list of values. > > Example: > mac_addresses: base-mac-address@10 { > #nvmem-cell-cells = <1>; > reg = <10 6>; > transformation = <NVMEM_T_ETH_OFFSET 0 1 7>; IMO, this is totally redundant. we could probably encode this info directly in the cell specifiers, something like: > } > > ð0 { > nvmem-cells = <&mac_addresses 0>; nvmem-cells = <&base_mac_addresses NVMEM_T_ETH_OFFSET 0>; And value of #nvmem-cell-cells is dependent on the first cell specifier. I understand that these 3 bits of info is required for this type of post processing and this can only come from DT and its not possible to determine this at runtime. Would this address other usecases? Are you in a position to test few of them? Lets wait for Rob's opinion on adding #nvmem-cell-cells property with cell specifiers describing the encoding information? --srini > nvmem-cell-names = "mac-address"; > }; > > ð1 { > nvmem-cells = <&mac_addresses 2>; > nvmem-cell-names = "mac-address"; > }; > > The NVMEM_T_ETH_OFFSET transformation takes N additional (dt) cells and > will generate N values. In this example BASE_MAC+0, BASE_MAC+1, BASE_MAC+7. > An nvmem consumer can then reference the nvmem cell with an index. So eth0 > will get BASE_MAC+0 and eth1 will get BASE_MAC+7. > > This should be sufficient flexible for many different transformations > without having to touch the bindings except for adding documentation and > checks for new transformations. > > I do have one question regarding "#nvmem-cell-cells" (aside from the > awkward naming): is it allowed to have that property optional if there > is no additional argument to the phandle? > > [1] https://lore.kernel.org/all/20210414152657.12097-2-michael@walle.cc/ > [2] https://lore.kernel.org/linux-devicetree/362f1c6a8b0ec191b285ac6a604500da@walle.cc/ > [3] https://lore.kernel.org/lkml/20211013131957.30271-1-srinivas.kandagatla@linaro.org/ > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > .../devicetree/bindings/nvmem/nvmem.yaml | 29 +++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml > index 456fb808100a..8893d045be77 100644 > --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml > @@ -26,11 +26,34 @@ properties: > "#size-cells": > const: 1 > > + '#nvmem-cell-cells': > + enum: [0, 1] > + description: > + Must be 1 if the transformations has multiple output values. > + The argument is then the index into the list of output values. > + For example, if the nvmem cell only specify a base ethernet > + address the transformation can then create different individual > + ethernet addresses. > + > read-only: > $ref: /schemas/types.yaml#/definitions/flag > description: > Mark the provider as read only. > > + transformation: > + description: > + Transform the content of a NVMEM cell. Sometimes it is necessary > + to preprocess the content of a cell so it is usable by the NVMEM > + consumer. There are also cases where one NVMEM cell value can > + generate a list of values. > + > + Use one of the NVMEM_T_* prefixed definitions from the header > + include/dt-bindings/nvmem/nvmem.h. > + > + Some transformations might have additional arguments. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + > wp-gpios: > description: > GPIO to which the write-protect pin of the chip is connected. > @@ -98,6 +121,12 @@ examples: > reg = <0xc 0x1>; > bits = <2 3>; > }; > + > + ethernet_base_mac: base-mac-address@100 { > + #nvmem-cell-cells = <1>; > + reg = <0x100 0x6>; > + transformation = <NVMEM_T_ETH_OFFSET 0 1 2 7>; > + }; > }; > > ... >
Hi Srinivas, Am 2021-11-29 13:54, schrieb Srinivas Kandagatla: > On 23/11/2021 13:44, Michael Walle wrote: >> 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. >> >> The first proposal [1] didn't find much appreciation and there wasn't >> any reply to my question or new proposal [2]. So here we are with my >> new >> proposal, that is more flexible and doesn't fix the ethernet mac only. >> This is just an RFC for the device tree representation for now to see >> if >> this is the correct way to tackle this. >> >> I'm also aware of the latest post process hook support [3]. This >> doesn't >> fix the base mac address issue, but I think it also doesn't solve the >> case with swapped ethernet addresses in the general case. That hook >> will >> involve the driver to do the swapping, but how would the driver know >> if that swapping is actually required. Usually the interpretation of >> the >> content is opaque to the driver, after all it is the user/board > > But this is the path for any post processing hook, which ever > direction we endup with(using core helpers or provider specific > post-processing). Mh? I don't understand. My point was that the driver is unlikely to know what it should process. Take the mtd (or the mtd otp) nvmem provider for example. If I understand it correctly, it just gets the nvmem name, for example, "mac-address". How should the post process hook know, what it should do? IMHO that just works for very specific drivers, which tied to the content they provide. >> manufacturer who does program the storage device. We might be lucky in >> the imx-ocotp case because the IMX reference manual actually states >> where and in which format the mac address is programmed. >> >> Introduce a transformation property. This is intended to be just an >> enumeration of operations. If there will be a new operation, support >> for >> it has to be added to the nvmem core. >> >> A transformation might 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 into a list of values. >> >> Example: >> mac_addresses: base-mac-address@10 { >> #nvmem-cell-cells = <1>; >> reg = <10 6>; >> transformation = <NVMEM_T_ETH_OFFSET 0 1 7>; > > IMO, this is totally redundant. we could probably encode this info > directly in the cell specifiers, something like: > >> } >> >> ð0 { >> nvmem-cells = <&mac_addresses 0>; > > nvmem-cells = <&base_mac_addresses NVMEM_T_ETH_OFFSET 0>; I had he same idea, but keep in mind, that there could be more than just one nvmem cells: nvmem-cells = <&phandle1 arg1 &pandle2 arg2 arg3>; nvmem-cell-names = "name1", "name2"; So you'd need the #nvmem-cell-cells either way. > And value of #nvmem-cell-cells is dependent on the first cell > specifier. What do you mean with first cell specifier? the phandle (base_mac_address in the example) or the NVMEM_T_ETH_OFFSET? I guess the latter, because the arguments depend on the transformation. But this is not how the of_parse_phandle_with_args() works, it will look the '#nvmem-cell-cells' property up, to see how many arguments it should expect, which is a property to the referenced node. Thus I've come up with the additional indirection. The number of arguments for the reference cell is either 0 or 1 and the transformation is part of the nvmem cell. > I understand that these 3 bits of info is required for this type of > post processing and this can only come from DT and its not possible to > determine this at runtime. ok :) > Would this address other usecases? I think so, but see above for why it can't work. Or I am missing something. > Are you in a position to test few of them? Sure (at least after my vacation). And TBH I think the imxotp mac swap should use the same or it will be likely that there are future SoCs which will always swap the ethnet > Lets wait for Rob's opinion on adding #nvmem-cell-cells property with > cell specifiers describing the encoding information? +1 Thanks for looking into this, -michael
On 29/11/2021 17:43, Michael Walle wrote: > Hi Srinivas, > > Am 2021-11-29 13:54, schrieb Srinivas Kandagatla: >> On 23/11/2021 13:44, Michael Walle wrote: >>> 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. >>> >>> The first proposal [1] didn't find much appreciation and there wasn't >>> any reply to my question or new proposal [2]. So here we are with my new >>> proposal, that is more flexible and doesn't fix the ethernet mac only. >>> This is just an RFC for the device tree representation for now to see if >>> this is the correct way to tackle this. >>> >>> I'm also aware of the latest post process hook support [3]. This doesn't >>> fix the base mac address issue, but I think it also doesn't solve the >>> case with swapped ethernet addresses in the general case. That hook will >>> involve the driver to do the swapping, but how would the driver know >>> if that swapping is actually required. Usually the interpretation of the >>> content is opaque to the driver, after all it is the user/board >> >> But this is the path for any post processing hook, which ever >> direction we endup with(using core helpers or provider specific >> post-processing). > > Mh? I don't understand. My point was that the driver is unlikely > to know what it should process. Take the mtd (or the mtd otp) What if the drivers know what it should do for post processing? TBH, all the post processing is provider centric, Its hard to really standardize this for every possible encoding that vendor programs into there nvmem. There is no standardization here that can go in to nvmem core. My approach for this would be to use the same callback hook. Either set this at provider driver level or at core level. > nvmem provider for example. If I understand it correctly, it just > gets the nvmem name, for example, "mac-address". How should > the post process hook know, what it should do? IMHO that just > works for very specific drivers, which tied to the content > they provide. Currently the callback hook is just dealing with names but it can be extended to support other arguments. > >>> manufacturer who does program the storage device. We might be lucky in >>> the imx-ocotp case because the IMX reference manual actually states >>> where and in which format the mac address is programmed. >>> >>> Introduce a transformation property. This is intended to be just an >>> enumeration of operations. If there will be a new operation, support for >>> it has to be added to the nvmem core. >>> >>> A transformation might 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 into a list of values. >>> >>> Example: >>> mac_addresses: base-mac-address@10 { >>> #nvmem-cell-cells = <1>; >>> reg = <10 6>; >>> transformation = <NVMEM_T_ETH_OFFSET 0 1 7>; >> >> IMO, this is totally redundant. we could probably encode this info >> directly in the cell specifiers, something like: >> >>> } >>> >>> ð0 { >>> nvmem-cells = <&mac_addresses 0>; >> >> nvmem-cells = <&base_mac_addresses NVMEM_T_ETH_OFFSET 0>; > > I had he same idea, but keep in mind, that there could be more > than just one nvmem cells: > > nvmem-cells = <&phandle1 arg1 &pandle2 arg2 arg3>; > nvmem-cell-names = "name1", "name2"; > AFAIU, This should just work. > So you'd need the #nvmem-cell-cells either way. > >> And value of #nvmem-cell-cells is dependent on the first cell specifier. That does not sound correct. You can see lots of example in dt that have phandles with different number of arguments. AFAIU, both phandle1 and phandle2 will have different #nvmem-cell-cells values specified in there dt nodes. --srini > > What do you mean with first cell specifier? the phandle (base_mac_address > in the example) or the NVMEM_T_ETH_OFFSET? I guess the latter, because the > arguments depend on the transformation. But this is not how the > of_parse_phandle_with_args() works, it will look the '#nvmem-cell-cells' > property up, to see how many arguments it should expect, which is ann > property to the referenced node. Thus I've come up with the additional > indirection. The number of arguments for the reference cell is either > 0 or 1 and the transformation is part of the nvmem cell. > >> I understand that these 3 bits of info is required for this type of >> post processing and this can only come from DT and its not possible to >> determine this at runtime. > > ok :) > >> Would this address other usecases? > > I think so, but see above for why it can't work. Or I am missing > something. > >> Are you in a position to test few of them? > > Sure (at least after my vacation). And TBH I think the imxotp mac > swap should use the same or it will be likely that there are future > SoCs which will always swap the ethnet > >> Lets wait for Rob's opinion on adding #nvmem-cell-cells property with >> cell specifiers describing the encoding information? > > +1 > > Thanks for looking into this, > -michael
On Tue, Nov 23, 2021 at 02:44:25PM +0100, Michael Walle wrote: > 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. > > The first proposal [1] didn't find much appreciation and there wasn't > any reply to my question or new proposal [2]. So here we are with my new > proposal, that is more flexible and doesn't fix the ethernet mac only. > This is just an RFC for the device tree representation for now to see if > this is the correct way to tackle this. > > I'm also aware of the latest post process hook support [3]. This doesn't > fix the base mac address issue, but I think it also doesn't solve the > case with swapped ethernet addresses in the general case. That hook will > involve the driver to do the swapping, but how would the driver know > if that swapping is actually required. Usually the interpretation of the > content is opaque to the driver, after all it is the user/board > manufacturer who does program the storage device. We might be lucky in > the imx-ocotp case because the IMX reference manual actually states > where and in which format the mac address is programmed. A compatible string can define what is the format of the data. > Introduce a transformation property. This is intended to be just an > enumeration of operations. If there will be a new operation, support for > it has to be added to the nvmem core. > > A transformation might 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 into a list of values. I still don't think trying to encode transformations of data into the DT is right approach. > > Example: > mac_addresses: base-mac-address@10 { > #nvmem-cell-cells = <1>; > reg = <10 6>; > transformation = <NVMEM_T_ETH_OFFSET 0 1 7>; > } > > ð0 { > nvmem-cells = <&mac_addresses 0>; > nvmem-cell-names = "mac-address"; > }; > > ð1 { > nvmem-cells = <&mac_addresses 2>; > nvmem-cell-names = "mac-address"; > }; > > The NVMEM_T_ETH_OFFSET transformation takes N additional (dt) cells and > will generate N values. In this example BASE_MAC+0, BASE_MAC+1, BASE_MAC+7. > An nvmem consumer can then reference the nvmem cell with an index. So eth0 > will get BASE_MAC+0 and eth1 will get BASE_MAC+7. > > This should be sufficient flexible for many different transformations > without having to touch the bindings except for adding documentation and > checks for new transformations. The content and number of cells is supposed to be opaque to the client and interpreted by the provider. That's sort of true here, but not really because the interpretation is tied to 'transformation'. So I'm okay with adding cells, but not fixing the interpretation of them. A compatible should determine how the cells are interpreted. > I do have one question regarding "#nvmem-cell-cells" (aside from the > awkward naming): is it allowed to have that property optional if there > is no additional argument to the phandle? We don't have any choice if we add "#nvmem-cell-cells". There's already cases without it. > > [1] https://lore.kernel.org/all/20210414152657.12097-2-michael@walle.cc/ > [2] https://lore.kernel.org/linux-devicetree/362f1c6a8b0ec191b285ac6a604500da@walle.cc/ > [3] https://lore.kernel.org/lkml/20211013131957.30271-1-srinivas.kandagatla@linaro.org/ > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > .../devicetree/bindings/nvmem/nvmem.yaml | 29 +++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml > index 456fb808100a..8893d045be77 100644 > --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml > @@ -26,11 +26,34 @@ properties: > "#size-cells": > const: 1 > > + '#nvmem-cell-cells': > + enum: [0, 1] > + description: > + Must be 1 if the transformations has multiple output values. > + The argument is then the index into the list of output values. > + For example, if the nvmem cell only specify a base ethernet > + address the transformation can then create different individual > + ethernet addresses. > + > read-only: > $ref: /schemas/types.yaml#/definitions/flag > description: > Mark the provider as read only. > > + transformation: > + description: > + Transform the content of a NVMEM cell. Sometimes it is necessary > + to preprocess the content of a cell so it is usable by the NVMEM > + consumer. There are also cases where one NVMEM cell value can > + generate a list of values. > + > + Use one of the NVMEM_T_* prefixed definitions from the header > + include/dt-bindings/nvmem/nvmem.h. > + > + Some transformations might have additional arguments. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > + > wp-gpios: > description: > GPIO to which the write-protect pin of the chip is connected. > @@ -98,6 +121,12 @@ examples: > reg = <0xc 0x1>; > bits = <2 3>; > }; > + > + ethernet_base_mac: base-mac-address@100 { > + #nvmem-cell-cells = <1>; > + reg = <0x100 0x6>; > + transformation = <NVMEM_T_ETH_OFFSET 0 1 2 7>; > + }; > }; > > ... > -- > 2.30.2 > >
Hi Rob, Am 2021-11-30 20:19, schrieb Rob Herring: > On Tue, Nov 23, 2021 at 02:44:25PM +0100, Michael Walle wrote: .. >> Introduce a transformation property. This is intended to be just an >> enumeration of operations. If there will be a new operation, support >> for >> it has to be added to the nvmem core. >> >> A transformation might 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 into a list of values. > > I still don't think trying to encode transformations of data into the > DT > is right approach. > >> >> Example: >> mac_addresses: base-mac-address@10 { >> #nvmem-cell-cells = <1>; >> reg = <10 6>; >> transformation = <NVMEM_T_ETH_OFFSET 0 1 7>; >> } >> >> ð0 { >> nvmem-cells = <&mac_addresses 0>; >> nvmem-cell-names = "mac-address"; >> }; >> >> ð1 { >> nvmem-cells = <&mac_addresses 2>; >> nvmem-cell-names = "mac-address"; >> }; >> >> The NVMEM_T_ETH_OFFSET transformation takes N additional (dt) cells >> and >> will generate N values. In this example BASE_MAC+0, BASE_MAC+1, >> BASE_MAC+7. >> An nvmem consumer can then reference the nvmem cell with an index. So >> eth0 >> will get BASE_MAC+0 and eth1 will get BASE_MAC+7. >> >> This should be sufficient flexible for many different transformations >> without having to touch the bindings except for adding documentation >> and >> checks for new transformations. > > The content and number of cells is supposed to be opaque to the client > and interpreted by the provider. That's sort of true here, but not > really because the interpretation is tied to 'transformation'. So I'm > okay with adding cells, but not fixing the interpretation of them. A > compatible should determine how the cells are interpreted. What do you mean by "adding cells"? The additional argument to the phandle? So an example would be: ethernet_base_mac: base-mac-address@100 { #nvmem-cell-cells = <1>; compatible = "nvmem-ethernet-address"; reg = <0x100 0x6>; }; ð0 { nvmem-cells = <ðernet_base_mac 0>; nvmem-cell-names = "mac-address"; }; ð1 { nvmem-cells = <ðernet_base_mac 7>; nvmem-cell-names = "mac-address"; }; Right? Any suggestions for a better compatible name? >> I do have one question regarding "#nvmem-cell-cells" (aside from the >> awkward naming): is it allowed to have that property optional if there >> is no additional argument to the phandle? > > We don't have any choice if we add "#nvmem-cell-cells". There's already > cases without it. Yes, that was the reason for the question. But I wasn't sure, whether that is allowed. -michael
On Wed, Dec 1, 2021 at 8:30 AM Michael Walle <michael@walle.cc> wrote: > > Hi Rob, > > Am 2021-11-30 20:19, schrieb Rob Herring: > > On Tue, Nov 23, 2021 at 02:44:25PM +0100, Michael Walle wrote: > .. > > >> Introduce a transformation property. This is intended to be just an > >> enumeration of operations. If there will be a new operation, support > >> for > >> it has to be added to the nvmem core. > >> > >> A transformation might 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 into a list of values. > > > > I still don't think trying to encode transformations of data into the > > DT > > is right approach. > > > >> > >> Example: > >> mac_addresses: base-mac-address@10 { > >> #nvmem-cell-cells = <1>; > >> reg = <10 6>; > >> transformation = <NVMEM_T_ETH_OFFSET 0 1 7>; > >> } > >> > >> ð0 { > >> nvmem-cells = <&mac_addresses 0>; > >> nvmem-cell-names = "mac-address"; > >> }; > >> > >> ð1 { > >> nvmem-cells = <&mac_addresses 2>; > >> nvmem-cell-names = "mac-address"; > >> }; > >> > >> The NVMEM_T_ETH_OFFSET transformation takes N additional (dt) cells > >> and > >> will generate N values. In this example BASE_MAC+0, BASE_MAC+1, > >> BASE_MAC+7. > >> An nvmem consumer can then reference the nvmem cell with an index. So > >> eth0 > >> will get BASE_MAC+0 and eth1 will get BASE_MAC+7. > >> > >> This should be sufficient flexible for many different transformations > >> without having to touch the bindings except for adding documentation > >> and > >> checks for new transformations. > > > > The content and number of cells is supposed to be opaque to the client > > and interpreted by the provider. That's sort of true here, but not > > really because the interpretation is tied to 'transformation'. So I'm > > okay with adding cells, but not fixing the interpretation of them. A > > compatible should determine how the cells are interpreted. > > What do you mean by "adding cells"? The additional argument to the > phandle? Yes. > > So an example would be: > > ethernet_base_mac: base-mac-address@100 { > #nvmem-cell-cells = <1>; > compatible = "nvmem-ethernet-address"; > reg = <0x100 0x6>; > }; > > ð0 { > nvmem-cells = <ðernet_base_mac 0>; > nvmem-cell-names = "mac-address"; > }; > > ð1 { > nvmem-cells = <ðernet_base_mac 7>; > nvmem-cell-names = "mac-address"; > }; > > Right? Any suggestions for a better compatible name? I think the compatible should be something platform specific (or specific to whatever entity defined the format (SoC vendor, OEM, s/w platform (e.g. OpenWRT, u-boot, etc.)) and perhaps up a level defining the whole nvmem region. Then the handler for that compatible can select whatever 'generic' mac address transformation it wants or implement its own custom one. IOW, specific compatibles can use generic implementations rather than generic compatibles using a generic implementation. Rob
diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml index 456fb808100a..8893d045be77 100644 --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml @@ -26,11 +26,34 @@ properties: "#size-cells": const: 1 + '#nvmem-cell-cells': + enum: [0, 1] + description: + Must be 1 if the transformations has multiple output values. + The argument is then the index into the list of output values. + For example, if the nvmem cell only specify a base ethernet + address the transformation can then create different individual + ethernet addresses. + read-only: $ref: /schemas/types.yaml#/definitions/flag description: Mark the provider as read only. + transformation: + description: + Transform the content of a NVMEM cell. Sometimes it is necessary + to preprocess the content of a cell so it is usable by the NVMEM + consumer. There are also cases where one NVMEM cell value can + generate a list of values. + + Use one of the NVMEM_T_* prefixed definitions from the header + include/dt-bindings/nvmem/nvmem.h. + + Some transformations might have additional arguments. + $ref: /schemas/types.yaml#/definitions/uint32 + + wp-gpios: description: GPIO to which the write-protect pin of the chip is connected. @@ -98,6 +121,12 @@ examples: reg = <0xc 0x1>; bits = <2 3>; }; + + ethernet_base_mac: base-mac-address@100 { + #nvmem-cell-cells = <1>; + reg = <0x100 0x6>; + transformation = <NVMEM_T_ETH_OFFSET 0 1 2 7>; + }; }; ...
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. The first proposal [1] didn't find much appreciation and there wasn't any reply to my question or new proposal [2]. So here we are with my new proposal, that is more flexible and doesn't fix the ethernet mac only. This is just an RFC for the device tree representation for now to see if this is the correct way to tackle this. I'm also aware of the latest post process hook support [3]. This doesn't fix the base mac address issue, but I think it also doesn't solve the case with swapped ethernet addresses in the general case. That hook will involve the driver to do the swapping, but how would the driver know if that swapping is actually required. Usually the interpretation of the content is opaque to the driver, after all it is the user/board manufacturer who does program the storage device. We might be lucky in the imx-ocotp case because the IMX reference manual actually states where and in which format the mac address is programmed. Introduce a transformation property. This is intended to be just an enumeration of operations. If there will be a new operation, support for it has to be added to the nvmem core. A transformation might 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 into a list of values. Example: mac_addresses: base-mac-address@10 { #nvmem-cell-cells = <1>; reg = <10 6>; transformation = <NVMEM_T_ETH_OFFSET 0 1 7>; } ð0 { nvmem-cells = <&mac_addresses 0>; nvmem-cell-names = "mac-address"; }; ð1 { nvmem-cells = <&mac_addresses 2>; nvmem-cell-names = "mac-address"; }; The NVMEM_T_ETH_OFFSET transformation takes N additional (dt) cells and will generate N values. In this example BASE_MAC+0, BASE_MAC+1, BASE_MAC+7. An nvmem consumer can then reference the nvmem cell with an index. So eth0 will get BASE_MAC+0 and eth1 will get BASE_MAC+7. This should be sufficient flexible for many different transformations without having to touch the bindings except for adding documentation and checks for new transformations. I do have one question regarding "#nvmem-cell-cells" (aside from the awkward naming): is it allowed to have that property optional if there is no additional argument to the phandle? [1] https://lore.kernel.org/all/20210414152657.12097-2-michael@walle.cc/ [2] https://lore.kernel.org/linux-devicetree/362f1c6a8b0ec191b285ac6a604500da@walle.cc/ [3] https://lore.kernel.org/lkml/20211013131957.30271-1-srinivas.kandagatla@linaro.org/ Signed-off-by: Michael Walle <michael@walle.cc> --- .../devicetree/bindings/nvmem/nvmem.yaml | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+)