| Message ID | 20230522133122.166841-3-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: > Handle SoundWire manager related interrupts in ACP PCI driver > interrupt handler and schedule SoundWire manager work queue for > further processing. > > Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> > --- > sound/soc/amd/ps/acp63.h | 4 ++++ > sound/soc/amd/ps/pci-ps.c | 43 ++++++++++++++++++++++++++++++++++----- > 2 files changed, 42 insertions(+), 5 deletions(-) > > diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h > index 95bb1cef900a..d296059be4f0 100644 > --- a/sound/soc/amd/ps/acp63.h > +++ b/sound/soc/amd/ps/acp63.h > @@ -88,6 +88,10 @@ > /* time in ms for acp timeout */ > #define ACP_TIMEOUT 500 > > +#define ACP_SDW0_STAT BIT(21) > +#define ACP_SDW1_STAT BIT(2) > +#define ACP_ERROR_IRQ BIT(29) > + > enum acp_config { > ACP_CONFIG_0 = 0, > ACP_CONFIG_1, > diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c > index 02caae6968ad..26514e340a33 100644 > --- a/sound/soc/amd/ps/pci-ps.c > +++ b/sound/soc/amd/ps/pci-ps.c > @@ -56,6 +56,7 @@ static int acp63_reset(void __iomem *acp_base) > static void acp63_enable_interrupts(void __iomem *acp_base) > { > writel(1, acp_base + ACP_EXTERNAL_INTR_ENB); > + writel(ACP_ERROR_IRQ, acp_base + ACP_EXTERNAL_INTR_CNTL); you may want to comment on why you don't have a read-modify-write approach for something that looks generic and not limited to a single error bit? > } > > static void acp63_disable_interrupts(void __iomem *acp_base) > @@ -102,23 +103,55 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id) > { > struct acp63_dev_data *adata; > struct pdm_dev_data *ps_pdm_data; > - u32 val; > + struct amd_sdw_manager *amd_manager; > + u32 ext_intr_stat, ext_intr_stat1; > + u16 irq_flag = 0; > u16 pdev_index; > > adata = dev_id; > if (!adata) > return IRQ_NONE; > + ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT); > + if (ext_intr_stat & ACP_SDW0_STAT) { > + writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT); [1] this is confusing, if this is w1c, should this be: writel(ext_intr_stat, adata->acp63_base + ACP_EXTERNAL_INTR_STAT); Otherwise you may be clearing fields that have not been set? > + pdev_index = adata->sdw0_dev_index; > + amd_manager = dev_get_drvdata(&adata->pdev[pdev_index]->dev); > + if (amd_manager) > + schedule_work(&amd_manager->amd_sdw_irq_thread); > + irq_flag = 1; > + } > > - val = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT); > - if (val & BIT(PDM_DMA_STAT)) { > + ext_intr_stat1 = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT1); > + if (ext_intr_stat1 & ACP_SDW1_STAT) { > + writel(ACP_SDW1_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT1); same comment here. > + pdev_index = adata->sdw1_dev_index; > + amd_manager = dev_get_drvdata(&adata->pdev[pdev_index]->dev); > + if (amd_manager) > + schedule_work(&amd_manager->amd_sdw_irq_thread); > + irq_flag = 1; > + } > + > + if (ext_intr_stat & ACP_ERROR_IRQ) { > + writel(ACP_ERROR_IRQ, adata->acp63_base + ACP_EXTERNAL_INTR_STAT); [2] and this is even move confusing because you may end-up writing twice to the same adata->acp63_base + ACP_EXTERNAL_INTR_STAT with [1] and [2], so the previous write writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT); may have cleared the register already. Something looks wrong here? > + /* TODO: Report SoundWire Manager instance errors */ > + writel(0, adata->acp63_base + ACP_SW0_I2S_ERROR_REASON); > + writel(0, adata->acp63_base + ACP_SW1_I2S_ERROR_REASON); > + writel(0, adata->acp63_base + ACP_ERROR_STATUS); > + irq_flag = 1; > + } > + > + if (ext_intr_stat & BIT(PDM_DMA_STAT)) { > pdev_index = adata->pdm_dev_index; > ps_pdm_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev); > writel(BIT(PDM_DMA_STAT), adata->acp63_base + ACP_EXTERNAL_INTR_STAT); > if (ps_pdm_data->capture_stream) > snd_pcm_period_elapsed(ps_pdm_data->capture_stream); > - return IRQ_HANDLED; > + irq_flag = 1; > } > - return IRQ_NONE; > + if (irq_flag) > + return IRQ_HANDLED; > + else > + return IRQ_NONE; > } > > static int sdw_amd_scan_controller(struct device *dev)
On 22/05/23 21:56, Pierre-Louis Bossart wrote: > > On 5/22/23 08:31, Vijendar Mukunda wrote: >> Handle SoundWire manager related interrupts in ACP PCI driver >> interrupt handler and schedule SoundWire manager work queue for >> further processing. >> >> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> >> --- >> sound/soc/amd/ps/acp63.h | 4 ++++ >> sound/soc/amd/ps/pci-ps.c | 43 ++++++++++++++++++++++++++++++++++----- >> 2 files changed, 42 insertions(+), 5 deletions(-) >> >> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h >> index 95bb1cef900a..d296059be4f0 100644 >> --- a/sound/soc/amd/ps/acp63.h >> +++ b/sound/soc/amd/ps/acp63.h >> @@ -88,6 +88,10 @@ >> /* time in ms for acp timeout */ >> #define ACP_TIMEOUT 500 >> >> +#define ACP_SDW0_STAT BIT(21) >> +#define ACP_SDW1_STAT BIT(2) >> +#define ACP_ERROR_IRQ BIT(29) >> + >> enum acp_config { >> ACP_CONFIG_0 = 0, >> ACP_CONFIG_1, >> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c >> index 02caae6968ad..26514e340a33 100644 >> --- a/sound/soc/amd/ps/pci-ps.c >> +++ b/sound/soc/amd/ps/pci-ps.c >> @@ -56,6 +56,7 @@ static int acp63_reset(void __iomem *acp_base) >> static void acp63_enable_interrupts(void __iomem *acp_base) >> { >> writel(1, acp_base + ACP_EXTERNAL_INTR_ENB); >> + writel(ACP_ERROR_IRQ, acp_base + ACP_EXTERNAL_INTR_CNTL); > you may want to comment on why you don't have a read-modify-write > approach for something that looks generic and not limited to a single > error bit? This function will be called when ACP enters D0 state, or during probe sequence. ACP reset will be applied which will put all registers to default value. In this case, ACP_EXTERNAL_INTR_CNTL register default value is zero. Our intention is to set only ACP error mask in acp external control register. Rest of the places, IRQ mask programming will use read-modify-write. >> } >> >> static void acp63_disable_interrupts(void __iomem *acp_base) >> @@ -102,23 +103,55 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id) >> { >> struct acp63_dev_data *adata; >> struct pdm_dev_data *ps_pdm_data; >> - u32 val; >> + struct amd_sdw_manager *amd_manager; >> + u32 ext_intr_stat, ext_intr_stat1; >> + u16 irq_flag = 0; >> u16 pdev_index; >> >> adata = dev_id; >> if (!adata) >> return IRQ_NONE; >> + ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT); >> + if (ext_intr_stat & ACP_SDW0_STAT) { >> + writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT); > [1] > > this is confusing, if this is w1c, should this be: > > writel(ext_intr_stat, adata->acp63_base + ACP_EXTERNAL_INTR_STAT); > > Otherwise you may be clearing fields that have not been set? As per our register spec, writing zero to register fields doesn't have any effect. Only writing 1 to register bit will clear that interrupt. We are handling bit by bit irq check and clearing the irq mask based on irq bit and take an action related to that particular irq bit. > >> + pdev_index = adata->sdw0_dev_index; >> + amd_manager = dev_get_drvdata(&adata->pdev[pdev_index]->dev); >> + if (amd_manager) >> + schedule_work(&amd_manager->amd_sdw_irq_thread); >> + irq_flag = 1; >> + } >> >> - val = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT); >> - if (val & BIT(PDM_DMA_STAT)) { >> + ext_intr_stat1 = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT1); >> + if (ext_intr_stat1 & ACP_SDW1_STAT) { >> + writel(ACP_SDW1_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT1); > same comment here. > >> + pdev_index = adata->sdw1_dev_index; >> + amd_manager = dev_get_drvdata(&adata->pdev[pdev_index]->dev); >> + if (amd_manager) >> + schedule_work(&amd_manager->amd_sdw_irq_thread); >> + irq_flag = 1; >> + } >> + >> + if (ext_intr_stat & ACP_ERROR_IRQ) { >> + writel(ACP_ERROR_IRQ, adata->acp63_base + ACP_EXTERNAL_INTR_STAT); > [2] > > and this is even move confusing because you may end-up writing twice to > the same adata->acp63_base + ACP_EXTERNAL_INTR_STAT with [1] and [2], so > the previous write > > writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT); > > may have cleared the register already. > > Something looks wrong here? As mentioned above, writing zero doesn't have any effect. We are handling bit by bit in irq stat register. > >> + /* TODO: Report SoundWire Manager instance errors */ >> + writel(0, adata->acp63_base + ACP_SW0_I2S_ERROR_REASON); >> + writel(0, adata->acp63_base + ACP_SW1_I2S_ERROR_REASON); >> + writel(0, adata->acp63_base + ACP_ERROR_STATUS); >> + irq_flag = 1; >> + } >> + >> + if (ext_intr_stat & BIT(PDM_DMA_STAT)) { >> pdev_index = adata->pdm_dev_index; >> ps_pdm_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev); >> writel(BIT(PDM_DMA_STAT), adata->acp63_base + ACP_EXTERNAL_INTR_STAT); >> if (ps_pdm_data->capture_stream) >> snd_pcm_period_elapsed(ps_pdm_data->capture_stream); >> - return IRQ_HANDLED; >> + irq_flag = 1; >> } >> - return IRQ_NONE; >> + if (irq_flag) >> + return IRQ_HANDLED; >> + else >> + return IRQ_NONE; >> } >> >> static int sdw_amd_scan_controller(struct device *dev)
>>> static void acp63_disable_interrupts(void __iomem *acp_base) >>> @@ -102,23 +103,55 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id) >>> { >>> struct acp63_dev_data *adata; >>> struct pdm_dev_data *ps_pdm_data; >>> - u32 val; >>> + struct amd_sdw_manager *amd_manager; >>> + u32 ext_intr_stat, ext_intr_stat1; >>> + u16 irq_flag = 0; >>> u16 pdev_index; >>> >>> adata = dev_id; >>> if (!adata) >>> return IRQ_NONE; >>> + ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT); >>> + if (ext_intr_stat & ACP_SDW0_STAT) { >>> + writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT); >> [1] >> >> this is confusing, if this is w1c, should this be: >> >> writel(ext_intr_stat, adata->acp63_base + ACP_EXTERNAL_INTR_STAT); >> >> Otherwise you may be clearing fields that have not been set? > As per our register spec, writing zero to register fields doesn't > have any effect. Only writing 1 to register bit will clear that > interrupt. > We are handling bit by bit irq check and clearing the irq mask > based on irq bit and take an action related to that particular irq > bit. Right, maybe an explanation in the commit message would help.
On 23/05/23 20:04, Pierre-Louis Bossart wrote: > >>>> static void acp63_disable_interrupts(void __iomem *acp_base) >>>> @@ -102,23 +103,55 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id) >>>> { >>>> struct acp63_dev_data *adata; >>>> struct pdm_dev_data *ps_pdm_data; >>>> - u32 val; >>>> + struct amd_sdw_manager *amd_manager; >>>> + u32 ext_intr_stat, ext_intr_stat1; >>>> + u16 irq_flag = 0; >>>> u16 pdev_index; >>>> >>>> adata = dev_id; >>>> if (!adata) >>>> return IRQ_NONE; >>>> + ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT); >>>> + if (ext_intr_stat & ACP_SDW0_STAT) { >>>> + writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT); >>> [1] >>> >>> this is confusing, if this is w1c, should this be: >>> >>> writel(ext_intr_stat, adata->acp63_base + ACP_EXTERNAL_INTR_STAT); >>> >>> Otherwise you may be clearing fields that have not been set? >> As per our register spec, writing zero to register fields doesn't >> have any effect. Only writing 1 to register bit will clear that >> interrupt. >> We are handling bit by bit irq check and clearing the irq mask >> based on irq bit and take an action related to that particular irq >> bit. > Right, maybe an explanation in the commit message would help. will update the commit message.
diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h index 95bb1cef900a..d296059be4f0 100644 --- a/sound/soc/amd/ps/acp63.h +++ b/sound/soc/amd/ps/acp63.h @@ -88,6 +88,10 @@ /* time in ms for acp timeout */ #define ACP_TIMEOUT 500 +#define ACP_SDW0_STAT BIT(21) +#define ACP_SDW1_STAT BIT(2) +#define ACP_ERROR_IRQ BIT(29) + enum acp_config { ACP_CONFIG_0 = 0, ACP_CONFIG_1, diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c index 02caae6968ad..26514e340a33 100644 --- a/sound/soc/amd/ps/pci-ps.c +++ b/sound/soc/amd/ps/pci-ps.c @@ -56,6 +56,7 @@ static int acp63_reset(void __iomem *acp_base) static void acp63_enable_interrupts(void __iomem *acp_base) { writel(1, acp_base + ACP_EXTERNAL_INTR_ENB); + writel(ACP_ERROR_IRQ, acp_base + ACP_EXTERNAL_INTR_CNTL); } static void acp63_disable_interrupts(void __iomem *acp_base) @@ -102,23 +103,55 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id) { struct acp63_dev_data *adata; struct pdm_dev_data *ps_pdm_data; - u32 val; + struct amd_sdw_manager *amd_manager; + u32 ext_intr_stat, ext_intr_stat1; + u16 irq_flag = 0; u16 pdev_index; adata = dev_id; if (!adata) return IRQ_NONE; + ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT); + if (ext_intr_stat & ACP_SDW0_STAT) { + writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT); + pdev_index = adata->sdw0_dev_index; + amd_manager = dev_get_drvdata(&adata->pdev[pdev_index]->dev); + if (amd_manager) + schedule_work(&amd_manager->amd_sdw_irq_thread); + irq_flag = 1; + } - val = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT); - if (val & BIT(PDM_DMA_STAT)) { + ext_intr_stat1 = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT1); + if (ext_intr_stat1 & ACP_SDW1_STAT) { + writel(ACP_SDW1_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT1); + pdev_index = adata->sdw1_dev_index; + amd_manager = dev_get_drvdata(&adata->pdev[pdev_index]->dev); + if (amd_manager) + schedule_work(&amd_manager->amd_sdw_irq_thread); + irq_flag = 1; + } + + if (ext_intr_stat & ACP_ERROR_IRQ) { + writel(ACP_ERROR_IRQ, adata->acp63_base + ACP_EXTERNAL_INTR_STAT); + /* TODO: Report SoundWire Manager instance errors */ + writel(0, adata->acp63_base + ACP_SW0_I2S_ERROR_REASON); + writel(0, adata->acp63_base + ACP_SW1_I2S_ERROR_REASON); + writel(0, adata->acp63_base + ACP_ERROR_STATUS); + irq_flag = 1; + } + + if (ext_intr_stat & BIT(PDM_DMA_STAT)) { pdev_index = adata->pdm_dev_index; ps_pdm_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev); writel(BIT(PDM_DMA_STAT), adata->acp63_base + ACP_EXTERNAL_INTR_STAT); if (ps_pdm_data->capture_stream) snd_pcm_period_elapsed(ps_pdm_data->capture_stream); - return IRQ_HANDLED; + irq_flag = 1; } - return IRQ_NONE; + if (irq_flag) + return IRQ_HANDLED; + else + return IRQ_NONE; } static int sdw_amd_scan_controller(struct device *dev)
Handle SoundWire manager related interrupts in ACP PCI driver interrupt handler and schedule SoundWire manager work queue for further processing. Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> --- sound/soc/amd/ps/acp63.h | 4 ++++ sound/soc/amd/ps/pci-ps.c | 43 ++++++++++++++++++++++++++++++++++----- 2 files changed, 42 insertions(+), 5 deletions(-)