[PATCH] ASoC: Intel: Do not load legacy SST driver on BYT when SND_SOC_SOF_BAYTRAIL is enabled

[PATCH] ASoC: Intel: Do not load legacy SST driver on BYT when SND_SOC_SOF_BAYTRAIL is enabled

[Hans de Goede]

The legacy 80860F28 / sst_acpi_baytrail_desc match in sst_acpi_match
is already conditional on the the newer SND_SST_IPC_ACPI driver not
being enabled.

But now that we have an even newer driver in the form of SOF support
for BYT devices, we also need to take this into account, so we also
must not include the sst_acpi_baytrail_desc match when
SND_SOC_SOF_BAYTRAIL is enabled.

This fixes snd-soc-sst-acpi binding to the 80860F28 platform device,
blocking snd-sof-acpi from binding, which breaks audio support.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/intel/common/sst-acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/intel/common/sst-acpi.c b/sound/soc/intel/common/sst-acpi.c
index 5854868650b9..b4b2e1e0e845 100644
--- a/sound/soc/intel/common/sst-acpi.c
+++ b/sound/soc/intel/common/sst-acpi.c
@@ -198,7 +198,7 @@ static struct sst_acpi_desc sst_acpi_broadwell_desc = {
 	.dma_size = SST_LPT_DSP_DMA_SIZE,
 };
 
-#if !IS_ENABLED(CONFIG_SND_SST_IPC_ACPI)
+#if !IS_ENABLED(CONFIG_SND_SST_IPC_ACPI) && !IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
 static struct sst_acpi_desc sst_acpi_baytrail_desc = {
 	.drv_name = "baytrail-pcm-audio",
 	.machines = snd_soc_acpi_intel_baytrail_legacy_machines,
@@ -214,7 +214,7 @@ static struct sst_acpi_desc sst_acpi_baytrail_desc = {
 static const struct acpi_device_id sst_acpi_match[] = {
 	{ "INT33C8", (unsigned long)&sst_acpi_haswell_desc },
 	{ "INT3438", (unsigned long)&sst_acpi_broadwell_desc },
-#if !IS_ENABLED(CONFIG_SND_SST_IPC_ACPI)
+#if !IS_ENABLED(CONFIG_SND_SST_IPC_ACPI) && !IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
 	{ "80860F28", (unsigned long)&sst_acpi_baytrail_desc },
 #endif
 	{ }
-- 
2.28.0

RE: [PATCH] ASoC: Intel: Do not load legacy SST driver on BYT when SND_SOC_SOF_BAYTRAIL is enabled

[Rojewski, Cezary]

On 2020-10-11 11:53 AM, Hans de Goede wrote:
> The legacy 80860F28 / sst_acpi_baytrail_desc match in sst_acpi_match
> is already conditional on the the newer SND_SST_IPC_ACPI driver not
> being enabled.
> 
> But now that we have an even newer driver in the form of SOF support
> for BYT devices, we also need to take this into account, so we also
> must not include the sst_acpi_baytrail_desc match when
> SND_SOC_SOF_BAYTRAIL is enabled.
> 
> This fixes snd-soc-sst-acpi binding to the 80860F28 platform device,
> blocking snd-sof-acpi from binding, which breaks audio support.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Hello,

Series:
[PATCH v2 00/13] ASoC: Intel: Remove obsolete solutions and components
https://lore.kernel.org/alsa-devel/20201006064907.16277-1-cezary.rojewski@intel.com/

removes sst-acpi component along with many others so further changes to
said component will only cause conflicts -or- require commit reordering.
I'd advice against that.

Thanks,
Czarek

Re: [PATCH] ASoC: Intel: Do not load legacy SST driver on BYT when SND_SOC_SOF_BAYTRAIL is enabled

[Hans de Goede]

Hi,

On 10/12/20 9:24 AM, Rojewski, Cezary wrote:
> On 2020-10-11 11:53 AM, Hans de Goede wrote:
>> The legacy 80860F28 / sst_acpi_baytrail_desc match in sst_acpi_match
>> is already conditional on the the newer SND_SST_IPC_ACPI driver not
>> being enabled.
>>
>> But now that we have an even newer driver in the form of SOF support
>> for BYT devices, we also need to take this into account, so we also
>> must not include the sst_acpi_baytrail_desc match when
>> SND_SOC_SOF_BAYTRAIL is enabled.
>>
>> This fixes snd-soc-sst-acpi binding to the 80860F28 platform device,
>> blocking snd-sof-acpi from binding, which breaks audio support.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
> 
> Hello,
> 
> Series:
> [PATCH v2 00/13] ASoC: Intel: Remove obsolete solutions and components
> https://lore.kernel.org/alsa-devel/20201006064907.16277-1-cezary.rojewski@intel.com/
> 
> removes sst-acpi component along with many others so further changes to
> said component will only cause conflicts -or- require commit reordering.
> I'd advice against that.

As I already mentioned in the private-thread which Pierre-Louis started
with me, Jaroslav Kysela and Liam about this I would advice against applying
that series for now. First we need to put in more work to make sure that
the new drivers are actually ready.

Also I must say that I'm quite disappointed that since I, as the person
who more or less single handedly have made sure that audio works properly o
Bay Trail and Cherry Traul devices (*), has not been Cc-ed on that series,
that seems like a huge oversight.

Anyways I will reply in the thread of the series and ask Mark to revert
the entire series. Since IMHO the new drivers are clearly not ready yet.
Yesterday I ran my first set of tested and I immediately hit a DSP
hang doing just a few very basic tests.

Regards,

Hans



*) And kept it working properly despite other people breaking it with changes
like moving the userspace stuff to UCM2.

RE: [PATCH] ASoC: Intel: Do not load legacy SST driver on BYT when SND_SOC_SOF_BAYTRAIL is enabled

[Rojewski, Cezary]

On 2020-10-12 9:42 AM, Hans de Goede wrote:
> Hi,
> 
> On 10/12/20 9:24 AM, Rojewski, Cezary wrote:

...

>>
>> Hello,
>>
>> Series:
>> [PATCH v2 00/13] ASoC: Intel: Remove obsolete solutions and components
>> https://lore.kernel.org/alsa-devel/20201006064907.16277-1-cezary.rojewski@intel.com/ 
>>
>>
>> removes sst-acpi component along with many others so further changes to
>> said component will only cause conflicts -or- require commit reordering.
>> I'd advice against that.
> 
> As I already mentioned in the private-thread which Pierre-Louis started
> with me, Jaroslav Kysela and Liam about this I would advice against 
> applying
> that series for now. First we need to put in more work to make sure that
> the new drivers are actually ready.
> 
> Also I must say that I'm quite disappointed that since I, as the person
> who more or less single handedly have made sure that audio works properly o
> Bay Trail and Cherry Traul devices (*), has not been Cc-ed on that series,
> that seems like a huge oversight.
> 
> Anyways I will reply in the thread of the series and ask Mark to revert
> the entire series. Since IMHO the new drivers are clearly not ready yet.
> Yesterday I ran my first set of tested and I immediately hit a DSP
> hang doing just a few very basic tests.
> 
> Regards,
> 
> Hans
> 
> 
> 
> *) And kept it working properly despite other people breaking it with 
> changes
> like moving the userspace stuff to UCM2.
> 

Hello,

What's the name of the private-thread? Or perhaps I'm not even invited
there?

Please, elaborate "new drivers". /baytrail/ has been deprecated for
years with only two available boards (machine boards) to it - which are
somewhat duplicates of /atom/ -or- SOF equivalents (bytcr-xxxx). From
linux-kernel perspective, having 3x baytrail driver is simply bad.

Several teams, clients and groups have been asked on multiple occasions
about the usage of the /baytrail/ folder. Not once positive answer has
been given.

Thanks,
Czarek

Re: [PATCH] ASoC: Intel: Do not load legacy SST driver on BYT when SND_SOC_SOF_BAYTRAIL is enabled

[Hans de Goede]

Hi Czarek,

On 10/12/20 9:58 AM, Rojewski, Cezary wrote:
> On 2020-10-12 9:42 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 10/12/20 9:24 AM, Rojewski, Cezary wrote:
> 
> ...
> 
>>>
>>> Hello,
>>>
>>> Series:
>>> [PATCH v2 00/13] ASoC: Intel: Remove obsolete solutions and components
>>> https://lore.kernel.org/alsa-devel/20201006064907.16277-1-cezary.rojewski@intel.com/
>>>
>>>
>>> removes sst-acpi component along with many others so further changes to
>>> said component will only cause conflicts -or- require commit reordering.
>>> I'd advice against that.
>>
>> As I already mentioned in the private-thread which Pierre-Louis started
>> with me, Jaroslav Kysela and Liam about this I would advice against
>> applying
>> that series for now. First we need to put in more work to make sure that
>> the new drivers are actually ready.
>>
>> Also I must say that I'm quite disappointed that since I, as the person
>> who more or less single handedly have made sure that audio works properly o
>> Bay Trail and Cherry Traul devices (*), has not been Cc-ed on that series,
>> that seems like a huge oversight.
>>
>> Anyways I will reply in the thread of the series and ask Mark to revert
>> the entire series. Since IMHO the new drivers are clearly not ready yet.
>> Yesterday I ran my first set of tested and I immediately hit a DSP
>> hang doing just a few very basic tests.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>> *) And kept it working properly despite other people breaking it with
>> changes
>> like moving the userspace stuff to UCM2.
>>
> 
> Hello,
> 
> What's the name of the private-thread? Or perhaps I'm not even invited
> there?

You were not on the Cc when Pierre-Louis started the thread,
I'll try to remember to Cc you on further replies. I'll give a summary
at the end of this email, since this is probably useful info for
everyone reading along to have.

> Please, elaborate "new drivers". /baytrail/ has been deprecated for
> years with only two available boards (machine boards) to it - which are
> somewhat duplicates of /atom/ -or- SOF equivalents (bytcr-xxxx). From
> linux-kernel perspective, having 3x baytrail driver is simply bad.

Ah, sorry I think I jumped the gun a bit on becoming grumpy about this.

On second reading I see you are removing the old-old Bay Trail code,
while keeping the medium-old (CONFIG_SND_SST_IPC_ACPI) code around for
now, correct ?

Yes that should be fine. One request though in the future please Cc
me on (non trivial) changes impacting Bay and Cherry Trail devices.

> Several teams, clients and groups have been asked on multiple occasions
> about the usage of the /baytrail/ folder. Not once positive answer has
> been given.

Right, I don't know about other distros but in Fedora we have had
the use of the old sound/soc/intel/common/sst-acpi.c code disabled
for BYT/CHT for a while now.

Note Fedora does have the common/sst-acpi.c Broadwell / Haswell
bits enabled all the way up to kernel 5.9, but lets discuss that
in the thread where you remove the common/sst-acpi.c code.

Regards,

Hans


p.s. The promised summary:

Pierre-Louis contacted me about moving BYT/CHT devices to the SOF
driver so that the medium-old / CONFIG_SND_SST_IPC_ACPI drivers can
eventually also be removed. I agreed with that plan, but I was and
still am against doing it immediately as I want to first run a set
of tests to make sure the switch will go smoothly.

The Bay / Cherry Trail work I do is a personal-time side project, which
means mostly working on it in the weekend. As discussed in the off-list
discussion at a minimum I would like to run the following setups:

Realtek codecs:
BYT(CR) RT5640 SSP0 AIF1
BYT(CR) RT5640 SSP0 AIF2
BYT     RT5640 SSP2
CHT     RT5640 (HP pavilion X2 10-p002nd uses this weird combo)
CHT     RT5645
BYT(CR) RT5651
CHT     RT5651
BYT    RT5672

Other:
BYT(CR) ESS8316
CHT     ESS8316
CHT     NAU8824

Through the following test plan:

1. Test speakers
2. Test internal mic.
3. Plugin headset, test headphones
4. Test headset-mic
5. Stop all audio, suspend + resume, test speakers
6. suspend + resume while playing audio, audio should
    resume playing after resume.

This weekend I ran the test-plan on the first setup and
at step 3 (a couple of minutes into testing) I hit a DSP
hang (which I could not reproduce). Other then the hang the
testing went smooth. We will need to see if the hang was a
glitch or if I will hit it more often when I test the other
setups.

I have dmesg output from the hang, if someone is interested.