Received-SPF: pass (google.com: domain of linux-kernel+bounces-8278-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
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
Date: Thu, 21 Dec 2023 12:24:34 +0100
Message-Id: <20231221112434.13730-1-alex.vinarskis@gmail.com>
In-Reply-To: <8170413d-07a8-4e77-b43d-78cd9e4ea76f@opensource.cirrus.com>
References: <8170413d-07a8-4e77-b43d-78cd9e4ea76f@opensource.cirrus.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

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/