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

[PATCH v7 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>
---
Changes since v6:
- rework the binding to put per-device config information into a
  separate subnode and have devices connected to the EBI bus
  defined as direct children of the EBI node
- make all EBI timings mandatory
- keep adding sub-devices when a failure occurs on one of them

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 :-)
 drivers/memory/Kconfig     |  11 +
 drivers/memory/Makefile    |   1 +
 drivers/memory/atmel-ebi.c | 681 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 693 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..7f6b70b
--- /dev/null
+++ b/drivers/memory/atmel-ebi.c
@@ -0,0 +1,681 @@
+/*
+ * 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 {
+	int cs;
+	union {
+		struct at91sam9_ebi_dev_config sam9;
+	};
+};
+
+struct at91_ebi;
+
+struct at91_ebi_dev {
+	struct list_head node;
+	struct at91_ebi *ebi;
+	u32 mode;
+	int numcs;
+	struct at91_ebi_dev_config configs[];
+};
+
+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 device_node *configs_np,
+			    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 list_head devs;
+	union {
+		struct at91sam9_smc_generic_fields sam9;
+	};
+};
+
+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->sam9;
+	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, conf->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, conf->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, conf->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, conf->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 device_node *np,
+				       struct at91sam9_smc_timings *timings)
+{
+	int ret;
+
+	ret = of_property_read_u32(np, "atmel,ncs-rd-setup-ns",
+				   &timings->ncs_rd_setup_ns);
+	ret |= of_property_read_u32(np, "atmel,nrd-setup-ns",
+				    &timings->nrd_setup_ns);
+	ret |= of_property_read_u32(np, "atmel,ncs-wr-setup-ns",
+				    &timings->ncs_wr_setup_ns);
+	ret |= of_property_read_u32(np, "atmel,nwe-setup-ns",
+				    &timings->nwe_setup_ns);
+	ret |= of_property_read_u32(np, "atmel,ncs-rd-pulse-ns",
+				    &timings->ncs_rd_pulse_ns);
+	ret |= of_property_read_u32(np, "atmel,nrd-pulse-ns",
+				    &timings->nrd_pulse_ns);
+	ret |= of_property_read_u32(np, "atmel,ncs-wr-pulse-ns",
+				    &timings->ncs_wr_pulse_ns);
+	ret |= of_property_read_u32(np, "atmel,nwe-pulse-ns",
+				    &timings->nwe_pulse_ns);
+	ret |= of_property_read_u32(np, "atmel,nwe-cycle-ns",
+				    &timings->nwe_cycle_ns);
+	ret |= of_property_read_u32(np, "atmel,nrd-cycle-ns",
+				    &timings->nrd_cycle_ns);
+	ret |= of_property_read_u32(np, "atmel,tdf-ns",
+				    &timings->tdf_ns);
+
+	if (ret) {
+		dev_err(ebid->ebi->dev,
+			"missing or invalid timings definition in %s",
+			np->name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int at91sam9_ebi_xslate_config(struct at91_ebi_dev *ebid,
+				      struct device_node *np,
+				      struct at91_ebi_dev_config *conf)
+{
+	struct at91sam9_ebi_dev_config *config = &conf->sam9;
+	const char *tmp_str;
+	u32 tmp;
+	int ret;
+
+	ret = of_property_read_u32(np, "atmel,bus-width", &tmp);
+	if (!ret) {
+		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 && !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 && !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 && !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 && !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) {
+		if (!strcmp(tmp_str, "frozen"))
+			config->mode |= AT91_SMC_EXNWMODE_FROZEN;
+		else if (!strcmp(tmp_str, "ready"))
+			config->mode |= AT91_SMC_EXNWMODE_READY;
+	}
+
+	ret = of_property_read_u32(np, "atmel,page-mode", &tmp);
+	if (!ret) {
+		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, np, &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->sam9;
+	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, conf->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, conf->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, conf->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, conf->cs,
+			    config->mode | AT91_SMC_TDF_(val));
+
+	return 0;
+}
+
+static int at91sam9_ebi_init(struct at91_ebi *ebi)
+{
+	struct at91sam9_smc_generic_fields *fields = &ebi->sam9;
+	struct reg_field field = REG_FIELD(0, 0, 31);
+
+	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);
+
+	return 0;
+}
+
+static int sama5d3_ebi_init(struct at91_ebi *ebi)
+{
+	struct at91sam9_smc_generic_fields *fields = &ebi->sam9;
+	struct reg_field field = REG_FIELD(0, 0, 31);
+
+	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);
+
+	return 0;
+}
+
+static int at91_ebi_dev_setup(struct at91_ebi *ebi, struct device_node *np,
+			      struct device_node *configs, int reg_cells)
+{
+	const struct at91_ebi_caps *caps = ebi->caps;
+	struct device *dev = ebi->dev;
+	struct at91_ebi_dev *ebid;
+	int ret, numcs = 0, i;
+
+	numcs = of_property_count_elems_of_size(np, "reg",
+						reg_cells * sizeof(u32));
+	if (numcs <= 0) {
+		dev_err(dev, "invalid reg property\n");
+		return -EINVAL;
+	}
+
+	ebid = devm_kzalloc(ebi->dev,
+			    sizeof(*ebid) + (numcs * sizeof(*ebid->configs)),
+			    GFP_KERNEL);
+	if (!ebid)
+		return -ENOMEM;
+
+	ebid->ebi = ebi;
+
+	for (i = 0; i < numcs; i++) {
+		struct at91_ebi_dev_config *conf = &ebid->configs[i];
+		struct device_node *conf_np;
+		char conf_name[16];
+		u32 cs;
+
+		ret = of_property_read_u32_index(np, "reg", i * reg_cells,
+						 &cs);
+		if (ret)
+			return ret;
+
+		if (cs > AT91_MATRIX_EBI_NUM_CS ||
+		    !(ebi->caps->available_cs & BIT(cs))) {
+			dev_err(dev, "invalid reg property\n");
+			return -EINVAL;
+		}
+
+		conf->cs = cs;
+		snprintf(conf_name, sizeof(conf_name), "config-%d", cs);
+		conf_np = of_get_child_by_name(configs, conf_name);
+		if (conf_np) {
+			ret = caps->xlate_config(ebid, conf_np, conf);
+			if (ret)
+				return ret;
+
+			ret = caps->apply_config(ebid, conf);
+			if (ret)
+				return ret;
+
+			/* Attach the EBI device to the generic SMC logic. */
+			if (ebi->ebi_csa)
+				regmap_field_update_bits(ebi->ebi_csa,
+							 BIT(cs), 0);
+			of_node_put(conf_np);
+
+		}
+
+		caps->get_config(ebid, conf);
+	}
+
+	list_add_tail(&ebid->node, &ebi->devs);
+
+	return 0;
+}
+
+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,
+	.get_config = at91sam9_ebi_get_config,
+	.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,
+	.get_config = at91sam9_ebi_get_config,
+	.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,
+	.get_config = at91sam9_ebi_get_config,
+	.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,
+	.get_config = at91sam9_ebi_get_config,
+	.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,
+	.get_config = at91sam9_ebi_get_config,
+	.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,
+	.get_config = at91sam9_ebi_get_config,
+	.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,
+	.get_config = at91sam9_ebi_get_config,
+	.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,
+	.get_config = at91sam9_ebi_get_config,
+	.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)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *child, *configs, *np = dev->of_node;
+	const struct of_device_id *match;
+	struct at91_ebi *ebi;
+	int ret, reg_cells;
+	struct clk *clk;
+	u32 val;
+
+	match = of_match_device(at91_ebi_id_table, dev);
+	if (!match || !match->data)
+		return -EINVAL;
+
+	ebi = devm_kzalloc(dev, sizeof(*ebi), GFP_KERNEL);
+	if (!ebi)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&ebi->devs);
+	ebi->caps = match->data;
+	ebi->dev = dev;
+
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ebi->clk = clk;
+
+	ebi->smc = syscon_regmap_lookup_by_phandle(np, "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(np, "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;
+
+	configs = of_get_child_by_name(np, "configs");
+
+	ret = of_property_read_u32(np, "#address-cells", &val);
+	if (ret) {
+		dev_err(dev, "missing #address-cells property\n");
+		return ret;
+	}
+
+	reg_cells = val;
+
+	ret = of_property_read_u32(np, "#size-cells", &val);
+	if (ret) {
+		dev_err(dev, "missing #address-cells property\n");
+		return ret;
+	}
+
+	reg_cells += val;
+
+	for_each_available_child_of_node(np, child) {
+		if (!of_find_property(child, "reg", NULL))
+			continue;
+
+		ret = at91_ebi_dev_setup(ebi, child, configs, reg_cells);
+		if (ret)
+			dev_err(dev, "failed to instantiate %s",
+				child->name);
+	}
+	of_node_put(configs);
+
+	return of_platform_populate(np, NULL, NULL, dev);
+}
+
+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

[PATCH v7 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      | 136 +++++++++++++++++++++
 1 file changed, 136 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..a6dca5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
@@ -0,0 +1,136 @@
+* 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 through the SMC
+(Static Memory Controller).
+
+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
+
+Children device nodes are representing device connected to the EBI bus.
+
+Required device node properties:
+
+- #reg:			Contains the chip-select id, the offset and the length
+			of the memory region requested by the device.
+
+EBI bus configuration associated with specific chip-select will be defined in
+the configs subnode. This configs node will in turn contain several subnodes
+named config-<cs-id>, each of them containing the following properties.
+
+Optional config-<cs-id> node properties:
+
+- atmel,bus-width:		width of the asynchronous device's data bus
+				8, 16 or 32.
+				Default to 8 when undefined.
+
+- atmel,byte-access-type	"write" or "select" (see Atmel datasheet).
+				Default to "select" when undefined.
+
+- atmel,read-mode		"nrd" or "ncs".
+				Default to "ncs" when undefined.
+
+- atmel,write-mode		"nwe" or "ncs".
+				Default to "ncs" when undefined.
+
+- atmel,exnw-mode		"disabled", "frozen" or "ready".
+				Default to "disabled" when undefined.
+
+- atmel,page-mode		enable page mode if present. The provided value
+				defines the page size (supported values: 4, 8,
+				16 and 32).
+
+- atmel,tdf-mode:		"normal" or "optimized". When 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).
+				Default to "normal" when undefined.
+
+Mandatory timings expressed in nanoseconds (see Atmel datasheet for a full
+description).
+
+- 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
+
+Example:
+
+	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>;
+		};
+	};
+
-- 
2.7.4

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

[Rob Herring]

On Thu, Apr 28, 2016 at 02:03:27PM +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      | 136 +++++++++++++++++++++
>  1 file changed, 136 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..a6dca5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> @@ -0,0 +1,136 @@
> +* 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 through the SMC
> +(Static Memory Controller).
> +
> +Required properties:
> +
> +- compatible:		"atmel,at91sam9260-ebi"
> +			"atmel,at91sam9261-ebi"
> +			"atmel,at91sam9263-ebi0"
> +			"atmel,at91sam9263-ebi1"

