Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7f389c1e-cc36-1cce-6be2-27f18b2be7d3@gmail.com>
Date:   Wed, 9 Mar 2022 15:06:56 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:96.0) Gecko/20100101
 Thunderbird/96.0
Subject: Re: [PATCH V3] dt-bindings: nvmem: add U-Boot environment variables
 binding
To:     Michal Simek <michal.simek@xilinx.com>,
        Rob Herring <robh+dt@kernel.org>,
        Tom Rini <trini@konsulko.com>, Simon Glass <sjg@chromium.org>
Cc:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
        Ricardo Salveti <ricardo@foundries.io>,
        Jorge Ramirez-Ortiz <jorge@foundries.io>,
        Sean Anderson <seanga2@gmail.com>, devicetree@vger.kernel.org,
        u-boot@lists.denx.de, linux-kernel@vger.kernel.org,
        =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>
References: <20220228131250.16943-1-zajec5@gmail.com>
 <578e243d-2273-add3-898c-959888c2a155@xilinx.com>
From:   =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>
In-Reply-To: <578e243d-2273-add3-898c-959888c2a155@xilinx.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 9.03.2022 14:42, Michal Simek wrote:
> On 2/28/22 14:12, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> U-Boot uses environment variables for storing device setup data. It
>> usually needs to be accessed by a bootloader, kernel and often
>> user-space.
>>
>> This binding allows describing environment data located in a raw flash
>> partition. It's treated as NVMEM device and can be reused later for
>> other storage devices.
>>
>> Using DT should be cleaner than hardcoding & duplicating such 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.
>>
>> A custom "compatible" string allows system to automatically load
>> relevant NVMEM driver but phandle can be also used for reading raw
>> location.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>> V2: Update descriptions to don't make this binding MTD (flash partition)
>>      specific. Mention multiple possible storage ways.
>> V3: Drop
>>      allOf:
>>        - $ref: nvmem.yaml#
>>      as we don't use anything rom the nvmem.yaml. Thanks Rob.
>> ---
>>   .../devicetree/bindings/nvmem/u-boot,env.yaml | 62 +++++++++++++++++++
>>   MAINTAINERS                                   |  5 ++
>>   2 files changed, 67 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..e70b2a60cb9a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
>> @@ -0,0 +1,62 @@
>> +# 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 using U-Boot specific formats (variant specific header and NUL
>> +  separated key-value pairs).
>> +
>> +  Environment data can be stored on various storage entities, e.g.:
>> +  1. Raw flash partition
>> +  2. UBI volume
>> +
>> +  This binding allows marking storage device (as containing env data) and
>> +  specifying used format.
>> +
>> +  Right now only flash partition case is covered but it may be extended to e.g.
>> +  UBI volumes in the future.
>> +
>> +maintainers:
>> +  - Rafał Miłecki <rafal@milecki.pl>
>> +
>> +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
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    partitions {
>> +        compatible = "fixed-partitions";
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +
>> +        partition@0 {
>> +            reg = <0x0 0x40000>;
>> +            label = "u-boot";
>> +            read-only;
>> +        };
>> +
>> +        env: partition@40000 {
>> +            compatible = "u-boot,env";
>> +            reg = <0x40000 0x10000>;
>> +        };
>> +    };
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index db8052bc1d26..24fc181a7e6c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19958,6 +19958,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>
> 
> I think that parsing these partitions is quite sw intensive process and I can't still see the value to have compatible string here.
> I would prefer to have just any link from u-boot node to partition instead.
> 
> But up to Simon or Tom to decide.

In the first place DT should describe hardware / platform / device as it
is. Without taking shortcuts. If environment data can be stored in flash
device partition it should be described exactly as that.

Systems like Linux can benefit from that.

If some systems (e.g. a bootloader - U-Boot or any other one) can't
afford parsing / covering whole DT structure due to some limitations -
then we can come with helpers. I've no objections here.

In dt-schema [1] we have place for U-Boot specific options. Please see
0986f729eff0 ("dt-bindings: u-boot: Add an initial binding for config").

We can add support e.g. for
u-boot {
         (...)
         u-boot,env = <&env>;
};

[1] https://github.com/devicetree-org/dt-schema
[2] https://github.com/devicetree-org/dt-schema/commit/0986f729eff0f40a66e85ab9dfb37681bf025ac4