On 9/22/21 8:42 PM, Tom Rini wrote:
On Wed, Sep 22, 2021 at 08:24:18PM +0200, Marek Vasut wrote:
On 9/22/21 7:29 PM, Marek Behún wrote:
(Adding also Tom.)

Hi Patrick, Marek,

I find this either not complete or not needed:

- either you need mtd names to be of this format so that old MTDPARTS
    config definitions do not need to be changed, i.e. something like
      CONFIG_MTDPARTS_DEFAULT="nor0:1M(u-boot),0x1000@0xfff000(env)"
    does not work currently, and you want to make it work.

    I find your solution here incomplete because MTDPARTS can also be
    used to be passed to Linux as mtdparts parameter, but there is no
    guarantee that the "norN" numbering you are creating in U-Boot will
    be the same as the one in kernel.

- or it is not needed, because you can remove MTDPARTS definition from
    the board config entirely and move the information into device tree.
    In fact this was the main idea behind making the series
      Support SPI NORs and OF partitions in `mtd list`
    The SPI-NOR MTDs after this series can have conflicting names,
    because you can still choose between them via OF path with the `mtd`
    command.

    Tom and I were of the opinion that MTDPARTS should be deprecated and
    removed in favor of OF. Marek Vasut says that this is not possible
    for every board, and so needs to stay.

BTW, I find it a little weird for Marek to defend old API which should
be converted to DT, when in discussion about DM USB / Nokia N900
USB TTY console [1] he was defending the opinion that we should be
heading to DT in U-Boot.

[1]
https://patchwork.ozlabs.org/project/uboot/patch/20210618145724.2558-1-p...@kernel.org/

That USB discussion is completely unrelated to the problem here, the USB
discussion is about internal (i.e. not user facing) conversion to DM/DT. The
user-facing ABI does not change there. Also, that discussion was about
patching USB stack to permit new non-DM/DT operation, not fixing existing
one.

This problem here is user facing ABI, the mtdparts/mtdids. That user facing
ABI got broken. Boards which do depend on it, even those currently in tree,
are broken. Not all boards can update their environment, so some backward
compatibility of the user facing ABI should be in place, even though it
might not be to the degree Linux kernel does so. So far, it seems most of
the U-Boot command line interface has managed to retain backward
compatibility, I don't see why this here should be handled any differently.

Note that there are not just a few boards that are broken, but hundreds. I
believe that itself justifies a fix, instead of just throwing all those
hundreds of boards overboard.

u-boot$ git grep -l CONFIG_MTDIDS configs | wc -l
203

Hopefully that clarifies the difference.

It doesn't quite, sorry.  If you have "mtdparts=... mtdids=..." in your
cmdline that you pass to Linux, U-Boot doesn't care.

The MTDIDS is used by UBI code in U-Boot to locate from which MTD device to attach UBI. That is currently broken at least for UBI on SPI NOR or parallel NOR. This has nothing to do with passing mtdparts/mtdids to Linux.

That's one of the
main users of CONFIG_MTDIDS/MTDPARTS today, which could in some good
number of cases be removed (take am335x_evm_defconfig for example, the
table has been defined in the upstream DT for forever).  Taking a look
at:
commit 938db6fe5da3581ed2c17d64c7e016a7a8df5237
Author: Miquel Raynal <miquel.ray...@bootlin.com>
Date:   Sat Sep 29 12:58:30 2018 +0200

     cmd: mtdparts: describe as legacy
The 'mtdparts' command is not needed anymore. While the environment
     variable is still valid (and useful, along with the 'mtdids' one), the
     command has been replaced by 'mtd' which is much more close to the MTD
     stack and do not add its own specific glue.
Signed-off-by: Miquel Raynal <miquel.ray...@bootlin.com>
     Reviewed-by: Stefan Roese <s...@denx.de>
     Reviewed-by: Boris Brezillon <boris.brezil...@bootlin.com>

Is when "mtdparts" in U-Boot was noted as legacy.  So what exactly are
we fixing with this series?  Nothing changed about hard-coded values
being passed along.  What may have broken was some progmatic way to set
those, but I think that's both fragile and deprecated in favor of the
table being in the DT.

Reply via email to