diff mbox

[U-Boot,1/2] mtd: denali: use CONFIG_SYS_NAND_SELF_INIT

Message ID 1415711151-15237-2-git-send-email-yamada.m@jp.panasonic.com
State Superseded
Delegated to: Scott Wood
Headers show

Commit Message

Masahiro Yamada Nov. 11, 2014, 1:05 p.m. UTC 
Some variants of the Denali NAND controller need some registers
set up based on the device information that has been detected during
nand_scan_ident().

CONFIG_SYS_NAND_SELF_INIT has to be defined to insert code between
nand_scan_ident() and nand_scan_tail().  It is also helpful to reduce
the difference between this driver and its Linux counterpart because
this driver was ported from Linux.  Moreover, doc/README.nand recommends
to use CONFIG_SYS_NAND_SELF_INIT.

Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
Cc: Scott Wood <scottwood@freescale.com>
Cc: Chin Liang See <clsee@altera.com>
---

 drivers/mtd/nand/Kconfig  |   7 +++
 drivers/mtd/nand/denali.c | 120 ++++++++++++++++++++++++++++++++--------------
 drivers/mtd/nand/denali.h |   5 +-
 3 files changed, 92 insertions(+), 40 deletions(-)

Comments

Scott Wood Nov. 12, 2014, 3:06 a.m. UTC | #1 
On Tue, 2014-11-11 at 22:05 +0900, Masahiro Yamada wrote:
> +	/*
> +	 * If CONFIG_SYS_NAND_SELF_INIT is defined, each driver is responsible
> +	 * for instantiating struct nand_chip, while drivers/mtd/nand/nand.c
> +	 * still provides a "struct mtd_info nand_info" instance.
> +	 */
> +	denali->mtd = nand_info;

&nand_info[0] would be clearer.

> +	/*
> +	 * In the future, these base addresses should be taken from
> +	 * Device Tree or platform data.
> +	 */
> +	denali->flash_reg = (void  __iomem *)CONFIG_SYS_NAND_REGS_BASE;
> +	denali->flash_mem = (void  __iomem *)CONFIG_SYS_NAND_DATA_BASE;
> +
> +	return denali_init(denali);
>  }
>  
> -int board_nand_init(struct nand_chip *chip)
> +void board_nand_init(void)
>  {
> -	return denali_nand_init(chip);
> +	__board_nand_init();
>  }

Why do you need this wrapper rather than putting the contents of
__board_nand_init() here?  

Also, you might want to print an error if denali_init() returns an
error, rather than just discarding it (or, make sure denali_init()
prints for error conditions, and have it return void).  I realize that
the existing self-init drivers aren't perfect in this regard. :-)

-Scott
Masahiro Yamada Nov. 12, 2014, 3:06 p.m. UTC | #2 
Hi Scott,


2014-11-12 12:06 GMT+09:00 Scott Wood <scottwood@freescale.com>:
> On Tue, 2014-11-11 at 22:05 +0900, Masahiro Yamada wrote:
>> +     /*
>> +      * If CONFIG_SYS_NAND_SELF_INIT is defined, each driver is responsible
>> +      * for instantiating struct nand_chip, while drivers/mtd/nand/nand.c
>> +      * still provides a "struct mtd_info nand_info" instance.
>> +      */
>> +     denali->mtd = nand_info;
>
> &nand_info[0] would be clearer.

OK, I will fix it.



>> +     /*
>> +      * In the future, these base addresses should be taken from
>> +      * Device Tree or platform data.
>> +      */
>> +     denali->flash_reg = (void  __iomem *)CONFIG_SYS_NAND_REGS_BASE;
>> +     denali->flash_mem = (void  __iomem *)CONFIG_SYS_NAND_DATA_BASE;
>> +
>> +     return denali_init(denali);
>>  }
>>
>> -int board_nand_init(struct nand_chip *chip)
>> +void board_nand_init(void)
>>  {
>> -     return denali_nand_init(chip);
>> +     __board_nand_init();
>>  }
>
> Why do you need this wrapper rather than putting the contents of
> __board_nand_init() here?

I'd like to return -ENOMEM rather than
printing "No memory" without return code.

"int __board_nand_init(void)" is a function where I write my best code
and "void board_nand_init(void)" is a wrapper to adjust it into a frame work.


> Also, you might want to print an error if denali_init() returns an
> error, rather than just discarding it (or, make sure denali_init()
> prints for error conditions, and have it return void).  I realize that
> the existing self-init drivers aren't perfect in this regard. :-)

I will print an error message in v2.
diff mbox

Patch

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 75c2c06..c242214 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -1,9 +1,16 @@ 
 menu "NAND Device Support"
 
+config SYS_NAND_SELF_INIT
+	bool
+	help
+	  This option, if enabled, provides more flexible and linux-like
+	  NAND initialization process.
+
 if !SPL_BUILD
 
 config NAND_DENALI
 	bool "Support Denali NAND controller"
+	select SYS_NAND_SELF_INIT
 	help
 	  Enable support for the Denali NAND controller.
 
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 308b784..a9838d8 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -44,7 +44,7 @@  static int onfi_timing_mode = NAND_DEFAULT_TIMINGS;
  * this macro allows us to convert from an MTD structure to our own
  * device context (denali) structure.
  */
-#define mtd_to_denali(m) (((struct nand_chip *)mtd->priv)->priv)
+#define mtd_to_denali(m) container_of(m->priv, struct denali_nand_info, nand)
 
 /* These constants are defined by the driver to enable common driver
  * configuration options. */
@@ -1144,70 +1144,116 @@  static void denali_hw_init(struct denali_nand_info *denali)
 
 static struct nand_ecclayout nand_oob;
 
-static int denali_nand_init(struct nand_chip *nand)
+static int denali_init(struct denali_nand_info *denali)
 {
-	struct denali_nand_info *denali;
+	int ret;
 
-	denali = malloc(sizeof(*denali));
-	if (!denali)
-		return -ENOMEM;
+	denali_hw_init(denali);
 
-	nand->priv = denali;
+	denali->mtd->name = "denali-nand";
+	denali->mtd->owner = THIS_MODULE;
+	denali->mtd->priv = &denali->nand;
 
-	denali->flash_reg = (void  __iomem *)CONFIG_SYS_NAND_REGS_BASE;
-	denali->flash_mem = (void  __iomem *)CONFIG_SYS_NAND_DATA_BASE;
+	/* register the driver with the NAND core subsystem */
+	denali->nand.select_chip = denali_select_chip;
+	denali->nand.cmdfunc = denali_cmdfunc;
+	denali->nand.read_byte = denali_read_byte;
+	denali->nand.read_buf = denali_read_buf;
+	denali->nand.waitfunc = denali_waitfunc;
+
+	/*
+	 * scan for NAND devices attached to the controller
+	 * this is the first stage in a two step process to register
+	 * with the nand subsystem
+	 */
+	if (nand_scan_ident(denali->mtd, denali->max_banks, NULL)) {
+		ret = -ENXIO;
+		goto fail;
+	}
 
 #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
 	/* check whether flash got BBT table (located at end of flash). As we
 	 * use NAND_BBT_NO_OOB, the BBT page will start with
 	 * bbt_pattern. We will have mirror pattern too */
-	nand->bbt_options |= NAND_BBT_USE_FLASH;
+	denali->nand.bbt_options |= NAND_BBT_USE_FLASH;
 	/*
 	 * We are using main + spare with ECC support. As BBT need ECC support,
 	 * we need to ensure BBT code don't write to OOB for the BBT pattern.
 	 * All BBT info will be stored into data area with ECC support.
 	 */
-	nand->bbt_options |= NAND_BBT_NO_OOB;
+	denali->nand.bbt_options |= NAND_BBT_NO_OOB;
 #endif
 
-	nand->ecc.mode = NAND_ECC_HW;
-	nand->ecc.size = CONFIG_NAND_DENALI_ECC_SIZE;
-	nand->ecc.read_oob = denali_read_oob;
-	nand->ecc.write_oob = denali_write_oob;
-	nand->ecc.read_page = denali_read_page;
-	nand->ecc.read_page_raw = denali_read_page_raw;
-	nand->ecc.write_page = denali_write_page;
-	nand->ecc.write_page_raw = denali_write_page_raw;
+	denali->nand.ecc.mode = NAND_ECC_HW;
+	denali->nand.ecc.size = CONFIG_NAND_DENALI_ECC_SIZE;
+
 	/*
 	 * Tell driver the ecc strength. This register may be already set
 	 * correctly. So we read this value out.
 	 */
-	nand->ecc.strength = readl(denali->flash_reg + ECC_CORRECTION);
-	switch (nand->ecc.size) {
+	denali->nand.ecc.strength = readl(denali->flash_reg + ECC_CORRECTION);
+	switch (denali->nand.ecc.size) {
 	case 512:
-		nand->ecc.bytes = (nand->ecc.strength * 13 + 15) / 16 * 2;
+		denali->nand.ecc.bytes =
+			(denali->nand.ecc.strength * 13 + 15) / 16 * 2;
 		break;
 	case 1024:
-		nand->ecc.bytes = (nand->ecc.strength * 14 + 15) / 16 * 2;
+		denali->nand.ecc.bytes =
+			(denali->nand.ecc.strength * 14 + 15) / 16 * 2;
 		break;
 	default:
 		pr_err("Unsupported ECC size\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto fail;
 	}
-	nand_oob.eccbytes = nand->ecc.bytes;
-	nand->ecc.layout = &nand_oob;
-
-	/* Set address of hardware control function */
-	nand->cmdfunc = denali_cmdfunc;
-	nand->read_byte = denali_read_byte;
-	nand->read_buf = denali_read_buf;
-	nand->select_chip = denali_select_chip;
-	nand->waitfunc = denali_waitfunc;
-	denali_hw_init(denali);
-	return 0;
+	nand_oob.eccbytes = denali->nand.ecc.bytes;
+	denali->nand.ecc.layout = &nand_oob;
+
+	/* override the default operations */
+	denali->nand.ecc.read_page = denali_read_page;
+	denali->nand.ecc.read_page_raw = denali_read_page_raw;
+	denali->nand.ecc.write_page = denali_write_page;
+	denali->nand.ecc.write_page_raw = denali_write_page_raw;
+	denali->nand.ecc.read_oob = denali_read_oob;
+	denali->nand.ecc.write_oob = denali_write_oob;
+
+	if (nand_scan_tail(denali->mtd)) {
+		ret = -ENXIO;
+		goto fail;
+	}
+
+	ret = nand_register(0);
+
+fail:
+	return ret;
+}
+
+static int __board_nand_init(void)
+{
+	struct denali_nand_info *denali;
+
+	denali = kzalloc(sizeof(*denali), GFP_KERNEL);
+	if (!denali)
+		return -ENOMEM;
+
+	/*
+	 * If CONFIG_SYS_NAND_SELF_INIT is defined, each driver is responsible
+	 * for instantiating struct nand_chip, while drivers/mtd/nand/nand.c
+	 * still provides a "struct mtd_info nand_info" instance.
+	 */
+	denali->mtd = nand_info;
+
+	/*
+	 * In the future, these base addresses should be taken from
+	 * Device Tree or platform data.
+	 */
+	denali->flash_reg = (void  __iomem *)CONFIG_SYS_NAND_REGS_BASE;
+	denali->flash_mem = (void  __iomem *)CONFIG_SYS_NAND_DATA_BASE;
+
+	return denali_init(denali);
 }
 
-int board_nand_init(struct nand_chip *chip)
+void board_nand_init(void)
 {
-	return denali_nand_init(chip);
+	__board_nand_init();
 }
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index 3277da7..a258df0 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -434,9 +434,8 @@  struct nand_buf {
 #define DT		3
 
 struct denali_nand_info {
-	struct mtd_info mtd;
-	struct nand_chip *nand;
-
+	struct mtd_info *mtd;
+	struct nand_chip nand;
 	int flash_bank; /* currently selected chip */
 	int status;
 	int platform;