* Re: [PATCH] dma: mdc: Correct terminate_all handling [not found] <1448288524-21572-1-git-send-email-Damien.Horsley@imgtec.com> @ 2015-12-05 8:43 ` Vinod Koul 2015-12-07 16:59 ` Damien Horsley 0 siblings, 1 reply; 3+ messages in thread From: Vinod Koul @ 2015-12-05 8:43 UTC (permalink / raw) To: Damien Horsley; +Cc: dmaengine, linux-kernel, Dan Williams On Mon, Nov 23, 2015 at 02:22:04PM +0000, Damien Horsley wrote: > From: "Damien.Horsley" <Damien.Horsley@imgtec.com> > > Use of the CANCEL bit in mdc_terminate_all creates an > additional 'command done' to appear in the registers (in > addition to an interrupt). > > In addition, there is a potential race between > mdc_terminate_all and the irq handler if a transfer > completes at the same time as the terminate all (presently > this results in an inappropriate warning). > > To handle these issues, any outstanding 'command done' > events are cleared during mdc_terminate_all and the irq > handler takes no action when there are no new 'command done' > events. SO what does 'command done' event represent, and what would be the behaviour of ignoring those > > Signed-off-by: Damien.Horsley <Damien.Horsley@imgtec.com> > --- > drivers/dma/img-mdc-dma.c | 71 ++++++++++++++++++++++++++++++----------------- > 1 file changed, 45 insertions(+), 26 deletions(-) > > diff --git a/drivers/dma/img-mdc-dma.c b/drivers/dma/img-mdc-dma.c > index 9ca5683..aa09590 100644 > --- a/drivers/dma/img-mdc-dma.c > +++ b/drivers/dma/img-mdc-dma.c > @@ -651,6 +651,42 @@ static enum dma_status mdc_tx_status(struct dma_chan *chan, > return ret; > } > > +static unsigned int mdc_get_new_events(struct mdc_chan *mchan) > +{ > + u32 val, processed, done1, done2; > + unsigned int ret; > + > + val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED); > + processed = (val >> MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT) & > + MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK; This should be right justfied > + /* > + * CMDS_DONE may have incremented between reading CMDS_PROCESSED > + * and clearing INT_ACTIVE. Re-read CMDS_PROCESSED to ensure we > + * didn't miss a command completion. > + */ > + do { > + val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED); > + done1 = (val >> MDC_CMDS_PROCESSED_CMDS_DONE_SHIFT) & > + MDC_CMDS_PROCESSED_CMDS_DONE_MASK; > + val &= ~((MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK << > + MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT) | > + MDC_CMDS_PROCESSED_INT_ACTIVE); > + val |= done1 << MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT; > + mdc_chan_writel(mchan, val, MDC_CMDS_PROCESSED); > + val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED); > + done2 = (val >> MDC_CMDS_PROCESSED_CMDS_DONE_SHIFT) & > + MDC_CMDS_PROCESSED_CMDS_DONE_MASK; this is very hard to read, can you give some empty lines on logical blocks > + } while (done1 != done2); > + > + if (done1 >= processed) > + ret = done1 - processed; > + else > + ret = ((MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK + 1) - > + processed) + done1; > + > + return ret; > +} > + > static int mdc_terminate_all(struct dma_chan *chan) > { > struct mdc_chan *mchan = to_mdc_chan(chan); > @@ -667,6 +703,8 @@ static int mdc_terminate_all(struct dma_chan *chan) > mchan->desc = NULL; > vchan_get_all_descriptors(&mchan->vc, &head); > > + mdc_get_new_events(mchan); > + Only getting? -- ~Vinod ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] dma: mdc: Correct terminate_all handling 2015-12-05 8:43 ` [PATCH] dma: mdc: Correct terminate_all handling Vinod Koul @ 2015-12-07 16:59 ` Damien Horsley 2015-12-10 4:20 ` Vinod Koul 0 siblings, 1 reply; 3+ messages in thread From: Damien Horsley @ 2015-12-07 16:59 UTC (permalink / raw) To: Vinod Koul; +Cc: dmaengine, linux-kernel, Dan Williams, James Hartley On 05/12/15 08:43, Vinod Koul wrote: > On Mon, Nov 23, 2015 at 02:22:04PM +0000, Damien Horsley wrote: >> From: "Damien.Horsley" <Damien.Horsley@imgtec.com> >> >> Use of the CANCEL bit in mdc_terminate_all creates an >> additional 'command done' to appear in the registers (in >> addition to an interrupt). >> >> In addition, there is a potential race between >> mdc_terminate_all and the irq handler if a transfer >> completes at the same time as the terminate all (presently >> this results in an inappropriate warning). >> >> To handle these issues, any outstanding 'command done' >> events are cleared during mdc_terminate_all and the irq >> handler takes no action when there are no new 'command done' >> events. > > SO what does 'command done' event represent, and what would be the behaviour > of ignoring those > The 'command done' event occurs whenever a new descriptor is loaded from memory, and once when the whole transfer is completed. Use of the CANCEL bit can cause additional 'command done' events to occur, which need to be cleared. >> >> Signed-off-by: Damien.Horsley <Damien.Horsley@imgtec.com> >> --- >> drivers/dma/img-mdc-dma.c | 71 ++++++++++++++++++++++++++++++----------------- >> 1 file changed, 45 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/dma/img-mdc-dma.c b/drivers/dma/img-mdc-dma.c >> index 9ca5683..aa09590 100644 >> --- a/drivers/dma/img-mdc-dma.c >> +++ b/drivers/dma/img-mdc-dma.c >> @@ -651,6 +651,42 @@ static enum dma_status mdc_tx_status(struct dma_chan *chan, >> return ret; >> } >> >> +static unsigned int mdc_get_new_events(struct mdc_chan *mchan) >> +{ >> + u32 val, processed, done1, done2; >> + unsigned int ret; >> + >> + val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED); >> + processed = (val >> MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT) & >> + MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK; > > This should be right justfied > Ok >> + /* >> + * CMDS_DONE may have incremented between reading CMDS_PROCESSED >> + * and clearing INT_ACTIVE. Re-read CMDS_PROCESSED to ensure we >> + * didn't miss a command completion. >> + */ >> + do { >> + val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED); >> + done1 = (val >> MDC_CMDS_PROCESSED_CMDS_DONE_SHIFT) & >> + MDC_CMDS_PROCESSED_CMDS_DONE_MASK; >> + val &= ~((MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK << >> + MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT) | >> + MDC_CMDS_PROCESSED_INT_ACTIVE); >> + val |= done1 << MDC_CMDS_PROCESSED_CMDS_PROCESSED_SHIFT; >> + mdc_chan_writel(mchan, val, MDC_CMDS_PROCESSED); >> + val = mdc_chan_readl(mchan, MDC_CMDS_PROCESSED); >> + done2 = (val >> MDC_CMDS_PROCESSED_CMDS_DONE_SHIFT) & >> + MDC_CMDS_PROCESSED_CMDS_DONE_MASK; > > this is very hard to read, can you give some empty lines on logical blocks > Ok >> + } while (done1 != done2); >> + >> + if (done1 >= processed) >> + ret = done1 - processed; >> + else >> + ret = ((MDC_CMDS_PROCESSED_CMDS_PROCESSED_MASK + 1) - >> + processed) + done1; >> + >> + return ret; >> +} >> + >> static int mdc_terminate_all(struct dma_chan *chan) >> { >> struct mdc_chan *mchan = to_mdc_chan(chan); >> @@ -667,6 +703,8 @@ static int mdc_terminate_all(struct dma_chan *chan) >> mchan->desc = NULL; >> vchan_get_all_descriptors(&mchan->vc, &head); >> >> + mdc_get_new_events(mchan); >> + > > Only getting? > Yes, the extra 'command done' events need to be ignored. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] dma: mdc: Correct terminate_all handling 2015-12-07 16:59 ` Damien Horsley @ 2015-12-10 4:20 ` Vinod Koul 0 siblings, 0 replies; 3+ messages in thread From: Vinod Koul @ 2015-12-10 4:20 UTC (permalink / raw) To: Damien Horsley; +Cc: dmaengine, linux-kernel, Dan Williams, James Hartley On Mon, Dec 07, 2015 at 04:59:41PM +0000, Damien Horsley wrote: > On 05/12/15 08:43, Vinod Koul wrote: > > On Mon, Nov 23, 2015 at 02:22:04PM +0000, Damien Horsley wrote: > >> From: "Damien.Horsley" <Damien.Horsley@imgtec.com> > >> > >> Use of the CANCEL bit in mdc_terminate_all creates an > >> additional 'command done' to appear in the registers (in > >> addition to an interrupt). > >> > >> In addition, there is a potential race between > >> mdc_terminate_all and the irq handler if a transfer > >> completes at the same time as the terminate all (presently > >> this results in an inappropriate warning). > >> > >> To handle these issues, any outstanding 'command done' > >> events are cleared during mdc_terminate_all and the irq > >> handler takes no action when there are no new 'command done' > >> events. > > > > SO what does 'command done' event represent, and what would be the behaviour > > of ignoring those > > > > The 'command done' event occurs whenever a new descriptor is loaded from > memory, and once when the whole transfer is completed. Use of the CANCEL > bit can cause additional 'command done' events to occur, which need to > be cleared. Okay, btw please fix the subsystem name. I'ts dmaengine -- ~Vinod ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-12-10 4:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1448288524-21572-1-git-send-email-Damien.Horsley@imgtec.com>
2015-12-05 8:43 ` [PATCH] dma: mdc: Correct terminate_all handling Vinod Koul
2015-12-07 16:59 ` Damien Horsley
2015-12-10 4:20 ` Vinod Koul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox