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

login
register
mail settings
Submitter Fabio Estevam
Date April 9, 2013, 11:06 p.m.
Message ID <1365548785-28684-1-git-send-email-festevam@gmail.com>
Download mbox | patch
Permalink /patch/235260/
State Awaiting Upstream
Delegated to: Stefano Babic
Headers show

Comments

Fabio Estevam - April 9, 2013, 11:06 p.m.
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(-)
Stefano Babic - April 10, 2013, 6:10 a.m.
On 10/04/2013 01:06, Fabio Estevam wrote:
> 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>
> ---

Hi Fabio,


> 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;
> +	reg_write(&regs->ctrl, reg_ctrl);
> +	reg_ctrl |=  MXC_CSPICTRL_EN;
> +	reg_write(&regs->ctrl, reg_ctrl);

I am afraid you are breaking MX3x / MX25 because you add
MXC_CSPICTRL_MODE_MASK only to MX5 / MX6.

Best regards,
Stefano
Fabio Estevam - April 10, 2013, 12:01 p.m.
Hi Stefano,

On Wed, Apr 10, 2013 at 3:10 AM, Stefano Babic <sbabic@denx.de> wrote:

> I am afraid you are breaking MX3x / MX25 because you add
> MXC_CSPICTRL_MODE_MASK only to MX5 / MX6.

It will not break other platforms because this code is protected with
a "#ifdef MXC_ECSPI".

And MXC_ECSPI is only defined on mx5/mx6:

arch/arm/include/asm/arch-mx5/imx-regs.h:#define MXC_ECSPI
arch/arm/include/asm/arch-mx6/imx-regs.h:#define MXC_ECSPI

,so we are safe here.

Regards,

Fabio Estevam
Stefano Babic - April 13, 2013, 3:50 p.m.
On 10/04/2013 14:01, Fabio Estevam wrote:
> Hi Stefano,
> 
> On Wed, Apr 10, 2013 at 3:10 AM, Stefano Babic <sbabic@denx.de> wrote:
> 
>> I am afraid you are breaking MX3x / MX25 because you add
>> MXC_CSPICTRL_MODE_MASK only to MX5 / MX6.
> 
> It will not break other platforms because this code is protected with
> a "#ifdef MXC_ECSPI".
> 
> And MXC_ECSPI is only defined on mx5/mx6:
> 
> arch/arm/include/asm/arch-mx5/imx-regs.h:#define MXC_ECSPI
> arch/arm/include/asm/arch-mx6/imx-regs.h:#define MXC_ECSPI
> 
> ,so we are safe here.

Thanks, you're right.

Applied to u-boot-imx, thanks.

Best regards,
Stefano Babic
Dirk Behme - April 14, 2013, 7:08 a.m.
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?

Best regards

Dirk

Patch

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;
+	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);
-
 	/* We need to disable SPI before changing registers */
 	reg_ctrl &= ~MXC_CSPICTRL_EN;