What are the differences between 0 and 1?

> +			"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
> +
> +Children device nodes are representing device connected to the EBI bus.
> +
> +Required device node properties:
> +
> +- #reg:			Contains the chip-select id, the offset and the length

s/#reg/reg/

> +			of the memory region requested by the device.
> +
> +EBI bus configuration associated with specific chip-select will be defined in
> +the configs subnode. This configs node will in turn contain several subnodes
> +named config-<cs-id>, each of them containing the following properties.

This is a bit unusual. Why not just part of the child device nodes?

> +
> +Optional config-<cs-id> node properties:
> +
> +- atmel,bus-width:		width of the asynchronous device's data bus
> +				8, 16 or 32.
> +				Default to 8 when undefined.
> +
> +- atmel,byte-access-type	"write" or "select" (see Atmel datasheet).
> +				Default to "select" when undefined.
> +
> +- atmel,read-mode		"nrd" or "ncs".
> +				Default to "ncs" when undefined.
> +
> +- atmel,write-mode		"nwe" or "ncs".
> +				Default to "ncs" when undefined.
> +
> +- atmel,exnw-mode		"disabled", "frozen" or "ready".
> +				Default to "disabled" when undefined.
> +
> +- atmel,page-mode		enable page mode if present. The provided value
> +				defines the page size (supported values: 4, 8,
> +				16 and 32).
> +
> +- atmel,tdf-mode:		"normal" or "optimized". When set to

This should be boolean.

> +				"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).
> +				Default to "normal" when undefined.
> +
> +Mandatory timings expressed in nanoseconds (see Atmel datasheet for a full
> +description).

Required first, then optional properties please.

> +
> +- 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
> +
> +Example:
> +
> +	ebi: ebi@10000000 {
> +		compatible = "atmel,sama5d3-ebi";
> +		#address-cells = <2>;
> +		#size-cells = <1>;

> +		atmel,smc = <&hsmc>;
> +		atmel,matrix = <&matrix>;

What are these? 

> +		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>;

Not documented.

> +
> +		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>;
> +		};
> +	};
> +
> -- 
> 2.7.4
> 

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

[Boris Brezillon]

Hi Rob,

On Tue, 3 May 2016 11:40:19 -0500
Rob Herring <robh@kernel.org> wrote:

> On Thu, Apr 28, 2016 at 02:03:27PM +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      | 136 +++++++++++++++++++++
> >  1 file changed, 136 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..a6dca5c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> > @@ -0,0 +1,136 @@
> > +* 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 through the SMC
> > +(Static Memory Controller).
> > +
> > +Required properties:
> > +
> > +- compatible:		"atmel,at91sam9260-ebi"
> > +			"atmel,at91sam9261-ebi"
> > +			"atmel,at91sam9263-ebi0"
> > +			"atmel,at91sam9263-ebi1"  
> 
> What are the differences between 0 and 1?

Because this SoC has 2 EBI busses with different capabilities.

> 
> > +			"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
> > +
> > +Children device nodes are representing device connected to the EBI bus.
> > +
> > +Required device node properties:
> > +
> > +- #reg:			Contains the chip-select id, the offset and the length  
> 
> s/#reg/reg/

Will fix that.

> 
> > +			of the memory region requested by the device.
> > +
> > +EBI bus configuration associated with specific chip-select will be defined in
> > +the configs subnode. This configs node will in turn contain several subnodes
> > +named config-<cs-id>, each of them containing the following properties.  
> 
> This is a bit unusual. Why not just part of the child device nodes?

Oh, come on! I reworked the binding because Mark complained about the
previous binding which was doing exactly what you're suggesting. Can
you please be consistent in your reviews... 


> 
> > +
> > +Optional config-<cs-id> node properties:
> > +
> > +- atmel,bus-width:		width of the asynchronous device's data bus
> > +				8, 16 or 32.
> > +				Default to 8 when undefined.
> > +
> > +- atmel,byte-access-type	"write" or "select" (see Atmel datasheet).
> > +				Default to "select" when undefined.
> > +
> > +- atmel,read-mode		"nrd" or "ncs".
> > +				Default to "ncs" when undefined.
> > +
> > +- atmel,write-mode		"nwe" or "ncs".
> > +				Default to "ncs" when undefined.
> > +
> > +- atmel,exnw-mode		"disabled", "frozen" or "ready".
> > +				Default to "disabled" when undefined.
> > +
> > +- atmel,page-mode		enable page mode if present. The provided value
> > +				defines the page size (supported values: 4, 8,
> > +				16 and 32).
> > +
> > +- atmel,tdf-mode:		"normal" or "optimized". When set to  
> 
> This should be boolean.

It was a formerly defined as a boolean, and when it's done like that we
have no way to identify whether the property was forgotten or
intentionally set to normal mode. What's the problem with this approach?

> 
> > +				"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).
> > +				Default to "normal" when undefined.
> > +
> > +Mandatory timings expressed in nanoseconds (see Atmel datasheet for a full
> > +description).  
> 
> Required first, then optional properties please.

Okay.

> 
> > +
> > +- 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
> > +
> > +Example:
> > +
> > +	ebi: ebi@10000000 {
> > +		compatible = "atmel,sama5d3-ebi";
> > +		#address-cells = <2>;
> > +		#size-cells = <1>;  
> 
> > +		atmel,smc = <&hsmc>;
> > +		atmel,matrix = <&matrix>;  
> 
> What are these? 
> 
> > +		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>;  
> 
> Not documented.

Will document those properties.

Regards,

Boris


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

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

[Rob Herring]

On Tue, May 3, 2016 at 11:51 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Rob,
>
> On Tue, 3 May 2016 11:40:19 -0500
> Rob Herring <robh@kernel.org> wrote:
>
>> On Thu, Apr 28, 2016 at 02:03:27PM +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      | 136 +++++++++++++++++++++
>> >  1 file changed, 136 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..a6dca5c
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
>> > @@ -0,0 +1,136 @@
>> > +* 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 through the SMC
>> > +(Static Memory Controller).
>> > +
>> > +Required properties:
>> > +
>> > +- compatible:              "atmel,at91sam9260-ebi"
>> > +                   "atmel,at91sam9261-ebi"
>> > +                   "atmel,at91sam9263-ebi0"
>> > +                   "atmel,at91sam9263-ebi1"
>>
>> What are the differences between 0 and 1?
>
> Because this SoC has 2 EBI busses with different capabilities.

Okay, correct answer. :)

[...]

>>
>> > +                   of the memory region requested by the device.
>> > +
>> > +EBI bus configuration associated with specific chip-select will be defined in
>> > +the configs subnode. This configs node will in turn contain several subnodes
>> > +named config-<cs-id>, each of them containing the following properties.
>>
>> This is a bit unusual. Why not just part of the child device nodes?
>
> Oh, come on! I reworked the binding because Mark complained about the
> previous binding which was doing exactly what you're suggesting. Can
> you please be consistent in your reviews...

No, Mark and I both have our opinions. Which part of this patch
explains the history? If the revision history is not in the patch, I'm
not looking at it.

My issue with it this way is that it has invented yet another way to
describe timings. I would like to be consistent across external bus
descriptions, but we're not very consistent to begin with though. The
most common seems to be the way you first did it. But I agree that it
is kind of screwy to have an intermediate node unless the controller
itself has sub-blocks within it and is not the established way to
describe a bus with chip selects. I would either put the properties
directly in the child nodes (e.g. flash@0,0) or put your config nodes
in the device node. I'd call it timings instead of config, but that's
just bikeshedding.

memory-controller@1000 {
  ...
  flash@0,0 {
    timings {
      ...
    };
  };
};

>> > +
>> > +Optional config-<cs-id> node properties:
>> > +
>> > +- atmel,bus-width:         width of the asynchronous device's data bus
>> > +                           8, 16 or 32.
>> > +                           Default to 8 when undefined.
>> > +
>> > +- atmel,byte-access-type   "write" or "select" (see Atmel datasheet).
>> > +                           Default to "select" when undefined.
>> > +
>> > +- atmel,read-mode          "nrd" or "ncs".
>> > +                           Default to "ncs" when undefined.
>> > +
>> > +- atmel,write-mode         "nwe" or "ncs".
>> > +                           Default to "ncs" when undefined.
>> > +
>> > +- atmel,exnw-mode          "disabled", "frozen" or "ready".
>> > +                           Default to "disabled" when undefined.
>> > +
>> > +- atmel,page-mode          enable page mode if present. The provided value
>> > +                           defines the page size (supported values: 4, 8,
>> > +                           16 and 32).
>> > +
>> > +- atmel,tdf-mode:          "normal" or "optimized". When set to
>>
>> This should be boolean.
>
> It was a formerly defined as a boolean, and when it's done like that we
> have no way to identify whether the property was forgotten or
> intentionally set to normal mode. What's the problem with this approach?

Only preference to use boolean when that is sufficient. With your
argument, then we should never have booleans. You state that missing
means "normal", not forgotten, so all these properties should be
required with no default.

BTW, I debated the same thing on the other properties, but I could see
them being expanded to a 3rd mode. I don't see that here though.

Rob

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

[Boris Brezillon]

Hi Rob,

On Tue, 3 May 2016 14:11:04 -0500
Rob Herring <robh@kernel.org> wrote:

> On Tue, May 3, 2016 at 11:51 AM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > Hi Rob,
> >
> > On Tue, 3 May 2016 11:40:19 -0500
> > Rob Herring <robh@kernel.org> wrote:
> >
> >> On Thu, Apr 28, 2016 at 02:03:27PM +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      | 136 +++++++++++++++++++++
> >> >  1 file changed, 136 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..a6dca5c
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> >> > @@ -0,0 +1,136 @@
> >> > +* 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 through the SMC
> >> > +(Static Memory Controller).
> >> > +
> >> > +Required properties:
> >> > +
> >> > +- compatible:              "atmel,at91sam9260-ebi"
> >> > +                   "atmel,at91sam9261-ebi"
> >> > +                   "atmel,at91sam9263-ebi0"
> >> > +                   "atmel,at91sam9263-ebi1"
> >>
> >> What are the differences between 0 and 1?
> >
> > Because this SoC has 2 EBI busses with different capabilities.
> 
> Okay, correct answer. :)
> 
> [...]
> 
> >>
> >> > +                   of the memory region requested by the device.
> >> > +
> >> > +EBI bus configuration associated with specific chip-select will be defined in
> >> > +the configs subnode. This configs node will in turn contain several subnodes
> >> > +named config-<cs-id>, each of them containing the following properties.
> >>
> >> This is a bit unusual. Why not just part of the child device nodes?
> >
> > Oh, come on! I reworked the binding because Mark complained about the
> > previous binding which was doing exactly what you're suggesting. Can
> > you please be consistent in your reviews...
> 
> No, Mark and I both have our opinions. Which part of this patch
> explains the history?

Hm, it's in patch 1/2 (just dropped the cover letter, which might not
be such a good idea).

> If the revision history is not in the patch, I'm
> not looking at it.
> 
> My issue with it this way is that it has invented yet another way to
> describe timings. I would like to be consistent across external bus
> descriptions, but we're not very consistent to begin with though. The
> most common seems to be the way you first did it. But I agree that it
> is kind of screwy to have an intermediate node unless the controller
> itself has sub-blocks within it and is not the established way to
> describe a bus with chip selects. I would either put the properties
> directly in the child nodes (e.g. flash@0,0) or put your config nodes
> in the device node. I'd call it timings instead of config, but that's
> just bikeshedding.

Well, it's not only describing timings (see atmel,bus-width,
atmel,byte-access-type, ...), but I'm fine with either names :).

> 
> memory-controller@1000 {
>   ...
>   flash@0,0 {
>     timings {
>       ...
>     };
>   };
> };

Okay. Mark, what do you think of this approach?

Note that one of my previous version was defining timings directly in
the EBI device node, and Arnd noted that doing so may cause problems
if one of the EBI property (or the config/timing node name) conflict
with the sub-device binding, which is why I decided to put the EBI
config definitions in a separate subnode.

> 
> >> > +
> >> > +Optional config-<cs-id> node properties:
> >> > +
> >> > +- atmel,bus-width:         width of the asynchronous device's data bus
> >> > +                           8, 16 or 32.
> >> > +                           Default to 8 when undefined.
> >> > +
> >> > +- atmel,byte-access-type   "write" or "select" (see Atmel datasheet).
> >> > +                           Default to "select" when undefined.
> >> > +
> >> > +- atmel,read-mode          "nrd" or "ncs".
> >> > +                           Default to "ncs" when undefined.
> >> > +
> >> > +- atmel,write-mode         "nwe" or "ncs".
> >> > +                           Default to "ncs" when undefined.
> >> > +
> >> > +- atmel,exnw-mode          "disabled", "frozen" or "ready".
> >> > +                           Default to "disabled" when undefined.
> >> > +
> >> > +- atmel,page-mode          enable page mode if present. The provided value
> >> > +                           defines the page size (supported values: 4, 8,
> >> > +                           16 and 32).
> >> > +
> >> > +- atmel,tdf-mode:          "normal" or "optimized". When set to
> >>
> >> This should be boolean.
> >
> > It was a formerly defined as a boolean, and when it's done like that we
> > have no way to identify whether the property was forgotten or
> > intentionally set to normal mode. What's the problem with this approach?
> 
> Only preference to use boolean when that is sufficient. With your
> argument, then we should never have booleans. You state that missing
> means "normal", not forgotten, so all these properties should be
> required with no default.
> 
> BTW, I debated the same thing on the other properties, but I could see
> them being expanded to a 3rd mode. I don't see that here though.

Well, another reason I switched to a string property is because I
implemented hardware readout in v4, and decided to retrieve the current
state from the hardware and only overload the config with what was
defined in the DT. In this case, if we move to a boolean property we
can't know whether the property is missing because we want to put the
bus in "normal" mode or because we want to keep this config unchanged
(keep the bootloader/bootstrap config).

This is not true in v5, so maybe we should go back to the boolean
atmel,tdf-optimized property.

Thanks,

Boris

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

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

[Boris Brezillon]

Hi Rob,

On Tue, 3 May 2016 14:11:04 -0500
Rob Herring <robh@kernel.org> wrote:

> On Tue, May 3, 2016 at 11:51 AM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > Hi Rob,
> >
> > On Tue, 3 May 2016 11:40:19 -0500
> > Rob Herring <robh@kernel.org> wrote:
> >
> >> On Thu, Apr 28, 2016 at 02:03:27PM +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      | 136 +++++++++++++++++++++
> >> >  1 file changed, 136 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..a6dca5c
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> >> > @@ -0,0 +1,136 @@
> >> > +* 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 through the SMC
> >> > +(Static Memory Controller).
> >> > +
> >> > +Required properties:
> >> > +
> >> > +- compatible:              "atmel,at91sam9260-ebi"
> >> > +                   "atmel,at91sam9261-ebi"
> >> > +                   "atmel,at91sam9263-ebi0"
> >> > +                   "atmel,at91sam9263-ebi1"
> >>
> >> What are the differences between 0 and 1?
> >
> > Because this SoC has 2 EBI busses with different capabilities.
> 
> Okay, correct answer. :)
> 
> [...]
> 
> >>
> >> > +                   of the memory region requested by the device.
> >> > +
> >> > +EBI bus configuration associated with specific chip-select will be defined in
> >> > +the configs subnode. This configs node will in turn contain several subnodes
> >> > +named config-<cs-id>, each of them containing the following properties.
> >>
> >> This is a bit unusual. Why not just part of the child device nodes?
> >
> > Oh, come on! I reworked the binding because Mark complained about the
> > previous binding which was doing exactly what you're suggesting. Can
> > you please be consistent in your reviews...
> 
> No, Mark and I both have our opinions. Which part of this patch
> explains the history?

Hm, it's in patch 1/2 (just dropped the cover letter, which might not
be such a good idea).

> If the revision history is not in the patch, I'm
> not looking at it.
> 
> My issue with it this way is that it has invented yet another way to
> describe timings. I would like to be consistent across external bus
> descriptions, but we're not very consistent to begin with though. The
> most common seems to be the way you first did it. But I agree that it
> is kind of screwy to have an intermediate node unless the controller
> itself has sub-blocks within it and is not the established way to
> describe a bus with chip selects. I would either put the properties
> directly in the child nodes (e.g. flash@0,0) or put your config nodes
> in the device node. I'd call it timings instead of config, but that's
> just bikeshedding.

Well, it's not only describing timings (see atmel,bus-width,
atmel,byte-access-type, ...), but I'm fine with either names :).

> 
> memory-controller@1000 {
>   ...
>   flash@0,0 {
>     timings {
>       ...
>     };
>   };
> };

Okay. Mark, what do you think of this approach?

Note that one of my previous version was defining timings directly in
the EBI device node, and Arnd noted that doing so may cause problems
if one of the EBI property (or the config/timing node name) conflict
with the sub-device binding, which is why I decided to put the EBI
config definitions in a separate subnode.

> 
> >> > +
> >> > +Optional config-<cs-id> node properties:
> >> > +
> >> > +- atmel,bus-width:         width of the asynchronous device's data bus
> >> > +                           8, 16 or 32.
> >> > +                           Default to 8 when undefined.
> >> > +
> >> > +- atmel,byte-access-type   "write" or "select" (see Atmel datasheet).
> >> > +                           Default to "select" when undefined.
> >> > +
> >> > +- atmel,read-mode          "nrd" or "ncs".
> >> > +                           Default to "ncs" when undefined.
> >> > +
> >> > +- atmel,write-mode         "nwe" or "ncs".
> >> > +                           Default to "ncs" when undefined.
> >> > +
> >> > +- atmel,exnw-mode          "disabled", "frozen" or "ready".
> >> > +                           Default to "disabled" when undefined.
> >> > +
> >> > +- atmel,page-mode          enable page mode if present. The provided value
> >> > +                           defines the page size (supported values: 4, 8,
> >> > +                           16 and 32).
> >> > +
> >> > +- atmel,tdf-mode:          "normal" or "optimized". When set to
> >>
> >> This should be boolean.
> >
> > It was a formerly defined as a boolean, and when it's done like that we
> > have no way to identify whether the property was forgotten or
> > intentionally set to normal mode. What's the problem with this approach?
> 
> Only preference to use boolean when that is sufficient. With your
> argument, then we should never have booleans. You state that missing
> means "normal", not forgotten, so all these properties should be
> required with no default.
> 
> BTW, I debated the same thing on the other properties, but I could see
> them being expanded to a 3rd mode. I don't see that here though.

Well, another reason I switched to a string property is because I
implemented hardware readout in v4, and decided to retrieve the current
state from the hardware and only overload the config with what was
defined in the DT. In this case, if we move to a boolean property we
can't know whether the property is missing because we want to put the
bus in "normal" mode or because we want to keep this config unchanged
(keep the bootloader/bootstrap config).

This is not true in v5, so maybe we should go back to the boolean
atmel,tdf-optimized property.

Thanks,

Boris

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

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

[Jean-Jacques Hiblot]

2016-05-03 21:11 GMT+02:00 Rob Herring <robh@kernel.org>:
> On Tue, May 3, 2016 at 11:51 AM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
>> Hi Rob,
>>
>> On Tue, 3 May 2016 11:40:19 -0500
>> Rob Herring <robh@kernel.org> wrote:
>>
>>> On Thu, Apr 28, 2016 at 02:03:27PM +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      | 136 +++++++++++++++++++++
>>> >  1 file changed, 136 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..a6dca5c
>>> > --- /dev/null
>>> > +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
>>> > @@ -0,0 +1,136 @@
>>> > +* 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 through the SMC
>>> > +(Static Memory Controller).
>>> > +
>>> > +Required properties:
>>> > +
>>> > +- compatible:              "atmel,at91sam9260-ebi"
>>> > +                   "atmel,at91sam9261-ebi"
>>> > +                   "atmel,at91sam9263-ebi0"
>>> > +                   "atmel,at91sam9263-ebi1"
>>>
>>> What are the differences between 0 and 1?
>>
>> Because this SoC has 2 EBI busses with different capabilities.
>
> Okay, correct answer. :)
>
> [...]
>
>>>
>>> > +                   of the memory region requested by the device.
>>> > +
>>> > +EBI bus configuration associated with specific chip-select will be defined in
>>> > +the configs subnode. This configs node will in turn contain several subnodes
>>> > +named config-<cs-id>, each of them containing the following properties.
>>>
>>> This is a bit unusual. Why not just part of the child device nodes?
>>
>> Oh, come on! I reworked the binding because Mark complained about the
>> previous binding which was doing exactly what you're suggesting. Can
>> you please be consistent in your reviews...
>
> No, Mark and I both have our opinions. Which part of this patch
> explains the history? If the revision history is not in the patch, I'm
> not looking at it.
>
> My issue with it this way is that it has invented yet another way to
> describe timings. I would like to be consistent across external bus
> descriptions, but we're not very consistent to begin with though. The
> most common seems to be the way you first did it. But I agree that it
> is kind of screwy to have an intermediate node unless the controller
> itself has sub-blocks within it and is not the established way to
> describe a bus with chip selects. I would either put the properties
> directly in the child nodes (e.g. flash@0,0) or put your config nodes
> in the device node. I'd call it timings instead of config, but that's
> just bikeshedding.
>
> memory-controller@1000 {
>   ...
>   flash@0,0 {
>     timings {
>       ...
>     };
>   };
> };
>

I don't think the timings belong in the child node as they really are
for the chip select and the chip select may select several devices at
once. I'm thinking (again) of a FPGA here that could implement or
example 4 serial ports at different addresses.

JJ

>>> > +
>>> > +Optional config-<cs-id> node properties:
>>> > +
>>> > +- atmel,bus-width:         width of the asynchronous device's data bus
>>> > +                           8, 16 or 32.
>>> > +                           Default to 8 when undefined.
>>> > +
>>> > +- atmel,byte-access-type   "write" or "select" (see Atmel datasheet).
>>> > +                           Default to "select" when undefined.
>>> > +
>>> > +- atmel,read-mode          "nrd" or "ncs".
>>> > +                           Default to "ncs" when undefined.
>>> > +
>>> > +- atmel,write-mode         "nwe" or "ncs".
>>> > +                           Default to "ncs" when undefined.
>>> > +
>>> > +- atmel,exnw-mode          "disabled", "frozen" or "ready".
>>> > +                           Default to "disabled" when undefined.
>>> > +
>>> > +- atmel,page-mode          enable page mode if present. The provided value
>>> > +                           defines the page size (supported values: 4, 8,
>>> > +                           16 and 32).
>>> > +
>>> > +- atmel,tdf-mode:          "normal" or "optimized". When set to
>>>
>>> This should be boolean.
>>
>> It was a formerly defined as a boolean, and when it's done like that we
>> have no way to identify whether the property was forgotten or
>> intentionally set to normal mode. What's the problem with this approach?
>
> Only preference to use boolean when that is sufficient. With your
> argument, then we should never have booleans. You state that missing
> means "normal", not forgotten, so all these properties should be
> required with no default.
>
> BTW, I debated the same thing on the other properties, but I could see
> them being expanded to a 3rd mode. I don't see that here though.
>
> Rob

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

[Rob Herring]

On Wed, May 4, 2016 at 5:06 AM, Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> 2016-05-03 21:11 GMT+02:00 Rob Herring <robh@kernel.org>:
>> On Tue, May 3, 2016 at 11:51 AM, Boris Brezillon
>> <boris.brezillon@free-electrons.com> wrote:
>>> Hi Rob,
>>>
>>> On Tue, 3 May 2016 11:40:19 -0500
>>> Rob Herring <robh@kernel.org> wrote:
>>>
>>>> On Thu, Apr 28, 2016 at 02:03:27PM +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      | 136 +++++++++++++++++++++
>>>> >  1 file changed, 136 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..a6dca5c
>>>> > --- /dev/null
>>>> > +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
>>>> > @@ -0,0 +1,136 @@
>>>> > +* 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 through the SMC
>>>> > +(Static Memory Controller).
>>>> > +
>>>> > +Required properties:
>>>> > +
>>>> > +- compatible:              "atmel,at91sam9260-ebi"
>>>> > +                   "atmel,at91sam9261-ebi"
>>>> > +                   "atmel,at91sam9263-ebi0"
>>>> > +                   "atmel,at91sam9263-ebi1"
>>>>
>>>> What are the differences between 0 and 1?
>>>
>>> Because this SoC has 2 EBI busses with different capabilities.
>>
>> Okay, correct answer. :)
>>
>> [...]
>>
>>>>
>>>> > +                   of the memory region requested by the device.
>>>> > +
>>>> > +EBI bus configuration associated with specific chip-select will be defined in
>>>> > +the configs subnode. This configs node will in turn contain several subnodes
>>>> > +named config-<cs-id>, each of them containing the following properties.
>>>>
>>>> This is a bit unusual. Why not just part of the child device nodes?
>>>
>>> Oh, come on! I reworked the binding because Mark complained about the
>>> previous binding which was doing exactly what you're suggesting. Can
>>> you please be consistent in your reviews...
>>
>> No, Mark and I both have our opinions. Which part of this patch
>> explains the history? If the revision history is not in the patch, I'm
>> not looking at it.
>>
>> My issue with it this way is that it has invented yet another way to
>> describe timings. I would like to be consistent across external bus
>> descriptions, but we're not very consistent to begin with though. The
>> most common seems to be the way you first did it. But I agree that it
>> is kind of screwy to have an intermediate node unless the controller
>> itself has sub-blocks within it and is not the established way to
>> describe a bus with chip selects. I would either put the properties
>> directly in the child nodes (e.g. flash@0,0) or put your config nodes
>> in the device node. I'd call it timings instead of config, but that's
>> just bikeshedding.
>>
>> memory-controller@1000 {
>>   ...
>>   flash@0,0 {
>>     timings {
>>       ...
>>     };
>>   };
>> };
>>
>
> I don't think the timings belong in the child node as they really are
> for the chip select and the chip select may select several devices at
> once. I'm thinking (again) of a FPGA here that could implement or
> example 4 serial ports at different addresses.

It is an established pattern already in i.MX WEIM and OMAP GPMC
bindings. The timings are a function of the device attached, so having
them in the device's node makes some sense. Arguably we should define
the timings in a generic way, but that's hard and no one wants to do
that.

Rob

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

[Rob Herring]

On Wed, May 4, 2016 at 4:38 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Rob,
>
> On Tue, 3 May 2016 14:11:04 -0500
> Rob Herring <robh@kernel.org> wrote:
>
>> On Tue, May 3, 2016 at 11:51 AM, Boris Brezillon
>> <boris.brezillon@free-electrons.com> wrote:
>> > Hi Rob,
>> >
>> > On Tue, 3 May 2016 11:40:19 -0500
>> > Rob Herring <robh@kernel.org> wrote:
>> >
>> >> On Thu, Apr 28, 2016 at 02:03:27PM +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.

[...]

>> >> > +EBI bus configuration associated with specific chip-select will be defined in
>> >> > +the configs subnode. This configs node will in turn contain several subnodes
>> >> > +named config-<cs-id>, each of them containing the following properties.
>> >>
>> >> This is a bit unusual. Why not just part of the child device nodes?
>> >
>> > Oh, come on! I reworked the binding because Mark complained about the
>> > previous binding which was doing exactly what you're suggesting. Can
>> > you please be consistent in your reviews...
>>
>> No, Mark and I both have our opinions. Which part of this patch
>> explains the history?
>
> Hm, it's in patch 1/2 (just dropped the cover letter, which might not
> be such a good idea).
>
>> If the revision history is not in the patch, I'm
>> not looking at it.
>>
>> My issue with it this way is that it has invented yet another way to
>> describe timings. I would like to be consistent across external bus
>> descriptions, but we're not very consistent to begin with though. The
>> most common seems to be the way you first did it. But I agree that it
>> is kind of screwy to have an intermediate node unless the controller
>> itself has sub-blocks within it and is not the established way to
>> describe a bus with chip selects. I would either put the properties
>> directly in the child nodes (e.g. flash@0,0) or put your config nodes
>> in the device node. I'd call it timings instead of config, but that's
>> just bikeshedding.
>
> Well, it's not only describing timings (see atmel,bus-width,
> atmel,byte-access-type, ...), but I'm fine with either names :).
>
>>
>> memory-controller@1000 {
>>   ...
>>   flash@0,0 {
>>     timings {
>>       ...
>>     };
>>   };
>> };
>
> Okay. Mark, what do you think of this approach?
>
> Note that one of my previous version was defining timings directly in
> the EBI device node, and Arnd noted that doing so may cause problems
> if one of the EBI property (or the config/timing node name) conflict
> with the sub-device binding, which is why I decided to put the EBI
> config definitions in a separate subnode.

You have vendor prefixes on all the properties so I don't think a
collision is really a problem. It's also an established pattern in
i.MX WEIM and OMAP GPMC (which are hiding in bindings/bus/) and I
prefer consistency.

>> >> > +
>> >> > +Optional config-<cs-id> node properties:
>> >> > +
>> >> > +- atmel,bus-width:         width of the asynchronous device's data bus
>> >> > +                           8, 16 or 32.
>> >> > +                           Default to 8 when undefined.
>> >> > +
>> >> > +- atmel,byte-access-type   "write" or "select" (see Atmel datasheet).
>> >> > +                           Default to "select" when undefined.
>> >> > +
>> >> > +- atmel,read-mode          "nrd" or "ncs".
>> >> > +                           Default to "ncs" when undefined.
>> >> > +
>> >> > +- atmel,write-mode         "nwe" or "ncs".
>> >> > +                           Default to "ncs" when undefined.
>> >> > +
>> >> > +- atmel,exnw-mode          "disabled", "frozen" or "ready".
>> >> > +                           Default to "disabled" when undefined.
>> >> > +
>> >> > +- atmel,page-mode          enable page mode if present. The provided value
>> >> > +                           defines the page size (supported values: 4, 8,
>> >> > +                           16 and 32).
>> >> > +
>> >> > +- atmel,tdf-mode:          "normal" or "optimized". When set to
>> >>
>> >> This should be boolean.
>> >
>> > It was a formerly defined as a boolean, and when it's done like that we
>> > have no way to identify whether the property was forgotten or
>> > intentionally set to normal mode. What's the problem with this approach?
>>
>> Only preference to use boolean when that is sufficient. With your
>> argument, then we should never have booleans. You state that missing
>> means "normal", not forgotten, so all these properties should be
>> required with no default.
>>
>> BTW, I debated the same thing on the other properties, but I could see
>> them being expanded to a 3rd mode. I don't see that here though.
>
> Well, another reason I switched to a string property is because I
> implemented hardware readout in v4, and decided to retrieve the current
> state from the hardware and only overload the config with what was
> defined in the DT. In this case, if we move to a boolean property we
> can't know whether the property is missing because we want to put the
> bus in "normal" mode or because we want to keep this config unchanged
> (keep the bootloader/bootstrap config).

Now you are giving yet another definition of what missing means.
Please pick and document one:
- Error, property missing
- Normal mode
- Retain firmware/bootloader settings (this should probably apply to
all or none)

The last is really the only reason I agree with for not doing a boolean.

Rob

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

[Boris Brezillon]

On Wed, 4 May 2016 08:06:10 -0500
Rob Herring <robh@kernel.org> wrote:

