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