[Please CC me at this address rather than my NXP address]

On Fri, 2016-04-01 at 16:59 +0530, Mugunthan V N wrote:

@@ -432,12 +435,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) * one before these commands can run, even if a partition specifier * for another device is to be used. */

• if (dev < 0 || dev >= CONFIG_SYS_MAX_NAND_DEVICE ||

•    !nand_info[dev].name) {
  

• puts("\nno devices available\n");
  

• return 1;
  

• }
• nand = &nand_info[dev];

• nand = get_nand_dev_by_index(dev);

  if (strcmp(cmd, "bad") == 0) {

You eliminated the error check -- now a NULL deref is likely if a bad dev is requested. Even if it's checked elsewhere when setting nand_curr_device, it's possible that the initial default is bad (no NAND devices present, or device 0 failed).

printf("\nDevice %d bad blocks:\n", dev);

@@ -496,13 +494,13 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) /* skip first two or three arguments, look for offset and size */ if (mtd_arg_off_size(argc - o, argv + o, &dev, &off, &size, &maxsize, MTD_DEV_TYPE_NAND,

• 		     nand_info[dev].size) != 0)
  

• 		     nand->size) != 0)
  return 1;
  
  

  if (set_dev(dev)) return 1;

• nand = &nand_info[dev];
  

• nand = get_nand_dev_by_index(dev);
  

Maybe have set_dev return the dev pointer?

Or have a global for the pointer rather than just the index, saving a bunch of these calls.

-Scott

On Friday 01 April 2016 04:59 PM, Mugunthan V N wrote:

nand_info is used all over the file so abstract it with get_nand_dev_by_index() which will help for DM conversion.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

cmd/nand.c | 72 ++++++++++++++++++++++++++----------------------- drivers/mtd/nand/nand.c | 8 ++++-- include/nand.h | 18 +++++++++++++ 3 files changed, 62 insertions(+), 36 deletions(-)

diff --git a/cmd/nand.c b/cmd/nand.c index a6b67e2..ffb8d43 100644 --- a/cmd/nand.c +++ b/cmd/nand.c @@ -114,21 +114,20 @@ free_dat:

static int set_dev(int dev) {

• if (dev < 0 || dev >= CONFIG_SYS_MAX_NAND_DEVICE ||

•    !nand_info[dev].name) {
  

• puts("No such device\n");
  

• return -1;
  

• }

• nand_info_t *nand = get_nand_dev_by_index(dev);

• if (!nand)

• return -ENODEV;
  
  

  if (nand_curr_device == dev) return 0;

• printf("Device %d: %s", dev, nand_info[dev].name);

• printf("Device %d: %s", dev, nand->name); puts("... is now current device\n"); nand_curr_device = dev;

#ifdef CONFIG_SYS_NAND_SELECT_DEVICE

• board_nand_select_device(nand_info[dev].priv, dev);

• board_nand_select_device(nand->priv, dev);

#endif

return 0; @@ -188,7 +187,7 @@ int do_nand_env_oob(cmd_tbl_t *cmdtp, int argc, char *const argv[]) { int ret; uint32_t oob_buf[ENV_OFFSET_SIZE/sizeof(uint32_t)];

• nand_info_t *nand = &nand_info[0];

• nand_info_t *nand = get_nand_dev_by_index(0); char *cmd = argv[1];

  if (CONFIG_SYS_MAX_NAND_DEVICE == 0 || !nand->name) {

@@ -213,9 +212,10 @@ int do_nand_env_oob(cmd_tbl_t *cmdtp, int argc, char *const argv[]) if (argc < 3) goto usage;

• nand = get_nand_dev_by_index(idx);
  

  /* We don't care about size, or maxsize. */ if (mtd_arg_off(argv[2], &idx, &addr, &maxsize, &maxsize,

• 		MTD_DEV_TYPE_NAND, nand_info[idx].size)) {
  

• 		MTD_DEV_TYPE_NAND, nand->size)) {
  puts("Offset or partition name expected\n");
  return 1;
  

  }

@@ -283,9 +283,14 @@ usage:

static void nand_print_and_set_info(int idx) {

• nand_info_t *nand = &nand_info[idx];
• struct nand_chip *chip = nand->priv;

• nand_info_t *nand;

• struct nand_chip *chip;

• nand = get_nand_dev_by_index(idx);

• if (!nand)

• return;
  

• chip = nand->priv; printf("Device %d: ", idx); if (chip->numchips > 1) printf("%dx ", chip->numchips);

@@ -348,7 +353,7 @@ static void adjust_size_for_badblocks(loff_t *size, loff_t offset, int dev) /* We grab the nand info object here fresh because this is usually * called after arg_off_size() which can change the value of dev. */

• nand_info_t *nand = &nand_info[dev];

• nand_info_t *nand = get_nand_dev_by_index(dev); loff_t maxoffset = offset + *size; int badblocks = 0;

@@ -397,10 +402,8 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (strcmp(cmd, "info") == 0) {

putc('\n');

• for (i = 0; i < CONFIG_SYS_MAX_NAND_DEVICE; i++) {
  

• 	if (nand_info[i].name)
  

• 		nand_print_and_set_info(i);
  

• }
  

• for (i = 0; i < CONFIG_SYS_MAX_NAND_DEVICE; i++)
  

• 	nand_print_and_set_info(i);
  

  return 0; }

@@ -432,12 +435,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) * one before these commands can run, even if a partition specifier * for another device is to be used. */

• if (dev < 0 || dev >= CONFIG_SYS_MAX_NAND_DEVICE ||

•    !nand_info[dev].name) {
  

• puts("\nno devices available\n");
  

• return 1;
  

• }
• nand = &nand_info[dev];

• nand = get_nand_dev_by_index(dev);

  if (strcmp(cmd, "bad") == 0) { printf("\nDevice %d bad blocks:\n", dev);

@@ -496,13 +494,13 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) /* skip first two or three arguments, look for offset and size */ if (mtd_arg_off_size(argc - o, argv + o, &dev, &off, &size, &maxsize, MTD_DEV_TYPE_NAND,

• 		     nand_info[dev].size) != 0)
  

• 		     nand->size) != 0)
  return 1;
  
  

  if (set_dev(dev)) return 1;

• nand = &nand_info[dev];
  

• nand = get_nand_dev_by_index(dev);
  
  

  memset(&opts, 0, sizeof(opts)); opts.offset = off;

@@ -561,13 +559,13 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])

	if (mtd_arg_off(argv[3], &dev, &off, &size, &maxsize,
			MTD_DEV_TYPE_NAND,

• 			nand_info[dev].size))
  

• 			nand->size))
  	return 1;
  
  if (set_dev(dev))
  	return 1;
  
  

• 	nand = &nand_info[dev];
  

• 	nand = get_nand_dev_by_index(dev);
  
  if (argc > 4 && !str2long(argv[4], &pagecount)) {
  	printf("'%s' is not a number\n", argv[4]);
  

@@ -584,7 +582,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (mtd_arg_off_size(argc - 3, argv + 3, &dev, &off, &size, &maxsize, MTD_DEV_TYPE_NAND,

• 			     nand_info[dev].size) != 0)
  

• 			     nand->size) != 0)
  	return 1;
  
  if (set_dev(dev))
  

@@ -596,7 +594,7 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) rwsize = size; }

• nand = &nand_info[dev];
  

• nand = get_nand_dev_by_index(dev);
  
  

  if (!s || !strcmp(s, ".jffs2") || !strcmp(s, ".e") || !strcmp(s, ".i")) {

@@ -727,13 +725,15 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])

if (mtd_arg_off_size(argc - 2, argv + 2, &dev, &off, &size,
		     &maxsize, MTD_DEV_TYPE_NAND,

• 		     nand_info[dev].size) < 0)
  

• 		     nand->size) < 0)
  return 1;
  
  

  if (set_dev(dev)) return 1;

