* state framework, fixed-partitions, eeprom and linux
@ 2020-02-03 12:12 Christian Eggers
2020-02-03 17:17 ` Ahmad Fatoum
0 siblings, 1 reply; 6+ messages in thread
From: Christian Eggers @ 2020-02-03 12:12 UTC (permalink / raw)
To: barebox
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: state framework, fixed-partitions, eeprom and linux 2020-02-03 12:12 state framework, fixed-partitions, eeprom and linux Christian Eggers @ 2020-02-03 17:17 ` Ahmad Fatoum 2020-02-03 17:47 ` Ahmad Fatoum 0 siblings, 1 reply; 6+ messages in thread From: Ahmad Fatoum @ 2020-02-03 17:17 UTC (permalink / raw) To: barebox 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: state framework, fixed-partitions, eeprom and linux 2020-02-03 17:17 ` Ahmad Fatoum @ 2020-02-03 17:47 ` Ahmad Fatoum 2020-02-04 13:06 ` Christian Eggers 0 siblings, 1 reply; 6+ messages in thread From: Ahmad Fatoum @ 2020-02-03 17:47 UTC (permalink / raw) To: barebox 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: state framework, fixed-partitions, eeprom and linux 2020-02-03 17:47 ` Ahmad Fatoum @ 2020-02-04 13:06 ` Christian Eggers 2020-02-04 13:45 ` Ahmad Fatoum 2021-03-05 20:35 ` Ahmad Fatoum 0 siblings, 2 replies; 6+ messages in thread From: Christian Eggers @ 2020-02-04 13:06 UTC (permalink / raw) To: barebox; +Cc: Ahmad Fatoum 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: state framework, fixed-partitions, eeprom and linux 2020-02-04 13:06 ` Christian Eggers @ 2020-02-04 13:45 ` Ahmad Fatoum 2021-03-05 20:35 ` Ahmad Fatoum 1 sibling, 0 replies; 6+ messages in thread From: Ahmad Fatoum @ 2020-02-04 13:45 UTC (permalink / raw) To: Christian Eggers, barebox 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: state framework, fixed-partitions, eeprom and linux 2020-02-04 13:06 ` Christian Eggers 2020-02-04 13:45 ` Ahmad Fatoum @ 2021-03-05 20:35 ` Ahmad Fatoum 1 sibling, 0 replies; 6+ messages in thread From: Ahmad Fatoum @ 2021-03-05 20:35 UTC (permalink / raw) To: Christian Eggers, barebox 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-05 20:37 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-03 12:12 state framework, fixed-partitions, eeprom and linux Christian Eggers 2020-02-03 17:17 ` Ahmad Fatoum 2020-02-03 17:47 ` Ahmad Fatoum 2020-02-04 13:06 ` Christian Eggers 2020-02-04 13:45 ` Ahmad Fatoum 2021-03-05 20:35 ` Ahmad Fatoum
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox