diff mbox series

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

Message ID 20181123085158.24753-3-u.kleine-koenig@pengutronix.de (mailing list archive)
State Superseded
Headers show
Series spi: imx: Fix polarity switching for mx51-ecspi | expand

Commit Message

Uwe Kleine-König Nov. 23, 2018, 8:51 a.m. UTC 
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 | 55 ++++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 22 deletions(-)

Comments

Robin Gong Nov. 26, 2018, 9:05 a.m. UTC | #1 
> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 2018年11月23日 16:52
> 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
> Subject: [PATCH v2 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.
Is this a real problem fix patch or just refine in theory? I have quick test and 
saw a big performance downgrade below on i.mx6sl-evk board(1.6MB/s->706KB/s)

root@imx6qpdlsolox:~# dd if=/dev/mtd0 of=/dev/null bs=1M count=1
1+0 records in
1+0 records out
1048576 bytes (1.0 MB, 1.0 MiB) copied, 1.48557 s, 706 kB/s
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/spi/spi-imx.c | 55 ++++++++++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> c7db42d6b3bc..88a7a53441f5 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,10 +507,6 @@ 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);
> 
> @@ -526,6 +517,19 @@ static int mx51_ecspi_config(struct spi_device *spi)
>  		ctrl |= (spi_imx->bits_per_word - 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
> +		testreg &= ~MX51_ECSPI_TESTREG_LBC;
> +	writel(testreg, spi_imx->base + MX51_ECSPI_TESTREG);
> +
>  	/*
>  	 * eCSPI burst completion by Chip Select signal in Slave mode
>  	 * is not functional for imx53 Soc, config SPI burst completed when @@
> -548,26 +552,34 @@ 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);
> 
> +	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
> +
> +	return 0;
> +}
> +
> +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;
> +
> +	/* 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;
> 
> -	/* CTRL register always go first to bring out controller from reset */
>  	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
> 
> -	reg = readl(spi_imx->base + MX51_ECSPI_TESTREG);
> -	if (spi->mode & SPI_LOOP)
> -		reg |= MX51_ECSPI_TESTREG_LBC;
> -	else
> -		reg &= ~MX51_ECSPI_TESTREG_LBC;
> -	writel(reg, spi_imx->base + MX51_ECSPI_TESTREG);
> -
> -	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
> -
>  	/*
>  	 * Wait until the changes in the configuration register CONFIGREG
>  	 * propagate into the hardware. It takes exactly one tick of the @@
> -594,7 +606,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.1
Uwe Kleine-König Nov. 26, 2018, 9:17 a.m. UTC | #2 
Hello Robin,

On Mon, Nov 26, 2018 at 09:05:53AM +0000, Robin Gong wrote:
> > -----Original Message-----
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Sent: 2018年11月23日 16:52
> > 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
> > Subject: [PATCH v2 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.
> Is this a real problem fix patch or just refine in theory? I have quick test and 
> saw a big performance downgrade below on i.mx6sl-evk board(1.6MB/s->706KB/s)

This is a real problem. If you have two spi devices on the bus with need
modes with different CPOL it can happen that after a transfer on one
CPOL is changed for the other while SS is already active and this is
already interpreted as the first latch.

> root@imx6qpdlsolox:~# dd if=/dev/mtd0 of=/dev/null bs=1M count=1
> 1+0 records in
> 1+0 records out
> 1048576 bytes (1.0 MB, 1.0 MiB) copied, 1.48557 s, 706 kB/s

I didn't test this, but I'm surprised about that. For single-transfer
messages the effect should just be that with the patch the following
happens:

	setup_spi_imx_registers()
	activate_gpio_for_SS()

instead of

	activate_gpio_for_SS()
	setup_spi_imx_registers()

with the previous logic. For multi-transfer messages the setup is only
done once instead of once per transfer.

You compared 4.20-rc1 against 4.20-rc1 + my patches 1 and 2?

Best regards
Uwe
Robin Gong Nov. 29, 2018, 12:53 p.m. UTC | #3 
> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 2018年11月26日 17:18
> To: Robin Gong <yibin.gong@nxp.com>
> Cc: Mark Brown <broonie@kernel.org>; Marek Vasut <marex@denx.de>;
> dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> linux-spi@vger.kernel.org
> Subject: Re: [PATCH v2 2/5] spi: imx: mx51-ecspi: Move some initialisation to
> prepare_message hook.
> 
> Hello Robin,
> 
> On Mon, Nov 26, 2018 at 09:05:53AM +0000, Robin Gong wrote:
> > > -----Original Message-----
> > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Sent: 2018年11月23日 16:52
> > > 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
> > > Subject: [PATCH v2 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.
> > Is this a real problem fix patch or just refine in theory? I have
> > quick test and saw a big performance downgrade below on i.mx6sl-evk
> > board(1.6MB/s->706KB/s)
> 
> This is a real problem. If you have two spi devices on the bus with need modes
> with different CPOL it can happen that after a transfer on one CPOL is changed
> for the other while SS is already active and this is already interpreted as the
> first latch.
> 
> > root@imx6qpdlsolox:~# dd if=/dev/mtd0 of=/dev/null bs=1M count=1
> > 1+0 records in
> > 1+0 records out
> > 1048576 bytes (1.0 MB, 1.0 MiB) copied, 1.48557 s, 706 kB/s
> 
> I didn't test this, but I'm surprised about that. For single-transfer messages the
> effect should just be that with the patch the following
> happens:
> 
> 	setup_spi_imx_registers()
> 	activate_gpio_for_SS()
> 
> instead of
> 
> 	activate_gpio_for_SS()
> 	setup_spi_imx_registers()
Yes, in theory your patch should not cause such performance issue since you just
move some jobs of spi configure ahead of activate_gpio_for_SS(). But unfortunately,
MX51_ECSPI_CTRL_BL_OFFSET may be broken by PIO transfer in case some small data,
such as small command send to SPI-NOR before 1M data reading.
Please refer to dynamic_burst related code of spi_imx_push(). If I keep BL setting in 
mx51_ecspi_prepare_transfer() instead of mx51_ecspi_prepare_message(), the issue
is gone. Below is my fix, please merge it in V3.

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 32d1c4f..05f9e50 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -510,13 +510,6 @@ static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
        /* 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;
-       else
-               ctrl |= (spi_imx->bits_per_word - 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.
@@ -570,6 +563,15 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
        u32 ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
        u32 clk = t->speed_hz, delay;

+       /* Clear BL filed 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
+               ctrl |= (spi_imx->bits_per_word - 1)
+                       << MX51_ECSPI_CTRL_BL_OFFSET;
+
        /* set clock speed */
        ctrl &= ~(0xf << MX51_ECSPI_CTRL_POSTDIV_OFFSET |
                  0xf << MX51_ECSPI_CTRL_PREDIV_OFFSET);

> 
> with the previous logic. For multi-transfer messages the setup is only done
> once instead of once per transfer.
> 
> You compared 4.20-rc1 against 4.20-rc1 + my patches 1 and 2?
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&amp;data=02%7C01%7Cyibin.gong%40nxp.com%7C828c8
> 376e9e4463f333808d65380075f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> 0%7C0%7C636788206700293491&amp;sdata=Um0RkXzahclhthk5Odq3ZkDINE
> ZdbsKvwaY8IvH8iEg%3D&amp;reserved=0  |
Uwe Kleine-König Nov. 30, 2018, 10:11 a.m. UTC | #4 
Hello Robin,

On Thu, Nov 29, 2018 at 12:53:33PM +0000, Robin Gong wrote:
> > -----Original Message-----
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Sent: 2018年11月26日 17:18
> > To: Robin Gong <yibin.gong@nxp.com>
> > Cc: Mark Brown <broonie@kernel.org>; Marek Vasut <marex@denx.de>;
> > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> > linux-spi@vger.kernel.org
> > Subject: Re: [PATCH v2 2/5] spi: imx: mx51-ecspi: Move some initialisation to
> > prepare_message hook.
> > 
> > Hello Robin,
> > 
> > On Mon, Nov 26, 2018 at 09:05:53AM +0000, Robin Gong wrote:
> > > > -----Original Message-----
> > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > Sent: 2018年11月23日 16:52
> > > > 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
> > > > Subject: [PATCH v2 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.
> > > Is this a real problem fix patch or just refine in theory? I have
> > > quick test and saw a big performance downgrade below on i.mx6sl-evk
> > > board(1.6MB/s->706KB/s)
> > 
> > This is a real problem. If you have two spi devices on the bus with need modes
> > with different CPOL it can happen that after a transfer on one CPOL is changed
> > for the other while SS is already active and this is already interpreted as the
> > first latch.
> > 
> > > root@imx6qpdlsolox:~# dd if=/dev/mtd0 of=/dev/null bs=1M count=1
> > > 1+0 records in
> > > 1+0 records out
> > > 1048576 bytes (1.0 MB, 1.0 MiB) copied, 1.48557 s, 706 kB/s
> > 
> > I didn't test this, but I'm surprised about that. For single-transfer messages the
> > effect should just be that with the patch the following
> > happens:
> > 
> > 	setup_spi_imx_registers()
> > 	activate_gpio_for_SS()
> > 
> > instead of
> > 
> > 	activate_gpio_for_SS()
> > 	setup_spi_imx_registers()
> Yes, in theory your patch should not cause such performance issue since you just
> move some jobs of spi configure ahead of activate_gpio_for_SS(). But unfortunately,
> MX51_ECSPI_CTRL_BL_OFFSET may be broken by PIO transfer in case some small data,
> such as small command send to SPI-NOR before 1M data reading.
> Please refer to dynamic_burst related code of spi_imx_push(). If I keep BL setting in 
> mx51_ecspi_prepare_transfer() instead of mx51_ecspi_prepare_message(), the issue
> is gone. Below is my fix, please merge it in V3.
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 32d1c4f..05f9e50 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -510,13 +510,6 @@ static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
>         /* 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;
> -       else
> -               ctrl |= (spi_imx->bits_per_word - 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.
> @@ -570,6 +563,15 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
>         u32 ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
>         u32 clk = t->speed_hz, delay;
> 
> +       /* Clear BL filed 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
> +               ctrl |= (spi_imx->bits_per_word - 1)
> +                       << MX51_ECSPI_CTRL_BL_OFFSET;
> +

Even if I sent out v3 now, I wonder if the problem is just that I didn't
clear MX51_ECSPI_CTRL_BL_MASK in mx51_ecspi_prepare_message()?

Best regards
Uwe
Robin Gong Dec. 3, 2018, 7:55 a.m. UTC | #5 
> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 2018年11月30日 18:11
> To: Robin Gong <yibin.gong@nxp.com>
> Cc: Mark Brown <broonie@kernel.org>; Marek Vasut <marex@denx.de>;
> dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> linux-spi@vger.kernel.org
> Subject: Re: [PATCH v2 2/5] spi: imx: mx51-ecspi: Move some initialisation to
> prepare_message hook.
> 
> Hello Robin,
> 
> On Thu, Nov 29, 2018 at 12:53:33PM +0000, Robin Gong wrote:
> > > -----Original Message-----
> > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Sent: 2018年11月26日 17:18
> > > To: Robin Gong <yibin.gong@nxp.com>
> > > Cc: Mark Brown <broonie@kernel.org>; Marek Vasut <marex@denx.de>;
> > > dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de;
> > > linux-spi@vger.kernel.org
> > > Subject: Re: [PATCH v2 2/5] spi: imx: mx51-ecspi: Move some
> > > initialisation to prepare_message hook.
> > >
> > > Hello Robin,
> > >
> > > On Mon, Nov 26, 2018 at 09:05:53AM +0000, Robin Gong wrote:
> > > > > -----Original Message-----
> > > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > Sent: 2018年11月23日 16:52
> > > > > 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
> > > > > Subject: [PATCH v2 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.
> > > > Is this a real problem fix patch or just refine in theory? I have
> > > > quick test and saw a big performance downgrade below on
> > > > i.mx6sl-evk
> > > > board(1.6MB/s->706KB/s)
> > >
> > > This is a real problem. If you have two spi devices on the bus with
> > > need modes with different CPOL it can happen that after a transfer
> > > on one CPOL is changed for the other while SS is already active and
> > > this is already interpreted as the first latch.
> > >
> > > > root@imx6qpdlsolox:~# dd if=/dev/mtd0 of=/dev/null bs=1M count=1
> > > > 1+0 records in
> > > > 1+0 records out
> > > > 1048576 bytes (1.0 MB, 1.0 MiB) copied, 1.48557 s, 706 kB/s
> > >
> > > I didn't test this, but I'm surprised about that. For
> > > single-transfer messages the effect should just be that with the
> > > patch the following
> > > happens:
> > >
> > > 	setup_spi_imx_registers()
> > > 	activate_gpio_for_SS()
> > >
> > > instead of
> > >
> > > 	activate_gpio_for_SS()
> > > 	setup_spi_imx_registers()
> > Yes, in theory your patch should not cause such performance issue
> > since you just move some jobs of spi configure ahead of
> > activate_gpio_for_SS(). But unfortunately, MX51_ECSPI_CTRL_BL_OFFSET
> > may be broken by PIO transfer in case some small data, such as small
> command send to SPI-NOR before 1M data reading.
> > Please refer to dynamic_burst related code of spi_imx_push(). If I
> > keep BL setting in
> > mx51_ecspi_prepare_transfer() instead of mx51_ecspi_prepare_message(),
> > the issue is gone. Below is my fix, please merge it in V3.
> >
> > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> > 32d1c4f..05f9e50 100644
> > --- a/drivers/spi/spi-imx.c
> > +++ b/drivers/spi/spi-imx.c
> > @@ -510,13 +510,6 @@ static int mx51_ecspi_prepare_message(struct
> spi_imx_data *spi_imx,
> >         /* 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;
> > -       else
> > -               ctrl |= (spi_imx->bits_per_word - 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.
> > @@ -570,6 +563,15 @@ static int mx51_ecspi_prepare_transfer(struct
> spi_imx_data *spi_imx,
> >         u32 ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
> >         u32 clk = t->speed_hz, delay;
> >
> > +       /* Clear BL filed 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
> > +               ctrl |= (spi_imx->bits_per_word - 1)
> > +                       << MX51_ECSPI_CTRL_BL_OFFSET;
> > +
> 
> Even if I sent out v3 now, I wonder if the problem is just that I didn't clear
> MX51_ECSPI_CTRL_BL_MASK in mx51_ecspi_prepare_message()?
Yes, now V3 works on my side :), and I have other few comments. 
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&amp;data=02%7C01%7Cyibin.gong%40nxp.com%7Cf7233
> 64dd51541ee041608d656ac27ba%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C636791694728543175&amp;sdata=OLIzu5IDtKST6%2FRs8bbOiup
> MRJsXVG6mF7aH003XxJU%3D&amp;reserved=0  |
diff mbox series

Patch

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index c7db42d6b3bc..88a7a53441f5 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,10 +507,6 @@  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);
 
@@ -526,6 +517,19 @@  static int mx51_ecspi_config(struct spi_device *spi)
 		ctrl |= (spi_imx->bits_per_word - 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
+		testreg &= ~MX51_ECSPI_TESTREG_LBC;
+	writel(testreg, spi_imx->base + MX51_ECSPI_TESTREG);
+
 	/*
 	 * eCSPI burst completion by Chip Select signal in Slave mode
 	 * is not functional for imx53 Soc, config SPI burst completed when
@@ -548,26 +552,34 @@  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);
 
+	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
+
+	return 0;
+}
+
+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;
+
+	/* 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;
 
-	/* CTRL register always go first to bring out controller from reset */
 	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
 
-	reg = readl(spi_imx->base + MX51_ECSPI_TESTREG);
-	if (spi->mode & SPI_LOOP)
-		reg |= MX51_ECSPI_TESTREG_LBC;
-	else
-		reg &= ~MX51_ECSPI_TESTREG_LBC;
-	writel(reg, spi_imx->base + MX51_ECSPI_TESTREG);
-
-	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
-
 	/*
 	 * Wait until the changes in the configuration register CONFIGREG
 	 * propagate into the hardware. It takes exactly one tick of the
@@ -594,7 +606,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) |