[PATCH 1/2] dmaengine: ep93xx: Always start from BASE0

[PATCH 1/2] dmaengine: ep93xx: Always start from BASE0

[Alexander Sverdlin]

The current buffer is being reset to zero on device_free_chan_resources()
but not on device_terminate_all(). It could happen that HW is restarted and
expects BASE0 to be used, but the driver is not synchronized and will start
from BASE1. Therefore, it's better to reset the buffer explicitly in
m2p_hw_setup(). While here, check that HW really starts from BASE0 and
warn otherwise.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
Cc: stable@vger.kernel.org
---
 drivers/dma/ep93xx_dma.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
index d37e8dda8079..a3d946d2a8e1 100644
--- a/drivers/dma/ep93xx_dma.c
+++ b/drivers/dma/ep93xx_dma.c
@@ -45,6 +45,7 @@
 
 #define M2P_PPALLOC			0x0008
 #define M2P_STATUS			0x000c
+#define M2P_STATUS_NEXTBUFFER		BIT(6)
 
 #define M2P_MAXCNT0			0x0020
 #define M2P_BASE0			0x0024
@@ -312,6 +313,13 @@ static void m2p_set_control(struct ep93xx_dma_chan *edmac, u32 control)
 	readl(edmac->regs + M2P_CONTROL);
 }
 
+static inline u32 m2p_channel_nextbuf(struct ep93xx_dma_chan *edmac)
+{
+	if (readl(edmac->regs + M2P_STATUS) & M2P_STATUS_NEXTBUFFER)
+		return 1;
+	return 0;
+}
+
 static int m2p_hw_setup(struct ep93xx_dma_chan *edmac)
 {
 	struct ep93xx_dma_data *data = edmac->chan.private;
@@ -323,6 +331,10 @@ static int m2p_hw_setup(struct ep93xx_dma_chan *edmac)
 		| M2P_CONTROL_ENABLE;
 	m2p_set_control(edmac, control);
 
+	if (m2p_channel_nextbuf(edmac) != 0)
+		dev_warn(chan2dev(edmac), "M2P: Starting from BASE1\n");
+	edmac->buffer = 0;
+
 	return 0;
 }
 
-- 
2.12.2

[PATCH 2/2] dmaengine: ep93xx: Don't drain the transfers in terminate_all()

[Alexander Sverdlin]

As dmaengine.h suggests, device_terminate_all() "Aborts all transfers on a
channel". Seems that no other driver tried to busy-wait and actually drain
the data. Moreover, this happens with IRQs disabled, therefore induces huge
latency:

 irqsoff latency trace v1.1.5 on 4.11.0
 --------------------------------------------------------------------
 latency: 39770 us, #57/57, CPU#0 | (M:preempt VP:0, KP:0, SP:0 HP:0)
    -----------------
    | task: process-129 (uid:0 nice:0 policy:2 rt_prio:50)
    -----------------
  => started at: _snd_pcm_stream_lock_irqsave
  => ended at:   snd_pcm_stream_unlock_irqrestore

                  _------=> CPU#
                 / _-----=> irqs-off
                | / _----=> need-resched
                || / _---=> hardirq/softirq
                ||| / _--=> preempt-depth
                |||| /     delay
  cmd     pid   ||||| time  |   caller
     \   /      |||||  \    |   /
