| Message ID | 20240807051341.1616925-2-Vijendar.Mukunda@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] ASoC: SOF: amd: Fix for incorrect acp error satus register offset | expand |
On Wed, Aug 07, 2024 at 10:43:14AM +0530, Vijendar Mukunda wrote: > Fix the incorrect register offsets for acp error reason registers. > Add 'acp_sw0_i2s_err_reason' as register field in acp descriptor structure > and update the value based on the acp variant. > ACP_SW1_ERROR_REASON register was added from Rembrandt platform onwards. > Add conditional check for the same. > > Fixes: 96eb81851012 ("ASoC: SOF: amd: add interrupt handling for SoundWire manager devices") This breaks an x86 allmodconfig build: /build/stage/linux/sound/soc/sof/amd/acp.c: In function ‘acp_irq_handler’: /build/stage/linux/sound/soc/sof/amd/acp.c:407:26: error: ‘struct acp_dev_data’ h as no member named ‘pci_rev’ 407 | if (adata->pci_rev >= ACP_RMB_PCI_ID) | ^~ /build/stage/linux/sound/soc/sof/amd/acp.c: In function ‘acp_power_on’: /build/stage/linux/sound/soc/sof/amd/acp.c:444:22: error: ‘struct acp_dev_data’ h as no member named ‘pci_rev’ 444 | switch (adata->pci_rev) { | ^~
On 09/08/24 01:02, Mark Brown wrote: > On Wed, Aug 07, 2024 at 10:43:14AM +0530, Vijendar Mukunda wrote: >> Fix the incorrect register offsets for acp error reason registers. >> Add 'acp_sw0_i2s_err_reason' as register field in acp descriptor structure >> and update the value based on the acp variant. >> ACP_SW1_ERROR_REASON register was added from Rembrandt platform onwards. >> Add conditional check for the same. >> >> Fixes: 96eb81851012 ("ASoC: SOF: amd: add interrupt handling for SoundWire manager devices") > This breaks an x86 allmodconfig build: > > /build/stage/linux/sound/soc/sof/amd/acp.c: In function ‘acp_irq_handler’: > /build/stage/linux/sound/soc/sof/amd/acp.c:407:26: error: ‘struct acp_dev_data’ h > as no member named ‘pci_rev’ > 407 | if (adata->pci_rev >= ACP_RMB_PCI_ID) > | ^~ This patch is part of https://github.com/thesofproject/linux/pull/5103 which got successfully merged into sof github without any build errors. This patch is dependent on Link: https://patch.msgid.link/20240801111821.18076-10-Vijendar.Mukunda@amd.com which got already merged in to ASoC tree for-next base. It shouldn't cause build error if the dependent patch already merged. > /build/stage/linux/sound/soc/sof/amd/acp.c: In function ‘acp_power_on’: > /build/stage/linux/sound/soc/sof/amd/acp.c:444:22: error: ‘struct acp_dev_data’ h > as no member named ‘pci_rev’ > 444 | switch (adata->pci_rev) { > | ^~
On Fri, Aug 09, 2024 at 07:30:54AM +0530, Mukunda,Vijendar wrote: > On 09/08/24 01:02, Mark Brown wrote: > > /build/stage/linux/sound/soc/sof/amd/acp.c: In function ‘acp_irq_handler’: > > /build/stage/linux/sound/soc/sof/amd/acp.c:407:26: error: ‘struct acp_dev_data’ h > > as no member named ‘pci_rev’ > > 407 | if (adata->pci_rev >= ACP_RMB_PCI_ID) > > | ^~ > This patch is part of https://github.com/thesofproject/linux/pull/5103 > which got successfully merged into sof github without any build errors. > This patch is dependent on > Link: https://patch.msgid.link/20240801111821.18076-10-Vijendar.Mukunda@amd.com > which got already merged in to ASoC tree for-next base. > It shouldn't cause build error if the dependent patch already merged. Are the patches it depends on actually before it in the patch series? We want the resulting git tree to be bisectable, that means testing each commit not just the final result.
On 09/08/24 13:05, Mark Brown wrote: > On Fri, Aug 09, 2024 at 07:30:54AM +0530, Mukunda,Vijendar wrote: >> On 09/08/24 01:02, Mark Brown wrote: >>> /build/stage/linux/sound/soc/sof/amd/acp.c: In function ‘acp_irq_handler’: >>> /build/stage/linux/sound/soc/sof/amd/acp.c:407:26: error: ‘struct acp_dev_data’ h >>> as no member named ‘pci_rev’ >>> 407 | if (adata->pci_rev >= ACP_RMB_PCI_ID) >>> | ^~ >> This patch is part of https://github.com/thesofproject/linux/pull/5103 >> which got successfully merged into sof github without any build errors. >> This patch is dependent on >> Link: https://patch.msgid.link/20240801111821.18076-10-Vijendar.Mukunda@amd.com >> which got already merged in to ASoC tree for-next base. >> It shouldn't cause build error if the dependent patch already merged. > Are the patches it depends on actually before it in the patch series? > We want the resulting git tree to be bisectable, that means testing each > commit not just the final result. This patch series is prepared on top of 20240801111821.18076-1-Vijendar.Mukunda@amd.com which are incremental changes and also has dependency. As 20240801111821.18076-1-Vijendar.Mukunda@amd.com got merged into for-next branch, compiling this patch series,which is prepared on top of it(20240801111821.18076-1-Vijendar.Mukunda@amd.com), shouldn't trigger any build failures. Within this patch series (20240807051341.1616925-1-Vijendar.Mukunda@amd.com), few patches are dependent patches, as changes are incremental. That's why I have resent whole patch series with cover letter again (20240808165753.3414464-1-Vijendar.Mukunda@amd.com) so that whole patch series can be merged at one go. Each commit can be buildable sequentially.
On Fri, Aug 09, 2024 at 01:49:37PM +0530, Mukunda,Vijendar wrote: > On 09/08/24 13:05, Mark Brown wrote: > > We want the resulting git tree to be bisectable, that means testing each > > commit not just the final result. > This patch series is prepared on top of > 20240801111821.18076-1-Vijendar.Mukunda@amd.com > which are incremental changes and also has dependency. For the benefit of those playing at home that's "ASoC: intel/sdw_utils: move dai id common macros" which is in -next as 8f87e292a34813e. It's not great to base a fix for something that's in Linus' tree like this one which has: Fixes: 96eb81851012 ("ASoC: SOF: amd: add interrupt handling for SoundWire manager devices") in it, and any such dependency really needs to get called out in the cover letter if it exists. In general you should expect bug fixes to be applied for Linus' tree by default, especially if they're tagged as fixing a particular commit in there. That means no dependencies on anything that's already in -next unless explicitly called out, and if the thing in -next is just a cleanup or refactoring then generally it's best to just do the fix for Linus' tree and then separately merge it up to -next and integrate with whtaever cleanup/refactoring is going on there. Please include human readable descriptions of things like commits and issues being discussed in e-mail in your mails, this makes them much easier for humans to read especially when they have no internet access. I do frequently catch up on my mail on flights or while otherwise travelling so this is even more pressing for me than just being about making things a bit easier to read.
On 10/08/24 04:56, Mark Brown wrote: > On Fri, Aug 09, 2024 at 01:49:37PM +0530, Mukunda,Vijendar wrote: >> On 09/08/24 13:05, Mark Brown wrote: >>> We want the resulting git tree to be bisectable, that means testing each >>> commit not just the final result. >> This patch series is prepared on top of >> 20240801111821.18076-1-Vijendar.Mukunda@amd.com >> which are incremental changes and also has dependency. > For the benefit of those playing at home that's "ASoC: intel/sdw_utils: > move dai id common macros" which is in -next as 8f87e292a34813e. It's > not great to base a fix for something that's in Linus' tree like this > one which has: > > Fixes: 96eb81851012 ("ASoC: SOF: amd: add interrupt handling for SoundWire manager devices") > > in it, and any such dependency really needs to get called out in the > cover letter if it exists. Agreed. Will mention the patch dependency in the cover letter and resend the patch series. > > In general you should expect bug fixes to be applied for Linus' tree by > default, especially if they're tagged as fixing a particular commit in > there. That means no dependencies on anything that's already in -next > unless explicitly called out, and if the thing in -next is just a > cleanup or refactoring then generally it's best to just do the fix for > Linus' tree and then separately merge it up to -next and integrate with > whtaever cleanup/refactoring is going on there. > > Please include human readable descriptions of things like commits and > issues being discussed in e-mail in your mails, this makes them much > easier for humans to read especially when they have no internet access. > I do frequently catch up on my mail on flights or while otherwise > travelling so this is even more pressing for me than just being about > making things a bit easier to read.
diff --git a/sound/soc/sof/amd/acp-dsp-offset.h b/sound/soc/sof/amd/acp-dsp-offset.h index 66968efda869..072b703f9b3f 100644 --- a/sound/soc/sof/amd/acp-dsp-offset.h +++ b/sound/soc/sof/amd/acp-dsp-offset.h @@ -83,7 +83,8 @@ #define ACP6X_AXI2DAGB_SEM_0 0x1874 /* ACP common registers to report errors related to I2S & SoundWire interfaces */ -#define ACP_SW0_I2S_ERROR_REASON 0x18B4 +#define ACP3X_SW_I2S_ERROR_REASON 0x18C8 +#define ACP6X_SW0_I2S_ERROR_REASON 0x18B4 #define ACP_SW1_I2S_ERROR_REASON 0x1A50 /* Registers from ACP_SHA block */ diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c index d0b7d1c54248..9ce8b5ccb3d7 100644 --- a/sound/soc/sof/amd/acp.c +++ b/sound/soc/sof/amd/acp.c @@ -403,8 +403,9 @@ static irqreturn_t acp_irq_handler(int irq, void *dev_id) if (val & ACP_ERROR_IRQ_MASK) { snd_sof_dsp_write(sdev, ACP_DSP_BAR, desc->ext_intr_stat, ACP_ERROR_IRQ_MASK); - snd_sof_dsp_write(sdev, ACP_DSP_BAR, base + ACP_SW0_I2S_ERROR_REASON, 0); - snd_sof_dsp_write(sdev, ACP_DSP_BAR, base + ACP_SW1_I2S_ERROR_REASON, 0); + snd_sof_dsp_write(sdev, ACP_DSP_BAR, desc->acp_sw0_i2s_err_reason, 0); + if (adata->pci_rev >= ACP_RMB_PCI_ID) + snd_sof_dsp_write(sdev, ACP_DSP_BAR, ACP_SW1_I2S_ERROR_REASON, 0); snd_sof_dsp_write(sdev, ACP_DSP_BAR, desc->acp_error_stat, 0); irq_flag = 1; } diff --git a/sound/soc/sof/amd/acp.h b/sound/soc/sof/amd/acp.h index 6ac853ff6093..f6f0fcfeb691 100644 --- a/sound/soc/sof/amd/acp.h +++ b/sound/soc/sof/amd/acp.h @@ -204,6 +204,7 @@ struct sof_amd_acp_desc { u32 reg_start_addr; u32 reg_end_addr; u32 acp_error_stat; + u32 acp_sw0_i2s_err_reason; u32 sdw_max_link_count; u64 sdw_acpi_dev_addr; }; diff --git a/sound/soc/sof/amd/pci-acp63.c b/sound/soc/sof/amd/pci-acp63.c index c3da70549995..e90658ba2bd7 100644 --- a/sound/soc/sof/amd/pci-acp63.c +++ b/sound/soc/sof/amd/pci-acp63.c @@ -36,6 +36,7 @@ static const struct sof_amd_acp_desc acp63_chip_info = { .ext_intr_stat = ACP6X_EXT_INTR_STAT, .ext_intr_stat1 = ACP6X_EXT_INTR_STAT1, .acp_error_stat = ACP6X_ERROR_STATUS, + .acp_sw0_i2s_err_reason = ACP6X_SW0_I2S_ERROR_REASON, .dsp_intr_base = ACP6X_DSP_SW_INTR_BASE, .sram_pte_offset = ACP6X_SRAM_PTE_OFFSET, .hw_semaphore_offset = ACP6X_AXI2DAGB_SEM_0, diff --git a/sound/soc/sof/amd/pci-rmb.c b/sound/soc/sof/amd/pci-rmb.c index 194b7ff37e9e..a366f904e6f3 100644 --- a/sound/soc/sof/amd/pci-rmb.c +++ b/sound/soc/sof/amd/pci-rmb.c @@ -34,6 +34,7 @@ static const struct sof_amd_acp_desc rembrandt_chip_info = { .ext_intr_stat = ACP6X_EXT_INTR_STAT, .dsp_intr_base = ACP6X_DSP_SW_INTR_BASE, .acp_error_stat = ACP6X_ERROR_STATUS, + .acp_sw0_i2s_err_reason = ACP6X_SW0_I2S_ERROR_REASON, .sram_pte_offset = ACP6X_SRAM_PTE_OFFSET, .hw_semaphore_offset = ACP6X_AXI2DAGB_SEM_0, .fusion_dsp_offset = ACP6X_DSP_FUSION_RUNSTALL, diff --git a/sound/soc/sof/amd/pci-rn.c b/sound/soc/sof/amd/pci-rn.c index bff2d979ea6a..2b7c53470ce8 100644 --- a/sound/soc/sof/amd/pci-rn.c +++ b/sound/soc/sof/amd/pci-rn.c @@ -34,6 +34,7 @@ static const struct sof_amd_acp_desc renoir_chip_info = { .ext_intr_stat = ACP3X_EXT_INTR_STAT, .dsp_intr_base = ACP3X_DSP_SW_INTR_BASE, .acp_error_stat = ACP3X_ERROR_STATUS, + .acp_sw0_i2s_err_reason = ACP3X_SW_I2S_ERROR_REASON, .sram_pte_offset = ACP3X_SRAM_PTE_OFFSET, .hw_semaphore_offset = ACP3X_AXI2DAGB_SEM_0, .acp_clkmux_sel = ACP3X_CLKMUX_SEL,
Fix the incorrect register offsets for acp error reason registers. Add 'acp_sw0_i2s_err_reason' as register field in acp descriptor structure and update the value based on the acp variant. ACP_SW1_ERROR_REASON register was added from Rembrandt platform onwards. Add conditional check for the same. Fixes: 96eb81851012 ("ASoC: SOF: amd: add interrupt handling for SoundWire manager devices") Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- sound/soc/sof/amd/acp-dsp-offset.h | 3 ++- sound/soc/sof/amd/acp.c | 5 +++-- sound/soc/sof/amd/acp.h | 1 + sound/soc/sof/amd/pci-acp63.c | 1 + sound/soc/sof/amd/pci-rmb.c | 1 + sound/soc/sof/amd/pci-rn.c | 1 + 6 files changed, 9 insertions(+), 3 deletions(-)