From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <528A6757.3020502@ti.com> Date: Mon, 18 Nov 2013 21:15:35 +0200 From: "ivan.khoronzhuk" MIME-Version: 1.0 To: Santosh Shilimkar Subject: Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver References: <1384187188-5776-1-git-send-email-ivan.khoronzhuk@ti.com>, <1384187188-5776-8-git-send-email-ivan.khoronzhuk@ti.com> <4F5844B3A985794BA902E12C070812375F8D2E@DNCE04.ent.ti.com> <52825283.7000905@ti.com> In-Reply-To: <52825283.7000905@ti.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Cc: Mark Rutland , "devicetree@vger.kernel.org" , "Strashko, Grygorii" , Russell King , Pawel Moll , Stephen Warren , Greg Kroah-Hartman , Ian Campbell , Kumar Gala , Rob Herring , "linux-kernel@vger.kernel.org" , "linux-mtd@lists.infradead.org" , Rob Landley , "linux-arm-kernel@lists.infradead.org" List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 >> --- >> 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 >> + * Copyright (C) Ivan Khoronzhuk >> + * >> + * 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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 > -- Regards, Ivan Khoronzhuk From mboxrd@z Thu Jan 1 00:00:00 1970 From: ivan.khoronzhuk@ti.com (ivan.khoronzhuk) Date: Mon, 18 Nov 2013 21:15:35 +0200 Subject: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver In-Reply-To: <52825283.7000905@ti.com> References: <1384187188-5776-1-git-send-email-ivan.khoronzhuk@ti.com>, <1384187188-5776-8-git-send-email-ivan.khoronzhuk@ti.com> <4F5844B3A985794BA902E12C070812375F8D2E@DNCE04.ent.ti.com> <52825283.7000905@ti.com> Message-ID: <528A6757.3020502@ti.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 >> --- >> 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 >> + * Copyright (C) Ivan Khoronzhuk >> + * >> + * 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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 > -- Regards, Ivan Khoronzhuk From mboxrd@z Thu Jan 1 00:00:00 1970 From: "ivan.khoronzhuk" Subject: Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver Date: Mon, 18 Nov 2013 21:15:35 +0200 Message-ID: <528A6757.3020502@ti.com> References: <1384187188-5776-1-git-send-email-ivan.khoronzhuk@ti.com>, <1384187188-5776-8-git-send-email-ivan.khoronzhuk@ti.com> <4F5844B3A985794BA902E12C070812375F8D2E@DNCE04.ent.ti.com> <52825283.7000905@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52825283.7000905@ti.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-mtd" Errors-To: linux-mtd-bounces+gldm-linux-mtd-36=gmane.org@lists.infradead.org To: Santosh Shilimkar Cc: Mark Rutland , "devicetree@vger.kernel.org" , "Strashko, Grygorii" , Russell King , Pawel Moll , Stephen Warren , Greg Kroah-Hartman , Ian Campbell , Kumar Gala , Rob Herring , "linux-kernel@vger.kernel.org" , "linux-mtd@lists.infradead.org" , Rob Landley , "linux-arm-kernel@lists.infradead.org" List-Id: devicetree@vger.kernel.org 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 >> --- >> 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 >> + * Copyright (C) Ivan Khoronzhuk >> + * >> + * 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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 > -- Regards, Ivan Khoronzhuk ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752083Ab3KRTRP (ORCPT ); Mon, 18 Nov 2013 14:17:15 -0500 Received: from devils.ext.ti.com ([198.47.26.153]:42289 "EHLO devils.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751631Ab3KRTRM (ORCPT ); Mon, 18 Nov 2013 14:17:12 -0500 Message-ID: <528A6757.3020502@ti.com> Date: Mon, 18 Nov 2013 21:15:35 +0200 From: "ivan.khoronzhuk" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0 MIME-Version: 1.0 To: Santosh Shilimkar CC: Rob Landley , Russell King , "devicetree@vger.kernel.org" , Pawel Moll , Mark Rutland , Rob Herring , Stephen Warren , Kumar Gala , Ian Campbell , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-mtd@lists.infradead.org" , "Strashko, Grygorii" , Greg Kroah-Hartman Subject: Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver References: <1384187188-5776-1-git-send-email-ivan.khoronzhuk@ti.com>,<1384187188-5776-8-git-send-email-ivan.khoronzhuk@ti.com> <4F5844B3A985794BA902E12C070812375F8D2E@DNCE04.ent.ti.com> <52825283.7000905@ti.com> In-Reply-To: <52825283.7000905@ti.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.167.145.122] X-EXCLAIMER-MD-CONFIG: f9c360f5-3d1e-4c3c-8703-f45bf52eff6b Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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 >> + * Copyright (C) Ivan Khoronzhuk >> + * >> + * 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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 > -- Regards, Ivan Khoronzhuk