Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
MIME-Version: 1.0
References: <20231004093620.2b1d6917@xps-13> <20231004113458.531124-1-mwalle@kernel.org>
 <CAPnjgZ2hWE6Sc=rg55W=-r-TnoWP7Y5gSpn41kwoyja-AMVw+w@mail.gmail.com>
 <9e588e3ec8c0c321a2861723d0d42b9a@kernel.org> <CAPnjgZ20ezipPWAj6bUM9_oCTcX1XzuLqQ7b7-nKjXf1t4p9-Q@mail.gmail.com>
 <a581ef73fa09c6ffeb83a1c1780053bd@kernel.org> <CAPnjgZ2PnKD5m0EgTdEAf-gcK3wuBZvWw_AO2iehb1dmfdoz3A@mail.gmail.com>
 <27d37d4c7cf353d99737a1e7a450f9f7@kernel.org>
In-Reply-To: <27d37d4c7cf353d99737a1e7a450f9f7@kernel.org>
From:   Simon Glass <sjg@chromium.org>
Date:   Fri, 6 Oct 2023 06:39:16 -0600
Message-ID: <CAPnjgZ0OZubVaj1RR6rWrJu2MWD=4PEPOD39zhaHgm6OzsyQnw@mail.gmail.com>
Subject: Re: [PATCH v2 1/3] dt-bindings: mtd: fixed-partitions: Add binman compatible
To:     Michael Walle <mwalle@kernel.org>
Cc:     miquel.raynal@bootlin.com, conor+dt@kernel.org,
        devicetree@vger.kernel.org, krzysztof.kozlowski+dt@linaro.org,
        linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org,
        ptyadav@amazon.de, rafal@milecki.pl, richard@nod.at,
        robh+dt@kernel.org, robh@kernel.org, trini@konsulko.com,
        u-boot@lists.denx.de, vigneshr@ti.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Michael,

On Fri, 6 Oct 2023 at 02:37, Michael Walle <mwalle@kernel.org> wrote:
>
> Hi,
>
> >> I'm still not sure why that compatible is needed. Also I'd need to
> >> change
> >> the label which might break user space apps looking for that specific
> >> name.
> >>
> >> Also, our board might have u-boot/spl or u-boot/spl/bl31/bl32, right
> >> now
> >> that's something which depends on an u-boot configuration variable,
> >> which
> >> then enables or disables binman nodes in the -u-boot.dtsi. So in linux
> >> we only have that "bootloader" partition, but there might be either
> >> u-boot+spl or u-boot+spl+bl31+bl32.
> >>
> >> Honestly, I'm really not sure this should go into a device tree.
> >
> > I think we might be getting a bit ahead of ourselves here. I thought
> > that the decision was that the label should indicate the contents.
> > If you have multiple things in a partition then it would become a
> > 'section' in Binman's terminology. Either the label programmatically
> > describes what is inside or it doesn't. We can't have it both ways.
> > What do you suggest?
>
> As Rob pointed out earlier, it's just a user-facing string. I'm a bit
> reluctant to use it programatically.
> Taking my example again, the string "bootloader" is sufficient for a
> user. He doesn't care if it's u-boot with spl or u-boot with tfa, or
> even coreboot. It just says, "in this partition is the bootloader".
> If you have an "bootloader" image you can flash it there.
>
> If it has a label "u-boot" and I want to switch to coreboot, will
> it have to change to "coreboot"? I really don't think this is practical,
> you are really putting software configuration into the device tree.

I thought Rob changed his mind on that?

I agree that compatible would make things easier. We could then
continue to use 'label' for whatever it currently has.

Note that some kernel drivers or user space will want to look at what
is there, e.g. to provide a way to extract, check or update a
particular part of the firmware.

>
> > At present it seems you have the image described in two places - one
> > is the binman node and the other is the partitions node. I would like
> > to unify these.
>
> And I'm not sure that will work for all the corner cases :/
>
> If you keep the binman section seperate from the flash partition
> definition you don't have any of these problems, although there is
> some redundancy:
>   - you only have compatible = "binman", "fixed-partition", no further
>     compatibles are required
>   - you don't have any conflicts with the current partition descriptions
>   - you could even use the labels, because binman is the (only?) user
>
> But of course you need to find a place where to put your node.

Sure, but I was pointed to partitions as the place where this should live.

>
> > What does user space do with the partition labels?
>
> I'm not sure. Also I'm not sure if it really matters, I just wanted to
> point out, that you'll force users to change it.

OK I'll send a version that uses compatible strings and see if that
makes any progress.

Regards,
Simon

>
> -michael
>
> >> >> What if a board uses eMMC to store the firmware binaries? Will that
> >> >> then
> >> >> be a subnode to the eMMC device?
> >> >
> >> > I thought there was a way to link the partition nodes and the device
> >> > using a property, without having the partition info as a subnode of
> >> > the device. But I may have imagined it as I cannot find it now. So
> >> > yes, it will be a subnode of the eMMC device.
> >>
> >> Not sure if that will fly.
> >
> > I can't find it anyway. There is somelike like that in
> > simple-framebuffer with the 'display' property.
> >
> > Regards,
> > SImon