state framework, fixed-partitions, eeprom and linux

state framework, fixed-partitions, eeprom and linux

[Christian Eggers]

I've tried to use the state framework with an SPI eeprom as described here:
https://www.barebox.org/doc/latest/user/state.html#eeprom

The eeprom uses a "partition" for the state data:
------------------------8<---------------------------
eeprom@50 {
        partitions {
                compatible = "fixed-partitions";
                #size-cells = <1>;
                #address-cells = <1>;
                backend_state_eeprom: eeprom_state_memory@400 {
                        reg = <0x400 0x100>;
                        label = "state-eeprom";
                };
        };
};
------------------------>8---------------------------


It looks like there a 2 different concepts of having partitions in flashes:
1. As direct subnodes to the eeprom/flash
2. As subnodes within a "fixed-partition" subnode (as in the example above)

From linux/Documentation/devicetree/bindings/mtd/partition.txt:
"For backwards compatibility partitions as direct subnodes of the flash device 
are supported. This use is discouraged."

So using the 2nd approach is right, isn't it?

Trying to use this with the at25 nvmem driver in Linux, I get the following 
error:

nvmem spi0.00: nvmem: invalid reg on /soc/aips-bus@2000000/spba-bus@2000000/
spi@2008000/fram@0/partitions

Looking into nvmem_add_cells_from_of() in the linux sources, the NVMEM code 
seems to differ from the MTD core. It only expects the partitions as direct 
subnodes (without "fixed-partitions").

In Barebox, of_partition_fixup() can be configured using the 
global.of_partition_bindingof_partition_binding variable. But I couldn't find 
any user of this and this would probably affect both, NVMEM and MTD.

From the barebox point of view it seems best to add "fixed-partitions" support 
to Linux NVMEM. Any other suggests?

regards
Christian




_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: state framework, fixed-partitions, eeprom and linux

[Ahmad Fatoum]

Hello Christian,

On 2/3/20 1:12 PM, Christian Eggers wrote:
> I've tried to use the state framework with an SPI eeprom as described here:
> https://www.barebox.org/doc/latest/user/state.html#eeprom
> 
> The eeprom uses a "partition" for the state data:
> ------------------------8<---------------------------
> eeprom@50 {
>         partitions {
>                 compatible = "fixed-partitions";
>                 #size-cells = <1>;
>                 #address-cells = <1>;
>                 backend_state_eeprom: eeprom_state_memory@400 {
>                         reg = <0x400 0x100>;
>                         label = "state-eeprom";
>                 };
>         };
> };
> ------------------------>8---------------------------
> 
> 
> It looks like there a 2 different concepts of having partitions in flashes:
> 1. As direct subnodes to the eeprom/flash
> 2. As subnodes within a "fixed-partition" subnode (as in the example above)
> 
> From linux/Documentation/devicetree/bindings/mtd/partition.txt:
> "For backwards compatibility partitions as direct subnodes of the flash device 
> are supported. This use is discouraged."
> 
> So using the 2nd approach is right, isn't it?
> 
> Trying to use this with the at25 nvmem driver in Linux, I get the following 
> error:
> 
> nvmem spi0.00: nvmem: invalid reg on /soc/aips-bus@2000000/spba-bus@2000000/
> spi@2008000/fram@0/partitions
> 
> Looking into nvmem_add_cells_from_of() in the linux sources, the NVMEM code 
> seems to differ from the MTD core. It only expects the partitions as direct 
> subnodes (without "fixed-partitions").
> 
> In Barebox, of_partition_fixup() can be configured using the 
> global.of_partition_bindingof_partition_binding variable. But I couldn't find 
> any user of this and this would probably affect both, NVMEM and MTD.

Use is usually in the environment which is patched in by the BSP.

> From the barebox point of view it seems best to add "fixed-partitions" support 
> to Linux NVMEM. Any other suggests?

A container node would be preferable yes, but for reasons of backwards-compatibility,
Kernel support for the old binding will likely continue, which clashes with our
EEPROM partitioning.

So, either we adapt (always fix up eeprom nodes as old, so they pass as kernel nvmem)
or the kernel binding changes.

