[U-Boot] [U-Boot, U-boot, 2/2] mtd: nand: davinci: allow to change ecclayout by ecclayout command

[Scott Wood]

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 at 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 at ti.com>
> >> Signed-off-by: WingMan Kwok <w-kwok2 at ti.com>
> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk at 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