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

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

[Rafał Miłecki]

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
+
+  reg:
+    maxItems: 1
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    partitions {
+        compatible = "fixed-partitions";
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        partition@0 {
+            label = "u-boot";
+            reg = <0x0 0x40000>;
+            read-only;
+        };
+
+        partition@40000 {
+            compatible = "u-boot,env";
+            label = "u-boot-env";
+            reg = <0x40000 0x10000>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index d20344ab7900..2f07f1d2290a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19905,6 +19905,11 @@ W:	http://linuxtv.org
 T:	git git://linuxtv.org/media_tree.git
 F:	drivers/media/pci/tw686x/
 
+U-BOOT ENVIRONMENT VARIABLES
+M:	Rafał Miłecki <rafal@milecki.pl>
+S:	Maintained
+F:	Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
+
 UACCE ACCELERATOR FRAMEWORK
 M:	Zhangfei Gao <zhangfei.gao@linaro.org>
 M:	Zhou Wang <wangzhou1@hisilicon.com>
-- 
2.34.1

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

[Michal Simek]

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>;
		#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>;
         	};
	};
};

Thanks,
Michal

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

[Sean Anderson]

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?

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

--Sean

>          #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>;
>              };
>      };
> };
> 
> Thanks,
> Michal
> 

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

[Michal Simek]

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

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

[Rafał Miłecki]

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


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


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


>          #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>;
	};
};

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

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

[Rafał Miłecki]

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>;
		};
	};
};

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

[Michal Simek]

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

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

[Michal Simek]

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