Re: [PATCH v7 RESEND 07/13] spi: imx: fix ERR009165

[Sascha Hauer <s.hauer@pengutronix.de>]

On Wed, May 13, 2020 at 09:05:33AM +0000, Robin Gong wrote:
> On 2020/05/13 Sascha Hauer <s.hauer@pengutronix.de> wrote:d
> > >  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,
> > >  	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)
> > >  	 * 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
> > > +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 |