> On Wed, May 4, 2016 at 4:38 AM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > Hi Rob,
> >
> > On Tue, 3 May 2016 14:11:04 -0500
> > Rob Herring <robh@kernel.org> wrote:
> >  
> >> On Tue, May 3, 2016 at 11:51 AM, Boris Brezillon
> >> <boris.brezillon@free-electrons.com> wrote:  
> >> > Hi Rob,
> >> >
> >> > On Tue, 3 May 2016 11:40:19 -0500
> >> > Rob Herring <robh@kernel.org> wrote:
> >> >  
> >> >> On Thu, Apr 28, 2016 at 02:03:27PM +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.  
> 
> [...]
> 
> >> >> > +EBI bus configuration associated with specific chip-select will be defined in
> >> >> > +the configs subnode. This configs node will in turn contain several subnodes
> >> >> > +named config-<cs-id>, each of them containing the following properties.  
> >> >>
> >> >> This is a bit unusual. Why not just part of the child device nodes?  
> >> >
> >> > Oh, come on! I reworked the binding because Mark complained about the
> >> > previous binding which was doing exactly what you're suggesting. Can
> >> > you please be consistent in your reviews...  
> >>
> >> No, Mark and I both have our opinions. Which part of this patch
> >> explains the history?  
> >
> > Hm, it's in patch 1/2 (just dropped the cover letter, which might not
> > be such a good idea).
> >  
> >> If the revision history is not in the patch, I'm
> >> not looking at it.
> >>
> >> My issue with it this way is that it has invented yet another way to
> >> describe timings. I would like to be consistent across external bus
> >> descriptions, but we're not very consistent to begin with though. The
> >> most common seems to be the way you first did it. But I agree that it
> >> is kind of screwy to have an intermediate node unless the controller
> >> itself has sub-blocks within it and is not the established way to
> >> describe a bus with chip selects. I would either put the properties
> >> directly in the child nodes (e.g. flash@0,0) or put your config nodes
> >> in the device node. I'd call it timings instead of config, but that's
> >> just bikeshedding.  
> >
> > Well, it's not only describing timings (see atmel,bus-width,
> > atmel,byte-access-type, ...), but I'm fine with either names :).
> >  
> >>
> >> memory-controller@1000 {
> >>   ...
> >>   flash@0,0 {
> >>     timings {
> >>       ...
> >>     };
> >>   };
> >> };  
> >
> > Okay. Mark, what do you think of this approach?
> >
> > Note that one of my previous version was defining timings directly in
> > the EBI device node, and Arnd noted that doing so may cause problems
> > if one of the EBI property (or the config/timing node name) conflict
> > with the sub-device binding, which is why I decided to put the EBI
> > config definitions in a separate subnode.  
> 
> You have vendor prefixes on all the properties so I don't think a
> collision is really a problem. It's also an established pattern in
> i.MX WEIM and OMAP GPMC (which are hiding in bindings/bus/) and I
> prefer consistency.

So let's summarize that.

memory-controller@1000 {
	...
	flash@0,0 {
		atmel,<ebi-prop-name> = <value>;
		...
		<flash-device-prop> = <value>;
	};
};

Would everyone agree on this representation?

With this approach, it's a bit more complicated to detect the case
where we want to keep bootloader/firmware config, but it should be
doable (it's much more easier to test for the presence of a
config/timing node than verifying that either all or none of the
mandatory properties are here).

Still remains the problem mentioned by Jean-Jacques: what if the
sub-device takes 2 CS lines. Should we apply the same setting to those
slots?

> 
> >> >> > +
> >> >> > +Optional config-<cs-id> node properties:
> >> >> > +
> >> >> > +- atmel,bus-width:         width of the asynchronous device's data bus
> >> >> > +                           8, 16 or 32.
> >> >> > +                           Default to 8 when undefined.
> >> >> > +
> >> >> > +- atmel,byte-access-type   "write" or "select" (see Atmel datasheet).
> >> >> > +                           Default to "select" when undefined.
> >> >> > +
> >> >> > +- atmel,read-mode          "nrd" or "ncs".
> >> >> > +                           Default to "ncs" when undefined.
> >> >> > +
> >> >> > +- atmel,write-mode         "nwe" or "ncs".
> >> >> > +                           Default to "ncs" when undefined.
> >> >> > +
> >> >> > +- atmel,exnw-mode          "disabled", "frozen" or "ready".
> >> >> > +                           Default to "disabled" when undefined.
> >> >> > +
> >> >> > +- atmel,page-mode          enable page mode if present. The provided value
> >> >> > +                           defines the page size (supported values: 4, 8,
> >> >> > +                           16 and 32).
> >> >> > +
> >> >> > +- atmel,tdf-mode:          "normal" or "optimized". When set to  
> >> >>
> >> >> This should be boolean.  
> >> >
> >> > It was a formerly defined as a boolean, and when it's done like that we
> >> > have no way to identify whether the property was forgotten or
> >> > intentionally set to normal mode. What's the problem with this approach?  
> >>
> >> Only preference to use boolean when that is sufficient. With your
> >> argument, then we should never have booleans. You state that missing
> >> means "normal", not forgotten, so all these properties should be
> >> required with no default.
> >>
> >> BTW, I debated the same thing on the other properties, but I could see
> >> them being expanded to a 3rd mode. I don't see that here though.  
> >
> > Well, another reason I switched to a string property is because I
> > implemented hardware readout in v4, and decided to retrieve the current
> > state from the hardware and only overload the config with what was
> > defined in the DT. In this case, if we move to a boolean property we
> > can't know whether the property is missing because we want to put the
> > bus in "normal" mode or because we want to keep this config unchanged
> > (keep the bootloader/bootstrap config).  
> 
> Now you are giving yet another definition of what missing means.
> Please pick and document one:
> - Error, property missing
> - Normal mode
> - Retain firmware/bootloader settings (this should probably apply to
> all or none)

As I said, this has changed in v5. In v4 we were first retrieving the
config from the hardware and then overloading this config with the
properties defined in the DT.
This ambiguity has gone along with the approach of putting the timings
directly in the sub-device node. In this version, EBI configs are
defined directly in configs/config-X nodes, which means that
- if configs/config-X exists, then all mandatory properties should be
  defined (otherwise -EINVAL is returned), and we can fallback to
  default values for optional properties.
- if configs/config-X does not exist, we retrieve the config directly
  from the hardware

So, with this assumption, we could definitely switch to a boolean
property. The only reason I did not change that is because I find it
clearer to have both modes clearly named than having a property called
atmel,tdf-optimized.

Anyway that's not something I'm particularly attached to, so let's
switch back to a boolean property.


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

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

[Boris Brezillon]

On Wed, 4 May 2016 15:35:47 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Wed, 4 May 2016 08:06:10 -0500
> Rob Herring <robh@kernel.org> wrote:
> 
> > On Wed, May 4, 2016 at 4:38 AM, Boris Brezillon
> > <boris.brezillon@free-electrons.com> wrote:  
> > > Hi Rob,
> > >
> > > On Tue, 3 May 2016 14:11:04 -0500
> > > Rob Herring <robh@kernel.org> wrote:
> > >    
> > >> On Tue, May 3, 2016 at 11:51 AM, Boris Brezillon
> > >> <boris.brezillon@free-electrons.com> wrote:    
> > >> > Hi Rob,
> > >> >
> > >> > On Tue, 3 May 2016 11:40:19 -0500
> > >> > Rob Herring <robh@kernel.org> wrote:
> > >> >    
> > >> >> On Thu, Apr 28, 2016 at 02:03:27PM +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.    
> > 
> > [...]
> >   
> > >> >> > +EBI bus configuration associated with specific chip-select will be defined in
> > >> >> > +the configs subnode. This configs node will in turn contain several subnodes
> > >> >> > +named config-<cs-id>, each of them containing the following properties.    
> > >> >>
> > >> >> This is a bit unusual. Why not just part of the child device nodes?    
> > >> >
> > >> > Oh, come on! I reworked the binding because Mark complained about the
> > >> > previous binding which was doing exactly what you're suggesting. Can
> > >> > you please be consistent in your reviews...    
> > >>
> > >> No, Mark and I both have our opinions. Which part of this patch
> > >> explains the history?    
> > >
> > > Hm, it's in patch 1/2 (just dropped the cover letter, which might not
> > > be such a good idea).
> > >    
> > >> If the revision history is not in the patch, I'm
> > >> not looking at it.
> > >>
> > >> My issue with it this way is that it has invented yet another way to
> > >> describe timings. I would like to be consistent across external bus
> > >> descriptions, but we're not very consistent to begin with though. The
> > >> most common seems to be the way you first did it. But I agree that it
> > >> is kind of screwy to have an intermediate node unless the controller
> > >> itself has sub-blocks within it and is not the established way to
> > >> describe a bus with chip selects. I would either put the properties
> > >> directly in the child nodes (e.g. flash@0,0) or put your config nodes
> > >> in the device node. I'd call it timings instead of config, but that's
> > >> just bikeshedding.    
> > >
> > > Well, it's not only describing timings (see atmel,bus-width,
> > > atmel,byte-access-type, ...), but I'm fine with either names :).
> > >    
> > >>
> > >> memory-controller@1000 {
> > >>   ...
> > >>   flash@0,0 {
> > >>     timings {
> > >>       ...
> > >>     };
> > >>   };
> > >> };    
> > >
> > > Okay. Mark, what do you think of this approach?
> > >
> > > Note that one of my previous version was defining timings directly in
> > > the EBI device node, and Arnd noted that doing so may cause problems
> > > if one of the EBI property (or the config/timing node name) conflict
> > > with the sub-device binding, which is why I decided to put the EBI
> > > config definitions in a separate subnode.    
> > 
> > You have vendor prefixes on all the properties so I don't think a
> > collision is really a problem. It's also an established pattern in
> > i.MX WEIM and OMAP GPMC (which are hiding in bindings/bus/) and I
> > prefer consistency.  
> 
> So let's summarize that.
> 
> memory-controller@1000 {
> 	...
> 	flash@0,0 {
> 		atmel,<ebi-prop-name> = <value>;
> 		...
> 		<flash-device-prop> = <value>;
> 	};
> };
> 
> Would everyone agree on this representation?
> 
> With this approach, it's a bit more complicated to detect the case
> where we want to keep bootloader/firmware config, but it should be
> doable (it's much more easier to test for the presence of a
> config/timing node than verifying that either all or none of the
> mandatory properties are here).
> 
> Still remains the problem mentioned by Jean-Jacques: what if the
> sub-device takes 2 CS lines. Should we apply the same setting to those
> slots?
> 

Rob, Mark, Arnd, can you take a decision regarding this binding? This
driver is floating around for quite some time, and we were asked to
rework the binding several times (in time in an opposite direction).

For the record, here is the thread I mentioned earlier [1]. In his
answer, Arnd suggests to put timing and bus config description
outside of the sub-device node. Mark recently complained about this
representation, which led me to the configs/config-X appraoch, and now
Rob suggests to go back to the first proposal.

I'm fine doing that, but can you please all confirm that you agree on
this binding?

Thanks,

Boris

[1]http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/222438.html

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

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

[Mark Rutland]

