[PATCH v2 0/2] ALSA: hda: Compilation and firmware-loading fixes

[PATCH v2 0/2] ALSA: hda: Compilation and firmware-loading fixes

[Cezary Rojewski]

Small set of fixes, one of which addresses the compilation issues with
snd_hdac_adsp_xxx() macros utilized currently by the avs-driver alone.

The more impactful change updates the firmware loading flow so that the
binary is loaded in two chunks rather than just one. This is to follow
recommendation from the hardware team: SDxLVI is expected to be at least
1 (0-index based so 1+1=2 BDL entries) when performing any data
transfer.

Changes in v2:
- removed the type-mismatch fixing patch as suggested by Takashi
- relocated snd_hdac_adsp_xxx() macros to avs/ space as suggested by
  Jaroslav and Takashi


Cezary Rojewski (2):
  ALSA: hda: Fix compilation of snd_hdac_adsp_xxx() helpers
  ALSA: hda: Transfer firmware in two chunks

 include/sound/hdaudio_ext.h     | 45 ---------------------------------
 sound/hda/hdac_stream.c         |  3 ++-
 sound/soc/intel/avs/apl.c       |  1 +
 sound/soc/intel/avs/cnl.c       |  1 +
 sound/soc/intel/avs/registers.h | 45 +++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/skl.c       |  1 +
 6 files changed, 50 insertions(+), 46 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] ALSA: hda: Fix compilation of snd_hdac_adsp_xxx() helpers

[Cezary Rojewski]

The snd_hdac_adsp_xxx() wrap snd_hdac_reg_xxx() helpers to simplify
register access for AudioDSP drivers e.g.: the avs-driver. Byte- and
word-variants of said helps do not expand to bare readx/writex()
operations but functions instead and, due to pointer type
incompatibility, cause compilation to fail.

