[PATCH 0/3] memory: atmel-ebi: Fix setting EBI timings through dts

[PATCH 0/3] memory: atmel-ebi: Fix setting EBI timings through dts

[Alexander Dahl]

Hello,

this small patch series based on v4.13-rc2 fixes three things I found
when trying to run the latest rc on an at91samg20 based platform with
a SRAM like memory connected to the EBI interface, for which the
timings should be set through dts. 

For all register settings of interest I checked the hardware manuals
of the at91sam9g20 and the sama5d3x to not cause regressions, as well
as the actual register settings in the running system. (I used the
devmem tool from busybox for that.)

Greets
Alex

[PATCH 1/3] memory: atmel-ebi: Fix smc timing return value evaluation

[Alexander Dahl]

Setting optional EBI/SMC properties through device tree always fails due
to wrong evaluation of the return value of
atmel_ebi_xslate_smc_timings().

If you put some of those properties in your dts file, but not
'atmel,smc-tdf-ns' the local variable 'required' in
atmel_ebi_xslate_smc_timings() stays on 'false' after the first 'if'
block. This leads to setting 'ret' to -EINVAL in the first run of the
following 'for' loop which is then the return value of this function.

However if you set 'atmel,smc-tdf-ns' in the dts file and everything in
atmel_ebi_xslate_smc_timings() works well, it returns the content of
'required' which is 'true' then.

So the function atmel_ebi_xslate_smc_timings() always returns non-zero
which lets its call in atmel_ebi_xslate_smc_config() always fail and
thus returning -EINVAL, so the EBI configuration for this node fails.

Judging from the following code evaluating the local 'required' variable
in atmel_ebi_xslate_smc_config() and the call of caps->xlate_config in
atmel_ebi_dev_setup() it's probably right to only let the call fail if a
negative error code is returned.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 drivers/memory/atmel-ebi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index 99e644c..1cf34d2 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -263,7 +263,7 @@ static int atmel_ebi_xslate_smc_config(struct atmel_ebi_dev *ebid,
 	}
 
 	ret = atmel_ebi_xslate_smc_timings(ebid, np, &conf->smcconf);
-	if (ret)
+	if (ret < 0)
 		return -EINVAL;
 
 	if ((ret > 0 && !required) || (!ret && required)) {
-- 
2.1.4

[PATCH 2/3] memory: atmel-ebi: Allow t_DF timings of zero ns

[Alexander Dahl]

As reported in [1] and in [2] it's not possible to set the device tree
property 'atmel,smc-tdf-ns' to zero, although the SoC allows a setting
of 0ns for the t_DF time.

Allow this setting by doing the same thing as in the atmel nand
controller driver by setting ncycles to ATMEL_SMC_MODE_TDF_MIN if zero
is set in the dts.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-March/490966.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-July/520652.html

Suggested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 drivers/memory/atmel-ebi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index 1cf34d2..f8a01ae 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);
 	}
 
-- 
2.1.4

[PATCH 3/3] memory: atmel-ebi: Fix smc cycle xlate converter

[Alexander Dahl]

The converter function for translating ns timings in register values was
initialized with a wrong function pointer. This resulted in wrong
register values also for the setup and pulse registers when configuring
the EBI interface trough dts.

Includes a small fix in a comment of the smc driver, which was probably
just a copy'n'paste mistake.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 drivers/memory/atmel-ebi.c | 2 +-
 drivers/mfd/atmel-smc.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index f8a01ae..ebf69ff 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -72,7 +72,7 @@ struct atmel_smc_timing_xlate {
 	{ .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}
+	{ .name = nm, .converter = atmel_smc_cs_conf_set_cycle, .shift = pos}
 
 static void at91sam9_ebi_get_config(struct atmel_ebi_dev *ebid,
 				    struct atmel_ebi_dev_config *conf)
diff --git a/drivers/mfd/atmel-smc.c b/drivers/mfd/atmel-smc.c
index 954cf0f..20cc0ea 100644
--- a/drivers/mfd/atmel-smc.c
+++ b/drivers/mfd/atmel-smc.c
@@ -206,7 +206,7 @@ EXPORT_SYMBOL_GPL(atmel_smc_cs_conf_set_pulse);
  *	     parameter
  *
  * This function encodes the @ncycles value as described in the datasheet
- * (section "SMC Pulse Register"), and then stores the result in the
+ * (section "SMC Cycle 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
-- 
2.1.4

Re: [PATCH 3/3] memory: atmel-ebi: Fix smc cycle xlate converter

[Lee Jones]

On Tue, 25 Jul 2017, Alexander Dahl wrote:

> The converter function for translating ns timings in register values was
> initialized with a wrong function pointer. This resulted in wrong
> register values also for the setup and pulse registers when configuring
> the EBI interface trough dts.
> 
> Includes a small fix in a comment of the smc driver, which was probably
> just a copy'n'paste mistake.
> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---
>  drivers/memory/atmel-ebi.c | 2 +-
>  drivers/mfd/atmel-smc.c    | 2 +-

Acked-by: Lee Jones <lee.jones@linaro.org>

>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
> index f8a01ae..ebf69ff 100644
> --- a/drivers/memory/atmel-ebi.c
> +++ b/drivers/memory/atmel-ebi.c
> @@ -72,7 +72,7 @@ struct atmel_smc_timing_xlate {
>  	{ .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}
> +	{ .name = nm, .converter = atmel_smc_cs_conf_set_cycle, .shift = pos}
>  
>  static void at91sam9_ebi_get_config(struct atmel_ebi_dev *ebid,
>  				    struct atmel_ebi_dev_config *conf)
> diff --git a/drivers/mfd/atmel-smc.c b/drivers/mfd/atmel-smc.c
> index 954cf0f..20cc0ea 100644
> --- a/drivers/mfd/atmel-smc.c
> +++ b/drivers/mfd/atmel-smc.c
> @@ -206,7 +206,7 @@ EXPORT_SYMBOL_GPL(atmel_smc_cs_conf_set_pulse);
>   *	     parameter
>   *
>   * This function encodes the @ncycles value as described in the datasheet
> - * (section "SMC Pulse Register"), and then stores the result in the
> + * (section "SMC Cycle 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

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

Re: [PATCH 0/3] memory: atmel-ebi: Fix setting EBI timings through dts

[Boris Brezillon]

Le Tue, 25 Jul 2017 14:00:21 +0200,
Alexander Dahl <ada@thorsis.com> a écrit :

> Hello,
> 
> this small patch series based on v4.13-rc2 fixes three things I found
> when trying to run the latest rc on an at91samg20 based platform with
> a SRAM like memory connected to the EBI interface, for which the
> timings should be set through dts. 
> 
> For all register settings of interest I checked the hardware manuals
> of the at91sam9g20 and the sama5d3x to not cause regressions, as well
> as the actual register settings in the running system. (I used the
> devmem tool from busybox for that.)

To the whole series:

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Alexandre, Nicolas, can you queue that for 4.13-rc3 or -rc4?

Re: [PATCH 0/3] memory: atmel-ebi: Fix setting EBI timings through dts

[Alexandre Belloni]

On 25/07/2017 at 14:00:21 +0200, Alexander Dahl wrote:
> Hello,
> 
> this small patch series based on v4.13-rc2 fixes three things I found
> when trying to run the latest rc on an at91samg20 based platform with
> a SRAM like memory connected to the EBI interface, for which the
> timings should be set through dts. 
> 
> For all register settings of interest I checked the hardware manuals
> of the at91sam9g20 and the sama5d3x to not cause regressions, as well
> as the actual register settings in the running system. (I used the
> devmem tool from busybox for that.)
> 

All applied, thanks.

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