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 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()

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                 | http://www.pengutronix.de/  |