The nvmem cell binding applies to all objects which match "^.*@[0-9a-f]+$",
without taking a compatible into account. This precludes extension of e.g.
eeprom nodes by any child nodes other than nvmem. Consider following example:
eeprom@0 {
reg = <0 64>;
#address-cells = <1>;
#size-cells = <1>;
partitions {
compatible = "fixed-partitions";
#address-cells = <1>;
#size-cells = <1>;
bits = <64 64 64>; /* to verify it's skipped */
part@0 {
reg = <0x00 16>;
};
};
no-cell@10 {
compatible = "not-nvmem-cell";
reg = <0x10 4>;
bits = <64 64 64>; /* to verify it's skipped */
};
cell-old@14 {
reg = <0x14 0x2>;
};
cell-new@16 {
compatible = "nvmem-cell";
reg = <0x16 4>;
};
};
Without this series, the NVMEM driver interprets all direct children of eeprom@0
as NVMEM cells and driver probe fails, because the partitions node lacks a reg
property, e.g.:
nvmem 0-00000: nvmem: invalid reg on /eeprom@0
Running dtbs_check on the snippet will skip partitions (it doesn't match above
regex), but will flag no-cell@10 and cell-new@16 as invalid.
With this series applied, the driver will skip partitions and no-cell@10,
because they have a compatible but it's not "nvmem-cell".
Both cell-old@14 and cell-new@16 will be interpreted as cells.
Likewise, running dtbs_check on the snippet will skip partitions (compatible
doesn't match and regex doesn't either) and no-cell@10, but accept the other two.
This series resolves an existing clash between this nvmem-cell binding and
the barebox bootloader binding that extends the fixed-partitions MTD
binding to EEPROMs[1]. It's also a building block for getting nvmem cells and
partitions in MTD devices to co-exist in the same device tree node[2].
The changes are backwards-compatible, because, per binding, a compatible
property in a cell node was so far invalid. More details in the commit
messages.
[1]: https://www.mail-archive.com/[email protected]/msg33944.html
[2]: https://patchwork.ozlabs.org/patch/890741/
v2 -> v3:
- use optional compatible property to weed out nodes instead of name
- extend binding documentation (Srini)
v1 -> v2:
- use ->full_name instead of ->name as to not break existing correct
cells (Christian)
Cheers,
Ahmad Fatoum (2):
dt-bindings: nvmem: skip nodes with compatibles other than
"nvmem-cell"
nvmem: core: skip nodes with compatibles other than "nvmem-cell"
Documentation/devicetree/bindings/nvmem/nvmem.yaml | 14 +++++++++++++-
drivers/nvmem/core.c | 4 ++++
2 files changed, 17 insertions(+), 1 deletion(-)
Cc: Rob Herring <[email protected]>
Cc: [email protected]
--
2.26.2
The nvmem cell binding used to apply to all objects which match
"^.*@[0-9a-f]+$", without taking a compatible into account, which
precluded extension of EEPROMs by child nodes other than nvmem.
A previous commit changed the binding, so that nvmem cells that
feature a compatible property must have "nvmem-cell" as the value,
otherwise they are skipped.
Adjust the driver to observe the new binding change. This change
does not change behavior for any device tree that was already
compliant with the nvmem binding.
Signed-off-by: Ahmad Fatoum <[email protected]>
---
drivers/nvmem/core.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 05c6ae4b0b97..eb697f5ad07d 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -547,6 +547,10 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
parent = dev->of_node;
for_each_child_of_node(parent, child) {
+ if (of_find_property(child, "compatible", NULL) &&
+ !of_device_is_compatible(child, "nvmem-cell"))
+ continue;
+
addr = of_get_property(child, "reg", &len);
if (!addr || (len < 2 * sizeof(u32))) {
dev_err(dev, "nvmem: invalid reg on %pOF\n", child);
--
2.26.2
For nodes matching the nvmem binding, all child objects matching
"^.*@[0-9a-f]+$" are assumed to be nvmem cells, without taking a
compatible into account.
This precludes:
- future extension of e.g. eeprom nodes by any child nodes other
than nvmem cells
- extending the NVMEM binding to nodes that already have other
child nodes, e.g., MTD and its partitions
To allow co-existence of nvmem-cells with other nodes, loosen the
binding to consult an optional compatible property for the cells:
- if a compatible exists, it must be "nvmem-cell"
- if none exists, it's assumed to be a nvmem cell, like before
As additionalProperties: false was specified for nvmem-cell bindings,
a compatible property was so far invalid. This means no already
compliant device tree should be reinterpreted differently after
this binding adjustment and in that regard, the change is completely
backwards-compatible.
This resolves an existing clash between this nvmem-cell binding and
the barebox bootloader binding that extends the fixed-partitions MTD
binding to EEPROMs.
Signed-off-by: Ahmad Fatoum <[email protected]>
---
Documentation/devicetree/bindings/nvmem/nvmem.yaml | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
index 65980224d550..c39f5dd7e1aa 100644
--- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
+++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
@@ -44,9 +44,21 @@ properties:
patternProperties:
"^.*@[0-9a-f]+$":
- type: object
+ if:
+ properties:
+ compatible:
+ items:
+ const: nvmem-cell
+ then:
+ $ref: "#/definitions/nvmem-cell"
+definitions:
+ nvmem-cell:
properties:
+ compatible:
+ items:
+ const: nvmem-cell
+
reg:
maxItems: 1
description:
--
2.26.2
Hello Srini,
Hello Rob,
On 4/28/20 1:18 PM, Ahmad Fatoum wrote:
> To allow co-existence of nvmem-cells with other nodes, loosen the
> binding to consult an optional compatible property for the cells:
Gentle ping.
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 28/04/2020 12:18, Ahmad Fatoum wrote:
> The nvmem cell binding used to apply to all objects which match
> "^.*@[0-9a-f]+$", without taking a compatible into account, which
> precluded extension of EEPROMs by child nodes other than nvmem.
>
> A previous commit changed the binding, so that nvmem cells that
> feature a compatible property must have "nvmem-cell" as the value,
> otherwise they are skipped.
>
> Adjust the driver to observe the new binding change. This change
> does not change behavior for any device tree that was already
> compliant with the nvmem binding.
>
> Signed-off-by: Ahmad Fatoum <[email protected]>
Thanks for the patch, this looks good to me, lets wait for Rob to ack
the bindings!
--srini
> ---
> drivers/nvmem/core.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 05c6ae4b0b97..eb697f5ad07d 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -547,6 +547,10 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
> parent = dev->of_node;
>
> for_each_child_of_node(parent, child) {
> + if (of_find_property(child, "compatible", NULL) &&
> + !of_device_is_compatible(child, "nvmem-cell"))
> + continue;
> +
> addr = of_get_property(child, "reg", &len);
> if (!addr || (len < 2 * sizeof(u32))) {
> dev_err(dev, "nvmem: invalid reg on %pOF\n", child);
>
On Tue, Apr 28, 2020 at 01:18:25PM +0200, Ahmad Fatoum wrote:
> The nvmem cell binding applies to all objects which match "^.*@[0-9a-f]+$",
> without taking a compatible into account. This precludes extension of e.g.
> eeprom nodes by any child nodes other than nvmem. Consider following example:
>
> eeprom@0 {
> reg = <0 64>;
> #address-cells = <1>;
> #size-cells = <1>;
>
> partitions {
> compatible = "fixed-partitions";
> #address-cells = <1>;
> #size-cells = <1>;
> bits = <64 64 64>; /* to verify it's skipped */
>
> part@0 {
> reg = <0x00 16>;
> };
> };
>
> no-cell@10 {
> compatible = "not-nvmem-cell";
> reg = <0x10 4>;
> bits = <64 64 64>; /* to verify it's skipped */
> };
>
> cell-old@14 {
> reg = <0x14 0x2>;
> };
>
> cell-new@16 {
> compatible = "nvmem-cell";
> reg = <0x16 4>;
> };
> };
>
> Without this series, the NVMEM driver interprets all direct children of eeprom@0
> as NVMEM cells and driver probe fails, because the partitions node lacks a reg
> property, e.g.:
>
> nvmem 0-00000: nvmem: invalid reg on /eeprom@0
>
> Running dtbs_check on the snippet will skip partitions (it doesn't match above
> regex), but will flag no-cell@10 and cell-new@16 as invalid.
>
> With this series applied, the driver will skip partitions and no-cell@10,
> because they have a compatible but it's not "nvmem-cell".
Because you have to support no compatible (forever), there's no point
adding this compatible.
> Both cell-old@14 and cell-new@16 will be interpreted as cells.
>
> Likewise, running dtbs_check on the snippet will skip partitions (compatible
> doesn't match and regex doesn't either) and no-cell@10, but accept the other two.
>
> This series resolves an existing clash between this nvmem-cell binding and
> the barebox bootloader binding that extends the fixed-partitions MTD
> binding to EEPROMs[1]. It's also a building block for getting nvmem cells and
> partitions in MTD devices to co-exist in the same device tree node[2].
This violates having multiple nodes at the same address because you are
independently overlaying partitions and nvmem cells on same address
ranges. It also seems seems pretty fragile if you want to update
partitions.
I think instead, nvmem cells should be contained within a partition.
The partition should then have a compatible to indicate it contains
nvmem cells.
Rob
Hello Rob,
Hello Srini,
On 5/12/20 4:18 PM, Rob Herring wrote:
> On Tue, Apr 28, 2020 at 01:18:25PM +0200, Ahmad Fatoum wrote:
>> The nvmem cell binding applies to all objects which match "^.*@[0-9a-f]+$",
>> without taking a compatible into account. This precludes extension of e.g.
>> eeprom nodes by any child nodes other than nvmem. Consider following example:
>>
>> eeprom@0 {
>> reg = <0 64>;
>> #address-cells = <1>;
>> #size-cells = <1>;
>>
>> partitions {
>> compatible = "fixed-partitions";
>> #address-cells = <1>;
>> #size-cells = <1>;
>> bits = <64 64 64>; /* to verify it's skipped */
>>
>> part@0 {
>> reg = <0x00 16>;
>> };
>> };
>>
>> no-cell@10 {
>> compatible = "not-nvmem-cell";
>> reg = <0x10 4>;
>> bits = <64 64 64>; /* to verify it's skipped */
>> };
>>
>> cell-old@14 {
>> reg = <0x14 0x2>;
>> };
>>
>> cell-new@16 {
>> compatible = "nvmem-cell";
>> reg = <0x16 4>;
>> };
>> };
>>
>> Without this series, the NVMEM driver interprets all direct children of eeprom@0
>> as NVMEM cells and driver probe fails, because the partitions node lacks a reg
>> property, e.g.:
>>
>> nvmem 0-00000: nvmem: invalid reg on /eeprom@0
>>
>> Running dtbs_check on the snippet will skip partitions (it doesn't match above
>> regex), but will flag no-cell@10 and cell-new@16 as invalid.
>>
>> With this series applied, the driver will skip partitions and no-cell@10,
>> because they have a compatible but it's not "nvmem-cell".
>
> Because you have to support no compatible (forever), there's no point
> adding this compatible.
IMO nvmem cells should have had a compatible since the beginning and I figured
better late than never.
>> Both cell-old@14 and cell-new@16 will be interpreted as cells.
>>
>> Likewise, running dtbs_check on the snippet will skip partitions (compatible
>> doesn't match and regex doesn't either) and no-cell@10, but accept the other two.
>>
>> This series resolves an existing clash between this nvmem-cell binding and
>> the barebox bootloader binding that extends the fixed-partitions MTD
>> binding to EEPROMs[1]. It's also a building block for getting nvmem cells and
>> partitions in MTD devices to co-exist in the same device tree node[2].
>
> This violates having multiple nodes at the same address because you are
> independently overlaying partitions and nvmem cells on same address
> ranges. It also seems seems pretty fragile if you want to update
> partitions.
>
> I think instead, nvmem cells should be contained within a partition.
> The partition should then have a compatible to indicate it contains
> nvmem cells.
I thought I had understood what needs to be done, but now that I finally have time
to do it, I see that this only solves the second issue "extending the NVMEM binding
to nodes that already have other child nodes, e.g., MTD and its partitions".
The first issue: "future extension of e.g. eeprom nodes by any child nodes other than
nvmem cells" isn't solved by having a containing partition.
My issue is that the bootloader fixes up a partitions { compatible = "fixed-partitions"; }
child node into the kernel device tree. The NVMEM core driver tries to parse all eeprom child
nodes as cells and will make the driver probe of the EEPROM fail, because it can't parse that
fixed-partitions node as a nvmem cell.
To allow for co-existence of NVMEM cells and other subnodes, would following patch be
acceptable to you and Srini?
---------------------------------------- 8< --------------------------------------
--- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
+++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
@@ -45,7 +45,15 @@ properties:
patternProperties:
"^.*@[0-9a-f]+$":
type: object
-
+ if:
+ not:
+ properties:
+ compatible:
+ then:
+ $ref: "#/definitions/nvmem-cell"
+
+definitions:
+ nvmem-cell:
properties:
reg:
maxItems: 1
Hello Rob,
Hello Srini,
On 10/12/20 5:36 PM, Ahmad Fatoum wrote:
> On 5/12/20 4:18 PM, Rob Herring wrote:
[snip]
>> I think instead, nvmem cells should be contained within a partition.
>> The partition should then have a compatible to indicate it contains
>> nvmem cells.
>
> I thought I had understood what needs to be done, but now that I finally have time
> to do it, I see that this only solves the second issue "extending the NVMEM binding
> to nodes that already have other child nodes, e.g., MTD and its partitions".
>
> The first issue: "future extension of e.g. eeprom nodes by any child nodes other than
> nvmem cells" isn't solved by having a containing partition.
>
>
> My issue is that the bootloader fixes up a partitions { compatible = "fixed-partitions"; }
> child node into the kernel device tree. The NVMEM core driver tries to parse all eeprom child
> nodes as cells and will make the driver probe of the EEPROM fail, because it can't parse that
> fixed-partitions node as a nvmem cell.
>
> To allow for co-existence of NVMEM cells and other subnodes, would following patch be
> acceptable to you and Srini?
Gentle ping. Would the patch below be acceptable?
>
> ---------------------------------------- 8< --------------------------------------
> --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> @@ -45,7 +45,15 @@ properties:
> patternProperties:
> "^.*@[0-9a-f]+$":
> type: object
> -
> + if:
> + not:
> + properties:
> + compatible:
> + then:
> + $ref: "#/definitions/nvmem-cell"
> +
> +definitions:
> + nvmem-cell:
> properties:
> reg:
> maxItems: 1
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hello Rob,
Hello Srini,
On 02.11.20 16:23, Ahmad Fatoum wrote:
>>> I think instead, nvmem cells should be contained within a partition.
>>> The partition should then have a compatible to indicate it contains
>>> nvmem cells.
>>
>> I thought I had understood what needs to be done, but now that I finally have time
>> to do it, I see that this only solves the second issue "extending the NVMEM binding
>> to nodes that already have other child nodes, e.g., MTD and its partitions".
>>
>> The first issue: "future extension of e.g. eeprom nodes by any child nodes other than
>> nvmem cells" isn't solved by having a containing partition.
>>
>>
>> My issue is that the bootloader fixes up a partitions { compatible = "fixed-partitions"; }
>> child node into the kernel device tree. The NVMEM core driver tries to parse all eeprom child
>> nodes as cells and will make the driver probe of the EEPROM fail, because it can't parse that
>> fixed-partitions node as a nvmem cell.
>>
>> To allow for co-existence of NVMEM cells and other subnodes, would following patch be
>> acceptable to you and Srini?
>
> Gentle ping. Would the patch below be acceptable?
Did you have time to look at this?
>
>>
>> ---------------------------------------- 8< --------------------------------------
>> --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
>> +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
>> @@ -45,7 +45,15 @@ properties:
>> patternProperties:
>> "^.*@[0-9a-f]+$":
>> type: object
>> -
>> + if:
>> + not:
>> + properties:
>> + compatible:
>> + then:
>> + $ref: "#/definitions/nvmem-cell"
>> +
>> +definitions:
>> + nvmem-cell:
>> properties:
>> reg:
>> maxItems: 1
>
> Cheers,
> Ahmad
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 16/11/2020 17:04, Ahmad Fatoum wrote:
>>> To allow for co-existence of NVMEM cells and other subnodes, would following patch be
>>> acceptable to you and Srini?
>> Gentle ping. Would the patch below be acceptable?
> Did you have time to look at this?
>
I did reply back to this thread way back in June saying that
"Thanks for the patch, this looks good to me, lets wait for Rob to ack
the bindings! "
--srini
Hello Srini,
On 16.11.20 18:21, Srinivas Kandagatla wrote:
>
>
> On 16/11/2020 17:04, Ahmad Fatoum wrote:
>>>> To allow for co-existence of NVMEM cells and other subnodes, would following patch be
>>>> acceptable to you and Srini?
>>> Gentle ping. Would the patch below be acceptable?
>> Did you have time to look at this?
>>
>
> I did reply back to this thread way back in June saying that
>
> "Thanks for the patch, this looks good to me, lets wait for Rob to ack the bindings! "
Rob replied a day later saying that he would prefer it done otherwise.
What Rob suggested doesn't solve my actual issue, so I suggested another
solution a month ago. I have not yet received feedback on it, so that's
why I pinged:
https://lore.kernel.org/lkml/[email protected]/
>
>
> --srini
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Ahmad,
On 4/28/20 1:18 PM, Ahmad Fatoum wrote:
> The nvmem cell binding used to apply to all objects which match
> "^.*@[0-9a-f]+$", without taking a compatible into account, which
> precluded extension of EEPROMs by child nodes other than nvmem.
>
> A previous commit changed the binding, so that nvmem cells that
> feature a compatible property must have "nvmem-cell" as the value,
> otherwise they are skipped.
>
> Adjust the driver to observe the new binding change. This change
> does not change behavior for any device tree that was already
> compliant with the nvmem binding.
>
> Signed-off-by: Ahmad Fatoum <[email protected]>
Tested-by: Kamel Bouhara <[email protected]>
> ---
> drivers/nvmem/core.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 05c6ae4b0b97..eb697f5ad07d 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -547,6 +547,10 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
> parent = dev->of_node;
>
> for_each_child_of_node(parent, child) {
> + if (of_find_property(child, "compatible", NULL) &&
> + !of_device_is_compatible(child, "nvmem-cell"))
> + continue;
> +
> addr = of_get_property(child, "reg", &len);
> if (!addr || (len < 2 * sizeof(u32))) {
> dev_err(dev, "nvmem: invalid reg on %pOF\n", child);
>
--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Hello Kamel,
On 30.12.20 14:43, Kamel Bouhara wrote:
> Hi Ahmad,
>
> On 4/28/20 1:18 PM, Ahmad Fatoum wrote:
>> The nvmem cell binding used to apply to all objects which match
>> "^.*@[0-9a-f]+$", without taking a compatible into account, which
>> precluded extension of EEPROMs by child nodes other than nvmem.
>>
>> A previous commit changed the binding, so that nvmem cells that
>> feature a compatible property must have "nvmem-cell" as the value,
>> otherwise they are skipped.
>>
>> Adjust the driver to observe the new binding change. This change
>> does not change behavior for any device tree that was already
>> compliant with the nvmem binding.
>>
>> Signed-off-by: Ahmad Fatoum <[email protected]>
>
> Tested-by: Kamel Bouhara <[email protected]>
Thanks for testing. Could you share what use case you have for this?
Rob had objections to this approach:
https://lore.kernel.org/linux-devicetree/20200512141834.GA3023@bogus/
But solving it within a partition doesn't address my original problem:
https://lore.kernel.org/lkml/[email protected]/
I just sent out a v4, which added you in CC.
Cheers,
Ahmad
>
>> ---
>> drivers/nvmem/core.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index 05c6ae4b0b97..eb697f5ad07d 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -547,6 +547,10 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
>> parent = dev->of_node;
>> for_each_child_of_node(parent, child) {
>> + if (of_find_property(child, "compatible", NULL) &&
>> + !of_device_is_compatible(child, "nvmem-cell"))
>> + continue;
>> +
>> addr = of_get_property(child, "reg", &len);
>> if (!addr || (len < 2 * sizeof(u32))) {
>> dev_err(dev, "nvmem: invalid reg on %pOF\n", child);
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |