Message ID | 1320078187-28423-6-git-send-email-simonschwarzcor@gmail.com |
---|---|
State | Superseded |
Delegated to: | Tom Rini |
Headers | show |
On 31/10/2011 17:23, Simon Schwarz wrote: > From: Simon Schwarz <simonschwarzcor@googlemail.com> > > spl_nand overwrote BSS section because it reads a whole block everytime. Now > loads the block to spare area and just copy the needed junk to destination. > Whole block read is necessary for ecc check! > > Signed-off-by: Simon Schwarz <simonschwarzcor@gmail.com> > --- > @@ -71,7 +71,8 @@ void spl_nand_load_image(void) > CONFIG_SYS_NAND_PAGE_SIZE, (void *)header); > spl_parse_image_header(header); > nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS, > - spl_image.size, (void *)spl_image.load_addr); > + spl_image.size, > + (void *)spl_image.load_addr - sizeof(header)); ^--- Do you mean maybe sizeof(*header) ? However, spl_image.load_addr was already decremented in spl_parse_image_header() and correctly set to 64 byte before the load address. Do we really need it ? I found the readon of the kernel corrupt image. We are setting a very hard address in /nand_spl_simple.c: ecc_calc = (u_char *)(CONFIG_SYS_SDRAM_BASE + 0x10000); Because the image for a TI SOC is loaded at 0x80008000, we have a conflict and the image is corrupted where the ECC is computed. It is not a really good idea to fix in this way where to compute the ECC. Should be not better to put it in the CONFIG_SYS_INIT_RAM_ADDR area ? Best regards, Stefano Babic
On 12/06/2011 06:18 PM, Stefano Babic wrote: > On 31/10/2011 17:23, Simon Schwarz wrote: >> From: Simon Schwarz<simonschwarzcor@googlemail.com> >> >> spl_nand overwrote BSS section because it reads a whole block everytime. Now >> loads the block to spare area and just copy the needed junk to destination. >> Whole block read is necessary for ecc check! >> >> Signed-off-by: Simon Schwarz<simonschwarzcor@gmail.com> >> --- > >> @@ -71,7 +71,8 @@ void spl_nand_load_image(void) >> CONFIG_SYS_NAND_PAGE_SIZE, (void *)header); >> spl_parse_image_header(header); >> nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS, >> - spl_image.size, (void *)spl_image.load_addr); >> + spl_image.size, >> + (void *)spl_image.load_addr - sizeof(header)); > ^--- > > > Do you mean maybe sizeof(*header) ? > > However, spl_image.load_addr was already decremented in > spl_parse_image_header() and correctly set to 64 byte before the load > address. Do we really need it ? > > I found the readon of the kernel corrupt image. We are setting a very > hard address in /nand_spl_simple.c: > > ecc_calc = (u_char *)(CONFIG_SYS_SDRAM_BASE + 0x10000); > > Because the image for a TI SOC is loaded at 0x80008000, we have a > conflict and the image is corrupted where the ECC is computed. > > It is not a really good idea to fix in this way where to compute the > ECC. Should be not better to put it in the CONFIG_SYS_INIT_RAM_ADDR area ? > > Best regards, > Stefano Babic > Hmm. This is from the former nand_spl. Why not use malloc for this? Thanks to changes in the FAT driver we now have it in the SPL. I don't see a reason to have this in SRAM when SDRAM is available. Regards Simon
On 12/06/2011 06:18 PM, Stefano Babic wrote: > On 31/10/2011 17:23, Simon Schwarz wrote: >> From: Simon Schwarz<simonschwarzcor@googlemail.com> >> >> spl_nand overwrote BSS section because it reads a whole block everytime. Now >> loads the block to spare area and just copy the needed junk to destination. >> Whole block read is necessary for ecc check! >> >> Signed-off-by: Simon Schwarz<simonschwarzcor@gmail.com> >> --- > >> @@ -71,7 +71,8 @@ void spl_nand_load_image(void) >> CONFIG_SYS_NAND_PAGE_SIZE, (void *)header); >> spl_parse_image_header(header); >> nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS, >> - spl_image.size, (void *)spl_image.load_addr); >> + spl_image.size, >> + (void *)spl_image.load_addr - sizeof(header)); > ^--- > > > Do you mean maybe sizeof(*header) ? > > However, spl_image.load_addr was already decremented in > spl_parse_image_header() and correctly set to 64 byte before the load > address. Do we really need it ? > [SNIP] > Best regards, > Stefano Babic > You are right -> V9. Lets see if we get it to two digits ;) Regards Simon
On 06/12/2011 18:53, Simon Schwarz wrote: > Hmm. This is from the former nand_spl. Yes, I know. Until the load address does not conflict with the fix offset, we have never seen this problem. > Why not use malloc for this? Of course, malloc - I want only to get rid of such hardcoded values ;-) > Thanks to changes in the FAT driver we now > have it in the SPL. > > I don't see a reason to have this in SRAM when SDRAM is available. No reason at all, of course. Stefano
Dear Simon Schwarz, In message <4EDE56AB.3070504@gmail.com> you wrote: > > Why not use malloc for this? Thanks to changes in the FAT driver we now > have it in the SPL. No, please do NOT do that!!! I knew I should have never accepted to allow this in the SPL fromt he beginning, and indeed it was a mistake not to block this. Please now don't use it without thinking if it's really, really needed! I recommend NOT to use malloc() at all in SPL code. [Guess I should have a more carefull eye on all SPL related submissions and install a "grep-malloc-and-NAK" script. :-( ] Best regards, Wolfgang Denk
On 12/06/2011 11:53 AM, Simon Schwarz wrote: > On 12/06/2011 06:18 PM, Stefano Babic wrote: >> I found the readon of the kernel corrupt image. We are setting a very >> hard address in /nand_spl_simple.c: >> >> ecc_calc = (u_char *)(CONFIG_SYS_SDRAM_BASE + 0x10000); >> >> Because the image for a TI SOC is loaded at 0x80008000, we have a >> conflict and the image is corrupted where the ECC is computed. >> >> It is not a really good idea to fix in this way where to compute the >> ECC. Should be not better to put it in the CONFIG_SYS_INIT_RAM_ADDR >> area ? There's a patch to fix this (with some minor changes requested): http://patchwork.ozlabs.org/patch/128018/ >> Best regards, >> Stefano Babic >> > > Hmm. This is from the former nand_spl. > > Why not use malloc for this? Thanks to changes in the FAT driver we now > have it in the SPL. > > I don't see a reason to have this in SRAM when SDRAM is available. nand_spl_simple.c is used by some very small SPL targets -- 4K or so. malloc doesn't fit. -Scott
diff --git a/arch/arm/cpu/armv7/omap-common/spl_nand.c b/arch/arm/cpu/armv7/omap-common/spl_nand.c index 1b9e171..903255b 100644 --- a/arch/arm/cpu/armv7/omap-common/spl_nand.c +++ b/arch/arm/cpu/armv7/omap-common/spl_nand.c @@ -71,7 +71,8 @@ void spl_nand_load_image(void) CONFIG_SYS_NAND_PAGE_SIZE, (void *)header); spl_parse_image_header(header); nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS, - spl_image.size, (void *)spl_image.load_addr); + spl_image.size, + (void *)spl_image.load_addr - sizeof(header)); } else #endif {