• if (!nand_unlock(&nand_info[dev], off, size, allexcept)) {
  

• nand = get_nand_dev_by_index(dev);
  

• if (!nand_unlock(nand, off, size, allexcept)) {
  puts("NAND flash successfully unlocked\n");
  

  } else { puts("Error unlocking NAND flash, "

@@ -899,6 +899,7 @@ static int do_nandboot(cmd_tbl_t *cmdtp, int flag, int argc, struct mtd_device *dev; struct part_info *part; u8 pnum;

• nand_info_t *nand;

  if (argc >= 2) { char *p = (argc == 2) ? argv[1] : argv[2];

@@ -914,8 +915,10 @@ static int do_nandboot(cmd_tbl_t *cmdtp, int flag, int argc, addr = simple_strtoul(argv[1], NULL, 16); else addr = CONFIG_SYS_LOAD_ADDR;

• 	return nand_load_image(cmdtp, &nand_info[dev->id->num],
  

• 			       part->offset, addr, argv[0]);
  

• 	nand = get_nand_dev_by_index(dev->id->num);
  

• 	return nand_load_image(cmdtp, nand, part->offset,
  

• 			       addr, argv[0]);
  

  } }

#endif @@ -957,14 +960,15 @@ usage:

idx = simple_strtoul(boot_device, NULL, 16);

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

• nand = get_nand_dev_by_index(idx);
• if (!nand) { printf("\n** Device %d not available\n", idx); bootstage_error(BOOTSTAGE_ID_NAND_AVAILABLE); return 1; } bootstage_mark(BOOTSTAGE_ID_NAND_AVAILABLE);

• return nand_load_image(cmdtp, &nand_info[idx], offset, addr, argv[0]);

• return nand_load_image(cmdtp, nand, offset, addr, argv[0]);

}

U_BOOT_CMD(nboot, 4, 1, do_nandboot, diff --git a/drivers/mtd/nand/nand.c b/drivers/mtd/nand/nand.c index 8f0a921..524ead3 100644 --- a/drivers/mtd/nand/nand.c +++ b/drivers/mtd/nand/nand.c @@ -38,7 +38,7 @@ int nand_register(int devnum) if (devnum >= CONFIG_SYS_MAX_NAND_DEVICE) return -EINVAL;

• mtd = &nand_info[devnum];

• mtd = get_nand_dev_by_index(devnum);

  sprintf(dev_name[devnum], "nand%d", devnum); mtd->name = dev_name[devnum];

@@ -62,7 +62,7 @@ int nand_register(int devnum) #ifndef CONFIG_SYS_NAND_SELF_INIT static void nand_init_chip(int i) {

• struct mtd_info *mtd = &nand_info[i];

• struct mtd_info *mtd; struct nand_chip *nand = &nand_chip[i]; ulong base_addr = base_address[i]; int maxchips = CONFIG_SYS_NAND_MAX_CHIPS;

@@ -70,6 +70,10 @@ static void nand_init_chip(int i) if (maxchips < 1) maxchips = 1;

• mtd = get_nand_dev_by_index(i);
• if (!mtd)

• return;
  

Found an issue here, in non-dm case get_nand_dev_by_index() will return NULL as this is the first access and none of the nand_info[] is initialized. below is the fix. Will include this in with v2 series

+#ifdef CONFIG_DM_NAND + mtd = get_nand_dev_by_index(i); + if (!mtd) + return; +#else + mtd = &nand_info[i]; +#endif +

Regards Mugunthan V N

• mtd->priv = nand; nand->IO_ADDR_R = nand->IO_ADDR_W = (void __iomem *)base_addr;

diff --git a/include/nand.h b/include/nand.h index 4ea24ac..23b2414 100644 --- a/include/nand.h +++ b/include/nand.h @@ -137,4 +137,22 @@ int get_nand_env_oob(nand_info_t *nand, unsigned long *result); #endif int spl_nand_erase_one(int block, int page);

+/*

• • get_nand_dev_by_index - Get the nand info based in index.
• • @dev - index to the nand device.
• • returns pointer to the nand device info structure or NULL on failure.
• */

+static inline nand_info_t *get_nand_dev_by_index(int dev) +{

• if (dev < 0 || dev >= CONFIG_SYS_MAX_NAND_DEVICE ||

•    !nand_info[dev].name) {
  

• puts("No such device\n");
  

• return NULL;
  

• }
• return &nand_info[dev];

+}

#endif /* _NAND_H_ */

On Fri, Apr 01, 2016 at 04:59:45PM +0530, Mugunthan V N wrote:

Implement a NAND uclass so that the NAND devices can be accessed via the DM framework.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

Minor:

@@ -0,0 +1,62 @@ +/*

• • NAND uclass driver for NAND bus.
• • (C) Copyright 2012-2016

• • Texas Instruments Incorporated, <www.ti.com>
    

The code isn't that old.

On Fri, 2016-04-01 at 16:59 +0530, Mugunthan V N wrote:

+static int nand_child_pre_probe(struct udevice *dev) +{

• nand_info_t *nand = dev_get_uclass_priv(dev);
• void *priv = dev_get_priv(dev);
• /*

• * Store nand device priv pointer in nand_info so that
  

• * it can be used by nand command
  

• */
  

• nand->priv = priv;

Wouldn't it make more sense to have a pointer to the device in the NAND struct, and let the driver manage both privs as it chooses?

• return 0;

+}

+UCLASS_DRIVER(nand) = {

• .id = UCLASS_NAND,
• .name = "nand",
• .flags = DM_UC_FLAG_SEQ_ALIAS,
• .post_probe = nand_child_pre_probe,
• .per_device_auto_alloc_size = sizeof(nand_info_t),

+};

Post probe = pre probe?

@@ -63,8 +63,10 @@ int nand_register(int devnum) static void nand_init_chip(int i) { struct mtd_info *mtd; +#ifndef CONFIG_DM_NAND struct nand_chip *nand = &nand_chip[i]; ulong base_addr = base_address[i]; +#endif int maxchips = CONFIG_SYS_NAND_MAX_CHIPS;

if (maxchips < 1) @@ -74,11 +76,13 @@ static void nand_init_chip(int i) if (!mtd) return;

+#ifndef CONFIG_DM_NAND mtd->priv = nand; nand->IO_ADDR_R = nand->IO_ADDR_W = (void __iomem *)base_addr;

if (board_nand_init(nand)) return; +#endif

if (nand_scan(mtd, maxchips)) return;

Please do not use this code for DM. Have drivers' probe functions call nand_scan (split into ident and tail if necessary) as is done with CONFIG_SYS_NAND_SELF_INIT.

diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 37c4176..6a88d39 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -49,6 +49,7 @@ enum uclass_id { UCLASS_MMC, /* SD / MMC card or chip */ UCLASS_MOD_EXP, /* RSA Mod Exp device */ UCLASS_MTD, /* Memory Technology Device (MTD) device */

• UCLASS_NAND, /* NAND bus */

NAND is not a bus.

+#ifndef CONFIG_DM_NAND static inline nand_info_t *get_nand_dev_by_index(int dev) { if (dev < 0 || dev >= CONFIG_SYS_MAX_NAND_DEVICE || @@ -154,5 +156,8 @@ static inline nand_info_t *get_nand_dev_by_index(int dev)

return &nand_info[dev]; } +#else +nand_info_t *get_nand_dev_by_index(int idx); +#endif

Please use "if X/else", not "if !X/else".

-Scott

On Wednesday 20 April 2016 08:09 PM, Simon Glass wrote:

Hi Mugunthan,

On 1 April 2016 at 05:29, Mugunthan V N mugunthanvnm@ti.com wrote:

...

Implement a NAND uclass so that the NAND devices can be accessed via the DM framework.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

drivers/mtd/nand/Kconfig | 10 +++++++ drivers/mtd/nand/Makefile | 2 ++ drivers/mtd/nand/nand-uclass.c | 62 ++++++++++++++++++++++++++++++++++++++++++ drivers/mtd/nand/nand.c | 6 +++- include/dm/uclass-id.h | 1 + include/nand.h | 5 ++++ 6 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 drivers/mtd/nand/nand-uclass.c

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index 7787505..53b57b8 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -8,6 +8,16 @@ menuconfig NAND

if NAND

+config DM_NAND

•   bool "Enable driver model for NAND"
  

•   depends on NAND && DM
  

•   help
  

•     Enable driver model for NAND. The NAND interface is then
  

•     implemented by the NAND uclass. Multiple NAND devices can
  

•     be attached and used. The 'nand' command works as normal.
  

•     If the NAND drivers doesn't support DM, say N.
  

config SYS_NAND_SELF_INIT bool help diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile index 6fb3718..7745705 100644 --- a/drivers/mtd/nand/Makefile +++ b/drivers/mtd/nand/Makefile @@ -39,6 +39,8 @@ endif # not spl

ifdef NORMAL_DRIVERS

+obj-$(CONFIG_DM_NAND) += nand-uclass.o

obj-$(CONFIG_NAND_ECC_BCH) += nand_bch.o

obj-$(CONFIG_NAND_ATMEL) += atmel_nand.o diff --git a/drivers/mtd/nand/nand-uclass.c b/drivers/mtd/nand/nand-uclass.c new file mode 100644 index 0000000..d68d357 --- /dev/null +++ b/drivers/mtd/nand/nand-uclass.c @@ -0,0 +1,62 @@ +/*

• • NAND uclass driver for NAND bus.
• • (C) Copyright 2012-2016

• • Texas Instruments Incorporated, <www.ti.com>
    

• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <dm.h> +#include <errno.h> +#include <nand.h>

+DECLARE_GLOBAL_DATA_PTR;

+#ifdef CONFIG_DM_NAND

+nand_info_t *get_nand_dev_by_index(int idx) +{

•   nand_info_t *nand;
  

•   struct udevice *dev;
  

•   int ret;
  

•   ret = uclass_get_device(UCLASS_NAND, idx, &dev);
  

•   if (ret) {
  

•           error("NAND device (%d) not found\n", idx);
  

This should return -ENODEV. Also please avoid printf() in drivers. The caller can report the error.

Return type is pointer so returned NULL and NULL denotes no dev. Will change this error to debug in v2.

...

•           return NULL;
  

•   }
  

•   nand = (nand_info_t *)dev_get_uclass_priv(dev);
  

•   if (!nand) {
  

•           error("Nand device not ready\n");
  

•           return NULL;
  

•   }
  

But if the device has been probed ((with uclass_get_device()) then this cannot be NULL.

This check is just a failsafe. ideally it should not fail here.

...

•   return nand;
  

+}

+static int nand_child_pre_probe(struct udevice *dev) +{

•   nand_info_t *nand = dev_get_uclass_priv(dev);
  

•   void *priv = dev_get_priv(dev);
  

Please use the actual struct here, not void.

nand->priv is a void *, so used void *

...

•   /*
  

•    * Store nand device priv pointer in nand_info so that
  

•    * it can be used by nand command
  

•    */
  

•   nand->priv = priv;
  

()> +

...

•   return 0;
  

+}

+UCLASS_DRIVER(nand) = {

•   .id                             = UCLASS_NAND,
  

•   .name                           = "nand",
  

•   .flags                          = DM_UC_FLAG_SEQ_ALIAS,
  

•   .post_probe             = nand_child_pre_probe,
  

•   .per_device_auto_alloc_size     = sizeof(nand_info_t),
  

+};

+#endif diff --git a/drivers/mtd/nand/nand.c b/drivers/mtd/nand/nand.c index 524ead3..c03a7e5 100644 --- a/drivers/mtd/nand/nand.c +++ b/drivers/mtd/nand/nand.c @@ -21,7 +21,7 @@ int nand_curr_device = -1;

nand_info_t nand_info[CONFIG_SYS_MAX_NAND_DEVICE];

-#ifndef CONFIG_SYS_NAND_SELF_INIT +#if !defined(CONFIG_SYS_NAND_SELF_INIT) && !defined(CONFIG_DM_NAND) static struct nand_chip nand_chip[CONFIG_SYS_MAX_NAND_DEVICE]; static ulong base_address[CONFIG_SYS_MAX_NAND_DEVICE] = CONFIG_SYS_NAND_BASE_LIST; #endif @@ -63,8 +63,10 @@ int nand_register(int devnum) static void nand_init_chip(int i) { struct mtd_info *mtd; +#ifndef CONFIG_DM_NAND struct nand_chip *nand = &nand_chip[i]; ulong base_addr = base_address[i]; +#endif int maxchips = CONFIG_SYS_NAND_MAX_CHIPS;

    if (maxchips < 1)

@@ -74,11 +76,13 @@ static void nand_init_chip(int i) if (!mtd) return;

+#ifndef CONFIG_DM_NAND mtd->priv = nand; nand->IO_ADDR_R = nand->IO_ADDR_W = (void __iomem *)base_addr;

    if (board_nand_init(nand))
            return;

+#endif

    if (nand_scan(mtd, maxchips))
            return;

diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 37c4176..6a88d39 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -49,6 +49,7 @@ enum uclass_id { UCLASS_MMC, /* SD / MMC card or chip */ UCLASS_MOD_EXP, /* RSA Mod Exp device */ UCLASS_MTD, /* Memory Technology Device (MTD) device */

•   UCLASS_NAND,            /* NAND bus */
  

Is 'bus' the right word? Perhaps 'device'?

Yeah, will fix this in v2

...

    UCLASS_NORTHBRIDGE,     /* Intel Northbridge / SDRAM controller */
    UCLASS_PANEL,           /* Display panel, such as an LCD */
    UCLASS_PANEL_BACKLIGHT, /* Backlight controller for panel */

diff --git a/include/nand.h b/include/nand.h index 23b2414..1580970 100644 --- a/include/nand.h +++ b/include/nand.h @@ -10,6 +10,7 @@ #define _NAND_H_

#include <config.h> +#include <dm.h>

/*

• All boards using a given driver must convert to self-init

@@ -144,6 +145,7 @@ int spl_nand_erase_one(int block, int page);

• returns pointer to the nand device info structure or NULL on failure.

*/ +#ifndef CONFIG_DM_NAND static inline nand_info_t *get_nand_dev_by_index(int dev) { if (dev < 0 || dev >= CONFIG_SYS_MAX_NAND_DEVICE || @@ -154,5 +156,8 @@ static inline nand_info_t *get_nand_dev_by_index(int dev)

    return &nand_info[dev];

} +#else +nand_info_t *get_nand_dev_by_index(int idx); +#endif

Can you please document in this header file what dev_get_priv() holds for a NAND device, and dev_get_uclass_priv().

Will document this in v2

Regards Mugunthan V N

On Thursday 21 April 2016 07:41 PM, Simon Glass wrote:

Hi Mugunthan,

On 20 April 2016 at 23:55, Mugunthan V N mugunthanvnm@ti.com wrote:

...

On Wednesday 20 April 2016 08:09 PM, Simon Glass wrote:

...

Hi Mugunthan,

On 1 April 2016 at 05:29, Mugunthan V N mugunthanvnm@ti.com wrote:

...

Implement a NAND uclass so that the NAND devices can be accessed via the DM framework.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

drivers/mtd/nand/Kconfig | 10 +++++++ drivers/mtd/nand/Makefile | 2 ++ drivers/mtd/nand/nand-uclass.c | 62 ++++++++++++++++++++++++++++++++++++++++++ drivers/mtd/nand/nand.c | 6 +++- include/dm/uclass-id.h | 1 + include/nand.h | 5 ++++ 6 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 drivers/mtd/nand/nand-uclass.c

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index 7787505..53b57b8 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -8,6 +8,16 @@ menuconfig NAND

if NAND

+config DM_NAND

•   bool "Enable driver model for NAND"
  

•   depends on NAND && DM
  

•   help
  

•     Enable driver model for NAND. The NAND interface is then
  

•     implemented by the NAND uclass. Multiple NAND devices can
  

•     be attached and used. The 'nand' command works as normal.
  

•     If the NAND drivers doesn't support DM, say N.
  

config SYS_NAND_SELF_INIT bool help diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile index 6fb3718..7745705 100644 --- a/drivers/mtd/nand/Makefile +++ b/drivers/mtd/nand/Makefile @@ -39,6 +39,8 @@ endif # not spl

ifdef NORMAL_DRIVERS

+obj-$(CONFIG_DM_NAND) += nand-uclass.o

obj-$(CONFIG_NAND_ECC_BCH) += nand_bch.o

obj-$(CONFIG_NAND_ATMEL) += atmel_nand.o diff --git a/drivers/mtd/nand/nand-uclass.c b/drivers/mtd/nand/nand-uclass.c new file mode 100644 index 0000000..d68d357 --- /dev/null +++ b/drivers/mtd/nand/nand-uclass.c @@ -0,0 +1,62 @@ +/*

• • NAND uclass driver for NAND bus.
• • (C) Copyright 2012-2016

• • Texas Instruments Incorporated, <www.ti.com>
    

• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <dm.h> +#include <errno.h> +#include <nand.h>

+DECLARE_GLOBAL_DATA_PTR;

+#ifdef CONFIG_DM_NAND

+nand_info_t *get_nand_dev_by_index(int idx) +{

•   nand_info_t *nand;
  

•   struct udevice *dev;
  

•   int ret;
  

•   ret = uclass_get_device(UCLASS_NAND, idx, &dev);
  

•   if (ret) {
  

•           error("NAND device (%d) not found\n", idx);
  

This should return -ENODEV. Also please avoid printf() in drivers. The caller can report the error.

Return type is pointer so returned NULL and NULL denotes no dev. Will change this error to debug in v2.

OK, I see that this is the existing API.

...

...

...

•           return NULL;
  

•   }
  

•   nand = (nand_info_t *)dev_get_uclass_priv(dev);
  

•   if (!nand) {
  

•           error("Nand device not ready\n");
  

•           return NULL;
  

•   }
  

But if the device has been probed ((with uclass_get_device()) then this cannot be NULL.

This check is just a failsafe. ideally it should not fail here.

I cannot get to probe the device without this data, since this would be a bug in DM. So the check has no value.

Okay then I will remove this check as it doesn't add value.

Regards Mugunthan V N

On Monday 04 April 2016 10:52 PM, Scott Wood wrote:

On Fri, 2016-04-01 at 16:59 +0530, Mugunthan V N wrote:

...

adopt omap_gpmc driver to driver model.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

drivers/mtd/nand/omap_gpmc.c | 205 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 205 insertions(+)

diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c index 4814fa2..df63491 100644 --- a/drivers/mtd/nand/omap_gpmc.c +++ b/drivers/mtd/nand/omap_gpmc.c @@ -16,6 +16,10 @@ #include <nand.h> #include <linux/mtd/omap_elm.h>

+#include <dm.h>

+DECLARE_GLOBAL_DATA_PTR;

#define BADBLOCK_MARKER_LENGTH 2 #define SECTOR_BYTES 512 #define ECCCLEAR (0x1 << 8) @@ -46,11 +50,22 @@ struct omap_nand_info { enum omap_ecc ecc_scheme; uint8_t cs; uint8_t ws; /* wait status pin (0,1) */

• uint8_t bus_width; /* Bus width of NAND device */

};

+#ifndef CONFIG_DM_NAND /* We are wasting a bit of memory but al least we are safe */ static struct omap_nand_info omap_nand_info[GPMC_MAX_CS];

+#else

+struct omap_gpmc_platdata {

• struct omap_nand_info *omap_nand_info;
• struct gpmc *gpmc_cfg;
• int max_cs;

+}; +#endif

/*

• omap_nand_hwcontrol - Set the address pointers corretly for the

• 	following address/data/command operation
  

@@ -943,6 +958,8 @@ int __maybe_unused omap_nand_switch_ecc(uint32_t hardware, uint32_t eccstrength) } #endif /* CONFIG_SPL_BUILD */

+#ifndef CONFIG_DM_NAND

/*

• Board-specific NAND initialization. The following members of the
• argument are board-specific:

@@ -1034,3 +1051,191 @@ int board_nand_init(struct nand_chip *nand)

return 0; }

...

+#else /* CONFIG_DM_NAND */

+static int omap_gpmc_probe(struct udevice *dev) +{

• struct nand_chip *nand = dev_get_priv(dev);
• struct omap_gpmc_platdata *pdata = dev_get_platdata(dev);
• struct gpmc *gpmc_cfg = pdata->gpmc_cfg;
• int32_t gpmc_config = 0;
• int ecc_opt;
• int cs = cs_next++;
• int err = 0;
• while (cs < pdata->max_cs) {

• /* Check if NAND type is set */
  

• if ((readl(&gpmc_cfg->cs[cs].config1) & 0xC00) == 0x800) {
  

• 	/* Found it!! */
  

• 	break;
  

• }
  

• cs++;
  

• }
• if (cs >= pdata->max_cs) {

• printf("nand: error: Unable to find NAND settings in GPMC
  

Configuration - quitting\n");

• return -ENODEV;
  

• }
• gpmc_config = readl(&gpmc_cfg->config);
• /* Disable Write protect */
• gpmc_config |= 0x10;
• writel(gpmc_config, &gpmc_cfg->config);
• nand->IO_ADDR_R = (void __iomem *)&gpmc_cfg->cs[cs].nand_dat;
• nand->IO_ADDR_W = (void __iomem *)&gpmc_cfg->cs[cs].nand_cmd;
• nand->priv = &pdata->omap_nand_info[cs];
• nand->cmd_ctrl = omap_nand_hwcontrol;
• nand->options |= NAND_NO_PADDING | NAND_CACHEPRG;
• nand->chip_delay = 100;
• nand->ecc.layout = &omap_ecclayout;
• /* configure driver and controller based on NAND device bus-width

*/

• gpmc_config = readl(&gpmc_cfg->cs[cs].config1);
• if (pdata->omap_nand_info[cs].bus_width == 16) {

• nand->options |= NAND_BUSWIDTH_16;
  

• writel(gpmc_config | (0x1 << 12), &gpmc_cfg
  

->cs[cs].config1);

• } else {

• nand->options &= ~NAND_BUSWIDTH_16;
  

• writel(gpmc_config & ~(0x1 << 12), &gpmc_cfg
  

->cs[cs].config1);

• }
• ecc_opt = pdata->omap_nand_info[cs].ecc_scheme;
• /* select ECC scheme */
• if (ecc_opt != OMAP_ECC_HAM1_CODE_SW) {

• err = omap_select_ecc_scheme(nand, ecc_opt,
  

• 			     CONFIG_SYS_NAND_PAGE_SIZE,
  

• 			     CONFIG_SYS_NAND_OOBSIZE);
  

• } else {

• /*
  

•  * pagesize and oobsize are not required to
  

•  * configure sw ecc-scheme
  

•  */
  

• err = omap_select_ecc_scheme(nand, ecc_opt, 0, 0);
  

• }
• if (err)

• return err;
  

+#ifdef CONFIG_NAND_OMAP_GPMC_PREFETCH

• nand->read_buf = omap_nand_read_prefetch;

+#else

• if (nand->options & NAND_BUSWIDTH_16)

• nand->read_buf = nand_read_buf16;
  

• else

• nand->read_buf = nand_read_buf;
  

+#endif

• nand->dev_ready = omap_dev_ready;
• return 0;

+}

Is it really necessary to duplicate the entire probe func based on DM?

It is not duplicated directly, if it is duplicate I would had a fuction defined and call it in both DM and non-DM case. Removed some of the #if/else. Only TODO I left is I have not remove nand driver callbacks and move it to DM ops structure, which will be done soon.

Regards Mugunthan V N