From mboxrd@z Thu Jan  1 00:00:00 1970
From: Miquel Raynal <miquel.raynal@bootlin.com>
Date: Thu, 12 Jul 2018 14:51:32 +0200
Subject: [U-Boot] [PATCH v2 20/21] cmd: mtd: add 'mtd' command
In-Reply-To: <20180712004213.71286527@bbrezillon>
References: <20180711152529.24547-1-miquel.raynal@bootlin.com>
 <20180711152529.24547-21-miquel.raynal@bootlin.com>
 <20180712004213.71286527@bbrezillon>
Message-ID: <20180712145132.0d22dcce@xps13>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
To: u-boot@lists.denx.de

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:
>=20
> > +
> > +static void mtd_show_device(struct mtd_info *mtd)
> > +{
> > +	printf("* %s", mtd->name); =20
>=20
> Printing the device type might be interesting? And maybe also the size,
> writesize, oobsize and erasesize?

Absolutely.

>=20
> > +	if (mtd->dev)
> > +		printf(" [device: %s] [parent: %s] [driver: %s]",
> > +		       mtd->dev->name, mtd->dev->parent->name,
> > +		       mtd->dev->driver->name);
> > +
> > +	printf("\n");
> > +} =20

[...]

> > +	} else if (!strcmp(cmd, "erase")) {
> > +		bool scrub =3D strstr(cmd, ".dontskipbad");
> > +		bool full_erase =3D strstr(cmd, ".chip"); =20
>=20
> 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.

>=20
> > +		struct erase_info erase_op =3D {};
> > +		u64 off, len;
> > +
> > +		off =3D argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
> > +		len =3D argc > 1 ? simple_strtoul(argv[1], NULL, 16) : mtd->erasesiz=
e; =20
>=20
> 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.

>=20
> > +
> > +		if (full_erase) {
> > +			off =3D 0;
> > +			len =3D mtd->size;
> > +		}
> > +
> > +		if ((u32)off % mtd->erasesize) { =20
>=20
> Sounds dangerous. We have 8GB NANDs on sunxi platforms...

[...]

> > +
> > +		if ((u32)len % mtd->erasesize) { =20
>=20
> 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=C3=A8l