diff mbox series

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

Message ID 1589218356-17475-8-git-send-email-yibin.gong@nxp.com (mailing list archive)
State New, archived
Headers show
Series add ecspi ERR009165 for i.mx6/7 soc family | expand

Commit Message

Robin Gong May 11, 2020, 5:32 p.m. UTC 
Change to XCH  mode even in dma mode, please refer to the below
errata:
https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf

Signed-off-by: Robin Gong <yibin.gong@nxp.com>
Acked-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-imx.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Sascha Hauer May 13, 2020, 7:21 a.m. UTC | #1 
On Tue, May 12, 2020 at 01:32:30AM +0800, Robin Gong wrote:
> Change to XCH  mode even in dma mode, please refer to the below
> errata:
> https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf

This patch is the one bisecting will end up with when somebody uses an
older SDMA firmware or the ROM scripts. It should have a better
description what happens and what should be done about it.

Sascha

> 
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> ---
>  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;
>  	ret = dmaengine_slave_config(master->dma_tx, &tx);
>  	if (ret) {
>  		dev_err(spi_imx->dev, "TX dma configuration failed with %d\n", ret);
> @@ -1265,10 +1269,6 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
>  {
>  	int ret;
>  
> -	/* use pio mode for i.mx6dl chip TKT238285 */
> -	if (of_machine_is_compatible("fsl,imx6dl"))
> -		return 0;
> -
>  	spi_imx->wml = spi_imx->devtype_data->fifo_size / 2;
>  
>  	/* Prepare for TX DMA: */
> -- 
> 2.7.4
> 
>
Sascha Hauer May 13, 2020, 7:33 a.m. UTC | #2 
On Tue, May 12, 2020 at 01:32:30AM +0800, Robin Gong wrote:
> Change to XCH  mode even in dma mode, please refer to the below
> errata:
> https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf
> 
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> ---
>  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.

Sascha
Robin Gong May 13, 2020, 8:38 a.m. UTC | #3 
On 2020/05/13 Sascha Hauer <s.hauer@pengutronix.de> wrote:
> This patch is the one bisecting will end up with when somebody uses an older
> SDMA firmware or the ROM scripts. It should have a better description what
> happens and what should be done about it.
Emm..That's true. Timeout will be caught in such case, hence, maybe we can fall back it to pio always.
> >
> > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > Acked-by: Mark Brown <broonie@kernel.org>
> > ---
> >  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;
> >  	ret = dmaengine_slave_config(master->dma_tx, &tx);
> >  	if (ret) {
> >  		dev_err(spi_imx->dev, "TX dma configuration failed with %d\n",
> > ret); @@ -1265,10 +1269,6 @@ static int spi_imx_sdma_init(struct
> > device *dev, struct spi_imx_data *spi_imx,  {
> >  	int ret;
> >
> > -	/* use pio mode for i.mx6dl chip TKT238285 */
> > -	if (of_machine_is_compatible("fsl,imx6dl"))
> > -		return 0;
> > -
> >  	spi_imx->wml = spi_imx->devtype_data->fifo_size / 2;
> >
> >  	/* Prepare for TX DMA: */
> > --
> > 2.7.4
> >
> >
> 
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pe
> ngutronix.de%2F&amp;data=02%7C01%7Cyibin.gong%40nxp.com%7C2f49309
> 819cc4c45418108d7f70e46fb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> 7C1%7C637249513003506970&amp;sdata=RoLVnDaCfG20i88OmmlpbMH6lZu
> qqW2CJv4VSSDkPcM%3D&amp;reserved=0  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |
Sascha Hauer May 13, 2020, 8:48 a.m. UTC | #4 
On Wed, May 13, 2020 at 08:38:26AM +0000, Robin Gong wrote:
> On 2020/05/13 Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > This patch is the one bisecting will end up with when somebody uses an older
> > SDMA firmware or the ROM scripts. It should have a better description what
> > happens and what should be done about it.
> Emm..That's true. Timeout will be caught in such case, hence, maybe we can fall back it to pio always.

With my patch applied sdma_load_context() will fail. I don't know how
exactly this hits into the SPI driver, but it won't be a timeout.

Sascha

> > >
> > > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > > Acked-by: Mark Brown <broonie@kernel.org>
> > > ---
> > >  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;
> > >  	ret = dmaengine_slave_config(master->dma_tx, &tx);
> > >  	if (ret) {
> > >  		dev_err(spi_imx->dev, "TX dma configuration failed with %d\n",
> > > ret); @@ -1265,10 +1269,6 @@ static int spi_imx_sdma_init(struct
> > > device *dev, struct spi_imx_data *spi_imx,  {
> > >  	int ret;
> > >
> > > -	/* use pio mode for i.mx6dl chip TKT238285 */
> > > -	if (of_machine_is_compatible("fsl,imx6dl"))
> > > -		return 0;
> > > -
> > >  	spi_imx->wml = spi_imx->devtype_data->fifo_size / 2;
> > >
> > >  	/* Prepare for TX DMA: */
> > > --
> > > 2.7.4
> > >
> > >
> > 
> > --
> > Pengutronix e.K.                           |
> > |
> > Steuerwalder Str. 21                       |
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pe
> > ngutronix.de%2F&amp;data=02%7C01%7Cyibin.gong%40nxp.com%7C2f49309
> > 819cc4c45418108d7f70e46fb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> > 7C1%7C637249513003506970&amp;sdata=RoLVnDaCfG20i88OmmlpbMH6lZu
> > qqW2CJv4VSSDkPcM%3D&amp;reserved=0  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0
> > |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:
> > +49-5121-206917-5555 |
>
Robin Gong May 13, 2020, 8:52 a.m. UTC | #5 
On 2020/05/13 16:48 Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, May 13, 2020 at 08:38:26AM +0000, Robin Gong wrote:
> > On 2020/05/13 Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > This patch is the one bisecting will end up with when somebody uses
> > > an older SDMA firmware or the ROM scripts. It should have a better
> > > description what happens and what should be done about it.
> > Emm..That's true. Timeout will be caught in such case, hence, maybe we can
> fall back it to pio always.
> 
> With my patch applied sdma_load_context() will fail. I don't know how exactly
> this hits into the SPI driver, but it won't be a timeout.
Thanks for your quick test, assume you use ROM firmware, right?
Robin Gong May 13, 2020, 9:05 a.m. UTC | #6 
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.
Sascha Hauer May 13, 2020, 9:20 a.m. UTC | #7 
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
Sascha Hauer May 13, 2020, 9:20 a.m. UTC | #8 
On Wed, May 13, 2020 at 08:52:39AM +0000, Robin Gong wrote:
> On 2020/05/13 16:48 Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Wed, May 13, 2020 at 08:38:26AM +0000, Robin Gong wrote:
> > > On 2020/05/13 Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > > This patch is the one bisecting will end up with when somebody uses
> > > > an older SDMA firmware or the ROM scripts. It should have a better
> > > > description what happens and what should be done about it.
> > > Emm..That's true. Timeout will be caught in such case, hence, maybe we can
> > fall back it to pio always.
> > 
> > With my patch applied sdma_load_context() will fail. I don't know how exactly
> > this hits into the SPI driver, but it won't be a timeout.
> Thanks for your quick test, assume you use ROM firmware, right?

Yes.

Sascha
Robin Gong May 13, 2020, 9:36 a.m. UTC | #9 
On 2020/05/13 Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 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"
> 
Okay, I'll merge those 2 changes into the next 08/13.
Robin Gong May 13, 2020, 3:48 p.m. UTC | #10 
On 2020/05/13 Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, May 13, 2020 at 08:52:39AM +0000, Robin Gong wrote:
> > On 2020/05/13 16:48 Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > On Wed, May 13, 2020 at 08:38:26AM +0000, Robin Gong wrote:
> > > > On 2020/05/13 Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > > > This patch is the one bisecting will end up with when somebody
> > > > > uses an older SDMA firmware or the ROM scripts. It should have a
> > > > > better description what happens and what should be done about it.
> > > > Emm..That's true. Timeout will be caught in such case, hence,
> > > > maybe we can
> > > fall back it to pio always.
> > >
> > > With my patch applied sdma_load_context() will fail. I don't know
> > > how exactly this hits into the SPI driver, but it won't be a timeout.
> > Thanks for your quick test, assume you use ROM firmware, right?
> 
> Yes.
Would you please have a try with the attached patch which is based this patch set?
diff mbox series

Patch

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;
 	ret = dmaengine_slave_config(master->dma_tx, &tx);
 	if (ret) {
 		dev_err(spi_imx->dev, "TX dma configuration failed with %d\n", ret);
@@ -1265,10 +1269,6 @@  static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
 {
 	int ret;
 
-	/* use pio mode for i.mx6dl chip TKT238285 */
-	if (of_machine_is_compatible("fsl,imx6dl"))
-		return 0;
-
 	spi_imx->wml = spi_imx->devtype_data->fifo_size / 2;
 
 	/* Prepare for TX DMA: */