As the macros are utilized by the avs-driver alone, relocate the code
introduced with commit c19bd02e9029 ("ALSA: hda: Add helper macros for
DSP capable devices") into the avs/ directory and update it to operate
on 'adev' i.e.: the avs-driver-context directly to fix the issue.

Fixes: c19bd02e9029 ("ALSA: hda: Add helper macros for DSP capable devices")
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/sound/hdaudio_ext.h     | 45 ---------------------------------
 sound/soc/intel/avs/apl.c       |  1 +
 sound/soc/intel/avs/cnl.c       |  1 +
 sound/soc/intel/avs/registers.h | 45 +++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/skl.c       |  1 +
 5 files changed, 48 insertions(+), 45 deletions(-)

diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
index 957295364a5e..4c7a40e149a5 100644
--- a/include/sound/hdaudio_ext.h
+++ b/include/sound/hdaudio_ext.h
@@ -2,8 +2,6 @@
 #ifndef __SOUND_HDAUDIO_EXT_H
 #define __SOUND_HDAUDIO_EXT_H
 
-#include <linux/io-64-nonatomic-lo-hi.h>
-#include <linux/iopoll.h>
 #include <sound/hdaudio.h>
 
 int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
@@ -119,49 +117,6 @@ int snd_hdac_ext_bus_link_put(struct hdac_bus *bus, struct hdac_ext_link *hlink)
 
 void snd_hdac_ext_bus_link_power(struct hdac_device *codec, bool enable);
 
-#define snd_hdac_adsp_writeb(chip, reg, value) \
-	snd_hdac_reg_writeb(chip, (chip)->dsp_ba + (reg), value)
-#define snd_hdac_adsp_readb(chip, reg) \
-	snd_hdac_reg_readb(chip, (chip)->dsp_ba + (reg))
-#define snd_hdac_adsp_writew(chip, reg, value) \
-	snd_hdac_reg_writew(chip, (chip)->dsp_ba + (reg), value)
-#define snd_hdac_adsp_readw(chip, reg) \
-	snd_hdac_reg_readw(chip, (chip)->dsp_ba + (reg))
-#define snd_hdac_adsp_writel(chip, reg, value) \
-	snd_hdac_reg_writel(chip, (chip)->dsp_ba + (reg), value)
-#define snd_hdac_adsp_readl(chip, reg) \
-	snd_hdac_reg_readl(chip, (chip)->dsp_ba + (reg))
-#define snd_hdac_adsp_writeq(chip, reg, value) \
-	snd_hdac_reg_writeq(chip, (chip)->dsp_ba + (reg), value)
-#define snd_hdac_adsp_readq(chip, reg) \
-	snd_hdac_reg_readq(chip, (chip)->dsp_ba + (reg))
-
-#define snd_hdac_adsp_updateb(chip, reg, mask, val) \
-	snd_hdac_adsp_writeb(chip, reg, \
-			(snd_hdac_adsp_readb(chip, reg) & ~(mask)) | (val))
-#define snd_hdac_adsp_updatew(chip, reg, mask, val) \
-	snd_hdac_adsp_writew(chip, reg, \
-			(snd_hdac_adsp_readw(chip, reg) & ~(mask)) | (val))
-#define snd_hdac_adsp_updatel(chip, reg, mask, val) \
-	snd_hdac_adsp_writel(chip, reg, \
-			(snd_hdac_adsp_readl(chip, reg) & ~(mask)) | (val))
-#define snd_hdac_adsp_updateq(chip, reg, mask, val) \
-	snd_hdac_adsp_writeq(chip, reg, \
-			(snd_hdac_adsp_readq(chip, reg) & ~(mask)) | (val))
-
-#define snd_hdac_adsp_readb_poll(chip, reg, val, cond, delay_us, timeout_us) \
-	readb_poll_timeout((chip)->dsp_ba + (reg), val, cond, \
-			   delay_us, timeout_us)
-#define snd_hdac_adsp_readw_poll(chip, reg, val, cond, delay_us, timeout_us) \
-	readw_poll_timeout((chip)->dsp_ba + (reg), val, cond, \
-			   delay_us, timeout_us)
-#define snd_hdac_adsp_readl_poll(chip, reg, val, cond, delay_us, timeout_us) \
-	readl_poll_timeout((chip)->dsp_ba + (reg), val, cond, \
-			   delay_us, timeout_us)
-#define snd_hdac_adsp_readq_poll(chip, reg, val, cond, delay_us, timeout_us) \
-	readq_poll_timeout((chip)->dsp_ba + (reg), val, cond, \
-			   delay_us, timeout_us)
-
 struct hdac_ext_device;
 
 /* ops common to all codec drivers */
diff --git a/sound/soc/intel/avs/apl.c b/sound/soc/intel/avs/apl.c
index 27516ef57185..a48d74daf48c 100644
--- a/sound/soc/intel/avs/apl.c
+++ b/sound/soc/intel/avs/apl.c
@@ -12,6 +12,7 @@
 #include "avs.h"
 #include "messages.h"
 #include "path.h"
+#include "registers.h"
 #include "topology.h"
 
 static irqreturn_t avs_apl_dsp_interrupt(struct avs_dev *adev)
diff --git a/sound/soc/intel/avs/cnl.c b/sound/soc/intel/avs/cnl.c
index bd3c4bb8bf5a..03f8fb0dc187 100644
--- a/sound/soc/intel/avs/cnl.c
+++ b/sound/soc/intel/avs/cnl.c
@@ -9,6 +9,7 @@
 #include <sound/hdaudio_ext.h>
 #include "avs.h"
 #include "messages.h"
+#include "registers.h"
 
 static void avs_cnl_ipc_interrupt(struct avs_dev *adev)
 {
diff --git a/sound/soc/intel/avs/registers.h b/sound/soc/intel/avs/registers.h
index f76e91cff2a9..5b6d60eb3c18 100644
--- a/sound/soc/intel/avs/registers.h
+++ b/sound/soc/intel/avs/registers.h
@@ -9,6 +9,8 @@
 #ifndef __SOUND_SOC_INTEL_AVS_REGS_H
 #define __SOUND_SOC_INTEL_AVS_REGS_H
 
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/iopoll.h>
 #include <linux/sizes.h>
 
 #define AZX_PCIREG_PGCTL		0x44
@@ -98,4 +100,47 @@
 #define avs_downlink_addr(adev) \
 	avs_sram_addr(adev, AVS_DOWNLINK_WINDOW)
 
+#define snd_hdac_adsp_writeb(adev, reg, value) \
+	snd_hdac_reg_writeb(&(adev)->base.core, (adev)->dsp_ba + (reg), value)
+#define snd_hdac_adsp_readb(adev, reg) \
+	snd_hdac_reg_readb(&(adev)->base.core, (adev)->dsp_ba + (reg))
+#define snd_hdac_adsp_writew(adev, reg, value) \
+	snd_hdac_reg_writew(&(adev)->base.core, (adev)->dsp_ba + (reg), value)
+#define snd_hdac_adsp_readw(adev, reg) \
+	snd_hdac_reg_readw(&(adev)->base.core, (adev)->dsp_ba + (reg))
+#define snd_hdac_adsp_writel(adev, reg, value) \
+	snd_hdac_reg_writel(&(adev)->base.core, (adev)->dsp_ba + (reg), value)
+#define snd_hdac_adsp_readl(adev, reg) \
+	snd_hdac_reg_readl(&(adev)->base.core, (adev)->dsp_ba + (reg))
+#define snd_hdac_adsp_writeq(adev, reg, value) \
+	snd_hdac_reg_writeq(&(adev)->base.core, (adev)->dsp_ba + (reg), value)
+#define snd_hdac_adsp_readq(adev, reg) \
+	snd_hdac_reg_readq(&(adev)->base.core, (adev)->dsp_ba + (reg))
+
+#define snd_hdac_adsp_updateb(adev, reg, mask, val) \
+	snd_hdac_adsp_writeb(adev, reg, \
+			(snd_hdac_adsp_readb(adev, reg) & ~(mask)) | (val))
+#define snd_hdac_adsp_updatew(adev, reg, mask, val) \
+	snd_hdac_adsp_writew(adev, reg, \
+			(snd_hdac_adsp_readw(adev, reg) & ~(mask)) | (val))
+#define snd_hdac_adsp_updatel(adev, reg, mask, val) \
+	snd_hdac_adsp_writel(adev, reg, \
+			(snd_hdac_adsp_readl(adev, reg) & ~(mask)) | (val))
+#define snd_hdac_adsp_updateq(adev, reg, mask, val) \
+	snd_hdac_adsp_writeq(adev, reg, \
+			(snd_hdac_adsp_readq(adev, reg) & ~(mask)) | (val))
+
+#define snd_hdac_adsp_readb_poll(adev, reg, val, cond, delay_us, timeout_us) \
+	readb_poll_timeout((adev)->dsp_ba + (reg), val, cond, \
+			   delay_us, timeout_us)
+#define snd_hdac_adsp_readw_poll(adev, reg, val, cond, delay_us, timeout_us) \
+	readw_poll_timeout((adev)->dsp_ba + (reg), val, cond, \
+			   delay_us, timeout_us)
+#define snd_hdac_adsp_readl_poll(adev, reg, val, cond, delay_us, timeout_us) \
+	readl_poll_timeout((adev)->dsp_ba + (reg), val, cond, \
+			   delay_us, timeout_us)
+#define snd_hdac_adsp_readq_poll(adev, reg, val, cond, delay_us, timeout_us) \
+	readq_poll_timeout((adev)->dsp_ba + (reg), val, cond, \
+			   delay_us, timeout_us)
+
 #endif /* __SOUND_SOC_INTEL_AVS_REGS_H */
diff --git a/sound/soc/intel/avs/skl.c b/sound/soc/intel/avs/skl.c
index 34f859d6e5a4..d66ef000de9e 100644
--- a/sound/soc/intel/avs/skl.c
+++ b/sound/soc/intel/avs/skl.c
@@ -12,6 +12,7 @@
 #include "avs.h"
 #include "cldma.h"
 #include "messages.h"
+#include "registers.h"
 
 void avs_skl_ipc_interrupt(struct avs_dev *adev)
 {
-- 
2.25.1

Re: [PATCH v2 1/2] ALSA: hda: Fix compilation of snd_hdac_adsp_xxx() helpers

[Takashi Iwai]

On Fri, 10 Jan 2025 12:33:25 +0100,
Cezary Rojewski wrote:
> 
> The snd_hdac_adsp_xxx() wrap snd_hdac_reg_xxx() helpers to simplify
> register access for AudioDSP drivers e.g.: the avs-driver. Byte- and
> word-variants of said helps do not expand to bare readx/writex()
> operations but functions instead and, due to pointer type
> incompatibility, cause compilation to fail.
> 
> As the macros are utilized by the avs-driver alone, relocate the code
> introduced with commit c19bd02e9029 ("ALSA: hda: Add helper macros for
> DSP capable devices") into the avs/ directory and update it to operate
> on 'adev' i.e.: the avs-driver-context directly to fix the issue.
> 
> Fixes: c19bd02e9029 ("ALSA: hda: Add helper macros for DSP capable devices")
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/sound/hdaudio_ext.h     | 45 ---------------------------------
>  sound/soc/intel/avs/apl.c       |  1 +
>  sound/soc/intel/avs/cnl.c       |  1 +
>  sound/soc/intel/avs/registers.h | 45 +++++++++++++++++++++++++++++++++
>  sound/soc/intel/avs/skl.c       |  1 +

I can take this, but as it touches ASoC stuff, need an ack from Mark.


thanks,

Takashi

>  5 files changed, 48 insertions(+), 45 deletions(-)
> 
> diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
> index 957295364a5e..4c7a40e149a5 100644
> --- a/include/sound/hdaudio_ext.h
> +++ b/include/sound/hdaudio_ext.h
> @@ -2,8 +2,6 @@
>  #ifndef __SOUND_HDAUDIO_EXT_H
>  #define __SOUND_HDAUDIO_EXT_H
>  
> -#include <linux/io-64-nonatomic-lo-hi.h>
> -#include <linux/iopoll.h>
>  #include <sound/hdaudio.h>
>  
>  int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
> @@ -119,49 +117,6 @@ int snd_hdac_ext_bus_link_put(struct hdac_bus *bus, struct hdac_ext_link *hlink)
>  
>  void snd_hdac_ext_bus_link_power(struct hdac_device *codec, bool enable);
>  
> -#define snd_hdac_adsp_writeb(chip, reg, value) \
> -	snd_hdac_reg_writeb(chip, (chip)->dsp_ba + (reg), value)
> -#define snd_hdac_adsp_readb(chip, reg) \
> -	snd_hdac_reg_readb(chip, (chip)->dsp_ba + (reg))
> -#define snd_hdac_adsp_writew(chip, reg, value) \
> -	snd_hdac_reg_writew(chip, (chip)->dsp_ba + (reg), value)
> -#define snd_hdac_adsp_readw(chip, reg) \
> -	snd_hdac_reg_readw(chip, (chip)->dsp_ba + (reg))
> -#define snd_hdac_adsp_writel(chip, reg, value) \
> -	snd_hdac_reg_writel(chip, (chip)->dsp_ba + (reg), value)
> -#define snd_hdac_adsp_readl(chip, reg) \
> -	snd_hdac_reg_readl(chip, (chip)->dsp_ba + (reg))
> -#define snd_hdac_adsp_writeq(chip, reg, value) \
> -	snd_hdac_reg_writeq(chip, (chip)->dsp_ba + (reg), value)
> -#define snd_hdac_adsp_readq(chip, reg) \
> -	snd_hdac_reg_readq(chip, (chip)->dsp_ba + (reg))
> -
> -#define snd_hdac_adsp_updateb(chip, reg, mask, val) \
> -	snd_hdac_adsp_writeb(chip, reg, \
> -			(snd_hdac_adsp_readb(chip, reg) & ~(mask)) | (val))
> -#define snd_hdac_adsp_updatew(chip, reg, mask, val) \
> -	snd_hdac_adsp_writew(chip, reg, \
> -			(snd_hdac_adsp_readw(chip, reg) & ~(mask)) | (val))
> -#define snd_hdac_adsp_updatel(chip, reg, mask, val) \
> -	snd_hdac_adsp_writel(chip, reg, \
> -			(snd_hdac_adsp_readl(chip, reg) & ~(mask)) | (val))
> -#define snd_hdac_adsp_updateq(chip, reg, mask, val) \
> -	snd_hdac_adsp_writeq(chip, reg, \
> -			(snd_hdac_adsp_readq(chip, reg) & ~(mask)) | (val))
> -
> -#define snd_hdac_adsp_readb_poll(chip, reg, val, cond, delay_us, timeout_us) \
> -	readb_poll_timeout((chip)->dsp_ba + (reg), val, cond, \
> -			   delay_us, timeout_us)
> -#define snd_hdac_adsp_readw_poll(chip, reg, val, cond, delay_us, timeout_us) \
> -	readw_poll_timeout((chip)->dsp_ba + (reg), val, cond, \
> -			   delay_us, timeout_us)
> -#define snd_hdac_adsp_readl_poll(chip, reg, val, cond, delay_us, timeout_us) \
> -	readl_poll_timeout((chip)->dsp_ba + (reg), val, cond, \
> -			   delay_us, timeout_us)
> -#define snd_hdac_adsp_readq_poll(chip, reg, val, cond, delay_us, timeout_us) \
> -	readq_poll_timeout((chip)->dsp_ba + (reg), val, cond, \
> -			   delay_us, timeout_us)
> -
>  struct hdac_ext_device;
>  
>  /* ops common to all codec drivers */
> diff --git a/sound/soc/intel/avs/apl.c b/sound/soc/intel/avs/apl.c
> index 27516ef57185..a48d74daf48c 100644
> --- a/sound/soc/intel/avs/apl.c
> +++ b/sound/soc/intel/avs/apl.c
> @@ -12,6 +12,7 @@
>  #include "avs.h"
>  #include "messages.h"
>  #include "path.h"
> +#include "registers.h"
>  #include "topology.h"
>  
>  static irqreturn_t avs_apl_dsp_interrupt(struct avs_dev *adev)
> diff --git a/sound/soc/intel/avs/cnl.c b/sound/soc/intel/avs/cnl.c
> index bd3c4bb8bf5a..03f8fb0dc187 100644
> --- a/sound/soc/intel/avs/cnl.c
> +++ b/sound/soc/intel/avs/cnl.c
> @@ -9,6 +9,7 @@
>  #include <sound/hdaudio_ext.h>
>  #include "avs.h"
>  #include "messages.h"
> +#include "registers.h"
>  
>  static void avs_cnl_ipc_interrupt(struct avs_dev *adev)
>  {
> diff --git a/sound/soc/intel/avs/registers.h b/sound/soc/intel/avs/registers.h
> index f76e91cff2a9..5b6d60eb3c18 100644
> --- a/sound/soc/intel/avs/registers.h
> +++ b/sound/soc/intel/avs/registers.h
> @@ -9,6 +9,8 @@
>  #ifndef __SOUND_SOC_INTEL_AVS_REGS_H
>  #define __SOUND_SOC_INTEL_AVS_REGS_H
>  
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/iopoll.h>
>  #include <linux/sizes.h>
>  
>  #define AZX_PCIREG_PGCTL		0x44
> @@ -98,4 +100,47 @@
>  #define avs_downlink_addr(adev) \
>  	avs_sram_addr(adev, AVS_DOWNLINK_WINDOW)
>  
> +#define snd_hdac_adsp_writeb(adev, reg, value) \
> +	snd_hdac_reg_writeb(&(adev)->base.core, (adev)->dsp_ba + (reg), value)
> +#define snd_hdac_adsp_readb(adev, reg) \
> +	snd_hdac_reg_readb(&(adev)->base.core, (adev)->dsp_ba + (reg))
> +#define snd_hdac_adsp_writew(adev, reg, value) \
> +	snd_hdac_reg_writew(&(adev)->base.core, (adev)->dsp_ba + (reg), value)
> +#define snd_hdac_adsp_readw(adev, reg) \
> +	snd_hdac_reg_readw(&(adev)->base.core, (adev)->dsp_ba + (reg))
> +#define snd_hdac_adsp_writel(adev, reg, value) \
> +	snd_hdac_reg_writel(&(adev)->base.core, (adev)->dsp_ba + (reg), value)
> +#define snd_hdac_adsp_readl(adev, reg) \
> +	snd_hdac_reg_readl(&(adev)->base.core, (adev)->dsp_ba + (reg))
> +#define snd_hdac_adsp_writeq(adev, reg, value) \
> +	snd_hdac_reg_writeq(&(adev)->base.core, (adev)->dsp_ba + (reg), value)
> +#define snd_hdac_adsp_readq(adev, reg) \
> +	snd_hdac_reg_readq(&(adev)->base.core, (adev)->dsp_ba + (reg))
> +
> +#define snd_hdac_adsp_updateb(adev, reg, mask, val) \
> +	snd_hdac_adsp_writeb(adev, reg, \
> +			(snd_hdac_adsp_readb(adev, reg) & ~(mask)) | (val))
> +#define snd_hdac_adsp_updatew(adev, reg, mask, val) \
> +	snd_hdac_adsp_writew(adev, reg, \
> +			(snd_hdac_adsp_readw(adev, reg) & ~(mask)) | (val))
> +#define snd_hdac_adsp_updatel(adev, reg, mask, val) \
> +	snd_hdac_adsp_writel(adev, reg, \
> +			(snd_hdac_adsp_readl(adev, reg) & ~(mask)) | (val))
> +#define snd_hdac_adsp_updateq(adev, reg, mask, val) \
> +	snd_hdac_adsp_writeq(adev, reg, \
> +			(snd_hdac_adsp_readq(adev, reg) & ~(mask)) | (val))
> +
> +#define snd_hdac_adsp_readb_poll(adev, reg, val, cond, delay_us, timeout_us) \
> +	readb_poll_timeout((adev)->dsp_ba + (reg), val, cond, \
> +			   delay_us, timeout_us)
> +#define snd_hdac_adsp_readw_poll(adev, reg, val, cond, delay_us, timeout_us) \
> +	readw_poll_timeout((adev)->dsp_ba + (reg), val, cond, \
> +			   delay_us, timeout_us)
> +#define snd_hdac_adsp_readl_poll(adev, reg, val, cond, delay_us, timeout_us) \
> +	readl_poll_timeout((adev)->dsp_ba + (reg), val, cond, \
> +			   delay_us, timeout_us)
> +#define snd_hdac_adsp_readq_poll(adev, reg, val, cond, delay_us, timeout_us) \
> +	readq_poll_timeout((adev)->dsp_ba + (reg), val, cond, \
> +			   delay_us, timeout_us)
> +
>  #endif /* __SOUND_SOC_INTEL_AVS_REGS_H */
> diff --git a/sound/soc/intel/avs/skl.c b/sound/soc/intel/avs/skl.c
> index 34f859d6e5a4..d66ef000de9e 100644
> --- a/sound/soc/intel/avs/skl.c
> +++ b/sound/soc/intel/avs/skl.c
> @@ -12,6 +12,7 @@
>  #include "avs.h"
>  #include "cldma.h"
>  #include "messages.h"
> +#include "registers.h"
>  
>  void avs_skl_ipc_interrupt(struct avs_dev *adev)
>  {
> -- 
> 2.25.1
> 

Re: [PATCH v2 1/2] ALSA: hda: Fix compilation of snd_hdac_adsp_xxx() helpers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 272 bytes --]

On Fri, Jan 10, 2025 at 05:29:30PM +0100, Takashi Iwai wrote:

> I can take this, but as it touches ASoC stuff, need an ack from Mark.

Acked-by: Mark Brown <broonie@kernel.org>

I tend to just not look at stuff with ALSA: on it since I get copied on
lots of HDA patches.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] ALSA: hda: Fix compilation of snd_hdac_adsp_xxx() helpers

[Takashi Iwai]

On Fri, 10 Jan 2025 17:31:33 +0100,
Mark Brown wrote:
> 
> On Fri, Jan 10, 2025 at 05:29:30PM +0100, Takashi Iwai wrote:
> 
> > I can take this, but as it touches ASoC stuff, need an ack from Mark.
> 
> Acked-by: Mark Brown <broonie@kernel.org>
> 
> I tend to just not look at stuff with ALSA: on it since I get copied on
> lots of HDA patches.

Heh, that's why I asked explicitly at this time ;)

Now applied the patch 1 to for-next branch.
I leave patch 2 for further discussion.


thanks,

Takashi

Re: [PATCH v2 1/2] ALSA: hda: Fix compilation of snd_hdac_adsp_xxx() helpers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 418 bytes --]

On Fri, Jan 10, 2025 at 05:47:41PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > I tend to just not look at stuff with ALSA: on it since I get copied on
> > lots of HDA patches.

> Heh, that's why I asked explicitly at this time ;)

I do tend to do it on subject line so it's a bit random if I'll see a
query in a thread (depends a bit on how quiet my inbox is and if it's
the next mail after something I read).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 2/2] ALSA: hda: Transfer firmware in two chunks

[Cezary Rojewski]

As per specification, SDxLVI shall be at least 1 i.e.: two chunks to
perform a valid transfer. This is true for the PCM transfer code but
not firmware-transfer one.

Technical background:
- the LVI > 0 rule shall be obeyed in PCM transfer
- HW permits LVI == 0 when transfer is SW-controlled (SPIB)
- FW download is not a PCM transfer and is SW-controlled (SPIB)

The above is the fundament which AudioDSP firmware loading functions
have been built upon and worked since 2016. The presented changes are to
align the loading flows and avoid rising more questions in the future.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/hda/hdac_stream.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
index 2670792f43b4..18d74a28a246 100644
--- a/sound/hda/hdac_stream.c
+++ b/sound/hda/hdac_stream.c
@@ -455,6 +455,7 @@ static int setup_bdle(struct hdac_bus *bus,
 		      struct hdac_stream *azx_dev, __le32 **bdlp,
 		      int ofs, int size, int with_ioc)
 {
+	u32 bdle_size = size / 2;
 	__le32 *bdl = *bdlp;
 
 	while (size > 0) {
@@ -469,7 +470,7 @@ static int setup_bdle(struct hdac_bus *bus,
 		bdl[0] = cpu_to_le32((u32)addr);
 		bdl[1] = cpu_to_le32(upper_32_bits(addr));
 		/* program the size field of the BDL entry */
-		chunk = snd_sgbuf_get_chunk_size(dmab, ofs, size);
+		chunk = snd_sgbuf_get_chunk_size(dmab, ofs, bdle_size);
 		/* one BDLE cannot cross 4K boundary on CTHDA chips */
 		if (bus->align_bdle_4k) {
 			u32 remain = 0x1000 - (ofs & 0xfff);
-- 
2.25.1

Re: [PATCH v2 2/2] ALSA: hda: Transfer firmware in two chunks

[Takashi Iwai]

On Fri, 10 Jan 2025 12:33:26 +0100,
Cezary Rojewski wrote:
> 
> As per specification, SDxLVI shall be at least 1 i.e.: two chunks to
> perform a valid transfer. This is true for the PCM transfer code but
> not firmware-transfer one.
> 
> Technical background:
> - the LVI > 0 rule shall be obeyed in PCM transfer
> - HW permits LVI == 0 when transfer is SW-controlled (SPIB)
> - FW download is not a PCM transfer and is SW-controlled (SPIB)
> 
> The above is the fundament which AudioDSP firmware loading functions
> have been built upon and worked since 2016. The presented changes are to
> align the loading flows and avoid rising more questions in the future.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  sound/hda/hdac_stream.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
> index 2670792f43b4..18d74a28a246 100644
> --- a/sound/hda/hdac_stream.c
> +++ b/sound/hda/hdac_stream.c
> @@ -455,6 +455,7 @@ static int setup_bdle(struct hdac_bus *bus,
>  		      struct hdac_stream *azx_dev, __le32 **bdlp,
>  		      int ofs, int size, int with_ioc)
>  {
> +	u32 bdle_size = size / 2;
>  	__le32 *bdl = *bdlp;
>  
>  	while (size > 0) {
> @@ -469,7 +470,7 @@ static int setup_bdle(struct hdac_bus *bus,
>  		bdl[0] = cpu_to_le32((u32)addr);
>  		bdl[1] = cpu_to_le32(upper_32_bits(addr));
>  		/* program the size field of the BDL entry */
> -		chunk = snd_sgbuf_get_chunk_size(dmab, ofs, size);
> +		chunk = snd_sgbuf_get_chunk_size(dmab, ofs, bdle_size);
>  		/* one BDLE cannot cross 4K boundary on CTHDA chips */
>  		if (bus->align_bdle_4k) {
>  			u32 remain = 0x1000 - (ofs & 0xfff);

I still think that it's not best place to change.

For PCM, this workaround isn't needed in most cases, because
setup_bdle() is called per period, and periods_min = 2 for
snd-hda-intel.  Doing extra splitting is superfluous for PCM.

That said, if we need a workaround of the split, it should be done
conditionally for the firmware stream.


thanks,

Takashi

Re: [PATCH v2 2/2] ALSA: hda: Transfer firmware in two chunks

[Cezary Rojewski]

On 2025-01-10 5:44 PM, Takashi Iwai wrote:
> On Fri, 10 Jan 2025 12:33:26 +0100,
> Cezary Rojewski wrote:
>>
>> As per specification, SDxLVI shall be at least 1 i.e.: two chunks to
>> perform a valid transfer. This is true for the PCM transfer code but
>> not firmware-transfer one.
>>
>> Technical background:
>> - the LVI > 0 rule shall be obeyed in PCM transfer
>> - HW permits LVI == 0 when transfer is SW-controlled (SPIB)
>> - FW download is not a PCM transfer and is SW-controlled (SPIB)
>>
>> The above is the fundament which AudioDSP firmware loading functions
>> have been built upon and worked since 2016. The presented changes are to
>> align the loading flows and avoid rising more questions in the future.
>>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   sound/hda/hdac_stream.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
>> index 2670792f43b4..18d74a28a246 100644
>> --- a/sound/hda/hdac_stream.c
>> +++ b/sound/hda/hdac_stream.c
>> @@ -455,6 +455,7 @@ static int setup_bdle(struct hdac_bus *bus,
>>   		      struct hdac_stream *azx_dev, __le32 **bdlp,
>>   		      int ofs, int size, int with_ioc)
>>   {
>> +	u32 bdle_size = size / 2;
>>   	__le32 *bdl = *bdlp;
>>   
>>   	while (size > 0) {
>> @@ -469,7 +470,7 @@ static int setup_bdle(struct hdac_bus *bus,
>>   		bdl[0] = cpu_to_le32((u32)addr);
>>   		bdl[1] = cpu_to_le32(upper_32_bits(addr));
>>   		/* program the size field of the BDL entry */
>> -		chunk = snd_sgbuf_get_chunk_size(dmab, ofs, size);
>> +		chunk = snd_sgbuf_get_chunk_size(dmab, ofs, bdle_size);
>>   		/* one BDLE cannot cross 4K boundary on CTHDA chips */
>>   		if (bus->align_bdle_4k) {
>>   			u32 remain = 0x1000 - (ofs & 0xfff);
> 
> I still think that it's not best place to change.
> 
> For PCM, this workaround isn't needed in most cases, because
> setup_bdle() is called per period, and periods_min = 2 for
> snd-hda-intel.  Doing extra splitting is superfluous for PCM.
> 
> That said, if we need a workaround of the split, it should be done
> conditionally for the firmware stream.

Somehow I did not understand your initial point. Now I do. Indeed, I 
should not have altered setup_bdle() in such fashion as it's used by 
_everyone_. Will review the change and come back with something better.

Kind regards,
Czarek