[U-Boot] [PATCH v2] spi: mxc_spi: Set master mode for all channels

[Dirk Behme <dirk.behme@gmail.com>]

Am 14.04.2013 09:08, schrieb Dirk Behme:
> Am 10.04.2013 01:06, schrieb Fabio Estevam:
>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>
>> The glitch in the SPI clock line, which commit 3cea335c34 (spi:
>> mxc_spi: Fix spi
>> clock glitch durant reset) solved, is back now and itwas
>> re-introduced by
>> commit d36b39bf0d (spi: mxc_spi: Fix ECSPI reset handling).
>>
>> Actually the glitch is happening due to always toggling between
>> slave mode
>> and master mode by configuring the CHANNEL_MODE bits in this reset
>> function.
>>
>> Since the spi driver only supports master mode, set the mode for all
>> channels
>> always to master mode in order to have a stable, "glitch-free" SPI
>> clock line.
>>
>> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
>> ---
>> Changes since v1:
>> - Introduce MXC_CSPICTRL_MODE_MASK definition
>> - Remove additional read of reg_ctrl
>>
>>   arch/arm/include/asm/arch-mx5/imx-regs.h |    1 +
>>   arch/arm/include/asm/arch-mx6/imx-regs.h |    1 +
>>   drivers/spi/mxc_spi.c                    |   17 +++++++++--------
>>   3 files changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/arch-mx5/imx-regs.h
>> b/arch/arm/include/asm/arch-mx5/imx-regs.h
>> index 249d15a..a71cc13 100644
>> --- a/arch/arm/include/asm/arch-mx5/imx-regs.h
>> +++ b/arch/arm/include/asm/arch-mx5/imx-regs.h
>> @@ -230,6 +230,7 @@
>>   #define MXC_CSPICTRL_EN        (1 << 0)
>>   #define MXC_CSPICTRL_MODE    (1 << 1)
>>   #define MXC_CSPICTRL_XCH    (1 << 2)
>> +#define MXC_CSPICTRL_MODE_MASK    (0xf << 4)
>>   #define MXC_CSPICTRL_CHIPSELECT(x)    (((x) & 0x3) << 12)
>>   #define MXC_CSPICTRL_BITCOUNT(x)    (((x) & 0xfff) << 20)
>>   #define MXC_CSPICTRL_PREDIV(x)    (((x) & 0xF) << 12)
>> diff --git a/arch/arm/include/asm/arch-mx6/imx-regs.h
>> b/arch/arm/include/asm/arch-mx6/imx-regs.h
>> index eaa7439..d79ab2f 100644
>> --- a/arch/arm/include/asm/arch-mx6/imx-regs.h
>> +++ b/arch/arm/include/asm/arch-mx6/imx-regs.h
>> @@ -346,6 +346,7 @@ struct cspi_regs {
>>   #define MXC_CSPICTRL_EN        (1 << 0)
>>   #define MXC_CSPICTRL_MODE    (1 << 1)
>>   #define MXC_CSPICTRL_XCH    (1 << 2)
>> +#define MXC_CSPICTRL_MODE_MASK (0xf << 4)
>>   #define MXC_CSPICTRL_CHIPSELECT(x)    (((x) & 0x3) << 12)
>>   #define MXC_CSPICTRL_BITCOUNT(x)    (((x) & 0xfff) << 20)
>>   #define MXC_CSPICTRL_PREDIV(x)    (((x) & 0xF) << 12)
>> diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
>> index 4c19e0b..20419e6 100644
>> --- a/drivers/spi/mxc_spi.c
>> +++ b/drivers/spi/mxc_spi.c
>> @@ -137,11 +137,15 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave
>> *mxcs, unsigned int cs,
>>           return -1;
>>       }
>>
>> -    /* Reset spi */
>> -    reg_write(&regs->ctrl, 0);
>> -    reg_write(&regs->ctrl, MXC_CSPICTRL_EN);
>> -
>> -    reg_ctrl = reg_read(&regs->ctrl);
>> +    /*
>> +     * Reset SPI and set all CSs to master mode, if toggling
>> +     * between slave and master mode we might see a glitch
>> +     * on the clock line
>> +     */
>> +    reg_ctrl = MXC_CSPICTRL_MODE_MASK;
>
> I was offline some days, giving me some time to think about this ;)
>
> Most probably it does no harm, but I somehow feel uncomfortable with
> setting *all* CSs to master mode. Just because there might be already
> (one, several?) CS at master mode and switching it back to the (reset
> default) slave will give a glitch on the clock line.
>
> Wouldn't it be cleaner to keep the master mode only for the CS which
> are already in master mode before?
>
> E.g. instead of
>
> reg_ctrl = MXC_CSPICTRL_MODE_MASK;
>
> from above something like
>
> reg_ctrl = reg_read(&regs->ctrl) & MXC_CSPICTRL_MODE_MASK;
>
> ?
>
> And then ...
>
>> +    reg_write(&regs->ctrl, reg_ctrl);
>> +    reg_ctrl |=  MXC_CSPICTRL_EN;
>> +    reg_write(&regs->ctrl, reg_ctrl);
>>
>>       /*
>>        * The following computation is taken directly from
>> Freescale's code.
>> @@ -174,9 +178,6 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave
>> *mxcs, unsigned int cs,
>>       reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_POSTDIV(0x0F)) |
>>           MXC_CSPICTRL_POSTDIV(post_div);
>>
>> -    /* always set to master mode */
>> -    reg_ctrl |= 1 << (cs + 4);
>
> ... keeping thins line?

What's about anything like below?

Best regards

Dirk

From: Dirk Behme <dirk.behme@gmail.com>
Date: Wed, 1 May 2013 07:28:38 +0200
Subject: [PATCH] spi: mxc_spi: Keep master mode only for configured 
channels

To avoid a glitch on the clock line while resetting the SPI controller,
the commit 0f1411bc8d (spi: mxc_spi: Set master mode for all channels)
enables the master mode for all channels.

Instead of enabling the master mode for all channels, keep only the
channels which are already configured in master mode in this mode.

To be able to switch additional channels to master mode, re-introduce
the master mode enable which was removed by above commit, then.

Signed-off-by: Dirk Behme <dirk.behme@gmail.com>
---
  drivers/spi/mxc_spi.c |   11 +++++++----
  1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
index 5bed858..843a1f2 100644
--- a/drivers/spi/mxc_spi.c
+++ b/drivers/spi/mxc_spi.c
@@ -138,11 +138,11 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave 
*mxcs, unsigned int cs,
  	}

  	/*
-	 * Reset SPI and set all CSs to master mode, if toggling
-	 * between slave and master mode we might see a glitch
-	 * on the clock line
+	 * Reset SPI and the CONREG, but keep the CSs which are already
+	 * in master mode. If toggling between slave and master mode we might
+	 * see a glitch on the clock line
  	 */
-	reg_ctrl = MXC_CSPICTRL_MODE_MASK;
+	reg_ctrl = reg_read(&regs->ctrl) & MXC_CSPICTRL_MODE_MASK;
  	reg_write(&regs->ctrl, reg_ctrl);
  	reg_ctrl |=  MXC_CSPICTRL_EN;
  	reg_write(&regs->ctrl, reg_ctrl);
@@ -178,6 +178,9 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, 
unsigned int cs,
  	reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_POSTDIV(0x0F)) |
  		MXC_CSPICTRL_POSTDIV(post_div);

+	/* always set to master mode */
+	reg_ctrl |= 1 << (cs + 4);
+
  	/* We need to disable SPI before changing registers */
  	reg_ctrl &= ~MXC_CSPICTRL_EN;

-- 
1.7.10.4