| Message ID | 1400264797-3593-3-git-send-email-ivan.khoronzhuk@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Scott Wood |
| Headers | show |
On Fri, May 16, 2014 at 09:26:37PM +0300, Khoronzhuk, Ivan wrote: > From: WingMan Kwok <w-kwok2@ti.com> > > This patch adds opportunity to change ecclayout of current nand > device during runtime. So we can change the current nand device > ecclayout using the "nand ecclayout set" command before writing > the data to nand flash. > > Signed-off-by: Hao Zhang <hzhang@ti.com> > Signed-off-by: WingMan Kwok <w-kwok2@ti.com> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> > > --- > drivers/mtd/nand/davinci_nand.c | 101 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 101 insertions(+) > > diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c > index 75b03a7..b33de0d 100644 > --- a/drivers/mtd/nand/davinci_nand.c > +++ b/drivers/mtd/nand/davinci_nand.c > @@ -306,6 +306,103 @@ static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { > #endif > }; > > +#if defined(CONFIG_CMD_NAND_ECCLAYOUT) > +#if defined(CONFIG_SYS_NAND_PAGE_2K) Undocumented CONFIG symbol (yes, I know it's not new)... Also redundant with CONFIG_SYS_NAND_PAGE_SIZE (though that's only documented in the context of SPL). > +static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = { > + .eccbytes = 40, > + .eccpos = { > + 6, 7, > + 8, 9, 10, 11, 12, 13, 14, 15, > + 22, 23, > + 24, 25, 26, 27, 28, 29, 30, 31, > + 38, 39, > + 40, 41, 42, 43, 44, 45, 46, 47, > + 54, 55, > + 56, 57, 58, 59, 60, 61, 62, 63, > + }, Why the odd 2/8 pattern? > + .oobfree = { > + {.offset = 2, .length = 4, }, > + {.offset = 16, .length = 6, }, > + {.offset = 32, .length = 6, }, > + {.offset = 48, .length = 6, }, > + }, > +}; > +#elif defined(CONFIG_SYS_NAND_PAGE_4K) > +static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = { > + .eccbytes = 80, > + .eccpos = { > + 6, 7, > + 8, 9, 10, 11, 12, 13, 14, 15, > + 22, 23, > + 24, 25, 26, 27, 28, 29, 30, 31, > + 38, 39, > + 40, 41, 42, 43, 44, 45, 46, 47, > + 54, 55, > + 56, 57, 58, 59, 60, 61, 62, 63, > + 70, 71, > + 72, 73, 74, 75, 76, 77, 78, 79, > + 86, 87, > + 88, 89, 90, 91, 92, 93, 94, 95, > + 102, 103, > + 104, 105, 106, 107, 108, 109, 110, 111, > + 118, 119, > + 120, 121, 122, 123, 124, 125, 126, 127, > + }, > + .oobfree = { > + {.offset = 2, .length = 4, }, > + {.offset = 16, .length = 6, }, > + {.offset = 32, .length = 6, }, > + {.offset = 48, .length = 6, }, > + {.offset = 64, .length = 6, }, > + {.offset = 80, .length = 6, }, > + {.offset = 96, .length = 6, }, > + {.offset = 112, .length = 6, }, > + }, > +}; > +#endif > + > +#define NAND_ECCLAYOUT_NUM 2 > + > +struct nand_ecclayout *davinci_nand_ecclayouts[NAND_ECCLAYOUT_NUM] = { > + &nand_davinci_4bit_layout_oobfirst, > + &nand_keystone_rbl_4bit_layout_oobfirst, > +}; > + > +int board_nand_ecclayout_get_idx(struct nand_chip *nand, > + struct nand_ecclayout *p) > +{ > + int i; > + > + if (!p) > + return -1; > + > + for (i = 0; i < NAND_ECCLAYOUT_NUM; i++) > + if (davinci_nand_ecclayouts[i] == p) > + return i; > + > + return -1; > +} Use ARRAY_SIZE(). > + > +struct nand_ecclayout *board_nand_ecclayout_get_layout(struct nand_chip *nand, > + int idx) > +{ > + if ((idx >= 0) && (idx < NAND_ECCLAYOUT_NUM)) > + return davinci_nand_ecclayouts[idx]; > + else > + return NULL; > +} > + > +int board_nand_ecclayout_set(struct nand_chip *nand, int idx) > +{ > + if (idx < 0 || idx >= NAND_ECCLAYOUT_NUM) > + return -1; > + > + nand->ecc.layout = davinci_nand_ecclayouts[idx]; > + > + return 0; > +} > +#endif Put a comment on this indicating what condition the #endif is ending. > static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int mode) > { > u32 val; > @@ -631,7 +728,11 @@ void davinci_nand_init(struct nand_chip *nand) > nand->ecc.calculate = nand_davinci_4bit_calculate_ecc; > nand->ecc.correct = nand_davinci_4bit_correct_data; > nand->ecc.hwctl = nand_davinci_4bit_enable_hwecc; > +#ifndef CONFIG_CMD_NAND_ECCLAYOUT > nand->ecc.layout = &nand_davinci_4bit_layout_oobfirst; > +#else > + nand->ecc.layout = &nand_keystone_rbl_4bit_layout_oobfirst; > +#endif Use "ifdef/else" rather than "ifndef/else". Why does enabling layout selection change the default layout? Nothing in the changelog suggests that. -Scott
On 06/20/2014 04:07 AM, Scott Wood wrote: > On Fri, May 16, 2014 at 09:26:37PM +0300, Khoronzhuk, Ivan wrote: >> From: WingMan Kwok <w-kwok2@ti.com> >> >> This patch adds opportunity to change ecclayout of current nand >> device during runtime. So we can change the current nand device >> ecclayout using the "nand ecclayout set" command before writing >> the data to nand flash. >> >> Signed-off-by: Hao Zhang <hzhang@ti.com> >> Signed-off-by: WingMan Kwok <w-kwok2@ti.com> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> >> >> --- >> drivers/mtd/nand/davinci_nand.c | 101 ++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 101 insertions(+) >> >> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c >> index 75b03a7..b33de0d 100644 >> --- a/drivers/mtd/nand/davinci_nand.c >> +++ b/drivers/mtd/nand/davinci_nand.c >> @@ -306,6 +306,103 @@ static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { >> #endif >> }; >> >> +#if defined(CONFIG_CMD_NAND_ECCLAYOUT) >> +#if defined(CONFIG_SYS_NAND_PAGE_2K) > Undocumented CONFIG symbol (yes, I know it's not new)... Also redundant > with CONFIG_SYS_NAND_PAGE_SIZE (though that's only documented in the > context of SPL). > >> +static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = { >> + .eccbytes = 40, >> + .eccpos = { >> + 6, 7, >> + 8, 9, 10, 11, 12, 13, 14, 15, >> + 22, 23, >> + 24, 25, 26, 27, 28, 29, 30, 31, >> + 38, 39, >> + 40, 41, 42, 43, 44, 45, 46, 47, >> + 54, 55, >> + 56, 57, 58, 59, 60, 61, 62, 63, >> + }, > Why the odd 2/8 pattern? ...I'll align. > >> + .oobfree = { >> + {.offset = 2, .length = 4, }, >> + {.offset = 16, .length = 6, }, >> + {.offset = 32, .length = 6, }, >> + {.offset = 48, .length = 6, }, >> + }, >> +}; >> +#elif defined(CONFIG_SYS_NAND_PAGE_4K) >> +static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = { >> + .eccbytes = 80, >> + .eccpos = { >> + 6, 7, >> + 8, 9, 10, 11, 12, 13, 14, 15, >> + 22, 23, >> + 24, 25, 26, 27, 28, 29, 30, 31, >> + 38, 39, >> + 40, 41, 42, 43, 44, 45, 46, 47, >> + 54, 55, >> + 56, 57, 58, 59, 60, 61, 62, 63, >> + 70, 71, >> + 72, 73, 74, 75, 76, 77, 78, 79, >> + 86, 87, >> + 88, 89, 90, 91, 92, 93, 94, 95, >> + 102, 103, >> + 104, 105, 106, 107, 108, 109, 110, 111, >> + 118, 119, >> + 120, 121, 122, 123, 124, 125, 126, 127, >> + }, >> + .oobfree = { >> + {.offset = 2, .length = 4, }, >> + {.offset = 16, .length = 6, }, >> + {.offset = 32, .length = 6, }, >> + {.offset = 48, .length = 6, }, >> + {.offset = 64, .length = 6, }, >> + {.offset = 80, .length = 6, }, >> + {.offset = 96, .length = 6, }, >> + {.offset = 112, .length = 6, }, >> + }, >> +}; >> +#endif >> + >> +#define NAND_ECCLAYOUT_NUM 2 >> + >> +struct nand_ecclayout *davinci_nand_ecclayouts[NAND_ECCLAYOUT_NUM] = { >> + &nand_davinci_4bit_layout_oobfirst, >> + &nand_keystone_rbl_4bit_layout_oobfirst, >> +}; >> + >> +int board_nand_ecclayout_get_idx(struct nand_chip *nand, >> + struct nand_ecclayout *p) >> +{ >> + int i; >> + >> + if (!p) >> + return -1; >> + >> + for (i = 0; i < NAND_ECCLAYOUT_NUM; i++) >> + if (davinci_nand_ecclayouts[i] == p) >> + return i; >> + >> + return -1; >> +} > Use ARRAY_SIZE(). Ok. > >> + >> +struct nand_ecclayout *board_nand_ecclayout_get_layout(struct nand_chip *nand, >> + int idx) >> +{ >> + if ((idx >= 0) && (idx < NAND_ECCLAYOUT_NUM)) >> + return davinci_nand_ecclayouts[idx]; >> + else >> + return NULL; >> +} >> + >> +int board_nand_ecclayout_set(struct nand_chip *nand, int idx) >> +{ >> + if (idx < 0 || idx >= NAND_ECCLAYOUT_NUM) >> + return -1; >> + >> + nand->ecc.layout = davinci_nand_ecclayouts[idx]; >> + >> + return 0; >> +} >> +#endif > Put a comment on this indicating what condition the #endif is ending. Ok. > >> static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int mode) >> { >> u32 val; >> @@ -631,7 +728,11 @@ void davinci_nand_init(struct nand_chip *nand) >> nand->ecc.calculate = nand_davinci_4bit_calculate_ecc; >> nand->ecc.correct = nand_davinci_4bit_correct_data; >> nand->ecc.hwctl = nand_davinci_4bit_enable_hwecc; >> +#ifndef CONFIG_CMD_NAND_ECCLAYOUT >> nand->ecc.layout = &nand_davinci_4bit_layout_oobfirst; >> +#else >> + nand->ecc.layout = &nand_keystone_rbl_4bit_layout_oobfirst; >> +#endif > Use "ifdef/else" rather than "ifndef/else". > > Why does enabling layout selection change the default layout? Nothing in > the changelog suggests that. It's not correct. I will move it to board code. I'll better do smth like the following: In board config: #define CONFIG_SYS_NAND_SELF_INIT in board config: In nand davinci header: #define NAND_DAVINCI_4BIT_LAYOUT 0 #define NAND_KEYSTONE_RBL_4BIT_LAYOUT 1 In board file: int board_nand_init(struct nand_chip *chip) { davinci_nand_init(chip); chip->ecc.layout = board_nand_ecclayout_get_layout(NAND_KEYSTONE_RBL_4BIT_LAYOUT); return 0; } > > -Scott Thanks a lot Scott, I will send the new series soon.
On Fri, 2014-06-20 at 17:59 +0300, Ivan Khoronzhuk wrote: > On 06/20/2014 04:07 AM, Scott Wood wrote: > > On Fri, May 16, 2014 at 09:26:37PM +0300, Khoronzhuk, Ivan wrote: > >> From: WingMan Kwok <w-kwok2@ti.com> > >> > >> This patch adds opportunity to change ecclayout of current nand > >> device during runtime. So we can change the current nand device > >> ecclayout using the "nand ecclayout set" command before writing > >> the data to nand flash. > >> > >> Signed-off-by: Hao Zhang <hzhang@ti.com> > >> Signed-off-by: WingMan Kwok <w-kwok2@ti.com> > >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> > >> > >> --- > >> drivers/mtd/nand/davinci_nand.c | 101 ++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 101 insertions(+) > >> > >> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c > >> index 75b03a7..b33de0d 100644 > >> --- a/drivers/mtd/nand/davinci_nand.c > >> +++ b/drivers/mtd/nand/davinci_nand.c > >> @@ -306,6 +306,103 @@ static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { > >> #endif > >> }; > >> > >> +#if defined(CONFIG_CMD_NAND_ECCLAYOUT) > >> +#if defined(CONFIG_SYS_NAND_PAGE_2K) > > Undocumented CONFIG symbol (yes, I know it's not new)... Also redundant > > with CONFIG_SYS_NAND_PAGE_SIZE (though that's only documented in the > > context of SPL). > > > >> +static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = { > >> + .eccbytes = 40, > >> + .eccpos = { > >> + 6, 7, > >> + 8, 9, 10, 11, 12, 13, 14, 15, > >> + 22, 23, > >> + 24, 25, 26, 27, 28, 29, 30, 31, > >> + 38, 39, > >> + 40, 41, 42, 43, 44, 45, 46, 47, > >> + 54, 55, > >> + 56, 57, 58, 59, 60, 61, 62, 63, > >> + }, > > Why the odd 2/8 pattern? > > ...I'll align. > > > > >> + .oobfree = { > >> + {.offset = 2, .length = 4, }, > >> + {.offset = 16, .length = 6, }, > >> + {.offset = 32, .length = 6, }, > >> + {.offset = 48, .length = 6, }, > >> + }, > >> +}; > >> +#elif defined(CONFIG_SYS_NAND_PAGE_4K) > >> +static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = { > >> + .eccbytes = 80, > >> + .eccpos = { > >> + 6, 7, > >> + 8, 9, 10, 11, 12, 13, 14, 15, > >> + 22, 23, > >> + 24, 25, 26, 27, 28, 29, 30, 31, > >> + 38, 39, > >> + 40, 41, 42, 43, 44, 45, 46, 47, > >> + 54, 55, > >> + 56, 57, 58, 59, 60, 61, 62, 63, > >> + 70, 71, > >> + 72, 73, 74, 75, 76, 77, 78, 79, > >> + 86, 87, > >> + 88, 89, 90, 91, 92, 93, 94, 95, > >> + 102, 103, > >> + 104, 105, 106, 107, 108, 109, 110, 111, > >> + 118, 119, > >> + 120, 121, 122, 123, 124, 125, 126, 127, > >> + }, > >> + .oobfree = { > >> + {.offset = 2, .length = 4, }, > >> + {.offset = 16, .length = 6, }, > >> + {.offset = 32, .length = 6, }, > >> + {.offset = 48, .length = 6, }, > >> + {.offset = 64, .length = 6, }, > >> + {.offset = 80, .length = 6, }, > >> + {.offset = 96, .length = 6, }, > >> + {.offset = 112, .length = 6, }, > >> + }, > >> +}; > >> +#endif > >> + > >> +#define NAND_ECCLAYOUT_NUM 2 > >> + > >> +struct nand_ecclayout *davinci_nand_ecclayouts[NAND_ECCLAYOUT_NUM] = { > >> + &nand_davinci_4bit_layout_oobfirst, > >> + &nand_keystone_rbl_4bit_layout_oobfirst, > >> +}; > >> + > >> +int board_nand_ecclayout_get_idx(struct nand_chip *nand, > >> + struct nand_ecclayout *p) > >> +{ > >> + int i; > >> + > >> + if (!p) > >> + return -1; > >> + > >> + for (i = 0; i < NAND_ECCLAYOUT_NUM; i++) > >> + if (davinci_nand_ecclayouts[i] == p) > >> + return i; > >> + > >> + return -1; > >> +} > > Use ARRAY_SIZE(). > > Ok. > > > > >> + > >> +struct nand_ecclayout *board_nand_ecclayout_get_layout(struct nand_chip *nand, > >> + int idx) > >> +{ > >> + if ((idx >= 0) && (idx < NAND_ECCLAYOUT_NUM)) > >> + return davinci_nand_ecclayouts[idx]; > >> + else > >> + return NULL; > >> +} > >> + > >> +int board_nand_ecclayout_set(struct nand_chip *nand, int idx) > >> +{ > >> + if (idx < 0 || idx >= NAND_ECCLAYOUT_NUM) > >> + return -1; > >> + > >> + nand->ecc.layout = davinci_nand_ecclayouts[idx]; > >> + > >> + return 0; > >> +} > >> +#endif > > Put a comment on this indicating what condition the #endif is ending. > > Ok. > > > > >> static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int mode) > >> { > >> u32 val; > >> @@ -631,7 +728,11 @@ void davinci_nand_init(struct nand_chip *nand) > >> nand->ecc.calculate = nand_davinci_4bit_calculate_ecc; > >> nand->ecc.correct = nand_davinci_4bit_correct_data; > >> nand->ecc.hwctl = nand_davinci_4bit_enable_hwecc; > >> +#ifndef CONFIG_CMD_NAND_ECCLAYOUT > >> nand->ecc.layout = &nand_davinci_4bit_layout_oobfirst; > >> +#else > >> + nand->ecc.layout = &nand_keystone_rbl_4bit_layout_oobfirst; > >> +#endif > > Use "ifdef/else" rather than "ifndef/else". > > > > Why does enabling layout selection change the default layout? Nothing in > > the changelog suggests that. > > It's not correct. I will move it to board code. > I'll better do smth like the following: > > In board config: > #define CONFIG_SYS_NAND_SELF_INIT in board config: > > In nand davinci header: > #define NAND_DAVINCI_4BIT_LAYOUT 0 > #define NAND_KEYSTONE_RBL_4BIT_LAYOUT 1 > > In board file: > int board_nand_init(struct nand_chip *chip) > { > davinci_nand_init(chip); > chip->ecc.layout = > board_nand_ecclayout_get_layout(NAND_KEYSTONE_RBL_4BIT_LAYOUT); > > return 0; > } What does CONFIG_SYS_NAND_SELF_INIT have to do with this? Note that with that set, board_nand_init does not take an argument. "board_nand_ecclayout_get_layout" is a bit long -- how about just "board_nand_get_ecclayout"? -Scott
On 06/20/2014 07:22 PM, Scott Wood wrote: > On Fri, 2014-06-20 at 17:59 +0300, Ivan Khoronzhuk wrote: >> On 06/20/2014 04:07 AM, Scott Wood wrote: >>> On Fri, May 16, 2014 at 09:26:37PM +0300, Khoronzhuk, Ivan wrote: >>>> From: WingMan Kwok <w-kwok2@ti.com> >>>> >>>> This patch adds opportunity to change ecclayout of current nand >>>> device during runtime. So we can change the current nand device >>>> ecclayout using the "nand ecclayout set" command before writing >>>> the data to nand flash. >>>> >>>> Signed-off-by: Hao Zhang <hzhang@ti.com> >>>> Signed-off-by: WingMan Kwok <w-kwok2@ti.com> >>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> >>>> >>>> --- >>>> drivers/mtd/nand/davinci_nand.c | 101 ++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 101 insertions(+) >>>> >>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c >>>> index 75b03a7..b33de0d 100644 >>>> --- a/drivers/mtd/nand/davinci_nand.c >>>> +++ b/drivers/mtd/nand/davinci_nand.c >>>> @@ -306,6 +306,103 @@ static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { >>>> #endif >>>> }; >>>> >>>> +#if defined(CONFIG_CMD_NAND_ECCLAYOUT) >>>> +#if defined(CONFIG_SYS_NAND_PAGE_2K) >>> Undocumented CONFIG symbol (yes, I know it's not new)... Also redundant >>> with CONFIG_SYS_NAND_PAGE_SIZE (though that's only documented in the >>> context of SPL). >>> >>>> +static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = { >>>> + .eccbytes = 40, >>>> + .eccpos = { >>>> + 6, 7, >>>> + 8, 9, 10, 11, 12, 13, 14, 15, >>>> + 22, 23, >>>> + 24, 25, 26, 27, 28, 29, 30, 31, >>>> + 38, 39, >>>> + 40, 41, 42, 43, 44, 45, 46, 47, >>>> + 54, 55, >>>> + 56, 57, 58, 59, 60, 61, 62, 63, >>>> + }, >>> Why the odd 2/8 pattern? >> ...I'll align. >> >>>> + .oobfree = { >>>> + {.offset = 2, .length = 4, }, >>>> + {.offset = 16, .length = 6, }, >>>> + {.offset = 32, .length = 6, }, >>>> + {.offset = 48, .length = 6, }, >>>> + }, >>>> +}; >>>> +#elif defined(CONFIG_SYS_NAND_PAGE_4K) >>>> +static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = { >>>> + .eccbytes = 80, >>>> + .eccpos = { >>>> + 6, 7, >>>> + 8, 9, 10, 11, 12, 13, 14, 15, >>>> + 22, 23, >>>> + 24, 25, 26, 27, 28, 29, 30, 31, >>>> + 38, 39, >>>> + 40, 41, 42, 43, 44, 45, 46, 47, >>>> + 54, 55, >>>> + 56, 57, 58, 59, 60, 61, 62, 63, >>>> + 70, 71, >>>> + 72, 73, 74, 75, 76, 77, 78, 79, >>>> + 86, 87, >>>> + 88, 89, 90, 91, 92, 93, 94, 95, >>>> + 102, 103, >>>> + 104, 105, 106, 107, 108, 109, 110, 111, >>>> + 118, 119, >>>> + 120, 121, 122, 123, 124, 125, 126, 127, >>>> + }, >>>> + .oobfree = { >>>> + {.offset = 2, .length = 4, }, >>>> + {.offset = 16, .length = 6, }, >>>> + {.offset = 32, .length = 6, }, >>>> + {.offset = 48, .length = 6, }, >>>> + {.offset = 64, .length = 6, }, >>>> + {.offset = 80, .length = 6, }, >>>> + {.offset = 96, .length = 6, }, >>>> + {.offset = 112, .length = 6, }, >>>> + }, >>>> +}; >>>> +#endif >>>> + >>>> +#define NAND_ECCLAYOUT_NUM 2 >>>> + >>>> +struct nand_ecclayout *davinci_nand_ecclayouts[NAND_ECCLAYOUT_NUM] = { >>>> + &nand_davinci_4bit_layout_oobfirst, >>>> + &nand_keystone_rbl_4bit_layout_oobfirst, >>>> +}; >>>> + >>>> +int board_nand_ecclayout_get_idx(struct nand_chip *nand, >>>> + struct nand_ecclayout *p) >>>> +{ >>>> + int i; >>>> + >>>> + if (!p) >>>> + return -1; >>>> + >>>> + for (i = 0; i < NAND_ECCLAYOUT_NUM; i++) >>>> + if (davinci_nand_ecclayouts[i] == p) >>>> + return i; >>>> + >>>> + return -1; >>>> +} >>> Use ARRAY_SIZE(). >> Ok. >> >>>> + >>>> +struct nand_ecclayout *board_nand_ecclayout_get_layout(struct nand_chip *nand, >>>> + int idx) >>>> +{ >>>> + if ((idx >= 0) && (idx < NAND_ECCLAYOUT_NUM)) >>>> + return davinci_nand_ecclayouts[idx]; >>>> + else >>>> + return NULL; >>>> +} >>>> + >>>> +int board_nand_ecclayout_set(struct nand_chip *nand, int idx) >>>> +{ >>>> + if (idx < 0 || idx >= NAND_ECCLAYOUT_NUM) >>>> + return -1; >>>> + >>>> + nand->ecc.layout = davinci_nand_ecclayouts[idx]; >>>> + >>>> + return 0; >>>> +} >>>> +#endif >>> Put a comment on this indicating what condition the #endif is ending. >> Ok. >> >>>> static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int mode) >>>> { >>>> u32 val; >>>> @@ -631,7 +728,11 @@ void davinci_nand_init(struct nand_chip *nand) >>>> nand->ecc.calculate = nand_davinci_4bit_calculate_ecc; >>>> nand->ecc.correct = nand_davinci_4bit_correct_data; >>>> nand->ecc.hwctl = nand_davinci_4bit_enable_hwecc; >>>> +#ifndef CONFIG_CMD_NAND_ECCLAYOUT >>>> nand->ecc.layout = &nand_davinci_4bit_layout_oobfirst; >>>> +#else >>>> + nand->ecc.layout = &nand_keystone_rbl_4bit_layout_oobfirst; >>>> +#endif >>> Use "ifdef/else" rather than "ifndef/else". >>> >>> Why does enabling layout selection change the default layout? Nothing in >>> the changelog suggests that. >> It's not correct. I will move it to board code. >> I'll better do smth like the following: >> >> In board config: >> #define CONFIG_SYS_NAND_SELF_INIT in board config: >> >> In nand davinci header: >> #define NAND_DAVINCI_4BIT_LAYOUT 0 >> #define NAND_KEYSTONE_RBL_4BIT_LAYOUT 1 >> >> In board file: >> int board_nand_init(struct nand_chip *chip) >> { >> davinci_nand_init(chip); >> chip->ecc.layout = >> board_nand_ecclayout_get_layout(NAND_KEYSTONE_RBL_4BIT_LAYOUT); >> >> return 0; >> } > What does CONFIG_SYS_NAND_SELF_INIT have to do with this? Note that > with that set, board_nand_init does not take an argument. Yes. Just seeing this while implementing...It's not needed. > > "board_nand_ecclayout_get_layout" is a bit long -- how about just > "board_nand_get_ecclayout"? I will rename to board_nand_get_ecclayout
diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c index 75b03a7..b33de0d 100644 --- a/drivers/mtd/nand/davinci_nand.c +++ b/drivers/mtd/nand/davinci_nand.c @@ -306,6 +306,103 @@ static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = { #endif }; +#if defined(CONFIG_CMD_NAND_ECCLAYOUT) +#if defined(CONFIG_SYS_NAND_PAGE_2K) +static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = { + .eccbytes = 40, + .eccpos = { + 6, 7, + 8, 9, 10, 11, 12, 13, 14, 15, + 22, 23, + 24, 25, 26, 27, 28, 29, 30, 31, + 38, 39, + 40, 41, 42, 43, 44, 45, 46, 47, + 54, 55, + 56, 57, 58, 59, 60, 61, 62, 63, + }, + .oobfree = { + {.offset = 2, .length = 4, }, + {.offset = 16, .length = 6, }, + {.offset = 32, .length = 6, }, + {.offset = 48, .length = 6, }, + }, +}; +#elif defined(CONFIG_SYS_NAND_PAGE_4K) +static struct nand_ecclayout nand_keystone_rbl_4bit_layout_oobfirst = { + .eccbytes = 80, + .eccpos = { + 6, 7, + 8, 9, 10, 11, 12, 13, 14, 15, + 22, 23, + 24, 25, 26, 27, 28, 29, 30, 31, + 38, 39, + 40, 41, 42, 43, 44, 45, 46, 47, + 54, 55, + 56, 57, 58, 59, 60, 61, 62, 63, + 70, 71, + 72, 73, 74, 75, 76, 77, 78, 79, + 86, 87, + 88, 89, 90, 91, 92, 93, 94, 95, + 102, 103, + 104, 105, 106, 107, 108, 109, 110, 111, + 118, 119, + 120, 121, 122, 123, 124, 125, 126, 127, + }, + .oobfree = { + {.offset = 2, .length = 4, }, + {.offset = 16, .length = 6, }, + {.offset = 32, .length = 6, }, + {.offset = 48, .length = 6, }, + {.offset = 64, .length = 6, }, + {.offset = 80, .length = 6, }, + {.offset = 96, .length = 6, }, + {.offset = 112, .length = 6, }, + }, +}; +#endif + +#define NAND_ECCLAYOUT_NUM 2 + +struct nand_ecclayout *davinci_nand_ecclayouts[NAND_ECCLAYOUT_NUM] = { + &nand_davinci_4bit_layout_oobfirst, + &nand_keystone_rbl_4bit_layout_oobfirst, +}; + +int board_nand_ecclayout_get_idx(struct nand_chip *nand, + struct nand_ecclayout *p) +{ + int i; + + if (!p) + return -1; + + for (i = 0; i < NAND_ECCLAYOUT_NUM; i++) + if (davinci_nand_ecclayouts[i] == p) + return i; + + return -1; +} + +struct nand_ecclayout *board_nand_ecclayout_get_layout(struct nand_chip *nand, + int idx) +{ + if ((idx >= 0) && (idx < NAND_ECCLAYOUT_NUM)) + return davinci_nand_ecclayouts[idx]; + else + return NULL; +} + +int board_nand_ecclayout_set(struct nand_chip *nand, int idx) +{ + if (idx < 0 || idx >= NAND_ECCLAYOUT_NUM) + return -1; + + nand->ecc.layout = davinci_nand_ecclayouts[idx]; + + return 0; +} +#endif + static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int mode) { u32 val; @@ -631,7 +728,11 @@ void davinci_nand_init(struct nand_chip *nand) nand->ecc.calculate = nand_davinci_4bit_calculate_ecc; nand->ecc.correct = nand_davinci_4bit_correct_data; nand->ecc.hwctl = nand_davinci_4bit_enable_hwecc; +#ifndef CONFIG_CMD_NAND_ECCLAYOUT nand->ecc.layout = &nand_davinci_4bit_layout_oobfirst; +#else + nand->ecc.layout = &nand_keystone_rbl_4bit_layout_oobfirst; +#endif #endif /* Set address of hardware control function */ nand->cmd_ctrl = nand_davinci_hwcontrol;