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

[Ivan Khoronzhuk]

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 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.

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

-- 
Regards,
Ivan Khoronzhuk