| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <53048822.2080307@ti.com>
Date: Wed, 19 Feb 2014 12:32:02 +0200
From: Ivan Khoronzhuk <ivan.khoronzhuk@...com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: <santosh.shilimkar@...com>, <rob@...dley.net>,
<linux@....linux.org.uk>, <galak@...nel.crashing.org>,
<devicetree@...r.kernel.org>, <pawel.moll@....com>,
<mark.rutland@....com>, <rob.herring@...xeda.com>,
<swarren@...dotorg.org>, <ijc+devicetree@...lion.org.uk>,
<linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-mtd@...ts.infradead.org>, <grygorii.strashko@...com>,
<dwmw2@...radead.org>, <nsekhar@...com>,
"[initial author] Murali Karicheri" <m-karicheri2@...com>
Subject: Re: [PATCH v4 1/2] memory: ti-aemif: introduce AEMIF driver
On 02/18/2014 10:30 PM, Greg KH wrote:
> On Wed, Feb 05, 2014 at 09:46:13PM +0200, Ivan Khoronzhuk wrote:
>> Add new AEMIF driver for EMIF16 Texas Instruments 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 4 chip selects with 64M byte access
>> per chip select.
>>
>> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR
>> are not supported.
>>
>> This controller is used on SoCs like Davinci, Keysone2
>>
>> Acked-by: Santosh Shilimkar <santosh.shilimkar@...com
>> Signed-off-by: [initial author] Murali Karicheri <m-karicheri2@...com>
> What's this [] stuff?
>
> If Murali wrote this, that name needs to be in a "From:" line in the
> patch to properly attribute it, and drop the [] here.
Ok, I'll delete [] stuff.
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@...com>
>> ---
>> drivers/memory/Kconfig | 11 ++
>> drivers/memory/Makefile | 1 +
>> drivers/memory/ti-aemif.c | 429 ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 441 insertions(+)
>> create mode 100644 drivers/memory/ti-aemif.c
>>
>> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
>> index 29a11db..7bc3982 100644
>> --- a/drivers/memory/Kconfig
>> +++ b/drivers/memory/Kconfig
>> @@ -7,6 +7,17 @@ menuconfig MEMORY
>>
>> if MEMORY
>>
>> +config TI_AEMIF
>> + tristate "Texas Instruments 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..d4e150c 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_AEMIF) += ti-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/ti-aemif.c b/drivers/memory/ti-aemif.c
>> new file mode 100644
>> index 0000000..8d15d87
>> --- /dev/null
>> +++ b/drivers/memory/ti-aemif.c
>> @@ -0,0 +1,429 @@
>> +/*
>> + * TI AEMIF driver
>> + *
>> + * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/
>> + *
>> + * Authors:
>> + * Murali Karicheri <m-karicheri2@...com>
>> + * Ivan Khoronzhuk <ivan.khoronzhuk@...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 "ti-aemif"
> What's wrong with KBUILD_MODNAME?
Yes, I'll drop DRV_NAME in favour of KBUILD_MODNAME
>> +static int aemif_probe(struct platform_device *pdev)
>> +{
>> + int ret = -ENODEV, i;
> Extra spaces?
>
> Split this into two lines?
Ok.
>> + struct resource *res;
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + struct device_node *child_np;
>> + struct aemif_device *aemif;
>> +
>> + if (np == NULL)
>> + return 0;
>> +
>> + aemif = devm_kzalloc(dev, sizeof(*aemif), GFP_KERNEL);
>> + if (!aemif) {
>> + dev_err(dev, "cannot allocate memory for aemif\n");
> Not needed, you'll get a message from kzalloc about this.\
Ok.
>
>> + return -ENOMEM;
>> + }
>> +
>> + platform_set_drvdata(pdev, aemif);
>> +
>> + aemif->clk = devm_clk_get(dev, NULL);
>> + if (IS_ERR(aemif->clk)) {
>> + dev_err(dev, "cannot get clock 'aemif'\n");
>> + return PTR_ERR(aemif->clk);
> No freeing memory?
There is no need to free memory explicitly.
devm_kzalloc is used instead of kzalloc.
>> + }
>> +
>> + clk_prepare_enable(aemif->clk);
>> + aemif->clk_rate = clk_get_rate(aemif->clk) / MSEC_PER_SEC;
>> +
>> + if (of_device_is_compatible(np, "ti,da850-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_available_child_of_node(np, child_np) {
>> + ret = of_aemif_parse_abus_config(pdev, child_np);
>> + if (ret < 0)
>> + goto error;
>> + }
>> +
>> + for (i = 0; i < aemif->num_cs; i++) {
>> + ret = aemif_config_abus(pdev, 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_available_child_of_node(np, child_np) {
>> + ret = of_platform_populate(child_np, NULL, NULL, dev);
>> + if (ret < 0)
>> + goto error;
>> + }
>> +
>> + return 0;
>> +error:
>> + clk_disable_unprepare(aemif->clk);
>> + return ret;
> No freeing memory?
devm_* allocations were used, so no need.
>
>> +}
>> +
>> +static int aemif_remove(struct platform_device *pdev)
>> +{
>> + struct aemif_device *aemif = platform_get_drvdata(pdev);
>> + clk_disable_unprepare(aemif->clk);
> Extra line needed.
Ok
Thanks!
--
Regards,
Ivan Khoronzhuk
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists