[PATCH 0/5] ALSA: hda / ASoC: SOF: Add support for PIO command mode

[PATCH 0/5] ALSA: hda / ASoC: SOF: Add support for PIO command mode

[Peter Ujfalusi]

Hi,

This series will introduce support for using PIO for commands (immediate
commands) instead of CORB while retaining the use of RIRB for unsolicited 
responses (notifications).

This mode of operation is the recommended one for Lunar Lake family. 

The series adds support for the PIO mode for SOF and the 'legacy' HDA
stack to make sure that they will work correctly when Lunar Lake is available
for consumers.

Regards,
Peter
---
Peter Ujfalusi (5):
  ALSA: hda: Introduce flags to force commands via PIO instead of CORB
  ALSA: hda: hdac_controller: Implement support for use_pio_for_commands
    mode
  ALSA: pci: hda: hda_controller: Add support for use_pio_for_commands
    mode
  ALSA: hda: Intel: Select AZX_DCAPS_PIO_COMMANDS for Lunar Lake
  ASoC: SOF: Intel: hda-bus: Use PIO mode for Lunar Lake

 include/sound/hdaudio.h        |   1 +
 sound/hda/hdac_controller.c    | 127 +++++++++++++++++++++++++++++++--
 sound/pci/hda/hda_controller.c |   7 +-
 sound/pci/hda/hda_controller.h |   1 +
 sound/pci/hda/hda_intel.c      |   5 +-
 sound/soc/sof/intel/hda-bus.c  |   5 ++
 6 files changed, 136 insertions(+), 10 deletions(-)

-- 
2.44.0

[PATCH 1/5] ALSA: hda: Introduce flags to force commands via PIO instead of CORB

[Peter Ujfalusi]

Add AZX_DCAPS_PIO_COMMANDS quirk (bit 31) and use_pio_for_commands flag to
be able to select PIO mode as alternative for CORB based command sending
while retaining the RIRB functionality to receive unsolicited responses.

This mode differs from the azx single_cmd mode when RIRB is disabled.

The mixed mode is needed on Lunar Lake family because it is recommended to
use Immediate Command Response (PIO mode) instead of CORB for HDA commands.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Liam Girdwood <liam.r.girdwood@intel.com>
---
 include/sound/hdaudio.h        | 1 +
 sound/pci/hda/hda_controller.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index a73d7f34f4e5..85d2c3d00a27 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -351,6 +351,7 @@ struct hdac_bus {
 	bool needs_damn_long_delay:1;
 	bool not_use_interrupts:1;	/* prohibiting the RIRB IRQ */
 	bool access_sdnctl_in_dword:1;	/* accessing the sdnctl register by dword */
+	bool use_pio_for_commands:1;	/* Use PIO instead of CORB for commands */
 
 	int poll_count;
 
diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index 8556031bcd68..c2d0109866e6 100644
--- a/sound/pci/hda/hda_controller.h
+++ b/sound/pci/hda/hda_controller.h
@@ -45,6 +45,7 @@
 #define AZX_DCAPS_CORBRP_SELF_CLEAR (1 << 28)	/* CORBRP clears itself after reset */
 #define AZX_DCAPS_NO_MSI64      (1 << 29)	/* Stick to 32-bit MSIs */
 #define AZX_DCAPS_SEPARATE_STREAM_TAG	(1 << 30) /* capture and playback use separate stream tag */
+#define AZX_DCAPS_PIO_COMMANDS (1 << 31)	/* Use PIO instead of CORB for commands */
 
 enum {
 	AZX_SNOOP_TYPE_NONE,
-- 
2.44.0

[PATCH 2/5] ALSA: hda: hdac_controller: Implement support for use_pio_for_commands mode

[Peter Ujfalusi]

In case the use_pio_for_commands flag is set we must not enable the
CORB DMA to make sure that it is not interfering with the immediate
command mode.

Convert the snd_hdac_bus_send_cmd/snd_hdac_bus_get_response as wrappers to
call either the PIO or CORB based command handling depending on the
use_pio_for_commands flag.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Liam Girdwood <liam.r.girdwood@intel.com>
---
 sound/hda/hdac_controller.c | 127 ++++++++++++++++++++++++++++++++++--
 1 file changed, 120 insertions(+), 7 deletions(-)

diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
index 7f3a000fab0c..b5c833b9f8b9 100644
--- a/sound/hda/hdac_controller.c
+++ b/sound/hda/hdac_controller.c
@@ -62,7 +62,8 @@ void snd_hdac_bus_init_cmd_io(struct hdac_bus *bus)
 		azx_clear_corbrp(bus);
 
 	/* enable corb dma */
-	snd_hdac_chip_writeb(bus, CORBCTL, AZX_CORBCTL_RUN);
+	if (!bus->use_pio_for_commands)
+		snd_hdac_chip_writeb(bus, CORBCTL, AZX_CORBCTL_RUN);
 
 	/* RIRB set up */
 	bus->rirb.addr = bus->rb.addr + 2048;
@@ -135,14 +136,94 @@ static unsigned int azx_command_addr(u32 cmd)
 	return addr;
 }
 
+/* receive an Immediate Response with PIO */
+static int snd_hdac_bus_wait_for_pio_response(struct hdac_bus *bus,
+					      unsigned int addr)
+{
+	int timeout = 50;
+
+	while (timeout--) {
+		/* check IRV bit */
+		if (snd_hdac_chip_readw(bus, IRS) & AZX_IRS_VALID) {
+			/* reuse rirb.res as the response return value */
+			bus->rirb.res[addr] = snd_hdac_chip_readl(bus, IR);
+			return 0;
+		}
+		udelay(1);
+	}
+
+	dev_dbg_ratelimited(bus->dev, "get_response_pio timeout: IRS=%#x\n",
+			    snd_hdac_chip_readw(bus, IRS));
+
+	bus->rirb.res[addr] = -1;
+
+	return -EIO;
+}
+
 /**
- * snd_hdac_bus_send_cmd - send a command verb via CORB
+ * snd_hdac_bus_send_cmd_pio - send a command verb via Immediate Command
  * @bus: HD-audio core bus
  * @val: encoded verb value to send
  *
  * Returns zero for success or a negative error code.
  */
-int snd_hdac_bus_send_cmd(struct hdac_bus *bus, unsigned int val)
+static int snd_hdac_bus_send_cmd_pio(struct hdac_bus *bus, unsigned int val)
+{
+	unsigned int addr = azx_command_addr(val);
+	int timeout = 50;
+	int ret = -EIO;
+
+	spin_lock_irq(&bus->reg_lock);
+
+	while (timeout--) {
+		/* check ICB bit */
+		if (!((snd_hdac_chip_readw(bus, IRS) & AZX_IRS_BUSY))) {
+			/* Clear IRV bit */
+			snd_hdac_chip_updatew(bus, IRS, AZX_IRS_VALID, AZX_IRS_VALID);
+			snd_hdac_chip_writel(bus, IC, val);
+			/* Set ICB bit */
+			snd_hdac_chip_updatew(bus, IRS, AZX_IRS_BUSY, AZX_IRS_BUSY);
+
+			ret = snd_hdac_bus_wait_for_pio_response(bus, addr);
+			goto out;
+		}
+		udelay(1);
+	}
+
+	dev_dbg_ratelimited(bus->dev, "send_cmd_pio timeout: IRS=%#x, val=%#x\n",
+			    snd_hdac_chip_readw(bus, IRS), val);
+
+out:
+	spin_unlock_irq(&bus->reg_lock);
+
+	return ret;
+}
+
+/**
+ * snd_hdac_bus_get_response_pio - receive a response via Immediate Response
+ * @bus: HD-audio core bus
+ * @addr: codec address
+ * @res: pointer to store the value, NULL when not needed
+ *
+ * Returns zero if a value is read, or a negative error code.
+ */
+static int snd_hdac_bus_get_response_pio(struct hdac_bus *bus,
+					 unsigned int addr, unsigned int *res)
+{
+	if (res)
+		*res = bus->rirb.res[addr];
+
+	return 0;
+}
+
+/**
+ * snd_hdac_bus_send_cmd_corb - send a command verb via CORB
+ * @bus: HD-audio core bus
+ * @val: encoded verb value to send
+ *
+ * Returns zero for success or a negative error code.
+ */
+static int snd_hdac_bus_send_cmd_corb(struct hdac_bus *bus, unsigned int val)
 {
 	unsigned int addr = azx_command_addr(val);
 	unsigned int wp, rp;
@@ -176,7 +257,6 @@ int snd_hdac_bus_send_cmd(struct hdac_bus *bus, unsigned int val)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(snd_hdac_bus_send_cmd);
 
 #define AZX_RIRB_EX_UNSOL_EV	(1<<4)
 
@@ -234,15 +314,15 @@ void snd_hdac_bus_update_rirb(struct hdac_bus *bus)
 EXPORT_SYMBOL_GPL(snd_hdac_bus_update_rirb);
 
 /**
- * snd_hdac_bus_get_response - receive a response via RIRB
+ * snd_hdac_bus_get_response_rirb - receive a response via RIRB
  * @bus: HD-audio core bus
  * @addr: codec address
  * @res: pointer to store the value, NULL when not needed
  *
  * Returns zero if a value is read, or a negative error code.
  */
-int snd_hdac_bus_get_response(struct hdac_bus *bus, unsigned int addr,
-			      unsigned int *res)
+static int snd_hdac_bus_get_response_rirb(struct hdac_bus *bus,
+					  unsigned int addr, unsigned int *res)
 {
 	unsigned long timeout;
 	unsigned long loopcounter;
@@ -293,6 +373,39 @@ int snd_hdac_bus_get_response(struct hdac_bus *bus, unsigned int addr,
 
 	return -EIO;
 }
+
+/**
+ * snd_hdac_bus_send_cmd - send a command verb via CORB or PIO
+ * @bus: HD-audio core bus
+ * @val: encoded verb value to send
+ *
+ * Returns zero for success or a negative error code.
+ */
+int snd_hdac_bus_send_cmd(struct hdac_bus *bus, unsigned int val)
+{
+	if (bus->use_pio_for_commands)
+		return snd_hdac_bus_send_cmd_pio(bus, val);
+
+	return snd_hdac_bus_send_cmd_corb(bus, val);
+}
+EXPORT_SYMBOL_GPL(snd_hdac_bus_send_cmd);
+
+/**
+ * snd_hdac_bus_get_response - receive a response via RIRB or PIO
+ * @bus: HD-audio core bus
+ * @addr: codec address
+ * @res: pointer to store the value, NULL when not needed
+ *
+ * Returns zero if a value is read, or a negative error code.
+ */
+int snd_hdac_bus_get_response(struct hdac_bus *bus, unsigned int addr,
+			      unsigned int *res)
+{
+	if (bus->use_pio_for_commands)
+		return snd_hdac_bus_get_response_pio(bus, addr, res);
+
+	return snd_hdac_bus_get_response_rirb(bus, addr, res);
+}
 EXPORT_SYMBOL_GPL(snd_hdac_bus_get_response);
 
 #define HDAC_MAX_CAPS 10
-- 
2.44.0

[PATCH 3/5] ALSA: pci: hda: hda_controller: Add support for use_pio_for_commands mode

[Peter Ujfalusi]

Set the use_pio_for_commands flag in case AZX_DCAPS_PIO_COMMANDS quirk is
enabled.

When the PIO command mode is used we can re-use the existing
azx_single_send_cmd() / azx_single_get_response() functions safely as the
CORB DMA is not going to be enabled in snd_hdac_bus_init_cmd_io().

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Liam Girdwood <liam.r.girdwood@intel.com>
---
 sound/pci/hda/hda_controller.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 206306a0eb82..8af5ee1b0ea8 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -914,7 +914,7 @@ static int azx_send_cmd(struct hdac_bus *bus, unsigned int val)
 
 	if (chip->disabled)
 		return 0;
-	if (chip->single_cmd)
+	if (chip->single_cmd || bus->use_pio_for_commands)
 		return azx_single_send_cmd(bus, val);
 	else
 		return snd_hdac_bus_send_cmd(bus, val);
@@ -928,7 +928,7 @@ static int azx_get_response(struct hdac_bus *bus, unsigned int addr,
 
 	if (chip->disabled)
 		return 0;
-	if (chip->single_cmd)
+	if (chip->single_cmd || bus->use_pio_for_commands)
 		return azx_single_get_response(bus, addr, res);
 	else
 		return azx_rirb_get_response(bus, addr, res);
@@ -1188,6 +1188,9 @@ int azx_bus_init(struct azx *chip, const char *model)
 	if (chip->driver_caps & AZX_DCAPS_4K_BDLE_BOUNDARY)
 		bus->core.align_bdle_4k = true;
 
+	if (chip->driver_caps & AZX_DCAPS_PIO_COMMANDS)
+		bus->core.use_pio_for_commands = true;
+
 	/* enable sync_write flag for stable communication as default */
 	bus->core.sync_write = 1;
 
-- 
2.44.0

[PATCH 4/5] ALSA: hda: Intel: Select AZX_DCAPS_PIO_COMMANDS for Lunar Lake

[Peter Ujfalusi]

It is recommended that on Lunar Lake the PIO (immediate command response)
is used instead of CORB/RIRB for commands/verbs.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Liam Girdwood <liam.r.girdwood@intel.com>
---
 sound/pci/hda/hda_intel.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 1b550c42db09..6a3022a87245 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -289,6 +289,9 @@ enum {
 
 #define AZX_DCAPS_INTEL_BROXTON		AZX_DCAPS_INTEL_SKYLAKE
 
+#define AZX_DCAPS_INTEL_LNL \
+	(AZX_DCAPS_INTEL_SKYLAKE | AZX_DCAPS_PIO_COMMANDS)
+
 /* quirks for ATI SB / AMD Hudson */
 #define AZX_DCAPS_PRESET_ATI_SB \
 	(AZX_DCAPS_NO_TCSEL | AZX_DCAPS_POSFIX_LPIB |\
@@ -2503,7 +2506,7 @@ static const struct pci_device_id azx_ids[] = {
 	{ PCI_DEVICE_DATA(INTEL, HDA_RPL_PX, AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE) },
 	{ PCI_DEVICE_DATA(INTEL, HDA_MTL, AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE) },
 	/* Lunarlake-P */
-	{ PCI_DEVICE_DATA(INTEL, HDA_LNL_P, AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE) },
+	{ PCI_DEVICE_DATA(INTEL, HDA_LNL_P, AZX_DRIVER_SKL | AZX_DCAPS_INTEL_LNL) },
 	/* Arrow Lake-S */
 	{ PCI_DEVICE_DATA(INTEL, HDA_ARL_S, AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE) },
 	/* Arrow Lake */
-- 
2.44.0

[PATCH 5/5] ASoC: SOF: Intel: hda-bus: Use PIO mode for Lunar Lake

[Peter Ujfalusi]

It is recommended that on Lunar Lake the PIO (immediate command response)
is used instead of CORB/RIRB for commands/verbs.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Liam Girdwood <liam.r.girdwood@intel.com>
---
 sound/soc/sof/intel/hda-bus.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/soc/sof/intel/hda-bus.c b/sound/soc/sof/intel/hda-bus.c
index fc63085d2d74..f78d6b998be0 100644
--- a/sound/soc/sof/intel/hda-bus.c
+++ b/sound/soc/sof/intel/hda-bus.c
@@ -72,7 +72,12 @@ void sof_hda_bus_init(struct snd_sof_dev *sdev, struct device *dev)
 
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_LINK)
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)
+	const struct sof_intel_dsp_desc *chip = get_chip_info(sdev->pdata);
+
 	snd_hdac_ext_bus_init(bus, dev, &bus_core_ops, sof_hda_ext_ops);
+
+	if (chip && chip->hw_ip_version == SOF_INTEL_ACE_2_0)
+		bus->use_pio_for_commands = true;
 #else
 	snd_hdac_ext_bus_init(bus, dev, NULL, NULL);
 #endif
-- 
2.44.0

Re: [PATCH 0/5] ALSA: hda / ASoC: SOF: Add support for PIO command mode

[Jaroslav Kysela]

On 09. 04. 24 10:38, Peter Ujfalusi wrote:
> Hi,
> 
> This series will introduce support for using PIO for commands (immediate
> commands) instead of CORB while retaining the use of RIRB for unsolicited
> responses (notifications).

Hi,

Could you elaborate, how this affects CPU usage (busy loops for I/O)? Do the 
bus read operation wait until the data transfer on bus is finished?

> This mode of operation is the recommended one for Lunar Lake family.

A bit more explanation, please. It looks like a step back.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH 5/5] ASoC: SOF: Intel: hda-bus: Use PIO mode for Lunar Lake

[Mark Brown]

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

On Tue, Apr 09, 2024 at 11:38:12AM +0300, Peter Ujfalusi wrote:
> It is recommended that on Lunar Lake the PIO (immediate command response)
> is used instead of CORB/RIRB for commands/verbs.

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

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

Re: [PATCH 0/5] ALSA: hda / ASoC: SOF: Add support for PIO command mode

[Péter Ujfalusi]

Hi Jaroslav,

On 09/04/2024 11:53, Jaroslav Kysela wrote:
> On 09. 04. 24 10:38, Peter Ujfalusi wrote:
>> Hi,
>>
>> This series will introduce support for using PIO for commands (immediate
>> commands) instead of CORB while retaining the use of RIRB for unsolicited
>> responses (notifications).
> 
> Hi,
> 
> Could you elaborate, how this affects CPU usage (busy loops for I/O)? Do
> the bus read operation wait until the data transfer on bus is finished?
> 
>> This mode of operation is the recommended one for Lunar Lake family.
> 
> A bit more explanation, please. It looks like a step back.

LunarLake is a different architecture compared to previous solutions, so
some things are improved with e.g. all audio interfaces using the
HDaudio DMA, and others are done differently (commands, wakes, etc).
In practice, we have not found any cases where performance would be
significantly impacted by the use of PIO v. DMA. The number of 'verbs'
sent is usually fairly limited.

The major concern we had initially was firmware download for Cirrus/TI
amps behind an HDaudio bridge, but all of those solutions rely on a side
I2C/SPI bus so the firmware does not rely on the HDaudio commands.
Hope this answers to your questions

-- 
Péter

Re: [PATCH 0/5] ALSA: hda / ASoC: SOF: Add support for PIO command mode

[Takashi Iwai]

On Tue, 09 Apr 2024 10:38:07 +0200,
Peter Ujfalusi wrote:
> 
> Hi,
> 
> This series will introduce support for using PIO for commands (immediate
> commands) instead of CORB while retaining the use of RIRB for unsolicited 
> responses (notifications).
> 
> This mode of operation is the recommended one for Lunar Lake family. 
> 
> The series adds support for the PIO mode for SOF and the 'legacy' HDA
> stack to make sure that they will work correctly when Lunar Lake is available
> for consumers.
> 
> Regards,
> Peter
> ---
> Peter Ujfalusi (5):
>   ALSA: hda: Introduce flags to force commands via PIO instead of CORB
>   ALSA: hda: hdac_controller: Implement support for use_pio_for_commands
>     mode
>   ALSA: pci: hda: hda_controller: Add support for use_pio_for_commands
>     mode
>   ALSA: hda: Intel: Select AZX_DCAPS_PIO_COMMANDS for Lunar Lake
>   ASoC: SOF: Intel: hda-bus: Use PIO mode for Lunar Lake

Now applied all patches to for-next branch.  Thanks.


Takashi