Re: [RFC v2 PATCH 1/2] memory: davinci - add aemif controller platform driver
Stephen, Thanks for reviewing this. See my responses below On 11/07/2012 03:05 PM, Stephen Warren wrote: On 11/06/2012 02:47 PM, Murali Karicheri wrote: This is a platform driver for asynchronous external memory interface available on TI SoCs. This driver was previously located inside the mach-davinci folder. As this DaVinci IP is re-used across multiple family of devices such as c6x, keystone etc, the driver is moved to drivers. The driver configures async bus parameters associated with a particular chip select. For DaVinci NAND controller driver and driver for other async devices such as NOR flash, ASRAM etc, the bus confuguration is done by this driver at probe. A set of APIs (set/get) are provided to update the values based on specific driver usage. diff --git a/Documentation/devicetree/bindings/memory/davinci-aemif.txt b/Documentation/devicetree/bindings/memory/davinci-aemif.txt +* Texas Instruments Davinci AEMIF bus interface + +This file provides information for the davinci-emif device and +async bus bindings. + +Required properties:= +- compatible: "ti,davinci-aemif"; +- #address-cells : Should be either two or three. The first cell is the + chipselect number, and the remaining cells are the + offset into the chipselect. +- #size-cells : Either one or two, depending on how large each chipselect +can be. What about "how large each chipselect can be" determines 2-vs-3 (address) or 1-vs-2 (size) cells? I assume it's 32-vs-64-bit bus? It might be best to make that explicit. I have re-used the bindings from another patch and don't know why the above description is used. I am using a value of 2 for address cells and 1 for size-cells which I believe is how these will be used as shown below. ranges = <2 0 0x7000 0x0800 3 0 0x7400 0x0400 4 0 0x7800 0x0400 5 0 0x7C00 0x0400 6 0 0x20c0 0x100>; So I will change the description as below. - compatible: must be "ti,davinci-aemif" - reg: AEMIF control registers base and size - #address-cells: must be 2 (chip-select + offset) - #size-cells: must be 1 - ranges: mapping from EMIF space to parent space. Each range corresponds to a single chip select, and covers the entire access window as configured. +- reg : contains offset/length value for AEMIF control registers space +- ranges : Each range corresponds to a single chipselect, and cover add "s" at the end of that line. Ok + the entire access window as configured. + +Child device nodes describe the devices connected to IFC such as NOR (e.g. +cfi-flash) and NAND (ti,davinci-nand, see Documentation/devicetree/bindings/ +mtd/davinci-nand.txt). There might be board specific devices like FPGAs. + +In addition, optional child sub nodes contains bindings for the async bus +interface for a given chip select. Hmmm. That's two different kinds of children, which appear to use different addressing schemes given the examples below; more on that below. +Optional cs node properties:- +- compatible: "ti,davinci-cs" + + All of the params below in nanoseconds and are optional + +- ti,davinci-cs-asize - asynchronous data bus width (0 - 8bit, 1 - 16 bit) Perhaps s/asize/bus-width/? Also, directly using values 8 and 16 would be a lot more obvious to read than 0 and 1. Ok. Will fix, +- ti,davinci-cs-ta - Minimum turn around time +- ti,davinci-cs-rhold - read hold width +- ti,davinci-cs-rstobe - read strobe width +- ti,davinci-cs-rsetup - read setup width +- ti,davinci-cs-whold - write hold width +- ti,davinci-cs-wstrobe - write strobe width +- ti,davinci-cs-wsetup - write setup width The comments in the examples indicate these are in nS. This should be mentioned here instead I think. Ok. Does there need to be a specification of bus clock rate? How does the driver convert nS into number-of-clock-cycles, which I assume is what the HW would be programmed with? aemif driver clk is either specified as a clk device node or as a device bindings. Aemif driver gets the clk rate and do the conversion of ns to emif clk cycles internally in the driver. +- ti,davinci-cs-ss - enable/disable select strobe (0 - disable, 1 - enable) +- ti,davinci-cs-ew - enable/disable extended wait cycles (0 - disable, 1 - enable) Boolean properties are usually represented as present (on/enabled/1) or missing (off/disabled/0). Shouldn't these two properties work that way? I see the comment below: +if any of the above parameters are absent, hardware register default or that +set by a boot loader are used. implied they're really tri-state (explicitly off or on, or just not programmed). However, I think it'd be better if the DT always represented the complete configuration, since the DT and binding should be a full description of the HW rather than just the data you expect a particular Linu
Re: [RFC v2 PATCH 1/2] memory: davinci - add aemif controller platform driver
On 11/06/2012 02:47 PM, Murali Karicheri wrote: > This is a platform driver for asynchronous external memory interface > available on TI SoCs. This driver was previously located inside the > mach-davinci folder. As this DaVinci IP is re-used across multiple > family of devices such as c6x, keystone etc, the driver is moved to drivers. > The driver configures async bus parameters associated with a particular > chip select. For DaVinci NAND controller driver and driver for other async > devices such as NOR flash, ASRAM etc, the bus confuguration is > done by this driver at probe. A set of APIs (set/get) are provided to > update the values based on specific driver usage. > diff --git a/Documentation/devicetree/bindings/memory/davinci-aemif.txt > b/Documentation/devicetree/bindings/memory/davinci-aemif.txt > +* Texas Instruments Davinci AEMIF bus interface > + > +This file provides information for the davinci-emif device and > +async bus bindings. > + > +Required properties:= > +- compatible: "ti,davinci-aemif"; > +- #address-cells : Should be either two or three. The first cell is the > + chipselect number, and the remaining cells are the > + offset into the chipselect. > +- #size-cells : Either one or two, depending on how large each chipselect > +can be. What about "how large each chipselect can be" determines 2-vs-3 (address) or 1-vs-2 (size) cells? I assume it's 32-vs-64-bit bus? It might be best to make that explicit. > +- reg : contains offset/length value for AEMIF control registers space > +- ranges : Each range corresponds to a single chipselect, and cover add "s" at the end of that line. > + the entire access window as configured. > + > +Child device nodes describe the devices connected to IFC such as NOR (e.g. > +cfi-flash) and NAND (ti,davinci-nand, see Documentation/devicetree/bindings/ > +mtd/davinci-nand.txt). There might be board specific devices like FPGAs. > + > +In addition, optional child sub nodes contains bindings for the async bus > +interface for a given chip select. Hmmm. That's two different kinds of children, which appear to use different addressing schemes given the examples below; more on that below. > +Optional cs node properties:- > +- compatible: "ti,davinci-cs" > + > + All of the params below in nanoseconds and are optional > + > +- ti,davinci-cs-asize - asynchronous data bus width (0 - 8bit, 1 - 16 bit) Perhaps s/asize/bus-width/? Also, directly using values 8 and 16 would be a lot more obvious to read than 0 and 1. > +- ti,davinci-cs-ta - Minimum turn around time > +- ti,davinci-cs-rhold - read hold width > +- ti,davinci-cs-rstobe - read strobe width > +- ti,davinci-cs-rsetup - read setup width > +- ti,davinci-cs-whold - write hold width > +- ti,davinci-cs-wstrobe - write strobe width > +- ti,davinci-cs-wsetup - write setup width The comments in the examples indicate these are in nS. This should be mentioned here instead I think. Does there need to be a specification of bus clock rate? How does the driver convert nS into number-of-clock-cycles, which I assume is what the HW would be programmed with? > +- ti,davinci-cs-ss - enable/disable select strobe (0 - disable, 1 - enable) > +- ti,davinci-cs-ew - enable/disable extended wait cycles (0 - disable, 1 - > enable) Boolean properties are usually represented as present (on/enabled/1) or missing (off/disabled/0). Shouldn't these two properties work that way? I see the comment below: > +if any of the above parameters are absent, hardware register default or that > +set by a boot loader are used. implied they're really tri-state (explicitly off or on, or just not programmed). However, I think it'd be better if the DT always represented the complete configuration, since the DT and binding should be a full description of the HW rather than just the data you expect a particular Linux driver to need after a particular boot loader has executed first. > +Example for aemif, davinci nand and nor flash chip select shown below. > + > +aemif@6000 { > + compatible = "ti,davinci-aemif"; > + #address-cells = <2>; > + #size-cells = <1>; > + reg = <0x6800 0x8>; > + ranges = <2 0 0x6000 0x0200 > + 3 0 0x6200 0x0200 > + 4 0 0x6400 0x0200 > + 5 0 0x6600 0x0200 > + 6 0 0x6800 0x0200>; > + > + nand_cs:cs2@6000 { This node has no reg property, but it needs one if you use a unit address ("@6000") in the node name. The numbering scheme unit address above doesn't seem to match the addresses provided to child nodes by the ranges property. Perhaps the node layout you want is: aemif { chip-selects { nand_cs:cs2@6000 { }; }; devices { nand@3,0 { } }; }; so that the chip-selects and devices nodes can both set appropriate and different #address-cells and #size-cells? That does feel a b
Re: [RFC v2 PATCH 1/2] memory: davinci - add aemif controller platform driver
Santhosh, Thanks for the review. Some of the comments below applies to the original code as this driver is moved from mach-davinci to memory. I will fix them though. See below my response. + +curr_cs_data = get_cs_data(chip_cs); +if (curr_cs_data) { +/* for configuration we use cs since it is used to index ACR */ +ret = davinci_aemif_config_abus(chip_cs, aemif->base, data); +if (!ret) { +*curr_cs_data = *data; +return 0; +} +} + +return ret; +} +EXPORT_SYMBOL(davinci_aemif_set_abus_params); + Who is the user of this EXPORT ? Mostly not needed, but there is a use case for connecting FPGA that needs to do the configuration dynamically I guess. Want to hear from others if this is needed? Same for below comment. May be we can remove this now and add it later if needed. +/** + * davinci_aemif_get_abus_params - Get bus configuration data for a given cs + * @cs: chip-select, values 2-5 + * returns: ptr to a struct having the current configuration data + */ +struct davinci_aemif_cs_data *davinci_aemif_get_abus_params(unsigned int cs) +{ +/* translate to chip CS which starts at 2 */ +return get_cs_data(cs + 2); +} +EXPORT_SYMBOL(davinci_aemif_get_abus_params); + This one too. [...] +static int __devinit davinci_aemif_probe(struct platform_device *pdev) +{ +struct davinci_aemif_pdata *cfg; +int ret = -ENODEV, i; +struct resource *res; + +aemif = devm_kzalloc(&pdev->dev, sizeof(*aemif), GFP_KERNEL); + +if (!aemif) +return -ENOMEM; + +aemif->clk = clk_get(NULL, "aemif"); Please use dev attributes. Above usage of clk_get isn't recommonded in drivers. You might have to add alias entries in your clock data for it to work though. Need to investigate. +if (IS_ERR(aemif->clk)) +return PTR_ERR(aemif->clk); + +clk_prepare_enable(aemif->clk); +aemif->clk_rate = clk_get_rate(aemif->clk) / 1000; + /1000 for what ? Converting it into KHz ? Yes. WIll add a comment. +res = platform_get_resource(pdev, IORESOURCE_MEM, 0); +if (!res) { +pr_err("No IO memory address defined\n"); +goto error; +} + +res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + +aemif->base = devm_request_and_ioremap(&pdev->dev, res); +if (!aemif->base) { +ret = -EBUSY; +pr_err("ioremap failed\n"); +goto error; +} + +if (pdev->dev.platform_data == NULL) { +/* Not platform data, we get the cs data from the cs nodes */ +cfg = devm_kzalloc(&pdev->dev, sizeof(*cfg), GFP_KERNEL); +if (cfg == NULL) +return -ENOMEM; + +aemif->cfg = cfg; +if (of_davinci_aemif_cs_init(pdev->dev.of_node) < 0) { +pr_err("No platform data or cs of node present\n"); +goto error; +} +} else { +cfg = pdev->dev.platform_data; +aemif->cfg = cfg; +} + +for (i = 0; i < cfg->num_cs; i++) { +/* cs is from 2-5. Internally we use cs-2 to access ACR */ +ret = davinci_aemif_config_abus(cfg->cs_data[i].cs - 2, +aemif->base, &cfg->cs_data[i]); +if (ret < 0) { +pr_err("Error configuring chip select %d\n", +cfg->cs_data[i].cs); +goto error; +} +} +return 0; +error: +clk_disable_unprepare(aemif->clk); +clk_put(aemif->clk); Alos unmap 'aemif->base' here.. I thought devm_ API do this automatically. I will check. +return ret; +} + +static struct platform_driver davinci_aemif_driver = { +.probe = davinci_aemif_probe, +.driver = { +.name = DRV_NAME, +.owner = THIS_MODULE, +.of_match_table = davinci_aemif_of_match, +}, +}; + +static int __init davinci_aemif_init(void) +{ +return platform_driver_register(&davinci_aemif_driver); +} +subsys_initcall(davinci_aemif_init); + +static void __exit davinci_aemif_exit(void) +{ +clk_disable_unprepare(aemif->clk); +clk_put(aemif->clk); +platform_driver_unregister(&davinci_aemif_driver); +} +module_exit(davinci_aemif_exit); + +MODULE_AUTHOR("Murali Karicheri "); +MODULE_DESCRIPTION("Texas Instruments AEMIF driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:" DRV_NAME); diff --git a/include/linux/platform_data/davinci-aemif.h b/include/linux/platform_data/davinci-aemif.h new file mode 100644 index 000..03f3ad0 --- /dev/null +++ b/include/linux/platform_data/davinci-aemif.h @@ -0,0 +1,47 @@ +/* + * TI DaVinci AEMIF support + * + * Copyright 2010 (C) Texas Instruments, Inc. http://www.ti.com/ + * s/2010/2012 + * 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. + */ +#ifndef _MACH_DAVINCI_AEMIF_H +#define _MACH_DAVINCI_AEMIF_H Fix the header guard as per new directory Ok. Regards Santosh
Re: [RFC v2 PATCH 1/2] memory: davinci - add aemif controller platform driver
On Tuesday 06 November 2012 03:47 PM, Murali Karicheri wrote: This is a platform driver for asynchronous external memory interface available on TI SoCs. This driver was previously located inside the mach-davinci folder. As this DaVinci IP is re-used across multiple family of devices such as c6x, keystone etc, the driver is moved to drivers. The driver configures async bus parameters associated with a particular chip select. For DaVinci NAND controller driver and driver for other async devices such as NOR flash, ASRAM etc, the bus confuguration is done by this driver at probe. A set of APIs (set/get) are provided to update the values based on specific driver usage. This supports configuration of the bus either through platform_data or through DT bindings. Signed-off-by: Murali Karicheri --- .../devicetree/bindings/memory/davinci-aemif.txt | 103 + drivers/memory/Kconfig | 10 + drivers/memory/Makefile|1 + drivers/memory/davinci-aemif.c | 479 include/linux/platform_data/davinci-aemif.h| 47 ++ 5 files changed, 640 insertions(+) create mode 100644 Documentation/devicetree/bindings/memory/davinci-aemif.txt create mode 100644 drivers/memory/davinci-aemif.c create mode 100644 include/linux/platform_data/davinci-aemif.h Please split the DT and drivers/memory/* changes in two seperate patches. diff --git a/Documentation/devicetree/bindings/memory/davinci-aemif.txt b/Documentation/devicetree/bindings/memory/davinci-aemif.txt new file mode 100644 index 000..a79b3ed --- /dev/null +++ b/Documentation/devicetree/bindings/memory/davinci-aemif.txt @@ -0,0 +1,103 @@ +* Texas Instruments Davinci AEMIF bus interface + +This file provides information for the davinci-emif device and +async bus bindings. + +Required properties:= +- compatible: "ti,davinci-aemif"; +- #address-cells : Should be either two or three. The first cell is the + chipselect number, and the remaining cells are the + offset into the chipselect. +- #size-cells : Either one or two, depending on how large each chipselect +can be. +- reg : contains offset/length value for AEMIF control registers space +- ranges : Each range corresponds to a single chipselect, and cover + the entire access window as configured. + +Child device nodes describe the devices connected to IFC such as NOR (e.g. +cfi-flash) and NAND (ti,davinci-nand, see Documentation/devicetree/bindings/ +mtd/davinci-nand.txt). There might be board specific devices like FPGAs. + +In addition, optional child sub nodes contains bindings for the async bus +interface for a given chip select. + +Optional cs node properties:- +- compatible: "ti,davinci-cs" + + All of the params below in nanoseconds and are optional + +- ti,davinci-cs-asize - asynchronous data bus width (0 - 8bit, 1 - 16 bit) +- ti,davinci-cs-ta - Minimum turn around time +- ti,davinci-cs-rhold - read hold width +- ti,davinci-cs-rstobe - read strobe width +- ti,davinci-cs-rsetup - read setup width +- ti,davinci-cs-whold - write hold width +- ti,davinci-cs-wstrobe - write strobe width +- ti,davinci-cs-wsetup - write setup width +- ti,davinci-cs-ss - enable/disable select strobe (0 - disable, 1 - enable) +- ti,davinci-cs-ew - enable/disable extended wait cycles (0 - disable, 1 - enable) + +if any of the above parameters are absent, hardware register default or that +set by a boot loader are used. + +Example for aemif, davinci nand and nor flash chip select shown below. + +aemif@6000 { + compatible = "ti,davinci-aemif"; + #address-cells = <2>; + #size-cells = <1>; + reg = <0x6800 0x8>; + ranges = <2 0 0x6000 0x0200 + 3 0 0x6200 0x0200 + 4 0 0x6400 0x0200 + 5 0 0x6600 0x0200 + 6 0 0x6800 0x0200>; + + nand_cs:cs2@6000 { + compatible = "ti,davinci-cs"; + #address-cells = <1>; + #size-cells = <1>; + /* all timings in nanoseconds */ + ti,davinci-cs-ta = <0>; + ti,davinci-cs-rhold = <7>; + ti,davinci-cs-rstrobe = <42>; + ti,davinci-cs-rsetup = <14>; + ti,davinci-cs-whold = <7>; + ti,davinci-cs-wstrobe = <42>; + ti,davinci-cs-wsetup = <14>; + }; + + nor_cs:cs3@6200 { + compatible = "ti,davinci-cs"; + #address-cells = <1>; + #size-cells = <1>; + /* all timings in nanoseconds */ + ti,davinci-cs-ta = <0>; + ti,davinci-cs-rhold = <7>; + ti,davinci-cs-rstrobe = <42>; + ti,davinci-cs-rsetup = <14>; + ti,davinci-cs-whold = <7>; + ti,davinci-cs-wstrobe = <42>; + ti,davinci-cs-