On Tue, May 10, 2016 at 10:04:48AM +0200, Boris Brezillon wrote:
> On Wed, 4 May 2016 15:35:47 +0200
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > On Wed, 4 May 2016 08:06:10 -0500
> > Rob Herring <robh@kernel.org> wrote:
> > 
> > > On Wed, May 4, 2016 at 4:38 AM, Boris Brezillon
> > > <boris.brezillon@free-electrons.com> wrote:  
> > > > Hi Rob,
> > > >
> > > > On Tue, 3 May 2016 14:11:04 -0500
> > > > Rob Herring <robh@kernel.org> wrote:
> > > >    
> > > >> On Tue, May 3, 2016 at 11:51 AM, Boris Brezillon
> > > >> <boris.brezillon@free-electrons.com> wrote:    
> > > >> > Hi Rob,
> > > >> >
> > > >> > On Tue, 3 May 2016 11:40:19 -0500
> > > >> > Rob Herring <robh@kernel.org> wrote:
> > > >> >    
> > > >> >> On Thu, Apr 28, 2016 at 02:03:27PM +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.    
> > > 
> > > [...]
> > >   
> > > >> >> > +EBI bus configuration associated with specific chip-select will be defined in
> > > >> >> > +the configs subnode. This configs node will in turn contain several subnodes
> > > >> >> > +named config-<cs-id>, each of them containing the following properties.    
> > > >> >>
> > > >> >> This is a bit unusual. Why not just part of the child device nodes?    
> > > >> >
> > > >> > Oh, come on! I reworked the binding because Mark complained about the
> > > >> > previous binding which was doing exactly what you're suggesting. Can
> > > >> > you please be consistent in your reviews...    
> > > >>
> > > >> No, Mark and I both have our opinions. Which part of this patch
> > > >> explains the history?    
> > > >
> > > > Hm, it's in patch 1/2 (just dropped the cover letter, which might not
> > > > be such a good idea).
> > > >    
> > > >> If the revision history is not in the patch, I'm
> > > >> not looking at it.
> > > >>
> > > >> My issue with it this way is that it has invented yet another way to
> > > >> describe timings. I would like to be consistent across external bus
> > > >> descriptions, but we're not very consistent to begin with though. The
> > > >> most common seems to be the way you first did it. But I agree that it
> > > >> is kind of screwy to have an intermediate node unless the controller
> > > >> itself has sub-blocks within it and is not the established way to
> > > >> describe a bus with chip selects. I would either put the properties
> > > >> directly in the child nodes (e.g. flash@0,0) or put your config nodes
> > > >> in the device node. I'd call it timings instead of config, but that's
> > > >> just bikeshedding.    
> > > >
> > > > Well, it's not only describing timings (see atmel,bus-width,
> > > > atmel,byte-access-type, ...), but I'm fine with either names :).
> > > >    
> > > >>
> > > >> memory-controller@1000 {
> > > >>   ...
> > > >>   flash@0,0 {
> > > >>     timings {
> > > >>       ...
> > > >>     };
> > > >>   };
> > > >> };    
> > > >
> > > > Okay. Mark, what do you think of this approach?
> > > >
> > > > Note that one of my previous version was defining timings directly in
> > > > the EBI device node, and Arnd noted that doing so may cause problems
> > > > if one of the EBI property (or the config/timing node name) conflict
> > > > with the sub-device binding, which is why I decided to put the EBI
> > > > config definitions in a separate subnode.    
> > > 
> > > You have vendor prefixes on all the properties so I don't think a
> > > collision is really a problem. It's also an established pattern in
> > > i.MX WEIM and OMAP GPMC (which are hiding in bindings/bus/) and I
> > > prefer consistency.  
> > 
> > So let's summarize that.
> > 
> > memory-controller@1000 {
> > 	...
> > 	flash@0,0 {
> > 		atmel,<ebi-prop-name> = <value>;
> > 		...
> > 		<flash-device-prop> = <value>;
> > 	};
> > };
> > 
> > Would everyone agree on this representation?
> > 
> > With this approach, it's a bit more complicated to detect the case
> > where we want to keep bootloader/firmware config, but it should be
> > doable (it's much more easier to test for the presence of a
> > config/timing node than verifying that either all or none of the
> > mandatory properties are here).
> > 
> > Still remains the problem mentioned by Jean-Jacques: what if the
> > sub-device takes 2 CS lines. Should we apply the same setting to those
> > slots?
> > 
> 
> Rob, Mark, Arnd, can you take a decision regarding this binding? This
> driver is floating around for quite some time, and we were asked to
> rework the binding several times (in time in an opposite direction).
> 
> For the record, here is the thread I mentioned earlier [1]. In his
> answer, Arnd suggests to put timing and bus config description
> outside of the sub-device node. Mark recently complained about this
> representation, which led me to the configs/config-X appraoch, and now
> Rob suggests to go back to the first proposal.
> 
> I'm fine doing that, but can you please all confirm that you agree on
> this binding?

Sorry for the delay in getting round to this, and sorry that this
appears to be going in circles.

Please go with Rob's suggestion.

I'm not sure about the case where a device takes 2 CS lines. I would
assume that in practice that a sub-device covered my multiple CS lines
expects the same timings for all its MMIO space, and so having that
uniform makes sense. Do we have a counter-example?

Thanks,
Mark.

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

[Boris Brezillon]

On Tue, 10 May 2016 12:07:42 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Tue, May 10, 2016 at 10:04:48AM +0200, Boris Brezillon wrote:
> > On Wed, 4 May 2016 15:35:47 +0200
> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >   
> > > On Wed, 4 May 2016 08:06:10 -0500
> > > Rob Herring <robh@kernel.org> wrote:
> > >   
> > > > On Wed, May 4, 2016 at 4:38 AM, Boris Brezillon
> > > > <boris.brezillon@free-electrons.com> wrote:    
> > > > > Hi Rob,
> > > > >
> > > > > On Tue, 3 May 2016 14:11:04 -0500
> > > > > Rob Herring <robh@kernel.org> wrote:
> > > > >      
> > > > >> On Tue, May 3, 2016 at 11:51 AM, Boris Brezillon
> > > > >> <boris.brezillon@free-electrons.com> wrote:      
> > > > >> > Hi Rob,
> > > > >> >
> > > > >> > On Tue, 3 May 2016 11:40:19 -0500
> > > > >> > Rob Herring <robh@kernel.org> wrote:
> > > > >> >      
> > > > >> >> On Thu, Apr 28, 2016 at 02:03:27PM +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.      
> > > > 
> > > > [...]
> > > >     
> > > > >> >> > +EBI bus configuration associated with specific chip-select will be defined in
> > > > >> >> > +the configs subnode. This configs node will in turn contain several subnodes
> > > > >> >> > +named config-<cs-id>, each of them containing the following properties.      
> > > > >> >>
> > > > >> >> This is a bit unusual. Why not just part of the child device nodes?      
> > > > >> >
> > > > >> > Oh, come on! I reworked the binding because Mark complained about the
> > > > >> > previous binding which was doing exactly what you're suggesting. Can
> > > > >> > you please be consistent in your reviews...      
> > > > >>
> > > > >> No, Mark and I both have our opinions. Which part of this patch
> > > > >> explains the history?      
> > > > >
> > > > > Hm, it's in patch 1/2 (just dropped the cover letter, which might not
> > > > > be such a good idea).
> > > > >      
> > > > >> If the revision history is not in the patch, I'm
> > > > >> not looking at it.
> > > > >>
> > > > >> My issue with it this way is that it has invented yet another way to
> > > > >> describe timings. I would like to be consistent across external bus
> > > > >> descriptions, but we're not very consistent to begin with though. The
> > > > >> most common seems to be the way you first did it. But I agree that it
> > > > >> is kind of screwy to have an intermediate node unless the controller
> > > > >> itself has sub-blocks within it and is not the established way to
> > > > >> describe a bus with chip selects. I would either put the properties
> > > > >> directly in the child nodes (e.g. flash@0,0) or put your config nodes
> > > > >> in the device node. I'd call it timings instead of config, but that's
> > > > >> just bikeshedding.      
> > > > >
> > > > > Well, it's not only describing timings (see atmel,bus-width,
> > > > > atmel,byte-access-type, ...), but I'm fine with either names :).
> > > > >      
> > > > >>
> > > > >> memory-controller@1000 {
> > > > >>   ...
> > > > >>   flash@0,0 {
> > > > >>     timings {
> > > > >>       ...
> > > > >>     };
> > > > >>   };
> > > > >> };      
> > > > >
> > > > > Okay. Mark, what do you think of this approach?
> > > > >
> > > > > Note that one of my previous version was defining timings directly in
> > > > > the EBI device node, and Arnd noted that doing so may cause problems
> > > > > if one of the EBI property (or the config/timing node name) conflict
> > > > > with the sub-device binding, which is why I decided to put the EBI
> > > > > config definitions in a separate subnode.      
> > > > 
> > > > You have vendor prefixes on all the properties so I don't think a
> > > > collision is really a problem. It's also an established pattern in
> > > > i.MX WEIM and OMAP GPMC (which are hiding in bindings/bus/) and I
> > > > prefer consistency.    
> > > 
> > > So let's summarize that.
> > > 
> > > memory-controller@1000 {
> > > 	...
> > > 	flash@0,0 {
> > > 		atmel,<ebi-prop-name> = <value>;
> > > 		...
> > > 		<flash-device-prop> = <value>;
> > > 	};
> > > };
> > > 
> > > Would everyone agree on this representation?
> > > 
> > > With this approach, it's a bit more complicated to detect the case
> > > where we want to keep bootloader/firmware config, but it should be
> > > doable (it's much more easier to test for the presence of a
> > > config/timing node than verifying that either all or none of the
> > > mandatory properties are here).
> > > 
> > > Still remains the problem mentioned by Jean-Jacques: what if the
> > > sub-device takes 2 CS lines. Should we apply the same setting to those
> > > slots?
> > >   
> > 
> > Rob, Mark, Arnd, can you take a decision regarding this binding? This
> > driver is floating around for quite some time, and we were asked to
> > rework the binding several times (in time in an opposite direction).
> > 
> > For the record, here is the thread I mentioned earlier [1]. In his
> > answer, Arnd suggests to put timing and bus config description
> > outside of the sub-device node. Mark recently complained about this
> > representation, which led me to the configs/config-X appraoch, and now
> > Rob suggests to go back to the first proposal.
> > 
> > I'm fine doing that, but can you please all confirm that you agree on
> > this binding?  
> 
> Sorry for the delay in getting round to this, and sorry that this
> appears to be going in circles.
> 
> Please go with Rob's suggestion.

Okay. This changes a bit the constraints defined in the binding doc
(no default values for undefined properties: we just keep the
bootloader/firmware config), but otherwise should be easy to implement.

> 
> I'm not sure about the case where a device takes 2 CS lines. I would
> assume that in practice that a sub-device covered my multiple CS lines
> expects the same timings for all its MMIO space, and so having that
> uniform makes sense. Do we have a counter-example?

