| Message ID | 20231221173421.13737-1-zajec5@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V3,1/6] dt-bindings: nvmem: layouts: add U-Boot environment variables layout | expand |
On Thu, Dec 21, 2023 at 06:34:16PM +0100, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > U-Boot env data is a way of storing firmware variables. It's a format > that can be used of top of various storage devices. Its binding should > be an NVMEM layout instead of a standalone device. > > This patch adds layout binding which allows using it on top of MTD NVMEM > device as well as any other. At the same time it deprecates the old > combined binding. I don't understand the issue. From a DT perspective, there isn't. A partition is not a device, but is describing the layout of storage already. Rob
Hello, robh@kernel.org wrote on Wed, 3 Jan 2024 17:11:29 -0700: > On Thu, Dec 21, 2023 at 06:34:16PM +0100, Rafał Miłecki wrote: > > From: Rafał Miłecki <rafal@milecki.pl> > > > > U-Boot env data is a way of storing firmware variables. It's a format > > that can be used of top of various storage devices. Its binding should > > be an NVMEM layout instead of a standalone device. > > > > This patch adds layout binding which allows using it on top of MTD NVMEM > > device as well as any other. At the same time it deprecates the old > > combined binding. > > I don't understand the issue. From a DT perspective, there isn't. A > partition is not a device, but is describing the layout of storage > already. Actually I think what Rafał wants to do goes in the right direction but I also understand from a binding perspective it may be a little confusing, even more if we consider "NVMEM" a Linux specific concept. There is today a "u-boot env" NVMEM *device* description which almost sits at the same level as eg. an eeprom device. We cannot compare "an eeprom device" and "a u-boot environment" of course. But that's truly what is currently described. * Current situation Flash device -> U-Boot env layout -> NVMEM cells * Improved situation Any storage device -> NVMEM -> U-Boot env layout -> NVMEM cells The latter is of course the most relevant description as we expect storage devices to expose a storage-agnostic interface (NVMEM in this case) which can then be parsed (by NVMEM layouts) in a storage agnostic way. In the current case, the current U-Boot env binding tells people to declare the env layout on top of a flash device (only). The current description also expects a partition node which is typical to flash devices. Whereas what we should have described in the first place is a layout that applies on any kind of NVMEM device. Bonus point: We've been working the last couple years on clarifying bindings, especially with mtd partitions (with the partitions{} container) and NVMEM layouts (with the nvmem-layout{} container). The switch proposed in this patch makes use of the latter, of course. Thanks, Miquèl
On 4.01.2024 08:58, Miquel Raynal wrote: > robh@kernel.org wrote on Wed, 3 Jan 2024 17:11:29 -0700: >> On Thu, Dec 21, 2023 at 06:34:16PM +0100, Rafał Miłecki wrote: >>> From: Rafał Miłecki <rafal@milecki.pl> >>> >>> U-Boot env data is a way of storing firmware variables. It's a format >>> that can be used of top of various storage devices. Its binding should >>> be an NVMEM layout instead of a standalone device. >>> >>> This patch adds layout binding which allows using it on top of MTD NVMEM >>> device as well as any other. At the same time it deprecates the old >>> combined binding. >> >> I don't understand the issue. From a DT perspective, there isn't. A >> partition is not a device, but is describing the layout of storage >> already. > > Actually I think what Rafał wants to do goes in the right direction but > I also understand from a binding perspective it may be a little > confusing, even more if we consider "NVMEM" a Linux specific concept. > > There is today a "u-boot env" NVMEM *device* description which > almost sits at the same level as eg. an eeprom device. We cannot > compare "an eeprom device" and "a u-boot environment" of course. But > that's truly what is currently described. > > * Current situation > > Flash device -> U-Boot env layout -> NVMEM cells > > * Improved situation > > Any storage device -> NVMEM -> U-Boot env layout -> NVMEM cells > > The latter is of course the most relevant description as we expect > storage devices to expose a storage-agnostic interface (NVMEM in > this case) which can then be parsed (by NVMEM layouts) in a storage > agnostic way. > > In the current case, the current U-Boot env binding tells people to > declare the env layout on top of a flash device (only). The current > description also expects a partition node which is typical to flash > devices. Whereas what we should have described in the first place is a > layout that applies on any kind of NVMEM device. > > Bonus point: We've been working the last couple years on clarifying > bindings, especially with mtd partitions (with the partitions{} > container) and NVMEM layouts (with the nvmem-layout{} container). > The switch proposed in this patch makes use of the latter, of course. Thanks Miquèl for filling bits I missed in commit description. Despite years in Linux/DT I still struggle with more complex designs documentation. As per Rob's comment I think I see his point and a possible design confusion. If you look from a pure DT perspective then "partitions" and "nvmem-layout" serve a very similar purpose. They describe device's data content structure. For fixed structures we have very similar "fixed-partitions" and "fixed-cells". If we were to design those bindings today I'm wondering if we couldn't have s/partitions/layout/ and s/nvmem-layout/layout/. Rob: other than having different bindings for MTD vs. NVMEM layouts I think they overall design makes sense. A single device may have content structurized on more than 1 level: 1. You may have fixed layout at top level (multiple partitions) 2. Single partitions may have their own layouts (like U-Boot env data) Maybe ideally above should look more like: flash@0 { compatible = "<flash-compatible>"; layout { compatible = "fixed-layout"; #address-cells = <1>; #size-cells = <1>; partition@0 { reg = <0x0 0x40000>; label = "u-boot"; }; partition@40000 { reg = <0x40000 0x10000>; label = "u-boot-env"; layout { compatible = "u-boot,env-layout"; }; }; partition@50000 { reg = <0x50000 0x100000>; label = "u-boot"; }; }; }; but I can clearly see a use for nested "layout"s. As I said maybe we just shouldn't be so open in calling those MTD or NVMEM devices as that is kind of Linux specific. I'm not sure if we should try renaming "nvmem-layout" to "layout" or "partitions" in similar way at this point.
On Thu, Jan 04, 2024 at 10:10:13AM +0100, Rafał Miłecki wrote: > On 4.01.2024 08:58, Miquel Raynal wrote: > > robh@kernel.org wrote on Wed, 3 Jan 2024 17:11:29 -0700: > > > On Thu, Dec 21, 2023 at 06:34:16PM +0100, Rafał Miłecki wrote: > > > > From: Rafał Miłecki <rafal@milecki.pl> > > > > > > > > U-Boot env data is a way of storing firmware variables. It's a format > > > > that can be used of top of various storage devices. Its binding should > > > > be an NVMEM layout instead of a standalone device. > > > > > > > > This patch adds layout binding which allows using it on top of MTD NVMEM > > > > device as well as any other. At the same time it deprecates the old > > > > combined binding. > > > > > > I don't understand the issue. From a DT perspective, there isn't. A > > > partition is not a device, but is describing the layout of storage > > > already. > > > > Actually I think what Rafał wants to do goes in the right direction but > > I also understand from a binding perspective it may be a little > > confusing, even more if we consider "NVMEM" a Linux specific concept. > > > > There is today a "u-boot env" NVMEM *device* description which > > almost sits at the same level as eg. an eeprom device. We cannot > > compare "an eeprom device" and "a u-boot environment" of course. But > > that's truly what is currently described. > > > > * Current situation > > > > Flash device -> U-Boot env layout -> NVMEM cells Isn't it?: Flash device -> fixed-partitions -> U-Boot env layout -> NVMEM cells > > > > * Improved situation > > > > Any storage device -> NVMEM -> U-Boot env layout -> NVMEM cells Why is this better? We don't need a container to say 'this is NVMEM stuff' or 'this is MTD stuff'. 'U-Boot env layout' can tell us 'this is NVMEM stuff' or whatever the kernel decides in the future. > > > > The latter is of course the most relevant description as we expect > > storage devices to expose a storage-agnostic interface (NVMEM in > > this case) which can then be parsed (by NVMEM layouts) in a storage > > agnostic way. > > > > In the current case, the current U-Boot env binding tells people to > > declare the env layout on top of a flash device (only). The current > > description also expects a partition node which is typical to flash > > devices. Whereas what we should have described in the first place is a > > layout that applies on any kind of NVMEM device. > > > > Bonus point: We've been working the last couple years on clarifying > > bindings, especially with mtd partitions (with the partitions{} > > container) and NVMEM layouts (with the nvmem-layout{} container). > > The switch proposed in this patch makes use of the latter, of course. > > Thanks Miquèl for filling bits I missed in commit description. Despite > years in Linux/DT I still struggle with more complex designs > documentation. > > > As per Rob's comment I think I see his point and a possible design > confusion. If you look from a pure DT perspective then "partitions" and > "nvmem-layout" serve a very similar purpose. They describe device's data > content structure. For fixed structures we have very similar > "fixed-partitions" and "fixed-cells". > > If we were to design those bindings today I'm wondering if we couldn't > have s/partitions/layout/ and s/nvmem-layout/layout/. Why!? It is just a name, and we can't get rid of the old names. We don't need 2 names. > Rob: other than having different bindings for MTD vs. NVMEM layouts I > think they overall design makes sense. A single device may have content > structurized on more than 1 level: > 1. You may have fixed layout at top level (multiple partitions) > 2. Single partitions may have their own layouts (like U-Boot env data) Sure. Partitions is for 1 and Layouts is for 2. > Maybe ideally above should look more like: > > flash@0 { > compatible = "<flash-compatible>"; > > layout { > compatible = "fixed-layout"; Why does 'partitions' and 'fixed-partitions' not work here? > #address-cells = <1>; > #size-cells = <1>; > > partition@0 { > reg = <0x0 0x40000>; > label = "u-boot"; > }; > > partition@40000 { > reg = <0x40000 0x10000>; > label = "u-boot-env"; > > layout { > compatible = "u-boot,env-layout"; > }; > }; > > partition@50000 { > reg = <0x50000 0x100000>; > label = "u-boot"; > }; > }; > }; > > but I can clearly see a use for nested "layout"s. As I said maybe we > just shouldn't be so open in calling those MTD or NVMEM devices as that > is kind of Linux specific. The overall structure should be agnostic to the subsystem. Specific compatibles like 'u-boot,env' can be tied to a subsystem. Maybe some things need to be both MTD and NVMEM. MTD to operate on the opague region and NVMEM to access the contents. > I'm not sure if we should try renaming "nvmem-layout" to "layout" or > "partitions" in similar way at this point. You can't rename. It's an ABI though maybe the whole "nvmem-layout" is new enough we can. It's looking like it was a mistake to accept any of this. Rob
Hi Rob, robh@kernel.org wrote on Mon, 15 Jan 2024 11:09:03 -0600: > On Thu, Jan 04, 2024 at 10:10:13AM +0100, Rafał Miłecki wrote: > > On 4.01.2024 08:58, Miquel Raynal wrote: > > > robh@kernel.org wrote on Wed, 3 Jan 2024 17:11:29 -0700: > > > > On Thu, Dec 21, 2023 at 06:34:16PM +0100, Rafał Miłecki wrote: > > > > > From: Rafał Miłecki <rafal@milecki.pl> > > > > > > > > > > U-Boot env data is a way of storing firmware variables. It's a format > > > > > that can be used of top of various storage devices. Its binding should > > > > > be an NVMEM layout instead of a standalone device. > > > > > > > > > > This patch adds layout binding which allows using it on top of MTD NVMEM > > > > > device as well as any other. At the same time it deprecates the old > > > > > combined binding. > > > > > > > > I don't understand the issue. From a DT perspective, there isn't. A > > > > partition is not a device, but is describing the layout of storage > > > > already. > > > > > > Actually I think what Rafał wants to do goes in the right direction but > > > I also understand from a binding perspective it may be a little > > > confusing, even more if we consider "NVMEM" a Linux specific concept. > > > > > > There is today a "u-boot env" NVMEM *device* description which > > > almost sits at the same level as eg. an eeprom device. We cannot > > > compare "an eeprom device" and "a u-boot environment" of course. But > > > that's truly what is currently described. > > > > > > * Current situation > > > > > > Flash device -> U-Boot env layout -> NVMEM cells > > Isn't it?: > > Flash device -> fixed-partitions -> U-Boot env layout -> NVMEM cells > > > > > > > * Improved situation > > > > > > Any storage device -> NVMEM -> U-Boot env layout -> NVMEM cells > > Why is this better? We don't need a container to say 'this is NVMEM > stuff' or 'this is MTD stuff'. 'U-Boot env layout' can tell us 'this is > NVMEM stuff' or whatever the kernel decides in the future. Yes, I also want the U-boot env layout to tell us "this is nvmem stuff". But that's not the case today. Today, it says "this is NVMEM stuff on top of mtd stuff". This was a mistake in the first place, but this compatible is heavily tight to mtd and cannot work on anything else. And correcting this is IMO the right direction. > > > The latter is of course the most relevant description as we expect > > > storage devices to expose a storage-agnostic interface (NVMEM in > > > this case) which can then be parsed (by NVMEM layouts) in a storage > > > agnostic way. > > > > > > In the current case, the current U-Boot env binding tells people to > > > declare the env layout on top of a flash device (only). The current > > > description also expects a partition node which is typical to flash > > > devices. Whereas what we should have described in the first place is a > > > layout that applies on any kind of NVMEM device. > > > > > > Bonus point: We've been working the last couple years on clarifying > > > bindings, especially with mtd partitions (with the partitions{} > > > container) and NVMEM layouts (with the nvmem-layout{} container). > > > The switch proposed in this patch makes use of the latter, of course. > > > > Thanks Miquèl for filling bits I missed in commit description. Despite > > years in Linux/DT I still struggle with more complex designs > > documentation. > > > > > > As per Rob's comment I think I see his point and a possible design > > confusion. If you look from a pure DT perspective then "partitions" and > > "nvmem-layout" serve a very similar purpose. They describe device's data > > content structure. For fixed structures we have very similar > > "fixed-partitions" and "fixed-cells". > > > > If we were to design those bindings today I'm wondering if we couldn't > > have s/partitions/layout/ and s/nvmem-layout/layout/. > > Why!? It is just a name, and we can't get rid of the old names. We don't > need 2 names. We need 2 names because we are not capturing the same concepts here? > > Rob: other than having different bindings for MTD vs. NVMEM layouts I > > think they overall design makes sense. A single device may have content > > structurized on more than 1 level: > > 1. You may have fixed layout at top level (multiple partitions) > > 2. Single partitions may have their own layouts (like U-Boot env data) > > Sure. Partitions is for 1 and Layouts is for 2. > > > Maybe ideally above should look more like: > > > > flash@0 { > > compatible = "<flash-compatible>"; > > > > layout { > > compatible = "fixed-layout"; > > Why does 'partitions' and 'fixed-partitions' not work here? They do, and that's actually what we use. This example just illustrates another proposal from Rafal. No panic :) > > > #address-cells = <1>; > > #size-cells = <1>; > > > > partition@0 { > > reg = <0x0 0x40000>; > > label = "u-boot"; > > }; > > > > partition@40000 { > > reg = <0x40000 0x10000>; > > label = "u-boot-env"; > > > > layout { > > compatible = "u-boot,env-layout"; > > }; > > }; > > > > partition@50000 { > > reg = <0x50000 0x100000>; > > label = "u-boot"; > > }; > > }; > > }; > > > > but I can clearly see a use for nested "layout"s. As I said maybe we > > just shouldn't be so open in calling those MTD or NVMEM devices as that > > is kind of Linux specific. > > The overall structure should be agnostic to the subsystem. Specific > compatibles like 'u-boot,env' can be tied to a subsystem. > > Maybe some things need to be both MTD and NVMEM. MTD to operate on the > opague region and NVMEM to access the contents. > > > > I'm not sure if we should try renaming "nvmem-layout" to "layout" or > > "partitions" in similar way at this point. > > You can't rename. It's an ABI though maybe the whole "nvmem-layout" is > new enough we can. It's looking like it was a mistake to accept any of > this. I don't think so. A partition and a layout are not the same concept, as acknowledged above. We need both, and we need both because we can encapsulate both as well: flash { partitions { partA@x { layout { cell@Y } } } } Renaming nvmem-layout to layout can be done if you want, I don't mind, but I don't see the point in doing that. Thanks, Miquèl
diff --git a/Documentation/devicetree/bindings/nvmem/layouts/u-boot,env-layout.yaml b/Documentation/devicetree/bindings/nvmem/layouts/u-boot,env-layout.yaml new file mode 100644 index 000000000000..3b4d8f2a44e1 --- /dev/null +++ b/Documentation/devicetree/bindings/nvmem/layouts/u-boot,env-layout.yaml @@ -0,0 +1,55 @@ +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/nvmem/layouts/u-boot,env-layout.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: NVMEM layout of U-Boot environment variables + +maintainers: + - Rafał Miłecki <rafal@milecki.pl> + +description: + U-Boot uses environment variables to store device parameters and + configuration. They may be used for booting process, setup or keeping end user + info. + + Data is stored using U-Boot specific formats (variant specific header and NUL + separated key-value pairs). + +properties: + compatible: + oneOf: + - description: A standalone env data block + const: u-boot,env-layout + - description: Two redundant blocks with active one flagged + const: u-boot,env-redundant-bool-layout + - description: Two redundant blocks with active having higher counter + const: u-boot,env-redundant-count-layout + - description: Broadcom's variant with custom header + const: brcm,env-layout + +additionalProperties: false + +examples: + - | + partitions { + compatible = "fixed-partitions"; + #address-cells = <1>; + #size-cells = <1>; + + partition@0 { + reg = <0x0 0x40000>; + label = "u-boot"; + read-only; + }; + + partition@40000 { + reg = <0x40000 0x10000>; + label = "u-boot-env"; + + nvmem-layout { + compatible = "u-boot,env-layout"; + }; + }; + }; diff --git a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml index 9c36afc7084b..6c2a3ca5f051 100644 --- a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml +++ b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml @@ -26,9 +26,15 @@ description: | Variables can be defined as NVMEM device subnodes. + Introduction of NVMEM layouts exposed a limitation of this binding design. + Description of NVMEM data content should be separated from NVMEM devices. + Since the introduction of U-Boot env data layout this binding is deprecated. + maintainers: - Rafał Miłecki <rafal@milecki.pl> +deprecated: true + properties: compatible: oneOf: