diff mbox series

dt/bindings: fwu-mdata-mtd: drop changes outside FWU

Message ID 20230410232112.72778-1-jaswinder.singh@linaro.org
State Accepted
Commit 56b481c42879ae171c6272f92ff5db7c767929af
Headers show
Series dt/bindings: fwu-mdata-mtd: drop changes outside FWU | expand

Commit Message

Jassi Brar April 10, 2023, 11:21 p.m. UTC
From: Jassi Brar <jaswinder.singh@linaro.org>

Any requirement of FWU should not require changes to bindings
of other subsystems. For example, for mtd-backed storage we
can do without requiring 'fixed-partitions' children to also
carry 'uuid', a property which is non-standard and not in the
bindings.

 There exists no code yet, so we can change the fwu-mtd bindings
to contain all properties within the fwu-mdata node.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---

Hi Rob, Hi Krzysztof,

  I was suggested, and I agree, it would be a good idea to get your blessings
for the location and meta-data (fwu-mdata) bindings for the FWU.

  The FWU images can be located in GPT partitions or MTD backed storage.
The basic bindings for fwu-mdata has already been merged in uboot (ideally they
too should have had your review). Now I am trying to fully support MTD backed
storage and hence looking for your review. The proposed bindings are totally
self-contained and don't require changes to any other subsystem.

Thanks.


 .../firmware/fwu-mdata-mtd.yaml               | 105 +++++++++++++++---
 1 file changed, 91 insertions(+), 14 deletions(-)

Comments

Krzysztof Kozlowski April 11, 2023, 5:38 a.m. UTC | #1
On 11/04/2023 01:21, jaswinder.singh@linaro.org wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> Any requirement of FWU should not require changes to bindings
> of other subsystems. For example, for mtd-backed storage we
> can do without requiring 'fixed-partitions' children to also
> carry 'uuid', a property which is non-standard and not in the
> bindings.
> 
>  There exists no code yet, so we can change the fwu-mtd bindings
> to contain all properties within the fwu-mdata node.
> 
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
> 
> Hi Rob, Hi Krzysztof,
> 
>   I was suggested, and I agree, it would be a good idea to get your blessings
> for the location and meta-data (fwu-mdata) bindings for the FWU.
> 
>   The FWU images can be located in GPT partitions or MTD backed storage.
> The basic bindings for fwu-mdata has already been merged in uboot (ideally they
> too should have had your review). Now I am trying to fully support MTD backed
> storage and hence looking for your review. The proposed bindings are totally
> self-contained and don't require changes to any other subsystem.
> 
> Thanks.

I think we do not review U-Boot bindings usually, except these put in
the Linux kernel. There were few targeting U-Boot specifically (e.g.
Documentation/devicetree/bindings/mtd/partitions/u-boot.yaml and
Documentation/devicetree/bindings/nvmem/u-boot,env.yaml) so if you want
our blessing, the bindings should be done in Linux kernel repo.

I am pretty sure that reviewing other project bindings would be too much
of work for me.

Best regards,
Krzysztof
Michal Simek April 14, 2023, 2:01 p.m. UTC | #2
On 4/11/23 07:38, Krzysztof Kozlowski wrote:
> On 11/04/2023 01:21, jaswinder.singh@linaro.org wrote:
>> From: Jassi Brar <jaswinder.singh@linaro.org>
>>
>> Any requirement of FWU should not require changes to bindings
>> of other subsystems. For example, for mtd-backed storage we
>> can do without requiring 'fixed-partitions' children to also
>> carry 'uuid', a property which is non-standard and not in the
>> bindings.
>>
>>   There exists no code yet, so we can change the fwu-mtd bindings
>> to contain all properties within the fwu-mdata node.
>>
>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>> ---
>>
>> Hi Rob, Hi Krzysztof,
>>
>>    I was suggested, and I agree, it would be a good idea to get your blessings
>> for the location and meta-data (fwu-mdata) bindings for the FWU.
>>
>>    The FWU images can be located in GPT partitions or MTD backed storage.
>> The basic bindings for fwu-mdata has already been merged in uboot (ideally they
>> too should have had your review). Now I am trying to fully support MTD backed
>> storage and hence looking for your review. The proposed bindings are totally
>> self-contained and don't require changes to any other subsystem.
>>
>> Thanks.
> 
> I think we do not review U-Boot bindings usually, except these put in
> the Linux kernel. There were few targeting U-Boot specifically (e.g.
> Documentation/devicetree/bindings/mtd/partitions/u-boot.yaml and
> Documentation/devicetree/bindings/nvmem/u-boot,env.yaml) so if you want
> our blessing, the bindings should be done in Linux kernel repo.

At least for us our goal is to have be able to share the same dt binding 
document in U-Boot and Linux.

> 
> I am pretty sure that reviewing other project bindings would be too much
> of work for me.

That's understandable. Pretty much it is more about to find a right location.

Simon added options node for u-boot directly to dt-schema
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/options/u-boot.yaml#L96

And if this is the same category it should maybe go directly to dt-schema instead.
What about?

options {
	fwu-mdata {
		...
	};
};

This thread is pretty much asking for suggesting where this can go to be able to 
pass yaml checking which is required by SR.

Thanks,
Michal
Ilias Apalodimas May 3, 2023, 2:37 p.m. UTC | #3
Hi Krzysztof,

On Tue, Apr 11, 2023 at 07:38:11AM +0200, Krzysztof Kozlowski wrote:
> On 11/04/2023 01:21, jaswinder.singh@linaro.org wrote:
> > From: Jassi Brar <jaswinder.singh@linaro.org>
> >
> > Any requirement of FWU should not require changes to bindings
> > of other subsystems. For example, for mtd-backed storage we
> > can do without requiring 'fixed-partitions' children to also
> > carry 'uuid', a property which is non-standard and not in the
> > bindings.
> >
> >  There exists no code yet, so we can change the fwu-mtd bindings
> > to contain all properties within the fwu-mdata node.
> >
> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > ---
> >
> > Hi Rob, Hi Krzysztof,
> >
> >   I was suggested, and I agree, it would be a good idea to get your blessings
> > for the location and meta-data (fwu-mdata) bindings for the FWU.
> >
> >   The FWU images can be located in GPT partitions or MTD backed storage.
> > The basic bindings for fwu-mdata has already been merged in uboot (ideally they
> > too should have had your review). Now I am trying to fully support MTD backed
> > storage and hence looking for your review. The proposed bindings are totally
> > self-contained and don't require changes to any other subsystem.
> >
> > Thanks.
>
> I think we do not review U-Boot bindings usually, except these put in
> the Linux kernel. There were few targeting U-Boot specifically (e.g.
> Documentation/devicetree/bindings/mtd/partitions/u-boot.yaml and
> Documentation/devicetree/bindings/nvmem/u-boot,env.yaml) so if you want
> our blessing, the bindings should be done in Linux kernel repo.
>
> I am pretty sure that reviewing other project bindings would be too much
> of work for me.

Sure that makes sense.  But an answer here of whether the bindings make
sense to the DT maintainers or not would help to move forward.

These bindings are trying to define a standardized interface for A/B *firmware*
updates [0] which is not what traditionally goes into a device tree.  OTOH we
already have some U-Boot specific bindings as you already mentioned.  As we
move forward we need to be very precise on what is allowed or not on the DT
since it's now tested and verified on SystemReady certifications.  IOW if
we add those binding in U-Boot only, we would need to strip them before
handing the DT to linux, otherwise certification would fail.  If you do
think that having them in the kernel repo makes sense,  it would help
standardizing other boot loaders (at least it would standardize where that
metadata lives) if they want to implement something similar.

Just keep in mind we would need a schema per storage medium.  IOW this tries to
standardize devices which keep the firmware binary in an mtd.  There's also
another biding which describes firmware files on a GPT [1].

[0] https://developer.arm.com/documentation/den0118/a
[1] https://source.denx.de/u-boot/u-boot/-/blob/master/doc/device-tree-bindings/firmware/fwu-mdata-gpt.yaml

Thanks
/Ilias

>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski May 3, 2023, 2:54 p.m. UTC | #4
On 03/05/2023 16:37, Ilias Apalodimas wrote:
> Hi Krzysztof,
> 
> On Tue, Apr 11, 2023 at 07:38:11AM +0200, Krzysztof Kozlowski wrote:
>> On 11/04/2023 01:21, jaswinder.singh@linaro.org wrote:
>>> From: Jassi Brar <jaswinder.singh@linaro.org>
>>>
>>> Any requirement of FWU should not require changes to bindings
>>> of other subsystems. For example, for mtd-backed storage we
>>> can do without requiring 'fixed-partitions' children to also
>>> carry 'uuid', a property which is non-standard and not in the
>>> bindings.
>>>
>>>  There exists no code yet, so we can change the fwu-mtd bindings
>>> to contain all properties within the fwu-mdata node.
>>>
>>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>>> ---
>>>
>>> Hi Rob, Hi Krzysztof,
>>>
>>>   I was suggested, and I agree, it would be a good idea to get your blessings
>>> for the location and meta-data (fwu-mdata) bindings for the FWU.
>>>
>>>   The FWU images can be located in GPT partitions or MTD backed storage.
>>> The basic bindings for fwu-mdata has already been merged in uboot (ideally they
>>> too should have had your review). Now I am trying to fully support MTD backed
>>> storage and hence looking for your review. The proposed bindings are totally
>>> self-contained and don't require changes to any other subsystem.
>>>
>>> Thanks.
>>
>> I think we do not review U-Boot bindings usually, except these put in
>> the Linux kernel. There were few targeting U-Boot specifically (e.g.
>> Documentation/devicetree/bindings/mtd/partitions/u-boot.yaml and
>> Documentation/devicetree/bindings/nvmem/u-boot,env.yaml) so if you want
>> our blessing, the bindings should be done in Linux kernel repo.
>>
>> I am pretty sure that reviewing other project bindings would be too much
>> of work for me.
> 
> Sure that makes sense.  But an answer here of whether the bindings make
> sense to the DT maintainers or not would help to move forward.

I am not a DT maintainer of other systems, components etc. Answering
anything for these other systems and components means nothing. I will
take no responsibility of whatever I say because I will bear no costs of
it. :) IOW, to me you can make any invalid binding inside U-Boot and it
will not matter for the Linux kernel. It will of course matter to U-Boot
in many aspects.

> 
> These bindings are trying to define a standardized interface for A/B *firmware*
> updates [0] which is not what traditionally goes into a device tree.  OTOH we
> already have some U-Boot specific bindings as you already mentioned.  As we
> move forward we need to be very precise on what is allowed or not on the DT
> since it's now tested and verified on SystemReady certifications.
>  IOW if
> we add those binding in U-Boot only, we would need to strip them before
> handing the DT to linux, otherwise certification would fail.

Which you can.

Or propose to add the bindings to the Linux kernel and to the Linux
kernel DTS, which then will get our review.

>  If you do
> think that having them in the kernel repo makes sense,  it would help
> standardizing other boot loaders (at least it would standardize where that
> metadata lives) if they want to implement something similar.

I cannot speak for Rob, but that's the only way I can make a review. I
cannot review or try to maintain all possible projects in the world and
their bindings. How would this even work in practice?

> 
> Just keep in mind we would need a schema per storage medium.  IOW this tries to
> standardize devices which keep the firmware binary in an mtd.  There's also
> another biding which describes firmware files on a GPT [1].
> 
> [0] https://developer.arm.com/documentation/den0118/a
> [1] https://source.denx.de/u-boot/u-boot/-/blob/master/doc/device-tree-bindings/firmware/fwu-mdata-gpt.yaml

Best regards,
Krzysztof
Tom Rini May 3, 2023, 4:26 p.m. UTC | #5
On Wed, May 03, 2023 at 04:54:45PM +0200, Krzysztof Kozlowski wrote:
> On 03/05/2023 16:37, Ilias Apalodimas wrote:
> > Hi Krzysztof,
> > 
> > On Tue, Apr 11, 2023 at 07:38:11AM +0200, Krzysztof Kozlowski wrote:
> >> On 11/04/2023 01:21, jaswinder.singh@linaro.org wrote:
> >>> From: Jassi Brar <jaswinder.singh@linaro.org>
> >>>
> >>> Any requirement of FWU should not require changes to bindings
> >>> of other subsystems. For example, for mtd-backed storage we
> >>> can do without requiring 'fixed-partitions' children to also
> >>> carry 'uuid', a property which is non-standard and not in the
> >>> bindings.
> >>>
> >>>  There exists no code yet, so we can change the fwu-mtd bindings
> >>> to contain all properties within the fwu-mdata node.
> >>>
> >>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> >>> ---
> >>>
> >>> Hi Rob, Hi Krzysztof,
> >>>
> >>>   I was suggested, and I agree, it would be a good idea to get your blessings
> >>> for the location and meta-data (fwu-mdata) bindings for the FWU.
> >>>
> >>>   The FWU images can be located in GPT partitions or MTD backed storage.
> >>> The basic bindings for fwu-mdata has already been merged in uboot (ideally they
> >>> too should have had your review). Now I am trying to fully support MTD backed
> >>> storage and hence looking for your review. The proposed bindings are totally
> >>> self-contained and don't require changes to any other subsystem.
> >>>
> >>> Thanks.
> >>
> >> I think we do not review U-Boot bindings usually, except these put in
> >> the Linux kernel. There were few targeting U-Boot specifically (e.g.
> >> Documentation/devicetree/bindings/mtd/partitions/u-boot.yaml and
> >> Documentation/devicetree/bindings/nvmem/u-boot,env.yaml) so if you want
> >> our blessing, the bindings should be done in Linux kernel repo.
> >>
> >> I am pretty sure that reviewing other project bindings would be too much
> >> of work for me.
> > 
> > Sure that makes sense.  But an answer here of whether the bindings make
> > sense to the DT maintainers or not would help to move forward.
> 
> I am not a DT maintainer of other systems, components etc. Answering
> anything for these other systems and components means nothing. I will
> take no responsibility of whatever I say because I will bear no costs of
> it. :) IOW, to me you can make any invalid binding inside U-Boot and it
> will not matter for the Linux kernel. It will of course matter to U-Boot
> in many aspects.
> 
> > 
> > These bindings are trying to define a standardized interface for A/B *firmware*
> > updates [0] which is not what traditionally goes into a device tree.  OTOH we
> > already have some U-Boot specific bindings as you already mentioned.  As we
> > move forward we need to be very precise on what is allowed or not on the DT
> > since it's now tested and verified on SystemReady certifications.
> >  IOW if
> > we add those binding in U-Boot only, we would need to strip them before
> > handing the DT to linux, otherwise certification would fail.
> 
> Which you can.
> 
> Or propose to add the bindings to the Linux kernel and to the Linux
> kernel DTS, which then will get our review.
> 
> >  If you do
> > think that having them in the kernel repo makes sense,  it would help
> > standardizing other boot loaders (at least it would standardize where that
> > metadata lives) if they want to implement something similar.
> 
> I cannot speak for Rob, but that's the only way I can make a review. I
> cannot review or try to maintain all possible projects in the world and
> their bindings. How would this even work in practice?
> 
> > 
> > Just keep in mind we would need a schema per storage medium.  IOW this tries to
> > standardize devices which keep the firmware binary in an mtd.  There's also
> > another biding which describes firmware files on a GPT [1].
> > 
> > [0] https://developer.arm.com/documentation/den0118/a
> > [1] https://source.denx.de/u-boot/u-boot/-/blob/master/doc/device-tree-bindings/firmware/fwu-mdata-gpt.yaml

This is one of the bindings that we need to upstream to
https://github.com/devicetree-org/dt-schema/ and so in this case there's
less likely to be interest from purely Linux kernel centric reviewers.
Once it's there, the dts nodes and so forth can be in the trees that end
up in the kernel, since they will pass validation.  In so far as I
understand what the plan for everything is, at least.
Krzysztof Kozlowski May 3, 2023, 4:32 p.m. UTC | #6
On 03/05/2023 18:26, Tom Rini wrote:
>>>>
>>>> I think we do not review U-Boot bindings usually, except these put in
>>>> the Linux kernel. There were few targeting U-Boot specifically (e.g.
>>>> Documentation/devicetree/bindings/mtd/partitions/u-boot.yaml and
>>>> Documentation/devicetree/bindings/nvmem/u-boot,env.yaml) so if you want
>>>> our blessing, the bindings should be done in Linux kernel repo.
>>>>
>>>> I am pretty sure that reviewing other project bindings would be too much
>>>> of work for me.
>>>
>>> Sure that makes sense.  But an answer here of whether the bindings make
>>> sense to the DT maintainers or not would help to move forward.
>>
>> I am not a DT maintainer of other systems, components etc. Answering
>> anything for these other systems and components means nothing. I will
>> take no responsibility of whatever I say because I will bear no costs of
>> it. :) IOW, to me you can make any invalid binding inside U-Boot and it
>> will not matter for the Linux kernel. It will of course matter to U-Boot
>> in many aspects.
>>
>>>
>>> These bindings are trying to define a standardized interface for A/B *firmware*
>>> updates [0] which is not what traditionally goes into a device tree.  OTOH we
>>> already have some U-Boot specific bindings as you already mentioned.  As we
>>> move forward we need to be very precise on what is allowed or not on the DT
>>> since it's now tested and verified on SystemReady certifications.
>>>  IOW if
>>> we add those binding in U-Boot only, we would need to strip them before
>>> handing the DT to linux, otherwise certification would fail.
>>
>> Which you can.
>>
>> Or propose to add the bindings to the Linux kernel and to the Linux
>> kernel DTS, which then will get our review.
>>
>>>  If you do
>>> think that having them in the kernel repo makes sense,  it would help
>>> standardizing other boot loaders (at least it would standardize where that
>>> metadata lives) if they want to implement something similar.
>>
>> I cannot speak for Rob, but that's the only way I can make a review. I
>> cannot review or try to maintain all possible projects in the world and
>> their bindings. How would this even work in practice?
>>
>>>
>>> Just keep in mind we would need a schema per storage medium.  IOW this tries to
>>> standardize devices which keep the firmware binary in an mtd.  There's also
>>> another biding which describes firmware files on a GPT [1].
>>>
>>> [0] https://developer.arm.com/documentation/den0118/a
>>> [1] https://source.denx.de/u-boot/u-boot/-/blob/master/doc/device-tree-bindings/firmware/fwu-mdata-gpt.yaml
> 
> This is one of the bindings that we need to upstream to
> https://github.com/devicetree-org/dt-schema/ 

Sure, this works as well.

Best regards,
Krzysztof
Rob Herring May 3, 2023, 5:24 p.m. UTC | #7
On Wed, May 3, 2023 at 9:37 AM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Krzysztof,
>
> On Tue, Apr 11, 2023 at 07:38:11AM +0200, Krzysztof Kozlowski wrote:
> > On 11/04/2023 01:21, jaswinder.singh@linaro.org wrote:
> > > From: Jassi Brar <jaswinder.singh@linaro.org>
> > >
> > > Any requirement of FWU should not require changes to bindings
> > > of other subsystems. For example, for mtd-backed storage we
> > > can do without requiring 'fixed-partitions' children to also
> > > carry 'uuid', a property which is non-standard and not in the
> > > bindings.
> > >
> > >  There exists no code yet, so we can change the fwu-mtd bindings
> > > to contain all properties within the fwu-mdata node.
> > >
> > > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > > ---
> > >
> > > Hi Rob, Hi Krzysztof,
> > >
> > >   I was suggested, and I agree, it would be a good idea to get your blessings
> > > for the location and meta-data (fwu-mdata) bindings for the FWU.
> > >
> > >   The FWU images can be located in GPT partitions or MTD backed storage.
> > > The basic bindings for fwu-mdata has already been merged in uboot (ideally they
> > > too should have had your review). Now I am trying to fully support MTD backed
> > > storage and hence looking for your review. The proposed bindings are totally
> > > self-contained and don't require changes to any other subsystem.
> > >
> > > Thanks.
> >
> > I think we do not review U-Boot bindings usually, except these put in
> > the Linux kernel. There were few targeting U-Boot specifically (e.g.
> > Documentation/devicetree/bindings/mtd/partitions/u-boot.yaml and
> > Documentation/devicetree/bindings/nvmem/u-boot,env.yaml) so if you want
> > our blessing, the bindings should be done in Linux kernel repo.
> >
> > I am pretty sure that reviewing other project bindings would be too much
> > of work for me.
>
> Sure that makes sense.  But an answer here of whether the bindings make
> sense to the DT maintainers or not would help to move forward.

I have lots of comments on the bindings, but I'm missing what is the
problem to solve. Yes, it's 'A/B firmware updates', but what
configuration data do you need, why and when do you need it. IOW, give
me enough information to understand why a binding is designed the way
it is and be able to propose alternatives.

That's easy enough to deduce for the GPT case. It's just needing to
know which device has the f/w update GPT? That's easily solved with an
alias, a boolean property in the device, or property with the disk
GUID. All of those options are much more simple than a whole node and
compatible.


