Re: [PATCH v4 01/11] FWU: Add FWU metadata structure and driver for accessing metadata

[Michal Simek <michal.simek@xilinx.com>]

On 2/8/22 15:14, Masami Hiramatsu wrote:
> 2022年2月8日(火) 22:45 Michal Simek <michal.simek@xilinx.com>:
>>
>>
>>
>> On 2/8/22 14:36, Masami Hiramatsu wrote:
>>> 2022年2月8日(火) 20:35 Sughosh Ganu <sughosh.ganu@linaro.org>:
>>>>
>>>> On Tue, 8 Feb 2022 at 16:26, Michal Simek <monstr@monstr.eu> wrote:
>>>>>
>>>>> po 7. 2. 2022 v 19:21 odesílatel Sughosh Ganu <sughosh.ganu@linaro.org> napsal:
>>>>>>
>>>>>> In the FWU Multi Bank Update feature, the information about the
>>>>>> updatable images is stored as part of the metadata, which is stored on
>>>>>> a dedicated partition. Add the metadata structure, and a driver model
>>>>>> uclass which provides functions to access the metadata. These are
>>>>>> generic API's, and implementations can be added based on parameters
>>>>>> like how the metadata partition is accessed and what type of storage
>>>>>> device houses the metadata.
>>>>>>
>>>>>> A device tree node fwu-mdata has been added, which is used for
>>>>>> pointing to the storage device which contains the FWU metadata. The
>>>>>> fwu-mdata node is u-boot specific, and can be added the platform's
>>>>>> u-boot dtsi file.
>>>>>>
>>>>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>>>>>> ---
>>>>>>
>>>>>> Changes since V3:
>>>>>> * Move the FWU metadata access to driver model
>>>>>> * Get the storage device containing the metadata from a device tree
>>>>>>     property instead of a platform helper function
>>>>>>
>>>>>>    arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi      |   7 +
>>>>>>    .../firmware/fwu-mdata.txt                    |  18 +
>>>>>>    drivers/Kconfig                               |   2 +
>>>>>>    drivers/Makefile                              |   1 +
>>>>>>    drivers/fwu-mdata/Kconfig                     |   7 +
>>>>>>    drivers/fwu-mdata/Makefile                    |   6 +
>>>>>>    drivers/fwu-mdata/fwu-mdata-uclass.c          | 434 ++++++++++++++++++
>>>>>>    include/dm/uclass-id.h                        |   1 +
>>>>>>    include/fwu.h                                 |  51 ++
>>>>>>    include/fwu_mdata.h                           |  67 +++
>>>>>>    10 files changed, 594 insertions(+)
>>>>>>    create mode 100644 doc/device-tree-bindings/firmware/fwu-mdata.txt
>>>>>>    create mode 100644 drivers/fwu-mdata/Kconfig
>>>>>>    create mode 100644 drivers/fwu-mdata/Makefile
>>>>>>    create mode 100644 drivers/fwu-mdata/fwu-mdata-uclass.c
>>>>>>    create mode 100644 include/fwu.h
>>>>>>    create mode 100644 include/fwu_mdata.h
>>>>>>
>>>>>> diff --git a/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi b/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi
>>>>>> index 06ef3a4095..3bec6107f7 100644
>>>>>> --- a/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi
>>>>>> +++ b/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi
>>>>>> @@ -4,3 +4,10 @@
>>>>>>     */
>>>>>>
>>>>>>    #include "stm32mp157a-dk1-u-boot.dtsi"
>>>>>> +
>>>>>> +/ {
>>>>>> +       fwu-mdata {
>>>>>> +               compatible = "u-boot,fwu-mdata";
>>>>>> +               fwu-mdata-store = <&sdmmc1>;
>>>>>> +       };
>>>>>> +};
>>>>>> diff --git a/doc/device-tree-bindings/firmware/fwu-mdata.txt b/doc/device-tree-bindings/firmware/fwu-mdata.txt
>>>>>> new file mode 100644
>>>>>> index 0000000000..c766b595ef
>>>>>> --- /dev/null
>>>>>> +++ b/doc/device-tree-bindings/firmware/fwu-mdata.txt
>>>>>> @@ -0,0 +1,18 @@
>>>>>> +FWU Metadata Access Devicetree Binding
>>>>>> +
>>>>>> +The FWU Multi Bank Update feature uses a metadata structure, stored on
>>>>>> +a separate partition for keeping information on the set of updatable
>>>>>> +images. The device tree node provides information on the storage
>>>>>> +device that contains the FWU metadata.
>>>>>> +
>>>>>> +Required properties :
>>>>>> +
>>>>>> +- compatible : "u-boot,fwu-mdata";
>>>>>> +- fwu-mdata-store : should point to the storage device which contains
>>>>>> +                   the FWU metadata partition.
>>>>>
>>>>> In 0/11 you are describing GPT partitions but I don't think this
>>>>> binding is generic enough to describe
>>>>> other cases. It is saying device but not where exactly it is.
>>>>> I didn't read the whole series yet but arm spec recommends GPT but dt
>>>>> binding should be generic enough to describe
>>>>> also other cases.
>>>>> We are saving this structure to qspi for example but current binding
>>>>> can't be used for it.
>>>>
>>>> I would wait to see Masami's implementation of this feature. If I am
>>>> not wrong, his platform too is enabling this feature with a spi as the
>>>> storage device. Maybe we can instead put a list of <device partition>
>>>> tuples which can then list the device and partition number of the
>>>> metadata partitions.
>>>
>>> Yes, I would like to define new compatible for sf backend, e.g.
>>> "u-boot,fwu-mdata-sf".
>>
>> backend is fine. What does this compatible string have to do with it?
>> I didn't see any fwu-mdata-gpt in this series.
> 
> Sughosh made it "fwu-mdata" but I think it should be "fwu-mdata-gpt"
> if the required parameters are different.
> 
>>
>>> It will have the fwu-mdata-store to point the SPI flash device, and
>>> will have a list of offset information for the metadata.
>>
>> Can you describe it?
>>
>> Something like for raw accesses?
>> fwu-mdta-store = <&qspi 0 XXXX>;
> 
> I'm still not sure what is the best way. What I thought was,
> 
> fwu-mdata-store = <&spi-flash>;
> fwu-mdata-offsets = <500000, 520000>;

as I said. Before this is applied I think we should work on generic proposal how 
to describe it. Because the same way can be used for u-boot variables and maybe 
others.
Definitely I would prefer to ask Rob for helping with this and get this to yaml 
to be able to use the whole infrastructure to check it.

Thanks,
Michal