| Message ID | 1389270709-32662-8-git-send-email-jjhiblot@traphandler.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello JJ, On 09/01/2014 13:31, Jean-Jacques Hiblot wrote: > 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. > It currently accepts timings in clock period and in nanoseconds. > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> > --- > drivers/memory/Kconfig | 10 ++ > drivers/memory/Makefile | 1 + > drivers/memory/atmel-smc.c | 431 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 442 insertions(+) > create mode 100644 drivers/memory/atmel-smc.c > > diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig > index 29a11db..fbdfd63 100644 > --- a/drivers/memory/Kconfig > +++ b/drivers/memory/Kconfig > @@ -50,4 +50,14 @@ config TEGRA30_MC > analysis, especially for IOMMU/SMMU(System Memory Management > Unit) module. > > +config ATMEL_SMC > + bool "Atmel SMC/EBI driver" > + default y > + 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, etc. > + > endif > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile > index 969d923..101abc4 100644 > --- a/drivers/memory/Makefile > +++ b/drivers/memory/Makefile > @@ -9,3 +9,4 @@ obj-$(CONFIG_TI_EMIF) += emif.o > obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o > obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o > obj-$(CONFIG_TEGRA30_MC) += tegra30-mc.o > +obj-$(CONFIG_ATMEL_SMC) += atmel-smc.o > diff --git a/drivers/memory/atmel-smc.c b/drivers/memory/atmel-smc.c > new file mode 100644 > index 0000000..0a1d9ba > --- /dev/null > +++ b/drivers/memory/atmel-smc.c > @@ -0,0 +1,431 @@ > +/* > + * 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> You should avoid machine specific headers inclusions: we're trying to get rid of them. Duplicate the code and macros you need in your driver instead. > + > +struct smc_data { > + struct clk *bus_clk; > + void __iomem *base; > + struct device *dev; > +}; > + > +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 width; > + u16 shift; > +}; > + > +static const struct smc_parameters_type smc_parameters[] = { > + {"smc,burst_size", 2, 28}, > + {"smc,burst_enabled", 1, 24}, > + {"smc,tdf_mode", 1, 20}, > + {"smc,bus_width", 2, 12}, > + {"smc,byte_access_type", 1, 8}, > + {"smc,nwait_mode", 2, 4}, > + {"smc,write_mode", 1, 0}, > + {"smc,read_mode", 1, 1}, > + {NULL} > +}; > + > +static int get_mode_register_from_dt(struct smc_data *smc, > + struct device_node *np, > + struct sam9_smc_config *cfg) > +{ > + int ret; > + u32 val; > + struct device *dev = smc->dev; > + const struct smc_parameters_type *p = smc_parameters; > + u32 mode = cfg->mode; > + > + while (p->name) { > + ret = of_property_read_u32(np, p->name , &val); > + if (ret == -EINVAL) { > + dev_dbg(dev, "%s: property %s not set.\n", np->name, > + p->name); > + p++; > + continue; > + } else if (ret) { > + dev_err(dev, "%s: can't get property %s.\n", np->name, > + p->name); > + return ret; > + } > + if (val >= (1<<p->width)) { > + dev_err(dev, "%s: property %s out of range.\n", > + np->name, p->name); > + return -ERANGE; > + } > + mode &= ~(((1<<p->width)-1) << p->shift); > + mode |= (val << p->shift); > + p++; > + } > + cfg->mode = mode; > + return 0; > +} > + > +static int generic_timing_from_dt(struct smc_data *smc, struct device_node *np, > + struct sam9_smc_config *cfg) > +{ > + u32 val; > + > + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &val)) > + cfg->ncs_read_setup = val; > + > + if (!of_property_read_u32(np, "smc,nrd_setup" , &val)) > + cfg->nrd_setup = val; > + > + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &val)) > + cfg->ncs_write_setup = val; > + > + if (!of_property_read_u32(np, "smc,nwe_setup" , &val)) > + cfg->nwe_setup = val; > + > + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &val)) > + cfg->ncs_read_pulse = val; > + > + if (!of_property_read_u32(np, "smc,nrd_pulse" , &val)) > + cfg->nrd_pulse = val; > + > + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &val)) > + cfg->ncs_write_pulse = val; > + > + if (!of_property_read_u32(np, "smc,nwe_pulse" , &val)) > + cfg->nwe_pulse = val; > + > + if (!of_property_read_u32(np, "smc,read_cycle" , &val)) > + cfg->read_cycle = val; > + > + if (!of_property_read_u32(np, "smc,write_cycle" , &val)) > + cfg->write_cycle = val; > + > + if (!of_property_read_u32(np, "smc,tdf_cycles" , &val)) > + cfg->tdf_cycles = val; > + > + return get_mode_register_from_dt(smc, np, cfg); > +} > + > +/* convert the time in ns in a number of clock cycles */ > +static u32 ns_to_cycles(u32 ns, u32 clk) > +{ > + /* > + * convert the clk to kHz for the rest of the calculation to avoid > + * overflow > + */ > + u32 clk_kHz = clk / 1000; > + u32 ncycles = ((ns * clk_kHz) + 1000000 - 1) / 1000000; What about using an u64 type and do_div ? > + return ncycles; > +} > + > +static u32 cycles_to_coded_cycle(u32 cycles, int a, int b) > +{ > + u32 mask_high = (1 << a) - 1; > + u32 mask_low = (1 << b) - 1; > + u32 coded; > + > + /* check if the value can be described with the coded format */ > + if (cycles & (mask_high & ~mask_low)) { > + /* not representable. we need to round up */ > + cycles |= mask_high; > + cycles += 1; > + } > + /* Now the value can be represented in the coded format */ > + coded = (cycles & ~mask_high) >> (a - b); > + coded |= (cycles & mask_low); > + return coded; > +} > + > +static u32 ns_to_rw_cycles(u32 ns, u32 clk) > +{ > + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 7); > +} > + > +static u32 ns_to_pulse_cycles(u32 ns, u32 clk) > +{ > + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 6); > +} > + > +static u32 ns_to_setup_cycles(u32 ns, u32 clk) > +{ > + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 7, 5); > +} > + > +static u32 cycles_to_ns(u32 cycles, u32 clk) > +{ > + /* > + * convert the clk to kHz for the rest of the calculation to avoid > + * overflow > + */ > + u32 clk_kHz = clk / 1000; Ditto (u64 + do_div). > + return (cycles * 1000000) / clk_kHz; > +} > + > +static u32 coded_cycle_to_cycles(u32 coded, int a, int b) > +{ > + u32 cycles = (coded >> b) << a; > + u32 mask_low = (1 << b) - 1; > + cycles |= (coded & mask_low); > + return cycles; > +} > + > +static u32 rw_cycles_to_ns(u32 reg, u32 clk) > +{ > + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 7), clk); > +} > + > +static u32 pulse_cycles_to_ns(u32 reg, u32 clk) > +{ > + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 6), clk); > +} > + > +static u32 setup_cycles_to_ns(u32 reg, u32 clk) > +{ > + return cycles_to_ns(coded_cycle_to_cycles(reg, 7, 5), clk); > +} > + > +static void dump_timing(struct smc_data *smc, struct sam9_smc_config *config) > +{ > + u32 freq = clk_get_rate(smc->bus_clk); > + struct device *dev = smc->dev; > + > +#define DUMP(fn, y) dev_info(dev, "%s : 0x%x (%d ns)\n", #y, config->y,\ > + fn(config->y, freq)) > +#define DUMP_PULSE(y) DUMP(pulse_cycles_to_ns, y) > +#define DUMP_RWCYCLE(y) DUMP(rw_cycles_to_ns, y) > +#define DUMP_SETUP(y) DUMP(setup_cycles_to_ns, y) > +#define DUMP_SIMPLE(y) DUMP(cycles_to_ns, y) > + > + DUMP_SETUP(nwe_setup); > + DUMP_SETUP(ncs_write_setup); > + DUMP_SETUP(nrd_setup); > + DUMP_SETUP(ncs_read_setup); > + DUMP_PULSE(nwe_pulse); > + DUMP_PULSE(ncs_write_pulse); > + DUMP_PULSE(nrd_pulse); > + DUMP_PULSE(ncs_read_pulse); > + DUMP_RWCYCLE(write_cycle); > + DUMP_RWCYCLE(read_cycle); > + DUMP_SIMPLE(tdf_cycles); > +} > + > +static int ns_timing_from_dt(struct smc_data *smc, struct device_node *np, > + struct sam9_smc_config *cfg) > +{ > + u32 t_ns; > + u32 freq = clk_get_rate(smc->bus_clk); > + > + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &t_ns)) > + cfg->ncs_read_setup = ns_to_setup_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,nrd_setup" , &t_ns)) > + cfg->nrd_setup = ns_to_setup_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &t_ns)) > + cfg->ncs_write_setup = ns_to_setup_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,nwe_setup" , &t_ns)) > + cfg->nwe_setup = ns_to_setup_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &t_ns)) > + cfg->ncs_read_pulse = ns_to_pulse_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,nrd_pulse" , &t_ns)) > + cfg->nrd_pulse = ns_to_pulse_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &t_ns)) > + cfg->ncs_write_pulse = ns_to_pulse_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,nwe_pulse" , &t_ns)) > + cfg->nwe_pulse = ns_to_pulse_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,read_cycle" , &t_ns)) > + cfg->read_cycle = ns_to_rw_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,write_cycle" , &t_ns)) > + cfg->write_cycle = ns_to_rw_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,tdf_cycles" , &t_ns)) > + cfg->tdf_cycles = ns_to_cycles(t_ns, freq); > + > + return get_mode_register_from_dt(smc, np, cfg); > +} > + > +struct converter { > + const char *name; > + int (*fn) (struct smc_data *smc, struct device_node *np, > + struct sam9_smc_config *cfg); > +}; > +static const struct converter converters[] = { > + {"raw", generic_timing_from_dt}, > + {"nanosec", ns_timing_from_dt}, > +}; Could you use more specific names like: "atmel,smc-converter-generic" "atmel,smc-converter-nand" ... IMHO the timing unit should be specified in the property names: smc,ncs_read_setup-ns smc,ncs_read_setup-cycles > + > +/* Parse and set the timing for this device. */ > +static int smc_timing_setup(struct smc_data *smc, struct device_node *np, > + const struct at91_smc_devtype *devtype) > +{ > + int ret; > + u32 cs; > + int i; > + struct device *dev = smc->dev; > + const struct converter *converter; > + const char *converter_name = NULL; > + struct sam9_smc_config cfg; > + > + ret = of_property_read_u32(np, "smc,cs" , &cs); Shouldn't this be stored in the reg property ? After all, in your DM9000 patch you use "@cs,offset" to identify the node... > + if (ret < 0) { > + dev_err(dev, "missing mandatory property : smc,cs\n"); > + return ret; > + } > + if (cs >= 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; > + } > + > + of_property_read_string(np, "smc,converter", &converter_name); What about using the "compatible" property + struct of_device_id instead of "smc,converter" property + struct converter ? > + if (converter_name) { > + for (i = 0; i < ARRAY_SIZE(converters); i++) > + if (strcmp(converters[i].name, converter_name) == 0) > + converter = &converters[i]; > + if (!converter) { > + dev_info(dev, "unknown converter. aborting\n"); > + return -EINVAL; > + } > + } else { > + dev_dbg(dev, "cs %d: no smc converter provided. using " > + "raw register values\n", cs); > + converter = &converters[0]; > + } > + dev_dbg(dev, "cs %d using converter : %s\n", cs, converter->name); > + sam9_smc_cs_read(smc->base + (0x10 * cs), &cfg); > + converter->fn(smc, np, &cfg); > + ret = sam9_smc_check_cs_configuration(&cfg); > + if (ret < 0) { > + dev_info(dev, "invalid smc configuration for cs %d." > + "clipping values\n", cs); > + sam9_smc_clip_cs_configuration(&cfg); > + dump_timing(smc, &cfg); > + } > +#ifdef DEBUG > + else > + dump_timing(smc, &cfg); > +#endif I'm not a big fan of #ifdef blocks inside the code. You could define a dummy dump_timing function if DEBUG is not defined: #ifdef DEBUG static void dump_timing(struct smc_data *smc, struct sam9_smc_config *config) { /* your implementation */ } #else static inline void dump_timing(struct smc_data *smc, struct sam9_smc_config *config) { } #endif Or just use dev_dbg when printing things in dump_timing. > + > + sam9_smc_cs_configure(smc->base + (0x10 * cs), &cfg); > + return 0; > +} > + > +static int smc_parse_dt(struct smc_data *smc) > +{ > + struct device *dev = smc->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; > + if (!of_device_is_available(child)) > + continue; > + ret = smc_timing_setup(smc, child, devtype); > + if (ret) { > + static struct property status = { > + .name = "status", > + .value = "disabled", > + .length = sizeof("disabled"), > + }; > + dev_err(dev, "%s set timing failed. This node will be disabled.\n", > + child->full_name); > + ret = of_update_property(child, &status); > + if (ret < 0) { > + dev_err(dev, "can't disable %s. aborting probe\n", > + child->full_name); > + break; The concept of disabling the device if timings cannot be met sounds interresting... Let's see what other maintainers say about this :). > + } > + } > + } > + > + ret = of_platform_populate(dev->of_node, of_default_bus_match_table, > + NULL, dev); > + if (ret) > + dev_err(dev, "%s fail to create devices.\n", > + dev->of_node->full_name); > + > + return ret; > +} > + > +static int smc_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + int ret; > + void __iomem *base; > + struct clk *clk; > + struct smc_data *smc = devm_kzalloc(&pdev->dev, sizeof(struct smc_data), > + GFP_KERNEL); > + > + if (!smc) > + return -ENOMEM; > + > + smc->dev = &pdev->dev; > + > + /* 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); > + } > + > + /* get the clock */ > + clk = devm_clk_get(&pdev->dev, "smc"); > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > + > + smc->bus_clk = clk; > + smc->base = base; > + > + /* parse the device node */ > + ret = smc_parse_dt(smc); > + 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"); That's all for now. :) I'll try to test it this week end on a sama5 board. Best Regards, Boris
Hi Boris, 2014/1/9 boris brezillon <b.brezillon@overkiz.com>: > Hello JJ, > > > On 09/01/2014 13:31, Jean-Jacques Hiblot wrote: >> >> 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. >> It currently accepts timings in clock period and in nanoseconds. >> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> >> --- >> drivers/memory/Kconfig | 10 ++ >> drivers/memory/Makefile | 1 + >> drivers/memory/atmel-smc.c | 431 >> +++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 442 insertions(+) >> create mode 100644 drivers/memory/atmel-smc.c >> >> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig >> index 29a11db..fbdfd63 100644 >> --- a/drivers/memory/Kconfig >> +++ b/drivers/memory/Kconfig >> @@ -50,4 +50,14 @@ config TEGRA30_MC >> analysis, especially for IOMMU/SMMU(System Memory Management >> Unit) module. >> +config ATMEL_SMC >> + bool "Atmel SMC/EBI driver" >> + default y >> + 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, etc. >> + >> endif >> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile >> index 969d923..101abc4 100644 >> --- a/drivers/memory/Makefile >> +++ b/drivers/memory/Makefile >> @@ -9,3 +9,4 @@ obj-$(CONFIG_TI_EMIF) += emif.o >> obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o >> obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o >> obj-$(CONFIG_TEGRA30_MC) += tegra30-mc.o >> +obj-$(CONFIG_ATMEL_SMC) += atmel-smc.o >> diff --git a/drivers/memory/atmel-smc.c b/drivers/memory/atmel-smc.c >> new file mode 100644 >> index 0000000..0a1d9ba >> --- /dev/null >> +++ b/drivers/memory/atmel-smc.c >> @@ -0,0 +1,431 @@ >> +/* >> + * 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> > > > You should avoid machine specific headers inclusions: we're trying to get > rid of them. > > Duplicate the code and macros you need in your driver instead. Is this the right way? We usually try to avoid duplication. > > >> + >> +struct smc_data { >> + struct clk *bus_clk; >> + void __iomem *base; >> + struct device *dev; >> +}; >> + >> +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 width; >> + u16 shift; >> +}; >> + >> +static const struct smc_parameters_type smc_parameters[] = { >> + {"smc,burst_size", 2, 28}, >> + {"smc,burst_enabled", 1, 24}, >> + {"smc,tdf_mode", 1, 20}, >> + {"smc,bus_width", 2, 12}, >> + {"smc,byte_access_type", 1, 8}, >> + {"smc,nwait_mode", 2, 4}, >> + {"smc,write_mode", 1, 0}, >> + {"smc,read_mode", 1, 1}, >> + {NULL} >> +}; >> + >> +static int get_mode_register_from_dt(struct smc_data *smc, >> + struct device_node *np, >> + struct sam9_smc_config *cfg) >> +{ >> + int ret; >> + u32 val; >> + struct device *dev = smc->dev; >> + const struct smc_parameters_type *p = smc_parameters; >> + u32 mode = cfg->mode; >> + >> + while (p->name) { >> + ret = of_property_read_u32(np, p->name , &val); >> + if (ret == -EINVAL) { >> + dev_dbg(dev, "%s: property %s not set.\n", >> np->name, >> + p->name); >> + p++; >> + continue; >> + } else if (ret) { >> + dev_err(dev, "%s: can't get property %s.\n", >> np->name, >> + p->name); >> + return ret; >> + } >> + if (val >= (1<<p->width)) { >> + dev_err(dev, "%s: property %s out of range.\n", >> + np->name, p->name); >> + return -ERANGE; >> + } >> + mode &= ~(((1<<p->width)-1) << p->shift); >> + mode |= (val << p->shift); >> + p++; >> + } >> + cfg->mode = mode; >> + return 0; >> +} >> + >> +static int generic_timing_from_dt(struct smc_data *smc, struct >> device_node *np, >> + struct sam9_smc_config *cfg) >> +{ >> + u32 val; >> + >> + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &val)) >> + cfg->ncs_read_setup = val; >> + >> + if (!of_property_read_u32(np, "smc,nrd_setup" , &val)) >> + cfg->nrd_setup = val; >> + >> + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &val)) >> + cfg->ncs_write_setup = val; >> + >> + if (!of_property_read_u32(np, "smc,nwe_setup" , &val)) >> + cfg->nwe_setup = val; >> + >> + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &val)) >> + cfg->ncs_read_pulse = val; >> + >> + if (!of_property_read_u32(np, "smc,nrd_pulse" , &val)) >> + cfg->nrd_pulse = val; >> + >> + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &val)) >> + cfg->ncs_write_pulse = val; >> + >> + if (!of_property_read_u32(np, "smc,nwe_pulse" , &val)) >> + cfg->nwe_pulse = val; >> + >> + if (!of_property_read_u32(np, "smc,read_cycle" , &val)) >> + cfg->read_cycle = val; >> + >> + if (!of_property_read_u32(np, "smc,write_cycle" , &val)) >> + cfg->write_cycle = val; >> + >> + if (!of_property_read_u32(np, "smc,tdf_cycles" , &val)) >> + cfg->tdf_cycles = val; >> + >> + return get_mode_register_from_dt(smc, np, cfg); >> +} >> + >> +/* convert the time in ns in a number of clock cycles */ >> +static u32 ns_to_cycles(u32 ns, u32 clk) >> +{ >> + /* >> + * convert the clk to kHz for the rest of the calculation to avoid >> + * overflow >> + */ >> + u32 clk_kHz = clk / 1000; >> + u32 ncycles = ((ns * clk_kHz) + 1000000 - 1) / 1000000; > > What about using an u64 type and do_div ? easier and faster (though it's not the point here) this way, and kHz ist not so imprecise :-) > >> + return ncycles; >> +} >> + >> +static u32 cycles_to_coded_cycle(u32 cycles, int a, int b) >> +{ >> + u32 mask_high = (1 << a) - 1; >> + u32 mask_low = (1 << b) - 1; >> + u32 coded; >> + >> + /* check if the value can be described with the coded format */ >> + if (cycles & (mask_high & ~mask_low)) { >> + /* not representable. we need to round up */ >> + cycles |= mask_high; >> + cycles += 1; >> + } >> + /* Now the value can be represented in the coded format */ >> + coded = (cycles & ~mask_high) >> (a - b); >> + coded |= (cycles & mask_low); >> + return coded; >> +} >> + >> +static u32 ns_to_rw_cycles(u32 ns, u32 clk) >> +{ >> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 7); >> +} >> + >> +static u32 ns_to_pulse_cycles(u32 ns, u32 clk) >> +{ >> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 6); >> +} >> + >> +static u32 ns_to_setup_cycles(u32 ns, u32 clk) >> +{ >> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 7, 5); >> +} >> + >> +static u32 cycles_to_ns(u32 cycles, u32 clk) >> +{ >> + /* >> + * convert the clk to kHz for the rest of the calculation to avoid >> + * overflow >> + */ >> + u32 clk_kHz = clk / 1000; > > > Ditto (u64 + do_div). > >> + return (cycles * 1000000) / clk_kHz; >> +} >> + >> +static u32 coded_cycle_to_cycles(u32 coded, int a, int b) >> +{ >> + u32 cycles = (coded >> b) << a; >> + u32 mask_low = (1 << b) - 1; >> + cycles |= (coded & mask_low); >> + return cycles; >> +} >> + >> +static u32 rw_cycles_to_ns(u32 reg, u32 clk) >> +{ >> + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 7), clk); >> +} >> + >> +static u32 pulse_cycles_to_ns(u32 reg, u32 clk) >> +{ >> + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 6), clk); >> +} >> + >> +static u32 setup_cycles_to_ns(u32 reg, u32 clk) >> +{ >> + return cycles_to_ns(coded_cycle_to_cycles(reg, 7, 5), clk); >> +} >> + >> +static void dump_timing(struct smc_data *smc, struct sam9_smc_config >> *config) >> +{ >> + u32 freq = clk_get_rate(smc->bus_clk); >> + struct device *dev = smc->dev; >> + >> +#define DUMP(fn, y) dev_info(dev, "%s : 0x%x (%d ns)\n", #y, >> config->y,\ >> + fn(config->y, freq)) >> +#define DUMP_PULSE(y) DUMP(pulse_cycles_to_ns, y) >> +#define DUMP_RWCYCLE(y) DUMP(rw_cycles_to_ns, y) >> +#define DUMP_SETUP(y) DUMP(setup_cycles_to_ns, y) >> +#define DUMP_SIMPLE(y) DUMP(cycles_to_ns, y) >> + >> + DUMP_SETUP(nwe_setup); >> + DUMP_SETUP(ncs_write_setup); >> + DUMP_SETUP(nrd_setup); >> + DUMP_SETUP(ncs_read_setup); >> + DUMP_PULSE(nwe_pulse); >> + DUMP_PULSE(ncs_write_pulse); >> + DUMP_PULSE(nrd_pulse); >> + DUMP_PULSE(ncs_read_pulse); >> + DUMP_RWCYCLE(write_cycle); >> + DUMP_RWCYCLE(read_cycle); >> + DUMP_SIMPLE(tdf_cycles); >> +} >> + >> +static int ns_timing_from_dt(struct smc_data *smc, struct device_node >> *np, >> + struct sam9_smc_config *cfg) >> +{ >> + u32 t_ns; >> + u32 freq = clk_get_rate(smc->bus_clk); >> + >> + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &t_ns)) >> + cfg->ncs_read_setup = ns_to_setup_cycles(t_ns, freq); >> + >> + if (!of_property_read_u32(np, "smc,nrd_setup" , &t_ns)) >> + cfg->nrd_setup = ns_to_setup_cycles(t_ns, freq); >> + >> + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &t_ns)) >> + cfg->ncs_write_setup = ns_to_setup_cycles(t_ns, freq); >> + >> + if (!of_property_read_u32(np, "smc,nwe_setup" , &t_ns)) >> + cfg->nwe_setup = ns_to_setup_cycles(t_ns, freq); >> + >> + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &t_ns)) >> + cfg->ncs_read_pulse = ns_to_pulse_cycles(t_ns, freq); >> + >> + if (!of_property_read_u32(np, "smc,nrd_pulse" , &t_ns)) >> + cfg->nrd_pulse = ns_to_pulse_cycles(t_ns, freq); >> + >> + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &t_ns)) >> + cfg->ncs_write_pulse = ns_to_pulse_cycles(t_ns, freq); >> + >> + if (!of_property_read_u32(np, "smc,nwe_pulse" , &t_ns)) >> + cfg->nwe_pulse = ns_to_pulse_cycles(t_ns, freq); >> + >> + if (!of_property_read_u32(np, "smc,read_cycle" , &t_ns)) >> + cfg->read_cycle = ns_to_rw_cycles(t_ns, freq); >> + >> + if (!of_property_read_u32(np, "smc,write_cycle" , &t_ns)) >> + cfg->write_cycle = ns_to_rw_cycles(t_ns, freq); >> + >> + if (!of_property_read_u32(np, "smc,tdf_cycles" , &t_ns)) >> + cfg->tdf_cycles = ns_to_cycles(t_ns, freq); >> + >> + return get_mode_register_from_dt(smc, np, cfg); >> +} >> + >> +struct converter { >> + const char *name; >> + int (*fn) (struct smc_data *smc, struct device_node *np, >> + struct sam9_smc_config *cfg); >> +}; >> +static const struct converter converters[] = { >> + {"raw", generic_timing_from_dt}, >> + {"nanosec", ns_timing_from_dt}, >> +}; > > > Could you use more specific names like: > "atmel,smc-converter-generic" > "atmel,smc-converter-nand" > ... Isn't it a bit redudant? smc,converter = "atmel,smc-converter-generic"; > > IMHO the timing unit should be specified in the property names: > smc,ncs_read_setup-ns > smc,ncs_read_setup-cycles > True. Although cycles is misleading. It's more a raw register value. For pulse, setup and rw cycle, the register value is not identical to the number of cycles. > > > >> + >> +/* Parse and set the timing for this device. */ >> +static int smc_timing_setup(struct smc_data *smc, struct device_node *np, >> + const struct at91_smc_devtype *devtype) >> +{ >> + int ret; >> + u32 cs; >> + int i; >> + struct device *dev = smc->dev; >> + const struct converter *converter; >> + const char *converter_name = NULL; >> + struct sam9_smc_config cfg; >> + >> + ret = of_property_read_u32(np, "smc,cs" , &cs); > > > Shouldn't this be stored in the reg property ? > After all, in your DM9000 patch you use "@cs,offset" to identify the node... True > > >> + if (ret < 0) { >> + dev_err(dev, "missing mandatory property : smc,cs\n"); >> + return ret; >> + } >> + if (cs >= 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; >> + } >> + >> + of_property_read_string(np, "smc,converter", &converter_name); > > > What about using the "compatible" property + struct of_device_id instead of > "smc,converter" property + struct converter ? Because one instance of the driver handles several chip selects and each may use a different converter. > > >> + if (converter_name) { >> + for (i = 0; i < ARRAY_SIZE(converters); i++) >> + if (strcmp(converters[i].name, converter_name) == >> 0) >> + converter = &converters[i]; >> + if (!converter) { >> + dev_info(dev, "unknown converter. aborting\n"); >> + return -EINVAL; >> + } >> + } else { >> + dev_dbg(dev, "cs %d: no smc converter provided. using " >> + "raw register values\n", cs); >> + converter = &converters[0]; >> + } >> + dev_dbg(dev, "cs %d using converter : %s\n", cs, converter->name); >> + sam9_smc_cs_read(smc->base + (0x10 * cs), &cfg); >> + converter->fn(smc, np, &cfg); >> + ret = sam9_smc_check_cs_configuration(&cfg); >> + if (ret < 0) { >> + dev_info(dev, "invalid smc configuration for cs %d." >> + "clipping values\n", cs); >> + sam9_smc_clip_cs_configuration(&cfg); >> + dump_timing(smc, &cfg); >> + } >> +#ifdef DEBUG >> + else >> + dump_timing(smc, &cfg); >> +#endif > > > I'm not a big fan of #ifdef blocks inside the code. > You could define a dummy dump_timing function if DEBUG is not defined: > > #ifdef DEBUG > > > static void dump_timing(struct smc_data *smc, struct sam9_smc_config > *config) > { > /* your implementation */ > } > > #else > > static inline void dump_timing(struct smc_data *smc, struct sam9_smc_config > *config) > { > } > > #endif > > Or just use dev_dbg when printing things in dump_timing. > I wanted to know the values when they were modified (clipped) by the driver. But it could be removed, knowing that clipping occurred is enough. > > >> + >> + sam9_smc_cs_configure(smc->base + (0x10 * cs), &cfg); >> + return 0; >> +} >> + >> +static int smc_parse_dt(struct smc_data *smc) >> +{ >> + struct device *dev = smc->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; >> + if (!of_device_is_available(child)) >> + continue; >> + ret = smc_timing_setup(smc, child, devtype); >> + if (ret) { >> + static struct property status = { >> + .name = "status", >> + .value = "disabled", >> + .length = sizeof("disabled"), >> + }; >> + dev_err(dev, "%s set timing failed. This node will >> be disabled.\n", >> + child->full_name); >> + ret = of_update_property(child, &status); >> + if (ret < 0) { >> + dev_err(dev, "can't disable %s. aborting >> probe\n", >> + child->full_name); >> + break; > > > The concept of disabling the device if timings cannot be met sounds > interresting... > Let's see what other maintainers say about this :). > > >> + } >> + } >> + } >> + >> + ret = of_platform_populate(dev->of_node, >> of_default_bus_match_table, >> + NULL, dev); >> + if (ret) >> + dev_err(dev, "%s fail to create devices.\n", >> + dev->of_node->full_name); >> + >> + return ret; >> +} >> + >> +static int smc_probe(struct platform_device *pdev) >> +{ >> + struct resource *res; >> + int ret; >> + void __iomem *base; >> + struct clk *clk; >> + struct smc_data *smc = devm_kzalloc(&pdev->dev, sizeof(struct >> smc_data), >> + GFP_KERNEL); >> + >> + if (!smc) >> + return -ENOMEM; >> + >> + smc->dev = &pdev->dev; >> + >> + /* 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); >> + } >> + >> + /* get the clock */ >> + clk = devm_clk_get(&pdev->dev, "smc"); >> + if (IS_ERR(clk)) >> + return PTR_ERR(clk); >> + >> + smc->bus_clk = clk; >> + smc->base = base; >> + >> + /* parse the device node */ >> + ret = smc_parse_dt(smc); >> + 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"); > > > > That's all for now. :) > > I'll try to test it this week end on a sama5 board. Thanks > > Best Regards, > > Boris
2014/1/9 Jean-Jacques Hiblot <jjhiblot@traphandler.com>: > Hi Boris, > > 2014/1/9 boris brezillon <b.brezillon@overkiz.com>: >> Hello JJ, >> >> >> On 09/01/2014 13:31, Jean-Jacques Hiblot wrote: >>> >>> 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. >>> It currently accepts timings in clock period and in nanoseconds. >>> >>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> >>> --- >>> drivers/memory/Kconfig | 10 ++ >>> drivers/memory/Makefile | 1 + >>> drivers/memory/atmel-smc.c | 431 >>> +++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 442 insertions(+) >>> create mode 100644 drivers/memory/atmel-smc.c >>> >>> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig >>> index 29a11db..fbdfd63 100644 >>> --- a/drivers/memory/Kconfig >>> +++ b/drivers/memory/Kconfig >>> @@ -50,4 +50,14 @@ config TEGRA30_MC >>> analysis, especially for IOMMU/SMMU(System Memory Management >>> Unit) module. >>> +config ATMEL_SMC >>> + bool "Atmel SMC/EBI driver" >>> + default y >>> + 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, etc. >>> + >>> endif >>> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile >>> index 969d923..101abc4 100644 >>> --- a/drivers/memory/Makefile >>> +++ b/drivers/memory/Makefile >>> @@ -9,3 +9,4 @@ obj-$(CONFIG_TI_EMIF) += emif.o >>> obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o >>> obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o >>> obj-$(CONFIG_TEGRA30_MC) += tegra30-mc.o >>> +obj-$(CONFIG_ATMEL_SMC) += atmel-smc.o >>> diff --git a/drivers/memory/atmel-smc.c b/drivers/memory/atmel-smc.c >>> new file mode 100644 >>> index 0000000..0a1d9ba >>> --- /dev/null >>> +++ b/drivers/memory/atmel-smc.c >>> @@ -0,0 +1,431 @@ >>> +/* >>> + * 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> >> >> >> You should avoid machine specific headers inclusions: we're trying to get >> rid of them. >> >> Duplicate the code and macros you need in your driver instead. > > Is this the right way? We usually try to avoid duplication. > >> >> >>> + >>> +struct smc_data { >>> + struct clk *bus_clk; >>> + void __iomem *base; >>> + struct device *dev; >>> +}; >>> + >>> +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 width; >>> + u16 shift; >>> +}; >>> + >>> +static const struct smc_parameters_type smc_parameters[] = { >>> + {"smc,burst_size", 2, 28}, >>> + {"smc,burst_enabled", 1, 24}, >>> + {"smc,tdf_mode", 1, 20}, >>> + {"smc,bus_width", 2, 12}, >>> + {"smc,byte_access_type", 1, 8}, >>> + {"smc,nwait_mode", 2, 4}, >>> + {"smc,write_mode", 1, 0}, >>> + {"smc,read_mode", 1, 1}, >>> + {NULL} >>> +}; >>> + >>> +static int get_mode_register_from_dt(struct smc_data *smc, >>> + struct device_node *np, >>> + struct sam9_smc_config *cfg) >>> +{ >>> + int ret; >>> + u32 val; >>> + struct device *dev = smc->dev; >>> + const struct smc_parameters_type *p = smc_parameters; >>> + u32 mode = cfg->mode; >>> + >>> + while (p->name) { >>> + ret = of_property_read_u32(np, p->name , &val); >>> + if (ret == -EINVAL) { >>> + dev_dbg(dev, "%s: property %s not set.\n", >>> np->name, >>> + p->name); >>> + p++; >>> + continue; >>> + } else if (ret) { >>> + dev_err(dev, "%s: can't get property %s.\n", >>> np->name, >>> + p->name); >>> + return ret; >>> + } >>> + if (val >= (1<<p->width)) { >>> + dev_err(dev, "%s: property %s out of range.\n", >>> + np->name, p->name); >>> + return -ERANGE; >>> + } >>> + mode &= ~(((1<<p->width)-1) << p->shift); >>> + mode |= (val << p->shift); >>> + p++; >>> + } >>> + cfg->mode = mode; >>> + return 0; >>> +} >>> + >>> +static int generic_timing_from_dt(struct smc_data *smc, struct >>> device_node *np, >>> + struct sam9_smc_config *cfg) >>> +{ >>> + u32 val; >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &val)) >>> + cfg->ncs_read_setup = val; >>> + >>> + if (!of_property_read_u32(np, "smc,nrd_setup" , &val)) >>> + cfg->nrd_setup = val; >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &val)) >>> + cfg->ncs_write_setup = val; >>> + >>> + if (!of_property_read_u32(np, "smc,nwe_setup" , &val)) >>> + cfg->nwe_setup = val; >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &val)) >>> + cfg->ncs_read_pulse = val; >>> + >>> + if (!of_property_read_u32(np, "smc,nrd_pulse" , &val)) >>> + cfg->nrd_pulse = val; >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &val)) >>> + cfg->ncs_write_pulse = val; >>> + >>> + if (!of_property_read_u32(np, "smc,nwe_pulse" , &val)) >>> + cfg->nwe_pulse = val; >>> + >>> + if (!of_property_read_u32(np, "smc,read_cycle" , &val)) >>> + cfg->read_cycle = val; >>> + >>> + if (!of_property_read_u32(np, "smc,write_cycle" , &val)) >>> + cfg->write_cycle = val; >>> + >>> + if (!of_property_read_u32(np, "smc,tdf_cycles" , &val)) >>> + cfg->tdf_cycles = val; >>> + >>> + return get_mode_register_from_dt(smc, np, cfg); >>> +} >>> + >>> +/* convert the time in ns in a number of clock cycles */ >>> +static u32 ns_to_cycles(u32 ns, u32 clk) >>> +{ >>> + /* >>> + * convert the clk to kHz for the rest of the calculation to avoid >>> + * overflow >>> + */ >>> + u32 clk_kHz = clk / 1000; >>> + u32 ncycles = ((ns * clk_kHz) + 1000000 - 1) / 1000000; >> >> What about using an u64 type and do_div ? > > easier and faster (though it's not the point here) this way, and kHz > ist not so imprecise :-) > >> >>> + return ncycles; >>> +} >>> + >>> +static u32 cycles_to_coded_cycle(u32 cycles, int a, int b) >>> +{ >>> + u32 mask_high = (1 << a) - 1; >>> + u32 mask_low = (1 << b) - 1; >>> + u32 coded; >>> + >>> + /* check if the value can be described with the coded format */ >>> + if (cycles & (mask_high & ~mask_low)) { >>> + /* not representable. we need to round up */ >>> + cycles |= mask_high; >>> + cycles += 1; >>> + } >>> + /* Now the value can be represented in the coded format */ >>> + coded = (cycles & ~mask_high) >> (a - b); >>> + coded |= (cycles & mask_low); >>> + return coded; >>> +} >>> + >>> +static u32 ns_to_rw_cycles(u32 ns, u32 clk) >>> +{ >>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 7); >>> +} >>> + >>> +static u32 ns_to_pulse_cycles(u32 ns, u32 clk) >>> +{ >>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 6); >>> +} >>> + >>> +static u32 ns_to_setup_cycles(u32 ns, u32 clk) >>> +{ >>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 7, 5); >>> +} >>> + >>> +static u32 cycles_to_ns(u32 cycles, u32 clk) >>> +{ >>> + /* >>> + * convert the clk to kHz for the rest of the calculation to avoid >>> + * overflow >>> + */ >>> + u32 clk_kHz = clk / 1000; >> >> >> Ditto (u64 + do_div). >> >>> + return (cycles * 1000000) / clk_kHz; >>> +} >>> + >>> +static u32 coded_cycle_to_cycles(u32 coded, int a, int b) >>> +{ >>> + u32 cycles = (coded >> b) << a; >>> + u32 mask_low = (1 << b) - 1; >>> + cycles |= (coded & mask_low); >>> + return cycles; >>> +} >>> + >>> +static u32 rw_cycles_to_ns(u32 reg, u32 clk) >>> +{ >>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 7), clk); >>> +} >>> + >>> +static u32 pulse_cycles_to_ns(u32 reg, u32 clk) >>> +{ >>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 6), clk); >>> +} >>> + >>> +static u32 setup_cycles_to_ns(u32 reg, u32 clk) >>> +{ >>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 7, 5), clk); >>> +} >>> + >>> +static void dump_timing(struct smc_data *smc, struct sam9_smc_config >>> *config) >>> +{ >>> + u32 freq = clk_get_rate(smc->bus_clk); >>> + struct device *dev = smc->dev; >>> + >>> +#define DUMP(fn, y) dev_info(dev, "%s : 0x%x (%d ns)\n", #y, >>> config->y,\ >>> + fn(config->y, freq)) >>> +#define DUMP_PULSE(y) DUMP(pulse_cycles_to_ns, y) >>> +#define DUMP_RWCYCLE(y) DUMP(rw_cycles_to_ns, y) >>> +#define DUMP_SETUP(y) DUMP(setup_cycles_to_ns, y) >>> +#define DUMP_SIMPLE(y) DUMP(cycles_to_ns, y) >>> + >>> + DUMP_SETUP(nwe_setup); >>> + DUMP_SETUP(ncs_write_setup); >>> + DUMP_SETUP(nrd_setup); >>> + DUMP_SETUP(ncs_read_setup); >>> + DUMP_PULSE(nwe_pulse); >>> + DUMP_PULSE(ncs_write_pulse); >>> + DUMP_PULSE(nrd_pulse); >>> + DUMP_PULSE(ncs_read_pulse); >>> + DUMP_RWCYCLE(write_cycle); >>> + DUMP_RWCYCLE(read_cycle); >>> + DUMP_SIMPLE(tdf_cycles); >>> +} >>> + >>> +static int ns_timing_from_dt(struct smc_data *smc, struct device_node >>> *np, >>> + struct sam9_smc_config *cfg) >>> +{ >>> + u32 t_ns; >>> + u32 freq = clk_get_rate(smc->bus_clk); >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &t_ns)) >>> + cfg->ncs_read_setup = ns_to_setup_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,nrd_setup" , &t_ns)) >>> + cfg->nrd_setup = ns_to_setup_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &t_ns)) >>> + cfg->ncs_write_setup = ns_to_setup_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,nwe_setup" , &t_ns)) >>> + cfg->nwe_setup = ns_to_setup_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &t_ns)) >>> + cfg->ncs_read_pulse = ns_to_pulse_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,nrd_pulse" , &t_ns)) >>> + cfg->nrd_pulse = ns_to_pulse_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &t_ns)) >>> + cfg->ncs_write_pulse = ns_to_pulse_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,nwe_pulse" , &t_ns)) >>> + cfg->nwe_pulse = ns_to_pulse_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,read_cycle" , &t_ns)) >>> + cfg->read_cycle = ns_to_rw_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,write_cycle" , &t_ns)) >>> + cfg->write_cycle = ns_to_rw_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,tdf_cycles" , &t_ns)) >>> + cfg->tdf_cycles = ns_to_cycles(t_ns, freq); >>> + >>> + return get_mode_register_from_dt(smc, np, cfg); >>> +} >>> + >>> +struct converter { >>> + const char *name; >>> + int (*fn) (struct smc_data *smc, struct device_node *np, >>> + struct sam9_smc_config *cfg); >>> +}; >>> +static const struct converter converters[] = { >>> + {"raw", generic_timing_from_dt}, >>> + {"nanosec", ns_timing_from_dt}, >>> +}; >> >> >> Could you use more specific names like: >> "atmel,smc-converter-generic" >> "atmel,smc-converter-nand" >> ... > Isn't it a bit redudant? smc,converter = "atmel,smc-converter-generic"; > >> >> IMHO the timing unit should be specified in the property names: >> smc,ncs_read_setup-ns >> smc,ncs_read_setup-cycles >> > True. Although cycles is misleading. It's more a raw register value. > For pulse, setup and rw cycle, the register value is not identical to > the number of cycles. >> >> >> >>> + >>> +/* Parse and set the timing for this device. */ >>> +static int smc_timing_setup(struct smc_data *smc, struct device_node *np, >>> + const struct at91_smc_devtype *devtype) >>> +{ >>> + int ret; >>> + u32 cs; >>> + int i; >>> + struct device *dev = smc->dev; >>> + const struct converter *converter; >>> + const char *converter_name = NULL; >>> + struct sam9_smc_config cfg; >>> + >>> + ret = of_property_read_u32(np, "smc,cs" , &cs); >> >> >> Shouldn't this be stored in the reg property ? >> After all, in your DM9000 patch you use "@cs,offset" to identify the node... > True I gave this more thoughts and don't think it should be in a reg property. We already have the cs information in the address translation. So maybe the CS should extr > >> >> >>> + if (ret < 0) { >>> + dev_err(dev, "missing mandatory property : smc,cs\n"); >>> + return ret; >>> + } >>> + if (cs >= 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; >>> + } >>> + >>> + of_property_read_string(np, "smc,converter", &converter_name); >> >> >> What about using the "compatible" property + struct of_device_id instead of >> "smc,converter" property + struct converter ? > > Because one instance of the driver handles several chip selects and > each may use a different converter. > >> >> >>> + if (converter_name) { >>> + for (i = 0; i < ARRAY_SIZE(converters); i++) >>> + if (strcmp(converters[i].name, converter_name) == >>> 0) >>> + converter = &converters[i]; >>> + if (!converter) { >>> + dev_info(dev, "unknown converter. aborting\n"); >>> + return -EINVAL; >>> + } >>> + } else { >>> + dev_dbg(dev, "cs %d: no smc converter provided. using " >>> + "raw register values\n", cs); >>> + converter = &converters[0]; >>> + } >>> + dev_dbg(dev, "cs %d using converter : %s\n", cs, converter->name); >>> + sam9_smc_cs_read(smc->base + (0x10 * cs), &cfg); >>> + converter->fn(smc, np, &cfg); >>> + ret = sam9_smc_check_cs_configuration(&cfg); >>> + if (ret < 0) { >>> + dev_info(dev, "invalid smc configuration for cs %d." >>> + "clipping values\n", cs); >>> + sam9_smc_clip_cs_configuration(&cfg); >>> + dump_timing(smc, &cfg); >>> + } >>> +#ifdef DEBUG >>> + else >>> + dump_timing(smc, &cfg); >>> +#endif >> >> >> I'm not a big fan of #ifdef blocks inside the code. >> You could define a dummy dump_timing function if DEBUG is not defined: >> >> #ifdef DEBUG >> >> >> static void dump_timing(struct smc_data *smc, struct sam9_smc_config >> *config) >> { >> /* your implementation */ >> } >> >> #else >> >> static inline void dump_timing(struct smc_data *smc, struct sam9_smc_config >> *config) >> { >> } >> >> #endif >> >> Or just use dev_dbg when printing things in dump_timing. >> > I wanted to know the values when they were modified (clipped) by the > driver. But it could be removed, knowing that clipping occurred is > enough. >> >> >>> + >>> + sam9_smc_cs_configure(smc->base + (0x10 * cs), &cfg); >>> + return 0; >>> +} >>> + >>> +static int smc_parse_dt(struct smc_data *smc) >>> +{ >>> + struct device *dev = smc->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; >>> + if (!of_device_is_available(child)) >>> + continue; >>> + ret = smc_timing_setup(smc, child, devtype); >>> + if (ret) { >>> + static struct property status = { >>> + .name = "status", >>> + .value = "disabled", >>> + .length = sizeof("disabled"), >>> + }; >>> + dev_err(dev, "%s set timing failed. This node will >>> be disabled.\n", >>> + child->full_name); >>> + ret = of_update_property(child, &status); >>> + if (ret < 0) { >>> + dev_err(dev, "can't disable %s. aborting >>> probe\n", >>> + child->full_name); >>> + break; >> >> >> The concept of disabling the device if timings cannot be met sounds >> interresting... >> Let's see what other maintainers say about this :). >> >> >>> + } >>> + } >>> + } >>> + >>> + ret = of_platform_populate(dev->of_node, >>> of_default_bus_match_table, >>> + NULL, dev); >>> + if (ret) >>> + dev_err(dev, "%s fail to create devices.\n", >>> + dev->of_node->full_name); >>> + >>> + return ret; >>> +} >>> + >>> +static int smc_probe(struct platform_device *pdev) >>> +{ >>> + struct resource *res; >>> + int ret; >>> + void __iomem *base; >>> + struct clk *clk; >>> + struct smc_data *smc = devm_kzalloc(&pdev->dev, sizeof(struct >>> smc_data), >>> + GFP_KERNEL); >>> + >>> + if (!smc) >>> + return -ENOMEM; >>> + >>> + smc->dev = &pdev->dev; >>> + >>> + /* 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); >>> + } >>> + >>> + /* get the clock */ >>> + clk = devm_clk_get(&pdev->dev, "smc"); >>> + if (IS_ERR(clk)) >>> + return PTR_ERR(clk); >>> + >>> + smc->bus_clk = clk; >>> + smc->base = base; >>> + >>> + /* parse the device node */ >>> + ret = smc_parse_dt(smc); >>> + 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"); >> >> >> >> That's all for now. :) >> >> I'll try to test it this week end on a sama5 board. > > Thanks > >> >> Best Regards, >> >> Boris
2014/1/10 Jean-Jacques Hiblot <jjhiblot@traphandler.com>: > 2014/1/9 Jean-Jacques Hiblot <jjhiblot@traphandler.com>: >> Hi Boris, >> >> 2014/1/9 boris brezillon <b.brezillon@overkiz.com>: >>> Hello JJ, >>> >>> >>> On 09/01/2014 13:31, Jean-Jacques Hiblot wrote: >>>> >>>> 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. >>>> It currently accepts timings in clock period and in nanoseconds. >>>> >>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> >>>> --- >>>> drivers/memory/Kconfig | 10 ++ >>>> drivers/memory/Makefile | 1 + >>>> drivers/memory/atmel-smc.c | 431 >>>> +++++++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 442 insertions(+) >>>> create mode 100644 drivers/memory/atmel-smc.c >>>> >>>> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig >>>> index 29a11db..fbdfd63 100644 >>>> --- a/drivers/memory/Kconfig >>>> +++ b/drivers/memory/Kconfig >>>> @@ -50,4 +50,14 @@ config TEGRA30_MC >>>> analysis, especially for IOMMU/SMMU(System Memory Management >>>> Unit) module. >>>> +config ATMEL_SMC >>>> + bool "Atmel SMC/EBI driver" >>>> + default y >>>> + 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, etc. >>>> + >>>> endif >>>> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile >>>> index 969d923..101abc4 100644 >>>> --- a/drivers/memory/Makefile >>>> +++ b/drivers/memory/Makefile >>>> @@ -9,3 +9,4 @@ obj-$(CONFIG_TI_EMIF) += emif.o >>>> obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o >>>> obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o >>>> obj-$(CONFIG_TEGRA30_MC) += tegra30-mc.o >>>> +obj-$(CONFIG_ATMEL_SMC) += atmel-smc.o >>>> diff --git a/drivers/memory/atmel-smc.c b/drivers/memory/atmel-smc.c >>>> new file mode 100644 >>>> index 0000000..0a1d9ba >>>> --- /dev/null >>>> +++ b/drivers/memory/atmel-smc.c >>>> @@ -0,0 +1,431 @@ >>>> +/* >>>> + * 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> >>> >>> >>> You should avoid machine specific headers inclusions: we're trying to get >>> rid of them. >>> >>> Duplicate the code and macros you need in your driver instead. >> >> Is this the right way? We usually try to avoid duplication. >> >>> >>> >>>> + >>>> +struct smc_data { >>>> + struct clk *bus_clk; >>>> + void __iomem *base; >>>> + struct device *dev; >>>> +}; >>>> + >>>> +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 width; >>>> + u16 shift; >>>> +}; >>>> + >>>> +static const struct smc_parameters_type smc_parameters[] = { >>>> + {"smc,burst_size", 2, 28}, >>>> + {"smc,burst_enabled", 1, 24}, >>>> + {"smc,tdf_mode", 1, 20}, >>>> + {"smc,bus_width", 2, 12}, >>>> + {"smc,byte_access_type", 1, 8}, >>>> + {"smc,nwait_mode", 2, 4}, >>>> + {"smc,write_mode", 1, 0}, >>>> + {"smc,read_mode", 1, 1}, >>>> + {NULL} >>>> +}; >>>> + >>>> +static int get_mode_register_from_dt(struct smc_data *smc, >>>> + struct device_node *np, >>>> + struct sam9_smc_config *cfg) >>>> +{ >>>> + int ret; >>>> + u32 val; >>>> + struct device *dev = smc->dev; >>>> + const struct smc_parameters_type *p = smc_parameters; >>>> + u32 mode = cfg->mode; >>>> + >>>> + while (p->name) { >>>> + ret = of_property_read_u32(np, p->name , &val); >>>> + if (ret == -EINVAL) { >>>> + dev_dbg(dev, "%s: property %s not set.\n", >>>> np->name, >>>> + p->name); >>>> + p++; >>>> + continue; >>>> + } else if (ret) { >>>> + dev_err(dev, "%s: can't get property %s.\n", >>>> np->name, >>>> + p->name); >>>> + return ret; >>>> + } >>>> + if (val >= (1<<p->width)) { >>>> + dev_err(dev, "%s: property %s out of range.\n", >>>> + np->name, p->name); >>>> + return -ERANGE; >>>> + } >>>> + mode &= ~(((1<<p->width)-1) << p->shift); >>>> + mode |= (val << p->shift); >>>> + p++; >>>> + } >>>> + cfg->mode = mode; >>>> + return 0; >>>> +} >>>> + >>>> +static int generic_timing_from_dt(struct smc_data *smc, struct >>>> device_node *np, >>>> + struct sam9_smc_config *cfg) >>>> +{ >>>> + u32 val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &val)) >>>> + cfg->ncs_read_setup = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,nrd_setup" , &val)) >>>> + cfg->nrd_setup = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &val)) >>>> + cfg->ncs_write_setup = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,nwe_setup" , &val)) >>>> + cfg->nwe_setup = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &val)) >>>> + cfg->ncs_read_pulse = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,nrd_pulse" , &val)) >>>> + cfg->nrd_pulse = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &val)) >>>> + cfg->ncs_write_pulse = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,nwe_pulse" , &val)) >>>> + cfg->nwe_pulse = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,read_cycle" , &val)) >>>> + cfg->read_cycle = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,write_cycle" , &val)) >>>> + cfg->write_cycle = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,tdf_cycles" , &val)) >>>> + cfg->tdf_cycles = val; >>>> + >>>> + return get_mode_register_from_dt(smc, np, cfg); >>>> +} >>>> + >>>> +/* convert the time in ns in a number of clock cycles */ >>>> +static u32 ns_to_cycles(u32 ns, u32 clk) >>>> +{ >>>> + /* >>>> + * convert the clk to kHz for the rest of the calculation to avoid >>>> + * overflow >>>> + */ >>>> + u32 clk_kHz = clk / 1000; >>>> + u32 ncycles = ((ns * clk_kHz) + 1000000 - 1) / 1000000; >>> >>> What about using an u64 type and do_div ? >> >> easier and faster (though it's not the point here) this way, and kHz >> ist not so imprecise :-) >> >>> >>>> + return ncycles; >>>> +} >>>> + >>>> +static u32 cycles_to_coded_cycle(u32 cycles, int a, int b) >>>> +{ >>>> + u32 mask_high = (1 << a) - 1; >>>> + u32 mask_low = (1 << b) - 1; >>>> + u32 coded; >>>> + >>>> + /* check if the value can be described with the coded format */ >>>> + if (cycles & (mask_high & ~mask_low)) { >>>> + /* not representable. we need to round up */ >>>> + cycles |= mask_high; >>>> + cycles += 1; >>>> + } >>>> + /* Now the value can be represented in the coded format */ >>>> + coded = (cycles & ~mask_high) >> (a - b); >>>> + coded |= (cycles & mask_low); >>>> + return coded; >>>> +} >>>> + >>>> +static u32 ns_to_rw_cycles(u32 ns, u32 clk) >>>> +{ >>>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 7); >>>> +} >>>> + >>>> +static u32 ns_to_pulse_cycles(u32 ns, u32 clk) >>>> +{ >>>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 6); >>>> +} >>>> + >>>> +static u32 ns_to_setup_cycles(u32 ns, u32 clk) >>>> +{ >>>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 7, 5); >>>> +} >>>> + >>>> +static u32 cycles_to_ns(u32 cycles, u32 clk) >>>> +{ >>>> + /* >>>> + * convert the clk to kHz for the rest of the calculation to avoid >>>> + * overflow >>>> + */ >>>> + u32 clk_kHz = clk / 1000; >>> >>> >>> Ditto (u64 + do_div). >>> >>>> + return (cycles * 1000000) / clk_kHz; >>>> +} >>>> + >>>> +static u32 coded_cycle_to_cycles(u32 coded, int a, int b) >>>> +{ >>>> + u32 cycles = (coded >> b) << a; >>>> + u32 mask_low = (1 << b) - 1; >>>> + cycles |= (coded & mask_low); >>>> + return cycles; >>>> +} >>>> + >>>> +static u32 rw_cycles_to_ns(u32 reg, u32 clk) >>>> +{ >>>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 7), clk); >>>> +} >>>> + >>>> +static u32 pulse_cycles_to_ns(u32 reg, u32 clk) >>>> +{ >>>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 6), clk); >>>> +} >>>> + >>>> +static u32 setup_cycles_to_ns(u32 reg, u32 clk) >>>> +{ >>>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 7, 5), clk); >>>> +} >>>> + >>>> +static void dump_timing(struct smc_data *smc, struct sam9_smc_config >>>> *config) >>>> +{ >>>> + u32 freq = clk_get_rate(smc->bus_clk); >>>> + struct device *dev = smc->dev; >>>> + >>>> +#define DUMP(fn, y) dev_info(dev, "%s : 0x%x (%d ns)\n", #y, >>>> config->y,\ >>>> + fn(config->y, freq)) >>>> +#define DUMP_PULSE(y) DUMP(pulse_cycles_to_ns, y) >>>> +#define DUMP_RWCYCLE(y) DUMP(rw_cycles_to_ns, y) >>>> +#define DUMP_SETUP(y) DUMP(setup_cycles_to_ns, y) >>>> +#define DUMP_SIMPLE(y) DUMP(cycles_to_ns, y) >>>> + >>>> + DUMP_SETUP(nwe_setup); >>>> + DUMP_SETUP(ncs_write_setup); >>>> + DUMP_SETUP(nrd_setup); >>>> + DUMP_SETUP(ncs_read_setup); >>>> + DUMP_PULSE(nwe_pulse); >>>> + DUMP_PULSE(ncs_write_pulse); >>>> + DUMP_PULSE(nrd_pulse); >>>> + DUMP_PULSE(ncs_read_pulse); >>>> + DUMP_RWCYCLE(write_cycle); >>>> + DUMP_RWCYCLE(read_cycle); >>>> + DUMP_SIMPLE(tdf_cycles); >>>> +} >>>> + >>>> +static int ns_timing_from_dt(struct smc_data *smc, struct device_node >>>> *np, >>>> + struct sam9_smc_config *cfg) >>>> +{ >>>> + u32 t_ns; >>>> + u32 freq = clk_get_rate(smc->bus_clk); >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &t_ns)) >>>> + cfg->ncs_read_setup = ns_to_setup_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,nrd_setup" , &t_ns)) >>>> + cfg->nrd_setup = ns_to_setup_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &t_ns)) >>>> + cfg->ncs_write_setup = ns_to_setup_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,nwe_setup" , &t_ns)) >>>> + cfg->nwe_setup = ns_to_setup_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &t_ns)) >>>> + cfg->ncs_read_pulse = ns_to_pulse_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,nrd_pulse" , &t_ns)) >>>> + cfg->nrd_pulse = ns_to_pulse_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &t_ns)) >>>> + cfg->ncs_write_pulse = ns_to_pulse_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,nwe_pulse" , &t_ns)) >>>> + cfg->nwe_pulse = ns_to_pulse_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,read_cycle" , &t_ns)) >>>> + cfg->read_cycle = ns_to_rw_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,write_cycle" , &t_ns)) >>>> + cfg->write_cycle = ns_to_rw_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,tdf_cycles" , &t_ns)) >>>> + cfg->tdf_cycles = ns_to_cycles(t_ns, freq); >>>> + >>>> + return get_mode_register_from_dt(smc, np, cfg); >>>> +} >>>> + >>>> +struct converter { >>>> + const char *name; >>>> + int (*fn) (struct smc_data *smc, struct device_node *np, >>>> + struct sam9_smc_config *cfg); >>>> +}; >>>> +static const struct converter converters[] = { >>>> + {"raw", generic_timing_from_dt}, >>>> + {"nanosec", ns_timing_from_dt}, >>>> +}; >>> >>> >>> Could you use more specific names like: >>> "atmel,smc-converter-generic" >>> "atmel,smc-converter-nand" >>> ... >> Isn't it a bit redudant? smc,converter = "atmel,smc-converter-generic"; >> >>> >>> IMHO the timing unit should be specified in the property names: >>> smc,ncs_read_setup-ns >>> smc,ncs_read_setup-cycles >>> >> True. Although cycles is misleading. It's more a raw register value. >> For pulse, setup and rw cycle, the register value is not identical to >> the number of cycles. >>> >>> >>> >>>> + >>>> +/* Parse and set the timing for this device. */ >>>> +static int smc_timing_setup(struct smc_data *smc, struct device_node *np, >>>> + const struct at91_smc_devtype *devtype) >>>> +{ >>>> + int ret; >>>> + u32 cs; >>>> + int i; >>>> + struct device *dev = smc->dev; >>>> + const struct converter *converter; >>>> + const char *converter_name = NULL; >>>> + struct sam9_smc_config cfg; >>>> + >>>> + ret = of_property_read_u32(np, "smc,cs" , &cs); >>> >>> >>> Shouldn't this be stored in the reg property ? >>> After all, in your DM9000 patch you use "@cs,offset" to identify the node... >> True > I gave this more thoughts and don't think it should be in a reg > property. We already have the cs information in the address > translation. So maybe the CS should extr sorry for the truncated message. I was about to write: I think the CS could be extracted from the range property. > >> >>> >>> >>>> + if (ret < 0) { >>>> + dev_err(dev, "missing mandatory property : smc,cs\n"); >>>> + return ret; >>>> + } >>>> + if (cs >= 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; >>>> + } >>>> + >>>> + of_property_read_string(np, "smc,converter", &converter_name); >>> >>> >>> What about using the "compatible" property + struct of_device_id instead of >>> "smc,converter" property + struct converter ? >> >> Because one instance of the driver handles several chip selects and >> each may use a different converter. >> >>> >>> >>>> + if (converter_name) { >>>> + for (i = 0; i < ARRAY_SIZE(converters); i++) >>>> + if (strcmp(converters[i].name, converter_name) == >>>> 0) >>>> + converter = &converters[i]; >>>> + if (!converter) { >>>> + dev_info(dev, "unknown converter. aborting\n"); >>>> + return -EINVAL; >>>> + } >>>> + } else { >>>> + dev_dbg(dev, "cs %d: no smc converter provided. using " >>>> + "raw register values\n", cs); >>>> + converter = &converters[0]; >>>> + } >>>> + dev_dbg(dev, "cs %d using converter : %s\n", cs, converter->name); >>>> + sam9_smc_cs_read(smc->base + (0x10 * cs), &cfg); >>>> + converter->fn(smc, np, &cfg); >>>> + ret = sam9_smc_check_cs_configuration(&cfg); >>>> + if (ret < 0) { >>>> + dev_info(dev, "invalid smc configuration for cs %d." >>>> + "clipping values\n", cs); >>>> + sam9_smc_clip_cs_configuration(&cfg); >>>> + dump_timing(smc, &cfg); >>>> + } >>>> +#ifdef DEBUG >>>> + else >>>> + dump_timing(smc, &cfg); >>>> +#endif >>> >>> >>> I'm not a big fan of #ifdef blocks inside the code. >>> You could define a dummy dump_timing function if DEBUG is not defined: >>> >>> #ifdef DEBUG >>> >>> >>> static void dump_timing(struct smc_data *smc, struct sam9_smc_config >>> *config) >>> { >>> /* your implementation */ >>> } >>> >>> #else >>> >>> static inline void dump_timing(struct smc_data *smc, struct sam9_smc_config >>> *config) >>> { >>> } >>> >>> #endif >>> >>> Or just use dev_dbg when printing things in dump_timing. >>> >> I wanted to know the values when they were modified (clipped) by the >> driver. But it could be removed, knowing that clipping occurred is >> enough. >>> >>> >>>> + >>>> + sam9_smc_cs_configure(smc->base + (0x10 * cs), &cfg); >>>> + return 0; >>>> +} >>>> + >>>> +static int smc_parse_dt(struct smc_data *smc) >>>> +{ >>>> + struct device *dev = smc->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; >>>> + if (!of_device_is_available(child)) >>>> + continue; >>>> + ret = smc_timing_setup(smc, child, devtype); >>>> + if (ret) { >>>> + static struct property status = { >>>> + .name = "status", >>>> + .value = "disabled", >>>> + .length = sizeof("disabled"), >>>> + }; >>>> + dev_err(dev, "%s set timing failed. This node will >>>> be disabled.\n", >>>> + child->full_name); >>>> + ret = of_update_property(child, &status); >>>> + if (ret < 0) { >>>> + dev_err(dev, "can't disable %s. aborting >>>> probe\n", >>>> + child->full_name); >>>> + break; >>> >>> >>> The concept of disabling the device if timings cannot be met sounds >>> interresting... >>> Let's see what other maintainers say about this :). >>> >>> >>>> + } >>>> + } >>>> + } >>>> + >>>> + ret = of_platform_populate(dev->of_node, >>>> of_default_bus_match_table, >>>> + NULL, dev); >>>> + if (ret) >>>> + dev_err(dev, "%s fail to create devices.\n", >>>> + dev->of_node->full_name); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int smc_probe(struct platform_device *pdev) >>>> +{ >>>> + struct resource *res; >>>> + int ret; >>>> + void __iomem *base; >>>> + struct clk *clk; >>>> + struct smc_data *smc = devm_kzalloc(&pdev->dev, sizeof(struct >>>> smc_data), >>>> + GFP_KERNEL); >>>> + >>>> + if (!smc) >>>> + return -ENOMEM; >>>> + >>>> + smc->dev = &pdev->dev; >>>> + >>>> + /* 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); >>>> + } >>>> + >>>> + /* get the clock */ >>>> + clk = devm_clk_get(&pdev->dev, "smc"); >>>> + if (IS_ERR(clk)) >>>> + return PTR_ERR(clk); >>>> + >>>> + smc->bus_clk = clk; >>>> + smc->base = base; >>>> + >>>> + /* parse the device node */ >>>> + ret = smc_parse_dt(smc); >>>> + 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"); >>> >>> >>> >>> That's all for now. :) >>> >>> I'll try to test it this week end on a sama5 board. >> >> Thanks >> >>> >>> Best Regards, >>> >>> Boris
On 09/01/2014 22:04, Jean-Jacques Hiblot wrote: > Hi Boris, > > 2014/1/9 boris brezillon <b.brezillon@overkiz.com>: >> Hello JJ, >> >> >> On 09/01/2014 13:31, Jean-Jacques Hiblot wrote: >>> 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. >>> It currently accepts timings in clock period and in nanoseconds. >>> >>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> >>> --- >>> drivers/memory/Kconfig | 10 ++ >>> drivers/memory/Makefile | 1 + >>> drivers/memory/atmel-smc.c | 431 >>> +++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 442 insertions(+) >>> create mode 100644 drivers/memory/atmel-smc.c >>> >>> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig >>> index 29a11db..fbdfd63 100644 >>> --- a/drivers/memory/Kconfig >>> +++ b/drivers/memory/Kconfig >>> @@ -50,4 +50,14 @@ config TEGRA30_MC >>> analysis, especially for IOMMU/SMMU(System Memory Management >>> Unit) module. >>> +config ATMEL_SMC >>> + bool "Atmel SMC/EBI driver" >>> + default y >>> + 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, etc. >>> + >>> endif >>> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile >>> index 969d923..101abc4 100644 >>> --- a/drivers/memory/Makefile >>> +++ b/drivers/memory/Makefile >>> @@ -9,3 +9,4 @@ obj-$(CONFIG_TI_EMIF) += emif.o >>> obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o >>> obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o >>> obj-$(CONFIG_TEGRA30_MC) += tegra30-mc.o >>> +obj-$(CONFIG_ATMEL_SMC) += atmel-smc.o >>> diff --git a/drivers/memory/atmel-smc.c b/drivers/memory/atmel-smc.c >>> new file mode 100644 >>> index 0000000..0a1d9ba >>> --- /dev/null >>> +++ b/drivers/memory/atmel-smc.c >>> @@ -0,0 +1,431 @@ >>> +/* >>> + * 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> >> >> You should avoid machine specific headers inclusions: we're trying to get >> rid of them. >> >> Duplicate the code and macros you need in your driver instead. > Is this the right way? Not necessarily. But in any case you should not reference machine specific headers in new drivers, because the ARM architecture maintainers are trying to get all architecture specific code moved into regular subsystems. There surely is a lot of pros to this approach (I'll let others detail these). The main one I see is that the subsystem maintainer is then able to track common designs in all available drivers and provide a common framework in order to : 1) ease other drivers development 2) avoid code duplication In your case, this leaves 2 solutions: 1) move the sam9_smc fonctions in the new driver and move the sam9_smc into include/linux/memory (which apparently does not exist). 2) copy all the functions and definition you need in your driver in order to get rid of the old implementation, and choose which one to compile using Kconfig options. If you think the sam9_smc existing implementation already match your new driver needs, I strongly recommend choosing solution 1, as it will help smoothly move to your new driver without having to modify already existing drivers using sam9_smc functions (except for the new header path ;)). > We usually try to avoid duplication. > >> >>> + >>> +struct smc_data { >>> + struct clk *bus_clk; >>> + void __iomem *base; >>> + struct device *dev; >>> +}; >>> + >>> +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 width; >>> + u16 shift; >>> +}; >>> + >>> +static const struct smc_parameters_type smc_parameters[] = { >>> + {"smc,burst_size", 2, 28}, >>> + {"smc,burst_enabled", 1, 24}, >>> + {"smc,tdf_mode", 1, 20}, >>> + {"smc,bus_width", 2, 12}, >>> + {"smc,byte_access_type", 1, 8}, >>> + {"smc,nwait_mode", 2, 4}, >>> + {"smc,write_mode", 1, 0}, >>> + {"smc,read_mode", 1, 1}, >>> + {NULL} >>> +}; >>> + >>> +static int get_mode_register_from_dt(struct smc_data *smc, >>> + struct device_node *np, >>> + struct sam9_smc_config *cfg) >>> +{ >>> + int ret; >>> + u32 val; >>> + struct device *dev = smc->dev; >>> + const struct smc_parameters_type *p = smc_parameters; >>> + u32 mode = cfg->mode; >>> + >>> + while (p->name) { >>> + ret = of_property_read_u32(np, p->name , &val); >>> + if (ret == -EINVAL) { >>> + dev_dbg(dev, "%s: property %s not set.\n", >>> np->name, >>> + p->name); >>> + p++; >>> + continue; >>> + } else if (ret) { >>> + dev_err(dev, "%s: can't get property %s.\n", >>> np->name, >>> + p->name); >>> + return ret; >>> + } >>> + if (val >= (1<<p->width)) { >>> + dev_err(dev, "%s: property %s out of range.\n", >>> + np->name, p->name); >>> + return -ERANGE; >>> + } >>> + mode &= ~(((1<<p->width)-1) << p->shift); >>> + mode |= (val << p->shift); >>> + p++; >>> + } >>> + cfg->mode = mode; >>> + return 0; >>> +} >>> + >>> +static int generic_timing_from_dt(struct smc_data *smc, struct >>> device_node *np, >>> + struct sam9_smc_config *cfg) >>> +{ >>> + u32 val; >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &val)) >>> + cfg->ncs_read_setup = val; >>> + >>> + if (!of_property_read_u32(np, "smc,nrd_setup" , &val)) >>> + cfg->nrd_setup = val; >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &val)) >>> + cfg->ncs_write_setup = val; >>> + >>> + if (!of_property_read_u32(np, "smc,nwe_setup" , &val)) >>> + cfg->nwe_setup = val; >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &val)) >>> + cfg->ncs_read_pulse = val; >>> + >>> + if (!of_property_read_u32(np, "smc,nrd_pulse" , &val)) >>> + cfg->nrd_pulse = val; >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &val)) >>> + cfg->ncs_write_pulse = val; >>> + >>> + if (!of_property_read_u32(np, "smc,nwe_pulse" , &val)) >>> + cfg->nwe_pulse = val; >>> + >>> + if (!of_property_read_u32(np, "smc,read_cycle" , &val)) >>> + cfg->read_cycle = val; >>> + >>> + if (!of_property_read_u32(np, "smc,write_cycle" , &val)) >>> + cfg->write_cycle = val; >>> + >>> + if (!of_property_read_u32(np, "smc,tdf_cycles" , &val)) >>> + cfg->tdf_cycles = val; >>> + >>> + return get_mode_register_from_dt(smc, np, cfg); >>> +} >>> + >>> +/* convert the time in ns in a number of clock cycles */ >>> +static u32 ns_to_cycles(u32 ns, u32 clk) >>> +{ >>> + /* >>> + * convert the clk to kHz for the rest of the calculation to avoid >>> + * overflow >>> + */ >>> + u32 clk_kHz = clk / 1000; >>> + u32 ncycles = ((ns * clk_kHz) + 1000000 - 1) / 1000000; >> What about using an u64 type and do_div ? > easier and faster (though it's not the point here) this way, and kHz > ist not so imprecise :-) > >>> + return ncycles; >>> +} >>> + >>> +static u32 cycles_to_coded_cycle(u32 cycles, int a, int b) >>> +{ >>> + u32 mask_high = (1 << a) - 1; >>> + u32 mask_low = (1 << b) - 1; >>> + u32 coded; >>> + >>> + /* check if the value can be described with the coded format */ >>> + if (cycles & (mask_high & ~mask_low)) { >>> + /* not representable. we need to round up */ >>> + cycles |= mask_high; >>> + cycles += 1; >>> + } >>> + /* Now the value can be represented in the coded format */ >>> + coded = (cycles & ~mask_high) >> (a - b); >>> + coded |= (cycles & mask_low); >>> + return coded; >>> +} >>> + >>> +static u32 ns_to_rw_cycles(u32 ns, u32 clk) >>> +{ >>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 7); >>> +} >>> + >>> +static u32 ns_to_pulse_cycles(u32 ns, u32 clk) >>> +{ >>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 6); >>> +} >>> + >>> +static u32 ns_to_setup_cycles(u32 ns, u32 clk) >>> +{ >>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 7, 5); >>> +} >>> + >>> +static u32 cycles_to_ns(u32 cycles, u32 clk) >>> +{ >>> + /* >>> + * convert the clk to kHz for the rest of the calculation to avoid >>> + * overflow >>> + */ >>> + u32 clk_kHz = clk / 1000; >> >> Ditto (u64 + do_div). >> >>> + return (cycles * 1000000) / clk_kHz; >>> +} >>> + >>> +static u32 coded_cycle_to_cycles(u32 coded, int a, int b) >>> +{ >>> + u32 cycles = (coded >> b) << a; >>> + u32 mask_low = (1 << b) - 1; >>> + cycles |= (coded & mask_low); >>> + return cycles; >>> +} >>> + >>> +static u32 rw_cycles_to_ns(u32 reg, u32 clk) >>> +{ >>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 7), clk); >>> +} >>> + >>> +static u32 pulse_cycles_to_ns(u32 reg, u32 clk) >>> +{ >>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 6), clk); >>> +} >>> + >>> +static u32 setup_cycles_to_ns(u32 reg, u32 clk) >>> +{ >>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 7, 5), clk); >>> +} >>> + >>> +static void dump_timing(struct smc_data *smc, struct sam9_smc_config >>> *config) >>> +{ >>> + u32 freq = clk_get_rate(smc->bus_clk); >>> + struct device *dev = smc->dev; >>> + >>> +#define DUMP(fn, y) dev_info(dev, "%s : 0x%x (%d ns)\n", #y, >>> config->y,\ >>> + fn(config->y, freq)) >>> +#define DUMP_PULSE(y) DUMP(pulse_cycles_to_ns, y) >>> +#define DUMP_RWCYCLE(y) DUMP(rw_cycles_to_ns, y) >>> +#define DUMP_SETUP(y) DUMP(setup_cycles_to_ns, y) >>> +#define DUMP_SIMPLE(y) DUMP(cycles_to_ns, y) >>> + >>> + DUMP_SETUP(nwe_setup); >>> + DUMP_SETUP(ncs_write_setup); >>> + DUMP_SETUP(nrd_setup); >>> + DUMP_SETUP(ncs_read_setup); >>> + DUMP_PULSE(nwe_pulse); >>> + DUMP_PULSE(ncs_write_pulse); >>> + DUMP_PULSE(nrd_pulse); >>> + DUMP_PULSE(ncs_read_pulse); >>> + DUMP_RWCYCLE(write_cycle); >>> + DUMP_RWCYCLE(read_cycle); >>> + DUMP_SIMPLE(tdf_cycles); >>> +} >>> + >>> +static int ns_timing_from_dt(struct smc_data *smc, struct device_node >>> *np, >>> + struct sam9_smc_config *cfg) >>> +{ >>> + u32 t_ns; >>> + u32 freq = clk_get_rate(smc->bus_clk); >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &t_ns)) >>> + cfg->ncs_read_setup = ns_to_setup_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,nrd_setup" , &t_ns)) >>> + cfg->nrd_setup = ns_to_setup_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &t_ns)) >>> + cfg->ncs_write_setup = ns_to_setup_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,nwe_setup" , &t_ns)) >>> + cfg->nwe_setup = ns_to_setup_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &t_ns)) >>> + cfg->ncs_read_pulse = ns_to_pulse_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,nrd_pulse" , &t_ns)) >>> + cfg->nrd_pulse = ns_to_pulse_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &t_ns)) >>> + cfg->ncs_write_pulse = ns_to_pulse_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,nwe_pulse" , &t_ns)) >>> + cfg->nwe_pulse = ns_to_pulse_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,read_cycle" , &t_ns)) >>> + cfg->read_cycle = ns_to_rw_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,write_cycle" , &t_ns)) >>> + cfg->write_cycle = ns_to_rw_cycles(t_ns, freq); >>> + >>> + if (!of_property_read_u32(np, "smc,tdf_cycles" , &t_ns)) >>> + cfg->tdf_cycles = ns_to_cycles(t_ns, freq); >>> + >>> + return get_mode_register_from_dt(smc, np, cfg); >>> +} >>> + >>> +struct converter { >>> + const char *name; >>> + int (*fn) (struct smc_data *smc, struct device_node *np, >>> + struct sam9_smc_config *cfg); >>> +}; >>> +static const struct converter converters[] = { >>> + {"raw", generic_timing_from_dt}, >>> + {"nanosec", ns_timing_from_dt}, >>> +}; >> >> Could you use more specific names like: >> "atmel,smc-converter-generic" >> "atmel,smc-converter-nand" >> ... > Isn't it a bit redudant? smc,converter = "atmel,smc-converter-generic"; > >> IMHO the timing unit should be specified in the property names: >> smc,ncs_read_setup-ns >> smc,ncs_read_setup-cycles >> > True. Although cycles is misleading. It's more a raw register value. > For pulse, setup and rw cycle, the register value is not identical to > the number of cycles. >> >> >>> + >>> +/* Parse and set the timing for this device. */ >>> +static int smc_timing_setup(struct smc_data *smc, struct device_node *np, >>> + const struct at91_smc_devtype *devtype) >>> +{ >>> + int ret; >>> + u32 cs; >>> + int i; >>> + struct device *dev = smc->dev; >>> + const struct converter *converter; >>> + const char *converter_name = NULL; >>> + struct sam9_smc_config cfg; >>> + >>> + ret = of_property_read_u32(np, "smc,cs" , &cs); >> >> Shouldn't this be stored in the reg property ? >> After all, in your DM9000 patch you use "@cs,offset" to identify the node... > True > >> >>> + if (ret < 0) { >>> + dev_err(dev, "missing mandatory property : smc,cs\n"); >>> + return ret; >>> + } >>> + if (cs >= 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; >>> + } >>> + >>> + of_property_read_string(np, "smc,converter", &converter_name); >> >> What about using the "compatible" property + struct of_device_id instead of >> "smc,converter" property + struct converter ? > Because one instance of the driver handles several chip selects and > each may use a different converter. > >> >>> + if (converter_name) { >>> + for (i = 0; i < ARRAY_SIZE(converters); i++) >>> + if (strcmp(converters[i].name, converter_name) == >>> 0) >>> + converter = &converters[i]; >>> + if (!converter) { >>> + dev_info(dev, "unknown converter. aborting\n"); >>> + return -EINVAL; >>> + } >>> + } else { >>> + dev_dbg(dev, "cs %d: no smc converter provided. using " >>> + "raw register values\n", cs); >>> + converter = &converters[0]; >>> + } >>> + dev_dbg(dev, "cs %d using converter : %s\n", cs, converter->name); >>> + sam9_smc_cs_read(smc->base + (0x10 * cs), &cfg); >>> + converter->fn(smc, np, &cfg); >>> + ret = sam9_smc_check_cs_configuration(&cfg); >>> + if (ret < 0) { >>> + dev_info(dev, "invalid smc configuration for cs %d." >>> + "clipping values\n", cs); >>> + sam9_smc_clip_cs_configuration(&cfg); >>> + dump_timing(smc, &cfg); >>> + } >>> +#ifdef DEBUG >>> + else >>> + dump_timing(smc, &cfg); >>> +#endif >> >> I'm not a big fan of #ifdef blocks inside the code. >> You could define a dummy dump_timing function if DEBUG is not defined: >> >> #ifdef DEBUG >> >> >> static void dump_timing(struct smc_data *smc, struct sam9_smc_config >> *config) >> { >> /* your implementation */ >> } >> >> #else >> >> static inline void dump_timing(struct smc_data *smc, struct sam9_smc_config >> *config) >> { >> } >> >> #endif >> >> Or just use dev_dbg when printing things in dump_timing. >> > I wanted to know the values when they were modified (clipped) by the > driver. But it could be removed, knowing that clipping occurred is > enough. >> >>> + >>> + sam9_smc_cs_configure(smc->base + (0x10 * cs), &cfg); >>> + return 0; >>> +} >>> + >>> +static int smc_parse_dt(struct smc_data *smc) >>> +{ >>> + struct device *dev = smc->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; >>> + if (!of_device_is_available(child)) >>> + continue; >>> + ret = smc_timing_setup(smc, child, devtype); >>> + if (ret) { >>> + static struct property status = { >>> + .name = "status", >>> + .value = "disabled", >>> + .length = sizeof("disabled"), >>> + }; >>> + dev_err(dev, "%s set timing failed. This node will >>> be disabled.\n", >>> + child->full_name); >>> + ret = of_update_property(child, &status); >>> + if (ret < 0) { >>> + dev_err(dev, "can't disable %s. aborting >>> probe\n", >>> + child->full_name); >>> + break; >> >> The concept of disabling the device if timings cannot be met sounds >> interresting... >> Let's see what other maintainers say about this :). >> >> >>> + } >>> + } >>> + } >>> + >>> + ret = of_platform_populate(dev->of_node, >>> of_default_bus_match_table, >>> + NULL, dev); >>> + if (ret) >>> + dev_err(dev, "%s fail to create devices.\n", >>> + dev->of_node->full_name); >>> + >>> + return ret; >>> +} >>> + >>> +static int smc_probe(struct platform_device *pdev) >>> +{ >>> + struct resource *res; >>> + int ret; >>> + void __iomem *base; >>> + struct clk *clk; >>> + struct smc_data *smc = devm_kzalloc(&pdev->dev, sizeof(struct >>> smc_data), >>> + GFP_KERNEL); >>> + >>> + if (!smc) >>> + return -ENOMEM; >>> + >>> + smc->dev = &pdev->dev; >>> + >>> + /* 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); >>> + } >>> + >>> + /* get the clock */ >>> + clk = devm_clk_get(&pdev->dev, "smc"); >>> + if (IS_ERR(clk)) >>> + return PTR_ERR(clk); >>> + >>> + smc->bus_clk = clk; >>> + smc->base = base; >>> + >>> + /* parse the device node */ >>> + ret = smc_parse_dt(smc); >>> + 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"); >> >> >> That's all for now. :) >> >> I'll try to test it this week end on a sama5 board. > Thanks > >> Best Regards, >> >> Boris
2014/1/11 boris brezillon <b.brezillon@overkiz.com>: > On 09/01/2014 22:04, Jean-Jacques Hiblot wrote: >> >> Hi Boris, >> >> 2014/1/9 boris brezillon <b.brezillon@overkiz.com>: >>> >>> Hello JJ, >>> >>> >>> On 09/01/2014 13:31, Jean-Jacques Hiblot wrote: >>>> >>>> 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. >>>> It currently accepts timings in clock period and in nanoseconds. >>>> >>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> >>>> --- >>>> drivers/memory/Kconfig | 10 ++ >>>> drivers/memory/Makefile | 1 + >>>> drivers/memory/atmel-smc.c | 431 >>>> +++++++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 442 insertions(+) >>>> create mode 100644 drivers/memory/atmel-smc.c >>>> >>>> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig >>>> index 29a11db..fbdfd63 100644 >>>> --- a/drivers/memory/Kconfig >>>> +++ b/drivers/memory/Kconfig >>>> @@ -50,4 +50,14 @@ config TEGRA30_MC >>>> analysis, especially for IOMMU/SMMU(System Memory Management >>>> Unit) module. >>>> +config ATMEL_SMC >>>> + bool "Atmel SMC/EBI driver" >>>> + default y >>>> + 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, etc. >>>> + >>>> endif >>>> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile >>>> index 969d923..101abc4 100644 >>>> --- a/drivers/memory/Makefile >>>> +++ b/drivers/memory/Makefile >>>> @@ -9,3 +9,4 @@ obj-$(CONFIG_TI_EMIF) += emif.o >>>> obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o >>>> obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o >>>> obj-$(CONFIG_TEGRA30_MC) += tegra30-mc.o >>>> +obj-$(CONFIG_ATMEL_SMC) += atmel-smc.o >>>> diff --git a/drivers/memory/atmel-smc.c b/drivers/memory/atmel-smc.c >>>> new file mode 100644 >>>> index 0000000..0a1d9ba >>>> --- /dev/null >>>> +++ b/drivers/memory/atmel-smc.c >>>> @@ -0,0 +1,431 @@ >>>> +/* >>>> + * 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> >>> >>> >>> You should avoid machine specific headers inclusions: we're trying to get >>> rid of them. >>> >>> Duplicate the code and macros you need in your driver instead. >> >> Is this the right way? > > Not necessarily. But in any case you should not reference machine specific > headers in new drivers, because the ARM architecture maintainers are > trying to get all architecture specific code moved into regular subsystems. > > There surely is a lot of pros to this approach (I'll let others detail > these). > The main one I see is that the subsystem maintainer is then able to track > common designs in all available drivers and provide a common framework > in order to : > 1) ease other drivers development > 2) avoid code duplication > > In your case, this leaves 2 solutions: > 1) move the sam9_smc fonctions in the new driver and move the sam9_smc > into include/linux/memory (which apparently does not exist). > 2) copy all the functions and definition you need in your driver in order to > get > rid of the old implementation, and choose which one to compile using > Kconfig > options. > > If you think the sam9_smc existing implementation already match your new > driver > needs, I strongly recommend choosing solution 1, as it will help smoothly > move to your > new driver without having to modify already existing drivers using sam9_smc > functions > (except for the new header path ;)). > I agree. Merging the code of arch/arm/mach-at91/sam9_smc.c into this new driver makes sense. Nicolas, Jean-Christophe, what is the stand point of the atmel's maintainers on this ? > >> We usually try to avoid duplication. > > > > >> >>> >>>> + >>>> +struct smc_data { >>>> + struct clk *bus_clk; >>>> + void __iomem *base; >>>> + struct device *dev; >>>> +}; >>>> + >>>> +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 width; >>>> + u16 shift; >>>> +}; >>>> + >>>> +static const struct smc_parameters_type smc_parameters[] = { >>>> + {"smc,burst_size", 2, 28}, >>>> + {"smc,burst_enabled", 1, 24}, >>>> + {"smc,tdf_mode", 1, 20}, >>>> + {"smc,bus_width", 2, 12}, >>>> + {"smc,byte_access_type", 1, 8}, >>>> + {"smc,nwait_mode", 2, 4}, >>>> + {"smc,write_mode", 1, 0}, >>>> + {"smc,read_mode", 1, 1}, >>>> + {NULL} >>>> +}; >>>> + >>>> +static int get_mode_register_from_dt(struct smc_data *smc, >>>> + struct device_node *np, >>>> + struct sam9_smc_config *cfg) >>>> +{ >>>> + int ret; >>>> + u32 val; >>>> + struct device *dev = smc->dev; >>>> + const struct smc_parameters_type *p = smc_parameters; >>>> + u32 mode = cfg->mode; >>>> + >>>> + while (p->name) { >>>> + ret = of_property_read_u32(np, p->name , &val); >>>> + if (ret == -EINVAL) { >>>> + dev_dbg(dev, "%s: property %s not set.\n", >>>> np->name, >>>> + p->name); >>>> + p++; >>>> + continue; >>>> + } else if (ret) { >>>> + dev_err(dev, "%s: can't get property %s.\n", >>>> np->name, >>>> + p->name); >>>> + return ret; >>>> + } >>>> + if (val >= (1<<p->width)) { >>>> + dev_err(dev, "%s: property %s out of range.\n", >>>> + np->name, p->name); >>>> + return -ERANGE; >>>> + } >>>> + mode &= ~(((1<<p->width)-1) << p->shift); >>>> + mode |= (val << p->shift); >>>> + p++; >>>> + } >>>> + cfg->mode = mode; >>>> + return 0; >>>> +} >>>> + >>>> +static int generic_timing_from_dt(struct smc_data *smc, struct >>>> device_node *np, >>>> + struct sam9_smc_config *cfg) >>>> +{ >>>> + u32 val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &val)) >>>> + cfg->ncs_read_setup = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,nrd_setup" , &val)) >>>> + cfg->nrd_setup = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &val)) >>>> + cfg->ncs_write_setup = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,nwe_setup" , &val)) >>>> + cfg->nwe_setup = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &val)) >>>> + cfg->ncs_read_pulse = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,nrd_pulse" , &val)) >>>> + cfg->nrd_pulse = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &val)) >>>> + cfg->ncs_write_pulse = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,nwe_pulse" , &val)) >>>> + cfg->nwe_pulse = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,read_cycle" , &val)) >>>> + cfg->read_cycle = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,write_cycle" , &val)) >>>> + cfg->write_cycle = val; >>>> + >>>> + if (!of_property_read_u32(np, "smc,tdf_cycles" , &val)) >>>> + cfg->tdf_cycles = val; >>>> + >>>> + return get_mode_register_from_dt(smc, np, cfg); >>>> +} >>>> + >>>> +/* convert the time in ns in a number of clock cycles */ >>>> +static u32 ns_to_cycles(u32 ns, u32 clk) >>>> +{ >>>> + /* >>>> + * convert the clk to kHz for the rest of the calculation to >>>> avoid >>>> + * overflow >>>> + */ >>>> + u32 clk_kHz = clk / 1000; >>>> + u32 ncycles = ((ns * clk_kHz) + 1000000 - 1) / 1000000; >>> >>> What about using an u64 type and do_div ? >> >> easier and faster (though it's not the point here) this way, and kHz >> ist not so imprecise :-) >> >>>> + return ncycles; >>>> +} >>>> + >>>> +static u32 cycles_to_coded_cycle(u32 cycles, int a, int b) >>>> +{ >>>> + u32 mask_high = (1 << a) - 1; >>>> + u32 mask_low = (1 << b) - 1; >>>> + u32 coded; >>>> + >>>> + /* check if the value can be described with the coded format */ >>>> + if (cycles & (mask_high & ~mask_low)) { >>>> + /* not representable. we need to round up */ >>>> + cycles |= mask_high; >>>> + cycles += 1; >>>> + } >>>> + /* Now the value can be represented in the coded format */ >>>> + coded = (cycles & ~mask_high) >> (a - b); >>>> + coded |= (cycles & mask_low); >>>> + return coded; >>>> +} >>>> + >>>> +static u32 ns_to_rw_cycles(u32 ns, u32 clk) >>>> +{ >>>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 7); >>>> +} >>>> + >>>> +static u32 ns_to_pulse_cycles(u32 ns, u32 clk) >>>> +{ >>>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 6); >>>> +} >>>> + >>>> +static u32 ns_to_setup_cycles(u32 ns, u32 clk) >>>> +{ >>>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 7, 5); >>>> +} >>>> + >>>> +static u32 cycles_to_ns(u32 cycles, u32 clk) >>>> +{ >>>> + /* >>>> + * convert the clk to kHz for the rest of the calculation to >>>> avoid >>>> + * overflow >>>> + */ >>>> + u32 clk_kHz = clk / 1000; >>> >>> >>> Ditto (u64 + do_div). >>> >>>> + return (cycles * 1000000) / clk_kHz; >>>> +} >>>> + >>>> +static u32 coded_cycle_to_cycles(u32 coded, int a, int b) >>>> +{ >>>> + u32 cycles = (coded >> b) << a; >>>> + u32 mask_low = (1 << b) - 1; >>>> + cycles |= (coded & mask_low); >>>> + return cycles; >>>> +} >>>> + >>>> +static u32 rw_cycles_to_ns(u32 reg, u32 clk) >>>> +{ >>>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 7), clk); >>>> +} >>>> + >>>> +static u32 pulse_cycles_to_ns(u32 reg, u32 clk) >>>> +{ >>>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 6), clk); >>>> +} >>>> + >>>> +static u32 setup_cycles_to_ns(u32 reg, u32 clk) >>>> +{ >>>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 7, 5), clk); >>>> +} >>>> + >>>> +static void dump_timing(struct smc_data *smc, struct sam9_smc_config >>>> *config) >>>> +{ >>>> + u32 freq = clk_get_rate(smc->bus_clk); >>>> + struct device *dev = smc->dev; >>>> + >>>> +#define DUMP(fn, y) dev_info(dev, "%s : 0x%x (%d ns)\n", #y, >>>> config->y,\ >>>> + fn(config->y, freq)) >>>> +#define DUMP_PULSE(y) DUMP(pulse_cycles_to_ns, y) >>>> +#define DUMP_RWCYCLE(y) DUMP(rw_cycles_to_ns, y) >>>> +#define DUMP_SETUP(y) DUMP(setup_cycles_to_ns, y) >>>> +#define DUMP_SIMPLE(y) DUMP(cycles_to_ns, y) >>>> + >>>> + DUMP_SETUP(nwe_setup); >>>> + DUMP_SETUP(ncs_write_setup); >>>> + DUMP_SETUP(nrd_setup); >>>> + DUMP_SETUP(ncs_read_setup); >>>> + DUMP_PULSE(nwe_pulse); >>>> + DUMP_PULSE(ncs_write_pulse); >>>> + DUMP_PULSE(nrd_pulse); >>>> + DUMP_PULSE(ncs_read_pulse); >>>> + DUMP_RWCYCLE(write_cycle); >>>> + DUMP_RWCYCLE(read_cycle); >>>> + DUMP_SIMPLE(tdf_cycles); >>>> +} >>>> + >>>> +static int ns_timing_from_dt(struct smc_data *smc, struct device_node >>>> *np, >>>> + struct sam9_smc_config *cfg) >>>> +{ >>>> + u32 t_ns; >>>> + u32 freq = clk_get_rate(smc->bus_clk); >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &t_ns)) >>>> + cfg->ncs_read_setup = ns_to_setup_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,nrd_setup" , &t_ns)) >>>> + cfg->nrd_setup = ns_to_setup_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &t_ns)) >>>> + cfg->ncs_write_setup = ns_to_setup_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,nwe_setup" , &t_ns)) >>>> + cfg->nwe_setup = ns_to_setup_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &t_ns)) >>>> + cfg->ncs_read_pulse = ns_to_pulse_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,nrd_pulse" , &t_ns)) >>>> + cfg->nrd_pulse = ns_to_pulse_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &t_ns)) >>>> + cfg->ncs_write_pulse = ns_to_pulse_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,nwe_pulse" , &t_ns)) >>>> + cfg->nwe_pulse = ns_to_pulse_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,read_cycle" , &t_ns)) >>>> + cfg->read_cycle = ns_to_rw_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,write_cycle" , &t_ns)) >>>> + cfg->write_cycle = ns_to_rw_cycles(t_ns, freq); >>>> + >>>> + if (!of_property_read_u32(np, "smc,tdf_cycles" , &t_ns)) >>>> + cfg->tdf_cycles = ns_to_cycles(t_ns, freq); >>>> + >>>> + return get_mode_register_from_dt(smc, np, cfg); >>>> +} >>>> + >>>> +struct converter { >>>> + const char *name; >>>> + int (*fn) (struct smc_data *smc, struct device_node *np, >>>> + struct sam9_smc_config *cfg); >>>> +}; >>>> +static const struct converter converters[] = { >>>> + {"raw", generic_timing_from_dt}, >>>> + {"nanosec", ns_timing_from_dt}, >>>> +}; >>> >>> >>> Could you use more specific names like: >>> "atmel,smc-converter-generic" >>> "atmel,smc-converter-nand" >>> ... >> >> Isn't it a bit redudant? smc,converter = "atmel,smc-converter-generic"; >> >>> IMHO the timing unit should be specified in the property names: >>> smc,ncs_read_setup-ns >>> smc,ncs_read_setup-cycles >>> >> True. Although cycles is misleading. It's more a raw register value. >> For pulse, setup and rw cycle, the register value is not identical to >> the number of cycles. >>> >>> >>> >>>> + >>>> +/* Parse and set the timing for this device. */ >>>> +static int smc_timing_setup(struct smc_data *smc, struct device_node >>>> *np, >>>> + const struct at91_smc_devtype *devtype) >>>> +{ >>>> + int ret; >>>> + u32 cs; >>>> + int i; >>>> + struct device *dev = smc->dev; >>>> + const struct converter *converter; >>>> + const char *converter_name = NULL; >>>> + struct sam9_smc_config cfg; >>>> + >>>> + ret = of_property_read_u32(np, "smc,cs" , &cs); >>> >>> >>> Shouldn't this be stored in the reg property ? >>> After all, in your DM9000 patch you use "@cs,offset" to identify the >>> node... >> >> True >> >>> >>>> + if (ret < 0) { >>>> + dev_err(dev, "missing mandatory property : smc,cs\n"); >>>> + return ret; >>>> + } >>>> + if (cs >= 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; >>>> + } >>>> + >>>> + of_property_read_string(np, "smc,converter", &converter_name); >>> >>> >>> What about using the "compatible" property + struct of_device_id instead >>> of >>> "smc,converter" property + struct converter ? >> >> Because one instance of the driver handles several chip selects and >> each may use a different converter. >> >>> >>>> + if (converter_name) { >>>> + for (i = 0; i < ARRAY_SIZE(converters); i++) >>>> + if (strcmp(converters[i].name, converter_name) >>>> == >>>> 0) >>>> + converter = &converters[i]; >>>> + if (!converter) { >>>> + dev_info(dev, "unknown converter. aborting\n"); >>>> + return -EINVAL; >>>> + } >>>> + } else { >>>> + dev_dbg(dev, "cs %d: no smc converter provided. using " >>>> + "raw register values\n", cs); >>>> + converter = &converters[0]; >>>> + } >>>> + dev_dbg(dev, "cs %d using converter : %s\n", cs, >>>> converter->name); >>>> + sam9_smc_cs_read(smc->base + (0x10 * cs), &cfg); >>>> + converter->fn(smc, np, &cfg); >>>> + ret = sam9_smc_check_cs_configuration(&cfg); >>>> + if (ret < 0) { >>>> + dev_info(dev, "invalid smc configuration for cs %d." >>>> + "clipping values\n", cs); >>>> + sam9_smc_clip_cs_configuration(&cfg); >>>> + dump_timing(smc, &cfg); >>>> + } >>>> +#ifdef DEBUG >>>> + else >>>> + dump_timing(smc, &cfg); >>>> +#endif >>> >>> >>> I'm not a big fan of #ifdef blocks inside the code. >>> You could define a dummy dump_timing function if DEBUG is not defined: >>> >>> #ifdef DEBUG >>> >>> >>> static void dump_timing(struct smc_data *smc, struct sam9_smc_config >>> *config) >>> { >>> /* your implementation */ >>> } >>> >>> #else >>> >>> static inline void dump_timing(struct smc_data *smc, struct >>> sam9_smc_config >>> *config) >>> { >>> } >>> >>> #endif >>> >>> Or just use dev_dbg when printing things in dump_timing. >>> >> I wanted to know the values when they were modified (clipped) by the >> driver. But it could be removed, knowing that clipping occurred is >> enough. >>> >>> >>>> + >>>> + sam9_smc_cs_configure(smc->base + (0x10 * cs), &cfg); >>>> + return 0; >>>> +} >>>> + >>>> +static int smc_parse_dt(struct smc_data *smc) >>>> +{ >>>> + struct device *dev = smc->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; >>>> + if (!of_device_is_available(child)) >>>> + continue; >>>> + ret = smc_timing_setup(smc, child, devtype); >>>> + if (ret) { >>>> + static struct property status = { >>>> + .name = "status", >>>> + .value = "disabled", >>>> + .length = sizeof("disabled"), >>>> + }; >>>> + dev_err(dev, "%s set timing failed. This node >>>> will >>>> be disabled.\n", >>>> + child->full_name); >>>> + ret = of_update_property(child, &status); >>>> + if (ret < 0) { >>>> + dev_err(dev, "can't disable %s. aborting >>>> probe\n", >>>> + child->full_name); >>>> + break; >>> >>> >>> The concept of disabling the device if timings cannot be met sounds >>> interresting... >>> Let's see what other maintainers say about this :). >>> >>> >>>> + } >>>> + } >>>> + } >>>> + >>>> + ret = of_platform_populate(dev->of_node, >>>> of_default_bus_match_table, >>>> + NULL, dev); >>>> + if (ret) >>>> + dev_err(dev, "%s fail to create devices.\n", >>>> + dev->of_node->full_name); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int smc_probe(struct platform_device *pdev) >>>> +{ >>>> + struct resource *res; >>>> + int ret; >>>> + void __iomem *base; >>>> + struct clk *clk; >>>> + struct smc_data *smc = devm_kzalloc(&pdev->dev, sizeof(struct >>>> smc_data), >>>> + GFP_KERNEL); >>>> + >>>> + if (!smc) >>>> + return -ENOMEM; >>>> + >>>> + smc->dev = &pdev->dev; >>>> + >>>> + /* 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); >>>> + } >>>> + >>>> + /* get the clock */ >>>> + clk = devm_clk_get(&pdev->dev, "smc"); >>>> + if (IS_ERR(clk)) >>>> + return PTR_ERR(clk); >>>> + >>>> + smc->bus_clk = clk; >>>> + smc->base = base; >>>> + >>>> + /* parse the device node */ >>>> + ret = smc_parse_dt(smc); >>>> + 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"); >>> >>> >>> >>> That's all for now. :) >>> >>> I'll try to test it this week end on a sama5 board. >> >> Thanks >> >>> Best Regards, >>> >>> Boris > >
On 14/01/2014 15:20, Jean-Jacques Hiblot : > 2014/1/11 boris brezillon <b.brezillon@overkiz.com>: >> On 09/01/2014 22:04, Jean-Jacques Hiblot wrote: >>> >>> Hi Boris, >>> >>> 2014/1/9 boris brezillon <b.brezillon@overkiz.com>: >>>> >>>> Hello JJ, >>>> >>>> >>>> On 09/01/2014 13:31, Jean-Jacques Hiblot wrote: >>>>> >>>>> 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. >>>>> It currently accepts timings in clock period and in nanoseconds. >>>>> >>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> >>>>> --- >>>>> drivers/memory/Kconfig | 10 ++ >>>>> drivers/memory/Makefile | 1 + >>>>> drivers/memory/atmel-smc.c | 431 >>>>> +++++++++++++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 442 insertions(+) >>>>> create mode 100644 drivers/memory/atmel-smc.c >>>>> >>>>> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig >>>>> index 29a11db..fbdfd63 100644 >>>>> --- a/drivers/memory/Kconfig >>>>> +++ b/drivers/memory/Kconfig >>>>> @@ -50,4 +50,14 @@ config TEGRA30_MC >>>>> analysis, especially for IOMMU/SMMU(System Memory Management >>>>> Unit) module. >>>>> +config ATMEL_SMC >>>>> + bool "Atmel SMC/EBI driver" >>>>> + default y >>>>> + 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, etc. >>>>> + >>>>> endif >>>>> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile >>>>> index 969d923..101abc4 100644 >>>>> --- a/drivers/memory/Makefile >>>>> +++ b/drivers/memory/Makefile >>>>> @@ -9,3 +9,4 @@ obj-$(CONFIG_TI_EMIF) += emif.o >>>>> obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o >>>>> obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o >>>>> obj-$(CONFIG_TEGRA30_MC) += tegra30-mc.o >>>>> +obj-$(CONFIG_ATMEL_SMC) += atmel-smc.o >>>>> diff --git a/drivers/memory/atmel-smc.c b/drivers/memory/atmel-smc.c >>>>> new file mode 100644 >>>>> index 0000000..0a1d9ba >>>>> --- /dev/null >>>>> +++ b/drivers/memory/atmel-smc.c >>>>> @@ -0,0 +1,431 @@ >>>>> +/* >>>>> + * 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> >>>> >>>> >>>> You should avoid machine specific headers inclusions: we're trying to get >>>> rid of them. >>>> >>>> Duplicate the code and macros you need in your driver instead. >>> >>> Is this the right way? >> >> Not necessarily. But in any case you should not reference machine specific >> headers in new drivers, because the ARM architecture maintainers are >> trying to get all architecture specific code moved into regular subsystems. >> >> There surely is a lot of pros to this approach (I'll let others detail >> these). >> The main one I see is that the subsystem maintainer is then able to track >> common designs in all available drivers and provide a common framework >> in order to : >> 1) ease other drivers development >> 2) avoid code duplication >> >> In your case, this leaves 2 solutions: >> 1) move the sam9_smc fonctions in the new driver and move the sam9_smc >> into include/linux/memory (which apparently does not exist). >> 2) copy all the functions and definition you need in your driver in order to >> get >> rid of the old implementation, and choose which one to compile using >> Kconfig >> options. >> >> If you think the sam9_smc existing implementation already match your new >> driver >> needs, I strongly recommend choosing solution 1, as it will help smoothly >> move to your >> new driver without having to modify already existing drivers using sam9_smc >> functions >> (except for the new header path ;)). >> > I agree. Merging the code of arch/arm/mach-at91/sam9_smc.c into this > new driver makes sense. > Nicolas, Jean-Christophe, what is the stand point of the atmel's > maintainers on this ? I do agree with solution 1. We will need to pay attention to the path modification on other files but the benefit of having a SMC driver and the move of code out of the mach-at91 directory definitively worth it. Thanks for you work Jean-Jacques. Bye, >>> We usually try to avoid duplication. >> >>> >>>> >>>>> + >>>>> +struct smc_data { >>>>> + struct clk *bus_clk; >>>>> + void __iomem *base; >>>>> + struct device *dev; >>>>> +}; >>>>> + >>>>> +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 width; >>>>> + u16 shift; >>>>> +}; >>>>> + >>>>> +static const struct smc_parameters_type smc_parameters[] = { >>>>> + {"smc,burst_size", 2, 28}, >>>>> + {"smc,burst_enabled", 1, 24}, >>>>> + {"smc,tdf_mode", 1, 20}, >>>>> + {"smc,bus_width", 2, 12}, >>>>> + {"smc,byte_access_type", 1, 8}, >>>>> + {"smc,nwait_mode", 2, 4}, >>>>> + {"smc,write_mode", 1, 0}, >>>>> + {"smc,read_mode", 1, 1}, >>>>> + {NULL} >>>>> +}; >>>>> + >>>>> +static int get_mode_register_from_dt(struct smc_data *smc, >>>>> + struct device_node *np, >>>>> + struct sam9_smc_config *cfg) >>>>> +{ >>>>> + int ret; >>>>> + u32 val; >>>>> + struct device *dev = smc->dev; >>>>> + const struct smc_parameters_type *p = smc_parameters; >>>>> + u32 mode = cfg->mode; >>>>> + >>>>> + while (p->name) { >>>>> + ret = of_property_read_u32(np, p->name , &val); >>>>> + if (ret == -EINVAL) { >>>>> + dev_dbg(dev, "%s: property %s not set.\n", >>>>> np->name, >>>>> + p->name); >>>>> + p++; >>>>> + continue; >>>>> + } else if (ret) { >>>>> + dev_err(dev, "%s: can't get property %s.\n", >>>>> np->name, >>>>> + p->name); >>>>> + return ret; >>>>> + } >>>>> + if (val >= (1<<p->width)) { >>>>> + dev_err(dev, "%s: property %s out of range.\n", >>>>> + np->name, p->name); >>>>> + return -ERANGE; >>>>> + } >>>>> + mode &= ~(((1<<p->width)-1) << p->shift); >>>>> + mode |= (val << p->shift); >>>>> + p++; >>>>> + } >>>>> + cfg->mode = mode; >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int generic_timing_from_dt(struct smc_data *smc, struct >>>>> device_node *np, >>>>> + struct sam9_smc_config *cfg) >>>>> +{ >>>>> + u32 val; >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &val)) >>>>> + cfg->ncs_read_setup = val; >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,nrd_setup" , &val)) >>>>> + cfg->nrd_setup = val; >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &val)) >>>>> + cfg->ncs_write_setup = val; >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,nwe_setup" , &val)) >>>>> + cfg->nwe_setup = val; >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &val)) >>>>> + cfg->ncs_read_pulse = val; >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,nrd_pulse" , &val)) >>>>> + cfg->nrd_pulse = val; >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &val)) >>>>> + cfg->ncs_write_pulse = val; >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,nwe_pulse" , &val)) >>>>> + cfg->nwe_pulse = val; >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,read_cycle" , &val)) >>>>> + cfg->read_cycle = val; >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,write_cycle" , &val)) >>>>> + cfg->write_cycle = val; >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,tdf_cycles" , &val)) >>>>> + cfg->tdf_cycles = val; >>>>> + >>>>> + return get_mode_register_from_dt(smc, np, cfg); >>>>> +} >>>>> + >>>>> +/* convert the time in ns in a number of clock cycles */ >>>>> +static u32 ns_to_cycles(u32 ns, u32 clk) >>>>> +{ >>>>> + /* >>>>> + * convert the clk to kHz for the rest of the calculation to >>>>> avoid >>>>> + * overflow >>>>> + */ >>>>> + u32 clk_kHz = clk / 1000; >>>>> + u32 ncycles = ((ns * clk_kHz) + 1000000 - 1) / 1000000; >>>> >>>> What about using an u64 type and do_div ? >>> >>> easier and faster (though it's not the point here) this way, and kHz >>> ist not so imprecise :-) >>> >>>>> + return ncycles; >>>>> +} >>>>> + >>>>> +static u32 cycles_to_coded_cycle(u32 cycles, int a, int b) >>>>> +{ >>>>> + u32 mask_high = (1 << a) - 1; >>>>> + u32 mask_low = (1 << b) - 1; >>>>> + u32 coded; >>>>> + >>>>> + /* check if the value can be described with the coded format */ >>>>> + if (cycles & (mask_high & ~mask_low)) { >>>>> + /* not representable. we need to round up */ >>>>> + cycles |= mask_high; >>>>> + cycles += 1; >>>>> + } >>>>> + /* Now the value can be represented in the coded format */ >>>>> + coded = (cycles & ~mask_high) >> (a - b); >>>>> + coded |= (cycles & mask_low); >>>>> + return coded; >>>>> +} >>>>> + >>>>> +static u32 ns_to_rw_cycles(u32 ns, u32 clk) >>>>> +{ >>>>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 7); >>>>> +} >>>>> + >>>>> +static u32 ns_to_pulse_cycles(u32 ns, u32 clk) >>>>> +{ >>>>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 6); >>>>> +} >>>>> + >>>>> +static u32 ns_to_setup_cycles(u32 ns, u32 clk) >>>>> +{ >>>>> + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 7, 5); >>>>> +} >>>>> + >>>>> +static u32 cycles_to_ns(u32 cycles, u32 clk) >>>>> +{ >>>>> + /* >>>>> + * convert the clk to kHz for the rest of the calculation to >>>>> avoid >>>>> + * overflow >>>>> + */ >>>>> + u32 clk_kHz = clk / 1000; >>>> >>>> >>>> Ditto (u64 + do_div). >>>> >>>>> + return (cycles * 1000000) / clk_kHz; >>>>> +} >>>>> + >>>>> +static u32 coded_cycle_to_cycles(u32 coded, int a, int b) >>>>> +{ >>>>> + u32 cycles = (coded >> b) << a; >>>>> + u32 mask_low = (1 << b) - 1; >>>>> + cycles |= (coded & mask_low); >>>>> + return cycles; >>>>> +} >>>>> + >>>>> +static u32 rw_cycles_to_ns(u32 reg, u32 clk) >>>>> +{ >>>>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 7), clk); >>>>> +} >>>>> + >>>>> +static u32 pulse_cycles_to_ns(u32 reg, u32 clk) >>>>> +{ >>>>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 6), clk); >>>>> +} >>>>> + >>>>> +static u32 setup_cycles_to_ns(u32 reg, u32 clk) >>>>> +{ >>>>> + return cycles_to_ns(coded_cycle_to_cycles(reg, 7, 5), clk); >>>>> +} >>>>> + >>>>> +static void dump_timing(struct smc_data *smc, struct sam9_smc_config >>>>> *config) >>>>> +{ >>>>> + u32 freq = clk_get_rate(smc->bus_clk); >>>>> + struct device *dev = smc->dev; >>>>> + >>>>> +#define DUMP(fn, y) dev_info(dev, "%s : 0x%x (%d ns)\n", #y, >>>>> config->y,\ >>>>> + fn(config->y, freq)) >>>>> +#define DUMP_PULSE(y) DUMP(pulse_cycles_to_ns, y) >>>>> +#define DUMP_RWCYCLE(y) DUMP(rw_cycles_to_ns, y) >>>>> +#define DUMP_SETUP(y) DUMP(setup_cycles_to_ns, y) >>>>> +#define DUMP_SIMPLE(y) DUMP(cycles_to_ns, y) >>>>> + >>>>> + DUMP_SETUP(nwe_setup); >>>>> + DUMP_SETUP(ncs_write_setup); >>>>> + DUMP_SETUP(nrd_setup); >>>>> + DUMP_SETUP(ncs_read_setup); >>>>> + DUMP_PULSE(nwe_pulse); >>>>> + DUMP_PULSE(ncs_write_pulse); >>>>> + DUMP_PULSE(nrd_pulse); >>>>> + DUMP_PULSE(ncs_read_pulse); >>>>> + DUMP_RWCYCLE(write_cycle); >>>>> + DUMP_RWCYCLE(read_cycle); >>>>> + DUMP_SIMPLE(tdf_cycles); >>>>> +} >>>>> + >>>>> +static int ns_timing_from_dt(struct smc_data *smc, struct device_node >>>>> *np, >>>>> + struct sam9_smc_config *cfg) >>>>> +{ >>>>> + u32 t_ns; >>>>> + u32 freq = clk_get_rate(smc->bus_clk); >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &t_ns)) >>>>> + cfg->ncs_read_setup = ns_to_setup_cycles(t_ns, freq); >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,nrd_setup" , &t_ns)) >>>>> + cfg->nrd_setup = ns_to_setup_cycles(t_ns, freq); >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &t_ns)) >>>>> + cfg->ncs_write_setup = ns_to_setup_cycles(t_ns, freq); >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,nwe_setup" , &t_ns)) >>>>> + cfg->nwe_setup = ns_to_setup_cycles(t_ns, freq); >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &t_ns)) >>>>> + cfg->ncs_read_pulse = ns_to_pulse_cycles(t_ns, freq); >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,nrd_pulse" , &t_ns)) >>>>> + cfg->nrd_pulse = ns_to_pulse_cycles(t_ns, freq); >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &t_ns)) >>>>> + cfg->ncs_write_pulse = ns_to_pulse_cycles(t_ns, freq); >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,nwe_pulse" , &t_ns)) >>>>> + cfg->nwe_pulse = ns_to_pulse_cycles(t_ns, freq); >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,read_cycle" , &t_ns)) >>>>> + cfg->read_cycle = ns_to_rw_cycles(t_ns, freq); >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,write_cycle" , &t_ns)) >>>>> + cfg->write_cycle = ns_to_rw_cycles(t_ns, freq); >>>>> + >>>>> + if (!of_property_read_u32(np, "smc,tdf_cycles" , &t_ns)) >>>>> + cfg->tdf_cycles = ns_to_cycles(t_ns, freq); >>>>> + >>>>> + return get_mode_register_from_dt(smc, np, cfg); >>>>> +} >>>>> + >>>>> +struct converter { >>>>> + const char *name; >>>>> + int (*fn) (struct smc_data *smc, struct device_node *np, >>>>> + struct sam9_smc_config *cfg); >>>>> +}; >>>>> +static const struct converter converters[] = { >>>>> + {"raw", generic_timing_from_dt}, >>>>> + {"nanosec", ns_timing_from_dt}, >>>>> +}; >>>> >>>> >>>> Could you use more specific names like: >>>> "atmel,smc-converter-generic" >>>> "atmel,smc-converter-nand" >>>> ... >>> >>> Isn't it a bit redudant? smc,converter = "atmel,smc-converter-generic"; >>> >>>> IMHO the timing unit should be specified in the property names: >>>> smc,ncs_read_setup-ns >>>> smc,ncs_read_setup-cycles >>>> >>> True. Although cycles is misleading. It's more a raw register value. >>> For pulse, setup and rw cycle, the register value is not identical to >>> the number of cycles. >>>> >>>> >>>> >>>>> + >>>>> +/* Parse and set the timing for this device. */ >>>>> +static int smc_timing_setup(struct smc_data *smc, struct device_node >>>>> *np, >>>>> + const struct at91_smc_devtype *devtype) >>>>> +{ >>>>> + int ret; >>>>> + u32 cs; >>>>> + int i; >>>>> + struct device *dev = smc->dev; >>>>> + const struct converter *converter; >>>>> + const char *converter_name = NULL; >>>>> + struct sam9_smc_config cfg; >>>>> + >>>>> + ret = of_property_read_u32(np, "smc,cs" , &cs); >>>> >>>> >>>> Shouldn't this be stored in the reg property ? >>>> After all, in your DM9000 patch you use "@cs,offset" to identify the >>>> node... >>> >>> True >>> >>>> >>>>> + if (ret < 0) { >>>>> + dev_err(dev, "missing mandatory property : smc,cs\n"); >>>>> + return ret; >>>>> + } >>>>> + if (cs >= 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; >>>>> + } >>>>> + >>>>> + of_property_read_string(np, "smc,converter", &converter_name); >>>> >>>> >>>> What about using the "compatible" property + struct of_device_id instead >>>> of >>>> "smc,converter" property + struct converter ? >>> >>> Because one instance of the driver handles several chip selects and >>> each may use a different converter. >>> >>>> >>>>> + if (converter_name) { >>>>> + for (i = 0; i < ARRAY_SIZE(converters); i++) >>>>> + if (strcmp(converters[i].name, converter_name) >>>>> == >>>>> 0) >>>>> + converter = &converters[i]; >>>>> + if (!converter) { >>>>> + dev_info(dev, "unknown converter. aborting\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> + } else { >>>>> + dev_dbg(dev, "cs %d: no smc converter provided. using " >>>>> + "raw register values\n", cs); >>>>> + converter = &converters[0]; >>>>> + } >>>>> + dev_dbg(dev, "cs %d using converter : %s\n", cs, >>>>> converter->name); >>>>> + sam9_smc_cs_read(smc->base + (0x10 * cs), &cfg); >>>>> + converter->fn(smc, np, &cfg); >>>>> + ret = sam9_smc_check_cs_configuration(&cfg); >>>>> + if (ret < 0) { >>>>> + dev_info(dev, "invalid smc configuration for cs %d." >>>>> + "clipping values\n", cs); >>>>> + sam9_smc_clip_cs_configuration(&cfg); >>>>> + dump_timing(smc, &cfg); >>>>> + } >>>>> +#ifdef DEBUG >>>>> + else >>>>> + dump_timing(smc, &cfg); >>>>> +#endif >>>> >>>> >>>> I'm not a big fan of #ifdef blocks inside the code. >>>> You could define a dummy dump_timing function if DEBUG is not defined: >>>> >>>> #ifdef DEBUG >>>> >>>> >>>> static void dump_timing(struct smc_data *smc, struct sam9_smc_config >>>> *config) >>>> { >>>> /* your implementation */ >>>> } >>>> >>>> #else >>>> >>>> static inline void dump_timing(struct smc_data *smc, struct >>>> sam9_smc_config >>>> *config) >>>> { >>>> } >>>> >>>> #endif >>>> >>>> Or just use dev_dbg when printing things in dump_timing. >>>> >>> I wanted to know the values when they were modified (clipped) by the >>> driver. But it could be removed, knowing that clipping occurred is >>> enough. >>>> >>>> >>>>> + >>>>> + sam9_smc_cs_configure(smc->base + (0x10 * cs), &cfg); >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int smc_parse_dt(struct smc_data *smc) >>>>> +{ >>>>> + struct device *dev = smc->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; >>>>> + if (!of_device_is_available(child)) >>>>> + continue; >>>>> + ret = smc_timing_setup(smc, child, devtype); >>>>> + if (ret) { >>>>> + static struct property status = { >>>>> + .name = "status", >>>>> + .value = "disabled", >>>>> + .length = sizeof("disabled"), >>>>> + }; >>>>> + dev_err(dev, "%s set timing failed. This node >>>>> will >>>>> be disabled.\n", >>>>> + child->full_name); >>>>> + ret = of_update_property(child, &status); >>>>> + if (ret < 0) { >>>>> + dev_err(dev, "can't disable %s. aborting >>>>> probe\n", >>>>> + child->full_name); >>>>> + break; >>>> >>>> >>>> The concept of disabling the device if timings cannot be met sounds >>>> interresting... >>>> Let's see what other maintainers say about this :). >>>> >>>> >>>>> + } >>>>> + } >>>>> + } >>>>> + >>>>> + ret = of_platform_populate(dev->of_node, >>>>> of_default_bus_match_table, >>>>> + NULL, dev); >>>>> + if (ret) >>>>> + dev_err(dev, "%s fail to create devices.\n", >>>>> + dev->of_node->full_name); >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> +static int smc_probe(struct platform_device *pdev) >>>>> +{ >>>>> + struct resource *res; >>>>> + int ret; >>>>> + void __iomem *base; >>>>> + struct clk *clk; >>>>> + struct smc_data *smc = devm_kzalloc(&pdev->dev, sizeof(struct >>>>> smc_data), >>>>> + GFP_KERNEL); >>>>> + >>>>> + if (!smc) >>>>> + return -ENOMEM; >>>>> + >>>>> + smc->dev = &pdev->dev; >>>>> + >>>>> + /* 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); >>>>> + } >>>>> + >>>>> + /* get the clock */ >>>>> + clk = devm_clk_get(&pdev->dev, "smc"); >>>>> + if (IS_ERR(clk)) >>>>> + return PTR_ERR(clk); >>>>> + >>>>> + smc->bus_clk = clk; >>>>> + smc->base = base; >>>>> + >>>>> + /* parse the device node */ >>>>> + ret = smc_parse_dt(smc); >>>>> + 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"); >>>> >>>> >>>> >>>> That's all for now. :) >>>> >>>> I'll try to test it this week end on a sama5 board. >>> >>> Thanks >>> >>>> Best Regards, >>>> >>>> Boris >> >> > >
On 13:31 Thu 09 Jan , Jean-Jacques Hiblot wrote: > 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. > It currently accepts timings in clock period and in nanoseconds. > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> > --- NACK on the binding will comment on the patch Best Regards, J. > drivers/memory/Kconfig | 10 ++ > drivers/memory/Makefile | 1 + > drivers/memory/atmel-smc.c | 431 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 442 insertions(+) > create mode 100644 drivers/memory/atmel-smc.c > > diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig > index 29a11db..fbdfd63 100644 > --- a/drivers/memory/Kconfig > +++ b/drivers/memory/Kconfig > @@ -50,4 +50,14 @@ config TEGRA30_MC > analysis, especially for IOMMU/SMMU(System Memory Management > Unit) module. > > +config ATMEL_SMC > + bool "Atmel SMC/EBI driver" > + default y > + 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, etc. > + > endif > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile > index 969d923..101abc4 100644 > --- a/drivers/memory/Makefile > +++ b/drivers/memory/Makefile > @@ -9,3 +9,4 @@ obj-$(CONFIG_TI_EMIF) += emif.o > obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o > obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o > obj-$(CONFIG_TEGRA30_MC) += tegra30-mc.o > +obj-$(CONFIG_ATMEL_SMC) += atmel-smc.o > diff --git a/drivers/memory/atmel-smc.c b/drivers/memory/atmel-smc.c > new file mode 100644 > index 0000000..0a1d9ba > --- /dev/null > +++ b/drivers/memory/atmel-smc.c > @@ -0,0 +1,431 @@ > +/* > + * 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 smc_data { > + struct clk *bus_clk; > + void __iomem *base; > + struct device *dev; > +}; > + > +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 width; > + u16 shift; > +}; > + > +static const struct smc_parameters_type smc_parameters[] = { > + {"smc,burst_size", 2, 28}, > + {"smc,burst_enabled", 1, 24}, > + {"smc,tdf_mode", 1, 20}, > + {"smc,bus_width", 2, 12}, > + {"smc,byte_access_type", 1, 8}, > + {"smc,nwait_mode", 2, 4}, > + {"smc,write_mode", 1, 0}, > + {"smc,read_mode", 1, 1}, > + {NULL} > +}; > + > +static int get_mode_register_from_dt(struct smc_data *smc, > + struct device_node *np, > + struct sam9_smc_config *cfg) > +{ > + int ret; > + u32 val; > + struct device *dev = smc->dev; > + const struct smc_parameters_type *p = smc_parameters; > + u32 mode = cfg->mode; > + > + while (p->name) { > + ret = of_property_read_u32(np, p->name , &val); > + if (ret == -EINVAL) { > + dev_dbg(dev, "%s: property %s not set.\n", np->name, > + p->name); > + p++; > + continue; > + } else if (ret) { > + dev_err(dev, "%s: can't get property %s.\n", np->name, > + p->name); > + return ret; > + } > + if (val >= (1<<p->width)) { > + dev_err(dev, "%s: property %s out of range.\n", > + np->name, p->name); > + return -ERANGE; > + } > + mode &= ~(((1<<p->width)-1) << p->shift); > + mode |= (val << p->shift); > + p++; > + } > + cfg->mode = mode; > + return 0; > +} > + > +static int generic_timing_from_dt(struct smc_data *smc, struct device_node *np, > + struct sam9_smc_config *cfg) > +{ > + u32 val; > + > + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &val)) > + cfg->ncs_read_setup = val; > + > + if (!of_property_read_u32(np, "smc,nrd_setup" , &val)) > + cfg->nrd_setup = val; > + > + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &val)) > + cfg->ncs_write_setup = val; > + > + if (!of_property_read_u32(np, "smc,nwe_setup" , &val)) > + cfg->nwe_setup = val; > + > + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &val)) > + cfg->ncs_read_pulse = val; > + > + if (!of_property_read_u32(np, "smc,nrd_pulse" , &val)) > + cfg->nrd_pulse = val; > + > + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &val)) > + cfg->ncs_write_pulse = val; > + > + if (!of_property_read_u32(np, "smc,nwe_pulse" , &val)) > + cfg->nwe_pulse = val; > + > + if (!of_property_read_u32(np, "smc,read_cycle" , &val)) > + cfg->read_cycle = val; > + > + if (!of_property_read_u32(np, "smc,write_cycle" , &val)) > + cfg->write_cycle = val; > + > + if (!of_property_read_u32(np, "smc,tdf_cycles" , &val)) > + cfg->tdf_cycles = val; > + > + return get_mode_register_from_dt(smc, np, cfg); > +} > + > +/* convert the time in ns in a number of clock cycles */ > +static u32 ns_to_cycles(u32 ns, u32 clk) > +{ > + /* > + * convert the clk to kHz for the rest of the calculation to avoid > + * overflow > + */ > + u32 clk_kHz = clk / 1000; > + u32 ncycles = ((ns * clk_kHz) + 1000000 - 1) / 1000000; > + return ncycles; > +} > + > +static u32 cycles_to_coded_cycle(u32 cycles, int a, int b) > +{ > + u32 mask_high = (1 << a) - 1; > + u32 mask_low = (1 << b) - 1; > + u32 coded; > + > + /* check if the value can be described with the coded format */ > + if (cycles & (mask_high & ~mask_low)) { > + /* not representable. we need to round up */ > + cycles |= mask_high; > + cycles += 1; > + } > + /* Now the value can be represented in the coded format */ > + coded = (cycles & ~mask_high) >> (a - b); > + coded |= (cycles & mask_low); > + return coded; > +} > + > +static u32 ns_to_rw_cycles(u32 ns, u32 clk) > +{ > + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 7); > +} > + > +static u32 ns_to_pulse_cycles(u32 ns, u32 clk) > +{ > + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 6); > +} > + > +static u32 ns_to_setup_cycles(u32 ns, u32 clk) > +{ > + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 7, 5); > +} > + > +static u32 cycles_to_ns(u32 cycles, u32 clk) > +{ > + /* > + * convert the clk to kHz for the rest of the calculation to avoid > + * overflow > + */ > + u32 clk_kHz = clk / 1000; > + return (cycles * 1000000) / clk_kHz; > +} > + > +static u32 coded_cycle_to_cycles(u32 coded, int a, int b) > +{ > + u32 cycles = (coded >> b) << a; > + u32 mask_low = (1 << b) - 1; > + cycles |= (coded & mask_low); > + return cycles; > +} > + > +static u32 rw_cycles_to_ns(u32 reg, u32 clk) > +{ > + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 7), clk); > +} > + > +static u32 pulse_cycles_to_ns(u32 reg, u32 clk) > +{ > + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 6), clk); > +} > + > +static u32 setup_cycles_to_ns(u32 reg, u32 clk) > +{ > + return cycles_to_ns(coded_cycle_to_cycles(reg, 7, 5), clk); > +} > + > +static void dump_timing(struct smc_data *smc, struct sam9_smc_config *config) > +{ > + u32 freq = clk_get_rate(smc->bus_clk); > + struct device *dev = smc->dev; > + > +#define DUMP(fn, y) dev_info(dev, "%s : 0x%x (%d ns)\n", #y, config->y,\ > + fn(config->y, freq)) > +#define DUMP_PULSE(y) DUMP(pulse_cycles_to_ns, y) > +#define DUMP_RWCYCLE(y) DUMP(rw_cycles_to_ns, y) > +#define DUMP_SETUP(y) DUMP(setup_cycles_to_ns, y) > +#define DUMP_SIMPLE(y) DUMP(cycles_to_ns, y) > + > + DUMP_SETUP(nwe_setup); > + DUMP_SETUP(ncs_write_setup); > + DUMP_SETUP(nrd_setup); > + DUMP_SETUP(ncs_read_setup); > + DUMP_PULSE(nwe_pulse); > + DUMP_PULSE(ncs_write_pulse); > + DUMP_PULSE(nrd_pulse); > + DUMP_PULSE(ncs_read_pulse); > + DUMP_RWCYCLE(write_cycle); > + DUMP_RWCYCLE(read_cycle); > + DUMP_SIMPLE(tdf_cycles); > +} > + > +static int ns_timing_from_dt(struct smc_data *smc, struct device_node *np, > + struct sam9_smc_config *cfg) > +{ > + u32 t_ns; > + u32 freq = clk_get_rate(smc->bus_clk); > + > + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &t_ns)) > + cfg->ncs_read_setup = ns_to_setup_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,nrd_setup" , &t_ns)) > + cfg->nrd_setup = ns_to_setup_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &t_ns)) > + cfg->ncs_write_setup = ns_to_setup_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,nwe_setup" , &t_ns)) > + cfg->nwe_setup = ns_to_setup_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &t_ns)) > + cfg->ncs_read_pulse = ns_to_pulse_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,nrd_pulse" , &t_ns)) > + cfg->nrd_pulse = ns_to_pulse_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &t_ns)) > + cfg->ncs_write_pulse = ns_to_pulse_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,nwe_pulse" , &t_ns)) > + cfg->nwe_pulse = ns_to_pulse_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,read_cycle" , &t_ns)) > + cfg->read_cycle = ns_to_rw_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,write_cycle" , &t_ns)) > + cfg->write_cycle = ns_to_rw_cycles(t_ns, freq); > + > + if (!of_property_read_u32(np, "smc,tdf_cycles" , &t_ns)) > + cfg->tdf_cycles = ns_to_cycles(t_ns, freq); > + > + return get_mode_register_from_dt(smc, np, cfg); > +} > + > +struct converter { > + const char *name; > + int (*fn) (struct smc_data *smc, struct device_node *np, > + struct sam9_smc_config *cfg); > +}; > +static const struct converter converters[] = { > + {"raw", generic_timing_from_dt}, > + {"nanosec", ns_timing_from_dt}, > +}; > + > +/* Parse and set the timing for this device. */ > +static int smc_timing_setup(struct smc_data *smc, struct device_node *np, > + const struct at91_smc_devtype *devtype) > +{ > + int ret; > + u32 cs; > + int i; > + struct device *dev = smc->dev; > + const struct converter *converter; > + const char *converter_name = NULL; > + struct sam9_smc_config cfg; > + > + ret = of_property_read_u32(np, "smc,cs" , &cs); > + if (ret < 0) { > + dev_err(dev, "missing mandatory property : smc,cs\n"); > + return ret; > + } > + if (cs >= 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; > + } > + > + of_property_read_string(np, "smc,converter", &converter_name); > + if (converter_name) { > + for (i = 0; i < ARRAY_SIZE(converters); i++) > + if (strcmp(converters[i].name, converter_name) == 0) > + converter = &converters[i]; > + if (!converter) { > + dev_info(dev, "unknown converter. aborting\n"); > + return -EINVAL; > + } > + } else { > + dev_dbg(dev, "cs %d: no smc converter provided. using " > + "raw register values\n", cs); > + converter = &converters[0]; > + } > + dev_dbg(dev, "cs %d using converter : %s\n", cs, converter->name); > + sam9_smc_cs_read(smc->base + (0x10 * cs), &cfg); > + converter->fn(smc, np, &cfg); > + ret = sam9_smc_check_cs_configuration(&cfg); > + if (ret < 0) { > + dev_info(dev, "invalid smc configuration for cs %d." > + "clipping values\n", cs); > + sam9_smc_clip_cs_configuration(&cfg); > + dump_timing(smc, &cfg); > + } > +#ifdef DEBUG > + else > + dump_timing(smc, &cfg); > +#endif > + > + sam9_smc_cs_configure(smc->base + (0x10 * cs), &cfg); > + return 0; > +} > + > +static int smc_parse_dt(struct smc_data *smc) > +{ > + struct device *dev = smc->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; > + if (!of_device_is_available(child)) > + continue; > + ret = smc_timing_setup(smc, child, devtype); > + if (ret) { > + static struct property status = { > + .name = "status", > + .value = "disabled", > + .length = sizeof("disabled"), > + }; > + dev_err(dev, "%s set timing failed. This node will be disabled.\n", > + child->full_name); > + ret = of_update_property(child, &status); > + if (ret < 0) { > + dev_err(dev, "can't disable %s. aborting probe\n", > + child->full_name); > + break; > + } > + } > + } > + > + ret = of_platform_populate(dev->of_node, of_default_bus_match_table, > + NULL, dev); > + if (ret) > + dev_err(dev, "%s fail to create devices.\n", > + dev->of_node->full_name); > + > + return ret; > +} > + > +static int smc_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + int ret; > + void __iomem *base; > + struct clk *clk; > + struct smc_data *smc = devm_kzalloc(&pdev->dev, sizeof(struct smc_data), > + GFP_KERNEL); > + > + if (!smc) > + return -ENOMEM; > + > + smc->dev = &pdev->dev; > + > + /* 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); > + } > + > + /* get the clock */ > + clk = devm_clk_get(&pdev->dev, "smc"); > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > + > + smc->bus_clk = clk; > + smc->base = base; > + > + /* parse the device node */ > + ret = smc_parse_dt(smc); > + 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"); > -- > 1.8.5.2 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig index 29a11db..fbdfd63 100644 --- a/drivers/memory/Kconfig +++ b/drivers/memory/Kconfig @@ -50,4 +50,14 @@ config TEGRA30_MC analysis, especially for IOMMU/SMMU(System Memory Management Unit) module. +config ATMEL_SMC + bool "Atmel SMC/EBI driver" + default y + 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, etc. + endif diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile index 969d923..101abc4 100644 --- a/drivers/memory/Makefile +++ b/drivers/memory/Makefile @@ -9,3 +9,4 @@ obj-$(CONFIG_TI_EMIF) += emif.o obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o obj-$(CONFIG_TEGRA30_MC) += tegra30-mc.o +obj-$(CONFIG_ATMEL_SMC) += atmel-smc.o diff --git a/drivers/memory/atmel-smc.c b/drivers/memory/atmel-smc.c new file mode 100644 index 0000000..0a1d9ba --- /dev/null +++ b/drivers/memory/atmel-smc.c @@ -0,0 +1,431 @@ +/* + * 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 smc_data { + struct clk *bus_clk; + void __iomem *base; + struct device *dev; +}; + +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 width; + u16 shift; +}; + +static const struct smc_parameters_type smc_parameters[] = { + {"smc,burst_size", 2, 28}, + {"smc,burst_enabled", 1, 24}, + {"smc,tdf_mode", 1, 20}, + {"smc,bus_width", 2, 12}, + {"smc,byte_access_type", 1, 8}, + {"smc,nwait_mode", 2, 4}, + {"smc,write_mode", 1, 0}, + {"smc,read_mode", 1, 1}, + {NULL} +}; + +static int get_mode_register_from_dt(struct smc_data *smc, + struct device_node *np, + struct sam9_smc_config *cfg) +{ + int ret; + u32 val; + struct device *dev = smc->dev; + const struct smc_parameters_type *p = smc_parameters; + u32 mode = cfg->mode; + + while (p->name) { + ret = of_property_read_u32(np, p->name , &val); + if (ret == -EINVAL) { + dev_dbg(dev, "%s: property %s not set.\n", np->name, + p->name); + p++; + continue; + } else if (ret) { + dev_err(dev, "%s: can't get property %s.\n", np->name, + p->name); + return ret; + } + if (val >= (1<<p->width)) { + dev_err(dev, "%s: property %s out of range.\n", + np->name, p->name); + return -ERANGE; + } + mode &= ~(((1<<p->width)-1) << p->shift); + mode |= (val << p->shift); + p++; + } + cfg->mode = mode; + return 0; +} + +static int generic_timing_from_dt(struct smc_data *smc, struct device_node *np, + struct sam9_smc_config *cfg) +{ + u32 val; + + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &val)) + cfg->ncs_read_setup = val; + + if (!of_property_read_u32(np, "smc,nrd_setup" , &val)) + cfg->nrd_setup = val; + + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &val)) + cfg->ncs_write_setup = val; + + if (!of_property_read_u32(np, "smc,nwe_setup" , &val)) + cfg->nwe_setup = val; + + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &val)) + cfg->ncs_read_pulse = val; + + if (!of_property_read_u32(np, "smc,nrd_pulse" , &val)) + cfg->nrd_pulse = val; + + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &val)) + cfg->ncs_write_pulse = val; + + if (!of_property_read_u32(np, "smc,nwe_pulse" , &val)) + cfg->nwe_pulse = val; + + if (!of_property_read_u32(np, "smc,read_cycle" , &val)) + cfg->read_cycle = val; + + if (!of_property_read_u32(np, "smc,write_cycle" , &val)) + cfg->write_cycle = val; + + if (!of_property_read_u32(np, "smc,tdf_cycles" , &val)) + cfg->tdf_cycles = val; + + return get_mode_register_from_dt(smc, np, cfg); +} + +/* convert the time in ns in a number of clock cycles */ +static u32 ns_to_cycles(u32 ns, u32 clk) +{ + /* + * convert the clk to kHz for the rest of the calculation to avoid + * overflow + */ + u32 clk_kHz = clk / 1000; + u32 ncycles = ((ns * clk_kHz) + 1000000 - 1) / 1000000; + return ncycles; +} + +static u32 cycles_to_coded_cycle(u32 cycles, int a, int b) +{ + u32 mask_high = (1 << a) - 1; + u32 mask_low = (1 << b) - 1; + u32 coded; + + /* check if the value can be described with the coded format */ + if (cycles & (mask_high & ~mask_low)) { + /* not representable. we need to round up */ + cycles |= mask_high; + cycles += 1; + } + /* Now the value can be represented in the coded format */ + coded = (cycles & ~mask_high) >> (a - b); + coded |= (cycles & mask_low); + return coded; +} + +static u32 ns_to_rw_cycles(u32 ns, u32 clk) +{ + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 7); +} + +static u32 ns_to_pulse_cycles(u32 ns, u32 clk) +{ + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 8, 6); +} + +static u32 ns_to_setup_cycles(u32 ns, u32 clk) +{ + return cycles_to_coded_cycle(ns_to_cycles(ns, clk), 7, 5); +} + +static u32 cycles_to_ns(u32 cycles, u32 clk) +{ + /* + * convert the clk to kHz for the rest of the calculation to avoid + * overflow + */ + u32 clk_kHz = clk / 1000; + return (cycles * 1000000) / clk_kHz; +} + +static u32 coded_cycle_to_cycles(u32 coded, int a, int b) +{ + u32 cycles = (coded >> b) << a; + u32 mask_low = (1 << b) - 1; + cycles |= (coded & mask_low); + return cycles; +} + +static u32 rw_cycles_to_ns(u32 reg, u32 clk) +{ + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 7), clk); +} + +static u32 pulse_cycles_to_ns(u32 reg, u32 clk) +{ + return cycles_to_ns(coded_cycle_to_cycles(reg, 8, 6), clk); +} + +static u32 setup_cycles_to_ns(u32 reg, u32 clk) +{ + return cycles_to_ns(coded_cycle_to_cycles(reg, 7, 5), clk); +} + +static void dump_timing(struct smc_data *smc, struct sam9_smc_config *config) +{ + u32 freq = clk_get_rate(smc->bus_clk); + struct device *dev = smc->dev; + +#define DUMP(fn, y) dev_info(dev, "%s : 0x%x (%d ns)\n", #y, config->y,\ + fn(config->y, freq)) +#define DUMP_PULSE(y) DUMP(pulse_cycles_to_ns, y) +#define DUMP_RWCYCLE(y) DUMP(rw_cycles_to_ns, y) +#define DUMP_SETUP(y) DUMP(setup_cycles_to_ns, y) +#define DUMP_SIMPLE(y) DUMP(cycles_to_ns, y) + + DUMP_SETUP(nwe_setup); + DUMP_SETUP(ncs_write_setup); + DUMP_SETUP(nrd_setup); + DUMP_SETUP(ncs_read_setup); + DUMP_PULSE(nwe_pulse); + DUMP_PULSE(ncs_write_pulse); + DUMP_PULSE(nrd_pulse); + DUMP_PULSE(ncs_read_pulse); + DUMP_RWCYCLE(write_cycle); + DUMP_RWCYCLE(read_cycle); + DUMP_SIMPLE(tdf_cycles); +} + +static int ns_timing_from_dt(struct smc_data *smc, struct device_node *np, + struct sam9_smc_config *cfg) +{ + u32 t_ns; + u32 freq = clk_get_rate(smc->bus_clk); + + if (!of_property_read_u32(np, "smc,ncs_read_setup" , &t_ns)) + cfg->ncs_read_setup = ns_to_setup_cycles(t_ns, freq); + + if (!of_property_read_u32(np, "smc,nrd_setup" , &t_ns)) + cfg->nrd_setup = ns_to_setup_cycles(t_ns, freq); + + if (!of_property_read_u32(np, "smc,ncs_write_setup" , &t_ns)) + cfg->ncs_write_setup = ns_to_setup_cycles(t_ns, freq); + + if (!of_property_read_u32(np, "smc,nwe_setup" , &t_ns)) + cfg->nwe_setup = ns_to_setup_cycles(t_ns, freq); + + if (!of_property_read_u32(np, "smc,ncs_read_pulse" , &t_ns)) + cfg->ncs_read_pulse = ns_to_pulse_cycles(t_ns, freq); + + if (!of_property_read_u32(np, "smc,nrd_pulse" , &t_ns)) + cfg->nrd_pulse = ns_to_pulse_cycles(t_ns, freq); + + if (!of_property_read_u32(np, "smc,ncs_write_pulse" , &t_ns)) + cfg->ncs_write_pulse = ns_to_pulse_cycles(t_ns, freq); + + if (!of_property_read_u32(np, "smc,nwe_pulse" , &t_ns)) + cfg->nwe_pulse = ns_to_pulse_cycles(t_ns, freq); + + if (!of_property_read_u32(np, "smc,read_cycle" , &t_ns)) + cfg->read_cycle = ns_to_rw_cycles(t_ns, freq); + + if (!of_property_read_u32(np, "smc,write_cycle" , &t_ns)) + cfg->write_cycle = ns_to_rw_cycles(t_ns, freq); + + if (!of_property_read_u32(np, "smc,tdf_cycles" , &t_ns)) + cfg->tdf_cycles = ns_to_cycles(t_ns, freq); + + return get_mode_register_from_dt(smc, np, cfg); +} + +struct converter { + const char *name; + int (*fn) (struct smc_data *smc, struct device_node *np, + struct sam9_smc_config *cfg); +}; +static const struct converter converters[] = { + {"raw", generic_timing_from_dt}, + {"nanosec", ns_timing_from_dt}, +}; + +/* Parse and set the timing for this device. */ +static int smc_timing_setup(struct smc_data *smc, struct device_node *np, + const struct at91_smc_devtype *devtype) +{ + int ret; + u32 cs; + int i; + struct device *dev = smc->dev; + const struct converter *converter; + const char *converter_name = NULL; + struct sam9_smc_config cfg; + + ret = of_property_read_u32(np, "smc,cs" , &cs); + if (ret < 0) { + dev_err(dev, "missing mandatory property : smc,cs\n"); + return ret; + } + if (cs >= 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; + } + + of_property_read_string(np, "smc,converter", &converter_name); + if (converter_name) { + for (i = 0; i < ARRAY_SIZE(converters); i++) + if (strcmp(converters[i].name, converter_name) == 0) + converter = &converters[i]; + if (!converter) { + dev_info(dev, "unknown converter. aborting\n"); + return -EINVAL; + } + } else { + dev_dbg(dev, "cs %d: no smc converter provided. using " + "raw register values\n", cs); + converter = &converters[0]; + } + dev_dbg(dev, "cs %d using converter : %s\n", cs, converter->name); + sam9_smc_cs_read(smc->base + (0x10 * cs), &cfg); + converter->fn(smc, np, &cfg); + ret = sam9_smc_check_cs_configuration(&cfg); + if (ret < 0) { + dev_info(dev, "invalid smc configuration for cs %d." + "clipping values\n", cs); + sam9_smc_clip_cs_configuration(&cfg); + dump_timing(smc, &cfg); + } +#ifdef DEBUG + else + dump_timing(smc, &cfg); +#endif + + sam9_smc_cs_configure(smc->base + (0x10 * cs), &cfg); + return 0; +} + +static int smc_parse_dt(struct smc_data *smc) +{ + struct device *dev = smc->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; + if (!of_device_is_available(child)) + continue; + ret = smc_timing_setup(smc, child, devtype); + if (ret) { + static struct property status = { + .name = "status", + .value = "disabled", + .length = sizeof("disabled"), + }; + dev_err(dev, "%s set timing failed. This node will be disabled.\n", + child->full_name); + ret = of_update_property(child, &status); + if (ret < 0) { + dev_err(dev, "can't disable %s. aborting probe\n", + child->full_name); + break; + } + } + } + + ret = of_platform_populate(dev->of_node, of_default_bus_match_table, + NULL, dev); + if (ret) + dev_err(dev, "%s fail to create devices.\n", + dev->of_node->full_name); + + return ret; +} + +static int smc_probe(struct platform_device *pdev) +{ + struct resource *res; + int ret; + void __iomem *base; + struct clk *clk; + struct smc_data *smc = devm_kzalloc(&pdev->dev, sizeof(struct smc_data), + GFP_KERNEL); + + if (!smc) + return -ENOMEM; + + smc->dev = &pdev->dev; + + /* 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); + } + + /* get the clock */ + clk = devm_clk_get(&pdev->dev, "smc"); + if (IS_ERR(clk)) + return PTR_ERR(clk); + + smc->bus_clk = clk; + smc->base = base; + + /* parse the device node */ + ret = smc_parse_dt(smc); + 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");
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. It currently accepts timings in clock period and in nanoseconds. Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> --- drivers/memory/Kconfig | 10 ++ drivers/memory/Makefile | 1 + drivers/memory/atmel-smc.c | 431 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 442 insertions(+) create mode 100644 drivers/memory/atmel-smc.c