| Message ID | 20230522133122.166841-6-Vijendar.Mukunda@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: amd: ps: add SoundWire support | expand |
On 5/22/23 08:31, Vijendar Mukunda wrote: > Initialize workqueue for SoundWire DMA interrupts handling. > Whenever audio data equal to the SoundWire FIFO watermark level > are produced/consumed, interrupt is generated. > Acknowledge the interrupt and schedule the workqueue. It would help to explain why a work queue is needed is the first place, as opposed to handling periods in the interrupt thread. > +static void acp63_sdw_dma_workthread(struct work_struct *work) > +{ > + struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data, > + acp_sdw_dma_work); > + struct sdw_dma_dev_data *sdw_dma_data; > + u32 stream_index; > + u16 pdev_index; > + > + pdev_index = adata->sdw_dma_dev_index; > + sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev); > + > + for (stream_index = 0; stream_index < ACP63_SDW0_DMA_MAX_STREAMS; stream_index++) { > + if (adata->sdw0_dma_intr_stat[stream_index]) { > + if (sdw_dma_data->sdw0_dma_stream[stream_index]) > + snd_pcm_period_elapsed(sdw_dma_data->sdw0_dma_stream[stream_index]); > + adata->sdw0_dma_intr_stat[stream_index] = 0; > + } > + } > + for (stream_index = 0; stream_index < ACP63_SDW1_DMA_MAX_STREAMS; stream_index++) { > + if (adata->sdw1_dma_intr_stat[stream_index]) { > + if (sdw_dma_data->sdw1_dma_stream[stream_index]) > + snd_pcm_period_elapsed(sdw_dma_data->sdw1_dma_stream[stream_index]); > + adata->sdw1_dma_intr_stat[stream_index] = 0; > + } > + } I am not clear on the benefits of the workqueue which only tests a flag that's set ... > +} > + > static irqreturn_t acp63_irq_handler(int irq, void *dev_id) > { > struct acp63_dev_data *adata; > struct pdm_dev_data *ps_pdm_data; > struct amd_sdw_manager *amd_manager; > u32 ext_intr_stat, ext_intr_stat1; > + u32 stream_id = 0; > u16 irq_flag = 0; > + u16 sdw_dma_irq_flag = 0; > u16 pdev_index; > + u16 index; > > adata = dev_id; > if (!adata) > @@ -148,7 +178,57 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id) > snd_pcm_period_elapsed(ps_pdm_data->capture_stream); > irq_flag = 1; > } > - if (irq_flag) > + if (ext_intr_stat & ACP_SDW_DMA_IRQ_MASK) { > + for (index = ACP_AUDIO2_RX_THRESHOLD; index <= ACP_AUDIO0_TX_THRESHOLD; index++) { > + if (ext_intr_stat & BIT(index)) { > + writel(BIT(index), adata->acp63_base + ACP_EXTERNAL_INTR_STAT); > + switch (index) { > + case ACP_AUDIO0_TX_THRESHOLD: > + stream_id = ACP_SDW0_AUDIO0_TX; > + break; > + case ACP_AUDIO1_TX_THRESHOLD: > + stream_id = ACP_SDW0_AUDIO1_TX; > + break; > + case ACP_AUDIO2_TX_THRESHOLD: > + stream_id = ACP_SDW0_AUDIO2_TX; > + break; > + case ACP_AUDIO0_RX_THRESHOLD: > + stream_id = ACP_SDW0_AUDIO0_RX; > + break; > + case ACP_AUDIO1_RX_THRESHOLD: > + stream_id = ACP_SDW0_AUDIO1_RX; > + break; > + case ACP_AUDIO2_RX_THRESHOLD: > + stream_id = ACP_SDW0_AUDIO2_RX; > + break; > + } > + > + adata->sdw0_dma_intr_stat[stream_id] = 1; .. here ... > + sdw_dma_irq_flag = 1; > + } > + } > + } > + > + /* SDW1 BT RX */ > + if (ext_intr_stat1 & ACP_P1_AUDIO1_RX_THRESHOLD) { > + writel(ACP_P1_AUDIO1_RX_THRESHOLD, > + adata->acp63_base + ACP_EXTERNAL_INTR_STAT1); > + adata->sdw1_dma_intr_stat[ACP_SDW1_AUDIO1_RX] = 1; ... and here ... > + sdw_dma_irq_flag = 1; > + } > + > + /* SDW1 BT TX*/ > + if (ext_intr_stat1 & ACP_P1_AUDIO1_TX_THRESHOLD) { > + writel(ACP_P1_AUDIO1_TX_THRESHOLD, > + adata->acp63_base + ACP_EXTERNAL_INTR_STAT1); > + adata->sdw1_dma_intr_stat[ACP_SDW1_AUDIO1_TX] = 1; ... or here ... > + sdw_dma_irq_flag = 1; > + } > + > + if (sdw_dma_irq_flag) > + schedule_work(&adata->acp_sdw_dma_work); > + > + if (irq_flag || sdw_dma_irq_flag) > return IRQ_HANDLED; > else > return IRQ_NONE;
On 22/05/23 23:42, Pierre-Louis Bossart wrote: > > On 5/22/23 08:31, Vijendar Mukunda wrote: >> Initialize workqueue for SoundWire DMA interrupts handling. >> Whenever audio data equal to the SoundWire FIFO watermark level >> are produced/consumed, interrupt is generated. >> Acknowledge the interrupt and schedule the workqueue. > It would help to explain why a work queue is needed is the first place, > as opposed to handling periods in the interrupt thread. For SoundWire DAI link, we are setting nonatomic flag to true. If we return period elapsed from hard irq handler instead of workqueue, soft lock up is observed during stream closure. We can use interrupt thread as well. To have a symmetry with SoundWire manager work queues, we have used workqueue for DMA interrupts. >> +static void acp63_sdw_dma_workthread(struct work_struct *work) >> +{ >> + struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data, >> + acp_sdw_dma_work); >> + struct sdw_dma_dev_data *sdw_dma_data; >> + u32 stream_index; >> + u16 pdev_index; >> + >> + pdev_index = adata->sdw_dma_dev_index; >> + sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev); >> + >> + for (stream_index = 0; stream_index < ACP63_SDW0_DMA_MAX_STREAMS; stream_index++) { >> + if (adata->sdw0_dma_intr_stat[stream_index]) { >> + if (sdw_dma_data->sdw0_dma_stream[stream_index]) >> + snd_pcm_period_elapsed(sdw_dma_data->sdw0_dma_stream[stream_index]); >> + adata->sdw0_dma_intr_stat[stream_index] = 0; >> + } >> + } >> + for (stream_index = 0; stream_index < ACP63_SDW1_DMA_MAX_STREAMS; stream_index++) { >> + if (adata->sdw1_dma_intr_stat[stream_index]) { >> + if (sdw_dma_data->sdw1_dma_stream[stream_index]) >> + snd_pcm_period_elapsed(sdw_dma_data->sdw1_dma_stream[stream_index]); >> + adata->sdw1_dma_intr_stat[stream_index] = 0; >> + } >> + } > I am not clear on the benefits of the workqueue which only tests a flag > that's set ... In top half, we are checking all stream irq mask and setting corresponding stream id index in interrupt status array when dma irq is raised. Our intention is to handle snd_pcm_period_elapsed in process context. if the flag is set, call the period elapsed for the substream based on stream id in work queue. > >> +} >> + >> static irqreturn_t acp63_irq_handler(int irq, void *dev_id) >> { >> struct acp63_dev_data *adata; >> struct pdm_dev_data *ps_pdm_data; >> struct amd_sdw_manager *amd_manager; >> u32 ext_intr_stat, ext_intr_stat1; >> + u32 stream_id = 0; >> u16 irq_flag = 0; >> + u16 sdw_dma_irq_flag = 0; >> u16 pdev_index; >> + u16 index; >> >> adata = dev_id; >> if (!adata) >> @@ -148,7 +178,57 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id) >> snd_pcm_period_elapsed(ps_pdm_data->capture_stream); >> irq_flag = 1; >> } >> - if (irq_flag) >> + if (ext_intr_stat & ACP_SDW_DMA_IRQ_MASK) { >> + for (index = ACP_AUDIO2_RX_THRESHOLD; index <= ACP_AUDIO0_TX_THRESHOLD; index++) { >> + if (ext_intr_stat & BIT(index)) { >> + writel(BIT(index), adata->acp63_base + ACP_EXTERNAL_INTR_STAT); >> + switch (index) { >> + case ACP_AUDIO0_TX_THRESHOLD: >> + stream_id = ACP_SDW0_AUDIO0_TX; >> + break; >> + case ACP_AUDIO1_TX_THRESHOLD: >> + stream_id = ACP_SDW0_AUDIO1_TX; >> + break; >> + case ACP_AUDIO2_TX_THRESHOLD: >> + stream_id = ACP_SDW0_AUDIO2_TX; >> + break; >> + case ACP_AUDIO0_RX_THRESHOLD: >> + stream_id = ACP_SDW0_AUDIO0_RX; >> + break; >> + case ACP_AUDIO1_RX_THRESHOLD: >> + stream_id = ACP_SDW0_AUDIO1_RX; >> + break; >> + case ACP_AUDIO2_RX_THRESHOLD: >> + stream_id = ACP_SDW0_AUDIO2_RX; >> + break; >> + } >> + >> + adata->sdw0_dma_intr_stat[stream_id] = 1; > .. here ... Please refer above comment. >> + sdw_dma_irq_flag = 1; >> + } >> + } >> + } >> + >> + /* SDW1 BT RX */ >> + if (ext_intr_stat1 & ACP_P1_AUDIO1_RX_THRESHOLD) { >> + writel(ACP_P1_AUDIO1_RX_THRESHOLD, >> + adata->acp63_base + ACP_EXTERNAL_INTR_STAT1); >> + adata->sdw1_dma_intr_stat[ACP_SDW1_AUDIO1_RX] = 1; > ... and here ... > >> + sdw_dma_irq_flag = 1; >> + } >> + >> + /* SDW1 BT TX*/ >> + if (ext_intr_stat1 & ACP_P1_AUDIO1_TX_THRESHOLD) { >> + writel(ACP_P1_AUDIO1_TX_THRESHOLD, >> + adata->acp63_base + ACP_EXTERNAL_INTR_STAT1); >> + adata->sdw1_dma_intr_stat[ACP_SDW1_AUDIO1_TX] = 1; > ... or here ... > >> + sdw_dma_irq_flag = 1; >> + } >> + >> + if (sdw_dma_irq_flag) >> + schedule_work(&adata->acp_sdw_dma_work); >> + >> + if (irq_flag || sdw_dma_irq_flag) >> return IRQ_HANDLED; >> else >> return IRQ_NONE;
On 5/23/23 02:36, Mukunda,Vijendar wrote: > On 22/05/23 23:42, Pierre-Louis Bossart wrote: >> >> On 5/22/23 08:31, Vijendar Mukunda wrote: >>> Initialize workqueue for SoundWire DMA interrupts handling. >>> Whenever audio data equal to the SoundWire FIFO watermark level >>> are produced/consumed, interrupt is generated. >>> Acknowledge the interrupt and schedule the workqueue. >> It would help to explain why a work queue is needed is the first place, >> as opposed to handling periods in the interrupt thread. > For SoundWire DAI link, we are setting nonatomic flag to true. > If we return period elapsed from hard irq handler instead of workqueue, > soft lock up is observed during stream closure. > > We can use interrupt thread as well. To have a symmetry with > SoundWire manager work queues, we have used workqueue for > DMA interrupts. Oh, I completely missed the model here. If you are using the bottom half/hard irq handler to read status information, the natural thing to do would be to have an irq thread, no? Not sure I see the benefit of aligning with the manager work queues - unless it makes your life simpler to avoid race conditions with cancel_work_sync()? >>> +static void acp63_sdw_dma_workthread(struct work_struct *work) >>> +{ >>> + struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data, >>> + acp_sdw_dma_work); >>> + struct sdw_dma_dev_data *sdw_dma_data; >>> + u32 stream_index; >>> + u16 pdev_index; >>> + >>> + pdev_index = adata->sdw_dma_dev_index; >>> + sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev); >>> + >>> + for (stream_index = 0; stream_index < ACP63_SDW0_DMA_MAX_STREAMS; stream_index++) { >>> + if (adata->sdw0_dma_intr_stat[stream_index]) { >>> + if (sdw_dma_data->sdw0_dma_stream[stream_index]) >>> + snd_pcm_period_elapsed(sdw_dma_data->sdw0_dma_stream[stream_index]); >>> + adata->sdw0_dma_intr_stat[stream_index] = 0; >>> + } >>> + } >>> + for (stream_index = 0; stream_index < ACP63_SDW1_DMA_MAX_STREAMS; stream_index++) { >>> + if (adata->sdw1_dma_intr_stat[stream_index]) { >>> + if (sdw_dma_data->sdw1_dma_stream[stream_index]) >>> + snd_pcm_period_elapsed(sdw_dma_data->sdw1_dma_stream[stream_index]); >>> + adata->sdw1_dma_intr_stat[stream_index] = 0; >>> + } >>> + } >> I am not clear on the benefits of the workqueue which only tests a flag >> that's set ... > In top half, we are checking all stream irq mask and setting > corresponding stream id index in interrupt status array when dma > irq is raised. > > Our intention is to handle snd_pcm_period_elapsed in process context. > if the flag is set, call the period elapsed for the substream based on stream > id in work queue.
On 23/05/23 20:30, Pierre-Louis Bossart wrote: > > On 5/23/23 02:36, Mukunda,Vijendar wrote: >> On 22/05/23 23:42, Pierre-Louis Bossart wrote: >>> On 5/22/23 08:31, Vijendar Mukunda wrote: >>>> Initialize workqueue for SoundWire DMA interrupts handling. >>>> Whenever audio data equal to the SoundWire FIFO watermark level >>>> are produced/consumed, interrupt is generated. >>>> Acknowledge the interrupt and schedule the workqueue. >>> It would help to explain why a work queue is needed is the first place, >>> as opposed to handling periods in the interrupt thread. >> For SoundWire DAI link, we are setting nonatomic flag to true. >> If we return period elapsed from hard irq handler instead of workqueue, >> soft lock up is observed during stream closure. >> >> We can use interrupt thread as well. To have a symmetry with >> SoundWire manager work queues, we have used workqueue for >> DMA interrupts. > Oh, I completely missed the model here. > > If you are using the bottom half/hard irq handler to read status > information, the natural thing to do would be to have an irq thread, no? > > Not sure I see the benefit of aligning with the manager work queues - > unless it makes your life simpler to avoid race conditions with > cancel_work_sync()? We can implement request_threaded_irq() and move the handling of DMA interrupts to thread function whereas we need to handle SoundWire manager interrupts in top half only. Reason as follows. As per our design, we are not masking the interrupts in top half and restoring mask after thread execution like Intel and our IP supports line based interrupts. If we move SoundWire manager interrupt handling to thread function, we have observed interrupts are reported but not handled properly due to thread execution is in progress sometimes. we will add comments for this design constraint in the code if we have to go with threaded_irq implementation. > >>>> +static void acp63_sdw_dma_workthread(struct work_struct *work) >>>> +{ >>>> + struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data, >>>> + acp_sdw_dma_work); >>>> + struct sdw_dma_dev_data *sdw_dma_data; >>>> + u32 stream_index; >>>> + u16 pdev_index; >>>> + >>>> + pdev_index = adata->sdw_dma_dev_index; >>>> + sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev); >>>> + >>>> + for (stream_index = 0; stream_index < ACP63_SDW0_DMA_MAX_STREAMS; stream_index++) { >>>> + if (adata->sdw0_dma_intr_stat[stream_index]) { >>>> + if (sdw_dma_data->sdw0_dma_stream[stream_index]) >>>> + snd_pcm_period_elapsed(sdw_dma_data->sdw0_dma_stream[stream_index]); >>>> + adata->sdw0_dma_intr_stat[stream_index] = 0; >>>> + } >>>> + } >>>> + for (stream_index = 0; stream_index < ACP63_SDW1_DMA_MAX_STREAMS; stream_index++) { >>>> + if (adata->sdw1_dma_intr_stat[stream_index]) { >>>> + if (sdw_dma_data->sdw1_dma_stream[stream_index]) >>>> + snd_pcm_period_elapsed(sdw_dma_data->sdw1_dma_stream[stream_index]); >>>> + adata->sdw1_dma_intr_stat[stream_index] = 0; >>>> + } >>>> + } >>> I am not clear on the benefits of the workqueue which only tests a flag >>> that's set ... >> In top half, we are checking all stream irq mask and setting >> corresponding stream id index in interrupt status array when dma >> irq is raised. >> >> Our intention is to handle snd_pcm_period_elapsed in process context. >> if the flag is set, call the period elapsed for the substream based on stream >> id in work queue.
On 24/05/23 13:15, Mukunda,Vijendar wrote: > On 23/05/23 20:30, Pierre-Louis Bossart wrote: >> On 5/23/23 02:36, Mukunda,Vijendar wrote: >>> On 22/05/23 23:42, Pierre-Louis Bossart wrote: >>>> On 5/22/23 08:31, Vijendar Mukunda wrote: >>>>> Initialize workqueue for SoundWire DMA interrupts handling. >>>>> Whenever audio data equal to the SoundWire FIFO watermark level >>>>> are produced/consumed, interrupt is generated. >>>>> Acknowledge the interrupt and schedule the workqueue. >>>> It would help to explain why a work queue is needed is the first place, >>>> as opposed to handling periods in the interrupt thread. >>> For SoundWire DAI link, we are setting nonatomic flag to true. >>> If we return period elapsed from hard irq handler instead of workqueue, >>> soft lock up is observed during stream closure. >>> >>> We can use interrupt thread as well. To have a symmetry with >>> SoundWire manager work queues, we have used workqueue for >>> DMA interrupts. >> Oh, I completely missed the model here. >> >> If you are using the bottom half/hard irq handler to read status >> information, the natural thing to do would be to have an irq thread, no? >> >> Not sure I see the benefit of aligning with the manager work queues - >> unless it makes your life simpler to avoid race conditions with >> cancel_work_sync()? > We can implement request_threaded_irq() and move the handling of > DMA interrupts to thread function whereas we need to handle SoundWire > manager interrupts in top half only. Reason as follows. > > As per our design, we are not masking the interrupts in top half and > restoring mask after thread execution like Intel and > our IP supports line based interrupts. If we move SoundWire manager > interrupt handling to thread function, we have observed interrupts are > reported but not handled properly due to thread execution is in progress > sometimes. > we will add comments for this design constraint in the code if we have to > go with threaded_irq implementation. > > @Bossart: we are waiting for your reply. >>>>> +static void acp63_sdw_dma_workthread(struct work_struct *work) >>>>> +{ >>>>> + struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data, >>>>> + acp_sdw_dma_work); >>>>> + struct sdw_dma_dev_data *sdw_dma_data; >>>>> + u32 stream_index; >>>>> + u16 pdev_index; >>>>> + >>>>> + pdev_index = adata->sdw_dma_dev_index; >>>>> + sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev); >>>>> + >>>>> + for (stream_index = 0; stream_index < ACP63_SDW0_DMA_MAX_STREAMS; stream_index++) { >>>>> + if (adata->sdw0_dma_intr_stat[stream_index]) { >>>>> + if (sdw_dma_data->sdw0_dma_stream[stream_index]) >>>>> + snd_pcm_period_elapsed(sdw_dma_data->sdw0_dma_stream[stream_index]); >>>>> + adata->sdw0_dma_intr_stat[stream_index] = 0; >>>>> + } >>>>> + } >>>>> + for (stream_index = 0; stream_index < ACP63_SDW1_DMA_MAX_STREAMS; stream_index++) { >>>>> + if (adata->sdw1_dma_intr_stat[stream_index]) { >>>>> + if (sdw_dma_data->sdw1_dma_stream[stream_index]) >>>>> + snd_pcm_period_elapsed(sdw_dma_data->sdw1_dma_stream[stream_index]); >>>>> + adata->sdw1_dma_intr_stat[stream_index] = 0; >>>>> + } >>>>> + } >>>> I am not clear on the benefits of the workqueue which only tests a flag >>>> that's set ... >>> In top half, we are checking all stream irq mask and setting >>> corresponding stream id index in interrupt status array when dma >>> irq is raised. >>> >>> Our intention is to handle snd_pcm_period_elapsed in process context. >>> if the flag is set, call the period elapsed for the substream based on stream >>> id in work queue.
On 31/05/23 19:23, Pierre-Louis Bossart wrote: > > On 5/31/23 02:28, Mukunda,Vijendar wrote: >> On 24/05/23 13:15, Mukunda,Vijendar wrote: >>> On 23/05/23 20:30, Pierre-Louis Bossart wrote: >>>> On 5/23/23 02:36, Mukunda,Vijendar wrote: >>>>> On 22/05/23 23:42, Pierre-Louis Bossart wrote: >>>>>> On 5/22/23 08:31, Vijendar Mukunda wrote: >>>>>>> Initialize workqueue for SoundWire DMA interrupts handling. >>>>>>> Whenever audio data equal to the SoundWire FIFO watermark level >>>>>>> are produced/consumed, interrupt is generated. >>>>>>> Acknowledge the interrupt and schedule the workqueue. >>>>>> It would help to explain why a work queue is needed is the first place, >>>>>> as opposed to handling periods in the interrupt thread. >>>>> For SoundWire DAI link, we are setting nonatomic flag to true. >>>>> If we return period elapsed from hard irq handler instead of workqueue, >>>>> soft lock up is observed during stream closure. >>>>> >>>>> We can use interrupt thread as well. To have a symmetry with >>>>> SoundWire manager work queues, we have used workqueue for >>>>> DMA interrupts. >>>> Oh, I completely missed the model here. >>>> >>>> If you are using the bottom half/hard irq handler to read status >>>> information, the natural thing to do would be to have an irq thread, no? >>>> >>>> Not sure I see the benefit of aligning with the manager work queues - >>>> unless it makes your life simpler to avoid race conditions with >>>> cancel_work_sync()? >>> We can implement request_threaded_irq() and move the handling of >>> DMA interrupts to thread function whereas we need to handle SoundWire >>> manager interrupts in top half only. Reason as follows. >>> >>> As per our design, we are not masking the interrupts in top half and >>> restoring mask after thread execution like Intel and >>> our IP supports line based interrupts. If we move SoundWire manager >>> interrupt handling to thread function, we have observed interrupts are >>> reported but not handled properly due to thread execution is in progress >>> sometimes. >>> we will add comments for this design constraint in the code if we have to >>> go with threaded_irq implementation. >>> >>> @Bossart: we are waiting for your reply. > I am not sure I get the point about using workqueues v. threads for the > manager, which in turn makes it difficult to understand why the DMA > interrupt handling should be aligned with that of the manager interrupt > handling. > > Using the combination of hard irq handler + workqueue feels odd. I may > very well 'work' but others should chime in since I am far from the most > knowledgeable reviewer in this area. Understood your point. We will use irq thread instead of workqueue for SoundWire DMA interrupts handling. We will push V3 version. > >>>>>>> +static void acp63_sdw_dma_workthread(struct work_struct *work) >>>>>>> +{ >>>>>>> + struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data, >>>>>>> + acp_sdw_dma_work); >>>>>>> + struct sdw_dma_dev_data *sdw_dma_data; >>>>>>> + u32 stream_index; >>>>>>> + u16 pdev_index; >>>>>>> + >>>>>>> + pdev_index = adata->sdw_dma_dev_index; >>>>>>> + sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev); >>>>>>> + >>>>>>> + for (stream_index = 0; stream_index < ACP63_SDW0_DMA_MAX_STREAMS; stream_index++) { >>>>>>> + if (adata->sdw0_dma_intr_stat[stream_index]) { >>>>>>> + if (sdw_dma_data->sdw0_dma_stream[stream_index]) >>>>>>> + snd_pcm_period_elapsed(sdw_dma_data->sdw0_dma_stream[stream_index]); >>>>>>> + adata->sdw0_dma_intr_stat[stream_index] = 0; >>>>>>> + } >>>>>>> + } >>>>>>> + for (stream_index = 0; stream_index < ACP63_SDW1_DMA_MAX_STREAMS; stream_index++) { >>>>>>> + if (adata->sdw1_dma_intr_stat[stream_index]) { >>>>>>> + if (sdw_dma_data->sdw1_dma_stream[stream_index]) >>>>>>> + snd_pcm_period_elapsed(sdw_dma_data->sdw1_dma_stream[stream_index]); >>>>>>> + adata->sdw1_dma_intr_stat[stream_index] = 0; >>>>>>> + } >>>>>>> + } >>>>>> I am not clear on the benefits of the workqueue which only tests a flag >>>>>> that's set ... >>>>> In top half, we are checking all stream irq mask and setting >>>>> corresponding stream id index in interrupt status array when dma >>>>> irq is raised. >>>>> >>>>> Our intention is to handle snd_pcm_period_elapsed in process context. >>>>> if the flag is set, call the period elapsed for the substream based on stream >>>>> id in work queue.
diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h index 37ae8f18225a..1a3e47fb9d02 100644 --- a/sound/soc/amd/ps/acp63.h +++ b/sound/soc/amd/ps/acp63.h @@ -173,6 +173,20 @@ enum acp_pdev_mask { ACP63_SDW_PDM_DEV_MASK, }; +enum amd_sdw0_channel { + ACP_SDW0_AUDIO0_TX = 0, + ACP_SDW0_AUDIO1_TX, + ACP_SDW0_AUDIO2_TX, + ACP_SDW0_AUDIO0_RX, + ACP_SDW0_AUDIO1_RX, + ACP_SDW0_AUDIO2_RX, +}; + +enum amd_sdw1_channel { + ACP_SDW1_AUDIO1_TX, + ACP_SDW1_AUDIO1_RX, +}; + struct pdm_stream_instance { u16 num_pages; u16 channels; @@ -230,6 +244,7 @@ struct sdw_dma_ring_buf_reg { * @pdev: array of child platform device node structures * @acp_lock: used to protect acp common registers * @sdw_fw_node: SoundWire controller fw node handle + * @acp_sdw_dma_work: SoundWire DMA interrupts workqueue * @pdev_mask: platform device mask * @pdev_count: platform devices count * @pdm_dev_index: pdm platform device index @@ -237,6 +252,8 @@ struct sdw_dma_ring_buf_reg { * @sdw0_dev_index: SoundWire Manager-0 platform device index * @sdw1_dev_index: SoundWire Manager-1 platform device index * @sdw_dma_dev_index: SoundWire DMA controller platform device index + * @sdw0-dma_intr_stat: DMA interrupt status array for SoundWire manager-SW0 instance + * @sdw_dma_intr_stat: DMA interrupt status array for SoundWire manager-SW1 instance * @acp_reset: flag set to true when bus reset is applied across all * the active SoundWire manager instances */ @@ -247,6 +264,7 @@ struct acp63_dev_data { struct platform_device *pdev[ACP63_DEVS]; struct mutex acp_lock; /* protect shared registers */ struct fwnode_handle *sdw_fw_node; + struct work_struct acp_sdw_dma_work; u16 pdev_mask; u16 pdev_count; u16 pdm_dev_index; @@ -254,6 +272,8 @@ struct acp63_dev_data { u16 sdw0_dev_index; u16 sdw1_dev_index; u16 sdw_dma_dev_index; + u16 sdw0_dma_intr_stat[ACP63_SDW0_DMA_MAX_STREAMS]; + u16 sdw1_dma_intr_stat[ACP63_SDW1_DMA_MAX_STREAMS]; bool acp_reset; }; diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c index 26514e340a33..c99ac5d90097 100644 --- a/sound/soc/amd/ps/pci-ps.c +++ b/sound/soc/amd/ps/pci-ps.c @@ -99,14 +99,44 @@ static int acp63_deinit(void __iomem *acp_base, struct device *dev) return 0; } +static void acp63_sdw_dma_workthread(struct work_struct *work) +{ + struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data, + acp_sdw_dma_work); + struct sdw_dma_dev_data *sdw_dma_data; + u32 stream_index; + u16 pdev_index; + + pdev_index = adata->sdw_dma_dev_index; + sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev); + + for (stream_index = 0; stream_index < ACP63_SDW0_DMA_MAX_STREAMS; stream_index++) { + if (adata->sdw0_dma_intr_stat[stream_index]) { + if (sdw_dma_data->sdw0_dma_stream[stream_index]) + snd_pcm_period_elapsed(sdw_dma_data->sdw0_dma_stream[stream_index]); + adata->sdw0_dma_intr_stat[stream_index] = 0; + } + } + for (stream_index = 0; stream_index < ACP63_SDW1_DMA_MAX_STREAMS; stream_index++) { + if (adata->sdw1_dma_intr_stat[stream_index]) { + if (sdw_dma_data->sdw1_dma_stream[stream_index]) + snd_pcm_period_elapsed(sdw_dma_data->sdw1_dma_stream[stream_index]); + adata->sdw1_dma_intr_stat[stream_index] = 0; + } + } +} + static irqreturn_t acp63_irq_handler(int irq, void *dev_id) { struct acp63_dev_data *adata; struct pdm_dev_data *ps_pdm_data; struct amd_sdw_manager *amd_manager; u32 ext_intr_stat, ext_intr_stat1; + u32 stream_id = 0; u16 irq_flag = 0; + u16 sdw_dma_irq_flag = 0; u16 pdev_index; + u16 index; adata = dev_id; if (!adata) @@ -148,7 +178,57 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id) snd_pcm_period_elapsed(ps_pdm_data->capture_stream); irq_flag = 1; } - if (irq_flag) + if (ext_intr_stat & ACP_SDW_DMA_IRQ_MASK) { + for (index = ACP_AUDIO2_RX_THRESHOLD; index <= ACP_AUDIO0_TX_THRESHOLD; index++) { + if (ext_intr_stat & BIT(index)) { + writel(BIT(index), adata->acp63_base + ACP_EXTERNAL_INTR_STAT); + switch (index) { + case ACP_AUDIO0_TX_THRESHOLD: + stream_id = ACP_SDW0_AUDIO0_TX; + break; + case ACP_AUDIO1_TX_THRESHOLD: + stream_id = ACP_SDW0_AUDIO1_TX; + break; + case ACP_AUDIO2_TX_THRESHOLD: + stream_id = ACP_SDW0_AUDIO2_TX; + break; + case ACP_AUDIO0_RX_THRESHOLD: + stream_id = ACP_SDW0_AUDIO0_RX; + break; + case ACP_AUDIO1_RX_THRESHOLD: + stream_id = ACP_SDW0_AUDIO1_RX; + break; + case ACP_AUDIO2_RX_THRESHOLD: + stream_id = ACP_SDW0_AUDIO2_RX; + break; + } + + adata->sdw0_dma_intr_stat[stream_id] = 1; + sdw_dma_irq_flag = 1; + } + } + } + + /* SDW1 BT RX */ + if (ext_intr_stat1 & ACP_P1_AUDIO1_RX_THRESHOLD) { + writel(ACP_P1_AUDIO1_RX_THRESHOLD, + adata->acp63_base + ACP_EXTERNAL_INTR_STAT1); + adata->sdw1_dma_intr_stat[ACP_SDW1_AUDIO1_RX] = 1; + sdw_dma_irq_flag = 1; + } + + /* SDW1 BT TX*/ + if (ext_intr_stat1 & ACP_P1_AUDIO1_TX_THRESHOLD) { + writel(ACP_P1_AUDIO1_TX_THRESHOLD, + adata->acp63_base + ACP_EXTERNAL_INTR_STAT1); + adata->sdw1_dma_intr_stat[ACP_SDW1_AUDIO1_TX] = 1; + sdw_dma_irq_flag = 1; + } + + if (sdw_dma_irq_flag) + schedule_work(&adata->acp_sdw_dma_work); + + if (irq_flag || sdw_dma_irq_flag) return IRQ_HANDLED; else return IRQ_NONE; @@ -231,6 +311,7 @@ static int get_acp63_device_config(u32 config, struct pci_dev *pci, struct acp63 sdw_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_SDW_ADDR, 0); if (sdw_dev) { acp_data->sdw_fw_node = acpi_fwnode_handle(sdw_dev); + INIT_WORK(&acp_data->acp_sdw_dma_work, acp63_sdw_dma_workthread); ret = sdw_amd_scan_controller(&pci->dev); /* is_sdw_dev flag will be set when SoundWire Manager device exists */ if (!ret) @@ -610,6 +691,10 @@ static void snd_acp63_remove(struct pci_dev *pci) int ret, index; adata = pci_get_drvdata(pci); + + if (adata->pdev_mask & ACP63_SDW_DEV_MASK) + cancel_work_sync(&adata->acp_sdw_dma_work); + for (index = 0; index < adata->pdev_count; index++) platform_device_unregister(adata->pdev[index]); ret = acp63_deinit(adata->acp63_base, &pci->dev);
Initialize workqueue for SoundWire DMA interrupts handling. Whenever audio data equal to the SoundWire FIFO watermark level are produced/consumed, interrupt is generated. Acknowledge the interrupt and schedule the workqueue. Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> --- sound/soc/amd/ps/acp63.h | 20 +++++++++ sound/soc/amd/ps/pci-ps.c | 87 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 106 insertions(+), 1 deletion(-)