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.
alex.vinarskis@gmail.com/