Hi Boris,
Boris Brezillon <boris.brezil...@bootlin.com> wrote on Thu, 12 Jul 2018
00:42:13 +0200:
> On Wed, 11 Jul 2018 17:25:28 +0200
> Miquel Raynal <miquel.ray...@bootlin.com> wrote:
>
> > +
> > +static void mtd_show_device(struct mtd_info *mtd)
> > +{
> > + printf("* %s", mtd->name);
>
> Printing the device type might be interesting? And maybe also the size,
> writesize, oobsize and erasesize?
Absolutely.
>
> > + if (mtd->dev)
> > + printf(" [device: %s] [parent: %s] [driver: %s]",
> > + mtd->dev->name, mtd->dev->parent->name,
> > + mtd->dev->driver->name);
> > +
> > + printf("\n");
> > +}
[...]
> > + } else if (!strcmp(cmd, "erase")) {
> > + bool scrub = strstr(cmd, ".dontskipbad");
> > + bool full_erase = strstr(cmd, ".chip");
>
> Again, I think .chip extension is not needed. Just consider a full erase
> is requested when offset and length are not provided. Plus, chip is
> misleading since I guess mtd partitions are also exposed as mtd devices,
> and could thus be erased with the .chip extension as well.
".chip" removed.
Default now is mtd->size (the entire MTD device) for erase/read/write,
and mtd->writesize (a page) for a dump.
>
> > + struct erase_info erase_op = {};
> > + u64 off, len;
> > +
> > + off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
> > + len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) :
> > mtd->erasesize;
>
> Hm. Are we sure we want the default to be 1 eraseblock? Shouldn't we
> consider that missing size means "erase up to the MTD device end". Same
> goes for the read/write accesses, but maybe not for dump where dumping
> a single page makes more sense.
See above.
>
> > +
> > + if (full_erase) {
> > + off = 0;
> > + len = mtd->size;
> > + }
> > +
> > + if ((u32)off % mtd->erasesize) {
>
> Sounds dangerous. We have 8GB NANDs on sunxi platforms...
[...]
> > +
> > + if ((u32)len % mtd->erasesize) {
>
> Same here. I guess there's a do_div() in uboot.
In both cases I take the less significant bytes of a 64-bit value. The
modulo operation is safe as long as mtd->erasesize is a 32-bit value
too. I don't think there is any danger?
[...]
Thanks,
Miquèl
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
