[Sound-open-firmware] [PATCH 2/2] cnl: dma: refine dma interrupt processing on cnl
On cnl, all the dma share the same interrupt pin, so all the dma controller need to be checked in interrupt function.
Signed-off-by: Rander Wang rander.wang@linux.intel.com --- src/drivers/dw-dma.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/src/drivers/dw-dma.c b/src/drivers/dw-dma.c index f736679..a06087e 100644 --- a/src/drivers/dw-dma.c +++ b/src/drivers/dw-dma.c @@ -1136,6 +1136,26 @@ static void dw_dma_irq_handler(void *data) } }
+/*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); +} + 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 interrupt_enable(dma_irq(dma));
return 0;
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 ?
+}
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).
Liam
interrupt_enable(dma_irq(dma));
return 0;
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
+}
- 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@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
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@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
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.
Each DMAC registers an IRQ handler with it's DMAC private data.
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@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Thu, 2018-03-08 at 14:39 +0800, rander.wang wrote:
hi Liam,
Please don't top post.
you mentioned : using IRQ core as 1st level IRQ handleri 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.
1st level handler does not check the DMAC. It only masks the 1st level IRQ and then calls all the 2nd level handlers. Each second level handler then checks if the IRQ is for itself and returns immediately if its not the IRQ owner.
Liam
Hi Liam,
On 3/8/2018 4:11 PM, Liam Girdwood wrote:
On Thu, 2018-03-08 at 14:39 +0800, rander.wang wrote:
hi Liam,
Please don't top post.
you mentioned : using IRQ core as 1st level IRQ handleri 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.
1st level handler does not check the DMAC. It only masks the 1st level IRQ and then calls all the 2nd level handlers. Each second level handler then checks if the IRQ is for itself and returns immediately if its not the IRQ owner.
i have discuss with Keyon. our idea is: maintain a list of interrupt function and private data
on second level data structure. In second level interrupt handler, call the irq functions
for each list member.
can you give some advice?
Thank you!
Liam
On Thu, 2018-03-08 at 17:47 +0800, rander.wang wrote:
Hi Liam,
On 3/8/2018 4:11 PM, Liam Girdwood wrote:
On Thu, 2018-03-08 at 14:39 +0800, rander.wang wrote:
hi Liam,
Please don't top post.
you mentioned : using IRQ core as 1st level IRQ handleri 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.
1st level handler does not check the DMAC. It only masks the 1st level IRQ and then calls all the 2nd level handlers. Each second level handler then checks if the IRQ is for itself and returns immediately if its not the IRQ owner.
i have discuss with Keyon. our idea is: maintain a list ofinterrupt function and private data
on second level data structure. In second level interrupt handler,call the irq functions
for each list member. can you give some advice?
Btw, the levels I'm referring to here are parent/child level and not priority level (so it's also worth sending an earlier patch that does some renaming i.e. parent_level3_handler -> level2_priority3_handler and IRQ_NUM_EXT_LEVEL2 -> IRQ_NUM_EXT_PRI2) as this should clear up levels and priorities. .
1) 1st level will be DSP interrupt - we then call second level handler..
2) 2nd level will be IRQ controller level (i.e handler will determine which child HW IP has the IRQ, DMAC, IPC, clock, SSP etc and call the 3rd level child handler).
3) 3rd level handler will iterate through it's list of client handlers. In this case it calls the handler for each DMAC.
This means we need the following :-
4) generic level3 handler code needs to be added to src/platform/cannonlake/interrupt.c. It will be similar to level2 handler in that it will loop through an array of clients.
5) platform_irq_get_parent() will need modified to support 3rd level handlers. It will return 3rd level handler for DW DMAC clients.
Does this make sense ?
Liam
Hi Liam,
On 3/8/2018 6:59 PM, Liam Girdwood wrote:
On Thu, 2018-03-08 at 17:47 +0800, rander.wang wrote:
Hi Liam,
On 3/8/2018 4:11 PM, Liam Girdwood wrote:
On Thu, 2018-03-08 at 14:39 +0800, rander.wang wrote:
hi Liam,
Please don't top post.
you mentioned : using IRQ core as 1st level IRQ handleri 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.
1st level handler does not check the DMAC. It only masks the 1st level IRQ and then calls all the 2nd level handlers. Each second level handler then checks if the IRQ is for itself and returns immediately if its not the IRQ owner.
i have discuss with Keyon. our idea is: maintain a list ofinterrupt function and private data
on second level data structure. In second level interrupt handler,call the irq functions
for each list member. can you give some advice?Btw, the levels I'm referring to here are parent/child level and not priority level (so it's also worth sending an earlier patch that does some renaming i.e. parent_level3_handler -> level2_priority3_handler and IRQ_NUM_EXT_LEVEL2 -> IRQ_NUM_EXT_PRI2) as this should clear up levels and priorities. .
1st level will be DSP interrupt - we then call second level handler..
2nd level will be IRQ controller level (i.e handler will determine which
child HW IP has the IRQ, DMAC, IPC, clock, SSP etc and call the 3rd level child handler).
- 3rd level handler will iterate through it's list of client handlers. In this
case it calls the handler for each DMAC.
This means we need the following :-
- generic level3 handler code needs to be added to
src/platform/cannonlake/interrupt.c. It will be similar to level2 handler in that it will loop through an array of clients.
- platform_irq_get_parent() will need modified to support 3rd level handlers.
It will return 3rd level handler for DW DMAC clients.
Does this make sense ?
i have some questions:
(1) in interrupt register function:
parent->child[REEF_IRQ_BIT(irq)]->handler = handler
parent->child[REEF_IRQ_BIT(irq)]->handler_arg = arg
for two DMA share the same irq bit, only one arg of dma can be recorded.
the issue also exists in interrupt unregister, disable, enable
(2) in interrupt function each child should check whether parent is register to system.
if not, register handler to system. The code only works for two level architecture,
not three level, and also for unregister.
I discuss with Keyon, to make three level interrupt algorithm work, the core code of
interrupt need to be refined. Anther method is to modify the irq_child, add a child list in it.
For linux kernel, interrupts are also recorded by a list, and check the list when interrupted
Or, to only make the modification in Cannonlake platform for irq difference, DMA irq
can be checked in parent_level5_handler, call the dma irq function two times with different
DMA data.
this is my idea, can you give some advice?
Rander
Liam
participants (3)
-
Liam Girdwood -
Rander Wang -
rander.wang