[PATCH v5 0/2] Introduce AEMIF driver for Davinci/Keystone archs

[PATCH v5 0/2] Introduce AEMIF driver for Davinci/Keystone archs

[Ivan Khoronzhuk]

These patches introduce Async External Memory Interface (EMIF16/AEMIF)
controller driver for Davinci/Keystone archs.

For more informations see documentation:
Davinci DM646x - http://www.ti.com/lit/ug/sprueq7c/sprueq7c.pdf
OMAP-L138 - http://www.ti.com/lit/ug/spruh77a/spruh77a.pdf
Kestone - http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf

Based on v3.14-rc3.

v4..v5:
- memory: ti-aemif: introduce AEMIF driver
	deleted DRV_NAME in favour of KBUILD_MODNAME
	deleted redundant err message in case of memory allocation
	some cosmetic changes

v3..v4:
	rebased on latest of linux-keystone.git keystone/master

v2..v3 (https://lkml.org/lkml/2013/12/11/148):
- memory: ti-aemif: introduce AEMIF driver
	changed to work with multiple AEMIF controllers
	corrected "copyright" to "authors" in header
	changed compatible "ti,omap-L138-aemif" to "ti,da850-aeimf"
	used NULL in clk_get() instead of "aemif" name
	driver can be build as loadable module
	treat all child nodes as cs nodes, it makes code simpler

- memory: ti-aemif: add bindings for AEMIF driver
	deleted direct link driver/memory/ti-aemif.c
	clarified description of controller ranges property
	changed compatible "ti,omap-L138-aemif" to "ti,da850-aeimf"
	added cs number information in commit log
	removed compatible property from cs node, it makes code simpler

v1..v2 (https://lkml.org/lkml/2013/11/21/170):
- memory: ti-aemif: introduce AEMIF driver
- memory: ti-aemif: add bindings for AEMIF driver
	added ti.cs-chipselect property instead of representing chipselect
	number in cs node name.

Ivan Khoronzhuk (2):
  memory: ti-aemif: introduce AEMIF driver
  memory: ti-aemif: add bindings for AEMIF driver

 .../bindings/memory-controllers/ti-aemif.txt       | 210 ++++++++++
 drivers/memory/Kconfig                             |  11 +
 drivers/memory/Makefile                            |   1 +
 drivers/memory/ti-aemif.c                          | 427 +++++++++++++++++++++
 4 files changed, 649 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
 create mode 100644 drivers/memory/ti-aemif.c

-- 
1.8.3.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v5 1/2] memory: ti-aemif: introduce AEMIF driver

[Ivan Khoronzhuk]

Add new AEMIF driver for EMIF16 Texas Instruments controller.
The EMIF16 module is intended to provide a glue-less interface to
a variety of asynchronous memory devices like ASRA M, NOR and NAND
memory. A total of 256M bytes of any of these memories can be
accessed at any given time via 4 chip selects with 64M byte access
per chip select.

Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR
are not supported.

This controller is used on SoCs like Davinci, Keysone2

Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 drivers/memory/Kconfig    |  11 ++
 drivers/memory/Makefile   |   1 +
 drivers/memory/ti-aemif.c | 427 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 439 insertions(+)
 create mode 100644 drivers/memory/ti-aemif.c

diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 29a11db..7bc3982 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -7,6 +7,17 @@ menuconfig MEMORY
 
 if MEMORY
 
+config TI_AEMIF
+	tristate "Texas Instruments AEMIF driver"
+	depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF
+	help
+	  This driver is for the AEMIF module available in Texas Instruments
+	  SoCs. AEMIF stands for Asynchronous External Memory Interface and
+	  is intended to provide a glue-less interface to a variety of
+	  asynchronuous memory devices like ASRAM, NOR and NAND memory. A total
+	  of 256M bytes of any of these memories can be accessed at a given
+	  time via four chip selects with 64M byte access per chip select.
+
 config TI_EMIF
 	tristate "Texas Instruments EMIF driver"
 	depends on ARCH_OMAP2PLUS
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index 969d923..d4e150c 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -5,6 +5,7 @@
 ifeq ($(CONFIG_DDR),y)
 obj-$(CONFIG_OF)		+= of_memory.o
 endif
+obj-$(CONFIG_TI_AEMIF)		+= ti-aemif.o
 obj-$(CONFIG_TI_EMIF)		+= emif.o
 obj-$(CONFIG_MVEBU_DEVBUS)	+= mvebu-devbus.o
 obj-$(CONFIG_TEGRA20_MC)	+= tegra20-mc.o
diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
new file mode 100644
index 0000000..fcc25a1
--- /dev/null
+++ b/drivers/memory/ti-aemif.c
@@ -0,0 +1,427 @@
+/*
+ * TI AEMIF driver
+ *
+ * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/
+ *
+ * Authors:
+ * Murali Karicheri <m-karicheri2@ti.com>
+ * Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#define TA_SHIFT	2
+#define RHOLD_SHIFT	4
+#define RSTROBE_SHIFT	7
+#define RSETUP_SHIFT	13
+#define WHOLD_SHIFT	17
+#define WSTROBE_SHIFT	20
+#define WSETUP_SHIFT	26
+#define EW_SHIFT	30
+#define SS_SHIFT	31
+
+#define TA(x)		((x) << TA_SHIFT)
+#define RHOLD(x)	((x) << RHOLD_SHIFT)
+#define RSTROBE(x)	((x) << RSTROBE_SHIFT)
+#define RSETUP(x)	((x) << RSETUP_SHIFT)
+#define WHOLD(x)	((x) << WHOLD_SHIFT)
+#define WSTROBE(x)	((x) << WSTROBE_SHIFT)
+#define WSETUP(x)	((x) << WSETUP_SHIFT)
+#define EW(x)		((x) << EW_SHIFT)
+#define SS(x)		((x) << SS_SHIFT)
+
+#define ASIZE_MAX	0x1
+#define TA_MAX		0x3
+#define RHOLD_MAX	0x7
+#define RSTROBE_MAX	0x3f
+#define RSETUP_MAX	0xf
+#define WHOLD_MAX	0x7
+#define WSTROBE_MAX	0x3f
+#define WSETUP_MAX	0xf
+#define EW_MAX		0x1
+#define SS_MAX		0x1
+#define NUM_CS		4
+
+#define TA_VAL(x)	(((x) & TA(TA_MAX)) >> TA_SHIFT)
+#define RHOLD_VAL(x)	(((x) & RHOLD(RHOLD_MAX)) >> RHOLD_SHIFT)
+#define RSTROBE_VAL(x)	(((x) & RSTROBE(RSTROBE_MAX)) >> RSTROBE_SHIFT)
+#define RSETUP_VAL(x)	(((x) & RSETUP(RSETUP_MAX)) >> RSETUP_SHIFT)
+#define WHOLD_VAL(x)	(((x) & WHOLD(WHOLD_MAX)) >> WHOLD_SHIFT)
+#define WSTROBE_VAL(x)	(((x) & WSTROBE(WSTROBE_MAX)) >> WSTROBE_SHIFT)
+#define WSETUP_VAL(x)	(((x) & WSETUP(WSETUP_MAX)) >> WSETUP_SHIFT)
+#define EW_VAL(x)	(((x) & EW(EW_MAX)) >> EW_SHIFT)
+#define SS_VAL(x)	(((x) & SS(SS_MAX)) >> SS_SHIFT)
+
+#define NRCSR_OFFSET	0x00
+#define AWCCR_OFFSET	0x04
+#define A1CR_OFFSET	0x10
+
+#define ACR_ASIZE_MASK	0x3
+#define ACR_EW_MASK	BIT(30)
+#define ACR_SS_MASK	BIT(31)
+#define ASIZE_16BIT	1
+
+#define CONFIG_MASK	(TA(TA_MAX) | \
+				RHOLD(RHOLD_MAX) | \
+				RSTROBE(RSTROBE_MAX) |	\
+				RSETUP(RSETUP_MAX) | \
+				WHOLD(WHOLD_MAX) | \
+				WSTROBE(WSTROBE_MAX) | \
+				WSETUP(WSETUP_MAX) | \
+				EW(EW_MAX) | SS(SS_MAX) | \
+				ASIZE_MAX)
+
+/**
+ * struct aemif_cs_data: structure to hold cs parameters
+ * @cs: chip-select number
+ * @wstrobe: write strobe width, ns
+ * @rstrobe: read strobe width, ns
+ * @wsetup: write setup width, ns
+ * @whold: write hold width, ns
+ * @rsetup: read setup width, ns
+ * @rhold: read hold width, ns
+ * @ta: minimum turn around time, ns
+ * @enable_ss: enable/disable select strobe mode
+ * @enable_ew: enable/disable extended wait mode
+ * @asize: width of the asynchronous device's data bus
+ */
+struct aemif_cs_data {
+	u8	cs;
+	u16	wstrobe;
+	u16	rstrobe;
+	u8	wsetup;
+	u8	whold;
+	u8	rsetup;
+	u8	rhold;
+	u8	ta;
+	u8	enable_ss;
+	u8	enable_ew;
+	u8	asize;
+};
+
+/**
+ * struct aemif_device: structure to hold device data
+ * @base: base address of AEMIF registers
+ * @clk: source clock
+ * @clk_rate: clock's rate in kHz
+ * @num_cs: number of assigned chip-selects
+ * @cs_offset: start number of cs nodes
+ * @cs_data: array of chip-select settings
+ */
+struct aemif_device {
+	void __iomem *base;
+	struct clk *clk;
+	unsigned long clk_rate;
+	u8 num_cs;
+	int cs_offset;
+	struct aemif_cs_data cs_data[NUM_CS];
+};
+
+/**
+ * aemif_calc_rate - calculate timing data.
+ * @pdev: platform device to calculate for
+ * @wanted: The cycle time needed in nanoseconds.
+ * @clk: The input clock rate in kHz.
+ * @max: The maximum divider value that can be programmed.
+ *
+ * On success, returns the calculated timing value minus 1 for easy
+ * programming into AEMIF timing registers, else negative errno.
+ */
+static int aemif_calc_rate(struct platform_device *pdev, int wanted,
+			   unsigned long clk, int max)
+{
+	int result;
+
+	result = DIV_ROUND_UP((wanted * clk), NSEC_PER_MSEC) - 1;
+
+	dev_dbg(&pdev->dev, "%s: result %d from %ld, %d\n", __func__, result,
+		clk, wanted);
+
+	/* It is generally OK to have a more relaxed timing than requested... */
+	if (result < 0)
+		result = 0;
+
+	/* ... But configuring tighter timings is not an option. */
+	else if (result > max)
+		result = -EINVAL;
+
+	return result;
+}
+
+/**
+ * aemif_config_abus - configure async bus parameters
+ * @pdev: platform device to configure for
+ * @csnum: aemif chip select number
+ *
+ * This function programs the given timing values (in real clock) into the
+ * AEMIF registers taking the AEMIF clock into account.
+ *
+ * This function does not use any locking while programming the AEMIF
+ * because it is expected that there is only one user of a given
+ * chip-select.
+ *
+ * Returns 0 on success, else negative errno.
+ */
+static int aemif_config_abus(struct platform_device *pdev, int csnum)
+{
+	struct aemif_device *aemif = platform_get_drvdata(pdev);
+	struct aemif_cs_data *data = &aemif->cs_data[csnum];
+	int ta, rhold, rstrobe, rsetup, whold, wstrobe, wsetup;
+	unsigned long clk_rate = aemif->clk_rate;
+	unsigned offset;
+	u32 set, val;
+
+	offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
+
+	ta	= aemif_calc_rate(pdev, data->ta, clk_rate, TA_MAX);
+	rhold	= aemif_calc_rate(pdev, data->rhold, clk_rate, RHOLD_MAX);
+	rstrobe	= aemif_calc_rate(pdev, data->rstrobe, clk_rate, RSTROBE_MAX);
+	rsetup	= aemif_calc_rate(pdev, data->rsetup, clk_rate, RSETUP_MAX);
+	whold	= aemif_calc_rate(pdev, data->whold, clk_rate, WHOLD_MAX);
+	wstrobe	= aemif_calc_rate(pdev, data->wstrobe, clk_rate, WSTROBE_MAX);
+	wsetup	= aemif_calc_rate(pdev, data->wsetup, clk_rate, WSETUP_MAX);
+
+	if (ta < 0 || rhold < 0 || rstrobe < 0 || rsetup < 0 ||
+	    whold < 0 || wstrobe < 0 || wsetup < 0) {
+		dev_err(&pdev->dev, "%s: cannot get suitable timings\n",
+			__func__);
+		return -EINVAL;
+	}
+
+	set = TA(ta) | RHOLD(rhold) | RSTROBE(rstrobe) | RSETUP(rsetup) |
+		WHOLD(whold) | WSTROBE(wstrobe) | WSETUP(wsetup);
+
+	set |= (data->asize & ACR_ASIZE_MASK);
+	if (data->enable_ew)
+		set |= ACR_EW_MASK;
+	if (data->enable_ss)
+		set |= ACR_SS_MASK;
+
+	val = readl(aemif->base + offset);
+	val &= ~CONFIG_MASK;
+	val |= set;
+	writel(val, aemif->base + offset);
+
+	return 0;
+}
+
+static inline int aemif_cycles_to_nsec(int val, unsigned long clk_rate)
+{
+	return ((val + 1) * NSEC_PER_MSEC) / clk_rate;
+}
+
+/**
+ * aemif_get_hw_params - function to read hw register values
+ * @pdev: platform device to read for
+ * @csnum: aemif chip select number
+ *
+ * This function reads the defaults from the registers and update
+ * the timing values. Required for get/set commands and also for
+ * the case when driver needs to use defaults in hardware.
+ */
+static void aemif_get_hw_params(struct platform_device *pdev, int csnum)
+{
+	struct aemif_device *aemif = platform_get_drvdata(pdev);
+	struct aemif_cs_data *data = &aemif->cs_data[csnum];
+	unsigned long clk_rate = aemif->clk_rate;
+	u32 val, offset;
+
+	offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
+	val = readl(aemif->base + offset);
+
+	data->ta = aemif_cycles_to_nsec(TA_VAL(val), clk_rate);
+	data->rhold = aemif_cycles_to_nsec(RHOLD_VAL(val), clk_rate);
+	data->rstrobe = aemif_cycles_to_nsec(RSTROBE_VAL(val), clk_rate);
+	data->rsetup = aemif_cycles_to_nsec(RSETUP_VAL(val), clk_rate);
+	data->whold = aemif_cycles_to_nsec(WHOLD_VAL(val), clk_rate);
+	data->wstrobe = aemif_cycles_to_nsec(WSTROBE_VAL(val), clk_rate);
+	data->wsetup = aemif_cycles_to_nsec(WSETUP_VAL(val), clk_rate);
+	data->enable_ew = EW_VAL(val);
+	data->enable_ss = SS_VAL(val);
+	data->asize = val & ASIZE_MAX;
+}
+
+/**
+ * of_aemif_parse_abus_config - parse CS configuration from DT
+ * @pdev: platform device to parse for
+ * @np: device node ptr
+ *
+ * This function update the emif async bus configuration based on the values
+ * configured in a cs device binding node.
+ */
+static int of_aemif_parse_abus_config(struct platform_device *pdev,
+				      struct device_node *np)
+{
+	struct aemif_device *aemif = platform_get_drvdata(pdev);
+	struct aemif_cs_data *data;
+	u32 cs;
+	u32 val;
+
+	if (of_property_read_u32(np, "ti,cs-chipselect", &cs)) {
+		dev_dbg(&pdev->dev, "cs property is required");
+		return -EINVAL;
+	}
+
+	if (cs - aemif->cs_offset >= NUM_CS || cs < aemif->cs_offset) {
+		dev_dbg(&pdev->dev, "cs number is incorrect %d", cs);
+		return -EINVAL;
+	}
+
+	if (aemif->num_cs >= NUM_CS) {
+		dev_dbg(&pdev->dev, "cs count is more than %d", NUM_CS);
+		return -EINVAL;
+	}
+
+	data = &aemif->cs_data[aemif->num_cs];
+	data->cs = cs;
+
+	/* read the current value in the hw register */
+	aemif_get_hw_params(pdev, aemif->num_cs++);
+
+	/* override the values from device node */
+	if (!of_property_read_u32(np, "ti,cs-ta", &val))
+		data->ta = val;
+
+	if (!of_property_read_u32(np, "ti,cs-rhold", &val))
+		data->rhold = val;
+
+	if (!of_property_read_u32(np, "ti,cs-rstrobe", &val))
+		data->rstrobe = val;
+
+	if (!of_property_read_u32(np, "ti,cs-rsetup", &val))
+		data->rsetup = val;
+
+	if (!of_property_read_u32(np, "ti,cs-whold", &val))
+		data->whold = val;
+
+	if (!of_property_read_u32(np, "ti,cs-wstrobe", &val))
+		data->wstrobe = val;
+
+	if (!of_property_read_u32(np, "ti,cs-wsetup", &val))
+		data->wsetup = val;
+
+	if (!of_property_read_u32(np, "ti,bus-width", &val))
+		if (val == 16)
+			data->asize = 1;
+	data->enable_ew = of_property_read_bool(np, "ti,cs-ew");
+	data->enable_ss = of_property_read_bool(np, "ti,cs-ss");
+	return 0;
+}
+
+static const struct of_device_id aemif_of_match[] = {
+	{ .compatible = "ti,davinci-aemif", },
+	{ .compatible = "ti,da850-aemif", },
+	{},
+};
+
+static int aemif_probe(struct platform_device *pdev)
+{
+	int i;
+	int ret = -ENODEV;
+	struct resource *res;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *child_np;
+	struct aemif_device *aemif;
+
+	if (np == NULL)
+		return 0;
+
+	aemif = devm_kzalloc(dev, sizeof(*aemif), GFP_KERNEL);
+	if (!aemif)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, aemif);
+
+	aemif->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(aemif->clk)) {
+		dev_err(dev, "cannot get clock 'aemif'\n");
+		return PTR_ERR(aemif->clk);
+	}
+
+	clk_prepare_enable(aemif->clk);
+	aemif->clk_rate = clk_get_rate(aemif->clk) / MSEC_PER_SEC;
+
+	if (of_device_is_compatible(np, "ti,da850-aemif"))
+		aemif->cs_offset = 2;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	aemif->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(aemif->base)) {
+		ret = PTR_ERR(aemif->base);
+		goto error;
+	}
+
+	/*
+	 * For every controller device node, there is a cs device node that
+	 * describe the bus configuration parameters. This functions iterate
+	 * over these nodes and update the cs data array.
+	 */
+	for_each_available_child_of_node(np, child_np) {
+		ret = of_aemif_parse_abus_config(pdev, child_np);
+		if (ret < 0)
+			goto error;
+	}
+
+	for (i = 0; i < aemif->num_cs; i++) {
+		ret = aemif_config_abus(pdev, i);
+		if (ret < 0) {
+			dev_err(dev, "Error configuring chip select %d\n",
+				aemif->cs_data[i].cs);
+			goto error;
+		}
+	}
+
+	/*
+	 * Create a child devices explicitly from here to
+	 * guarantee that the child will be probed after the AEMIF timing
+	 * parameters are set.
+	 */
+	for_each_available_child_of_node(np, child_np) {
+		ret = of_platform_populate(child_np, NULL, NULL, dev);
+		if (ret < 0)
+			goto error;
+	}
+
+	return 0;
+error:
+	clk_disable_unprepare(aemif->clk);
+	return ret;
+}
+
+static int aemif_remove(struct platform_device *pdev)
+{
+	struct aemif_device *aemif = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(aemif->clk);
+	return 0;
+}
+
+static struct platform_driver aemif_driver = {
+	.probe = aemif_probe,
+	.remove = aemif_remove,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(aemif_of_match),
+	},
+};
+
+module_platform_driver(aemif_driver);
+
+MODULE_AUTHOR("Murali Karicheri <m-karicheri2@ti.com>");
+MODULE_AUTHOR("Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>");
+MODULE_DESCRIPTION("Texas Instruments AEMIF driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
-- 
1.8.3.2

[PATCH v5 2/2] memory: ti-aemif: add bindings for AEMIF driver

[Ivan Khoronzhuk]

Add bindings for TI Async External Memory Interface (AEMIF) controller.

The Async External Memory Interface (EMIF16/AEMIF) controller is intended to
provide a glue-less interface to a variety of asynchronous memory devices like
ASRA M, NOR and NAND memory. A total of 256M bytes of any of these memories
can be accessed via 4 chip selects with 64M byte access per chip select.

We are not encoding CS number in reg property, it's memory partition number.
The CS number is encoded for Davinci NAND node using standalone property
"ti,davinci-chipselect" and we need to provide two memory ranges to it,
as result we can't encode CS number in "reg" for AEMIF child devices
(NAND/NOR/etc), as it will break bindings compatibility.

In this patch, NAND node is used just as an example of child node.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 .../bindings/memory-controllers/ti-aemif.txt       | 210 +++++++++++++++++++++
 1 file changed, 210 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt

diff --git a/Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt b/Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
new file mode 100644
index 0000000..48f82e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
@@ -0,0 +1,210 @@
+* Device tree bindings for Texas instruments AEMIF controller
+
+The Async External Memory Interface (EMIF16/AEMIF) controller is intended to
+provide a glue-less interface to a variety of asynchronous memory devices like
+ASRA M, NOR and NAND memory. A total of 256M bytes of any of these memories
+can be accessed at any given time via four chip selects with 64M byte access
+per chip select. Synchronous memories such as DDR1 SD RAM, SDR SDRAM
+and Mobile SDR are not supported.
+
+Documentation:
+Davinci DM646x - http://www.ti.com/lit/ug/sprueq7c/sprueq7c.pdf
+OMAP-L138 (DA850) - http://www.ti.com/lit/ug/spruh77a/spruh77a.pdf
+Kestone - http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf
+
+Required properties:
+
+- compatible:		"ti,davinci-aemif"
+			"ti,keystone-aemif"
+			"ti,da850-aemif"
+
+- reg:			contains offset/length value for AEMIF control registers
+			space.
+
+- #address-cells:	Must be 2. The partition number has to be encoded in the
+			first address cell and it may accept values 0..N-1
+			(N - total number of partitions). It's recommended to
+			assign N-1 number for the control partition. The second
+			cell is the offset into the partition.
+
+- #size-cells:		Must be set to 1.
+
+- ranges:		Contains memory regions. There are two types of
+			ranges/partitions:
+			- CS-specific partition/range. If continuous, must be
+			set up to reflect the memory layout for 4 chipselects,
+			if not then additional range/partition can be added and
+			child device can select the proper one.
+			- control partition which is common for all CS
+			interfaces.
+
+- clocks:		the clock feeding the controller clock. Required only
+			if clock tree data present in device tree.
+			See clock-bindings.txt
+
+- clock-names:		clock name. It has to be "aemif". Required only if clock
+			tree data present in device tree, in another case don't
+			use it.
+			See clock-bindings.txt
+
+- clock-ranges:		Empty property indicating that child nodes can inherit
+			named clocks. Required only if clock tree data present
+			in device tree.
+			See clock-bindings.txt
+
+
+Child chip-select (cs) nodes contain the memory devices nodes connected to
+such as NOR (e.g. cfi-flash) and NAND (ti,davinci-nand, see davinci-nand.txt).
+There might be board specific devices like FPGAs.
+
+Required child cs node properties:
+
+- #address-cells:	Must be 2.
+
+- #size-cells:		Must be 1.
+
+- ranges:		Empty property indicating that child nodes can inherit
+			memory layout.
+
+- clock-ranges:		Empty property indicating that child nodes can inherit
+			named clocks. Required only if clock tree data present
+			in device tree.
+
+- ti,cs-chipselect:	number of chipselect. Indicates on the aemif driver
+			which chipselect is used for accessing the memory. For
+			compatibles "ti,davinci-aemif" and "ti,keystone-aemif"
+			it can be in range [0-3]. For compatible
+			"ti,da850-aemif" range is [2-5].
+
+Optional child cs node properties:
+
+- ti,bus-width:			width of the asynchronous device's data bus
+				8 or 16 if not preset 8
+
+- ti,cs-ss:		enable/disable select strobe mode
+				In select strobe mode chip select behaves as
+				the strobe and is active only during the strobe
+				period. If present then enable.
+
+- ti,cs-ew:		enable/disable extended wait mode
+				if set, the controller monitors the EMIFWAIT pin
+				mapped to that chip select to determine if the
+				device wants to extend the strobe period. If
+				present then enable.
+
+- ti,cs-ta:		minimum turn around time, ns
+				Time between the end of one asynchronous memory
+				access and the start of another asynchronous
+				memory access. This delay is not incurred
+				between a read followed by read or a write
+				followed by a write to same chip select.
+
+- ti,cs-rsetup:		read setup width, ns
+				Time between the beginning of a memory cycle
+				and the activation of read strobe.
+				Minimum value is 1 (0 treated as 1).
+
+- ti,cs-rstobe:		read strobe width, ns
+				Time between the activation and deactivation of
+				the read strobe.
+				Minimum value is 1 (0 treated as 1).
+
+- ti,cs-rhold:		read hold width, ns
+				Time between the deactivation of the read
+				strobe and the end of the cycle (which may be
+				either an address change or the deactivation of
+				the chip select signal.
+				Minimum value is 1 (0 treated as 1).
+
+- ti,cs-wsetup:		write setup width, ns
+				Time between the beginning of a memory cycle
+				and the activation of write strobe.
+				Minimum value is 1 (0 treated as 1).
+
+- ti,cs-wstrobe:	write strobe width, ns
+				Time between the activation and deactivation of
+				the write strobe.
+				Minimum value is 1 (0 treated as 1).
+
+- ti,cs-whold:		write hold width, ns
+				Time between the deactivation of the write
+				strobe and the end of the cycle (which may be
+				either an address change or the deactivation of
+				the chip select signal.
+				Minimum value is 1 (0 treated as 1).
+
+If any of the above parameters are absent, current parameter value will be taken
+from the corresponding HW reg.
+
+Example for aemif, davinci nand and nor flash chip select shown below.
+
+memory-controller@21000A00 {
+	compatible = "ti,davinci-aemif";
+	#address-cells = <2>;
+	#size-cells = <1>;
+	clocks = <&clkaemif 0>;
+	clock-names = "aemif";
+	clock-ranges;
+	reg = <0x21000A00 0x00000100>;
+	ranges = <0 0 0x70000000 0x10000000
+		  1 0 0x21000A00 0x00000100>;
+		  /*
+		   * Partition0: CS-specific memory range which is
+		   * implemented as continuous physical memory region
+		   * Partition1: control memory range
+		   */
+
+	nand:cs2 {
+		#address-cells = <2>;
+		#size-cells = <1>;
+		clock-ranges;
+		ranges;
+
+		ti,cs-chipselect = <2>;
+		/* all timings in nanoseconds */
+		ti,cs-ta = <0>;
+		ti,cs-rhold = <7>;
+		ti,cs-rstrobe = <42>;
+		ti,cs-rsetup = <14>;
+		ti,cs-whold = <7>;
+		ti,cs-wstrobe = <42>;
+		ti,cs-wsetup = <14>;
+
+		nand@0,0x8000000 {
+			compatible = "ti,davinci-nand";
+			reg = <0 0x8000000 0x4000000
+			       1 0x0000000 0x0000100>;
+			/*
+			 * Partition0, offset 0x8000000, size 0x4000000
+			 * Partition1, offset 0x0000000, size 0x0000100
+			 */
+
+			.. see davinci-nand.txt
+		};
+	};
+
+	nor:cs0 {
+		#address-cells = <2>;
+		#size-cells = <1>;
+		clock-ranges;
+		ranges;
+
+		ti,cs-chipselect = <0>;
+		/* all timings in nanoseconds */
+		ti,cs-ta = <0>;
+		ti,cs-rhold = <8>;
+		ti,cs-rstrobe = <40>;
+		ti,cs-rsetup = <14>;
+		ti,cs-whold = <7>;
+		ti,cs-wstrobe = <40>;
+		ti,cs-wsetup = <14>;
+		ti,cs-asize = <1>;
+
+		flash@0,0x0000000 {
+			compatible = "cfi-flash";
+			reg = <0 0x0000000 0x4000000>;
+
+			...
+		};
+	};
+};
-- 
1.8.3.2

Re: [PATCH v5 2/2] memory: ti-aemif: add bindings for AEMIF driver

[Santosh Shilimkar]

On Wednesday 19 February 2014 08:40 AM, Ivan Khoronzhuk wrote:
> Add bindings for TI Async External Memory Interface (AEMIF) controller.
> 
> The Async External Memory Interface (EMIF16/AEMIF) controller is intended to
> provide a glue-less interface to a variety of asynchronous memory devices like
> ASRA M, NOR and NAND memory. A total of 256M bytes of any of these memories
> can be accessed via 4 chip selects with 64M byte access per chip select.
> 
> We are not encoding CS number in reg property, it's memory partition number.
> The CS number is encoded for Davinci NAND node using standalone property
> "ti,davinci-chipselect" and we need to provide two memory ranges to it,
> as result we can't encode CS number in "reg" for AEMIF child devices
> (NAND/NOR/etc), as it will break bindings compatibility.
> 
> In this patch, NAND node is used just as an example of child node.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---

Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH v5 2/2] memory: ti-aemif: add bindings for AEMIF driver

[Mark Rutland]

On Wed, Feb 19, 2014 at 01:40:10PM +0000, Ivan Khoronzhuk wrote:
> Add bindings for TI Async External Memory Interface (AEMIF) controller.
> 
> The Async External Memory Interface (EMIF16/AEMIF) controller is intended to
> provide a glue-less interface to a variety of asynchronous memory devices like
> ASRA M, NOR and NAND memory. A total of 256M bytes of any of these memories
> can be accessed via 4 chip selects with 64M byte access per chip select.
> 
> We are not encoding CS number in reg property, it's memory partition number.
> The CS number is encoded for Davinci NAND node using standalone property
> "ti,davinci-chipselect" and we need to provide two memory ranges to it,
> as result we can't encode CS number in "reg" for AEMIF child devices
> (NAND/NOR/etc), as it will break bindings compatibility.
> 
> In this patch, NAND node is used just as an example of child node.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-l0cyMroinI0@public.gmane.org>
> ---
>  .../bindings/memory-controllers/ti-aemif.txt       | 210 +++++++++++++++++++++
>  1 file changed, 210 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt

[...]

> +- ranges:		Contains memory regions. There are two types of
> +			ranges/partitions:
> +			- CS-specific partition/range. If continuous, must be
> +			set up to reflect the memory layout for 4 chipselects,
> +			if not then additional range/partition can be added and
> +			child device can select the proper one.
> +			- control partition which is common for all CS
> +			interfaces.

This really doesn't sound anything like the typical meaning of ranges on
a bus.

Why isn't this information encoded in reg properties on the child nodes?

[...]

> +- ti,cs-ss:		enable/disable select strobe mode
> +				In select strobe mode chip select behaves as
> +				the strobe and is active only during the strobe
> +				period. If present then enable.
> +
> +- ti,cs-ew:		enable/disable extended wait mode
> +				if set, the controller monitors the EMIFWAIT pin
> +				mapped to that chip select to determine if the
> +				device wants to extend the strobe period. If
> +				present then enable.

When would you have these two in the DT and when wouldn't you? Why can't
the driver decide?

These names are also opaque. We can clearly do better.

> +
> +- ti,cs-ta:		minimum turn around time, ns
> +				Time between the end of one asynchronous memory
> +				access and the start of another asynchronous
> +				memory access. This delay is not incurred
> +				between a read followed by read or a write
> +				followed by a write to same chip select.

The name is opaque. How about ti,min-turnaround-time-ns ?

> +
> +- ti,cs-rsetup:		read setup width, ns
> +				Time between the beginning of a memory cycle
> +				and the activation of read strobe.
> +				Minimum value is 1 (0 treated as 1).
> +
> +- ti,cs-rstobe:		read strobe width, ns
> +				Time between the activation and deactivation of
> +				the read strobe.
> +				Minimum value is 1 (0 treated as 1).
> +
> +- ti,cs-rhold:		read hold width, ns
> +				Time between the deactivation of the read
> +				strobe and the end of the cycle (which may be
> +				either an address change or the deactivation of
> +				the chip select signal.
> +				Minimum value is 1 (0 treated as 1).
> +
> +- ti,cs-wsetup:		write setup width, ns
> +				Time between the beginning of a memory cycle
> +				and the activation of write strobe.
> +				Minimum value is 1 (0 treated as 1).
> +
> +- ti,cs-wstrobe:	write strobe width, ns
> +				Time between the activation and deactivation of
> +				the write strobe.
> +				Minimum value is 1 (0 treated as 1).
> +
> +- ti,cs-whold:		write hold width, ns
> +				Time between the deactivation of the write
> +				strobe and the end of the cycle (which may be
> +				either an address change or the deactivation of
> +				the chip select signal.
> +				Minimum value is 1 (0 treated as 1).

Likewise I think these can be given more descriptive names.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/2] memory: ti-aemif: add bindings for AEMIF driver

[Ivan Khoronzhuk]

On 02/19/2014 08:11 PM, Mark Rutland wrote:
> On Wed, Feb 19, 2014 at 01:40:10PM +0000, Ivan Khoronzhuk wrote:
>> Add bindings for TI Async External Memory Interface (AEMIF) controller.
>>
>> The Async External Memory Interface (EMIF16/AEMIF) controller is intended to
>> provide a glue-less interface to a variety of asynchronous memory devices like
>> ASRA M, NOR and NAND memory. A total of 256M bytes of any of these memories
>> can be accessed via 4 chip selects with 64M byte access per chip select.
>>
>> We are not encoding CS number in reg property, it's memory partition number.
>> The CS number is encoded for Davinci NAND node using standalone property
>> "ti,davinci-chipselect" and we need to provide two memory ranges to it,
>> as result we can't encode CS number in "reg" for AEMIF child devices
>> (NAND/NOR/etc), as it will break bindings compatibility.
>>
>> In this patch, NAND node is used just as an example of child node.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-l0cyMroinI0@public.gmane.org>
>> ---
>>   .../bindings/memory-controllers/ti-aemif.txt       | 210 +++++++++++++++++++++
>>   1 file changed, 210 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
> [...]
>
>> +- ranges:		Contains memory regions. There are two types of
>> +			ranges/partitions:
>> +			- CS-specific partition/range. If continuous, must be
>> +			set up to reflect the memory layout for 4 chipselects,
>> +			if not then additional range/partition can be added and
>> +			child device can select the proper one.
>> +			- control partition which is common for all CS
>> +			interfaces.
> This really doesn't sound anything like the typical meaning of ranges on
> a bus.
>
> Why isn't this information encoded in reg properties on the child nodes?
>
> [...]

Note that we do not introduce NAND device bindings here.
The Davinci NAND bindings was introduced and accepted more then one year ago.
And the CS number is encoded for Davinci NAND node using standalone property
"ti,davinci-chipselect" and we need to provide two memory ranges to it,
as result we can't encode CS number in "reg" for AEMIF child devices (NAND/NOR/etc),
as it will break bindings compatibility.

In this document, NAND node is used just as an example of child node.

>> +- ti,cs-ss:		enable/disable select strobe mode
>> +				In select strobe mode chip select behaves as
>> +				the strobe and is active only during the strobe
>> +				period. If present then enable.
>> +
>> +- ti,cs-ew:		enable/disable extended wait mode
>> +				if set, the controller monitors the EMIFWAIT pin
>> +				mapped to that chip select to determine if the
>> +				device wants to extend the strobe period. If
>> +				present then enable.
> When would you have these two in the DT and when wouldn't you? Why can't
> the driver decide?

These are properties of cs node, not the driver itself.
The driver cannot know what child device you are going to attach to cs node
, as result it cannot decide what settings you are using for particular 
cs node.

> These names are also opaque. We can clearly do better.

I propose the following names?:

ti,cs-ss  --> ti,cs-select-strobe-mode
ti,cs-ew --> ti,cs-extended-waite-mode

Are you OK with it?

>> +
>> +- ti,cs-ta:		minimum turn around time, ns
>> +				Time between the end of one asynchronous memory
>> +				access and the start of another asynchronous
>> +				memory access. This delay is not incurred
>> +				between a read followed by read or a write
>> +				followed by a write to same chip select.
> The name is opaque. How about ti,min-turnaround-time-ns ?

I like without -ns suffix and with cs- prefix:
ti,cs-ta --> ti,cs-min-turnaround-time

Is it OK?

>> +
>> +- ti,cs-rsetup:		read setup width, ns
>> +				Time between the beginning of a memory cycle
>> +				and the activation of read strobe.
>> +				Minimum value is 1 (0 treated as 1).
>> +
>> +- ti,cs-rstobe:		read strobe width, ns
>> +				Time between the activation and deactivation of
>> +				the read strobe.
>> +				Minimum value is 1 (0 treated as 1).
>> +
>> +- ti,cs-rhold:		read hold width, ns
>> +				Time between the deactivation of the read
>> +				strobe and the end of the cycle (which may be
>> +				either an address change or the deactivation of
>> +				the chip select signal.
>> +				Minimum value is 1 (0 treated as 1).
>> +
>> +- ti,cs-wsetup:		write setup width, ns
>> +				Time between the beginning of a memory cycle
>> +				and the activation of write strobe.
>> +				Minimum value is 1 (0 treated as 1).
>> +
>> +- ti,cs-wstrobe:	write strobe width, ns
>> +				Time between the activation and deactivation of
>> +				the write strobe.
>> +				Minimum value is 1 (0 treated as 1).
>> +
>> +- ti,cs-whold:		write hold width, ns
>> +				Time between the deactivation of the write
>> +				strobe and the end of the cycle (which may be
>> +				either an address change or the deactivation of
>> +				the chip select signal.
>> +				Minimum value is 1 (0 treated as 1).
> Likewise I think these can be given more descriptive names.

Ok. How about:

ti,cs-rsetup --> ti,cs-read-setup-time
ti,cs-rstobe --> ti,cs-read-strobe-time
ti,cs-rhold  --> ti,cs-read-hold-time
ti,cs-wsetup --> ti,cs-write-setup-time
ti,cs-wstrobe --> ti,cs-write-strobe-time
ti,cs-whold --> ti,cs-write-hold-time

Thanks

-- 
Regards,
Ivan Khoronzhuk

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/2] memory: ti-aemif: add bindings for AEMIF driver

[Rob Herring]

On Thu, Feb 20, 2014 at 6:44 AM, Ivan Khoronzhuk <ivan.khoronzhuk-l0cyMroinI0@public.gmane.org> wrote:
>
> On 02/19/2014 08:11 PM, Mark Rutland wrote:
>>
>> On Wed, Feb 19, 2014 at 01:40:10PM +0000, Ivan Khoronzhuk wrote:
>>>
>>> Add bindings for TI Async External Memory Interface (AEMIF) controller.
>>>
>>> The Async External Memory Interface (EMIF16/AEMIF) controller is intended
>>> to
>>> provide a glue-less interface to a variety of asynchronous memory devices
>>> like
>>> ASRA M, NOR and NAND memory. A total of 256M bytes of any of these
>>> memories
>>> can be accessed via 4 chip selects with 64M byte access per chip select.
>>>
>>> We are not encoding CS number in reg property, it's memory partition
>>> number.
>>> The CS number is encoded for Davinci NAND node using standalone property
>>> "ti,davinci-chipselect" and we need to provide two memory ranges to it,
>>> as result we can't encode CS number in "reg" for AEMIF child devices
>>> (NAND/NOR/etc), as it will break bindings compatibility.
>>>
>>> In this patch, NAND node is used just as an example of child node.
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk-l0cyMroinI0@public.gmane.org>
>>> ---
>>>   .../bindings/memory-controllers/ti-aemif.txt       | 210
>>> +++++++++++++++++++++
>>>   1 file changed, 210 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
>>
>> [...]
>>
>>> +- ranges:              Contains memory regions. There are two types of
>>> +                       ranges/partitions:
>>> +                       - CS-specific partition/range. If continuous,
>>> must be
>>> +                       set up to reflect the memory layout for 4
>>> chipselects,
>>> +                       if not then additional range/partition can be
>>> added and
>>> +                       child device can select the proper one.
>>> +                       - control partition which is common for all CS
>>> +                       interfaces.
>>
>> This really doesn't sound anything like the typical meaning of ranges on
>> a bus.
>>
>> Why isn't this information encoded in reg properties on the child nodes?
>>
>> [...]
>
>
> Note that we do not introduce NAND device bindings here.
> The Davinci NAND bindings was introduced and accepted more then one year
> ago.
> And the CS number is encoded for Davinci NAND node using standalone property
>
> "ti,davinci-chipselect" and we need to provide two memory ranges to it,
> as result we can't encode CS number in "reg" for AEMIF child devices
> (NAND/NOR/etc),
> as it will break bindings compatibility.
>
> In this document, NAND node is used just as an example of child node.
>
>
>>> +- ti,cs-ss:            enable/disable select strobe mode
>>> +                               In select strobe mode chip select behaves
>>> as
>>> +                               the strobe and is active only during the
>>> strobe
>>> +                               period. If present then enable.
>>> +
>>> +- ti,cs-ew:            enable/disable extended wait mode
>>> +                               if set, the controller monitors the
>>> EMIFWAIT pin
>>> +                               mapped to that chip select to determine
>>> if the
>>> +                               device wants to extend the strobe period.
>>> If
>>> +                               present then enable.
>>
>> When would you have these two in the DT and when wouldn't you? Why can't
>> the driver decide?
>
>
> These are properties of cs node, not the driver itself.
> The driver cannot know what child device you are going to attach to cs node
> , as result it cannot decide what settings you are using for particular cs
> node.
>
>
>> These names are also opaque. We can clearly do better.
>
>
> I propose the following names?:
>
> ti,cs-ss  --> ti,cs-select-strobe-mode
> ti,cs-ew --> ti,cs-extended-waite-mode
>
> Are you OK with it?
>
>
>>> +
>>> +- ti,cs-ta:            minimum turn around time, ns
>>> +                               Time between the end of one asynchronous
>>> memory
>>> +                               access and the start of another
>>> asynchronous
>>> +                               memory access. This delay is not incurred
>>> +                               between a read followed by read or a
>>> write
>>> +                               followed by a write to same chip select.
>>
>> The name is opaque. How about ti,min-turnaround-time-ns ?
>
>
> I like without -ns suffix and with cs- prefix:
> ti,cs-ta --> ti,cs-min-turnaround-time
>
> Is it OK?
>
>
>>> +
>>> +- ti,cs-rsetup:                read setup width, ns
>>> +                               Time between the beginning of a memory
>>> cycle
>>> +                               and the activation of read strobe.
>>> +                               Minimum value is 1 (0 treated as 1).
>>> +
>>> +- ti,cs-rstobe:                read strobe width, ns
>>> +                               Time between the activation and
>>> deactivation of
>>> +                               the read strobe.
>>> +                               Minimum value is 1 (0 treated as 1).
>>> +
>>> +- ti,cs-rhold:         read hold width, ns
>>> +                               Time between the deactivation of the read
>>> +                               strobe and the end of the cycle (which
>>> may be
>>> +                               either an address change or the
>>> deactivation of
>>> +                               the chip select signal.
>>> +                               Minimum value is 1 (0 treated as 1).
>>> +
>>> +- ti,cs-wsetup:                write setup width, ns
>>> +                               Time between the beginning of a memory
>>> cycle
>>> +                               and the activation of write strobe.
>>> +                               Minimum value is 1 (0 treated as 1).
>>> +
>>> +- ti,cs-wstrobe:       write strobe width, ns
>>> +                               Time between the activation and
>>> deactivation of
>>> +                               the write strobe.
>>> +                               Minimum value is 1 (0 treated as 1).
>>> +
>>> +- ti,cs-whold:         write hold width, ns
>>> +                               Time between the deactivation of the
>>> write
>>> +                               strobe and the end of the cycle (which
>>> may be
>>> +                               either an address change or the
>>> deactivation of
>>> +                               the chip select signal.
>>> +                               Minimum value is 1 (0 treated as 1).
>>
>> Likewise I think these can be given more descriptive names.
>
>
> Ok. How about:
>
> ti,cs-rsetup --> ti,cs-read-setup-time
> ti,cs-rstobe --> ti,cs-read-strobe-time
> ti,cs-rhold  --> ti,cs-read-hold-time
> ti,cs-wsetup --> ti,cs-write-setup-time
> ti,cs-wstrobe --> ti,cs-write-strobe-time
> ti,cs-whold --> ti,cs-write-hold-time

Properties that have a unit of measure should have that unit in the
name. So replace "time" with "ns".

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/2] memory: ti-aemif: add bindings for AEMIF driver

[Ivan Khoronzhuk]

On 02/20/2014 03:44 PM, Rob Herring wrote:
> On Thu, Feb 20, 2014 at 6:44 AM, Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> wrote:
>> On 02/19/2014 08:11 PM, Mark Rutland wrote:
>>> On Wed, Feb 19, 2014 at 01:40:10PM +0000, Ivan Khoronzhuk wrote:
>>>> Add bindings for TI Async External Memory Interface (AEMIF) controller.
>>>>
>>>> The Async External Memory Interface (EMIF16/AEMIF) controller is intended
>>>> to
>>>> provide a glue-less interface to a variety of asynchronous memory devices
>>>> like
>>>> ASRA M, NOR and NAND memory. A total of 256M bytes of any of these
>>>> memories
>>>> can be accessed via 4 chip selects with 64M byte access per chip select.
>>>>
>>>> We are not encoding CS number in reg property, it's memory partition
>>>> number.
>>>> The CS number is encoded for Davinci NAND node using standalone property
>>>> "ti,davinci-chipselect" and we need to provide two memory ranges to it,
>>>> as result we can't encode CS number in "reg" for AEMIF child devices
>>>> (NAND/NOR/etc), as it will break bindings compatibility.
>>>>
>>>> In this patch, NAND node is used just as an example of child node.
>>>>
>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>> ---
>>>>    .../bindings/memory-controllers/ti-aemif.txt       | 210
>>>> +++++++++++++++++++++
>>>>    1 file changed, 210 insertions(+)
>>>>    create mode 100644
>>>> Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
>>> [...]
>>>
>>>> +- ranges:              Contains memory regions. There are two types of
>>>> +                       ranges/partitions:
>>>> +                       - CS-specific partition/range. If continuous,
>>>> must be
>>>> +                       set up to reflect the memory layout for 4
>>>> chipselects,
>>>> +                       if not then additional range/partition can be
>>>> added and
>>>> +                       child device can select the proper one.
>>>> +                       - control partition which is common for all CS
>>>> +                       interfaces.
>>> This really doesn't sound anything like the typical meaning of ranges on
>>> a bus.
>>>
>>> Why isn't this information encoded in reg properties on the child nodes?
>>>
>>> [...]
>>
>> Note that we do not introduce NAND device bindings here.
>> The Davinci NAND bindings was introduced and accepted more then one year
>> ago.
>> And the CS number is encoded for Davinci NAND node using standalone property
>>
>> "ti,davinci-chipselect" and we need to provide two memory ranges to it,
>> as result we can't encode CS number in "reg" for AEMIF child devices
>> (NAND/NOR/etc),
>> as it will break bindings compatibility.
>>
>> In this document, NAND node is used just as an example of child node.
>>
>>
>>>> +- ti,cs-ss:            enable/disable select strobe mode
>>>> +                               In select strobe mode chip select behaves
>>>> as
>>>> +                               the strobe and is active only during the
>>>> strobe
>>>> +                               period. If present then enable.
>>>> +
>>>> +- ti,cs-ew:            enable/disable extended wait mode
>>>> +                               if set, the controller monitors the
>>>> EMIFWAIT pin
>>>> +                               mapped to that chip select to determine
>>>> if the
>>>> +                               device wants to extend the strobe period.
>>>> If
>>>> +                               present then enable.
>>> When would you have these two in the DT and when wouldn't you? Why can't
>>> the driver decide?
>>
>> These are properties of cs node, not the driver itself.
>> The driver cannot know what child device you are going to attach to cs node
>> , as result it cannot decide what settings you are using for particular cs
>> node.
>>
>>
>>> These names are also opaque. We can clearly do better.
>>
>> I propose the following names?:
>>
>> ti,cs-ss  --> ti,cs-select-strobe-mode
>> ti,cs-ew --> ti,cs-extended-waite-mode
>>
>> Are you OK with it?
>>
>>
>>>> +
>>>> +- ti,cs-ta:            minimum turn around time, ns
>>>> +                               Time between the end of one asynchronous
>>>> memory
>>>> +                               access and the start of another
>>>> asynchronous
>>>> +                               memory access. This delay is not incurred
>>>> +                               between a read followed by read or a
>>>> write
>>>> +                               followed by a write to same chip select.
>>> The name is opaque. How about ti,min-turnaround-time-ns ?
>>
>> I like without -ns suffix and with cs- prefix:
>> ti,cs-ta --> ti,cs-min-turnaround-time
>>
>> Is it OK?
>>
>>
>>>> +
>>>> +- ti,cs-rsetup:                read setup width, ns
>>>> +                               Time between the beginning of a memory
>>>> cycle
>>>> +                               and the activation of read strobe.
>>>> +                               Minimum value is 1 (0 treated as 1).
>>>> +
>>>> +- ti,cs-rstobe:                read strobe width, ns
>>>> +                               Time between the activation and
>>>> deactivation of
>>>> +                               the read strobe.
>>>> +                               Minimum value is 1 (0 treated as 1).
>>>> +
>>>> +- ti,cs-rhold:         read hold width, ns
>>>> +                               Time between the deactivation of the read
>>>> +                               strobe and the end of the cycle (which
>>>> may be
>>>> +                               either an address change or the
>>>> deactivation of
>>>> +                               the chip select signal.
>>>> +                               Minimum value is 1 (0 treated as 1).
>>>> +
>>>> +- ti,cs-wsetup:                write setup width, ns
>>>> +                               Time between the beginning of a memory
>>>> cycle
>>>> +                               and the activation of write strobe.
>>>> +                               Minimum value is 1 (0 treated as 1).
>>>> +
>>>> +- ti,cs-wstrobe:       write strobe width, ns
>>>> +                               Time between the activation and
>>>> deactivation of
>>>> +                               the write strobe.
>>>> +                               Minimum value is 1 (0 treated as 1).
>>>> +
>>>> +- ti,cs-whold:         write hold width, ns
>>>> +                               Time between the deactivation of the
>>>> write
>>>> +                               strobe and the end of the cycle (which
>>>> may be
>>>> +                               either an address change or the
>>>> deactivation of
>>>> +                               the chip select signal.
>>>> +                               Minimum value is 1 (0 treated as 1).
>>> Likewise I think these can be given more descriptive names.
>>
>> Ok. How about:
>>
>> ti,cs-rsetup --> ti,cs-read-setup-time
>> ti,cs-rstobe --> ti,cs-read-strobe-time
>> ti,cs-rhold  --> ti,cs-read-hold-time
>> ti,cs-wsetup --> ti,cs-write-setup-time
>> ti,cs-wstrobe --> ti,cs-write-strobe-time
>> ti,cs-whold --> ti,cs-write-hold-time
> Properties that have a unit of measure should have that unit in the
> name. So replace "time" with "ns".
>
> Rob

Ok, I will replace

Thanks!

-- 
Regards,
Ivan Khoronzhuk