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;