Re: [PATCH] dt-bindings: nvmem: add U-Boot environment variables binding

From: Michal Simek
Date: Wed Feb 16 2022 - 09:24:41 EST




On 2/16/22 13:54, Rafał Miłecki wrote:
On 15.02.2022 15:02, Michal Simek wrote:
On 2/15/22 14:49, Rafał Miłecki wrote:
From: Rafał Miłecki <rafal@xxxxxxxxxx>

U-Boot uses environment variables for storing device setup data on
flash. That data usually needs to be accessed by a bootloader, kernel
and often user-space.

This binding allows describing environment data location and its format
clearly. In some/many cases it should be cleaner than hardcoding &
duplicating that info in multiple places. Bootloader & kernel can share
DTS and user-space can try reading it too or just have correct data
exposed by a kernel.

Signed-off-by: Rafał Miłecki <rafal@xxxxxxxxxx>
---
  .../devicetree/bindings/nvmem/u-boot,env.yaml | 58 +++++++++++++++++++
  MAINTAINERS                                   |  5 ++
  2 files changed, 63 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/nvmem/u-boot,env.yaml

diff --git a/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
new file mode 100644
index 000000000000..a2b3a9b88eb8
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/u-boot,env.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: U-Boot environment variables
+
+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 on flash in a U-Boot specific format (header and NUL separated
+  key-value pairs).
+
+  This binding allows specifying data location and used format.
+
+maintainers:
+  - Rafał Miłecki <rafal@xxxxxxxxxx>
+
+allOf:
+  - $ref: nvmem.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - description: A standalone env data block
+        const: u-boot,env
+      - description: Two redundant blocks with active one flagged
+        const: u-boot,env-redundant-bool
+      - description: Two redundant blocks with active having higher counter
+        const: u-boot,env-redundant-count

I am not convinced that this is the best way how to do it. Because in u-boot implementation you would have to enable MTD partitions to get there.
And the whole parsing will take a lot of time.

We'll need to find some consensus considering all points:
1. DT objectives
2. U-Boot needs
3. Linux needs

DT should mainly describe hardware / platform without focusing on a
single implementation details. If U-Boot env data is indeed stored in a
flash block (or blocks) / UBI volume, its binding should be just that.

If U-Boot requires MTD to parse proposed binding and it can't be
afforded at the same time - maybe it can come with different
implementation?

I am ok to even say if you want to use this description you should have some options enabled.



I think the way how I think this can be handled is.

One minor note: I don't think you can have one "standard" format and one
"redundant" format. If env data is stored in two places - both use the
redundant format.

What you have below is fine for me. I am just saying that it should be pretty much generic and description should be able to cover all configurations.

Another thing what came to my mind was that with the same description should be possible to describe for u-boot for example where boot scripts are placed in qspi which can be different based on spi size.



# I don't think that discussion with Simon was finished.
But for example (chosen or firmware node)
chosen {
     u-boot {
         u-boot,env = <&qspi &part0>;
         u-boot,env-redundant = <&qspi &part1>;

1. Using &qspi seems reundant here, you can get parent flash device by
   walking DT.

correct but on the other hand code should be able to do it in a easy way. If this is easy to implement just remove it.

2. Using "chosen" seems to be a /shortcut/ for getting env data
   location, I don't see any direct conflict with using "compatible"
   string as proposed in my binding.

If compatible string should be added there then you need to maintain it.
Also if this is purely based on compatible string only then you need to parse all nodes to find out where it is.
Having fixed location in any node will give you this information without need to parse a lot of data via DT.
And not sure when exactly in u-boot this is needed that you would have to avoid to use u-boot,pre-reloc flags to get to it.



         #or
         u-boot,env = <&qspi 0 40000>;
         u-boot,env-redundant = <&qspi 40000 40000>;

Here you moved code describing partition from "partitions" into "chosen"
which seems incorrect to me. We already have bindings for partitions and
they should be children of flash node.

For flashes yes. For SD/EMMC/SATA?



         #or
         u-boot,env = <&mmc 0 0 10000>; #device/start/size - raw mode
         u-boot,env = <&mmc 0 1>; # device/partition - as file to FS
         #etc.
     };
};


&qspi {
     flash {
         partitions {
             compatible = "fixed-partitions";
             #address-cells = <1>;
             #size-cells = <1>;

             part0: partition@0 {
                 label = "u-boot-env";
                 reg = <0x0 0x40000>;
             };

             part1: partition@40000 {
                 label = "u-boot-env-redundant";
                 reg = <0x40000 0x10000>;
             };
     };
};

So my summary for this would be:
1. Let's use partitions for placing env data partition binding
2. Let's add minimal U-Boot setup into "chosen" if needed

Please consider this:

chosen {
    u-boot {
        u-boot,env = <&env0>, <&env1>;

I am fine with this description. But you need to come up with description for SDs where it can be filesystem based or RAW. And you don't have partition description as you have for spis, nand, nors, eeproms.

    };
};

&qspi {
    flash {
        partitions {
        compatible = "fixed-partitions";
        #address-cells = <1>;
        #size-cells = <1>;

        env0: partition@0 {
            label = "u-boot-env";
            reg = <0x0 0x40000>;
        };

        env1: partition@40000 {
            label = "u-boot-env-redundant";
            reg = <0x40000 0x10000>;
        };
    };
};

If you still need to access flash content directly, you can pretty
easily calculate offset from &env0 and &env1 nodes.

No doubt about it. But then you are forcing everybody to define partition definition. I am fine with it but hopefully it won't be problem for others.

I am just trying to get to the point that dt binding which we are going to use will be universal across SOCs and we can use the same description for other purpose. Environment is one of them, another can be boot script addresses/location, A/B update location, etc.

Thanks,
Michal