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;
Subject: Re: [PATCH] dt-bindings: nvmem: add U-Boot environment variables
 binding
To:     Michal Simek <michal.simek@xilinx.com>,
        =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.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>,
        devicetree@vger.kernel.org, u-boot@lists.denx.de,
        linux-kernel@vger.kernel.org,
        =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>
References: <20220215134925.32691-1-zajec5@gmail.com>
 <193f9354-5e1d-def8-c8ab-fbe88d8d9b8f@xilinx.com>
From:   Sean Anderson <seanga2@gmail.com>
Message-ID: <6679b98e-f5f0-8608-a36c-c0c41fb5ea6f@gmail.com>
Date:   Tue, 15 Feb 2022 09:57:21 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <193f9354-5e1d-def8-c8ab-fbe88d8d9b8f@xilinx.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 2/15/22 9:02 AM, Michal Simek wrote:
>=20
>=20
> On 2/15/22 14:49, Rafa=C5=82 Mi=C5=82ecki wrote:
>> From: Rafa=C5=82 Mi=C5=82ecki <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 forma=
t
>> clearly. In some/many cases it should be cleaner than hardcoding &
>> duplicating that info in multiple places. Bootloader & kernel can shar=
e
>> DTS and user-space can try reading it too or just have correct data
>> exposed by a kernel.
>>
>> Signed-off-by: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>> ---
>> =C2=A0 .../devicetree/bindings/nvmem/u-boot,env.yaml | 58 ++++++++++++=
+++++++
>> =C2=A0 MAINTAINERS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
 |=C2=A0 5 ++
>> =C2=A0 2 files changed, 63 insertions(+)
>> =C2=A0 create mode 100644 Documentation/devicetree/bindings/nvmem/u-bo=
ot,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: |
>> +=C2=A0 U-Boot uses environment variables to store device parameters a=
nd
>> +=C2=A0 configuration. They may be used for booting process, setup or =
keeping end user
>> +=C2=A0 info.
>> +
>> +=C2=A0 Data is stored on flash in a U-Boot specific format (header an=
d NUL separated
>> +=C2=A0 key-value pairs).
>> +
>> +=C2=A0 This binding allows specifying data location and used format.
>> +
>> +maintainers:
>> +=C2=A0 - Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
>> +
>> +allOf:
>> +=C2=A0 - $ref: nvmem.yaml#
>> +
>> +properties:
>> +=C2=A0 compatible:
>> +=C2=A0=C2=A0=C2=A0 oneOf:
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - description: A standalone env data b=
lock
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const: u-boot,env
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - description: Two redundant blocks wi=
th active one flagged
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const: u-boot,env-redundan=
t-bool
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - description: Two redundant blocks wi=
th active having higher counter
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const: u-boot,env-redundan=
t-count
>=20
> 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=
=2E
> And the whole parsing will take a lot of time.
>=20
> I think the way how I think this can be handled is.
>=20
> # I don't think that discussion with Simon was finished.
> But for example (chosen or firmware node)
> chosen {
>  =C2=A0=C2=A0=C2=A0=C2=A0u-boot {
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u-boot,env =3D <&qspi &part=
0>;
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u-boot,env-redundant =3D <&=
qspi &part1>;
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 #or
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u-boot,env =3D <&qspi 0 400=
00>;
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u-boot,env-redundant =3D <&=
qspi 40000 40000>;

What about when the environment is on top of UBI?

>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 #or
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u-boot,env =3D <&mmc 0 0 10=
000>; #device/start/size - raw mode
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u-boot,env =3D <&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

>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 #etc.
>  =C2=A0=C2=A0=C2=A0=C2=A0};
> };
>=20
>=20
> &qspi {
>  =C2=A0=C2=A0=C2=A0=C2=A0flash {
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 partitions {
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 com=
patible =3D "fixed-partitions";
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 #ad=
dress-cells =3D <1>;
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 #si=
ze-cells =3D <1>;
>=20
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 par=
t0: partition@0 {
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 label =3D "u-boot-env";
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 reg =3D <0x0 0x40000>;
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 };
>=20
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 par=
t1: partition@40000 {
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 label =3D "u-boot-env-redundant";
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 reg =3D <0x40000 0x10000>;
>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 };
>  =C2=A0=C2=A0=C2=A0=C2=A0};
> };
>=20
> Thanks,
> Michal
>=20