Hi Simon,
> -----Original Message-----
> From: s...@google.com [mailto:s...@google.com] On Behalf Of Simon Glass
> Sent: 2017年2月8日 13:09
> To: Wenyou Yang - A41535 <wenyou.y...@microchip.com>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Andreas Bießmann
> <andr...@biessmann.org>; Scott Wood <scottw...@freescale.com>; Wenyou
> Yang - A41535 <wenyou.y...@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.y...@atmel.com> wrote:
> >
> > In order to use the driver model gpio APIs, add the device tree
> > support.
> >
> > Signed-off-by: Wenyou Yang <wenyou.y...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
