| Message ID | 1421745137-27292-1-git-send-email-yang.jie@intel.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Tue, 2015-01-20 at 17:12 +0800, Jie Yang wrote: > When dma controller is not used by any user and set off, > we should disble interrupt handler, at least the interrupt > reset part, for some subsystem, e.g. ADSP, may use the > dma in its own logic, here reset the interrupt may make > this subsystem work abnormally. I think like on Intel MID (Medfield) there should be a specific register to route interrupt pin from ADSP to CPU and vise versa. So, my understanding we have to utilize that register somehow. Since it's quite specific to ADSP HW I would consider that ADSP driver would take care of this. Vinod, it seems we already discussed this earlier, though I'm not quite sure I remember your point. Can you share your opinion? > > Signed-off-by: Jie Yang <yang.jie@intel.com> > --- > drivers/dma/dw/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c > index 3804785..ac0d6f6 100644 > --- a/drivers/dma/dw/core.c > +++ b/drivers/dma/dw/core.c > @@ -619,7 +619,7 @@ static irqreturn_t dw_dma_interrupt(int irq, void *dev_id) > dev_vdbg(dw->dma.dev, "%s: status=0x%x\n", __func__, status); > > /* Check if we have any interrupt from the DMAC */ > - if (!status) > + if (!status || !dw->in_use) > return IRQ_NONE; > > /*
On Tue, Jan 20, 2015 at 11:36:05AM +0200, Andy Shevchenko wrote: > On Tue, 2015-01-20 at 17:12 +0800, Jie Yang wrote: > > When dma controller is not used by any user and set off, > > we should disble interrupt handler, at least the interrupt > > reset part, for some subsystem, e.g. ADSP, may use the > > dma in its own logic, here reset the interrupt may make > > this subsystem work abnormally. > > I think like on Intel MID (Medfield) there should be a specific register > to route interrupt pin from ADSP to CPU and vise versa. So, my > understanding we have to utilize that register somehow. > > Since it's quite specific to ADSP HW I would consider that ADSP driver > would take care of this. > > Vinod, it seems we already discussed this earlier, though I'm not quite > sure I remember your point. Can you share your opinion? Yes using those registers is recommended. Not handling a kernel irq seems not a good option. Do we have an updated patch for this now?
> -----Original Message----- > From: Koul, Vinod > Sent: Wednesday, February 11, 2015 9:19 AM > To: Andy Shevchenko > Cc: Jie, Yang; dmaengine@vger.kernel.org; Girdwood, Liam R > Subject: Re: [PATCH] dmaengine: dw: don't handle interrupt when > dmaengine is not used > > On Tue, Jan 20, 2015 at 11:36:05AM +0200, Andy Shevchenko wrote: > > On Tue, 2015-01-20 at 17:12 +0800, Jie Yang wrote: > > > When dma controller is not used by any user and set off, we should > > > disble interrupt handler, at least the interrupt reset part, for > > > some subsystem, e.g. ADSP, may use the dma in its own logic, here > > > reset the interrupt may make this subsystem work abnormally. > > > > I think like on Intel MID (Medfield) there should be a specific > > register to route interrupt pin from ADSP to CPU and vise versa. So, > > my understanding we have to utilize that register somehow. > > > > Since it's quite specific to ADSP HW I would consider that ADSP driver > > would take care of this. > > > > Vinod, it seems we already discussed this earlier, though I'm not > > quite sure I remember your point. Can you share your opinion? > Yes using those registers is recommended. Not handling a kernel irq seems > not a good option. > > Do we have an updated patch for this now? [Keyon] Andy sent out a update patch on 2015/1/27: [PATCH] dmaengine: dw: disable BLOCK interrupts in dw_dma_off() What's your opinion for that solution? > > -- > ~Vinod > > > > > > > > Signed-off-by: Jie Yang <yang.jie@intel.com> > > > --- > > > drivers/dma/dw/core.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c index > > > 3804785..ac0d6f6 100644 > > > --- a/drivers/dma/dw/core.c > > > +++ b/drivers/dma/dw/core.c > > > @@ -619,7 +619,7 @@ static irqreturn_t dw_dma_interrupt(int irq, void > *dev_id) > > > dev_vdbg(dw->dma.dev, "%s: status=0x%x\n", __func__, status); > > > > > > /* Check if we have any interrupt from the DMAC */ > > > - if (!status) > > > + if (!status || !dw->in_use) > > > return IRQ_NONE; > > > > > > /* > > > > > > -- > > Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy > > > > -- -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 11, 2015 at 05:42:43PM +0530, Jie, Yang wrote: > > -----Original Message----- > > From: Koul, Vinod > > Sent: Wednesday, February 11, 2015 9:19 AM > > To: Andy Shevchenko > > Cc: Jie, Yang; dmaengine@vger.kernel.org; Girdwood, Liam R > > Subject: Re: [PATCH] dmaengine: dw: don't handle interrupt when > > dmaengine is not used > > > > On Tue, Jan 20, 2015 at 11:36:05AM +0200, Andy Shevchenko wrote: > > > On Tue, 2015-01-20 at 17:12 +0800, Jie Yang wrote: > > > > When dma controller is not used by any user and set off, we should > > > > disble interrupt handler, at least the interrupt reset part, for > > > > some subsystem, e.g. ADSP, may use the dma in its own logic, here > > > > reset the interrupt may make this subsystem work abnormally. > > > > > > I think like on Intel MID (Medfield) there should be a specific > > > register to route interrupt pin from ADSP to CPU and vise versa. So, > > > my understanding we have to utilize that register somehow. > > > > > > Since it's quite specific to ADSP HW I would consider that ADSP driver > > > would take care of this. > > > > > > Vinod, it seems we already discussed this earlier, though I'm not > > > quite sure I remember your point. Can you share your opinion? > > Yes using those registers is recommended. Not handling a kernel irq seems > > not a good option. > > > > Do we have an updated patch for this now? > [Keyon] Andy sent out a update patch on 2015/1/27: > [PATCH] dmaengine: dw: disable BLOCK interrupts in dw_dma_off() > What's your opinion for that solution? Thats not a right solution either. The _right_ solution is to use mask registers so that interrupt is delivered to the target that needs them.
> -----Original Message----- > From: Koul, Vinod > Sent: Friday, February 13, 2015 4:37 PM > To: Jie, Yang > Cc: Andy Shevchenko; dmaengine@vger.kernel.org; Girdwood, Liam R > Subject: Re: [PATCH] dmaengine: dw: don't handle interrupt when > dmaengine is not used > > On Wed, Feb 11, 2015 at 05:42:43PM +0530, Jie, Yang wrote: > > > -----Original Message----- > > > From: Koul, Vinod > > > Sent: Wednesday, February 11, 2015 9:19 AM > > > To: Andy Shevchenko > > > Cc: Jie, Yang; dmaengine@vger.kernel.org; Girdwood, Liam R > > > Subject: Re: [PATCH] dmaengine: dw: don't handle interrupt when > > > dmaengine is not used > > > > > > On Tue, Jan 20, 2015 at 11:36:05AM +0200, Andy Shevchenko wrote: > > > > On Tue, 2015-01-20 at 17:12 +0800, Jie Yang wrote: > > > > > When dma controller is not used by any user and set off, we > > > > > should disble interrupt handler, at least the interrupt reset > > > > > part, for some subsystem, e.g. ADSP, may use the dma in its own > > > > > logic, here reset the interrupt may make this subsystem work > abnormally. > > > > > > > > I think like on Intel MID (Medfield) there should be a specific > > > > register to route interrupt pin from ADSP to CPU and vise versa. > > > > So, my understanding we have to utilize that register somehow. > > > > > > > > Since it's quite specific to ADSP HW I would consider that ADSP > > > > driver would take care of this. > > > > > > > > Vinod, it seems we already discussed this earlier, though I'm not > > > > quite sure I remember your point. Can you share your opinion? > > > Yes using those registers is recommended. Not handling a kernel irq > > > seems not a good option. > > > > > > Do we have an updated patch for this now? > > [Keyon] Andy sent out a update patch on 2015/1/27: > > [PATCH] dmaengine: dw: disable BLOCK interrupts in dw_dma_off() What's > > your opinion for that solution? > Thats not a right solution either. The _right_ solution is to use mask registers > so that interrupt is delivered to the target that needs them. [Keyon] just come back from CNY vacation, so which register do you mean we should mask: ISC/IMC, ISD/IMD, or STSINT(MSKERR, MSKDSTTRAN, MSKSRCTRAN, MSKBLOCK and MSKTFR)? Masking IMC can't really prevent DMA receive the interrupt, and it's not easy to mask STSINT in audio driver side. > > -- > ~Vinod -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 27, 2015 at 01:51:51PM +0530, Jie, Yang wrote: > > > [Keyon] Andy sent out a update patch on 2015/1/27: > > > [PATCH] dmaengine: dw: disable BLOCK interrupts in dw_dma_off() What's > > > your opinion for that solution? > > Thats not a right solution either. The _right_ solution is to use mask registers > > so that interrupt is delivered to the target that needs them. > [Keyon] just come back from CNY vacation, so which register do you mean we > should mask: ISC/IMC, ISD/IMD, or STSINT(MSKERR, MSKDSTTRAN, MSKSRCTRAN, > MSKBLOCK and MSKTFR)? > Masking IMC can't really prevent DMA receive the interrupt, and it's not easy > to mask STSINT in audio driver side. Pls see the specs, i dont have for this one, so cant really point to one
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c index 3804785..ac0d6f6 100644 --- a/drivers/dma/dw/core.c +++ b/drivers/dma/dw/core.c @@ -619,7 +619,7 @@ static irqreturn_t dw_dma_interrupt(int irq, void *dev_id) dev_vdbg(dw->dma.dev, "%s: status=0x%x\n", __func__, status); /* Check if we have any interrupt from the DMAC */ - if (!status) + if (!status || !dw->in_use) return IRQ_NONE; /*
When dma controller is not used by any user and set off, we should disble interrupt handler, at least the interrupt reset part, for some subsystem, e.g. ADSP, may use the dma in its own logic, here reset the interrupt may make this subsystem work abnormally. Signed-off-by: Jie Yang <yang.jie@intel.com> --- drivers/dma/dw/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)