[PATCH v6 0/2] memory: add Atmel EBI (External Bus Interface) driver

[PATCH v6 0/2] memory: add Atmel EBI (External Bus Interface) driver

[Boris Brezillon]

The EBI (External Bus Interface) is used to access external peripherals
(NOR, SRAM, NAND, and other specific devices like ethernet controllers).
Each device is assigned a CS line and an address range and can have its
own configuration (timings, access mode, bus width, ...).
This driver provides a generic DT binding to configure a device according
to its requirements.
For specific device controllers (like the NAND one) the SMC timings
should be configured by the controller driver through the matrix and
smc syscon regmaps (or maybe we should provide an API for that?).

Changes since v5:
- remove the "atmel,specialized-logic" property: now all devices are
  are attached to the SMC logic by default, and specialized drivers
  (like the NAND controller one), should change this configuration
  manually.
- provide hardware readout to allow partial config description.
  This is mainly here to keep existing platforms (where everything
  is configured by the bootloader/bootstrap) in working state.
- rename "atmel,tdf-optimized" into "atmel,tdf-mode" and switch from
  a boolean to string property to properly support partial config

Changes since v4:
- fix inconsistencies in SMC and MATRIX registers definition
- add missing compatible strings for at91sam9rl SoC
- fix DT bindings documentation
- replace "atmel,generic-dev" property by "atmel,specialized-logic"

Changes since v3:
- added AT91_MATRIX_USBPUCR_PUON definition
- removed useless macros (those directly returning SoC specific register
  offsets)
- use syscon_regmap_lookup_by_phandle instead of of_parse_phandle +
  syscon_node_to_regmap
- drop AT91_EBICSA_REGFIELD and AT91_MULTI_EBICSA_REGFIELD macros

Changes since v2:
- minor fixes int DT bindings doc
- fix SMC macros
- make use of SMC macros defined in include/linux/mfd/syscon/atmel-smc.h

Changes since v1:
- almost everything :-)

Boris Brezillon (2):
  memory: add Atmel EBI (External Bus Interface) driver
  memory: atmel-ebi: add DT bindings documentation

 .../bindings/memory-controllers/atmel,ebi.txt      | 146 +++++
 drivers/memory/Kconfig                             |  11 +
 drivers/memory/Makefile                            |   1 +
 drivers/memory/atmel-ebi.c                         | 662 +++++++++++++++++++++
 4 files changed, 820 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
 create mode 100644 drivers/memory/atmel-ebi.c

-- 
2.7.4

[PATCH v6 1/2] memory: add Atmel EBI (External Bus Interface) driver

[Boris Brezillon]

The EBI (External Bus Interface) is used to access external peripherals
(NOR, SRAM, NAND, and other specific devices like ethernet controllers).
Each device is assigned a CS line and an address range and can have its
own configuration (timings, access mode, bus width, ...).
This driver provides a generic DT binding to configure a device according
to its requirements.
For specific device controllers (like the NAND one) the SMC timings
should be configured by the controller driver through the matrix and
smc syscon regmaps.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/memory/Kconfig     |  11 +
 drivers/memory/Makefile    |   1 +
 drivers/memory/atmel-ebi.c | 662 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 674 insertions(+)
 create mode 100644 drivers/memory/atmel-ebi.c

diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 51d5cd2..4780136 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -25,6 +25,17 @@ config ATMEL_SDRAMC
 	  Starting with the at91sam9g45, this controller supports SDR, DDR and
 	  LP-DDR memories.
 
+config ATMEL_EBI
+	bool "Atmel EBI driver"
+	default y
+	depends on ARCH_AT91 && OF
+	select MFD_SYSCON
+	help
+	  Driver for Atmel EBI controller.
+	  Used to configure the EBI (external bus interface) when the device-
+	  tree is used. This bus supports NANDs, external ethernet controller,
+	  SRAMs, ATA devices, etc.
+
 config TI_AEMIF
 	tristate "Texas Instruments AEMIF driver"
 	depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index 890bdf4..965da6b 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_OF)		+= of_memory.o
 endif
 obj-$(CONFIG_ARM_PL172_MPMC)	+= pl172.o
 obj-$(CONFIG_ATMEL_SDRAMC)	+= atmel-sdramc.o
+obj-$(CONFIG_ATMEL_EBI)		+= atmel-ebi.o
 obj-$(CONFIG_TI_AEMIF)		+= ti-aemif.o
 obj-$(CONFIG_TI_EMIF)		+= emif.o
 obj-$(CONFIG_OMAP_GPMC)		+= omap-gpmc.o
diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
new file mode 100644
index 0000000..2a72b53
--- /dev/null
+++ b/drivers/memory/atmel-ebi.c
@@ -0,0 +1,662 @@
+/*
+ * EBI driver for Atmel chips
+ * inspired by the fsl weim bus driver
+ *
+ * Copyright (C) 2013 JJ Hiblot.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mfd/syscon/atmel-matrix.h>
+#include <linux/mfd/syscon/atmel-smc.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+
+struct at91sam9_smc_timings {
+	u32 ncs_rd_setup_ns;
+	u32 nrd_setup_ns;
+	u32 ncs_wr_setup_ns;
+	u32 nwe_setup_ns;
+	u32 ncs_rd_pulse_ns;
+	u32 nrd_pulse_ns;
+	u32 ncs_wr_pulse_ns;
+	u32 nwe_pulse_ns;
+	u32 nrd_cycle_ns;
+	u32 nwe_cycle_ns;
+	u32 tdf_ns;
+};
+
+struct at91sam9_smc_generic_fields {
+	struct regmap_field *setup;
+	struct regmap_field *pulse;
+	struct regmap_field *cycle;
+	struct regmap_field *mode;
+};
+
+struct at91sam9_ebi_dev_config {
+	struct at91sam9_smc_timings timings;
+	u32 mode;
+};
+
+struct at91_ebi_dev_config {
+	union {
+		struct at91sam9_ebi_dev_config sam9;
+	};
+};
+
+struct at91_ebi;
+
+struct at91_ebi_dev {
+	struct device_node *np;
+	struct at91_ebi *ebi;
+	u32 mode;
+	int cs;
+	struct at91_ebi_dev_config config;
+};
+
+struct at91_ebi_caps {
+	unsigned int available_cs;
+	const struct reg_field *ebi_csa;
+	void (*get_config)(struct at91_ebi_dev *ebid,
+			   struct at91_ebi_dev_config *conf);
+	int (*xlate_config)(struct at91_ebi_dev *ebid,
+			    struct at91_ebi_dev_config *conf);
+	int (*apply_config)(struct at91_ebi_dev *ebid,
+			    struct at91_ebi_dev_config *conf);
+	int (*init)(struct at91_ebi *ebi);
+};
+
+struct at91_ebi {
+	struct clk *clk;
+	struct regmap *smc;
+	struct regmap *matrix;
+
+	struct regmap_field *ebi_csa;
+
+	struct device *dev;
+	const struct at91_ebi_caps *caps;
+	struct at91_ebi_dev *devs[AT91_MATRIX_EBI_NUM_CS];
+	void *priv;
+};
+
+static void at91sam9_ebi_get_config(struct at91_ebi_dev *ebid,
+				    struct at91_ebi_dev_config *conf)
+{
+	struct at91sam9_smc_generic_fields *fields = ebid->ebi->priv;
+	unsigned int clk_rate = clk_get_rate(ebid->ebi->clk);
+	struct at91sam9_ebi_dev_config *config = &conf->sam9;
+	struct at91sam9_smc_timings *timings = &config->timings;
+	unsigned int val;
+
+	regmap_fields_read(fields->mode, ebid->cs, &val);
+	config->mode = val & ~AT91_SMC_TDF;
+
+	val = (val & AT91_SMC_TDF) >> 16;
+	timings->tdf_ns = clk_rate * val;
+
+	regmap_fields_read(fields->setup, ebid->cs, &val);
+	timings->ncs_rd_setup_ns = (val >> 24) & 0x1f;
+	timings->ncs_rd_setup_ns += ((val >> 29) & 0x1) * 128;
+	timings->ncs_rd_setup_ns *= clk_rate;
+	timings->nrd_setup_ns = (val >> 16) & 0x1f;
+	timings->nrd_setup_ns += ((val >> 21) & 0x1) * 128;
+	timings->nrd_setup_ns *= clk_rate;
+	timings->ncs_wr_setup_ns = (val >> 8) & 0x1f;
+	timings->ncs_wr_setup_ns += ((val >> 13) & 0x1) * 128;
+	timings->ncs_wr_setup_ns *= clk_rate;
+	timings->nwe_setup_ns = val & 0x1f;
+	timings->nwe_setup_ns += ((val >> 5) & 0x1) * 128;
+	timings->nwe_setup_ns *= clk_rate;
+
+	regmap_fields_read(fields->pulse, ebid->cs, &val);
+	timings->ncs_rd_pulse_ns = (val >> 24) & 0x3f;
+	timings->ncs_rd_pulse_ns += ((val >> 30) & 0x1) * 256;
+	timings->ncs_rd_pulse_ns *= clk_rate;
+	timings->nrd_pulse_ns = (val >> 16) & 0x3f;
+	timings->nrd_pulse_ns += ((val >> 22) & 0x1) * 256;
+	timings->nrd_pulse_ns *= clk_rate;
+	timings->ncs_wr_pulse_ns = (val >> 8) & 0x3f;
+	timings->ncs_wr_pulse_ns += ((val >> 14) & 0x1) * 256;
+	timings->ncs_wr_pulse_ns *= clk_rate;
+	timings->nwe_pulse_ns = val & 0x3f;
+	timings->nwe_pulse_ns += ((val >> 6) & 0x1) * 256;
+	timings->nwe_pulse_ns *= clk_rate;
+
+	regmap_fields_read(fields->cycle, ebid->cs, &val);
+	timings->nrd_cycle_ns = (val >> 16) & 0x7f;
+	timings->nrd_cycle_ns += ((val >> 23) & 0x3) * 256;
+	timings->nrd_cycle_ns *= clk_rate;
+	timings->nwe_cycle_ns = val & 0x7f;
+	timings->nwe_cycle_ns += ((val >> 7) & 0x3) * 256;
+	timings->nwe_cycle_ns *= clk_rate;
+}
+
+static int at91sam9_smc_xslate_timings(struct at91_ebi_dev *ebid,
+				       struct at91sam9_smc_timings *timings)
+{
+	struct device_node *np = ebid->np;
+
+	of_property_read_u32(np, "atmel,ncs-rd-setup-ns",
+			     &timings->ncs_rd_setup_ns);
+	of_property_read_u32(np, "atmel,nrd-setup-ns",
+			     &timings->nrd_setup_ns);
+	of_property_read_u32(np, "atmel,ncs-wr-setup-ns",
+			     &timings->ncs_wr_setup_ns);
+	of_property_read_u32(np, "atmel,nwe-setup-ns",
+			     &timings->nwe_setup_ns);
+	of_property_read_u32(np, "atmel,ncs-rd-pulse-ns",
+			     &timings->ncs_rd_pulse_ns);
+	of_property_read_u32(np, "atmel,nrd-pulse-ns",
+			     &timings->nrd_pulse_ns);
+	of_property_read_u32(np, "atmel,ncs-wr-pulse-ns",
+			     &timings->ncs_wr_pulse_ns);
+	of_property_read_u32(np, "atmel,nwe-pulse-ns", &timings->nwe_pulse_ns);
+	of_property_read_u32(np, "atmel,nwe-cycle-ns", &timings->nwe_cycle_ns);
+	of_property_read_u32(np, "atmel,nrd-cycle-ns", &timings->nrd_cycle_ns);
+	of_property_read_u32(np, "atmel,tdf-ns", &timings->tdf_ns);
+
+	return 0;
+}
+
+static int at91sam9_ebi_xslate_config(struct at91_ebi_dev *ebid,
+				      struct at91_ebi_dev_config *conf)
+{
+	struct at91sam9_ebi_dev_config *config = &conf->sam9;
+	struct device_node *np = ebid->np;
+	const char *tmp_str;
+	u32 tmp;
+	int ret;
+
+	ret = of_property_read_u32(np, "atmel,bus-width", &tmp);
+	if (!ret) {
+		config->mode &= ~AT91_SMC_DBW;
+		switch (tmp) {
+		case 8:
+			config->mode |= AT91_SMC_DBW_8;
+			break;
+
+		case 16:
+			config->mode |= AT91_SMC_DBW_16;
+			break;
+
+		case 32:
+			config->mode |= AT91_SMC_DBW_32;
+			break;
+
+		default:
+			return -EINVAL;
+		}
+	}
+
+	tmp_str = NULL;
+	of_property_read_string(np, "atmel,tdf-mode", &tmp_str);
+	if (tmp_str) {
+		config->mode &= ~AT91_SMC_TDFMODE_OPTIMIZED;
+		if (!strcmp(tmp_str, "optimized"))
+			config->mode |= AT91_SMC_TDFMODE_OPTIMIZED;
+	}
+
+	tmp_str = NULL;
+	of_property_read_string(np, "atmel,byte-access-type", &tmp_str);
+	if (tmp_str) {
+		config->mode &= AT91_SMC_BAT;
+		if (!strcmp(tmp_str, "write"))
+			config->mode |= AT91_SMC_BAT_WRITE;
+	}
+
+	tmp_str = NULL;
+	of_property_read_string(np, "atmel,read-mode", &tmp_str);
+	if (tmp_str) {
+		config->mode &= ~AT91_SMC_READMODE;
+		if (!strcmp(tmp_str, "nrd"))
+			config->mode |= AT91_SMC_READMODE_NRD;
+	}
+
+	tmp_str = NULL;
+	of_property_read_string(np, "atmel,write-mode", &tmp_str);
+	if (tmp_str) {
+		config->mode &= ~AT91_SMC_WRITEMODE;
+		if (!strcmp(tmp_str, "nwe"))
+			config->mode |= AT91_SMC_WRITEMODE_NWE;
+	}
+
+	tmp_str = NULL;
+	of_property_read_string(np, "atmel,exnw-mode", &tmp_str);
+	if (tmp_str) {
+		config->mode &= ~AT91_SMC_EXNWMODE;
+		if (!strcmp(tmp_str, "frozen"))
+			config->mode |= AT91_SMC_EXNWMODE_FROZEN;
+		else if (!strcmp(tmp_str, "ready"))
+			config->mode |= AT91_SMC_EXNWMODE_READY;
+	}
+
+	tmp = 0;
+	ret = of_property_read_u32(np, "atmel,page-mode", &tmp);
+	if (!ret) {
+		config->mode &= ~AT91_SMC_PS;
+		switch (tmp) {
+		case 4:
+			config->mode |= AT91_SMC_PS_4;
+			break;
+
+		case 8:
+			config->mode |= AT91_SMC_PS_8;
+			break;
+
+		case 16:
+			config->mode |= AT91_SMC_PS_16;
+			break;
+
+		case 32:
+			config->mode |= AT91_SMC_PS_32;
+			break;
+
+		default:
+			return -EINVAL;
+		}
+
+		config->mode |= AT91_SMC_PMEN;
+	}
+
+	return at91sam9_smc_xslate_timings(ebid, &config->timings);
+}
+
+static int at91sam9_ebi_apply_config(struct at91_ebi_dev *ebid,
+				     struct at91_ebi_dev_config *conf)
+{
+	unsigned int clk_rate = clk_get_rate(ebid->ebi->clk);
+	struct at91sam9_ebi_dev_config *config = &conf->sam9;
+	struct at91sam9_smc_timings *timings = &config->timings;
+	struct at91sam9_smc_generic_fields *fields = ebid->ebi->priv;
+	u32 coded_val;
+	u32 val;
+
+	coded_val = at91sam9_smc_setup_ns_to_cycles(clk_rate,
+						    timings->ncs_rd_setup_ns);
+	val = AT91SAM9_SMC_NCS_NRDSETUP(coded_val);
+	coded_val = at91sam9_smc_setup_ns_to_cycles(clk_rate,
+						    timings->nrd_setup_ns);
+	val |= AT91SAM9_SMC_NRDSETUP(coded_val);
+	coded_val = at91sam9_smc_setup_ns_to_cycles(clk_rate,
+						    timings->ncs_wr_setup_ns);
+	val |= AT91SAM9_SMC_NCS_WRSETUP(coded_val);
+	coded_val = at91sam9_smc_setup_ns_to_cycles(clk_rate,
+						    timings->nwe_setup_ns);
+	val |= AT91SAM9_SMC_NWESETUP(coded_val);
+	regmap_fields_write(fields->setup, ebid->cs, val);
+
+	coded_val = at91sam9_smc_pulse_ns_to_cycles(clk_rate,
+						    timings->ncs_rd_pulse_ns);
+	val = AT91SAM9_SMC_NCS_NRDPULSE(coded_val);
+	coded_val = at91sam9_smc_pulse_ns_to_cycles(clk_rate,
+						    timings->nrd_pulse_ns);
+	val |= AT91SAM9_SMC_NRDPULSE(coded_val);
+	coded_val = at91sam9_smc_pulse_ns_to_cycles(clk_rate,
+						    timings->ncs_wr_pulse_ns);
+	val |= AT91SAM9_SMC_NCS_WRPULSE(coded_val);
+	coded_val = at91sam9_smc_pulse_ns_to_cycles(clk_rate,
+						    timings->nwe_pulse_ns);
+	val |= AT91SAM9_SMC_NWEPULSE(coded_val);
+	regmap_fields_write(fields->pulse, ebid->cs, val);
+
+	coded_val = at91sam9_smc_cycle_ns_to_cycles(clk_rate,
+						    timings->nrd_cycle_ns);
+	val = AT91SAM9_SMC_NRDCYCLE(coded_val);
+	coded_val = at91sam9_smc_cycle_ns_to_cycles(clk_rate,
+						    timings->nwe_cycle_ns);
+	val |= AT91SAM9_SMC_NWECYCLE(coded_val);
+	regmap_fields_write(fields->cycle, ebid->cs, val);
+
+	val = DIV_ROUND_UP(timings->tdf_ns, clk_rate);
+	if (val > AT91_SMC_TDF_MAX)
+		val = AT91_SMC_TDF_MAX;
+	regmap_fields_write(fields->mode, ebid->cs,
+			    config->mode | AT91_SMC_TDF_(val));
+
+	/*
+	 * Adjust config to what's actually configured in the hardware block.
+	 * Can be slightly different because of rounding policies.
+	 */
+	at91sam9_ebi_get_config(ebid, conf);
+
+	return 0;
+}
+
+
+static int at91sam9_ebi_init(struct at91_ebi *ebi)
+{
+	struct at91sam9_smc_generic_fields *fields;
+	struct reg_field field = REG_FIELD(0, 0, 31);
+
+	fields = devm_kzalloc(ebi->dev, sizeof(*fields), GFP_KERNEL);
+	if (!fields)
+		return -ENOMEM;
+
+	field.id_size = fls(ebi->caps->available_cs);
+	field.id_offset = AT91SAM9_SMC_GENERIC_BLK_SZ;
+
+	field.reg = AT91SAM9_SMC_SETUP(AT91SAM9_SMC_GENERIC);
+	fields->setup = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
+	if (IS_ERR(fields->setup))
+		return PTR_ERR(fields->setup);
+
+	field.reg = AT91SAM9_SMC_PULSE(AT91SAM9_SMC_GENERIC);
+	fields->pulse = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
+	if (IS_ERR(fields->pulse))
+		return PTR_ERR(fields->pulse);
+
+	field.reg = AT91SAM9_SMC_CYCLE(AT91SAM9_SMC_GENERIC);
+	fields->cycle = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
+	if (IS_ERR(fields->cycle))
+		return PTR_ERR(fields->cycle);
+
+	field.reg = AT91SAM9_SMC_MODE(AT91SAM9_SMC_GENERIC);
+	fields->mode = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
+	if (IS_ERR(fields->mode))
+		return PTR_ERR(fields->mode);
+
+	ebi->priv = fields;
+
+	return 0;
+}
+
+static int sama5d3_ebi_init(struct at91_ebi *ebi)
+{
+	struct at91sam9_smc_generic_fields *fields;
+	struct reg_field field = REG_FIELD(0, 0, 31);
+
+	fields = devm_kzalloc(ebi->dev, sizeof(*fields), GFP_KERNEL);
+	if (!fields)
+		return -ENOMEM;
+
+	field.id_size = fls(ebi->caps->available_cs);
+	field.id_offset = SAMA5_SMC_GENERIC_BLK_SZ;
+
+	field.reg = AT91SAM9_SMC_SETUP(SAMA5_SMC_GENERIC);
+	fields->setup = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
+	if (IS_ERR(fields->setup))
+		return PTR_ERR(fields->setup);
+
+	field.reg = AT91SAM9_SMC_PULSE(SAMA5_SMC_GENERIC);
+	fields->pulse = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
+	if (IS_ERR(fields->pulse))
+		return PTR_ERR(fields->pulse);
+
+	field.reg = AT91SAM9_SMC_CYCLE(SAMA5_SMC_GENERIC);
+	fields->cycle = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
+	if (IS_ERR(fields->cycle))
+		return PTR_ERR(fields->cycle);
+
+	field.reg = SAMA5_SMC_MODE(SAMA5_SMC_GENERIC);
+	fields->mode = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
+	if (IS_ERR(fields->mode))
+		return PTR_ERR(fields->mode);
+
+	ebi->priv = fields;
+
+	return 0;
+}
+
+static int at91_ebi_dev_setup(struct at91_ebi *ebi, struct device_node *np)
+{
+	const struct at91_ebi_caps *caps = ebi->caps;
+	struct at91_ebi_dev_config conf;
+	struct device *dev = ebi->dev;
+	struct device_node *dev_np;
+	struct at91_ebi_dev *ebid;
+	u32 tmp;
+	int ret;
+
+	dev_np = of_get_next_child(np, NULL);
+	if (!dev_np)
+		return -EINVAL;
+
+	if (!of_device_is_available(dev_np))
+		return 0;
+
+	ebid = devm_kzalloc(ebi->dev, sizeof(*ebid), GFP_KERNEL);
+	if (!ebid)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(dev_np, "reg", &tmp);
+	if (ret < 0) {
+		dev_err(dev, "missing mandatory reg property\n");
+		return ret;
+	}
+
+	if (tmp > AT91_MATRIX_EBI_NUM_CS ||
+	    !(BIT(tmp) & ebi->caps->available_cs)) {
+		dev_err(dev, "invalid reg property\n");
+		return -EINVAL;
+	}
+
+	ebid->cs = tmp;
+	ebid->np = np;
+	ebid->ebi = ebi;
+
+	/*
+	 * Attach the EBI device to the generic SMC logic.
+	 * FIXME: some drivers (like the NAND controller driver) might have
+	 * to change this config afterwards. We should expose a new API once
+	 * this requirement becomes a reality.
+	 */
+	if (ebi->ebi_csa)
+		regmap_field_update_bits(ebi->ebi_csa, BIT(ebid->cs), 0);
+
+	caps->get_config(ebid, &conf);
+
+	ret = caps->xlate_config(ebid, &conf);
+	if (ret)
+		return ret;
+
+	ret = caps->apply_config(ebid, &conf);
+	if (ret)
+		return ret;
+
+	ebid->config = conf;
+	ebi->devs[ebid->cs] = ebid;
+
+	return of_platform_populate(np, of_default_bus_match_table, NULL, dev);
+}
+
+static const struct reg_field at91sam9260_ebi_csa =
+				REG_FIELD(AT91SAM9260_MATRIX_EBICSA, 0,
+					  AT91_MATRIX_EBI_NUM_CS - 1);
+
+static const struct at91_ebi_caps at91sam9260_ebi_caps = {
+	.available_cs = 0xff,
+	.ebi_csa = &at91sam9260_ebi_csa,
+	.xlate_config = at91sam9_ebi_xslate_config,
+	.apply_config = at91sam9_ebi_apply_config,
+	.init = at91sam9_ebi_init,
+};
+
+static const struct reg_field at91sam9261_ebi_csa =
+				REG_FIELD(AT91SAM9261_MATRIX_EBICSA, 0,
+					  AT91_MATRIX_EBI_NUM_CS - 1);
+
+static const struct at91_ebi_caps at91sam9261_ebi_caps = {
+	.available_cs = 0xff,
+	.ebi_csa = &at91sam9261_ebi_csa,
+	.xlate_config = at91sam9_ebi_xslate_config,
+	.apply_config = at91sam9_ebi_apply_config,
+	.init = at91sam9_ebi_init,
+};
+
+static const struct reg_field at91sam9263_ebi0_csa =
+				REG_FIELD(AT91SAM9263_MATRIX_EBI0CSA, 0,
+					  AT91_MATRIX_EBI_NUM_CS - 1);
+
+static const struct at91_ebi_caps at91sam9263_ebi0_caps = {
+	.available_cs = 0x3f,
+	.ebi_csa = &at91sam9263_ebi0_csa,
+	.xlate_config = at91sam9_ebi_xslate_config,
+	.apply_config = at91sam9_ebi_apply_config,
+	.init = at91sam9_ebi_init,
+};
+
+static const struct reg_field at91sam9263_ebi1_csa =
+				REG_FIELD(AT91SAM9263_MATRIX_EBI1CSA, 0,
+					  AT91_MATRIX_EBI_NUM_CS - 1);
+
+static const struct at91_ebi_caps at91sam9263_ebi1_caps = {
+	.available_cs = 0x7,
+	.ebi_csa = &at91sam9263_ebi1_csa,
+	.xlate_config = at91sam9_ebi_xslate_config,
+	.apply_config = at91sam9_ebi_apply_config,
+	.init = at91sam9_ebi_init,
+};
+
+static const struct reg_field at91sam9rl_ebi_csa =
+				REG_FIELD(AT91SAM9RL_MATRIX_EBICSA, 0,
+					  AT91_MATRIX_EBI_NUM_CS - 1);
+
+static const struct at91_ebi_caps at91sam9rl_ebi_caps = {
+	.available_cs = 0x3f,
+	.ebi_csa = &at91sam9rl_ebi_csa,
+	.xlate_config = at91sam9_ebi_xslate_config,
+	.apply_config = at91sam9_ebi_apply_config,
+	.init = at91sam9_ebi_init,
+};
+
+static const struct reg_field at91sam9g45_ebi_csa =
+				REG_FIELD(AT91SAM9G45_MATRIX_EBICSA, 0,
+					  AT91_MATRIX_EBI_NUM_CS - 1);
+
+static const struct at91_ebi_caps at91sam9g45_ebi_caps = {
+	.available_cs = 0x3f,
+	.ebi_csa = &at91sam9g45_ebi_csa,
+	.xlate_config = at91sam9_ebi_xslate_config,
+	.apply_config = at91sam9_ebi_apply_config,
+	.init = at91sam9_ebi_init,
+};
+
+static const struct at91_ebi_caps at91sam9x5_ebi_caps = {
+	.available_cs = 0x3f,
+	.ebi_csa = &at91sam9263_ebi0_csa,
+	.xlate_config = at91sam9_ebi_xslate_config,
+	.apply_config = at91sam9_ebi_apply_config,
+	.init = at91sam9_ebi_init,
+};
+
+static const struct at91_ebi_caps sama5d3_ebi_caps = {
+	.available_cs = 0xf,
+	.xlate_config = at91sam9_ebi_xslate_config,
+	.apply_config = at91sam9_ebi_apply_config,
+	.init = sama5d3_ebi_init,
+};
+
+static const struct of_device_id at91_ebi_id_table[] = {
+	{
+		.compatible = "atmel,at91sam9260-ebi",
+		.data = &at91sam9260_ebi_caps,
+	},
+	{
+		.compatible = "atmel,at91sam9261-ebi",
+		.data = &at91sam9261_ebi_caps,
+	},
+	{
+		.compatible = "atmel,at91sam9263-ebi0",
+		.data = &at91sam9263_ebi0_caps,
+	},
+	{
+		.compatible = "atmel,at91sam9263-ebi1",
+		.data = &at91sam9263_ebi1_caps,
+	},
+	{
+		.compatible = "atmel,at91sam9rl-ebi",
+		.data = &at91sam9rl_ebi_caps,
+	},
+	{
+		.compatible = "atmel,at91sam9g45-ebi",
+		.data = &at91sam9g45_ebi_caps,
+	},
+	{
+		.compatible = "atmel,at91sam9x5-ebi",
+		.data = &at91sam9x5_ebi_caps,
+	},
+	{
+		.compatible = "atmel,sama5d3-ebi",
+		.data = &sama5d3_ebi_caps,
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, at91_ebi_id_table);
+
+static int at91_ebi_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	struct device_node *child;
+	struct at91_ebi *ebi;
+	struct clk *clk;
+	int ret;
+
+	match = of_match_device(at91_ebi_id_table, &pdev->dev);
+	if (!match || !match->data)
+		return -EINVAL;
+
+	ebi = devm_kzalloc(&pdev->dev, sizeof(*ebi), GFP_KERNEL);
+	if (!ebi)
+		return -ENOMEM;
+
+	ebi->caps = match->data;
+	ebi->dev = &pdev->dev;
+
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ebi->clk = clk;
+
+	ebi->smc = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+						   "atmel,smc");
+	if (IS_ERR(ebi->smc))
+		return PTR_ERR(ebi->smc);
+
+	/*
+	 * The sama5d3 does not provide an EBICSA register and thus does need
+	 * to access the matrix registers.
+	 */
+	if (ebi->caps->ebi_csa) {
+		ebi->matrix =
+			syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							"atmel,matrix");
+		if (IS_ERR(ebi->matrix))
+			return PTR_ERR(ebi->matrix);
+
+		ebi->ebi_csa = regmap_field_alloc(ebi->matrix,
+						  *ebi->caps->ebi_csa);
+		if (IS_ERR(ebi->ebi_csa))
+			return PTR_ERR(ebi->ebi_csa);
+	}
+
+	ret = ebi->caps->init(ebi);
+	if (ret)
+		return ret;
+
+	for_each_child_of_node(pdev->dev.of_node, child) {
+		ret = at91_ebi_dev_setup(ebi, child);
+		if (ret)
+			return ret;
+	}
+
+	return ret;
+}
+
+static struct platform_driver at91_ebi_driver = {
+	.driver = {
+		.name = "atmel-ebi",
+		.of_match_table	= at91_ebi_id_table,
+	},
+};
+module_platform_driver_probe(at91_ebi_driver, at91_ebi_probe);
+
+MODULE_AUTHOR("JJ Hiblot");
+MODULE_DESCRIPTION("Atmel EBI driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [PATCH v6 1/2] memory: add Atmel EBI (External Bus Interface) driver

[Jean-Jacques Hiblot]

2016-04-27 16:35 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> The EBI (External Bus Interface) is used to access external peripherals
> (NOR, SRAM, NAND, and other specific devices like ethernet controllers).
> Each device is assigned a CS line and an address range and can have its
> own configuration (timings, access mode, bus width, ...).
> This driver provides a generic DT binding to configure a device according
> to its requirements.
> For specific device controllers (like the NAND one) the SMC timings
> should be configured by the controller driver through the matrix and
> smc syscon regmaps.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/memory/Kconfig     |  11 +
>  drivers/memory/Makefile    |   1 +
>  drivers/memory/atmel-ebi.c | 662 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 674 insertions(+)
>  create mode 100644 drivers/memory/atmel-ebi.c
>
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index 51d5cd2..4780136 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -25,6 +25,17 @@ config ATMEL_SDRAMC
>           Starting with the at91sam9g45, this controller supports SDR, DDR and
>           LP-DDR memories.
>
> +config ATMEL_EBI
> +       bool "Atmel EBI driver"
> +       default y
> +       depends on ARCH_AT91 && OF
> +       select MFD_SYSCON
> +       help
> +         Driver for Atmel EBI controller.
> +         Used to configure the EBI (external bus interface) when the device-
> +         tree is used. This bus supports NANDs, external ethernet controller,
> +         SRAMs, ATA devices, etc.
> +
>  config TI_AEMIF
>         tristate "Texas Instruments AEMIF driver"
>         depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index 890bdf4..965da6b 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_OF)                += of_memory.o
>  endif
>  obj-$(CONFIG_ARM_PL172_MPMC)   += pl172.o
>  obj-$(CONFIG_ATMEL_SDRAMC)     += atmel-sdramc.o
> +obj-$(CONFIG_ATMEL_EBI)                += atmel-ebi.o
>  obj-$(CONFIG_TI_AEMIF)         += ti-aemif.o
>  obj-$(CONFIG_TI_EMIF)          += emif.o
>  obj-$(CONFIG_OMAP_GPMC)                += omap-gpmc.o
> diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
> new file mode 100644
> index 0000000..2a72b53
> --- /dev/null
> +++ b/drivers/memory/atmel-ebi.c
> @@ -0,0 +1,662 @@
> +/*
> + * EBI driver for Atmel chips
> + * inspired by the fsl weim bus driver
> + *
> + * Copyright (C) 2013 JJ Hiblot.
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mfd/syscon/atmel-matrix.h>
> +#include <linux/mfd/syscon/atmel-smc.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +
> +struct at91sam9_smc_timings {
> +       u32 ncs_rd_setup_ns;
> +       u32 nrd_setup_ns;
> +       u32 ncs_wr_setup_ns;
> +       u32 nwe_setup_ns;
> +       u32 ncs_rd_pulse_ns;
> +       u32 nrd_pulse_ns;
> +       u32 ncs_wr_pulse_ns;
> +       u32 nwe_pulse_ns;
> +       u32 nrd_cycle_ns;
> +       u32 nwe_cycle_ns;
> +       u32 tdf_ns;
> +};
> +
> +struct at91sam9_smc_generic_fields {
> +       struct regmap_field *setup;
> +       struct regmap_field *pulse;
> +       struct regmap_field *cycle;
> +       struct regmap_field *mode;
> +};
> +
> +struct at91sam9_ebi_dev_config {
> +       struct at91sam9_smc_timings timings;
> +       u32 mode;
> +};
> +
> +struct at91_ebi_dev_config {
> +       union {
> +               struct at91sam9_ebi_dev_config sam9;
> +       };
> +};
> +
> +struct at91_ebi;
> +
> +struct at91_ebi_dev {
> +       struct device_node *np;
> +       struct at91_ebi *ebi;
> +       u32 mode;
> +       int cs;
> +       struct at91_ebi_dev_config config;
> +};
> +
> +struct at91_ebi_caps {
> +       unsigned int available_cs;
> +       const struct reg_field *ebi_csa;
> +       void (*get_config)(struct at91_ebi_dev *ebid,
> +                          struct at91_ebi_dev_config *conf);
> +       int (*xlate_config)(struct at91_ebi_dev *ebid,
> +                           struct at91_ebi_dev_config *conf);
> +       int (*apply_config)(struct at91_ebi_dev *ebid,
> +                           struct at91_ebi_dev_config *conf);
> +       int (*init)(struct at91_ebi *ebi);
> +};
> +
> +struct at91_ebi {
> +       struct clk *clk;
> +       struct regmap *smc;
> +       struct regmap *matrix;
> +
> +       struct regmap_field *ebi_csa;
> +
> +       struct device *dev;
> +       const struct at91_ebi_caps *caps;
> +       struct at91_ebi_dev *devs[AT91_MATRIX_EBI_NUM_CS];
> +       void *priv;
> +};
> +
> +static void at91sam9_ebi_get_config(struct at91_ebi_dev *ebid,
> +                                   struct at91_ebi_dev_config *conf)
> +{
> +       struct at91sam9_smc_generic_fields *fields = ebid->ebi->priv;
> +       unsigned int clk_rate = clk_get_rate(ebid->ebi->clk);
> +       struct at91sam9_ebi_dev_config *config = &conf->sam9;
> +       struct at91sam9_smc_timings *timings = &config->timings;
> +       unsigned int val;
> +
> +       regmap_fields_read(fields->mode, ebid->cs, &val);
> +       config->mode = val & ~AT91_SMC_TDF;
> +
> +       val = (val & AT91_SMC_TDF) >> 16;
> +       timings->tdf_ns = clk_rate * val;
> +
> +       regmap_fields_read(fields->setup, ebid->cs, &val);
> +       timings->ncs_rd_setup_ns = (val >> 24) & 0x1f;
> +       timings->ncs_rd_setup_ns += ((val >> 29) & 0x1) * 128;
> +       timings->ncs_rd_setup_ns *= clk_rate;
> +       timings->nrd_setup_ns = (val >> 16) & 0x1f;
> +       timings->nrd_setup_ns += ((val >> 21) & 0x1) * 128;
> +       timings->nrd_setup_ns *= clk_rate;
> +       timings->ncs_wr_setup_ns = (val >> 8) & 0x1f;
> +       timings->ncs_wr_setup_ns += ((val >> 13) & 0x1) * 128;
> +       timings->ncs_wr_setup_ns *= clk_rate;
> +       timings->nwe_setup_ns = val & 0x1f;
> +       timings->nwe_setup_ns += ((val >> 5) & 0x1) * 128;
> +       timings->nwe_setup_ns *= clk_rate;
> +
> +       regmap_fields_read(fields->pulse, ebid->cs, &val);
> +       timings->ncs_rd_pulse_ns = (val >> 24) & 0x3f;
> +       timings->ncs_rd_pulse_ns += ((val >> 30) & 0x1) * 256;
> +       timings->ncs_rd_pulse_ns *= clk_rate;
> +       timings->nrd_pulse_ns = (val >> 16) & 0x3f;
> +       timings->nrd_pulse_ns += ((val >> 22) & 0x1) * 256;
> +       timings->nrd_pulse_ns *= clk_rate;
> +       timings->ncs_wr_pulse_ns = (val >> 8) & 0x3f;
> +       timings->ncs_wr_pulse_ns += ((val >> 14) & 0x1) * 256;
> +       timings->ncs_wr_pulse_ns *= clk_rate;
> +       timings->nwe_pulse_ns = val & 0x3f;
> +       timings->nwe_pulse_ns += ((val >> 6) & 0x1) * 256;
> +       timings->nwe_pulse_ns *= clk_rate;
> +
> +       regmap_fields_read(fields->cycle, ebid->cs, &val);
> +       timings->nrd_cycle_ns = (val >> 16) & 0x7f;
> +       timings->nrd_cycle_ns += ((val >> 23) & 0x3) * 256;
> +       timings->nrd_cycle_ns *= clk_rate;
> +       timings->nwe_cycle_ns = val & 0x7f;
> +       timings->nwe_cycle_ns += ((val >> 7) & 0x3) * 256;
> +       timings->nwe_cycle_ns *= clk_rate;
> +}
> +
> +static int at91sam9_smc_xslate_timings(struct at91_ebi_dev *ebid,
> +                                      struct at91sam9_smc_timings *timings)
> +{
> +       struct device_node *np = ebid->np;
> +
> +       of_property_read_u32(np, "atmel,ncs-rd-setup-ns",
> +                            &timings->ncs_rd_setup_ns);
> +       of_property_read_u32(np, "atmel,nrd-setup-ns",
> +                            &timings->nrd_setup_ns);
> +       of_property_read_u32(np, "atmel,ncs-wr-setup-ns",
> +                            &timings->ncs_wr_setup_ns);
> +       of_property_read_u32(np, "atmel,nwe-setup-ns",
> +                            &timings->nwe_setup_ns);
> +       of_property_read_u32(np, "atmel,ncs-rd-pulse-ns",
> +                            &timings->ncs_rd_pulse_ns);
> +       of_property_read_u32(np, "atmel,nrd-pulse-ns",
> +                            &timings->nrd_pulse_ns);
> +       of_property_read_u32(np, "atmel,ncs-wr-pulse-ns",
> +                            &timings->ncs_wr_pulse_ns);
> +       of_property_read_u32(np, "atmel,nwe-pulse-ns", &timings->nwe_pulse_ns);
> +       of_property_read_u32(np, "atmel,nwe-cycle-ns", &timings->nwe_cycle_ns);
> +       of_property_read_u32(np, "atmel,nrd-cycle-ns", &timings->nrd_cycle_ns);
> +       of_property_read_u32(np, "atmel,tdf-ns", &timings->tdf_ns);
> +
> +       return 0;
> +}
> +
> +static int at91sam9_ebi_xslate_config(struct at91_ebi_dev *ebid,
> +                                     struct at91_ebi_dev_config *conf)
> +{
> +       struct at91sam9_ebi_dev_config *config = &conf->sam9;
> +       struct device_node *np = ebid->np;
> +       const char *tmp_str;
> +       u32 tmp;
> +       int ret;
> +
> +       ret = of_property_read_u32(np, "atmel,bus-width", &tmp);
> +       if (!ret) {
> +               config->mode &= ~AT91_SMC_DBW;
> +               switch (tmp) {
> +               case 8:
> +                       config->mode |= AT91_SMC_DBW_8;
> +                       break;
> +
> +               case 16:
> +                       config->mode |= AT91_SMC_DBW_16;
> +                       break;
> +
> +               case 32:
> +                       config->mode |= AT91_SMC_DBW_32;
> +                       break;
> +
> +               default:
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       tmp_str = NULL;
> +       of_property_read_string(np, "atmel,tdf-mode", &tmp_str);
> +       if (tmp_str) {
> +               config->mode &= ~AT91_SMC_TDFMODE_OPTIMIZED;
> +               if (!strcmp(tmp_str, "optimized"))
> +                       config->mode |= AT91_SMC_TDFMODE_OPTIMIZED;
> +       }
> +
> +       tmp_str = NULL;
> +       of_property_read_string(np, "atmel,byte-access-type", &tmp_str);
> +       if (tmp_str) {
> +               config->mode &= AT91_SMC_BAT;
> +               if (!strcmp(tmp_str, "write"))
> +                       config->mode |= AT91_SMC_BAT_WRITE;
> +       }
> +
> +       tmp_str = NULL;
> +       of_property_read_string(np, "atmel,read-mode", &tmp_str);
> +       if (tmp_str) {
> +               config->mode &= ~AT91_SMC_READMODE;
> +               if (!strcmp(tmp_str, "nrd"))
> +                       config->mode |= AT91_SMC_READMODE_NRD;
> +       }
> +
> +       tmp_str = NULL;
> +       of_property_read_string(np, "atmel,write-mode", &tmp_str);
> +       if (tmp_str) {
> +               config->mode &= ~AT91_SMC_WRITEMODE;
> +               if (!strcmp(tmp_str, "nwe"))
> +                       config->mode |= AT91_SMC_WRITEMODE_NWE;
> +       }
> +
> +       tmp_str = NULL;
> +       of_property_read_string(np, "atmel,exnw-mode", &tmp_str);
> +       if (tmp_str) {
> +               config->mode &= ~AT91_SMC_EXNWMODE;
> +               if (!strcmp(tmp_str, "frozen"))
> +                       config->mode |= AT91_SMC_EXNWMODE_FROZEN;
> +               else if (!strcmp(tmp_str, "ready"))
> +                       config->mode |= AT91_SMC_EXNWMODE_READY;
> +       }
> +
> +       tmp = 0;
> +       ret = of_property_read_u32(np, "atmel,page-mode", &tmp);
> +       if (!ret) {
> +               config->mode &= ~AT91_SMC_PS;
> +               switch (tmp) {
> +               case 4:
> +                       config->mode |= AT91_SMC_PS_4;
> +                       break;
> +
> +               case 8:
> +                       config->mode |= AT91_SMC_PS_8;
> +                       break;
> +
> +               case 16:
> +                       config->mode |= AT91_SMC_PS_16;
> +                       break;
> +
> +               case 32:
> +                       config->mode |= AT91_SMC_PS_32;
> +                       break;
> +
> +               default:
> +                       return -EINVAL;
> +               }
> +
> +               config->mode |= AT91_SMC_PMEN;
> +       }
> +
> +       return at91sam9_smc_xslate_timings(ebid, &config->timings);
> +}
> +
> +static int at91sam9_ebi_apply_config(struct at91_ebi_dev *ebid,
> +                                    struct at91_ebi_dev_config *conf)
> +{
> +       unsigned int clk_rate = clk_get_rate(ebid->ebi->clk);
> +       struct at91sam9_ebi_dev_config *config = &conf->sam9;
> +       struct at91sam9_smc_timings *timings = &config->timings;
> +       struct at91sam9_smc_generic_fields *fields = ebid->ebi->priv;
> +       u32 coded_val;
> +       u32 val;
> +
> +       coded_val = at91sam9_smc_setup_ns_to_cycles(clk_rate,
> +                                                   timings->ncs_rd_setup_ns);
> +       val = AT91SAM9_SMC_NCS_NRDSETUP(coded_val);
> +       coded_val = at91sam9_smc_setup_ns_to_cycles(clk_rate,
> +                                                   timings->nrd_setup_ns);
> +       val |= AT91SAM9_SMC_NRDSETUP(coded_val);
> +       coded_val = at91sam9_smc_setup_ns_to_cycles(clk_rate,
> +                                                   timings->ncs_wr_setup_ns);
> +       val |= AT91SAM9_SMC_NCS_WRSETUP(coded_val);
> +       coded_val = at91sam9_smc_setup_ns_to_cycles(clk_rate,
> +                                                   timings->nwe_setup_ns);
> +       val |= AT91SAM9_SMC_NWESETUP(coded_val);
> +       regmap_fields_write(fields->setup, ebid->cs, val);
> +
> +       coded_val = at91sam9_smc_pulse_ns_to_cycles(clk_rate,
> +                                                   timings->ncs_rd_pulse_ns);
> +       val = AT91SAM9_SMC_NCS_NRDPULSE(coded_val);
> +       coded_val = at91sam9_smc_pulse_ns_to_cycles(clk_rate,
> +                                                   timings->nrd_pulse_ns);
> +       val |= AT91SAM9_SMC_NRDPULSE(coded_val);
> +       coded_val = at91sam9_smc_pulse_ns_to_cycles(clk_rate,
> +                                                   timings->ncs_wr_pulse_ns);
> +       val |= AT91SAM9_SMC_NCS_WRPULSE(coded_val);
> +       coded_val = at91sam9_smc_pulse_ns_to_cycles(clk_rate,
> +                                                   timings->nwe_pulse_ns);
> +       val |= AT91SAM9_SMC_NWEPULSE(coded_val);
> +       regmap_fields_write(fields->pulse, ebid->cs, val);
> +
> +       coded_val = at91sam9_smc_cycle_ns_to_cycles(clk_rate,
> +                                                   timings->nrd_cycle_ns);
> +       val = AT91SAM9_SMC_NRDCYCLE(coded_val);
> +       coded_val = at91sam9_smc_cycle_ns_to_cycles(clk_rate,
> +                                                   timings->nwe_cycle_ns);
> +       val |= AT91SAM9_SMC_NWECYCLE(coded_val);
> +       regmap_fields_write(fields->cycle, ebid->cs, val);
> +
> +       val = DIV_ROUND_UP(timings->tdf_ns, clk_rate);
> +       if (val > AT91_SMC_TDF_MAX)
> +               val = AT91_SMC_TDF_MAX;
> +       regmap_fields_write(fields->mode, ebid->cs,
> +                           config->mode | AT91_SMC_TDF_(val));
> +
> +       /*
> +        * Adjust config to what's actually configured in the hardware block.
> +        * Can be slightly different because of rounding policies.
> +        */
> +       at91sam9_ebi_get_config(ebid, conf);
> +
> +       return 0;
> +}
> +
> +
> +static int at91sam9_ebi_init(struct at91_ebi *ebi)
> +{
> +       struct at91sam9_smc_generic_fields *fields;
> +       struct reg_field field = REG_FIELD(0, 0, 31);
> +
> +       fields = devm_kzalloc(ebi->dev, sizeof(*fields), GFP_KERNEL);
> +       if (!fields)
> +               return -ENOMEM;
> +
> +       field.id_size = fls(ebi->caps->available_cs);
> +       field.id_offset = AT91SAM9_SMC_GENERIC_BLK_SZ;
> +
> +       field.reg = AT91SAM9_SMC_SETUP(AT91SAM9_SMC_GENERIC);
> +       fields->setup = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
> +       if (IS_ERR(fields->setup))
> +               return PTR_ERR(fields->setup);
> +
> +       field.reg = AT91SAM9_SMC_PULSE(AT91SAM9_SMC_GENERIC);
> +       fields->pulse = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
> +       if (IS_ERR(fields->pulse))
> +               return PTR_ERR(fields->pulse);
> +
> +       field.reg = AT91SAM9_SMC_CYCLE(AT91SAM9_SMC_GENERIC);
> +       fields->cycle = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
> +       if (IS_ERR(fields->cycle))
> +               return PTR_ERR(fields->cycle);
> +
> +       field.reg = AT91SAM9_SMC_MODE(AT91SAM9_SMC_GENERIC);
> +       fields->mode = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
> +       if (IS_ERR(fields->mode))
> +               return PTR_ERR(fields->mode);
> +
> +       ebi->priv = fields;
> +
> +       return 0;
> +}
> +
> +static int sama5d3_ebi_init(struct at91_ebi *ebi)
> +{
> +       struct at91sam9_smc_generic_fields *fields;
> +       struct reg_field field = REG_FIELD(0, 0, 31);
> +
> +       fields = devm_kzalloc(ebi->dev, sizeof(*fields), GFP_KERNEL);
> +       if (!fields)
> +               return -ENOMEM;
> +
> +       field.id_size = fls(ebi->caps->available_cs);
> +       field.id_offset = SAMA5_SMC_GENERIC_BLK_SZ;
> +
> +       field.reg = AT91SAM9_SMC_SETUP(SAMA5_SMC_GENERIC);
> +       fields->setup = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
> +       if (IS_ERR(fields->setup))
> +               return PTR_ERR(fields->setup);
> +
> +       field.reg = AT91SAM9_SMC_PULSE(SAMA5_SMC_GENERIC);
> +       fields->pulse = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
> +       if (IS_ERR(fields->pulse))
> +               return PTR_ERR(fields->pulse);
> +
> +       field.reg = AT91SAM9_SMC_CYCLE(SAMA5_SMC_GENERIC);
> +       fields->cycle = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
> +       if (IS_ERR(fields->cycle))
> +               return PTR_ERR(fields->cycle);
> +
> +       field.reg = SAMA5_SMC_MODE(SAMA5_SMC_GENERIC);
> +       fields->mode = devm_regmap_field_alloc(ebi->dev, ebi->smc, field);
> +       if (IS_ERR(fields->mode))
> +               return PTR_ERR(fields->mode);
> +
> +       ebi->priv = fields;
> +
> +       return 0;
> +}
> +
> +static int at91_ebi_dev_setup(struct at91_ebi *ebi, struct device_node *np)
> +{
> +       const struct at91_ebi_caps *caps = ebi->caps;
> +       struct at91_ebi_dev_config conf;
> +       struct device *dev = ebi->dev;
> +       struct device_node *dev_np;
> +       struct at91_ebi_dev *ebid;
> +       u32 tmp;
> +       int ret;
> +
> +       dev_np = of_get_next_child(np, NULL);
> +       if (!dev_np)
> +               return -EINVAL;
> +
> +       if (!of_device_is_available(dev_np))
> +               return 0;
> +
> +       ebid = devm_kzalloc(ebi->dev, sizeof(*ebid), GFP_KERNEL);
> +       if (!ebid)
> +               return -ENOMEM;
> +
> +       ret = of_property_read_u32(dev_np, "reg", &tmp);
> +       if (ret < 0) {
> +               dev_err(dev, "missing mandatory reg property\n");
> +               return ret;
> +       }
> +
> +       if (tmp > AT91_MATRIX_EBI_NUM_CS ||
> +           !(BIT(tmp) & ebi->caps->available_cs)) {
> +               dev_err(dev, "invalid reg property\n");
> +               return -EINVAL;
> +       }
> +
> +       ebid->cs = tmp;
> +       ebid->np = np;
> +       ebid->ebi = ebi;
> +
> +       /*
> +        * Attach the EBI device to the generic SMC logic.
> +        * FIXME: some drivers (like the NAND controller driver) might have
> +        * to change this config afterwards. We should expose a new API once
> +        * this requirement becomes a reality.
> +        */
> +       if (ebi->ebi_csa)
> +               regmap_field_update_bits(ebi->ebi_csa, BIT(ebid->cs), 0);
> +
> +       caps->get_config(ebid, &conf);
> +
> +       ret = caps->xlate_config(ebid, &conf);
> +       if (ret)
> +               return ret;
> +
> +       ret = caps->apply_config(ebid, &conf);
> +       if (ret)
> +               return ret;
> +
> +       ebid->config = conf;
> +       ebi->devs[ebid->cs] = ebid;
> +
> +       return of_platform_populate(np, of_default_bus_match_table, NULL, dev);
> +}
> +
> +static const struct reg_field at91sam9260_ebi_csa =
> +                               REG_FIELD(AT91SAM9260_MATRIX_EBICSA, 0,
> +                                         AT91_MATRIX_EBI_NUM_CS - 1);
> +
> +static const struct at91_ebi_caps at91sam9260_ebi_caps = {
> +       .available_cs = 0xff,
> +       .ebi_csa = &at91sam9260_ebi_csa,
> +       .xlate_config = at91sam9_ebi_xslate_config,
> +       .apply_config = at91sam9_ebi_apply_config,
> +       .init = at91sam9_ebi_init,
> +};
> +
> +static const struct reg_field at91sam9261_ebi_csa =
> +                               REG_FIELD(AT91SAM9261_MATRIX_EBICSA, 0,
> +                                         AT91_MATRIX_EBI_NUM_CS - 1);
> +
> +static const struct at91_ebi_caps at91sam9261_ebi_caps = {
> +       .available_cs = 0xff,
> +       .ebi_csa = &at91sam9261_ebi_csa,
> +       .xlate_config = at91sam9_ebi_xslate_config,
> +       .apply_config = at91sam9_ebi_apply_config,
> +       .init = at91sam9_ebi_init,
> +};
> +
> +static const struct reg_field at91sam9263_ebi0_csa =
> +                               REG_FIELD(AT91SAM9263_MATRIX_EBI0CSA, 0,
> +                                         AT91_MATRIX_EBI_NUM_CS - 1);
> +
> +static const struct at91_ebi_caps at91sam9263_ebi0_caps = {
> +       .available_cs = 0x3f,
> +       .ebi_csa = &at91sam9263_ebi0_csa,
> +       .xlate_config = at91sam9_ebi_xslate_config,
> +       .apply_config = at91sam9_ebi_apply_config,
> +       .init = at91sam9_ebi_init,
> +};
> +
> +static const struct reg_field at91sam9263_ebi1_csa =
> +                               REG_FIELD(AT91SAM9263_MATRIX_EBI1CSA, 0,
> +                                         AT91_MATRIX_EBI_NUM_CS - 1);
> +
> +static const struct at91_ebi_caps at91sam9263_ebi1_caps = {
> +       .available_cs = 0x7,
> +       .ebi_csa = &at91sam9263_ebi1_csa,
> +       .xlate_config = at91sam9_ebi_xslate_config,
> +       .apply_config = at91sam9_ebi_apply_config,
> +       .init = at91sam9_ebi_init,
> +};
> +
> +static const struct reg_field at91sam9rl_ebi_csa =
> +                               REG_FIELD(AT91SAM9RL_MATRIX_EBICSA, 0,
> +                                         AT91_MATRIX_EBI_NUM_CS - 1);
> +
> +static const struct at91_ebi_caps at91sam9rl_ebi_caps = {
> +       .available_cs = 0x3f,
> +       .ebi_csa = &at91sam9rl_ebi_csa,
> +       .xlate_config = at91sam9_ebi_xslate_config,
> +       .apply_config = at91sam9_ebi_apply_config,
> +       .init = at91sam9_ebi_init,
> +};
> +
> +static const struct reg_field at91sam9g45_ebi_csa =
> +                               REG_FIELD(AT91SAM9G45_MATRIX_EBICSA, 0,
> +                                         AT91_MATRIX_EBI_NUM_CS - 1);
> +
> +static const struct at91_ebi_caps at91sam9g45_ebi_caps = {
> +       .available_cs = 0x3f,
> +       .ebi_csa = &at91sam9g45_ebi_csa,
> +       .xlate_config = at91sam9_ebi_xslate_config,
> +       .apply_config = at91sam9_ebi_apply_config,
> +       .init = at91sam9_ebi_init,
> +};
> +
> +static const struct at91_ebi_caps at91sam9x5_ebi_caps = {
> +       .available_cs = 0x3f,
> +       .ebi_csa = &at91sam9263_ebi0_csa,
> +       .xlate_config = at91sam9_ebi_xslate_config,
> +       .apply_config = at91sam9_ebi_apply_config,
> +       .init = at91sam9_ebi_init,
> +};
> +
> +static const struct at91_ebi_caps sama5d3_ebi_caps = {
> +       .available_cs = 0xf,
> +       .xlate_config = at91sam9_ebi_xslate_config,
> +       .apply_config = at91sam9_ebi_apply_config,
> +       .init = sama5d3_ebi_init,
> +};
> +
> +static const struct of_device_id at91_ebi_id_table[] = {
> +       {
> +               .compatible = "atmel,at91sam9260-ebi",
> +               .data = &at91sam9260_ebi_caps,
> +       },
> +       {
> +               .compatible = "atmel,at91sam9261-ebi",
> +               .data = &at91sam9261_ebi_caps,
> +       },
> +       {
> +               .compatible = "atmel,at91sam9263-ebi0",
> +               .data = &at91sam9263_ebi0_caps,
> +       },
> +       {
> +               .compatible = "atmel,at91sam9263-ebi1",
> +               .data = &at91sam9263_ebi1_caps,
> +       },
> +       {
> +               .compatible = "atmel,at91sam9rl-ebi",
> +               .data = &at91sam9rl_ebi_caps,
> +       },
> +       {
> +               .compatible = "atmel,at91sam9g45-ebi",
> +               .data = &at91sam9g45_ebi_caps,
> +       },
> +       {
> +               .compatible = "atmel,at91sam9x5-ebi",
> +               .data = &at91sam9x5_ebi_caps,
> +       },
> +       {
> +               .compatible = "atmel,sama5d3-ebi",
> +               .data = &sama5d3_ebi_caps,
> +       },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, at91_ebi_id_table);
> +
> +static int at91_ebi_probe(struct platform_device *pdev)
> +{
> +       const struct of_device_id *match;
> +       struct device_node *child;
> +       struct at91_ebi *ebi;
> +       struct clk *clk;
> +       int ret;
> +
> +       match = of_match_device(at91_ebi_id_table, &pdev->dev);
> +       if (!match || !match->data)
> +               return -EINVAL;
> +
> +       ebi = devm_kzalloc(&pdev->dev, sizeof(*ebi), GFP_KERNEL);
> +       if (!ebi)
> +               return -ENOMEM;
> +
> +       ebi->caps = match->data;
> +       ebi->dev = &pdev->dev;
> +
> +       clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +
> +       ebi->clk = clk;
> +
> +       ebi->smc = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +                                                  "atmel,smc");
> +       if (IS_ERR(ebi->smc))
> +               return PTR_ERR(ebi->smc);
> +
> +       /*
> +        * The sama5d3 does not provide an EBICSA register and thus does need
> +        * to access the matrix registers.
> +        */
> +       if (ebi->caps->ebi_csa) {
> +               ebi->matrix =
> +                       syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +                                                       "atmel,matrix");
> +               if (IS_ERR(ebi->matrix))
> +                       return PTR_ERR(ebi->matrix);
> +
> +               ebi->ebi_csa = regmap_field_alloc(ebi->matrix,
> +                                                 *ebi->caps->ebi_csa);
> +               if (IS_ERR(ebi->ebi_csa))
> +                       return PTR_ERR(ebi->ebi_csa);
> +       }
> +
> +       ret = ebi->caps->init(ebi);
> +       if (ret)
> +               return ret;
> +
> +       for_each_child_of_node(pdev->dev.of_node, child) {
> +               ret = at91_ebi_dev_setup(ebi, child);
> +               if (ret)
> +                       return ret;

I'm not sure about breaking out of the loop here, if
at91_ebi_dev_setup() for a device fail then the remaining devices
won't be probed. If you add a bad config for a fpga on CS1, you can
prevent your NAND  on CS2 to be probed.

> +       }
> +
> +       return ret;
> +}
> +
> +static struct platform_driver at91_ebi_driver = {
> +       .driver = {
> +               .name = "atmel-ebi",
> +               .of_match_table = at91_ebi_id_table,
> +       },
> +};
> +module_platform_driver_probe(at91_ebi_driver, at91_ebi_probe);
> +
> +MODULE_AUTHOR("JJ Hiblot");
> +MODULE_DESCRIPTION("Atmel EBI driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>

Re: [PATCH v6 1/2] memory: add Atmel EBI (External Bus Interface) driver

[Boris Brezillon]

On Thu, 28 Apr 2016 10:47:24 +0200
Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote:

> > +static int at91_ebi_probe(struct platform_device *pdev)
> > +{
> > +       const struct of_device_id *match;
> > +       struct device_node *child;
> > +       struct at91_ebi *ebi;
> > +       struct clk *clk;
> > +       int ret;
> > +
> > +       match = of_match_device(at91_ebi_id_table, &pdev->dev);
> > +       if (!match || !match->data)
> > +               return -EINVAL;
> > +
> > +       ebi = devm_kzalloc(&pdev->dev, sizeof(*ebi), GFP_KERNEL);
> > +       if (!ebi)
> > +               return -ENOMEM;
> > +
> > +       ebi->caps = match->data;
> > +       ebi->dev = &pdev->dev;
> > +
> > +       clk = devm_clk_get(&pdev->dev, NULL);
> > +       if (IS_ERR(clk))
> > +               return PTR_ERR(clk);
> > +
> > +       ebi->clk = clk;
> > +
> > +       ebi->smc = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> > +                                                  "atmel,smc");
> > +       if (IS_ERR(ebi->smc))
> > +               return PTR_ERR(ebi->smc);
> > +
> > +       /*
> > +        * The sama5d3 does not provide an EBICSA register and thus does need
> > +        * to access the matrix registers.
> > +        */
> > +       if (ebi->caps->ebi_csa) {
> > +               ebi->matrix =
> > +                       syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> > +                                                       "atmel,matrix");
> > +               if (IS_ERR(ebi->matrix))
> > +                       return PTR_ERR(ebi->matrix);
> > +
> > +               ebi->ebi_csa = regmap_field_alloc(ebi->matrix,
> > +                                                 *ebi->caps->ebi_csa);
> > +               if (IS_ERR(ebi->ebi_csa))
> > +                       return PTR_ERR(ebi->ebi_csa);
> > +       }
> > +
> > +       ret = ebi->caps->init(ebi);
> > +       if (ret)
> > +               return ret;
> > +
> > +       for_each_child_of_node(pdev->dev.of_node, child) {
> > +               ret = at91_ebi_dev_setup(ebi, child);
> > +               if (ret)
> > +                       return ret;  
> 
> I'm not sure about breaking out of the loop here, if
> at91_ebi_dev_setup() for a device fail then the remaining devices
> won't be probed. If you add a bad config for a fpga on CS1, you can
> prevent your NAND  on CS2 to be probed.

Fair enough, I'll continue iterating even if an error occurs.


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v6 2/2] memory: atmel-ebi: add DT bindings documentation