> These bindings are trying to define a standardized interface for A/B *firmware*
> updates [0] which is not what traditionally goes into a device tree.  OTOH we
> already have some U-Boot specific bindings as you already mentioned.  As we
> move forward we need to be very precise on what is allowed or not on the DT
> since it's now tested and verified on SystemReady certifications.  IOW if
> we add those binding in U-Boot only, we would need to strip them before
> handing the DT to linux, otherwise certification would fail.  If you do
> think that having them in the kernel repo makes sense,  it would help
> standardizing other boot loaders (at least it would standardize where that
> metadata lives) if they want to implement something similar.

I think it is fine if u-boot has things in DT which aren't an ABI, so
private and bundled, but I agree those should be stripped before
passing. To put it another way, if it's not an ABI nor shared amongst
projects, I don't think we need a schema nor to care outside of
u-boot.

However, it doesn't seem to me that needs for A/B updates would be
unique to u-boot.

> Just keep in mind we would need a schema per storage medium.  IOW this tries to
> standardize devices which keep the firmware binary in an mtd.  There's also
> another biding which describes firmware files on a GPT [1].

I'm assuming it's per partition type rather than storage medium (e.g.
SATA, USB, SD, NAND, SPI-NOR)? GPT, 'fixed-partitions', other DT
partition bindings, etc. If so, then I'm really wondering why it's a
standalone tree rather than integrated into those existing partition
bindings.

Rob
Ilias Apalodimas May 4, 2023, 10:45 a.m. UTC | #8
Hi Rob,

On Wed, May 03, 2023 at 12:24:39PM -0500, Rob Herring wrote:
> On Wed, May 3, 2023 at 9:37 AM Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Krzysztof,
> >
> > On Tue, Apr 11, 2023 at 07:38:11AM +0200, Krzysztof Kozlowski wrote:
> > > On 11/04/2023 01:21, jaswinder.singh@linaro.org wrote:
> > > > From: Jassi Brar <jaswinder.singh@linaro.org>
> > > >
> > > > Any requirement of FWU should not require changes to bindings
> > > > of other subsystems. For example, for mtd-backed storage we
> > > > can do without requiring 'fixed-partitions' children to also
> > > > carry 'uuid', a property which is non-standard and not in the
> > > > bindings.
> > > >
> > > >  There exists no code yet, so we can change the fwu-mtd bindings
> > > > to contain all properties within the fwu-mdata node.
> > > >
> > > > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > > > ---
> > > >
> > > > Hi Rob, Hi Krzysztof,
> > > >
> > > >   I was suggested, and I agree, it would be a good idea to get your blessings
> > > > for the location and meta-data (fwu-mdata) bindings for the FWU.
> > > >
> > > >   The FWU images can be located in GPT partitions or MTD backed storage.
> > > > The basic bindings for fwu-mdata has already been merged in uboot (ideally they
> > > > too should have had your review). Now I am trying to fully support MTD backed
> > > > storage and hence looking for your review. The proposed bindings are totally
> > > > self-contained and don't require changes to any other subsystem.
> > > >
> > > > Thanks.
> > >
> > > I think we do not review U-Boot bindings usually, except these put in
> > > the Linux kernel. There were few targeting U-Boot specifically (e.g.
> > > Documentation/devicetree/bindings/mtd/partitions/u-boot.yaml and
> > > Documentation/devicetree/bindings/nvmem/u-boot,env.yaml) so if you want
> > > our blessing, the bindings should be done in Linux kernel repo.
> > >
> > > I am pretty sure that reviewing other project bindings would be too much
> > > of work for me.
> >
> > Sure that makes sense.  But an answer here of whether the bindings make
> > sense to the DT maintainers or not would help to move forward.
>
> I have lots of comments on the bindings, but I'm missing what is the
> problem to solve. Yes, it's 'A/B firmware updates', but what
> configuration data do you need, why and when do you need it. IOW, give
> me enough information to understand why a binding is designed the way
> it is and be able to propose alternatives.

Thanks for taking the time on this.  I do have questions for Jassi as
well, but let me describe what I have in mind first. What we are trying
to do here is standardize how EFI compliant devices (and in extent SystemReady-IR)
can reliably perform firmware updates adding capabilities like rollback, brick
protection etc.

The high level description of how this is done is described here [0].
The really short version of that document is that you:
- Perform a capsule update of the non active partition
- Reboot to that partition and launch your OS
- If the OS approves the updates (which is implementation defined), then
  you install an empty capsule indicating the acceptance of the update
- If the device for any reason reboots for X amounts of times and the
  acceptance capsule is not found, it will revert to the primary partition.
- If the acceptance capsule is found the device permanently switches over
  to the new firmware, bumping any rollback counters it might have (that's
  optional).
- All these information along with UUIDs for the storage medium and the
  image types are stored in the metadata [1], which also has a backup
  location.

What you fundamentally need to perform an A/B booting is a first stage boot
loader (or a masked ROM) that can understand the metadata described here
[1].

Ideally we would only require the DT to describe where we can find that
metadata -- but the reality is a bit different.  Currently we only support
of 2 types of devices
- metadata lives in a GPT partition
- They are located in an specific offset on a SPI-NOR

GPT based devices are straightforward.  As you noticed we only need the phandle
of the device.  There is a defined UUID for the metadata, so we discover
them along with the backup partition.

The MTD case is a bit more complicated. The metadata format only
defines GUIDs for
- UUID identifying the image type (image type GUID)
- the UUID of the storage volume where the image is located

Since we now don't have a UUID of the storage volume where the image is
located, we somehow need to map the image UUID to a device offset. I think
this is what Jassi is trying to define here -- it's worth noting that this
has to happen for all non-GPT devices.

