Messages in this thread | | | Date | Fri, 03 Jan 2014 11:19:18 +0100 | From | boris brezillon <> | 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@overkiz.com>: >> On 31/12/2013 17:32, jjhiblot@traphandler.com wrote: >>> From: jean-jacques hiblot <jean-jacques.hiblot@jdsu.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@traphandler.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"); >>>
| |