Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
On Mon, Oct 8, 2018 at 2:10 PM Thomas Petazzoni wrote: > > Hello, > > On Mon, 8 Oct 2018 19:46:27 +0200, Boris Brezillon wrote: > > > > + /* Check if mtdparts/mtdids changed since last call, otherwise: exit > > > */ > > > + if (!strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids)) > > > + return 0; > > > > Should be: > > > > if ((!mtdparts && !old_mtdparts && !mtdids && !old_mtdids) || > > If there's a || here, it means you can split the conditions into to: > > if (!mtdparts && !old_mtdparts && !mtdids && !old_mtdids) > return 0; > > if (mtdparts && old_mtdparts && mtdids && old_mtdids && > !strcmp(mtdparts, old_mtdparts) && > !strcmp(mtdids, old_mtdids))) > return 0; > > Best regards, Is there a reason, it can't be left the way it was? adam > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > ___ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
Hello, On Mon, 8 Oct 2018 19:46:27 +0200, Boris Brezillon wrote: > > + /* Check if mtdparts/mtdids changed since last call, otherwise: exit */ > > + if (!strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids)) > > + return 0; > > Should be: > > if ((!mtdparts && !old_mtdparts && !mtdids && !old_mtdids) || If there's a || here, it means you can split the conditions into to: if (!mtdparts && !old_mtdparts && !mtdids && !old_mtdids) return 0; if (mtdparts && old_mtdparts && mtdids && old_mtdids && !strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))) return 0; Best regards, Thomas -- Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
On Mon, Oct 8, 2018 at 12:46 PM Boris Brezillon
wrote:
>
> On Mon, 1 Oct 2018 15:43:29 +0200
> Miquel Raynal wrote:
>
> > +#if defined(CONFIG_MTD_PARTITIONS)
> > +int mtd_probe_devices(void)
> > +{
> > + static char *old_mtdparts;
> > + static char *old_mtdids;
> > + const char *mtdparts = env_get("mtdparts");
> > + const char *mtdids = env_get("mtdids");
> > + bool remaining_partitions = true;
> > + struct mtd_info *mtd;
> > +
> > + mtd_probe_uclass_mtd_devs();
> > +
> > + /* Check if mtdparts/mtdids changed since last call, otherwise: exit
> > */
> > + if (!strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))
> > + return 0;
>
> Should be:
>
> if ((!mtdparts && !old_mtdparts && !mtdids && !old_mtdids) ||
> (mtdparts && old_mtdparts && mtdids && old_mtdids &&
> !strcmp(mtdparts, old_mtdparts) &&
> !strcmp(mtdids, old_mtdids)))
> return 0;
>
Thanks for the suggestion.
I created a patch to change this. I forgot to add the Suggested-by
note. If whomever the maintainer wants to edit the patch, comments,
etc. go ahead. Just know it now works on omap3_logic.
> > +
> > + /* Update the local copy of mtdparts */
> > + free(old_mtdparts);
> > + free(old_mtdids);
> > + old_mtdparts = strdup(mtdparts);
> > + old_mtdids = strdup(mtdids);
> > +
> > + /* If at least one partition is still in use, do not delete anything
> > */
> > + mtd_for_each_device(mtd) {
> > + if (mtd->usecount) {
> > + printf("Partition \"%s\" already in use, aborting\n",
> > +mtd->name);
> > + return -EACCES;
> > + }
> > + }
> > +
> > + /*
> > + * Everything looks clear, remove all partitions. It is not safe to
> > + * remove entries from the mtd_for_each_device loop as it uses idr
> > + * indexes and the partitions removal is done in bulk (all partitions
> > of
> > + * one device at the same time), so break and iterate from start each
> > + * time a new partition is found and deleted.
> > + */
> > + while (remaining_partitions) {
> > + remaining_partitions = false;
> > + mtd_for_each_device(mtd) {
> > + if (!mtd_is_partition(mtd) &&
> > mtd_has_partitions(mtd)) {
> > + del_mtd_partitions(mtd);
> > + remaining_partitions = true;
> > + break;
> > + }
> > + }
> > + }
> > +
>
> We should bail out if either mtdparts or mtdids is NULL:
>
> if (!mtdparts || !mtdids)
> return 0;
And add this in the patch as well.
>
> > + /* Start the parsing by ignoring the extra 'mtdparts=' prefix, if any
> > */
> > + if (strstr(mtdparts, "mtdparts="))
> > + mtdparts += 9;
> > +
> > + /* For each MTD device in mtdparts */
> > + while (mtdparts[0] != '\0') {
> > + char mtd_name[MTD_NAME_MAX_LEN], *colon;
> > + struct mtd_partition *parts;
> > + int mtd_name_len, nparts;
> > + int ret;
> > +
> > + colon = strchr(mtdparts, ':');
> > + if (!colon) {
> > + printf("Wrong mtdparts: %s\n", mtdparts);
> > + return -EINVAL;
> > + }
> > +
> > + mtd_name_len = colon - mtdparts;
> > + strncpy(mtd_name, mtdparts, mtd_name_len);
> > + mtd_name[mtd_name_len] = '\0';
> > + /* Move the pointer forward (including the ':') */
> > + mtdparts += mtd_name_len + 1;
> > + mtd = get_mtd_device_nm(mtd_name);
> > + if (IS_ERR_OR_NULL(mtd)) {
> > + char linux_name[MTD_NAME_MAX_LEN];
> > +
> > + /*
> > + * The MTD device named "mtd_name" does not exist. Try
> > + * to find a correspondance with an MTD device having
> > + * the same type and number as defined in the mtdids.
> > + */
> > + debug("No device named %s\n", mtd_name);
> > + ret = mtd_search_alternate_name(mtd_name, linux_name,
> > + MTD_NAME_MAX_LEN);
> > + if (!ret)
> > + mtd = get_mtd_device_nm(linux_name);
> > +
> > + /*
> > + * If no device could be found, move the mtdparts
> > + * pointer forward until the next set of partitions.
> > + */
> > + if (ret || IS_ERR_OR_NULL(mtd)) {
> > + printf("Could not find a valid device for
> > %s\n",
> > +mtd_name);
> > + mtdparts =
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
On Mon, 1 Oct 2018 15:43:29 +0200
Miquel Raynal wrote:
> +#if defined(CONFIG_MTD_PARTITIONS)
> +int mtd_probe_devices(void)
> +{
> + static char *old_mtdparts;
> + static char *old_mtdids;
> + const char *mtdparts = env_get("mtdparts");
> + const char *mtdids = env_get("mtdids");
> + bool remaining_partitions = true;
> + struct mtd_info *mtd;
> +
> + mtd_probe_uclass_mtd_devs();
> +
> + /* Check if mtdparts/mtdids changed since last call, otherwise: exit */
> + if (!strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))
> + return 0;
Should be:
if ((!mtdparts && !old_mtdparts && !mtdids && !old_mtdids) ||
(mtdparts && old_mtdparts && mtdids && old_mtdids &&
!strcmp(mtdparts, old_mtdparts) &&
!strcmp(mtdids, old_mtdids)))
return 0;
> +
> + /* Update the local copy of mtdparts */
> + free(old_mtdparts);
> + free(old_mtdids);
> + old_mtdparts = strdup(mtdparts);
> + old_mtdids = strdup(mtdids);
> +
> + /* If at least one partition is still in use, do not delete anything */
> + mtd_for_each_device(mtd) {
> + if (mtd->usecount) {
> + printf("Partition \"%s\" already in use, aborting\n",
> +mtd->name);
> + return -EACCES;
> + }
> + }
> +
> + /*
> + * Everything looks clear, remove all partitions. It is not safe to
> + * remove entries from the mtd_for_each_device loop as it uses idr
> + * indexes and the partitions removal is done in bulk (all partitions of
> + * one device at the same time), so break and iterate from start each
> + * time a new partition is found and deleted.
> + */
> + while (remaining_partitions) {
> + remaining_partitions = false;
> + mtd_for_each_device(mtd) {
> + if (!mtd_is_partition(mtd) && mtd_has_partitions(mtd)) {
> + del_mtd_partitions(mtd);
> + remaining_partitions = true;
> + break;
> + }
> + }
> + }
> +
We should bail out if either mtdparts or mtdids is NULL:
if (!mtdparts || !mtdids)
return 0;
> + /* Start the parsing by ignoring the extra 'mtdparts=' prefix, if any */
> + if (strstr(mtdparts, "mtdparts="))
> + mtdparts += 9;
> +
> + /* For each MTD device in mtdparts */
> + while (mtdparts[0] != '\0') {
> + char mtd_name[MTD_NAME_MAX_LEN], *colon;
> + struct mtd_partition *parts;
> + int mtd_name_len, nparts;
> + int ret;
> +
> + colon = strchr(mtdparts, ':');
> + if (!colon) {
> + printf("Wrong mtdparts: %s\n", mtdparts);
> + return -EINVAL;
> + }
> +
> + mtd_name_len = colon - mtdparts;
> + strncpy(mtd_name, mtdparts, mtd_name_len);
> + mtd_name[mtd_name_len] = '\0';
> + /* Move the pointer forward (including the ':') */
> + mtdparts += mtd_name_len + 1;
> + mtd = get_mtd_device_nm(mtd_name);
> + if (IS_ERR_OR_NULL(mtd)) {
> + char linux_name[MTD_NAME_MAX_LEN];
> +
> + /*
> + * The MTD device named "mtd_name" does not exist. Try
> + * to find a correspondance with an MTD device having
> + * the same type and number as defined in the mtdids.
> + */
> + debug("No device named %s\n", mtd_name);
> + ret = mtd_search_alternate_name(mtd_name, linux_name,
> + MTD_NAME_MAX_LEN);
> + if (!ret)
> + mtd = get_mtd_device_nm(linux_name);
> +
> + /*
> + * If no device could be found, move the mtdparts
> + * pointer forward until the next set of partitions.
> + */
> + if (ret || IS_ERR_OR_NULL(mtd)) {
> + printf("Could not find a valid device for %s\n",
> +mtd_name);
> + mtdparts = strchr(mtdparts, ';');
> + if (mtdparts)
> + mtdparts++;
> +
> + continue;
> + }
> + }
> +
> + /*
> + * Parse the MTD device partitions. It will update the mtdparts
> + * pointer, create an array of parts (that must be freed), and
> + * return the number of partition structures in the array.
> + */
> + ret = mtd_parse_partitions(mtd, , , );
> + if (ret) {
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
On Mon, 8 Oct 2018 11:58:40 -0500
Adam Ford wrote:
> On Mon, Oct 8, 2018 at 11:52 AM Adam Ford wrote:
> >
> > On Mon, Oct 8, 2018 at 11:28 AM Boris Brezillon
> > wrote:
> > >
> > > Hi Adam,
> > >
> > > On Mon, 8 Oct 2018 11:13:40 -0500
> > > Adam Ford wrote:
> > >
> > > > On Wed, Oct 3, 2018 at 8:41 AM Adam Ford wrote:
> > > > >
> > > > > On Wed, Oct 3, 2018 at 8:35 AM Miquel Raynal
> > > > > wrote:
> > > > > >
> > > > > > Hi Adam,
> > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > I can use the nand read/write functions and mtdparts lists
> > > > > > > > > > the
> > > > > > > > > > partitions, so I know nand works. My defconfig
> > > > > > > > > > lists the partitions, so if we're not supposed to use
> > > > > > > > > > mtdparts, where
> > > > > > > > > > I do store the partition information?
> > > > > > > > >
> > > > > > > > > You are not supposed to use the mtdpart _command_, but the
> > > > > > > > > mtdparts
> > > > > > > > > _variable_ must be used in order to declare the partitions.
> > > > > > > >
> > > > > > > > OK. If I can get MTD working, I'll work to remove the other
> > > > > > > > commands
> > > > > > > > like NAND and MTDPARTS
> > > > > > >
> > > > > > > As of today, the process of migration is not entirely finished to
> > > > > > > DM
> > > > > > > and you might still need to issue *first* a "nand probe" to
> > > > > > > register
> > > > > > > the device operations.
> > > > > >
> > > > > > Mmmh there is no nand probe actually, for raw nands like the one you
> > > > > > have it should work out of the box.
> > > >
> > > > i haven't actually insterted debug code yet, but I started a quick
> > > > code review. There is a function called 'mtd_probe_uclass_mtd_devs'
> > > > which states it will probe with DM compliant drivers.
> > > > I am thinking the nand and/or GPMC drivers are not yet DM compliant
> > > > yet. The DM tree doesn't list any of the MTD parts.
> > > >
> > > > Is it save to assume it just wont' work until the drives are DM
> > > > compliant, or is this driver designed to play with the lower-level
> > > > drivers as-is?
> > >
> > > The whole point of this rework was to allow both old (non-DM compliant)
> > > and new (DM compliant) drivers to be exposed the same way to the upper
> > > layers. If it doesn't work for regular NAND drivers it's a bug, and we
> > > should fix it.
> >
> > That's what I assummed, so I kept trying to debug, but I wanted to
> > make sure I asked the question, because I've been burned by
> > assumptions before.
> >
> > I narrowed the hang down to the following in driver/mtd/mtd_uboot,
> > mtd_probe_devices
> >
> > int mtd_probe_devices(void)
> > {
> > ...
> > /* Check if mtdparts/mtdids changed since last call, otherwise: exit */
> > if (!strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))
Can you try with
if (old_mtdparts && old_mtdids &&
!strcmp(mtdparts, old_mtdparts) &&
!strcmp(mtdids, old_mtdids))
> > return 0;
> > }
> >
> > The above function neither returns nor exits. I added a brace and a
> > printf just before the return, and the printf never appears.
> > Only when I put #if 0 around the 'if' statement above, and 'mtd list'
> > returned and did a dump.
> >
> > I tried initializing
> > static char *old_mtdparts = '\0';
> > static char *old_mtdids = '\0';
> >
> > and
> >
> > static char *old_mtdparts = NULL;
> > static char *old_mtdids = NULL;
> >
> > Neither fixed the hang.
> >
> > adam
> >
> > The functioning dump (with the ifdef) is as follows:
> >
> > List of MTD devices:
> > * nand0
> > - type: NAND flash
> > - block size: 0x2 bytes
> > - min I/O: 0x800 bytes
> > - OOB size: 64 bytes
> > - OOB available: 10 bytes
> > - ECC strength: 8 bits
> > - ECC step size: 512 bytes
> > - bitflip threshold: 6 bits
> > - 0x-0x2000 : "nand0"
> > - 0x-0x0008 : "MLO"
> > - 0x0008-0x0024 : "u-boot"
> > - 0x0024-0x0026 : "spl-os"
> > - 0x0026-0x0028 : "u-boot-env"
> > - 0x0028-0x0088 : "kernel"
> > - 0x0088-0x2000 : "fs"
> >
> >
> >
> > >
>
> Lastly, I noticed that I am not able to de-select CMD_MTDPARTS from my
> config because CMD_UBI (which I use) automatically selects
> CMD_MTDPARTS. I only mention it because I know you're trying to
> deprecate CMD_MTDPARTS.
>
> adam
>
> > > Regards,
> > >
> > > Boris
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
On Mon, Oct 8, 2018 at 11:52 AM Adam Ford wrote:
>
> On Mon, Oct 8, 2018 at 11:28 AM Boris Brezillon
> wrote:
> >
> > Hi Adam,
> >
> > On Mon, 8 Oct 2018 11:13:40 -0500
> > Adam Ford wrote:
> >
> > > On Wed, Oct 3, 2018 at 8:41 AM Adam Ford wrote:
> > > >
> > > > On Wed, Oct 3, 2018 at 8:35 AM Miquel Raynal
> > > > wrote:
> > > > >
> > > > > Hi Adam,
> > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > I can use the nand read/write functions and mtdparts lists the
> > > > > > > > > partitions, so I know nand works. My defconfig
> > > > > > > > > lists the partitions, so if we're not supposed to use
> > > > > > > > > mtdparts, where
> > > > > > > > > I do store the partition information?
> > > > > > > >
> > > > > > > > You are not supposed to use the mtdpart _command_, but the
> > > > > > > > mtdparts
> > > > > > > > _variable_ must be used in order to declare the partitions.
> > > > > > >
> > > > > > > OK. If I can get MTD working, I'll work to remove the other
> > > > > > > commands
> > > > > > > like NAND and MTDPARTS
> > > > > >
> > > > > > As of today, the process of migration is not entirely finished to DM
> > > > > > and you might still need to issue *first* a "nand probe" to register
> > > > > > the device operations.
> > > > >
> > > > > Mmmh there is no nand probe actually, for raw nands like the one you
> > > > > have it should work out of the box.
> > >
> > > i haven't actually insterted debug code yet, but I started a quick
> > > code review. There is a function called 'mtd_probe_uclass_mtd_devs'
> > > which states it will probe with DM compliant drivers.
> > > I am thinking the nand and/or GPMC drivers are not yet DM compliant
> > > yet. The DM tree doesn't list any of the MTD parts.
> > >
> > > Is it save to assume it just wont' work until the drives are DM
> > > compliant, or is this driver designed to play with the lower-level
> > > drivers as-is?
> >
> > The whole point of this rework was to allow both old (non-DM compliant)
> > and new (DM compliant) drivers to be exposed the same way to the upper
> > layers. If it doesn't work for regular NAND drivers it's a bug, and we
> > should fix it.
>
> That's what I assummed, so I kept trying to debug, but I wanted to
> make sure I asked the question, because I've been burned by
> assumptions before.
>
> I narrowed the hang down to the following in driver/mtd/mtd_uboot,
> mtd_probe_devices
>
> int mtd_probe_devices(void)
> {
> ...
> /* Check if mtdparts/mtdids changed since last call, otherwise: exit */
> if (!strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))
> return 0;
> }
>
> The above function neither returns nor exits. I added a brace and a
> printf just before the return, and the printf never appears.
> Only when I put #if 0 around the 'if' statement above, and 'mtd list'
> returned and did a dump.
>
> I tried initializing
> static char *old_mtdparts = '\0';
> static char *old_mtdids = '\0';
>
> and
>
> static char *old_mtdparts = NULL;
> static char *old_mtdids = NULL;
>
> Neither fixed the hang.
>
> adam
>
> The functioning dump (with the ifdef) is as follows:
>
> List of MTD devices:
> * nand0
> - type: NAND flash
> - block size: 0x2 bytes
> - min I/O: 0x800 bytes
> - OOB size: 64 bytes
> - OOB available: 10 bytes
> - ECC strength: 8 bits
> - ECC step size: 512 bytes
> - bitflip threshold: 6 bits
> - 0x-0x2000 : "nand0"
> - 0x-0x0008 : "MLO"
> - 0x0008-0x0024 : "u-boot"
> - 0x0024-0x0026 : "spl-os"
> - 0x0026-0x0028 : "u-boot-env"
> - 0x0028-0x0088 : "kernel"
> - 0x0088-0x2000 : "fs"
>
>
>
> >
Lastly, I noticed that I am not able to de-select CMD_MTDPARTS from my
config because CMD_UBI (which I use) automatically selects
CMD_MTDPARTS. I only mention it because I know you're trying to
deprecate CMD_MTDPARTS.
adam
> > Regards,
> >
> > Boris
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
On Mon, Oct 8, 2018 at 11:28 AM Boris Brezillon
wrote:
>
> Hi Adam,
>
> On Mon, 8 Oct 2018 11:13:40 -0500
> Adam Ford wrote:
>
> > On Wed, Oct 3, 2018 at 8:41 AM Adam Ford wrote:
> > >
> > > On Wed, Oct 3, 2018 at 8:35 AM Miquel Raynal
> > > wrote:
> > > >
> > > > Hi Adam,
> > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > I can use the nand read/write functions and mtdparts lists the
> > > > > > > > partitions, so I know nand works. My defconfig
> > > > > > > > lists the partitions, so if we're not supposed to use mtdparts,
> > > > > > > > where
> > > > > > > > I do store the partition information?
> > > > > > >
> > > > > > > You are not supposed to use the mtdpart _command_, but the
> > > > > > > mtdparts
> > > > > > > _variable_ must be used in order to declare the partitions.
> > > > > >
> > > > > > OK. If I can get MTD working, I'll work to remove the other
> > > > > > commands
> > > > > > like NAND and MTDPARTS
> > > > >
> > > > > As of today, the process of migration is not entirely finished to DM
> > > > > and you might still need to issue *first* a "nand probe" to register
> > > > > the device operations.
> > > >
> > > > Mmmh there is no nand probe actually, for raw nands like the one you
> > > > have it should work out of the box.
> >
> > i haven't actually insterted debug code yet, but I started a quick
> > code review. There is a function called 'mtd_probe_uclass_mtd_devs'
> > which states it will probe with DM compliant drivers.
> > I am thinking the nand and/or GPMC drivers are not yet DM compliant
> > yet. The DM tree doesn't list any of the MTD parts.
> >
> > Is it save to assume it just wont' work until the drives are DM
> > compliant, or is this driver designed to play with the lower-level
> > drivers as-is?
>
> The whole point of this rework was to allow both old (non-DM compliant)
> and new (DM compliant) drivers to be exposed the same way to the upper
> layers. If it doesn't work for regular NAND drivers it's a bug, and we
> should fix it.
That's what I assummed, so I kept trying to debug, but I wanted to
make sure I asked the question, because I've been burned by
assumptions before.
I narrowed the hang down to the following in driver/mtd/mtd_uboot,
mtd_probe_devices
int mtd_probe_devices(void)
{
...
/* Check if mtdparts/mtdids changed since last call, otherwise: exit */
if (!strcmp(mtdparts, old_mtdparts) && !strcmp(mtdids, old_mtdids))
return 0;
}
The above function neither returns nor exits. I added a brace and a
printf just before the return, and the printf never appears.
Only when I put #if 0 around the 'if' statement above, and 'mtd list'
returned and did a dump.
I tried initializing
static char *old_mtdparts = '\0';
static char *old_mtdids = '\0';
and
static char *old_mtdparts = NULL;
static char *old_mtdids = NULL;
Neither fixed the hang.
adam
The functioning dump (with the ifdef) is as follows:
List of MTD devices:
* nand0
- type: NAND flash
- block size: 0x2 bytes
- min I/O: 0x800 bytes
- OOB size: 64 bytes
- OOB available: 10 bytes
- ECC strength: 8 bits
- ECC step size: 512 bytes
- bitflip threshold: 6 bits
- 0x-0x2000 : "nand0"
- 0x-0x0008 : "MLO"
- 0x0008-0x0024 : "u-boot"
- 0x0024-0x0026 : "spl-os"
- 0x0026-0x0028 : "u-boot-env"
- 0x0028-0x0088 : "kernel"
- 0x0088-0x2000 : "fs"
>
> Regards,
>
> Boris
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
Hi Adam, On Mon, 8 Oct 2018 11:13:40 -0500 Adam Ford wrote: > On Wed, Oct 3, 2018 at 8:41 AM Adam Ford wrote: > > > > On Wed, Oct 3, 2018 at 8:35 AM Miquel Raynal > > wrote: > > > > > > Hi Adam, > > > > > > > > > > > > > > > > > > > > > > > > > > > > I can use the nand read/write functions and mtdparts lists the > > > > > > > partitions, so I know nand works. My defconfig > > > > > > > lists the partitions, so if we're not supposed to use mtdparts, > > > > > > > where > > > > > > > I do store the partition information? > > > > > > > > > > > > You are not supposed to use the mtdpart _command_, but the mtdparts > > > > > > _variable_ must be used in order to declare the partitions. > > > > > > > > > > OK. If I can get MTD working, I'll work to remove the other commands > > > > > like NAND and MTDPARTS > > > > > > > > As of today, the process of migration is not entirely finished to DM > > > > and you might still need to issue *first* a "nand probe" to register > > > > the device operations. > > > > > > Mmmh there is no nand probe actually, for raw nands like the one you > > > have it should work out of the box. > > i haven't actually insterted debug code yet, but I started a quick > code review. There is a function called 'mtd_probe_uclass_mtd_devs' > which states it will probe with DM compliant drivers. > I am thinking the nand and/or GPMC drivers are not yet DM compliant > yet. The DM tree doesn't list any of the MTD parts. > > Is it save to assume it just wont' work until the drives are DM > compliant, or is this driver designed to play with the lower-level > drivers as-is? The whole point of this rework was to allow both old (non-DM compliant) and new (DM compliant) drivers to be exposed the same way to the upper layers. If it doesn't work for regular NAND drivers it's a bug, and we should fix it. Regards, Boris ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
On Wed, Oct 3, 2018 at 8:41 AM Adam Ford wrote: > > On Wed, Oct 3, 2018 at 8:35 AM Miquel Raynal > wrote: > > > > Hi Adam, > > > > > > > > > > > > > > > > > > > > > > > I can use the nand read/write functions and mtdparts lists the > > > > > > partitions, so I know nand works. My defconfig > > > > > > lists the partitions, so if we're not supposed to use mtdparts, > > > > > > where > > > > > > I do store the partition information? > > > > > > > > > > You are not supposed to use the mtdpart _command_, but the mtdparts > > > > > _variable_ must be used in order to declare the partitions. > > > > > > > > OK. If I can get MTD working, I'll work to remove the other commands > > > > like NAND and MTDPARTS > > > > > > As of today, the process of migration is not entirely finished to DM > > > and you might still need to issue *first* a "nand probe" to register > > > the device operations. > > > > Mmmh there is no nand probe actually, for raw nands like the one you > > have it should work out of the box. i haven't actually insterted debug code yet, but I started a quick code review. There is a function called 'mtd_probe_uclass_mtd_devs' which states it will probe with DM compliant drivers. I am thinking the nand and/or GPMC drivers are not yet DM compliant yet. The DM tree doesn't list any of the MTD parts. Is it save to assume it just wont' work until the drives are DM compliant, or is this driver designed to play with the lower-level drivers as-is? adam > > I have a few tasks to do today for work, but I'll try to do some > testing as you suggested this week and possibly later tonight. > > adam > > ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
On Wed, Oct 3, 2018 at 8:35 AM Miquel Raynal wrote: > > Hi Adam, > > > > > > > > > > > > > > > > > > I can use the nand read/write functions and mtdparts lists the > > > > > partitions, so I know nand works. My defconfig > > > > > lists the partitions, so if we're not supposed to use mtdparts, where > > > > > I do store the partition information? > > > > > > > > You are not supposed to use the mtdpart _command_, but the mtdparts > > > > _variable_ must be used in order to declare the partitions. > > > > > > OK. If I can get MTD working, I'll work to remove the other commands > > > like NAND and MTDPARTS > > > > As of today, the process of migration is not entirely finished to DM > > and you might still need to issue *first* a "nand probe" to register > > the device operations. > > Mmmh there is no nand probe actually, for raw nands like the one you > have it should work out of the box. I have a few tasks to do today for work, but I'll try to do some testing as you suggested this week and possibly later tonight. adam > ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
Hi Adam, > > > > > > > > > > > > > I can use the nand read/write functions and mtdparts lists the > > > > partitions, so I know nand works. My defconfig > > > > lists the partitions, so if we're not supposed to use mtdparts, where > > > > I do store the partition information? > > > > > > You are not supposed to use the mtdpart _command_, but the mtdparts > > > _variable_ must be used in order to declare the partitions. > > > > OK. If I can get MTD working, I'll work to remove the other commands > > like NAND and MTDPARTS > > As of today, the process of migration is not entirely finished to DM > and you might still need to issue *first* a "nand probe" to register > the device operations. Mmmh there is no nand probe actually, for raw nands like the one you have it should work out of the box. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
Hi Adam, Adam Ford wrote on Wed, 3 Oct 2018 07:47:25 -0500: > On Wed, Oct 3, 2018 at 7:43 AM Miquel Raynal > wrote: > > > > Hi Adam, > > > > Adam Ford wrote on Wed, 3 Oct 2018 07:35:15 -0500: > > > > > On Mon, Oct 1, 2018 at 8:48 AM Miquel Raynal > > > wrote: > > > > > > > > There should not be a 'nand' command, a 'sf' command and certainly not > > > > a new 'spi-nand' command. Write a 'mtd' command instead to manage all > > > > MTD devices/partitions at once. This should be the preferred way to > > > > access any MTD device. > > > > > > What is the expected behavior when I type 'mtd list' on my omap37 > > > board, it just hangs. > > > > What do you mean "hangs", does U-Boot crashes? Or is it really hanging > > with no more on the console? Can you Ctrl-C to cancel the command or is > > it really stuck? > > It's really stuck > > U-Boot 2018.11-rc1-00636-g592cd5defd (Oct 03 2018 - 07:28:27 -0500) > > OMAP3630/3730-GP ES1.2, CPU-OPP2, L3-200MHz, Max CPU Clock 800 MHz > Model: LogicPD Zoom OMAP3 Development Kit > Logic DM37x/OMAP35x reference board + LPDDR/NAND > DRAM: 256 MiB > NAND: 512 MiB > MMC: OMAP SD/MMC: 0 > Loading Environment from NAND... OK > OMAP die ID: 15529ff80168301018021018 > Board: DM37xx Torpedo > Net: smc911x-0 > Hit any key to stop autoboot: 0 > OMAP Logic # mtd list > > Control-C does nothing. > > > > > > > > > > I can use the nand read/write functions and mtdparts lists the > > > partitions, so I know nand works. My defconfig > > > lists the partitions, so if we're not supposed to use mtdparts, where > > > I do store the partition information? > > > > You are not supposed to use the mtdpart _command_, but the mtdparts > > _variable_ must be used in order to declare the partitions. > > OK. If I can get MTD working, I'll work to remove the other commands > like NAND and MTDPARTS As of today, the process of migration is not entirely finished to DM and you might still need to issue *first* a "nand probe" to register the device operations. For the hang, could you check the while (remaining_partitions) loop in drivers/mtd/mtd_uboot.c:mtd_probe_devices()? Otherwise if you have some time you may add more traces to track down where it hangs? > > > > > > > > > I intentionally removed it from the device tree a while ago, because > > > U-Boot was passing the partition info to Linux. > > > > Indeed, that's his primary role. > > OK, I just want to make sure I'm understanding it correctly. Sure, no pb! Thanks, Miquèl ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
On Wed, Oct 3, 2018 at 7:43 AM Miquel Raynal wrote: > > Hi Adam, > > Adam Ford wrote on Wed, 3 Oct 2018 07:35:15 -0500: > > > On Mon, Oct 1, 2018 at 8:48 AM Miquel Raynal > > wrote: > > > > > > There should not be a 'nand' command, a 'sf' command and certainly not > > > a new 'spi-nand' command. Write a 'mtd' command instead to manage all > > > MTD devices/partitions at once. This should be the preferred way to > > > access any MTD device. > > > > What is the expected behavior when I type 'mtd list' on my omap37 > > board, it just hangs. > > What do you mean "hangs", does U-Boot crashes? Or is it really hanging > with no more on the console? Can you Ctrl-C to cancel the command or is > it really stuck? It's really stuck U-Boot 2018.11-rc1-00636-g592cd5defd (Oct 03 2018 - 07:28:27 -0500) OMAP3630/3730-GP ES1.2, CPU-OPP2, L3-200MHz, Max CPU Clock 800 MHz Model: LogicPD Zoom OMAP3 Development Kit Logic DM37x/OMAP35x reference board + LPDDR/NAND DRAM: 256 MiB NAND: 512 MiB MMC: OMAP SD/MMC: 0 Loading Environment from NAND... OK OMAP die ID: 15529ff80168301018021018 Board: DM37xx Torpedo Net: smc911x-0 Hit any key to stop autoboot: 0 OMAP Logic # mtd list Control-C does nothing. > > > > > I can use the nand read/write functions and mtdparts lists the > > partitions, so I know nand works. My defconfig > > lists the partitions, so if we're not supposed to use mtdparts, where > > I do store the partition information? > > You are not supposed to use the mtdpart _command_, but the mtdparts > _variable_ must be used in order to declare the partitions. OK. If I can get MTD working, I'll work to remove the other commands like NAND and MTDPARTS > > > > > I intentionally removed it from the device tree a while ago, because > > U-Boot was passing the partition info to Linux. > > Indeed, that's his primary role. OK, I just want to make sure I'm understanding it correctly. thanks, adam > > Thanks, > Miquèl ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
Hi Adam, Adam Ford wrote on Wed, 3 Oct 2018 07:35:15 -0500: > On Mon, Oct 1, 2018 at 8:48 AM Miquel Raynal > wrote: > > > > There should not be a 'nand' command, a 'sf' command and certainly not > > a new 'spi-nand' command. Write a 'mtd' command instead to manage all > > MTD devices/partitions at once. This should be the preferred way to > > access any MTD device. > > What is the expected behavior when I type 'mtd list' on my omap37 > board, it just hangs. What do you mean "hangs", does U-Boot crashes? Or is it really hanging with no more on the console? Can you Ctrl-C to cancel the command or is it really stuck? > > I can use the nand read/write functions and mtdparts lists the > partitions, so I know nand works. My defconfig > lists the partitions, so if we're not supposed to use mtdparts, where > I do store the partition information? You are not supposed to use the mtdpart _command_, but the mtdparts _variable_ must be used in order to declare the partitions. > > I intentionally removed it from the device tree a while ago, because > U-Boot was passing the partition info to Linux. Indeed, that's his primary role. Thanks, Miquèl ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
On Mon, Oct 1, 2018 at 8:48 AM Miquel Raynal wrote:
>
> There should not be a 'nand' command, a 'sf' command and certainly not
> a new 'spi-nand' command. Write a 'mtd' command instead to manage all
> MTD devices/partitions at once. This should be the preferred way to
> access any MTD device.
What is the expected behavior when I type 'mtd list' on my omap37
board, it just hangs.
I can use the nand read/write functions and mtdparts lists the
partitions, so I know nand works. My defconfig
lists the partitions, so if we're not supposed to use mtdparts, where
I do store the partition information?
I intentionally removed it from the device tree a while ago, because
U-Boot was passing the partition info to Linux.
adam
>
> Signed-off-by: Miquel Raynal
> Acked-by: Jagan Teki
> Reviewed-by: Stefan Roese
> Reviewed-by: Boris Brezillon
> ---
> cmd/Kconfig | 10 +-
> cmd/Makefile| 1 +
> cmd/mtd.c | 473
> drivers/mtd/Makefile| 2 +-
> drivers/mtd/mtd_uboot.c | 161 +-
> include/mtd.h | 16 ++
> 6 files changed, 659 insertions(+), 4 deletions(-)
> create mode 100644 cmd/mtd.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 13d4c991bf..c9be85989c 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -864,6 +864,12 @@ config CMD_MMC_SWRITE
> Enable support for the "mmc swrite" command to write Android sparse
> images to eMMC.
>
> +config CMD_MTD
> + bool "mtd"
> + select MTD_PARTITIONS
> + help
> + MTD commands support.
> +
> config CMD_NAND
> bool "nand"
> default y if NAND_SUNXI
> @@ -1697,14 +1703,14 @@ config CMD_MTDPARTS
>
> config MTDIDS_DEFAULT
> string "Default MTD IDs"
> - depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
> + depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
> help
> Defines a default MTD IDs list for use with MTD partitions in the
> Linux MTD command line partitions format.
>
> config MTDPARTS_DEFAULT
> string "Default MTD partition scheme"
> - depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
> + depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
> help
> Defines a default MTD partitioning scheme in the Linux MTD command
> line partitions format
> diff --git a/cmd/Makefile b/cmd/Makefile
> index a61fab6583..3ba65d4d93 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -92,6 +92,7 @@ obj-$(CONFIG_CMD_MISC) += misc.o
> obj-$(CONFIG_CMD_MMC) += mmc.o
> obj-$(CONFIG_CMD_MMC_SPI) += mmc_spi.o
> obj-$(CONFIG_MP) += mp.o
> +obj-$(CONFIG_CMD_MTD) += mtd.o
> obj-$(CONFIG_CMD_MTDPARTS) += mtdparts.o
> obj-$(CONFIG_CMD_NAND) += nand.o
> obj-$(CONFIG_CMD_NET) += net.o
> diff --git a/cmd/mtd.c b/cmd/mtd.c
> new file mode 100644
> index 00..6142223984
> --- /dev/null
> +++ b/cmd/mtd.c
> @@ -0,0 +1,473 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * mtd.c
> + *
> + * Generic command to handle basic operations on any memory device.
> + *
> + * Copyright: Bootlin, 2018
> + * Author: Miquèl Raynal
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static uint mtd_len_to_pages(struct mtd_info *mtd, u64 len)
> +{
> + do_div(len, mtd->writesize);
> +
> + return len;
> +}
> +
> +static bool mtd_is_aligned_with_min_io_size(struct mtd_info *mtd, u64 size)
> +{
> + return !do_div(size, mtd->writesize);
> +}
> +
> +static bool mtd_is_aligned_with_block_size(struct mtd_info *mtd, u64 size)
> +{
> + return !do_div(size, mtd->erasesize);
> +}
> +
> +static void mtd_dump_buf(const u8 *buf, uint len, uint offset)
> +{
> + int i, j;
> +
> + for (i = 0; i < len; ) {
> + printf("0x%08x:\t", offset + i);
> + for (j = 0; j < 8; j++)
> + printf("%02x ", buf[i + j]);
> + printf(" ");
> + i += 8;
> + for (j = 0; j < 8; j++)
> + printf("%02x ", buf[i + j]);
> + printf("\n");
> + i += 8;
> + }
> +}
> +
> +static void mtd_dump_device_buf(struct mtd_info *mtd, u64 start_off,
> + const u8 *buf, u64 len, bool woob)
> +{
> + bool has_pages = mtd->type == MTD_NANDFLASH ||
> + mtd->type == MTD_MLCNANDFLASH;
> + int npages = mtd_len_to_pages(mtd, len);
> + uint page;
> +
> + if (has_pages) {
> + for (page = 0; page < npages; page++) {
> + u64 data_off = page * mtd->writesize;
> +
> + printf("\nDump %d data bytes from 0x%08llx:\n",
> + mtd->writesize, start_off + data_off);
> + mtd_dump_buf([data_off],
> +mtd->writesize, start_off + data_off);
> +
> + if (woob) {
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
Hi Jagan, Jagan Teki wrote on Mon, 1 Oct 2018 21:49:32 +0530: > On Monday 01 October 2018 07:13 PM, Miquel Raynal wrote: > > There should not be a 'nand' command, a 'sf' command and certainly not > > a new 'spi-nand' command. Write a 'mtd' command instead to manage all > > MTD devices/partitions at once. This should be the preferred way to > > access any MTD device. > > > Signed-off-by: Miquel Raynal > > Acked-by: Jagan Teki > > Reviewed-by: Stefan Roese > > Reviewed-by: Boris Brezillon > > --- > > [snip] > > > > static int get_part(const char *partname, int *idx, loff_t *off, > > loff_t *size, > > diff --git a/include/mtd.h b/include/mtd.h > > index 6e6da3002f..011f26b3e1 100644 > > --- a/include/mtd.h > > +++ b/include/mtd.h > > @@ -8,6 +8,9 @@ > > > #include > > > +struct udevice; > > + > > +#if defined(CONFIG_DM) > > it should be CONFIG_MTD. Same, I don't mind. > > > /* > >* Get mtd_info structure of the dev, which is stored as uclass private. > >* > > @@ -20,5 +23,18 @@ static inline struct mtd_info *mtd_get_info(struct > > udevice *dev) > > } > > > int mtd_probe(struct udevice *dev); > [...] > > there is not caller for this in non-dm, better this block empty is it? So I can't remove mtd_get_info() which has no callers at all but you want to remove the dummy helper which is not used when DM is not enabled? I have no problem with that, but the logic is not very coherent. > > Let me know so-that I can do that and apply. Yes please, you can amend this patch and apply it. BTW, the build is successful with all configurations -> https://travis-ci.org/miquelraynal/u-boot/builds/435575048 Thanks, Miquèl ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v13 5/7] cmd: mtd: add 'mtd' command
On Monday 01 October 2018 07:13 PM, Miquel Raynal wrote:
There should not be a 'nand' command, a 'sf' command and certainly not
a new 'spi-nand' command. Write a 'mtd' command instead to manage all
MTD devices/partitions at once. This should be the preferred way to
access any MTD device.
Signed-off-by: Miquel Raynal
Acked-by: Jagan Teki
Reviewed-by: Stefan Roese
Reviewed-by: Boris Brezillon
---
[snip]
static int get_part(const char *partname, int *idx, loff_t *off, loff_t *size,
diff --git a/include/mtd.h b/include/mtd.h
index 6e6da3002f..011f26b3e1 100644
--- a/include/mtd.h
+++ b/include/mtd.h
@@ -8,6 +8,9 @@
#include
+struct udevice;
+
+#if defined(CONFIG_DM)
it should be CONFIG_MTD.
/*
* Get mtd_info structure of the dev, which is stored as uclass private.
*
@@ -20,5 +23,18 @@ static inline struct mtd_info *mtd_get_info(struct udevice
*dev)
}
int mtd_probe(struct udevice *dev);
+#else
+static inline struct mtd_info *mtd_get_info(struct udevice *dev)
+{
+ return NULL;
+}
+
+static inline int mtd_probe(struct udevice *dev)
+{
+ return 0;
+}
there is not caller for this in non-dm, better this block empty is it?
Let me know so-that I can do that and apply.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
