On Tue, 30 Oct 2018 23:43:40 +0100 Miquel Raynal <miquel.ray...@bootlin.com> wrote:
> Hi Boris, > > Boris Brezillon <boris.brezil...@bootlin.com> wrote on Tue, 30 Oct 2018 > 23:02:50 +0100: > > > On Tue, 30 Oct 2018 11:59:13 +0100 > > Stefan Roese <s...@denx.de> wrote: > > > > > On 30.10.18 11:41, Boris Brezillon wrote: > > > > On Tue, 30 Oct 2018 11:13:37 +0100 > > > > Stefan Roese <s...@denx.de> wrote: > > > > > > > >> Hi Boris, > > > >> > > > >> On 30.10.18 11:03, Boris Brezillon wrote: > > > >>> On Tue, 30 Oct 2018 10:51:51 +0100 > > > >>> Stefan Roese <s...@denx.de> wrote: > > > >>> > > > >>>> Calling "mtdparts" currently fails when its called before any other > > > >>>> mtd > > > >>>> command (or ubi command) has been called. The MTD devices are not > > > >>>> probed at this point and therefore it fails e.g. with this message: > > > >>>> > > > >>>> => mtdparts > > > >>>> Device spi-nand0 not found! > > > >>> > > > >>> IIRC, we decided that mtdparts should not call mtd_probe_devices() to > > > >>> encourage people to stop using it. > > > >> > > > >> I see. But I don't quite get how this missing call (and reslting > > > >> error message) would encourage people to stop using it. > > > > > > > > You're right, this message does not encourage people to stop using > > > > mtdparts on existing setups (mtdparts should work just fine on any MTD > > > > devices except SPI NANDs) but it does discourage them from using it on > > > > spi-nand devices since it returns an error. > > > > > > IMHO, that's more confusing than discouraging. > > > > > > >>>> > > > >>>> This patch adds a call to mtd_probe_devices() to mtdparts_init() to > > > >>>> solve this issue. This also fixes a problem when calling "ubi part" > > > >>>> as first flash storage related command. > > > >>> > > > >>> Hm, this one is unexpected. Miquel, any idea why this happens. Do we > > > >>> need to enable a specific option if we want mtd_probe_devices() to be > > > >>> called in the ubi part path? > > > >> > > > >> Please note that "ubi part part-foo" does still work. It only > > > >> prints this error message before attaching the MTD partition. > > > >> The error is printed because of this call-chain: > > > >> > > > >> ubi_part() > > > >> -> ubi_detach() > > > >> -> mtdparts_init() > > > >> > > > >> So again, mtdparts_init() is called without the MTD devices > > > >> being probed. > > > > > > > > I guess we forgot to remove this mtdparts_init() call from the detach > > > > path. I think it's no longer needed since we now call > > > > mtd_probe_devices() in ubi_part(), and mtd_probe_devices() will take > > > > care of creating MTD partitions based on the mtdparts= and mtdids= > > > > variables. > > > > > > A quick test reveals that this removal does not remove the > > > error message. Instead the command does not work anymore at > > > all: > > > > > > => ubi part nand > > > Partition nand not found! > > > > > > Before (and without my patch) its this: > > > > > > => ubi part nand > > > Device spi-nand0 not found! > > > Error initializing mtdparts! > > > ubi0: attaching mtd2 > > > ubi0: scanning is finished > > > ubi0: attached mtd2 (name "nand", size 128 MiB) > > > > I think I found what's missing in mtd_probe_devices(): we don't use the > > default mtdparts and mtdids when those env vars are NULL (see what's > > done in mtdparts_init() to handle this case [1]). > > > > [1]https://elixir.bootlin.com/u-boot/v2018.11-rc3/source/cmd/mtdparts.c#L1763 > > > > Isn't the right solution to always define these env variables when they > are needed? Defining such default behavior with a Kconfig entry is, > from my opinion, a lot of noise for such an useless feature... The thing is, we want to support existing setups, and apparently not everyone define its default mtdids/mtdparts in their default env. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