Nope, I don't. JJH had one (interfacing with an FPGA), maybe he can
detail this use case.


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

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

[Jean-Jacques Hiblot]

2016-05-10 14:41 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Tue, 10 May 2016 12:07:42 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
>
>> On Tue, May 10, 2016 at 10:04:48AM +0200, Boris Brezillon wrote:
>> > On Wed, 4 May 2016 15:35:47 +0200
>> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>> >
>> > > On Wed, 4 May 2016 08:06:10 -0500
>> > > Rob Herring <robh@kernel.org> wrote:
>> > >
>> > > > On Wed, May 4, 2016 at 4:38 AM, Boris Brezillon
>> > > > <boris.brezillon@free-electrons.com> wrote:
>> > > > > Hi Rob,
>> > > > >
>> > > > > On Tue, 3 May 2016 14:11:04 -0500
>> > > > > Rob Herring <robh@kernel.org> wrote:
>> > > > >
>> > > > >> On Tue, May 3, 2016 at 11:51 AM, Boris Brezillon
>> > > > >> <boris.brezillon@free-electrons.com> wrote:
>> > > > >> > Hi Rob,
>> > > > >> >
>> > > > >> > On Tue, 3 May 2016 11:40:19 -0500
>> > > > >> > Rob Herring <robh@kernel.org> wrote:
>> > > > >> >
>> > > > >> >> On Thu, Apr 28, 2016 at 02:03:27PM +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.
>> > > >
>> > > > [...]
>> > > >
>> > > > >> >> > +EBI bus configuration associated with specific chip-select will be defined in
>> > > > >> >> > +the configs subnode. This configs node will in turn contain several subnodes
>> > > > >> >> > +named config-<cs-id>, each of them containing the following properties.
>> > > > >> >>
>> > > > >> >> This is a bit unusual. Why not just part of the child device nodes?
>> > > > >> >
>> > > > >> > Oh, come on! I reworked the binding because Mark complained about the
>> > > > >> > previous binding which was doing exactly what you're suggesting. Can
>> > > > >> > you please be consistent in your reviews...
>> > > > >>
>> > > > >> No, Mark and I both have our opinions. Which part of this patch
>> > > > >> explains the history?
>> > > > >
>> > > > > Hm, it's in patch 1/2 (just dropped the cover letter, which might not
>> > > > > be such a good idea).
>> > > > >
>> > > > >> If the revision history is not in the patch, I'm
>> > > > >> not looking at it.
>> > > > >>
>> > > > >> My issue with it this way is that it has invented yet another way to
>> > > > >> describe timings. I would like to be consistent across external bus
>> > > > >> descriptions, but we're not very consistent to begin with though. The
>> > > > >> most common seems to be the way you first did it. But I agree that it
>> > > > >> is kind of screwy to have an intermediate node unless the controller
>> > > > >> itself has sub-blocks within it and is not the established way to
>> > > > >> describe a bus with chip selects. I would either put the properties
>> > > > >> directly in the child nodes (e.g. flash@0,0) or put your config nodes
>> > > > >> in the device node. I'd call it timings instead of config, but that's
>> > > > >> just bikeshedding.
>> > > > >
>> > > > > Well, it's not only describing timings (see atmel,bus-width,
>> > > > > atmel,byte-access-type, ...), but I'm fine with either names :).
>> > > > >
>> > > > >>
>> > > > >> memory-controller@1000 {
>> > > > >>   ...
>> > > > >>   flash@0,0 {
>> > > > >>     timings {
>> > > > >>       ...
>> > > > >>     };
>> > > > >>   };
>> > > > >> };
>> > > > >
>> > > > > Okay. Mark, what do you think of this approach?
>> > > > >
>> > > > > Note that one of my previous version was defining timings directly in
>> > > > > the EBI device node, and Arnd noted that doing so may cause problems
>> > > > > if one of the EBI property (or the config/timing node name) conflict
>> > > > > with the sub-device binding, which is why I decided to put the EBI
>> > > > > config definitions in a separate subnode.
>> > > >
>> > > > You have vendor prefixes on all the properties so I don't think a
>> > > > collision is really a problem. It's also an established pattern in
>> > > > i.MX WEIM and OMAP GPMC (which are hiding in bindings/bus/) and I
>> > > > prefer consistency.
>> > >
>> > > So let's summarize that.
>> > >
>> > > memory-controller@1000 {
>> > >   ...
>> > >   flash@0,0 {
>> > >           atmel,<ebi-prop-name> = <value>;
>> > >           ...
>> > >           <flash-device-prop> = <value>;
>> > >   };
>> > > };
>> > >
>> > > Would everyone agree on this representation?
>> > >
>> > > With this approach, it's a bit more complicated to detect the case
>> > > where we want to keep bootloader/firmware config, but it should be
>> > > doable (it's much more easier to test for the presence of a
>> > > config/timing node than verifying that either all or none of the
>> > > mandatory properties are here).
>> > >
>> > > Still remains the problem mentioned by Jean-Jacques: what if the
>> > > sub-device takes 2 CS lines. Should we apply the same setting to those
>> > > slots?
>> > >
>> >
>> > Rob, Mark, Arnd, can you take a decision regarding this binding? This
>> > driver is floating around for quite some time, and we were asked to
>> > rework the binding several times (in time in an opposite direction).
>> >
>> > For the record, here is the thread I mentioned earlier [1]. In his
>> > answer, Arnd suggests to put timing and bus config description
>> > outside of the sub-device node. Mark recently complained about this
>> > representation, which led me to the configs/config-X appraoch, and now
>> > Rob suggests to go back to the first proposal.
>> >
>> > I'm fine doing that, but can you please all confirm that you agree on
>> > this binding?
>>
>> Sorry for the delay in getting round to this, and sorry that this
>> appears to be going in circles.
>>
>> Please go with Rob's suggestion.
>
> Okay. This changes a bit the constraints defined in the binding doc
> (no default values for undefined properties: we just keep the
> bootloader/firmware config), but otherwise should be easy to implement.
>
>>
>> I'm not sure about the case where a device takes 2 CS lines. I would
>> assume that in practice that a sub-device covered my multiple CS lines
>> expects the same timings for all its MMIO space, and so having that
>> uniform makes sense. Do we have a counter-example?
>
> Nope, I don't. JJH had one (interfacing with an FPGA), maybe he can
> detail this use case.
>
I don't either. It makes sense that a single device with 2 CS uses the
same timings.
My use case was the other way around: 1 CS for several devices.

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

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

[Rob Herring]

On Tue, May 10, 2016 at 8:08 AM, Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> 2016-05-10 14:41 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> On Tue, 10 May 2016 12:07:42 +0100
>> Mark Rutland <mark.rutland@arm.com> wrote:
>>
>>> On Tue, May 10, 2016 at 10:04:48AM +0200, Boris Brezillon wrote:
>>> > On Wed, 4 May 2016 15:35:47 +0200
>>> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>>> >
>>> > > On Wed, 4 May 2016 08:06:10 -0500
>>> > > Rob Herring <robh@kernel.org> wrote:
>>> > >
>>> > > > On Wed, May 4, 2016 at 4:38 AM, Boris Brezillon
>>> > > > <boris.brezillon@free-electrons.com> wrote:
>>> > > > > Hi Rob,
>>> > > > >
>>> > > > > On Tue, 3 May 2016 14:11:04 -0500
>>> > > > > Rob Herring <robh@kernel.org> wrote:
>>> > > > >
>>> > > > >> On Tue, May 3, 2016 at 11:51 AM, Boris Brezillon
>>> > > > >> <boris.brezillon@free-electrons.com> wrote:
>>> > > > >> > Hi Rob,
>>> > > > >> >
>>> > > > >> > On Tue, 3 May 2016 11:40:19 -0500
>>> > > > >> > Rob Herring <robh@kernel.org> wrote:
>>> > > > >> >
>>> > > > >> >> On Thu, Apr 28, 2016 at 02:03:27PM +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.
>>> > > >
>>> > > > [...]
>>> > > >
>>> > > > >> >> > +EBI bus configuration associated with specific chip-select will be defined in
>>> > > > >> >> > +the configs subnode. This configs node will in turn contain several subnodes
>>> > > > >> >> > +named config-<cs-id>, each of them containing the following properties.
>>> > > > >> >>
>>> > > > >> >> This is a bit unusual. Why not just part of the child device nodes?
>>> > > > >> >
>>> > > > >> > Oh, come on! I reworked the binding because Mark complained about the
>>> > > > >> > previous binding which was doing exactly what you're suggesting. Can
>>> > > > >> > you please be consistent in your reviews...
>>> > > > >>
>>> > > > >> No, Mark and I both have our opinions. Which part of this patch
>>> > > > >> explains the history?
>>> > > > >
>>> > > > > Hm, it's in patch 1/2 (just dropped the cover letter, which might not
>>> > > > > be such a good idea).
>>> > > > >
>>> > > > >> If the revision history is not in the patch, I'm
>>> > > > >> not looking at it.
>>> > > > >>
>>> > > > >> My issue with it this way is that it has invented yet another way to
>>> > > > >> describe timings. I would like to be consistent across external bus
>>> > > > >> descriptions, but we're not very consistent to begin with though. The
>>> > > > >> most common seems to be the way you first did it. But I agree that it
>>> > > > >> is kind of screwy to have an intermediate node unless the controller
>>> > > > >> itself has sub-blocks within it and is not the established way to
>>> > > > >> describe a bus with chip selects. I would either put the properties
>>> > > > >> directly in the child nodes (e.g. flash@0,0) or put your config nodes
>>> > > > >> in the device node. I'd call it timings instead of config, but that's
>>> > > > >> just bikeshedding.
>>> > > > >
>>> > > > > Well, it's not only describing timings (see atmel,bus-width,
>>> > > > > atmel,byte-access-type, ...), but I'm fine with either names :).
>>> > > > >
>>> > > > >>
>>> > > > >> memory-controller@1000 {
>>> > > > >>   ...
>>> > > > >>   flash@0,0 {
>>> > > > >>     timings {
>>> > > > >>       ...
>>> > > > >>     };
>>> > > > >>   };
>>> > > > >> };
>>> > > > >
>>> > > > > Okay. Mark, what do you think of this approach?
>>> > > > >
>>> > > > > Note that one of my previous version was defining timings directly in
>>> > > > > the EBI device node, and Arnd noted that doing so may cause problems
>>> > > > > if one of the EBI property (or the config/timing node name) conflict
>>> > > > > with the sub-device binding, which is why I decided to put the EBI
>>> > > > > config definitions in a separate subnode.
>>> > > >
>>> > > > You have vendor prefixes on all the properties so I don't think a
>>> > > > collision is really a problem. It's also an established pattern in
>>> > > > i.MX WEIM and OMAP GPMC (which are hiding in bindings/bus/) and I
>>> > > > prefer consistency.
>>> > >
>>> > > So let's summarize that.
>>> > >
>>> > > memory-controller@1000 {
>>> > >   ...
>>> > >   flash@0,0 {
>>> > >           atmel,<ebi-prop-name> = <value>;
>>> > >           ...
>>> > >           <flash-device-prop> = <value>;
>>> > >   };
>>> > > };
>>> > >
>>> > > Would everyone agree on this representation?
>>> > >
>>> > > With this approach, it's a bit more complicated to detect the case
>>> > > where we want to keep bootloader/firmware config, but it should be
>>> > > doable (it's much more easier to test for the presence of a
>>> > > config/timing node than verifying that either all or none of the
>>> > > mandatory properties are here).
>>> > >
>>> > > Still remains the problem mentioned by Jean-Jacques: what if the
>>> > > sub-device takes 2 CS lines. Should we apply the same setting to those
>>> > > slots?
>>> > >
>>> >
>>> > Rob, Mark, Arnd, can you take a decision regarding this binding? This
>>> > driver is floating around for quite some time, and we were asked to
>>> > rework the binding several times (in time in an opposite direction).
>>> >
>>> > For the record, here is the thread I mentioned earlier [1]. In his
>>> > answer, Arnd suggests to put timing and bus config description
>>> > outside of the sub-device node. Mark recently complained about this
>>> > representation, which led me to the configs/config-X appraoch, and now
>>> > Rob suggests to go back to the first proposal.
>>> >
>>> > I'm fine doing that, but can you please all confirm that you agree on
>>> > this binding?
>>>
>>> Sorry for the delay in getting round to this, and sorry that this
>>> appears to be going in circles.
>>>
>>> Please go with Rob's suggestion.
>>
>> Okay. This changes a bit the constraints defined in the binding doc
>> (no default values for undefined properties: we just keep the
>> bootloader/firmware config), but otherwise should be easy to implement.
>>
>>>
>>> I'm not sure about the case where a device takes 2 CS lines. I would
>>> assume that in practice that a sub-device covered my multiple CS lines
>>> expects the same timings for all its MMIO space, and so having that
>>> uniform makes sense. Do we have a counter-example?
>>
>> Nope, I don't. JJH had one (interfacing with an FPGA), maybe he can
>> detail this use case.
>>
> I don't either. It makes sense that a single device with 2 CS uses the
> same timings.
> My use case was the other way around: 1 CS for several devices.

Ah, I thought it was just wanting to share timings for several CS. In
this case, it would probably make sense to have 3 levels of nodes
(EBI, CS node with timings, device nodes) as you do have some logic in
between to do address decoding. But I think the simple case should
still be 2 levels of nodes and that doesn't really affect the EBI
binding. It just cares that timings are in the immediate child nodes.

Rob

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

[Boris Brezillon]

On Tue, 10 May 2016 09:52:41 -0500
Rob Herring <robh@kernel.org> wrote:

> On Tue, May 10, 2016 at 8:08 AM, Jean-Jacques Hiblot
> <jjhiblot@traphandler.com> wrote:
> > 2016-05-10 14:41 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:  
> >> On Tue, 10 May 2016 12:07:42 +0100
> >> Mark Rutland <mark.rutland@arm.com> wrote:
> >>  
> >>> On Tue, May 10, 2016 at 10:04:48AM +0200, Boris Brezillon wrote:  
> >>> > On Wed, 4 May 2016 15:35:47 +0200
> >>> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >>> >  
> >>> > > On Wed, 4 May 2016 08:06:10 -0500
> >>> > > Rob Herring <robh@kernel.org> wrote:
> >>> > >  
> >>> > > > On Wed, May 4, 2016 at 4:38 AM, Boris Brezillon
> >>> > > > <boris.brezillon@free-electrons.com> wrote:  
> >>> > > > > Hi Rob,
> >>> > > > >
> >>> > > > > On Tue, 3 May 2016 14:11:04 -0500
> >>> > > > > Rob Herring <robh@kernel.org> wrote:
> >>> > > > >  
> >>> > > > >> On Tue, May 3, 2016 at 11:51 AM, Boris Brezillon
> >>> > > > >> <boris.brezillon@free-electrons.com> wrote:  
> >>> > > > >> > Hi Rob,
> >>> > > > >> >
> >>> > > > >> > On Tue, 3 May 2016 11:40:19 -0500
> >>> > > > >> > Rob Herring <robh@kernel.org> wrote:
> >>> > > > >> >  
> >>> > > > >> >> On Thu, Apr 28, 2016 at 02:03:27PM +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.  
> >>> > > >
> >>> > > > [...]
> >>> > > >  
> >>> > > > >> >> > +EBI bus configuration associated with specific chip-select will be defined in
> >>> > > > >> >> > +the configs subnode. This configs node will in turn contain several subnodes
> >>> > > > >> >> > +named config-<cs-id>, each of them containing the following properties.  
> >>> > > > >> >>
> >>> > > > >> >> This is a bit unusual. Why not just part of the child device nodes?  
> >>> > > > >> >
> >>> > > > >> > Oh, come on! I reworked the binding because Mark complained about the
> >>> > > > >> > previous binding which was doing exactly what you're suggesting. Can
> >>> > > > >> > you please be consistent in your reviews...  
> >>> > > > >>
> >>> > > > >> No, Mark and I both have our opinions. Which part of this patch
> >>> > > > >> explains the history?  
> >>> > > > >
> >>> > > > > Hm, it's in patch 1/2 (just dropped the cover letter, which might not
> >>> > > > > be such a good idea).
> >>> > > > >  
> >>> > > > >> If the revision history is not in the patch, I'm
> >>> > > > >> not looking at it.
> >>> > > > >>
> >>> > > > >> My issue with it this way is that it has invented yet another way to
> >>> > > > >> describe timings. I would like to be consistent across external bus
> >>> > > > >> descriptions, but we're not very consistent to begin with though. The
> >>> > > > >> most common seems to be the way you first did it. But I agree that it
> >>> > > > >> is kind of screwy to have an intermediate node unless the controller
> >>> > > > >> itself has sub-blocks within it and is not the established way to
> >>> > > > >> describe a bus with chip selects. I would either put the properties
> >>> > > > >> directly in the child nodes (e.g. flash@0,0) or put your config nodes
> >>> > > > >> in the device node. I'd call it timings instead of config, but that's
> >>> > > > >> just bikeshedding.  
> >>> > > > >
> >>> > > > > Well, it's not only describing timings (see atmel,bus-width,
> >>> > > > > atmel,byte-access-type, ...), but I'm fine with either names :).
> >>> > > > >  
> >>> > > > >>
> >>> > > > >> memory-controller@1000 {
> >>> > > > >>   ...
> >>> > > > >>   flash@0,0 {
> >>> > > > >>     timings {
> >>> > > > >>       ...
> >>> > > > >>     };
> >>> > > > >>   };
> >>> > > > >> };  
> >>> > > > >
> >>> > > > > Okay. Mark, what do you think of this approach?
> >>> > > > >
> >>> > > > > Note that one of my previous version was defining timings directly in
> >>> > > > > the EBI device node, and Arnd noted that doing so may cause problems
> >>> > > > > if one of the EBI property (or the config/timing node name) conflict
> >>> > > > > with the sub-device binding, which is why I decided to put the EBI
> >>> > > > > config definitions in a separate subnode.  
> >>> > > >
> >>> > > > You have vendor prefixes on all the properties so I don't think a
> >>> > > > collision is really a problem. It's also an established pattern in
> >>> > > > i.MX WEIM and OMAP GPMC (which are hiding in bindings/bus/) and I
> >>> > > > prefer consistency.  
> >>> > >
> >>> > > So let's summarize that.
> >>> > >
> >>> > > memory-controller@1000 {
> >>> > >   ...
> >>> > >   flash@0,0 {
> >>> > >           atmel,<ebi-prop-name> = <value>;
> >>> > >           ...
> >>> > >           <flash-device-prop> = <value>;
> >>> > >   };
> >>> > > };
> >>> > >
> >>> > > Would everyone agree on this representation?
> >>> > >
> >>> > > With this approach, it's a bit more complicated to detect the case
> >>> > > where we want to keep bootloader/firmware config, but it should be
> >>> > > doable (it's much more easier to test for the presence of a
> >>> > > config/timing node than verifying that either all or none of the
> >>> > > mandatory properties are here).
> >>> > >
> >>> > > Still remains the problem mentioned by Jean-Jacques: what if the
> >>> > > sub-device takes 2 CS lines. Should we apply the same setting to those
> >>> > > slots?
> >>> > >  
> >>> >
> >>> > Rob, Mark, Arnd, can you take a decision regarding this binding? This
> >>> > driver is floating around for quite some time, and we were asked to
> >>> > rework the binding several times (in time in an opposite direction).
> >>> >
> >>> > For the record, here is the thread I mentioned earlier [1]. In his
> >>> > answer, Arnd suggests to put timing and bus config description
> >>> > outside of the sub-device node. Mark recently complained about this
> >>> > representation, which led me to the configs/config-X appraoch, and now
> >>> > Rob suggests to go back to the first proposal.
> >>> >
> >>> > I'm fine doing that, but can you please all confirm that you agree on
> >>> > this binding?  
> >>>
> >>> Sorry for the delay in getting round to this, and sorry that this
> >>> appears to be going in circles.
> >>>
> >>> Please go with Rob's suggestion.  
> >>
> >> Okay. This changes a bit the constraints defined in the binding doc
> >> (no default values for undefined properties: we just keep the
> >> bootloader/firmware config), but otherwise should be easy to implement.
> >>  
> >>>
> >>> I'm not sure about the case where a device takes 2 CS lines. I would
> >>> assume that in practice that a sub-device covered my multiple CS lines
> >>> expects the same timings for all its MMIO space, and so having that
> >>> uniform makes sense. Do we have a counter-example?  
> >>
> >> Nope, I don't. JJH had one (interfacing with an FPGA), maybe he can
> >> detail this use case.
> >>  
> > I don't either. It makes sense that a single device with 2 CS uses the
> > same timings.
> > My use case was the other way around: 1 CS for several devices.  
> 
> Ah, I thought it was just wanting to share timings for several CS. In
> this case, it would probably make sense to have 3 levels of nodes
> (EBI, CS node with timings, device nodes) as you do have some logic in
> between to do address decoding. But I think the simple case should
> still be 2 levels of nodes and that doesn't really affect the EBI
> binding. It just cares that timings are in the immediate child nodes.

I'd expect the sub-device driver to change the configuration by itself
in such complex cases (I'm planning to expose APIs to let other drivers
manually configure the EBI bus for their specific use case: it's kind
of required for the NAND controller driver anyway).

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