Re: [U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver

Hi, Thanks for the reply. This is my first patch to u-boot. Any advice is appreciated. Comments in-line below.

----- Original Message ----

From: Scott Wood scottwood@freescale.com To: Chris Kiick ckiick@att.net Cc: u-boot@lists.denx.de Sent: Wed, December 19, 2012 1:02:52 PM Subject: Re: [U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver

On 12/18/2012 05:27:00 PM, Chris Kiick wrote:

...

Allow boards to set their own ECC layouts and functions in NAND_PLAT_INIT without being stomped on by nand_base.c intialization.

Signed-off-by: ckiick <chris at kiicks.net>

drivers/mtd/nand/nand_base.c | 11 +++++++---- drivers/mtd/nand/nand_plat.c | 4 ++-- include/configs/qong.h | 2 +- 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index a2d06be..614fc72 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -3035,8 +3035,10 @@ int nand_scan_tail(struct mtd_info *mtd) chip->ecc.mode = NAND_ECC_SOFT;

 case  NAND_ECC_SOFT:

•    chip->ecc.calculate =  nand_calculate_ecc;
  

•    chip->ecc.correct =  nand_correct_data;
  

•    if  (!chip->ecc.calculate)
  

•         chip->ecc.calculate = nand_calculate_ecc;
  

•     if (!chip->ecc.correct)
  

•         chip->ecc.correct = nand_correct_data;
      chip->ecc.read_page = nand_read_page_swecc;
      chip->ecc.read_subpage =  nand_read_subpage;
      chip->ecc.write_page = nand_write_page_swecc;
  

@@ -3044,9 +3046,10 @@ int nand_scan_tail(struct mtd_info *mtd) chip->ecc.write_page_raw = nand_write_page_raw; chip->ecc.read_oob = nand_read_oob_std; chip->ecc.write_oob = nand_write_oob_std;

•    if (!chip->ecc.size)
  

•     if (!chip->ecc.size) {
          chip->ecc.size = 256;
  

•     chip->ecc.bytes = 3;
  

•         chip->ecc.bytes = 3;
  

•     }
     break;
  
  case NAND_ECC_SOFT_BCH:
  

How is this part specific to nand plat?

OK, it's not, but if you are using nand plat, then you are forced to go through this code path with no chance of making any changes after because of the way board_nand_init() is written. I seem to see other nand drivers setting up ecc layouts and then calling nand_scan_tail(), I'm not sure how they are getting around this. I reasoned that the change was safe for existing code because if something was not setting its own ecc layout, it would still use the default, but it if was, then it had to be after the call to nand_scan_tail() and that would override whatever was set there anyway.

I'm not sure how specifying your own ECC functions fits with the purpose of either NAND_ECC_SOFT or nand plat.

Well, NAND_ECC_SOFT is the only scheme that does fit with custom ECC algorithms. You have to pick one of the existing ECC schemes, and SOFT is the scheme that allows you to do your own ECC, if you setup the layout, calculate and correct parts. Looking at the code, that's what I thought it was for.

Is there another way to implement custom ECC algorithms, done in software?

As for the plat driver, it shouldn't care what ECC I'm using. It's just running the low-level byte-bang part of the driver for me, so I don't have to duplicate the code. Isn't that its purpose? Do I have to re-write a driver interface that does the same thing as nand plat just because my ECC isn't the default?

If I'm going in the wrong direction, I'd appreciate advice on how it's supposed to be done.

...

diff --git a/drivers/mtd/nand/nand_plat.c b/drivers/mtd/nand/nand_plat.c index 37a0206..b3bda11 100644 --- a/drivers/mtd/nand/nand_plat.c +++ b/drivers/mtd/nand/nand_plat.c @@ -8,7 +8,7 @@ /* Your board must implement the following macros:

• NAND_PLAT_WRITE_CMD(chip, cmd)
• NAND_PLAT_WRITE_ADR(chip, cmd)

• • NAND_PLAT_INIT()

• • NAND_PLAT_INIT(nand)
  • It may also implement the following:
  • NAND_PLAT_DEV_READY(chip)

@@ -53,7 +53,7 @@ int board_nand_init(struct nand_chip *nand) #endif

#ifdef NAND_PLAT_INIT

• NAND_PLAT_INIT();
  

• NAND_PLAT_INIT(nand); #endif

  nand->cmd_ctrl = plat_cmd_ctrl;

diff --git a/include/configs/qong.h b/include/configs/qong.h index d9bf201..077cbae 100644 --- a/include/configs/qong.h +++ b/include/configs/qong.h @@ -226,7 +226,7 @@ extern int qong_nand_rdy(void *chip); #define CONFIG_NAND_PLAT #define CONFIG_SYS_MAX_NAND_DEVICE 1 #define CONFIG_SYS_NAND_BASE CS3_BASE -#define NAND_PLAT_INIT() qong_nand_plat_init(nand) +#define NAND_PLAT_INIT(nand) qong_nand_plat_init(nand)

#define QONG_NAND_CLE(chip) ((unsigned long)(chip)->IO_ADDR_W | (1 <<

24))

...

#define QONG_NAND_ALE(chip) ((unsigned long)(chip)->IO_ADDR_W | (1 << 23))

This part looks unrelated.

It follows as a logical consequence of the other change. If NAND_PLAT_INIT function is going to make changes to the nand chip structure (which it would have to do to setup the ECC), then it should be an explicit parameter. And if that macro is changed, then all the boards that use it have to follow. Which in this case is just the qong board, which was assuming it could do that anyway. I'm really not sure why NAND_PLAT_INIT() doesn't have that parameter already.

So the reason for all this is... I have this board. I'm sure you hear that a lot. The VAR for the device decided (who knows why) to use this extended ECC algorithm instead of the hardware supported one. They originally stuffed it into u-boot 1.1.2 with a crude hack that pretty much ignored the interface and changed the base code, so it would never be compatible with anything else. Now I'm trying to move up to a better version of u-boot, and I'm stuck with the legacy ECC. Eventually it would be nice to get the board code added to u-boot, so I'm trying to pave the way with some more generic changes that it depends on. And avoid adding anything to the base code that would be dead code on any other board. This is the minimal change to u-boot that would still allow me to support this board.

Anyway, let me know what you think. I'm open to any suggestions.

Thanks,

-Scott

-- Chris J. Kiick chris@kiicks.net