All of lore.kernel.org
 help / color / mirror / Atom feed
From: b.brezillon@overkiz.com (boris brezillon)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 07/12] at91: dt: smc: Added smc bus driver
Date: Sat, 11 Jan 2014 09:06:16 +0100	[thread overview]
Message-ID: <52D0FB78.6010102@overkiz.com> (raw)
In-Reply-To: <CACh+v5MvJptBsEcdmcDRTBZ1rKbt-iWKEQUpiME382B0qePC3A@mail.gmail.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 ;)).

> 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

WARNING: multiple messages have this Message-ID (diff)
From: boris brezillon <b.brezillon@overkiz.com>
To: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
Cc: nicolas.ferre@atmel.com, Arnd Bergmann <arnd@arndb.de>,
	linux-arm-kernel@lists.infradead.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 07/12] at91: dt: smc: Added smc bus driver
Date: Sat, 11 Jan 2014 09:06:16 +0100	[thread overview]
Message-ID: <52D0FB78.6010102@overkiz.com> (raw)
In-Reply-To: <CACh+v5MvJptBsEcdmcDRTBZ1rKbt-iWKEQUpiME382B0qePC3A@mail.gmail.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 ;)).

> 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


  parent reply	other threads:[~2014-01-11  8:06 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-01-09 12:31 [PATCH v2 00/12] Device Tree support for the at91sam9261ek Jean-Jacques Hiblot
2014-01-09 12:31 ` Jean-Jacques Hiblot
2014-01-09 12:31 ` [PATCH v2 01/12] at91: dt: Add at91sam9261 dt SoC support Jean-Jacques Hiblot
2014-01-09 12:31   ` Jean-Jacques Hiblot
2014-01-14 17:01   ` Nicolas Ferre
2014-01-14 17:01     ` Nicolas Ferre
2014-01-15 10:08     ` Jean-Jacques Hiblot
2014-01-15 10:08       ` Jean-Jacques Hiblot
2014-01-15 10:14       ` Nicolas Ferre
2014-01-15 10:14         ` Nicolas Ferre
2014-01-15 11:31     ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-15 11:31       ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-09 12:31 ` [PATCH v2 02/12] at91: dt: sam9261: Basic Device Tree support for the at91sam9261ek Jean-Jacques Hiblot
2014-01-09 12:31   ` Jean-Jacques Hiblot
2014-01-14 17:06   ` Nicolas Ferre
2014-01-14 17:06     ` Nicolas Ferre
2014-01-09 12:31 ` [PATCH v2 03/12] at91: dt: sam9261: Added support for the lcd display Jean-Jacques Hiblot
2014-01-09 12:31   ` Jean-Jacques Hiblot
2014-01-09 17:07   ` boris brezillon
2014-01-09 17:07     ` boris brezillon
2014-01-15 11:27     ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-15 11:27       ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-14 17:09   ` Nicolas Ferre
2014-01-14 17:09     ` Nicolas Ferre
2014-01-09 12:31 ` [PATCH v2 04/12] at91: smc: export sam9_smc_cs_read and sam9_smc_cs_configure Jean-Jacques Hiblot
2014-01-09 12:31   ` Jean-Jacques Hiblot
2014-01-09 12:31 ` [PATCH v2 05/12] at91: smc: Increased the size of tdf_cycles in struct sam9_smc_config Jean-Jacques Hiblot
2014-01-09 12:31   ` Jean-Jacques Hiblot
2014-01-09 12:31 ` [PATCH v2 06/12] at91: smc: Adds helper functions to validate and clip the smc timings Jean-Jacques Hiblot
2014-01-09 12:31   ` Jean-Jacques Hiblot
2014-01-15  9:44   ` Nicolas Ferre
2014-01-15  9:44     ` Nicolas Ferre
2014-01-15  9:54     ` Jean-Jacques Hiblot
2014-01-15  9:54       ` Jean-Jacques Hiblot
2014-01-15 10:18       ` Nicolas Ferre
2014-01-15 10:18         ` Nicolas Ferre
2014-01-09 12:31 ` [PATCH v2 07/12] at91: dt: smc: Added smc bus driver Jean-Jacques Hiblot
2014-01-09 12:31   ` Jean-Jacques Hiblot
2014-01-09 16:59   ` boris brezillon
2014-01-09 16:59     ` boris brezillon
2014-01-09 21:04     ` Jean-Jacques Hiblot
2014-01-09 21:04       ` Jean-Jacques Hiblot
2014-01-10 11:01       ` Jean-Jacques Hiblot
2014-01-10 11:01         ` Jean-Jacques Hiblot
2014-01-10 11:08         ` Jean-Jacques Hiblot
2014-01-10 11:08           ` Jean-Jacques Hiblot
2014-01-11  8:06       ` boris brezillon [this message]
2014-01-11  8:06         ` boris brezillon
2014-01-14 14:20         ` Jean-Jacques Hiblot
2014-01-14 14:20           ` Jean-Jacques Hiblot
2014-01-14 15:01           ` Nicolas Ferre
2014-01-14 15:01             ` Nicolas Ferre
2014-02-07  8:42   ` Jean-Christophe PLAGNIOL-VILLARD
2014-02-07  8:42     ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-09 12:31 ` [PATCH v2 08/12] at91: sam9261: Add a clock definition for the smc Jean-Jacques Hiblot
2014-01-09 12:31   ` Jean-Jacques Hiblot
2014-01-09 12:31 ` [PATCH v2 09/12] at91: dt: sam9261: Pinmux DT entries for the SMC/EBI interface Jean-Jacques Hiblot
2014-01-09 12:31   ` Jean-Jacques Hiblot
2014-01-15 11:25   ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-15 11:25     ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-09 12:31 ` [PATCH v2 10/12] at91: dt: sam9261: Add an entry in the DT for the SMC/EBI bus driver Jean-Jacques Hiblot
2014-01-09 12:31   ` Jean-Jacques Hiblot
2014-01-09 12:31 ` [PATCH v2 11/12] at91: dt: sam9261: moved the NAND under the smc node Jean-Jacques Hiblot
2014-01-09 12:31   ` Jean-Jacques Hiblot
2014-01-09 12:31 ` [PATCH v2 12/12] at91: dt: sam9261: Added DM9000 in the device tree Jean-Jacques Hiblot
2014-01-09 12:31   ` Jean-Jacques Hiblot
2014-01-15 11:23   ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-15 11:23     ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-14 16:54 ` [PATCH v2 00/12] Device Tree support for the at91sam9261ek Nicolas Ferre
2014-01-14 16:54   ` Nicolas Ferre
2014-01-15  8:34   ` Jean-Jacques Hiblot
2014-01-15  8:34     ` Jean-Jacques Hiblot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=52D0FB78.6010102@overkiz.com \
    --to=b.brezillon@overkiz.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.