[v3,2/5] spi: imx: mx51-ecspi: Move some initialisation to prepare_message hook.

Message ID 20181130064709.6998-3-u.kleine-koenig@pengutronix.de
State New
Headers show
Series
  • spi: imx: Fix polarity switching for mx51-ecspi
Related show

Commit Message

Uwe Kleine-König Nov. 30, 2018, 6:47 a.m.
The relevant difference between prepare_message and config is that the
former is run before the CS signal is asserted. So the polarity of the
CLK line must be configured in prepare_message as an edge generated by
config might already result in a latch of the MOSI line.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-imx.c | 67 ++++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 27 deletions(-)

Comments

Robin Gong Dec. 3, 2018, 8 a.m. | #1
> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 2018年11月30日 14:47
> To: Mark Brown <broonie@kernel.org>; Robin Gong <yibin.gong@nxp.com>
> Cc: Marek Vasut <marex@denx.de>; dl-linux-imx <linux-imx@nxp.com>;
> kernel@pengutronix.de; linux-spi@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: [PATCH v3 2/5] spi: imx: mx51-ecspi: Move some initialisation to
> prepare_message hook.
> 
> The relevant difference between prepare_message and config is that the
> former is run before the CS signal is asserted. So the polarity of the CLK line
> must be configured in prepare_message as an edge generated by config might
> already result in a latch of the MOSI line.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/spi/spi-imx.c | 67 ++++++++++++++++++++++++++-----------------
>  1 file changed, 40 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> c7db42d6b3bc..d3a1e7104556 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -490,14 +490,9 @@ static void mx51_ecspi_disable(struct spi_imx_data
> *spi_imx)  static int mx51_ecspi_prepare_message(struct spi_imx_data
> *spi_imx,
>  				      struct spi_message *msg)
>  {
> -	return 0;
> -}
> -
> -static int mx51_ecspi_config(struct spi_device *spi) -{
> -	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
> +	struct spi_device *spi = msg->spi;
>  	u32 ctrl = MX51_ECSPI_CTRL_ENABLE;
> -	u32 clk = spi_imx->speed_hz, delay, reg;
> +	u32 testreg;
>  	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
> 
>  	/* set Master or Slave mode */
> @@ -512,19 +507,21 @@ static int mx51_ecspi_config(struct spi_device *spi)
>  	if (spi->mode & SPI_READY)
>  		ctrl |= MX51_ECSPI_CTRL_DRCTL(spi_imx->spi_drctl);
> 
> -	/* set clock speed */
> -	ctrl |= mx51_ecspi_clkdiv(spi_imx, spi_imx->speed_hz, &clk);
> -	spi_imx->spi_bus_clk = clk;
> -
>  	/* set chip select to use */
>  	ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select);
> 
> -	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
> -		ctrl |= (spi_imx->slave_burst * 8 - 1)
> -			<< MX51_ECSPI_CTRL_BL_OFFSET;
> +	/*
> +	 * The ctrl register must be written first, with the EN bit set other
> +	 * registers must not be written to.
> +	 */
> +	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
> +
> +	testreg = readl(spi_imx->base + MX51_ECSPI_TESTREG);
> +	if (spi->mode & SPI_LOOP)
> +		testreg |= MX51_ECSPI_TESTREG_LBC;
>  	else
> -		ctrl |= (spi_imx->bits_per_word - 1)
> -			<< MX51_ECSPI_CTRL_BL_OFFSET;
> +		testreg &= ~MX51_ECSPI_TESTREG_LBC;
> +	writel(testreg, spi_imx->base + MX51_ECSPI_TESTREG);
> 
>  	/*
>  	 * eCSPI burst completion by Chip Select signal in Slave mode @@ -548,25
> +545,42 @@ static int mx51_ecspi_config(struct spi_device *spi)
>  		cfg &= ~MX51_ECSPI_CONFIG_SCLKPOL(spi->chip_select);
>  		cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(spi->chip_select);
>  	}
> +
>  	if (spi->mode & SPI_CS_HIGH)
>  		cfg |= MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
>  	else
>  		cfg &= ~MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
> 
> -	if (spi_imx->usedma)
> -		ctrl |= MX51_ECSPI_CTRL_SMC;
> +	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
> 
> -	/* CTRL register always go first to bring out controller from reset */
> -	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
> +	return 0;
> +}
> 
> -	reg = readl(spi_imx->base + MX51_ECSPI_TESTREG);
> -	if (spi->mode & SPI_LOOP)
> -		reg |= MX51_ECSPI_TESTREG_LBC;
> +static int mx51_ecspi_config(struct spi_device *spi) {
> +	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
> +	u32 ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
> +	u32 clk = spi_imx->speed_hz, delay;
> +
> +	/* Clear BL field and set the right value */
> +	ctrl &= ~MX51_ECSPI_CTRL_BL_MASK;
> +	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
> +		ctrl |= (spi_imx->slave_burst * 8 - 1)
> +			<< MX51_ECSPI_CTRL_BL_OFFSET;
>  	else
> -		reg &= ~MX51_ECSPI_TESTREG_LBC;
> -	writel(reg, spi_imx->base + MX51_ECSPI_TESTREG);
> +		ctrl |= (spi_imx->bits_per_word - 1)
> +			<< MX51_ECSPI_CTRL_BL_OFFSET;
> 
> -	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
> +	/* set clock speed */
> +	ctrl &= ~(0xf << MX51_ECSPI_CTRL_POSTDIV_OFFSET |
> +		  0xf << MX51_ECSPI_CTRL_PREDIV_OFFSET);
> +	ctrl |= mx51_ecspi_clkdiv(spi_imx, spi_imx->speed_hz, &clk);
> +	spi_imx->spi_bus_clk = clk;
> +
> +	if (spi_imx->usedma)
> +		ctrl |= MX51_ECSPI_CTRL_SMC;
> +
> +	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
> 
>  	/*
>  	 * Wait until the changes in the configuration register CONFIGREG @@
Seems we don't need this commit anymore, your patch fix it by the way?

commit 6fd8b8503a0dcf66510314dc054745087ae89f94
Author: Marek Vasut <marex@denx.de>
Date:   Wed Dec 18 18:31:47 2013 +0100

    spi: spi-imx: Fix out-of-order CS/SCLK operation at low speeds

> -594,7 +608,6 @@ static void mx51_setup_wml(struct spi_imx_data *spi_imx)
>  	 * Configure the DMA register: setup the watermark
>  	 * and enable DMA request.
>  	 */
> -
>  	writel(MX51_ECSPI_DMA_RX_WML(spi_imx->wml - 1) |
>  		MX51_ECSPI_DMA_TX_WML(spi_imx->wml) |
>  		MX51_ECSPI_DMA_RXT_WML(spi_imx->wml) |
> --
> 2.19.2
Uwe Kleine-König Dec. 3, 2018, 8:18 a.m. | #2
Hello,

On Mon, Dec 03, 2018 at 08:00:21AM +0000, Robin Gong wrote:
> >  	/*
> >  	 * Wait until the changes in the configuration register CONFIGREG @@
> Seems we don't need this commit anymore, your patch fix it by the way?
> 
> commit 6fd8b8503a0dcf66510314dc054745087ae89f94
> Author: Marek Vasut <marex@denx.de>
> Date:   Wed Dec 18 18:31:47 2013 +0100
> 
>     spi: spi-imx: Fix out-of-order CS/SCLK operation at low speeds

I don't know, that's why I put Marek on Cc and talked to him in irc. I
couldn't lure him into testing on his machine if this indeed fixes his
problem.

Best regards
Uwe

Patch

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index c7db42d6b3bc..d3a1e7104556 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -490,14 +490,9 @@  static void mx51_ecspi_disable(struct spi_imx_data *spi_imx)
 static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
 				      struct spi_message *msg)
 {
-	return 0;
-}
-
-static int mx51_ecspi_config(struct spi_device *spi)
-{
-	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
+	struct spi_device *spi = msg->spi;
 	u32 ctrl = MX51_ECSPI_CTRL_ENABLE;
-	u32 clk = spi_imx->speed_hz, delay, reg;
+	u32 testreg;
 	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
 
 	/* set Master or Slave mode */
@@ -512,19 +507,21 @@  static int mx51_ecspi_config(struct spi_device *spi)
 	if (spi->mode & SPI_READY)
 		ctrl |= MX51_ECSPI_CTRL_DRCTL(spi_imx->spi_drctl);
 
-	/* set clock speed */
-	ctrl |= mx51_ecspi_clkdiv(spi_imx, spi_imx->speed_hz, &clk);
-	spi_imx->spi_bus_clk = clk;
-
 	/* set chip select to use */
 	ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select);
 
-	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
-		ctrl |= (spi_imx->slave_burst * 8 - 1)
-			<< MX51_ECSPI_CTRL_BL_OFFSET;
+	/*
+	 * The ctrl register must be written first, with the EN bit set other
+	 * registers must not be written to.
+	 */
+	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
+
+	testreg = readl(spi_imx->base + MX51_ECSPI_TESTREG);
+	if (spi->mode & SPI_LOOP)
+		testreg |= MX51_ECSPI_TESTREG_LBC;
 	else
-		ctrl |= (spi_imx->bits_per_word - 1)
-			<< MX51_ECSPI_CTRL_BL_OFFSET;
+		testreg &= ~MX51_ECSPI_TESTREG_LBC;
+	writel(testreg, spi_imx->base + MX51_ECSPI_TESTREG);
 
 	/*
 	 * eCSPI burst completion by Chip Select signal in Slave mode
@@ -548,25 +545,42 @@  static int mx51_ecspi_config(struct spi_device *spi)
 		cfg &= ~MX51_ECSPI_CONFIG_SCLKPOL(spi->chip_select);
 		cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(spi->chip_select);
 	}
+
 	if (spi->mode & SPI_CS_HIGH)
 		cfg |= MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
 	else
 		cfg &= ~MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
 
-	if (spi_imx->usedma)
-		ctrl |= MX51_ECSPI_CTRL_SMC;
+	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
 
-	/* CTRL register always go first to bring out controller from reset */
-	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
+	return 0;
+}
 
-	reg = readl(spi_imx->base + MX51_ECSPI_TESTREG);
-	if (spi->mode & SPI_LOOP)
-		reg |= MX51_ECSPI_TESTREG_LBC;
+static int mx51_ecspi_config(struct spi_device *spi)
+{
+	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
+	u32 ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
+	u32 clk = spi_imx->speed_hz, delay;
+
+	/* Clear BL field and set the right value */
+	ctrl &= ~MX51_ECSPI_CTRL_BL_MASK;
+	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
+		ctrl |= (spi_imx->slave_burst * 8 - 1)
+			<< MX51_ECSPI_CTRL_BL_OFFSET;
 	else
-		reg &= ~MX51_ECSPI_TESTREG_LBC;
-	writel(reg, spi_imx->base + MX51_ECSPI_TESTREG);
+		ctrl |= (spi_imx->bits_per_word - 1)
+			<< MX51_ECSPI_CTRL_BL_OFFSET;
 
-	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
+	/* set clock speed */
+	ctrl &= ~(0xf << MX51_ECSPI_CTRL_POSTDIV_OFFSET |
+		  0xf << MX51_ECSPI_CTRL_PREDIV_OFFSET);
+	ctrl |= mx51_ecspi_clkdiv(spi_imx, spi_imx->speed_hz, &clk);
+	spi_imx->spi_bus_clk = clk;
+
+	if (spi_imx->usedma)
+		ctrl |= MX51_ECSPI_CTRL_SMC;
+
+	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
 
 	/*
 	 * Wait until the changes in the configuration register CONFIGREG
@@ -594,7 +608,6 @@  static void mx51_setup_wml(struct spi_imx_data *spi_imx)
 	 * Configure the DMA register: setup the watermark
 	 * and enable DMA request.
 	 */
-
 	writel(MX51_ECSPI_DMA_RX_WML(spi_imx->wml - 1) |
 		MX51_ECSPI_DMA_TX_WML(spi_imx->wml) |
 		MX51_ECSPI_DMA_RXT_WML(spi_imx->wml) |