[Boris Brezillon]

The EBI (External Bus Interface) is used to access external peripherals
(NOR, SRAM, NAND, and other specific devices like ethernet controllers).
Each device is assigned a CS line and an address range and can have its
own configuration (timings, access mode, bus width, ...).
This driver provides a generic DT binding to configure a device according
to its requirements.
For specific device controllers (like the NAND one) the SMC timings
should be configured by the controller driver through the matrix and smc
syscon regmaps.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 .../bindings/memory-controllers/atmel,ebi.txt      | 146 +++++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt

diff --git a/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
new file mode 100644
index 0000000..0f332d2
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
@@ -0,0 +1,146 @@
+* Device tree bindings for Atmel EBI
+
+The External Bus Interface (EBI) controller is a bus where you can connect
+asynchronous (NAND, NOR, SRAM, ....) and synchronous memories (SDR/DDR SDRAMs).
+The EBI provides a glue-less interface to asynchronous memories though the SMC
+(Static Memory Controller).
+Synchronous memories (and some asynchronous memories like NANDs) can be
+attached to specialized controllers which are responsible for configuring the
+bus appropriately according to the connected device.
+In the other hand, the bus interface can be automated for simple asynchronous
+devices.
+
+Required properties:
+
+- compatible:		"atmel,at91sam9260-ebi"
+			"atmel,at91sam9261-ebi"
+			"atmel,at91sam9263-ebi0"
+			"atmel,at91sam9263-ebi1"
+			"atmel,at91sam9rl-ebi"
+			"atmel,at91sam9g45-ebi"
+			"atmel,at91sam9x5-ebi"
+			"atmel,sama5d3-ebi"
+
+- reg:			Contains offset/length value for EBI memory mapping.
+			This property might contain several entries if the EBI
+			memory range is not contiguous
+
+- #address-cells:	Must be 2.
+			The first cell encodes the CS.
+			The second cell encode the offset into the CS memory
+			range.
+
+- #size-cells:		Must be set to 1.
+
+- ranges:		Encodes CS to memory region association.
+
+- clocks:		Clock feeding the EBI controller.
+			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.
+There might be board specific devices like FPGAs.
+You'll define you device requirements in these child nodes.
+
+Required child cs node properties:
+
+- #address-cells:	Must be 2.
+
+- #size-cells:		Must be 1.
+
+- ranges:		Empty property indicating that child nodes can inherit
+			memory layout.
+
+Optional child cs node properties:
+- atmel,bus-width:		width of the asynchronous device's data bus
+				8, 16 or 32.
+				8 if not present.
+
+- atmel,byte-access-type	"write" or "select" (see Atmel datasheet).
+				"select" if not present.
+
+- atmel,read-mode		"nrd" or "ncs".
+				"ncs" is not present.
+
+- atmel,write-mode		"nwe" or "ncs".
+				"ncs" is not present.
+
+- atmel,exnw-mode		"disabled", "frozen" or "ready".
+				"disabled" if not present.
+
+- atmel,page-mode		enable page mode if present. The provided value
+				defines the page size (supported values: 4, 8,
+				16 and 32).
+
+Optional device timings expressed in nanoseconds (if the property is not
+present 0 is assumed):
+
+- atmel,ncs-rd-setup-ns
+- atmel,nrd-setup-ns
+- atmel,ncs-wr-setup-ns
+- atmel,nwe-setup-ns
+- atmel,ncs-rd-pulse-ns
+- atmel,nrd-pulse-ns
+- atmel,ncs-wr-pulse-ns
+- atmel,nwe-pulse-ns
+- atmel,nwe-cycle-ns
+- atmel,nrd-cycle-ns
+- atmel,tdf-ns
+
+- atmel,tdf-mode:		"normal" or "optimized". If set to "optimized"
+				the data float time is optimized depending on
+				the next device being accessed (next device
+				setup time is subtracted to the current device
+				data float time).
+
+
+
+Example:
+
+	ebi: ebi@10000000 {
+		compatible = "atmel,sama5d3-ebi", "simple-bus";
+		#address-cells = <2>;
+		#size-cells = <1>;
+		atmel,smc = <&hsmc>;
+		atmel,matrix = <&matrix>;
+		reg = <0x10000000 0x10000000
+		       0x40000000 0x30000000>;
+		ranges = <0x0 0x0 0x10000000 0x10000000
+			  0x1 0x0 0x40000000 0x10000000
+			  0x2 0x0 0x50000000 0x10000000
+			  0x3 0x0 0x60000000 0x10000000>;
+		clocks = <&mck>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_ebi_addr>;
+
+		cs@0 {
+			#address-cells = <2>;
+			#size-cells = <1>;
+			ranges;
+			atmel,generic-dev;
+			atmel,read-mode = "nrd";
+			atmel,write-mode = "nwe";
+			atmel,bus-width = <16>;
+			atmel,ncs-rd-setup-ns = <0>;
+			atmel,ncs-wr-setup-ns = <0>;
+			atmel,nwe-setup-ns = <8>;
+			atmel,nrd-setup-ns = <16>;
+			atmel,ncs-rd-pulse-ns = <84>;
+			atmel,ncs-wr-pulse-ns = <84>;
+			atmel,nrd-pulse-ns = <76>;
+			atmel,nwe-pulse-ns = <76>;
+			atmel,nrd-cycle-ns = <107>;
+			atmel,nwe-cycle-ns = <84>;
+			atmel,tdf-ns = <16>;
+
+			nor: flash@0,0 {
+				compatible = "cfi-flash";
+				#address-cells = <1>;
+				#size-cells = <1>;
+				reg = <0x0 0x0 0x1000000>;
+				bank-width = <2>;
+			};
+		};
+	};
+
-- 
2.7.4

Re: [PATCH v6 2/2] memory: atmel-ebi: add DT bindings documentation

[Mark Rutland]

On Wed, Apr 27, 2016 at 04:35:54PM +0200, Boris Brezillon wrote:
> The EBI (External Bus Interface) is used to access external peripherals
> (NOR, SRAM, NAND, and other specific devices like ethernet controllers).
> Each device is assigned a CS line and an address range and can have its
> own configuration (timings, access mode, bus width, ...).
> This driver provides a generic DT binding to configure a device according
> to its requirements.
> For specific device controllers (like the NAND one) the SMC timings
> should be configured by the controller driver through the matrix and smc
> syscon regmaps.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  .../bindings/memory-controllers/atmel,ebi.txt      | 146 +++++++++++++++++++++
>  1 file changed, 146 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> new file mode 100644
> index 0000000..0f332d2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> @@ -0,0 +1,146 @@
> +* Device tree bindings for Atmel EBI
> +
> +The External Bus Interface (EBI) controller is a bus where you can connect
> +asynchronous (NAND, NOR, SRAM, ....) and synchronous memories (SDR/DDR SDRAMs).
> +The EBI provides a glue-less interface to asynchronous memories though the SMC
> +(Static Memory Controller).
> +Synchronous memories (and some asynchronous memories like NANDs) can be
> +attached to specialized controllers which are responsible for configuring the
> +bus appropriately according to the connected device.
> +In the other hand, the bus interface can be automated for simple asynchronous
> +devices.
> +
> +Required properties:
> +
> +- compatible:		"atmel,at91sam9260-ebi"
> +			"atmel,at91sam9261-ebi"
> +			"atmel,at91sam9263-ebi0"
> +			"atmel,at91sam9263-ebi1"
> +			"atmel,at91sam9rl-ebi"
> +			"atmel,at91sam9g45-ebi"
> +			"atmel,at91sam9x5-ebi"
> +			"atmel,sama5d3-ebi"
> +
> +- reg:			Contains offset/length value for EBI memory mapping.
> +			This property might contain several entries if the EBI
> +			memory range is not contiguous
> +
> +- #address-cells:	Must be 2.
> +			The first cell encodes the CS.
> +			The second cell encode the offset into the CS memory
> +			range.
> +
> +- #size-cells:		Must be set to 1.
> +
> +- ranges:		Encodes CS to memory region association.
> +
> +- clocks:		Clock feeding the EBI controller.
> +			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.
> +There might be board specific devices like FPGAs.
> +You'll define you device requirements in these child nodes.
> +
> +Required child cs node properties:
> +
> +- #address-cells:	Must be 2.
> +
> +- #size-cells:		Must be 1.
> +
> +- ranges:		Empty property indicating that child nodes can inherit
> +			memory layout.
> +
> +Optional child cs node properties:
> +- atmel,bus-width:		width of the asynchronous device's data bus
> +				8, 16 or 32.
> +				8 if not present.
> +
> +- atmel,byte-access-type	"write" or "select" (see Atmel datasheet).
> +				"select" if not present.
> +
> +- atmel,read-mode		"nrd" or "ncs".
> +				"ncs" is not present.
> +
> +- atmel,write-mode		"nwe" or "ncs".
> +				"ncs" is not present.
> +
> +- atmel,exnw-mode		"disabled", "frozen" or "ready".
> +				"disabled" if not present.
> +
> +- atmel,page-mode		enable page mode if present. The provided value
> +				defines the page size (supported values: 4, 8,
> +				16 and 32).
> +
> +Optional device timings expressed in nanoseconds (if the property is not
> +present 0 is assumed):
> +
> +- atmel,ncs-rd-setup-ns
> +- atmel,nrd-setup-ns
> +- atmel,ncs-wr-setup-ns
> +- atmel,nwe-setup-ns
> +- atmel,ncs-rd-pulse-ns
> +- atmel,nrd-pulse-ns
> +- atmel,ncs-wr-pulse-ns
> +- atmel,nwe-pulse-ns
> +- atmel,nwe-cycle-ns
> +- atmel,nrd-cycle-ns
> +- atmel,tdf-ns

I assume that these are defined in a datasheet. It may be worth noting
"see datasheet" above this collection of properties.

> +
> +- atmel,tdf-mode:		"normal" or "optimized". If set to "optimized"
> +				the data float time is optimized depending on
> +				the next device being accessed (next device
> +				setup time is subtracted to the current device
> +				data float time).
> +
> +
> +
> +Example:
> +
> +	ebi: ebi@10000000 {
> +		compatible = "atmel,sama5d3-ebi", "simple-bus";

I don't believe that "simple-bus" should be here.

> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		atmel,smc = <&hsmc>;
> +		atmel,matrix = <&matrix>;
> +		reg = <0x10000000 0x10000000
> +		       0x40000000 0x30000000>;
> +		ranges = <0x0 0x0 0x10000000 0x10000000
> +			  0x1 0x0 0x40000000 0x10000000
> +			  0x2 0x0 0x50000000 0x10000000
> +			  0x3 0x0 0x60000000 0x10000000>;
> +		clocks = <&mck>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_ebi_addr>;
> +
> +		cs@0 {

I suspect recent DTC will warn here, as the unit-address should match
the reg for the node (and no reg is defined). Here the reg would have to
be two cells, which means we have to make up an address, no?

We can either give the node a name without a unit-address (e.g. cs_0),
or define some reg format.

> +			#address-cells = <2>;
> +			#size-cells = <1>;
> +			ranges;
> +			atmel,generic-dev;

The cover letter mentioned this should go.

> +			atmel,read-mode = "nrd";
> +			atmel,write-mode = "nwe";
> +			atmel,bus-width = <16>;
> +			atmel,ncs-rd-setup-ns = <0>;
> +			atmel,ncs-wr-setup-ns = <0>;
> +			atmel,nwe-setup-ns = <8>;
> +			atmel,nrd-setup-ns = <16>;
> +			atmel,ncs-rd-pulse-ns = <84>;
> +			atmel,ncs-wr-pulse-ns = <84>;
> +			atmel,nrd-pulse-ns = <76>;
> +			atmel,nwe-pulse-ns = <76>;
> +			atmel,nrd-cycle-ns = <107>;
> +			atmel,nwe-cycle-ns = <84>;
> +			atmel,tdf-ns = <16>;
> +
> +			nor: flash@0,0 {
> +				compatible = "cfi-flash";
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				reg = <0x0 0x0 0x1000000>;

It feels odd that in the node for chipselect N, sub-devices have to
encode the chipselect number in their reg, when it's obvious from their
container. It may make more sense for the cs node to have a non-empty
reg (or somehow to make that translation/truncation implicit).

Thanks,
Mark,

Re: [PATCH v6 2/2] memory: atmel-ebi: add DT bindings documentation

[Boris Brezillon]

Hi Mark,

On Wed, 27 Apr 2016 16:07:38 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Wed, Apr 27, 2016 at 04:35:54PM +0200, Boris Brezillon wrote:
> > The EBI (External Bus Interface) is used to access external peripherals
> > (NOR, SRAM, NAND, and other specific devices like ethernet controllers).
> > Each device is assigned a CS line and an address range and can have its
> > own configuration (timings, access mode, bus width, ...).
> > This driver provides a generic DT binding to configure a device according
> > to its requirements.
> > For specific device controllers (like the NAND one) the SMC timings
> > should be configured by the controller driver through the matrix and smc
> > syscon regmaps.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  .../bindings/memory-controllers/atmel,ebi.txt      | 146 +++++++++++++++++++++
> >  1 file changed, 146 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> > new file mode 100644
> > index 0000000..0f332d2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> > @@ -0,0 +1,146 @@
> > +* Device tree bindings for Atmel EBI
> > +
> > +The External Bus Interface (EBI) controller is a bus where you can connect
> > +asynchronous (NAND, NOR, SRAM, ....) and synchronous memories (SDR/DDR SDRAMs).
> > +The EBI provides a glue-less interface to asynchronous memories though the SMC
> > +(Static Memory Controller).
> > +Synchronous memories (and some asynchronous memories like NANDs) can be
> > +attached to specialized controllers which are responsible for configuring the
> > +bus appropriately according to the connected device.
> > +In the other hand, the bus interface can be automated for simple asynchronous
> > +devices.
> > +
> > +Required properties:
> > +
> > +- compatible:		"atmel,at91sam9260-ebi"
> > +			"atmel,at91sam9261-ebi"
> > +			"atmel,at91sam9263-ebi0"
> > +			"atmel,at91sam9263-ebi1"
> > +			"atmel,at91sam9rl-ebi"
> > +			"atmel,at91sam9g45-ebi"
> > +			"atmel,at91sam9x5-ebi"
> > +			"atmel,sama5d3-ebi"
> > +
> > +- reg:			Contains offset/length value for EBI memory mapping.
> > +			This property might contain several entries if the EBI
> > +			memory range is not contiguous
> > +
> > +- #address-cells:	Must be 2.
> > +			The first cell encodes the CS.
> > +			The second cell encode the offset into the CS memory
> > +			range.
> > +
> > +- #size-cells:		Must be set to 1.
> > +
> > +- ranges:		Encodes CS to memory region association.
> > +
> > +- clocks:		Clock feeding the EBI controller.
> > +			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.
> > +There might be board specific devices like FPGAs.
> > +You'll define you device requirements in these child nodes.
> > +
> > +Required child cs node properties:
> > +
> > +- #address-cells:	Must be 2.
> > +
> > +- #size-cells:		Must be 1.
> > +
> > +- ranges:		Empty property indicating that child nodes can inherit
> > +			memory layout.
> > +
> > +Optional child cs node properties:
> > +- atmel,bus-width:		width of the asynchronous device's data bus
> > +				8, 16 or 32.
> > +				8 if not present.
> > +
> > +- atmel,byte-access-type	"write" or "select" (see Atmel datasheet).
> > +				"select" if not present.
> > +
> > +- atmel,read-mode		"nrd" or "ncs".
> > +				"ncs" is not present.
> > +
> > +- atmel,write-mode		"nwe" or "ncs".
> > +				"ncs" is not present.
> > +
> > +- atmel,exnw-mode		"disabled", "frozen" or "ready".
> > +				"disabled" if not present.
> > +
> > +- atmel,page-mode		enable page mode if present. The provided value
> > +				defines the page size (supported values: 4, 8,
> > +				16 and 32).
> > +
> > +Optional device timings expressed in nanoseconds (if the property is not
> > +present 0 is assumed):
> > +
> > +- atmel,ncs-rd-setup-ns
> > +- atmel,nrd-setup-ns
> > +- atmel,ncs-wr-setup-ns
> > +- atmel,nwe-setup-ns
> > +- atmel,ncs-rd-pulse-ns
> > +- atmel,nrd-pulse-ns
> > +- atmel,ncs-wr-pulse-ns
> > +- atmel,nwe-pulse-ns
> > +- atmel,nwe-cycle-ns
> > +- atmel,nrd-cycle-ns
> > +- atmel,tdf-ns  
> 
> I assume that these are defined in a datasheet. It may be worth noting
> "see datasheet" above this collection of properties.

Yep.

> 
> > +
> > +- atmel,tdf-mode:		"normal" or "optimized". If set to "optimized"
> > +				the data float time is optimized depending on
> > +				the next device being accessed (next device
> > +				setup time is subtracted to the current device
> > +				data float time).
> > +
> > +
> > +
> > +Example:
> > +
> > +	ebi: ebi@10000000 {
> > +		compatible = "atmel,sama5d3-ebi", "simple-bus";  
> 
> I don't believe that "simple-bus" should be here.
> 
> > +		#address-cells = <2>;
> > +		#size-cells = <1>;
> > +		atmel,smc = <&hsmc>;
> > +		atmel,matrix = <&matrix>;
> > +		reg = <0x10000000 0x10000000
> > +		       0x40000000 0x30000000>;
> > +		ranges = <0x0 0x0 0x10000000 0x10000000
> > +			  0x1 0x0 0x40000000 0x10000000
> > +			  0x2 0x0 0x50000000 0x10000000
> > +			  0x3 0x0 0x60000000 0x10000000>;
> > +		clocks = <&mck>;
> > +
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_ebi_addr>;
> > +
> > +		cs@0 {  
> 
> I suspect recent DTC will warn here, as the unit-address should match
> the reg for the node (and no reg is defined). Here the reg would have to
> be two cells, which means we have to make up an address, no?
> 
> We can either give the node a name without a unit-address (e.g. cs_0),
> or define some reg format.

I think I'll go for the cs_X or csX solution.

> 
> > +			#address-cells = <2>;
> > +			#size-cells = <1>;
> > +			ranges;
> > +			atmel,generic-dev;  
> 
> The cover letter mentioned this should go.

Yes, it's a leftover from the previous version. I'll remove it.

> 
> > +			atmel,read-mode = "nrd";
> > +			atmel,write-mode = "nwe";
> > +			atmel,bus-width = <16>;
> > +			atmel,ncs-rd-setup-ns = <0>;
> > +			atmel,ncs-wr-setup-ns = <0>;
> > +			atmel,nwe-setup-ns = <8>;
> > +			atmel,nrd-setup-ns = <16>;
> > +			atmel,ncs-rd-pulse-ns = <84>;
> > +			atmel,ncs-wr-pulse-ns = <84>;
> > +			atmel,nrd-pulse-ns = <76>;
> > +			atmel,nwe-pulse-ns = <76>;
> > +			atmel,nrd-cycle-ns = <107>;
> > +			atmel,nwe-cycle-ns = <84>;
> > +			atmel,tdf-ns = <16>;
> > +
> > +			nor: flash@0,0 {
> > +				compatible = "cfi-flash";
> > +				#address-cells = <1>;
> > +				#size-cells = <1>;
> > +				reg = <0x0 0x0 0x1000000>;  
> 
> It feels odd that in the node for chipselect N, sub-devices have to
> encode the chipselect number in their reg, when it's obvious from their
> container. It may make more sense for the cs node to have a non-empty
> reg (or somehow to make that translation/truncation implicit).

Well, it's using the ranges property to automate reg => struct resource
conversion, and the CS number is part of this conversion. Do you see a
simpler solution to do that.

Also, I'd like to mention that I'm not really happy with the current
DT representation: ideally, I'd like to have devices connected to the
EBI bus as direct sub-nodes of the ebi node, but that's not possible,
because we have to isolate EBI bus config (timings and other bus
related configs) from device description (which is driver specific).

I considered splitting the subdevices and configs descriptions in
two different subnodes, but decided to go for the representation
already used by other memory controllers (see TI AEMIF binding).

Any suggestions?

Regards,

Boris
-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v6 2/2] memory: atmel-ebi: add DT bindings documentation

[Boris Brezillon]

Hi Mark,

On Wed, 27 Apr 2016 16:07:38 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Wed, Apr 27, 2016 at 04:35:54PM +0200, Boris Brezillon wrote:
> > The EBI (External Bus Interface) is used to access external peripherals
> > (NOR, SRAM, NAND, and other specific devices like ethernet controllers).
> > Each device is assigned a CS line and an address range and can have its
> > own configuration (timings, access mode, bus width, ...).
> > This driver provides a generic DT binding to configure a device according
> > to its requirements.
> > For specific device controllers (like the NAND one) the SMC timings
> > should be configured by the controller driver through the matrix and smc
> > syscon regmaps.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  .../bindings/memory-controllers/atmel,ebi.txt      | 146 +++++++++++++++++++++
> >  1 file changed, 146 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> > new file mode 100644
> > index 0000000..0f332d2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> > @@ -0,0 +1,146 @@
> > +* Device tree bindings for Atmel EBI
> > +
> > +The External Bus Interface (EBI) controller is a bus where you can connect
> > +asynchronous (NAND, NOR, SRAM, ....) and synchronous memories (SDR/DDR SDRAMs).
> > +The EBI provides a glue-less interface to asynchronous memories though the SMC
> > +(Static Memory Controller).
> > +Synchronous memories (and some asynchronous memories like NANDs) can be
> > +attached to specialized controllers which are responsible for configuring the
> > +bus appropriately according to the connected device.
> > +In the other hand, the bus interface can be automated for simple asynchronous
> > +devices.
> > +
> > +Required properties:
> > +
> > +- compatible:		"atmel,at91sam9260-ebi"
> > +			"atmel,at91sam9261-ebi"
> > +			"atmel,at91sam9263-ebi0"
> > +			"atmel,at91sam9263-ebi1"
> > +			"atmel,at91sam9rl-ebi"
> > +			"atmel,at91sam9g45-ebi"
> > +			"atmel,at91sam9x5-ebi"
> > +			"atmel,sama5d3-ebi"
> > +
> > +- reg:			Contains offset/length value for EBI memory mapping.
> > +			This property might contain several entries if the EBI
> > +			memory range is not contiguous
> > +
> > +- #address-cells:	Must be 2.
> > +			The first cell encodes the CS.
> > +			The second cell encode the offset into the CS memory
> > +			range.
> > +
> > +- #size-cells:		Must be set to 1.
> > +
> > +- ranges:		Encodes CS to memory region association.
> > +
> > +- clocks:		Clock feeding the EBI controller.
> > +			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.
> > +There might be board specific devices like FPGAs.
> > +You'll define you device requirements in these child nodes.
> > +
> > +Required child cs node properties:
> > +
> > +- #address-cells:	Must be 2.
> > +
> > +- #size-cells:		Must be 1.
> > +
> > +- ranges:		Empty property indicating that child nodes can inherit
> > +			memory layout.
> > +
> > +Optional child cs node properties:
> > +- atmel,bus-width:		width of the asynchronous device's data bus
> > +				8, 16 or 32.
> > +				8 if not present.
> > +
> > +- atmel,byte-access-type	"write" or "select" (see Atmel datasheet).
> > +				"select" if not present.
> > +
> > +- atmel,read-mode		"nrd" or "ncs".
> > +				"ncs" is not present.
> > +
> > +- atmel,write-mode		"nwe" or "ncs".
> > +				"ncs" is not present.
> > +
> > +- atmel,exnw-mode		"disabled", "frozen" or "ready".
> > +				"disabled" if not present.
> > +
> > +- atmel,page-mode		enable page mode if present. The provided value
> > +				defines the page size (supported values: 4, 8,
> > +				16 and 32).
> > +
> > +Optional device timings expressed in nanoseconds (if the property is not
> > +present 0 is assumed):
> > +
> > +- atmel,ncs-rd-setup-ns
> > +- atmel,nrd-setup-ns
> > +- atmel,ncs-wr-setup-ns
> > +- atmel,nwe-setup-ns
> > +- atmel,ncs-rd-pulse-ns
> > +- atmel,nrd-pulse-ns
> > +- atmel,ncs-wr-pulse-ns
> > +- atmel,nwe-pulse-ns
> > +- atmel,nwe-cycle-ns
> > +- atmel,nrd-cycle-ns
> > +- atmel,tdf-ns  
> 
> I assume that these are defined in a datasheet. It may be worth noting
> "see datasheet" above this collection of properties.
> 
> > +
> > +- atmel,tdf-mode:		"normal" or "optimized". If set to "optimized"
> > +				the data float time is optimized depending on
> > +				the next device being accessed (next device
> > +				setup time is subtracted to the current device
> > +				data float time).
> > +
> > +
> > +
> > +Example:
> > +
> > +	ebi: ebi@10000000 {
> > +		compatible = "atmel,sama5d3-ebi", "simple-bus";  
> 
> I don't believe that "simple-bus" should be here.
> 
> > +		#address-cells = <2>;
> > +		#size-cells = <1>;
> > +		atmel,smc = <&hsmc>;
> > +		atmel,matrix = <&matrix>;
> > +		reg = <0x10000000 0x10000000
> > +		       0x40000000 0x30000000>;
> > +		ranges = <0x0 0x0 0x10000000 0x10000000
> > +			  0x1 0x0 0x40000000 0x10000000
> > +			  0x2 0x0 0x50000000 0x10000000
> > +			  0x3 0x0 0x60000000 0x10000000>;
> > +		clocks = <&mck>;
> > +
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_ebi_addr>;
> > +
> > +		cs@0 {  
> 
> I suspect recent DTC will warn here, as the unit-address should match
> the reg for the node (and no reg is defined). Here the reg would have to
> be two cells, which means we have to make up an address, no?
> 
> We can either give the node a name without a unit-address (e.g. cs_0),
> or define some reg format.
> 
> > +			#address-cells = <2>;
> > +			#size-cells = <1>;
> > +			ranges;
> > +			atmel,generic-dev;  
> 
> The cover letter mentioned this should go.
> 
> > +			atmel,read-mode = "nrd";
> > +			atmel,write-mode = "nwe";
> > +			atmel,bus-width = <16>;
> > +			atmel,ncs-rd-setup-ns = <0>;
> > +			atmel,ncs-wr-setup-ns = <0>;
> > +			atmel,nwe-setup-ns = <8>;
> > +			atmel,nrd-setup-ns = <16>;
> > +			atmel,ncs-rd-pulse-ns = <84>;
> > +			atmel,ncs-wr-pulse-ns = <84>;
> > +			atmel,nrd-pulse-ns = <76>;
> > +			atmel,nwe-pulse-ns = <76>;
> > +			atmel,nrd-cycle-ns = <107>;
> > +			atmel,nwe-cycle-ns = <84>;
> > +			atmel,tdf-ns = <16>;
> > +
> > +			nor: flash@0,0 {
> > +				compatible = "cfi-flash";
> > +				#address-cells = <1>;
> > +				#size-cells = <1>;
> > +				reg = <0x0 0x0 0x1000000>;  
> 
> It feels odd that in the node for chipselect N, sub-devices have to
> encode the chipselect number in their reg, when it's obvious from their
> container. It may make more sense for the cs node to have a non-empty
> reg (or somehow to make that translation/truncation implicit).

Would you agree with the following representation?

	ebi: ebi@10000000 {
		compatible = "atmel,sama5d3-ebi";
		#address-cells = <2>;
		#size-cells = <1>;
		atmel,smc = <&hsmc>;
		atmel,matrix = <&matrix>;
		reg = <0x10000000 0x10000000
		       0x40000000 0x30000000>;
		ranges = <0x0 0x0 0x10000000 0x10000000
			  0x1 0x0 0x40000000 0x10000000
			  0x2 0x0 0x50000000 0x10000000
			  0x3 0x0 0x60000000 0x10000000>;
		clocks = <&mck>;

		pinctrl-names = "default";
		pinctrl-0 = <&pinctrl_ebi_addr>;

		configs {
			config-0 {
				atmel,read-mode = "nrd";
				atmel,write-mode = "nwe";
				atmel,bus-width = <16>;
				atmel,ncs-rd-setup-ns = <0>;
				atmel,ncs-wr-setup-ns = <0>;
				atmel,nwe-setup-ns = <8>;
				atmel,nrd-setup-ns = <16>;
				atmel,ncs-rd-pulse-ns = <84>;
				atmel,ncs-wr-pulse-ns = <84>;
				atmel,nrd-pulse-ns = <76>;
				atmel,nwe-pulse-ns = <76>;
				atmel,nrd-cycle-ns = <107>;
				atmel,nwe-cycle-ns = <84>;
				atmel,tdf-ns = <16>;
			};
		};

		nor: flash@0,0 {
			compatible = "cfi-flash";
			#address-cells = <1>;
			#size-cells = <1>;
			reg = <0x0 0x0 0x1000000>;
			bank-width = <2>;
		};
	};


Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v6 2/2] memory: atmel-ebi: add DT bindings documentation

[Mark Rutland]

Hi,

On Thu, Apr 28, 2016 at 08:44:05AM +0200, Boris Brezillon wrote:
> On Wed, 27 Apr 2016 16:07:38 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> > On Wed, Apr 27, 2016 at 04:35:54PM +0200, Boris Brezillon wrote:
> > > +	ebi: ebi@10000000 {
> > > +		compatible = "atmel,sama5d3-ebi", "simple-bus";  
> > > +		#address-cells = <2>;
> > > +		#size-cells = <1>;
> > > +		atmel,smc = <&hsmc>;
> > > +		atmel,matrix = <&matrix>;
> > > +		reg = <0x10000000 0x10000000
> > > +		       0x40000000 0x30000000>;
> > > +		ranges = <0x0 0x0 0x10000000 0x10000000
> > > +			  0x1 0x0 0x40000000 0x10000000
> > > +			  0x2 0x0 0x50000000 0x10000000
> > > +			  0x3 0x0 0x60000000 0x10000000>;
> > > +		clocks = <&mck>;
> > > +
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&pinctrl_ebi_addr>;
> > > +
> > > +		cs@0 {  
> > > +			#address-cells = <2>;
> > > +			#size-cells = <1>;
> > > +			ranges;
> > > +			atmel,generic-dev;  
> > > +			atmel,read-mode = "nrd";
> > > +			atmel,write-mode = "nwe";
> > > +			atmel,bus-width = <16>;
> > > +			atmel,ncs-rd-setup-ns = <0>;
> > > +			atmel,ncs-wr-setup-ns = <0>;
> > > +			atmel,nwe-setup-ns = <8>;
> > > +			atmel,nrd-setup-ns = <16>;
> > > +			atmel,ncs-rd-pulse-ns = <84>;
> > > +			atmel,ncs-wr-pulse-ns = <84>;
> > > +			atmel,nrd-pulse-ns = <76>;
> > > +			atmel,nwe-pulse-ns = <76>;
> > > +			atmel,nrd-cycle-ns = <107>;
> > > +			atmel,nwe-cycle-ns = <84>;
> > > +			atmel,tdf-ns = <16>;
> > > +
> > > +			nor: flash@0,0 {
> > > +				compatible = "cfi-flash";
> > > +				#address-cells = <1>;
> > > +				#size-cells = <1>;
> > > +				reg = <0x0 0x0 0x1000000>;  
> > 
> > It feels odd that in the node for chipselect N, sub-devices have to
> > encode the chipselect number in their reg, when it's obvious from their
> > container. It may make more sense for the cs node to have a non-empty
> > reg (or somehow to make that translation/truncation implicit).
> 
> Would you agree with the following representation?
> 
> 	ebi: ebi@10000000 {
> 		compatible = "atmel,sama5d3-ebi";
> 		#address-cells = <2>;
> 		#size-cells = <1>;
> 		atmel,smc = <&hsmc>;
> 		atmel,matrix = <&matrix>;
> 		reg = <0x10000000 0x10000000
> 		       0x40000000 0x30000000>;
> 		ranges = <0x0 0x0 0x10000000 0x10000000
> 			  0x1 0x0 0x40000000 0x10000000
> 			  0x2 0x0 0x50000000 0x10000000
> 			  0x3 0x0 0x60000000 0x10000000>;
> 		clocks = <&mck>;
> 
> 		pinctrl-names = "default";
> 		pinctrl-0 = <&pinctrl_ebi_addr>;
> 
> 		configs {
> 			config-0 {
> 				atmel,read-mode = "nrd";
> 				atmel,write-mode = "nwe";
> 				atmel,bus-width = <16>;
> 				atmel,ncs-rd-setup-ns = <0>;
> 				atmel,ncs-wr-setup-ns = <0>;
> 				atmel,nwe-setup-ns = <8>;
> 				atmel,nrd-setup-ns = <16>;
> 				atmel,ncs-rd-pulse-ns = <84>;
> 				atmel,ncs-wr-pulse-ns = <84>;
> 				atmel,nrd-pulse-ns = <76>;
> 				atmel,nwe-pulse-ns = <76>;
> 				atmel,nrd-cycle-ns = <107>;
> 				atmel,nwe-cycle-ns = <84>;
> 				atmel,tdf-ns = <16>;
> 			};
> 		};
> 
> 		nor: flash@0,0 {
> 			compatible = "cfi-flash";
> 			#address-cells = <1>;
> 			#size-cells = <1>;
> 			reg = <0x0 0x0 0x1000000>;
> 			bank-width = <2>;
> 		};
> 	};

Something of that sort looks good to me, yes.

Thanks,
Mark.

Re: [PATCH v6 2/2] memory: atmel-ebi: add DT bindings documentation

[Jean-Jacques Hiblot]

Hi Boris,

i haven't seen this code in a while :) I'm glad you're working on it

2016-04-27 16:35 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> The EBI (External Bus Interface) is used to access external peripherals
> (NOR, SRAM, NAND, and other specific devices like ethernet controllers).
> Each device is assigned a CS line and an address range and can have its
> own configuration (timings, access mode, bus width, ...).
> This driver provides a generic DT binding to configure a device according
> to its requirements.
> For specific device controllers (like the NAND one) the SMC timings
> should be configured by the controller driver through the matrix and smc
> syscon regmaps.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  .../bindings/memory-controllers/atmel,ebi.txt      | 146 +++++++++++++++++++++
>  1 file changed, 146 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
>
> diff --git a/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> new file mode 100644
> index 0000000..0f332d2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> @@ -0,0 +1,146 @@
> +* Device tree bindings for Atmel EBI
> +
> +The External Bus Interface (EBI) controller is a bus where you can connect
> +asynchronous (NAND, NOR, SRAM, ....) and synchronous memories (SDR/DDR SDRAMs).
> +The EBI provides a glue-less interface to asynchronous memories though the SMC
> +(Static Memory Controller).
> +Synchronous memories (and some asynchronous memories like NANDs) can be
> +attached to specialized controllers which are responsible for configuring the
> +bus appropriately according to the connected device.
> +In the other hand, the bus interface can be automated for simple asynchronous
> +devices.
> +
> +Required properties:
> +
> +- compatible:          "atmel,at91sam9260-ebi"
> +                       "atmel,at91sam9261-ebi"
> +                       "atmel,at91sam9263-ebi0"
> +                       "atmel,at91sam9263-ebi1"
> +                       "atmel,at91sam9rl-ebi"
> +                       "atmel,at91sam9g45-ebi"
> +                       "atmel,at91sam9x5-ebi"
> +                       "atmel,sama5d3-ebi"
> +
> +- reg:                 Contains offset/length value for EBI memory mapping.
> +                       This property might contain several entries if the EBI
> +                       memory range is not contiguous
> +
> +- #address-cells:      Must be 2.
> +                       The first cell encodes the CS.
> +                       The second cell encode the offset into the CS memory
> +                       range.
> +
> +- #size-cells:         Must be set to 1.
> +
> +- ranges:              Encodes CS to memory region association.
> +
> +- clocks:              Clock feeding the EBI controller.
> +                       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.
> +There might be board specific devices like FPGAs.
> +You'll define you device requirements in these child nodes.
> +
> +Required child cs node properties:
> +
> +- #address-cells:      Must be 2.
> +
> +- #size-cells:         Must be 1.
> +
> +- ranges:              Empty property indicating that child nodes can inherit
> +                       memory layout.
> +
> +Optional child cs node properties:
> +- atmel,bus-width:             width of the asynchronous device's data bus
> +                               8, 16 or 32.
> +                               8 if not present.
> +
> +- atmel,byte-access-type       "write" or "select" (see Atmel datasheet).
> +                               "select" if not present.
> +
> +- atmel,read-mode              "nrd" or "ncs".
> +                               "ncs" is not present.
> +
> +- atmel,write-mode             "nwe" or "ncs".
> +                               "ncs" is not present.
> +
> +- atmel,exnw-mode              "disabled", "frozen" or "ready".
> +                               "disabled" if not present.
> +
> +- atmel,page-mode              enable page mode if present. The provided value
> +                               defines the page size (supported values: 4, 8,
> +                               16 and 32).
> +
> +Optional device timings expressed in nanoseconds (if the property is not
> +present 0 is assumed):

IMO If no timing information is provided, it's probably better not to
change it. It may have been configured by the bootloader or romboot.
It may not be the perfect timings but at least it could work, whereas
filling up with 0 is most probably going to break the access: it's a
kind of very agressive timing optimization

> +
> +- atmel,ncs-rd-setup-ns
> +- atmel,nrd-setup-ns
> +- atmel,ncs-wr-setup-ns
> +- atmel,nwe-setup-ns
> +- atmel,ncs-rd-pulse-ns
> +- atmel,nrd-pulse-ns
> +- atmel,ncs-wr-pulse-ns
> +- atmel,nwe-pulse-ns
> +- atmel,nwe-cycle-ns
> +- atmel,nrd-cycle-ns
> +- atmel,tdf-ns

One thought about the configuration in 'ns' unit: Some devices may
have requirements expressed in clock cycles (I'm thinking of FPGA
here). At  a fixed frequency one can always convert manually from 'ns'
to 'clocks' but it's a bit tedious and prone to rounding errors. And
It 'll  break when the EBI frequency is changed

JJ

> +
> +- atmel,tdf-mode:              "normal" or "optimized". If set to "optimized"
> +                               the data float time is optimized depending on
> +                               the next device being accessed (next device
> +                               setup time is subtracted to the current device
> +                               data float time).
> +
> +
> +
> +Example:
> +
> +       ebi: ebi@10000000 {
> +               compatible = "atmel,sama5d3-ebi", "simple-bus";
> +               #address-cells = <2>;
> +               #size-cells = <1>;
> +               atmel,smc = <&hsmc>;
> +               atmel,matrix = <&matrix>;
> +               reg = <0x10000000 0x10000000
> +                      0x40000000 0x30000000>;
> +               ranges = <0x0 0x0 0x10000000 0x10000000
> +                         0x1 0x0 0x40000000 0x10000000
> +                         0x2 0x0 0x50000000 0x10000000
> +                         0x3 0x0 0x60000000 0x10000000>;
> +               clocks = <&mck>;
> +
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&pinctrl_ebi_addr>;
> +
> +               cs@0 {
> +                       #address-cells = <2>;
> +                       #size-cells = <1>;
> +                       ranges;
> +                       atmel,generic-dev;
> +                       atmel,read-mode = "nrd";
> +                       atmel,write-mode = "nwe";
> +                       atmel,bus-width = <16>;
> +                       atmel,ncs-rd-setup-ns = <0>;
> +                       atmel,ncs-wr-setup-ns = <0>;
> +                       atmel,nwe-setup-ns = <8>;
> +                       atmel,nrd-setup-ns = <16>;
> +                       atmel,ncs-rd-pulse-ns = <84>;
> +                       atmel,ncs-wr-pulse-ns = <84>;
> +                       atmel,nrd-pulse-ns = <76>;
> +                       atmel,nwe-pulse-ns = <76>;
> +                       atmel,nrd-cycle-ns = <107>;
> +                       atmel,nwe-cycle-ns = <84>;
> +                       atmel,tdf-ns = <16>;
> +
> +                       nor: flash@0,0 {
> +                               compatible = "cfi-flash";
> +                               #address-cells = <1>;
> +                               #size-cells = <1>;
> +                               reg = <0x0 0x0 0x1000000>;
> +                               bank-width = <2>;
> +                       };
> +               };
> +       };
> +
> --
> 2.7.4
>

Re: [PATCH v6 2/2] memory: atmel-ebi: add DT bindings documentation

[Boris Brezillon]

Hi Jean-Jacques,

On Thu, 28 Apr 2016 10:32:49 +0200
Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote:

> Hi Boris,
> 
> i haven't seen this code in a while :) I'm glad you're working on it
> 
> 2016-04-27 16:35 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > The EBI (External Bus Interface) is used to access external peripherals
> > (NOR, SRAM, NAND, and other specific devices like ethernet controllers).
> > Each device is assigned a CS line and an address range and can have its
> > own configuration (timings, access mode, bus width, ...).
> > This driver provides a generic DT binding to configure a device according
> > to its requirements.
> > For specific device controllers (like the NAND one) the SMC timings
> > should be configured by the controller driver through the matrix and smc
> > syscon regmaps.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  .../bindings/memory-controllers/atmel,ebi.txt      | 146 +++++++++++++++++++++
> >  1 file changed, 146 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> >
> > diff --git a/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> > new file mode 100644
> > index 0000000..0f332d2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> > @@ -0,0 +1,146 @@
> > +* Device tree bindings for Atmel EBI
> > +
> > +The External Bus Interface (EBI) controller is a bus where you can connect
> > +asynchronous (NAND, NOR, SRAM, ....) and synchronous memories (SDR/DDR SDRAMs).
> > +The EBI provides a glue-less interface to asynchronous memories though the SMC
> > +(Static Memory Controller).
> > +Synchronous memories (and some asynchronous memories like NANDs) can be
> > +attached to specialized controllers which are responsible for configuring the
> > +bus appropriately according to the connected device.
> > +In the other hand, the bus interface can be automated for simple asynchronous
> > +devices.
> > +
> > +Required properties:
> > +
> > +- compatible:          "atmel,at91sam9260-ebi"
> > +                       "atmel,at91sam9261-ebi"
> > +                       "atmel,at91sam9263-ebi0"
> > +                       "atmel,at91sam9263-ebi1"
> > +                       "atmel,at91sam9rl-ebi"
> > +                       "atmel,at91sam9g45-ebi"
> > +                       "atmel,at91sam9x5-ebi"
> > +                       "atmel,sama5d3-ebi"
> > +
> > +- reg:                 Contains offset/length value for EBI memory mapping.
> > +                       This property might contain several entries if the EBI
> > +                       memory range is not contiguous
> > +
> > +- #address-cells:      Must be 2.
> > +                       The first cell encodes the CS.
> > +                       The second cell encode the offset into the CS memory
> > +                       range.
> > +
> > +- #size-cells:         Must be set to 1.
> > +
> > +- ranges:              Encodes CS to memory region association.
> > +
> > +- clocks:              Clock feeding the EBI controller.
> > +                       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.
> > +There might be board specific devices like FPGAs.
> > +You'll define you device requirements in these child nodes.
> > +
> > +Required child cs node properties:
> > +
> > +- #address-cells:      Must be 2.
> > +
> > +- #size-cells:         Must be 1.
> > +
> > +- ranges:              Empty property indicating that child nodes can inherit
> > +                       memory layout.
> > +
> > +Optional child cs node properties:
> > +- atmel,bus-width:             width of the asynchronous device's data bus
> > +                               8, 16 or 32.
> > +                               8 if not present.
> > +
> > +- atmel,byte-access-type       "write" or "select" (see Atmel datasheet).
> > +                               "select" if not present.
> > +
> > +- atmel,read-mode              "nrd" or "ncs".
> > +                               "ncs" is not present.
> > +
> > +- atmel,write-mode             "nwe" or "ncs".
> > +                               "ncs" is not present.
> > +
> > +- atmel,exnw-mode              "disabled", "frozen" or "ready".
> > +                               "disabled" if not present.
> > +
> > +- atmel,page-mode              enable page mode if present. The provided value
> > +                               defines the page size (supported values: 4, 8,
> > +                               16 and 32).
> > +
> > +Optional device timings expressed in nanoseconds (if the property is not
> > +present 0 is assumed):  
> 
> IMO If no timing information is provided, it's probably better not to
> change it. It may have been configured by the bootloader or romboot.
> It may not be the perfect timings but at least it could work, whereas
> filling up with 0 is most probably going to break the access: it's a
> kind of very agressive timing optimization

Yes, probably. I'll make all those timings mandatory in the next
version.

> 
> > +
> > +- atmel,ncs-rd-setup-ns
> > +- atmel,nrd-setup-ns
> > +- atmel,ncs-wr-setup-ns
> > +- atmel,nwe-setup-ns
> > +- atmel,ncs-rd-pulse-ns
> > +- atmel,nrd-pulse-ns
> > +- atmel,ncs-wr-pulse-ns
> > +- atmel,nwe-pulse-ns
> > +- atmel,nwe-cycle-ns
> > +- atmel,nrd-cycle-ns
> > +- atmel,tdf-ns  
> 
> One thought about the configuration in 'ns' unit: Some devices may
> have requirements expressed in clock cycles (I'm thinking of FPGA
> here). At  a fixed frequency one can always convert manually from 'ns'
> to 'clocks' but it's a bit tedious and prone to rounding errors. And
> It 'll  break when the EBI frequency is changed

If you don't mind, I'd like to first get this version accepted, and
we'll extend it with timings expressed in clock cycles afterward.

BTW, could you describe a real use case where timings should be
expressed in clock cycles? I mean, usually the devices have some timing
constraints (tXX_min = Y ns), and I don't see why it would differ for
FPGA interfaces, but I'm clearly not an FPGA expert.

Thanks,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v6 2/2] memory: atmel-ebi: add DT bindings documentation

[Jean-Jacques Hiblot]

2016-04-28 10:49 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> Hi Jean-Jacques,
>
> On Thu, 28 Apr 2016 10:32:49 +0200
> Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote:
>
>> Hi Boris,
>>
>> i haven't seen this code in a while :) I'm glad you're working on it
>>
>> 2016-04-27 16:35 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> > The EBI (External Bus Interface) is used to access external peripherals
>> > (NOR, SRAM, NAND, and other specific devices like ethernet controllers).
>> > Each device is assigned a CS line and an address range and can have its
>> > own configuration (timings, access mode, bus width, ...).
>> > This driver provides a generic DT binding to configure a device according
>> > to its requirements.
>> > For specific device controllers (like the NAND one) the SMC timings
>> > should be configured by the controller driver through the matrix and smc
>> > syscon regmaps.
>> >
>> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>> > ---
>> >  .../bindings/memory-controllers/atmel,ebi.txt      | 146 +++++++++++++++++++++
>> >  1 file changed, 146 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
>> > new file mode 100644
>> > index 0000000..0f332d2
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
>> > @@ -0,0 +1,146 @@
>> > +* Device tree bindings for Atmel EBI
>> > +
>> > +The External Bus Interface (EBI) controller is a bus where you can connect
>> > +asynchronous (NAND, NOR, SRAM, ....) and synchronous memories (SDR/DDR SDRAMs).
>> > +The EBI provides a glue-less interface to asynchronous memories though the SMC
>> > +(Static Memory Controller).
>> > +Synchronous memories (and some asynchronous memories like NANDs) can be
>> > +attached to specialized controllers which are responsible for configuring the
>> > +bus appropriately according to the connected device.
>> > +In the other hand, the bus interface can be automated for simple asynchronous
>> > +devices.
>> > +
>> > +Required properties:
>> > +
>> > +- compatible:          "atmel,at91sam9260-ebi"
>> > +                       "atmel,at91sam9261-ebi"
>> > +                       "atmel,at91sam9263-ebi0"
>> > +                       "atmel,at91sam9263-ebi1"
>> > +                       "atmel,at91sam9rl-ebi"
>> > +                       "atmel,at91sam9g45-ebi"
>> > +                       "atmel,at91sam9x5-ebi"
>> > +                       "atmel,sama5d3-ebi"
>> > +
>> > +- reg:                 Contains offset/length value for EBI memory mapping.
>> > +                       This property might contain several entries if the EBI
>> > +                       memory range is not contiguous
>> > +
>> > +- #address-cells:      Must be 2.
>> > +                       The first cell encodes the CS.
>> > +                       The second cell encode the offset into the CS memory
>> > +                       range.
>> > +
>> > +- #size-cells:         Must be set to 1.
>> > +
>> > +- ranges:              Encodes CS to memory region association.
>> > +
>> > +- clocks:              Clock feeding the EBI controller.
>> > +                       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.
>> > +There might be board specific devices like FPGAs.
>> > +You'll define you device requirements in these child nodes.
>> > +
>> > +Required child cs node properties:
>> > +
>> > +- #address-cells:      Must be 2.
>> > +
>> > +- #size-cells:         Must be 1.
>> > +
>> > +- ranges:              Empty property indicating that child nodes can inherit
>> > +                       memory layout.
>> > +
>> > +Optional child cs node properties:
>> > +- atmel,bus-width:             width of the asynchronous device's data bus
>> > +                               8, 16 or 32.
>> > +                               8 if not present.
>> > +
>> > +- atmel,byte-access-type       "write" or "select" (see Atmel datasheet).
>> > +                               "select" if not present.
>> > +
>> > +- atmel,read-mode              "nrd" or "ncs".
>> > +                               "ncs" is not present.
>> > +
>> > +- atmel,write-mode             "nwe" or "ncs".
>> > +                               "ncs" is not present.
>> > +
>> > +- atmel,exnw-mode              "disabled", "frozen" or "ready".
>> > +                               "disabled" if not present.
>> > +
>> > +- atmel,page-mode              enable page mode if present. The provided value
>> > +                               defines the page size (supported values: 4, 8,
>> > +                               16 and 32).
>> > +
>> > +Optional device timings expressed in nanoseconds (if the property is not
>> > +present 0 is assumed):
>>
>> IMO If no timing information is provided, it's probably better not to
>> change it. It may have been configured by the bootloader or romboot.
>> It may not be the perfect timings but at least it could work, whereas
>> filling up with 0 is most probably going to break the access: it's a
>> kind of very agressive timing optimization
>
> Yes, probably. I'll make all those timings mandatory in the next
> version.
>
>>
>> > +
>> > +- atmel,ncs-rd-setup-ns
>> > +- atmel,nrd-setup-ns
>> > +- atmel,ncs-wr-setup-ns
>> > +- atmel,nwe-setup-ns
>> > +- atmel,ncs-rd-pulse-ns
>> > +- atmel,nrd-pulse-ns
>> > +- atmel,ncs-wr-pulse-ns
>> > +- atmel,nwe-pulse-ns
>> > +- atmel,nwe-cycle-ns
>> > +- atmel,nrd-cycle-ns
>> > +- atmel,tdf-ns
>>
>> One thought about the configuration in 'ns' unit: Some devices may
>> have requirements expressed in clock cycles (I'm thinking of FPGA
>> here). At  a fixed frequency one can always convert manually from 'ns'
>> to 'clocks' but it's a bit tedious and prone to rounding errors. And
>> It 'll  break when the EBI frequency is changed
>
> If you don't mind, I'd like to first get this version accepted, and
> we'll extend it with timings expressed in clock cycles afterward.
>
> BTW, could you describe a real use case where timings should be
> expressed in clock cycles? I mean, usually the devices have some timing
> constraints (tXX_min = Y ns), and I don't see why it would differ for
> FPGA interfaces, but I'm clearly not an FPGA expert.

I'm not either, I only toyed with FPGA. That's just what experienced
FPGA designer told me.
I guess that it boils down to: FPGA are more suited for a synchronous
design than an asynchronous one.

JJ

>
> Thanks,
>
> Boris
>
> --
> Boris Brezillon, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 2/2] memory: atmel-ebi: add DT bindings documentation

[Boris Brezillon]

On Thu, 28 Apr 2016 14:18:25 +0200
Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote:
> >> > +
> >> > +- atmel,ncs-rd-setup-ns
> >> > +- atmel,nrd-setup-ns
> >> > +- atmel,ncs-wr-setup-ns
> >> > +- atmel,nwe-setup-ns
> >> > +- atmel,ncs-rd-pulse-ns
> >> > +- atmel,nrd-pulse-ns
> >> > +- atmel,ncs-wr-pulse-ns
> >> > +- atmel,nwe-pulse-ns
> >> > +- atmel,nwe-cycle-ns
> >> > +- atmel,nrd-cycle-ns
> >> > +- atmel,tdf-ns  
> >>
> >> One thought about the configuration in 'ns' unit: Some devices may
> >> have requirements expressed in clock cycles (I'm thinking of FPGA
> >> here). At  a fixed frequency one can always convert manually from 'ns'
> >> to 'clocks' but it's a bit tedious and prone to rounding errors. And
> >> It 'll  break when the EBI frequency is changed  
> >
> > If you don't mind, I'd like to first get this version accepted, and
> > we'll extend it with timings expressed in clock cycles afterward.
> >
> > BTW, could you describe a real use case where timings should be
> > expressed in clock cycles? I mean, usually the devices have some timing
> > constraints (tXX_min = Y ns), and I don't see why it would differ for
> > FPGA interfaces, but I'm clearly not an FPGA expert.  
> 
> I'm not either, I only toyed with FPGA. That's just what experienced
> FPGA designer told me.
> I guess that it boils down to: FPGA are more suited for a synchronous
> design than an asynchronous one.

The thing is, all the timings are based on the master clock, and,
AFAICS, this clk signal is not exposed, so you're basing your clk-cycle
based timings on something that can change depending on how the
bootstrap/bootloader decided to configure the master clk.

One option would be to define one of the timing as the reference,
define this one in nanosecond, and define the other ones as multiple of
the reference timing. But I'm not sure it's easier to do that than
defining all the timings directly in nanoseconds.

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v6 2/2] memory: atmel-ebi: add DT bindings documentation

[Jean-Jacques Hiblot]

2016-04-28 14:46 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Thu, 28 Apr 2016 14:18:25 +0200
> Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote:
>> >> > +
>> >> > +- atmel,ncs-rd-setup-ns
>> >> > +- atmel,nrd-setup-ns
>> >> > +- atmel,ncs-wr-setup-ns
>> >> > +- atmel,nwe-setup-ns
>> >> > +- atmel,ncs-rd-pulse-ns
>> >> > +- atmel,nrd-pulse-ns
>> >> > +- atmel,ncs-wr-pulse-ns
>> >> > +- atmel,nwe-pulse-ns
>> >> > +- atmel,nwe-cycle-ns
>> >> > +- atmel,nrd-cycle-ns
>> >> > +- atmel,tdf-ns
>> >>
>> >> One thought about the configuration in 'ns' unit: Some devices may
>> >> have requirements expressed in clock cycles (I'm thinking of FPGA
>> >> here). At  a fixed frequency one can always convert manually from 'ns'
>> >> to 'clocks' but it's a bit tedious and prone to rounding errors. And
>> >> It 'll  break when the EBI frequency is changed
>> >
>> > If you don't mind, I'd like to first get this version accepted, and
>> > we'll extend it with timings expressed in clock cycles afterward.
>> >
>> > BTW, could you describe a real use case where timings should be
>> > expressed in clock cycles? I mean, usually the devices have some timing
>> > constraints (tXX_min = Y ns), and I don't see why it would differ for
>> > FPGA interfaces, but I'm clearly not an FPGA expert.
>>
>> I'm not either, I only toyed with FPGA. That's just what experienced
>> FPGA designer told me.
>> I guess that it boils down to: FPGA are more suited for a synchronous
>> design than an asynchronous one.
>
> The thing is, all the timings are based on the master clock, and,
> AFAICS, this clk signal is not exposed, so you're basing your clk-cycle
while EBI itself is asynchronous, the clk can be exposed through one
of the PCK. I've seen this in real projects.
> based timings on something that can change depending on how the
> bootstrap/bootloader decided to configure the master clk.
>
> One option would be to define one of the timing as the reference,
> define this one in nanosecond, and define the other ones as multiple of
> the reference timing. But I'm not sure it's easier to do that than
> defining all the timings directly in nanoseconds.
>
> --
> Boris Brezillon, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 2/2] memory: atmel-ebi: add DT bindings documentation

[Boris Brezillon]

On Thu, 28 Apr 2016 14:54:39 +0200
Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote:

> 2016-04-28 14:46 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Thu, 28 Apr 2016 14:18:25 +0200
> > Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote:  
> >> >> > +
> >> >> > +- atmel,ncs-rd-setup-ns
> >> >> > +- atmel,nrd-setup-ns
> >> >> > +- atmel,ncs-wr-setup-ns
> >> >> > +- atmel,nwe-setup-ns
> >> >> > +- atmel,ncs-rd-pulse-ns
> >> >> > +- atmel,nrd-pulse-ns
> >> >> > +- atmel,ncs-wr-pulse-ns
> >> >> > +- atmel,nwe-pulse-ns
> >> >> > +- atmel,nwe-cycle-ns
> >> >> > +- atmel,nrd-cycle-ns
> >> >> > +- atmel,tdf-ns  
> >> >>
> >> >> One thought about the configuration in 'ns' unit: Some devices may
> >> >> have requirements expressed in clock cycles (I'm thinking of FPGA
> >> >> here). At  a fixed frequency one can always convert manually from 'ns'
> >> >> to 'clocks' but it's a bit tedious and prone to rounding errors. And
> >> >> It 'll  break when the EBI frequency is changed  
> >> >
> >> > If you don't mind, I'd like to first get this version accepted, and
> >> > we'll extend it with timings expressed in clock cycles afterward.
> >> >
> >> > BTW, could you describe a real use case where timings should be
> >> > expressed in clock cycles? I mean, usually the devices have some timing
> >> > constraints (tXX_min = Y ns), and I don't see why it would differ for
> >> > FPGA interfaces, but I'm clearly not an FPGA expert.  
> >>
> >> I'm not either, I only toyed with FPGA. That's just what experienced
> >> FPGA designer told me.
> >> I guess that it boils down to: FPGA are more suited for a synchronous
> >> design than an asynchronous one.  
> >
> > The thing is, all the timings are based on the master clock, and,
> > AFAICS, this clk signal is not exposed, so you're basing your clk-cycle  
> while EBI itself is asynchronous, the clk can be exposed through one
> of the PCK. I've seen this in real projects.

Okay, then it makes sense. But if you need such a complex thing it
would probably be better to create a new driver and let this driver
adapt the timings dynamically (I plan to expose an few fonctions for
the NAND controller, so it would be possible to create an FPGA driver
referencing the PCK clk and adapting the EBI timings).

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com