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

2014-11-12 Thread Masahiro YAMADA
Hi Scott,


2014-11-12 12:06 GMT+09:00 Scott Wood scottw...@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.



-- 
Best Regards
Masahiro Yamada
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2014-11-11 Thread Masahiro Yamada
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 yamad...@jp.panasonic.com
Cc: Scott Wood scottw...@freescale.com
Cc: Chin Liang See cl...@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(-)

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;
+   

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

2014-11-11 Thread Scott Wood
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


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot