| Message ID | 20231212195243.10666-1-alex.vinarskis@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] ALSA: hda: cs35l41: Dell Fiorano add missing _DSD properties | expand |
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 <josbeir@gmail.com> >> Signed-off-by: Jasper Smet <josbeir@gmail.com> >> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com> > 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 <sbinding@opensource.cirrus.com> >> >> #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 <josbeir@gmail.com> > >> Signed-off-by: Jasper Smet <josbeir@gmail.com> > >> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com> > > 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 <sbinding@opensource.cirrus.com> > >> #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!
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",
Hi, On 20/12/2023 07:38, Aleksandrs Vinarskis wrote: > Add new model entries into configuration table. > > Co-developed-by: Jasper Smet <josbeir@gmail.com> > Signed-off-by: Jasper Smet <josbeir@gmail.com> > Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com> > --- > 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/20231220205621.8575-1-alex.vinarskis@gmail.com/
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/
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 <sbinding@opensource.cirrus.com> #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 }, {} };