[PATCH 1/1] commands/ubi.c: Add support for VID header offset in ubiattach

[PATCH 1/1] commands/ubi.c: Add support for VID header offset in ubiattach

[Xavier Douville]

Hi

On my board I needed to pass a custom VID header offset to ubiattach.
This patch adds the required option to the barebox command.

I also fixed a bug in the error handling of the return value of 
ubi_attach_mtd_dev().
This function doesn't always return 0 on success, it returns the UBI number.
So without this patch, an error message is printed when attaching many 
UBI devices.

thanks for your work

Xavier Douville


  commands/ubi.c |   25 +++++++++++++++++--------
  1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/commands/ubi.c b/commands/ubi.c
index 1653eaa..a10ea1c 100644
--- a/commands/ubi.c
+++ b/commands/ubi.c
@@ -60,9 +60,14 @@ static int do_ubiattach(int argc, char *argv[])
  {
      struct mtd_info_user user;
      int fd, ret;
+    int vid_hdr_offset = 0;

-    if (argc != 2)
-        return COMMAND_ERROR_USAGE;
+    if (argc != 2) {
+        if (argc == 3)
+            vid_hdr_offset = simple_strtol(argv[2], NULL, 0);
+        else
+            return COMMAND_ERROR_USAGE;
+    }

      fd = open(argv[1], O_RDWR);
      if (fd < 0) {
@@ -71,19 +76,23 @@ static int do_ubiattach(int argc, char *argv[])
      }

      ret = ioctl(fd, MEMGETINFO, &user);
-    if (!ret)
-        ret = ubi_attach_mtd_dev(user.mtd, UBI_DEV_NUM_AUTO, 0);
+    if (!ret) {
+        ret = ubi_attach_mtd_dev(user.mtd, UBI_DEV_NUM_AUTO, 
vid_hdr_offset);
+        if (ret >= 0) {
+            close(fd);
+            return 0;
+        }
+    }

-    if (ret)
-        printf("failed to attach: %s\n", strerror(-ret));
+    printf("failed to attach: %s\n", strerror(ret));

      close(fd);

-    return ret ? 1 : 0;
+    return 1;
  }

  static const __maybe_unused char cmd_ubiattach_help[] =
-"Usage: ubiattach <mtddev>\n"
+"Usage: ubiattach <mtddev> [vid_hdr_offset]\n"
  "Attach <mtddev> to ubi\n";

  BAREBOX_CMD_START(ubiattach)


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] commands/ubi.c: Add support for VID header offset in ubiattach

[Sascha Hauer]

Hi Xavier,

On Fri, Feb 08, 2013 at 03:01:53PM -0500, Xavier Douville wrote:
> Hi
> 
> On my board I needed to pass a custom VID header offset to ubiattach.
> This patch adds the required option to the barebox command.

I don't know exactly, but I suspect something is wrong on your board
then. Normally the VID header offset should be detected correctly
automatically. Does the kernel also need a special VID header offset?
Have you flashed an UBI image or did you generate the UBI on the target
directly?

> 
> I also fixed a bug in the error handling of the return value of
> ubi_attach_mtd_dev().
> This function doesn't always return 0 on success, it returns the UBI number.
> So without this patch, an error message is printed when attaching
> many UBI devices.
> 
> thanks for your work
> 
> Xavier Douville
> 
> 
>  commands/ubi.c |   25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/commands/ubi.c b/commands/ubi.c
> index 1653eaa..a10ea1c 100644
> --- a/commands/ubi.c
> +++ b/commands/ubi.c
> @@ -60,9 +60,14 @@ static int do_ubiattach(int argc, char *argv[])
>  {
>      struct mtd_info_user user;
>      int fd, ret;
> +    int vid_hdr_offset = 0;
> 
> -    if (argc != 2)
> -        return COMMAND_ERROR_USAGE;
> +    if (argc != 2) {
> +        if (argc == 3)
> +            vid_hdr_offset = simple_strtol(argv[2], NULL, 0);
> +        else
> +            return COMMAND_ERROR_USAGE;
> +    }

Please use getopt() for additional arguments.

> 
>      fd = open(argv[1], O_RDWR);
>      if (fd < 0) {
> @@ -71,19 +76,23 @@ static int do_ubiattach(int argc, char *argv[])
>      }
> 
>      ret = ioctl(fd, MEMGETINFO, &user);
> -    if (!ret)
> -        ret = ubi_attach_mtd_dev(user.mtd, UBI_DEV_NUM_AUTO, 0);
> +    if (!ret) {
> +        ret = ubi_attach_mtd_dev(user.mtd, UBI_DEV_NUM_AUTO,
> vid_hdr_offset);

You mailer wraps lines. I won't be able to apply this patch.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] commands/ubi.c: Add support for VID header offset in ubiattach

[Xavier Douville]

Hi

Sascha Hauer wrote:
> I don't know exactly, but I suspect something is wrong on your board then. Normally the VID header offset should be detected correctly automatically. Does the kernel also need a special VID header offset? Have you flashed an UBI image or did you generate the UBI on the target directly? 

I also need to specify VID header offset 2048 to the kernel. It came configured as such from the vendor (phytec, board am335x).
I flashed an UBI image generated by ptxdist.

If the kernel has this option, shouldn't barebox have it too?

If you are still interested by the patch, I will format it properly, using getopt() and without line wraps.

thanks

Xavier Douville

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] commands/ubi.c: Add support for VID header offset in ubiattach

[Sascha Hauer]

On Mon, Feb 11, 2013 at 10:14:50AM -0500, Xavier Douville wrote:
> Hi
> 
> Sascha Hauer wrote:
> >I don't know exactly, but I suspect something is wrong on your
> >board then. Normally the VID header offset should be detected
> >correctly automatically. Does the kernel also need a special VID
> >header offset? Have you flashed an UBI image or did you generate
> >the UBI on the target directly?
> 
> I also need to specify VID header offset 2048 to the kernel. It came configured as such from the vendor (phytec, board am335x).
> I flashed an UBI image generated by ptxdist.
> 
> If the kernel has this option, shouldn't barebox have it too?
> 
> If you are still interested by the patch, I will format it properly, using getopt() and without line wraps.

I'm interested in this patch once it's clear that this option is needed.
Right now I think that the work is better invested in making this option
unneeded. Jan, do you have insights why this is needed?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH] Do not print error message when successfully attaching more than one UBI device

