diff mbox series

[v3,4/5] usb: musb: musb_cppi41: Workaround DMA stall issue during teardown

Message ID 20170119140835.18981-5-abailon@baylibre.com
State Superseded
Headers show
Series usb: musb: da8xx: Add DMA support | expand

Commit Message

Alexandre Bailon Jan. 19, 2017, 2:08 p.m. UTC 
The DMA may hung up if a teardown is initiated while an endpoint is still
active (Advisory 2.3.27 of DA8xx errata).
To workaround this issue, add a delay before to initiate the teardown.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

---
 drivers/usb/musb/da8xx.c       | 2 +-
 drivers/usb/musb/musb_core.h   | 1 +
 drivers/usb/musb/musb_cppi41.c | 4 ++++
 3 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Tony Lindgren Jan. 19, 2017, 4:56 p.m. UTC | #1 
* Alexandre Bailon <abailon@baylibre.com> [170119 06:09]:
> The DMA may hung up if a teardown is initiated while an endpoint is still

> active (Advisory 2.3.27 of DA8xx errata).

> To workaround this issue, add a delay before to initiate the teardown.

> 

> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

> ---

>  drivers/usb/musb/da8xx.c       | 2 +-

>  drivers/usb/musb/musb_core.h   | 1 +

>  drivers/usb/musb/musb_cppi41.c | 4 ++++

>  3 files changed, 6 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c

> index 5f50a78..8c9850c 100644

> --- a/drivers/usb/musb/da8xx.c

> +++ b/drivers/usb/musb/da8xx.c

> @@ -483,7 +483,7 @@ da8xx_dma_controller_create(struct musb *musb, void __iomem *base)

>  #endif

>  

>  static const struct musb_platform_ops da8xx_ops = {

> -	.quirks		= MUSB_INDEXED_EP | MUSB_DMA_CPPI41,

> +	.quirks		= MUSB_INDEXED_EP | MUSB_DMA_CPPI41 | MUSB_DA8XX,

>  	.init		= da8xx_musb_init,

>  	.exit		= da8xx_musb_exit,

>  

> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h

> index ade902e..d129278 100644

> --- a/drivers/usb/musb/musb_core.h

> +++ b/drivers/usb/musb/musb_core.h

> @@ -172,6 +172,7 @@ struct musb_io;

>   */

>  struct musb_platform_ops {

>  

> +#define MUSB_DA8XX		BIT(7)

>  #define MUSB_DMA_UX500		BIT(6)

>  #define MUSB_DMA_CPPI41		BIT(5)

>  #define MUSB_DMA_CPPI		BIT(4)

> diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c

> index 1fe7eae..d371d05 100644

> --- a/drivers/usb/musb/musb_cppi41.c

> +++ b/drivers/usb/musb/musb_cppi41.c

> @@ -554,6 +554,10 @@ static int cppi41_dma_channel_abort(struct dma_channel *channel)

>  		}

>  	}

>  

> +	/* DA8xx Advisory 2.3.27: wait 250 ms before to start the teardown */

> +	if (musb->io.quirks & MUSB_DA8XX)

> +		mdelay(250);

> +

>  	tdbit = 1 << cppi41_channel->port_num;

>  	if (is_tx)

>  		tdbit <<= 16;


How about replace the do while loop calling  dmaengine_terminate_all() with
deferred work doing that?

That way it's more generic as it seems there's no hurry doing that
after the musb registers are cleared. That would also allow getting rid of
the two current udelay() calls there that also seems nasty.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Bailon Jan. 20, 2017, 3:03 p.m. UTC | #2 
On 01/19/2017 05:56 PM, Tony Lindgren wrote:
> * Alexandre Bailon <abailon@baylibre.com> [170119 06:09]:

>> The DMA may hung up if a teardown is initiated while an endpoint is still

>> active (Advisory 2.3.27 of DA8xx errata).

>> To workaround this issue, add a delay before to initiate the teardown.

>>

>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

>> ---

>>  drivers/usb/musb/da8xx.c       | 2 +-

>>  drivers/usb/musb/musb_core.h   | 1 +

>>  drivers/usb/musb/musb_cppi41.c | 4 ++++

>>  3 files changed, 6 insertions(+), 1 deletion(-)

>>

>> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c

>> index 5f50a78..8c9850c 100644

>> --- a/drivers/usb/musb/da8xx.c

>> +++ b/drivers/usb/musb/da8xx.c

>> @@ -483,7 +483,7 @@ da8xx_dma_controller_create(struct musb *musb, void __iomem *base)

>>  #endif

>>  

>>  static const struct musb_platform_ops da8xx_ops = {

>> -	.quirks		= MUSB_INDEXED_EP | MUSB_DMA_CPPI41,

>> +	.quirks		= MUSB_INDEXED_EP | MUSB_DMA_CPPI41 | MUSB_DA8XX,

>>  	.init		= da8xx_musb_init,

>>  	.exit		= da8xx_musb_exit,

>>  

>> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h

>> index ade902e..d129278 100644

>> --- a/drivers/usb/musb/musb_core.h

>> +++ b/drivers/usb/musb/musb_core.h

>> @@ -172,6 +172,7 @@ struct musb_io;

>>   */

>>  struct musb_platform_ops {

>>  

>> +#define MUSB_DA8XX		BIT(7)

>>  #define MUSB_DMA_UX500		BIT(6)

>>  #define MUSB_DMA_CPPI41		BIT(5)

>>  #define MUSB_DMA_CPPI		BIT(4)

>> diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c

>> index 1fe7eae..d371d05 100644

>> --- a/drivers/usb/musb/musb_cppi41.c

>> +++ b/drivers/usb/musb/musb_cppi41.c

>> @@ -554,6 +554,10 @@ static int cppi41_dma_channel_abort(struct dma_channel *channel)

>>  		}

>>  	}

>>  

>> +	/* DA8xx Advisory 2.3.27: wait 250 ms before to start the teardown */

>> +	if (musb->io.quirks & MUSB_DA8XX)

>> +		mdelay(250);

>> +

>>  	tdbit = 1 << cppi41_channel->port_num;

>>  	if (is_tx)

>>  		tdbit <<= 16;

> 

> How about replace the do while loop calling  dmaengine_terminate_all() with

> deferred work doing that?

That would be great but I don't know how hard to would be to do it.
Right after the teardown, we may have to queue a new request.
But we must be sure that the teardown has completed before to start
a new transfer. And because it happen in atomic section, we must also
defer it.
> 

> That way it's more generic as it seems there's no hurry doing that

> after the musb registers are cleared. That would also allow getting rid of

> the two current udelay() calls there that also seems nasty.

> 

> Regards,

> 

> Tony

> 

Regards,
Alexandre
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bin Liu Jan. 20, 2017, 5:23 p.m. UTC | #3 
On Thu, Jan 19, 2017 at 08:56:46AM -0800, Tony Lindgren wrote:
> * Alexandre Bailon <abailon@baylibre.com> [170119 06:09]:

> > The DMA may hung up if a teardown is initiated while an endpoint is still

> > active (Advisory 2.3.27 of DA8xx errata).

> > To workaround this issue, add a delay before to initiate the teardown.

> > 

> > Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

> > ---

> >  drivers/usb/musb/da8xx.c       | 2 +-

> >  drivers/usb/musb/musb_core.h   | 1 +

> >  drivers/usb/musb/musb_cppi41.c | 4 ++++

> >  3 files changed, 6 insertions(+), 1 deletion(-)

> > 

> > diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c

> > index 5f50a78..8c9850c 100644

> > --- a/drivers/usb/musb/da8xx.c

> > +++ b/drivers/usb/musb/da8xx.c

> > @@ -483,7 +483,7 @@ da8xx_dma_controller_create(struct musb *musb, void __iomem *base)

> >  #endif

> >  

> >  static const struct musb_platform_ops da8xx_ops = {

> > -	.quirks		= MUSB_INDEXED_EP | MUSB_DMA_CPPI41,

> > +	.quirks		= MUSB_INDEXED_EP | MUSB_DMA_CPPI41 | MUSB_DA8XX,

> >  	.init		= da8xx_musb_init,

> >  	.exit		= da8xx_musb_exit,

> >  

> > diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h

> > index ade902e..d129278 100644

> > --- a/drivers/usb/musb/musb_core.h

> > +++ b/drivers/usb/musb/musb_core.h

> > @@ -172,6 +172,7 @@ struct musb_io;

> >   */

> >  struct musb_platform_ops {

> >  

> > +#define MUSB_DA8XX		BIT(7)

> >  #define MUSB_DMA_UX500		BIT(6)

> >  #define MUSB_DMA_CPPI41		BIT(5)

> >  #define MUSB_DMA_CPPI		BIT(4)

> > diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c

> > index 1fe7eae..d371d05 100644

> > --- a/drivers/usb/musb/musb_cppi41.c

> > +++ b/drivers/usb/musb/musb_cppi41.c

> > @@ -554,6 +554,10 @@ static int cppi41_dma_channel_abort(struct dma_channel *channel)

> >  		}

> >  	}

> >  

> > +	/* DA8xx Advisory 2.3.27: wait 250 ms before to start the teardown */

> > +	if (musb->io.quirks & MUSB_DA8XX)

> > +		mdelay(250);

> > +

> >  	tdbit = 1 << cppi41_channel->port_num;

> >  	if (is_tx)

> >  		tdbit <<= 16;

> 

> How about replace the do while loop calling  dmaengine_terminate_all() with

> deferred work doing that?


It would be great, but it seems a little hard to implement. If the
_abort is initiated by application due to transfer timeout, the
application might immediately retry the transfer, then the driver would
have to ensure the deferred work is done and the channel is free by that
time.

I guess we have to evaluate the impact of the do-while loop and the
performance impact of the deferred work to know if it is worth it.

> 

> That way it's more generic as it seems there's no hurry doing that

> after the musb registers are cleared. That would also allow getting rid of

> the two current udelay() calls there that also seems nasty.


I don't think replacing the do-while loop will allow to get rid of the
two udelay() calls, which are there as barriers for the register changes
- clearing MUSB_RXCSR_DMAENAB and flushing FIFO.

Regards,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren Jan. 20, 2017, 6:45 p.m. UTC | #4 
* Bin Liu <b-liu@ti.com> [170120 09:24]:
> On Thu, Jan 19, 2017 at 08:56:46AM -0800, Tony Lindgren wrote:

> > * Alexandre Bailon <abailon@baylibre.com> [170119 06:09]:

> > > The DMA may hung up if a teardown is initiated while an endpoint is still

> > > active (Advisory 2.3.27 of DA8xx errata).

> > > To workaround this issue, add a delay before to initiate the teardown.

> > > 

> > > Signed-off-by: Alexandre Bailon <abailon@baylibre.com>

> > > ---

> > >  drivers/usb/musb/da8xx.c       | 2 +-

> > >  drivers/usb/musb/musb_core.h   | 1 +

> > >  drivers/usb/musb/musb_cppi41.c | 4 ++++

> > >  3 files changed, 6 insertions(+), 1 deletion(-)

> > > 

> > > diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c

> > > index 5f50a78..8c9850c 100644

> > > --- a/drivers/usb/musb/da8xx.c

> > > +++ b/drivers/usb/musb/da8xx.c

> > > @@ -483,7 +483,7 @@ da8xx_dma_controller_create(struct musb *musb, void __iomem *base)

> > >  #endif

> > >  

> > >  static const struct musb_platform_ops da8xx_ops = {

> > > -	.quirks		= MUSB_INDEXED_EP | MUSB_DMA_CPPI41,

> > > +	.quirks		= MUSB_INDEXED_EP | MUSB_DMA_CPPI41 | MUSB_DA8XX,

> > >  	.init		= da8xx_musb_init,

> > >  	.exit		= da8xx_musb_exit,

> > >  

> > > diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h

> > > index ade902e..d129278 100644

> > > --- a/drivers/usb/musb/musb_core.h

> > > +++ b/drivers/usb/musb/musb_core.h

> > > @@ -172,6 +172,7 @@ struct musb_io;

> > >   */

> > >  struct musb_platform_ops {

> > >  

> > > +#define MUSB_DA8XX		BIT(7)

> > >  #define MUSB_DMA_UX500		BIT(6)

> > >  #define MUSB_DMA_CPPI41		BIT(5)

> > >  #define MUSB_DMA_CPPI		BIT(4)

> > > diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c

> > > index 1fe7eae..d371d05 100644

> > > --- a/drivers/usb/musb/musb_cppi41.c

> > > +++ b/drivers/usb/musb/musb_cppi41.c

> > > @@ -554,6 +554,10 @@ static int cppi41_dma_channel_abort(struct dma_channel *channel)

> > >  		}

> > >  	}

> > >  

> > > +	/* DA8xx Advisory 2.3.27: wait 250 ms before to start the teardown */

> > > +	if (musb->io.quirks & MUSB_DA8XX)

> > > +		mdelay(250);

> > > +

> > >  	tdbit = 1 << cppi41_channel->port_num;

> > >  	if (is_tx)

> > >  		tdbit <<= 16;

> > 

> > How about replace the do while loop calling  dmaengine_terminate_all() with

> > deferred work doing that?

> 

> It would be great, but it seems a little hard to implement. If the

> _abort is initiated by application due to transfer timeout, the

> application might immediately retry the transfer, then the driver would

> have to ensure the deferred work is done and the channel is free by that

> time.

> 

> I guess we have to evaluate the impact of the do-while loop and the

> performance impact of the deferred work to know if it is worth it.

> 

> > 

> > That way it's more generic as it seems there's no hurry doing that

> > after the musb registers are cleared. That would also allow getting rid of

> > the two current udelay() calls there that also seems nasty.

> 

> I don't think replacing the do-while loop will allow to get rid of the

> two udelay() calls, which are there as barriers for the register changes

> - clearing MUSB_RXCSR_DMAENAB and flushing FIFO.


OK fine with me.

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index 5f50a78..8c9850c 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -483,7 +483,7 @@  da8xx_dma_controller_create(struct musb *musb, void __iomem *base)
 #endif
 
 static const struct musb_platform_ops da8xx_ops = {
-	.quirks		= MUSB_INDEXED_EP | MUSB_DMA_CPPI41,
+	.quirks		= MUSB_INDEXED_EP | MUSB_DMA_CPPI41 | MUSB_DA8XX,
 	.init		= da8xx_musb_init,
 	.exit		= da8xx_musb_exit,
 
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index ade902e..d129278 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -172,6 +172,7 @@  struct musb_io;
  */
 struct musb_platform_ops {
 
+#define MUSB_DA8XX		BIT(7)
 #define MUSB_DMA_UX500		BIT(6)
 #define MUSB_DMA_CPPI41		BIT(5)
 #define MUSB_DMA_CPPI		BIT(4)
diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index 1fe7eae..d371d05 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -554,6 +554,10 @@  static int cppi41_dma_channel_abort(struct dma_channel *channel)
 		}
 	}
 
+	/* DA8xx Advisory 2.3.27: wait 250 ms before to start the teardown */
+	if (musb->io.quirks & MUSB_DA8XX)
+		mdelay(250);
+
 	tdbit = 1 << cppi41_channel->port_num;
 	if (is_tx)
 		tdbit <<= 16;