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

[Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>]

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@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;
> +

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |