Re: [PATCH 2/9] ASoC: amd: ps: handle soundwire interrupts in acp pci driver

[Mukunda,Vijendar]

On 16/05/23 20:09, Pierre-Louis Bossart wrote:
>
> On 5/16/23 05:35, Vijendar Mukunda wrote:
>> Handle soundwire manager related interrupts in ACP PCI driver
>> interrupt handler and schedule soundwire manager work queue for
>> further processing.
> "SoundWire" please
Will fix it.
>
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
>> ---
>>  sound/soc/amd/ps/acp63.h  |  4 ++++
>>  sound/soc/amd/ps/pci-ps.c | 42 ++++++++++++++++++++++++++++++++++-----
>>  2 files changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
>> index f27f71116598..faf7be4d77c2 100644
>> --- a/sound/soc/amd/ps/acp63.h
>> +++ b/sound/soc/amd/ps/acp63.h
>> @@ -67,6 +67,10 @@
>>  /* time in ms for acp timeout */
>>  #define ACP_TIMEOUT		500
>>  
>> +#define ACP_SDW0_IRQ_MASK		21
>> +#define ACP_SDW1_IRQ_MASK		2
>> +#define ACP_ERROR_IRQ_MASK		29
> Shouldn't this be in 0x representation or BIT/GENMASK?
All above IRQ mask fields are bit positions in the register.
Will change the name and representation in Hex.
>
>> +
>>  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 f3aa08dc05b2..6566ee14d300 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(BIT(ACP_ERROR_IRQ_MASK), acp_base + ACP_EXTERNAL_INTR_CNTL);
> BIT(FOO_MASK) is very odd.
>
> BIT(ACP_ERROR_IRQ) or ACP_ERROR_IRQ_MASK?
We followed register field description name as it is. It denotes a bit position only.
We will change it.
>
>>  }
>>  
>>  static void acp63_disable_interrupts(void __iomem *acp_base)
>> @@ -102,23 +103,54 @@ 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 & BIT(ACP_SDW0_IRQ_MASK)) {
>> +		writel(BIT(ACP_SDW0_IRQ_MASK), 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 & BIT(ACP_SDW1_IRQ_MASK)) {
>> +		writel(BIT(ACP_SDW1_IRQ_MASK), 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 & BIT(ACP_ERROR_IRQ_MASK)) {
>> +		writel(BIT(ACP_ERROR_IRQ_MASK), adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>> +		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;
> it's not clear what this does? Looks like just filtering out interrupts
> without doing anything about the interrupt source?
When ACP error interrupt is raised, As per our design, ACP IRQ handler will clear
the error interrupt by clearing ACP_ERROR_IRQ bit in ACP_EXTERNAL_INTR_STAT.

To know the error source reason, we need to read the ACP_ERROR_STATUS,
ACP_SW0_I2S_ERROR_REASON, ACP_SW1_I2S_ERROR_REASON registers.
After reading the Error registers, we need to clear these registers.
Currently, we haven't added read register operations for error reason registers.
This is for debugging purpose only.

In Current context, we refer ACP_SW0_I2S_ERROR_REASON  register to know errors like
Sound Wire Bus clash detections, Command and Response Errors, BRA mode errors,
FIFO underflow/overflow errors detected for SoundWire Manager -0 instance.
Similarly, ACP_SW1_I2S_ERROR_REASON register referred to know errors detected for
SoundWire Manager instance - 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)