Received-SPF: pass (google.com: domain of linux-kernel+bounces-6983-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <8170413d-07a8-4e77-b43d-78cd9e4ea76f@opensource.cirrus.com>
Date: Wed, 20 Dec 2023 13:43:55 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/2] ALSA: hda: cs35l41: Safety-guard against capped
 SPI speed
To: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>, <tiwai@suse.de>
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>
References: <87ttokpyws.wl-tiwai@suse.de>
 <20231220073809.22027-1-alex.vinarskis@gmail.com>
 <20231220073809.22027-2-alex.vinarskis@gmail.com>
Content-Language: en-GB
From: Stefan Binding <sbinding@opensource.cirrus.com>
In-Reply-To: <20231220073809.22027-2-alex.vinarskis@gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

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",