Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver

On Fri, Sep 14, 2012 at 08:21:11PM +0200, Marek Vasut wrote:

Dear José Miguel Gonçalves,

...

NAND Flash driver with HW ECC for the S3C24XX SoCs. Currently it only supports SLC NAND chips.

Signed-off-by: José Miguel Gonçalves jose.goncalves@inov.pt

[...]

...

+#include <common.h> +#include <nand.h> +#include <asm/io.h> +#include <asm/arch/s3c24xx_cpu.h> +#include <asm/errno.h>

+#define MAX_CHIPS 2 +static int nand_cs[MAX_CHIPS] = { 0, 1 };

+#ifdef CONFIG_SPL_BUILD +#define printf(arg...) do {} while (0)

This doesn't seem quite right ...

1. this should be in CPU directory
2. should be enabled only if CONFIG_SPL_SERIAL_SUPPORT is not set
3. should be inline function, not a macro

What specifically should be in the CPU directory?

...

+#endif

+static void s3c_nand_select_chip(struct mtd_info *mtd, int chip) +{

• struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
• u_long nfcont;
• nfcont = readl(&nand->nfcont);
• switch (chip) {
• case -1:

• nfcont |= NFCONT_NCE1 | NFCONT_NCE0;
  

• break;
  

• case 0:

• nfcont &= ~NFCONT_NCE0;
  

• break;
  

• case 1:

• nfcont &= ~NFCONT_NCE1;
  

• break;
  

• default:

• return;
  

• }
• writel(nfcont, &nand->nfcont);

+}

+/*

• • Hardware specific access to control-lines function
• */

+static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl) +{

• struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
• struct nand_chip *this = mtd->priv;
• if (ctrl & NAND_CTRL_CHANGE) {

• if (ctrl & NAND_CLE)
  

• 	this->IO_ADDR_W = &nand->nfcmmd;
  

• else if (ctrl & NAND_ALE)
  

• 	this->IO_ADDR_W = &nand->nfaddr;
  

• else
  

• 	this->IO_ADDR_W = &nand->nfdata;
  

Why don't you use local variable here?

Because then you'll access the wrong data when the function is called again without NAND_CTRL_CHANGE. This is a common way of writing the hwcontrol function, though the way ndfc.c does it is simpler (you can use a local variable if you ignore NAND_CTRL_CHANGE altogether).

...

• if (ctrl & NAND_NCE)
  

• 	s3c_nand_select_chip(mtd, *(int *)this->priv);
  

Uh, how's this *(int *) supposed to work?

Usually driver-private data is a struct; apparently in this driver it's an int.

-Scott