process-129     0d.s.    3us : _snd_pcm_stream_lock_irqsave
process-129     0d.s1    9us : snd_pcm_stream_lock <-_snd_pcm_stream_lock_irqsave
process-129     0d.s1   15us : preempt_count_add <-snd_pcm_stream_lock
process-129     0d.s2   22us : preempt_count_add <-snd_pcm_stream_lock
process-129     0d.s3   32us : snd_pcm_update_hw_ptr0 <-snd_pcm_period_elapsed
process-129     0d.s3   41us : soc_pcm_pointer <-snd_pcm_update_hw_ptr0
process-129     0d.s3   50us : dmaengine_pcm_pointer <-soc_pcm_pointer
process-129     0d.s3   58us+: snd_dmaengine_pcm_pointer_no_residue <-dmaengine_pcm_pointer
process-129     0d.s3   96us : update_audio_tstamp <-snd_pcm_update_hw_ptr0
process-129     0d.s3  103us : snd_pcm_update_state <-snd_pcm_update_hw_ptr0
process-129     0d.s3  112us : xrun <-snd_pcm_update_state
process-129     0d.s3  119us : snd_pcm_stop <-xrun
process-129     0d.s3  126us : snd_pcm_action <-snd_pcm_stop
process-129     0d.s3  134us : snd_pcm_action_single <-snd_pcm_action
process-129     0d.s3  141us : snd_pcm_pre_stop <-snd_pcm_action_single
process-129     0d.s3  150us : snd_pcm_do_stop <-snd_pcm_action_single
process-129     0d.s3  157us : soc_pcm_trigger <-snd_pcm_do_stop
process-129     0d.s3  166us : snd_dmaengine_pcm_trigger <-soc_pcm_trigger
process-129     0d.s3  175us : ep93xx_dma_terminate_all <-snd_dmaengine_pcm_trigger
process-129     0d.s3  182us : preempt_count_add <-ep93xx_dma_terminate_all
process-129     0d.s4  189us*: m2p_hw_shutdown <-ep93xx_dma_terminate_all
process-129     0d.s4 39472us : m2p_hw_setup <-ep93xx_dma_terminate_all

 ... rest skipped...

process-129     0d.s. 40080us : <stack trace>
 => ep93xx_dma_tasklet
 => tasklet_action
 => __do_softirq
 => irq_exit
 => __handle_domain_irq
 => vic_handle_irq
 => __irq_usr
 => 0xb66c6668

Just abort the transfers and warn if the HW state is not what we expect.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
Cc: stable@vger.kernel.org
---
 drivers/dma/ep93xx_dma.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
index a3d946d2a8e1..e78d1fbf2179 100644
--- a/drivers/dma/ep93xx_dma.c
+++ b/drivers/dma/ep93xx_dma.c
@@ -345,19 +345,10 @@ static inline u32 m2p_channel_state(struct ep93xx_dma_chan *edmac)
 
 static void m2p_hw_shutdown(struct ep93xx_dma_chan *edmac)
 {
-	u32 control;
-
-	control = readl(edmac->regs + M2P_CONTROL);
-	control &= ~(M2P_CONTROL_STALLINT | M2P_CONTROL_NFBINT);
-	m2p_set_control(edmac, control);
-
-	while (m2p_channel_state(edmac) >= M2P_STATE_ON)
-		cpu_relax();
-
 	m2p_set_control(edmac, 0);
 
-	while (m2p_channel_state(edmac) == M2P_STATE_STALL)
-		cpu_relax();
+	while (m2p_channel_state(edmac) != M2P_STATE_IDLE)
+		dev_warn(chan2dev(edmac), "M2P: Not yet IDLE\n");
 }
 
 static void m2p_fill_desc(struct ep93xx_dma_chan *edmac)
-- 
2.12.2

Re: [PATCH 1/2] dmaengine: ep93xx: Always start from BASE0

[Vinod Koul]