As for when the data is needed, since this is backed up by EFI capsule updates,
we only need access from the bootloader (and that's where the update agent
currently resides, it's part of the u-boot EFI capsule functionality).

>
> That's easy enough to deduce for the GPT case. It's just needing to
> know which device has the f/w update GPT? That's easily solved with an
> alias, a boolean property in the device, or property with the disk
> GUID. All of those options are much more simple than a whole node and
> compatible.
>

Sure you can do that.  But wouldn't it be easier for bootloader to 'just'
look for a compatible node regardless of where the metadata is stored?
Sughosh did the u-boot implementation so maybe he can chime in on the
challenges he saw code-wise.

>
> > These bindings are trying to define a standardized interface for A/B *firmware*
> > updates [0] which is not what traditionally goes into a device tree.  OTOH we
> > already have some U-Boot specific bindings as you already mentioned.  As we
> > move forward we need to be very precise on what is allowed or not on the DT
> > since it's now tested and verified on SystemReady certifications.  IOW if
> > we add those binding in U-Boot only, we would need to strip them before
> > handing the DT to linux, otherwise certification would fail.  If you do
> > think that having them in the kernel repo makes sense,  it would help
> > standardizing other boot loaders (at least it would standardize where that
> > metadata lives) if they want to implement something similar.
>
> I think it is fine if u-boot has things in DT which aren't an ABI, so
> private and bundled, but I agree those should be stripped before
> passing. To put it another way, if it's not an ABI nor shared amongst
> projects, I don't think we need a schema nor to care outside of
> u-boot.

Exactly, we completely agree on this.

>
> However, it doesn't seem to me that needs for A/B updates would be
> unique to u-boot.

Yes.  TBH I was reluctant in trying to send the bindings upstream, since
they are not related to hardware at all.  But as I mentioned the only
reason to have them standardized is if people buy in to that update
document and want to replicate the functionality.

>
> > Just keep in mind we would need a schema per storage medium.  IOW this tries to
> > standardize devices which keep the firmware binary in an mtd.  There's also
> > another biding which describes firmware files on a GPT [1].
>
> I'm assuming it's per partition type rather than storage medium (e.g.
> SATA, USB, SD, NAND, SPI-NOR)? GPT, 'fixed-partitions', other DT
> partition bindings, etc. If so, then I'm really wondering why it's a
> standalone tree rather than integrated into those existing partition
> bindings.

I think it's per medium, unless I misunderstood something.  Sughosh?


Thanks!
/Ilias
>
> Rob

[0] https://gitlab.com/Linaro/trustedsubstrate/mbfw/uploads/3d0d7d11ca9874dc9115616b418aa330/mbfw.pdf
[1] https://developer.arm.com/documentation/den0118/a
Sughosh Ganu May 4, 2023, 11:57 a.m. UTC | #9
hi Ilias,

On Thu, 4 May 2023 at 16:15, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Rob,
>
> On Wed, May 03, 2023 at 12:24:39PM -0500, Rob Herring wrote:
> > On Wed, May 3, 2023 at 9:37 AM Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Krzysztof,
> > >
> > > On Tue, Apr 11, 2023 at 07:38:11AM +0200, Krzysztof Kozlowski wrote:
> > > > On 11/04/2023 01:21, jaswinder.singh@linaro.org wrote:
> > > > > From: Jassi Brar <jaswinder.singh@linaro.org>
> > > > >
> > > > > Any requirement of FWU should not require changes to bindings
> > > > > of other subsystems. For example, for mtd-backed storage we
> > > > > can do without requiring 'fixed-partitions' children to also
> > > > > carry 'uuid', a property which is non-standard and not in the
> > > > > bindings.
> > > > >
> > > > >  There exists no code yet, so we can change the fwu-mtd bindings
> > > > > to contain all properties within the fwu-mdata node.
> > > > >
> > > > > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > > > > ---
> > > > >
> > > > > Hi Rob, Hi Krzysztof,
> > > > >
> > > > >   I was suggested, and I agree, it would be a good idea to get your blessings
> > > > > for the location and meta-data (fwu-mdata) bindings for the FWU.
> > > > >
> > > > >   The FWU images can be located in GPT partitions or MTD backed storage.
> > > > > The basic bindings for fwu-mdata has already been merged in uboot (ideally they
> > > > > too should have had your review). Now I am trying to fully support MTD backed
> > > > > storage and hence looking for your review. The proposed bindings are totally
> > > > > self-contained and don't require changes to any other subsystem.
> > > > >
> > > > > Thanks.
> > > >
> > > > I think we do not review U-Boot bindings usually, except these put in
> > > > the Linux kernel. There were few targeting U-Boot specifically (e.g.
> > > > Documentation/devicetree/bindings/mtd/partitions/u-boot.yaml and
> > > > Documentation/devicetree/bindings/nvmem/u-boot,env.yaml) so if you want
> > > > our blessing, the bindings should be done in Linux kernel repo.
> > > >
> > > > I am pretty sure that reviewing other project bindings would be too much
> > > > of work for me.
> > >
> > > Sure that makes sense.  But an answer here of whether the bindings make
> > > sense to the DT maintainers or not would help to move forward.
> >
> > I have lots of comments on the bindings, but I'm missing what is the
> > problem to solve. Yes, it's 'A/B firmware updates', but what
> > configuration data do you need, why and when do you need it. IOW, give
> > me enough information to understand why a binding is designed the way
> > it is and be able to propose alternatives.
>
> Thanks for taking the time on this.  I do have questions for Jassi as
> well, but let me describe what I have in mind first. What we are trying
> to do here is standardize how EFI compliant devices (and in extent SystemReady-IR)
> can reliably perform firmware updates adding capabilities like rollback, brick
> protection etc.
>
> The high level description of how this is done is described here [0].
> The really short version of that document is that you:
> - Perform a capsule update of the non active partition
> - Reboot to that partition and launch your OS
> - If the OS approves the updates (which is implementation defined), then
>   you install an empty capsule indicating the acceptance of the update
> - If the device for any reason reboots for X amounts of times and the
>   acceptance capsule is not found, it will revert to the primary partition.
> - If the acceptance capsule is found the device permanently switches over
>   to the new firmware, bumping any rollback counters it might have (that's
>   optional).
> - All these information along with UUIDs for the storage medium and the
>   image types are stored in the metadata [1], which also has a backup
>   location.
>
> What you fundamentally need to perform an A/B booting is a first stage boot
> loader (or a masked ROM) that can understand the metadata described here
> [1].
>
> Ideally we would only require the DT to describe where we can find that
> metadata -- but the reality is a bit different.  Currently we only support
> of 2 types of devices
> - metadata lives in a GPT partition
> - They are located in an specific offset on a SPI-NOR
>
> GPT based devices are straightforward.  As you noticed we only need the phandle
> of the device.  There is a defined UUID for the metadata, so we discover
> them along with the backup partition.
>
> The MTD case is a bit more complicated. The metadata format only
> defines GUIDs for
> - UUID identifying the image type (image type GUID)
> - the UUID of the storage volume where the image is located
>
> Since we now don't have a UUID of the storage volume where the image is
> located, we somehow need to map the image UUID to a device offset. I think
> this is what Jassi is trying to define here -- it's worth noting that this
> has to happen for all non-GPT devices.
>
> As for when the data is needed, since this is backed up by EFI capsule updates,
> we only need access from the bootloader (and that's where the update agent
> currently resides, it's part of the u-boot EFI capsule functionality).
>
> >
> > That's easy enough to deduce for the GPT case. It's just needing to
> > know which device has the f/w update GPT? That's easily solved with an
> > alias, a boolean property in the device, or property with the disk
> > GUID. All of those options are much more simple than a whole node and
> > compatible.
> >
>
> Sure you can do that.  But wouldn't it be easier for bootloader to 'just'
> look for a compatible node regardless of where the metadata is stored?
> Sughosh did the u-boot implementation so maybe he can chime in on the
> challenges he saw code-wise.

We can indeed have a boolean property in the device which will have
the fwu metadata stored. The reason for having the compatible property
is that the fwu metadata access functions have been implemented in a
driver, and the u-boot driver model requires having a node with a
compatible property for the driver's probe function to be called, when
the corresponding device is accessed. This was primarily the reason
for having a separate node with the compatible property.

>
> >
> > > These bindings are trying to define a standardized interface for A/B *firmware*
> > > updates [0] which is not what traditionally goes into a device tree.  OTOH we
> > > already have some U-Boot specific bindings as you already mentioned.  As we
> > > move forward we need to be very precise on what is allowed or not on the DT
> > > since it's now tested and verified on SystemReady certifications.  IOW if
> > > we add those binding in U-Boot only, we would need to strip them before
> > > handing the DT to linux, otherwise certification would fail.  If you do
> > > think that having them in the kernel repo makes sense,  it would help
> > > standardizing other boot loaders (at least it would standardize where that
> > > metadata lives) if they want to implement something similar.
> >
> > I think it is fine if u-boot has things in DT which aren't an ABI, so
> > private and bundled, but I agree those should be stripped before
> > passing. To put it another way, if it's not an ABI nor shared amongst
> > projects, I don't think we need a schema nor to care outside of
> > u-boot.
>
> Exactly, we completely agree on this.
>
> >
> > However, it doesn't seem to me that needs for A/B updates would be
> > unique to u-boot.
>
> Yes.  TBH I was reluctant in trying to send the bindings upstream, since
> they are not related to hardware at all.  But as I mentioned the only
> reason to have them standardized is if people buy in to that update
> document and want to replicate the functionality.
>
> >
> > > Just keep in mind we would need a schema per storage medium.  IOW this tries to
> > > standardize devices which keep the firmware binary in an mtd.  There's also
> > > another biding which describes firmware files on a GPT [1].
> >
> > I'm assuming it's per partition type rather than storage medium (e.g.
> > SATA, USB, SD, NAND, SPI-NOR)? GPT, 'fixed-partitions', other DT
> > partition bindings, etc. If so, then I'm really wondering why it's a
> > standalone tree rather than integrated into those existing partition
> > bindings.
>
> I think it's per medium, unless I misunderstood something.  Sughosh?

The bindings would be required as per the metadata access mechanism.
So for example, the MTD bindings would suffice for all the storage
mediums which would have MTD based partitioning schemes. Same for all
GPT partitioned devices. Again, as for the need for a separate node
with the compatible property, it is as per the driver model design.
For the other properties, we can indeed have them integrated with the
corresponding partition bindings.

-sughosh
Ilias Apalodimas May 4, 2023, 12:08 p.m. UTC | #10
[...]

> > > I'm assuming it's per partition type rather than storage medium (e.g.
> > > SATA, USB, SD, NAND, SPI-NOR)? GPT, 'fixed-partitions', other DT
> > > partition bindings, etc. If so, then I'm really wondering why it's a
> > > standalone tree rather than integrated into those existing partition
> > > bindings.
> >
> > I think it's per medium, unless I misunderstood something.  Sughosh?
>
> The bindings would be required as per the metadata access mechanism.
> So for example, the MTD bindings would suffice for all the storage
> mediums which would have MTD based partitioning schemes. Same for all
> GPT partitioned devices. Again, as for the need for a separate node
> with the compatible property, it is as per the driver model design.
> For the other properties, we can indeed have them integrated with the
> corresponding partition bindings.

Ok, Rob is correct then it really is per partition type.

Thanks
/Ilias
>
> -sughosh
Jassi Brar May 4, 2023, 2:01 p.m. UTC | #11
On Thu, 4 May 2023 at 07:08, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
> [...]
>
> > > > I'm assuming it's per partition type rather than storage medium (e.g.
> > > > SATA, USB, SD, NAND, SPI-NOR)? GPT, 'fixed-partitions', other DT
> > > > partition bindings, etc. If so, then I'm really wondering why it's a
> > > > standalone tree rather than integrated into those existing partition
> > > > bindings.
> > >
> > > I think it's per medium, unless I misunderstood something.  Sughosh?
> >
> > The bindings would be required as per the metadata access mechanism.
> > So for example, the MTD bindings would suffice for all the storage
> > mediums which would have MTD based partitioning schemes. Same for all
> > GPT partitioned devices. Again, as for the need for a separate node
> > with the compatible property, it is as per the driver model design.
> > For the other properties, we can indeed have them integrated with the
> > corresponding partition bindings.
>
> Ok, Rob is correct then it really is per partition type.
>
Originally the fwu related properties were embedded into existing nodes.

As Sughosh already explained, we need a compatible string, and hence a
node, for the u-boot driver core to call probe().
 I may be wrong, but I see having fwu properties contained within the
fwu node is cleaner than having them embedded into existing bindings
(potentially different classes in future). So I moved to the current
design.

cheers.
Rob Herring May 4, 2023, 3:19 p.m. UTC | #12
On Thu, May 4, 2023 at 9:01 AM Jassi Brar <jaswinder.singh@linaro.org> wrote:
>
> On Thu, 4 May 2023 at 07:08, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> > [...]
> >
> > > > > I'm assuming it's per partition type rather than storage medium (e.g.
> > > > > SATA, USB, SD, NAND, SPI-NOR)? GPT, 'fixed-partitions', other DT
> > > > > partition bindings, etc. If so, then I'm really wondering why it's a
> > > > > standalone tree rather than integrated into those existing partition
> > > > > bindings.
> > > >
> > > > I think it's per medium, unless I misunderstood something.  Sughosh?
> > >
> > > The bindings would be required as per the metadata access mechanism.
> > > So for example, the MTD bindings would suffice for all the storage
> > > mediums which would have MTD based partitioning schemes. Same for all
> > > GPT partitioned devices. Again, as for the need for a separate node
> > > with the compatible property, it is as per the driver model design.
> > > For the other properties, we can indeed have them integrated with the
> > > corresponding partition bindings.
> >
> > Ok, Rob is correct then it really is per partition type.
> >
> Originally the fwu related properties were embedded into existing nodes.
>
> As Sughosh already explained, we need a compatible string, and hence a
> node, for the u-boot driver core to call probe().

DT's purpose is not OS driver instantiation. DT cannot cater to a
client's evolving driver structure.

>  I may be wrong, but I see having fwu properties contained within the
> fwu node is cleaner than having them embedded into existing bindings
> (potentially different classes in future). So I moved to the current
> design.

Having all the information related to a device/node in one place is cleaner IMO.

As I said, if u-boot wants private interfaces between the DT and
itself, then fine, but that should remain private and be stripped by
u-boot. A separate node would certainly be easier for doing that.

Rob
Jassi Brar May 4, 2023, 3:39 p.m. UTC | #13
On Thu, 4 May 2023 at 10:19, Rob Herring <robh+dt@kernel.org> wrote:
> On Thu, May 4, 2023 at 9:01 AM Jassi Brar <jaswinder.singh@linaro.org> wrote:
>
> >  I may be wrong, but I see having fwu properties contained within the
> > fwu node is cleaner than having them embedded into existing bindings
> > (potentially different classes in future). So I moved to the current
> > design.
>
> Having all the information related to a device/node in one place is cleaner IMO.
>
> As I said, if u-boot wants private interfaces between the DT and
> itself, then fine, but that should remain private and be stripped by
> u-boot. A separate node would certainly be easier for doing that.
>
Seems we are on the same page(?). Current implementation does exactly
that -- we have a separate fwu node containing all the properties it
needs.

thanks.
Tom Rini May 4, 2023, 4:19 p.m. UTC | #14
On Thu, May 04, 2023 at 10:39:06AM -0500, Jassi Brar wrote:
> On Thu, 4 May 2023 at 10:19, Rob Herring <robh+dt@kernel.org> wrote:
> > On Thu, May 4, 2023 at 9:01 AM Jassi Brar <jaswinder.singh@linaro.org> wrote:
> >
> > >  I may be wrong, but I see having fwu properties contained within the
> > > fwu node is cleaner than having them embedded into existing bindings
> > > (potentially different classes in future). So I moved to the current
> > > design.
> >
> > Having all the information related to a device/node in one place is cleaner IMO.
> >
> > As I said, if u-boot wants private interfaces between the DT and
> > itself, then fine, but that should remain private and be stripped by
> > u-boot. A separate node would certainly be easier for doing that.
> >
> Seems we are on the same page(?). Current implementation does exactly
> that -- we have a separate fwu node containing all the properties it
> needs.

Well, isn't part of why we're here is that this isn't strictly a U-Boot
only thing? My question is can, and then is, this also being used in
other projects yet?
Ilias Apalodimas May 4, 2023, 4:30 p.m. UTC | #15
Replying to both Jassi and Tom here since it makes more sense,

On Thu, 4 May 2023 at 19:19, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, May 04, 2023 at 10:39:06AM -0500, Jassi Brar wrote:
> > On Thu, 4 May 2023 at 10:19, Rob Herring <robh+dt@kernel.org> wrote:
> > > On Thu, May 4, 2023 at 9:01 AM Jassi Brar <jaswinder.singh@linaro.org> wrote:
> > >
> > > >  I may be wrong, but I see having fwu properties contained within the
> > > > fwu node is cleaner than having them embedded into existing bindings
> > > > (potentially different classes in future). So I moved to the current
> > > > design.
> > >
> > > Having all the information related to a device/node in one place is cleaner IMO.
> > >
> > > As I said, if u-boot wants private interfaces between the DT and
> > > itself, then fine, but that should remain private and be stripped by
> > > u-boot. A separate node would certainly be easier for doing that.
> > >
> > Seems we are on the same page(?). Current implementation does exactly
> > that -- we have a separate fwu node containing all the properties it
> > needs.

I think Rob is saying the exact opposite.  The way I see it we either
- Keep the bindings as an internal u-boot ABI, in which case the
current format is fine, but we need to strip it from the DT before
handing it over to the OS.
- Alternatively, if we want to submit it upstream, we need to change
where that data lives and ideally have them under existing partition
bindings.

Both would work, with the latter offering a bit more standardization
if another bootloader tries to implement something similar.

>
> Well, isn't part of why we're here is that this isn't strictly a U-Boot
> only thing? My question is can, and then is, this also being used in
> other projects yet?

I am not aware of any other projects currently using it.  I'll repeat
myself though, it would be useful to have the format standardized in
case other bootloaders have similar needs.  In any case I am happy
with the discussion so far.  We (as in u-boot) need to decide which of
the above directions suits as best and either send the bindings
upstream, or clean then up before we boot.

Cheers
/Ilias

>
> --
> Tom
Jassi Brar May 4, 2023, 4:41 p.m. UTC | #16
On Thu, 4 May 2023 at 11:31, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Replying to both Jassi and Tom here since it makes more sense,
>
> On Thu, 4 May 2023 at 19:19, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, May 04, 2023 at 10:39:06AM -0500, Jassi Brar wrote:
> > > On Thu, 4 May 2023 at 10:19, Rob Herring <robh+dt@kernel.org> wrote:
> > > > On Thu, May 4, 2023 at 9:01 AM Jassi Brar <jaswinder.singh@linaro.org> wrote:
> > > >
> > > > >  I may be wrong, but I see having fwu properties contained within the
> > > > > fwu node is cleaner than having them embedded into existing bindings
> > > > > (potentially different classes in future). So I moved to the current
> > > > > design.
> > > >
> > > > Having all the information related to a device/node in one place is cleaner IMO.
> > > >
> > > > As I said, if u-boot wants private interfaces between the DT and
> > > > itself, then fine, but that should remain private and be stripped by
> > > > u-boot. A separate node would certainly be easier for doing that.
> > > >
> > > Seems we are on the same page(?). Current implementation does exactly
> > > that -- we have a separate fwu node containing all the properties it
> > > needs.
>
> I think Rob is saying the exact opposite.  The way I see it we either
> - Keep the bindings as an internal u-boot ABI, in which case the
> current format is fine, but we need to strip it from the DT before
> handing it over to the OS.
>
I interpreted "Having all the information related to a device/node in
one place is cleaner IMO." to suggest this approach.
Though the stripping part remains tbd. Where should that be done in u-boot?

> - Alternatively, if we want to submit it upstream, we need to change
> where that data lives and ideally have them under existing partition
> bindings.
>
Here we may have to add to bindings of various subsystems (as support
widens) and still have to strip the property before handover to the
kernel. right?

> Both would work, with the latter offering a bit more standardization
> if another bootloader tries to implement something similar.
>
> >
> > Well, isn't part of why we're here is that this isn't strictly a U-Boot
> > only thing? My question is can, and then is, this also being used in
> > other projects yet?
>
> I am not aware of any other projects currently using it.
>
Maybe I am overlooking something, but the kernel should not have
anything to do with it. EDK2 may use the node as such and BL1/2 would
have the meta-data location info hardcoded into them (?)

thanks.
Michal Simek May 5, 2023, 8:38 a.m. UTC | #17
On 5/4/23 17:19, Rob Herring wrote:
> On Thu, May 4, 2023 at 9:01 AM Jassi Brar <jaswinder.singh@linaro.org> wrote:
>>
>> On Thu, 4 May 2023 at 07:08, Ilias Apalodimas
>> <ilias.apalodimas@linaro.org> wrote:
>>> [...]
>>>
>>>>>> I'm assuming it's per partition type rather than storage medium (e.g.
>>>>>> SATA, USB, SD, NAND, SPI-NOR)? GPT, 'fixed-partitions', other DT
>>>>>> partition bindings, etc. If so, then I'm really wondering why it's a
>>>>>> standalone tree rather than integrated into those existing partition
>>>>>> bindings.
>>>>>
>>>>> I think it's per medium, unless I misunderstood something.  Sughosh?
>>>>
>>>> The bindings would be required as per the metadata access mechanism.
>>>> So for example, the MTD bindings would suffice for all the storage
>>>> mediums which would have MTD based partitioning schemes. Same for all
>>>> GPT partitioned devices. Again, as for the need for a separate node
>>>> with the compatible property, it is as per the driver model design.
>>>> For the other properties, we can indeed have them integrated with the
>>>> corresponding partition bindings.
>>>
>>> Ok, Rob is correct then it really is per partition type.
>>>
>> Originally the fwu related properties were embedded into existing nodes.
>>
>> As Sughosh already explained, we need a compatible string, and hence a
>> node, for the u-boot driver core to call probe().
> 
> DT's purpose is not OS driver instantiation. DT cannot cater to a
> client's evolving driver structure.
> 
>>   I may be wrong, but I see having fwu properties contained within the
>> fwu node is cleaner than having them embedded into existing bindings
>> (potentially different classes in future). So I moved to the current
>> design.
> 
> Having all the information related to a device/node in one place is cleaner IMO.
> 
> As I said, if u-boot wants private interfaces between the DT and
> itself, then fine, but that should remain private and be stripped by
> u-boot. A separate node would certainly be easier for doing that.

In dt-schema
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/options/u-boot.yaml#L14

description: |
   The u-boot,config node provides basic configuration information to U-Boot,
   for use during its execution. It can be used to control U-Boot's behaviour
   in various ways.

We are preparing patch to add some more properties for u-boot itself.
I thought that Simon already solved what should be location for U-Boot only 
binding by creating this file.

Thanks,
Michal
Rob Herring May 5, 2023, 3:47 p.m. UTC | #18
On Thu, May 4, 2023 at 11:42 AM Jassi Brar <jaswinder.singh@linaro.org> wrote:
>
> On Thu, 4 May 2023 at 11:31, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Replying to both Jassi and Tom here since it makes more sense,
> >
> > On Thu, 4 May 2023 at 19:19, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, May 04, 2023 at 10:39:06AM -0500, Jassi Brar wrote:
> > > > On Thu, 4 May 2023 at 10:19, Rob Herring <robh+dt@kernel.org> wrote:
> > > > > On Thu, May 4, 2023 at 9:01 AM Jassi Brar <jaswinder.singh@linaro.org> wrote:
> > > > >
> > > > > >  I may be wrong, but I see having fwu properties contained within the
> > > > > > fwu node is cleaner than having them embedded into existing bindings
> > > > > > (potentially different classes in future). So I moved to the current
> > > > > > design.
> > > > >
> > > > > Having all the information related to a device/node in one place is cleaner IMO.
> > > > >
> > > > > As I said, if u-boot wants private interfaces between the DT and
> > > > > itself, then fine, but that should remain private and be stripped by
> > > > > u-boot. A separate node would certainly be easier for doing that.
> > > > >
> > > > Seems we are on the same page(?). Current implementation does exactly
> > > > that -- we have a separate fwu node containing all the properties it
> > > > needs.
> >
> > I think Rob is saying the exact opposite.  The way I see it we either
> > - Keep the bindings as an internal u-boot ABI, in which case the
> > current format is fine, but we need to strip it from the DT before
> > handing it over to the OS.
> >
> I interpreted "Having all the information related to a device/node in
> one place is cleaner IMO." to suggest this approach.

The one place being where there is already partition information in
the partition nodes. This binding adds a 2nd location with pointers to
the 1st location.

> Though the stripping part remains tbd. Where should that be done in u-boot?

Yes.

> > - Alternatively, if we want to submit it upstream, we need to change
> > where that data lives and ideally have them under existing partition
> > bindings.
> >
> Here we may have to add to bindings of various subsystems (as support
> widens) and still have to strip the property before handover to the
> kernel. right?
>
> > Both would work, with the latter offering a bit more standardization
> > if another bootloader tries to implement something similar.
> >
> > >
> > > Well, isn't part of why we're here is that this isn't strictly a U-Boot
> > > only thing? My question is can, and then is, this also being used in
> > > other projects yet?
> >
> > I am not aware of any other projects currently using it.
> >
> Maybe I am overlooking something, but the kernel should not have
> anything to do with it.

Um, LinuxBoot, kexec?

> EDK2 may use the node as such and BL1/2 would
> have the meta-data location info hardcoded into them (?)

Certainly not hard to imagine others needing this.

Rob
diff mbox series

Patch

diff --git a/doc/device-tree-bindings/firmware/fwu-mdata-mtd.yaml b/doc/device-tree-bindings/firmware/fwu-mdata-mtd.yaml
index 4f5404f999..6a22aeea30 100644
--- a/doc/device-tree-bindings/firmware/fwu-mdata-mtd.yaml
+++ b/doc/device-tree-bindings/firmware/fwu-mdata-mtd.yaml
@@ -1,13 +1,13 @@ 
 # SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/firmware/u-boot,fwu-mdata-sf.yaml#
-$schema: http://devicetree.org/meta-schemas/core.yaml#
+$id: http://devicetree.org/schemas/firmware/u-boot,fwu-mdata-mtd.yaml#
+$schema: http://devicetree.org/meta-schemas/base.yaml#
 
 title: FWU metadata on MTD device without GPT
 
 maintainers:
- - Masami Hiramatsu <masami.hiramatsu@linaro.org>
+ - Jassi Brar <jaswinder.singh@linaro.org>
 
 properties:
   compatible:
@@ -15,24 +15,101 @@  properties:
       - const: u-boot,fwu-mdata-mtd
 
   fwu-mdata-store:
-    maxItems: 1
-    description: Phandle of the MTD device which contains the FWU medatata.
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Phandle of the MTD device which contains the FWU MetaData and Banks.
 
-  mdata-offsets:
+  mdata-parts:
+    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
     minItems: 2
-    description: Offsets of the primary and secondary FWU metadata in the NOR flash.
+    maxItems: 2
+    description: labels of the primary and secondary FWU metadata partitions in the 'fixed-partitions' subnode of the 'jedec,spi-nor' flash device node.
+
+  patternProperties:
+    "fwu-bank[0-9]":
+    type: object
+    description: List of FWU mtd-backed banks. Typically two banks.
+
+    properties:
+      id:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: Index of the bank.
+
+      label:
+        $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+        minItems: 1
+        maxItems: 1
+        description: label of the partition, in the 'fixed-partitions' subnode of the 'jedec,spi-nor' flash device node, that holds this bank.
+
+      patternProperties:
+        "fwu-image[0-9]":
+        type: object
+        description: List of images in the FWU mtd-backed bank.
+
+        properties:
+          id:
+            $ref: /schemas/types.yaml#/definitions/uint32
+            description: Index of the bank.
+
+          offset:
+            $ref: /schemas/types.yaml#/definitions/uint32
+            description: Offset, from start of the bank, where the image is located.
+
+          size:
+            $ref: /schemas/types.yaml#/definitions/uint32
+            description: Size reserved for the image.
+
+          uuid:
+            $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+            minItems: 1
+            maxItems: 1
+            description: UUID of the image.
+
+        required:
+          - id
+          - offset
+          - size
+          - uuid
+        additionalProperties: false
+
+    required:
+      - id
+      - label
+      - fwu-images
+    additionalProperties: false
 
 required:
   - compatible
   - fwu-mdata-store
-  - mdata-offsets
-
+  - mdata-parts
+  - fwu-banks
 additionalProperties: false
 
 examples:
   - |
-    fwu-mdata {
-        compatible = "u-boot,fwu-mdata-mtd";
-        fwu-mdata-store = <&spi-flash>;
-        mdata-offsets = <0x500000 0x530000>;
-    };
+	fwu-mdata {
+		compatible = "u-boot,fwu-mdata-mtd";
+		fwu-mdata-store = <&flash0>;
+		mdata-parts = "MDATA-Pri", "MDATA-Sec";
+
+		fwu-bank0 {
+			id = <0>;
+			label = "FIP-Bank0";
+			fwu-image0 {
+				id = <0>;
+				offset = <0x0>;
+				size = <0x400000>;
+				uuid = "5a66a702-99fd-4fef-a392-c26e261a2828";
+			};
+		};
+		fwu-bank1 {
+			id = <1>;
+			label = "FIP-Bank1";
+			fwu-image0 {
+				id = <0>;
+				offset = <0x0>;
+				size = <0x400000>;
+				uuid = "a8f868a1-6e5c-4757-878d-ce63375ef2c0";
+			};
+		};
+	};
+...