On 2/15/22 15:57, Sean Anderson wrote:

On 2/15/22 9:02 AM, Michal Simek wrote:

...

On 2/15/22 14:49, Rafał Miłecki wrote:

...

From: Rafał Miłecki rafal@milecki.pl

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@milecki.pl

.../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@milecki.pl

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

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

# 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>;          #or          u-boot,env = <&qspi 0 40000>;          u-boot,env-redundant = <&qspi 40000 40000>;

What about when the environment is on top of UBI?

I expect we should list all possible combinations and cover them here.

...

#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

For emmc at least you will need another cell for the hardware partition. And of course, you can name the environment file whatever you want, so that needs to be recorded as well.

IMO to do this properly you'd need to have a property corresponding to each of the major configs in the env menu.

Agree 100%. I just wanted to share how I think this should be done. At the end we should be able to describe any combination in generic way.

Thanks, Michal

On 2/16/22 13:59, Rafał Miłecki wrote:

On 15.02.2022 15:57, Sean Anderson wrote:

...

On 2/15/22 9:02 AM, Michal Simek wrote:

...

On 2/15/22 14:49, Rafał Miłecki wrote:

...

From: Rafał Miłecki rafal@milecki.pl

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@milecki.pl

.../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@milecki.pl

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

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

# 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>;          #or          u-boot,env = <&qspi 0 40000>;          u-boot,env-redundant = <&qspi 40000 40000>;

What about when the environment is on top of UBI?

We can always add support for binding UBI volumes in DT. Somethig more-or-less like:

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

partition@0 {         compatible = "ubi";         label = "ubi";         reg = <0x0 0x1000000>;

env0: partition-0 {             volume-name = "env";         };

env1: partition-1 {             volume-id = <10>;         };     }; };

If this is something ack by Rob I have no problem with it. But it has to go to schema.

M

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@milecki.pl

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@milecki.pl

.../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@milecki.pl

+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