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

[Dirk Behme]

Am 10.04.2013 01:06, schrieb Fabio Estevam:
> From: Fabio Estevam <fabio.estevam at 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 at 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