Patchwork [U-Boot,V4,01/11] imximage: mx53 needs transfer length a multiple of 512

login
register
mail settings
Submitter Troy Kisky
Date Nov. 28, 2012, 1:31 a.m.
Message ID <1354066303-29762-2-git-send-email-troy.kisky@boundarydevices.com>
Download mbox | patch
Permalink /patch/202338/
State Changes Requested
Delegated to: Stefano Babic
Headers show

Comments

Troy Kisky - Nov. 28, 2012, 1:31 a.m.
The mx53 ROM will truncate the length at a multiple of 512.
Transferring too much is not a problem, so round up.

Problem reported by Stefano Babic.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
 tools/imximage.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
Wolfgang Denk - Nov. 28, 2012, 9:27 a.m.
Dear Troy Kisky,

In message <1354066303-29762-2-git-send-email-troy.kisky@boundarydevices.com> you wrote:
> The mx53 ROM will truncate the length at a multiple of 512.
> Transferring too much is not a problem, so round up.

What about other SoCs using the same code?

> +#define ALIGN(a, b)	(((a) + (b) - 1) & ~((b) - 1))

NAK. This macro is mis-named; it has nothing to do with alignment -
you write yourself: "round up".


And you don't have to re-invent the wheel.  Please use the existing
macros for this purpose.

Best regards,

Wolfgang Denk
Liu Hui-R64343 - Nov. 28, 2012, 10:34 a.m.
>-----Original Message-----
>From: Troy Kisky [mailto:troy.kisky@boundarydevices.com]
>Sent: Wednesday, November 28, 2012 9:32 AM
>To: sbabic@denx.de
>Cc: dirk.behme@googlemail.com; u-boot@lists.denx.de; Liu Hui-R64343;
>festevam@gmail.com; Troy Kisky
>Subject: [PATCH V4 01/11] imximage: mx53 needs transfer length a multiple
>of 512
>
>The mx53 ROM will truncate the length at a multiple of 512.
>Transferring too much is not a problem, so round up.
>
>Problem reported by Stefano Babic.
>
>Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>

Acked-by: Jason Liu <r64343@freescale.com>

>---
> tools/imximage.c |   10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
>diff --git a/tools/imximage.c b/tools/imximage.c index 63f88b6..7e54e97
>100644
>--- a/tools/imximage.c
>+++ b/tools/imximage.c
>@@ -494,6 +494,8 @@ static void imximage_print_header(const void *ptr)
> 	}
> }
>
>+#define ALIGN(a, b)	(((a) + (b) - 1) & ~((b) - 1))
>+
> static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
> 				struct mkimage_params *params)
> {
>@@ -515,7 +517,13 @@ static void imximage_set_header(void *ptr, struct
>stat *sbuf, int ifd,
>
> 	/* Set the imx header */
> 	(*set_imx_hdr)(imxhdr, dcd_len, params->ep, imxhdr->flash_offset);
>-	*header_size_ptr = sbuf->st_size + imxhdr->flash_offset;
>+	/*
>+	 * ROM bug alert
>+	 * mx53 only loads 512 byte multiples.
>+	 * The remaining fraction of a block bytes would
>+	 * not be loaded.
>+	 */
>+	*header_size_ptr = ALIGN(sbuf->st_size + imxhdr->flash_offset, 512);
> }
>
> int imximage_check_params(struct mkimage_params *params)
>--
>1.7.9.5
>
Troy Kisky - Nov. 28, 2012, 6:18 p.m.
On 11/28/2012 2:27 AM, Wolfgang Denk wrote:
> Dear Troy Kisky,
>
> In message <1354066303-29762-2-git-send-email-troy.kisky@boundarydevices.com> you wrote:
>> The mx53 ROM will truncate the length at a multiple of 512.
>> Transferring too much is not a problem, so round up.
> What about other SoCs using the same code?
>
>> +#define ALIGN(a, b)	(((a) + (b) - 1) & ~((b) - 1))
> NAK. This macro is mis-named; it has nothing to do with alignment -
> you write yourself: "round up".
>
>
> And you don't have to re-invent the wheel.  Please use the existing
> macros for this purpose.
>
> Best regards,
>
> Wolfgang Denk
>

Oddly enough, I originally called it ROUND_UP. But then I saw these lines
in include/common.h

#define ALIGN(x,a)              __ALIGN_MASK((x),(typeof(x))(a)-1)
#define __ALIGN_MASK(x,mask)    (((x)+(mask))&~(mask))

So, I deleted my definition of ROUND_UP and used ALIGN. But imximage.c
did not automatically include common.h. Instead of trying to include
common.h and all the files it pulled in, I added the ALIGN definition.

Troy
Troy Kisky - Nov. 28, 2012, 6:26 p.m.
On 11/28/2012 2:27 AM, Wolfgang Denk wrote:
> Dear Troy Kisky,
>
> In message <1354066303-29762-2-git-send-email-troy.kisky@boundarydevices.com> you wrote:
>> The mx53 ROM will truncate the length at a multiple of 512.
>> Transferring too much is not a problem, so round up.
> What about other SoCs using the same code?
>
>
It would be easy to add a version 2 header test, but that would not
distinguish between mx53 and mx6. Should I create a version 2bug
header that I can specify in mx53's config file?


Thanks
Troy
Wolfgang Denk - Nov. 28, 2012, 8:25 p.m.
Dear Troy Kisky,

In message <50B65583.1070309@boundarydevices.com> you wrote:
>
> Oddly enough, I originally called it ROUND_UP. But then I saw these lines
> in include/common.h

And why didn't you find (and use) ROUND() in include/common.h ?

Best regards,

Wolfgang Denk
Troy Kisky - Nov. 28, 2012, 9:05 p.m.
On 11/28/2012 1:25 PM, Wolfgang Denk wrote:
> Dear Troy Kisky,
>
> In message <50B65583.1070309@boundarydevices.com> you wrote:
>> Oddly enough, I originally called it ROUND_UP. But then I saw these lines
>> in include/common.h
> And why didn't you find (and use) ROUND() in include/common.h ?
>
> Best regards,
>
> Wolfgang Denk
>
I did also find ROUND, so I checked to see what Linux did. Linux does not
have ROUND, but it does have ALIGN.

But I personally prefer ROUND, or even better ROUND_UP. I just wanted
to use the most common form. u-boot seems to use ROUND in config files
and ALIGN in .c files

But the reason I didn't include common.h is because of the target specific
files that it also includes. Would you mind if I moved
Wolfgang Denk - Nov. 28, 2012, 9:35 p.m.
Dear Troy Kisky,

In message <50B67C99.8080609@boundarydevices.com> you wrote:
>
> But the reason I didn't include common.h is because of the target specific
> files that it also includes. Would you mind if I moved

Why would these hurt?  They don't anywhere else.

> and included common_macro.h instead?

I see no benefit for that.

Best regards,

Wolfgang Denk
Troy Kisky - Nov. 29, 2012, 2:42 a.m.
On 11/28/2012 2:35 PM, Wolfgang Denk wrote:
> Dear Troy Kisky,
>
> In message <50B67C99.8080609@boundarydevices.com> you wrote:
>> But the reason I didn't include common.h is because of the target specific
>> files that it also includes. Would you mind if I moved
> Why would these hurt?  They don't anywhere else.
>

I'm not saying that including common.h wouldn't work.
I'm saying that it seems wrong to include target specific include
files in an executable that should generate the same code regardless
of the target selected.

I really don't care enough to argue. I just want you to understand why
I did it the way I did. It wasn't because I was crazy, or lazy. We just
hold different priorities.

Would you like to see the Linux way of ALIGN, or ROUND?

Now, back to the other topic you raised. Should I apply the bug work-around
for all version 2 headers, or find a way to distinguish mx53/mx6?


Thanks
Troy
Wolfgang Denk - Nov. 29, 2012, 5:28 a.m.
Dear Troy Kisky,

In message <50B6CB79.4030007@boundarydevices.com> you wrote:
>
> Would you like to see the Linux way of ALIGN, or ROUND?

Do you align some buffer or similar, or do you round (up) a size?

> Now, back to the other topic you raised. Should I apply the bug work-around
> for all version 2 headers, or find a way to distinguish mx53/mx6?

I cannot tell.  Stefano, what do you think?

Best regards,

Wolfgang Denk
Stefano Babic - Dec. 3, 2012, 9:12 a.m.
On 28/11/2012 22:35, Wolfgang Denk wrote:
> Dear Troy Kisky,
> 
> In message <50B67C99.8080609@boundarydevices.com> you wrote:
>>
>> But the reason I didn't include common.h is because of the target specific
>> files that it also includes. Would you mind if I moved
> 
> Why would these hurt?  They don't anywhere else.

Personally, I think that mkimage as generic tool should not include
common.h. Doing that, it does not allow to compile mkimage without
running config, and let's think that we need a different mkimage for
each target, and that is not true. This will break also support from
distros, because their packages (for example, u-boot-tools,
uboot-mkimage under Ubuntu) are compiled without configuring u-boot -
and I think it is correct.

IMHO we are discussing about a single macro. We can let it in mkimage as
in patch and move it in a general file only if we will have a use case
with a bunch of macros.

Best regards,
Stefano Babic
Stefano Babic - Dec. 3, 2012, 9:23 a.m.
On 29/11/2012 06:28, Wolfgang Denk wrote:
> Dear Troy Kisky,
> 
> In message <50B6CB79.4030007@boundarydevices.com> you wrote:
>>
>> Would you like to see the Linux way of ALIGN, or ROUND?
> 
> Do you align some buffer or similar, or do you round (up) a size?
> 
>> Now, back to the other topic you raised. Should I apply the bug work-around
>> for all version 2 headers, or find a way to distinguish mx53/mx6?

> 
> I cannot tell.  Stefano, what do you think?

Well, I am thinking about which are the real benefits. If we always
round up the size to 512 bytes for V2 header, *maybe* we constrain the
i.MX6 to load some bytes more, but it is the only drawback. And this if
the i.MX6 does not suffer of the same problem found on i.MX53.

On the other side, having two different versions of V2 header is
confusing. It is then undocumented by Freescale, and maybe it is
possible to find a note in some errata. Having the same interface
without special hacking for each SOC overcomes the increment in the
footprint for the i.MX6 (in worst case 511 bytes - and not a lot
compared to current size of U-Boot for MX5/MX6, usually several hundred
of KB).

Best regards,
Stefano Babic

Patch

diff --git a/tools/imximage.c b/tools/imximage.c
index 63f88b6..7e54e97 100644
--- a/tools/imximage.c
+++ b/tools/imximage.c
@@ -494,6 +494,8 @@  static void imximage_print_header(const void *ptr)
 	}
 }
 
+#define ALIGN(a, b)	(((a) + (b) - 1) & ~((b) - 1))
+
 static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
 				struct mkimage_params *params)
 {
@@ -515,7 +517,13 @@  static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
 
 	/* Set the imx header */
 	(*set_imx_hdr)(imxhdr, dcd_len, params->ep, imxhdr->flash_offset);
-	*header_size_ptr = sbuf->st_size + imxhdr->flash_offset;
+	/*
+	 * ROM bug alert
+	 * mx53 only loads 512 byte multiples.
+	 * The remaining fraction of a block bytes would
+	 * not be loaded.
+	 */
+	*header_size_ptr = ALIGN(sbuf->st_size + imxhdr->flash_offset, 512);
 }
 
 int imximage_check_params(struct mkimage_params *params)