On Sun, May 14, 2017 at 12:13:26AM +0200, Alexander Sverdlin wrote:
> The current buffer is being reset to zero on device_free_chan_resources()
> but not on device_terminate_all(). It could happen that HW is restarted and
> expects BASE0 to be used, but the driver is not synchronized and will start
> from BASE1. Therefore, it's better to reset the buffer explicitly in
> m2p_hw_setup(). While here, check that HW really starts from BASE0 and
> warn otherwise.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/dma/ep93xx_dma.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
> index d37e8dda8079..a3d946d2a8e1 100644
> --- a/drivers/dma/ep93xx_dma.c
> +++ b/drivers/dma/ep93xx_dma.c
> @@ -45,6 +45,7 @@
>  
>  #define M2P_PPALLOC			0x0008
>  #define M2P_STATUS			0x000c
> +#define M2P_STATUS_NEXTBUFFER		BIT(6)
>  
>  #define M2P_MAXCNT0			0x0020
>  #define M2P_BASE0			0x0024
> @@ -312,6 +313,13 @@ static void m2p_set_control(struct ep93xx_dma_chan *edmac, u32 control)
>  	readl(edmac->regs + M2P_CONTROL);
>  }
>  
> +static inline u32 m2p_channel_nextbuf(struct ep93xx_dma_chan *edmac)
> +{
> +	if (readl(edmac->regs + M2P_STATUS) & M2P_STATUS_NEXTBUFFER)
> +		return 1;
> +	return 0;
> +}
> +
>  static int m2p_hw_setup(struct ep93xx_dma_chan *edmac)
>  {
>  	struct ep93xx_dma_data *data = edmac->chan.private;
> @@ -323,6 +331,10 @@ static int m2p_hw_setup(struct ep93xx_dma_chan *edmac)
>  		| M2P_CONTROL_ENABLE;
>  	m2p_set_control(edmac, control);
>  
> +	if (m2p_channel_nextbuf(edmac) != 0)
> +		dev_warn(chan2dev(edmac), "M2P: Starting from BASE1\n");

But then you are actually not restarting from BASE1 as you reset, so the
warn is wrong.. Perhaps a more meaningful msg would be to say "expected 0
but found 1, so resetting"

> +	edmac->buffer = 0;
> +
>  	return 0;
>  }
>  
> -- 
> 2.12.2
> 

-- 
~Vinod

Re: [PATCH 2/2] dmaengine: ep93xx: Don't drain the transfers in terminate_all()

[Vinod Koul]

On Sun, May 14, 2017 at 12:13:27AM +0200, Alexander Sverdlin wrote:
> As dmaengine.h suggests, device_terminate_all() "Aborts all transfers on a
> channel". Seems that no other driver tried to busy-wait and actually drain
> the data. Moreover, this happens with IRQs disabled, therefore induces huge
> latency:

Perhaps you should move current code to device_synchronize callback to allow
people to do so, if desired.

> 
>  irqsoff latency trace v1.1.5 on 4.11.0
>  --------------------------------------------------------------------
>  latency: 39770 us, #57/57, CPU#0 | (M:preempt VP:0, KP:0, SP:0 HP:0)
>     -----------------
>     | task: process-129 (uid:0 nice:0 policy:2 rt_prio:50)
>     -----------------
>   => started at: _snd_pcm_stream_lock_irqsave
>   => ended at:   snd_pcm_stream_unlock_irqrestore
> 
>                   _------=> CPU#
>                  / _-----=> irqs-off
>                 | / _----=> need-resched
>                 || / _---=> hardirq/softirq
>                 ||| / _--=> preempt-depth
>                 |||| /     delay
>   cmd     pid   ||||| time  |   caller
>      \   /      |||||  \    |   /
> process-129     0d.s.    3us : _snd_pcm_stream_lock_irqsave
> process-129     0d.s1    9us : snd_pcm_stream_lock <-_snd_pcm_stream_lock_irqsave
> process-129     0d.s1   15us : preempt_count_add <-snd_pcm_stream_lock
> process-129     0d.s2   22us : preempt_count_add <-snd_pcm_stream_lock
> process-129     0d.s3   32us : snd_pcm_update_hw_ptr0 <-snd_pcm_period_elapsed
> process-129     0d.s3   41us : soc_pcm_pointer <-snd_pcm_update_hw_ptr0
> process-129     0d.s3   50us : dmaengine_pcm_pointer <-soc_pcm_pointer
> process-129     0d.s3   58us+: snd_dmaengine_pcm_pointer_no_residue <-dmaengine_pcm_pointer
> process-129     0d.s3   96us : update_audio_tstamp <-snd_pcm_update_hw_ptr0
> process-129     0d.s3  103us : snd_pcm_update_state <-snd_pcm_update_hw_ptr0
> process-129     0d.s3  112us : xrun <-snd_pcm_update_state
> process-129     0d.s3  119us : snd_pcm_stop <-xrun
> process-129     0d.s3  126us : snd_pcm_action <-snd_pcm_stop
> process-129     0d.s3  134us : snd_pcm_action_single <-snd_pcm_action
> process-129     0d.s3  141us : snd_pcm_pre_stop <-snd_pcm_action_single
> process-129     0d.s3  150us : snd_pcm_do_stop <-snd_pcm_action_single
> process-129     0d.s3  157us : soc_pcm_trigger <-snd_pcm_do_stop
> process-129     0d.s3  166us : snd_dmaengine_pcm_trigger <-soc_pcm_trigger
> process-129     0d.s3  175us : ep93xx_dma_terminate_all <-snd_dmaengine_pcm_trigger
> process-129     0d.s3  182us : preempt_count_add <-ep93xx_dma_terminate_all
> process-129     0d.s4  189us*: m2p_hw_shutdown <-ep93xx_dma_terminate_all
> process-129     0d.s4 39472us : m2p_hw_setup <-ep93xx_dma_terminate_all
> 
>  ... rest skipped...
> 
> process-129     0d.s. 40080us : <stack trace>
>  => ep93xx_dma_tasklet
>  => tasklet_action
>  => __do_softirq
>  => irq_exit
>  => __handle_domain_irq
>  => vic_handle_irq
>  => __irq_usr
>  => 0xb66c6668
> 
> Just abort the transfers and warn if the HW state is not what we expect.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/dma/ep93xx_dma.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
> index a3d946d2a8e1..e78d1fbf2179 100644
> --- a/drivers/dma/ep93xx_dma.c
> +++ b/drivers/dma/ep93xx_dma.c
> @@ -345,19 +345,10 @@ static inline u32 m2p_channel_state(struct ep93xx_dma_chan *edmac)
>  
>  static void m2p_hw_shutdown(struct ep93xx_dma_chan *edmac)
>  {
> -	u32 control;
> -
> -	control = readl(edmac->regs + M2P_CONTROL);
> -	control &= ~(M2P_CONTROL_STALLINT | M2P_CONTROL_NFBINT);
> -	m2p_set_control(edmac, control);
> -
> -	while (m2p_channel_state(edmac) >= M2P_STATE_ON)
> -		cpu_relax();
> -
>  	m2p_set_control(edmac, 0);
>  
> -	while (m2p_channel_state(edmac) == M2P_STATE_STALL)
> -		cpu_relax();
> +	while (m2p_channel_state(edmac) != M2P_STATE_IDLE)
> +		dev_warn(chan2dev(edmac), "M2P: Not yet IDLE\n");
>  }
>  
>  static void m2p_fill_desc(struct ep93xx_dma_chan *edmac)
> -- 
> 2.12.2
> 

-- 
~Vinod

Re: [PATCH 1/2] dmaengine: ep93xx: Always start from BASE0

[Alexander Sverdlin]

Hello Vinod,

On 14/05/17 15:14, Vinod Koul wrote:
>> @@ -323,6 +331,10 @@ static int m2p_hw_setup(struct ep93xx_dma_chan *edmac)
>>  		| M2P_CONTROL_ENABLE;
>>  	m2p_set_control(edmac, control);
>>  
>> +	if (m2p_channel_nextbuf(edmac) != 0)
>> +		dev_warn(chan2dev(edmac), "M2P: Starting from BASE1\n");
> But then you are actually not restarting from BASE1 as you reset, so the
> warn is wrong.. Perhaps a more meaningful msg would be to say "expected 0
> but found 1, so resetting"

This is, maybe not so obvious, a warning, which should never fire. It just
proves, that I understand the HW correctly. As long as nobody sees the warning,
it's the case. And it proves that HW behaves according to user manual.
We can remove this warning completely.

Once again, the story is following:

- m2p_hw_setup() is always called after channel reset
- after reset channel expects to start from buffer 0
- therefore we must always start from buffer 0 <= this was not the case before
- we can read the expected "next buffer" from the controller => hence the warning
  maybe if there will be a bug somewhere and the channel will not be reset before
  next transfer, it will be triggered

Regards,
Alexander.