diff mbox

[2/7] memory: atmel-ebi: Simplify SMC config code

Message ID 1487609701-10300-3-git-send-email-boris.brezillon@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Boris BREZILLON Feb. 20, 2017, 4:54 p.m. UTC 
New helpers/macros have been to atmel-smc.h introduced to simplify SMC
regs manipulation. Rework the code to use those helpers, and simplify
the ->xlate_config(), ->get_config() and ->apply_config() implementations.

SMC configs are now stored in a struct atmel_smc_cs_conf object that
directly contains registers values, which should help implementing
->suspend()/->resume() hooks.

We can also get rid of those regmap fields (and the associated ->init()
hook) which are not longer needed thanks to the
atmel_[h]smc_cs_conf_{apply,get}() helpers.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/memory/atmel-ebi.c | 429 +++++++++++++--------------------------------
 1 file changed, 126 insertions(+), 303 deletions(-)

Comments

Alexander Dahl March 2, 2017, 12:02 p.m. UTC | #1 
Hei hei,

With 

#define ATMEL_SMC_MODE_TDF(x)           (((x) - 1) << 16)

from include/linux/mfd/syscon/atmel-smc.h you added this:

> +	ret = of_property_read_u32(np, "atmel,smc-tdf-ns", &val);
> +	if (!ret) {
> +		required = true;
> +		ncycles = DIV_ROUND_UP(val, clk_period_ns);
> +		if (ncycles > ATMEL_SMC_MODE_TDF_MAX) {
> +			ret = -EINVAL;
> +			goto out;
> +		}

[…]

> +		smcconf->mode |= ATMEL_SMC_MODE_TDF(ncycles);
> +	}

This was the same algorithm at some other location in atmel-ebi.c 
before:

	#define AT91_SMC_TDF_(x)		((((x) - 1) << 16) & AT91_SMC_TDF)

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

The hardware manual (AT91SAM9G20) says values from 0 to 15 (4bit, 0x0 to 
0xF) are possible and I guess the goal is to set it to a value 
corresponding to the value in ns from the dts or to 15 if it's greater 
(or -EINVAL in the new version).

However how can one set it to zero? Put in zero to the div you get zero 
for ncycles or val and that goes as x into (((x) - 1) << 16) which 
results in 0xF ending up as TDF_CYCLES in the mode register, right?

I can of course set a slightly greater value, which ends up in a 
calculated register value of zero, but that seems more a hack to me and 
is not obvious if I just look at the DTS.

If I'm right this might be topic of another bugfix patch, or should it 
be done right in a v2 of this one?

Greets
Alex
Boris BREZILLON March 2, 2017, 12:30 p.m. UTC | #2 
Hi Alexander,

On Thu, 02 Mar 2017 13:02:16 +0100
Alexander Dahl <ada@thorsis.com> wrote:

> Hei hei,
> 
> With 
> 
> #define ATMEL_SMC_MODE_TDF(x)           (((x) - 1) << 16)
> 
> from include/linux/mfd/syscon/atmel-smc.h you added this:
> 
> > +	ret = of_property_read_u32(np, "atmel,smc-tdf-ns", &val);
> > +	if (!ret) {
> > +		required = true;
> > +		ncycles = DIV_ROUND_UP(val, clk_period_ns);
> > +		if (ncycles > ATMEL_SMC_MODE_TDF_MAX) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}  
> 
> […]
> 
> > +		smcconf->mode |= ATMEL_SMC_MODE_TDF(ncycles);
> > +	}  
> 
> This was the same algorithm at some other location in atmel-ebi.c 
> before:
> 
> 	#define AT91_SMC_TDF_(x)		((((x) - 1) << 16) & AT91_SMC_TDF)
> 
> 	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));
> 
> The hardware manual (AT91SAM9G20) says values from 0 to 15 (4bit, 0x0 to 
> 0xF) are possible and I guess the goal is to set it to a value 
> corresponding to the value in ns from the dts or to 15 if it's greater 
> (or -EINVAL in the new version).
> 
> However how can one set it to zero? Put in zero to the div you get zero 
> for ncycles or val and that goes as x into (((x) - 1) << 16) which 
> results in 0xF ending up as TDF_CYCLES in the mode register, right?

Indeed.

> 
> I can of course set a slightly greater value, which ends up in a 
> calculated register value of zero, but that seems more a hack to me and 
> is not obvious if I just look at the DTS.

No, we should fix the bug.

> 
> If I'm right this might be topic of another bugfix patch, or should it 
> be done right in a v2 of this one?

It should be done right in a v2. Something like:

		if (ncycles < ATMEL_SMC_MODE_TDF_MIN)
			ncycles = ATMEL_SMC_MODE_TDF_MIN;

with

#define ATMEL_SMC_MODE_TDF_MIN 1

I don't think we need to backport the fix, since no-one uses this driver
yet.

Thanks for this report.

Boris
Alexander Dahl July 24, 2017, 9:12 a.m. UTC | #3 
Hello Boris,

while testing v4.13-rc2 on an at91sam9g20 baed platform I'm coming back 
to this topic. Meanwhile the whole new SMC and NAND below EBI stuff is 
in mainline, this TDF bug however is still in there. See below (all 
quoted code parts from v4.13-rc2):

Am Donnerstag, 2. März 2017, 13:30:13 schrieb Boris Brezillon:
> Alexander Dahl <ada@thorsis.com> wrote:
> > #define ATMEL_SMC_MODE_TDF(x)           (((x) - 1) << 16)

[…]

> > The hardware manual (AT91SAM9G20) says values from 0 to 15 (4bit,
> > 0x0 to 0xF) are possible and I guess the goal is to set it to a
> > value corresponding to the value in ns from the dts or to 15 if
> > it's greater (or -EINVAL in the new version).
> > 
> > However how can one set it to zero? Put in zero to the div you get
> > zero for ncycles or val and that goes as x into (((x) - 1) << 16)
> > which results in 0xF ending up as TDF_CYCLES in the mode register,
> > right?
> 
> Indeed.
> 
> > I can of course set a slightly greater value, which ends up in a
> > calculated register value of zero, but that seems more a hack to me
> > and is not obvious if I just look at the DTS.
> 
> No, we should fix the bug.
> 
> > If I'm right this might be topic of another bugfix patch, or should
> > it be done right in a v2 of this one?
> 
> It should be done right in a v2. Something like:
> 
> 		if (ncycles < ATMEL_SMC_MODE_TDF_MIN)
> 			ncycles = ATMEL_SMC_MODE_TDF_MIN;
> 
> with
> 
> #define ATMEL_SMC_MODE_TDF_MIN 1

I checked the SAMA5D3x datasheet today and it has the same mode register 
layout regarding the TDF parts. So allowed are register values from 0 to 
15 ending up in 0 to 15 clock cycles of Data Float Time.

The code in include/linux/mfd/syscon/atmel-smc.h is this:

#define ATMEL_SMC_MODE_TDF_MASK			GENMASK(19, 16)
#define ATMEL_SMC_MODE_TDF(x)			(((x) - 1) << 16)
#define ATMEL_SMC_MODE_TDF_MAX			16
#define ATMEL_SMC_MODE_TDF_MIN			1
#define ATMEL_SMC_MODE_TDFMODE_OPTIMIZED	BIT(20)

This ATMEL_SMC_MODE_TDF() is used in drivers/memory/atmel-ebi.c to setup 
the external memory interface with timings from dts. A line there inside 
an ebi node may look like this (see for example 
arch/arm/boot/dts/sama5d3xcm.dtsi):

atmel,smc-tdf-ns = <0>;

