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

[PATCH 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.

V1:
https://lkml.org/lkml/2013/11/11/352

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

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

-- 
1.7.9.5

[PATCH 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 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.

This controller is used on SoCs like Davinci, Keysone2

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

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 drivers/memory/Kconfig    |   11 ++
 drivers/memory/Makefile   |    1 +
 drivers/memory/ti-aemif.c |  415 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 427 insertions(+)
 create mode 100644 drivers/memory/ti-aemif.c

diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 29a11db..cc0e3c8 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -7,6 +7,17 @@ menuconfig MEMORY
 
 if MEMORY
 
+config TI_AEMIF
+	bool "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..a4b479a
--- /dev/null
+++ b/drivers/memory/ti-aemif.c
@@ -0,0 +1,415 @@
+/*
+ * TI AEMIF driver
+ *
+ * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/
+ * Copyright (C) Heiko Schocher <hs@denx.de>
+ * Copyright (C) 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)
+
+#define DRV_NAME	"ti-aemif"
+
+/**
+ * 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;
+};
+
+/**
+ * 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_data: array of chip-select settings
+ */
+struct aemif_device {
+	struct device *dev;
+	void __iomem *base;
+	struct clk *clk;
+	unsigned long clk_rate;
+	u8 num_cs;
+	int cs_offset;
+	struct aemif_cs_data cs_data[NUM_CS];
+};
+
+static struct aemif_device *aemif;
+/**
+ * aemif_calc_rate - calculate timing data.
+ * @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(int wanted, unsigned long clk, int max)
+{
+	int result;
+
+	result = DIV_ROUND_UP((wanted * clk), NSEC_PER_MSEC) - 1;
+
+	dev_dbg(aemif->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
+ * @data: aemif chip select configuration
+ *
+ * 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 aemif_cs_data *data)
+{
+	int ta, rhold, rstrobe, rsetup, whold, wstrobe, wsetup;
+	unsigned offset;
+	u32 set, val;
+
+	offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
+
+	ta	= aemif_calc_rate(data->ta, aemif->clk_rate, TA_MAX);
+	rhold	= aemif_calc_rate(data->rhold, aemif->clk_rate,
+					  RHOLD_MAX);
+	rstrobe	= aemif_calc_rate(data->rstrobe, aemif->clk_rate,
+					  RSTROBE_MAX);
+	rsetup	= aemif_calc_rate(data->rsetup, aemif->clk_rate,
+					  RSETUP_MAX);
+	whold	= aemif_calc_rate(data->whold, aemif->clk_rate,
+					  WHOLD_MAX);
+	wstrobe	= aemif_calc_rate(data->wstrobe, aemif->clk_rate,
+					  WSTROBE_MAX);
+	wsetup	= aemif_calc_rate(data->wsetup, aemif->clk_rate,
+					  WSETUP_MAX);
+
+	if (ta < 0 || rhold < 0 || rstrobe < 0 || rsetup < 0 ||
+			whold < 0 || wstrobe < 0 || wsetup < 0) {
+		dev_err(aemif->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)
+{
+	return ((val + 1) * NSEC_PER_MSEC) / aemif->clk_rate;
+}
+
+/**
+ * aemif_get_hw_params - function to read hw register values
+ * @data: ptr to cs data
+ *
+ * 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 aemif_cs_data *data)
+{
+	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));
+	data->rhold = aemif_cycles_to_nsec(RHOLD_VAL(val));
+	data->rstrobe = aemif_cycles_to_nsec(RSTROBE_VAL(val));
+	data->rsetup = aemif_cycles_to_nsec(RSETUP_VAL(val));
+	data->whold = aemif_cycles_to_nsec(WHOLD_VAL(val));
+	data->wstrobe = aemif_cycles_to_nsec(WSTROBE_VAL(val));
+	data->wsetup = aemif_cycles_to_nsec(WSETUP_VAL(val));
+	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
+ * @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 device_node *np)
+{
+	struct aemif_cs_data *data;
+	unsigned long cs;
+	u32 val;
+
+	if (kstrtoul(np->name + 2, 10, &cs) < 0) {
+		dev_dbg(aemif->dev, "cs name is incorrect");
+		return -EINVAL;
+	}
+
+	if (cs - aemif->cs_offset >= NUM_CS || cs < aemif->cs_offset) {
+		dev_dbg(aemif->dev, "cs number is incorrect %lu", cs);
+		return -EINVAL;
+	}
+
+	if (aemif->num_cs >= NUM_CS) {
+		dev_dbg(aemif->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(data);
+
+	/* override the values from device node */
+	of_property_read_u8(np, "ti,cs-ta", &data->ta);
+	of_property_read_u8(np, "ti,cs-rhold", &data->rhold);
+	of_property_read_u16(np, "ti,cs-rstrobe", &data->rstrobe);
+	of_property_read_u8(np, "ti,cs-rsetup", &data->rsetup);
+	of_property_read_u8(np, "ti,cs-whold", &data->whold);
+	of_property_read_u16(np, "ti,cs-wstrobe", &data->wstrobe);
+	of_property_read_u8(np, "ti,cs-wsetup", &data->wsetup);
+	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,keystone-aemif", },
+	{ .compatible = "ti,omap-L138-aemif", },
+	{},
+};
+
+static const struct of_device_id cs_of_match[] = {
+	{ .compatible = "ti,aemif-cs", },
+	{},
+};
+
+static int aemif_probe(struct platform_device *pdev)
+{
+	int ret  = -ENODEV, i;
+	struct resource *res;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	if (np == NULL)
+		return 0;
+
+	if (aemif) {
+		dev_err(dev, "ti aemif driver is in use currently\n");
+		return -EBUSY;
+	}
+
+	aemif = devm_kzalloc(dev, sizeof(*aemif), GFP_KERNEL);
+
+	if (!aemif) {
+		dev_err(dev, "cannot allocate memory for aemif\n");
+		return -ENOMEM;
+	}
+
+	aemif->clk = devm_clk_get(dev, "aemif");
+	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;
+
+	aemif->dev = dev;
+
+	if (of_device_is_compatible(np, "ti,omap-L138-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_matching_node(np, of_match_ptr(cs_of_match)) {
+		ret = of_aemif_parse_abus_config(np);
+		if (ret < 0)
+			goto error;
+	}
+
+	for (i = 0; i < aemif->num_cs; i++) {
+		ret = aemif_config_abus(&aemif->cs_data[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_matching_node(np, of_match_ptr(cs_of_match)) {
+		ret = of_platform_populate(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)
+{
+	clk_disable_unprepare(aemif->clk);
+	return 0;
+}
+
+static struct platform_driver aemif_driver = {
+	.probe = aemif_probe,
+	.remove = aemif_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.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:" DRV_NAME);
-- 
1.7.9.5

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

[Ivan Khoronzhuk]

Add bindings for AEMIF controller drivers/memory/ti-aemif.c

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 .../bindings/memory-controllers/ti-aemif.txt       |  198 ++++++++++++++++++++
 1 file changed, 198 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..be0c0cb
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
@@ -0,0 +1,198 @@
+* Device tree bindings for Texas instruments AEMIF controller
+
+Th 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 - 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,omap-L138-aemif"
+
+- #address-cells:	Must be 2. The first cell is the memory partition
+			number. The 0 partition is for chip selects. And the
+			second cell is the offset into the partition, for the 0
+			partition it corresponds to chip select offset.
+
+- #size-cells:		Must be set to 1.
+
+- reg:			contains offset/length value for AEMIF control registers
+			space.
+
+- ranges:		Must be set up to reflect the memory layout for 4
+			chipselects and for AEMIF control range.
+
+- clocks:		phandle reference to 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:
+
+- compatible:		"ti,aemif-cs"
+
+- #address-cells:	Must be 2. The first cell is the memory partition
+			number. The 0 partition is for chip selects. And the
+			second cell is the offset into the partition, for the 0
+			partition it corresponds to chip select offset.
+
+- #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.
+
+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.
+
+The name for cs node must be in format csN, where N is the cs number.
+For compatibles "ti,davinci-aemif" and "ti,keystone-aemif" N = [0-3].
+For compatible "ti,omap-L138-aemif" N = [2-5].
+
+Example for aemif, davinci nand and nor flash chip select shown below.
+
+memory-controller@21000A00 {
+	compatible = "ti,keystone-aemif";
+	#address-cells = <2>;
+	#size-cells = <1>;
+	clocks = <&clkaemif 0>;
+	clock-names = "aemif";
+	clock-ranges;
+	reg = <0x2100A00 0x00000100>;
+	ranges = <0 0 0x70000000 0x10000000
+		  1 0 0x21000A00 0x0000100>;
+
+	nand:cs2 {
+		compatible = "ti,aemif-cs";
+		#address-cells = <2>;
+		#size-cells = <1>;
+		clock-ranges;
+		ranges;
+
+		/* 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>;
+
+			.. see davinci-nand.txt
+		};
+	};
+
+	nor:cs0 {
+		compatible = "ti,aemif-cs";
+		#address-cells = <2>;
+		#size-cells = <1>;
+		clock-ranges;
+		ranges;
+
+		/* 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.7.9.5

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

[Jean-Christophe PLAGNIOL-VILLARD]

> +				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.
> +
> +The name for cs node must be in format csN, where N is the cs number.

this is wired we should use reg instead to represent the cs as done for SPI
or a an other property

Best Regards,
J.

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

[ivan.khoronzhuk]

On 11/20/2013 08:21 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> +				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.
>> +
>> +The name for cs node must be in format csN, where N is the cs number.
> 
> this is wired we should use reg instead to represent the cs as done for SPI
> or a an other property
> 
> Best Regards,
> J.
> 

Ok, I will add new property cs-chipselect like following :

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,omap-L138-aemif" range is [2-5].

Is it OK?

-- 
Regards,
Ivan Khoronzhuk

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 21:03 Wed 20 Nov     , ivan.khoronzhuk wrote:
> On 11/20/2013 08:21 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >> +				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.
> >> +
> >> +The name for cs node must be in format csN, where N is the cs number.
> > 
> > this is wired we should use reg instead to represent the cs as done for SPI
> > or a an other property
> > 
> > Best Regards,
> > J.
> > 
> 
> Ok, I will add new property cs-chipselect like following :
> 
> 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,omap-L138-aemif" range is [2-5].
> 
> Is it OK?

yes

I just have one issue the whole memory concept

for me we should do as done on pinctrl have a phandle on the device that
require it and handle it at device core level

as the memory controller is not necessarely on the same bus as the memory
device them selves

Best Regards,
J.
> 
> -- 
> Regards,
> Ivan Khoronzhuk

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

[Kumar Gala]

On Nov 20, 2013, at 9:46 AM, Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> wrote:

> Add bindings for AEMIF controller drivers/memory/ti-aemif.c
> 

Binding shouldn’t normally refer to code.

Just saying something like:

Adding binging for TI Async External Memory Interface (AEMIF) controller.


> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
> .../bindings/memory-controllers/ti-aemif.txt       |  198 ++++++++++++++++++++
> 1 file changed, 198 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..be0c0cb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
> @@ -0,0 +1,198 @@
> +* Device tree bindings for Texas instruments AEMIF controller
> +
> +Th Async External Memory Interface (EMIF16/AEMIF) controller is intended to

The?

> +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 - 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,omap-L138-aemif"
> +
> +- #address-cells:	Must be 2. The first cell is the memory partition
> +			number. The 0 partition is for chip selects. And the
> +			second cell is the offset into the partition, for the 0
> +			partition it corresponds to chip select offset.
> +

Is the first cell just the chip select number?

I’m wondering if this is similar to FSL Localbus controller, see the binding:

bindings/powerpc/fsl/lbc.txt

> +- #size-cells:		Must be set to 1.
> +
> +- reg:			contains offset/length value for AEMIF control registers
> +			space.
> +
> +- ranges:		Must be set up to reflect the memory layout for 4
> +			chipselects and for AEMIF control range.
> +
> +- clocks:		phandle reference to 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:
> +
> +- compatible:		"ti,aemif-cs"
> +
> +- #address-cells:	Must be 2. The first cell is the memory partition
> +			number. The 0 partition is for chip selects. And the
> +			second cell is the offset into the partition, for the 0
> +			partition it corresponds to chip select offset.
> +
> +- #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.
> +
> +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.
> +
> +The name for cs node must be in format csN, where N is the cs number.
> +For compatibles "ti,davinci-aemif" and "ti,keystone-aemif" N = [0-3].
> +For compatible "ti,omap-L138-aemif" N = [2-5].
> +
> +Example for aemif, davinci nand and nor flash chip select shown below.
> +
> +memory-controller@21000A00 {
> +	compatible = "ti,keystone-aemif";
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +	clocks = <&clkaemif 0>;
> +	clock-names = "aemif";
> +	clock-ranges;
> +	reg = <0x2100A00 0x00000100>;
> +	ranges = <0 0 0x70000000 0x10000000
> +		  1 0 0x21000A00 0x0000100>;
> +
> +	nand:cs2 {
> +		compatible = "ti,aemif-cs";
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		clock-ranges;
> +		ranges;
> +
> +		/* 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>;
> +
> +			.. see davinci-nand.txt
> +		};
> +	};
> +
> +	nor:cs0 {
> +		compatible = "ti,aemif-cs";
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		clock-ranges;
> +		ranges;
> +
> +		/* 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.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Kumar Gala]

On Nov 20, 2013, at 1:03 PM, ivan.khoronzhuk <ivan.khoronzhuk@ti.com> wrote:

> On 11/20/2013 08:21 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> +				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.
>>> +
>>> +The name for cs node must be in format csN, where N is the cs number.
>> 
>> this is wired we should use reg instead to represent the cs as done for SPI
>> or a an other property
>> 
>> Best Regards,
>> J.
>> 
> 
> Ok, I will add new property cs-chipselect like following :
> 
> 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,omap-L138-aemif" range is [2-5].
> 
> Is it OK?

Why do you need this? As it was mentioned just use reg:

So you’d have something like:

memory-controller@21000A00 {
	…
	nand:cs2@2 {
		reg = <2 0 0>;
		ranges;
		...

	}:
};

However, I’m confused by the example in which you have:

+		nand@0,0x8000000 {
+			compatible = "ti,davinci-nand";
+			reg = <0 0x8000000 0x4000000
+			       1 0x0000000 0x0000100>;
+
+			.. see davinci-nand.txt
+		};

What chipselects is this on 0 & 1?

- k
			
> 
> -- 
> Regards,
> Ivan Khoronzhuk
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Grygorii Strashko]

On 11/22/2013 11:04 PM, Kumar Gala wrote:
> 
> On Nov 20, 2013, at 9:46 AM, Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> wrote:
> 
>> Add bindings for AEMIF controller drivers/memory/ti-aemif.c
>>
> 
> Binding shouldn’t normally refer to code.
> 
> Just saying something like:
> 
> Adding binging for TI Async External Memory Interface (AEMIF) controller.
> 
> 
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>> .../bindings/memory-controllers/ti-aemif.txt       |  198 ++++++++++++++++++++
>> 1 file changed, 198 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..be0c0cb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
>> @@ -0,0 +1,198 @@
>> +* Device tree bindings for Texas instruments AEMIF controller
>> +
>> +Th Async External Memory Interface (EMIF16/AEMIF) controller is intended to
> 
> The?
> 
>> +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 - 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,omap-L138-aemif"
>> +
>> +- #address-cells:	Must be 2. The first cell is the memory partition
>> +			number. The 0 partition is for chip selects. And the
>> +			second cell is the offset into the partition, for the 0
>> +			partition it corresponds to chip select offset.
>> +
> 
> Is the first cell just the chip select number?

No. It's rather memory range/partition number. Now there are 2 partitions:
- control partition which is common for all CS interfaces
- CS-specific partition/range
(this one can be splitted according to specific SoC requirement)

As per Keystone TCI6638K2K
 Datasheet http://www.ti.com/lit/ds/sprs836d/sprs836d.pdf:

1) the memory range 0 will be from 0x30000000 size 0x10000000:
00 3000 0000 - 00 33FF FFFF 64M EMIF16 CE0
00 3400 0000 - 00 37FF FFFF 64M EMIF16 CE1
00 3800 0000 - 00 3BFF FFFF 64M EMIF16 CE2
00 3C00 0000 - 00 3FFF FFFF 64M EMIF16 CE3

2) the memory range 1:
00 2100 0A00 - 00 2100 0AFF 256 AEMIF Config

And AEMIF node contains definition:
ranges = <0 0 0x30000000 0x10000000
	  1 0 0x21000A00 0x0000100>;


Child node has (nand):
 reg = <0 0 0x4000000 (cs0)
        - or - 0 0x4000000 0x4000000 (cs1)
        - or - 0 0x8000000 0x4000000 (cs2)
	- or - 0 0xC000000 0x4000000 (cs3)
	- and -
        1 0 0x0000100>; (for all cs)

For example for cs2 child node the resulting mem range 0 will be calculated as

from: 0x30000000 + (0 0x8000000 - 0 0)
size: 0x4000000

We don't encode CS number in reg/ranges, because it will allow simply change 
AEMIF DT definitions depending on each SoC
(AEMIF CS memory range can be continuous as above or not, if not - additional
range/partition can be added and child device can select the proper one).

> 
> I’m wondering if this is similar to FSL Localbus controller, see the binding:
> 
> bindings/powerpc/fsl/lbc.txt

Don't think so :) it looks similar just because of using standard bindings
(gpmc-nand.c mvebu-devbus.txt ti-gpmc.txt and etc.)

> 
>> +- #size-cells:		Must be set to 1.
>> +
>> +- reg:			contains offset/length value for AEMIF control registers
>> +			space.
>> +
>> +- ranges:		Must be set up to reflect the memory layout for 4
>> +			chipselects and for AEMIF control range.
>> +
>> +- clocks:		phandle reference to the controller clock. Required only
>> +			if clock tree data present in device tree.
>> +			See clock-bindings.txt
[..]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Regards,
-grygorii

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

[ivan.khoronzhuk]

On 11/22/2013 11:04 PM, Kumar Gala wrote:
>
> On Nov 20, 2013, at 9:46 AM, Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> wrote:
>
>> Add bindings for AEMIF controller drivers/memory/ti-aemif.c
>>
>
> Binding shouldn’t normally refer to code.
>
> Just saying something like:
>
> Adding binging for TI Async External Memory Interface (AEMIF) controller.
>
>

Ok

>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>> .../bindings/memory-controllers/ti-aemif.txt       |  198 ++++++++++++++++++++
>> 1 file changed, 198 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..be0c0cb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
>> @@ -0,0 +1,198 @@
>> +* Device tree bindings for Texas instruments AEMIF controller
>> +
>> +Th Async External Memory Interface (EMIF16/AEMIF) controller is intended to
>
> The?
>

The

>> +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 - 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,omap-L138-aemif"
>> +
>> +- #address-cells:	Must be 2. The first cell is the memory partition
>> +			number. The 0 partition is for chip selects. And the
>> +			second cell is the offset into the partition, for the 0
>> +			partition it corresponds to chip select offset.
>> +
>
> Is the first cell just the chip select number?
>
> I’m wondering if this is similar to FSL Localbus controller, see the binding:
>
> bindings/powerpc/fsl/lbc.txt
>

No, it is a memory range number

>> +- #size-cells:		Must be set to 1.
>> +
>> +- reg:			contains offset/length value for AEMIF control registers
>> +			space.
>> +
>> +- ranges:		Must be set up to reflect the memory layout for 4
>> +			chipselects and for AEMIF control range.
>> +
>> +- clocks:		phandle reference to 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:
>> +
>> +- compatible:		"ti,aemif-cs"
>> +
>> +- #address-cells:	Must be 2. The first cell is the memory partition
>> +			number. The 0 partition is for chip selects. And the
>> +			second cell is the offset into the partition, for the 0
>> +			partition it corresponds to chip select offset.
>> +
>> +- #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.
>> +
>> +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.
>> +
>> +The name for cs node must be in format csN, where N is the cs number.
>> +For compatibles "ti,davinci-aemif" and "ti,keystone-aemif" N = [0-3].
>> +For compatible "ti,omap-L138-aemif" N = [2-5].
>> +
>> +Example for aemif, davinci nand and nor flash chip select shown below.
>> +
>> +memory-controller@21000A00 {
>> +	compatible = "ti,keystone-aemif";
>> +	#address-cells = <2>;
>> +	#size-cells = <1>;
>> +	clocks = <&clkaemif 0>;
>> +	clock-names = "aemif";
>> +	clock-ranges;
>> +	reg = <0x2100A00 0x00000100>;
>> +	ranges = <0 0 0x70000000 0x10000000
>> +		  1 0 0x21000A00 0x0000100>;
>> +
>> +	nand:cs2 {
>> +		compatible = "ti,aemif-cs";
>> +		#address-cells = <2>;
>> +		#size-cells = <1>;
>> +		clock-ranges;
>> +		ranges;
>> +
>> +		/* 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>;
>> +
>> +			.. see davinci-nand.txt
>> +		};
>> +	};
>> +
>> +	nor:cs0 {
>> +		compatible = "ti,aemif-cs";
>> +		#address-cells = <2>;
>> +		#size-cells = <1>;
>> +		clock-ranges;
>> +		ranges;
>> +
>> +		/* 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.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Regards,
Ivan Khoronzhuk

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

[ivan.khoronzhuk]

On 11/22/2013 11:06 PM, Kumar Gala wrote:
>
> On Nov 20, 2013, at 1:03 PM, ivan.khoronzhuk <ivan.khoronzhuk@ti.com> wrote:
>
>> On 11/20/2013 08:21 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>> +				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.
>>>> +
>>>> +The name for cs node must be in format csN, where N is the cs number.
>>>
>>> this is wired we should use reg instead to represent the cs as done for SPI
>>> or a an other property
>>>
>>> Best Regards,
>>> J.
>>>
>>
>> Ok, I will add new property cs-chipselect like following :
>>
>> 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,omap-L138-aemif" range is [2-5].
>>
>> Is it OK?
>
> Why do you need this? As it was mentioned just use reg:
>
> So you’d have something like:
>
> memory-controller@21000A00 {
> 	…
> 	nand:cs2@2 {
> 		reg = <2 0 0>;
> 		ranges;
> 		...
>
> 	}:
> };
>
> However, I’m confused by the example in which you have:
>
> +		nand@0,0x8000000 {
> +			compatible = "ti,davinci-nand";
> +			reg = <0 0x8000000 0x4000000
It means, use memory of 0 AEMIF range beginning from 0x800000 (started 
from beginning of this range) and with size 0x4000000
> +			       1 0x0000000 0x0000100>;
It means, use memory of 1 AEMIF range + 0x0000000 and with size 0x0000100
> +
> +			.. see davinci-nand.txt
> +		};
>
> What chipselects is this on 0 & 1?
Any of them is not cs number. 0 - is just a memory range number.
>
> - k
>

The ranges property provides a means of defining a mapping or 
translation between the address space of the AEMIF and the address space 
of the cs nodes.

The reg property describes the address of the cs's resources within the 
address space defined by AEMIF. The reg is the memory range, not cs number.

See Grygorii Starshko comment on it, he described it very well.
			
>>
>> --
>> Regards,
>> Ivan Khoronzhuk
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Regards,
Ivan Khoronzhuk

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

[Grygorii Strashko]

Hi Jean-Christophe,

On 11/22/2013 08:42 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 21:03 Wed 20 Nov     , ivan.khoronzhuk wrote:
>> On 11/20/2013 08:21 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>> +				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.
>>>> +
>>>> +The name for cs node must be in format csN, where N is the cs number.
>>>
>>> this is wired we should use reg instead to represent the cs as done for SPI
>>> or a an other property
>>>
>>> Best Regards,
>>> J.
>>>
>>
>> Ok, I will add new property cs-chipselect like following :
>>
>> 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,omap-L138-aemif" range is [2-5].
>>
>> Is it OK?
> 
> yes
> 
> I just have one issue the whole memory concept
> 
> for me we should do as done on pinctrl have a phandle on the device that
> require it and handle it at device core level
> 
> as the memory controller is not necessarely on the same bus as the memory
> device them selves

Could you clarify your point a bit, pls?
Are you talking about external ASRAM, NOR and NAND chips wired to CS interface?

Regards,
- grygorii

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

[Grygorii Strashko]

Hi Kumar Gala,

On 11/22/2013 11:06 PM, Kumar Gala wrote:
> 
> On Nov 20, 2013, at 1:03 PM, ivan.khoronzhuk <ivan.khoronzhuk@ti.com> wrote:
> 
>> On 11/20/2013 08:21 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>> +				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.
>>>> +
>>>> +The name for cs node must be in format csN, where N is the cs number.
>>>
>>> this is wired we should use reg instead to represent the cs as done for SPI
>>> or a an other property
>>>
>>> Best Regards,
>>> J.
>>>
>>
>> Ok, I will add new property cs-chipselect like following :
>>
>> 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,omap-L138-aemif" range is [2-5].
>>
>> Is it OK?
> 
> Why do you need this? As it was mentioned just use reg:
> 
> So you’d have something like:
> 
> memory-controller@21000A00 {
> 	…
> 	nand:cs2@2 {
> 		reg = <2 0 0>;
> 		ranges;
> 		...
> 
> 	}:
> };

I'd prefer to continue with "ti,cs-chipselect" (this is more human friendly definition, as for me),
but if you insist - it can be changed as:
memory-controller@21000A00 {
	compatible = "ti,keystone-aemif";
...

	cs2 {
		compatible = "ti,aemif-cs";
		reg = <2>;
...
	}

	cs0 {
		compatible = "ti,aemif-cs";
		reg = <0>;
...
	}

> 
> However, I’m confused by the example in which you have:
> 
> +		nand@0,0x8000000 {
> +			compatible = "ti,davinci-nand";
> +			reg = <0 0x8000000 0x4000000
> +			       1 0x0000000 0x0000100>;
> +
> +			.. see davinci-nand.txt
> +		};
> 
> What chipselects is this on 0 & 1?

As I described in https://lkml.org/lkml/2013/11/26/282 we are not encoding CS number in reg
 - it's memory partition number.

Also, I'd like to note that we *DO NOT introduce* NAND device bindings here.
The Davinci NAND bindings was introduced and accepted more then one year ago, and
we've just updated its a bit (keeping full compatibility) and reused
(see https://lkml.org/lkml/2013/11/21/182). 
And the CS number is encoded for Davinci NAND node using standalone property
"ti,davinci-chipselect" and we need to provide (2) 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.

Regards,
- grygorii

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

[Santosh Shilimkar]

On Friday 29 November 2013 09:56 AM, Grygorii Strashko wrote:
> Hi Jean-Christophe,
> 
> On 11/22/2013 08:42 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> On 21:03 Wed 20 Nov     , ivan.khoronzhuk wrote:
>>> On 11/20/2013 08:21 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>> +				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.
>>>>> +
>>>>> +The name for cs node must be in format csN, where N is the cs number.
>>>>
>>>> this is wired we should use reg instead to represent the cs as done for SPI
>>>> or a an other property
>>>>
>>>> Best Regards,
>>>> J.
>>>>
>>>
>>> Ok, I will add new property cs-chipselect like following :
>>>
>>> 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,omap-L138-aemif" range is [2-5].
>>>
>>> Is it OK?
>>
>> yes
>>
>> I just have one issue the whole memory concept
>>
>> for me we should do as done on pinctrl have a phandle on the device that
>> require it and handle it at device core level
>>
>> as the memory controller is not necessarely on the same bus as the memory
>> device them selves
> 
> Could you clarify your point a bit, pls?
> Are you talking about external ASRAM, NOR and NAND chips wired to CS interface?
> 
Thats probably what he means.
The mtd devices can be interface with different memory controllers having different
layouts to set-up timing registers. If these layouts were standard, then it would
have been possible to manage the information at the core layers.

Regards,
Santosh

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

[Santosh Shilimkar]

On Friday 29 November 2013 10:00 AM, Grygorii Strashko wrote:
> Hi Kumar Gala,
> 
> On 11/22/2013 11:06 PM, Kumar Gala wrote:
>>
>> On Nov 20, 2013, at 1:03 PM, ivan.khoronzhuk <ivan.khoronzhuk@ti.com> wrote:
>>
>>> On 11/20/2013 08:21 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>> +				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.
>>>>> +
>>>>> +The name for cs node must be in format csN, where N is the cs number.
>>>>
>>>> this is wired we should use reg instead to represent the cs as done for SPI
>>>> or a an other property
>>>>
>>>> Best Regards,
>>>> J.
>>>>
>>>
>>> Ok, I will add new property cs-chipselect like following :
>>>
>>> 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,omap-L138-aemif" range is [2-5].
>>>
>>> Is it OK?
>>
>> Why do you need this? As it was mentioned just use reg:
>>
>> So you’d have something like:
>>
>> memory-controller@21000A00 {
>> 	…
>> 	nand:cs2@2 {
>> 		reg = <2 0 0>;
>> 		ranges;
>> 		...
>>
>> 	}:
>> };
> 
> I'd prefer to continue with "ti,cs-chipselect" (this is more human friendly definition, as for me),
> but if you insist - it can be changed as:
> memory-controller@21000A00 {
> 	compatible = "ti,keystone-aemif";
> ...
> 
> 	cs2 {
> 		compatible = "ti,aemif-cs";
> 		reg = <2>;
> ...
> 	}
> 
> 	cs0 {
> 		compatible = "ti,aemif-cs";
> 		reg = <0>;
> ...
> 	}
> 
>>
>> However, I’m confused by the example in which you have:
>>
>> +		nand@0,0x8000000 {
>> +			compatible = "ti,davinci-nand";
>> +			reg = <0 0x8000000 0x4000000
>> +			       1 0x0000000 0x0000100>;
>> +
>> +			.. see davinci-nand.txt
>> +		};
>>
>> What chipselects is this on 0 & 1?
> 
> As I described in https://lkml.org/lkml/2013/11/26/282 we are not encoding CS number in reg
>  - it's memory partition number.
> 
> Also, I'd like to note that we *DO NOT introduce* NAND device bindings here.
> The Davinci NAND bindings was introduced and accepted more then one year ago, and
> we've just updated its a bit (keeping full compatibility) and reused
> (see https://lkml.org/lkml/2013/11/21/182). 
> And the CS number is encoded for Davinci NAND node using standalone property
> "ti,davinci-chipselect" and we need to provide (2) 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.
> 
The above should have been really captured in the commit log to get a better
picture. No way on earth, a reviewer can figure out whether this is new bindings
or copy of bindings already used.

Regards,
Santosh

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

[Santosh Shilimkar]

On Wednesday 20 November 2013 10:46 AM, Ivan Khoronzhuk wrote:
> 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 four chip selects with 64M byte access
> per chip select.
> 
Please add the number of CS info here.

> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR
> are not supported.
> 
> This controller is used on SoCs like Davinci, Keysone2
>

 
> 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
>
This information probably can go below ---
 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
>  drivers/memory/Kconfig    |   11 ++
>  drivers/memory/Makefile   |    1 +
>  drivers/memory/ti-aemif.c |  415 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 427 insertions(+)
>  create mode 100644 drivers/memory/ti-aemif.c
> 
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index 29a11db..cc0e3c8 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -7,6 +7,17 @@ menuconfig MEMORY
>  
>  if MEMORY
>  
> +config TI_AEMIF
> +	bool "Texas Instruments AEMIF driver"
Driver can be build as loadable module as well so fix above.

> +	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..a4b479a
> --- /dev/null
> +++ b/drivers/memory/ti-aemif.c
> @@ -0,0 +1,415 @@
> +/*
> + * TI AEMIF driver
> + *
> + * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/
> + * Copyright (C) Heiko Schocher <hs@denx.de>
> + * Copyright (C) Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> + *
You mean author above, right, Please fix that for both emails ids.

> + * 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)
> +
> +#define DRV_NAME	"ti-aemif"
> +
> +/**
> + * structure to hold cs parameters
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;
> +};
> +
> +/**
> + * structure to hold device data
Prefix structure name here as suggested.

> + * @base: base address of AEMIF registers
> + * @clk: source clock
> + * @clk_rate: clock's rate in kHz
> + * @num_cs: number of assigned chip-selects
> + * @cs_data: array of chip-select settings
> + */
> +struct aemif_device {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct clk *clk;
> +	unsigned long clk_rate;
> +	u8 num_cs;
> +	int cs_offset;
> +	struct aemif_cs_data cs_data[NUM_CS];
> +};
> +
> +static struct aemif_device *aemif;
Add one extra line here. From other comment looks
like you are going to get rid of this global which will be
nice.

> +/**
> + * aemif_calc_rate - calculate timing data.
> + * @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(int wanted, unsigned long clk, int max)
> +{
> +	int result;
> +
> +	result = DIV_ROUND_UP((wanted * clk), NSEC_PER_MSEC) - 1;
> +
> +	dev_dbg(aemif->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
> + * @data: aemif chip select configuration
> + *
> + * 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 aemif_cs_data *data)
> +{
> +	int ta, rhold, rstrobe, rsetup, whold, wstrobe, wsetup;
> +	unsigned offset;
> +	u32 set, val;
> +
> +	offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
> +
> +	ta	= aemif_calc_rate(data->ta, aemif->clk_rate, TA_MAX);
> +	rhold	= aemif_calc_rate(data->rhold, aemif->clk_rate,
> +					  RHOLD_MAX);
> +	rstrobe	= aemif_calc_rate(data->rstrobe, aemif->clk_rate,
> +					  RSTROBE_MAX);
> +	rsetup	= aemif_calc_rate(data->rsetup, aemif->clk_rate,
> +					  RSETUP_MAX);
> +	whold	= aemif_calc_rate(data->whold, aemif->clk_rate,
> +					  WHOLD_MAX);
> +	wstrobe	= aemif_calc_rate(data->wstrobe, aemif->clk_rate,
> +					  WSTROBE_MAX);
> +	wsetup	= aemif_calc_rate(data->wsetup, aemif->clk_rate,
> +					  WSETUP_MAX);
> +
> +	if (ta < 0 || rhold < 0 || rstrobe < 0 || rsetup < 0 ||
> +			whold < 0 || wstrobe < 0 || wsetup < 0) {
> +		dev_err(aemif->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)
> +{
> +	return ((val + 1) * NSEC_PER_MSEC) / aemif->clk_rate;
> +}
> +
> +/**
> + * aemif_get_hw_params - function to read hw register values
> + * @data: ptr to cs data
> + *
> + * 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 aemif_cs_data *data)
> +{
> +	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));
> +	data->rhold = aemif_cycles_to_nsec(RHOLD_VAL(val));
> +	data->rstrobe = aemif_cycles_to_nsec(RSTROBE_VAL(val));
> +	data->rsetup = aemif_cycles_to_nsec(RSETUP_VAL(val));
> +	data->whold = aemif_cycles_to_nsec(WHOLD_VAL(val));
> +	data->wstrobe = aemif_cycles_to_nsec(WSTROBE_VAL(val));
> +	data->wsetup = aemif_cycles_to_nsec(WSETUP_VAL(val));
> +	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
> + * @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 device_node *np)
> +{
> +	struct aemif_cs_data *data;
> +	unsigned long cs;
> +	u32 val;
> +
> +	if (kstrtoul(np->name + 2, 10, &cs) < 0) {
> +		dev_dbg(aemif->dev, "cs name is incorrect");
> +		return -EINVAL;
> +	}
> +
> +	if (cs - aemif->cs_offset >= NUM_CS || cs < aemif->cs_offset) {
> +		dev_dbg(aemif->dev, "cs number is incorrect %lu", cs);
> +		return -EINVAL;
> +	}
> +
> +	if (aemif->num_cs >= NUM_CS) {
> +		dev_dbg(aemif->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(data);
> +
> +	/* override the values from device node */
> +	of_property_read_u8(np, "ti,cs-ta", &data->ta);
> +	of_property_read_u8(np, "ti,cs-rhold", &data->rhold);
> +	of_property_read_u16(np, "ti,cs-rstrobe", &data->rstrobe);
> +	of_property_read_u8(np, "ti,cs-rsetup", &data->rsetup);
> +	of_property_read_u8(np, "ti,cs-whold", &data->whold);
> +	of_property_read_u16(np, "ti,cs-wstrobe", &data->wstrobe);
> +	of_property_read_u8(np, "ti,cs-wsetup", &data->wsetup);
> +	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,keystone-aemif", },
> +	{ .compatible = "ti,omap-L138-aemif", },
> +	{},
> +};
> +
Looks like you are yet to update the patches from
previous comments. Did I miss v2 or you haven't posted 
that yet ?

Regards,
Santosh 

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

[Grygorii Strashko]

On 11/29/2013 05:32 PM, Santosh Shilimkar wrote:
> On Wednesday 20 November 2013 10:46 AM, Ivan Khoronzhuk wrote:
>> 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 four chip selects with 64M byte access

[...]

>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id aemif_of_match[] = {
>> +	{ .compatible = "ti,davinci-aemif", },
>> +	{ .compatible = "ti,keystone-aemif", },
>> +	{ .compatible = "ti,omap-L138-aemif", },
>> +	{},
>> +};
>> +
> Looks like you are yet to update the patches from
> previous comments. Did I miss v2 or you haven't posted
> that yet ?

No. This is v2, actually :( (the mess was already noticed) it was posted 
after v1 (https://lkml.org/lkml/2013/11/11/352), but people continue 
commenting v1, for some reasons.

Next version will be posted when bindings will be clarified finally.

Regards,
-grygorii

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

[Santosh Shilimkar]

On Friday 29 November 2013 10:35 AM, Grygorii Strashko wrote:
> On 11/29/2013 05:32 PM, Santosh Shilimkar wrote:
>> On Wednesday 20 November 2013 10:46 AM, Ivan Khoronzhuk wrote:
>>> 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 four chip selects with 64M byte access
> 
> [...]
> 
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct of_device_id aemif_of_match[] = {
>>> +    { .compatible = "ti,davinci-aemif", },
>>> +    { .compatible = "ti,keystone-aemif", },
>>> +    { .compatible = "ti,omap-L138-aemif", },
>>> +    {},
>>> +};
>>> +
>> Looks like you are yet to update the patches from
>> previous comments. Did I miss v2 or you haven't posted
>> that yet ?
> 
> No. This is v2, actually :( (the mess was already noticed) it was posted after v1 (https://lkml.org/lkml/2013/11/11/352), but people continue commenting v1, for some reasons.
>
OK.
 
> Next version will be posted when bindings will be clarified finally.
> 
Yeah. lets get the binding sorted out first and send the update
with all the comments included.

Regards,
Santosh

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

[ivan.khoronzhuk]

On 11/29/2013 05:32 PM, Santosh Shilimkar wrote:
> On Wednesday 20 November 2013 10:46 AM, Ivan Khoronzhuk wrote:
>> 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 four chip selects with 64M byte access
>> per chip select.
>>
> Please add the number of CS info here.
> 

Ok

>> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR
>> are not supported.
>>
>> This controller is used on SoCs like Davinci, Keysone2
>>
> 
>   
>> 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
>>
> This information probably can go below ---
>

Ok

>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>>   drivers/memory/Kconfig    |   11 ++
>>   drivers/memory/Makefile   |    1 +
>>   drivers/memory/ti-aemif.c |  415 +++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 427 insertions(+)
>>   create mode 100644 drivers/memory/ti-aemif.c
>>
>> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
>> index 29a11db..cc0e3c8 100644
>> --- a/drivers/memory/Kconfig
>> +++ b/drivers/memory/Kconfig
>> @@ -7,6 +7,17 @@ menuconfig MEMORY
>>   
>>   if MEMORY
>>   
>> +config TI_AEMIF
>> +	bool "Texas Instruments AEMIF driver"
> Driver can be build as loadable module as well so fix above.
> 

Ok

>> +	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..a4b479a
>> --- /dev/null
>> +++ b/drivers/memory/ti-aemif.c
>> @@ -0,0 +1,415 @@
>> +/*
>> + * TI AEMIF driver
>> + *
>> + * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/
>> + * Copyright (C) Heiko Schocher <hs@denx.de>
>> + * Copyright (C) Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> + *
> You mean author above, right, Please fix that for both emails ids.
> 

Ok

>> + * 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)
>> +
>> +#define DRV_NAME	"ti-aemif"
>> +
>> +/**
>> + * structure to hold cs parameters
> struct aemif_cs_data : structure to hold cs parameters

Ok

>> + * @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;
>> +};
>> +
>> +/**
>> + * structure to hold device data
> Prefix structure name here as suggested.
>

Ok
 
>> + * @base: base address of AEMIF registers
>> + * @clk: source clock
>> + * @clk_rate: clock's rate in kHz
>> + * @num_cs: number of assigned chip-selects
>> + * @cs_data: array of chip-select settings
>> + */
>> +struct aemif_device {
>> +	struct device *dev;
>> +	void __iomem *base;
>> +	struct clk *clk;
>> +	unsigned long clk_rate;
>> +	u8 num_cs;
>> +	int cs_offset;
>> +	struct aemif_cs_data cs_data[NUM_CS];
>> +};
>> +
>> +static struct aemif_device *aemif;
> Add one extra line here. From other comment looks
> like you are going to get rid of this global which will be
> nice.
> 

Yes

>> +/**
>> + * aemif_calc_rate - calculate timing data.
>> + * @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(int wanted, unsigned long clk, int max)
>> +{
>> +	int result;
>> +
>> +	result = DIV_ROUND_UP((wanted * clk), NSEC_PER_MSEC) - 1;
>> +
>> +	dev_dbg(aemif->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
>> + * @data: aemif chip select configuration
>> + *
>> + * 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 aemif_cs_data *data)
>> +{
>> +	int ta, rhold, rstrobe, rsetup, whold, wstrobe, wsetup;
>> +	unsigned offset;
>> +	u32 set, val;
>> +
>> +	offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
>> +
>> +	ta	= aemif_calc_rate(data->ta, aemif->clk_rate, TA_MAX);
>> +	rhold	= aemif_calc_rate(data->rhold, aemif->clk_rate,
>> +					  RHOLD_MAX);
>> +	rstrobe	= aemif_calc_rate(data->rstrobe, aemif->clk_rate,
>> +					  RSTROBE_MAX);
>> +	rsetup	= aemif_calc_rate(data->rsetup, aemif->clk_rate,
>> +					  RSETUP_MAX);
>> +	whold	= aemif_calc_rate(data->whold, aemif->clk_rate,
>> +					  WHOLD_MAX);
>> +	wstrobe	= aemif_calc_rate(data->wstrobe, aemif->clk_rate,
>> +					  WSTROBE_MAX);
>> +	wsetup	= aemif_calc_rate(data->wsetup, aemif->clk_rate,
>> +					  WSETUP_MAX);
>> +
>> +	if (ta < 0 || rhold < 0 || rstrobe < 0 || rsetup < 0 ||
>> +			whold < 0 || wstrobe < 0 || wsetup < 0) {
>> +		dev_err(aemif->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)
>> +{
>> +	return ((val + 1) * NSEC_PER_MSEC) / aemif->clk_rate;
>> +}
>> +
>> +/**
>> + * aemif_get_hw_params - function to read hw register values
>> + * @data: ptr to cs data
>> + *
>> + * 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 aemif_cs_data *data)
>> +{
>> +	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));
>> +	data->rhold = aemif_cycles_to_nsec(RHOLD_VAL(val));
>> +	data->rstrobe = aemif_cycles_to_nsec(RSTROBE_VAL(val));
>> +	data->rsetup = aemif_cycles_to_nsec(RSETUP_VAL(val));
>> +	data->whold = aemif_cycles_to_nsec(WHOLD_VAL(val));
>> +	data->wstrobe = aemif_cycles_to_nsec(WSTROBE_VAL(val));
>> +	data->wsetup = aemif_cycles_to_nsec(WSETUP_VAL(val));
>> +	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
>> + * @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 device_node *np)
>> +{
>> +	struct aemif_cs_data *data;
>> +	unsigned long cs;
>> +	u32 val;
>> +
>> +	if (kstrtoul(np->name + 2, 10, &cs) < 0) {
>> +		dev_dbg(aemif->dev, "cs name is incorrect");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (cs - aemif->cs_offset >= NUM_CS || cs < aemif->cs_offset) {
>> +		dev_dbg(aemif->dev, "cs number is incorrect %lu", cs);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (aemif->num_cs >= NUM_CS) {
>> +		dev_dbg(aemif->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(data);
>> +
>> +	/* override the values from device node */
>> +	of_property_read_u8(np, "ti,cs-ta", &data->ta);
>> +	of_property_read_u8(np, "ti,cs-rhold", &data->rhold);
>> +	of_property_read_u16(np, "ti,cs-rstrobe", &data->rstrobe);
>> +	of_property_read_u8(np, "ti,cs-rsetup", &data->rsetup);
>> +	of_property_read_u8(np, "ti,cs-whold", &data->whold);
>> +	of_property_read_u16(np, "ti,cs-wstrobe", &data->wstrobe);
>> +	of_property_read_u8(np, "ti,cs-wsetup", &data->wsetup);
>> +	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,keystone-aemif", },
>> +	{ .compatible = "ti,omap-L138-aemif", },
>> +	{},
>> +};
>> +
> Looks like you are yet to update the patches from
> previous comments. Did I miss v2 or you haven't posted
> that yet ?
> 
> Regards,
> Santosh
> 

FYI: The part of suggestions had been mentioned by Sekhar Nori
at https://lkml.org/lkml/2013/11/26/44.

I has added your corrections. I'll send updated series after the bindings
are clarified finally.

-- 
Regards,
Ivan Khoronzhuk

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

[ivan.khoronzhuk]

On 11/29/2013 05:10 PM, Santosh Shilimkar wrote:
> On Friday 29 November 2013 10:00 AM, Grygorii Strashko wrote:
>> Hi Kumar Gala,
>>
>> On 11/22/2013 11:06 PM, Kumar Gala wrote:
>>>
>>> On Nov 20, 2013, at 1:03 PM, ivan.khoronzhuk <ivan.khoronzhuk@ti.com> wrote:
>>>
>>>> On 11/20/2013 08:21 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>>>> +				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.
>>>>>> +
>>>>>> +The name for cs node must be in format csN, where N is the cs number.
>>>>>
>>>>> this is wired we should use reg instead to represent the cs as done for SPI
>>>>> or a an other property
>>>>>
>>>>> Best Regards,
>>>>> J.
>>>>>
>>>>
>>>> Ok, I will add new property cs-chipselect like following :
>>>>
>>>> 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,omap-L138-aemif" range is [2-5].
>>>>
>>>> Is it OK?
>>>
>>> Why do you need this? As it was mentioned just use reg:
>>>
>>> So you’d have something like:
>>>
>>> memory-controller@21000A00 {
>>> 	…
>>> 	nand:cs2@2 {
>>> 		reg = <2 0 0>;
>>> 		ranges;
>>> 		...
>>>
>>> 	}:
>>> };
>>
>> I'd prefer to continue with "ti,cs-chipselect" (this is more human friendly definition, as for me),
>> but if you insist - it can be changed as:
>> memory-controller@21000A00 {
>> 	compatible = "ti,keystone-aemif";
>> ...
>>
>> 	cs2 {
>> 		compatible = "ti,aemif-cs";
>> 		reg = <2>;
>> ...
>> 	}
>>
>> 	cs0 {
>> 		compatible = "ti,aemif-cs";
>> 		reg = <0>;
>> ...
>> 	}
>>
>>>
>>> However, I’m confused by the example in which you have:
>>>
>>> +		nand@0,0x8000000 {
>>> +			compatible = "ti,davinci-nand";
>>> +			reg = <0 0x8000000 0x4000000
>>> +			       1 0x0000000 0x0000100>;
>>> +
>>> +			.. see davinci-nand.txt
>>> +		};
>>>
>>> What chipselects is this on 0 & 1?
>>
>> As I described in https://lkml.org/lkml/2013/11/26/282 we are not encoding CS number in reg
>>   - it's memory partition number.
>>
>> Also, I'd like to note that we *DO NOT introduce* NAND device bindings here.
>> The Davinci NAND bindings was introduced and accepted more then one year ago, and
>> we've just updated its a bit (keeping full compatibility) and reused
>> (see https://lkml.org/lkml/2013/11/21/182).
>> And the CS number is encoded for Davinci NAND node using standalone property
>> "ti,davinci-chipselect" and we need to provide (2) 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.
>>
> The above should have been really captured in the commit log to get a better
> picture. No way on earth, a reviewer can figure out whether this is new bindings
> or copy of bindings already used.
>
> Regards,
> Santosh
>

Ok, I will add it.

-- 
Regards,
Ivan Khoronzhuk

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

[Santosh Shilimkar]

Kumar,

On Tuesday 26 November 2013 11:27 AM, Grygorii Strashko wrote:
> On 11/22/2013 11:04 PM, Kumar Gala wrote:
>>
>> On Nov 20, 2013, at 9:46 AM, Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> wrote:
>>
>>> Add bindings for AEMIF controller drivers/memory/ti-aemif.c
>>>
>>
>> Binding shouldn’t normally refer to code.
>>
>> Just saying something like:
>>
>> Adding binging for TI Async External Memory Interface (AEMIF) controller.
>>
>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>> ---
>>> .../bindings/memory-controllers/ti-aemif.txt       |  198 ++++++++++++++++++++
>>> 1 file changed, 198 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..be0c0cb
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
>>> @@ -0,0 +1,198 @@
>>> +* Device tree bindings for Texas instruments AEMIF controller
>>> +
>>> +Th Async External Memory Interface (EMIF16/AEMIF) controller is intended to
>>
>> The?
>>
>>> +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 - 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,omap-L138-aemif"
>>> +
>>> +- #address-cells:	Must be 2. The first cell is the memory partition
>>> +			number. The 0 partition is for chip selects. And the
>>> +			second cell is the offset into the partition, for the 0
>>> +			partition it corresponds to chip select offset.
>>> +
>>
>> Is the first cell just the chip select number?
> 
> No. It's rather memory range/partition number. Now there are 2 partitions:
> - control partition which is common for all CS interfaces
> - CS-specific partition/range
> (this one can be splitted according to specific SoC requirement)
> 
> As per Keystone TCI6638K2K
>  Datasheet http://www.ti.com/lit/ds/sprs836d/sprs836d.pdf:
> 
> 1) the memory range 0 will be from 0x30000000 size 0x10000000:
> 00 3000 0000 - 00 33FF FFFF 64M EMIF16 CE0
> 00 3400 0000 - 00 37FF FFFF 64M EMIF16 CE1
> 00 3800 0000 - 00 3BFF FFFF 64M EMIF16 CE2
> 00 3C00 0000 - 00 3FFF FFFF 64M EMIF16 CE3
> 
> 2) the memory range 1:
> 00 2100 0A00 - 00 2100 0AFF 256 AEMIF Config
> 
> And AEMIF node contains definition:
> ranges = <0 0 0x30000000 0x10000000
> 	  1 0 0x21000A00 0x0000100>;
> 
> 
> Child node has (nand):
>  reg = <0 0 0x4000000 (cs0)
>         - or - 0 0x4000000 0x4000000 (cs1)
>         - or - 0 0x8000000 0x4000000 (cs2)
> 	- or - 0 0xC000000 0x4000000 (cs3)
> 	- and -
>         1 0 0x0000100>; (for all cs)
> 
> For example for cs2 child node the resulting mem range 0 will be calculated as
> 
> from: 0x30000000 + (0 0x8000000 - 0 0)
> size: 0x4000000
> 
> We don't encode CS number in reg/ranges, because it will allow simply change 
> AEMIF DT definitions depending on each SoC
> (AEMIF CS memory range can be continuous as above or not, if not - additional
> range/partition can be added and child device can select the proper one).
> 
>>
>> I’m wondering if this is similar to FSL Localbus controller, see the binding:
>>
>> bindings/powerpc/fsl/lbc.txt
> 
> Don't think so :) it looks similar just because of using standard bindings
> (gpmc-nand.c mvebu-devbus.txt ti-gpmc.txt and etc.)
> 
>>
>>> +- #size-cells:		Must be set to 1.
>>> +
>>> +- reg:			contains offset/length value for AEMIF control registers
>>> +			space.
>>> +
>>> +- ranges:		Must be set up to reflect the memory layout for 4
>>> +			chipselects and for AEMIF control range.
>>> +
>>> +- clocks:		phandle reference to the controller clock. Required only
>>> +			if clock tree data present in device tree.
>>> +			See clock-bindings.txt

Are you ok with above description on why cs property is needed. We will need
your ack ;-) to take these patches forward.

Regards,
Santosh

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

[Kumar Gala]

On Nov 26, 2013, at 10:27 AM, Grygorii Strashko <grygorii.strashko@ti.com> wrote:

> On 11/22/2013 11:04 PM, Kumar Gala wrote:
>> 
>> On Nov 20, 2013, at 9:46 AM, Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> wrote:
>> 
>>> Add bindings for AEMIF controller drivers/memory/ti-aemif.c
>>> 
>> 
>> Binding shouldn’t normally refer to code.
>> 
>> Just saying something like:
>> 
>> Adding binging for TI Async External Memory Interface (AEMIF) controller.
>> 
>> 
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>> ---
>>> .../bindings/memory-controllers/ti-aemif.txt       |  198 ++++++++++++++++++++
>>> 1 file changed, 198 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..be0c0cb
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
>>> @@ -0,0 +1,198 @@
>>> +* Device tree bindings for Texas instruments AEMIF controller
>>> +
>>> +Th Async External Memory Interface (EMIF16/AEMIF) controller is intended to
>> 
>> The?
>> 
>>> +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 - 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,omap-L138-aemif"
>>> +
>>> +- #address-cells:	Must be 2. The first cell is the memory partition
>>> +			number. The 0 partition is for chip selects. And the
>>> +			second cell is the offset into the partition, for the 0
>>> +			partition it corresponds to chip select offset.
>>> +
>> 
>> Is the first cell just the chip select number?
> 
> No. It's rather memory range/partition number. Now there are 2 partitions:
> - control partition which is common for all CS interfaces
> - CS-specific partition/range
> (this one can be splitted according to specific SoC requirement)
> 
> As per Keystone TCI6638K2K
> Datasheet http://www.ti.com/lit/ds/sprs836d/sprs836d.pdf:
> 
> 1) the memory range 0 will be from 0x30000000 size 0x10000000:
> 00 3000 0000 - 00 33FF FFFF 64M EMIF16 CE0
> 00 3400 0000 - 00 37FF FFFF 64M EMIF16 CE1
> 00 3800 0000 - 00 3BFF FFFF 64M EMIF16 CE2
> 00 3C00 0000 - 00 3FFF FFFF 64M EMIF16 CE3
> 
> 2) the memory range 1:
> 00 2100 0A00 - 00 2100 0AFF 256 AEMIF Config
> 
> And AEMIF node contains definition:
> ranges = <0 0 0x30000000 0x10000000
> 	  1 0 0x21000A00 0x0000100>;
> 
> 
> Child node has (nand):
> reg = <0 0 0x4000000 (cs0)
>        - or - 0 0x4000000 0x4000000 (cs1)
>        - or - 0 0x8000000 0x4000000 (cs2)
> 	- or - 0 0xC000000 0x4000000 (cs3)
> 	- and -
>        1 0 0x0000100>; (for all cs)
> 
> For example for cs2 child node the resulting mem range 0 will be calculated as
> 
> from: 0x30000000 + (0 0x8000000 - 0 0)
> size: 0x4000000
> 
> We don't encode CS number in reg/ranges, because it will allow simply change 
> AEMIF DT definitions depending on each SoC
> (AEMIF CS memory range can be continuous as above or not, if not - additional
> range/partition can be added and child device can select the proper one).

This is quite confusing.  I think the ranges property needs far more description as part of the top level controller node.  It spec out what the various fields are in the ranges property.

- k

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

[ivan.khoronzhuk]

On 12/10/2013 01:09 AM, Kumar Gala wrote:
> 
> On Nov 26, 2013, at 10:27 AM, Grygorii Strashko <grygorii.strashko@ti.com> wrote:
> 
>> On 11/22/2013 11:04 PM, Kumar Gala wrote:
>>>
>>> On Nov 20, 2013, at 9:46 AM, Ivan Khoronzhuk <ivan.khoronzhuk@ti.com> wrote:
>>>
>>>> Add bindings for AEMIF controller drivers/memory/ti-aemif.c
>>>>
>>>
>>> Binding shouldn’t normally refer to code.
>>>
>>> Just saying something like:
>>>
>>> Adding binging for TI Async External Memory Interface (AEMIF) controller.
>>>
>>>
>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>> ---
>>>> .../bindings/memory-controllers/ti-aemif.txt       |  198 ++++++++++++++++++++
>>>> 1 file changed, 198 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..be0c0cb
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
>>>> @@ -0,0 +1,198 @@
>>>> +* Device tree bindings for Texas instruments AEMIF controller
>>>> +
>>>> +Th Async External Memory Interface (EMIF16/AEMIF) controller is intended to
>>>
>>> The?
>>>
>>>> +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 - 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,omap-L138-aemif"
>>>> +
>>>> +- #address-cells:	Must be 2. The first cell is the memory partition
>>>> +			number. The 0 partition is for chip selects. And the
>>>> +			second cell is the offset into the partition, for the 0
>>>> +			partition it corresponds to chip select offset.
>>>> +
>>>
>>> Is the first cell just the chip select number?
>>
>> No. It's rather memory range/partition number. Now there are 2 partitions:
>> - control partition which is common for all CS interfaces
>> - CS-specific partition/range
>> (this one can be splitted according to specific SoC requirement)
>>
>> As per Keystone TCI6638K2K
>> Datasheet http://www.ti.com/lit/ds/sprs836d/sprs836d.pdf:
>>
>> 1) the memory range 0 will be from 0x30000000 size 0x10000000:
>> 00 3000 0000 - 00 33FF FFFF 64M EMIF16 CE0
>> 00 3400 0000 - 00 37FF FFFF 64M EMIF16 CE1
>> 00 3800 0000 - 00 3BFF FFFF 64M EMIF16 CE2
>> 00 3C00 0000 - 00 3FFF FFFF 64M EMIF16 CE3
>>
>> 2) the memory range 1:
>> 00 2100 0A00 - 00 2100 0AFF 256 AEMIF Config
>>
>> And AEMIF node contains definition:
>> ranges = <0 0 0x30000000 0x10000000
>> 	  1 0 0x21000A00 0x0000100>;
>>
>>
>> Child node has (nand):
>> reg = <0 0 0x4000000 (cs0)
>>         - or - 0 0x4000000 0x4000000 (cs1)
>>         - or - 0 0x8000000 0x4000000 (cs2)
>> 	- or - 0 0xC000000 0x4000000 (cs3)
>> 	- and -
>>         1 0 0x0000100>; (for all cs)
>>
>> For example for cs2 child node the resulting mem range 0 will be calculated as
>>
>> from: 0x30000000 + (0 0x8000000 - 0 0)
>> size: 0x4000000
>>
>> We don't encode CS number in reg/ranges, because it will allow simply change
>> AEMIF DT definitions depending on each SoC
>> (AEMIF CS memory range can be continuous as above or not, if not - additional
>> range/partition can be added and child device can select the proper one).
> 
> This is quite confusing.  I think the ranges property needs far more description as part of the top level controller node.  It spec out what the various fields are in the ranges property.
> 

Yes, you are right, description is quite poor. I should describe it more clearly

-- 
Regards,
Ivan Khoronzhuk