Received-SPF: pass (google.com: domain of linux-kernel+bounces-8619-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Stefan Binding <sbinding@opensource.cirrus.com>
To: 'Aleksandrs Vinarskis' <alex.vinarskis@gmail.com>
CC: <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>
References: <8170413d-07a8-4e77-b43d-78cd9e4ea76f@opensource.cirrus.com> <20231221112434.13730-1-alex.vinarskis@gmail.com>
In-Reply-To: <20231221112434.13730-1-alex.vinarskis@gmail.com>
Subject: RE: [PATCH v2 1/2] ALSA: hda: cs35l41: Safety-guard against capped SPI speed
Date: Thu, 21 Dec 2023 15:09:16 +0000
Message-ID: <006c01da341f$aa9dcdb0$ffd96910$@opensource.cirrus.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Content-Language: en-gb
Thread-Index: AQHBvASMLXdMRoUjXEjgpQelQ430kgGT5HMZsNgh7ZA=

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/