Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191127105522.31445-1-miquel.raynal@bootlin.com>
 <20191127105522.31445-5-miquel.raynal@bootlin.com> <20191209113506.41341ed4@collabora.com>
In-Reply-To: <20191209113506.41341ed4@collabora.com>
From:   Rob Herring <robh+dt@kernel.org>
Date:   Tue, 14 Jan 2020 11:46:18 -0600
Message-ID: <CAL_JsqJP3-h7bPAommzt7KQKoohZpkk=RMxfN1j3rXbisD4eCA@mail.gmail.com>
Subject: Re: [PATCH v5 4/4] mtd: Add driver for concatenating devices
To:     Boris Brezillon <boris.brezillon@collabora.com>
Cc:     Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Tudor Ambarus <Tudor.Ambarus@microchip.com>,
        Mark Rutland <mark.rutland@arm.com>,
        MTD Maling List <linux-mtd@lists.infradead.org>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        devicetree@vger.kernel.org,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Mark Brown <broonie@kernel.org>,
        Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
        Bernhard Frauendienst <kernel@nospam.obeliks.de>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Dec 9, 2019 at 4:35 AM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Wed, 27 Nov 2019 11:55:22 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> > Introduce a generic way to define concatenated MTD devices. This may
> > be very useful in the case of ie. stacked SPI-NOR. Partitions to
> > concatenate are described in an additional property of the partitions
> > subnode:
> >
> >         flash0 {
> >                 partitions {
> >                         compatible = "fixed-partitions";
> >                         part-concat = <&flash0_part1>, <&flash1_part0>;
> >
> >                       part0@0 {
> >                               label = "part0_0";
> >                               reg = <0x0 0x800000>;
> >                       };
> >
> >                       flash0_part1: part1@800000 {
> >                               label = "part0_1";
> >                               reg = <0x800000 0x800000>;
>
> So, flash0_part1 and flash0_part2 will be created even though the user
> probably doesn't need them?

I don't follow?

>
> >                       };
> >                 };
> >         };
> >
> >         flash1 {
> >                 partitions {
> >                         compatible = "fixed-partitions";
> >
> >                       flash0_part1: part1@0 {
> >                               label = "part1_0";
> >                               reg = <0x0 0x800000>;
> >                       };
> >
> >                       part0@800000 {
> >                               label = "part1_1";
> >                               reg = <0x800000 0x800000>;
> >                       };
> >                 };
> >         };
>
> IMHO this representation is far from intuitive. At first glance it's not
> obvious which partitions are linked together and what's the name of the
> resulting concatenated part. I definitely prefer the solution where we
> have a virtual device describing the concatenation. I also understand
> that this goes against the #1 DT rule: "DT only decribes HW blocks, not
> how they should be used/configured", but maybe we can find a compromise
> here, like moving this description to the /chosen node?
>
> chosen {
>         flash-arrays {
>                 /*
>                  * my-flash-array is the MTD name if label is
>                  * not present.
>                  */
>                 my-flash-array {
>                         /*
>                          * We could have
>                          * compatible = "flash-array";
>                          * but we can also do without it.
>                          */
>                         label = "foo";
>                         flashes = <&flash1 &flash2 ...>;
>                         partitions {
>                                 /* usual partition description. */
>                                 ...
>                         };
>                 };
>         };
> };
>
> Rob, what do you think?

I don't think chosen is the right place to put all the partition
information. It's not something the bootloader configures.

This suffers from the same issue I have with the original proposal. It
will not work for existing s/w. There's only 1 logical partition that
concatenated. The rest of the partitions shouldn't need any special
handling. So we really only need some way to say 'link these 2
partitions into 1 logical partition'. Though perhaps one could want to
combine any number of physical partitions into logical partitions, but
then none of the proposals could support that. Then again, maybe
that's a userspace problem like with disks.

To throw out another option, what if the first device contains the
complete partitions for both devices with some property in one or both
devices pointing to the other device? That would make the partitions
in the 1st device still accessible to existing s/w (unless it bounds
checks the partitions).

Rob