Message-ID: <e312dca4-0187-d3fb-505e-2b370ea7cb93@xilinx.com>
Date:   Wed, 16 Feb 2022 15:24:27 +0100
From:   Michal Simek <michal.simek@...inx.com>
To:     Rafał Miłecki <zajec5@...il.com>,
        Michal Simek <michal.simek@...inx.com>,
        Rob Herring <robh+dt@...nel.org>,
        Tom Rini <trini@...sulko.com>, Simon Glass <sjg@...omium.org>
CC:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Ricardo Salveti <ricardo@...ndries.io>,
        Jorge Ramirez-Ortiz <jorge@...ndries.io>,
        <devicetree@...r.kernel.org>, <u-boot@...ts.denx.de>,
        <linux-kernel@...r.kernel.org>,
        Rafał Miłecki <rafal@...ecki.pl>
Subject: Re: [PATCH] dt-bindings: nvmem: add U-Boot environment variables
 binding



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@...ecki.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@...ecki.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@...ecki.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@...00 {
>>                  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@...00 {
>              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

Powered by blists - more mailing lists