[RFC PATCH 0/5] ASoC: SOF: Intel: SoundWire initial integration

[RFC PATCH 0/5] ASoC: SOF: Intel: SoundWire initial integration

[Pierre-Louis Bossart]

This RFC is the companion of the other RFC 'soundwire: intel: simplify
DAI/PDI handling​'. Our purpose at this point is to gather feedback on
the interfaces between the Intel SOF parts and the SoundWire code.

The suggested solution is a simple init/release inserted at
probe/remove and resume/suspend, as well as two callbacks for the SOF
driver to generate IPC configurations with the firmware. That level of
separation completely hides the details of the SoundWire DAIs and will
allow for 'transparent' multi-cpu DAI support, which will be handled
in the machine driver and the soundwire DAIs.

This solution was tested on IceLake and CometLake, and captures the
feedback from SOF contributors on an initial integration that was
deemed too complicated (and rightly so).

Pierre-Louis Bossart (5):
  ASoC: SOF: IPC: dai-intel: move ALH declarations in header file
  ASoC: SOF: Intel: hda: add helper to initialize SoundWire IP
  ASoC: SOF: Intel: hda: add SoundWire IP support
  ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks
  ASoC: SOF: Intel: add support for SoundWire suspend/resume

 include/sound/sof/dai-intel.h |  18 ++--
 sound/soc/sof/intel/hda-dsp.c |  11 +++
 sound/soc/sof/intel/hda.c     | 157 ++++++++++++++++++++++++++++++++++
 sound/soc/sof/intel/hda.h     |  11 +++
 4 files changed, 188 insertions(+), 9 deletions(-)


base-commit: 3b3aaa017e8072b1bfddda92be296b3463d870be
-- 
2.20.1

[RFC PATCH 1/5] ASoC: SOF: IPC: dai-intel: move ALH declarations in header file

[Pierre-Louis Bossart]

ALH was inserted in the wrong place during integration, add after DMIC
to mirror the file used by SOF firmware.

No functional change, just text move in the same file to better track
changes, if any.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/sound/sof/dai-intel.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/sound/sof/dai-intel.h b/include/sound/sof/dai-intel.h
index 5f1ef5565be6..04e48227f542 100644
--- a/include/sound/sof/dai-intel.h
+++ b/include/sound/sof/dai-intel.h
@@ -87,6 +87,15 @@ struct sof_ipc_dai_hda_params {
 	uint32_t link_dma_ch;
 } __packed;
 
+/* ALH Configuration Request - SOF_IPC_DAI_ALH_CONFIG */
+struct sof_ipc_dai_alh_params {
+	struct sof_ipc_hdr hdr;
+	uint32_t stream_id;
+
+	/* reserved for future use */
+	uint32_t reserved[15];
+} __packed;
+
 /* DMIC Configuration Request - SOF_IPC_DAI_DMIC_CONFIG */
 
 /* This struct is defined per 2ch PDM controller available in the platform.
@@ -179,13 +188,4 @@ struct sof_ipc_dai_dmic_params {
 	struct sof_ipc_dai_dmic_pdm_ctrl pdm[0];
 } __packed;
 
-/* ALH Configuration Request - SOF_IPC_DAI_ALH_CONFIG */
-struct sof_ipc_dai_alh_params {
-	struct sof_ipc_hdr hdr;
-	uint32_t stream_id;
-
-	/* reserved for future use */
-	uint32_t reserved[15];
-} __packed;
-
 #endif
-- 
2.20.1

[RFC PATCH 2/5] ASoC: SOF: Intel: hda: add helper to initialize SoundWire IP

[Pierre-Louis Bossart]

The helper calls the SoundWire initializations and sets the resource
structure defined as an interface with the platform driver. The
resource deals with PCI base address, interrupts and the callback for
the stream configuration.

The interrupts are gated/ungated at the top-level with the
ADSPIPC2.SNDW field

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/intel/hda.c | 63 +++++++++++++++++++++++++++++++++++++++
 sound/soc/sof/intel/hda.h |  1 +
 2 files changed, 64 insertions(+)

diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index c72e9a09eee1..a968890d0754 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -18,7 +18,9 @@
 #include <sound/hdaudio_ext.h>
 #include <sound/hda_register.h>
 
+#include <linux/acpi.h>
 #include <linux/module.h>
+#include <linux/soundwire/sdw_intel.h>
 #include <sound/intel-nhlt.h>
 #include <sound/sof.h>
 #include <sound/sof/xtensa.h>
@@ -35,6 +37,67 @@
 #define IS_CFL(pci) ((pci)->vendor == 0x8086 && (pci)->device == 0xa348)
 #define IS_CNL(pci) ((pci)->vendor == 0x8086 && (pci)->device == 0x9dc8)
 
+#if IS_ENABLED(CONFIG_SOUNDWIRE_INTEL)
+
+static void hda_sdw_int_enable(struct snd_sof_dev *sdev, bool enable)
+{
+	if (enable)
+		snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR,
+					HDA_DSP_REG_ADSPIC2,
+					HDA_DSP_ADSPIC2_SNDW,
+					HDA_DSP_ADSPIC2_SNDW);
+	else
+		snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR,
+					HDA_DSP_REG_ADSPIC2,
+					HDA_DSP_ADSPIC2_SNDW,
+					0);
+}
+
+static int hda_sdw_init(struct snd_sof_dev *sdev)
+{
+	acpi_handle handle;
+	struct sdw_intel_res res;
+
+	handle = ACPI_HANDLE(sdev->dev);
+
+	memset(&res, 0, sizeof(res));
+
+	res.mmio_base = sdev->bar[HDA_DSP_BAR];
+	res.irq = sdev->ipc_irq;
+	res.parent = sdev->dev;
+
+	hda_sdw_int_enable(sdev, true);
+
+	sdev->sdw = sdw_intel_init(handle, &res);
+	if (!sdev->sdw) {
+		dev_err(sdev->dev, "SDW Init failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int hda_sdw_exit(struct snd_sof_dev *sdev)
+{
+	hda_sdw_int_enable(sdev, false);
+
+	if (sdev->sdw)
+		sdw_intel_exit(sdev->sdw);
+
+	return 0;
+}
+#else
+static int hda_sdw_init(struct snd_sof_dev *sdev)
+{
+	return 0;
+}
+
+static int hda_sdw_exit(struct snd_sof_dev *sdev)
+{
+	return 0;
+}
+#endif
+
 /*
  * Debug
  */
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 5591841a1b6f..c8f93317aeb4 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -211,6 +211,7 @@
 
 #define HDA_DSP_ADSPIC_IPC			1
 #define HDA_DSP_ADSPIS_IPC			1
+#define HDA_DSP_ADSPIC2_SNDW			BIT(5)
 
 /* Intel HD Audio General DSP Registers */
 #define HDA_DSP_GEN_BASE		0x0
-- 
2.20.1

[RFC PATCH 3/5] ASoC: SOF: Intel: hda: add SoundWire IP support

[Pierre-Louis Bossart]

The Core0 needs to be powered before the SoundWire IP is initialized.

Call sdw_intel_init/exit and store the context. We only have one
context, but depending on the hardware capabilities and BIOS settings
may enable multiple SoundWire links.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/intel/hda.c | 40 +++++++++++++++++++++++++++++++++------
 sound/soc/sof/intel/hda.h |  5 +++++
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index a968890d0754..e754058e3679 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -57,6 +57,8 @@ static int hda_sdw_init(struct snd_sof_dev *sdev)
 {
 	acpi_handle handle;
 	struct sdw_intel_res res;
+	struct sof_intel_hda_dev *hdev;
+	void *sdw;
 
 	handle = ACPI_HANDLE(sdev->dev);
 
@@ -66,23 +68,32 @@ static int hda_sdw_init(struct snd_sof_dev *sdev)
 	res.irq = sdev->ipc_irq;
 	res.parent = sdev->dev;
 
-	hda_sdw_int_enable(sdev, true);
-
-	sdev->sdw = sdw_intel_init(handle, &res);
-	if (!sdev->sdw) {
+	sdw = sdw_intel_init(handle, &res);
+	if (!sdw) {
 		dev_err(sdev->dev, "SDW Init failed\n");
 		return -EIO;
 	}
 
+	hda_sdw_int_enable(sdev, true);
+
+	/* save context */
+	hdev = sdev->pdata->hw_pdata;
+	hdev->sdw = sdw;
+
 	return 0;
 }
 
 static int hda_sdw_exit(struct snd_sof_dev *sdev)
 {
+	struct sof_intel_hda_dev *hdev;
+
+	hdev = sdev->pdata->hw_pdata;
+
 	hda_sdw_int_enable(sdev, false);
 
-	if (sdev->sdw)
-		sdw_intel_exit(sdev->sdw);
+	if (hdev->sdw)
+		sdw_intel_exit(hdev->sdw);
+	hdev->sdw = NULL;
 
 	return 0;
 }
@@ -713,6 +724,21 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
 	/* set default mailbox offset for FW ready message */
 	sdev->dsp_box.offset = HDA_DSP_MBOX_UPLINK_OFFSET;
 
+	/* need to power-up core before setting-up capabilities */
+	ret = hda_dsp_core_power_up(sdev, HDA_DSP_CORE_MASK(0));
+	if (ret < 0) {
+		dev_err(sdev->dev, "error: could not power-up DSP subsystem\n");
+		goto free_ipc_irq;
+	}
+
+	/* initialize SoundWire capabilities */
+	ret = hda_sdw_init(sdev);
+	if (ret < 0) {
+		dev_err(sdev->dev, "error: SoundWire get caps error\n");
+		hda_dsp_core_power_down(sdev, HDA_DSP_CORE_MASK(0));
+		goto free_ipc_irq;
+	}
+
 	return 0;
 
 free_ipc_irq:
@@ -744,6 +770,8 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
 	snd_hdac_ext_bus_device_remove(bus);
 #endif
 
+	hda_sdw_exit(sdev);
+
 	if (!IS_ERR_OR_NULL(hda->dmic_dev))
 		platform_device_unregister(hda->dmic_dev);
 
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index c8f93317aeb4..48e09b7daf0a 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -399,6 +399,11 @@ struct sof_intel_hda_dev {
 
 	/* DMIC device */
 	struct platform_device *dmic_dev;
+
+#if IS_ENABLED(CONFIG_SOUNDWIRE_INTEL)
+	/* sdw context */
+	void *sdw;
+#endif
 };
 
 static inline struct hdac_bus *sof_to_bus(struct snd_sof_dev *s)
-- 
2.20.1

[RFC PATCH 4/5] ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks

[Pierre-Louis Bossart]

These callbacks are invoked when a matching hw_params/hw_free() DAI
operation takes place, and will result in IPC operations with the SOF
firmware.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/intel/hda.c | 66 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index e754058e3679..1e84ea9e6fce 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -53,6 +53,70 @@ static void hda_sdw_int_enable(struct snd_sof_dev *sdev, bool enable)
 					0);
 }
 
+static int sdw_config_stream(void *arg, void *s, void *dai,
+			     void *params, int link_id, int alh_stream_id)
+{
+	struct snd_sof_dev *sdev = arg;
+	struct snd_soc_dai *d = dai;
+	struct sof_ipc_dai_config config;
+	struct sof_ipc_reply reply;
+	int ret;
+	u32 size = sizeof(config);
+
+	memset(&config, 0, size);
+	config.hdr.size = size;
+	config.hdr.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG;
+	config.type = SOF_DAI_INTEL_ALH;
+	config.dai_index = (link_id << 8) | (d->id);
+	config.alh.stream_id = alh_stream_id;
+
+	/* send message to DSP */
+	ret = sof_ipc_tx_message(sdev->ipc,
+				 config.hdr.cmd, &config, size, &reply,
+				 sizeof(reply));
+	if (ret < 0) {
+		dev_err(sdev->dev,
+			"error: failed to set DAI hw_params for link %d dai->id %d ALH %d\n",
+			link_id, d->id, alh_stream_id);
+	}
+
+	return ret;
+}
+
+static int sdw_free_stream(void *arg, void *s, void *dai, int link_id)
+{
+	struct snd_sof_dev *sdev = arg;
+	struct snd_soc_dai *d = dai;
+	struct sof_ipc_dai_config config;
+	struct sof_ipc_reply reply;
+	int ret;
+	u32 size = sizeof(config);
+
+	memset(&config, 0, size);
+	config.hdr.size = size;
+	config.hdr.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG;
+	config.type = SOF_DAI_INTEL_ALH;
+	config.dai_index = (link_id << 8) | d->id;
+	config.alh.stream_id = 0xFFFF; /* invalid value on purpose */
+
+	/* send message to DSP */
+	ret = sof_ipc_tx_message(sdev->ipc,
+				 config.hdr.cmd, &config, size, &reply,
+				 sizeof(reply));
+	if (ret < 0) {
+		dev_err(sdev->dev,
+			"error: failed to free stream for link %d dai->id %d\n",
+			link_id, d->id);
+	}
+
+	return ret;
+}
+
+static const struct sdw_intel_ops sdw_callback = {
+	.config_stream = sdw_config_stream,
+	.free_stream = sdw_free_stream,
+};
+
 static int hda_sdw_init(struct snd_sof_dev *sdev)
 {
 	acpi_handle handle;
@@ -67,6 +131,8 @@ static int hda_sdw_init(struct snd_sof_dev *sdev)
 	res.mmio_base = sdev->bar[HDA_DSP_BAR];
 	res.irq = sdev->ipc_irq;
 	res.parent = sdev->dev;
+	res.ops = &sdw_callback;
+	res.arg = sdev;
 
 	sdw = sdw_intel_init(handle, &res);
 	if (!sdw) {
-- 
2.20.1

[RFC PATCH 5/5] ASoC: SOF: Intel: add support for SoundWire suspend/resume

[Pierre-Louis Bossart]

Somehow the core0 needs to be on to set-up the interrupts and power-up
the SoundWire IP.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/intel/hda-dsp.c | 11 +++++++++++
 sound/soc/sof/intel/hda.c     |  2 +-
 sound/soc/sof/intel/hda.h     |  5 +++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
index fb55a3c5afd0..e1ade59ac6e1 100644
--- a/sound/soc/sof/intel/hda-dsp.c
+++ b/sound/soc/sof/intel/hda-dsp.c
@@ -374,6 +374,17 @@ static int hda_resume(struct snd_sof_dev *sdev, bool runtime_resume)
 	hda_dsp_ctrl_ppcap_enable(sdev, true);
 	hda_dsp_ctrl_ppcap_int_enable(sdev, true);
 
+#if IS_ENABLED(CONFIG_SOUNDWIRE_INTEL)
+	/* need to power-up core before setting-up capabilities */
+	ret = hda_dsp_core_power_up(sdev, HDA_DSP_CORE_MASK(0));
+	if (ret < 0) {
+		dev_err(sdev->dev, "error: could not power-up DSP subsystem\n");
+		return ret;
+	}
+
+	hda_sdw_int_enable(sdev, true);
+#endif
+
 	return 0;
 }
 
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index 1e84ea9e6fce..09aa0cfa6099 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -39,7 +39,7 @@
 
 #if IS_ENABLED(CONFIG_SOUNDWIRE_INTEL)
 
-static void hda_sdw_int_enable(struct snd_sof_dev *sdev, bool enable)
+void hda_sdw_int_enable(struct snd_sof_dev *sdev, bool enable)
 {
 	if (enable)
 		snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR,
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 48e09b7daf0a..de71c92b2f39 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -591,6 +591,11 @@ int hda_dsp_trace_init(struct snd_sof_dev *sdev, u32 *stream_tag);
 int hda_dsp_trace_release(struct snd_sof_dev *sdev);
 int hda_dsp_trace_trigger(struct snd_sof_dev *sdev, int cmd);
 
+/*
+ * SoundWire support
+ */
+void hda_sdw_int_enable(struct snd_sof_dev *sdev, bool enable);
+
 /* common dai driver */
 extern struct snd_soc_dai_driver skl_dai[];
 
-- 
2.20.1

Re: [RFC PATCH 4/5] ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks

[Guennadi Liakhovetski]

Hi Pierre,

A couple of comments below

On Wed, Aug 21, 2019 at 03:17:19PM -0500, Pierre-Louis Bossart wrote:
> These callbacks are invoked when a matching hw_params/hw_free() DAI
> operation takes place, and will result in IPC operations with the SOF
> firmware.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/soc/sof/intel/hda.c | 66 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
> index e754058e3679..1e84ea9e6fce 100644
> --- a/sound/soc/sof/intel/hda.c
> +++ b/sound/soc/sof/intel/hda.c
> @@ -53,6 +53,70 @@ static void hda_sdw_int_enable(struct snd_sof_dev *sdev, bool enable)
>  					0);
>  }
>  
> +static int sdw_config_stream(void *arg, void *s, void *dai,
> +			     void *params, int link_id, int alh_stream_id)

I realise, that these function prototypes aren't being introduced by these 
patches, but just wondering whether such overly generic prototype is really 
a good idea here, whether some of those "void *" pointers could be given 
real types. The first one could be "struct device *" etc.

> +{
> +	struct snd_sof_dev *sdev = arg;
> +	struct snd_soc_dai *d = dai;
> +	struct sof_ipc_dai_config config;
> +	struct sof_ipc_reply reply;
> +	int ret;
> +	u32 size = sizeof(config);
> +
> +	memset(&config, 0, size);
> +	config.hdr.size = size;
> +	config.hdr.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG;
> +	config.type = SOF_DAI_INTEL_ALH;
> +	config.dai_index = (link_id << 8) | (d->id);
> +	config.alh.stream_id = alh_stream_id;

Entirely up to you, in such cases I usually do something like

+	struct sof_ipc_dai_config config = {
+		.type = SOF_DAI_INTEL_ALH,
+		.hre = {
+			.size = sizeof(config),
+			.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG,
+			...

which then also avoids a memset(). But that's mostly a matter of personal 
preference, since this is on stack, the compiler would probably internally 
anyway translate the above initialisation to a memset() with all the 
following assignments.

> +
> +	/* send message to DSP */
> +	ret = sof_ipc_tx_message(sdev->ipc,
> +				 config.hdr.cmd, &config, size, &reply,
> +				 sizeof(reply));
> +	if (ret < 0) {
> +		dev_err(sdev->dev,
> +			"error: failed to set DAI hw_params for link %d dai->id %d ALH %d\n",

Are readers really expected to understand what "dai->id" means? Wouldn't 
"DAI ID" be friendlier, although I understand you - who might not know 
what "x->y" stands for?.. ;-)

> +			link_id, d->id, alh_stream_id);
> +	}
> +
> +	return ret;
> +}
> +
> +static int sdw_free_stream(void *arg, void *s, void *dai, int link_id)
> +{
> +	struct snd_sof_dev *sdev = arg;
> +	struct snd_soc_dai *d = dai;
> +	struct sof_ipc_dai_config config;
> +	struct sof_ipc_reply reply;
> +	int ret;
> +	u32 size = sizeof(config);
> +
> +	memset(&config, 0, size);
> +	config.hdr.size = size;
> +	config.hdr.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG;
> +	config.type = SOF_DAI_INTEL_ALH;
> +	config.dai_index = (link_id << 8) | d->id;
> +	config.alh.stream_id = 0xFFFF; /* invalid value on purpose */

ditto

> +
> +	/* send message to DSP */
> +	ret = sof_ipc_tx_message(sdev->ipc,
> +				 config.hdr.cmd, &config, size, &reply,
> +				 sizeof(reply));
> +	if (ret < 0) {
> +		dev_err(sdev->dev,
> +			"error: failed to free stream for link %d dai->id %d\n",
> +			link_id, d->id);

ditto

> +	}
> +
> +	return ret;
> +}
> +
> +static const struct sdw_intel_ops sdw_callback = {
> +	.config_stream = sdw_config_stream,
> +	.free_stream = sdw_free_stream,
> +};
> +
>  static int hda_sdw_init(struct snd_sof_dev *sdev)
>  {
>  	acpi_handle handle;
> @@ -67,6 +131,8 @@ static int hda_sdw_init(struct snd_sof_dev *sdev)
>  	res.mmio_base = sdev->bar[HDA_DSP_BAR];
>  	res.irq = sdev->ipc_irq;
>  	res.parent = sdev->dev;
> +	res.ops = &sdw_callback;
> +	res.arg = sdev;
>  
>  	sdw = sdw_intel_init(handle, &res);
>  	if (!sdw) {

Hm, looks like this function is using spaces for indentation... Let me check 
if this is coming from an earlier patch

Thanks
Guennadi

> -- 
> 2.20.1
> 

Re: [RFC PATCH 4/5] ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks

[Guennadi Liakhovetski]

On Thu, Aug 22, 2019 at 09:18:35AM +0200, Guennadi Liakhovetski wrote:

[snip]

> >  static int hda_sdw_init(struct snd_sof_dev *sdev)
> >  {
> >  	acpi_handle handle;
> > @@ -67,6 +131,8 @@ static int hda_sdw_init(struct snd_sof_dev *sdev)
> >  	res.mmio_base = sdev->bar[HDA_DSP_BAR];
> >  	res.irq = sdev->ipc_irq;
> >  	res.parent = sdev->dev;
> > +	res.ops = &sdw_callback;
> > +	res.arg = sdev;
> >  
> >  	sdw = sdw_intel_init(handle, &res);
> >  	if (!sdw) {
> 
> Hm, looks like this function is using spaces for indentation... Let me check 
> if this is coming from an earlier patch

Ouch, it's mutt or whatever editor it's using... Sorry for the noise.

Thanks
Guennadi

Re: [RFC PATCH 4/5] ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks

[Pierre-Louis Bossart]

Thanks for the review Guennadi

>> +static int sdw_config_stream(void *arg, void *s, void *dai,
>> +			     void *params, int link_id, int alh_stream_id)
> 
> I realise, that these function prototypes aren't being introduced by these
> patches, but just wondering whether such overly generic prototype is really
> a good idea here, whether some of those "void *" pointers could be given
> real types. The first one could be "struct device *" etc.

In this case the 'arg' parameter is actually a private 'struct 
snd_sof_dev', as shown below [1]. We probably want to keep this 
relatively opaque, this is a context that doesn't need to be exposed to 
the SoundWire code.

The dai and params are indeed cases where we could use stronger types, 
they are snd_soc_dai and hw_params respectively. I don't recall why the 
existing code is this way, Vinod and Sanyog may have the history of this.

> 
>> +{
>> +	struct snd_sof_dev *sdev = arg;
>> +	struct snd_soc_dai *d = dai;
[1]

>> +	struct sof_ipc_dai_config config;
>> +	struct sof_ipc_reply reply;
>> +	int ret;
>> +	u32 size = sizeof(config);
>> +
>> +	memset(&config, 0, size);
>> +	config.hdr.size = size;
>> +	config.hdr.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG;
>> +	config.type = SOF_DAI_INTEL_ALH;
>> +	config.dai_index = (link_id << 8) | (d->id);
>> +	config.alh.stream_id = alh_stream_id;
> 
> Entirely up to you, in such cases I usually do something like
> 
> +	struct sof_ipc_dai_config config = {
> +		.type = SOF_DAI_INTEL_ALH,
> +		.hre = {
> +			.size = sizeof(config),
> +			.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG,
> +			...
> 
> which then also avoids a memset(). But that's mostly a matter of personal
> preference, since this is on stack, the compiler would probably internally
> anyway translate the above initialisation to a memset() with all the
> following assignments.

I have no preference, so in this case I will go with consistency with 
existing code, which uses the suggested style for all IPCs.

> 
>> +
>> +	/* send message to DSP */
>> +	ret = sof_ipc_tx_message(sdev->ipc,
>> +				 config.hdr.cmd, &config, size, &reply,
>> +				 sizeof(reply));
>> +	if (ret < 0) {
>> +		dev_err(sdev->dev,
>> +			"error: failed to set DAI hw_params for link %d dai->id %d ALH %d\n",
> 
> Are readers really expected to understand what "dai->id" means? Wouldn't
> "DAI ID" be friendlier, although I understand you - who might not know
> what "x->y" stands for?.. ;-)

I was trying to avoid a confusion here, we have config->dai_index which 
are shared concepts between topology and firmware, and dai->id which is 
shared between topology and machine driver (which refers to the dai in 
the dai_link which has its own .id). In topology files we have the three 
indices and of course after a couple of weeks I can't recall which one 
maps to what.
I am afraid DAI ID might be confused with dai_index. If there are 
suggestions on this I am all ears, all I care about is avoiding 
ambiguity and having to ask Ranjani what index this really is :-)

Re: [RFC PATCH 4/5] ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks

[Guennadi Liakhovetski]

On Thu, Aug 22, 2019 at 08:53:06AM -0500, Pierre-Louis Bossart wrote:
> Thanks for the review Guennadi
> 
> > > +static int sdw_config_stream(void *arg, void *s, void *dai,
> > > +			     void *params, int link_id, int alh_stream_id)
> > 
> > I realise, that these function prototypes aren't being introduced by these
> > patches, but just wondering whether such overly generic prototype is really
> > a good idea here, whether some of those "void *" pointers could be given
> > real types. The first one could be "struct device *" etc.
> 
> In this case the 'arg' parameter is actually a private 'struct snd_sof_dev',
> as shown below [1]. We probably want to keep this relatively opaque, this is
> a context that doesn't need to be exposed to the SoundWire code.

Right, that's why I proposed struct device and not struct snd_sof_dev, to not 
make it SOF-specific, then sdev could be obtained from dev_get_drvdata(). But 
yes, that's unrelated to this series.

Thanks
Guennadi

Re: [alsa-devel] [RFC PATCH 0/5] ASoC: SOF: Intel: SoundWire initial integration

[Guennadi Liakhovetski]

Hi Pierre,

In patch 4/5 I forgot to mention superfluous braces around dev_err() 
in sdw_config_stream() and sdw_free_stream(). Otherwise for the series:

Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Thanks
Guennadi

On Wed, Aug 21, 2019 at 03:17:15PM -0500, Pierre-Louis Bossart wrote:
> This RFC is the companion of the other RFC 'soundwire: intel: simplify
> DAI/PDI handling​'. Our purpose at this point is to gather feedback on
> the interfaces between the Intel SOF parts and the SoundWire code.
> 
> The suggested solution is a simple init/release inserted at
> probe/remove and resume/suspend, as well as two callbacks for the SOF
> driver to generate IPC configurations with the firmware. That level of
> separation completely hides the details of the SoundWire DAIs and will
> allow for 'transparent' multi-cpu DAI support, which will be handled
> in the machine driver and the soundwire DAIs.
> 
> This solution was tested on IceLake and CometLake, and captures the
> feedback from SOF contributors on an initial integration that was
> deemed too complicated (and rightly so).
> 
> Pierre-Louis Bossart (5):
>   ASoC: SOF: IPC: dai-intel: move ALH declarations in header file
>   ASoC: SOF: Intel: hda: add helper to initialize SoundWire IP
>   ASoC: SOF: Intel: hda: add SoundWire IP support
>   ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks
>   ASoC: SOF: Intel: add support for SoundWire suspend/resume
> 
>  include/sound/sof/dai-intel.h |  18 ++--
>  sound/soc/sof/intel/hda-dsp.c |  11 +++
>  sound/soc/sof/intel/hda.c     | 157 ++++++++++++++++++++++++++++++++++
>  sound/soc/sof/intel/hda.h     |  11 +++
>  4 files changed, 188 insertions(+), 9 deletions(-)
> 
> 
> base-commit: 3b3aaa017e8072b1bfddda92be296b3463d870be
> -- 
> 2.20.1
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [RFC PATCH 0/5] ASoC: SOF: Intel: SoundWire initial integration

[Pierre-Louis Bossart]

> In patch 4/5 I forgot to mention superfluous braces around dev_err()
> in sdw_config_stream() and sdw_free_stream(). Otherwise for the series:

Will fix, thanks for spotting this.

> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Thanks, I appreciate the overnight review!

> 
> Thanks
> Guennadi
> 
> On Wed, Aug 21, 2019 at 03:17:15PM -0500, Pierre-Louis Bossart wrote:
>> This RFC is the companion of the other RFC 'soundwire: intel: simplify
>> DAI/PDI handling​'. Our purpose at this point is to gather feedback on
>> the interfaces between the Intel SOF parts and the SoundWire code.
>>
>> The suggested solution is a simple init/release inserted at
>> probe/remove and resume/suspend, as well as two callbacks for the SOF
>> driver to generate IPC configurations with the firmware. That level of
>> separation completely hides the details of the SoundWire DAIs and will
>> allow for 'transparent' multi-cpu DAI support, which will be handled
>> in the machine driver and the soundwire DAIs.
>>
>> This solution was tested on IceLake and CometLake, and captures the
>> feedback from SOF contributors on an initial integration that was
>> deemed too complicated (and rightly so).
>>
>> Pierre-Louis Bossart (5):
>>    ASoC: SOF: IPC: dai-intel: move ALH declarations in header file
>>    ASoC: SOF: Intel: hda: add helper to initialize SoundWire IP
>>    ASoC: SOF: Intel: hda: add SoundWire IP support
>>    ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks
>>    ASoC: SOF: Intel: add support for SoundWire suspend/resume
>>
>>   include/sound/sof/dai-intel.h |  18 ++--
>>   sound/soc/sof/intel/hda-dsp.c |  11 +++
>>   sound/soc/sof/intel/hda.c     | 157 ++++++++++++++++++++++++++++++++++
>>   sound/soc/sof/intel/hda.h     |  11 +++
>>   4 files changed, 188 insertions(+), 9 deletions(-)
>>
>>
>> base-commit: 3b3aaa017e8072b1bfddda92be296b3463d870be
>> -- 
>> 2.20.1
>>
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [RFC PATCH 3/5] ASoC: SOF: Intel: hda: add SoundWire IP support

[Vinod Koul]

On 21-08-19, 15:17, Pierre-Louis Bossart wrote:
> The Core0 needs to be powered before the SoundWire IP is initialized.
> 
> Call sdw_intel_init/exit and store the context. We only have one
> context, but depending on the hardware capabilities and BIOS settings
> may enable multiple SoundWire links.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/soc/sof/intel/hda.c | 40 +++++++++++++++++++++++++++++++++------
>  sound/soc/sof/intel/hda.h |  5 +++++
>  2 files changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
> index a968890d0754..e754058e3679 100644
> --- a/sound/soc/sof/intel/hda.c
> +++ b/sound/soc/sof/intel/hda.c
> @@ -57,6 +57,8 @@ static int hda_sdw_init(struct snd_sof_dev *sdev)
>  {
>  	acpi_handle handle;
>  	struct sdw_intel_res res;
> +	struct sof_intel_hda_dev *hdev;
> +	void *sdw;
>  
>  	handle = ACPI_HANDLE(sdev->dev);
>  
> @@ -66,23 +68,32 @@ static int hda_sdw_init(struct snd_sof_dev *sdev)
>  	res.irq = sdev->ipc_irq;
>  	res.parent = sdev->dev;
>  
> -	hda_sdw_int_enable(sdev, true);
> -
> -	sdev->sdw = sdw_intel_init(handle, &res);
> -	if (!sdev->sdw) {
> +	sdw = sdw_intel_init(handle, &res);

should this be called for platforms without sdw, I was hoping that some
checks would be performed.. For example how would skl deal with this?

> +	if (!sdw) {
>  		dev_err(sdev->dev, "SDW Init failed\n");
>  		return -EIO;
>  	}
>  
> +	hda_sdw_int_enable(sdev, true);
> +
> +	/* save context */
> +	hdev = sdev->pdata->hw_pdata;
> +	hdev->sdw = sdw;
> +
>  	return 0;
>  }
>  
>  static int hda_sdw_exit(struct snd_sof_dev *sdev)
>  {
> +	struct sof_intel_hda_dev *hdev;
> +
> +	hdev = sdev->pdata->hw_pdata;
> +
>  	hda_sdw_int_enable(sdev, false);
>  
> -	if (sdev->sdw)
> -		sdw_intel_exit(sdev->sdw);

this looks suspect, you are adding sdw calls here so how is this getting
removed? Did I miss something...

> +	if (hdev->sdw)
> +		sdw_intel_exit(hdev->sdw);
> +	hdev->sdw = NULL;
>  
>  	return 0;
>  }
> @@ -713,6 +724,21 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
>  	/* set default mailbox offset for FW ready message */
>  	sdev->dsp_box.offset = HDA_DSP_MBOX_UPLINK_OFFSET;
>  
> +	/* need to power-up core before setting-up capabilities */
> +	ret = hda_dsp_core_power_up(sdev, HDA_DSP_CORE_MASK(0));
> +	if (ret < 0) {
> +		dev_err(sdev->dev, "error: could not power-up DSP subsystem\n");
> +		goto free_ipc_irq;
> +	}
> +
> +	/* initialize SoundWire capabilities */
> +	ret = hda_sdw_init(sdev);
> +	if (ret < 0) {
> +		dev_err(sdev->dev, "error: SoundWire get caps error\n");
> +		hda_dsp_core_power_down(sdev, HDA_DSP_CORE_MASK(0));
> +		goto free_ipc_irq;
> +	}
> +
>  	return 0;
>  
>  free_ipc_irq:
> @@ -744,6 +770,8 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
>  	snd_hdac_ext_bus_device_remove(bus);
>  #endif
>  
> +	hda_sdw_exit(sdev);
> +
>  	if (!IS_ERR_OR_NULL(hda->dmic_dev))
>  		platform_device_unregister(hda->dmic_dev);
>  
> diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
> index c8f93317aeb4..48e09b7daf0a 100644
> --- a/sound/soc/sof/intel/hda.h
> +++ b/sound/soc/sof/intel/hda.h
> @@ -399,6 +399,11 @@ struct sof_intel_hda_dev {
>  
>  	/* DMIC device */
>  	struct platform_device *dmic_dev;
> +
> +#if IS_ENABLED(CONFIG_SOUNDWIRE_INTEL)

is this really required, context is a void pointer

> +	/* sdw context */
> +	void *sdw;

> +#endif
>  };
>  
>  static inline struct hdac_bus *sof_to_bus(struct snd_sof_dev *s)
> -- 
> 2.20.1

-- 
~Vinod

Re: [RFC PATCH 4/5] ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks

[Vinod Koul]

On 22-08-19, 08:53, Pierre-Louis Bossart wrote:
> Thanks for the review Guennadi
> 
> > > +static int sdw_config_stream(void *arg, void *s, void *dai,
> > > +			     void *params, int link_id, int alh_stream_id)
> > 
> > I realise, that these function prototypes aren't being introduced by these
> > patches, but just wondering whether such overly generic prototype is really
> > a good idea here, whether some of those "void *" pointers could be given
> > real types. The first one could be "struct device *" etc.
> 
> In this case the 'arg' parameter is actually a private 'struct snd_sof_dev',
> as shown below [1]. We probably want to keep this relatively opaque, this is
> a context that doesn't need to be exposed to the SoundWire code.

This does look bit ugly.

> The dai and params are indeed cases where we could use stronger types, they
> are snd_soc_dai and hw_params respectively. I don't recall why the existing
> code is this way, Vinod and Sanyog may have the history of this.

Yes we wanted to decouple the sdw and audio bits that is the reason why
none of the audio types are used here, but I think it should be revisited
and perhaps made as:

sdw_config_stream(struct device *sdw, struct sdw_callback_ctx *ctx)

where the callback context contains all the other args. That would make
it look lot neater too and of course use real structs if possible

-- 
~Vinod

Re: [alsa-devel] [RFC PATCH 3/5] ASoC: SOF: Intel: hda: add SoundWire IP support

[Pierre-Louis Bossart]

On 9/4/19 2:21 AM, Vinod Koul wrote:
> On 21-08-19, 15:17, Pierre-Louis Bossart wrote:
>> The Core0 needs to be powered before the SoundWire IP is initialized.
>>
>> Call sdw_intel_init/exit and store the context. We only have one
>> context, but depending on the hardware capabilities and BIOS settings
>> may enable multiple SoundWire links.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   sound/soc/sof/intel/hda.c | 40 +++++++++++++++++++++++++++++++++------
>>   sound/soc/sof/intel/hda.h |  5 +++++
>>   2 files changed, 39 insertions(+), 6 deletions(-)
>>
>> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
>> index a968890d0754..e754058e3679 100644
>> --- a/sound/soc/sof/intel/hda.c
>> +++ b/sound/soc/sof/intel/hda.c
>> @@ -57,6 +57,8 @@ static int hda_sdw_init(struct snd_sof_dev *sdev)
>>   {
>>   	acpi_handle handle;
>>   	struct sdw_intel_res res;
>> +	struct sof_intel_hda_dev *hdev;
>> +	void *sdw;
>>   
>>   	handle = ACPI_HANDLE(sdev->dev);
>>   
>> @@ -66,23 +68,32 @@ static int hda_sdw_init(struct snd_sof_dev *sdev)
>>   	res.irq = sdev->ipc_irq;
>>   	res.parent = sdev->dev;
>>   
>> -	hda_sdw_int_enable(sdev, true);
>> -
>> -	sdev->sdw = sdw_intel_init(handle, &res);
>> -	if (!sdev->sdw) {
>> +	sdw = sdw_intel_init(handle, &res);
> 
> should this be called for platforms without sdw, I was hoping that some
> checks would be performed.. For example how would skl deal with this?

Good point. For now we rely on CONFIG_SOUNDWIRE_INTEL to use a fallback, 
but if the kernel defines this config and we run on an older platform 
the only safety would be the hardware capabilities and BIOS 
dependencies, I need to test if it works.
Thanks for the feedback.

> 
>> +	if (!sdw) {
>>   		dev_err(sdev->dev, "SDW Init failed\n");
>>   		return -EIO;
>>   	}
>>   
>> +	hda_sdw_int_enable(sdev, true);
>> +
>> +	/* save context */
>> +	hdev = sdev->pdata->hw_pdata;
>> +	hdev->sdw = sdw;
>> +
>>   	return 0;
>>   }
>>   
>>   static int hda_sdw_exit(struct snd_sof_dev *sdev)
>>   {
>> +	struct sof_intel_hda_dev *hdev;
>> +
>> +	hdev = sdev->pdata->hw_pdata;
>> +
>>   	hda_sdw_int_enable(sdev, false);
>>   
>> -	if (sdev->sdw)
>> -		sdw_intel_exit(sdev->sdw);
> 
> this looks suspect, you are adding sdw calls here so how is this getting
> removed? Did I miss something...

That must be a squash/tick-tock error, we moved the 'sdw' field from the 
top-level 'sdev' structure to an intel-specific one. In the latest code 
I have a single patch to add the helper and all dependencies in one shot.

> 
>> +	if (hdev->sdw)
>> +		sdw_intel_exit(hdev->sdw);
>> +	hdev->sdw = NULL;
>>   
>>   	return 0;
>>   }
>> @@ -713,6 +724,21 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
>>   	/* set default mailbox offset for FW ready message */
>>   	sdev->dsp_box.offset = HDA_DSP_MBOX_UPLINK_OFFSET;
>>   
>> +	/* need to power-up core before setting-up capabilities */
>> +	ret = hda_dsp_core_power_up(sdev, HDA_DSP_CORE_MASK(0));
>> +	if (ret < 0) {
>> +		dev_err(sdev->dev, "error: could not power-up DSP subsystem\n");
>> +		goto free_ipc_irq;
>> +	}
>> +
>> +	/* initialize SoundWire capabilities */
>> +	ret = hda_sdw_init(sdev);
>> +	if (ret < 0) {
>> +		dev_err(sdev->dev, "error: SoundWire get caps error\n");
>> +		hda_dsp_core_power_down(sdev, HDA_DSP_CORE_MASK(0));
>> +		goto free_ipc_irq;
>> +	}
>> +
>>   	return 0;
>>   
>>   free_ipc_irq:
>> @@ -744,6 +770,8 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
>>   	snd_hdac_ext_bus_device_remove(bus);
>>   #endif
>>   
>> +	hda_sdw_exit(sdev);
>> +
>>   	if (!IS_ERR_OR_NULL(hda->dmic_dev))
>>   		platform_device_unregister(hda->dmic_dev);
>>   
>> diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
>> index c8f93317aeb4..48e09b7daf0a 100644
>> --- a/sound/soc/sof/intel/hda.h
>> +++ b/sound/soc/sof/intel/hda.h
>> @@ -399,6 +399,11 @@ struct sof_intel_hda_dev {
>>   
>>   	/* DMIC device */
>>   	struct platform_device *dmic_dev;
>> +
>> +#if IS_ENABLED(CONFIG_SOUNDWIRE_INTEL)
> 
> is this really required, context is a void pointer
> 
>> +	/* sdw context */
>> +	void *sdw;
> 
>> +#endif
>>   };
>>   
>>   static inline struct hdac_bus *sof_to_bus(struct snd_sof_dev *s)
>> -- 
>> 2.20.1
> 

Re: [alsa-devel] [RFC PATCH 4/5] ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks

[Pierre-Louis Bossart]

On 9/4/19 2:35 AM, Vinod Koul wrote:
> On 22-08-19, 08:53, Pierre-Louis Bossart wrote:
>> Thanks for the review Guennadi
>>
>>>> +static int sdw_config_stream(void *arg, void *s, void *dai,
>>>> +			     void *params, int link_id, int alh_stream_id)
>>>
>>> I realise, that these function prototypes aren't being introduced by these
>>> patches, but just wondering whether such overly generic prototype is really
>>> a good idea here, whether some of those "void *" pointers could be given
>>> real types. The first one could be "struct device *" etc.
>>
>> In this case the 'arg' parameter is actually a private 'struct snd_sof_dev',
>> as shown below [1]. We probably want to keep this relatively opaque, this is
>> a context that doesn't need to be exposed to the SoundWire code.
> 
> This does look bit ugly.
> 
>> The dai and params are indeed cases where we could use stronger types, they
>> are snd_soc_dai and hw_params respectively. I don't recall why the existing
>> code is this way, Vinod and Sanyog may have the history of this.
> 
> Yes we wanted to decouple the sdw and audio bits that is the reason why
> none of the audio types are used here, but I think it should be revisited
> and perhaps made as:
> 
> sdw_config_stream(struct device *sdw, struct sdw_callback_ctx *ctx)
> 
> where the callback context contains all the other args. That would make
> it look lot neater too and of course use real structs if possible

the suggested sdw_callbback_ctx is really intel-specific at the moment, 
e.g. the notion of link_id and alh_stream_id are due to the hardware, 
it's not generic at all. And in the latest code we also pass the dai->id.

Re: [alsa-devel] [RFC PATCH 3/5] ASoC: SOF: Intel: hda: add SoundWire IP support

[Vinod Koul]

On 04-09-19, 08:25, Pierre-Louis Bossart wrote:
> On 9/4/19 2:21 AM, Vinod Koul wrote:
> > On 21-08-19, 15:17, Pierre-Louis Bossart wrote:
> > > The Core0 needs to be powered before the SoundWire IP is initialized.
> > > 
> > > Call sdw_intel_init/exit and store the context. We only have one
> > > context, but depending on the hardware capabilities and BIOS settings
> > > may enable multiple SoundWire links.
> > > 
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > ---
> > >   sound/soc/sof/intel/hda.c | 40 +++++++++++++++++++++++++++++++++------
> > >   sound/soc/sof/intel/hda.h |  5 +++++
> > >   2 files changed, 39 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
> > > index a968890d0754..e754058e3679 100644
> > > --- a/sound/soc/sof/intel/hda.c
> > > +++ b/sound/soc/sof/intel/hda.c
> > > @@ -57,6 +57,8 @@ static int hda_sdw_init(struct snd_sof_dev *sdev)
> > >   {
> > >   	acpi_handle handle;
> > >   	struct sdw_intel_res res;
> > > +	struct sof_intel_hda_dev *hdev;
> > > +	void *sdw;
> > >   	handle = ACPI_HANDLE(sdev->dev);
> > > @@ -66,23 +68,32 @@ static int hda_sdw_init(struct snd_sof_dev *sdev)
> > >   	res.irq = sdev->ipc_irq;
> > >   	res.parent = sdev->dev;
> > > -	hda_sdw_int_enable(sdev, true);
> > > -
> > > -	sdev->sdw = sdw_intel_init(handle, &res);
> > > -	if (!sdev->sdw) {
> > > +	sdw = sdw_intel_init(handle, &res);
> > 
> > should this be called for platforms without sdw, I was hoping that some
> > checks would be performed.. For example how would skl deal with this?
> 
> Good point. For now we rely on CONFIG_SOUNDWIRE_INTEL to use a fallback, but
> if the kernel defines this config and we run on an older platform the only
> safety would be the hardware capabilities and BIOS dependencies, I need to
> test if it works.

Yes I am not sure given the experience with BIOS relying on that is a
great idea ! But if that works, that would be better.


> > > diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
> > > index c8f93317aeb4..48e09b7daf0a 100644
> > > --- a/sound/soc/sof/intel/hda.h
> > > +++ b/sound/soc/sof/intel/hda.h
> > > @@ -399,6 +399,11 @@ struct sof_intel_hda_dev {
> > >   	/* DMIC device */
> > >   	struct platform_device *dmic_dev;
> > > +
> > > +#if IS_ENABLED(CONFIG_SOUNDWIRE_INTEL)
> > 
> > is this really required, context is a void pointer

??

> > > +	/* sdw context */
> > > +	void *sdw;
> > 
> > > +#endif

-- 
~Vinod

Re: [alsa-devel] [RFC PATCH 4/5] ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks

[Vinod Koul]

On 04-09-19, 08:31, Pierre-Louis Bossart wrote:
> On 9/4/19 2:35 AM, Vinod Koul wrote:
> > On 22-08-19, 08:53, Pierre-Louis Bossart wrote:
> > > Thanks for the review Guennadi
> > > 
> > > > > +static int sdw_config_stream(void *arg, void *s, void *dai,
> > > > > +			     void *params, int link_id, int alh_stream_id)
> > > > 
> > > > I realise, that these function prototypes aren't being introduced by these
> > > > patches, but just wondering whether such overly generic prototype is really
> > > > a good idea here, whether some of those "void *" pointers could be given
> > > > real types. The first one could be "struct device *" etc.
> > > 
> > > In this case the 'arg' parameter is actually a private 'struct snd_sof_dev',
> > > as shown below [1]. We probably want to keep this relatively opaque, this is
> > > a context that doesn't need to be exposed to the SoundWire code.
> > 
> > This does look bit ugly.
> > 
> > > The dai and params are indeed cases where we could use stronger types, they
> > > are snd_soc_dai and hw_params respectively. I don't recall why the existing
> > > code is this way, Vinod and Sanyog may have the history of this.
> > 
> > Yes we wanted to decouple the sdw and audio bits that is the reason why
> > none of the audio types are used here, but I think it should be revisited
> > and perhaps made as:
> > 
> > sdw_config_stream(struct device *sdw, struct sdw_callback_ctx *ctx)
> > 
> > where the callback context contains all the other args. That would make
> > it look lot neater too and of course use real structs if possible
> 
> the suggested sdw_callbback_ctx is really intel-specific at the moment, e.g.
> the notion of link_id and alh_stream_id are due to the hardware, it's not
> generic at all. And in the latest code we also pass the dai->id.

s/sdw_callback_ctx/intel_sdw_callback_ctx

Yes this code is intel specific and this would be intel specific too

-- 
~Vinod

Re: [alsa-devel] [RFC PATCH 3/5] ASoC: SOF: Intel: hda: add SoundWire IP support

[Pierre-Louis Bossart]

On 9/4/19 11:51 AM, Vinod Koul wrote:
> On 04-09-19, 08:25, Pierre-Louis Bossart wrote:
>> On 9/4/19 2:21 AM, Vinod Koul wrote:
>>> On 21-08-19, 15:17, Pierre-Louis Bossart wrote:
>>>> The Core0 needs to be powered before the SoundWire IP is initialized.
>>>>
>>>> Call sdw_intel_init/exit and store the context. We only have one
>>>> context, but depending on the hardware capabilities and BIOS settings
>>>> may enable multiple SoundWire links.
>>>>
>>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>> ---
>>>>    sound/soc/sof/intel/hda.c | 40 +++++++++++++++++++++++++++++++++------
>>>>    sound/soc/sof/intel/hda.h |  5 +++++
>>>>    2 files changed, 39 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
>>>> index a968890d0754..e754058e3679 100644
>>>> --- a/sound/soc/sof/intel/hda.c
>>>> +++ b/sound/soc/sof/intel/hda.c
>>>> @@ -57,6 +57,8 @@ static int hda_sdw_init(struct snd_sof_dev *sdev)
>>>>    {
>>>>    	acpi_handle handle;
>>>>    	struct sdw_intel_res res;
>>>> +	struct sof_intel_hda_dev *hdev;
>>>> +	void *sdw;
>>>>    	handle = ACPI_HANDLE(sdev->dev);
>>>> @@ -66,23 +68,32 @@ static int hda_sdw_init(struct snd_sof_dev *sdev)
>>>>    	res.irq = sdev->ipc_irq;
>>>>    	res.parent = sdev->dev;
>>>> -	hda_sdw_int_enable(sdev, true);
>>>> -
>>>> -	sdev->sdw = sdw_intel_init(handle, &res);
>>>> -	if (!sdev->sdw) {
>>>> +	sdw = sdw_intel_init(handle, &res);
>>>
>>> should this be called for platforms without sdw, I was hoping that some
>>> checks would be performed.. For example how would skl deal with this?
>>
>> Good point. For now we rely on CONFIG_SOUNDWIRE_INTEL to use a fallback, but
>> if the kernel defines this config and we run on an older platform the only
>> safety would be the hardware capabilities and BIOS dependencies, I need to
>> test if it works.
> 
> Yes I am not sure given the experience with BIOS relying on that is a
> great idea ! But if that works, that would be better.

I don't think it's going to be that bad, first we need to find the ACPI 
description for the controller, then see which links are active, and 
even with all links disabled nothing bad will happen.

What I am more worried about are inconsistencies where e.g we have both 
I2C/I2S and SoundWire devices exposed at the same time. The BIOS deals 
with this with dynamic changes depending on user changes, and we are 
likely to see reports of problems due to BIOS configuration selection, 
not the BIOS itself.

Re: [alsa-devel] [RFC PATCH 4/5] ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks

[Pierre-Louis Bossart]

On 9/4/19 11:55 AM, Vinod Koul wrote:
> On 04-09-19, 08:31, Pierre-Louis Bossart wrote:
>> On 9/4/19 2:35 AM, Vinod Koul wrote:
>>> On 22-08-19, 08:53, Pierre-Louis Bossart wrote:
>>>> Thanks for the review Guennadi
>>>>
>>>>>> +static int sdw_config_stream(void *arg, void *s, void *dai,
>>>>>> +			     void *params, int link_id, int alh_stream_id)
>>>>>
>>>>> I realise, that these function prototypes aren't being introduced by these
>>>>> patches, but just wondering whether such overly generic prototype is really
>>>>> a good idea here, whether some of those "void *" pointers could be given
>>>>> real types. The first one could be "struct device *" etc.
>>>>
>>>> In this case the 'arg' parameter is actually a private 'struct snd_sof_dev',
>>>> as shown below [1]. We probably want to keep this relatively opaque, this is
>>>> a context that doesn't need to be exposed to the SoundWire code.
>>>
>>> This does look bit ugly.
>>>
>>>> The dai and params are indeed cases where we could use stronger types, they
>>>> are snd_soc_dai and hw_params respectively. I don't recall why the existing
>>>> code is this way, Vinod and Sanyog may have the history of this.
>>>
>>> Yes we wanted to decouple the sdw and audio bits that is the reason why
>>> none of the audio types are used here, but I think it should be revisited
>>> and perhaps made as:
>>>
>>> sdw_config_stream(struct device *sdw, struct sdw_callback_ctx *ctx)
>>>
>>> where the callback context contains all the other args. That would make
>>> it look lot neater too and of course use real structs if possible
>>
>> the suggested sdw_callbback_ctx is really intel-specific at the moment, e.g.
>> the notion of link_id and alh_stream_id are due to the hardware, it's not
>> generic at all. And in the latest code we also pass the dai->id.
> 
> s/sdw_callback_ctx/intel_sdw_callback_ctx
> 
> Yes this code is intel specific and this would be intel specific too

ok, I'll fold all the fields in a structure then. That's a nice cleanup, 
thanks Guennadi and Vinod for the reviews.