| Message ID | 1354066303-29762-2-git-send-email-troy.kisky@boundarydevices.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
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
>-----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 >
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
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
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
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
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
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
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
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
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
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)
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(-)