* [PATCH 1/2] dmaengine: ep93xx: Always start from BASE0 [not found] <20170513221327.10114-1-alexander.sverdlin@gmail.com> @ 2017-05-13 22:13 ` Alexander Sverdlin 2017-05-14 13:14 ` Vinod Koul 2017-05-13 22:13 ` [PATCH 2/2] dmaengine: ep93xx: Don't drain the transfers in terminate_all() Alexander Sverdlin 1 sibling, 1 reply; 5+ messages in thread From: Alexander Sverdlin @ 2017-05-13 22:13 UTC (permalink / raw) To: dmaengine; +Cc: Alexander Sverdlin, Dan Williams, Vinod Koul, stable 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] dmaengine: ep93xx: Always start from BASE0 2017-05-13 22:13 ` [PATCH 1/2] dmaengine: ep93xx: Always start from BASE0 Alexander Sverdlin @ 2017-05-14 13:14 ` Vinod Koul 2017-05-14 13:33 ` Alexander Sverdlin 0 siblings, 1 reply; 5+ messages in thread From: Vinod Koul @ 2017-05-14 13:14 UTC (permalink / raw) To: Alexander Sverdlin; +Cc: dmaengine, Dan Williams, stable 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] dmaengine: ep93xx: Always start from BASE0 2017-05-14 13:14 ` Vinod Koul @ 2017-05-14 13:33 ` Alexander Sverdlin 0 siblings, 0 replies; 5+ messages in thread From: Alexander Sverdlin @ 2017-05-14 13:33 UTC (permalink / raw) To: Vinod Koul; +Cc: dmaengine, Dan Williams, stable 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] dmaengine: ep93xx: Don't drain the transfers in terminate_all() [not found] <20170513221327.10114-1-alexander.sverdlin@gmail.com> 2017-05-13 22:13 ` [PATCH 1/2] dmaengine: ep93xx: Always start from BASE0 Alexander Sverdlin @ 2017-05-13 22:13 ` Alexander Sverdlin 2017-05-14 13:16 ` Vinod Koul 1 sibling, 1 reply; 5+ messages in thread From: Alexander Sverdlin @ 2017-05-13 22:13 UTC (permalink / raw) To: dmaengine; +Cc: Alexander Sverdlin, Dan Williams, Vinod Koul, stable 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] dmaengine: ep93xx: Don't drain the transfers in terminate_all() 2017-05-13 22:13 ` [PATCH 2/2] dmaengine: ep93xx: Don't drain the transfers in terminate_all() Alexander Sverdlin @ 2017-05-14 13:16 ` Vinod Koul 0 siblings, 0 replies; 5+ messages in thread From: Vinod Koul @ 2017-05-14 13:16 UTC (permalink / raw) To: Alexander Sverdlin; +Cc: dmaengine, Dan Williams, stable 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-05-14 13:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20170513221327.10114-1-alexander.sverdlin@gmail.com>
2017-05-13 22:13 ` [PATCH 1/2] dmaengine: ep93xx: Always start from BASE0 Alexander Sverdlin
2017-05-14 13:14 ` Vinod Koul
2017-05-14 13:33 ` Alexander Sverdlin
2017-05-13 22:13 ` [PATCH 2/2] dmaengine: ep93xx: Don't drain the transfers in terminate_all() Alexander Sverdlin
2017-05-14 13:16 ` Vinod Koul
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.