| Message ID | 20190511144444.GU81826@meh.true.cz |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | NVMEM address DT post processing [Was: Re: [PATCH net 0/3] addproperty "nvmem_macaddr_swap" to swap macaddr bytes order] | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | "total: 1 errors, 3 warnings, 70 lines checked" |
On Sat, May 11, 2019 at 04:44:44PM +0200, Petr Štetiar wrote: > So something like this? > > diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt > index fd06c09b822b..d781e47b049d 100644 > --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt > +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt > @@ -1,12 +1,14 @@ > = NVMEM(Non Volatile Memory) Data Device Tree Bindings = > > This binding is intended to represent the location of hardware > -configuration data stored in NVMEMs like eeprom, efuses and so on. > +configuration data stored in NVMEMs like eeprom, efuses and so on. This > +binding provides some basic transformation of the stored data as well. > > On a significant proportion of boards, the manufacturer has stored > some data on NVMEM, for the OS to be able to retrieve these information > and act upon it. Obviously, the OS has to know about where to retrieve > -these data from, and where they are stored on the storage device. > +these data from, where they are stored on the storage device and how to > +postprocess them. > > This document is here to document this. > > @@ -29,6 +31,19 @@ Optional properties: > bits: Is pair of bit location and number of bits, which specifies offset > in bit and number of bits within the address range specified by reg property. > Offset takes values from 0-7. > +byte-indices: array, encoded as an arbitrary number of (offset, length) pairs, > + within the address range specified by reg property. Each pair is > + then processed with byte-transform in order to produce single u8 > + sized byte. > +byte-transform: string, specifies the transformation which should be applied > + to every byte-indices pair in order to produce usable u8 sized byte, > + possible values are "none", "ascii" and "bcd". Default is "none". > +byte-adjust: number, value by which should be adjusted resulting output byte at > + byte-adjust-at offset. > +byte-adjust-at: number, specifies offset of resulting output byte which should be > + adjusted by byte-adjust value, default is 0. > +byte-result-swap: boolean, specifies if the resulting output bytes should be > + swapped prior to return > > For example: > > @@ -59,6 +74,36 @@ For example: > ... > }; > > +Another example where we've MAC address for eth1 stored in the NOR EEPROM as > +following sequence of bytes (output of hexdump -C /dev/mtdX): > + > + 00000180 66 61 63 5f 6d 61 63 20 3d 20 44 34 3a 45 45 3a |fac_mac = D4:EE:| > + 00000190 30 37 3a 33 33 3a 36 43 3a 32 30 0a 42 44 49 4e |07:33:6C:20.BDIN| > + > +Which means, that MAC address is stored in EEPROM as D4:EE:07:33:6C:20, so > +ASCII delimited by colons, but we can't use this MAC address directly as > +there's only one MAC address stored in the EEPROM and we need to increment last > +octet/byte in this address in order to get usable MAC address for eth1 device. > + > + eth1_addr: eth-mac-addr@18a { > + reg = <0x18a 0x11>; > + byte-indices = < 0 2 > + 3 2 > + 6 2 > + 9 2 > + 12 2 > + 15 2>; > + byte-transform = "ascii"; > + byte-increment = <1>; > + byte-increment-at = <5>; > + byte-result-swap; > + }; > + > + ð1 { > + nvmem-cells = <ð1_addr>; > + nvmem-cell-names = "mac-address"; > + }; > + Something along those lines yes. I'm not sure why in your example the cell doesn't start at the mac address itself, instead of starting at the key + having to specify an offset though. The reg property is the offset already. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 11/05/2019 15:44, Petr Štetiar wrote: > }; > > +Another example where we've MAC address for eth1 stored in the NOR EEPROM as > +following sequence of bytes (output of hexdump -C /dev/mtdX): > + > + 00000180 66 61 63 5f 6d 61 63 20 3d 20 44 34 3a 45 45 3a |fac_mac = D4:EE:| > + 00000190 30 37 3a 33 33 3a 36 43 3a 32 30 0a 42 44 49 4e |07:33:6C:20.BDIN| > + > +Which means, that MAC address is stored in EEPROM as D4:EE:07:33:6C:20, so > +ASCII delimited by colons, but we can't use this MAC address directly as > +there's only one MAC address stored in the EEPROM and we need to increment last > +octet/byte in this address in order to get usable MAC address for eth1 device. > + > + eth1_addr: eth-mac-addr@18a { > + reg = <0x18a 0x11>; > + byte-indices = < 0 2 > + 3 2 > + 6 2 > + 9 2 > + 12 2 > + 15 2>; > + byte-transform = "ascii"; > + byte-increment = <1>; > + byte-increment-at = <5>; > + byte-result-swap; > + }; > + > + ð1 { > + nvmem-cells = <ð1_addr>; > + nvmem-cell-names = "mac-address"; > + }; > + > = Data consumers = > Are device nodes which consume nvmem data cells/providers. TBH, I have not see the full thread as I don't seem to be added to the original thread for some reason! Looking at the comments from last few emails, I think the overall idea of moving the transformations in to nvmem core looks fine with me. I remember we discuss this while nvmem was first added and the plan was to add some kinda nvmem plugin/transformation interface at cell/provider level. And this looks like correct time to introduce this. My initial idea was to add compatible strings to the cell so that most of the encoding information can be derived from it. For example if the encoding representing in your example is pretty standard or vendor specific we could just do with a simple compatible like below: eth1_addr: eth-mac-addr@18a { compatible = "xxx,nvmem-mac-address"; reg = <0x18a 0x11>; }; ð1 { nvmem-cells = <ð1_addr>; nvmem-cell-names = "mac-address"; } thanks, srini
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 09:25:55]: Hi, > My initial idea was to add compatible strings to the cell so that most of > the encoding information can be derived from it. For example if the encoding > representing in your example is pretty standard or vendor specific we could > just do with a simple compatible like below: that vendor/compatible list would be quite long[1], there are hundreds of devices in current OpenWrt tree (using currently custom patch) and probably dozens currently unsupported (ASCII encoded MAC address in NVMEM). So my goal is to add some DT functionality which would cover all of these. > eth1_addr: eth-mac-addr@18a { > compatible = "xxx,nvmem-mac-address"; > reg = <0x18a 0x11>; > }; while sketching the possible DT use cases I came to the this option as well, it was very compeling as it would kill two birds with one stone (fix outstanding MTD/NVMEM OF clash as well[2]), but I think, that it makes more sense to add this functionality to nvmem core so it could be reused by other consumers, not just by network layer. 1. https://git.openwrt.org/?p=openwrt%2Fopenwrt.git&a=search&h=HEAD&st=grep&s=mtd-mac-address 2. https://lore.kernel.org/netdev/20190418133646.GA94236@meh.true.cz -- ynezz
Maxime Ripard <maxime.ripard@bootlin.com> [2019-05-12 14:19:10]: > > @@ -29,6 +31,19 @@ Optional properties: > > bits: Is pair of bit location and number of bits, which specifies offset > > in bit and number of bits within the address range specified by reg property. > > Offset takes values from 0-7. > > +byte-indices: array, encoded as an arbitrary number of (offset, length) pairs, > > + within the address range specified by reg property. Each pair is > > + then processed with byte-transform in order to produce single u8 > > + sized byte. > > +byte-transform: string, specifies the transformation which should be applied > > + to every byte-indices pair in order to produce usable u8 sized byte, > > + possible values are "none", "ascii" and "bcd". Default is "none". > > +byte-adjust: number, value by which should be adjusted resulting output byte at > > + byte-adjust-at offset. > > +byte-adjust-at: number, specifies offset of resulting output byte which should be > > + adjusted by byte-adjust value, default is 0. > > +byte-result-swap: boolean, specifies if the resulting output bytes should be > > + swapped prior to return > > > > For example: > > > > @@ -59,6 +74,36 @@ For example: > > ... > > }; > > > > +Another example where we've MAC address for eth1 stored in the NOR EEPROM as > > +following sequence of bytes (output of hexdump -C /dev/mtdX): > > + > > + 00000180 66 61 63 5f 6d 61 63 20 3d 20 44 34 3a 45 45 3a |fac_mac = D4:EE:| > > + 00000190 30 37 3a 33 33 3a 36 43 3a 32 30 0a 42 44 49 4e |07:33:6C:20.BDIN| > > + > > +Which means, that MAC address is stored in EEPROM as D4:EE:07:33:6C:20, so > > +ASCII delimited by colons, but we can't use this MAC address directly as > > +there's only one MAC address stored in the EEPROM and we need to increment last > > +octet/byte in this address in order to get usable MAC address for eth1 device. > > + > > + eth1_addr: eth-mac-addr@18a { > > + reg = <0x18a 0x11>; > > + byte-indices = < 0 2 > > + 3 2 > > + 6 2 > > + 9 2 > > + 12 2 > > + 15 2>; > > + byte-transform = "ascii"; > > + byte-increment = <1>; > > + byte-increment-at = <5>; > > + byte-result-swap; > > + }; > > + > > + ð1 { > > + nvmem-cells = <ð1_addr>; > > + nvmem-cell-names = "mac-address"; > > + }; > > + > > Something along those lines yes. I'm not sure why in your example the > cell doesn't start at the mac address itself, instead of starting at > the key + having to specify an offset though. The reg property is the > offset already. The cell starts at the MAC address itself, 0x180 is offset within the EEPROM and 0xa is byte within the offset (off-by-one, correct should be 0x9 though). EEPROM byte within EEPROM offset offset 1 2 3 4 5 5 6 7 8 9 a b c d e f ------------------------------------------------------------|----------------- 00000180 66 61 63 5f 6d 61 63 20 3d 20 44 34 3a 45 45 3a |fac_mac = D4:EE:| 00000190 30 37 3a 33 33 3a 36 43 3a 32 30 0a 42 44 49 4e |07:33:6C:20.BDIN| So this would produce following: eth1_addr: eth-mac-addr@189 { reg = <0x189 0x11>; /* 0x44 0x34 0x3a 0x45 0x45 0x3a 0x30 0x37 * 0x3a 0x33 0x33 0x3a 0x36 0x43 0x3a 0x32 0x30 */ byte-indices = < 0 2 /* 0x44 0x34 */ 3 2 /* 0x45 0x45 */ 6 2 /* 0x30 0x37 */ 9 2 /* 0x33 0x33 */ 12 2 /* 0x36 0x43 */ 15 2>; /* 0x32 0x30 */ byte-transform = "ascii"; /* 0xd4 0xee 0x7 0x33 0x6c 0x20 */ byte-increment = <1>; byte-increment-at = <5>; /* 0xd4 0xee 0x7 0x33 0x6c 0x21 */ byte-result-swap; /* 0x21 0x6c 0x33 0x7 0xee 0xd4 */ }; -- ynezz
On 13/05/2019 10:07, Petr Štetiar wrote: > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 09:25:55]: > > Hi, > >> My initial idea was to add compatible strings to the cell so that most of >> the encoding information can be derived from it. For example if the encoding >> representing in your example is pretty standard or vendor specific we could >> just do with a simple compatible like below: > > that vendor/compatible list would be quite long[1], there are hundreds of You are right just vendor list could be very long, but I was hoping that the post-processing would fall in some categories which can be used in compatible string. Irrespective of which we need to have some sort of compatible string to enable nvmem core to know that there is some form of post processing to be done on the cells!. Without which there is a danger of continuing to adding new properties to the cell bindings which have no relation to each other. > devices in current OpenWrt tree (using currently custom patch) and probably > dozens currently unsupported (ASCII encoded MAC address in NVMEM). So my goal > is to add some DT functionality which would cover all of these. > >> eth1_addr: eth-mac-addr@18a { >> compatible = "xxx,nvmem-mac-address"; >> reg = <0x18a 0x11>; >> }; > > while sketching the possible DT use cases I came to the this option as well, it > was very compeling as it would kill two birds with one stone (fix outstanding > MTD/NVMEM OF clash as well[2]), but I think, that it makes more sense to add > this functionality to nvmem core so it could be reused by other consumers, not > just by network layer. Changes to nvmem dt bindings have been already rejected, for this more discussion at: https://lore.kernel.org/patchwork/patch/936312/ --srini > > 1. https://git.openwrt.org/?p=openwrt%2Fopenwrt.git&a=search&h=HEAD&st=grep&s=mtd-mac-address > 2. https://lore.kernel.org/netdev/20190418133646.GA94236@meh.true.cz > > -- ynezz >
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 11:06:48]: > On 13/05/2019 10:07, Petr Štetiar wrote: > > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 09:25:55]: > > > > > My initial idea was to add compatible strings to the cell so that most of > > > the encoding information can be derived from it. For example if the encoding > > > representing in your example is pretty standard or vendor specific we could > > > just do with a simple compatible like below: > > > > that vendor/compatible list would be quite long[1], there are hundreds of > > You are right just vendor list could be very long, but I was hoping that the > post-processing would fall in some categories which can be used in > compatible string. > > Irrespective of which we need to have some sort of compatible string to > enable nvmem core to know that there is some form of post processing to be > done on the cells!. Without which there is a danger of continuing to adding > new properties to the cell bindings which have no relation to each other. makes sense, so something like this would be acceptable? eth1_addr: eth-mac-addr@18a { /* or rather linux,nvmem-post-process ? */ compatible = "openwrt,nvmem-post-process"; reg = <0x189 0x11>; indices = < 0 2 3 2 6 2 9 2 12 2 15 2>; transform = "ascii"; increment = <1>; increment-at = <5>; result-swap; }; ð1 { nvmem-cells = <ð1_addr>; nvmem-cell-names = "mac-address"; }; > > was very compeling as it would kill two birds with one stone (fix outstanding > > MTD/NVMEM OF clash as well[2]), > > Changes to nvmem dt bindings have been already rejected, for this more > discussion at: https://lore.kernel.org/patchwork/patch/936312/ I know, I've re-read this thread several times, but it's still unclear to me, how this should be approached in order to be accepted by you and by DT maintainers as well. Anyway, to sum it up, from your point of view, following is fine? flash@0 { partitions { art: partition@ff0000 { nvmem_dev: nvmem-cells { compatible = "nvmem-cells"; eth1_addr: eth-mac-addr@189 { compatible = "openwrt,nvmem-post-process"; reg = <0x189 0x6>; increment = <1>; increment-at = <5>; result-swap; }; }; }; }; }; ð1 { nvmem = <&nvmem_dev>; nvmem-cells = <ð1_addr>; nvmem-cell-names = "mac-address"; }; -- ynezz
On 13/05/2019 12:16, Petr Štetiar wrote: > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 11:06:48]: > >> On 13/05/2019 10:07, Petr Štetiar wrote: >>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 09:25:55]: >>> >>>> My initial idea was to add compatible strings to the cell so that most of >>>> the encoding information can be derived from it. For example if the encoding >>>> representing in your example is pretty standard or vendor specific we could >>>> just do with a simple compatible like below: >>> >>> that vendor/compatible list would be quite long[1], there are hundreds of >> >> You are right just vendor list could be very long, but I was hoping that the >> post-processing would fall in some categories which can be used in >> compatible string. >> >> Irrespective of which we need to have some sort of compatible string to >> enable nvmem core to know that there is some form of post processing to be >> done on the cells!. Without which there is a danger of continuing to adding >> new properties to the cell bindings which have no relation to each other. > > makes sense, so something like this would be acceptable? > > eth1_addr: eth-mac-addr@18a { > /* or rather linux,nvmem-post-process ? */ > compatible = "openwrt,nvmem-post-process"; I don't think this would be a correct compatible string to use here. Before we decide on naming, I would like to understand bit more on what are the other possible forms of storing mac address, Here is what I found, Type 1: Octets in ASCII without delimiters. (Swapped/non-Swapped) Type 2: Octets in ASCII with delimiters like (":", ",", ".", "-"... so on) (Swapped/non-Swapped) Type 3: Is the one which stores mac address in Type1/2 but this has to be incremented to be used on other instances of eth. Did I miss anything? My suggestion for type1 and type2 would be something like this, as long as its okay with DT maintainers eth1_addr: eth-mac-addr@18a { compatible = "ascii-mac-address"; reg = <0x18a 2>, <0x192 2>, <0x196 2>, <0x200 2>, <0x304 2>, <0x306 2>; swap-mac-address; delimiter = ":"; }; For type 3: This sounds like very much vendor specific optimization thing which am not 100% sure atm. If dt maintainers are okay, may be we can add an increment in the "ascii-mac-address" binding itself. Do you think "increment-at " would ever change? > reg = <0x189 0x11>; > indices = < 0 2 > 3 2 > 6 2 > 9 2 > 12 2 > 15 2>; > transform = "ascii"; > increment = <1>; > increment-at = <5>; > result-swap; > }; > > ð1 { > nvmem-cells = <ð1_addr>; > nvmem-cell-names = "mac-address"; > }; > >>> was very compeling as it would kill two birds with one stone (fix outstanding >>> MTD/NVMEM OF clash as well[2]), >> >> Changes to nvmem dt bindings have been already rejected, for this more >> discussion at: https://lore.kernel.org/patchwork/patch/936312/ > > I know, I've re-read this thread several times, but it's still unclear to me, > how this should be approached in order to be accepted by you and by DT > maintainers as well. > > Anyway, to sum it up, from your point of view, following is fine? > currently mtd already has support for nvmem but without dt support. > flash@0 { > partitions { > art: partition@ff0000 { > nvmem_dev: nvmem-cells { > compatible = "nvmem-cells"; > eth1_addr: eth-mac-addr@189 { > compatible = "openwrt,nvmem-post-process"; > reg = <0x189 0x6>; > increment = <1>; > increment-at = <5>; > result-swap; > }; > }; > }; > }; > }; This [1] is what I had suggested at the end, where in its possible to add provider node with its own custom bindings. In above example nvmem_dev would be a proper nvmem provider. Thanks, srini [1] https://lkml.org/lkml/2018/6/7/972 > > ð1 { > nvmem = <&nvmem_dev>; > nvmem-cells = <ð1_addr>; > nvmem-cell-names = "mac-address"; > }; > > -- ynezz >
Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-14 16:13:22]: > On 13/05/2019 12:16, Petr Štetiar wrote: > > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 11:06:48]: > > > > > On 13/05/2019 10:07, Petr Štetiar wrote: > > > > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 09:25:55]: > > > > > > > > > My initial idea was to add compatible strings to the cell so that most of > > > > > the encoding information can be derived from it. For example if the encoding > > > > > representing in your example is pretty standard or vendor specific we could > > > > > just do with a simple compatible like below: > > > > > > > > that vendor/compatible list would be quite long[1], there are hundreds of > > > > > > You are right just vendor list could be very long, but I was hoping that the > > > post-processing would fall in some categories which can be used in > > > compatible string. > > > > > > Irrespective of which we need to have some sort of compatible string to > > > enable nvmem core to know that there is some form of post processing to be > > > done on the cells!. Without which there is a danger of continuing to adding > > > new properties to the cell bindings which have no relation to each other. > > > > makes sense, so something like this would be acceptable? > > > > eth1_addr: eth-mac-addr@18a { > > /* or rather linux,nvmem-post-process ? */ > > compatible = "openwrt,nvmem-post-process"; > > I don't think this would be a correct compatible string to use here. > Before we decide on naming, I would like to understand bit more on what are > the other possible forms of storing mac address, > Here is what I found, > > Type 1: Octets in ASCII without delimiters. (Swapped/non-Swapped) > Type 2: Octets in ASCII with delimiters like (":", ",", ".", "-"... so on) > (Swapped/non-Swapped) > Type 3: Is the one which stores mac address in Type1/2 but this has to be > incremented to be used on other instances of eth. > > Did I miss anything? Type 4: Octets as bytes/u8, swapped/non-swapped Currently just type4-non-swapped is supported. Support for type4-swapped was goal of this patch series. I've simply tried to avoid using mac-address for the compatible as this provider could be reused by other potential nvmem consumers. The question is, how much abstracted it should be then. > My suggestion for type1 and type2 would be something like this, as long as > its okay with DT maintainers > > eth1_addr: eth-mac-addr@18a { > compatible = "ascii-mac-address"; > reg = <0x18a 2>, <0x192 2>, <0x196 2>, <0x200 2>, <0x304 2>, <0x306 2>; > swap-mac-address; > delimiter = ":"; > }; with this reg array, you don't need the delimiter property anymore, do you? > For type 3: > > This sounds like very much vendor specific optimization thing which am not > 100% sure atm. If dt maintainers are okay, may be we can add an increment > in the "ascii-mac-address" binding itself. > > Do you think "increment-at " would ever change? Currently there's just one such real world use case in OpenWrt tree[1]. Probably some vendor decided to increment 4th octet. > This [1] is what I had suggested at the end, where in its possible to add > provider node with its own custom bindings. In above example nvmem_dev would > be a proper nvmem provider. Ok, thanks. 1. https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/ath79/dts/ar9331_embeddedwireless_dorin.dts;h=43bec35fa2860fe4d52880ad24ff7c56f5060a0a;hb=HEAD#l109 -- ynezz
On 14/05/2019 18:44, Petr Štetiar wrote: > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-14 16:13:22]: > >> On 13/05/2019 12:16, Petr Štetiar wrote: >>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 11:06:48]: >>> >>>> On 13/05/2019 10:07, Petr Štetiar wrote: >>>>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 09:25:55]: >>>>> >>>>>> My initial idea was to add compatible strings to the cell so that most of >>>>>> the encoding information can be derived from it. For example if the encoding >>>>>> representing in your example is pretty standard or vendor specific we could >>>>>> just do with a simple compatible like below: >>>>> >>>>> that vendor/compatible list would be quite long[1], there are hundreds of >>>> >>>> You are right just vendor list could be very long, but I was hoping that the >>>> post-processing would fall in some categories which can be used in >>>> compatible string. >>>> >>>> Irrespective of which we need to have some sort of compatible string to >>>> enable nvmem core to know that there is some form of post processing to be >>>> done on the cells!. Without which there is a danger of continuing to adding >>>> new properties to the cell bindings which have no relation to each other. >>> >>> makes sense, so something like this would be acceptable? >>> >>> eth1_addr: eth-mac-addr@18a { >>> /* or rather linux,nvmem-post-process ? */ >>> compatible = "openwrt,nvmem-post-process"; >> >> I don't think this would be a correct compatible string to use here. >> Before we decide on naming, I would like to understand bit more on what are >> the other possible forms of storing mac address, >> Here is what I found, >> >> Type 1: Octets in ASCII without delimiters. (Swapped/non-Swapped) >> Type 2: Octets in ASCII with delimiters like (":", ",", ".", "-"... so on) >> (Swapped/non-Swapped) >> Type 3: Is the one which stores mac address in Type1/2 but this has to be >> incremented to be used on other instances of eth. >> >> Did I miss anything? > > Type 4: Octets as bytes/u8, swapped/non-swapped > > Currently just type4-non-swapped is supported. Support for type4-swapped was > goal of this patch series. > Can we just get away with swapped/non-swapped by using order of reg dt property? If that works for you then we do not need a special compatible string too. Note that current nvmem core only supports single reg value pair which needs to be extended to support multiple reg value. > I've simply tried to avoid using mac-address for the compatible as this > provider could be reused by other potential nvmem consumers. The question is, > how much abstracted it should be then. > >> My suggestion for type1 and type2 would be something like this, as long as >> its okay with DT maintainers >> >> eth1_addr: eth-mac-addr@18a { >> compatible = "ascii-mac-address"; >> reg = <0x18a 2>, <0x192 2>, <0x196 2>, <0x200 2>, <0x304 2>, <0x306 2>; >> swap-mac-address; >> delimiter = ":"; >> }; > > with this reg array, you don't need the delimiter property anymore, do you? > You are right we do not need it. >> For type 3: >> >> This sounds like very much vendor specific optimization thing which am not >> 100% sure atm. If dt maintainers are okay, may be we can add an increment >> in the "ascii-mac-address" binding itself. >> >> Do you think "increment-at " would ever change? > > Currently there's just one such real world use case in OpenWrt tree[1]. If that is the case then we definitely need bindings prefixed with vendor name or something on those lines for this. > Probably some vendor decided to increment 4th octet. Incrementing 4th octet does not really make sense! Thanks, srini
On Mon, May 13, 2019 at 6:16 AM Petr Štetiar <ynezz@true.cz> wrote: > > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 11:06:48]: > > > On 13/05/2019 10:07, Petr Štetiar wrote: > > > Srinivas Kandagatla <srinivas.kandagatla@linaro.org> [2019-05-13 09:25:55]: > > > > > > > My initial idea was to add compatible strings to the cell so that most of > > > > the encoding information can be derived from it. For example if the encoding > > > > representing in your example is pretty standard or vendor specific we could > > > > just do with a simple compatible like below: > > > > > > that vendor/compatible list would be quite long[1], there are hundreds of > > > > You are right just vendor list could be very long, but I was hoping that the > > post-processing would fall in some categories which can be used in > > compatible string. > > > > Irrespective of which we need to have some sort of compatible string to > > enable nvmem core to know that there is some form of post processing to be > > done on the cells!. Without which there is a danger of continuing to adding > > new properties to the cell bindings which have no relation to each other. > > makes sense, so something like this would be acceptable? No. Don't try to put a data processing language into DT. > > eth1_addr: eth-mac-addr@18a { > /* or rather linux,nvmem-post-process ? */ > compatible = "openwrt,nvmem-post-process"; > reg = <0x189 0x11>; > indices = < 0 2 > 3 2 > 6 2 > 9 2 > 12 2 > 15 2>; > transform = "ascii"; > increment = <1>; > increment-at = <5>; > result-swap; > }; > > ð1 { > nvmem-cells = <ð1_addr>; > nvmem-cell-names = "mac-address"; > }; > > > > was very compeling as it would kill two birds with one stone (fix outstanding > > > MTD/NVMEM OF clash as well[2]), > > > > Changes to nvmem dt bindings have been already rejected, for this more > > discussion at: https://lore.kernel.org/patchwork/patch/936312/ > > I know, I've re-read this thread several times, but it's still unclear to me, > how this should be approached in order to be accepted by you and by DT > maintainers as well. > > Anyway, to sum it up, from your point of view, following is fine? > > flash@0 { > partitions { > art: partition@ff0000 { > nvmem_dev: nvmem-cells { > compatible = "nvmem-cells"; My suggestion would be to add a specific compatible here and that can provide a driver or hooks to read and transform the data. Rob > eth1_addr: eth-mac-addr@189 { > compatible = "openwrt,nvmem-post-process"; > reg = <0x189 0x6>; > increment = <1>; > increment-at = <5>; > result-swap; > }; > }; > }; > }; > }; > > ð1 { > nvmem = <&nvmem_dev>; > nvmem-cells = <ð1_addr>; > nvmem-cell-names = "mac-address"; > }; > > -- ynezz
diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt index fd06c09b822b..d781e47b049d 100644 --- a/Documentation/devicetree/bindings/nvmem/nvmem.txt +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt @@ -1,12 +1,14 @@ = NVMEM(Non Volatile Memory) Data Device Tree Bindings = This binding is intended to represent the location of hardware -configuration data stored in NVMEMs like eeprom, efuses and so on. +configuration data stored in NVMEMs like eeprom, efuses and so on. This +binding provides some basic transformation of the stored data as well. On a significant proportion of boards, the manufacturer has stored some data on NVMEM, for the OS to be able to retrieve these information and act upon it. Obviously, the OS has to know about where to retrieve -these data from, and where they are stored on the storage device. +these data from, where they are stored on the storage device and how to +postprocess them. This document is here to document this. @@ -29,6 +31,19 @@ Optional properties: bits: Is pair of bit location and number of bits, which specifies offset in bit and number of bits within the address range specified by reg property. Offset takes values from 0-7. +byte-indices: array, encoded as an arbitrary number of (offset, length) pairs, + within the address range specified by reg property. Each pair is + then processed with byte-transform in order to produce single u8 + sized byte. +byte-transform: string, specifies the transformation which should be applied + to every byte-indices pair in order to produce usable u8 sized byte, + possible values are "none", "ascii" and "bcd". Default is "none". +byte-adjust: number, value by which should be adjusted resulting output byte at + byte-adjust-at offset. +byte-adjust-at: number, specifies offset of resulting output byte which should be + adjusted by byte-adjust value, default is 0. +byte-result-swap: boolean, specifies if the resulting output bytes should be + swapped prior to return For example: @@ -59,6 +74,36 @@ For example: ... }; +Another example where we've MAC address for eth1 stored in the NOR EEPROM as +following sequence of bytes (output of hexdump -C /dev/mtdX): + + 00000180 66 61 63 5f 6d 61 63 20 3d 20 44 34 3a 45 45 3a |fac_mac = D4:EE:| + 00000190 30 37 3a 33 33 3a 36 43 3a 32 30 0a 42 44 49 4e |07:33:6C:20.BDIN| + +Which means, that MAC address is stored in EEPROM as D4:EE:07:33:6C:20, so +ASCII delimited by colons, but we can't use this MAC address directly as +there's only one MAC address stored in the EEPROM and we need to increment last +octet/byte in this address in order to get usable MAC address for eth1 device. + + eth1_addr: eth-mac-addr@18a { + reg = <0x18a 0x11>; + byte-indices = < 0 2 + 3 2 + 6 2 + 9 2 + 12 2 + 15 2>; + byte-transform = "ascii"; + byte-increment = <1>; + byte-increment-at = <5>; + byte-result-swap; + }; + + ð1 { + nvmem-cells = <ð1_addr>; + nvmem-cell-names = "mac-address"; + }; + = Data consumers = Are device nodes which consume nvmem data cells/providers.