[Sound-open-firmware] [PATCH] cnl: gp-dma: fix gp-dma dead loop issue

[Liam Girdwood]

On Thu, 2018-03-01 at 08:59 +0000, Wang, Rander wrote:
> Hi Liam,
> 
> 	See my comments
> 
> Thank you！
> Rander
> > -----Original Message-----
> > From: Liam Girdwood [mailto:liam.r.girdwood at linux.intel.com]
> > Sent: Wednesday, February 28, 2018 5:23 PM
> > To: Wang, Rander <rander.wang at intel.com>;
> > sound-open-firmware at alsa-project.org
> > Subject: Re: [Sound-open-firmware] [PATCH] cnl: gp-dma: fix gp-dma
> > dead loop
> > issue
> > 
> > On Wed, 2018-02-28 at 16:52 +0800, Rander Wang wrote:
> > > In irq function, two dma would be checked which one is the
> > > interrupted
> > > one, so get it after check.
> > > 
> > > Signed-off-by: Rander Wang <rander.wang at intel.com>
> > > ---
> > >  src/drivers/dw-dma.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/drivers/dw-dma.c b/src/drivers/dw-dma.c index
> > > be19e12..6fd46e0 100644
> > > --- a/src/drivers/dw-dma.c
> > > +++ b/src/drivers/dw-dma.c
> > > @@ -1034,7 +1034,7 @@ static int dw_dma_probe(struct dma *dma)
> > 
> > static
> > > void dw_dma_irq_handler(void *data)  {
> > >  	struct dma *dma = (struct dma *)data;
> > > -	struct dma_pdata *p = dma_get_drvdata(dma);
> > > +	struct dma_pdata *p;
> > >  	struct dma_sg_elem next;
> > >  	uint32_t status_tfr = 0;
> > >  	uint32_t status_block = 0;
> > > @@ -1058,6 +1058,7 @@ static void dw_dma_irq_handler(void *data)
> > >  	}
> > > 
> > >  	tracev_dma("DIr");
> > > +	p = dma_get_drvdata(dma);
> > > 
> > >  	/* get the source of our IRQ. */
> > >  	status_block = dw_read(dma, DW_STATUS_BLOCK);
> > 
> > 
> > Iiuc, the IRQ for both DMACs on CNL is shared ? and we determine
> > which
> > DMAC has the IRQ by reading each status register above ?
> > 
> > If so, this fails when both DMACs have IRQs at the same time.
> 
> If only one core is enabled, the interrupt with the same irq level
> would trigger
> Irq function two times?

No, the IRQ line is ORed between both DMACs. Either or both will cause
and IRQ. Both status register must be read though.

> 
> And if multicore is enabled, if two core received the interrupt at
> the same time,
> Maybe something wrong?


That is possible, but the IRQ should be masked in other cores.

Btw, this is wrong too:-

static int dw_dma_probe(struct dma *dma)
{
	struct dma_pdata *dw_pdata;
	struct dma *dmac = dma;
	int i;
#ifdef CONFIG_CANNONLAKE
	int j;

	for (j = 0; j < MAX_GPDMA_COUNT; j++)
#endif
	{

We dont need this for loop. dw_dma_probe () will be called in
platform.c for each DMAC.


> 
> > Please do something like :-
> > 
> > dw_dma_irq_handler_cnl()
> > {
> > 	dma = dmac0;
> > 
> > 	/* read status for DMAC0 */
> > 	status_intr = dw_read(dma, DW_INTR_STATUS);
> > 	if (status_intr)
> > 		dw_dma_irq_handler(dma);
> > 
> > 	dma = dmac1;
> > 
> > 	/* read status for DMAC1 */
> > 	status_intr = dw_read(dma, DW_INTR_STATUS);
> > 	if (status_intr)
> > 		dw_dma_irq_handler(dma);
> > }
> > 
> > Liam