[Xavier Douville]

Hi

This patch fix a bug where an error message is printed when 
attaching more than one UBI device. ubi_attach_mtd_dev() returns
the UBI device number (>=0) on success.

Signed-off-by: Xavier Douville <barebox@douville.org>
---
 commands/ubi.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/commands/ubi.c b/commands/ubi.c
index 1653eaa..8dc95ee 100644
--- a/commands/ubi.c
+++ b/commands/ubi.c
@@ -71,15 +71,19 @@ static int do_ubiattach(int argc, char *argv[])
 	}
 
 	ret = ioctl(fd, MEMGETINFO, &user);
-	if (!ret)
+	if (!ret) {
 		ret = ubi_attach_mtd_dev(user.mtd, UBI_DEV_NUM_AUTO, 0);
+		if (ret >= 0) {
+			close(fd);
+			return 0;
+		}
+	}
 
-	if (ret)
-		printf("failed to attach: %s\n", strerror(-ret));
+	printf("failed to attach: %s\n", strerror(ret));
 
 	close(fd);
 
-	return ret ? 1 : 0;
+	return 1;
 }
 
 static const __maybe_unused char cmd_ubiattach_help[] =
-- 
1.7.9.5

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] commands/ubi.c: Add support for VID header offset in ubiattach

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 953 bytes --]

On 02/11/2013 04:14 PM, Xavier Douville wrote:
> Hi
> 
> Sascha Hauer wrote:
>> I don't know exactly, but I suspect something is wrong on your board
>> then. Normally the VID header offset should be detected correctly
>> automatically. Does the kernel also need a special VID header offset?
>> Have you flashed an UBI image or did you generate the UBI on the
>> target directly? 
> 
> I also need to specify VID header offset 2048 to the kernel. It came
> configured as such from the vendor (phytec, board am335x).
> I flashed an UBI image generated by ptxdist.

Why don't you configure ptxdist to generate an image with the correct
VID header offset?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] commands/ubi.c: Add support for VID header offset in ubiattach

[Jean-Christophe PLAGNIOL-VILLARD]

On 23:49 Tue 12 Feb     , Marc Kleine-Budde wrote:
> On 02/11/2013 04:14 PM, Xavier Douville wrote:
> > Hi
> > 
> > Sascha Hauer wrote:
> >> I don't know exactly, but I suspect something is wrong on your board
> >> then. Normally the VID header offset should be detected correctly
> >> automatically. Does the kernel also need a special VID header offset?
> >> Have you flashed an UBI image or did you generate the UBI on the
> >> target directly? 
> > 
> > I also need to specify VID header offset 2048 to the kernel. It came
> > configured as such from the vendor (phytec, board am335x).
> > I flashed an UBI image generated by ptxdist.
> 
> Why don't you configure ptxdist to generate an image with the correct
> VID header offset?
Some time you want to flash the same UBI on different NAND so this is usefull

Best Regards,
J.
> 
> Marc
> 
> -- 
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
> 



> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] commands/ubi.c: Add support for VID header offset in ubiattach

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1216 bytes --]

On 02/12/2013 11:52 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 23:49 Tue 12 Feb     , Marc Kleine-Budde wrote:
>> On 02/11/2013 04:14 PM, Xavier Douville wrote:
>>> Hi
>>>
>>> Sascha Hauer wrote:
>>>> I don't know exactly, but I suspect something is wrong on your board
>>>> then. Normally the VID header offset should be detected correctly
>>>> automatically. Does the kernel also need a special VID header offset?
>>>> Have you flashed an UBI image or did you generate the UBI on the
>>>> target directly? 
>>>
>>> I also need to specify VID header offset 2048 to the kernel. It came
>>> configured as such from the vendor (phytec, board am335x).
>>> I flashed an UBI image generated by ptxdist.
>>
>> Why don't you configure ptxdist to generate an image with the correct
>> VID header offset?
> Some time you want to flash the same UBI on different NAND so this is usefull

...but a bit scary, too :)

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] commands/ubi.c: Add support for VID header offset in ubiattach

[Jan Lübbe]

On Mon, 2013-02-11 at 16:21 +0100, Sascha Hauer wrote:
> I'm interested in this patch once it's clear that this option is needed.
> Right now I think that the work is better invested in making this option
> unneeded. Jan, do you have insights why this is needed?

Linux with current mainline + pending GPMC/ELM patches thinks it
supports sub-page reads/writes on 2k page NAND with BCH8. This is simply
wrong. By using "root=ubi0:root ubi.mtd=nand0.root,2048
rootfstype=ubifs" in the kernel cmdline, you will get this override in
the kernel.

The GMPC NAND driver in barebox correctly does not use sub-page-access
and works fine without overriding the VID header offset.

When building the UBIFS/UBI the offset must obviously be set correctly.

Which kernel & barebox are you using?

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Re: [PATCH 1/1] commands/ubi.c: Add support for VID header offset in ubiattach

[Xavier Douville]

On 02/14/2013 11:23, Jan Lübbe wrote:
>
> Which kernel & barebox are you using?
>

I am using kernel 3.2.0 and barebox 2012.11.0.
Both with patches from Phytec (phyCORE-AM335x-PD12.1.1).


Xavier

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Re: [PATCH 1/1] commands/ubi.c: Add support for VID header offset in ubiattach

[Jan Lübbe]

On Fri, 2013-02-15 at 10:54 -0500, Xavier Douville wrote:
> On 02/14/2013 11:23, Jan Lübbe wrote:
> >
> > Which kernel & barebox are you using?
> >
> 
> I am using kernel 3.2.0 and barebox 2012.11.0.
> Both with patches from Phytec (phyCORE-AM335x-PD12.1.1).

I'm not sure if that barebox is different to the latest one regarding
the GPMC NAND support...

Does it actually fail on your board without this patch?

I have here:

...
NAND device: Manufacturer ID: 0x2c, Chip ID: 0xda (Micron NAND 256MiB 3,3V 8-bit), page size: 2048, OOB size: 64
...
barebox:/ devinfo nand0
resources:
driver: none
bus: none

Parameters:
            size = 268435456
       erasesize = 131072
       writesize = 2048
         oobsize = 64 
barebox:/ ubiattach /dev/nand0.root
UBI: attaching mtd0 to ubi0
UBI: physical eraseblock size:   131072 bytes (128 KiB)
UBI: logical eraseblock size:    126976 bytes
UBI: smallest flash I/O unit:    2048
UBI: VID header offset:          2048 (aligned 2048)
UBI: data offset:                4096
registering /dev/ubi0
registering root as /dev/ubi0.root
UBI: attached mtd0 to ubi0
UBI: MTD device name:            "nand0.root"
UBI: MTD device size:            96 MiB
UBI: number of good PEBs:        768
UBI: number of bad PEBs:         0
UBI: max. allowed volumes:       128
UBI: wear-leveling threshold:    4096
UBI: number of internal volumes: 1
UBI: number of user volumes:     1
UBI: available PEBs:             33
UBI: total number of reserved PEBs: 735
UBI: number of PEBs reserved for bad PEB handling: 7
UBI: max/mean erase counter: 15/8
barebox:/ 

So it figures out the VID offset correctly from the writesize of 2048.

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/1] commands/ubi.c: Add support for VID header offset in ubiattach

[Xavier Douville]

On 2013-02-15 11:06, Jan Lübbe wrote:
> 
> I'm not sure if that barebox is different to the latest one regarding
> the GPMC NAND support...
> 
> Does it actually fail on your board without this patch?

I tried barebox 2013.02.0 but it doesn't detect my NAND, so I guess the
patch is still required.

> 
> So it figures out the VID offset correctly from the writesize of 2048.
> 
> Regards,
> Jan
> 

What board and barebox version did you run this on?
I guess that means I should try to fix the problem in my NAND driver?

thanks for your help

Xavier

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox