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

["Mukunda,Vijendar" <vijendar.mukunda@amd.com>]

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)