The value is expected in nanoseconds and I would expect a direct mapping 
from 0ns to a register value of 0. This is not the case in code 
(drivers/memory/atmel-ebi.c):

		if (ncycles > ATMEL_SMC_MODE_TDF_MAX ||
		    ncycles < ATMEL_SMC_MODE_TDF_MIN) {
			ret = -EINVAL;
			goto out;
		}

ATMEL_SMC_MODE_TDF_MIN is 1, so a possible 0 value from dts is not 
allowed in here and atmel_ebi_xslate_smc_timings() fails. In fact to get 
a register value of 0 for 0 TDF clock cycles I would have to set e.g. 
8ns in dts. So this didn't make it in some v2 and is still broken.

I could fix this and provide a patch, but I'm not sure about the second 
place where ATMEL_SMC_MODE_TDF() is used which is the NAND driver in 
drivers/mtd/nand/atmel/nand-controller.c. I'm not familiar enough with 
NAND to judge if this "min = 1, max = 16, decrease by 1 for applying to 
register" approach is needed there. I would say no, because it also is a 
counterintuitive offset, but I would prefer a explanation why the code 
is like this, before touching and breaking anything. ;-)

Greets
Alex
diff mbox

Patch

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index 4e83a8b92665..8363735f8aef 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -18,37 +18,9 @@ 
 #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 atmel_smc_cs_conf smcconf;
 };
 
 struct at91_ebi;
@@ -69,9 +41,8 @@  struct at91_ebi_caps {
 	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);
+	void (*apply_config)(struct at91_ebi_dev *ebid,
+			     struct at91_ebi_dev_config *conf);
 };
 
 struct at91_ebi {
@@ -86,151 +57,118 @@  struct at91_ebi {
 	struct device *dev;
 	const struct at91_ebi_caps *caps;
 	struct list_head devs;
-	union {
-		struct at91sam9_smc_generic_fields sam9;
-	};
 };
 
+struct atmel_smc_timing_xlate {
+	const char *name;
+	int (*converter)(struct atmel_smc_cs_conf *conf,
+			 unsigned int shift, unsigned int nycles);
+	unsigned int shift;
+};
+
+#define ATMEL_SMC_SETUP_XLATE(nm, pos)	\
+	{ .name = nm, .converter = atmel_smc_cs_conf_set_setup, .shift = pos}
+
+#define ATMEL_SMC_PULSE_XLATE(nm, pos)	\
+	{ .name = nm, .converter = atmel_smc_cs_conf_set_pulse, .shift = pos}
+
+#define ATMEL_SMC_CYCLE_XLATE(nm, pos)	\
+	{ .name = nm, .converter = atmel_smc_cs_conf_set_setup, .shift = pos}
+
 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_period = NSEC_PER_SEC / 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_period * 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_period;
-	timings->nrd_setup_ns = (val >> 16) & 0x1f;
-	timings->nrd_setup_ns += ((val >> 21) & 0x1) * 128;
-	timings->nrd_setup_ns *= clk_period;
-	timings->ncs_wr_setup_ns = (val >> 8) & 0x1f;
-	timings->ncs_wr_setup_ns += ((val >> 13) & 0x1) * 128;
-	timings->ncs_wr_setup_ns *= clk_period;
-	timings->nwe_setup_ns = val & 0x1f;
-	timings->nwe_setup_ns += ((val >> 5) & 0x1) * 128;
-	timings->nwe_setup_ns *= clk_period;
-
-	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_period;
-	timings->nrd_pulse_ns = (val >> 16) & 0x3f;
-	timings->nrd_pulse_ns += ((val >> 22) & 0x1) * 256;
-	timings->nrd_pulse_ns *= clk_period;
-	timings->ncs_wr_pulse_ns = (val >> 8) & 0x3f;
-	timings->ncs_wr_pulse_ns += ((val >> 14) & 0x1) * 256;
-	timings->ncs_wr_pulse_ns *= clk_period;
-	timings->nwe_pulse_ns = val & 0x3f;
-	timings->nwe_pulse_ns += ((val >> 6) & 0x1) * 256;
-	timings->nwe_pulse_ns *= clk_period;
-
-	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_period;
-	timings->nwe_cycle_ns = val & 0x7f;
-	timings->nwe_cycle_ns += ((val >> 7) & 0x3) * 256;
-	timings->nwe_cycle_ns *= clk_period;
+	atmel_smc_cs_conf_get(ebid->ebi->smc.regmap, conf->cs,
+			      &conf->smcconf);
 }
 
