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-kw...@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 <hzh...@ti.com>
> >> Signed-off-by: WingMan Kwok <w-kw...@ti.com>
> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronz...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
