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

[Robin Gong <yibin.gong@xxxxxxx>]

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> Sent: 2018年11月26日 17:18
> To: Robin Gong <yibin.gong@xxxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxx>; Marek Vasut <marex@xxxxxxx>;
> dl-linux-imx <linux-imx@xxxxxxx>; kernel@xxxxxxxxxxxxxx;
> linux-spi@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxx>
> > > Sent: 2018年11月23日 16:52
> > > To: Mark Brown <broonie@xxxxxxxxxx>; Robin Gong
> <yibin.gong@xxxxxxx>
> > > Cc: Marek Vasut <marex@xxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx>;
> > > kernel@xxxxxxxxxxxxxx; linux-spi@xxxxxxxxxxxxxxx
> > > 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  |