Re: [PATCH V3 1/6] dt-bindings: nvmem: layouts: add U-Boot environment variables layout

On Thu, Jan 04, 2024 at 10:10:13AM +0100, Rafał Miłecki wrote:

On 4.01.2024 08:58, Miquel Raynal wrote:

quoted

robh@kernel.org wrote on Wed, 3 Jan 2024 17:11:29 -0700:

quoted

On Thu, Dec 21, 2023 at 06:34:16PM +0100, Rafał Miłecki wrote:

quoted

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

quoted

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

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel