| Message ID | 1322498261-20645-12-git-send-email-yanok@emcraft.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Scott Wood |
| Headers | show |
On 11/28/2011 10:37 AM, Ilya Yanok wrote: > Currently nand_spl_simple puts it's temp data at 0x10000 offset in SDRAM > which is likely to contain already loaded data. I can't see any way to > determine some safe address automagically so make it up to board porter > to provide the safe-to-use address via CONFIG_SPL_NAND_WORKSPACE value. > > Signed-off-by: Ilya Yanok <yanok@emcraft.com> > --- > drivers/mtd/nand/nand_spl_simple.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c > index ed821f2..70f3cfe 100644 > --- a/drivers/mtd/nand/nand_spl_simple.c > +++ b/drivers/mtd/nand/nand_spl_simple.c > @@ -199,8 +199,13 @@ static int nand_read_page(int block, int page, void *dst) > > /* No malloc available for now, just use some temporary locations > * in SDRAM > + * Please provide some safe value for CONFIG_SPL_NAND_WORKSPACE in > + * your board configuration, this is just a guess!! > */ > - ecc_calc = (u_char *)(CONFIG_SYS_SDRAM_BASE + 0x10000); > +#ifndef CONFIG_SPL_NAND_WORKSPACE > +#define CONFIG_SPL_NAND_WORKSPACE (CONFIG_SYS_SDRAM_BASE + 0x10000) > +#endif > + ecc_calc = (u_char *)CONFIG_SPL_NAND_WORKSPACE; > ecc_code = ecc_calc + 0x100; > oob_data = ecc_calc + 0x200; > Please document this config symbol in README. -Scott
Hi Ilya, On 11/28/11 18:37, Ilya Yanok wrote: > Currently nand_spl_simple puts it's temp data at 0x10000 offset in SDRAM > which is likely to contain already loaded data. I can't see any way to > determine some safe address automagically so make it up to board porter > to provide the safe-to-use address via CONFIG_SPL_NAND_WORKSPACE value. > > Signed-off-by: Ilya Yanok <yanok@emcraft.com> > --- > drivers/mtd/nand/nand_spl_simple.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c > index ed821f2..70f3cfe 100644 > --- a/drivers/mtd/nand/nand_spl_simple.c > +++ b/drivers/mtd/nand/nand_spl_simple.c > @@ -199,8 +199,13 @@ static int nand_read_page(int block, int page, void *dst) > > /* No malloc available for now, just use some temporary locations > * in SDRAM > + * Please provide some safe value for CONFIG_SPL_NAND_WORKSPACE in > + * your board configuration, this is just a guess!! > */ > - ecc_calc = (u_char *)(CONFIG_SYS_SDRAM_BASE + 0x10000); > +#ifndef CONFIG_SPL_NAND_WORKSPACE > +#define CONFIG_SPL_NAND_WORKSPACE (CONFIG_SYS_SDRAM_BASE + 0x10000) > +#endif > + ecc_calc = (u_char *)CONFIG_SPL_NAND_WORKSPACE; > ecc_code = ecc_calc + 0x100; > oob_data = ecc_calc + 0x200; If you change this, you probably should change also the second version of this function (CONFIG_SYS_NAND_HW_ECC_OOBFIRST) or instead pass it as a parameter to the finction(s) from an upper layer. Have you tried to use the stack as a buffer for those calculations?
On 28/11/2011 17:37, Ilya Yanok wrote: > Currently nand_spl_simple puts it's temp data at 0x10000 offset in SDRAM > which is likely to contain already loaded data. I can't see any way to > determine some safe address automagically so make it up to board porter > to provide the safe-to-use address via CONFIG_SPL_NAND_WORKSPACE value. > > Signed-off-by: Ilya Yanok <yanok@emcraft.com> > --- > drivers/mtd/nand/nand_spl_simple.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c > index ed821f2..70f3cfe 100644 Hi Ilya, > --- a/drivers/mtd/nand/nand_spl_simple.c > +++ b/drivers/mtd/nand/nand_spl_simple.c > @@ -199,8 +199,13 @@ static int nand_read_page(int block, int page, void *dst) > > /* No malloc available for now, just use some temporary locations > * in SDRAM > + * Please provide some safe value for CONFIG_SPL_NAND_WORKSPACE in > + * your board configuration, this is just a guess!! > */ > - ecc_calc = (u_char *)(CONFIG_SYS_SDRAM_BASE + 0x10000); > +#ifndef CONFIG_SPL_NAND_WORKSPACE > +#define CONFIG_SPL_NAND_WORKSPACE (CONFIG_SYS_SDRAM_BASE + 0x10000) > +#endif Maybe it is better to not have a default value somewhere in the SDRAM and to get a compile error. If we do not want to fix also the related boards, we could at least use a #warn message to advise at compile time that a default value is taken (and at the end, to force the board maintainers to fix it...). Best regards, Stefano Babic
Hi Stefano, On 07.12.2011 13:06, Stefano Babic wrote: >> +#ifndef CONFIG_SPL_NAND_WORKSPACE >> +#define CONFIG_SPL_NAND_WORKSPACE (CONFIG_SYS_SDRAM_BASE + 0x10000) >> +#endif > > Maybe it is better to not have a default value somewhere in the SDRAM > and to get a compile error. If we do not want to fix also the related My intention was not to break something as I can't fix every board using this driver. That's why I preserved the existing address for the case where CONFIG_SPL_NAND_WORKSPACE is not defined. > boards, we could at least use a #warn message to advise at compile time > that a default value is taken (and at the end, to force the board > maintainers to fix it...). Probably. Wolfgang, Scott, do you think we should add a warning here? Regards, Ilya.
On 12/07/2011 11:46 AM, Ilya Yanok wrote: > Hi Stefano, > > On 07.12.2011 13:06, Stefano Babic wrote: >>> +#ifndef CONFIG_SPL_NAND_WORKSPACE >>> +#define CONFIG_SPL_NAND_WORKSPACE (CONFIG_SYS_SDRAM_BASE + 0x10000) >>> +#endif >> >> Maybe it is better to not have a default value somewhere in the SDRAM >> and to get a compile error. If we do not want to fix also the related > > My intention was not to break something as I can't fix every board using > this driver. That's why I preserved the existing address for the case > where CONFIG_SPL_NAND_WORKSPACE is not defined. How many boards are using the new SPL for NAND so far? It shouldn't be that bad to just fix them. -Scott
Dear Ilya Yanok, In message <4EDFA65E.8080809@emcraft.com> you wrote: > > > boards, we could at least use a #warn message to advise at compile time > > that a default value is taken (and at the end, to force the board > > maintainers to fix it...). > > Probably. Wolfgang, Scott, do you think we should add a warning here? How much space are we talking about? Can it be put on the stack? Best regards, Wolfgang Denk
Dear Wolfgang, On 07.12.2011 22:45, Wolfgang Denk wrote: >>> boards, we could at least use a #warn message to advise at compile time >>> that a default value is taken (and at the end, to force the board >>> maintainers to fix it...). >> >> Probably. Wolfgang, Scott, do you think we should add a warning here? > > How much space are we talking about? Can it be put on the stack? Well, currently the code uses 0x200 + CONFIG_SYS_NAND_OOBSIZE bytes but I think a lot of this space is unused. What we really need is CONFIG_SYS_NAND_ECCTOTAL + CONFIG_SYS_NAND_OOBSIZE. This is probably acceptable to put on the stack. Regards, Ilya.
On 07/12/2011 19:45, Wolfgang Denk wrote: > Dear Ilya Yanok, > > In message <4EDFA65E.8080809@emcraft.com> you wrote: >> >>> boards, we could at least use a #warn message to advise at compile time >>> that a default value is taken (and at the end, to force the board >>> maintainers to fix it...). >> >> Probably. Wolfgang, Scott, do you think we should add a warning here? > > How much space are we talking about? Can it be put on the stack? > I think we are talking about 24 bytes, getting the value for CONFIG_SYS_NAND_ECCTOTAL in the configuration file. Reading the code it is not clear to me why the computed ecc is stored in this way at a fixed offset in RAM - anybody knows why cannot we set a static char ecc_calc[] at the beginning of the nand_spl_simple.c file ? Stefano
diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c index ed821f2..70f3cfe 100644 --- a/drivers/mtd/nand/nand_spl_simple.c +++ b/drivers/mtd/nand/nand_spl_simple.c @@ -199,8 +199,13 @@ static int nand_read_page(int block, int page, void *dst) /* No malloc available for now, just use some temporary locations * in SDRAM + * Please provide some safe value for CONFIG_SPL_NAND_WORKSPACE in + * your board configuration, this is just a guess!! */ - ecc_calc = (u_char *)(CONFIG_SYS_SDRAM_BASE + 0x10000); +#ifndef CONFIG_SPL_NAND_WORKSPACE +#define CONFIG_SPL_NAND_WORKSPACE (CONFIG_SYS_SDRAM_BASE + 0x10000) +#endif + ecc_calc = (u_char *)CONFIG_SPL_NAND_WORKSPACE; ecc_code = ecc_calc + 0x100; oob_data = ecc_calc + 0x200;
Currently nand_spl_simple puts it's temp data at 0x10000 offset in SDRAM which is likely to contain already loaded data. I can't see any way to determine some safe address automagically so make it up to board porter to provide the safe-to-use address via CONFIG_SPL_NAND_WORKSPACE value. Signed-off-by: Ilya Yanok <yanok@emcraft.com> --- drivers/mtd/nand/nand_spl_simple.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)