[Sound-open-firmware] [PATCH 2/2] cnl: dma: refine dma interrupt processing on cnl

[Liam Girdwood]

On Wed, 2018-03-07 at 10:15 +0800, rander.wang wrote:
> hi Liam,
> 
> On 3/6/2018 7:40 PM, Liam Girdwood wrote:
> > On Tue, 2018-03-06 at 13:26 +0800, Rander Wang wrote:
> > > +/*All the dma share the same interrupt on CNL, so check each dma*/
> > > +/*controller when interrupt coming, then do the work            */
> > > +static void dw_dma_irq_cnl(void *data)
> > > +{
> > > +	struct dma *dma;
> > > +	uint32_t status_intr;
> > > +
> > > +	/*check interrupt status of DMA controller 0*/
> > > +	dma = dma_get(DMA_GP_LP_DMAC0);
> > > +	status_intr = dw_read(dma, DW_INTR_STATUS);
> > > +	if (status_intr)
> > > +		dw_dma_irq_handler(dma);
> > > +
> > > +	/*check interrupt status of DMA controller 1*/
> > > +	dma = dma_get(DMA_GP_LP_DMAC1);
> > > +	status_intr = dw_read(dma, DW_INTR_STATUS);
> > > +	if (status_intr)
> > > +		dw_dma_irq_handler(dma);
> > 
> > Btw, shouldn't the CNL IRQ hanler be using the same code as APL since
> > the DMAC is second level on both APL and CNL ?
> 
> for APL, there is a different irq for each DMA channel, but for CNL two
> dma controller share the same irq, and dma status should be checked for
> which channal. So it is a three level irq on CNL

Ok, now I see.

This should still use the 1st and 2nd level handlers like APL.

1) Each DMAC registers an IRQ handler with it's DMAC private data.

2) On IRQ, the first level IRQ core handler is called and it it turns
calls both second level handlers (DMAC handlers).

Can you redo 2/2 to use the above method. Your current patch does the
above steps in the DMA driver instead of the IRQ core.

Liam

> 
> > > +}
> > > +
> > >   static int dw_dma_probe(struct dma *dma)
> > >   {
> > >   	struct dma_pdata *dw_pdata;
> > > @@ -1157,7 +1177,11 @@ static int dw_dma_probe(struct dma *dma)
> > >   	}
> > >   
> > >   	/* register our IRQ handler */
> > > +#ifdef CONFIG_CANNONLAKE
> > > +	interrupt_register(dma_irq(dma), dw_dma_irq_cnl, dma);
> > > +#else
> > >   	interrupt_register(dma_irq(dma), dw_dma_irq_handler, dma);
> > > +#endif
> > 
> > You dont need this #ifdef if both IRQ handler functions have the same
> > name (and we select the correct one at build time).
> 
>      sorry, i don't understand this issue.
> > 
> > Liam
> > 
> > >   	interrupt_enable(dma_irq(dma));
> > >   
> > >   	return 0;
> > 
> > _______________________________________________
> > Sound-open-firmware mailing list
> > Sound-open-firmware at alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
> 
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware