Message-ID: <52C68EA6.7040207@overkiz.com>
Date: Fri, 03 Jan 2014 11:19:18 +0100
From: boris brezillon <b.brezillon@overkiz.com>
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Thunderbird/24.2.0
MIME-Version: 1.0
To: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
CC: nicolas.ferre@atmel.com,
        jean-jacques hiblot <jean-jacques.hiblot@jdsu.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 3/6] At91: dt: Added smc bus driver
References: <1388507534-10570-1-git-send-email-jjhiblot@traphandler.com>	<1388507534-10570-4-git-send-email-jjhiblot@traphandler.com>	<52C60352.7060006@overkiz.com> <CACh+v5PEWDUU0QhncDKzE5EzU=P18ikHHF+PeSkdFNaLTPNUZQ@mail.gmail.com>
In-Reply-To: <CACh+v5PEWDUU0QhncDKzE5EzU=P18ikHHF+PeSkdFNaLTPNUZQ@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 12759
Lines: 344

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

NOR flashes seems to have a standard naming convention too...

Anyway, I think you should first implement the mechanism to support several
converters and the generic converter.
This way we can have a working version of the SMC driver and we'll be 
able to
add new converters later.

>> Best Regards,
>>
>> Boris
>>
>>
>>> +
>>> +static int __init smc_parse_dt(struct platform_device *pdev,
>>> +                               void __iomem *base)
>>> +{
>>> +       struct device *dev = &pdev->dev;
>>> +       const struct of_device_id *of_id = of_match_device(smc_id_table,
>>> dev);
>>> +       const struct at91_smc_devtype *devtype = of_id->data;
>>> +       struct device_node *child;
>>> +       int ret;
>>> +
>>> +       for_each_child_of_node(dev->of_node, child) {
>>> +               if (!child->name)
>>> +                       continue;
>>> +
>>> +               ret = smc_timing_setup(dev, child, base, devtype);
>>> +               if (ret) {
>>> +                       dev_err(dev, "%s set timing failed.\n",
>>> +                               child->full_name);
>>> +                       return ret;
>>> +               }
>>> +       }
>>> +
>>> +       ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
>>> +       if (ret)
>>> +               dev_err(dev, "%s fail to create devices.\n",
>>> +                       dev->of_node->full_name);
>>> +       return ret;
>>> +}
>>> +
>>> +static int __init smc_probe(struct platform_device *pdev)
>>> +{
>>> +       struct resource *res;
>>> +       void __iomem *base;
>>> +       int ret;
>>> +
>>> +       /* get the resource */
>>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +       base = devm_request_and_ioremap(&pdev->dev, res);
>>> +       if (IS_ERR(base)) {
>>> +               dev_err(&pdev->dev, "can't map SMC base address\n");
>>> +               return PTR_ERR(base);
>>> +       }
>>> +
>>> +       /* parse the device node */
>>> +       ret = smc_parse_dt(pdev, base);
>>> +       if (!ret)
>>> +               dev_info(&pdev->dev, "Driver registered.\n");
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +static struct platform_driver smc_driver = {
>>> +       .driver = {
>>> +               .name           = "atmel-smc",
>>> +               .owner          = THIS_MODULE,
>>> +               .of_match_table = smc_id_table,
>>> +       },
>>> +};
>>> +module_platform_driver_probe(smc_driver, smc_probe);
>>> +
>>> +MODULE_AUTHOR("JJ Hiblot");
>>> +MODULE_DESCRIPTION("Atmel's SMC/EBI driver");
>>> +MODULE_LICENSE("GPL");
>>>

--
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/