(apologies for multiple mails, but mails from my client were rejected due to some client configuration)

On Tue, 2013-09-03 at 11:26 +0530, Pekon Gupta wrote:

...

diff --git a/doc/README.nand b/doc/README.nand index 913e9b5..f72f618 100644 --- a/doc/README.nand +++ b/doc/README.nand @@ -169,6 +169,19 @@ Configuration Options: Please convert your driver even if you don't need the extra flexibility, so that one day we can eliminate the old mechanism.

• CONFIG_SYS_NAND_ONFI_DETECTION
• Enables detection of ONFI compliant devices during probe.
• And fetching device parameters flashed on device, by parsing
• ONFI parameter page.

I don't see this used anywhere in the patch (defined in config headers, yes, but not ifdeffed).

What does "DETECTION" add here, versus just CONFIG_NAND_ONFI?

It should be CONFIG rather than CONFIG_SYS because it just controls whether the support is built, not making an assertion about the hardware, right?

This is not the new define, it was already there in generic code (nand_base.c) But not documented, so I documented it and enabled it for am33xx platform Please refer: $UBOOT/drivers/mtd/nand/nand_base.c It was added as part of following commit commit 0272c718ba69c60a9d719db6806971d98db98090 Author: Florian Fainelli florian@openwrt.org AuthorDate: 2011-02-25

...

• CONFIG_SYS_NAND_ECCSCHEME
• specifies which ECC scheme to use.

Specifies it how? What are the possible values?

If the answer involves "enum omap_ecc", then OMAP should be somewhere in the name.

I wanted this define to be generic for all NAND drivers. Therefore CONFIG_SYS_ But I don't know how other drivers would use this. For AM335x its updated in Patch: [PATCH v5 5/5] board/ti/am335x/README: update for NAND boot

Instead, should I make it more generic for all SoC ? like CONFIG_SYS_NAND_ECCSCHEME can take in following strings defining ECC scheme used by NAND driver in SPL boot: "ham1" - 1 bit hamming code "bch4" - 4-bit BCH ecc code "bch8" - 8-bit BCH ecc code "bch16" - 16-bit BCH ecc code

...

+/**

• • omap_select_ecc_scheme - configures driver for particular ecc-scheme
• • @nand: NAND chip device structure
• • @ecc_scheme: ecc scheme to configure
• • @pagesize: number of main-area bytes per page of NAND device
• • @oobsize: number of OOB/spare bytes per page of NAND device
• */

[snip]

...

• default:

• debug("nand: error: ecc scheme not enabled or
  

supported\n");

...

• return -EINVAL;
  

• }

This will result in NAND_ECC_NONE and likely an eventual NULL pointer dereference if a bad value is passed and the -EINVAL return path is taken.

OK, I now see that you've got the caller patching this up in the error case, but that's awkward and error prone.

Sorry, din't understand your feedback here. omap_select_ecc_scheme() is common function which is used for - changing ECC scheme if 'nandecc' like utility is needed later - selecting ECC scheme during probe. This function populates all necessary fields in struct *nand_chip for nand driver to work. During probe (both SPL and u-boot), 'CONFIG_SYS_NAND_ECCSCHEME' is used to determine the ecc-scheme.

'default' statement protects the driver from any invalid or un-supported ECC scheme selected by user in his board file. So that driver probe cleanly exits probe instead of causing an exception. Can you please elaborate, what I'm missing here ?

...

@@ -767,67 +811,41 @@ void omap_nand_switch_ecc(uint32_t hardware,

uint32_t eccstrength)

...

{ struct nand_chip *nand; struct mtd_info *mtd;

• struct nand_bch_priv *bch;

• int err = 0;

  if (nand_curr_device < 0 || nand_curr_device >= CONFIG_SYS_MAX_NAND_DEVICE || !nand_info[nand_curr_device].name) {

• printf("Error: Can't switch ecc, no devices available\n");
  

• printf("nand: error: no devices available\n");
  

  return; }

Why are you making the error message less specific?

There is already another error message in board_nand_init() which will be flagged when there is problem in selecting ECC. printf("nand: error: cannot select ecc scheme\n"); But thanks, you pointed out a bug, I should return -ENODEV here, Otherwise caller would not know that ecc-scheme could not be changed.

...

diff --git a/include/configs/tricorder.h b/include/configs/tricorder.h index a9b2714..2b01486 100644 --- a/include/configs/tricorder.h +++ b/include/configs/tricorder.h @@ -298,6 +298,8 @@

#define CONFIG_SYS_NAND_ECCSIZE 512 #define CONFIG_SYS_NAND_ECCBYTES 13 +#define CONFIG_SYS_NAND_ECCSCHEME 3 +#define CONFIG_SYS_NAND_ONFI_DETECTION

What does 3 mean? Please use a symbolic name.

For AM335x this is updated in board/ti/am335x/README as part of following patch: [PATCH v5 5/5] board/ti/am335x/README: update for NAND boot but would it be better to use strings like "bch8" as mentioned above ?

with regards, pekon