All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] memory: atmel-ebi: Add PM ops
@ 2017-02-20 16:54 ` Boris Brezillon
  0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:54 UTC (permalink / raw)
  To: Nicolas Ferre, Alexandre Belloni
  Cc: linux-arm-kernel, Samuel Ortiz, Lee Jones, linux-kernel, Boris Brezillon

Hello,

This patchset adds a ->resume() hook to the EBI driver, but a few more
changes were required in order to easily implement this PM hook.

Patches 1 to 5 are just cleanup patches to simplify the EBI driver.
Patch 6 is fixing a bug in the EBI driver. Note that this bug has no
consequences until we start implementing the ->resume() hook.
And finally, patch 7 is implementing ->resume() so that the SMC config
is correctly restored when resuming the system.

This patchset depends on [1] which is removing the unused pata_at91
driver, which in turn prevents us from patching it before dropping
the old SMC macro definitions in patch 4.

Regards,

Boris

[1]http://lkml.iu.edu/hypermail/linux/kernel/1702.2/00990.html

Boris Brezillon (7):
  mfd: syscon: atmel-smc: Add new helpers to ease SMC regs manipulation
  memory: atmel-ebi: Simplify SMC config code
  memory: atmel-ebi: Stop using reg_field objects for simple things
  mfd: syscon: atmel-smc: Remove unused helpers/macros
  memory: atmel-ebi: Change naming scheme
  memory: atmel-ebi: Add missing ->numcs assignment
  memory: atmel-ebi: Add PM ops

 drivers/memory/atmel-ebi.c           | 583 ++++++++++++-----------------------
 include/linux/mfd/syscon/atmel-smc.h | 456 +++++++++++++++++++--------
 2 files changed, 532 insertions(+), 507 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH 0/7] memory: atmel-ebi: Add PM ops
@ 2017-02-20 16:54 ` Boris Brezillon
  0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:54 UTC (permalink / raw)
  To: linux-arm-kernel

Hello,

This patchset adds a ->resume() hook to the EBI driver, but a few more
changes were required in order to easily implement this PM hook.

Patches 1 to 5 are just cleanup patches to simplify the EBI driver.
Patch 6 is fixing a bug in the EBI driver. Note that this bug has no
consequences until we start implementing the ->resume() hook.
And finally, patch 7 is implementing ->resume() so that the SMC config
is correctly restored when resuming the system.

This patchset depends on [1] which is removing the unused pata_at91
driver, which in turn prevents us from patching it before dropping
the old SMC macro definitions in patch 4.

Regards,

Boris

[1]http://lkml.iu.edu/hypermail/linux/kernel/1702.2/00990.html

Boris Brezillon (7):
  mfd: syscon: atmel-smc: Add new helpers to ease SMC regs manipulation
  memory: atmel-ebi: Simplify SMC config code
  memory: atmel-ebi: Stop using reg_field objects for simple things
  mfd: syscon: atmel-smc: Remove unused helpers/macros
  memory: atmel-ebi: Change naming scheme
  memory: atmel-ebi: Add missing ->numcs assignment
  memory: atmel-ebi: Add PM ops

 drivers/memory/atmel-ebi.c           | 583 ++++++++++++-----------------------
 include/linux/mfd/syscon/atmel-smc.h | 456 +++++++++++++++++++--------
 2 files changed, 532 insertions(+), 507 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH 1/7] mfd: syscon: atmel-smc: Add new helpers to ease SMC regs manipulation
  2017-02-20 16:54 ` Boris Brezillon
@ 2017-02-20 16:54   ` Boris Brezillon
  -1 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:54 UTC (permalink / raw)
  To: Nicolas Ferre, Alexandre Belloni
  Cc: linux-arm-kernel, Samuel Ortiz, Lee Jones, linux-kernel, Boris Brezillon

These new helpers + macro definitions are meant to replace the old ones
which are unpractical to use.

Note that the macros and function prefixes have been intentionally
changed to ATMEL_[H]SMC_XX and atmel_[h]smc_ to reflect the fact that
this IP is also embedded in avr32 SoCs (and not only in at91 ones).

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 include/linux/mfd/syscon/atmel-smc.h | 362 +++++++++++++++++++++++++++++++++++
 1 file changed, 362 insertions(+)

diff --git a/include/linux/mfd/syscon/atmel-smc.h b/include/linux/mfd/syscon/atmel-smc.h
index be6ebe64eebe..6124f3d2b965 100644
--- a/include/linux/mfd/syscon/atmel-smc.h
+++ b/include/linux/mfd/syscon/atmel-smc.h
@@ -69,6 +69,368 @@
 #define AT91_SMC_PS_16			(2 << 28)
 #define AT91_SMC_PS_32			(3 << 28)
 
+#define ATMEL_SMC_SETUP(cs)			(((cs) * 0x10))
+#define ATMEL_HSMC_SETUP(cs)			(0x600 + ((cs) * 0x14))
+#define ATMEL_SMC_PULSE(cs)			(((cs) * 0x10) + 0x4)
+#define ATMEL_HSMC_PULSE(cs)			(0x600 + ((cs) * 0x14) + 0x4)
+#define ATMEL_SMC_CYCLE(cs)			(((cs) * 0x10) + 0x8)
+#define ATMEL_HSMC_CYCLE(cs)			(0x600 + ((cs) * 0x14) + 0x8)
+#define ATMEL_SMC_NWE_SHIFT			0
+#define ATMEL_SMC_NCS_WR_SHIFT			8
+#define ATMEL_SMC_NRD_SHIFT			16
+#define ATMEL_SMC_NCS_RD_SHIFT			24
+
+#define ATMEL_SMC_MODE(cs)			(((cs) * 0x10) + 0xc)
+#define ATMEL_HSMC_MODE(cs)			(0x600 + ((cs) * 0x14) + 0x10)
+#define ATMEL_SMC_MODE_READMODE_MASK		BIT(0)
+#define ATMEL_SMC_MODE_READMODE_NCS		(0 << 0)
+#define ATMEL_SMC_MODE_READMODE_NRD		(1 << 0)
+#define ATMEL_SMC_MODE_WRITEMODE_MASK		BIT(1)
+#define ATMEL_SMC_MODE_WRITEMODE_NCS		(0 << 1)
+#define ATMEL_SMC_MODE_WRITEMODE_NWE		(1 << 1)
+#define ATMEL_SMC_MODE_EXNWMODE_MASK		GENMASK(5, 4)
+#define ATMEL_SMC_MODE_EXNWMODE_DISABLE		(0 << 4)
+#define ATMEL_SMC_MODE_EXNWMODE_FROZEN		(2 << 4)
+#define ATMEL_SMC_MODE_EXNWMODE_READY		(3 << 4)
+#define ATMEL_SMC_MODE_BAT_MASK			BIT(8)
+#define ATMEL_SMC_MODE_BAT_SELECT		(0 << 8)
+#define ATMEL_SMC_MODE_BAT_WRITE		(1 << 8)
+#define ATMEL_SMC_MODE_DBW_MASK			GENMASK(13, 12)
+#define ATMEL_SMC_MODE_DBW_8			(0 << 12)
+#define ATMEL_SMC_MODE_DBW_16			(1 << 12)
+#define ATMEL_SMC_MODE_DBW_32			(2 << 12)
+#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_TDFMODE_OPTIMIZED	BIT(20)
+#define ATMEL_SMC_MODE_PMEN			BIT(24)
+#define ATMEL_SMC_MODE_PS_MASK			GENMASK(29, 28)
+#define ATMEL_SMC_MODE_PS_4			(0 << 28)
+#define ATMEL_SMC_MODE_PS_8			(1 << 28)
+#define ATMEL_SMC_MODE_PS_16			(2 << 28)
+#define ATMEL_SMC_MODE_PS_32			(3 << 28)
+
+#define ATMEL_HSMC_TIMINGS(cs)			(0x600 + ((cs) * 0x14) + 0xc)
+#define ATMEL_HSMC_TIMINGS_OCMS			BIT(12)
+#define ATMEL_HSMC_TIMINGS_RBNSEL(x)		((x) << 28)
+#define ATMEL_HSMC_TIMINGS_NFSEL		BIT(31)
+#define ATMEL_HSMC_TIMINGS_TCLR_SHIFT		0
+#define ATMEL_HSMC_TIMINGS_TADL_SHIFT		4
+#define ATMEL_HSMC_TIMINGS_TAR_SHIFT		8
+#define ATMEL_HSMC_TIMINGS_TRR_SHIFT		16
+#define ATMEL_HSMC_TIMINGS_TWB_SHIFT		24
+
+/**
+ * struct atmel_smc_cs_conf - SMC CS config as described in the datasheet.
+ * @setup: NCS/NWE/NRD setup timings (not applicable to at91rm9200)
+ * @pulse: NCS/NWE/NRD pulse timings (not applicable to at91rm9200)
+ * @cycle: NWE/NRD cycle timings (not applicable to at91rm9200)
+ * @timings: advanced NAND related timings (only applicable to HSMC)
+ * @mode: all kind of config parameters (see the fields definition above).
+ *	  The mode fields are different on at91rm9200
+ */
+struct atmel_smc_cs_conf {
+	u32 setup;
+	u32 pulse;
+	u32 cycle;
+	u32 timings;
+	u32 mode;
+};
+
+/**
+ * atmel_smc_cs_conf_init - initialize a SMC CS conf
+ * @conf: the SMC CS conf to initialize
+ *
+ * Set all fields to 0 so that one can start defining a new config.
+ */
+static inline void atmel_smc_cs_conf_init(struct atmel_smc_cs_conf *conf)
+{
+	memset(conf, 0, sizeof(*conf));
+}
+
+/**
+ * atmel_smc_cs_encode_ncycles - encode a number of MCK clk cycles in the
+ *				 format expected by the SMC engine
+ * @ncycles: number of MCK clk cycles
+ * @msbpos: position of the MSB part of the timing field
+ * @msbwidth: width of the MSB part of the timing field
+ * @msbfactor: factor applied to the MSB
+ * @encodedval: param used to store the encoding result
+ *
+ * This function encodes the @ncycles value as described in the datasheet
+ * (section "SMC Setup/Pulse/Cycle/Timings Register"). This is a generic
+ * helper which called with different parameter depending on the encoding
+ * scheme.
+ *
+ * If the @ncycles value is too big to be encoded, -ERANGE is returned and
+ * the encodedval is contains the maximum val. Otherwise, 0 is returned.
+ */
+static inline int atmel_smc_cs_encode_ncycles(unsigned int ncycles,
+					      unsigned int msbpos,
+					      unsigned int msbwidth,
+					      unsigned int msbfactor,
+					      unsigned int *encodedval)
+{
+	unsigned int lsbmask = GENMASK(msbpos - 1, 0);
+	unsigned int msbmask = GENMASK(msbwidth - 1, 0);
+	unsigned int msb, lsb;
+	int ret = 0;
+
+	msb = ncycles / msbfactor;
+	lsb = ncycles % msbfactor;
+
+	if (lsb > lsbmask) {
+		lsb = 0;
+		msb++;
+	}
+
+	/*
+	 * Let's just put the maximum we can if the requested setting does
+	 * not fit in the register field.
+	 * We still return -ERANGE in case the caller cares.
+	 */
+	if (msb > msbmask) {
+		msb = msbmask;
+		lsb = lsbmask;
+		ret = -ERANGE;
+	}
+
+	*encodedval = (msb << msbpos) | lsb;
+
+	return ret;
+}
+
+/**
+ * atmel_smc_cs_conf_set_timing - set the SMC CS conf Txx parameter to a
+ *				  specific value
+ * @conf: SMC CS conf descriptor
+ * @shift: the position of the Txx field in the TIMINGS register
+ * @ncycles: value (expressed in MCK clk cycles) to assign to this Txx
+ *	     parameter
+ *
+ * This function encodes the @ncycles value as described in the datasheet
+ * (section "SMC Timings Register"), and then stores the result in the
+ * @conf->timings field at @shift position.
+ *
+ * Returns -EINVAL if shift is invalid, -ERANGE if ncycles does not fit in
+ * the field, and 0 otherwise.
+ */
+static inline int atmel_smc_cs_conf_set_timing(struct atmel_smc_cs_conf *conf,
+					       unsigned int shift,
+					       unsigned int ncycles)
+{
+	unsigned int val;
+	int ret;
+
+	if (shift != ATMEL_HSMC_TIMINGS_TCLR_SHIFT &&
+	    shift != ATMEL_HSMC_TIMINGS_TADL_SHIFT &&
+	    shift != ATMEL_HSMC_TIMINGS_TAR_SHIFT &&
+	    shift != ATMEL_HSMC_TIMINGS_TRR_SHIFT &&
+	    shift != ATMEL_HSMC_TIMINGS_TWB_SHIFT)
+		return -EINVAL;
+
+	/*
+	 * The formula described in atmel datasheets (section "HSMC Timings
+	 * Register"):
+	 *
+	 * ncycles = (Txx[3] * 64) + Txx[2:0]
+	 */
+	ret = atmel_smc_cs_encode_ncycles(ncycles, 3, 1, 64, &val);
+	conf->timings &= ~GENMASK(shift + 3, shift);
+	conf->timings |= val << shift;
+
+	return ret;
+}
+
+/**
+ * atmel_smc_cs_conf_set_setup - set the SMC CS conf xx_SETUP parameter to a
+ *				 specific value
+ * @conf: SMC CS conf descriptor
+ * @shift: the position of the xx_SETUP field in the SETUP register
+ * @ncycles: value (expressed in MCK clk cycles) to assign to this xx_SETUP
+ *	     parameter
+ *
+ * This function encodes the @ncycles value as described in the datasheet
+ * (section "SMC Setup Register"), and then stores the result in the
+ * @conf->setup field at @shift position.
+ *
+ * Returns -EINVAL if @shift is invalid, -ERANGE if @ncycles does not fit in
+ * the field, and 0 otherwise.
+ */
+static inline int atmel_smc_cs_conf_set_setup(struct atmel_smc_cs_conf *conf,
+					      unsigned int shift,
+					      unsigned int ncycles)
+{
+	unsigned int val;
+	int ret;
+
+	if (shift != ATMEL_SMC_NWE_SHIFT && shift != ATMEL_SMC_NCS_WR_SHIFT &&
+	    shift != ATMEL_SMC_NRD_SHIFT && shift != ATMEL_SMC_NCS_RD_SHIFT)
+		return -EINVAL;
+
+	/*
+	 * The formula described in atmel datasheets (section "SMC Setup
+	 * Register"):
+	 *
+	 * ncycles = (128 * xx_SETUP[5]) + xx_SETUP[4:0]
+	 */
+	ret = atmel_smc_cs_encode_ncycles(ncycles, 5, 1, 128, &val);
+	conf->setup &= ~GENMASK(shift + 7, shift);
+	conf->setup |= val << shift;
+
+	return ret;
+}
+
+/**
+ * atmel_smc_cs_conf_set_pulse - set the SMC CS conf xx_PULSE parameter to a
+ *				 specific value
+ * @conf: SMC CS conf descriptor
+ * @shift: the position of the xx_PULSE field in the PULSE register
+ * @ncycles: value (expressed in MCK clk cycles) to assign to this xx_PULSE
+ *	     parameter
+ *
+ * This function encodes the @ncycles value as described in the datasheet
+ * (section "SMC Pulse Register"), and then stores the result in the
+ * @conf->setup field at @shift position.
+ *
+ * Returns -EINVAL if @shift is invalid, -ERANGE if @ncycles does not fit in
+ * the field, and 0 otherwise.
+ */
+static inline int atmel_smc_cs_conf_set_pulse(struct atmel_smc_cs_conf *conf,
+					      unsigned int shift,
+					      unsigned int ncycles)
+{
+	unsigned int val;
+	int ret;
+
+	if (shift != ATMEL_SMC_NWE_SHIFT && shift != ATMEL_SMC_NCS_WR_SHIFT &&
+	    shift != ATMEL_SMC_NRD_SHIFT && shift != ATMEL_SMC_NCS_RD_SHIFT)
+		return -EINVAL;
+
+	/*
+	 * The formula described in atmel datasheets (section "SMC Pulse
+	 * Register"):
+	 *
+	 * ncycles = (256 * xx_PULSE[6]) + xx_PULSE[5:0]
+	 */
+	ret = atmel_smc_cs_encode_ncycles(ncycles, 6, 1, 256, &val);
+	conf->pulse &= ~GENMASK(shift + 7, shift);
+	conf->pulse |= val << shift;
+
+	return ret;
+}
+
+/**
+ * atmel_smc_cs_conf_set_cycle - set the SMC CS conf xx_CYCLE parameter to a
+ *				 specific value
+ * @conf: SMC CS conf descriptor
+ * @shift: the position of the xx_CYCLE field in the CYCLE register
+ * @ncycles: value (expressed in MCK clk cycles) to assign to this xx_CYCLE
+ *	     parameter
+ *
+ * This function encodes the @ncycles value as described in the datasheet
+ * (section "SMC Pulse Register"), and then stores the result in the
+ * @conf->setup field at @shift position.
+ *
+ * Returns -EINVAL if @shift is invalid, -ERANGE if @ncycles does not fit in
+ * the field, and 0 otherwise.
+ */
+static inline int atmel_smc_cs_conf_set_cycle(struct atmel_smc_cs_conf *conf,
+					      unsigned int shift,
+					      unsigned int ncycles)
+{
+	unsigned int val;
+	int ret;
+
+	if (shift != ATMEL_SMC_NWE_SHIFT && shift != ATMEL_SMC_NRD_SHIFT)
+		return -EINVAL;
+
+	/*
+	 * The formula described in atmel datasheets (section "SMC Cycle
+	 * Register"):
+	 *
+	 * ncycles = (xx_CYCLE[8:7] * 256) + xx_CYCLE[6:0]
+	 */
+	ret = atmel_smc_cs_encode_ncycles(ncycles, 7, 2, 256, &val);
+	conf->cycle &= ~GENMASK(shift + 15, shift);
+	conf->cycle |= val << shift;
+
+	return ret;
+}
+
+/**
+ * atmel_smc_cs_conf_apply - apply an SMC CS conf
+ * @regmap: the SMC regmap
+ * @cs: the CS id
+ * @conf the SMC CS conf to apply
+ *
+ * Applies an SMC CS configuration.
+ * Only valid on at91sam9/avr32 SoCs.
+ */
+static inline void atmel_smc_cs_conf_apply(struct regmap *regmap, int cs,
+					   const struct atmel_smc_cs_conf *conf)
+{
+	regmap_write(regmap, ATMEL_SMC_SETUP(cs), conf->setup);
+	regmap_write(regmap, ATMEL_SMC_PULSE(cs), conf->pulse);
+	regmap_write(regmap, ATMEL_SMC_CYCLE(cs), conf->cycle);
+	regmap_write(regmap, ATMEL_SMC_MODE(cs), conf->mode);
+}
+
+/**
+ * atmel_hsmc_cs_conf_apply - apply an SMC CS conf
+ * @regmap: the HSMC regmap
+ * @cs: the CS id
+ * @conf the SMC CS conf to apply
+ *
+ * Applies an SMC CS configuration.
+ * Only valid on post-sama5 SoCs.
+ */
+static inline void atmel_hsmc_cs_conf_apply(struct regmap *regmap, int cs,
+					const struct atmel_smc_cs_conf *conf)
+{
+	regmap_write(regmap, ATMEL_HSMC_SETUP(cs), conf->setup);
+	regmap_write(regmap, ATMEL_HSMC_PULSE(cs), conf->pulse);
+	regmap_write(regmap, ATMEL_HSMC_CYCLE(cs), conf->cycle);
+	regmap_write(regmap, ATMEL_HSMC_TIMINGS(cs), conf->timings);
+	regmap_write(regmap, ATMEL_HSMC_MODE(cs), conf->mode);
+}
+
+/**
+ * atmel_smc_cs_conf_get - retrieve the current SMC CS conf
+ * @regmap: the SMC regmap
+ * @cs: the CS id
+ * @conf: the SMC CS conf object to store the current conf
+ *
+ * Retrieve the SMC CS configuration.
+ * Only valid on at91sam9/avr32 SoCs.
+ */
+static inline void atmel_smc_cs_conf_get(struct regmap *regmap, int cs,
+					 struct atmel_smc_cs_conf *conf)
+{
+	regmap_read(regmap, ATMEL_SMC_SETUP(cs), &conf->setup);
+	regmap_read(regmap, ATMEL_SMC_PULSE(cs), &conf->pulse);
+	regmap_read(regmap, ATMEL_SMC_CYCLE(cs), &conf->cycle);
+	regmap_read(regmap, ATMEL_SMC_MODE(cs), &conf->mode);
+}
+
+/**
+ * atmel_hsmc_cs_conf_get - retrieve the current SMC CS conf
+ * @regmap: the HSMC regmap
+ * @cs: the CS id
+ * @conf: the SMC CS conf object to store the current conf
+ *
+ * Retrieve the SMC CS configuration.
+ * Only valid on post-sama5 SoCs.
+ */
+static inline void atmel_hsmc_cs_conf_get(struct regmap *regmap, int cs,
+					  struct atmel_smc_cs_conf *conf)
+{
+	regmap_read(regmap, ATMEL_HSMC_SETUP(cs), &conf->setup);
+	regmap_read(regmap, ATMEL_HSMC_PULSE(cs), &conf->pulse);
+	regmap_read(regmap, ATMEL_HSMC_CYCLE(cs), &conf->cycle);
+	regmap_read(regmap, ATMEL_HSMC_TIMINGS(cs), &conf->timings);
+	regmap_read(regmap, ATMEL_HSMC_MODE(cs), &conf->mode);
+}
 
 /*
  * This function converts a setup timing expressed in nanoseconds into an
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH 1/7] mfd: syscon: atmel-smc: Add new helpers to ease SMC regs manipulation
@ 2017-02-20 16:54   ` Boris Brezillon
  0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:54 UTC (permalink / raw)
  To: linux-arm-kernel

These new helpers + macro definitions are meant to replace the old ones
which are unpractical to use.

Note that the macros and function prefixes have been intentionally
changed to ATMEL_[H]SMC_XX and atmel_[h]smc_ to reflect the fact that
this IP is also embedded in avr32 SoCs (and not only in at91 ones).

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 include/linux/mfd/syscon/atmel-smc.h | 362 +++++++++++++++++++++++++++++++++++
 1 file changed, 362 insertions(+)

diff --git a/include/linux/mfd/syscon/atmel-smc.h b/include/linux/mfd/syscon/atmel-smc.h
index be6ebe64eebe..6124f3d2b965 100644
--- a/include/linux/mfd/syscon/atmel-smc.h
+++ b/include/linux/mfd/syscon/atmel-smc.h
@@ -69,6 +69,368 @@
 #define AT91_SMC_PS_16			(2 << 28)
 #define AT91_SMC_PS_32			(3 << 28)
 
+#define ATMEL_SMC_SETUP(cs)			(((cs) * 0x10))
+#define ATMEL_HSMC_SETUP(cs)			(0x600 + ((cs) * 0x14))
+#define ATMEL_SMC_PULSE(cs)			(((cs) * 0x10) + 0x4)
+#define ATMEL_HSMC_PULSE(cs)			(0x600 + ((cs) * 0x14) + 0x4)
+#define ATMEL_SMC_CYCLE(cs)			(((cs) * 0x10) + 0x8)
+#define ATMEL_HSMC_CYCLE(cs)			(0x600 + ((cs) * 0x14) + 0x8)
+#define ATMEL_SMC_NWE_SHIFT			0
+#define ATMEL_SMC_NCS_WR_SHIFT			8
+#define ATMEL_SMC_NRD_SHIFT			16
+#define ATMEL_SMC_NCS_RD_SHIFT			24
+
+#define ATMEL_SMC_MODE(cs)			(((cs) * 0x10) + 0xc)
+#define ATMEL_HSMC_MODE(cs)			(0x600 + ((cs) * 0x14) + 0x10)
+#define ATMEL_SMC_MODE_READMODE_MASK		BIT(0)
+#define ATMEL_SMC_MODE_READMODE_NCS		(0 << 0)
+#define ATMEL_SMC_MODE_READMODE_NRD		(1 << 0)
+#define ATMEL_SMC_MODE_WRITEMODE_MASK		BIT(1)
+#define ATMEL_SMC_MODE_WRITEMODE_NCS		(0 << 1)
+#define ATMEL_SMC_MODE_WRITEMODE_NWE		(1 << 1)
+#define ATMEL_SMC_MODE_EXNWMODE_MASK		GENMASK(5, 4)
+#define ATMEL_SMC_MODE_EXNWMODE_DISABLE		(0 << 4)
+#define ATMEL_SMC_MODE_EXNWMODE_FROZEN		(2 << 4)
+#define ATMEL_SMC_MODE_EXNWMODE_READY		(3 << 4)
+#define ATMEL_SMC_MODE_BAT_MASK			BIT(8)
+#define ATMEL_SMC_MODE_BAT_SELECT		(0 << 8)
+#define ATMEL_SMC_MODE_BAT_WRITE		(1 << 8)
+#define ATMEL_SMC_MODE_DBW_MASK			GENMASK(13, 12)
+#define ATMEL_SMC_MODE_DBW_8			(0 << 12)
+#define ATMEL_SMC_MODE_DBW_16			(1 << 12)
+#define ATMEL_SMC_MODE_DBW_32			(2 << 12)
+#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_TDFMODE_OPTIMIZED	BIT(20)
+#define ATMEL_SMC_MODE_PMEN			BIT(24)
+#define ATMEL_SMC_MODE_PS_MASK			GENMASK(29, 28)
+#define ATMEL_SMC_MODE_PS_4			(0 << 28)
+#define ATMEL_SMC_MODE_PS_8			(1 << 28)
+#define ATMEL_SMC_MODE_PS_16			(2 << 28)
+#define ATMEL_SMC_MODE_PS_32			(3 << 28)
+
+#define ATMEL_HSMC_TIMINGS(cs)			(0x600 + ((cs) * 0x14) + 0xc)
+#define ATMEL_HSMC_TIMINGS_OCMS			BIT(12)
+#define ATMEL_HSMC_TIMINGS_RBNSEL(x)		((x) << 28)
+#define ATMEL_HSMC_TIMINGS_NFSEL		BIT(31)
+#define ATMEL_HSMC_TIMINGS_TCLR_SHIFT		0
+#define ATMEL_HSMC_TIMINGS_TADL_SHIFT		4
+#define ATMEL_HSMC_TIMINGS_TAR_SHIFT		8
+#define ATMEL_HSMC_TIMINGS_TRR_SHIFT		16
+#define ATMEL_HSMC_TIMINGS_TWB_SHIFT		24
+
+/**
+ * struct atmel_smc_cs_conf - SMC CS config as described in the datasheet.
+ * @setup: NCS/NWE/NRD setup timings (not applicable to at91rm9200)
+ * @pulse: NCS/NWE/NRD pulse timings (not applicable to at91rm9200)
+ * @cycle: NWE/NRD cycle timings (not applicable to at91rm9200)
+ * @timings: advanced NAND related timings (only applicable to HSMC)
+ * @mode: all kind of config parameters (see the fields definition above).
+ *	  The mode fields are different on at91rm9200
+ */
+struct atmel_smc_cs_conf {
+	u32 setup;
+	u32 pulse;
+	u32 cycle;
+	u32 timings;
+	u32 mode;
+};
+
+/**
+ * atmel_smc_cs_conf_init - initialize a SMC CS conf
+ * @conf: the SMC CS conf to initialize
+ *
+ * Set all fields to 0 so that one can start defining a new config.
+ */
+static inline void atmel_smc_cs_conf_init(struct atmel_smc_cs_conf *conf)
+{
+	memset(conf, 0, sizeof(*conf));
+}
+
+/**
+ * atmel_smc_cs_encode_ncycles - encode a number of MCK clk cycles in the
+ *				 format expected by the SMC engine
+ * @ncycles: number of MCK clk cycles
+ * @msbpos: position of the MSB part of the timing field
+ * @msbwidth: width of the MSB part of the timing field
+ * @msbfactor: factor applied to the MSB
+ * @encodedval: param used to store the encoding result
+ *
+ * This function encodes the @ncycles value as described in the datasheet
+ * (section "SMC Setup/Pulse/Cycle/Timings Register"). This is a generic
+ * helper which called with different parameter depending on the encoding
+ * scheme.
+ *
+ * If the @ncycles value is too big to be encoded, -ERANGE is returned and
+ * the encodedval is contains the maximum val. Otherwise, 0 is returned.
+ */
+static inline int atmel_smc_cs_encode_ncycles(unsigned int ncycles,
+					      unsigned int msbpos,
+					      unsigned int msbwidth,
+					      unsigned int msbfactor,
+					      unsigned int *encodedval)
+{
+	unsigned int lsbmask = GENMASK(msbpos - 1, 0);
+	unsigned int msbmask = GENMASK(msbwidth - 1, 0);
+	unsigned int msb, lsb;
+	int ret = 0;
+
+	msb = ncycles / msbfactor;
+	lsb = ncycles % msbfactor;
+
+	if (lsb > lsbmask) {
+		lsb = 0;
+		msb++;
+	}
+
+	/*
+	 * Let's just put the maximum we can if the requested setting does
+	 * not fit in the register field.
+	 * We still return -ERANGE in case the caller cares.
+	 */
+	if (msb > msbmask) {
+		msb = msbmask;
+		lsb = lsbmask;
+		ret = -ERANGE;
+	}
+
+	*encodedval = (msb << msbpos) | lsb;
+
+	return ret;
+}
+
+/**
+ * atmel_smc_cs_conf_set_timing - set the SMC CS conf Txx parameter to a
+ *				  specific value
+ * @conf: SMC CS conf descriptor
+ * @shift: the position of the Txx field in the TIMINGS register
+ * @ncycles: value (expressed in MCK clk cycles) to assign to this Txx
+ *	     parameter
+ *
+ * This function encodes the @ncycles value as described in the datasheet
+ * (section "SMC Timings Register"), and then stores the result in the
+ * @conf->timings field at @shift position.
+ *
+ * Returns -EINVAL if shift is invalid, -ERANGE if ncycles does not fit in
+ * the field, and 0 otherwise.
+ */
+static inline int atmel_smc_cs_conf_set_timing(struct atmel_smc_cs_conf *conf,
+					       unsigned int shift,
+					       unsigned int ncycles)
+{
+	unsigned int val;
+	int ret;
+
+	if (shift != ATMEL_HSMC_TIMINGS_TCLR_SHIFT &&
+	    shift != ATMEL_HSMC_TIMINGS_TADL_SHIFT &&
+	    shift != ATMEL_HSMC_TIMINGS_TAR_SHIFT &&
+	    shift != ATMEL_HSMC_TIMINGS_TRR_SHIFT &&
+	    shift != ATMEL_HSMC_TIMINGS_TWB_SHIFT)
+		return -EINVAL;
+
+	/*
+	 * The formula described in atmel datasheets (section "HSMC Timings
+	 * Register"):
+	 *
+	 * ncycles = (Txx[3] * 64) + Txx[2:0]
+	 */
+	ret = atmel_smc_cs_encode_ncycles(ncycles, 3, 1, 64, &val);
+	conf->timings &= ~GENMASK(shift + 3, shift);
+	conf->timings |= val << shift;
+
+	return ret;
+}
+
+/**
+ * atmel_smc_cs_conf_set_setup - set the SMC CS conf xx_SETUP parameter to a
+ *				 specific value
+ * @conf: SMC CS conf descriptor
+ * @shift: the position of the xx_SETUP field in the SETUP register
+ * @ncycles: value (expressed in MCK clk cycles) to assign to this xx_SETUP
+ *	     parameter
+ *
+ * This function encodes the @ncycles value as described in the datasheet
+ * (section "SMC Setup Register"), and then stores the result in the
+ * @conf->setup field at @shift position.
+ *
+ * Returns -EINVAL if @shift is invalid, -ERANGE if @ncycles does not fit in
+ * the field, and 0 otherwise.
+ */
+static inline int atmel_smc_cs_conf_set_setup(struct atmel_smc_cs_conf *conf,
+					      unsigned int shift,
+					      unsigned int ncycles)
+{
+	unsigned int val;
+	int ret;
+
+	if (shift != ATMEL_SMC_NWE_SHIFT && shift != ATMEL_SMC_NCS_WR_SHIFT &&
+	    shift != ATMEL_SMC_NRD_SHIFT && shift != ATMEL_SMC_NCS_RD_SHIFT)
+		return -EINVAL;
+
+	/*
+	 * The formula described in atmel datasheets (section "SMC Setup
+	 * Register"):
+	 *
+	 * ncycles = (128 * xx_SETUP[5]) + xx_SETUP[4:0]
+	 */
+	ret = atmel_smc_cs_encode_ncycles(ncycles, 5, 1, 128, &val);
+	conf->setup &= ~GENMASK(shift + 7, shift);
+	conf->setup |= val << shift;
+
+	return ret;
+}
+
+/**
+ * atmel_smc_cs_conf_set_pulse - set the SMC CS conf xx_PULSE parameter to a
+ *				 specific value
+ * @conf: SMC CS conf descriptor
+ * @shift: the position of the xx_PULSE field in the PULSE register
+ * @ncycles: value (expressed in MCK clk cycles) to assign to this xx_PULSE
+ *	     parameter
+ *
+ * This function encodes the @ncycles value as described in the datasheet
+ * (section "SMC Pulse Register"), and then stores the result in the
+ * @conf->setup field at @shift position.
+ *
+ * Returns -EINVAL if @shift is invalid, -ERANGE if @ncycles does not fit in
+ * the field, and 0 otherwise.
+ */
+static inline int atmel_smc_cs_conf_set_pulse(struct atmel_smc_cs_conf *conf,
+					      unsigned int shift,
+					      unsigned int ncycles)
+{
+	unsigned int val;
+	int ret;
+
+	if (shift != ATMEL_SMC_NWE_SHIFT && shift != ATMEL_SMC_NCS_WR_SHIFT &&
+	    shift != ATMEL_SMC_NRD_SHIFT && shift != ATMEL_SMC_NCS_RD_SHIFT)
+		return -EINVAL;
+
+	/*
+	 * The formula described in atmel datasheets (section "SMC Pulse
+	 * Register"):
+	 *
+	 * ncycles = (256 * xx_PULSE[6]) + xx_PULSE[5:0]
+	 */
+	ret = atmel_smc_cs_encode_ncycles(ncycles, 6, 1, 256, &val);
+	conf->pulse &= ~GENMASK(shift + 7, shift);
+	conf->pulse |= val << shift;
+
+	return ret;
+}
+
+/**
+ * atmel_smc_cs_conf_set_cycle - set the SMC CS conf xx_CYCLE parameter to a
+ *				 specific value
+ * @conf: SMC CS conf descriptor
+ * @shift: the position of the xx_CYCLE field in the CYCLE register
+ * @ncycles: value (expressed in MCK clk cycles) to assign to this xx_CYCLE
+ *	     parameter
+ *
+ * This function encodes the @ncycles value as described in the datasheet
+ * (section "SMC Pulse Register"), and then stores the result in the
+ * @conf->setup field at @shift position.
+ *
+ * Returns -EINVAL if @shift is invalid, -ERANGE if @ncycles does not fit in
+ * the field, and 0 otherwise.
+ */
+static inline int atmel_smc_cs_conf_set_cycle(struct atmel_smc_cs_conf *conf,
+					      unsigned int shift,
+					      unsigned int ncycles)
+{
+	unsigned int val;
+	int ret;
+
+	if (shift != ATMEL_SMC_NWE_SHIFT && shift != ATMEL_SMC_NRD_SHIFT)
+		return -EINVAL;
+
+	/*
+	 * The formula described in atmel datasheets (section "SMC Cycle
+	 * Register"):
+	 *
+	 * ncycles = (xx_CYCLE[8:7] * 256) + xx_CYCLE[6:0]
+	 */
+	ret = atmel_smc_cs_encode_ncycles(ncycles, 7, 2, 256, &val);
+	conf->cycle &= ~GENMASK(shift + 15, shift);
+	conf->cycle |= val << shift;
+
+	return ret;
+}
+
+/**
+ * atmel_smc_cs_conf_apply - apply an SMC CS conf
+ * @regmap: the SMC regmap
+ * @cs: the CS id
+ * @conf the SMC CS conf to apply
+ *
+ * Applies an SMC CS configuration.
+ * Only valid on at91sam9/avr32 SoCs.
+ */
+static inline void atmel_smc_cs_conf_apply(struct regmap *regmap, int cs,
+					   const struct atmel_smc_cs_conf *conf)
+{
+	regmap_write(regmap, ATMEL_SMC_SETUP(cs), conf->setup);
+	regmap_write(regmap, ATMEL_SMC_PULSE(cs), conf->pulse);
+	regmap_write(regmap, ATMEL_SMC_CYCLE(cs), conf->cycle);
+	regmap_write(regmap, ATMEL_SMC_MODE(cs), conf->mode);
+}
+
+/**
+ * atmel_hsmc_cs_conf_apply - apply an SMC CS conf
+ * @regmap: the HSMC regmap
+ * @cs: the CS id
+ * @conf the SMC CS conf to apply
+ *
+ * Applies an SMC CS configuration.
+ * Only valid on post-sama5 SoCs.
+ */
+static inline void atmel_hsmc_cs_conf_apply(struct regmap *regmap, int cs,
+					const struct atmel_smc_cs_conf *conf)
+{
+	regmap_write(regmap, ATMEL_HSMC_SETUP(cs), conf->setup);
+	regmap_write(regmap, ATMEL_HSMC_PULSE(cs), conf->pulse);
+	regmap_write(regmap, ATMEL_HSMC_CYCLE(cs), conf->cycle);
+	regmap_write(regmap, ATMEL_HSMC_TIMINGS(cs), conf->timings);
+	regmap_write(regmap, ATMEL_HSMC_MODE(cs), conf->mode);
+}
+
+/**
+ * atmel_smc_cs_conf_get - retrieve the current SMC CS conf
+ * @regmap: the SMC regmap
+ * @cs: the CS id
+ * @conf: the SMC CS conf object to store the current conf
+ *
+ * Retrieve the SMC CS configuration.
+ * Only valid on at91sam9/avr32 SoCs.
+ */
+static inline void atmel_smc_cs_conf_get(struct regmap *regmap, int cs,
+					 struct atmel_smc_cs_conf *conf)
+{
+	regmap_read(regmap, ATMEL_SMC_SETUP(cs), &conf->setup);
+	regmap_read(regmap, ATMEL_SMC_PULSE(cs), &conf->pulse);
+	regmap_read(regmap, ATMEL_SMC_CYCLE(cs), &conf->cycle);
+	regmap_read(regmap, ATMEL_SMC_MODE(cs), &conf->mode);
+}
+
+/**
+ * atmel_hsmc_cs_conf_get - retrieve the current SMC CS conf
+ * @regmap: the HSMC regmap
+ * @cs: the CS id
+ * @conf: the SMC CS conf object to store the current conf
+ *
+ * Retrieve the SMC CS configuration.
+ * Only valid on post-sama5 SoCs.
+ */
+static inline void atmel_hsmc_cs_conf_get(struct regmap *regmap, int cs,
+					  struct atmel_smc_cs_conf *conf)
+{
+	regmap_read(regmap, ATMEL_HSMC_SETUP(cs), &conf->setup);
+	regmap_read(regmap, ATMEL_HSMC_PULSE(cs), &conf->pulse);
+	regmap_read(regmap, ATMEL_HSMC_CYCLE(cs), &conf->cycle);
+	regmap_read(regmap, ATMEL_HSMC_TIMINGS(cs), &conf->timings);
+	regmap_read(regmap, ATMEL_HSMC_MODE(cs), &conf->mode);
+}
 
 /*
  * This function converts a setup timing expressed in nanoseconds into an
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code
  2017-02-20 16:54 ` Boris Brezillon
@ 2017-02-20 16:54   ` Boris Brezillon
  -1 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:54 UTC (permalink / raw)
  To: Nicolas Ferre, Alexandre Belloni
  Cc: linux-arm-kernel, Samuel Ortiz, Lee Jones, linux-kernel, Boris Brezillon

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

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

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code
@ 2017-02-20 16:54   ` Boris Brezillon
  0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:54 UTC (permalink / raw)
  To: linux-arm-kernel

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

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

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH 3/7] memory: atmel-ebi: Stop using reg_field objects for simple things
  2017-02-20 16:54 ` Boris Brezillon
@ 2017-02-20 16:54   ` Boris Brezillon
  -1 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:54 UTC (permalink / raw)
  To: Nicolas Ferre, Alexandre Belloni
  Cc: linux-arm-kernel, Samuel Ortiz, Lee Jones, linux-kernel, Boris Brezillon

Turn the ->ebi_csa reg field into a simple offset that can be used with
with the matrix regmap. Using reg fields was overkill for this use case.

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

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index 8363735f8aef..6b3c9fc7a602 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -35,7 +35,7 @@ struct at91_ebi_dev {
 
 struct at91_ebi_caps {
 	unsigned int available_cs;
-	const struct reg_field *ebi_csa;
+	unsigned int ebi_csa_offs;
 	void (*get_config)(struct at91_ebi_dev *ebid,
 			   struct at91_ebi_dev_config *conf);
 	int (*xlate_config)(struct at91_ebi_dev *ebid,
@@ -52,7 +52,6 @@ struct at91_ebi {
 		struct regmap *regmap;
 		struct clk *clk;
 	} smc;
-	struct regmap_field *ebi_csa;
 
 	struct device *dev;
 	const struct at91_ebi_caps *caps;
@@ -354,9 +353,10 @@ static int at91_ebi_dev_setup(struct at91_ebi *ebi, struct device_node *np,
 		 * Attach the EBI device to the generic SMC logic if at least
 		 * one "atmel,smc-" property is present.
 		 */
-		if (ebi->ebi_csa && apply)
-			regmap_field_update_bits(ebi->ebi_csa,
-						 BIT(cs), 0);
+		if (ebi->caps->ebi_csa_offs && apply)
+			regmap_update_bits(ebi->matrix,
+					   ebi->caps->ebi_csa_offs,
+					   BIT(cs), 0);
 
 		i++;
 	}
@@ -366,73 +366,49 @@ static int at91_ebi_dev_setup(struct at91_ebi *ebi, struct device_node *np,
 	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,
+	.ebi_csa_offs = AT91SAM9260_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
 	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-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,
+	.ebi_csa_offs = AT91SAM9261_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
 	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-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,
+	.ebi_csa_offs = AT91SAM9263_MATRIX_EBI0CSA,
 	.get_config = at91sam9_ebi_get_config,
 	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-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,
+	.ebi_csa_offs = AT91SAM9263_MATRIX_EBI1CSA,
 	.get_config = at91sam9_ebi_get_config,
 	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-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,
+	.ebi_csa_offs = AT91SAM9RL_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
 	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-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,
+	.ebi_csa_offs = AT91SAM9G45_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
 	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
@@ -440,7 +416,7 @@ static const struct at91_ebi_caps at91sam9g45_ebi_caps = {
 
 static const struct at91_ebi_caps at91sam9x5_ebi_caps = {
 	.available_cs = 0x3f,
-	.ebi_csa = &at91sam9263_ebi0_csa,
+	.ebi_csa_offs = AT91SAM9X5_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
 	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
@@ -560,16 +536,11 @@ static int at91_ebi_probe(struct platform_device *pdev)
 	 * The sama5d3 does not provide an EBICSA register and thus does need
 	 * to access the matrix registers.
 	 */
-	if (ebi->caps->ebi_csa) {
+	if (ebi->caps->ebi_csa_offs) {
 		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 = of_property_read_u32(np, "#address-cells", &val);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH 3/7] memory: atmel-ebi: Stop using reg_field objects for simple things
@ 2017-02-20 16:54   ` Boris Brezillon
  0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:54 UTC (permalink / raw)
  To: linux-arm-kernel

Turn the ->ebi_csa reg field into a simple offset that can be used with
with the matrix regmap. Using reg fields was overkill for this use case.

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

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index 8363735f8aef..6b3c9fc7a602 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -35,7 +35,7 @@ struct at91_ebi_dev {
 
 struct at91_ebi_caps {
 	unsigned int available_cs;
-	const struct reg_field *ebi_csa;
+	unsigned int ebi_csa_offs;
 	void (*get_config)(struct at91_ebi_dev *ebid,
 			   struct at91_ebi_dev_config *conf);
 	int (*xlate_config)(struct at91_ebi_dev *ebid,
@@ -52,7 +52,6 @@ struct at91_ebi {
 		struct regmap *regmap;
 		struct clk *clk;
 	} smc;
-	struct regmap_field *ebi_csa;
 
 	struct device *dev;
 	const struct at91_ebi_caps *caps;
@@ -354,9 +353,10 @@ static int at91_ebi_dev_setup(struct at91_ebi *ebi, struct device_node *np,
 		 * Attach the EBI device to the generic SMC logic if at least
 		 * one "atmel,smc-" property is present.
 		 */
-		if (ebi->ebi_csa && apply)
-			regmap_field_update_bits(ebi->ebi_csa,
-						 BIT(cs), 0);
+		if (ebi->caps->ebi_csa_offs && apply)
+			regmap_update_bits(ebi->matrix,
+					   ebi->caps->ebi_csa_offs,
+					   BIT(cs), 0);
 
 		i++;
 	}
@@ -366,73 +366,49 @@ static int at91_ebi_dev_setup(struct at91_ebi *ebi, struct device_node *np,
 	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,
+	.ebi_csa_offs = AT91SAM9260_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
 	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-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,
+	.ebi_csa_offs = AT91SAM9261_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
 	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-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,
+	.ebi_csa_offs = AT91SAM9263_MATRIX_EBI0CSA,
 	.get_config = at91sam9_ebi_get_config,
 	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-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,
+	.ebi_csa_offs = AT91SAM9263_MATRIX_EBI1CSA,
 	.get_config = at91sam9_ebi_get_config,
 	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-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,
+	.ebi_csa_offs = AT91SAM9RL_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
 	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-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,
+	.ebi_csa_offs = AT91SAM9G45_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
 	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
@@ -440,7 +416,7 @@ static const struct at91_ebi_caps at91sam9g45_ebi_caps = {
 
 static const struct at91_ebi_caps at91sam9x5_ebi_caps = {
 	.available_cs = 0x3f,
-	.ebi_csa = &at91sam9263_ebi0_csa,
+	.ebi_csa_offs = AT91SAM9X5_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
 	.xlate_config = at91_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
@@ -560,16 +536,11 @@ static int at91_ebi_probe(struct platform_device *pdev)
 	 * The sama5d3 does not provide an EBICSA register and thus does need
 	 * to access the matrix registers.
 	 */
-	if (ebi->caps->ebi_csa) {
+	if (ebi->caps->ebi_csa_offs) {
 		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 = of_property_read_u32(np, "#address-cells", &val);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH 4/7] mfd: syscon: atmel-smc: Remove unused helpers/macros
  2017-02-20 16:54 ` Boris Brezillon
@ 2017-02-20 16:54   ` Boris Brezillon
  -1 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:54 UTC (permalink / raw)
  To: Nicolas Ferre, Alexandre Belloni
  Cc: linux-arm-kernel, Samuel Ortiz, Lee Jones, linux-kernel, Boris Brezillon

All macros prefixed with AT91[SAM9]_SMC have been replaced by equivalent
definitions prefixed with ATMEL_SMC, and the at91sam9_smc_xxxx() helpers
are no longer used.
Drop these definitions before someone starts using them again.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 include/linux/mfd/syscon/atmel-smc.h | 152 -----------------------------------
 1 file changed, 152 deletions(-)

diff --git a/include/linux/mfd/syscon/atmel-smc.h b/include/linux/mfd/syscon/atmel-smc.h
index 6124f3d2b965..6ebef0d4d9d5 100644
--- a/include/linux/mfd/syscon/atmel-smc.h
+++ b/include/linux/mfd/syscon/atmel-smc.h
@@ -17,58 +17,6 @@
 #include <linux/kernel.h>
 #include <linux/regmap.h>
 
-#define AT91SAM9_SMC_GENERIC		0x00
-#define AT91SAM9_SMC_GENERIC_BLK_SZ	0x10
-
-#define SAMA5_SMC_GENERIC		0x600
-#define SAMA5_SMC_GENERIC_BLK_SZ	0x14
-
-#define AT91SAM9_SMC_SETUP(o)		((o) + 0x00)
-#define AT91SAM9_SMC_NWESETUP(x)	(x)
-#define AT91SAM9_SMC_NCS_WRSETUP(x)	((x) << 8)
-#define AT91SAM9_SMC_NRDSETUP(x)	((x) << 16)
-#define AT91SAM9_SMC_NCS_NRDSETUP(x)	((x) << 24)
-
-#define AT91SAM9_SMC_PULSE(o)		((o) + 0x04)
-#define AT91SAM9_SMC_NWEPULSE(x)	(x)
-#define AT91SAM9_SMC_NCS_WRPULSE(x)	((x) << 8)
-#define AT91SAM9_SMC_NRDPULSE(x)	((x) << 16)
-#define AT91SAM9_SMC_NCS_NRDPULSE(x)	((x) << 24)
-
-#define AT91SAM9_SMC_CYCLE(o)		((o) + 0x08)
-#define AT91SAM9_SMC_NWECYCLE(x)	(x)
-#define AT91SAM9_SMC_NRDCYCLE(x)	((x) << 16)
-
-#define AT91SAM9_SMC_MODE(o)		((o) + 0x0c)
-#define SAMA5_SMC_MODE(o)		((o) + 0x10)
-#define AT91_SMC_READMODE		BIT(0)
-#define AT91_SMC_READMODE_NCS		(0 << 0)
-#define AT91_SMC_READMODE_NRD		(1 << 0)
-#define AT91_SMC_WRITEMODE		BIT(1)
-#define AT91_SMC_WRITEMODE_NCS		(0 << 1)
-#define AT91_SMC_WRITEMODE_NWE		(1 << 1)
-#define AT91_SMC_EXNWMODE		GENMASK(5, 4)
-#define AT91_SMC_EXNWMODE_DISABLE	(0 << 4)
-#define AT91_SMC_EXNWMODE_FROZEN	(2 << 4)
-#define AT91_SMC_EXNWMODE_READY		(3 << 4)
-#define AT91_SMC_BAT			BIT(8)
-#define AT91_SMC_BAT_SELECT		(0 << 8)
-#define AT91_SMC_BAT_WRITE		(1 << 8)
-#define AT91_SMC_DBW			GENMASK(13, 12)
-#define AT91_SMC_DBW_8			(0 << 12)
-#define AT91_SMC_DBW_16			(1 << 12)
-#define AT91_SMC_DBW_32			(2 << 12)
-#define AT91_SMC_TDF			GENMASK(19, 16)
-#define AT91_SMC_TDF_(x)		((((x) - 1) << 16) & AT91_SMC_TDF)
-#define AT91_SMC_TDF_MAX		16
-#define AT91_SMC_TDFMODE_OPTIMIZED	BIT(20)
-#define AT91_SMC_PMEN			BIT(24)
-#define AT91_SMC_PS			GENMASK(29, 28)
-#define AT91_SMC_PS_4			(0 << 28)
-#define AT91_SMC_PS_8			(1 << 28)
-#define AT91_SMC_PS_16			(2 << 28)
-#define AT91_SMC_PS_32			(3 << 28)
-
 #define ATMEL_SMC_SETUP(cs)			(((cs) * 0x10))
 #define ATMEL_HSMC_SETUP(cs)			(0x600 + ((cs) * 0x14))
 #define ATMEL_SMC_PULSE(cs)			(((cs) * 0x10) + 0x4)
@@ -432,104 +380,4 @@ static inline void atmel_hsmc_cs_conf_get(struct regmap *regmap, int cs,
 	regmap_read(regmap, ATMEL_HSMC_MODE(cs), &conf->mode);
 }
 
-/*
- * This function converts a setup timing expressed in nanoseconds into an
- * encoded value that can be written in the SMC_SETUP register.
- *
- * The following formula is described in atmel datasheets (section
- * "SMC Setup Register"):
- *
- * setup length = (128* SETUP[5] + SETUP[4:0])
- *
- * where setup length is the timing expressed in cycles.
- */
-static inline u32 at91sam9_smc_setup_ns_to_cycles(unsigned int clk_rate,
-						  u32 timing_ns)
-{
-	u32 clk_period = DIV_ROUND_UP(NSEC_PER_SEC, clk_rate);
-	u32 coded_cycles = 0;
-	u32 cycles;
-
-	cycles = DIV_ROUND_UP(timing_ns, clk_period);
-	if (cycles / 32) {
-		coded_cycles |= 1 << 5;
-		if (cycles < 128)
-			cycles = 0;
-	}
-
-	coded_cycles |= cycles % 32;
-
-	return coded_cycles;
-}
-
-/*
- * This function converts a pulse timing expressed in nanoseconds into an
- * encoded value that can be written in the SMC_PULSE register.
- *
- * The following formula is described in atmel datasheets (section
- * "SMC Pulse Register"):
- *
- * pulse length = (256* PULSE[6] + PULSE[5:0])
- *
- * where pulse length is the timing expressed in cycles.
- */
-static inline u32 at91sam9_smc_pulse_ns_to_cycles(unsigned int clk_rate,
-						  u32 timing_ns)
-{
-	u32 clk_period = DIV_ROUND_UP(NSEC_PER_SEC, clk_rate);
-	u32 coded_cycles = 0;
-	u32 cycles;
-
-	cycles = DIV_ROUND_UP(timing_ns, clk_period);
-	if (cycles / 64) {
-		coded_cycles |= 1 << 6;
-		if (cycles < 256)
-			cycles = 0;
-	}
-
-	coded_cycles |= cycles % 64;
-
-	return coded_cycles;
-}
-
-/*
- * This function converts a cycle timing expressed in nanoseconds into an
- * encoded value that can be written in the SMC_CYCLE register.
- *
- * The following formula is described in atmel datasheets (section
- * "SMC Cycle Register"):
- *
- * cycle length = (CYCLE[8:7]*256 + CYCLE[6:0])
- *
- * where cycle length is the timing expressed in cycles.
- */
-static inline u32 at91sam9_smc_cycle_ns_to_cycles(unsigned int clk_rate,
-						  u32 timing_ns)
-{
-	u32 clk_period = DIV_ROUND_UP(NSEC_PER_SEC, clk_rate);
-	u32 coded_cycles = 0;
-	u32 cycles;
-
-	cycles = DIV_ROUND_UP(timing_ns, clk_period);
-	if (cycles / 128) {
-		coded_cycles = cycles / 256;
-		cycles %= 256;
-		if (cycles >= 128) {
-			coded_cycles++;
-			cycles = 0;
-		}
-
-		if (coded_cycles > 0x3) {
-			coded_cycles = 0x3;
-			cycles = 0x7f;
-		}
-
-		coded_cycles <<= 7;
-	}
-
-	coded_cycles |= cycles % 128;
-
-	return coded_cycles;
-}
-
 #endif /* _LINUX_MFD_SYSCON_ATMEL_SMC_H_ */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH 4/7] mfd: syscon: atmel-smc: Remove unused helpers/macros
@ 2017-02-20 16:54   ` Boris Brezillon
  0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:54 UTC (permalink / raw)
  To: linux-arm-kernel

All macros prefixed with AT91[SAM9]_SMC have been replaced by equivalent
definitions prefixed with ATMEL_SMC, and the at91sam9_smc_xxxx() helpers
are no longer used.
Drop these definitions before someone starts using them again.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 include/linux/mfd/syscon/atmel-smc.h | 152 -----------------------------------
 1 file changed, 152 deletions(-)

diff --git a/include/linux/mfd/syscon/atmel-smc.h b/include/linux/mfd/syscon/atmel-smc.h
index 6124f3d2b965..6ebef0d4d9d5 100644
--- a/include/linux/mfd/syscon/atmel-smc.h
+++ b/include/linux/mfd/syscon/atmel-smc.h
@@ -17,58 +17,6 @@
 #include <linux/kernel.h>
 #include <linux/regmap.h>
 
-#define AT91SAM9_SMC_GENERIC		0x00
-#define AT91SAM9_SMC_GENERIC_BLK_SZ	0x10
-
-#define SAMA5_SMC_GENERIC		0x600
-#define SAMA5_SMC_GENERIC_BLK_SZ	0x14
-
-#define AT91SAM9_SMC_SETUP(o)		((o) + 0x00)
-#define AT91SAM9_SMC_NWESETUP(x)	(x)
-#define AT91SAM9_SMC_NCS_WRSETUP(x)	((x) << 8)
-#define AT91SAM9_SMC_NRDSETUP(x)	((x) << 16)
-#define AT91SAM9_SMC_NCS_NRDSETUP(x)	((x) << 24)
-
-#define AT91SAM9_SMC_PULSE(o)		((o) + 0x04)
-#define AT91SAM9_SMC_NWEPULSE(x)	(x)
-#define AT91SAM9_SMC_NCS_WRPULSE(x)	((x) << 8)
-#define AT91SAM9_SMC_NRDPULSE(x)	((x) << 16)
-#define AT91SAM9_SMC_NCS_NRDPULSE(x)	((x) << 24)
-
-#define AT91SAM9_SMC_CYCLE(o)		((o) + 0x08)
-#define AT91SAM9_SMC_NWECYCLE(x)	(x)
-#define AT91SAM9_SMC_NRDCYCLE(x)	((x) << 16)
-
-#define AT91SAM9_SMC_MODE(o)		((o) + 0x0c)
-#define SAMA5_SMC_MODE(o)		((o) + 0x10)
-#define AT91_SMC_READMODE		BIT(0)
-#define AT91_SMC_READMODE_NCS		(0 << 0)
-#define AT91_SMC_READMODE_NRD		(1 << 0)
-#define AT91_SMC_WRITEMODE		BIT(1)
-#define AT91_SMC_WRITEMODE_NCS		(0 << 1)
-#define AT91_SMC_WRITEMODE_NWE		(1 << 1)
-#define AT91_SMC_EXNWMODE		GENMASK(5, 4)
-#define AT91_SMC_EXNWMODE_DISABLE	(0 << 4)
-#define AT91_SMC_EXNWMODE_FROZEN	(2 << 4)
-#define AT91_SMC_EXNWMODE_READY		(3 << 4)
-#define AT91_SMC_BAT			BIT(8)
-#define AT91_SMC_BAT_SELECT		(0 << 8)
-#define AT91_SMC_BAT_WRITE		(1 << 8)
-#define AT91_SMC_DBW			GENMASK(13, 12)
-#define AT91_SMC_DBW_8			(0 << 12)
-#define AT91_SMC_DBW_16			(1 << 12)
-#define AT91_SMC_DBW_32			(2 << 12)
-#define AT91_SMC_TDF			GENMASK(19, 16)
-#define AT91_SMC_TDF_(x)		((((x) - 1) << 16) & AT91_SMC_TDF)
-#define AT91_SMC_TDF_MAX		16
-#define AT91_SMC_TDFMODE_OPTIMIZED	BIT(20)
-#define AT91_SMC_PMEN			BIT(24)
-#define AT91_SMC_PS			GENMASK(29, 28)
-#define AT91_SMC_PS_4			(0 << 28)
-#define AT91_SMC_PS_8			(1 << 28)
-#define AT91_SMC_PS_16			(2 << 28)
-#define AT91_SMC_PS_32			(3 << 28)
-
 #define ATMEL_SMC_SETUP(cs)			(((cs) * 0x10))
 #define ATMEL_HSMC_SETUP(cs)			(0x600 + ((cs) * 0x14))
 #define ATMEL_SMC_PULSE(cs)			(((cs) * 0x10) + 0x4)
@@ -432,104 +380,4 @@ static inline void atmel_hsmc_cs_conf_get(struct regmap *regmap, int cs,
 	regmap_read(regmap, ATMEL_HSMC_MODE(cs), &conf->mode);
 }
 
-/*
- * This function converts a setup timing expressed in nanoseconds into an
- * encoded value that can be written in the SMC_SETUP register.
- *
- * The following formula is described in atmel datasheets (section
- * "SMC Setup Register"):
- *
- * setup length = (128* SETUP[5] + SETUP[4:0])
- *
- * where setup length is the timing expressed in cycles.
- */
-static inline u32 at91sam9_smc_setup_ns_to_cycles(unsigned int clk_rate,
-						  u32 timing_ns)
-{
-	u32 clk_period = DIV_ROUND_UP(NSEC_PER_SEC, clk_rate);
-	u32 coded_cycles = 0;
-	u32 cycles;
-
-	cycles = DIV_ROUND_UP(timing_ns, clk_period);
-	if (cycles / 32) {
-		coded_cycles |= 1 << 5;
-		if (cycles < 128)
-			cycles = 0;
-	}
-
-	coded_cycles |= cycles % 32;
-
-	return coded_cycles;
-}
-
-/*
- * This function converts a pulse timing expressed in nanoseconds into an
- * encoded value that can be written in the SMC_PULSE register.
- *
- * The following formula is described in atmel datasheets (section
- * "SMC Pulse Register"):
- *
- * pulse length = (256* PULSE[6] + PULSE[5:0])
- *
- * where pulse length is the timing expressed in cycles.
- */
-static inline u32 at91sam9_smc_pulse_ns_to_cycles(unsigned int clk_rate,
-						  u32 timing_ns)
-{
-	u32 clk_period = DIV_ROUND_UP(NSEC_PER_SEC, clk_rate);
-	u32 coded_cycles = 0;
-	u32 cycles;
-
-	cycles = DIV_ROUND_UP(timing_ns, clk_period);
-	if (cycles / 64) {
-		coded_cycles |= 1 << 6;
-		if (cycles < 256)
-			cycles = 0;
-	}
-
-	coded_cycles |= cycles % 64;
-
-	return coded_cycles;
-}
-
-/*
- * This function converts a cycle timing expressed in nanoseconds into an
- * encoded value that can be written in the SMC_CYCLE register.
- *
- * The following formula is described in atmel datasheets (section
- * "SMC Cycle Register"):
- *
- * cycle length = (CYCLE[8:7]*256 + CYCLE[6:0])
- *
- * where cycle length is the timing expressed in cycles.
- */
-static inline u32 at91sam9_smc_cycle_ns_to_cycles(unsigned int clk_rate,
-						  u32 timing_ns)
-{
-	u32 clk_period = DIV_ROUND_UP(NSEC_PER_SEC, clk_rate);
-	u32 coded_cycles = 0;
-	u32 cycles;
-
-	cycles = DIV_ROUND_UP(timing_ns, clk_period);
-	if (cycles / 128) {
-		coded_cycles = cycles / 256;
-		cycles %= 256;
-		if (cycles >= 128) {
-			coded_cycles++;
-			cycles = 0;
-		}
-
-		if (coded_cycles > 0x3) {
-			coded_cycles = 0x3;
-			cycles = 0x7f;
-		}
-
-		coded_cycles <<= 7;
-	}
-
-	coded_cycles |= cycles % 128;
-
-	return coded_cycles;
-}
-
 #endif /* _LINUX_MFD_SYSCON_ATMEL_SMC_H_ */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH 5/7] memory: atmel-ebi: Change naming scheme
  2017-02-20 16:54 ` Boris Brezillon
@ 2017-02-20 16:54   ` Boris Brezillon
  -1 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:54 UTC (permalink / raw)
  To: Nicolas Ferre, Alexandre Belloni
  Cc: linux-arm-kernel, Samuel Ortiz, Lee Jones, linux-kernel, Boris Brezillon

The EBI block is not only available on at91 SoCs, but also on avr32 ones.
Change the structure and function prefixes from at91_ebi to atmel_ebi to
match this fact and make the prefix and driver name consistent.

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

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index 6b3c9fc7a602..833af8d02424 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -18,34 +18,34 @@
 #include <linux/of_device.h>
 #include <linux/regmap.h>
 
-struct at91_ebi_dev_config {
+struct atmel_ebi_dev_config {
 	int cs;
 	struct atmel_smc_cs_conf smcconf;
 };
 
-struct at91_ebi;
+struct atmel_ebi;
 
-struct at91_ebi_dev {
+struct atmel_ebi_dev {
 	struct list_head node;
-	struct at91_ebi *ebi;
+	struct atmel_ebi *ebi;
 	u32 mode;
 	int numcs;
-	struct at91_ebi_dev_config configs[];
+	struct atmel_ebi_dev_config configs[];
 };
 
-struct at91_ebi_caps {
+struct atmel_ebi_caps {
 	unsigned int available_cs;
 	unsigned int ebi_csa_offs;
-	void (*get_config)(struct at91_ebi_dev *ebid,
-			   struct at91_ebi_dev_config *conf);
-	int (*xlate_config)(struct at91_ebi_dev *ebid,
+	void (*get_config)(struct atmel_ebi_dev *ebid,
+			   struct atmel_ebi_dev_config *conf);
+	int (*xlate_config)(struct atmel_ebi_dev *ebid,
 			    struct device_node *configs_np,
-			    struct at91_ebi_dev_config *conf);
-	void (*apply_config)(struct at91_ebi_dev *ebid,
-			     struct at91_ebi_dev_config *conf);
+			    struct atmel_ebi_dev_config *conf);
+	void (*apply_config)(struct atmel_ebi_dev *ebid,
+			     struct atmel_ebi_dev_config *conf);
 };
 
-struct at91_ebi {
+struct atmel_ebi {
 	struct clk *clk;
 	struct regmap *matrix;
 	struct  {
@@ -54,7 +54,7 @@ struct at91_ebi {
 	} smc;
 
 	struct device *dev;
-	const struct at91_ebi_caps *caps;
+	const struct atmel_ebi_caps *caps;
 	struct list_head devs;
 };
 
@@ -74,15 +74,15 @@ struct atmel_smc_timing_xlate {
 #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)
+static void at91sam9_ebi_get_config(struct atmel_ebi_dev *ebid,
+				    struct atmel_ebi_dev_config *conf)
 {
 	atmel_smc_cs_conf_get(ebid->ebi->smc.regmap, conf->cs,
 			      &conf->smcconf);
 }
 
-static void sama5_ebi_get_config(struct at91_ebi_dev *ebid,
-				 struct at91_ebi_dev_config *conf)
+static void sama5_ebi_get_config(struct atmel_ebi_dev *ebid,
+				 struct atmel_ebi_dev_config *conf)
 {
 	atmel_hsmc_cs_conf_get(ebid->ebi->smc.regmap, conf->cs,
 			       &conf->smcconf);
@@ -105,9 +105,9 @@ static const struct atmel_smc_timing_xlate timings_xlate_table[] = {
 	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 atmel_smc_cs_conf *smcconf)
+static int atmel_ebi_xslate_smc_timings(struct atmel_ebi_dev *ebid,
+					struct device_node *np,
+					struct atmel_smc_cs_conf *smcconf)
 {
 	unsigned int clk_rate = clk_get_rate(ebid->ebi->clk);
 	unsigned int clk_period_ns = NSEC_PER_SEC / clk_rate;
@@ -163,9 +163,9 @@ static int at91_ebi_xslate_smc_timings(struct at91_ebi_dev *ebid,
 	return required;
 }
 
-static int at91_ebi_xslate_smc_config(struct at91_ebi_dev *ebid,
-				      struct device_node *np,
-				      struct at91_ebi_dev_config *conf)
+static int atmel_ebi_xslate_smc_config(struct atmel_ebi_dev *ebid,
+				       struct device_node *np,
+				       struct atmel_ebi_dev_config *conf)
 {
 	struct atmel_smc_cs_conf *smcconf = &conf->smcconf;
 	bool required = false;
@@ -261,7 +261,7 @@ static int at91_ebi_xslate_smc_config(struct at91_ebi_dev *ebid,
 		required = true;
 	}
 
-	ret = at91_ebi_xslate_smc_timings(ebid, np, &conf->smcconf);
+	ret = atmel_ebi_xslate_smc_timings(ebid, np, &conf->smcconf);
 	if (ret)
 		return -EINVAL;
 
@@ -274,27 +274,27 @@ static int at91_ebi_xslate_smc_config(struct at91_ebi_dev *ebid,
 	return required;
 }
 
-static void at91sam9_ebi_apply_config(struct at91_ebi_dev *ebid,
-				      struct at91_ebi_dev_config *conf)
+static void at91sam9_ebi_apply_config(struct atmel_ebi_dev *ebid,
+				      struct atmel_ebi_dev_config *conf)
 {
 	atmel_smc_cs_conf_apply(ebid->ebi->smc.regmap, conf->cs,
 				&conf->smcconf);
 }
 
-static void sama5_ebi_apply_config(struct at91_ebi_dev *ebid,
-				   struct at91_ebi_dev_config *conf)
+static void sama5_ebi_apply_config(struct atmel_ebi_dev *ebid,
+				   struct atmel_ebi_dev_config *conf)
 {
 	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,
-			      int reg_cells)
+static int atmel_ebi_dev_setup(struct atmel_ebi *ebi, struct device_node *np,
+			       int reg_cells)
 {
-	const struct at91_ebi_caps *caps = ebi->caps;
-	struct at91_ebi_dev_config conf = { };
+	const struct atmel_ebi_caps *caps = ebi->caps;
+	struct atmel_ebi_dev_config conf = { };
 	struct device *dev = ebi->dev;
-	struct at91_ebi_dev *ebid;
+	struct atmel_ebi_dev *ebid;
 	unsigned long cslines = 0;
 	int ret, numcs = 0, nentries, i;
 	bool apply = false;
@@ -366,70 +366,70 @@ static int at91_ebi_dev_setup(struct at91_ebi *ebi, struct device_node *np,
 	return 0;
 }
 
-static const struct at91_ebi_caps at91sam9260_ebi_caps = {
+static const struct atmel_ebi_caps at91sam9260_ebi_caps = {
 	.available_cs = 0xff,
 	.ebi_csa_offs = AT91SAM9260_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-static const struct at91_ebi_caps at91sam9261_ebi_caps = {
+static const struct atmel_ebi_caps at91sam9261_ebi_caps = {
 	.available_cs = 0xff,
 	.ebi_csa_offs = AT91SAM9261_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-static const struct at91_ebi_caps at91sam9263_ebi0_caps = {
+static const struct atmel_ebi_caps at91sam9263_ebi0_caps = {
 	.available_cs = 0x3f,
 	.ebi_csa_offs = AT91SAM9263_MATRIX_EBI0CSA,
 	.get_config = at91sam9_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-static const struct at91_ebi_caps at91sam9263_ebi1_caps = {
+static const struct atmel_ebi_caps at91sam9263_ebi1_caps = {
 	.available_cs = 0x7,
 	.ebi_csa_offs = AT91SAM9263_MATRIX_EBI1CSA,
 	.get_config = at91sam9_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-static const struct at91_ebi_caps at91sam9rl_ebi_caps = {
+static const struct atmel_ebi_caps at91sam9rl_ebi_caps = {
 	.available_cs = 0x3f,
 	.ebi_csa_offs = AT91SAM9RL_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-static const struct at91_ebi_caps at91sam9g45_ebi_caps = {
+static const struct atmel_ebi_caps at91sam9g45_ebi_caps = {
 	.available_cs = 0x3f,
 	.ebi_csa_offs = AT91SAM9G45_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-static const struct at91_ebi_caps at91sam9x5_ebi_caps = {
+static const struct atmel_ebi_caps at91sam9x5_ebi_caps = {
 	.available_cs = 0x3f,
 	.ebi_csa_offs = AT91SAM9X5_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-static const struct at91_ebi_caps sama5d3_ebi_caps = {
+static const struct atmel_ebi_caps sama5d3_ebi_caps = {
 	.available_cs = 0xf,
 	.get_config = sama5_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = sama5_ebi_apply_config,
 };
 
-static const struct of_device_id at91_ebi_id_table[] = {
+static const struct of_device_id atmel_ebi_id_table[] = {
 	{
 		.compatible = "atmel,at91sam9260-ebi",
 		.data = &at91sam9260_ebi_caps,
@@ -465,7 +465,7 @@ static const struct of_device_id at91_ebi_id_table[] = {
 	{ /* sentinel */ }
 };
 
-static int at91_ebi_dev_disable(struct at91_ebi *ebi, struct device_node *np)
+static int atmel_ebi_dev_disable(struct atmel_ebi *ebi, struct device_node *np)
 {
 	struct device *dev = ebi->dev;
 	struct property *newprop;
@@ -487,17 +487,17 @@ static int at91_ebi_dev_disable(struct at91_ebi *ebi, struct device_node *np)
 	return of_update_property(np, newprop);
 }
 
-static int at91_ebi_probe(struct platform_device *pdev)
+static int atmel_ebi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *child, *np = dev->of_node, *smc_np;
 	const struct of_device_id *match;
-	struct at91_ebi *ebi;
+	struct atmel_ebi *ebi;
 	int ret, reg_cells;
 	struct clk *clk;
 	u32 val;
 
-	match = of_match_device(at91_ebi_id_table, dev);
+	match = of_match_device(atmel_ebi_id_table, dev);
 	if (!match || !match->data)
 		return -EINVAL;
 
@@ -563,12 +563,12 @@ static int at91_ebi_probe(struct platform_device *pdev)
 		if (!of_find_property(child, "reg", NULL))
 			continue;
 
-		ret = at91_ebi_dev_setup(ebi, child, reg_cells);
+		ret = atmel_ebi_dev_setup(ebi, child, reg_cells);
 		if (ret) {
 			dev_err(dev, "failed to configure EBI bus for %s, disabling the device",
 				child->full_name);
 
-			ret = at91_ebi_dev_disable(ebi, child);
+			ret = atmel_ebi_dev_disable(ebi, child);
 			if (ret)
 				return ret;
 		}
@@ -577,10 +577,10 @@ static int at91_ebi_probe(struct platform_device *pdev)
 	return of_platform_populate(np, NULL, NULL, dev);
 }
 
-static struct platform_driver at91_ebi_driver = {
+static struct platform_driver atmel_ebi_driver = {
 	.driver = {
 		.name = "atmel-ebi",
-		.of_match_table	= at91_ebi_id_table,
+		.of_match_table	= atmel_ebi_id_table,
 	},
 };
-builtin_platform_driver_probe(at91_ebi_driver, at91_ebi_probe);
+builtin_platform_driver_probe(atmel_ebi_driver, atmel_ebi_probe);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH 5/7] memory: atmel-ebi: Change naming scheme
@ 2017-02-20 16:54   ` Boris Brezillon
  0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:54 UTC (permalink / raw)
  To: linux-arm-kernel

The EBI block is not only available on at91 SoCs, but also on avr32 ones.
Change the structure and function prefixes from at91_ebi to atmel_ebi to
match this fact and make the prefix and driver name consistent.

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

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index 6b3c9fc7a602..833af8d02424 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -18,34 +18,34 @@
 #include <linux/of_device.h>
 #include <linux/regmap.h>
 
-struct at91_ebi_dev_config {
+struct atmel_ebi_dev_config {
 	int cs;
 	struct atmel_smc_cs_conf smcconf;
 };
 
-struct at91_ebi;
+struct atmel_ebi;
 
-struct at91_ebi_dev {
+struct atmel_ebi_dev {
 	struct list_head node;
-	struct at91_ebi *ebi;
+	struct atmel_ebi *ebi;
 	u32 mode;
 	int numcs;
-	struct at91_ebi_dev_config configs[];
+	struct atmel_ebi_dev_config configs[];
 };
 
-struct at91_ebi_caps {
+struct atmel_ebi_caps {
 	unsigned int available_cs;
 	unsigned int ebi_csa_offs;
-	void (*get_config)(struct at91_ebi_dev *ebid,
-			   struct at91_ebi_dev_config *conf);
-	int (*xlate_config)(struct at91_ebi_dev *ebid,
+	void (*get_config)(struct atmel_ebi_dev *ebid,
+			   struct atmel_ebi_dev_config *conf);
+	int (*xlate_config)(struct atmel_ebi_dev *ebid,
 			    struct device_node *configs_np,
-			    struct at91_ebi_dev_config *conf);
-	void (*apply_config)(struct at91_ebi_dev *ebid,
-			     struct at91_ebi_dev_config *conf);
+			    struct atmel_ebi_dev_config *conf);
+	void (*apply_config)(struct atmel_ebi_dev *ebid,
+			     struct atmel_ebi_dev_config *conf);
 };
 
-struct at91_ebi {
+struct atmel_ebi {
 	struct clk *clk;
 	struct regmap *matrix;
 	struct  {
@@ -54,7 +54,7 @@ struct at91_ebi {
 	} smc;
 
 	struct device *dev;
-	const struct at91_ebi_caps *caps;
+	const struct atmel_ebi_caps *caps;
 	struct list_head devs;
 };
 
@@ -74,15 +74,15 @@ struct atmel_smc_timing_xlate {
 #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)
+static void at91sam9_ebi_get_config(struct atmel_ebi_dev *ebid,
+				    struct atmel_ebi_dev_config *conf)
 {
 	atmel_smc_cs_conf_get(ebid->ebi->smc.regmap, conf->cs,
 			      &conf->smcconf);
 }
 
-static void sama5_ebi_get_config(struct at91_ebi_dev *ebid,
-				 struct at91_ebi_dev_config *conf)
+static void sama5_ebi_get_config(struct atmel_ebi_dev *ebid,
+				 struct atmel_ebi_dev_config *conf)
 {
 	atmel_hsmc_cs_conf_get(ebid->ebi->smc.regmap, conf->cs,
 			       &conf->smcconf);
@@ -105,9 +105,9 @@ static const struct atmel_smc_timing_xlate timings_xlate_table[] = {
 	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 atmel_smc_cs_conf *smcconf)
+static int atmel_ebi_xslate_smc_timings(struct atmel_ebi_dev *ebid,
+					struct device_node *np,
+					struct atmel_smc_cs_conf *smcconf)
 {
 	unsigned int clk_rate = clk_get_rate(ebid->ebi->clk);
 	unsigned int clk_period_ns = NSEC_PER_SEC / clk_rate;
@@ -163,9 +163,9 @@ static int at91_ebi_xslate_smc_timings(struct at91_ebi_dev *ebid,
 	return required;
 }
 
-static int at91_ebi_xslate_smc_config(struct at91_ebi_dev *ebid,
-				      struct device_node *np,
-				      struct at91_ebi_dev_config *conf)
+static int atmel_ebi_xslate_smc_config(struct atmel_ebi_dev *ebid,
+				       struct device_node *np,
+				       struct atmel_ebi_dev_config *conf)
 {
 	struct atmel_smc_cs_conf *smcconf = &conf->smcconf;
 	bool required = false;
@@ -261,7 +261,7 @@ static int at91_ebi_xslate_smc_config(struct at91_ebi_dev *ebid,
 		required = true;
 	}
 
-	ret = at91_ebi_xslate_smc_timings(ebid, np, &conf->smcconf);
+	ret = atmel_ebi_xslate_smc_timings(ebid, np, &conf->smcconf);
 	if (ret)
 		return -EINVAL;
 
@@ -274,27 +274,27 @@ static int at91_ebi_xslate_smc_config(struct at91_ebi_dev *ebid,
 	return required;
 }
 
-static void at91sam9_ebi_apply_config(struct at91_ebi_dev *ebid,
-				      struct at91_ebi_dev_config *conf)
+static void at91sam9_ebi_apply_config(struct atmel_ebi_dev *ebid,
+				      struct atmel_ebi_dev_config *conf)
 {
 	atmel_smc_cs_conf_apply(ebid->ebi->smc.regmap, conf->cs,
 				&conf->smcconf);
 }
 
-static void sama5_ebi_apply_config(struct at91_ebi_dev *ebid,
-				   struct at91_ebi_dev_config *conf)
+static void sama5_ebi_apply_config(struct atmel_ebi_dev *ebid,
+				   struct atmel_ebi_dev_config *conf)
 {
 	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,
-			      int reg_cells)
+static int atmel_ebi_dev_setup(struct atmel_ebi *ebi, struct device_node *np,
+			       int reg_cells)
 {
-	const struct at91_ebi_caps *caps = ebi->caps;
-	struct at91_ebi_dev_config conf = { };
+	const struct atmel_ebi_caps *caps = ebi->caps;
+	struct atmel_ebi_dev_config conf = { };
 	struct device *dev = ebi->dev;
-	struct at91_ebi_dev *ebid;
+	struct atmel_ebi_dev *ebid;
 	unsigned long cslines = 0;
 	int ret, numcs = 0, nentries, i;
 	bool apply = false;
@@ -366,70 +366,70 @@ static int at91_ebi_dev_setup(struct at91_ebi *ebi, struct device_node *np,
 	return 0;
 }
 
-static const struct at91_ebi_caps at91sam9260_ebi_caps = {
+static const struct atmel_ebi_caps at91sam9260_ebi_caps = {
 	.available_cs = 0xff,
 	.ebi_csa_offs = AT91SAM9260_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-static const struct at91_ebi_caps at91sam9261_ebi_caps = {
+static const struct atmel_ebi_caps at91sam9261_ebi_caps = {
 	.available_cs = 0xff,
 	.ebi_csa_offs = AT91SAM9261_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-static const struct at91_ebi_caps at91sam9263_ebi0_caps = {
+static const struct atmel_ebi_caps at91sam9263_ebi0_caps = {
 	.available_cs = 0x3f,
 	.ebi_csa_offs = AT91SAM9263_MATRIX_EBI0CSA,
 	.get_config = at91sam9_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-static const struct at91_ebi_caps at91sam9263_ebi1_caps = {
+static const struct atmel_ebi_caps at91sam9263_ebi1_caps = {
 	.available_cs = 0x7,
 	.ebi_csa_offs = AT91SAM9263_MATRIX_EBI1CSA,
 	.get_config = at91sam9_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-static const struct at91_ebi_caps at91sam9rl_ebi_caps = {
+static const struct atmel_ebi_caps at91sam9rl_ebi_caps = {
 	.available_cs = 0x3f,
 	.ebi_csa_offs = AT91SAM9RL_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-static const struct at91_ebi_caps at91sam9g45_ebi_caps = {
+static const struct atmel_ebi_caps at91sam9g45_ebi_caps = {
 	.available_cs = 0x3f,
 	.ebi_csa_offs = AT91SAM9G45_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-static const struct at91_ebi_caps at91sam9x5_ebi_caps = {
+static const struct atmel_ebi_caps at91sam9x5_ebi_caps = {
 	.available_cs = 0x3f,
 	.ebi_csa_offs = AT91SAM9X5_MATRIX_EBICSA,
 	.get_config = at91sam9_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = at91sam9_ebi_apply_config,
 };
 
-static const struct at91_ebi_caps sama5d3_ebi_caps = {
+static const struct atmel_ebi_caps sama5d3_ebi_caps = {
 	.available_cs = 0xf,
 	.get_config = sama5_ebi_get_config,
-	.xlate_config = at91_ebi_xslate_smc_config,
+	.xlate_config = atmel_ebi_xslate_smc_config,
 	.apply_config = sama5_ebi_apply_config,
 };
 
-static const struct of_device_id at91_ebi_id_table[] = {
+static const struct of_device_id atmel_ebi_id_table[] = {
 	{
 		.compatible = "atmel,at91sam9260-ebi",
 		.data = &at91sam9260_ebi_caps,
@@ -465,7 +465,7 @@ static const struct of_device_id at91_ebi_id_table[] = {
 	{ /* sentinel */ }
 };
 
-static int at91_ebi_dev_disable(struct at91_ebi *ebi, struct device_node *np)
+static int atmel_ebi_dev_disable(struct atmel_ebi *ebi, struct device_node *np)
 {
 	struct device *dev = ebi->dev;
 	struct property *newprop;
@@ -487,17 +487,17 @@ static int at91_ebi_dev_disable(struct at91_ebi *ebi, struct device_node *np)
 	return of_update_property(np, newprop);
 }
 
-static int at91_ebi_probe(struct platform_device *pdev)
+static int atmel_ebi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *child, *np = dev->of_node, *smc_np;
 	const struct of_device_id *match;
-	struct at91_ebi *ebi;
+	struct atmel_ebi *ebi;
 	int ret, reg_cells;
 	struct clk *clk;
 	u32 val;
 
-	match = of_match_device(at91_ebi_id_table, dev);
+	match = of_match_device(atmel_ebi_id_table, dev);
 	if (!match || !match->data)
 		return -EINVAL;
 
@@ -563,12 +563,12 @@ static int at91_ebi_probe(struct platform_device *pdev)
 		if (!of_find_property(child, "reg", NULL))
 			continue;
 
-		ret = at91_ebi_dev_setup(ebi, child, reg_cells);
+		ret = atmel_ebi_dev_setup(ebi, child, reg_cells);
 		if (ret) {
 			dev_err(dev, "failed to configure EBI bus for %s, disabling the device",
 				child->full_name);
 
-			ret = at91_ebi_dev_disable(ebi, child);
+			ret = atmel_ebi_dev_disable(ebi, child);
 			if (ret)
 				return ret;
 		}
@@ -577,10 +577,10 @@ static int at91_ebi_probe(struct platform_device *pdev)
 	return of_platform_populate(np, NULL, NULL, dev);
 }
 
-static struct platform_driver at91_ebi_driver = {
+static struct platform_driver atmel_ebi_driver = {
 	.driver = {
 		.name = "atmel-ebi",
-		.of_match_table	= at91_ebi_id_table,
+		.of_match_table	= atmel_ebi_id_table,
 	},
 };
-builtin_platform_driver_probe(at91_ebi_driver, at91_ebi_probe);
+builtin_platform_driver_probe(atmel_ebi_driver, atmel_ebi_probe);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH 6/7] memory: atmel-ebi: Add missing ->numcs assignment
  2017-02-20 16:54 ` Boris Brezillon
@ 2017-02-20 16:55   ` Boris Brezillon
  -1 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:55 UTC (permalink / raw)
  To: Nicolas Ferre, Alexandre Belloni
  Cc: linux-arm-kernel, Samuel Ortiz, Lee Jones, linux-kernel, Boris Brezillon

ebid->numcs is never assigned, set it to numcs after allocating the
EBI dev object.

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

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index 833af8d02424..06a1a136d448 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -331,6 +331,7 @@ static int atmel_ebi_dev_setup(struct atmel_ebi *ebi, struct device_node *np,
 		return -ENOMEM;
 
 	ebid->ebi = ebi;
+	ebid->numcs = numcs;
 
 	ret = caps->xlate_config(ebid, np, &conf);
 	if (ret < 0)
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH 6/7] memory: atmel-ebi: Add missing ->numcs assignment
@ 2017-02-20 16:55   ` Boris Brezillon
  0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:55 UTC (permalink / raw)
  To: linux-arm-kernel

ebid->numcs is never assigned, set it to numcs after allocating the
EBI dev object.

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

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index 833af8d02424..06a1a136d448 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -331,6 +331,7 @@ static int atmel_ebi_dev_setup(struct atmel_ebi *ebi, struct device_node *np,
 		return -ENOMEM;
 
 	ebid->ebi = ebi;
+	ebid->numcs = numcs;
 
 	ret = caps->xlate_config(ebid, np, &conf);
 	if (ret < 0)
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH 7/7] memory: atmel-ebi: Add PM ops
  2017-02-20 16:54 ` Boris Brezillon
@ 2017-02-20 16:55   ` Boris Brezillon
  -1 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:55 UTC (permalink / raw)
  To: Nicolas Ferre, Alexandre Belloni
  Cc: linux-arm-kernel, Samuel Ortiz, Lee Jones, linux-kernel, Boris Brezillon

Add a ->resume() hook to make sure the EBI dev configs are correctly
restored when resuming the platform.

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

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index 06a1a136d448..15120cb37cf0 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -506,6 +506,8 @@ static int atmel_ebi_probe(struct platform_device *pdev)
 	if (!ebi)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, ebi);
+
 	INIT_LIST_HEAD(&ebi->devs);
 	ebi->caps = match->data;
 	ebi->dev = dev;
@@ -578,10 +580,28 @@ static int atmel_ebi_probe(struct platform_device *pdev)
 	return of_platform_populate(np, NULL, NULL, dev);
 }
 
+static int atmel_ebi_resume(struct device *dev)
+{
+	struct atmel_ebi *ebi = dev_get_drvdata(dev);
+	struct atmel_ebi_dev *ebid;
+
+	list_for_each_entry(ebid, &ebi->devs, node) {
+		int i;
+
+		for (i = 0; i < ebid->numcs; i++)
+			ebid->ebi->caps->apply_config(ebid, &ebid->configs[i]);
+	}
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(atmel_ebi_pm_ops, NULL, atmel_ebi_resume);
+
 static struct platform_driver atmel_ebi_driver = {
 	.driver = {
 		.name = "atmel-ebi",
 		.of_match_table	= atmel_ebi_id_table,
+		.pm = &atmel_ebi_pm_ops,
 	},
 };
 builtin_platform_driver_probe(atmel_ebi_driver, atmel_ebi_probe);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH 7/7] memory: atmel-ebi: Add PM ops
@ 2017-02-20 16:55   ` Boris Brezillon
  0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-02-20 16:55 UTC (permalink / raw)
  To: linux-arm-kernel

Add a ->resume() hook to make sure the EBI dev configs are correctly
restored when resuming the platform.

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

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index 06a1a136d448..15120cb37cf0 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -506,6 +506,8 @@ static int atmel_ebi_probe(struct platform_device *pdev)
 	if (!ebi)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, ebi);
+
 	INIT_LIST_HEAD(&ebi->devs);
 	ebi->caps = match->data;
 	ebi->dev = dev;
@@ -578,10 +580,28 @@ static int atmel_ebi_probe(struct platform_device *pdev)
 	return of_platform_populate(np, NULL, NULL, dev);
 }
 
+static int atmel_ebi_resume(struct device *dev)
+{
+	struct atmel_ebi *ebi = dev_get_drvdata(dev);
+	struct atmel_ebi_dev *ebid;
+
+	list_for_each_entry(ebid, &ebi->devs, node) {
+		int i;
+
+		for (i = 0; i < ebid->numcs; i++)
+			ebid->ebi->caps->apply_config(ebid, &ebid->configs[i]);
+	}
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(atmel_ebi_pm_ops, NULL, atmel_ebi_resume);
+
 static struct platform_driver atmel_ebi_driver = {
 	.driver = {
 		.name = "atmel-ebi",
 		.of_match_table	= atmel_ebi_id_table,
+		.pm = &atmel_ebi_pm_ops,
 	},
 };
 builtin_platform_driver_probe(atmel_ebi_driver, atmel_ebi_probe);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* Re: [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code
  2017-02-20 16:54   ` Boris Brezillon
@ 2017-03-02 12:02     ` Alexander Dahl
  -1 siblings, 0 replies; 34+ messages in thread
From: Alexander Dahl @ 2017-03-02 12:02 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Boris Brezillon, Nicolas Ferre, Alexandre Belloni, Lee Jones,
	Samuel Ortiz, linux-kernel

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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code
@ 2017-03-02 12:02     ` Alexander Dahl
  0 siblings, 0 replies; 34+ messages in thread
From: Alexander Dahl @ 2017-03-02 12:02 UTC (permalink / raw)
  To: linux-arm-kernel

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@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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code
  2017-03-02 12:02     ` Alexander Dahl
@ 2017-03-02 12:30       ` Boris Brezillon
  -1 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-03-02 12:30 UTC (permalink / raw)
  To: Alexander Dahl
  Cc: linux-arm-kernel, Nicolas Ferre, Alexandre Belloni, Lee Jones,
	Samuel Ortiz, linux-kernel

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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code
@ 2017-03-02 12:30       ` Boris Brezillon
  0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-03-02 12:30 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH 1/7] mfd: syscon: atmel-smc: Add new helpers to ease SMC regs manipulation
  2017-02-20 16:54   ` Boris Brezillon
@ 2017-03-14 17:00     ` Lee Jones
  -1 siblings, 0 replies; 34+ messages in thread
From: Lee Jones @ 2017-03-14 17:00 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Nicolas Ferre, Alexandre Belloni, linux-arm-kernel, Samuel Ortiz,
	linux-kernel

On Mon, 20 Feb 2017, Boris Brezillon wrote:

> These new helpers + macro definitions are meant to replace the old ones
> which are unpractical to use.
> 
> Note that the macros and function prefixes have been intentionally
> changed to ATMEL_[H]SMC_XX and atmel_[h]smc_ to reflect the fact that
> this IP is also embedded in avr32 SoCs (and not only in at91 ones).

I'm going to NACK this patch.

I don't see any reason why all those functions have to be inline and
shoved into a header file.

> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  include/linux/mfd/syscon/atmel-smc.h | 362 +++++++++++++++++++++++++++++++++++
>  1 file changed, 362 insertions(+)
> 
> diff --git a/include/linux/mfd/syscon/atmel-smc.h b/include/linux/mfd/syscon/atmel-smc.h
> index be6ebe64eebe..6124f3d2b965 100644
> --- a/include/linux/mfd/syscon/atmel-smc.h
> +++ b/include/linux/mfd/syscon/atmel-smc.h
> @@ -69,6 +69,368 @@
>  #define AT91_SMC_PS_16			(2 << 28)
>  #define AT91_SMC_PS_32			(3 << 28)
>  
> +#define ATMEL_SMC_SETUP(cs)			(((cs) * 0x10))
> +#define ATMEL_HSMC_SETUP(cs)			(0x600 + ((cs) * 0x14))
> +#define ATMEL_SMC_PULSE(cs)			(((cs) * 0x10) + 0x4)
> +#define ATMEL_HSMC_PULSE(cs)			(0x600 + ((cs) * 0x14) + 0x4)
> +#define ATMEL_SMC_CYCLE(cs)			(((cs) * 0x10) + 0x8)
> +#define ATMEL_HSMC_CYCLE(cs)			(0x600 + ((cs) * 0x14) + 0x8)
> +#define ATMEL_SMC_NWE_SHIFT			0
> +#define ATMEL_SMC_NCS_WR_SHIFT			8
> +#define ATMEL_SMC_NRD_SHIFT			16
> +#define ATMEL_SMC_NCS_RD_SHIFT			24
> +
> +#define ATMEL_SMC_MODE(cs)			(((cs) * 0x10) + 0xc)
> +#define ATMEL_HSMC_MODE(cs)			(0x600 + ((cs) * 0x14) + 0x10)
> +#define ATMEL_SMC_MODE_READMODE_MASK		BIT(0)
> +#define ATMEL_SMC_MODE_READMODE_NCS		(0 << 0)
> +#define ATMEL_SMC_MODE_READMODE_NRD		(1 << 0)
> +#define ATMEL_SMC_MODE_WRITEMODE_MASK		BIT(1)
> +#define ATMEL_SMC_MODE_WRITEMODE_NCS		(0 << 1)
> +#define ATMEL_SMC_MODE_WRITEMODE_NWE		(1 << 1)
> +#define ATMEL_SMC_MODE_EXNWMODE_MASK		GENMASK(5, 4)
> +#define ATMEL_SMC_MODE_EXNWMODE_DISABLE		(0 << 4)
> +#define ATMEL_SMC_MODE_EXNWMODE_FROZEN		(2 << 4)
> +#define ATMEL_SMC_MODE_EXNWMODE_READY		(3 << 4)
> +#define ATMEL_SMC_MODE_BAT_MASK			BIT(8)
> +#define ATMEL_SMC_MODE_BAT_SELECT		(0 << 8)
> +#define ATMEL_SMC_MODE_BAT_WRITE		(1 << 8)
> +#define ATMEL_SMC_MODE_DBW_MASK			GENMASK(13, 12)
> +#define ATMEL_SMC_MODE_DBW_8			(0 << 12)
> +#define ATMEL_SMC_MODE_DBW_16			(1 << 12)
> +#define ATMEL_SMC_MODE_DBW_32			(2 << 12)
> +#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_TDFMODE_OPTIMIZED	BIT(20)
> +#define ATMEL_SMC_MODE_PMEN			BIT(24)
> +#define ATMEL_SMC_MODE_PS_MASK			GENMASK(29, 28)
> +#define ATMEL_SMC_MODE_PS_4			(0 << 28)
> +#define ATMEL_SMC_MODE_PS_8			(1 << 28)
> +#define ATMEL_SMC_MODE_PS_16			(2 << 28)
> +#define ATMEL_SMC_MODE_PS_32			(3 << 28)
> +
> +#define ATMEL_HSMC_TIMINGS(cs)			(0x600 + ((cs) * 0x14) + 0xc)
> +#define ATMEL_HSMC_TIMINGS_OCMS			BIT(12)
> +#define ATMEL_HSMC_TIMINGS_RBNSEL(x)		((x) << 28)
> +#define ATMEL_HSMC_TIMINGS_NFSEL		BIT(31)
> +#define ATMEL_HSMC_TIMINGS_TCLR_SHIFT		0
> +#define ATMEL_HSMC_TIMINGS_TADL_SHIFT		4
> +#define ATMEL_HSMC_TIMINGS_TAR_SHIFT		8
> +#define ATMEL_HSMC_TIMINGS_TRR_SHIFT		16
> +#define ATMEL_HSMC_TIMINGS_TWB_SHIFT		24
> +
> +/**
> + * struct atmel_smc_cs_conf - SMC CS config as described in the datasheet.
> + * @setup: NCS/NWE/NRD setup timings (not applicable to at91rm9200)
> + * @pulse: NCS/NWE/NRD pulse timings (not applicable to at91rm9200)
> + * @cycle: NWE/NRD cycle timings (not applicable to at91rm9200)
> + * @timings: advanced NAND related timings (only applicable to HSMC)
> + * @mode: all kind of config parameters (see the fields definition above).
> + *	  The mode fields are different on at91rm9200
> + */
> +struct atmel_smc_cs_conf {
> +	u32 setup;
> +	u32 pulse;
> +	u32 cycle;
> +	u32 timings;
> +	u32 mode;
> +};
> +
> +/**
> + * atmel_smc_cs_conf_init - initialize a SMC CS conf
> + * @conf: the SMC CS conf to initialize
> + *
> + * Set all fields to 0 so that one can start defining a new config.
> + */
> +static inline void atmel_smc_cs_conf_init(struct atmel_smc_cs_conf *conf)
> +{
> +	memset(conf, 0, sizeof(*conf));
> +}
> +
> +/**
> + * atmel_smc_cs_encode_ncycles - encode a number of MCK clk cycles in the
> + *				 format expected by the SMC engine
> + * @ncycles: number of MCK clk cycles
> + * @msbpos: position of the MSB part of the timing field
> + * @msbwidth: width of the MSB part of the timing field
> + * @msbfactor: factor applied to the MSB
> + * @encodedval: param used to store the encoding result
> + *
> + * This function encodes the @ncycles value as described in the datasheet
> + * (section "SMC Setup/Pulse/Cycle/Timings Register"). This is a generic
> + * helper which called with different parameter depending on the encoding
> + * scheme.
> + *
> + * If the @ncycles value is too big to be encoded, -ERANGE is returned and
> + * the encodedval is contains the maximum val. Otherwise, 0 is returned.
> + */
> +static inline int atmel_smc_cs_encode_ncycles(unsigned int ncycles,
> +					      unsigned int msbpos,
> +					      unsigned int msbwidth,
> +					      unsigned int msbfactor,
> +					      unsigned int *encodedval)
> +{
> +	unsigned int lsbmask = GENMASK(msbpos - 1, 0);
> +	unsigned int msbmask = GENMASK(msbwidth - 1, 0);
> +	unsigned int msb, lsb;
> +	int ret = 0;
> +
> +	msb = ncycles / msbfactor;
> +	lsb = ncycles % msbfactor;
> +
> +	if (lsb > lsbmask) {
> +		lsb = 0;
> +		msb++;
> +	}
> +
> +	/*
> +	 * Let's just put the maximum we can if the requested setting does
> +	 * not fit in the register field.
> +	 * We still return -ERANGE in case the caller cares.
> +	 */
> +	if (msb > msbmask) {
> +		msb = msbmask;
> +		lsb = lsbmask;
> +		ret = -ERANGE;
> +	}
> +
> +	*encodedval = (msb << msbpos) | lsb;
> +
> +	return ret;
> +}
> +
> +/**
> + * atmel_smc_cs_conf_set_timing - set the SMC CS conf Txx parameter to a
> + *				  specific value
> + * @conf: SMC CS conf descriptor
> + * @shift: the position of the Txx field in the TIMINGS register
> + * @ncycles: value (expressed in MCK clk cycles) to assign to this Txx
> + *	     parameter
> + *
> + * This function encodes the @ncycles value as described in the datasheet
> + * (section "SMC Timings Register"), and then stores the result in the
> + * @conf->timings field at @shift position.
> + *
> + * Returns -EINVAL if shift is invalid, -ERANGE if ncycles does not fit in
> + * the field, and 0 otherwise.
> + */
> +static inline int atmel_smc_cs_conf_set_timing(struct atmel_smc_cs_conf *conf,
> +					       unsigned int shift,
> +					       unsigned int ncycles)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	if (shift != ATMEL_HSMC_TIMINGS_TCLR_SHIFT &&
> +	    shift != ATMEL_HSMC_TIMINGS_TADL_SHIFT &&
> +	    shift != ATMEL_HSMC_TIMINGS_TAR_SHIFT &&
> +	    shift != ATMEL_HSMC_TIMINGS_TRR_SHIFT &&
> +	    shift != ATMEL_HSMC_TIMINGS_TWB_SHIFT)
> +		return -EINVAL;
> +
> +	/*
> +	 * The formula described in atmel datasheets (section "HSMC Timings
> +	 * Register"):
> +	 *
> +	 * ncycles = (Txx[3] * 64) + Txx[2:0]
> +	 */
> +	ret = atmel_smc_cs_encode_ncycles(ncycles, 3, 1, 64, &val);
> +	conf->timings &= ~GENMASK(shift + 3, shift);
> +	conf->timings |= val << shift;
> +
> +	return ret;
> +}
> +
> +/**
> + * atmel_smc_cs_conf_set_setup - set the SMC CS conf xx_SETUP parameter to a
> + *				 specific value
> + * @conf: SMC CS conf descriptor
> + * @shift: the position of the xx_SETUP field in the SETUP register
> + * @ncycles: value (expressed in MCK clk cycles) to assign to this xx_SETUP
> + *	     parameter
> + *
> + * This function encodes the @ncycles value as described in the datasheet
> + * (section "SMC Setup Register"), and then stores the result in the
> + * @conf->setup field at @shift position.
> + *
> + * Returns -EINVAL if @shift is invalid, -ERANGE if @ncycles does not fit in
> + * the field, and 0 otherwise.
> + */
> +static inline int atmel_smc_cs_conf_set_setup(struct atmel_smc_cs_conf *conf,
> +					      unsigned int shift,
> +					      unsigned int ncycles)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	if (shift != ATMEL_SMC_NWE_SHIFT && shift != ATMEL_SMC_NCS_WR_SHIFT &&
> +	    shift != ATMEL_SMC_NRD_SHIFT && shift != ATMEL_SMC_NCS_RD_SHIFT)
> +		return -EINVAL;
> +
> +	/*
> +	 * The formula described in atmel datasheets (section "SMC Setup
> +	 * Register"):
> +	 *
> +	 * ncycles = (128 * xx_SETUP[5]) + xx_SETUP[4:0]
> +	 */
> +	ret = atmel_smc_cs_encode_ncycles(ncycles, 5, 1, 128, &val);
> +	conf->setup &= ~GENMASK(shift + 7, shift);
> +	conf->setup |= val << shift;
> +
> +	return ret;
> +}
> +
> +/**
> + * atmel_smc_cs_conf_set_pulse - set the SMC CS conf xx_PULSE parameter to a
> + *				 specific value
> + * @conf: SMC CS conf descriptor
> + * @shift: the position of the xx_PULSE field in the PULSE register
> + * @ncycles: value (expressed in MCK clk cycles) to assign to this xx_PULSE
> + *	     parameter
> + *
> + * This function encodes the @ncycles value as described in the datasheet
> + * (section "SMC Pulse Register"), and then stores the result in the
> + * @conf->setup field at @shift position.
> + *
> + * Returns -EINVAL if @shift is invalid, -ERANGE if @ncycles does not fit in
> + * the field, and 0 otherwise.
> + */
> +static inline int atmel_smc_cs_conf_set_pulse(struct atmel_smc_cs_conf *conf,
> +					      unsigned int shift,
> +					      unsigned int ncycles)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	if (shift != ATMEL_SMC_NWE_SHIFT && shift != ATMEL_SMC_NCS_WR_SHIFT &&
> +	    shift != ATMEL_SMC_NRD_SHIFT && shift != ATMEL_SMC_NCS_RD_SHIFT)
> +		return -EINVAL;
> +
> +	/*
> +	 * The formula described in atmel datasheets (section "SMC Pulse
> +	 * Register"):
> +	 *
> +	 * ncycles = (256 * xx_PULSE[6]) + xx_PULSE[5:0]
> +	 */
> +	ret = atmel_smc_cs_encode_ncycles(ncycles, 6, 1, 256, &val);
> +	conf->pulse &= ~GENMASK(shift + 7, shift);
> +	conf->pulse |= val << shift;
> +
> +	return ret;
> +}
> +
> +/**
> + * atmel_smc_cs_conf_set_cycle - set the SMC CS conf xx_CYCLE parameter to a
> + *				 specific value
> + * @conf: SMC CS conf descriptor
> + * @shift: the position of the xx_CYCLE field in the CYCLE register
> + * @ncycles: value (expressed in MCK clk cycles) to assign to this xx_CYCLE
> + *	     parameter
> + *
> + * This function encodes the @ncycles value as described in the datasheet
> + * (section "SMC Pulse Register"), and then stores the result in the
> + * @conf->setup field at @shift position.
> + *
> + * Returns -EINVAL if @shift is invalid, -ERANGE if @ncycles does not fit in
> + * the field, and 0 otherwise.
> + */
> +static inline int atmel_smc_cs_conf_set_cycle(struct atmel_smc_cs_conf *conf,
> +					      unsigned int shift,
> +					      unsigned int ncycles)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	if (shift != ATMEL_SMC_NWE_SHIFT && shift != ATMEL_SMC_NRD_SHIFT)
> +		return -EINVAL;
> +
> +	/*
> +	 * The formula described in atmel datasheets (section "SMC Cycle
> +	 * Register"):
> +	 *
> +	 * ncycles = (xx_CYCLE[8:7] * 256) + xx_CYCLE[6:0]
> +	 */
> +	ret = atmel_smc_cs_encode_ncycles(ncycles, 7, 2, 256, &val);
> +	conf->cycle &= ~GENMASK(shift + 15, shift);
> +	conf->cycle |= val << shift;
> +
> +	return ret;
> +}
> +
> +/**
> + * atmel_smc_cs_conf_apply - apply an SMC CS conf
> + * @regmap: the SMC regmap
> + * @cs: the CS id
> + * @conf the SMC CS conf to apply
> + *
> + * Applies an SMC CS configuration.
> + * Only valid on at91sam9/avr32 SoCs.
> + */
> +static inline void atmel_smc_cs_conf_apply(struct regmap *regmap, int cs,
> +					   const struct atmel_smc_cs_conf *conf)
> +{
> +	regmap_write(regmap, ATMEL_SMC_SETUP(cs), conf->setup);
> +	regmap_write(regmap, ATMEL_SMC_PULSE(cs), conf->pulse);
> +	regmap_write(regmap, ATMEL_SMC_CYCLE(cs), conf->cycle);
> +	regmap_write(regmap, ATMEL_SMC_MODE(cs), conf->mode);
> +}
> +
> +/**
> + * atmel_hsmc_cs_conf_apply - apply an SMC CS conf
> + * @regmap: the HSMC regmap
> + * @cs: the CS id
> + * @conf the SMC CS conf to apply
> + *
> + * Applies an SMC CS configuration.
> + * Only valid on post-sama5 SoCs.
> + */
> +static inline void atmel_hsmc_cs_conf_apply(struct regmap *regmap, int cs,
> +					const struct atmel_smc_cs_conf *conf)
> +{
> +	regmap_write(regmap, ATMEL_HSMC_SETUP(cs), conf->setup);
> +	regmap_write(regmap, ATMEL_HSMC_PULSE(cs), conf->pulse);
> +	regmap_write(regmap, ATMEL_HSMC_CYCLE(cs), conf->cycle);
> +	regmap_write(regmap, ATMEL_HSMC_TIMINGS(cs), conf->timings);
> +	regmap_write(regmap, ATMEL_HSMC_MODE(cs), conf->mode);
> +}
> +
> +/**
> + * atmel_smc_cs_conf_get - retrieve the current SMC CS conf
> + * @regmap: the SMC regmap
> + * @cs: the CS id
> + * @conf: the SMC CS conf object to store the current conf
> + *
> + * Retrieve the SMC CS configuration.
> + * Only valid on at91sam9/avr32 SoCs.
> + */
> +static inline void atmel_smc_cs_conf_get(struct regmap *regmap, int cs,
> +					 struct atmel_smc_cs_conf *conf)
> +{
> +	regmap_read(regmap, ATMEL_SMC_SETUP(cs), &conf->setup);
> +	regmap_read(regmap, ATMEL_SMC_PULSE(cs), &conf->pulse);
> +	regmap_read(regmap, ATMEL_SMC_CYCLE(cs), &conf->cycle);
> +	regmap_read(regmap, ATMEL_SMC_MODE(cs), &conf->mode);
> +}
> +
> +/**
> + * atmel_hsmc_cs_conf_get - retrieve the current SMC CS conf
> + * @regmap: the HSMC regmap
> + * @cs: the CS id
> + * @conf: the SMC CS conf object to store the current conf
> + *
> + * Retrieve the SMC CS configuration.
> + * Only valid on post-sama5 SoCs.
> + */
> +static inline void atmel_hsmc_cs_conf_get(struct regmap *regmap, int cs,
> +					  struct atmel_smc_cs_conf *conf)
> +{
> +	regmap_read(regmap, ATMEL_HSMC_SETUP(cs), &conf->setup);
> +	regmap_read(regmap, ATMEL_HSMC_PULSE(cs), &conf->pulse);
> +	regmap_read(regmap, ATMEL_HSMC_CYCLE(cs), &conf->cycle);
> +	regmap_read(regmap, ATMEL_HSMC_TIMINGS(cs), &conf->timings);
> +	regmap_read(regmap, ATMEL_HSMC_MODE(cs), &conf->mode);
> +}
>  
>  /*
>   * This function converts a setup timing expressed in nanoseconds into an

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH 1/7] mfd: syscon: atmel-smc: Add new helpers to ease SMC regs manipulation
@ 2017-03-14 17:00     ` Lee Jones
  0 siblings, 0 replies; 34+ messages in thread
From: Lee Jones @ 2017-03-14 17:00 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, 20 Feb 2017, Boris Brezillon wrote:

> These new helpers + macro definitions are meant to replace the old ones
> which are unpractical to use.
> 
> Note that the macros and function prefixes have been intentionally
> changed to ATMEL_[H]SMC_XX and atmel_[h]smc_ to reflect the fact that
> this IP is also embedded in avr32 SoCs (and not only in at91 ones).

I'm going to NACK this patch.

I don't see any reason why all those functions have to be inline and
shoved into a header file.

> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  include/linux/mfd/syscon/atmel-smc.h | 362 +++++++++++++++++++++++++++++++++++
>  1 file changed, 362 insertions(+)
> 
> diff --git a/include/linux/mfd/syscon/atmel-smc.h b/include/linux/mfd/syscon/atmel-smc.h
> index be6ebe64eebe..6124f3d2b965 100644
> --- a/include/linux/mfd/syscon/atmel-smc.h
> +++ b/include/linux/mfd/syscon/atmel-smc.h
> @@ -69,6 +69,368 @@
>  #define AT91_SMC_PS_16			(2 << 28)
>  #define AT91_SMC_PS_32			(3 << 28)
>  
> +#define ATMEL_SMC_SETUP(cs)			(((cs) * 0x10))
> +#define ATMEL_HSMC_SETUP(cs)			(0x600 + ((cs) * 0x14))
> +#define ATMEL_SMC_PULSE(cs)			(((cs) * 0x10) + 0x4)
> +#define ATMEL_HSMC_PULSE(cs)			(0x600 + ((cs) * 0x14) + 0x4)
> +#define ATMEL_SMC_CYCLE(cs)			(((cs) * 0x10) + 0x8)
> +#define ATMEL_HSMC_CYCLE(cs)			(0x600 + ((cs) * 0x14) + 0x8)
> +#define ATMEL_SMC_NWE_SHIFT			0
> +#define ATMEL_SMC_NCS_WR_SHIFT			8
> +#define ATMEL_SMC_NRD_SHIFT			16
> +#define ATMEL_SMC_NCS_RD_SHIFT			24
> +
> +#define ATMEL_SMC_MODE(cs)			(((cs) * 0x10) + 0xc)
> +#define ATMEL_HSMC_MODE(cs)			(0x600 + ((cs) * 0x14) + 0x10)
> +#define ATMEL_SMC_MODE_READMODE_MASK		BIT(0)
> +#define ATMEL_SMC_MODE_READMODE_NCS		(0 << 0)
> +#define ATMEL_SMC_MODE_READMODE_NRD		(1 << 0)
> +#define ATMEL_SMC_MODE_WRITEMODE_MASK		BIT(1)
> +#define ATMEL_SMC_MODE_WRITEMODE_NCS		(0 << 1)
> +#define ATMEL_SMC_MODE_WRITEMODE_NWE		(1 << 1)
> +#define ATMEL_SMC_MODE_EXNWMODE_MASK		GENMASK(5, 4)
> +#define ATMEL_SMC_MODE_EXNWMODE_DISABLE		(0 << 4)
> +#define ATMEL_SMC_MODE_EXNWMODE_FROZEN		(2 << 4)
> +#define ATMEL_SMC_MODE_EXNWMODE_READY		(3 << 4)
> +#define ATMEL_SMC_MODE_BAT_MASK			BIT(8)
> +#define ATMEL_SMC_MODE_BAT_SELECT		(0 << 8)
> +#define ATMEL_SMC_MODE_BAT_WRITE		(1 << 8)
> +#define ATMEL_SMC_MODE_DBW_MASK			GENMASK(13, 12)
> +#define ATMEL_SMC_MODE_DBW_8			(0 << 12)
> +#define ATMEL_SMC_MODE_DBW_16			(1 << 12)
> +#define ATMEL_SMC_MODE_DBW_32			(2 << 12)
> +#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_TDFMODE_OPTIMIZED	BIT(20)
> +#define ATMEL_SMC_MODE_PMEN			BIT(24)
> +#define ATMEL_SMC_MODE_PS_MASK			GENMASK(29, 28)
> +#define ATMEL_SMC_MODE_PS_4			(0 << 28)
> +#define ATMEL_SMC_MODE_PS_8			(1 << 28)
> +#define ATMEL_SMC_MODE_PS_16			(2 << 28)
> +#define ATMEL_SMC_MODE_PS_32			(3 << 28)
> +
> +#define ATMEL_HSMC_TIMINGS(cs)			(0x600 + ((cs) * 0x14) + 0xc)
> +#define ATMEL_HSMC_TIMINGS_OCMS			BIT(12)
> +#define ATMEL_HSMC_TIMINGS_RBNSEL(x)		((x) << 28)
> +#define ATMEL_HSMC_TIMINGS_NFSEL		BIT(31)
> +#define ATMEL_HSMC_TIMINGS_TCLR_SHIFT		0
> +#define ATMEL_HSMC_TIMINGS_TADL_SHIFT		4
> +#define ATMEL_HSMC_TIMINGS_TAR_SHIFT		8
> +#define ATMEL_HSMC_TIMINGS_TRR_SHIFT		16
> +#define ATMEL_HSMC_TIMINGS_TWB_SHIFT		24
> +
> +/**
> + * struct atmel_smc_cs_conf - SMC CS config as described in the datasheet.
> + * @setup: NCS/NWE/NRD setup timings (not applicable to at91rm9200)
> + * @pulse: NCS/NWE/NRD pulse timings (not applicable to at91rm9200)
> + * @cycle: NWE/NRD cycle timings (not applicable to at91rm9200)
> + * @timings: advanced NAND related timings (only applicable to HSMC)
> + * @mode: all kind of config parameters (see the fields definition above).
> + *	  The mode fields are different on at91rm9200
> + */
> +struct atmel_smc_cs_conf {
> +	u32 setup;
> +	u32 pulse;
> +	u32 cycle;
> +	u32 timings;
> +	u32 mode;
> +};
> +
> +/**
> + * atmel_smc_cs_conf_init - initialize a SMC CS conf
> + * @conf: the SMC CS conf to initialize
> + *
> + * Set all fields to 0 so that one can start defining a new config.
> + */
> +static inline void atmel_smc_cs_conf_init(struct atmel_smc_cs_conf *conf)
> +{
> +	memset(conf, 0, sizeof(*conf));
> +}
> +
> +/**
> + * atmel_smc_cs_encode_ncycles - encode a number of MCK clk cycles in the
> + *				 format expected by the SMC engine
> + * @ncycles: number of MCK clk cycles
> + * @msbpos: position of the MSB part of the timing field
> + * @msbwidth: width of the MSB part of the timing field
> + * @msbfactor: factor applied to the MSB
> + * @encodedval: param used to store the encoding result
> + *
> + * This function encodes the @ncycles value as described in the datasheet
> + * (section "SMC Setup/Pulse/Cycle/Timings Register"). This is a generic
> + * helper which called with different parameter depending on the encoding
> + * scheme.
> + *
> + * If the @ncycles value is too big to be encoded, -ERANGE is returned and
> + * the encodedval is contains the maximum val. Otherwise, 0 is returned.
> + */
> +static inline int atmel_smc_cs_encode_ncycles(unsigned int ncycles,
> +					      unsigned int msbpos,
> +					      unsigned int msbwidth,
> +					      unsigned int msbfactor,
> +					      unsigned int *encodedval)
> +{
> +	unsigned int lsbmask = GENMASK(msbpos - 1, 0);
> +	unsigned int msbmask = GENMASK(msbwidth - 1, 0);
> +	unsigned int msb, lsb;
> +	int ret = 0;
> +
> +	msb = ncycles / msbfactor;
> +	lsb = ncycles % msbfactor;
> +
> +	if (lsb > lsbmask) {
> +		lsb = 0;
> +		msb++;
> +	}
> +
> +	/*
> +	 * Let's just put the maximum we can if the requested setting does
> +	 * not fit in the register field.
> +	 * We still return -ERANGE in case the caller cares.
> +	 */
> +	if (msb > msbmask) {
> +		msb = msbmask;
> +		lsb = lsbmask;
> +		ret = -ERANGE;
> +	}
> +
> +	*encodedval = (msb << msbpos) | lsb;
> +
> +	return ret;
> +}
> +
> +/**
> + * atmel_smc_cs_conf_set_timing - set the SMC CS conf Txx parameter to a
> + *				  specific value
> + * @conf: SMC CS conf descriptor
> + * @shift: the position of the Txx field in the TIMINGS register
> + * @ncycles: value (expressed in MCK clk cycles) to assign to this Txx
> + *	     parameter
> + *
> + * This function encodes the @ncycles value as described in the datasheet
> + * (section "SMC Timings Register"), and then stores the result in the
> + * @conf->timings field at @shift position.
> + *
> + * Returns -EINVAL if shift is invalid, -ERANGE if ncycles does not fit in
> + * the field, and 0 otherwise.
> + */
> +static inline int atmel_smc_cs_conf_set_timing(struct atmel_smc_cs_conf *conf,
> +					       unsigned int shift,
> +					       unsigned int ncycles)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	if (shift != ATMEL_HSMC_TIMINGS_TCLR_SHIFT &&
> +	    shift != ATMEL_HSMC_TIMINGS_TADL_SHIFT &&
> +	    shift != ATMEL_HSMC_TIMINGS_TAR_SHIFT &&
> +	    shift != ATMEL_HSMC_TIMINGS_TRR_SHIFT &&
> +	    shift != ATMEL_HSMC_TIMINGS_TWB_SHIFT)
> +		return -EINVAL;
> +
> +	/*
> +	 * The formula described in atmel datasheets (section "HSMC Timings
> +	 * Register"):
> +	 *
> +	 * ncycles = (Txx[3] * 64) + Txx[2:0]
> +	 */
> +	ret = atmel_smc_cs_encode_ncycles(ncycles, 3, 1, 64, &val);
> +	conf->timings &= ~GENMASK(shift + 3, shift);
> +	conf->timings |= val << shift;
> +
> +	return ret;
> +}
> +
> +/**
> + * atmel_smc_cs_conf_set_setup - set the SMC CS conf xx_SETUP parameter to a
> + *				 specific value
> + * @conf: SMC CS conf descriptor
> + * @shift: the position of the xx_SETUP field in the SETUP register
> + * @ncycles: value (expressed in MCK clk cycles) to assign to this xx_SETUP
> + *	     parameter
> + *
> + * This function encodes the @ncycles value as described in the datasheet
> + * (section "SMC Setup Register"), and then stores the result in the
> + * @conf->setup field at @shift position.
> + *
> + * Returns -EINVAL if @shift is invalid, -ERANGE if @ncycles does not fit in
> + * the field, and 0 otherwise.
> + */
> +static inline int atmel_smc_cs_conf_set_setup(struct atmel_smc_cs_conf *conf,
> +					      unsigned int shift,
> +					      unsigned int ncycles)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	if (shift != ATMEL_SMC_NWE_SHIFT && shift != ATMEL_SMC_NCS_WR_SHIFT &&
> +	    shift != ATMEL_SMC_NRD_SHIFT && shift != ATMEL_SMC_NCS_RD_SHIFT)
> +		return -EINVAL;
> +
> +	/*
> +	 * The formula described in atmel datasheets (section "SMC Setup
> +	 * Register"):
> +	 *
> +	 * ncycles = (128 * xx_SETUP[5]) + xx_SETUP[4:0]
> +	 */
> +	ret = atmel_smc_cs_encode_ncycles(ncycles, 5, 1, 128, &val);
> +	conf->setup &= ~GENMASK(shift + 7, shift);
> +	conf->setup |= val << shift;
> +
> +	return ret;
> +}
> +
> +/**
> + * atmel_smc_cs_conf_set_pulse - set the SMC CS conf xx_PULSE parameter to a
> + *				 specific value
> + * @conf: SMC CS conf descriptor
> + * @shift: the position of the xx_PULSE field in the PULSE register
> + * @ncycles: value (expressed in MCK clk cycles) to assign to this xx_PULSE
> + *	     parameter
> + *
> + * This function encodes the @ncycles value as described in the datasheet
> + * (section "SMC Pulse Register"), and then stores the result in the
> + * @conf->setup field at @shift position.
> + *
> + * Returns -EINVAL if @shift is invalid, -ERANGE if @ncycles does not fit in
> + * the field, and 0 otherwise.
> + */
> +static inline int atmel_smc_cs_conf_set_pulse(struct atmel_smc_cs_conf *conf,
> +					      unsigned int shift,
> +					      unsigned int ncycles)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	if (shift != ATMEL_SMC_NWE_SHIFT && shift != ATMEL_SMC_NCS_WR_SHIFT &&
> +	    shift != ATMEL_SMC_NRD_SHIFT && shift != ATMEL_SMC_NCS_RD_SHIFT)
> +		return -EINVAL;
> +
> +	/*
> +	 * The formula described in atmel datasheets (section "SMC Pulse
> +	 * Register"):
> +	 *
> +	 * ncycles = (256 * xx_PULSE[6]) + xx_PULSE[5:0]
> +	 */
> +	ret = atmel_smc_cs_encode_ncycles(ncycles, 6, 1, 256, &val);
> +	conf->pulse &= ~GENMASK(shift + 7, shift);
> +	conf->pulse |= val << shift;
> +
> +	return ret;
> +}
> +
> +/**
> + * atmel_smc_cs_conf_set_cycle - set the SMC CS conf xx_CYCLE parameter to a
> + *				 specific value
> + * @conf: SMC CS conf descriptor
> + * @shift: the position of the xx_CYCLE field in the CYCLE register
> + * @ncycles: value (expressed in MCK clk cycles) to assign to this xx_CYCLE
> + *	     parameter
> + *
> + * This function encodes the @ncycles value as described in the datasheet
> + * (section "SMC Pulse Register"), and then stores the result in the
> + * @conf->setup field at @shift position.
> + *
> + * Returns -EINVAL if @shift is invalid, -ERANGE if @ncycles does not fit in
> + * the field, and 0 otherwise.
> + */
> +static inline int atmel_smc_cs_conf_set_cycle(struct atmel_smc_cs_conf *conf,
> +					      unsigned int shift,
> +					      unsigned int ncycles)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	if (shift != ATMEL_SMC_NWE_SHIFT && shift != ATMEL_SMC_NRD_SHIFT)
> +		return -EINVAL;
> +
> +	/*
> +	 * The formula described in atmel datasheets (section "SMC Cycle
> +	 * Register"):
> +	 *
> +	 * ncycles = (xx_CYCLE[8:7] * 256) + xx_CYCLE[6:0]
> +	 */
> +	ret = atmel_smc_cs_encode_ncycles(ncycles, 7, 2, 256, &val);
> +	conf->cycle &= ~GENMASK(shift + 15, shift);
> +	conf->cycle |= val << shift;
> +
> +	return ret;
> +}
> +
> +/**
> + * atmel_smc_cs_conf_apply - apply an SMC CS conf
> + * @regmap: the SMC regmap
> + * @cs: the CS id
> + * @conf the SMC CS conf to apply
> + *
> + * Applies an SMC CS configuration.
> + * Only valid on at91sam9/avr32 SoCs.
> + */
> +static inline void atmel_smc_cs_conf_apply(struct regmap *regmap, int cs,
> +					   const struct atmel_smc_cs_conf *conf)
> +{
> +	regmap_write(regmap, ATMEL_SMC_SETUP(cs), conf->setup);
> +	regmap_write(regmap, ATMEL_SMC_PULSE(cs), conf->pulse);
> +	regmap_write(regmap, ATMEL_SMC_CYCLE(cs), conf->cycle);
> +	regmap_write(regmap, ATMEL_SMC_MODE(cs), conf->mode);
> +}
> +
> +/**
> + * atmel_hsmc_cs_conf_apply - apply an SMC CS conf
> + * @regmap: the HSMC regmap
> + * @cs: the CS id
> + * @conf the SMC CS conf to apply
> + *
> + * Applies an SMC CS configuration.
> + * Only valid on post-sama5 SoCs.
> + */
> +static inline void atmel_hsmc_cs_conf_apply(struct regmap *regmap, int cs,
> +					const struct atmel_smc_cs_conf *conf)
> +{
> +	regmap_write(regmap, ATMEL_HSMC_SETUP(cs), conf->setup);
> +	regmap_write(regmap, ATMEL_HSMC_PULSE(cs), conf->pulse);
> +	regmap_write(regmap, ATMEL_HSMC_CYCLE(cs), conf->cycle);
> +	regmap_write(regmap, ATMEL_HSMC_TIMINGS(cs), conf->timings);
> +	regmap_write(regmap, ATMEL_HSMC_MODE(cs), conf->mode);
> +}
> +
> +/**
> + * atmel_smc_cs_conf_get - retrieve the current SMC CS conf
> + * @regmap: the SMC regmap
> + * @cs: the CS id
> + * @conf: the SMC CS conf object to store the current conf
> + *
> + * Retrieve the SMC CS configuration.
> + * Only valid on at91sam9/avr32 SoCs.
> + */
> +static inline void atmel_smc_cs_conf_get(struct regmap *regmap, int cs,
> +					 struct atmel_smc_cs_conf *conf)
> +{
> +	regmap_read(regmap, ATMEL_SMC_SETUP(cs), &conf->setup);
> +	regmap_read(regmap, ATMEL_SMC_PULSE(cs), &conf->pulse);
> +	regmap_read(regmap, ATMEL_SMC_CYCLE(cs), &conf->cycle);
> +	regmap_read(regmap, ATMEL_SMC_MODE(cs), &conf->mode);
> +}
> +
> +/**
> + * atmel_hsmc_cs_conf_get - retrieve the current SMC CS conf
> + * @regmap: the HSMC regmap
> + * @cs: the CS id
> + * @conf: the SMC CS conf object to store the current conf
> + *
> + * Retrieve the SMC CS configuration.
> + * Only valid on post-sama5 SoCs.
> + */
> +static inline void atmel_hsmc_cs_conf_get(struct regmap *regmap, int cs,
> +					  struct atmel_smc_cs_conf *conf)
> +{
> +	regmap_read(regmap, ATMEL_HSMC_SETUP(cs), &conf->setup);
> +	regmap_read(regmap, ATMEL_HSMC_PULSE(cs), &conf->pulse);
> +	regmap_read(regmap, ATMEL_HSMC_CYCLE(cs), &conf->cycle);
> +	regmap_read(regmap, ATMEL_HSMC_TIMINGS(cs), &conf->timings);
> +	regmap_read(regmap, ATMEL_HSMC_MODE(cs), &conf->mode);
> +}
>  
>  /*
>   * This function converts a setup timing expressed in nanoseconds into an

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH 1/7] mfd: syscon: atmel-smc: Add new helpers to ease SMC regs manipulation
  2017-03-14 17:00     ` Lee Jones
@ 2017-03-14 17:21       ` Boris Brezillon
  -1 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-03-14 17:21 UTC (permalink / raw)
  To: Lee Jones
  Cc: Nicolas Ferre, Alexandre Belloni, linux-arm-kernel, Samuel Ortiz,
	linux-kernel

On Tue, 14 Mar 2017 17:00:03 +0000
Lee Jones <lee.jones@linaro.org> wrote:

> On Mon, 20 Feb 2017, Boris Brezillon wrote:
> 
> > These new helpers + macro definitions are meant to replace the old ones
> > which are unpractical to use.
> > 
> > Note that the macros and function prefixes have been intentionally
> > changed to ATMEL_[H]SMC_XX and atmel_[h]smc_ to reflect the fact that
> > this IP is also embedded in avr32 SoCs (and not only in at91 ones).  
> 
> I'm going to NACK this patch.
> 
> I don't see any reason why all those functions have to be inline and
> shoved into a header file.

Because those are simple conversion helpers, and I thought a
dedicated C file was not required, but I can move them into a C file
and export these functions using EXPORT_SYMBOL_GPL() if you prefer.

Would that work for you?

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH 1/7] mfd: syscon: atmel-smc: Add new helpers to ease SMC regs manipulation
@ 2017-03-14 17:21       ` Boris Brezillon
  0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-03-14 17:21 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, 14 Mar 2017 17:00:03 +0000
Lee Jones <lee.jones@linaro.org> wrote:

> On Mon, 20 Feb 2017, Boris Brezillon wrote:
> 
> > These new helpers + macro definitions are meant to replace the old ones
> > which are unpractical to use.
> > 
> > Note that the macros and function prefixes have been intentionally
> > changed to ATMEL_[H]SMC_XX and atmel_[h]smc_ to reflect the fact that
> > this IP is also embedded in avr32 SoCs (and not only in at91 ones).  
> 
> I'm going to NACK this patch.
> 
> I don't see any reason why all those functions have to be inline and
> shoved into a header file.

Because those are simple conversion helpers, and I thought a
dedicated C file was not required, but I can move them into a C file
and export these functions using EXPORT_SYMBOL_GPL() if you prefer.

Would that work for you?

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH 1/7] mfd: syscon: atmel-smc: Add new helpers to ease SMC regs manipulation
  2017-03-14 17:21       ` Boris Brezillon
@ 2017-03-15 12:19         ` Lee Jones
  -1 siblings, 0 replies; 34+ messages in thread
From: Lee Jones @ 2017-03-15 12:19 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Nicolas Ferre, Alexandre Belloni, linux-arm-kernel, Samuel Ortiz,
	linux-kernel

On Tue, 14 Mar 2017, Boris Brezillon wrote:

> On Tue, 14 Mar 2017 17:00:03 +0000
> Lee Jones <lee.jones@linaro.org> wrote:
> 
> > On Mon, 20 Feb 2017, Boris Brezillon wrote:
> > 
> > > These new helpers + macro definitions are meant to replace the old ones
> > > which are unpractical to use.
> > > 
> > > Note that the macros and function prefixes have been intentionally
> > > changed to ATMEL_[H]SMC_XX and atmel_[h]smc_ to reflect the fact that
> > > this IP is also embedded in avr32 SoCs (and not only in at91 ones).  
> > 
> > I'm going to NACK this patch.
> > 
> > I don't see any reason why all those functions have to be inline and
> > shoved into a header file.
> 
> Because those are simple conversion helpers, and I thought a
> dedicated C file was not required, but I can move them into a C file
> and export these functions using EXPORT_SYMBOL_GPL() if you prefer.
> 
> Would that work for you?

I think that would be better TBH.  Those functions aren't all that
simple.  There is certainly too much meat on the bones for me to be
happy for them to be shoved into a header file.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH 1/7] mfd: syscon: atmel-smc: Add new helpers to ease SMC regs manipulation
@ 2017-03-15 12:19         ` Lee Jones
  0 siblings, 0 replies; 34+ messages in thread
From: Lee Jones @ 2017-03-15 12:19 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, 14 Mar 2017, Boris Brezillon wrote:

> On Tue, 14 Mar 2017 17:00:03 +0000
> Lee Jones <lee.jones@linaro.org> wrote:
> 
> > On Mon, 20 Feb 2017, Boris Brezillon wrote:
> > 
> > > These new helpers + macro definitions are meant to replace the old ones
> > > which are unpractical to use.
> > > 
> > > Note that the macros and function prefixes have been intentionally
> > > changed to ATMEL_[H]SMC_XX and atmel_[h]smc_ to reflect the fact that
> > > this IP is also embedded in avr32 SoCs (and not only in at91 ones).  
> > 
> > I'm going to NACK this patch.
> > 
> > I don't see any reason why all those functions have to be inline and
> > shoved into a header file.
> 
> Because those are simple conversion helpers, and I thought a
> dedicated C file was not required, but I can move them into a C file
> and export these functions using EXPORT_SYMBOL_GPL() if you prefer.
> 
> Would that work for you?

I think that would be better TBH.  Those functions aren't all that
simple.  There is certainly too much meat on the bones for me to be
happy for them to be shoved into a header file.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH 1/7] mfd: syscon: atmel-smc: Add new helpers to ease SMC regs manipulation
  2017-03-15 12:19         ` Lee Jones
@ 2017-03-15 13:21           ` Boris Brezillon
  -1 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-03-15 13:21 UTC (permalink / raw)
  To: Lee Jones
  Cc: Nicolas Ferre, Alexandre Belloni, linux-arm-kernel, Samuel Ortiz,
	linux-kernel

On Wed, 15 Mar 2017 12:19:48 +0000
Lee Jones <lee.jones@linaro.org> wrote:

> On Tue, 14 Mar 2017, Boris Brezillon wrote:
> 
> > On Tue, 14 Mar 2017 17:00:03 +0000
> > Lee Jones <lee.jones@linaro.org> wrote:
> >   
> > > On Mon, 20 Feb 2017, Boris Brezillon wrote:
> > >   
> > > > These new helpers + macro definitions are meant to replace the old ones
> > > > which are unpractical to use.
> > > > 
> > > > Note that the macros and function prefixes have been intentionally
> > > > changed to ATMEL_[H]SMC_XX and atmel_[h]smc_ to reflect the fact that
> > > > this IP is also embedded in avr32 SoCs (and not only in at91 ones).    
> > > 
> > > I'm going to NACK this patch.
> > > 
> > > I don't see any reason why all those functions have to be inline and
> > > shoved into a header file.  
> > 
> > Because those are simple conversion helpers, and I thought a
> > dedicated C file was not required, but I can move them into a C file
> > and export these functions using EXPORT_SYMBOL_GPL() if you prefer.
> > 
> > Would that work for you?  
> 
> I think that would be better TBH.  Those functions aren't all that
> simple.  There is certainly too much meat on the bones for me to be
> happy for them to be shoved into a header file.
> 

I'm fine with that. Are you okay if I put this code in
drivers/mfd/atmel-smc.c?

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH 1/7] mfd: syscon: atmel-smc: Add new helpers to ease SMC regs manipulation
@ 2017-03-15 13:21           ` Boris Brezillon
  0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-03-15 13:21 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, 15 Mar 2017 12:19:48 +0000
Lee Jones <lee.jones@linaro.org> wrote:

> On Tue, 14 Mar 2017, Boris Brezillon wrote:
> 
> > On Tue, 14 Mar 2017 17:00:03 +0000
> > Lee Jones <lee.jones@linaro.org> wrote:
> >   
> > > On Mon, 20 Feb 2017, Boris Brezillon wrote:
> > >   
> > > > These new helpers + macro definitions are meant to replace the old ones
> > > > which are unpractical to use.
> > > > 
> > > > Note that the macros and function prefixes have been intentionally
> > > > changed to ATMEL_[H]SMC_XX and atmel_[h]smc_ to reflect the fact that
> > > > this IP is also embedded in avr32 SoCs (and not only in at91 ones).    
> > > 
> > > I'm going to NACK this patch.
> > > 
> > > I don't see any reason why all those functions have to be inline and
> > > shoved into a header file.  
> > 
> > Because those are simple conversion helpers, and I thought a
> > dedicated C file was not required, but I can move them into a C file
> > and export these functions using EXPORT_SYMBOL_GPL() if you prefer.
> > 
> > Would that work for you?  
> 
> I think that would be better TBH.  Those functions aren't all that
> simple.  There is certainly too much meat on the bones for me to be
> happy for them to be shoved into a header file.
> 

I'm fine with that. Are you okay if I put this code in
drivers/mfd/atmel-smc.c?

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code
  2017-03-02 12:30       ` Boris Brezillon
@ 2017-07-24  9:12         ` Alexander Dahl
  -1 siblings, 0 replies; 34+ messages in thread
From: Alexander Dahl @ 2017-07-24  9:12 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: linux-arm-kernel, Nicolas Ferre, Alexandre Belloni, Lee Jones,
	Samuel Ortiz, linux-kernel

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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code
@ 2017-07-24  9:12         ` Alexander Dahl
  0 siblings, 0 replies; 34+ messages in thread
From: Alexander Dahl @ 2017-07-24  9:12 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code
  2017-07-24  9:12         ` Alexander Dahl
@ 2017-07-24 19:21           ` Boris Brezillon
  -1 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-07-24 19:21 UTC (permalink / raw)
  To: Alexander Dahl
  Cc: linux-arm-kernel, Nicolas Ferre, Alexandre Belloni, Lee Jones,
	Samuel Ortiz, linux-kernel

Hi Alexander,

Le Mon, 24 Jul 2017 11:12:18 +0200,
Alexander Dahl <ada@thorsis.com> a écrit :

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

Yep, sorry about that.

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

There is a good reason for this "- 1": the doc says the exact number of
tDF cycles is TDF_CYCLES + 1. When you are expressing timings in ns it does
matter, because you don't want to wait more than necessary. Say the master
clk period is X ns and you want a tDF of X ns. If you divide tDF_ns by the
clk period you get one, and you only want to wait 1 cycle, not two.

The NAND driver seems to do the right thing already [1].

Below is my suggestion below to fix the problem. Did you have something else
in mind? In any case, can you send a patch to fix it (either using my
suggestion or something else if you prefer).

Regards,

Boris


[1]http://elixir.free-electrons.com/linux/v4.13-rc1/source/drivers/mtd/nand/atmel/nand-controller.c#L1309

--->8---
diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index 99e644cda4d1..d94604590d02 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -120,12 +120,14 @@ static int atmel_ebi_xslate_smc_timings(struct atmel_ebi_dev *ebid,
        if (!ret) {
                required = true;
                ncycles = DIV_ROUND_UP(val, clk_period_ns);
-               if (ncycles > ATMEL_SMC_MODE_TDF_MAX ||
-                   ncycles < ATMEL_SMC_MODE_TDF_MIN) {
+               if (ncycles > ATMEL_SMC_MODE_TDF_MAX) {
                        ret = -EINVAL;
                        goto out;
                }
 
+               if (ncycles < ATMEL_SMC_MODE_TDF_MIN)
+                       ncycles = ATMEL_SMC_MODE_TDF_MIN;
+
                smcconf->mode |= ATMEL_SMC_MODE_TDF(ncycles);
        }
 

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code
@ 2017-07-24 19:21           ` Boris Brezillon
  0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2017-07-24 19:21 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Alexander,

Le Mon, 24 Jul 2017 11:12:18 +0200,
Alexander Dahl <ada@thorsis.com> a ?crit :

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

Yep, sorry about that.

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

There is a good reason for this "- 1": the doc says the exact number of
tDF cycles is TDF_CYCLES + 1. When you are expressing timings in ns it does
matter, because you don't want to wait more than necessary. Say the master
clk period is X ns and you want a tDF of X ns. If you divide tDF_ns by the
clk period you get one, and you only want to wait 1 cycle, not two.

The NAND driver seems to do the right thing already [1].

Below is my suggestion below to fix the problem. Did you have something else
in mind? In any case, can you send a patch to fix it (either using my
suggestion or something else if you prefer).

Regards,

Boris


[1]http://elixir.free-electrons.com/linux/v4.13-rc1/source/drivers/mtd/nand/atmel/nand-controller.c#L1309

--->8---
diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index 99e644cda4d1..d94604590d02 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -120,12 +120,14 @@ static int atmel_ebi_xslate_smc_timings(struct atmel_ebi_dev *ebid,
        if (!ret) {
                required = true;
                ncycles = DIV_ROUND_UP(val, clk_period_ns);
-               if (ncycles > ATMEL_SMC_MODE_TDF_MAX ||
-                   ncycles < ATMEL_SMC_MODE_TDF_MIN) {
+               if (ncycles > ATMEL_SMC_MODE_TDF_MAX) {
                        ret = -EINVAL;
                        goto out;
                }
 
+               if (ncycles < ATMEL_SMC_MODE_TDF_MIN)
+                       ncycles = ATMEL_SMC_MODE_TDF_MIN;
+
                smcconf->mode |= ATMEL_SMC_MODE_TDF(ncycles);
        }
 

^ permalink raw reply related	[flat|nested] 34+ messages in thread
* Re: [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code
  2017-07-24 19:21           ` Boris Brezillon
@ 2017-07-25 11:43             ` Alexander Dahl
  -1 siblings, 0 replies; 34+ messages in thread
From: Alexander Dahl @ 2017-07-25 11:43 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Boris Brezillon, Samuel Ortiz, Nicolas Ferre, linux-kernel,
	Alexandre Belloni, Lee Jones

Hello Boris,

Am Montag, 24. Juli 2017, 21:21:09 schrieb Boris Brezillon:
> There is a good reason for this "- 1": the doc says the exact number
> of tDF cycles is TDF_CYCLES + 1. When you are expressing timings in
> ns it does matter, because you don't want to wait more than
> necessary. Say the master clk period is X ns and you want a tDF of X
> ns. If you divide tDF_ns by the clk period you get one, and you only
> want to wait 1 cycle, not two.

This makes sense. I tried several values through the same algorithm with 
a small test program and it gives the expected results. Thanks for the 
explanation. :-)

> The NAND driver seems to do the right thing already [1].
> 
> Below is my suggestion below to fix the problem. Did you have
> something else in mind? In any case, can you send a patch to fix it
> (either using my suggestion or something else if you prefer).

I prepared a small patch series together with two other small fixes for 
the atmel ebi driver and will send it to the list in a minute.

Greets
Alex

^ permalink raw reply	[flat|nested] 34+ messages in thread
* [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code
@ 2017-07-25 11:43             ` Alexander Dahl
  0 siblings, 0 replies; 34+ messages in thread
From: Alexander Dahl @ 2017-07-25 11:43 UTC (permalink / raw)
  To: linux-arm-kernel

Hello Boris,

Am Montag, 24. Juli 2017, 21:21:09 schrieb Boris Brezillon:
> There is a good reason for this "- 1": the doc says the exact number
> of tDF cycles is TDF_CYCLES + 1. When you are expressing timings in
> ns it does matter, because you don't want to wait more than
> necessary. Say the master clk period is X ns and you want a tDF of X
> ns. If you divide tDF_ns by the clk period you get one, and you only
> want to wait 1 cycle, not two.

This makes sense. I tried several values through the same algorithm with 
a small test program and it gives the expected results. Thanks for the 
explanation. :-)

> The NAND driver seems to do the right thing already [1].
> 
> Below is my suggestion below to fix the problem. Did you have
> something else in mind? In any case, can you send a patch to fix it
> (either using my suggestion or something else if you prefer).

I prepared a small patch series together with two other small fixes for 
the atmel ebi driver and will send it to the list in a minute.

Greets
Alex

^ permalink raw reply	[flat|nested] 34+ messages in thread
end of thread, other threads:[~2017-07-25 11:43 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-20 16:54 [PATCH 0/7] memory: atmel-ebi: Add PM ops Boris Brezillon
2017-02-20 16:54 ` Boris Brezillon
2017-02-20 16:54 ` [PATCH 1/7] mfd: syscon: atmel-smc: Add new helpers to ease SMC regs manipulation Boris Brezillon
2017-02-20 16:54   ` Boris Brezillon
2017-03-14 17:00   ` Lee Jones
2017-03-14 17:00     ` Lee Jones
2017-03-14 17:21     ` Boris Brezillon
2017-03-14 17:21       ` Boris Brezillon
2017-03-15 12:19       ` Lee Jones
2017-03-15 12:19         ` Lee Jones
2017-03-15 13:21         ` Boris Brezillon
2017-03-15 13:21           ` Boris Brezillon
2017-02-20 16:54 ` [PATCH 2/7] memory: atmel-ebi: Simplify SMC config code Boris Brezillon
2017-02-20 16:54   ` Boris Brezillon
2017-03-02 12:02   ` Alexander Dahl
2017-03-02 12:02     ` Alexander Dahl
2017-03-02 12:30     ` Boris Brezillon
2017-03-02 12:30       ` Boris Brezillon
2017-07-24  9:12       ` Alexander Dahl
2017-07-24  9:12         ` Alexander Dahl
2017-07-24 19:21         ` Boris Brezillon
2017-07-24 19:21           ` Boris Brezillon
2017-07-25 11:43           ` Alexander Dahl
2017-07-25 11:43             ` Alexander Dahl
2017-02-20 16:54 ` [PATCH 3/7] memory: atmel-ebi: Stop using reg_field objects for simple things Boris Brezillon
2017-02-20 16:54   ` Boris Brezillon
2017-02-20 16:54 ` [PATCH 4/7] mfd: syscon: atmel-smc: Remove unused helpers/macros Boris Brezillon
2017-02-20 16:54   ` Boris Brezillon
2017-02-20 16:54 ` [PATCH 5/7] memory: atmel-ebi: Change naming scheme Boris Brezillon
2017-02-20 16:54   ` Boris Brezillon
2017-02-20 16:55 ` [PATCH 6/7] memory: atmel-ebi: Add missing ->numcs assignment Boris Brezillon
2017-02-20 16:55   ` Boris Brezillon
2017-02-20 16:55 ` [PATCH 7/7] memory: atmel-ebi: Add PM ops Boris Brezillon
2017-02-20 16:55   ` Boris Brezillon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.