-static int at91_xlate_timing(struct device_node *np, const char *prop,
-			     u32 *val, bool *required)
+static void sama5_ebi_get_config(struct at91_ebi_dev *ebid,
+				 struct at91_ebi_dev_config *conf)
 {
-	if (!of_property_read_u32(np, prop, val)) {
-		*required = true;
-		return 0;
-	}
-
-	if (*required)
-		return -EINVAL;
-
-	return 0;
+	atmel_hsmc_cs_conf_get(ebid->ebi->smc.regmap, conf->cs,
+			       &conf->smcconf);
 }
 
-static int at91sam9_smc_xslate_timings(struct at91_ebi_dev *ebid,
+static const struct atmel_smc_timing_xlate timings_xlate_table[] = {
+	ATMEL_SMC_SETUP_XLATE("atmel,smc-ncs-rd-setup-ns",
+			      ATMEL_SMC_NCS_RD_SHIFT),
+	ATMEL_SMC_SETUP_XLATE("atmel,smc-ncs-wr-setup-ns",
+			      ATMEL_SMC_NCS_WR_SHIFT),
+	ATMEL_SMC_SETUP_XLATE("atmel,smc-nrd-setup-ns", ATMEL_SMC_NRD_SHIFT),
+	ATMEL_SMC_SETUP_XLATE("atmel,smc-nwe-setup-ns", ATMEL_SMC_NWE_SHIFT),
+	ATMEL_SMC_PULSE_XLATE("atmel,smc-ncs-rd-pulse-ns",
+			      ATMEL_SMC_NCS_RD_SHIFT),
+	ATMEL_SMC_PULSE_XLATE("atmel,smc-ncs-wr-pulse-ns",
+			      ATMEL_SMC_NCS_WR_SHIFT),
+	ATMEL_SMC_PULSE_XLATE("atmel,smc-nrd-pulse-ns", ATMEL_SMC_NRD_SHIFT),
+	ATMEL_SMC_PULSE_XLATE("atmel,smc-nwe-pulse-ns", ATMEL_SMC_NWE_SHIFT),
+	ATMEL_SMC_CYCLE_XLATE("atmel,smc-nrd-cycle-ns", ATMEL_SMC_NRD_SHIFT),
+	ATMEL_SMC_CYCLE_XLATE("atmel,smc-nwe-cycle-ns", ATMEL_SMC_NWE_SHIFT),
+};
+
+static int at91_ebi_xslate_smc_timings(struct at91_ebi_dev *ebid,
 				       struct device_node *np,
-				       struct at91sam9_smc_timings *timings,
-				       bool *required)
+				       struct atmel_smc_cs_conf *smcconf)
 {
-	int ret;
-
-	ret = at91_xlate_timing(np, "atmel,smc-ncs-rd-setup-ns",
-				&timings->ncs_rd_setup_ns, required);
-	if (ret)
-		goto out;
-
-	ret = at91_xlate_timing(np, "atmel,smc-nrd-setup-ns",
-				&timings->nrd_setup_ns, required);
-	if (ret)
-		goto out;
-
-	ret = at91_xlate_timing(np, "atmel,smc-ncs-wr-setup-ns",
-				&timings->ncs_wr_setup_ns, required);
-	if (ret)
-		goto out;
+	unsigned int clk_rate = clk_get_rate(ebid->ebi->clk);
+	unsigned int clk_period_ns = NSEC_PER_SEC / clk_rate;
+	bool required = false;
+	unsigned int ncycles;
+	int ret, i;
+	u32 val;
 
-	ret = at91_xlate_timing(np, "atmel,smc-nwe-setup-ns",
-				&timings->nwe_setup_ns, required);
-	if (ret)
-		goto out;
+	ret = of_property_read_u32(np, "atmel,smc-tdf-ns", &val);
+	if (!ret) {
+		required = true;
+		ncycles = DIV_ROUND_UP(val, clk_period_ns);
+		if (ncycles > ATMEL_SMC_MODE_TDF_MAX) {
+			ret = -EINVAL;
+			goto out;
+		}
 
-	ret = at91_xlate_timing(np, "atmel,smc-ncs-rd-pulse-ns",
-				&timings->ncs_rd_pulse_ns, required);
-	if (ret)
-		goto out;
+		smcconf->mode |= ATMEL_SMC_MODE_TDF(ncycles);
+	}
 
-	ret = at91_xlate_timing(np, "atmel,smc-nrd-pulse-ns",
-				&timings->nrd_pulse_ns, required);
-	if (ret)
-		goto out;
+	for (i = 0; i < ARRAY_SIZE(timings_xlate_table); i++) {
+		const struct atmel_smc_timing_xlate *xlate;
 
-	ret = at91_xlate_timing(np, "atmel,smc-ncs-wr-pulse-ns",
-				&timings->ncs_wr_pulse_ns, required);
-	if (ret)
-		goto out;
+		xlate = &timings_xlate_table[i];
 
-	ret = at91_xlate_timing(np, "atmel,smc-nwe-pulse-ns",
-				&timings->nwe_pulse_ns, required);
-	if (ret)
-		goto out;
-
-	ret = at91_xlate_timing(np, "atmel,smc-nwe-cycle-ns",
-				&timings->nwe_cycle_ns, required);
-	if (ret)
-		goto out;
+		ret = of_property_read_u32(np, xlate->name, &val);
+		if (ret) {
+			if (!required)
+				continue;
+			else
+				break;
+		}
 
-	ret = at91_xlate_timing(np, "atmel,smc-nrd-cycle-ns",
-				&timings->nrd_cycle_ns, required);
-	if (ret)
-		goto out;
+		if (!required) {
+			ret = -EINVAL;
+			break;
+		}
 
-	ret = at91_xlate_timing(np, "atmel,smc-tdf-ns",
-				&timings->tdf_ns, required);
+		ncycles = DIV_ROUND_UP(val, clk_period_ns);
+		ret = xlate->converter(smcconf, xlate->shift, ncycles);
+		if (ret)
+			goto out;
+	}
 
 out:
-	if (ret)
+	if (ret) {
 		dev_err(ebid->ebi->dev,
 			"missing or invalid timings definition in %s",
 			np->full_name);
+		return ret;
+	}
 
-	return ret;
+	return required;
 }
 
-static int at91sam9_ebi_xslate_config(struct at91_ebi_dev *ebid,
+static int at91_ebi_xslate_smc_config(struct at91_ebi_dev *ebid,
 				      struct device_node *np,
 				      struct at91_ebi_dev_config *conf)
 {
-	struct at91sam9_ebi_dev_config *config = &conf->sam9;
+	struct atmel_smc_cs_conf *smcconf = &conf->smcconf;
 	bool required = false;
 	const char *tmp_str;
 	u32 tmp;
@@ -240,15 +178,15 @@  static int at91sam9_ebi_xslate_config(struct at91_ebi_dev *ebid,
 	if (!ret) {
 		switch (tmp) {
 		case 8:
-			config->mode |= AT91_SMC_DBW_8;
+			smcconf->mode |= ATMEL_SMC_MODE_DBW_8;
 			break;
 
 		case 16:
-			config->mode |= AT91_SMC_DBW_16;
+			smcconf->mode |= ATMEL_SMC_MODE_DBW_16;
 			break;
 
 		case 32:
-			config->mode |= AT91_SMC_DBW_32;
+			smcconf->mode |= ATMEL_SMC_MODE_DBW_32;
 			break;
 
 		default:
@@ -259,28 +197,28 @@  static int at91sam9_ebi_xslate_config(struct at91_ebi_dev *ebid,
 	}
 
 	if (of_property_read_bool(np, "atmel,smc-tdf-optimized")) {
-		config->mode |= AT91_SMC_TDFMODE_OPTIMIZED;
+		smcconf->mode |= ATMEL_SMC_MODE_TDFMODE_OPTIMIZED;
 		required = true;
 	}
 
 	tmp_str = NULL;
 	of_property_read_string(np, "atmel,smc-byte-access-type", &tmp_str);
 	if (tmp_str && !strcmp(tmp_str, "write")) {
-		config->mode |= AT91_SMC_BAT_WRITE;
+		smcconf->mode |= ATMEL_SMC_MODE_BAT_WRITE;
 		required = true;
 	}
 
 	tmp_str = NULL;
 	of_property_read_string(np, "atmel,smc-read-mode", &tmp_str);
 	if (tmp_str && !strcmp(tmp_str, "nrd")) {
-		config->mode |= AT91_SMC_READMODE_NRD;
+		smcconf->mode |= ATMEL_SMC_MODE_READMODE_NRD;
 		required = true;
 	}
 
 	tmp_str = NULL;
 	of_property_read_string(np, "atmel,smc-write-mode", &tmp_str);
 	if (tmp_str && !strcmp(tmp_str, "nwe")) {
-		config->mode |= AT91_SMC_WRITEMODE_NWE;
+		smcconf->mode |= ATMEL_SMC_MODE_WRITEMODE_NWE;
 		required = true;
 	}
 
@@ -288,9 +226,9 @@  static int at91sam9_ebi_xslate_config(struct at91_ebi_dev *ebid,
 	of_property_read_string(np, "atmel,smc-exnw-mode", &tmp_str);
 	if (tmp_str) {
 		if (!strcmp(tmp_str, "frozen"))
-			config->mode |= AT91_SMC_EXNWMODE_FROZEN;
+			smcconf->mode |= ATMEL_SMC_MODE_EXNWMODE_FROZEN;
 		else if (!strcmp(tmp_str, "ready"))
-			config->mode |= AT91_SMC_EXNWMODE_READY;
+			smcconf->mode |= ATMEL_SMC_MODE_EXNWMODE_READY;
 		else if (strcmp(tmp_str, "disabled"))
 			return -EINVAL;
 
@@ -301,155 +239,54 @@  static int at91sam9_ebi_xslate_config(struct at91_ebi_dev *ebid,
 	if (!ret) {
 		switch (tmp) {
 		case 4:
-			config->mode |= AT91_SMC_PS_4;
+			smcconf->mode |= ATMEL_SMC_MODE_PS_4;
 			break;
 
 		case 8:
-			config->mode |= AT91_SMC_PS_8;
+			smcconf->mode |= ATMEL_SMC_MODE_PS_8;
 			break;
 
 		case 16:
-			config->mode |= AT91_SMC_PS_16;
+			smcconf->mode |= ATMEL_SMC_MODE_PS_16;
 			break;
 
 		case 32:
-			config->mode |= AT91_SMC_PS_32;
+			smcconf->mode |= ATMEL_SMC_MODE_PS_32;
 			break;
 
 		default:
 			return -EINVAL;
 		}
 
-		config->mode |= AT91_SMC_PMEN;
+		smcconf->mode |= ATMEL_SMC_MODE_PMEN;
 		required = true;
 	}
 
-	ret = at91sam9_smc_xslate_timings(ebid, np, &config->timings,
-					  &required);
+	ret = at91_ebi_xslate_smc_timings(ebid, np, &conf->smcconf);
 	if (ret)
-		return ret;
-
-	return required;
-}
-
-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);
-	unsigned int clk_period = NSEC_PER_SEC / clk_rate;
-	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;
+		return -EINVAL;
 
-	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_period);
-	if (val > AT91_SMC_TDF_MAX)
-		val = AT91_SMC_TDF_MAX;
-	regmap_fields_write(fields->mode, conf->cs,
-			    config->mode | AT91_SMC_TDF_(val));
+	if ((ret > 0 && !required) || (!ret && required)) {
+		dev_err(ebid->ebi->dev, "missing atmel,smc- properties in %s",
+			np->full_name);
+		return -EINVAL;
+	}
 
-	return 0;
+	return required;
 }
 
-static int at91sam9_ebi_init(struct at91_ebi *ebi)
+static void at91sam9_ebi_apply_config(struct at91_ebi_dev *ebid,
+				      struct at91_ebi_dev_config *conf)
 {
-	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.regmap,
-						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.regmap,
-						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.regmap,
-						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.regmap,
-					       field);
-	return PTR_ERR_OR_ZERO(fields->mode);
+	atmel_smc_cs_conf_apply(ebid->ebi->smc.regmap, conf->cs,
+				&conf->smcconf);
 }
 
-static int sama5d3_ebi_init(struct at91_ebi *ebi)
+static void sama5_ebi_apply_config(struct at91_ebi_dev *ebid,
+				   struct at91_ebi_dev_config *conf)
 {
-	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.regmap,
-						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.regmap,
-						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.regmap,
-						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.regmap,
-					       field);
-	return PTR_ERR_OR_ZERO(fields->mode);
+	atmel_hsmc_cs_conf_apply(ebid->ebi->smc.regmap, conf->cs,
+				 &conf->smcconf);
 }
 
 static int at91_ebi_dev_setup(struct at91_ebi *ebi, struct device_node *np,
@@ -508,9 +345,7 @@  static int at91_ebi_dev_setup(struct at91_ebi *ebi, struct device_node *np,
 
 		if (apply) {
 			conf.cs = cs;
-			ret = caps->apply_config(ebid, &conf);
-			if (ret)
-				return ret;
+			caps->apply_config(ebid, &conf);
 		}
 
 		caps->get_config(ebid, &ebid->configs[i]);
@@ -539,9 +374,8 @@  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,
+	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
-	.init = at91sam9_ebi_init,
 };
 
 static const struct reg_field at91sam9261_ebi_csa =
@@ -552,9 +386,8 @@  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,
+	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
-	.init = at91sam9_ebi_init,
 };
 
 static const struct reg_field at91sam9263_ebi0_csa =
@@ -565,9 +398,8 @@  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,
+	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
-	.init = at91sam9_ebi_init,
 };
 
 static const struct reg_field at91sam9263_ebi1_csa =
@@ -578,9 +410,8 @@  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,
+	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
-	.init = at91sam9_ebi_init,
 };
 
 static const struct reg_field at91sam9rl_ebi_csa =
@@ -591,9 +422,8 @@  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,
+	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
-	.init = at91sam9_ebi_init,
 };
 
 static const struct reg_field at91sam9g45_ebi_csa =
@@ -604,26 +434,23 @@  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,
+	.xlate_config = at91_ebi_xslate_smc_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,
+	.xlate_config = at91_ebi_xslate_smc_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,
+	.get_config = sama5_ebi_get_config,
+	.xlate_config = at91_ebi_xslate_smc_config,
+	.apply_config = sama5_ebi_apply_config,
 };
 
 static const struct of_device_id at91_ebi_id_table[] = {
@@ -745,10 +572,6 @@  static int at91_ebi_probe(struct platform_device *pdev)
 			return PTR_ERR(ebi->ebi_csa);
 	}
 
-	ret = ebi->caps->init(ebi);
-	if (ret)
-		return ret;
-
 	ret = of_property_read_u32(np, "#address-cells", &val);
 	if (ret) {
 		dev_err(dev, "missing #address-cells property\n");