[RFC,3/5] ASoC: SOF: Intel: hda: add SoundWire IP support
diff mbox series

Message ID 20190821201720.17768-4-pierre-louis.bossart@linux.intel.com
State New
Headers show
Series
  • ASoC: SOF: Intel: SoundWire initial integration
Related show

Commit Message

Pierre-Louis Bossart Aug. 21, 2019, 8:17 p.m. UTC
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(-)

Comments

Vinod Koul Sept. 4, 2019, 7:21 a.m. UTC | #1
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
Pierre-Louis Bossart Sept. 4, 2019, 1:25 p.m. UTC | #2
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
>
Vinod Koul Sept. 4, 2019, 4:51 p.m. UTC | #3
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
Pierre-Louis Bossart Sept. 4, 2019, 5:47 p.m. UTC | #4
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.

Patch
diff mbox series

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)