Re: [PATCH v8 08/10] ASoC: qcom: Add QAIF IRQ handling and platform register

[Harendra Gautam]

> > +             }
> > +             ret = regmap_update_bits(map,
> > +                                      qaif_dmacfg_reg(v, idx, substream->stream, dai_id),
> > +                                      QAIF_DMACFG_DYNCLK_BIT, 0);
> > +             if (ret)
> > +                     dev_err(soc_runtime->dev, "error disabling dma_dynclk: %d\n", ret);
>
> Should this preserve the first error while still attempting the
> IRQ-disable cleanup? As written, if clearing `QAIF_DMACFG_DYNCLK_BIT`
> fails but `qaif_platform_irq_op(..., QAIF_IRQ_DISABLE)` succeeds, `ret`
> is overwritten and the trigger callback returns 0.
This was overlooked. Will correct in next version.

>
> > +             ret = qaif_platform_irq_op(drvdata, substream->stream, irq_type,
> > +                                        idx, QAIF_IRQ_DISABLE);
> > +             if (ret)
> > +                     dev_err(soc_runtime->dev, "error disabling irq regs: %d\n", ret);
> > +             break;
>
> The QAIF programming guide's STOP sequences describe one more hardware
> completion step after clearing DMA ENABLE: wait for the relevant DMA
> shutdown status to complete, then clear/disable the associated IRQs.
> This applies to the AIF RDDMA/WRDMA paths and also to the codec
> RDDMA/WRDMA paths.
>
> This patch already has register definitions for the shutdown-status
> registers
> (`QAIF_RDDMA_SHUTDOWN_STAT_REG()`, `QAIF_WRDMA_SHUTDOWN_STAT_REG()`,
> QAIF_CDC_RDDMA_SHUTDOWN_STAT_REG(),&QAIF_CDC_WRDMA_SHUTDOWN_STAT_REG()),
> but I do not see them used in this stop path or in `sync_stop()`.
>
> Should the driver poll the appropriate `*_SHUTDOWN_STATUS` register, for
> example with `regmap_read_poll_timeout()`, before disabling DYNCLK and
> IRQs? Otherwise a fast STOP->START or suspend/resume path may race with
> a DMA channel that hardware still reports as shutting down.
>
> Thanks
> Ajay Kumar Nandam
>
Thanks Ajay for the deep thought here. This was intentional, on the
assumption that once a stream is closed the QAIF IP is inactive as its
clocks are gated. But you're right, concurrent or suspend/resume
scenarios can be affected without a complete shutdown. Will add the
full shutdown sequence in the next version.
-Harendra