* [PATCH 5/7] S3C64XX DMA: TC-IRQ implemented. @ 2009-09-15 10:01 Jassi 2009-09-15 10:13 ` Mark Brown 2009-09-16 0:23 ` Ben Dooks 0 siblings, 2 replies; 5+ messages in thread From: Jassi @ 2009-09-15 10:01 UTC (permalink / raw) To: linux-arm-kernel The available irq handler simply doesn't work. The Dma state-machine is broken. This patch fixes it. Also, chan->curr,next,end pointer manipulation is protected. Signed-Off-by: Jassi <jassi.brar@samsung.com> --- arch/arm/plat-s3c64xx/dma.c | 61 +++++++++++++++++++++++++++--------------- 1 files changed, 39 insertions(+), 22 deletions(-) diff --git a/arch/arm/plat-s3c64xx/dma.c b/arch/arm/plat-s3c64xx/dma.c index 02bc82b..e68469d 100644 --- a/arch/arm/plat-s3c64xx/dma.c +++ b/arch/arm/plat-s3c64xx/dma.c @@ -339,6 +339,7 @@ int s3c2410_dma_enqueue(unsigned int channel, void *id, struct s3c64xx_dma_buff *next; struct s3c64xx_dma_buff *buff; struct pl080s_lli *lli; + unsigned long flags; int ret; WARN_ON(!chan); @@ -366,6 +367,8 @@ int s3c2410_dma_enqueue(unsigned int channel, void *id, s3c64xx_dma_fill_lli(chan, lli, data, size); + local_irq_save(flags); + if ((next = chan->next) != NULL) { struct s3c64xx_dma_buff *end = chan->end; struct pl080s_lli *endlli = end->lli; @@ -399,6 +402,8 @@ int s3c2410_dma_enqueue(unsigned int channel, void *id, s3c64xx_lli_to_regs(chan, lli); } + local_irq_restore(flags); + show_lli(lli); dbg_showchan(chan); @@ -562,27 +567,13 @@ int s3c2410_dma_free(unsigned int channel, struct s3c2410_dma_client *client) EXPORT_SYMBOL(s3c2410_dma_free); - -static void s3c64xx_dma_tcirq(struct s3c64xx_dmac *dmac, int offs) -{ - struct s3c2410_dma_chan *chan = dmac->channels + offs; - - /* note, we currently do not bother to work out which buffer - * or buffers have been completed since the last tc-irq. */ - - if (chan->callback_fn) - (chan->callback_fn)(chan, chan->curr->pw, 0, S3C2410_RES_OK); -} - -static void s3c64xx_dma_errirq(struct s3c64xx_dmac *dmac, int offs) -{ - printk(KERN_DEBUG "%s: offs %d\n", __func__, offs); -} - static irqreturn_t s3c64xx_dma_irq(int irq, void *pw) { + struct s3c64xx_dma_buff *buff; struct s3c64xx_dmac *dmac = pw; - u32 tcstat, errstat; + struct s3c2410_dma_chan *chan; + enum s3c2410_dma_buffresult res; + u32 tcstat, errstat, val; u32 bit; int offs; @@ -590,15 +581,41 @@ static irqreturn_t s3c64xx_dma_irq(int irq, void *pw) errstat = readl(dmac->regs + PL080_ERR_STATUS); for (offs = 0, bit = 1; offs < 8; offs++, bit <<= 1) { + + if (!(errstat & bit) && !(tcstat & bit)) + continue; + + res = S3C2410_RES_ERR; + + if (errstat & bit) + writel(bit, dmac->regs + PL080_ERR_CLEAR); + if (tcstat & bit) { writel(bit, dmac->regs + PL080_TC_CLEAR); - s3c64xx_dma_tcirq(dmac, offs); + res = S3C2410_RES_OK; } - if (errstat & bit) { - s3c64xx_dma_errirq(dmac, offs); - writel(bit, dmac->regs + PL080_ERR_CLEAR); + chan = dmac->channels + offs; + if(chan->curr == NULL) + continue; + + buff = chan->curr; + if (chan->curr != chan->end) { + BUG_ON(chan->curr->next == NULL); + chan->curr = chan->curr->next; + + val = readl(chan->regs + PL080S_CH_CONFIG); + if (!(val & PL080_CONFIG_ACTIVE)) { + s3c64xx_lli_to_regs(chan, chan->curr->lli); + val |= PL080_CONFIG_ENABLE; + writel(val, chan->regs + PL080S_CH_CONFIG); + } + } else { + chan->curr = chan->next = chan->end = NULL; } + + s3c64xx_dma_bufffdone(chan, buff, res); + s3c64xx_dma_freebuff(buff); } return IRQ_HANDLED; -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 5/7] S3C64XX DMA: TC-IRQ implemented. 2009-09-15 10:01 [PATCH 5/7] S3C64XX DMA: TC-IRQ implemented Jassi @ 2009-09-15 10:13 ` Mark Brown 2009-09-15 11:04 ` jassi brar 2009-09-16 0:23 ` Ben Dooks 1 sibling, 1 reply; 5+ messages in thread From: Mark Brown @ 2009-09-15 10:13 UTC (permalink / raw) To: linux-arm-kernel On Tue, Sep 15, 2009 at 07:01:23PM +0900, Jassi wrote: > + if (errstat & bit) > + writel(bit, dmac->regs + PL080_ERR_CLEAR); The old code would complain in the logs on error, which seems like a reasonable approach if we've no better idea what to do - it at least tells people something about what might be going on if things break. > + chan = dmac->channels + offs; > + if(chan->curr == NULL) > + continue; It's probably a good idea to run your patches through scripts/checkpatch.pl before submitting them. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 5/7] S3C64XX DMA: TC-IRQ implemented. 2009-09-15 10:13 ` Mark Brown @ 2009-09-15 11:04 ` jassi brar 0 siblings, 0 replies; 5+ messages in thread From: jassi brar @ 2009-09-15 11:04 UTC (permalink / raw) To: linux-arm-kernel On Tue, Sep 15, 2009 at 7:13 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Tue, Sep 15, 2009 at 07:01:23PM +0900, Jassi wrote: > >> + ? ? ? ? ? ? if (errstat & bit) >> + ? ? ? ? ? ? ? ? ? ? writel(bit, dmac->regs + PL080_ERR_CLEAR); > > The old code would complain in the logs on error, which seems like a > reasonable approach if we've no better idea what to do - it at least > tells people something about what might be going on if things break. Yes, some debug mssg wud be better. >> + ? ? ? ? ? ? chan = dmac->channels + offs; >> + ? ? ? ? ? ? if(chan->curr == NULL) >> + ? ? ? ? ? ? ? ? ? ? continue; > > It's probably a good idea to run your patches through > scripts/checkpatch.pl before submitting them. ahhhh.... my me!! ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 5/7] S3C64XX DMA: TC-IRQ implemented. 2009-09-15 10:01 [PATCH 5/7] S3C64XX DMA: TC-IRQ implemented Jassi 2009-09-15 10:13 ` Mark Brown @ 2009-09-16 0:23 ` Ben Dooks 2009-09-16 2:43 ` jassi brar 1 sibling, 1 reply; 5+ messages in thread From: Ben Dooks @ 2009-09-16 0:23 UTC (permalink / raw) To: linux-arm-kernel On Tue, Sep 15, 2009 at 07:01:23PM +0900, Jassi wrote: > The available irq handler simply doesn't work. The Dma state-machine > is broken. This patch fixes it. A better explanation would have been nice, so I could actually evaluate what is going on here. > Also, chan->curr,next,end pointer manipulation is protected. Should have been a seperate patch, I would have been able to apply that reasonably easily. > Signed-Off-by: Jassi <jassi.brar@samsung.com> > --- > arch/arm/plat-s3c64xx/dma.c | 61 +++++++++++++++++++++++++++--------------- > 1 files changed, 39 insertions(+), 22 deletions(-) > > diff --git a/arch/arm/plat-s3c64xx/dma.c b/arch/arm/plat-s3c64xx/dma.c > index 02bc82b..e68469d 100644 > --- a/arch/arm/plat-s3c64xx/dma.c > +++ b/arch/arm/plat-s3c64xx/dma.c > @@ -339,6 +339,7 @@ int s3c2410_dma_enqueue(unsigned int channel, void *id, > struct s3c64xx_dma_buff *next; > struct s3c64xx_dma_buff *buff; > struct pl080s_lli *lli; > + unsigned long flags; > int ret; > > WARN_ON(!chan); > @@ -366,6 +367,8 @@ int s3c2410_dma_enqueue(unsigned int channel, void *id, > > s3c64xx_dma_fill_lli(chan, lli, data, size); > > + local_irq_save(flags); > + > if ((next = chan->next) != NULL) { > struct s3c64xx_dma_buff *end = chan->end; > struct pl080s_lli *endlli = end->lli; > @@ -399,6 +402,8 @@ int s3c2410_dma_enqueue(unsigned int channel, void *id, > s3c64xx_lli_to_regs(chan, lli); > } > > + local_irq_restore(flags); > + > show_lli(lli); > > dbg_showchan(chan); > @@ -562,27 +567,13 @@ int s3c2410_dma_free(unsigned int channel, struct s3c2410_dma_client *client) > > EXPORT_SYMBOL(s3c2410_dma_free); > > - > -static void s3c64xx_dma_tcirq(struct s3c64xx_dmac *dmac, int offs) > -{ > - struct s3c2410_dma_chan *chan = dmac->channels + offs; > - > - /* note, we currently do not bother to work out which buffer > - * or buffers have been completed since the last tc-irq. */ > - > - if (chan->callback_fn) > - (chan->callback_fn)(chan, chan->curr->pw, 0, S3C2410_RES_OK); > -} I belive a callback is necessary for the audio code to update the current dma position of the stream. I also thought this worked? > -static void s3c64xx_dma_errirq(struct s3c64xx_dmac *dmac, int offs) > -{ > - printk(KERN_DEBUG "%s: offs %d\n", __func__, offs); > -} > - > static irqreturn_t s3c64xx_dma_irq(int irq, void *pw) > { > + struct s3c64xx_dma_buff *buff; > struct s3c64xx_dmac *dmac = pw; > - u32 tcstat, errstat; > + struct s3c2410_dma_chan *chan; > + enum s3c2410_dma_buffresult res; > + u32 tcstat, errstat, val; > u32 bit; > int offs; > > @@ -590,15 +581,41 @@ static irqreturn_t s3c64xx_dma_irq(int irq, void *pw) > errstat = readl(dmac->regs + PL080_ERR_STATUS); > > for (offs = 0, bit = 1; offs < 8; offs++, bit <<= 1) { > + > + if (!(errstat & bit) && !(tcstat & bit)) > + continue; > + > + res = S3C2410_RES_ERR; > + > + if (errstat & bit) > + writel(bit, dmac->regs + PL080_ERR_CLEAR); > + > if (tcstat & bit) { > writel(bit, dmac->regs + PL080_TC_CLEAR); > - s3c64xx_dma_tcirq(dmac, offs); > + res = S3C2410_RES_OK; > } > > - if (errstat & bit) { > - s3c64xx_dma_errirq(dmac, offs); > - writel(bit, dmac->regs + PL080_ERR_CLEAR); > + chan = dmac->channels + offs; > + if(chan->curr == NULL) > + continue; > + > + buff = chan->curr; > + if (chan->curr != chan->end) { > + BUG_ON(chan->curr->next == NULL); > + chan->curr = chan->curr->next; > + > + val = readl(chan->regs + PL080S_CH_CONFIG); > + if (!(val & PL080_CONFIG_ACTIVE)) { > + s3c64xx_lli_to_regs(chan, chan->curr->lli); > + val |= PL080_CONFIG_ENABLE; > + writel(val, chan->regs + PL080S_CH_CONFIG); > + } > + } else { > + chan->curr = chan->next = chan->end = NULL; > } > + > + s3c64xx_dma_bufffdone(chan, buff, res); > + s3c64xx_dma_freebuff(buff); > } Hmm, this looks like a train wreck. Please do not move code around like this. -- Ben Q: What's a light-year? A: One-third less calories than a regular year. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 5/7] S3C64XX DMA: TC-IRQ implemented. 2009-09-16 0:23 ` Ben Dooks @ 2009-09-16 2:43 ` jassi brar 0 siblings, 0 replies; 5+ messages in thread From: jassi brar @ 2009-09-16 2:43 UTC (permalink / raw) To: linux-arm-kernel On Wed, Sep 16, 2009 at 9:23 AM, Ben Dooks <ben-linux@fluff.org> wrote: > On Tue, Sep 15, 2009 at 07:01:23PM +0900, Jassi wrote: >> Also, chan->curr,next,end pointer manipulation is protected. > > Should have been a seperate patch, I would have been able to apply that > reasonably easily. Ok, will resend as a separate patch. >> Signed-Off-by: Jassi <jassi.brar@samsung.com> >> --- >> ?arch/arm/plat-s3c64xx/dma.c | ? 61 +++++++++++++++++++++++++++--------------- >> ?1 files changed, 39 insertions(+), 22 deletions(-) >> >> diff --git a/arch/arm/plat-s3c64xx/dma.c b/arch/arm/plat-s3c64xx/dma.c >> index 02bc82b..e68469d 100644 >> --- a/arch/arm/plat-s3c64xx/dma.c >> +++ b/arch/arm/plat-s3c64xx/dma.c >> @@ -339,6 +339,7 @@ int s3c2410_dma_enqueue(unsigned int channel, void *id, >> ? ? ? struct s3c64xx_dma_buff *next; >> ? ? ? struct s3c64xx_dma_buff *buff; >> ? ? ? struct pl080s_lli *lli; >> + ? ? unsigned long flags; >> ? ? ? int ret; >> >> ? ? ? WARN_ON(!chan); >> @@ -366,6 +367,8 @@ int s3c2410_dma_enqueue(unsigned int channel, void *id, >> >> ? ? ? s3c64xx_dma_fill_lli(chan, lli, data, size); >> >> + ? ? local_irq_save(flags); >> + >> ? ? ? if ((next = chan->next) != NULL) { >> ? ? ? ? ? ? ? struct s3c64xx_dma_buff *end = chan->end; >> ? ? ? ? ? ? ? struct pl080s_lli *endlli = end->lli; >> @@ -399,6 +402,8 @@ int s3c2410_dma_enqueue(unsigned int channel, void *id, >> ? ? ? ? ? ? ? s3c64xx_lli_to_regs(chan, lli); >> ? ? ? } >> >> + ? ? local_irq_restore(flags); >> + >> ? ? ? show_lli(lli); >> >> ? ? ? dbg_showchan(chan); >> @@ -562,27 +567,13 @@ int s3c2410_dma_free(unsigned int channel, struct s3c2410_dma_client *client) >> >> ?EXPORT_SYMBOL(s3c2410_dma_free); >> >> - >> -static void s3c64xx_dma_tcirq(struct s3c64xx_dmac *dmac, int offs) >> -{ >> - ? ? struct s3c2410_dma_chan *chan = dmac->channels + offs; >> - >> - ? ? /* note, we currently do not bother to work out which buffer >> - ? ? ?* or buffers have been completed since the last tc-irq. */ >> - >> - ? ? if (chan->callback_fn) >> - ? ? ? ? ? ? (chan->callback_fn)(chan, chan->curr->pw, 0, S3C2410_RES_OK); >> -} > > I belive a callback is necessary for the audio code to update the > current dma position of the stream. I also thought this worked? Of course, it is necessary. And my patch doesn't skip that. Instead of special wrapper for callback during TC-IRQ, I make use of the existing function s3c64xx_dma_buffdone with return code as per the IRQ status. Also, with the patch, callback_fn is called for each buffer that was enqueued. >> -static void s3c64xx_dma_errirq(struct s3c64xx_dmac *dmac, int offs) >> -{ >> - ? ? printk(KERN_DEBUG "%s: offs %d\n", __func__, offs); >> -} >> - >> ?static irqreturn_t s3c64xx_dma_irq(int irq, void *pw) >> ?{ >> + ? ? struct s3c64xx_dma_buff *buff; >> ? ? ? struct s3c64xx_dmac *dmac = pw; >> - ? ? u32 tcstat, errstat; >> + ? ? struct s3c2410_dma_chan *chan; >> + ? ? enum s3c2410_dma_buffresult res; >> + ? ? u32 tcstat, errstat, val; >> ? ? ? u32 bit; >> ? ? ? int offs; >> >> @@ -590,15 +581,41 @@ static irqreturn_t s3c64xx_dma_irq(int irq, void *pw) >> ? ? ? errstat = readl(dmac->regs + PL080_ERR_STATUS); >> >> ? ? ? for (offs = 0, bit = 1; offs < 8; offs++, bit <<= 1) { >> + >> + ? ? ? ? ? ? if (!(errstat & bit) && !(tcstat & bit)) >> + ? ? ? ? ? ? ? ? ? ? continue; >> + >> + ? ? ? ? ? ? res = S3C2410_RES_ERR; >> + >> + ? ? ? ? ? ? if (errstat & bit) >> + ? ? ? ? ? ? ? ? ? ? writel(bit, dmac->regs + PL080_ERR_CLEAR); >> + >> ? ? ? ? ? ? ? if (tcstat & bit) { >> ? ? ? ? ? ? ? ? ? ? ? writel(bit, dmac->regs + PL080_TC_CLEAR); >> - ? ? ? ? ? ? ? ? ? ? s3c64xx_dma_tcirq(dmac, offs); >> + ? ? ? ? ? ? ? ? ? ? res = S3C2410_RES_OK; >> ? ? ? ? ? ? ? } >> >> - ? ? ? ? ? ? if (errstat & bit) { >> - ? ? ? ? ? ? ? ? ? ? s3c64xx_dma_errirq(dmac, offs); >> - ? ? ? ? ? ? ? ? ? ? writel(bit, dmac->regs + PL080_ERR_CLEAR); >> + ? ? ? ? ? ? chan = dmac->channels + offs; >> + ? ? ? ? ? ? if(chan->curr == NULL) >> + ? ? ? ? ? ? ? ? ? ? continue; >> + >> + ? ? ? ? ? ? buff = chan->curr; >> + ? ? ? ? ? ? if (chan->curr != chan->end) { >> + ? ? ? ? ? ? ? ? ? ? BUG_ON(chan->curr->next == NULL); >> + ? ? ? ? ? ? ? ? ? ? chan->curr = chan->curr->next; >> + >> + ? ? ? ? ? ? ? ? ? ? val = readl(chan->regs + PL080S_CH_CONFIG); >> + ? ? ? ? ? ? ? ? ? ? if (!(val & PL080_CONFIG_ACTIVE)) { >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? s3c64xx_lli_to_regs(chan, chan->curr->lli); >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? val |= PL080_CONFIG_ENABLE; >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(val, chan->regs + PL080S_CH_CONFIG); >> + ? ? ? ? ? ? ? ? ? ? } >> + ? ? ? ? ? ? } else { >> + ? ? ? ? ? ? ? ? ? ? chan->curr = chan->next = chan->end = NULL; >> ? ? ? ? ? ? ? } >> + >> + ? ? ? ? ? ? s3c64xx_dma_bufffdone(chan, buff, res); >> + ? ? ? ? ? ? s3c64xx_dma_freebuff(buff); >> ? ? ? } > > Hmm, this looks like a train wreck. Please do not move code around > like this. Please have a parallel look at the driver with and without the patch to see the overall picture. I know the reviewer shudn't need to do that, but its code modified more than moved. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-09-16 2:43 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-09-15 10:01 [PATCH 5/7] S3C64XX DMA: TC-IRQ implemented Jassi 2009-09-15 10:13 ` Mark Brown 2009-09-15 11:04 ` jassi brar 2009-09-16 0:23 ` Ben Dooks 2009-09-16 2:43 ` jassi brar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).