| Message ID | 4F5844B3A985794BA902E12C070812375F8D2E@DNCE04.ent.ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
+ Greg KH (drivers/memory/* patches goes through his queue) On Monday 11 November 2013 12:06 PM, Khoronzhuk, Ivan wrote: > Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module > is intended to provide a glue-less interface to a variety of > asynchronous memory devices like ASRA M, NOR and NAND memory. A total > of 256M bytes of any of these memories can be accessed at any given > time via four chip selects with 64M byte access per chip select. > > Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR > are not supported. > > See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf > > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> > --- > drivers/memory/Kconfig | 11 ++ > drivers/memory/Makefile | 1 + > drivers/memory/davinci-aemif.c | 415 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 427 insertions(+) > create mode 100644 drivers/memory/davinci-aemif.c > > diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig > index 29a11db..010e75e 100644 > --- a/drivers/memory/Kconfig > +++ b/drivers/memory/Kconfig > @@ -7,6 +7,17 @@ menuconfig MEMORY > > if MEMORY > > +config TI_DAVINCI_AEMIF s/TI_DAVINCI_AEMIF/TI_AEMIF > + bool "Texas Instruments DaVinci AEMIF driver" Drop DaVinci above since its used on more SOCs. > + depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF > + help > + This driver is for the AEMIF module available in Texas Instruments > + SoCs. AEMIF stands for Asynchronous External Memory Interface and > + is intended to provide a glue-less interface to a variety of > + asynchronuous memory devices like ASRAM, NOR and NAND memory. A total > + of 256M bytes of any of these memories can be accessed at a given > + time via four chip selects with 64M byte access per chip select. > + > config TI_EMIF > tristate "Texas Instruments EMIF driver" > depends on ARCH_OMAP2PLUS > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile > index 969d923..af14126 100644 > --- a/drivers/memory/Makefile > +++ b/drivers/memory/Makefile > @@ -5,6 +5,7 @@ > ifeq ($(CONFIG_DDR),y) > obj-$(CONFIG_OF) += of_memory.o > endif > +obj-$(CONFIG_TI_DAVINCI_AEMIF) += davinci-aemif.o Change this accordingly once the config is renamed. > obj-$(CONFIG_TI_EMIF) += emif.o > obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o > obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o > diff --git a/drivers/memory/davinci-aemif.c b/drivers/memory/davinci-aemif.c > new file mode 100644 > index 0000000..e36b74b > --- /dev/null > +++ b/drivers/memory/davinci-aemif.c > @@ -0,0 +1,415 @@ > +/* > + * DaVinci/Keystone AEMIF driver s/{DaVinci/Keystone}/TI > + * > + * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/ > + * Copyright (C) Heiko Schocher <hs@denx.de> > + * Copyright (C) Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > + > +#define TA_SHIFT 2 > +#define RHOLD_SHIFT 4 > +#define RSTROBE_SHIFT 7 > +#define RSETUP_SHIFT 13 > +#define WHOLD_SHIFT 17 > +#define WSTROBE_SHIFT 20 > +#define WSETUP_SHIFT 26 > +#define EW_SHIFT 30 > +#define SS_SHIFT 31 > + > +#define TA(x) ((x) << TA_SHIFT) > +#define RHOLD(x) ((x) << RHOLD_SHIFT) > +#define RSTROBE(x) ((x) << RSTROBE_SHIFT) > +#define RSETUP(x) ((x) << RSETUP_SHIFT) > +#define WHOLD(x) ((x) << WHOLD_SHIFT) > +#define WSTROBE(x) ((x) << WSTROBE_SHIFT) > +#define WSETUP(x) ((x) << WSETUP_SHIFT) > +#define EW(x) ((x) << EW_SHIFT) > +#define SS(x) ((x) << SS_SHIFT) > + > +#define ASIZE_MAX 0x1 > +#define TA_MAX 0x3 > +#define RHOLD_MAX 0x7 > +#define RSTROBE_MAX 0x3f > +#define RSETUP_MAX 0xf > +#define WHOLD_MAX 0x7 > +#define WSTROBE_MAX 0x3f > +#define WSETUP_MAX 0xf > +#define EW_MAX 0x1 > +#define SS_MAX 0x1 > +#define NUM_CS 4 > + > +#define TA_VAL(x) (((x) & TA(TA_MAX)) >> TA_SHIFT) > +#define RHOLD_VAL(x) (((x) & RHOLD(RHOLD_MAX)) >> RHOLD_SHIFT) > +#define RSTROBE_VAL(x) (((x) & RSTROBE(RSTROBE_MAX)) >> RSTROBE_SHIFT) > +#define RSETUP_VAL(x) (((x) & RSETUP(RSETUP_MAX)) >> RSETUP_SHIFT) > +#define WHOLD_VAL(x) (((x) & WHOLD(WHOLD_MAX)) >> WHOLD_SHIFT) > +#define WSTROBE_VAL(x) (((x) & WSTROBE(WSTROBE_MAX)) >> WSTROBE_SHIFT) > +#define WSETUP_VAL(x) (((x) & WSETUP(WSETUP_MAX)) >> WSETUP_SHIFT) > +#define EW_VAL(x) (((x) & EW(EW_MAX)) >> EW_SHIFT) > +#define SS_VAL(x) (((x) & SS(SS_MAX)) >> SS_SHIFT) > + > +#define NRCSR_OFFSET 0x00 > +#define AWCCR_OFFSET 0x04 > +#define A1CR_OFFSET 0x10 > + > +#define ACR_ASIZE_MASK 0x3 > +#define ACR_EW_MASK BIT(30) > +#define ACR_SS_MASK BIT(31) > +#define ASIZE_16BIT 1 > + > +#define CONFIG_MASK (TA(TA_MAX) | \ > + RHOLD(RHOLD_MAX) | \ > + RSTROBE(RSTROBE_MAX) | \ > + RSETUP(RSETUP_MAX) | \ > + WHOLD(WHOLD_MAX) | \ > + WSTROBE(WSTROBE_MAX) | \ > + WSETUP(WSETUP_MAX) | \ > + EW(EW_MAX) | SS(SS_MAX) | \ > + ASIZE_MAX) > + > +#define DRV_NAME "davinci-aemif" s/davinci-aemif/ti-aemif > + > +/** > + * structure to hold cs parameters > + * @cs: chip-select number > + * @wstrobe: write strobe width, ns > + * @rstrobe: read strobe width, ns > + * @wsetup: write setup width, ns > + * @whold: write hold width, ns > + * @rsetup: read setup width, ns > + * @rhold: read hold width, ns > + * @ta: minimum turn around time, ns > + * @enable_ss: enable/disable select strobe mode > + * @enable_ew: enable/disable extended wait mode > + * @asize: width of the asynchronous device's data bus > + */ > +struct davinci_aemif_cs_data { > + u8 cs; > + u16 wstrobe; > + u16 rstrobe; > + u8 wsetup; > + u8 whold; > + u8 rsetup; > + u8 rhold; > + u8 ta; > + u8 enable_ss; > + u8 enable_ew; > + u8 asize; > +}; > + I suggest you drop davinci prefix throughout the driver. [..] > +static const struct of_device_id davinci_aemif_of_match[] = { > + { .compatible = "ti,davinci-aemif", }, > + { .compatible = "ti,keystone-aemif", }, > + { .compatible = "ti,omap-L138-aemif", }, > + {}, > +}; Do we need three seperate compatible or "ti,aemif" should be enough ? Are there differences which we need to handled based on compatibles ? > + > +static const struct of_device_id davinci_cs_of_match[] = { > + { .compatible = "ti,davinci-cs", }, > + {}, > +}; > + > +static int davinci_aemif_probe(struct platform_device *pdev) > +{ > + int ret = -ENODEV, i; > + struct resource *res; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + > + if (np == NULL) > + return 0; > + > + if (aemif) { > + dev_err(dev, "davinci_aemif driver is in use currently\n"); > + return -EBUSY; > + } > + > + aemif = devm_kzalloc(dev, sizeof(*aemif), GFP_KERNEL); > + Drop this extra line. > + if (!aemif) { > + dev_err(dev, "cannot allocate memory for aemif\n"); > + return -ENOMEM; > + } > + > + aemif->clk = devm_clk_get(dev, "aemif"); > + if (IS_ERR(aemif->clk)) { > + dev_err(dev, "cannot get clock 'aemif'\n"); > + return PTR_ERR(aemif->clk); > + } > + > + clk_prepare_enable(aemif->clk); > + aemif->clk_rate = clk_get_rate(aemif->clk) / 1000; > + 1000 divider ? Define a marco and use it to be clear. Other than above comments patch looks fine to me. Regards, Santosh
On 11/12/2013 06:08 PM, Santosh Shilimkar wrote: > + Greg KH (drivers/memory/* patches goes through his queue) > > On Monday 11 November 2013 12:06 PM, Khoronzhuk, Ivan wrote: >> Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module >> is intended to provide a glue-less interface to a variety of >> asynchronous memory devices like ASRA M, NOR and NAND memory. A total >> of 256M bytes of any of these memories can be accessed at any given >> time via four chip selects with 64M byte access per chip select. >> >> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR >> are not supported. >> >> See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf >> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> >> --- >> drivers/memory/Kconfig | 11 ++ >> drivers/memory/Makefile | 1 + >> drivers/memory/davinci-aemif.c | 415 ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 427 insertions(+) >> create mode 100644 drivers/memory/davinci-aemif.c >> >> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig >> index 29a11db..010e75e 100644 >> --- a/drivers/memory/Kconfig >> +++ b/drivers/memory/Kconfig >> @@ -7,6 +7,17 @@ menuconfig MEMORY >> >> if MEMORY >> >> +config TI_DAVINCI_AEMIF > s/TI_DAVINCI_AEMIF/TI_AEMIF > Ok >> + bool "Texas Instruments DaVinci AEMIF driver" > Drop DaVinci above since its used on more SOCs. >> + depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF >> + help >> + This driver is for the AEMIF module available in Texas Instruments >> + SoCs. AEMIF stands for Asynchronous External Memory Interface and >> + is intended to provide a glue-less interface to a variety of >> + asynchronuous memory devices like ASRAM, NOR and NAND memory. A total >> + of 256M bytes of any of these memories can be accessed at a given >> + time via four chip selects with 64M byte access per chip select. >> + >> config TI_EMIF >> tristate "Texas Instruments EMIF driver" >> depends on ARCH_OMAP2PLUS >> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile >> index 969d923..af14126 100644 >> --- a/drivers/memory/Makefile >> +++ b/drivers/memory/Makefile >> @@ -5,6 +5,7 @@ >> ifeq ($(CONFIG_DDR),y) >> obj-$(CONFIG_OF) += of_memory.o >> endif >> +obj-$(CONFIG_TI_DAVINCI_AEMIF) += davinci-aemif.o > > Change this accordingly once the config is renamed. > Ok >> obj-$(CONFIG_TI_EMIF) += emif.o >> obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o >> obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o >> diff --git a/drivers/memory/davinci-aemif.c b/drivers/memory/davinci-aemif.c >> new file mode 100644 >> index 0000000..e36b74b >> --- /dev/null >> +++ b/drivers/memory/davinci-aemif.c >> @@ -0,0 +1,415 @@ >> +/* >> + * DaVinci/Keystone AEMIF driver > s/{DaVinci/Keystone}/TI >> + * >> + * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/ >> + * Copyright (C) Heiko Schocher <hs@denx.de> >> + * Copyright (C) Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/err.h> >> +#include <linux/io.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_platform.h> >> +#include <linux/platform_device.h> >> + >> +#define TA_SHIFT 2 >> +#define RHOLD_SHIFT 4 >> +#define RSTROBE_SHIFT 7 >> +#define RSETUP_SHIFT 13 >> +#define WHOLD_SHIFT 17 >> +#define WSTROBE_SHIFT 20 >> +#define WSETUP_SHIFT 26 >> +#define EW_SHIFT 30 >> +#define SS_SHIFT 31 >> + >> +#define TA(x) ((x) << TA_SHIFT) >> +#define RHOLD(x) ((x) << RHOLD_SHIFT) >> +#define RSTROBE(x) ((x) << RSTROBE_SHIFT) >> +#define RSETUP(x) ((x) << RSETUP_SHIFT) >> +#define WHOLD(x) ((x) << WHOLD_SHIFT) >> +#define WSTROBE(x) ((x) << WSTROBE_SHIFT) >> +#define WSETUP(x) ((x) << WSETUP_SHIFT) >> +#define EW(x) ((x) << EW_SHIFT) >> +#define SS(x) ((x) << SS_SHIFT) >> + >> +#define ASIZE_MAX 0x1 >> +#define TA_MAX 0x3 >> +#define RHOLD_MAX 0x7 >> +#define RSTROBE_MAX 0x3f >> +#define RSETUP_MAX 0xf >> +#define WHOLD_MAX 0x7 >> +#define WSTROBE_MAX 0x3f >> +#define WSETUP_MAX 0xf >> +#define EW_MAX 0x1 >> +#define SS_MAX 0x1 >> +#define NUM_CS 4 >> + >> +#define TA_VAL(x) (((x) & TA(TA_MAX)) >> TA_SHIFT) >> +#define RHOLD_VAL(x) (((x) & RHOLD(RHOLD_MAX)) >> RHOLD_SHIFT) >> +#define RSTROBE_VAL(x) (((x) & RSTROBE(RSTROBE_MAX)) >> RSTROBE_SHIFT) >> +#define RSETUP_VAL(x) (((x) & RSETUP(RSETUP_MAX)) >> RSETUP_SHIFT) >> +#define WHOLD_VAL(x) (((x) & WHOLD(WHOLD_MAX)) >> WHOLD_SHIFT) >> +#define WSTROBE_VAL(x) (((x) & WSTROBE(WSTROBE_MAX)) >> WSTROBE_SHIFT) >> +#define WSETUP_VAL(x) (((x) & WSETUP(WSETUP_MAX)) >> WSETUP_SHIFT) >> +#define EW_VAL(x) (((x) & EW(EW_MAX)) >> EW_SHIFT) >> +#define SS_VAL(x) (((x) & SS(SS_MAX)) >> SS_SHIFT) >> + >> +#define NRCSR_OFFSET 0x00 >> +#define AWCCR_OFFSET 0x04 >> +#define A1CR_OFFSET 0x10 >> + >> +#define ACR_ASIZE_MASK 0x3 >> +#define ACR_EW_MASK BIT(30) >> +#define ACR_SS_MASK BIT(31) >> +#define ASIZE_16BIT 1 >> + >> +#define CONFIG_MASK (TA(TA_MAX) | \ >> + RHOLD(RHOLD_MAX) | \ >> + RSTROBE(RSTROBE_MAX) | \ >> + RSETUP(RSETUP_MAX) | \ >> + WHOLD(WHOLD_MAX) | \ >> + WSTROBE(WSTROBE_MAX) | \ >> + WSETUP(WSETUP_MAX) | \ >> + EW(EW_MAX) | SS(SS_MAX) | \ >> + ASIZE_MAX) >> + >> +#define DRV_NAME "davinci-aemif" > s/davinci-aemif/ti-aemif Ok >> + >> +/** >> + * structure to hold cs parameters >> + * @cs: chip-select number >> + * @wstrobe: write strobe width, ns >> + * @rstrobe: read strobe width, ns >> + * @wsetup: write setup width, ns >> + * @whold: write hold width, ns >> + * @rsetup: read setup width, ns >> + * @rhold: read hold width, ns >> + * @ta: minimum turn around time, ns >> + * @enable_ss: enable/disable select strobe mode >> + * @enable_ew: enable/disable extended wait mode >> + * @asize: width of the asynchronous device's data bus >> + */ >> +struct davinci_aemif_cs_data { >> + u8 cs; >> + u16 wstrobe; >> + u16 rstrobe; >> + u8 wsetup; >> + u8 whold; >> + u8 rsetup; >> + u8 rhold; >> + u8 ta; >> + u8 enable_ss; >> + u8 enable_ew; >> + u8 asize; >> +}; >> + > I suggest you drop davinci prefix throughout the > driver. > > [..] > Seems there is nothing to prevent Ok >> +static const struct of_device_id davinci_aemif_of_match[] = { >> + { .compatible = "ti,davinci-aemif", }, >> + { .compatible = "ti,keystone-aemif", }, >> + { .compatible = "ti,omap-L138-aemif", }, >> + {}, >> +}; > Do we need three seperate compatible or "ti,aemif" should > be enough ? Are there differences which we need to handled > based on compatibles ? > We need separate compatibles. Depending on this cs start number is evaluated. For omap-L138-aemif it is 2 for keystone-aemif it is 0. >> + >> +static const struct of_device_id davinci_cs_of_match[] = { >> + { .compatible = "ti,davinci-cs", }, >> + {}, >> +}; >> + >> +static int davinci_aemif_probe(struct platform_device *pdev) >> +{ >> + int ret = -ENODEV, i; >> + struct resource *res; >> + struct device *dev = &pdev->dev; >> + struct device_node *np = dev->of_node; >> + >> + if (np == NULL) >> + return 0; >> + >> + if (aemif) { >> + dev_err(dev, "davinci_aemif driver is in use currently\n"); >> + return -EBUSY; >> + } >> + >> + aemif = devm_kzalloc(dev, sizeof(*aemif), GFP_KERNEL); >> + > Drop this extra line. > Ok, I will >> + if (!aemif) { >> + dev_err(dev, "cannot allocate memory for aemif\n"); >> + return -ENOMEM; >> + } >> + >> + aemif->clk = devm_clk_get(dev, "aemif"); >> + if (IS_ERR(aemif->clk)) { >> + dev_err(dev, "cannot get clock 'aemif'\n"); >> + return PTR_ERR(aemif->clk); >> + } >> + >> + clk_prepare_enable(aemif->clk); >> + aemif->clk_rate = clk_get_rate(aemif->clk) / 1000; >> + > 1000 divider ? Define a marco and use it to be clear. Is it OK if I use MSEC_PER_SEC (linux/time.h). > > Other than above comments patch looks fine to me. > > Regards, > Santosh >
On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote: > Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module > is intended to provide a glue-less interface to a variety of > asynchronous memory devices like ASRA M, NOR and NAND memory. A total > of 256M bytes of any of these memories can be accessed at any given > time via four chip selects with 64M byte access per chip select. > > Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR > are not supported. > > See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf > > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> > --- > drivers/memory/Kconfig | 11 ++ > drivers/memory/Makefile | 1 + > drivers/memory/davinci-aemif.c | 415 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 427 insertions(+) > create mode 100644 drivers/memory/davinci-aemif.c > > diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig > index 29a11db..010e75e 100644 > --- a/drivers/memory/Kconfig > +++ b/drivers/memory/Kconfig > @@ -7,6 +7,17 @@ menuconfig MEMORY > > if MEMORY > > +config TI_DAVINCI_AEMIF > + bool "Texas Instruments DaVinci AEMIF driver" This driver cannot be a module? If not, why not? > + depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF > + help > + This driver is for the AEMIF module available in Texas Instruments > + SoCs. AEMIF stands for Asynchronous External Memory Interface and > + is intended to provide a glue-less interface to a variety of > + asynchronuous memory devices like ASRAM, NOR and NAND memory. A total > + of 256M bytes of any of these memories can be accessed at a given > + time via four chip selects with 64M byte access per chip select. > + > config TI_EMIF > tristate "Texas Instruments EMIF driver" > depends on ARCH_OMAP2PLUS > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile > index 969d923..af14126 100644 > --- a/drivers/memory/Makefile > +++ b/drivers/memory/Makefile > @@ -5,6 +5,7 @@ > ifeq ($(CONFIG_DDR),y) > obj-$(CONFIG_OF) += of_memory.o > endif > +obj-$(CONFIG_TI_DAVINCI_AEMIF) += davinci-aemif.o > obj-$(CONFIG_TI_EMIF) += emif.o > obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o > obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o > diff --git a/drivers/memory/davinci-aemif.c b/drivers/memory/davinci-aemif.c > new file mode 100644 > index 0000000..e36b74b > --- /dev/null > +++ b/drivers/memory/davinci-aemif.c > @@ -0,0 +1,415 @@ > +/* > + * DaVinci/Keystone AEMIF driver > + * > + * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/ > + * Copyright (C) Heiko Schocher <hs@denx.de> > + * Copyright (C) Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> Just checking: You meant Author: ? Code is already copyrighted to TI by line above. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > + > +#define TA_SHIFT 2 > +#define RHOLD_SHIFT 4 > +#define RSTROBE_SHIFT 7 > +#define RSETUP_SHIFT 13 > +#define WHOLD_SHIFT 17 > +#define WSTROBE_SHIFT 20 > +#define WSETUP_SHIFT 26 > +#define EW_SHIFT 30 > +#define SS_SHIFT 31 > + > +#define TA(x) ((x) << TA_SHIFT) > +#define RHOLD(x) ((x) << RHOLD_SHIFT) > +#define RSTROBE(x) ((x) << RSTROBE_SHIFT) > +#define RSETUP(x) ((x) << RSETUP_SHIFT) > +#define WHOLD(x) ((x) << WHOLD_SHIFT) > +#define WSTROBE(x) ((x) << WSTROBE_SHIFT) > +#define WSETUP(x) ((x) << WSETUP_SHIFT) > +#define EW(x) ((x) << EW_SHIFT) > +#define SS(x) ((x) << SS_SHIFT) > + > +#define ASIZE_MAX 0x1 > +#define TA_MAX 0x3 > +#define RHOLD_MAX 0x7 > +#define RSTROBE_MAX 0x3f > +#define RSETUP_MAX 0xf > +#define WHOLD_MAX 0x7 > +#define WSTROBE_MAX 0x3f > +#define WSETUP_MAX 0xf > +#define EW_MAX 0x1 > +#define SS_MAX 0x1 > +#define NUM_CS 4 > + > +#define TA_VAL(x) (((x) & TA(TA_MAX)) >> TA_SHIFT) > +#define RHOLD_VAL(x) (((x) & RHOLD(RHOLD_MAX)) >> RHOLD_SHIFT) > +#define RSTROBE_VAL(x) (((x) & RSTROBE(RSTROBE_MAX)) >> RSTROBE_SHIFT) > +#define RSETUP_VAL(x) (((x) & RSETUP(RSETUP_MAX)) >> RSETUP_SHIFT) > +#define WHOLD_VAL(x) (((x) & WHOLD(WHOLD_MAX)) >> WHOLD_SHIFT) > +#define WSTROBE_VAL(x) (((x) & WSTROBE(WSTROBE_MAX)) >> WSTROBE_SHIFT) > +#define WSETUP_VAL(x) (((x) & WSETUP(WSETUP_MAX)) >> WSETUP_SHIFT) > +#define EW_VAL(x) (((x) & EW(EW_MAX)) >> EW_SHIFT) > +#define SS_VAL(x) (((x) & SS(SS_MAX)) >> SS_SHIFT) > + > +#define NRCSR_OFFSET 0x00 > +#define AWCCR_OFFSET 0x04 > +#define A1CR_OFFSET 0x10 > + > +#define ACR_ASIZE_MASK 0x3 > +#define ACR_EW_MASK BIT(30) > +#define ACR_SS_MASK BIT(31) > +#define ASIZE_16BIT 1 > + > +#define CONFIG_MASK (TA(TA_MAX) | \ > + RHOLD(RHOLD_MAX) | \ > + RSTROBE(RSTROBE_MAX) | \ > + RSETUP(RSETUP_MAX) | \ > + WHOLD(WHOLD_MAX) | \ > + WSTROBE(WSTROBE_MAX) | \ > + WSETUP(WSETUP_MAX) | \ > + EW(EW_MAX) | SS(SS_MAX) | \ > + ASIZE_MAX) > + > +#define DRV_NAME "davinci-aemif" > + > +/** > + * structure to hold cs parameters > + * @cs: chip-select number > + * @wstrobe: write strobe width, ns > + * @rstrobe: read strobe width, ns > + * @wsetup: write setup width, ns > + * @whold: write hold width, ns > + * @rsetup: read setup width, ns > + * @rhold: read hold width, ns > + * @ta: minimum turn around time, ns > + * @enable_ss: enable/disable select strobe mode > + * @enable_ew: enable/disable extended wait mode > + * @asize: width of the asynchronous device's data bus > + */ > +struct davinci_aemif_cs_data { > + u8 cs; > + u16 wstrobe; > + u16 rstrobe; > + u8 wsetup; > + u8 whold; > + u8 rsetup; > + u8 rhold; > + u8 ta; > + u8 enable_ss; > + u8 enable_ew; > + u8 asize; > +}; > + > +/** > + * structure to hold device data > + * @base: base address of AEMIF registers > + * @clk: source clock > + * @clk_rate: clock's rate in kHz > + * @num_cs: number of assigned chip-selects > + * @cs_data: array of chip-select settings > + */ > +struct davinci_aemif_device { > + struct device *dev; > + void __iomem *base; > + struct clk *clk; > + unsigned long clk_rate; > + u8 num_cs; > + int cs_offset; > + struct davinci_aemif_cs_data cs_data[NUM_CS]; > +}; > + > +static struct davinci_aemif_device *aemif; > +/** > + * davinci_aemif_calc_rate - calculate timing data. > + * @wanted: The cycle time needed in nanoseconds. > + * @clk: The input clock rate in kHz. > + * @max: The maximum divider value that can be programmed. > + * > + * On success, returns the calculated timing value minus 1 for easy > + * programming into AEMIF timing registers, else negative errno. > + */ > +static int davinci_aemif_calc_rate(int wanted, unsigned long clk, int max) > +{ > + int result; > + > + result = DIV_ROUND_UP((wanted * clk), NSEC_PER_MSEC) - 1; > + > + dev_dbg(aemif->dev, "%s: result %d from %ld, %d\n", __func__, result, > + clk, wanted); > + > + /* It is generally OK to have a more relaxed timing than requested... */ > + if (result < 0) > + result = 0; > + > + /* ... But configuring tighter timings is not an option. */ > + else if (result > max) > + result = -EINVAL; > + > + return result; > +} > + > +/** > + * davinci_aemif_config_abus - configure async bus parameters > + * @data: aemif chip select configuration > + * > + * This function programs the given timing values (in real clock) into the > + * AEMIF registers taking the AEMIF clock into account. > + * > + * This function does not use any locking while programming the AEMIF > + * because it is expected that there is only one user of a given > + * chip-select. > + * > + * Returns 0 on success, else negative errno. > + */ > +static int davinci_aemif_config_abus(struct davinci_aemif_cs_data *data) > +{ > + int ta, rhold, rstrobe, rsetup, whold, wstrobe, wsetup; > + unsigned offset; > + u32 set, val; > + > + offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4; > + > + ta = davinci_aemif_calc_rate(data->ta, aemif->clk_rate, TA_MAX); > + rhold = davinci_aemif_calc_rate(data->rhold, aemif->clk_rate, > + RHOLD_MAX); > + rstrobe = davinci_aemif_calc_rate(data->rstrobe, aemif->clk_rate, > + RSTROBE_MAX); > + rsetup = davinci_aemif_calc_rate(data->rsetup, aemif->clk_rate, > + RSETUP_MAX); > + whold = davinci_aemif_calc_rate(data->whold, aemif->clk_rate, > + WHOLD_MAX); > + wstrobe = davinci_aemif_calc_rate(data->wstrobe, aemif->clk_rate, > + WSTROBE_MAX); > + wsetup = davinci_aemif_calc_rate(data->wsetup, aemif->clk_rate, > + WSETUP_MAX); > + > + if (ta < 0 || rhold < 0 || rstrobe < 0 || rsetup < 0 || > + whold < 0 || wstrobe < 0 || wsetup < 0) { > + dev_err(aemif->dev, "%s: cannot get suitable timings\n", > + __func__); > + return -EINVAL; > + } > + > + set = TA(ta) | RHOLD(rhold) | RSTROBE(rstrobe) | RSETUP(rsetup) | > + WHOLD(whold) | WSTROBE(wstrobe) | WSETUP(wsetup); > + > + set |= (data->asize & ACR_ASIZE_MASK); > + if (data->enable_ew) > + set |= ACR_EW_MASK; > + if (data->enable_ss) > + set |= ACR_SS_MASK; > + > + val = readl(aemif->base + offset); > + val &= ~CONFIG_MASK; > + val |= set; > + writel(val, aemif->base + offset); > + > + return 0; > +} > + > +inline int davinci_aemif_cycles_to_nsec(int val) > +{ > + return ((val + 1) * NSEC_PER_MSEC) / aemif->clk_rate; > +} > + > +/** > + * davinci_aemif_get_hw_params - function to read hw register values > + * @data: ptr to cs data > + * > + * This function reads the defaults from the registers and update > + * the timing values. Required for get/set commands and also for > + * the case when driver needs to use defaults in hardware. > + */ > +static void davinci_aemif_get_hw_params(struct davinci_aemif_cs_data *data) > +{ > + u32 val, offset; > + offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4; > + > + val = readl(aemif->base + offset); > + data->ta = davinci_aemif_cycles_to_nsec(TA_VAL(val)); > + data->rhold = davinci_aemif_cycles_to_nsec(RHOLD_VAL(val)); > + data->rstrobe = davinci_aemif_cycles_to_nsec(RSTROBE_VAL(val)); > + data->rsetup = davinci_aemif_cycles_to_nsec(RSETUP_VAL(val)); > + data->whold = davinci_aemif_cycles_to_nsec(WHOLD_VAL(val)); > + data->wstrobe = davinci_aemif_cycles_to_nsec(WSTROBE_VAL(val)); > + data->wsetup = davinci_aemif_cycles_to_nsec(WSETUP_VAL(val)); > + data->enable_ew = EW_VAL(val); > + data->enable_ss = SS_VAL(val); > + data->asize = val & ASIZE_MAX; > +} > + > +/** > + * of_davinci_aemif_parse_abus_config - parse CS configuration from DT > + * @np: device node ptr > + * > + * This function update the emif async bus configuration based on the values > + * configured in a cs device binding node. > + */ > +static int of_davinci_aemif_parse_abus_config(struct device_node *np) > +{ > + struct davinci_aemif_cs_data *data; > + unsigned long cs; > + u32 val; > + > + if (kstrtoul(np->name + 2, 10, &cs) < 0) { > + dev_dbg(aemif->dev, "cs name is incorrect"); > + return -EINVAL; > + } > + > + if (cs - aemif->cs_offset >= NUM_CS || cs < aemif->cs_offset) { > + dev_dbg(aemif->dev, "cs number is incorrect %lu", cs); > + return -EINVAL; > + } > + > + if (aemif->num_cs >= NUM_CS) { > + dev_dbg(aemif->dev, "cs count is more than %d", NUM_CS); > + return -EINVAL; > + } > + > + data = &aemif->cs_data[aemif->num_cs++]; > + data->cs = cs; > + > + /* read the current value in the hw register */ > + davinci_aemif_get_hw_params(data); > + > + /* override the values from device node */ > + of_property_read_u8(np, "ti,davinci-cs-ta", &data->ta); > + of_property_read_u8(np, "ti,davinci-cs-rhold", &data->rhold); > + of_property_read_u16(np, "ti,davinci-cs-rstrobe", &data->rstrobe); > + of_property_read_u8(np, "ti,davinci-cs-rsetup", &data->rsetup); > + of_property_read_u8(np, "ti,davinci-cs-whold", &data->whold); > + of_property_read_u16(np, "ti,davinci-cs-wstrobe", &data->wstrobe); > + of_property_read_u8(np, "ti,davinci-cs-wsetup", &data->wsetup); > + if (!of_property_read_u32(np, "ti,bus-width", &val)) > + if (val == 16) > + data->asize = 1; > + data->enable_ew = of_property_read_bool(np, "ti,davinci-cs-ew"); > + data->enable_ss = of_property_read_bool(np, "ti,davinci-cs-ss"); > + return 0; > +} > + > +static const struct of_device_id davinci_aemif_of_match[] = { > + { .compatible = "ti,davinci-aemif", }, > + { .compatible = "ti,keystone-aemif", }, > + { .compatible = "ti,omap-L138-aemif", }, Please use ti,da850-aemif instead. We dont use the omap-like paper part number for these devices in kernel to avoid confusion with real OMAP devices. > + {}, > +}; > + > +static const struct of_device_id davinci_cs_of_match[] = { > + { .compatible = "ti,davinci-cs", }, > + {}, > +}; > + > +static int davinci_aemif_probe(struct platform_device *pdev) > +{ > + int ret = -ENODEV, i; > + struct resource *res; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + > + if (np == NULL) > + return 0; > + > + if (aemif) { > + dev_err(dev, "davinci_aemif driver is in use currently\n"); > + return -EBUSY; > + } Why expressly prevent multiple AEMIF devices? Its entirely conceivable to have two memories like NAND and NOR flash connect to two different AEMIF interfaces. > + > + aemif = devm_kzalloc(dev, sizeof(*aemif), GFP_KERNEL); > + > + if (!aemif) { > + dev_err(dev, "cannot allocate memory for aemif\n"); > + return -ENOMEM; > + } > + > + aemif->clk = devm_clk_get(dev, "aemif"); If this is the only clock you care about, you can use NULL for connection id. > + if (IS_ERR(aemif->clk)) { > + dev_err(dev, "cannot get clock 'aemif'\n"); > + return PTR_ERR(aemif->clk); > + } > + > + clk_prepare_enable(aemif->clk); > + aemif->clk_rate = clk_get_rate(aemif->clk) / 1000; > + > + aemif->dev = dev; > + > + if (of_device_is_compatible(np, "ti,omap-L138-aemif")) > + aemif->cs_offset = 2; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + aemif->base = devm_ioremap_resource(dev, res); > + if (IS_ERR(aemif->base)) { > + ret = PTR_ERR(aemif->base); > + goto error; > + } > + > + /* > + * For every controller device node, there is a cs device node that > + * describe the bus configuration parameters. This functions iterate > + * over these nodes and update the cs data array. > + */ > + for_each_matching_node(np, of_match_ptr(davinci_cs_of_match)) { > + ret = of_davinci_aemif_parse_abus_config(np); > + if (ret < 0) > + goto error; > + } > + > + for (i = 0; i < aemif->num_cs; i++) { > + ret = davinci_aemif_config_abus(&aemif->cs_data[i]); > + if (ret < 0) { > + dev_err(dev, "Error configuring chip select %d\n", > + aemif->cs_data[i].cs); > + goto error; > + } > + } > + > + /* > + * Create a child devices explicitly from here to > + * guarantee that the child will be probed after the AEMIF timing > + * parameters are set. > + */ > + for_each_matching_node(np, of_match_ptr(davinci_cs_of_match)) { > + ret = of_platform_populate(np, NULL, NULL, dev); > + if (ret < 0) > + goto error; > + } > + > + return 0; > +error: > + clk_disable_unprepare(aemif->clk); > + return ret; > +} > + > +static int davinci_aemif_remove(struct platform_device *pdev) > +{ > + clk_disable_unprepare(aemif->clk); > + return 0; > +} > + > +static struct platform_driver davinci_aemif_driver = { > + .probe = davinci_aemif_probe, > + .remove = davinci_aemif_remove, > + .driver = { > + .name = DRV_NAME, > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(davinci_aemif_of_match), > + }, > +}; > + > +module_platform_driver(davinci_aemif_driver); > + > +MODULE_AUTHOR("Murali Karicheri <m-karicheri2@ti.com>"); Is this correct? If yes, I would have expected him to sign-off on the patch too. If not, please drop this. Thanks, Sekhar
On Tuesday 26 November 2013 02:20 AM, Sekhar Nori wrote: > On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote: >> Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module >> is intended to provide a glue-less interface to a variety of >> asynchronous memory devices like ASRA M, NOR and NAND memory. A total >> of 256M bytes of any of these memories can be accessed at any given >> time via four chip selects with 64M byte access per chip select. >> >> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR >> are not supported. >> >> See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf >> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> >> --- [...] >> +static int davinci_aemif_probe(struct platform_device *pdev) >> +{ >> + int ret = -ENODEV, i; >> + struct resource *res; >> + struct device *dev = &pdev->dev; >> + struct device_node *np = dev->of_node; >> + >> + if (np == NULL) >> + return 0; >> + >> + if (aemif) { >> + dev_err(dev, "davinci_aemif driver is in use currently\n"); >> + return -EBUSY; >> + } > > Why expressly prevent multiple AEMIF devices? Its entirely conceivable > to have two memories like NAND and NOR flash connect to two different > AEMIF interfaces. > Ivan wanted me to clarify the Keystone hardware which supports 1 instance of controller with 4 CS. That allows already four devices to be connected. Currently NAND and NOR are connected on it and two more slots are free. Since driver support what hardware is, lets not build a driver for hardware which don't exist. And if at all such a support would be needed in future, we can always add it. Regards, Santosh
On 11/26/2013 8:35 PM, Santosh Shilimkar wrote: > On Tuesday 26 November 2013 02:20 AM, Sekhar Nori wrote: >> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote: >>> Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module >>> is intended to provide a glue-less interface to a variety of >>> asynchronous memory devices like ASRA M, NOR and NAND memory. A total >>> of 256M bytes of any of these memories can be accessed at any given >>> time via four chip selects with 64M byte access per chip select. >>> >>> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR >>> are not supported. >>> >>> See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf >>> >>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> >>> --- > > [...] > >>> +static int davinci_aemif_probe(struct platform_device *pdev) >>> +{ >>> + int ret = -ENODEV, i; >>> + struct resource *res; >>> + struct device *dev = &pdev->dev; >>> + struct device_node *np = dev->of_node; >>> + >>> + if (np == NULL) >>> + return 0; >>> + >>> + if (aemif) { >>> + dev_err(dev, "davinci_aemif driver is in use currently\n"); >>> + return -EBUSY; >>> + } >> >> Why expressly prevent multiple AEMIF devices? Its entirely conceivable >> to have two memories like NAND and NOR flash connect to two different >> AEMIF interfaces. >> > Ivan wanted me to clarify the Keystone hardware which supports > 1 instance of controller with 4 CS. That allows already four > devices to be connected. Currently NAND and NOR are connected on it > and two more slots are free. > > Since driver support what hardware is, lets not build a driver for > hardware which don't exist. And if at all such a support would be > needed in future, we can always add it. I understand the lack of hardware part, but its common to write the driver such that it can handle multiple instances. Is there any gain on current hardware because of this check? From what I am hearing, the code in question wont be exercised at all. So why go all the way and add it in first place? Thanks, Sekhar
On 11/26/2013 09:20 AM, Sekhar Nori wrote: > On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote: >> Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module >> is intended to provide a glue-less interface to a variety of >> asynchronous memory devices like ASRA M, NOR and NAND memory. A total >> of 256M bytes of any of these memories can be accessed at any given >> time via four chip selects with 64M byte access per chip select. >> >> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR >> are not supported. >> >> See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf >> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> >> --- >> drivers/memory/Kconfig | 11 ++ >> drivers/memory/Makefile | 1 + >> drivers/memory/davinci-aemif.c | 415 ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 427 insertions(+) >> create mode 100644 drivers/memory/davinci-aemif.c >> >> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig >> index 29a11db..010e75e 100644 >> --- a/drivers/memory/Kconfig >> +++ b/drivers/memory/Kconfig >> @@ -7,6 +7,17 @@ menuconfig MEMORY >> >> if MEMORY >> >> +config TI_DAVINCI_AEMIF >> + bool "Texas Instruments DaVinci AEMIF driver" > > This driver cannot be a module? If not, why not? > Yes it can be a module, I will correct it. >> + depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF >> + help >> + This driver is for the AEMIF module available in Texas Instruments >> + SoCs. AEMIF stands for Asynchronous External Memory Interface and >> + is intended to provide a glue-less interface to a variety of >> + asynchronuous memory devices like ASRAM, NOR and NAND memory. A total >> + of 256M bytes of any of these memories can be accessed at a given >> + time via four chip selects with 64M byte access per chip select. >> + >> config TI_EMIF >> tristate "Texas Instruments EMIF driver" >> depends on ARCH_OMAP2PLUS >> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile >> index 969d923..af14126 100644 >> --- a/drivers/memory/Makefile >> +++ b/drivers/memory/Makefile >> @@ -5,6 +5,7 @@ >> ifeq ($(CONFIG_DDR),y) >> obj-$(CONFIG_OF) += of_memory.o >> endif >> +obj-$(CONFIG_TI_DAVINCI_AEMIF) += davinci-aemif.o >> obj-$(CONFIG_TI_EMIF) += emif.o >> obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o >> obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o >> diff --git a/drivers/memory/davinci-aemif.c b/drivers/memory/davinci-aemif.c >> new file mode 100644 >> index 0000000..e36b74b >> --- /dev/null >> +++ b/drivers/memory/davinci-aemif.c >> @@ -0,0 +1,415 @@ >> +/* >> + * DaVinci/Keystone AEMIF driver >> + * >> + * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/ >> + * Copyright (C) Heiko Schocher <hs@denx.de> >> + * Copyright (C) Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> > > Just checking: You meant Author: ? Code is already copyrighted to TI by > line above. > It will be Author: Heiko Schocher <hs@denx.de> Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/err.h> >> +#include <linux/io.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_platform.h> >> +#include <linux/platform_device.h> >> + >> +#define TA_SHIFT 2 >> +#define RHOLD_SHIFT 4 >> +#define RSTROBE_SHIFT 7 >> +#define RSETUP_SHIFT 13 >> +#define WHOLD_SHIFT 17 >> +#define WSTROBE_SHIFT 20 >> +#define WSETUP_SHIFT 26 >> +#define EW_SHIFT 30 >> +#define SS_SHIFT 31 >> + >> +#define TA(x) ((x) << TA_SHIFT) >> +#define RHOLD(x) ((x) << RHOLD_SHIFT) >> +#define RSTROBE(x) ((x) << RSTROBE_SHIFT) >> +#define RSETUP(x) ((x) << RSETUP_SHIFT) >> +#define WHOLD(x) ((x) << WHOLD_SHIFT) >> +#define WSTROBE(x) ((x) << WSTROBE_SHIFT) >> +#define WSETUP(x) ((x) << WSETUP_SHIFT) >> +#define EW(x) ((x) << EW_SHIFT) >> +#define SS(x) ((x) << SS_SHIFT) >> + >> +#define ASIZE_MAX 0x1 >> +#define TA_MAX 0x3 >> +#define RHOLD_MAX 0x7 >> +#define RSTROBE_MAX 0x3f >> +#define RSETUP_MAX 0xf >> +#define WHOLD_MAX 0x7 >> +#define WSTROBE_MAX 0x3f >> +#define WSETUP_MAX 0xf >> +#define EW_MAX 0x1 >> +#define SS_MAX 0x1 >> +#define NUM_CS 4 >> + >> +#define TA_VAL(x) (((x) & TA(TA_MAX)) >> TA_SHIFT) >> +#define RHOLD_VAL(x) (((x) & RHOLD(RHOLD_MAX)) >> RHOLD_SHIFT) >> +#define RSTROBE_VAL(x) (((x) & RSTROBE(RSTROBE_MAX)) >> RSTROBE_SHIFT) >> +#define RSETUP_VAL(x) (((x) & RSETUP(RSETUP_MAX)) >> RSETUP_SHIFT) >> +#define WHOLD_VAL(x) (((x) & WHOLD(WHOLD_MAX)) >> WHOLD_SHIFT) >> +#define WSTROBE_VAL(x) (((x) & WSTROBE(WSTROBE_MAX)) >> WSTROBE_SHIFT) >> +#define WSETUP_VAL(x) (((x) & WSETUP(WSETUP_MAX)) >> WSETUP_SHIFT) >> +#define EW_VAL(x) (((x) & EW(EW_MAX)) >> EW_SHIFT) >> +#define SS_VAL(x) (((x) & SS(SS_MAX)) >> SS_SHIFT) >> + >> +#define NRCSR_OFFSET 0x00 >> +#define AWCCR_OFFSET 0x04 >> +#define A1CR_OFFSET 0x10 >> + >> +#define ACR_ASIZE_MASK 0x3 >> +#define ACR_EW_MASK BIT(30) >> +#define ACR_SS_MASK BIT(31) >> +#define ASIZE_16BIT 1 >> + >> +#define CONFIG_MASK (TA(TA_MAX) | \ >> + RHOLD(RHOLD_MAX) | \ >> + RSTROBE(RSTROBE_MAX) | \ >> + RSETUP(RSETUP_MAX) | \ >> + WHOLD(WHOLD_MAX) | \ >> + WSTROBE(WSTROBE_MAX) | \ >> + WSETUP(WSETUP_MAX) | \ >> + EW(EW_MAX) | SS(SS_MAX) | \ >> + ASIZE_MAX) >> + >> +#define DRV_NAME "davinci-aemif" >> + >> +/** >> + * structure to hold cs parameters >> + * @cs: chip-select number >> + * @wstrobe: write strobe width, ns >> + * @rstrobe: read strobe width, ns >> + * @wsetup: write setup width, ns >> + * @whold: write hold width, ns >> + * @rsetup: read setup width, ns >> + * @rhold: read hold width, ns >> + * @ta: minimum turn around time, ns >> + * @enable_ss: enable/disable select strobe mode >> + * @enable_ew: enable/disable extended wait mode >> + * @asize: width of the asynchronous device's data bus >> + */ >> +struct davinci_aemif_cs_data { >> + u8 cs; >> + u16 wstrobe; >> + u16 rstrobe; >> + u8 wsetup; >> + u8 whold; >> + u8 rsetup; >> + u8 rhold; >> + u8 ta; >> + u8 enable_ss; >> + u8 enable_ew; >> + u8 asize; >> +}; >> + >> +/** >> + * structure to hold device data >> + * @base: base address of AEMIF registers >> + * @clk: source clock >> + * @clk_rate: clock's rate in kHz >> + * @num_cs: number of assigned chip-selects >> + * @cs_data: array of chip-select settings >> + */ >> +struct davinci_aemif_device { >> + struct device *dev; >> + void __iomem *base; >> + struct clk *clk; >> + unsigned long clk_rate; >> + u8 num_cs; >> + int cs_offset; >> + struct davinci_aemif_cs_data cs_data[NUM_CS]; >> +}; >> + >> +static struct davinci_aemif_device *aemif; >> +/** >> + * davinci_aemif_calc_rate - calculate timing data. >> + * @wanted: The cycle time needed in nanoseconds. >> + * @clk: The input clock rate in kHz. >> + * @max: The maximum divider value that can be programmed. >> + * >> + * On success, returns the calculated timing value minus 1 for easy >> + * programming into AEMIF timing registers, else negative errno. >> + */ >> +static int davinci_aemif_calc_rate(int wanted, unsigned long clk, int max) >> +{ >> + int result; >> + >> + result = DIV_ROUND_UP((wanted * clk), NSEC_PER_MSEC) - 1; >> + >> + dev_dbg(aemif->dev, "%s: result %d from %ld, %d\n", __func__, result, >> + clk, wanted); >> + >> + /* It is generally OK to have a more relaxed timing than requested... */ >> + if (result < 0) >> + result = 0; >> + >> + /* ... But configuring tighter timings is not an option. */ >> + else if (result > max) >> + result = -EINVAL; >> + >> + return result; >> +} >> + >> +/** >> + * davinci_aemif_config_abus - configure async bus parameters >> + * @data: aemif chip select configuration >> + * >> + * This function programs the given timing values (in real clock) into the >> + * AEMIF registers taking the AEMIF clock into account. >> + * >> + * This function does not use any locking while programming the AEMIF >> + * because it is expected that there is only one user of a given >> + * chip-select. >> + * >> + * Returns 0 on success, else negative errno. >> + */ >> +static int davinci_aemif_config_abus(struct davinci_aemif_cs_data *data) >> +{ >> + int ta, rhold, rstrobe, rsetup, whold, wstrobe, wsetup; >> + unsigned offset; >> + u32 set, val; >> + >> + offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4; >> + >> + ta = davinci_aemif_calc_rate(data->ta, aemif->clk_rate, TA_MAX); >> + rhold = davinci_aemif_calc_rate(data->rhold, aemif->clk_rate, >> + RHOLD_MAX); >> + rstrobe = davinci_aemif_calc_rate(data->rstrobe, aemif->clk_rate, >> + RSTROBE_MAX); >> + rsetup = davinci_aemif_calc_rate(data->rsetup, aemif->clk_rate, >> + RSETUP_MAX); >> + whold = davinci_aemif_calc_rate(data->whold, aemif->clk_rate, >> + WHOLD_MAX); >> + wstrobe = davinci_aemif_calc_rate(data->wstrobe, aemif->clk_rate, >> + WSTROBE_MAX); >> + wsetup = davinci_aemif_calc_rate(data->wsetup, aemif->clk_rate, >> + WSETUP_MAX); >> + >> + if (ta < 0 || rhold < 0 || rstrobe < 0 || rsetup < 0 || >> + whold < 0 || wstrobe < 0 || wsetup < 0) { >> + dev_err(aemif->dev, "%s: cannot get suitable timings\n", >> + __func__); >> + return -EINVAL; >> + } >> + >> + set = TA(ta) | RHOLD(rhold) | RSTROBE(rstrobe) | RSETUP(rsetup) | >> + WHOLD(whold) | WSTROBE(wstrobe) | WSETUP(wsetup); >> + >> + set |= (data->asize & ACR_ASIZE_MASK); >> + if (data->enable_ew) >> + set |= ACR_EW_MASK; >> + if (data->enable_ss) >> + set |= ACR_SS_MASK; >> + >> + val = readl(aemif->base + offset); >> + val &= ~CONFIG_MASK; >> + val |= set; >> + writel(val, aemif->base + offset); >> + >> + return 0; >> +} >> + >> +inline int davinci_aemif_cycles_to_nsec(int val) >> +{ >> + return ((val + 1) * NSEC_PER_MSEC) / aemif->clk_rate; >> +} >> + >> +/** >> + * davinci_aemif_get_hw_params - function to read hw register values >> + * @data: ptr to cs data >> + * >> + * This function reads the defaults from the registers and update >> + * the timing values. Required for get/set commands and also for >> + * the case when driver needs to use defaults in hardware. >> + */ >> +static void davinci_aemif_get_hw_params(struct davinci_aemif_cs_data *data) >> +{ >> + u32 val, offset; >> + offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4; >> + >> + val = readl(aemif->base + offset); >> + data->ta = davinci_aemif_cycles_to_nsec(TA_VAL(val)); >> + data->rhold = davinci_aemif_cycles_to_nsec(RHOLD_VAL(val)); >> + data->rstrobe = davinci_aemif_cycles_to_nsec(RSTROBE_VAL(val)); >> + data->rsetup = davinci_aemif_cycles_to_nsec(RSETUP_VAL(val)); >> + data->whold = davinci_aemif_cycles_to_nsec(WHOLD_VAL(val)); >> + data->wstrobe = davinci_aemif_cycles_to_nsec(WSTROBE_VAL(val)); >> + data->wsetup = davinci_aemif_cycles_to_nsec(WSETUP_VAL(val)); >> + data->enable_ew = EW_VAL(val); >> + data->enable_ss = SS_VAL(val); >> + data->asize = val & ASIZE_MAX; >> +} >> + >> +/** >> + * of_davinci_aemif_parse_abus_config - parse CS configuration from DT >> + * @np: device node ptr >> + * >> + * This function update the emif async bus configuration based on the values >> + * configured in a cs device binding node. >> + */ >> +static int of_davinci_aemif_parse_abus_config(struct device_node *np) >> +{ >> + struct davinci_aemif_cs_data *data; >> + unsigned long cs; >> + u32 val; >> + >> + if (kstrtoul(np->name + 2, 10, &cs) < 0) { >> + dev_dbg(aemif->dev, "cs name is incorrect"); >> + return -EINVAL; >> + } >> + >> + if (cs - aemif->cs_offset >= NUM_CS || cs < aemif->cs_offset) { >> + dev_dbg(aemif->dev, "cs number is incorrect %lu", cs); >> + return -EINVAL; >> + } >> + >> + if (aemif->num_cs >= NUM_CS) { >> + dev_dbg(aemif->dev, "cs count is more than %d", NUM_CS); >> + return -EINVAL; >> + } >> + >> + data = &aemif->cs_data[aemif->num_cs++]; >> + data->cs = cs; >> + >> + /* read the current value in the hw register */ >> + davinci_aemif_get_hw_params(data); >> + >> + /* override the values from device node */ >> + of_property_read_u8(np, "ti,davinci-cs-ta", &data->ta); >> + of_property_read_u8(np, "ti,davinci-cs-rhold", &data->rhold); >> + of_property_read_u16(np, "ti,davinci-cs-rstrobe", &data->rstrobe); >> + of_property_read_u8(np, "ti,davinci-cs-rsetup", &data->rsetup); >> + of_property_read_u8(np, "ti,davinci-cs-whold", &data->whold); >> + of_property_read_u16(np, "ti,davinci-cs-wstrobe", &data->wstrobe); >> + of_property_read_u8(np, "ti,davinci-cs-wsetup", &data->wsetup); >> + if (!of_property_read_u32(np, "ti,bus-width", &val)) >> + if (val == 16) >> + data->asize = 1; >> + data->enable_ew = of_property_read_bool(np, "ti,davinci-cs-ew"); >> + data->enable_ss = of_property_read_bool(np, "ti,davinci-cs-ss"); >> + return 0; >> +} >> + >> +static const struct of_device_id davinci_aemif_of_match[] = { >> + { .compatible = "ti,davinci-aemif", }, >> + { .compatible = "ti,keystone-aemif", }, >> + { .compatible = "ti,omap-L138-aemif", }, > > Please use ti,da850-aemif instead. We dont use the omap-like paper part > number for these devices in kernel to avoid confusion with real OMAP > devices. > Ok, I'll replace ti,omap-L138-aemif -> ti,da850-aemif >> + {}, >> +}; >> + >> +static const struct of_device_id davinci_cs_of_match[] = { >> + { .compatible = "ti,davinci-cs", }, >> + {}, >> +}; >> + >> +static int davinci_aemif_probe(struct platform_device *pdev) >> +{ >> + int ret = -ENODEV, i; >> + struct resource *res; >> + struct device *dev = &pdev->dev; >> + struct device_node *np = dev->of_node; >> + >> + if (np == NULL) >> + return 0; >> + >> + if (aemif) { >> + dev_err(dev, "davinci_aemif driver is in use currently\n"); >> + return -EBUSY; >> + } > > Why expressly prevent multiple AEMIF devices? Its entirely conceivable > to have two memories like NAND and NOR flash connect to two different > AEMIF interfaces. > It can be, but I'm not sure if it is needed. Currently I've not seen case where more than 2 cses were used, I mean we have 2 cs free, why do we need the second AEMIF controller? >> + >> + aemif = devm_kzalloc(dev, sizeof(*aemif), GFP_KERNEL); >> + >> + if (!aemif) { >> + dev_err(dev, "cannot allocate memory for aemif\n"); >> + return -ENOMEM; >> + } >> + >> + aemif->clk = devm_clk_get(dev, "aemif"); > > If this is the only clock you care about, you can use NULL for > connection id. > Yes I can, I'll replace aemif name on NULL. >> + if (IS_ERR(aemif->clk)) { >> + dev_err(dev, "cannot get clock 'aemif'\n"); >> + return PTR_ERR(aemif->clk); >> + } >> + >> + clk_prepare_enable(aemif->clk); >> + aemif->clk_rate = clk_get_rate(aemif->clk) / 1000; >> + >> + aemif->dev = dev; >> + >> + if (of_device_is_compatible(np, "ti,omap-L138-aemif")) >> + aemif->cs_offset = 2; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + aemif->base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(aemif->base)) { >> + ret = PTR_ERR(aemif->base); >> + goto error; >> + } >> + >> + /* >> + * For every controller device node, there is a cs device node that >> + * describe the bus configuration parameters. This functions iterate >> + * over these nodes and update the cs data array. >> + */ >> + for_each_matching_node(np, of_match_ptr(davinci_cs_of_match)) { >> + ret = of_davinci_aemif_parse_abus_config(np); >> + if (ret < 0) >> + goto error; >> + } >> + >> + for (i = 0; i < aemif->num_cs; i++) { >> + ret = davinci_aemif_config_abus(&aemif->cs_data[i]); >> + if (ret < 0) { >> + dev_err(dev, "Error configuring chip select %d\n", >> + aemif->cs_data[i].cs); >> + goto error; >> + } >> + } >> + >> + /* >> + * Create a child devices explicitly from here to >> + * guarantee that the child will be probed after the AEMIF timing >> + * parameters are set. >> + */ >> + for_each_matching_node(np, of_match_ptr(davinci_cs_of_match)) { >> + ret = of_platform_populate(np, NULL, NULL, dev); >> + if (ret < 0) >> + goto error; >> + } >> + >> + return 0; >> +error: >> + clk_disable_unprepare(aemif->clk); >> + return ret; >> +} >> + >> +static int davinci_aemif_remove(struct platform_device *pdev) >> +{ >> + clk_disable_unprepare(aemif->clk); >> + return 0; >> +} >> + >> +static struct platform_driver davinci_aemif_driver = { >> + .probe = davinci_aemif_probe, >> + .remove = davinci_aemif_remove, >> + .driver = { >> + .name = DRV_NAME, >> + .owner = THIS_MODULE, >> + .of_match_table = of_match_ptr(davinci_aemif_of_match), >> + }, >> +}; >> + >> +module_platform_driver(davinci_aemif_driver); >> + >> +MODULE_AUTHOR("Murali Karicheri <m-karicheri2@ti.com>"); > > Is this correct? If yes, I would have expected him to sign-off on the > patch too. If not, please drop this. I will add sign-off > > Thanks, > Sekhar >
On Tuesday 26 November 2013 12:21 PM, Sekhar Nori wrote: > On 11/26/2013 8:35 PM, Santosh Shilimkar wrote: >> On Tuesday 26 November 2013 02:20 AM, Sekhar Nori wrote: >>> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote: >>>> Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module >>>> is intended to provide a glue-less interface to a variety of >>>> asynchronous memory devices like ASRA M, NOR and NAND memory. A total >>>> of 256M bytes of any of these memories can be accessed at any given >>>> time via four chip selects with 64M byte access per chip select. >>>> >>>> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR >>>> are not supported. >>>> >>>> See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf >>>> >>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> >>>> --- >> >> [...] >> >>>> +static int davinci_aemif_probe(struct platform_device *pdev) >>>> +{ >>>> + int ret = -ENODEV, i; >>>> + struct resource *res; >>>> + struct device *dev = &pdev->dev; >>>> + struct device_node *np = dev->of_node; >>>> + >>>> + if (np == NULL) >>>> + return 0; >>>> + >>>> + if (aemif) { >>>> + dev_err(dev, "davinci_aemif driver is in use currently\n"); >>>> + return -EBUSY; >>>> + } >>> >>> Why expressly prevent multiple AEMIF devices? Its entirely conceivable >>> to have two memories like NAND and NOR flash connect to two different >>> AEMIF interfaces. >>> >> Ivan wanted me to clarify the Keystone hardware which supports >> 1 instance of controller with 4 CS. That allows already four >> devices to be connected. Currently NAND and NOR are connected on it >> and two more slots are free. >> >> Since driver support what hardware is, lets not build a driver for >> hardware which don't exist. And if at all such a support would be >> needed in future, we can always add it. > > I understand the lack of hardware part, but its common to write the > driver such that it can handle multiple instances. Is there any gain on > current hardware because of this check? From what I am hearing, the code > in question wont be exercised at all. So why go all the way and add it > in first place? > Fair enough. The check can be dropped. Regards, Santosh
On 11/26/2013 08:26 PM, Santosh Shilimkar wrote:
> Fair enough
Ok, I will do
Ivan, On Tuesday 26 November 2013 12:44 PM, ivan.khoronzhuk wrote: > On 11/26/2013 09:20 AM, Sekhar Nori wrote: >> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote: >>> +static struct platform_driver davinci_aemif_driver = { >>> + .probe = davinci_aemif_probe, >>> + .remove = davinci_aemif_remove, >>> + .driver = { >>> + .name = DRV_NAME, >>> + .owner = THIS_MODULE, >>> + .of_match_table = of_match_ptr(davinci_aemif_of_match), >>> + }, >>> +}; >>> + >>> +module_platform_driver(davinci_aemif_driver); >>> + >>> +MODULE_AUTHOR("Murali Karicheri <m-karicheri2@ti.com>"); >> >> Is this correct? If yes, I would have expected him to sign-off on the >> patch too. If not, please drop this. > > I will add sign-off > Please add you as a MODULE_AUTHOR as well since you re-wrote the downstream driver. Ok to have both of you sign of on it. Regards, Santosh
On 11/26/2013 08:30 PM, Santosh Shilimkar wrote: > Ivan, > > On Tuesday 26 November 2013 12:44 PM, ivan.khoronzhuk wrote: >> On 11/26/2013 09:20 AM, Sekhar Nori wrote: >>> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote: > >>>> +static struct platform_driver davinci_aemif_driver = { >>>> + .probe = davinci_aemif_probe, >>>> + .remove = davinci_aemif_remove, >>>> + .driver = { >>>> + .name = DRV_NAME, >>>> + .owner = THIS_MODULE, >>>> + .of_match_table = of_match_ptr(davinci_aemif_of_match), >>>> + }, >>>> +}; >>>> + >>>> +module_platform_driver(davinci_aemif_driver); >>>> + >>>> +MODULE_AUTHOR("Murali Karicheri <m-karicheri2@ti.com>"); >>> >>> Is this correct? If yes, I would have expected him to sign-off on the >>> patch too. If not, please drop this. >> >> I will add sign-off >> > Please add you as a MODULE_AUTHOR as well since you re-wrote the > downstream driver. Ok to have both of you sign of on it. > > Regards, > Santosh > Ok
On Tue, Nov 26, 2013 at 01:26:44PM -0500, Santosh Shilimkar wrote: > On Tuesday 26 November 2013 12:21 PM, Sekhar Nori wrote: > > On 11/26/2013 8:35 PM, Santosh Shilimkar wrote: > >> On Tuesday 26 November 2013 02:20 AM, Sekhar Nori wrote: > >>> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote: > >>>> +static int davinci_aemif_probe(struct platform_device *pdev) > >>>> +{ > >>>> + int ret = -ENODEV, i; > >>>> + struct resource *res; > >>>> + struct device *dev = &pdev->dev; > >>>> + struct device_node *np = dev->of_node; > >>>> + > >>>> + if (np == NULL) > >>>> + return 0; > >>>> + > >>>> + if (aemif) { > >>>> + dev_err(dev, "davinci_aemif driver is in use currently\n"); > >>>> + return -EBUSY; > >>>> + } > >>> > >>> Why expressly prevent multiple AEMIF devices? Its entirely conceivable > >>> to have two memories like NAND and NOR flash connect to two different > >>> AEMIF interfaces. > >>> > >> Ivan wanted me to clarify the Keystone hardware which supports > >> 1 instance of controller with 4 CS. That allows already four > >> devices to be connected. Currently NAND and NOR are connected on it > >> and two more slots are free. > >> > >> Since driver support what hardware is, lets not build a driver for > >> hardware which don't exist. And if at all such a support would be > >> needed in future, we can always add it. > > > > I understand the lack of hardware part, but its common to write the > > driver such that it can handle multiple instances. Is there any gain on > > current hardware because of this check? From what I am hearing, the code > > in question wont be exercised at all. So why go all the way and add it > > in first place? > > > Fair enough. The check can be dropped. Hmm, while the sentiment expressed by Sekhar is noble (to avoid unnecessarily constraining the driver), removing the check is not enough. You're still using a global static variable 'aemif', and it is important not to accidentally re-use this struct if a second device ever becomes available. So for the current implementation, the check is necessary IMO. If instead, you were to make 'aemif' a per-device struct (like with platform_set_drvdata()), then you would not have this issue. Brian
On Tuesday 26 November 2013 11:14 PM, ivan.khoronzhuk wrote: >>> +static int davinci_aemif_probe(struct platform_device *pdev) >>> +{ >>> + int ret = -ENODEV, i; >>> + struct resource *res; >>> + struct device *dev = &pdev->dev; >>> + struct device_node *np = dev->of_node; >>> + >>> + if (np == NULL) >>> + return 0; >>> + >>> + if (aemif) { >>> + dev_err(dev, "davinci_aemif driver is in use currently\n"); >>> + return -EBUSY; >>> + } >> >> Why expressly prevent multiple AEMIF devices? Its entirely conceivable >> to have two memories like NAND and NOR flash connect to two different >> AEMIF interfaces. >> > > It can be, but I'm not sure if it is needed. Currently I've not seen case where > more than 2 cses were used, I mean we have 2 cs free, why do we need the second AEMIF > controller? One usual reason is pinmux constraints. Its probably not a concern on the device you are working with right now but as devices get smaller, functionality on pins is multiplexed to handle multiple different use cases. Thanks, Sekhar
On 11/27/2013 02:37 AM, Brian Norris wrote: > On Tue, Nov 26, 2013 at 01:26:44PM -0500, Santosh Shilimkar wrote: >> On Tuesday 26 November 2013 12:21 PM, Sekhar Nori wrote: >>> On 11/26/2013 8:35 PM, Santosh Shilimkar wrote: >>>> On Tuesday 26 November 2013 02:20 AM, Sekhar Nori wrote: >>>>> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote: >>>>>> +static int davinci_aemif_probe(struct platform_device *pdev) >>>>>> +{ >>>>>> + int ret = -ENODEV, i; >>>>>> + struct resource *res; >>>>>> + struct device *dev = &pdev->dev; >>>>>> + struct device_node *np = dev->of_node; >>>>>> + >>>>>> + if (np == NULL) >>>>>> + return 0; >>>>>> + >>>>>> + if (aemif) { >>>>>> + dev_err(dev, "davinci_aemif driver is in use currently\n"); >>>>>> + return -EBUSY; >>>>>> + } >>>>> >>>>> Why expressly prevent multiple AEMIF devices? Its entirely conceivable >>>>> to have two memories like NAND and NOR flash connect to two different >>>>> AEMIF interfaces. >>>>> >>>> Ivan wanted me to clarify the Keystone hardware which supports >>>> 1 instance of controller with 4 CS. That allows already four >>>> devices to be connected. Currently NAND and NOR are connected on it >>>> and two more slots are free. >>>> >>>> Since driver support what hardware is, lets not build a driver for >>>> hardware which don't exist. And if at all such a support would be >>>> needed in future, we can always add it. >>> >>> I understand the lack of hardware part, but its common to write the >>> driver such that it can handle multiple instances. Is there any gain on >>> current hardware because of this check? From what I am hearing, the code >>> in question wont be exercised at all. So why go all the way and add it >>> in first place? >>> >> Fair enough. The check can be dropped. > > Hmm, while the sentiment expressed by Sekhar is noble (to avoid > unnecessarily constraining the driver), removing the check is not > enough. You're still using a global static variable 'aemif', and it is > important not to accidentally re-use this struct if a second device ever > becomes available. So for the current implementation, the check is > necessary IMO. If instead, you were to make 'aemif' a per-device struct > (like with platform_set_drvdata()), then you would not have this issue. > > Brian > Yes, that is the plan to make it a per-device.
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig index 29a11db..010e75e 100644 --- a/drivers/memory/Kconfig +++ b/drivers/memory/Kconfig @@ -7,6 +7,17 @@ menuconfig MEMORY if MEMORY +config TI_DAVINCI_AEMIF + bool "Texas Instruments DaVinci AEMIF driver" + depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF + help + This driver is for the AEMIF module available in Texas Instruments + SoCs. AEMIF stands for Asynchronous External Memory Interface and + is intended to provide a glue-less interface to a variety of + asynchronuous memory devices like ASRAM, NOR and NAND memory. A total + of 256M bytes of any of these memories can be accessed at a given + time via four chip selects with 64M byte access per chip select. + config TI_EMIF tristate "Texas Instruments EMIF driver" depends on ARCH_OMAP2PLUS diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile index 969d923..af14126 100644 --- a/drivers/memory/Makefile +++ b/drivers/memory/Makefile @@ -5,6 +5,7 @@ ifeq ($(CONFIG_DDR),y) obj-$(CONFIG_OF) += of_memory.o endif +obj-$(CONFIG_TI_DAVINCI_AEMIF) += davinci-aemif.o obj-$(CONFIG_TI_EMIF) += emif.o obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o diff --git a/drivers/memory/davinci-aemif.c b/drivers/memory/davinci-aemif.c new file mode 100644 index 0000000..e36b74b --- /dev/null +++ b/drivers/memory/davinci-aemif.c @@ -0,0 +1,415 @@ +/* + * DaVinci/Keystone AEMIF driver + * + * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/ + * Copyright (C) Heiko Schocher <hs@denx.de> + * Copyright (C) Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> + +#define TA_SHIFT 2 +#define RHOLD_SHIFT 4 +#define RSTROBE_SHIFT 7 +#define RSETUP_SHIFT 13 +#define WHOLD_SHIFT 17 +#define WSTROBE_SHIFT 20 +#define WSETUP_SHIFT 26 +#define EW_SHIFT 30 +#define SS_SHIFT 31 + +#define TA(x) ((x) << TA_SHIFT) +#define RHOLD(x) ((x) << RHOLD_SHIFT) +#define RSTROBE(x) ((x) << RSTROBE_SHIFT) +#define RSETUP(x) ((x) << RSETUP_SHIFT) +#define WHOLD(x) ((x) << WHOLD_SHIFT) +#define WSTROBE(x) ((x) << WSTROBE_SHIFT) +#define WSETUP(x) ((x) << WSETUP_SHIFT) +#define EW(x) ((x) << EW_SHIFT) +#define SS(x) ((x) << SS_SHIFT) + +#define ASIZE_MAX 0x1 +#define TA_MAX 0x3 +#define RHOLD_MAX 0x7 +#define RSTROBE_MAX 0x3f +#define RSETUP_MAX 0xf +#define WHOLD_MAX 0x7 +#define WSTROBE_MAX 0x3f +#define WSETUP_MAX 0xf +#define EW_MAX 0x1 +#define SS_MAX 0x1 +#define NUM_CS 4 + +#define TA_VAL(x) (((x) & TA(TA_MAX)) >> TA_SHIFT) +#define RHOLD_VAL(x) (((x) & RHOLD(RHOLD_MAX)) >> RHOLD_SHIFT) +#define RSTROBE_VAL(x) (((x) & RSTROBE(RSTROBE_MAX)) >> RSTROBE_SHIFT) +#define RSETUP_VAL(x) (((x) & RSETUP(RSETUP_MAX)) >> RSETUP_SHIFT) +#define WHOLD_VAL(x) (((x) & WHOLD(WHOLD_MAX)) >> WHOLD_SHIFT) +#define WSTROBE_VAL(x) (((x) & WSTROBE(WSTROBE_MAX)) >> WSTROBE_SHIFT) +#define WSETUP_VAL(x) (((x) & WSETUP(WSETUP_MAX)) >> WSETUP_SHIFT) +#define EW_VAL(x) (((x) & EW(EW_MAX)) >> EW_SHIFT) +#define SS_VAL(x) (((x) & SS(SS_MAX)) >> SS_SHIFT) + +#define NRCSR_OFFSET 0x00 +#define AWCCR_OFFSET 0x04 +#define A1CR_OFFSET 0x10 + +#define ACR_ASIZE_MASK 0x3 +#define ACR_EW_MASK BIT(30) +#define ACR_SS_MASK BIT(31) +#define ASIZE_16BIT 1 + +#define CONFIG_MASK (TA(TA_MAX) | \ + RHOLD(RHOLD_MAX) | \ + RSTROBE(RSTROBE_MAX) | \ + RSETUP(RSETUP_MAX) | \ + WHOLD(WHOLD_MAX) | \ + WSTROBE(WSTROBE_MAX) | \ + WSETUP(WSETUP_MAX) | \ + EW(EW_MAX) | SS(SS_MAX) | \ + ASIZE_MAX) + +#define DRV_NAME "davinci-aemif" + +/** + * structure to hold cs parameters + * @cs: chip-select number + * @wstrobe: write strobe width, ns + * @rstrobe: read strobe width, ns + * @wsetup: write setup width, ns + * @whold: write hold width, ns + * @rsetup: read setup width, ns + * @rhold: read hold width, ns + * @ta: minimum turn around time, ns + * @enable_ss: enable/disable select strobe mode + * @enable_ew: enable/disable extended wait mode + * @asize: width of the asynchronous device's data bus + */ +struct davinci_aemif_cs_data { + u8 cs; + u16 wstrobe; + u16 rstrobe; + u8 wsetup; + u8 whold; + u8 rsetup; + u8 rhold; + u8 ta; + u8 enable_ss; + u8 enable_ew; + u8 asize; +}; + +/** + * structure to hold device data + * @base: base address of AEMIF registers + * @clk: source clock + * @clk_rate: clock's rate in kHz + * @num_cs: number of assigned chip-selects + * @cs_data: array of chip-select settings + */ +struct davinci_aemif_device { + struct device *dev; + void __iomem *base; + struct clk *clk; + unsigned long clk_rate; + u8 num_cs; + int cs_offset; + struct davinci_aemif_cs_data cs_data[NUM_CS]; +}; + +static struct davinci_aemif_device *aemif; +/** + * davinci_aemif_calc_rate - calculate timing data. + * @wanted: The cycle time needed in nanoseconds. + * @clk: The input clock rate in kHz. + * @max: The maximum divider value that can be programmed. + * + * On success, returns the calculated timing value minus 1 for easy + * programming into AEMIF timing registers, else negative errno. + */ +static int davinci_aemif_calc_rate(int wanted, unsigned long clk, int max) +{ + int result; + + result = DIV_ROUND_UP((wanted * clk), NSEC_PER_MSEC) - 1; + + dev_dbg(aemif->dev, "%s: result %d from %ld, %d\n", __func__, result, + clk, wanted); + + /* It is generally OK to have a more relaxed timing than requested... */ + if (result < 0) + result = 0; + + /* ... But configuring tighter timings is not an option. */ + else if (result > max) + result = -EINVAL; + + return result; +} + +/** + * davinci_aemif_config_abus - configure async bus parameters + * @data: aemif chip select configuration + * + * This function programs the given timing values (in real clock) into the + * AEMIF registers taking the AEMIF clock into account. + * + * This function does not use any locking while programming the AEMIF + * because it is expected that there is only one user of a given + * chip-select. + * + * Returns 0 on success, else negative errno. + */ +static int davinci_aemif_config_abus(struct davinci_aemif_cs_data *data) +{ + int ta, rhold, rstrobe, rsetup, whold, wstrobe, wsetup; + unsigned offset; + u32 set, val; + + offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4; + + ta = davinci_aemif_calc_rate(data->ta, aemif->clk_rate, TA_MAX); + rhold = davinci_aemif_calc_rate(data->rhold, aemif->clk_rate, + RHOLD_MAX); + rstrobe = davinci_aemif_calc_rate(data->rstrobe, aemif->clk_rate, + RSTROBE_MAX); + rsetup = davinci_aemif_calc_rate(data->rsetup, aemif->clk_rate, + RSETUP_MAX); + whold = davinci_aemif_calc_rate(data->whold, aemif->clk_rate, + WHOLD_MAX); + wstrobe = davinci_aemif_calc_rate(data->wstrobe, aemif->clk_rate, + WSTROBE_MAX); + wsetup = davinci_aemif_calc_rate(data->wsetup, aemif->clk_rate, + WSETUP_MAX); + + if (ta < 0 || rhold < 0 || rstrobe < 0 || rsetup < 0 || + whold < 0 || wstrobe < 0 || wsetup < 0) { + dev_err(aemif->dev, "%s: cannot get suitable timings\n", + __func__); + return -EINVAL; + } + + set = TA(ta) | RHOLD(rhold) | RSTROBE(rstrobe) | RSETUP(rsetup) | + WHOLD(whold) | WSTROBE(wstrobe) | WSETUP(wsetup); + + set |= (data->asize & ACR_ASIZE_MASK); + if (data->enable_ew) + set |= ACR_EW_MASK; + if (data->enable_ss) + set |= ACR_SS_MASK; + + val = readl(aemif->base + offset); + val &= ~CONFIG_MASK; + val |= set; + writel(val, aemif->base + offset); + + return 0; +} + +inline int davinci_aemif_cycles_to_nsec(int val) +{ + return ((val + 1) * NSEC_PER_MSEC) / aemif->clk_rate; +} + +/** + * davinci_aemif_get_hw_params - function to read hw register values + * @data: ptr to cs data + * + * This function reads the defaults from the registers and update + * the timing values. Required for get/set commands and also for + * the case when driver needs to use defaults in hardware. + */ +static void davinci_aemif_get_hw_params(struct davinci_aemif_cs_data *data) +{ + u32 val, offset; + offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4; + + val = readl(aemif->base + offset); + data->ta = davinci_aemif_cycles_to_nsec(TA_VAL(val)); + data->rhold = davinci_aemif_cycles_to_nsec(RHOLD_VAL(val)); + data->rstrobe = davinci_aemif_cycles_to_nsec(RSTROBE_VAL(val)); + data->rsetup = davinci_aemif_cycles_to_nsec(RSETUP_VAL(val)); + data->whold = davinci_aemif_cycles_to_nsec(WHOLD_VAL(val)); + data->wstrobe = davinci_aemif_cycles_to_nsec(WSTROBE_VAL(val)); + data->wsetup = davinci_aemif_cycles_to_nsec(WSETUP_VAL(val)); + data->enable_ew = EW_VAL(val); + data->enable_ss = SS_VAL(val); + data->asize = val & ASIZE_MAX; +} + +/** + * of_davinci_aemif_parse_abus_config - parse CS configuration from DT + * @np: device node ptr + * + * This function update the emif async bus configuration based on the values + * configured in a cs device binding node. + */ +static int of_davinci_aemif_parse_abus_config(struct device_node *np) +{ + struct davinci_aemif_cs_data *data; + unsigned long cs; + u32 val; + + if (kstrtoul(np->name + 2, 10, &cs) < 0) { + dev_dbg(aemif->dev, "cs name is incorrect"); + return -EINVAL; + } + + if (cs - aemif->cs_offset >= NUM_CS || cs < aemif->cs_offset) { + dev_dbg(aemif->dev, "cs number is incorrect %lu", cs); + return -EINVAL; + } + + if (aemif->num_cs >= NUM_CS) { + dev_dbg(aemif->dev, "cs count is more than %d", NUM_CS); + return -EINVAL; + } + + data = &aemif->cs_data[aemif->num_cs++]; + data->cs = cs; + + /* read the current value in the hw register */ + davinci_aemif_get_hw_params(data); + + /* override the values from device node */ + of_property_read_u8(np, "ti,davinci-cs-ta", &data->ta); + of_property_read_u8(np, "ti,davinci-cs-rhold", &data->rhold); + of_property_read_u16(np, "ti,davinci-cs-rstrobe", &data->rstrobe); + of_property_read_u8(np, "ti,davinci-cs-rsetup", &data->rsetup); + of_property_read_u8(np, "ti,davinci-cs-whold", &data->whold); + of_property_read_u16(np, "ti,davinci-cs-wstrobe", &data->wstrobe); + of_property_read_u8(np, "ti,davinci-cs-wsetup", &data->wsetup); + if (!of_property_read_u32(np, "ti,bus-width", &val)) + if (val == 16) + data->asize = 1; + data->enable_ew = of_property_read_bool(np, "ti,davinci-cs-ew"); + data->enable_ss = of_property_read_bool(np, "ti,davinci-cs-ss"); + return 0; +} + +static const struct of_device_id davinci_aemif_of_match[] = { + { .compatible = "ti,davinci-aemif", }, + { .compatible = "ti,keystone-aemif", }, + { .compatible = "ti,omap-L138-aemif", }, + {}, +}; + +static const struct of_device_id davinci_cs_of_match[] = { + { .compatible = "ti,davinci-cs", }, + {}, +}; + +static int davinci_aemif_probe(struct platform_device *pdev) +{ + int ret = -ENODEV, i; + struct resource *res; + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + + if (np == NULL) + return 0; + + if (aemif) { + dev_err(dev, "davinci_aemif driver is in use currently\n"); + return -EBUSY; + } + + aemif = devm_kzalloc(dev, sizeof(*aemif), GFP_KERNEL); + + if (!aemif) { + dev_err(dev, "cannot allocate memory for aemif\n"); + return -ENOMEM; + } + + aemif->clk = devm_clk_get(dev, "aemif"); + if (IS_ERR(aemif->clk)) { + dev_err(dev, "cannot get clock 'aemif'\n"); + return PTR_ERR(aemif->clk); + } + + clk_prepare_enable(aemif->clk); + aemif->clk_rate = clk_get_rate(aemif->clk) / 1000; + + aemif->dev = dev; + + if (of_device_is_compatible(np, "ti,omap-L138-aemif")) + aemif->cs_offset = 2; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + aemif->base = devm_ioremap_resource(dev, res); + if (IS_ERR(aemif->base)) { + ret = PTR_ERR(aemif->base); + goto error; + } + + /* + * For every controller device node, there is a cs device node that + * describe the bus configuration parameters. This functions iterate + * over these nodes and update the cs data array. + */ + for_each_matching_node(np, of_match_ptr(davinci_cs_of_match)) { + ret = of_davinci_aemif_parse_abus_config(np); + if (ret < 0) + goto error; + } + + for (i = 0; i < aemif->num_cs; i++) { + ret = davinci_aemif_config_abus(&aemif->cs_data[i]); + if (ret < 0) { + dev_err(dev, "Error configuring chip select %d\n", + aemif->cs_data[i].cs); + goto error; + } + } + + /* + * Create a child devices explicitly from here to + * guarantee that the child will be probed after the AEMIF timing + * parameters are set. + */ + for_each_matching_node(np, of_match_ptr(davinci_cs_of_match)) { + ret = of_platform_populate(np, NULL, NULL, dev); + if (ret < 0) + goto error; + } + + return 0; +error: + clk_disable_unprepare(aemif->clk); + return ret; +} + +static int davinci_aemif_remove(struct platform_device *pdev) +{ + clk_disable_unprepare(aemif->clk); + return 0; +} + +static struct platform_driver davinci_aemif_driver = { + .probe = davinci_aemif_probe, + .remove = davinci_aemif_remove, + .driver = { + .name = DRV_NAME, + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(davinci_aemif_of_match), + }, +}; + +module_platform_driver(davinci_aemif_driver); + +MODULE_AUTHOR("Murali Karicheri <m-karicheri2@ti.com>"); +MODULE_AUTHOR("Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>"); +MODULE_DESCRIPTION("Texas Instruments AEMIF driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:" DRV_NAME);
Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module is intended to provide a glue-less interface to a variety of asynchronous memory devices like ASRA M, NOR and NAND memory. A total of 256M bytes of any of these memories can be accessed at any given time via four chip selects with 64M byte access per chip select. Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR are not supported. See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> --- drivers/memory/Kconfig | 11 ++ drivers/memory/Makefile | 1 + drivers/memory/davinci-aemif.c | 415 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 427 insertions(+) create mode 100644 drivers/memory/davinci-aemif.c -- 1.7.9.5