[U-Boot] [PATCH] nand: atmel: add drvice tree support and dm gpio APIs

Wenyou.Yang at microchip.com Wenyou.Yang at microchip.com
Thu Feb 9 05:06:56 UTC 2017


Hi Simon,

> -----Original Message-----
> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
> Sent: 2017年2月8日 13:09
> To: Wenyou Yang - A41535 <Wenyou.Yang at microchip.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Andreas Bießmann
> <andreas at biessmann.org>; Scott Wood <scottwood at freescale.com>; Wenyou
> Yang - A41535 <Wenyou.Yang at microchip.com>
> Subject: Re: [PATCH] nand: atmel: add drvice tree support and dm gpio APIs
> 
> Hi Wenyou,
> 
> On 3 February 2017 at 18:32, Wenyou Yang <wenyou.yang at atmel.com> wrote:
> >
> > In order to use the driver model gpio APIs, add the device tree
> > support.
> >
> > Signed-off-by: Wenyou Yang <wenyou.yang at atmel.com>
> > ---
> >
> >  drivers/mtd/nand/atmel_nand.c | 143
> ++++++++++++++++++++++++++++++++++--------
> >  include/fdtdec.h              |   1 +
> >  lib/fdtdec.c                  |   1 +
> >  3 files changed, 119 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/atmel_nand.c
> > b/drivers/mtd/nand/atmel_nand.c index 8669432deb..87ff5c2eb4 100644
> > --- a/drivers/mtd/nand/atmel_nand.c
> > +++ b/drivers/mtd/nand/atmel_nand.c
> > @@ -14,29 +14,34 @@
> >  #include <common.h>
> >  #include <asm/gpio.h>
> >  #include <asm/arch/gpio.h>
> > +#ifdef CONFIG_DM_GPIO
> > +#include <asm/gpio.h>
> > +#endif
> >
> > +#include <fdtdec.h>
> >  #include <malloc.h>
> >  #include <nand.h>
> >  #include <watchdog.h>
> >  #include <linux/mtd/nand_ecc.h>
> >
> > -#ifdef CONFIG_ATMEL_NAND_HWECC
> > -
> > -/* Register access macros */
> > -#define ecc_readl(add, reg)                            \
> > -       readl(add + ATMEL_ECC_##reg)
> > -#define ecc_writel(add, reg, value)                    \
> > -       writel((value), add + ATMEL_ECC_##reg)
> 
> What is happening here? Seems like it should be a separate patch.
> 
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +struct atmel_nand_data {
> > +       struct gpio_desc enable_pin;    /* chip enable */
> > +       struct gpio_desc det_pin;       /* card detect */
> > +       struct gpio_desc rdy_pin;       /* ready/busy */
> > +       u8              ale;    /* address line number connected to ALE */
> > +       u8              cle;    /* address line number connected to CLE */
> > +       u8              bus_width_16;   /* buswidth is 16 bit */
> > +       u8              ecc_mode;       /* ecc mode */
> > +       u8              on_flash_bbt;   /* bbt on flash */
> > +};
> >
> > -#include "atmel_nand_ecc.h"    /* Hardware ECC registers */
> > +struct atmel_nand_host {
> > +       void __iomem            *io_base;
> > +       struct atmel_nand_data  board;
> >
> >  #ifdef CONFIG_ATMEL_NAND_HW_PMECC
> > -
> > -#ifdef CONFIG_SPL_BUILD
> > -#undef CONFIG_SYS_NAND_ONFI_DETECTION -#endif
> > -
> > -struct atmel_nand_host {
> >         struct pmecc_regs __iomem *pmecc;
> >         struct pmecc_errloc_regs __iomem *pmerrloc;
> >         void __iomem            *pmecc_rom_base;
> > @@ -63,9 +68,26 @@ struct atmel_nand_host {
> >         int     *pmecc_mu;
> >         int     *pmecc_dmu;
> >         int     *pmecc_delta;
> > +#endif
> >  };
> >
> > -static struct atmel_nand_host pmecc_host;
> > +static struct atmel_nand_host nand_host; #ifdef
> > +CONFIG_ATMEL_NAND_HWECC
> > +
> > +/* Register access macros */
> > +#define ecc_readl(add, reg)                            \
> > +       readl(add + ATMEL_ECC_##reg)
> > +#define ecc_writel(add, reg, value)                    \
> > +       writel((value), add + ATMEL_ECC_##reg)
> > +
> > +#include "atmel_nand_ecc.h"    /* Hardware ECC registers */
> > +
> > +#ifdef CONFIG_ATMEL_NAND_HW_PMECC
> > +
> > +#ifdef CONFIG_SPL_BUILD
> > +#undef CONFIG_SYS_NAND_ONFI_DETECTION #endif
> > +
> >  static struct nand_ecclayout atmel_pmecc_oobinfo;
> >
> >  /*
> > @@ -805,12 +827,9 @@ static uint16_t *create_lookup_table(int
> > sector_size)  static int atmel_pmecc_nand_init_params(struct nand_chip *nand,
> >                 struct mtd_info *mtd)
> >  {
> > -       struct atmel_nand_host *host;
> > +       struct atmel_nand_host *host = nand_get_controller_data(nand);
> >         int cap, sector_size;
> >
> > -       host = &pmecc_host;
> > -       nand_set_controller_data(nand, host);
> > -
> >         nand->ecc.mode = NAND_ECC_HW;
> >         nand->ecc.calculate = NULL;
> >         nand->ecc.correct = NULL;
> > @@ -1207,12 +1226,24 @@ int atmel_hwecc_nand_init_param(struct
> > nand_chip *nand, struct mtd_info *mtd)  #endif /*
> > CONFIG_ATMEL_NAND_HWECC */
> >
> >  static void at91_nand_hwcontrol(struct mtd_info *mtd,
> > -                                        int cmd, unsigned int ctrl)
> > +                               int cmd, unsigned int ctrl)
> >  {
> >         struct nand_chip *this = mtd_to_nand(mtd);
> > +#if CONFIG_IS_ENABLED(OF_CONTROL)
> > +       struct atmel_nand_host *host = nand_get_controller_data(this);
> > +#endif
> >
> >         if (ctrl & NAND_CTRL_CHANGE) {
> > -               ulong IO_ADDR_W = (ulong) this->IO_ADDR_W;
> > +               ulong IO_ADDR_W = (ulong)this->IO_ADDR_W; #if
> > +CONFIG_IS_ENABLED(OF_CONTROL)
> > +               IO_ADDR_W &= ~((1 << host->board.ale)
> > +                            | (1 << host->board.cle));
> > +
> > +               if (ctrl & NAND_CLE)
> > +                       IO_ADDR_W |= (1 << host->board.cle);
> > +               if (ctrl & NAND_ALE)
> > +                       IO_ADDR_W |= (1 << host->board.ale); #else
> >                 IO_ADDR_W &= ~(CONFIG_SYS_NAND_MASK_ALE
> >                              | CONFIG_SYS_NAND_MASK_CLE);
> 
> You could adjust it so that CONFIG_SYS_NAND_MASK_CLE is assigned to
> host->board.cle in the init function, and then avoid this #ifdef here.

Accepted

> 
> >
> > @@ -1220,10 +1251,18 @@ static void at91_nand_hwcontrol(struct mtd_info
> *mtd,
> >                         IO_ADDR_W |= CONFIG_SYS_NAND_MASK_CLE;
> >                 if (ctrl & NAND_ALE)
> >                         IO_ADDR_W |= CONFIG_SYS_NAND_MASK_ALE;
> > +#endif
> >
> > +#ifdef CONFIG_DM_GPIO
> > +               if (dm_gpio_is_valid(&host->board.enable_pin))
> > +                       dm_gpio_set_value(&host->board.enable_pin,
> > +                                         !(ctrl & NAND_NCE)); #else
> >  #ifdef CONFIG_SYS_NAND_ENABLE_PIN
> >                 gpio_set_value(CONFIG_SYS_NAND_ENABLE_PIN, !(ctrl &
> > NAND_NCE));  #endif
> > +#endif
> > +
> >                 this->IO_ADDR_W = (void *) IO_ADDR_W;
> >         }
> >
> > @@ -1231,12 +1270,24 @@ static void at91_nand_hwcontrol(struct mtd_info
> *mtd,
> >                 writeb(cmd, this->IO_ADDR_W);  }
> >
> > -#ifdef CONFIG_SYS_NAND_READY_PIN
> >  static int at91_nand_ready(struct mtd_info *mtd)  {
> > +#ifdef CONFIG_DM_GPIO
> > +       struct nand_chip *nand_chip = mtd_to_nand(mtd);
> > +       struct atmel_nand_host *host =
> > +nand_get_controller_data(nand_chip);
> > +
> > +       if (dm_gpio_is_valid(&host->board.rdy_pin))
> > +               return dm_gpio_get_value(&host->board.rdy_pin);
> > +
> > +       return 0;
> > +#else
> > +#ifdef CONFIG_SYS_NAND_READY_PIN
> >         return gpio_get_value(CONFIG_SYS_NAND_READY_PIN);
> > -}
> > +#else
> > +       return 0;
> >  #endif
> > +#endif
> > +}
> >
> >  #ifdef CONFIG_SPL_BUILD
> >  /* The following code is for SPL */
> > @@ -1419,6 +1470,10 @@ int at91_nand_wait_ready(struct mtd_info *mtd)
> > int board_nand_init(struct nand_chip *nand)  {
> >         int ret = 0;
> > +       struct atmel_nand_host *host;
> > +
> > +       host = &nand_host;
> > +       nand_set_controller_data(nand, host);
> >
> >         nand->ecc.mode = NAND_ECC_SOFT;  #ifdef
> CONFIG_SYS_NAND_DBW_16
> > @@ -1486,9 +1541,45 @@ int atmel_nand_chip_init(int devnum, ulong
> base_addr)
> >         int ret;
> >         struct nand_chip *nand = &nand_chip[devnum];
> >         struct mtd_info *mtd = nand_to_mtd(nand);
> > +       struct atmel_nand_host *host;
> > +
> > +       host = &nand_host;
> > +       nand_set_controller_data(nand, host);
> > +
> > +#if CONFIG_IS_ENABLED(OF_CONTROL)
> > +       struct atmel_nand_data *board;
> > +       int node;
> > +
> > +       board = &host->board;
> > +       node = fdtdec_next_compatible(gd->fdt_blob, 0,
> > +                                     COMPAT_ATMEL_NAND);
> 
> Can you instead use a proper driver? Then you can avoid adding a COMPAT
> string. I think you have a nand uclass.

You mean the patch:
http://lists.denx.de/pipermail/u-boot/2017-January/279904.html

If so, that would be very nice. I will convert the nand driver to support driver model, and device tree as well.

> 
> > +       if (node < 0)
> > +               return -1;
> 
> This is -EPERM. Perhaps use -EINVAL ?
> 
> > +
> > +       nand->IO_ADDR_R = (void __iomem *)
> > +                         fdtdec_get_addr_size_auto_noparent(gd->fdt_blob,
> > +                                                            node, "reg",
> > +                                                            0, NULL, true);
> > +       nand->IO_ADDR_W = nand->IO_ADDR_R;
> >
> > -       nand->IO_ADDR_R = nand->IO_ADDR_W = (void  __iomem *)base_addr;
> > +       board->ale = fdtdec_get_uint(gd->fdt_blob, node,
> > +                                    "atmel,nand-addr-offset", 21);
> >
> > +       board->cle = fdtdec_get_uint(gd->fdt_blob, node,
> > +                                    "atmel,nand-cmd-offset", 22);
> > +
> > +       gpio_request_by_name_nodev(gd->fdt_blob, node, "gpios", 0,
> > +                                  &board->rdy_pin, GPIOD_IS_IN);
> > +
> > +       gpio_request_by_name_nodev(gd->fdt_blob, node, "gpios", 1,
> > +                                  &board->enable_pin, GPIOD_IS_OUT);
> > +
> > +       gpio_request_by_name_nodev(gd->fdt_blob, node, "gpios", 2,
> > +                                  &board->det_pin, GPIOD_IS_IN);
> > +#else
> > +       nand->IO_ADDR_R = (void  __iomem *)base_addr;
> > +       nand->IO_ADDR_W = (void  __iomem *)base_addr; #endif
> >  #ifdef CONFIG_NAND_ECC_BCH
> >         nand->ecc.mode = NAND_ECC_SOFT_BCH;  #else @@ -1498,9 +1589,9
> > @@ int atmel_nand_chip_init(int devnum, ulong base_addr)
> >         nand->options = NAND_BUSWIDTH_16;  #endif
> >         nand->cmd_ctrl = at91_nand_hwcontrol; -#ifdef
> > CONFIG_SYS_NAND_READY_PIN
> > +
> >         nand->dev_ready = at91_nand_ready; -#endif
> > +
> >         nand->chip_delay = 75;
> >  #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
> >         nand->bbt_options |= NAND_BBT_USE_FLASH; diff --git
> > a/include/fdtdec.h b/include/fdtdec.h index d074478f14..eacf251d63
> > 100644
> > --- a/include/fdtdec.h
> > +++ b/include/fdtdec.h
> > @@ -155,6 +155,7 @@ enum fdt_compat_id {
> >         COMPAT_INTEL_BAYTRAIL_FSP_MDP,  /* Intel FSP memory-down
> params */
> >         COMPAT_INTEL_IVYBRIDGE_FSP,     /* Intel Ivy Bridge FSP */
> >         COMPAT_SUNXI_NAND,              /* SUNXI NAND controller */
> > +       COMPAT_ATMEL_NAND,              /* Atmel NAND controller */
> >
> >         COMPAT_COUNT,
> >  };
> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 81f47ef2c7..a89e8272e1
> > 100644
> > --- a/lib/fdtdec.c
> > +++ b/lib/fdtdec.c
> > @@ -66,6 +66,7 @@ static const char * const
> compat_names[COMPAT_COUNT] = {
> >         COMPAT(INTEL_BAYTRAIL_FSP_MDP, "intel,baytrail-fsp-mdp"),
> >         COMPAT(INTEL_IVYBRIDGE_FSP, "intel,ivybridge-fsp"),
> >         COMPAT(COMPAT_SUNXI_NAND, "allwinner,sun4i-a10-nand"),
> > +       COMPAT(COMPAT_ATMEL_NAND, "atmel,at91rm9200-nand"),
> >  };
> >
> >  const char *fdtdec_get_compatible(enum fdt_compat_id id)
> > --
> > 2.11.0
> >
> 
> Regards,
> Simon

Thank you


Best Regards,
Wenyou Yang


More information about the U-Boot mailing list