On Wed, May 13, 2020 at 09:05:33AM +0000, Robin Gong wrote:

On 2020/05/13 Sascha Hauer [off-list ref] wrote:d

quoted

quoted

 drivers/spi/spi-imx.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index

f4f28a4..70df8e6 100644

--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -585,8 +585,8 @@ static int mx51_ecspi_prepare_transfer(struct

spi_imx_data *spi_imx,

quoted

 	ctrl |= mx51_ecspi_clkdiv(spi_imx, t->speed_hz, &clk);
 	spi_imx->spi_bus_clk = clk;

-	if (spi_imx->usedma)
-		ctrl |= MX51_ECSPI_CTRL_SMC;
+	/* ERR009165: work in XHC mode as PIO */
+	ctrl &= ~MX51_ECSPI_CTRL_SMC;

 	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);

@@ -617,7 +617,7 @@ static void mx51_setup_wml(struct spi_imx_data

*spi_imx)

quoted

 	 * 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_TX_WML(0) |
 		MX51_ECSPI_DMA_RXT_WML(spi_imx->wml) |
 		MX51_ECSPI_DMA_TEDEN | MX51_ECSPI_DMA_RXDEN |
 		MX51_ECSPI_DMA_RXTDEN, spi_imx->base + MX51_ECSPI_DMA);

@@ -1171,7

quoted

+1171,11 @@ static int spi_imx_dma_configure(struct spi_master *master)
 	tx.direction = DMA_MEM_TO_DEV;
 	tx.dst_addr = spi_imx->base_phys + MXC_CSPITXDATA;
 	tx.dst_addr_width = buswidth;
-	tx.dst_maxburst = spi_imx->wml;
+	/*
+	 * For ERR009165 with tx_wml = 0 could enlarge burst size to fifo size
+	 * to speed up fifo filling as possible.
+	 */
+	tx.dst_maxburst = spi_imx->devtype_data->fifo_size;

In the next patch this is changed again to:

+       if (spi_imx->devtype_data->tx_glitch_fixed)
+               tx.dst_maxburst = spi_imx->wml;
+       else
+               tx.dst_maxburst = spi_imx->devtype_data->fifo_size;

So with tx_glitch_fixed we end up with tx.dst_maxburst being the same as two
patches before which is rather confusing. Better introduce tx_glitch_fixed in
this patch, or maybe even merge this patch and the next one.

Sorry confused you, I should repleace 'tx_wml=0' in the above comments
with ' TX_THRESHOLD=0', which means tx transfer dma have to wait all
the tx data in tx fifo transferred with ERR009165 rather than
generically 'tx_wml' (for example --half fifo size used as
TX_THRESHOLD). Obviously TX_THRESHOLD=0 would down performance, so
enlarge dst_maxburst to fifo size as PIO with ERR009165. After
ERR009165 fixed at HW level. TX_THRESHOLD could be used as common
'spi_imx->wml' so change it back. Will add more detail information in
v8.

I am not confused, I meant the patches are confusing. What you are doing
is:

No patch:
	tx.dst_maxburst = a;

1st patch
	tx.dst_maxburst = b;

2nd patch:

	if (foo)
		tx.dst_maxburst = a;
	else
		tx.dst_maxburst = b;

It would be better readable and understandable if you did that in one
patch, because that would directly say "Under certain conditions we have
to choose a, otherwise b". That's much better than changing "a" to "b" and
then to "a or b"

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |