Some devices with intel-lpss based SPI controllers may have misconfigured
clock divider due to firmware bug. This would result in capped SPI speeds,
which leads to longer DSP firmware loading times.
This safety guards against possible hangs during wake-up by not
initializing the device if lpss was not patched/fixed UEFI was not
installed

Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
---
 sound/pci/hda/cs35l41_hda_property.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/cs35l41_hda_property.c b/sound/pci/hda/cs35l41_hda_property.c
index c9eb70290973..cb305b093311 100644
--- a/sound/pci/hda/cs35l41_hda_property.c
+++ b/sound/pci/hda/cs35l41_hda_property.c
@@ -210,6 +210,19 @@ static int generic_dsd_config(struct cs35l41_hda *cs35l41, struct device *physde
 
 	if (cfg->bus == SPI) {
 		cs35l41->index = id;
+		/*
+		 * Some devices with intel-lpss based SPI controllers may have misconfigured
+		 * clock divider due to firmware bug. This would result in capped SPI speeds,
+		 * which leads to longer DSP firmware loading times.
+		 * Avoid initializing device if lpss was not patched/fixed UEFI was not installed
+		 */
+		spi = to_spi_device(cs35l41->dev);
+		if (spi->max_speed_hz < CS35L41_SPI_MAX_FREQ/2) {
+			dev_err(cs35l41->dev,
+				"SPI's max_speed_hz is capped at %u Hz, will not continue to avoid hanging\n",
+				spi->max_speed_hz);
+			return -EINVAL;
+		}
 		/*
 		 * Manually set the Chip Select for the second amp <cs_gpio_index> in the node.
 		 * This is only supported for systems with 2 amps, since we cannot expand the
@@ -219,8 +232,6 @@ static int generic_dsd_config(struct cs35l41_hda *cs35l41, struct device *physde
 		 * first.
 		 */
 		if (cfg->cs_gpio_index >= 0) {
-			spi = to_spi_device(cs35l41->dev);
-
 			if (cfg->num_amps != 2) {
 				dev_warn(cs35l41->dev,
 					 "Cannot update SPI CS, Number of Amps (%d) != 2\n",
-- 
2.40.1

Hi,

On 20/12/2023 07:38, Aleksandrs Vinarskis wrote:
> Some devices with intel-lpss based SPI controllers may have misconfigured
> clock divider due to firmware bug. This would result in capped SPI speeds,
> which leads to longer DSP firmware loading times.
> This safety guards against possible hangs during wake-up by not
> initializing the device if lpss was not patched/fixed UEFI was not
> installed
>
> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> ---
>   sound/pci/hda/cs35l41_hda_property.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/sound/pci/hda/cs35l41_hda_property.c b/sound/pci/hda/cs35l41_hda_property.c
> index c9eb70290973..cb305b093311 100644
> --- a/sound/pci/hda/cs35l41_hda_property.c
> +++ b/sound/pci/hda/cs35l41_hda_property.c
> @@ -210,6 +210,19 @@ static int generic_dsd_config(struct cs35l41_hda *cs35l41, struct device *physde
>   
>   	if (cfg->bus == SPI) {
>   		cs35l41->index = id;
> +		/*
> +		 * Some devices with intel-lpss based SPI controllers may have misconfigured
> +		 * clock divider due to firmware bug. This would result in capped SPI speeds,
> +		 * which leads to longer DSP firmware loading times.
> +		 * Avoid initializing device if lpss was not patched/fixed UEFI was not installed
> +		 */
> +		spi = to_spi_device(cs35l41->dev);
> +		if (spi->max_speed_hz < CS35L41_SPI_MAX_FREQ/2) {
> +			dev_err(cs35l41->dev,
> +				"SPI's max_speed_hz is capped at %u Hz, will not continue to avoid hanging\n",
> +				spi->max_speed_hz);
> +			return -EINVAL;
> +		}

Not sure I agree with completely disabling the CS35L41 Speaker Driver if 
the SPI speed is low (for laptops without _DSD).
With a slow speed the driver does not hang - it just takes a long time 
(~80s per amp) to load the firmware.
Instead I would prefer that we instead disable the loading of the 
firmware in this case.
Without loading firmware, the volume is much lower, but at least you 
still have audio.
I have a patch to do that, which I was planning on pushing up 
(hopefully) today.

Thanks,

Stefan

>   		/*
>   		 * Manually set the Chip Select for the second amp <cs_gpio_index> in the node.
>   		 * This is only supported for systems with 2 amps, since we cannot expand the
> @@ -219,8 +232,6 @@ static int generic_dsd_config(struct cs35l41_hda *cs35l41, struct device *physde
>   		 * first.
>   		 */
>   		if (cfg->cs_gpio_index >= 0) {
> -			spi = to_spi_device(cs35l41->dev);
> -
>   			if (cfg->num_amps != 2) {
>   				dev_warn(cs35l41->dev,
>   					 "Cannot update SPI CS, Number of Amps (%d) != 2\n",

Sorry for incorrect expression and confusion, it is indeed not the driver
that hangs. What I meant is that _computer_ "hangs" on wake up from
suspend. Unlike boot, where driver does not delay boot process, on wake up
from suspend it seems it does - after lid was opened/power button pressed,
with firmware loading taking ~180seconds in total, computer still has
black screen and is irresponsive for said duration, which is completely
unacceptable.

I do not have enough expertise in particular area, but it sounds very weird
to me that audio driver is delaying system wake up process at first place.
Was this intentional? I would assume/guess most correct solution would be
for driver to run non-blocking, like it does on boot, but again, I am not
too familiar with the subject.

> (~80s per amp) to load the firmware.

Besides firmware loading, there are general initialization/communication
taking place as well. I have disabled firmware loading to try: at a speed
of 3051Hz, it takes ~16 seconds on boot (non blocking, so not a big deal)
and ~7-8 seconds on wake up from suspend (blocking, so it is still not
acceptable).

I am myself extremely exited to get support for 9530 in upstream, but I am
just afraid that such a big wake up delay is a huge hit on a end user, and
would affect everyone with 9530 where intel-lpss patch was not applied yet.

> Instead I would prefer that we instead disable the loading of the 
> firmware in this case.
> Without loading firmware, the volume is much lower, but at least you 
> still have audio.

This indeed sounds like a better approach, I did not think of that. This
should work much better for generic cases, but unfortunately, will still
not prevent devices with _extremely_ slow SPI from badly affecting UX

Taking into account the above, and unless driver being blocking on wake up
can be resolved, perhaps it would makes sense to do both?
a) Your suggestion - disable firmware loading if SPI speed is not in MHz
range and
b) Do not initialize device at all, if SPI speed is ridiculously low (like
for example 3051 Hz)?

I have tested on 9530 without firmware loading, with SPI speed set to
50000Hz: it delays wake up by ~0.9-1 seconds. Subjectively, I think this is
the maximum acceptable delay.

> I have a patch to do that, which I was planning on pushing up 
> (hopefully) today.

Thanks for following up on this!

> 
> Thanks,
> 
> Stefan
> 
> >   		/*
> >   		 * Manually set the Chip Select for the second amp <cs_gpio_index> in the node.
> >   		 * This is only supported for systems with 2 amps, since we cannot expand the
> > @@ -219,8 +232,6 @@ static int generic_dsd_config(struct cs35l41_hda *cs35l41, struct device *physde
> >   		 * first.
> >   		 */
> >   		if (cfg->cs_gpio_index >= 0) {
> > -			spi = to_spi_device(cs35l41->dev);
> > -
> >   			if (cfg->num_amps != 2) {
> >   				dev_warn(cs35l41->dev,
> >   					 "Cannot update SPI CS, Number of Amps (%d) != 2\n",

FYI intel-lpss patch was submitted for review [1]. However, as it is in
different tree, it cannot be guaranteed that it will be always applied
when your patch for 9530 and other Dell devices will be applied, which is
why I am insisting on safety guard against _extremely_ low SPI speeds.

[1]: https://lore.kernel.org/all/20231220205621.8575-1-alex.vinarskis@gmail.com/

Hi,

> -----Original Message-----
> From: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> Sent: Thursday, December 21, 2023 11:25 AM
> To: sbinding@opensource.cirrus.com
> Cc: alex.vinarskis@gmail.com; alsa-devel@alsa-project.org;
> david.rhodes@cirrus.com; james.schulman@cirrus.com;
> josbeir@gmail.com; linux-kernel@vger.kernel.org;
> patches@opensource.cirrus.com; perex@perex.cz;
> stuarth@opensource.cirrus.com; tiwai@suse.com; tiwai@suse.de
> Subject: Re: [PATCH v2 1/2] ALSA: hda: cs35l41: Safety-guard against
> capped SPI speed
> 
> Sorry for incorrect expression and confusion, it is indeed not the
driver
> that hangs. What I meant is that _computer_ "hangs" on wake up from
> suspend. Unlike boot, where driver does not delay boot process, on
wake
> up
> from suspend it seems it does - after lid was opened/power button
> pressed,
> with firmware loading taking ~180seconds in total, computer still
has
> black screen and is irresponsive for said duration, which is
completely
> unacceptable.
> 
> I do not have enough expertise in particular area, but it sounds
very weird
> to me that audio driver is delaying system wake up process at first
place.
> Was this intentional? I would assume/guess most correct solution
would be
> for driver to run non-blocking, like it does on boot, but again, I
am not
> too familiar with the subject.
> 
> > (~80s per amp) to load the firmware.
> 
> Besides firmware loading, there are general
initialization/communication
> taking place as well. I have disabled firmware loading to try: at a
speed
> of 3051Hz, it takes ~16 seconds on boot (non blocking, so not a big
deal)
> and ~7-8 seconds on wake up from suspend (blocking, so it is still
not
> acceptable).

In my opinion it would be wrong to kill the speaker driver because the
SPI
performance is so poor, even if the cost is an extra ~8s on wake up.
In the case where an extra 8s on wake up is unacceptable, there are
easier
ways to disable the driver without having to modify kernel code, than
if you
had to do the opposite, and re-enable it in code.

Thanks,
Stefan

> 
> I am myself extremely exited to get support for 9530 in upstream,
but I am
> just afraid that such a big wake up delay is a huge hit on a end
user, and
> would affect everyone with 9530 where intel-lpss patch was not
applied
> yet.
> 
> > Instead I would prefer that we instead disable the loading of the
> > firmware in this case.
> > Without loading firmware, the volume is much lower, but at least
you
> > still have audio.
> 
> This indeed sounds like a better approach, I did not think of that.
This
> should work much better for generic cases, but unfortunately, will
still
> not prevent devices with _extremely_ slow SPI from badly affecting
UX
> 
> Taking into account the above, and unless driver being blocking on
wake up
> can be resolved, perhaps it would makes sense to do both?
> a) Your suggestion - disable firmware loading if SPI speed is not in
MHz
> range and
> b) Do not initialize device at all, if SPI speed is ridiculously low
(like
> for example 3051 Hz)?
> 
> I have tested on 9530 without firmware loading, with SPI speed set
to
> 50000Hz: it delays wake up by ~0.9-1 seconds. Subjectively, I think
this is
> the maximum acceptable delay.
> 
> > I have a patch to do that, which I was planning on pushing up
> > (hopefully) today.
> 
> Thanks for following up on this!
> 
> >
> > Thanks,
> >
> > Stefan
> >
> > >   		/*
> > >   		 * Manually set the Chip Select for the second
amp
> <cs_gpio_index> in the node.
> > >   		 * This is only supported for systems with 2
amps, since
> we cannot expand the
> > > @@ -219,8 +232,6 @@ static int generic_dsd_config(struct
> cs35l41_hda *cs35l41, struct device *physde
> > >   		 * first.
> > >   		 */
> > >   		if (cfg->cs_gpio_index >= 0) {
> > > -			spi = to_spi_device(cs35l41->dev);
> > > -
> > >   			if (cfg->num_amps != 2) {
> > >   				dev_warn(cs35l41->dev,
> > >   					 "Cannot update SPI
CS, Number
> of Amps (%d) != 2\n",
> 
> FYI intel-lpss patch was submitted for review [1]. However, as it is
in
> different tree, it cannot be guaranteed that it will be always
applied
> when your patch for 9530 and other Dell devices will be applied,
which is
> why I am insisting on safety guard against _extremely_ low SPI
speeds.
> 
> [1]: https://lore.kernel.org/all/20231220205621.8575-1-
> alex.vinarskis@gmail.com/