Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757907AbaAIVls (ORCPT ); Thu, 9 Jan 2014 16:41:48 -0500 Received: from 3.mo2.mail-out.ovh.net ([46.105.58.226]:43529 "EHLO mo2.mail-out.ovh.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1757570AbaAIVln (ORCPT ); Thu, 9 Jan 2014 16:41:43 -0500 X-Greylist: delayed 1199 seconds by postgrey-1.27 at vger.kernel.org; Thu, 09 Jan 2014 16:41:42 EST MIME-Version: 1.0 In-Reply-To: <52CED586.7030602@overkiz.com> References: <1389270709-32662-1-git-send-email-jjhiblot@traphandler.com> <1389270709-32662-8-git-send-email-jjhiblot@traphandler.com> <52CED586.7030602@overkiz.com> Date: Thu, 9 Jan 2014 22:04:00 +0100 Message-ID: Subject: Re: [PATCH v2 07/12] at91: dt: smc: Added smc bus driver From: Jean-Jacques Hiblot To: boris brezillon Cc: Jean-Jacques Hiblot , nicolas.ferre@atmel.com, Arnd Bergmann , linux-arm-kernel@lists.infradead.org, Linux Kernel Mailing List Content-Type: text/plain; charset=ISO-8859-1 X-Ovh-Tracer-Id: 1063130987161212952 X-Ovh-Remote: 209.85.160.45 (mail-pb0-f45.google.com) X-Ovh-Local: 213.186.33.20 (ns0.ovh.net) X-OVH-SPAMSTATE: OK X-OVH-SPAMSCORE: -100 X-OVH-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrfeejtddrfeduucetufdoteggodetrfcurfhrohhfihhlvgemucfqggfjnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd X-Spam-Check: DONE|U 0.5/N X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: -100 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrfeejtddrfeduucetufdoteggodetrfcurfhrohhfihhlvgemucfqggfjnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Boris, 2014/1/9 boris brezillon : > 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 >> --- >> 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 >> +#include >> +#include >> +#include >> +#include > > > 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<width)) { >> + dev_err(dev, "%s: property %s out of range.\n", >> + np->name, p->name); >> + return -ERANGE; >> + } >> + mode &= ~(((1<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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/