[U-Boot] [PATCH v2 20/21] cmd: mtd: add 'mtd' command

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Boris,

Boris Brezillon <boris.brezillon@bootlin.com> wrote on Thu, 12 Jul 2018
00:42:13 +0200:

> On Wed, 11 Jul 2018 17:25:28 +0200
> Miquel Raynal <miquel.raynal@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