[RFC] [DO NOT MERGE] Integration of Soundwire and SOF #1130

plbossart · 2019-08-13T23:01:08Z

The integration of SoundWire support is quite hacky (the spell checker suggests wacky which isn't far from the truth). @RanderWang suggested that this isn't quite right as well so let's try and use GitHub to collect feedback.

We will need bandwidth from the usual suspects @ranj063 @kv2019i @lyakh @juimonen to help us make progress.

The goal is not to merge this in topic/sof-dev for now, this is to be applied on top of all the queued SoundWire patches (kept in an integration branch for reference).

For SoundWire/ALH, we have BE-specific inits to run, so don't return and give a chance to the hardware driver to run. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

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>

The Core0 needs to be powered before the SoundWire IP is initialized. The storage of the SoundWire information the snd_sof_dev structure will have to be revisited. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Once we get the ALH stream ID, we need to send an IPC to configure the firmware. Open: how do we get the DAI index from the dai? Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Implement all required back-end operations for SoundWire FIXMEs: 1. the use of indices to detect SoundWire is a bad design to be revisited 2. this patch was squashed for simplicity but may have to be better split to simplify reviews. 3.we will have to rework all the stream handling since we have 4 structures called 'blah_stream' Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

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>

RanderWang · 2019-08-14T04:46:26Z

@plbossart & all, how about my solution for sdw in SOF based on this PR? plbossart#3

we don't need to make such strange code.

	if (rtd->cpu_dai->id < SDW_DAI_ID_RANGE_START ||
	    rtd->cpu_dai->id > SDW_DAI_ID_RANGE_END)
		return 0;

	hda_stream = container_of(hstream,
				  struct sof_intel_hda_stream,
				  hda_stream.hstream);

And some functions, such as prepare, trigger, free, should be done in BE sdw dai link driver, just like hda dai link driver. In this PR, the sdw setting is recourse to FE stream, do it like the following code

        static int sof_pcm_trigger(struct snd_pcm_substream *substream, int cmd) {
	/* skip front-end specific inits for BE */
	if (rtd->dai_link->no_pcm) {
		snd_sof_pcm_platform_trigger(sdev, substream, cmd);
        }
       ...
 }

And found it will affect HDMI, although we can add more code to check whether it is HDMI or not.

plbossart · 2019-08-14T16:41:02Z

@plbossart & all, how about my solution for sdw in SOF based on this PR? plbossart#3

Your solution was posted after this PR, so please give people time to review. I provided comments already.
Also your solution only impacts the PCM parts but the other 4 patches in this PR need to be reviewed as well.

And some functions, such as prepare, trigger, free, should be done in BE sdw dai link driver, just like hda dai link driver. In this PR, the sdw setting is recourse to FE stream, do it like the following code
        static int sof_pcm_trigger(struct snd_pcm_substream *substream, int cmd) {
	/* skip front-end specific inits for BE */
	if (rtd->dai_link->no_pcm) {
		snd_sof_pcm_platform_trigger(sdev, substream, cmd);
        }
       ...
 }
And found it will affect HDMI, although we can add more code to check whether it is HDMI or not.

I cannot figure out what you were trying to say here. Just boiler-plate answer is that I don't want any ties between SoundWire and front-ends, nor any warts related to HDMI.

ranj063 · 2019-08-14T23:13:33Z

@plbossart & all, how about my solution for sdw in SOF based on this PR? plbossart#3

@plbossart I do like the direction in Rander's PR.

With the move to add the trigger and prepare() callbacks to the dai_ops, the SOF code looks much cleaner but...
Should sdw_stream_setup() be in intel_startup() instead of intel_hw_params()? That would be in line with what you have in this PR
I dont understand why the sdw_stream_add_slave() was moved from the rt700's hw_params to prepare(). It seems incorrect to me
And there are some other simple fixes needed that I have commented on.

bardliao

Thanks @plbossart for posting it. Some minor comments from my side.

bardliao · 2019-08-15T11:21:38Z

sound/soc/sof/intel/hda-pcm.c

+int sdw_pcm_prepare(struct snd_sof_dev *sdev,
+		    struct snd_pcm_substream *substream)
+{
+	//struct snd_pcm_runtime *runtime = substream->runtime;


remove it :)

bardliao · 2019-08-15T11:30:20Z

sound/soc/sof/intel/hda-pcm.c

 }

 int hda_dsp_pcm_close(struct snd_sof_dev *sdev,
 		      struct snd_pcm_substream *substream)
 {
-	struct hdac_stream *hstream = substream->runtime->private_data;
+	struct hdac_stream *hstream = NULL;


move below hstream = runtime->private_data; here?

bardliao · 2019-08-15T11:30:27Z

sound/soc/sof/intel/hda-pcm.c

 	ret = hda_dsp_stream_put(sdev, direction, hstream->stream_tag);

 	if (ret) {
 		dev_dbg(sdev->dev, "stream %s not opened!\n", substream->name);
 		return -ENODEV;
 	}

+	goto free;
+
+be:


Probably check if it is soundwire here. e.g.

if (rtd->cpu_dai->id < SDW_DAI_ID_RANGE_START || rtd->cpu_dai->id > SDW_DAI_ID_RANGE_END) return 0;

bardliao · 2019-08-15T11:31:32Z

sound/soc/sof/intel/hda-stream.c

@@ -500,6 +512,35 @@ int hda_dsp_stream_hw_free(struct snd_sof_dev *sdev,
 	spin_unlock_irq(&bus->reg_lock);

 	return 0;
+be:
+


Probably check if it is soundwire here, too

bardliao · 2019-08-15T11:34:43Z

sound/soc/sof/pcm.c

-	if (rtd->dai_link->no_pcm)
+	/* skip front-end specific inits for BE */
+	if (rtd->dai_link->no_pcm) {
+		snd_sof_pcm_platform_prepare(sdev, substream);


return snd_sof_pcm_platform_prepare(sdev, substream);?

bardliao · 2019-08-15T11:35:52Z

sound/soc/sof/pcm.c

-	if (rtd->dai_link->no_pcm)
+	/* skip front-end specific inits for BE */
+	if (rtd->dai_link->no_pcm) {
+		snd_sof_pcm_platform_trigger(sdev, substream, cmd);


return snd_sof_pcm_platform_trigger(sdev, substream, cmd);?

bardliao · 2019-08-15T11:56:34Z

sound/soc/sof/intel/hda-dsp.c

+		return ret;
+	}
+
+	hda_sdw_int_enable(sdev, true);


Is this necessary? SdW interrupt should work in suspended, right?

no. If the IP is power gated then it can't generate interrupts.
This is different from the bus wake, which is something that we need to handle with the clock-stop mode, but that's for future work.

RanderWang · 2019-08-16T09:05:01Z

@plbossart & all, how about my solution for sdw in SOF based on this PR? plbossart#3

@plbossart I do like the direction in Rander's PR.

With the move to add the trigger and prepare() callbacks to the dai_ops, the SOF code looks much cleaner but...

Should sdw_stream_setup() be in intel_startup() instead of intel_hw_params()? That would be in line with what you have in this PR

yes, I also want to do it.

I dont understand why the sdw_stream_add_slave() was moved from the rt700's hw_params to prepare(). It seems incorrect to me

because hw_params in codecs is invoke before dai hw_params, so if sdw data is set in dai hw_params,
it can't be gotten in codec hw_param. But with your advice to set sdw data in startup(), this prepare is no need.

And there are some other simple fixes needed that I have commented on.

plbossart · 2019-08-19T21:09:34Z

closed and replaced by #1139

plbossart added 6 commits August 13, 2019 17:53

ASoC: SOF: pcm: initialize back-end if required

74be523

For SoundWire/ALH, we have BE-specific inits to run, so don't return and give a chance to the hardware driver to run. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: SOF: Intel: hda: add SoundWire IP support

7b6c5dc

The Core0 needs to be powered before the SoundWire IP is initialized. The storage of the SoundWire information the snd_sof_dev structure will have to be revisited. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: SOF: Intel: hda: add SoundWire stream config callback

45bdf46

Once we get the ALH stream ID, we need to send an IPC to configure the firmware. Open: how do we get the DAI index from the dai? Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: SOF: Intel: add support for SoundWire suspend/resume

b6ce989

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>

plbossart requested review from juimonen, keyonjie, lgirdwood, lyakh and ranj063 as code owners August 13, 2019 23:01

plbossart requested review from kv2019i, xiulipan, bardliao, libinyang and RanderWang August 13, 2019 23:01

bardliao reviewed Aug 15, 2019

View reviewed changes

plbossart closed this Aug 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] [DO NOT MERGE] Integration of Soundwire and SOF #1130

[RFC] [DO NOT MERGE] Integration of Soundwire and SOF #1130

plbossart commented Aug 13, 2019

RanderWang commented Aug 14, 2019 •

edited

Loading

plbossart commented Aug 14, 2019

ranj063 commented Aug 14, 2019

bardliao left a comment

bardliao Aug 15, 2019

bardliao Aug 15, 2019

bardliao Aug 15, 2019

bardliao Aug 15, 2019

bardliao Aug 15, 2019

bardliao Aug 15, 2019

bardliao Aug 15, 2019

plbossart Aug 15, 2019

RanderWang commented Aug 16, 2019

plbossart commented Aug 19, 2019

[RFC] [DO NOT MERGE] Integration of Soundwire and SOF #1130

[RFC] [DO NOT MERGE] Integration of Soundwire and SOF #1130

Conversation

plbossart commented Aug 13, 2019

RanderWang commented Aug 14, 2019 • edited Loading

plbossart commented Aug 14, 2019

ranj063 commented Aug 14, 2019

bardliao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanderWang commented Aug 16, 2019

plbossart commented Aug 19, 2019

RanderWang commented Aug 14, 2019 •

edited

Loading