| Message ID | 1379347536-21219-1-git-send-email-prabhakar@freescale.com |
|---|---|
| State | RFC |
| Delegated to: | York Sun |
| Headers | show |
On Mon, 2013-09-16 at 21:35 +0530, Prabhakar Kushwaha wrote: > fsl_ifs_spl.c reads data from NAND and store at a memory location in raw mode. > It does not used MTD layer. > To read env variable from NAND MTD layer read/write required. > > Hence, add mtd_block_isbad & nand_read_skip_bad function required during > env variable read. > > Also, avoid nand_info during env read for SPL > > Signed-off-by: Prabhakar Kushwaha <prabhakar@freescale.com> > --- > common/env_nand.c | 7 ++++--- > drivers/mtd/nand/Makefile | 2 +- > drivers/mtd/nand/fsl_ifc_spl.c | 22 ++++++++++++++++++++++ > 3 files changed, 27 insertions(+), 4 deletions(-) > > diff --git a/common/env_nand.c b/common/env_nand.c > index 7530962..7a7107f 100644 > --- a/common/env_nand.c > +++ b/common/env_nand.c > @@ -246,11 +246,13 @@ int readenv(size_t offset, u_char *buf) > u_char *char_ptr; > > blocksize = nand_info[0].erasesize; > +#ifndef CONFIG_SPL_BUILD > if (!blocksize) > return 1; > - > len = min(blocksize, CONFIG_ENV_SIZE); > - > +#else > + len = CONFIG_ENV_SIZE; > +#endif Use positive logic (ifdef/else, not ifndef/else). Are you sure that CONFIG_ENV_SIZE will always be appropriate? Shouldn't you use CONFIG_SYS_NAND_BLOCK_SIZE in place of nand_info[0].erasesize? > @@ -396,7 +398,6 @@ void env_relocate_spec(void) > return; > } > #endif > - > ret = readenv(CONFIG_ENV_OFFSET, (u_char *)buf); > if (ret) { > set_default_env("!readenv() failed"); Remove unrelated whitespace changes. > diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile > index 366dee6..06d5d14 100644 > --- a/drivers/mtd/nand/Makefile > +++ b/drivers/mtd/nand/Makefile > @@ -24,6 +24,7 @@ COBJS-$(CONFIG_SPL_NAND_LOAD) += nand_spl_load.o > COBJS-$(CONFIG_SPL_NAND_ECC) += nand_ecc.o > COBJS-$(CONFIG_SPL_NAND_BASE) += nand_base.o > COBJS-$(CONFIG_SPL_NAND_INIT) += nand.o > +COBJS-$(CONFIG_NAND_FSL_IFC) += fsl_ifc_spl.o No, it's still a minimal NAND driver (i.e. it doesn't use fsl_ifc.o). Minimal NAND drivers are not related to minimal SPL init. > diff --git a/drivers/mtd/nand/fsl_ifc_spl.c b/drivers/mtd/nand/fsl_ifc_spl.c > index d462265..e7edacf 100644 > --- a/drivers/mtd/nand/fsl_ifc_spl.c > +++ b/drivers/mtd/nand/fsl_ifc_spl.c > @@ -11,6 +11,28 @@ > #include <asm/io.h> > #include <asm/fsl_ifc.h> > #include <linux/mtd/nand.h> > +#ifndef CONFIG_SPL_INIT_MINIMAL > +#include <linux/mtd/mtd.h> > +#endif > + > +static void nand_load(unsigned int offs, int uboot_size, uchar *dst); > + > +#ifndef CONFIG_SPL_INIT_MINIMAL > +struct mtd_info nand_info[CONFIG_SYS_MAX_NAND_DEVICE]; > + > +int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs) > +{ > + return 0; > +} > + > +int nand_read_skip_bad(struct mtd_info *nand, loff_t offset, size_t *length, > + size_t *actual, loff_t lim, u_char *buffer) > +{ > + nand_load(offset, *length, buffer); > + return 0; > +} > +#endif What does this have to do with minimal init? -Scott
Thanks Scott for taking time and reviewing the RFC patch. Please find my reply in-lined. On 09/17/2013 05:23 AM, Scott Wood wrote: > On Mon, 2013-09-16 at 21:35 +0530, Prabhakar Kushwaha wrote: >> fsl_ifs_spl.c reads data from NAND and store at a memory location in raw mode. >> It does not used MTD layer. >> To read env variable from NAND MTD layer read/write required. >> >> Hence, add mtd_block_isbad & nand_read_skip_bad function required during >> env variable read. >> >> Also, avoid nand_info during env read for SPL >> >> Signed-off-by: Prabhakar Kushwaha <prabhakar@freescale.com> >> --- >> common/env_nand.c | 7 ++++--- >> drivers/mtd/nand/Makefile | 2 +- >> drivers/mtd/nand/fsl_ifc_spl.c | 22 ++++++++++++++++++++++ >> 3 files changed, 27 insertions(+), 4 deletions(-) >> >> diff --git a/common/env_nand.c b/common/env_nand.c >> index 7530962..7a7107f 100644 >> --- a/common/env_nand.c >> +++ b/common/env_nand.c >> @@ -246,11 +246,13 @@ int readenv(size_t offset, u_char *buf) >> u_char *char_ptr; >> >> blocksize = nand_info[0].erasesize; >> +#ifndef CONFIG_SPL_BUILD >> if (!blocksize) >> return 1; >> - >> len = min(blocksize, CONFIG_ENV_SIZE); >> - >> +#else >> + len = CONFIG_ENV_SIZE; >> +#endif > Use positive logic (ifdef/else, not ifndef/else). I will fix it. > Are you sure that CONFIG_ENV_SIZE will always be appropriate? Shouldn't > you use CONFIG_SYS_NAND_BLOCK_SIZE in place of nand_info[0].erasesize? I can use CONFIG_SYS_NAND_BLOCK_SIZE . but i can not use in SPL as its is defined as 128K. >> @@ -396,7 +398,6 @@ void env_relocate_spec(void) >> return; >> } >> #endif >> - >> ret = readenv(CONFIG_ENV_OFFSET, (u_char *)buf); >> if (ret) { >> set_default_env("!readenv() failed"); > Remove unrelated whitespace changes. Sure >> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile >> index 366dee6..06d5d14 100644 >> --- a/drivers/mtd/nand/Makefile >> +++ b/drivers/mtd/nand/Makefile >> @@ -24,6 +24,7 @@ COBJS-$(CONFIG_SPL_NAND_LOAD) += nand_spl_load.o >> COBJS-$(CONFIG_SPL_NAND_ECC) += nand_ecc.o >> COBJS-$(CONFIG_SPL_NAND_BASE) += nand_base.o >> COBJS-$(CONFIG_SPL_NAND_INIT) += nand.o >> +COBJS-$(CONFIG_NAND_FSL_IFC) += fsl_ifc_spl.o > No, it's still a minimal NAND driver (i.e. it doesn't use fsl_ifc.o). > Minimal NAND drivers are not related to minimal SPL init. I will take care of it >> diff --git a/drivers/mtd/nand/fsl_ifc_spl.c b/drivers/mtd/nand/fsl_ifc_spl.c >> index d462265..e7edacf 100644 >> --- a/drivers/mtd/nand/fsl_ifc_spl.c >> +++ b/drivers/mtd/nand/fsl_ifc_spl.c >> @@ -11,6 +11,28 @@ >> #include <asm/io.h> >> #include <asm/fsl_ifc.h> >> #include <linux/mtd/nand.h> >> +#ifndef CONFIG_SPL_INIT_MINIMAL >> +#include <linux/mtd/mtd.h> >> +#endif >> + >> +static void nand_load(unsigned int offs, int uboot_size, uchar *dst); >> + >> +#ifndef CONFIG_SPL_INIT_MINIMAL >> +struct mtd_info nand_info[CONFIG_SYS_MAX_NAND_DEVICE]; >> + >> +int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs) >> +{ >> + return 0; >> +} >> + >> +int nand_read_skip_bad(struct mtd_info *nand, loff_t offset, size_t *length, >> + size_t *actual, loff_t lim, u_char *buffer) >> +{ >> + nand_load(offset, *length, buffer); >> + return 0; >> +} >> +#endif > What does this have to do with minimal init? This has nothing to do with minimal init. These function will comes into during CONFIG_SPL_BUILD and !defined CONFIG_SPL_INIT_MINIMAL. These function will be used for reading env variables. Regards, Prabhakar
On Wed, 2013-09-18 at 16:40 +0530, Prabhakar Kushwaha wrote: > Thanks Scott for taking time and reviewing the RFC patch. > > Please find my reply in-lined. > > On 09/17/2013 05:23 AM, Scott Wood wrote: > > On Mon, 2013-09-16 at 21:35 +0530, Prabhakar Kushwaha wrote: > >> fsl_ifs_spl.c reads data from NAND and store at a memory location in raw mode. > >> It does not used MTD layer. > >> To read env variable from NAND MTD layer read/write required. > >> > >> Hence, add mtd_block_isbad & nand_read_skip_bad function required during > >> env variable read. > >> > >> Also, avoid nand_info during env read for SPL > >> > >> Signed-off-by: Prabhakar Kushwaha <prabhakar@freescale.com> > >> --- > >> common/env_nand.c | 7 ++++--- > >> drivers/mtd/nand/Makefile | 2 +- > >> drivers/mtd/nand/fsl_ifc_spl.c | 22 ++++++++++++++++++++++ > >> 3 files changed, 27 insertions(+), 4 deletions(-) > >> > >> diff --git a/common/env_nand.c b/common/env_nand.c > >> index 7530962..7a7107f 100644 > >> --- a/common/env_nand.c > >> +++ b/common/env_nand.c > >> @@ -246,11 +246,13 @@ int readenv(size_t offset, u_char *buf) > >> u_char *char_ptr; > >> > >> blocksize = nand_info[0].erasesize; > >> +#ifndef CONFIG_SPL_BUILD > >> if (!blocksize) > >> return 1; > >> - > >> len = min(blocksize, CONFIG_ENV_SIZE); > >> - > >> +#else > >> + len = CONFIG_ENV_SIZE; > >> +#endif > > Use positive logic (ifdef/else, not ifndef/else). > > I will fix it. > > > Are you sure that CONFIG_ENV_SIZE will always be appropriate? Shouldn't > > you use CONFIG_SYS_NAND_BLOCK_SIZE in place of nand_info[0].erasesize? > > I can use CONFIG_SYS_NAND_BLOCK_SIZE . but i can not use in SPL as its > is defined as 128K. You can't redefine the block size to suit your needs. It's a property of the hardware. What difference does it make, as long as CONFIG_ENV_SIZE is smaller? > >> +#ifndef CONFIG_SPL_INIT_MINIMAL > >> +struct mtd_info nand_info[CONFIG_SYS_MAX_NAND_DEVICE]; > >> + > >> +int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs) > >> +{ > >> + return 0; > >> +} > >> + > >> +int nand_read_skip_bad(struct mtd_info *nand, loff_t offset, size_t *length, > >> + size_t *actual, loff_t lim, u_char *buffer) > >> +{ > >> + nand_load(offset, *length, buffer); > >> + return 0; > >> +} > >> +#endif > > What does this have to do with minimal init? > This has nothing to do with minimal init. Then what does it have to do with CONFIG_SPL_INIT_MINIMAL, which is specifically for configuring minimial init? > These function will comes into during CONFIG_SPL_BUILD and !defined > CONFIG_SPL_INIT_MINIMAL. > > These function will be used for reading env variables. Then use some SPL symbol indicating environment support is needed. -Scott
On 09/23/2013 11:49 PM, Scott Wood wrote: > On Wed, 2013-09-18 at 16:40 +0530, Prabhakar Kushwaha wrote: >> Thanks Scott for taking time and reviewing the RFC patch. >> >> Please find my reply in-lined. >> >> On 09/17/2013 05:23 AM, Scott Wood wrote: >>> On Mon, 2013-09-16 at 21:35 +0530, Prabhakar Kushwaha wrote: >>>> fsl_ifs_spl.c reads data from NAND and store at a memory location in raw mode. >>>> It does not used MTD layer. >>>> To read env variable from NAND MTD layer read/write required. >>>> >>>> Hence, add mtd_block_isbad & nand_read_skip_bad function required during >>>> env variable read. >>>> >>>> Also, avoid nand_info during env read for SPL >>>> >>>> Signed-off-by: Prabhakar Kushwaha <prabhakar@freescale.com> >>>> --- >>>> common/env_nand.c | 7 ++++--- >>>> drivers/mtd/nand/Makefile | 2 +- >>>> drivers/mtd/nand/fsl_ifc_spl.c | 22 ++++++++++++++++++++++ >>>> 3 files changed, 27 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/common/env_nand.c b/common/env_nand.c >>>> index 7530962..7a7107f 100644 >>>> --- a/common/env_nand.c >>>> +++ b/common/env_nand.c >>>> @@ -246,11 +246,13 @@ int readenv(size_t offset, u_char *buf) >>>> u_char *char_ptr; >>>> >>>> blocksize = nand_info[0].erasesize; >>>> +#ifndef CONFIG_SPL_BUILD >>>> if (!blocksize) >>>> return 1; >>>> - >>>> len = min(blocksize, CONFIG_ENV_SIZE); >>>> - >>>> +#else >>>> + len = CONFIG_ENV_SIZE; >>>> +#endif >>> Use positive logic (ifdef/else, not ifndef/else). >> I will fix it. >> >>> Are you sure that CONFIG_ENV_SIZE will always be appropriate? Shouldn't >>> you use CONFIG_SYS_NAND_BLOCK_SIZE in place of nand_info[0].erasesize? >> I can use CONFIG_SYS_NAND_BLOCK_SIZE . but i can not use in SPL as its >> is defined as 128K. > You can't redefine the block size to suit your needs. It's a property > of the hardware. What difference does it make, as long as > CONFIG_ENV_SIZE is smaller? Earlier code was like len = min(blocksize, CONFIG_ENV_SIZE) as SPL never defines nand_info. So the block size is 0. causing len = 0. I am avoiding it by setting = CONFIG_ENV_SIZE. No redefinition of block size. Regards, Prabhakar
On Wed, 2013-09-25 at 09:47 +0530, Prabhakar Kushwaha wrote: > On 09/23/2013 11:49 PM, Scott Wood wrote: > > On Wed, 2013-09-18 at 16:40 +0530, Prabhakar Kushwaha wrote: > >> Thanks Scott for taking time and reviewing the RFC patch. > >> > >> Please find my reply in-lined. > >> > >> On 09/17/2013 05:23 AM, Scott Wood wrote: > >>> On Mon, 2013-09-16 at 21:35 +0530, Prabhakar Kushwaha wrote: > >>>> fsl_ifs_spl.c reads data from NAND and store at a memory location in raw mode. > >>>> It does not used MTD layer. > >>>> To read env variable from NAND MTD layer read/write required. > >>>> > >>>> Hence, add mtd_block_isbad & nand_read_skip_bad function required during > >>>> env variable read. > >>>> > >>>> Also, avoid nand_info during env read for SPL > >>>> > >>>> Signed-off-by: Prabhakar Kushwaha <prabhakar@freescale.com> > >>>> --- > >>>> common/env_nand.c | 7 ++++--- > >>>> drivers/mtd/nand/Makefile | 2 +- > >>>> drivers/mtd/nand/fsl_ifc_spl.c | 22 ++++++++++++++++++++++ > >>>> 3 files changed, 27 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/common/env_nand.c b/common/env_nand.c > >>>> index 7530962..7a7107f 100644 > >>>> --- a/common/env_nand.c > >>>> +++ b/common/env_nand.c > >>>> @@ -246,11 +246,13 @@ int readenv(size_t offset, u_char *buf) > >>>> u_char *char_ptr; > >>>> > >>>> blocksize = nand_info[0].erasesize; > >>>> +#ifndef CONFIG_SPL_BUILD > >>>> if (!blocksize) > >>>> return 1; > >>>> - > >>>> len = min(blocksize, CONFIG_ENV_SIZE); > >>>> - > >>>> +#else > >>>> + len = CONFIG_ENV_SIZE; > >>>> +#endif > >>> Use positive logic (ifdef/else, not ifndef/else). > >> I will fix it. > >> > >>> Are you sure that CONFIG_ENV_SIZE will always be appropriate? Shouldn't > >>> you use CONFIG_SYS_NAND_BLOCK_SIZE in place of nand_info[0].erasesize? > >> I can use CONFIG_SYS_NAND_BLOCK_SIZE . but i can not use in SPL as its > >> is defined as 128K. > > You can't redefine the block size to suit your needs. It's a property > > of the hardware. What difference does it make, as long as > > CONFIG_ENV_SIZE is smaller? > Earlier code was like > len = min(blocksize, CONFIG_ENV_SIZE) as SPL never defines nand_info. > So the block size is 0. > causing len = 0. > > I am avoiding it by setting = CONFIG_ENV_SIZE. No redefinition of block > size. This is generic code. It's possible (even if a bit unlikely) that some board has an environment that is larger than the block size. That's why the loop exists in the first place (though I'm not sure why it can't be replaced with a single call to nand_read_skip_bad). Since your problem is with how you determine the block size, just replace the source of the block size but don't touch the min(). -Scott
diff --git a/common/env_nand.c b/common/env_nand.c index 7530962..7a7107f 100644 --- a/common/env_nand.c +++ b/common/env_nand.c @@ -246,11 +246,13 @@ int readenv(size_t offset, u_char *buf) u_char *char_ptr; blocksize = nand_info[0].erasesize; +#ifndef CONFIG_SPL_BUILD if (!blocksize) return 1; - len = min(blocksize, CONFIG_ENV_SIZE); - +#else + len = CONFIG_ENV_SIZE; +#endif while (amount_loaded < CONFIG_ENV_SIZE && offset < end) { if (nand_block_isbad(&nand_info[0], offset)) { offset += blocksize; @@ -396,7 +398,6 @@ void env_relocate_spec(void) return; } #endif - ret = readenv(CONFIG_ENV_OFFSET, (u_char *)buf); if (ret) { set_default_env("!readenv() failed"); diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile index 366dee6..06d5d14 100644 --- a/drivers/mtd/nand/Makefile +++ b/drivers/mtd/nand/Makefile @@ -24,6 +24,7 @@ COBJS-$(CONFIG_SPL_NAND_LOAD) += nand_spl_load.o COBJS-$(CONFIG_SPL_NAND_ECC) += nand_ecc.o COBJS-$(CONFIG_SPL_NAND_BASE) += nand_base.o COBJS-$(CONFIG_SPL_NAND_INIT) += nand.o +COBJS-$(CONFIG_NAND_FSL_IFC) += fsl_ifc_spl.o else # not spl @@ -68,7 +69,6 @@ COBJS-$(CONFIG_NAND_DOCG4) += docg4.o else # minimal SPL drivers COBJS-$(CONFIG_NAND_FSL_ELBC) += fsl_elbc_spl.o -COBJS-$(CONFIG_NAND_FSL_IFC) += fsl_ifc_spl.o COBJS-$(CONFIG_NAND_MXC) += mxc_nand_spl.o endif # drivers diff --git a/drivers/mtd/nand/fsl_ifc_spl.c b/drivers/mtd/nand/fsl_ifc_spl.c index d462265..e7edacf 100644 --- a/drivers/mtd/nand/fsl_ifc_spl.c +++ b/drivers/mtd/nand/fsl_ifc_spl.c @@ -11,6 +11,28 @@ #include <asm/io.h> #include <asm/fsl_ifc.h> #include <linux/mtd/nand.h> +#ifndef CONFIG_SPL_INIT_MINIMAL +#include <linux/mtd/mtd.h> +#endif + +static void nand_load(unsigned int offs, int uboot_size, uchar *dst); + +#ifndef CONFIG_SPL_INIT_MINIMAL +struct mtd_info nand_info[CONFIG_SYS_MAX_NAND_DEVICE]; + +int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs) +{ + return 0; +} + +int nand_read_skip_bad(struct mtd_info *nand, loff_t offset, size_t *length, + size_t *actual, loff_t lim, u_char *buffer) +{ + nand_load(offset, *length, buffer); + return 0; +} +#endif + static inline int is_blank(uchar *addr, int page_size) {
fsl_ifs_spl.c reads data from NAND and store at a memory location in raw mode. It does not used MTD layer. To read env variable from NAND MTD layer read/write required. Hence, add mtd_block_isbad & nand_read_skip_bad function required during env variable read. Also, avoid nand_info during env read for SPL Signed-off-by: Prabhakar Kushwaha <prabhakar@freescale.com> --- common/env_nand.c | 7 ++++--- drivers/mtd/nand/Makefile | 2 +- drivers/mtd/nand/fsl_ifc_spl.c | 22 ++++++++++++++++++++++ 3 files changed, 27 insertions(+), 4 deletions(-)