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

[rander.wang]

hi Liam,

     you mentioned :  using IRQ core as 1st level IRQ handler

i don't get your idea.  to check which dma triggers the interrupt, dma 
status register

would be queried. Do this in 1st level IRQ handler is very ugly.

Rander

On 3/7/2018 4:57 PM, Liam Girdwood wrote:
> 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