| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 17 May 2023 12:48:13 +0530
From: "Mukunda,Vijendar" <vijendar.mukunda@....com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
Mark Brown <broonie@...nel.org>
Cc: alsa-devel@...a-project.org, Basavaraj.Hiregoudar@....com,
Sunil-kumar.Dommati@....com, Mastan.Katragadda@....com,
Arungopal.kondaveeti@....com, Liam Girdwood <lgirdwood@...il.com>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
Syed Saba Kareem <Syed.SabaKareem@....com>,
Mario Limonciello <mario.limonciello@....com>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/9] ASoC: amd: ps: handle soundwire interrupts in acp pci
driver
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@....com>
>> ---
>> 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)
Powered by blists - more mailing lists