[U-Boot] [PATCH 4/9] drivers: nand: implement a NAND uclass

From: Mugunthan V N <mugunthanvnm@ti.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 4/9] drivers: nand: implement a NAND uclass
Date: Thu, 21 Apr 2016 11:25:40 +0530	[thread overview]
Message-ID: <57186B5C.1050506@ti.com> (raw)
In-Reply-To: <CAPnjgZ05z7LdsXwFpNbvqCrsGPSwbxPehJmGzUoS6ty6kQUYoA@mail.gmail.com>

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