Dell XPS 9530 (2023) has two SPI connected CS35L41 amplifiers, however
is missing _DSD properties, cs-gpios and has a firmware bug which caps SPI
controller's speed to unusable 3051Hz. This patch adds _DSD properties and
sets second cs-gpio. In case SPI speed bug is detected, it will not
initialize the device to avoid hangs on wake up.
Resolution of SPI speed bug requires either a patch to `intel-lpss.c` or an
UEFI update with corrected values from Dell. Tested with locally applied
patch to `intel-lpss` on multiple XPS 9530 devices.
Co-developed-by: Jasper Smet <[email protected]>
Signed-off-by: Jasper Smet <[email protected]>
Signed-off-by: Aleksandrs Vinarskis <[email protected]>
---
sound/pci/hda/cs35l41_hda_property.c | 47 ++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/sound/pci/hda/cs35l41_hda_property.c b/sound/pci/hda/cs35l41_hda_property.c
index c83328971728..69446a794397 100644
--- a/sound/pci/hda/cs35l41_hda_property.c
+++ b/sound/pci/hda/cs35l41_hda_property.c
@@ -7,9 +7,55 @@
// Author: Stefan Binding <[email protected]>
#include <linux/gpio/consumer.h>
+#include <linux/spi/spi.h>
#include <linux/string.h>
#include "cs35l41_hda_property.h"
+/*
+ * Device 10280BEB (Dell XPS 9530) doesn't have _DSD at all. Moreover, pin that is typically
+ * used for `speaker_id` is missing. SPI's cs-gpios definitions are also missing.
+ */
+static int dell_fiorano_no_acpi(struct cs35l41_hda *cs35l41, struct device *physdev, int id,
+ const char *hid)
+{
+ struct cs35l41_hw_cfg *hw_cfg = &cs35l41->hw_cfg;
+ struct spi_device *spi = to_spi_device(cs35l41->dev);
+
+ /*
+ * 10280BEB has a firmware bug, which wrongly enables clock divider for intel-lpss
+ * Resultant SPI clock is 100Mhz/32767=3051Hz, which leads to ~3 minute hang on boot/wake up
+ * Avoid initializing device if lpss was not patched/fixed UEFI was not installed
+ */
+ if (spi->max_speed_hz < CS35L41_SPI_MAX_FREQ) {
+ 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;
+ }
+
+ dev_info(cs35l41->dev, "Adding DSD properties for %s\n", cs35l41->acpi_subsystem_id);
+
+ /* check SPI address to assign the index */
+ cs35l41->index = id;
+ cs35l41->channel_index = 0;
+ /* 10280BEB is missing pin which is typically assigned to `spk-id-gpios` */
+ cs35l41->speaker_id = cs35l41_get_speaker_id(physdev, cs35l41->index, 2, -1);
+ cs35l41->reset_gpio = gpiod_get_index(physdev, NULL, 1, GPIOD_OUT_LOW);
+
+ hw_cfg->spk_pos = cs35l41->index ? 1 : 0; // 0th L, 1st R
+ hw_cfg->bst_type = CS35L41_EXT_BOOST;
+ hw_cfg->gpio1.func = CS35l41_VSPK_SWITCH;
+ hw_cfg->gpio1.valid = true;
+ hw_cfg->gpio2.func = CS35L41_INTERRUPT;
+ hw_cfg->gpio2.valid = true;
+ hw_cfg->valid = true;
+
+ /* Add second cs-gpio here */
+ if (cs35l41->index)
+ spi->cs_gpiod = gpiod_get_index(physdev, NULL, 0, GPIOD_OUT_HIGH);
+
+ return 0;
+}
+
/*
* Device CLSA010(0/1) doesn't have _DSD so a gpiod_get by the label reset won't work.
* And devices created by serial-multi-instantiate don't have their device struct
@@ -92,6 +138,7 @@ static const struct cs35l41_prop_model cs35l41_prop_model_table[] = {
{ "CLSA0100", NULL, lenovo_legion_no_acpi },
{ "CLSA0101", NULL, lenovo_legion_no_acpi },
{ "CSC3551", "103C89C6", hp_vision_acpi_fix },
+ { "CSC3551", "10280BEB", dell_fiorano_no_acpi },
{}
};
--
2.40.1
On Tue, 12 Dec 2023 20:52:43 +0100,
Aleksandrs Vinarskis wrote:
>
> Dell XPS 9530 (2023) has two SPI connected CS35L41 amplifiers, however
> is missing _DSD properties, cs-gpios and has a firmware bug which caps SPI
> controller's speed to unusable 3051Hz. This patch adds _DSD properties and
> sets second cs-gpio. In case SPI speed bug is detected, it will not
> initialize the device to avoid hangs on wake up.
>
> Resolution of SPI speed bug requires either a patch to `intel-lpss.c` or an
> UEFI update with corrected values from Dell. Tested with locally applied
> patch to `intel-lpss` on multiple XPS 9530 devices.
>
> Co-developed-by: Jasper Smet <[email protected]>
> Signed-off-by: Jasper Smet <[email protected]>
> Signed-off-by: Aleksandrs Vinarskis <[email protected]>
Can Cirrus team review this?
thanks,
Takashi
> ---
> sound/pci/hda/cs35l41_hda_property.c | 47 ++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/sound/pci/hda/cs35l41_hda_property.c b/sound/pci/hda/cs35l41_hda_property.c
> index c83328971728..69446a794397 100644
> --- a/sound/pci/hda/cs35l41_hda_property.c
> +++ b/sound/pci/hda/cs35l41_hda_property.c
> @@ -7,9 +7,55 @@
> // Author: Stefan Binding <[email protected]>
>
> #include <linux/gpio/consumer.h>
> +#include <linux/spi/spi.h>
> #include <linux/string.h>
> #include "cs35l41_hda_property.h"
>
> +/*
> + * Device 10280BEB (Dell XPS 9530) doesn't have _DSD at all. Moreover, pin that is typically
> + * used for `speaker_id` is missing. SPI's cs-gpios definitions are also missing.
> + */
> +static int dell_fiorano_no_acpi(struct cs35l41_hda *cs35l41, struct device *physdev, int id,
> + const char *hid)
> +{
> + struct cs35l41_hw_cfg *hw_cfg = &cs35l41->hw_cfg;
> + struct spi_device *spi = to_spi_device(cs35l41->dev);
> +
> + /*
> + * 10280BEB has a firmware bug, which wrongly enables clock divider for intel-lpss
> + * Resultant SPI clock is 100Mhz/32767=3051Hz, which leads to ~3 minute hang on boot/wake up
> + * Avoid initializing device if lpss was not patched/fixed UEFI was not installed
> + */
> + if (spi->max_speed_hz < CS35L41_SPI_MAX_FREQ) {
> + 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;
> + }
> +
> + dev_info(cs35l41->dev, "Adding DSD properties for %s\n", cs35l41->acpi_subsystem_id);
> +
> + /* check SPI address to assign the index */
> + cs35l41->index = id;
> + cs35l41->channel_index = 0;
> + /* 10280BEB is missing pin which is typically assigned to `spk-id-gpios` */
> + cs35l41->speaker_id = cs35l41_get_speaker_id(physdev, cs35l41->index, 2, -1);
> + cs35l41->reset_gpio = gpiod_get_index(physdev, NULL, 1, GPIOD_OUT_LOW);
> +
> + hw_cfg->spk_pos = cs35l41->index ? 1 : 0; // 0th L, 1st R
> + hw_cfg->bst_type = CS35L41_EXT_BOOST;
> + hw_cfg->gpio1.func = CS35l41_VSPK_SWITCH;
> + hw_cfg->gpio1.valid = true;
> + hw_cfg->gpio2.func = CS35L41_INTERRUPT;
> + hw_cfg->gpio2.valid = true;
> + hw_cfg->valid = true;
> +
> + /* Add second cs-gpio here */
> + if (cs35l41->index)
> + spi->cs_gpiod = gpiod_get_index(physdev, NULL, 0, GPIOD_OUT_HIGH);
> +
> + return 0;
> +}
> +
> /*
> * Device CLSA010(0/1) doesn't have _DSD so a gpiod_get by the label reset won't work.
> * And devices created by serial-multi-instantiate don't have their device struct
> @@ -92,6 +138,7 @@ static const struct cs35l41_prop_model cs35l41_prop_model_table[] = {
> { "CLSA0100", NULL, lenovo_legion_no_acpi },
> { "CLSA0101", NULL, lenovo_legion_no_acpi },
> { "CSC3551", "103C89C6", hp_vision_acpi_fix },
> + { "CSC3551", "10280BEB", dell_fiorano_no_acpi },
> {}
> };
>
> --
> 2.40.1
>
On 14/12/2023 11:02, Takashi Iwai wrote:
> On Tue, 12 Dec 2023 20:52:43 +0100,
> Aleksandrs Vinarskis wrote:
>> Dell XPS 9530 (2023) has two SPI connected CS35L41 amplifiers, however
>> is missing _DSD properties, cs-gpios and has a firmware bug which caps SPI
>> controller's speed to unusable 3051Hz. This patch adds _DSD properties and
>> sets second cs-gpio. In case SPI speed bug is detected, it will not
>> initialize the device to avoid hangs on wake up.
>>
>> Resolution of SPI speed bug requires either a patch to `intel-lpss.c` or an
>> UEFI update with corrected values from Dell. Tested with locally applied
>> patch to `intel-lpss` on multiple XPS 9530 devices.
>>
>> Co-developed-by: Jasper Smet <[email protected]>
>> Signed-off-by: Jasper Smet <[email protected]>
>> Signed-off-by: Aleksandrs Vinarskis <[email protected]>
> Can Cirrus team review this?
>
>
> thanks,
>
> Takashi
The patch looks sensible, with some minor comments below, however we're
just at the tail end of testing a patch chain that genericises a lot of
this code and adds support for a rather large batch of other laptops
with incomplete DSD. We're hoping to push this upstream on Monday.
Can I be awkward and ask that we hold off on this patch chain until
then? Then we can add this laptop using the new approach.
If/when the chain is accepted, we will add support for a few Dell
laptops as well, including this one.
>> ---
>> sound/pci/hda/cs35l41_hda_property.c | 47 ++++++++++++++++++++++++++++
>> 1 file changed, 47 insertions(+)
>>
>> diff --git a/sound/pci/hda/cs35l41_hda_property.c b/sound/pci/hda/cs35l41_hda_property.c
>> index c83328971728..69446a794397 100644
>> --- a/sound/pci/hda/cs35l41_hda_property.c
>> +++ b/sound/pci/hda/cs35l41_hda_property.c
>> @@ -7,9 +7,55 @@
>> // Author: Stefan Binding <[email protected]>
>>
>> #include <linux/gpio/consumer.h>
>> +#include <linux/spi/spi.h>
>> #include <linux/string.h>
>> #include "cs35l41_hda_property.h"
>>
>> +/*
>> + * Device 10280BEB (Dell XPS 9530) doesn't have _DSD at all. Moreover, pin that is typically
>> + * used for `speaker_id` is missing. SPI's cs-gpios definitions are also missing.
>> + */
>> +static int dell_fiorano_no_acpi(struct cs35l41_hda *cs35l41, struct device *physdev, int id,
>> + const char *hid)
>> +{
>> + struct cs35l41_hw_cfg *hw_cfg = &cs35l41->hw_cfg;
>> + struct spi_device *spi = to_spi_device(cs35l41->dev);
>> +
>> + /*
>> + * 10280BEB has a firmware bug, which wrongly enables clock divider for intel-lpss
>> + * Resultant SPI clock is 100Mhz/32767=3051Hz, which leads to ~3 minute hang on boot/wake up
>> + * Avoid initializing device if lpss was not patched/fixed UEFI was not installed
>> + */
>> + if (spi->max_speed_hz < CS35L41_SPI_MAX_FREQ) {
>> + 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;
>> + }
Instead of erroring out, I wonder if we can noodle our way to the
appropriate clk and clk_set_rate it up to 4MHz for this particular
laptop only? Stefan's taking a look at that.
Also, any SPI rate >~100k is probably just about usable, so we don't
want to error on <4MHz. Quite often the spi clock is set at some value
just below 4MHz. It's unclear if this is going to get fixed in the BIOS
at this point, so we don't know what exact rate we'd eventually receive.
>> +
>> + dev_info(cs35l41->dev, "Adding DSD properties for %s\n", cs35l41->acpi_subsystem_id);
>> +
>> + /* check SPI address to assign the index */
>> + cs35l41->index = id;
>> + cs35l41->channel_index = 0;
>> + /* 10280BEB is missing pin which is typically assigned to `spk-id-gpios` */
>> + cs35l41->speaker_id = cs35l41_get_speaker_id(physdev, cs35l41->index, 2, -1);
>> + cs35l41->reset_gpio = gpiod_get_index(physdev, NULL, 1, GPIOD_OUT_LOW);
>> +
>> + hw_cfg->spk_pos = cs35l41->index ? 1 : 0; // 0th L, 1st R
>> + hw_cfg->bst_type = CS35L41_EXT_BOOST;
>> + hw_cfg->gpio1.func = CS35l41_VSPK_SWITCH;
>> + hw_cfg->gpio1.valid = true;
>> + hw_cfg->gpio2.func = CS35L41_INTERRUPT;
>> + hw_cfg->gpio2.valid = true;
>> + hw_cfg->valid = true;
>> +
>> + /* Add second cs-gpio here */
>> + if (cs35l41->index)
>> + spi->cs_gpiod = gpiod_get_index(physdev, NULL, 0, GPIOD_OUT_HIGH);
This will break once you pick up AMD's multi-cs patches, we should use
spi_set_csgpiod instead.
>> +
>> + return 0;
>> +}
>> +
>> /*
>> * Device CLSA010(0/1) doesn't have _DSD so a gpiod_get by the label reset won't work.
>> * And devices created by serial-multi-instantiate don't have their device struct
>> @@ -92,6 +138,7 @@ static const struct cs35l41_prop_model cs35l41_prop_model_table[] = {
>> { "CLSA0100", NULL, lenovo_legion_no_acpi },
>> { "CLSA0101", NULL, lenovo_legion_no_acpi },
>> { "CSC3551", "103C89C6", hp_vision_acpi_fix },
>> + { "CSC3551", "10280BEB", dell_fiorano_no_acpi },
>> {}
>> };
>>
>> --
>> 2.40.1
>>
On Thu, 14 Dec 2023 16:39:58 +0100,
Stuart Henderson wrote:
>
>
> On 14/12/2023 11:02, Takashi Iwai wrote:
> > On Tue, 12 Dec 2023 20:52:43 +0100,
> > Aleksandrs Vinarskis wrote:
> >> Dell XPS 9530 (2023) has two SPI connected CS35L41 amplifiers, however
> >> is missing _DSD properties, cs-gpios and has a firmware bug which caps SPI
> >> controller's speed to unusable 3051Hz. This patch adds _DSD properties and
> >> sets second cs-gpio. In case SPI speed bug is detected, it will not
> >> initialize the device to avoid hangs on wake up.
> >>
> >> Resolution of SPI speed bug requires either a patch to `intel-lpss.c` or an
> >> UEFI update with corrected values from Dell. Tested with locally applied
> >> patch to `intel-lpss` on multiple XPS 9530 devices.
> >>
> >> Co-developed-by: Jasper Smet <[email protected]>
> >> Signed-off-by: Jasper Smet <[email protected]>
> >> Signed-off-by: Aleksandrs Vinarskis <[email protected]>
> > Can Cirrus team review this?
> >
> >
> > thanks,
> >
> > Takashi
>
> The patch looks sensible, with some minor comments below, however
> we're just at the tail end of testing a patch chain that genericises a
> lot of this code and adds support for a rather large batch of other
> laptops with incomplete DSD.? We're hoping to push this upstream on
> Monday.
>
> Can I be awkward and ask that we hold off on this patch chain until
> then?? Then we can add this laptop using the new approach.
>
> If/when the chain is accepted, we will add support for a few Dell
> laptops as well, including this one.
It's fine to wait for a while for me.
Hopefully we can make it in 6.7, and we can catch up in the next
week.
(BTW, I'll be off from the next Tuesday, and the reply will be
delayed, but I can eventually check and merge patches remotely :)
> >> ---
> >> sound/pci/hda/cs35l41_hda_property.c | 47 ++++++++++++++++++++++++++++
> >> 1 file changed, 47 insertions(+)
> >>
> >> diff --git a/sound/pci/hda/cs35l41_hda_property.c b/sound/pci/hda/cs35l41_hda_property.c
> >> index c83328971728..69446a794397 100644
> >> --- a/sound/pci/hda/cs35l41_hda_property.c
> >> +++ b/sound/pci/hda/cs35l41_hda_property.c
> >> @@ -7,9 +7,55 @@
> >> // Author: Stefan Binding <[email protected]>
> >> #include <linux/gpio/consumer.h>
> >> +#include <linux/spi/spi.h>
> >> #include <linux/string.h>
> >> #include "cs35l41_hda_property.h"
> >> +/*
> >> + * Device 10280BEB (Dell XPS 9530) doesn't have _DSD at all. Moreover, pin that is typically
> >> + * used for `speaker_id` is missing. SPI's cs-gpios definitions are also missing.
> >> + */
> >> +static int dell_fiorano_no_acpi(struct cs35l41_hda *cs35l41, struct device *physdev, int id,
> >> + const char *hid)
> >> +{
> >> + struct cs35l41_hw_cfg *hw_cfg = &cs35l41->hw_cfg;
> >> + struct spi_device *spi = to_spi_device(cs35l41->dev);
> >> +
> >> + /*
> >> + * 10280BEB has a firmware bug, which wrongly enables clock divider for intel-lpss
> >> + * Resultant SPI clock is 100Mhz/32767=3051Hz, which leads to ~3 minute hang on boot/wake up
> >> + * Avoid initializing device if lpss was not patched/fixed UEFI was not installed
> >> + */
> >> + if (spi->max_speed_hz < CS35L41_SPI_MAX_FREQ) {
> >> + 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;
> >> + }
>
> Instead of erroring out, I wonder if we can noodle our way to the
> appropriate clk and clk_set_rate it up to 4MHz for this particular
> laptop only?? Stefan's taking a look at that.
>
> Also, any SPI rate >~100k is probably just about usable, so we don't
> want to error on <4MHz.? Quite often the spi clock is set at some
> value just below 4MHz.? It's unclear if this is going to get fixed in
> the BIOS at this point, so we don't know what exact rate we'd
> eventually receive.
I suppose the error-out was due to safety reasons, but the clock
adjustment works, it should be fine. Let's see.
> >> +
> >> + dev_info(cs35l41->dev, "Adding DSD properties for %s\n", cs35l41->acpi_subsystem_id);
> >> +
> >> + /* check SPI address to assign the index */
> >> + cs35l41->index = id;
> >> + cs35l41->channel_index = 0;
> >> + /* 10280BEB is missing pin which is typically assigned to `spk-id-gpios` */
> >> + cs35l41->speaker_id = cs35l41_get_speaker_id(physdev, cs35l41->index, 2, -1);
> >> + cs35l41->reset_gpio = gpiod_get_index(physdev, NULL, 1, GPIOD_OUT_LOW);
> >> +
> >> + hw_cfg->spk_pos = cs35l41->index ? 1 : 0; // 0th L, 1st R
> >> + hw_cfg->bst_type = CS35L41_EXT_BOOST;
> >> + hw_cfg->gpio1.func = CS35l41_VSPK_SWITCH;
> >> + hw_cfg->gpio1.valid = true;
> >> + hw_cfg->gpio2.func = CS35L41_INTERRUPT;
> >> + hw_cfg->gpio2.valid = true;
> >> + hw_cfg->valid = true;
> >> +
> >> + /* Add second cs-gpio here */
> >> + if (cs35l41->index)
> >> + spi->cs_gpiod = gpiod_get_index(physdev, NULL, 0, GPIOD_OUT_HIGH);
> This will break once you pick up AMD's multi-cs patches, we should use
> spi_set_csgpiod instead.
Thanks, good to know.
I'm looking forward to your revised patches :)
Takashi
> Can I be awkward and ask that we hold off on this patch chain until
> then? Then we can add this laptop using the new approach.
> If/when the chain is accepted, we will add support for a few Dell
> laptops as well, including this one.
Sounds reasonable. I'll be looking forward to your new framework.
Once up, I can adjust my patch, and if everything still works as expected,
push updated version for review.
> Instead of erroring out, I wonder if we can noodle our way to the
> appropriate clk and clk_set_rate it up to 4MHz for this particular
> laptop only? Stefan's taking a look at that.
Thanks for the initiative. Potentially that would work, however, it would
require to go up the clock tree to the divider. Since its clearly a
firmware bug causing lpss miss-configuration, I intially thought it would
be best to have it resolved there. If you need more information, I would be
happy to share results of our debugging with you via private email.
> Also, any SPI rate >~100k is probably just about usable, so we don't
> want to error on <4MHz. Quite often the spi clock is set at some value
> just below 4MHz. It's unclear if this is going to get fixed in the BIOS
> at this point, so we don't know what exact rate we'd eventually receive.
I'm afraid I have to disagree here, 100k is _way_ too slow. Not sure
intentionally or not, but wake up from suspend is held back by Cirrus
driver. At 100k, I got these results, on boot:
```
[ 5.561244] cs35l41-hda spi1-CSC3551:00-cs35l41-hda.0: Adding DSD properties for 10280BEB
..
[ 11.251145] cs35l41-hda spi1-CSC3551:00-cs35l41-hda.1: CS35L41 Bound - SSID: 10280BEB, BST: 1, VSPK: 1, CH: R, FW EN: 1, SPKID: -19
```
And on wake-up from suspend:
```
[ 307.162720] cs35l41-hda spi1-CSC3551:00-cs35l41-hda.0: DSP1: Firmware version: 3
...
[ 312.515588] cs35l41-hda spi1-CSC3551:00-cs35l41-hda.1: 100000 Hz actual, DMA
```
This means ~5.5 additional seconds of black screen on wake up, in my opnion
this is completely unacceptable. With 4Mhz, it takes sub 1second. Moreover,
the first time (after preconfigured by ALSA delay) sound is played, it
seems it needs to communicate with amplifier, and it takes additional few
seconds to start playing at 100Khz. With 4Mhz, its practically instant.
I agree that it is unlikely for Dell to ever fix its firmware. Thus either
in case of intel-lpss patch, or via clk_set_rate direclty from Cirrus
driver, lpss divider would be set 1:1, SPI controller would receive 100Mhz
clock directly, and set it to whatever is requested internally (currently
4Mhz). This way, lowest 'usable' rate should be irrelevant.
> Quite often the spi clock is set at some value just below 4MHz
Perhaps to address this, we could error out on say half requested rate? In
reality, it will be either requested rate/just sub requested, or something
totally off, like the default 3051Hz in this case.
> This will break once you pick up AMD's multi-cs patches, we should use
> spi_set_csgpiod instead.
Thanks, will correct.
> I suppose the error-out was due to safety reasons, but the clock
> adjustment works, it should be fine. Let's see.
Precisely, since it is indeed unknown when exaclty/if ever firmware will
be fixed, and/or when our patch to intel-lpss will be accepted.
Regards,
Alex
On Thu, Dec 14, 2023 at 04:49:23PM +0100, Takashi Iwai wrote:
> On Thu, 14 Dec 2023 16:39:58 +0100,
> Stuart Henderson wrote:
> > On 14/12/2023 11:02, Takashi Iwai wrote:
> > > On Tue, 12 Dec 2023 20:52:43 +0100,
> > > Aleksandrs Vinarskis wrote:
...
> > > Can Cirrus team review this?
> > The patch looks sensible, with some minor comments below, however
> > we're just at the tail end of testing a patch chain that genericises a
> > lot of this code and adds support for a rather large batch of other
> > laptops with incomplete DSD.? We're hoping to push this upstream on
> > Monday.
> >
> > Can I be awkward and ask that we hold off on this patch chain until
> > then?? Then we can add this laptop using the new approach.
> >
> > If/when the chain is accepted, we will add support for a few Dell
> > laptops as well, including this one.
>
> It's fine to wait for a while for me.
> Hopefully we can make it in 6.7, and we can catch up in the next
> week.
>
> (BTW, I'll be off from the next Tuesday, and the reply will be
> delayed, but I can eventually check and merge patches remotely :)
Since v6.7 is going to be an LTS, I think we are eagerly want to have
one or the other to be included. In case Cirrus is slow with their
patch, I would like to see this one being included. Let's make an
xmas/ny gift to all users of this codec!
--
With Best Regards,
Andy Shevchenko
Following changes by Cirrus, attaching rebased (and greatly simplified)
patch to support Dell XPS 9530 (2023) laptop.
Safety-guard against capped SPI speed is added via separate patch. It is
not conditioned to particular subsystem, as all devices normally should
pass this check. Following suggestion by Stuart Henderson, error out
threshold is lowered to 1/2 of requested rate.
Aleksandrs Vinarskis (2):
ALSA: hda: cs35l41: Safety-guard against capped SPI speed
ALSA: hda: cs35l41: Support Dell XPS 9530 (2023)
sound/pci/hda/cs35l41_hda_property.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
--
2.40.1
Add new model entries into configuration table.
Co-developed-by: Jasper Smet <[email protected]>
Signed-off-by: Jasper Smet <[email protected]>
Signed-off-by: Aleksandrs Vinarskis <[email protected]>
---
sound/pci/hda/cs35l41_hda_property.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/sound/pci/hda/cs35l41_hda_property.c b/sound/pci/hda/cs35l41_hda_property.c
index cb305b093311..ee105743333f 100644
--- a/sound/pci/hda/cs35l41_hda_property.c
+++ b/sound/pci/hda/cs35l41_hda_property.c
@@ -41,6 +41,7 @@ static const struct cs35l41_config cs35l41_config_table[] = {
* Since this laptop has valid ACPI, we do not need to handle cs-gpios, since that already exists
* in the ACPI. The Reset GPIO is also valid, so we can use the Reset defined in _DSD.
*/
+ { "10280BEB", SPI, 2, EXTERNAL, { CS35L41_LEFT, CS35L41_RIGHT, 0, 0 }, 1, -1, 0, 0, 0, 0 },
{ "103C89C6", SPI, 2, INTERNAL, { CS35L41_RIGHT, CS35L41_LEFT, 0, 0 }, -1, -1, -1, 1000, 4500, 24 },
{ "104312AF", SPI, 2, INTERNAL, { CS35L41_LEFT, CS35L41_RIGHT, 0, 0 }, 1, 2, 0, 1000, 4500, 24 },
{ "10431433", I2C, 2, INTERNAL, { CS35L41_LEFT, CS35L41_RIGHT, 0, 0 }, 0, 1, -1, 1000, 4500, 24 },
@@ -355,6 +356,7 @@ struct cs35l41_prop_model {
static const struct cs35l41_prop_model cs35l41_prop_model_table[] = {
{ "CLSA0100", NULL, lenovo_legion_no_acpi },
{ "CLSA0101", NULL, lenovo_legion_no_acpi },
+ { "CSC3551", "10280BEB", generic_dsd_config },
{ "CSC3551", "103C89C6", generic_dsd_config },
{ "CSC3551", "104312AF", generic_dsd_config },
{ "CSC3551", "10431433", generic_dsd_config },
--
2.40.1
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 <[email protected]>
---
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 <[email protected]>
> ---
> 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",
Hi,
On 20/12/2023 07:38, Aleksandrs Vinarskis wrote:
> Add new model entries into configuration table.
>
> Co-developed-by: Jasper Smet <[email protected]>
> Signed-off-by: Jasper Smet <[email protected]>
> Signed-off-by: Aleksandrs Vinarskis <[email protected]>
> ---
> sound/pci/hda/cs35l41_hda_property.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/sound/pci/hda/cs35l41_hda_property.c b/sound/pci/hda/cs35l41_hda_property.c
> index cb305b093311..ee105743333f 100644
> --- a/sound/pci/hda/cs35l41_hda_property.c
> +++ b/sound/pci/hda/cs35l41_hda_property.c
> @@ -41,6 +41,7 @@ static const struct cs35l41_config cs35l41_config_table[] = {
> * Since this laptop has valid ACPI, we do not need to handle cs-gpios, since that already exists
> * in the ACPI. The Reset GPIO is also valid, so we can use the Reset defined in _DSD.
> */
> + { "10280BEB", SPI, 2, EXTERNAL, { CS35L41_LEFT, CS35L41_RIGHT, 0, 0 }, 1, -1, 0, 0, 0, 0 },
The configuration is correct, however the comment above it applies to
the laptop below, so the entry needs to be above the comment.
I was planning on pushing up a patch myself (hopefully today) which
includes this laptop, as well as a couple of other Dell laptops.
In the same chain is a patch to prevent firmware loading on systems with
slow SPI speed.
It may be wise to wait for those patches instead.
Thanks,
Stefan
> { "103C89C6", SPI, 2, INTERNAL, { CS35L41_RIGHT, CS35L41_LEFT, 0, 0 }, -1, -1, -1, 1000, 4500, 24 },
> { "104312AF", SPI, 2, INTERNAL, { CS35L41_LEFT, CS35L41_RIGHT, 0, 0 }, 1, 2, 0, 1000, 4500, 24 },
> { "10431433", I2C, 2, INTERNAL, { CS35L41_LEFT, CS35L41_RIGHT, 0, 0 }, 0, 1, -1, 1000, 4500, 24 },
> @@ -355,6 +356,7 @@ struct cs35l41_prop_model {
> static const struct cs35l41_prop_model cs35l41_prop_model_table[] = {
> { "CLSA0100", NULL, lenovo_legion_no_acpi },
> { "CLSA0101", NULL, lenovo_legion_no_acpi },
> + { "CSC3551", "10280BEB", generic_dsd_config },
> { "CSC3551", "103C89C6", generic_dsd_config },
> { "CSC3551", "104312AF", generic_dsd_config },
> { "CSC3551", "10431433", generic_dsd_config },
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/[email protected]/
Hi,
> -----Original Message-----
> From: Aleksandrs Vinarskis <[email protected]>
> Sent: Thursday, December 21, 2023 11:25 AM
> To: [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> 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-
> [email protected]/