| 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
| ||
|
Date: Fri, 03 Jan 2014 11:19:18 +0100
From: boris brezillon <b.brezillon@...rkiz.com>
To: Jean-Jacques Hiblot <jjhiblot@...phandler.com>
CC: nicolas.ferre@...el.com,
jean-jacques hiblot <jean-jacques.hiblot@...u.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 3/6] At91: dt: Added smc bus driver
On 03/01/2014 10:42, Jean-Jacques Hiblot wrote:
> 2014/1/3 boris brezillon <b.brezillon@...rkiz.com>:
>> On 31/12/2013 17:32, jjhiblot@...phandler.com wrote:
>>> From: jean-jacques hiblot <jean-jacques.hiblot@...u.com>
>>>
>>> The EBI/SMC external interface is used to access external peripherals
>>> (NAND
>>> and Ethernet controller in the case of sam9261ek). Different
>>> configurations and
>>> timings are required for those peripherals. This bus driver can be used
>>> to
>>> setup the bus timings/configuration from the device tree.
>>>
>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@...phandler.com>
>>> ---
>>> drivers/bus/Kconfig | 9 +++
>>> drivers/bus/Makefile | 1 +
>>> drivers/bus/atmel-smc.c | 182
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 192 insertions(+)
>>> create mode 100644 drivers/bus/atmel-smc.c
>>>
>>> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
>>> index 552373c..8c944db5 100644
>>> --- a/drivers/bus/Kconfig
>>> +++ b/drivers/bus/Kconfig
>>> @@ -12,6 +12,15 @@ config IMX_WEIM
>>> The WEIM(Wireless External Interface Module) works like a bus.
>>> You can attach many different devices on it, such as NOR,
>>> onenand.
>>>
>>> +config ATMEL_SMC
>>> + bool "Atmel SMC/EBI driver"
>>> + depends on SOC_AT91SAM9 && OF
>>> + help
>>> + Driver for Atmel SMC/EBI controller.
>>> + Used to configure the EBI (external bus interface) when the
>>> device-
>>> + tree is used. This bus supports NANDs, external ethernet
>>> controller,
>>> + SRAMs, ATA devices.
>>> +
>>> config MVEBU_MBUS
>>> bool
>>> depends on PLAT_ORION
>>> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
>>> index 8947bdd..4364003 100644
>>> --- a/drivers/bus/Makefile
>>> +++ b/drivers/bus/Makefile
>>> @@ -2,6 +2,7 @@
>>> # Makefile for the bus drivers.
>>> #
>>>
>>> +obj-$(CONFIG_ATMEL_SMC) += atmel-smc.o
>>> obj-$(CONFIG_IMX_WEIM) += imx-weim.o
>>> obj-$(CONFIG_MVEBU_MBUS) += mvebu-mbus.o
>>> obj-$(CONFIG_OMAP_OCP2SCP) += omap-ocp2scp.o
>>> diff --git a/drivers/bus/atmel-smc.c b/drivers/bus/atmel-smc.c
>>> new file mode 100644
>>> index 0000000..06e530d
>>> --- /dev/null
>>> +++ b/drivers/bus/atmel-smc.c
>>> @@ -0,0 +1,182 @@
>>> +/*
>>> + * EBI driver for Atmel SAM9 chips
>>> + * inspired by the fsl weim bus driver
>>> + *
>>> + * Copyright (C) 2013 JJ Hiblot.
>>> + *
>>> + * This file is licensed under the terms of the GNU General Public
>>> + * License version 2. This program is licensed "as is" without any
>>> + * warranty of any kind, whether express or implied.
>>> + */
>>> +#include <linux/module.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/io.h>
>>> +#include <linux/of_device.h>
>>> +#include <mach/at91sam9_smc.h>
>>> +
>>> +struct at91_smc_devtype {
>>> + unsigned int cs_count;
>>> +};
>>> +
>>> +static const struct at91_smc_devtype sam9261_smc_devtype = {
>>> + .cs_count = 6,
>>> +};
>>> +
>>> +static const struct of_device_id smc_id_table[] = {
>>> + { .compatible = "atmel,at91sam9261-smc", .data =
>>> &sam9261_smc_devtype},
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, smc_id_table);
>>> +
>>> +struct smc_parameters_type {
>>> + const char *name;
>>> + u16 reg;
>>> + u16 width;
>>> + u16 shift;
>>> +};
>>> +
>>> +#define SETUP 0
>>> +#define PULSE 1
>>> +#define CYCLE 2
>>> +#define MODE 3
>>> +static const struct smc_parameters_type smc_parameters[] = {
>>> + {"smc,ncs_read_setup", SETUP, 6, 24},
>>> + {"smc,nrd_setup", SETUP, 6, 16},
>>> + {"smc,ncs_write_setup", SETUP, 6, 8},
>>> + {"smc,nwe_setup", SETUP, 6, 0},
>>> + {"smc,ncs_read_pulse", PULSE, 6, 24},
>>> + {"smc,nrd_pulse", PULSE, 6, 16},
>>> + {"smc,ncs_write_pulse", PULSE, 6, 8},
>>> + {"smc,nwe_pulse", PULSE, 6, 0},
>>> + {"smc,read_cycle", CYCLE, 9, 16},
>>> + {"smc,write_cycle", CYCLE, 9, 0},
>>> + {"smc,burst_size", MODE, 2, 28},
>>> + {"smc,burst_enabled", MODE, 1, 24},
>>> + {"smc,tdf_mode", MODE, 1, 20},
>>> + {"smc,tdf_cycles", MODE, 4, 16},
>>> + {"smc,bus_width", MODE, 2, 12},
>>> + {"smc,byte_access_type", MODE, 1, 8},
>>> + {"smc,nwait_mode", MODE, 2, 4},
>>> + {"smc,write_mode", MODE, 1, 0},
>>> + {"smc,read_mode", MODE, 1, 1},
>>> + {NULL}
>>> +};
>>> +
>>> +/* Parse and set the timing for this device. */
>>> +static int __init smc_timing_setup(struct device *dev, struct device_node
>>> *np,
>>> + void __iomem *base, const struct at91_smc_devtype
>>> *devtype)
>>> +{
>>> + u32 val;
>>> + int ret;
>>> + u32 cs;
>>> + const struct smc_parameters_type *p = smc_parameters;
>>> + u32 shadow_smc_regs[5];
>>> +
>>> + ret = of_property_read_u32(np, "smc,cs" , &cs);
>>> + if (ret < 0) {
>>> + dev_err(dev, "missing mandatory property : smc,cs\n");
>>> + return ret;
>>> + }
>>> + if (val >= devtype->cs_count) {
>>> + dev_err(dev, "invalid value for property smc,cs (=%d)."
>>> + "Must be in range 0 to %d\n", cs, devtype->cs_count-1);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* set the timing for EBI */
>>> + base += (0x10 * cs);
>>> + shadow_smc_regs[SETUP] = readl_relaxed(base + AT91_SMC_SETUP);
>>> + shadow_smc_regs[PULSE] = readl_relaxed(base + AT91_SMC_PULSE);
>>> + shadow_smc_regs[CYCLE] = readl_relaxed(base + AT91_SMC_CYCLE);
>>> + shadow_smc_regs[MODE] = readl_relaxed(base + AT91_SMC_MODE);
>>> +
>>> + while (p->name) {
>>> + ret = of_property_read_u32(np, p->name , &val);
>>> + if (ret == -EINVAL) {
>>> + dev_dbg(dev, "cs %d: property %s not set.\n", cs,
>>> + p->name);
>>> + p++;
>>> + continue;
>>> + } else if (ret) {
>>> + dev_err(dev, "cs %d: can't get property %s.\n",
>>> cs,
>>> + p->name);
>>> + return ret;
>>> + }
>>> + if (val >= (1<<p->width)) {
>>> + dev_err(dev, "cs %d: property %s out of range.\n",
>>> cs,
>>> + p->name);
>>> + return -ERANGE;
>>> + }
>>> + shadow_smc_regs[p->reg] &= ~(((1<<p->width)-1) <<
>>> p->shift);
>>> + shadow_smc_regs[p->reg] |= (val << p->shift);
>>> + p++;
>>> + }
>>> + writel_relaxed(shadow_smc_regs[SETUP], base + AT91_SMC_SETUP);
>>> + writel_relaxed(shadow_smc_regs[PULSE], base + AT91_SMC_PULSE);
>>> + writel_relaxed(shadow_smc_regs[CYCLE], base + AT91_SMC_CYCLE);
>>> + writel_relaxed(shadow_smc_regs[MODE], base + AT91_SMC_MODE);
>>> + return 0;
>>> +}
>>
>> I think we should consider defining timings in nanoseconds instead of
>> clk (actually master clk) cycles, because if we ever support master clk
>> rate change (I hope so :)), then we won't be able to recalculate the
>> appropriate timings (in cycles).
>> Moreover this ease dt writer's work.
>>
>> tcycle calulation :
>> mck_rate = clk_get_rate(mck);
>> tcycle = (tns * 10^9) / mck_rate;
>>
>> dt binding:
>>
>> smc@xxx {
>> clocks = <&mck>;
>> dev@zz {
>> smc,ncs_read_setup = <y>; /* in nsecs */
>> ...
>> };
>> }
>>
> This is indeed something that could be done easily.
> However I did this a few years ago for the EBI (and for another OS,
> not linux) and it turned out to be not so magical. There were a lot of
> side effects because most of the times the timings are defined (or at
> least amended) empirically. There are also some cases when it's
> easier to have the value in clock cycle (FPGA with synchronous IF)
> I would be interested in the opinion of Nicolas in this matter.
>
>> And what about defining a mechanism capable of handling several
>> converter types (not just the generic one you described with the
>> ncs, nrd, ... timings) ?
>> For example NAND chip vendors use a common timing naming convention (tCLS,
>> tCS, ...), and this is sometime annoying (and error-prone) to
>> convert these timings into the SMC model.
>>
>> It would be great if we could define specific converters for these
>> standardized protocols (NAND, NOR, ???).
>>
>> dt binding example:
>>
>> smc@xxx {
>> clocks = <&mck>;
>> ...
>> dev@cs,offset {
>> compatible = "atmel,at91-smc-generic-converter";
>> smc,ncs_read_setup = <y>; /* in nsecs */
>> ...
>> };
>>
>> nand@cs,offset {
>> compatible = "atmel,at91-smc-nand-converter";
>> nand-chip {
>> /* see atmel-nand dt binding */
>> timings {
>> tCLS = <..>;
>> ...
>> };
>> /*
>> * these timings are contained by the nand-chip
>> * node because they describe the NAND timings
>> * (as defined in many nand datasheets).
>> */
>> };
>> };
>> }
>>
>>
>> These are just some thoughts, feel free to argue ;).
> The idea is appealing. It could be done for NAND but I wonder if it
> makes sense for the rest of the devices.
NOR flashes seems to have a standard naming convention too...
Anyway, I think you should first implement the mechanism to support several
converters and the generic converter.
This way we can have a working version of the SMC driver and we'll be
able to
add new converters later.
>> Best Regards,
>>
>> Boris
>>
>>
>>> +
>>> +static int __init smc_parse_dt(struct platform_device *pdev,
>>> + void __iomem *base)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + const struct of_device_id *of_id = of_match_device(smc_id_table,
>>> dev);
>>> + const struct at91_smc_devtype *devtype = of_id->data;
>>> + struct device_node *child;
>>> + int ret;
>>> +
>>> + for_each_child_of_node(dev->of_node, child) {
>>> + if (!child->name)
>>> + continue;
>>> +
>>> + ret = smc_timing_setup(dev, child, base, devtype);
>>> + if (ret) {
>>> + dev_err(dev, "%s set timing failed.\n",
>>> + child->full_name);
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
>>> + if (ret)
>>> + dev_err(dev, "%s fail to create devices.\n",
>>> + dev->of_node->full_name);
>>> + return ret;
>>> +}
>>> +
>>> +static int __init smc_probe(struct platform_device *pdev)
>>> +{
>>> + struct resource *res;
>>> + void __iomem *base;
>>> + int ret;
>>> +
>>> + /* get the resource */
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + base = devm_request_and_ioremap(&pdev->dev, res);
>>> + if (IS_ERR(base)) {
>>> + dev_err(&pdev->dev, "can't map SMC base address\n");
>>> + return PTR_ERR(base);
>>> + }
>>> +
>>> + /* parse the device node */
>>> + ret = smc_parse_dt(pdev, base);
>>> + if (!ret)
>>> + dev_info(&pdev->dev, "Driver registered.\n");
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static struct platform_driver smc_driver = {
>>> + .driver = {
>>> + .name = "atmel-smc",
>>> + .owner = THIS_MODULE,
>>> + .of_match_table = smc_id_table,
>>> + },
>>> +};
>>> +module_platform_driver_probe(smc_driver, smc_probe);
>>> +
>>> +MODULE_AUTHOR("JJ Hiblot");
>>> +MODULE_DESCRIPTION("Atmel's SMC/EBI driver");
>>> +MODULE_LICENSE("GPL");
>>>
--
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