[U-Boot] [PATCH] spi: mxc_spi: Fix ECSPI reset handling

[U-Boot] [PATCH] spi: mxc_spi: Fix ECSPI reset handling

[Dirk Behme]

Reviewing the ECSPI reset handling shows two issues:

1. For the enable/reset bit (MXC_CSPICTRL_EN) in the control reg
   (ECSPIx_CONGREG) the i.MX6 technical reference manual states:

   -- cut --
   ECSPIx_CONREG[0]: EN: Writing zero to this bit disables the block
   and resets the internal logic with the exception of the ECSPI_CONREG.
   -- cut --

   Note the exception mentioned: The CONREG itself isn't reset.

   Fix this by manually writing the reset value 0 to the whole register.
   This sets the EN bit to zero, too (i.e. includes the old
   ~MXC_CSPICTRL_EN).

2. We want to reset the whole SPI block here. So it makes no sense
   to first read the old value of the CONREG and write it back, later.
   This will give us the old (historic/random) value of the CONREG back.
   And doesn't reset the CONREG.

   To get a clean CONREG after the reset of the block, too, don't use
   the old (historic/random) value of the CONREG while doing the reset.
   And read the clean CONREG after the reset.

This was found while working on a SPI boot device where the i.MX6 boot
ROM has already initialized the SPI block. The initialization by the
boot ROM might be different to what the U-Boot driver wants to configure.
I.e. we need a clean reset of SPI block, including the CONREG.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
CC: Stefano Babic <sbabic@denx.de>
CC: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/spi/mxc_spi.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
index d792d8d..cb48019 100644
--- a/drivers/spi/mxc_spi.c
+++ b/drivers/spi/mxc_spi.c
@@ -137,11 +137,11 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, unsigned int cs,
 		return -1;
 	}
 
-	reg_ctrl = reg_read(&regs->ctrl);
-
 	/* Reset spi */
-	reg_write(&regs->ctrl, (reg_ctrl & ~MXC_CSPICTRL_EN));
-	reg_write(&regs->ctrl, (reg_ctrl | MXC_CSPICTRL_EN));
+	reg_write(&regs->ctrl, 0);
+	reg_write(&regs->ctrl, MXC_CSPICTRL_EN);
+
+	reg_ctrl = reg_read(&regs->ctrl);
 
 	/*
 	 * The following computation is taken directly from Freescale's code.
-- 
1.7.0.4

[U-Boot] [PATCH] spi: mxc_spi: Fix ECSPI reset handling

[Stefano Babic]

On 21/03/2013 09:03, Dirk Behme wrote:
> Reviewing the ECSPI reset handling shows two issues:
> 

Hi Dirk,


agree completely, only a very minor question..


> +
> +	reg_ctrl = reg_read(&regs->ctrl);

As you says, it makes no sense to read back the value of the register,
also because reg_ctrl is overwritten some lines later ;-)

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH] spi: mxc_spi: Fix ECSPI reset handling

[Dirk Behme]

Am 03.04.2013 11:12, schrieb Stefano Babic:
> On 21/03/2013 09:03, Dirk Behme wrote:
>> Reviewing the ECSPI reset handling shows two issues:
>>
>
> Hi Dirk,
>
>
> agree completely, only a very minor question..
>
>
>> +
>> +	reg_ctrl = reg_read(&regs->ctrl);
>
> As you says, it makes no sense to read back the value of the register,
> also because reg_ctrl is overwritten some lines later ;-)

Hmm, sorry if I overlooked something, but we have to initialize the 
variable 'reg_ctrl' with the recent register content because it is 
first used and _then_ overwritten in the next step:

reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_SELCHAN(3)) | 
MXC_CSPICTRL_SELCHAN(cs);

http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=drivers/spi/mxc_spi.c;h=d792d8d493c13c475ec8ca03694f4efd8fde0e7f;hb=HEAD#l170

(?)

Best regards

Dirk

[U-Boot] [PATCH] spi: mxc_spi: Fix ECSPI reset handling

[Stefano Babic]

On 03/04/2013 18:54, Dirk Behme wrote:
> Am 03.04.2013 11:12, schrieb Stefano Babic:
>> On 21/03/2013 09:03, Dirk Behme wrote:
>>> Reviewing the ECSPI reset handling shows two issues:
>>>
>>
>> Hi Dirk,
>>
>>
>> agree completely, only a very minor question..
>>
>>
>>> +
>>> +    reg_ctrl = reg_read(&regs->ctrl);
>>
>> As you says, it makes no sense to read back the value of the register,
>> also because reg_ctrl is overwritten some lines later ;-)
> 
> Hmm, sorry if I overlooked something, but we have to initialize the
> variable 'reg_ctrl' with the recent register content because it is first
> used and _then_ overwritten in the next step:
> 
> reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_SELCHAN(3)) |
> MXC_CSPICTRL_SELCHAN(cs);

What I meant is that we know already the value of the register. Your
patch set it to MXC_CSPICTRL_EN, and this could be put in or later when
you set again the register ;-)

Nevermind, your patch is ok and fixes a real problem. What I commented
are more or less style-related.

I merge it and push for the release.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================