I would be in favor of:

        for_each_child_of_node(parent, child) {
+               if (of_property_read_bool(child, "compatible"))
+                       continue;
                addr = of_get_property(child, "reg", &len);

in the NVMEM core. I don't have thought out the argument to back it yet though.

Thoughts? 

Ahmad



> 
> regards
> Christian
> 
> 
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

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

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: state framework, fixed-partitions, eeprom and linux

[Ahmad Fatoum]

On 2/3/20 6:17 PM, Ahmad Fatoum wrote:
> Hello Christian,
>> Trying to use this with the at25 nvmem driver in Linux, I get the following 
>> error:
>>
>> nvmem spi0.00: nvmem: invalid reg on /soc/aips-bus@2000000/spba-bus@2000000/
>> spi@2008000/fram@0/partitions
>>
>> Looking into nvmem_add_cells_from_of() in the linux sources, the NVMEM code 
>> seems to differ from the MTD core. It only expects the partitions as direct 
>> subnodes (without "fixed-partitions").
>>
>> In Barebox, of_partition_fixup() can be configured using the 
>> global.of_partition_bindingof_partition_binding variable. But I couldn't find 
>> any user of this and this would probably affect both, NVMEM and MTD.
> 
> Use is usually in the environment which is patched in by the BSP.
> 
>> From the barebox point of view it seems best to add "fixed-partitions" support 
>> to Linux NVMEM. Any other suggests?
> 
> A container node would be preferable yes, but for reasons of backwards-compatibility,
> Kernel support for the old binding will likely continue, which clashes with our
> EEPROM partitioning.

Fortunately, I was mistaken. The upstream bindings says that only objects
matching "^.*@[0-9a-f]+$" should be considered for nvmem cells. a partitions node
doesn't match this. So I'd instead suggest this:

    nvmem: core: don't consider subnodes not matching binding
    
    The nvmem cell binding applies to objects which match "^.*@[0-9a-f]+$",
    but so far the driver has matched all objects and failed if they didn't
    have the expected properties.
    
    The driver's behavior in this regard precludes future extension of
    EEPROMs by child nodes other than nvmem and clashes with the barebox
    bootloader binding that extends the fixed-partitions MTD binding to
    EEPROMs.
    
    Solve this issue by checking whether the node name contains a @.
    This still matches against node names like partitions@0,0, but this is
    much less likely to cause future collisions.
    
    Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 9f1ee9c766ec..254ac1bb6066 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -269,6 +269,8 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
        parent = dev->of_node;
 
        for_each_child_of_node(parent, child) {
+               if (!strchr(child->name, '@'))
+                       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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: state framework, fixed-partitions, eeprom and linux

[Christian Eggers]

Dear Ahmad,

Am Montag, 3. Februar 2020, 18:47:29 CET schrieb Ahmad Fatoum:
> Fortunately, I was mistaken. The upstream bindings says that only objects
> matching "^.*@[0-9a-f]+$" should be considered for nvmem cells. a partitions
> node doesn't match this. So I'd instead suggest this:
> 
>     nvmem: core: don't consider subnodes not matching binding
> 
>     The nvmem cell binding applies to objects which match "^.*@[0-9a-f]+$",
>     but so far the driver has matched all objects and failed if they didn't
>     have the expected properties.
> 
>     The driver's behavior in this regard precludes future extension of
>     EEPROMs by child nodes other than nvmem and clashes with the barebox
>     bootloader binding that extends the fixed-partitions MTD binding to
>     EEPROMs.
> 
>     Solve this issue by checking whether the node name contains a @.
>     This still matches against node names like partitions@0,0, but this is
>     much less likely to cause future collisions.
> 
>     Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 

I have a different approach, taken from the Linux MTD sources. Here the DT partitions are added as NVMEM cells. Is there any conceptual difference between "MTD partitions" and "NVMEM cells"? 

Are you able to bring one/both patches into Linux?

Regards
Christian

From 927f3aa9c4a64802f25ef2f292caa1dc951ce667 Mon Sep 17 00:00:00 2001
From: Christian Eggers <ceggers@arri.de>
Date: Tue, 4 Feb 2020 13:52:36 +0100
Subject: [PATCH] nvmem: core: Move OF cells to a dedicated dt node

In 5cfdedb7b9 ("mtd: ofpart: move ofpart partitions to a dedicated dt
node"), mtd introduced the separate "partitions" node as container for
the partitions of a MTD device.

This commit applies a similar behavior to NVMEM partitions/cells.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 drivers/nvmem/core.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 057d1ff87d5d..3e93b82b96bd 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -287,7 +287,7 @@ nvmem_find_cell_by_name(struct nvmem_device *nvmem, const char *cell_id)
 
 static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
 {
-	struct device_node *parent, *child;
+	struct device_node *parent, *child, *ofpart_node;
 	struct device *dev = &nvmem->dev;
 	struct nvmem_cell *cell;
 	const __be32 *addr;
@@ -295,7 +295,16 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
 
 	parent = dev->of_node;
 
-	for_each_child_of_node(parent, child) {
+	ofpart_node = of_get_child_by_name(parent, "partitions");
+	if (!ofpart_node) {
+		/* Try to parse direct subnodes */
+		ofpart_node = parent;
+	} else if (!of_device_is_compatible(ofpart_node, "fixed-partitions")) {
+		/* The 'partitions' subnode might be used by another parser */
+		return 0;
+	}
+
+	for_each_child_of_node(ofpart_node, child) {
 		addr = of_get_property(child, "reg", &len);
 		if (!addr || (len < 2 * sizeof(u32))) {
 			dev_err(dev, "nvmem: invalid reg on %pOF\n", child);
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler







_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: state framework, fixed-partitions, eeprom and linux

[Ahmad Fatoum]

Hello Christian,

On 2/4/20 2:06 PM, Christian Eggers wrote:
> Dear Ahmad,
> 
> Am Montag, 3. Februar 2020, 18:47:29 CET schrieb Ahmad Fatoum:
>> Fortunately, I was mistaken. The upstream bindings says that only objects
>> matching "^.*@[0-9a-f]+$" should be considered for nvmem cells. a partitions
>> node doesn't match this. So I'd instead suggest this:
>>
>>     nvmem: core: don't consider subnodes not matching binding
>>
>>     The nvmem cell binding applies to objects which match "^.*@[0-9a-f]+$",
>>     but so far the driver has matched all objects and failed if they didn't
>>     have the expected properties.
>>
>>     The driver's behavior in this regard precludes future extension of
>>     EEPROMs by child nodes other than nvmem and clashes with the barebox
>>     bootloader binding that extends the fixed-partitions MTD binding to
>>     EEPROMs.
>>
>>     Solve this issue by checking whether the node name contains a @.
>>     This still matches against node names like partitions@0,0, but this is
>>     much less likely to cause future collisions.
>>
>>     Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>
> 
> I have a different approach, taken from the Linux MTD sources. Here the DT partitions are added as NVMEM cells. Is there any conceptual difference between "MTD partitions" and "NVMEM cells"? 

Theoretically, you could have both NVMEM cells and MTD partitions in the same node.
There has been a patch introducing this binding[1], but it seems it was abandoned.

But even without this, they are different subsystems with different device tree
bindings, in-kernel API for accessing and userspace interface and I don't think
mixing them is a good idea.
 
> Are you able to bring one/both patches into Linux?

I can try posting my patch. I'll wait a bit to see if someone has feedback.
I will add you to CC when I send it out.

Cheers
Ahmad

[1]: https://lore.kernel.org/patchwork/patch/765436/

> 
> Regards
> Christian
> 
> From 927f3aa9c4a64802f25ef2f292caa1dc951ce667 Mon Sep 17 00:00:00 2001
> From: Christian Eggers <ceggers@arri.de>
> Date: Tue, 4 Feb 2020 13:52:36 +0100
> Subject: [PATCH] nvmem: core: Move OF cells to a dedicated dt node
> 
> In 5cfdedb7b9 ("mtd: ofpart: move ofpart partitions to a dedicated dt
> node"), mtd introduced the separate "partitions" node as container for
> the partitions of a MTD device.
> 
> This commit applies a similar behavior to NVMEM partitions/cells.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---
>  drivers/nvmem/core.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 057d1ff87d5d..3e93b82b96bd 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -287,7 +287,7 @@ nvmem_find_cell_by_name(struct nvmem_device *nvmem, const char *cell_id)
>  
>  static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
>  {
> -	struct device_node *parent, *child;
> +	struct device_node *parent, *child, *ofpart_node;
>  	struct device *dev = &nvmem->dev;
>  	struct nvmem_cell *cell;
>  	const __be32 *addr;
> @@ -295,7 +295,16 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
>  
>  	parent = dev->of_node;
>  
> -	for_each_child_of_node(parent, child) {
> +	ofpart_node = of_get_child_by_name(parent, "partitions");
> +	if (!ofpart_node) {
> +		/* Try to parse direct subnodes */
> +		ofpart_node = parent;
> +	} else if (!of_device_is_compatible(ofpart_node, "fixed-partitions")) {
> +		/* The 'partitions' subnode might be used by another parser */
> +		return 0;
> +	}
> +
> +	for_each_child_of_node(ofpart_node, child) {
>  		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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: state framework, fixed-partitions, eeprom and linux

[Ahmad Fatoum]

Hello Christian,

On 04.02.20 14:06, Christian Eggers wrote:
> I have a different approach, taken from the Linux MTD sources. Here the DT partitions are added as NVMEM cells. Is there any conceptual difference between "MTD partitions" and "NVMEM cells"? 
> 
> Are you able to bring one/both patches into Linux?

It took a while, but it's now included as of v5.12-rc1 as 0445efacec75
("nvmem: core: skip child nodes not matching binding").
Newest stable releases for v5.4. v5.10 and v5.11 also include it.

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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox