[PATCH v1 0/8] System Suspend fixes and improvements for CS35L41 HDA
There is a report of a single laptop which uses CS35L41 HDA having an issue with System Suspend. This particular laptop uses S3 (Deep) Sleep. The reported issue states that when the laptop resumes from a system suspend, audio no longer works. The root cause of this issue is due to the CS35L41 being returned to us in an unexpected state after a suspend/resume cycle. When the driver resumes, it expects the parts to have been reset, which leads to issues with audio and firmware loading. To prevent this issue, and the possibility of similar issues, patches 2-5 force the driver to reset during probe, system suspend, and system resume, which ensures that the part is always in the correct state. Patches 6-8 are improvements in the suspend and firmware loading code, which makes it easier to detect issues in the future, as well as simplifiying the suspend code. Patch 1 is a fix for an incorrect configuration for the HP Zbook Fury 17, which is the laptop which had the original issue. Stefan Binding (8): ALSA: hda: cs35l41: Use reset label to get GPIO for HP Zbook Fury 17 G9 ALSA: hda: cs35l41: Assert reset before system suspend ALSA: hda: cs35l41: Assert Reset prior to de-asserting in probe and system resume ALSA: hda: cs35l41: Run boot process during resume callbacks ALSA: hda: cs35l41: Force a software reset after hardware reset ALSA: hda: cs35l41: Do not unload firmware before reset in system suspend ALSA: hda: cs35l41: Check CSPL state after loading firmware ASoC: cs35l41: Detect CSPL errors when sending CSPL commands include/sound/cs35l41.h | 3 + sound/pci/hda/cs35l41_hda.c | 170 +++++++++++++++++---------- sound/pci/hda/cs35l41_hda_property.c | 11 +- sound/soc/codecs/cs35l41-lib.c | 6 + 4 files changed, 124 insertions(+), 66 deletions(-) -- 2.34.1
This laptop has an incorrect setting in its _DSD for boost type, but the rest of the _DSD is correct, and we can still use the reset label to obtain the reset gpio. Also fix channel map so that amp 0 is right, and amp 1 is left. Fixes: 581523ee3652 ("ALSA: hda: cs35l41: Override the _DSD for HP Zbook Fury 17 G9 to correct boost type") Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> --- sound/pci/hda/cs35l41_hda_property.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/sound/pci/hda/cs35l41_hda_property.c b/sound/pci/hda/cs35l41_hda_property.c index b62a4e6968e2..c83328971728 100644 --- a/sound/pci/hda/cs35l41_hda_property.c +++ b/sound/pci/hda/cs35l41_hda_property.c @@ -58,9 +58,16 @@ static int hp_vision_acpi_fix(struct cs35l41_hda *cs35l41, struct device *physde cs35l41->index = id; cs35l41->channel_index = 0; - cs35l41->reset_gpio = gpiod_get_index(physdev, NULL, 1, GPIOD_OUT_HIGH); + + /* + * This system has _DSD, it just contains an error, so we can still get the reset using + * the "reset" label. + */ + cs35l41->reset_gpio = fwnode_gpiod_get_index(acpi_fwnode_handle(cs35l41->dacpi), "reset", + cs35l41->index, GPIOD_OUT_LOW, + "cs35l41-reset"); cs35l41->speaker_id = -ENOENT; - hw_cfg->spk_pos = cs35l41->index ? 1 : 0; // right:left + hw_cfg->spk_pos = cs35l41->index ? 0 : 1; // right:left hw_cfg->gpio1.func = CS35L41_NOT_USED; hw_cfg->gpio1.valid = true; hw_cfg->gpio2.func = CS35L41_INTERRUPT; -- 2.34.1
Some system suspend modes may remove power supplies. To ensure we are not running during this time, we should assert reset. Note: since the amps use a shared reset, asserting reset prior to system suspend only works if the amps are suspended in the reverse order to probe. Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> --- sound/pci/hda/cs35l41_hda.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index 28cb10ddd191..919e38213975 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -813,14 +813,17 @@ static int cs35l41_system_suspend(struct device *dev) /* Shutdown DSP before system suspend */ ret = cs35l41_ready_for_reset(cs35l41); - if (ret) dev_err(dev, "System Suspend Failed, not ready for Reset: %d\n", ret); - /* - * Reset GPIO may be shared, so cannot reset here. - * However beyond this point, amps may be powered down. - */ + if (cs35l41->reset_gpio) { + dev_info(cs35l41->dev, "Asserting Reset\n"); + gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); + usleep_range(2000, 2100); + } + + dev_dbg(cs35l41->dev, "System Suspended\n"); + return ret; } -- 2.34.1
To ensure we are in a known state, exiting from reset at the point of probe or in system resume, assert reset before we de-assert it. Since the BIOS may enter into a pre-boot environment to control the amps (for example for boot beep), we need to ensure we start from a known, reset state prior to probe or system resume. Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> --- sound/pci/hda/cs35l41_hda.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index 919e38213975..1ac721085fb5 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -840,6 +840,7 @@ static int cs35l41_system_resume(struct device *dev) } if (cs35l41->reset_gpio) { + gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); usleep_range(2000, 2100); gpiod_set_value_cansleep(cs35l41->reset_gpio, 1); } @@ -1693,6 +1694,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i } } if (cs35l41->reset_gpio) { + gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); usleep_range(2000, 2100); gpiod_set_value_cansleep(cs35l41->reset_gpio, 1); } -- 2.34.1
During initial probe, after reset is asserted for the first time, the driver goes through a boot process to ensure the amp is ready to be used. This involves verifying a boot flag, as well as verifying the chip ids. This is necessary since it is possible for the amp to have been fully reset by the system suspend calls. Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> --- sound/pci/hda/cs35l41_hda.c | 105 ++++++++++++++++++++++++------------ 1 file changed, 72 insertions(+), 33 deletions(-) diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index 1ac721085fb5..e787788c1be2 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -730,6 +730,34 @@ static int cs35l41_hda_channel_map(struct device *dev, unsigned int tx_num, unsi rx_slot); } +int cs35l41_verify_id(struct cs35l41_hda *cs35l41, unsigned int *regid, unsigned int *reg_revid) +{ + unsigned int mtl_revid, chipid; + int ret; + + ret = regmap_read(cs35l41->regmap, CS35L41_DEVID, regid); + if (ret) { + dev_err_probe(cs35l41->dev, ret, "Get Device ID failed\n"); + return ret; + } + + ret = regmap_read(cs35l41->regmap, CS35L41_REVID, reg_revid); + if (ret) { + dev_err_probe(cs35l41->dev, ret, "Get Revision ID failed\n"); + return ret; + } + + mtl_revid = *reg_revid & CS35L41_MTLREVID_MASK; + + chipid = (mtl_revid % 2) ? CS35L41R_CHIP_ID : CS35L41_CHIP_ID; + if (*regid != chipid) { + dev_err(cs35l41->dev, "CS35L41 Device ID (%X). Expected ID %X\n", *regid, chipid); + return -ENODEV; + } + + return 0; +} + static int cs35l41_ready_for_reset(struct cs35l41_hda *cs35l41) { int ret = 0; @@ -827,6 +855,30 @@ static int cs35l41_system_suspend(struct device *dev) return ret; } +static int cs35l41_wait_boot_done(struct cs35l41_hda *cs35l41) +{ + unsigned int int_status; + int ret; + + ret = regmap_read_poll_timeout(cs35l41->regmap, CS35L41_IRQ1_STATUS4, int_status, + int_status & CS35L41_OTP_BOOT_DONE, 1000, 100000); + if (ret) { + dev_err(cs35l41->dev, "Failed waiting for OTP_BOOT_DONE\n"); + return ret; + } + + ret = regmap_read(cs35l41->regmap, CS35L41_IRQ1_STATUS3, &int_status); + if (ret || (int_status & CS35L41_OTP_BOOT_ERR)) { + dev_err(cs35l41->dev, "OTP Boot status %x error\n", + int_status & CS35L41_OTP_BOOT_ERR); + if (!ret) + ret = -EIO; + return ret; + } + + return 0; +} + static int cs35l41_system_resume(struct device *dev) { struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); @@ -847,6 +899,14 @@ static int cs35l41_system_resume(struct device *dev) usleep_range(2000, 2100); + regcache_cache_only(cs35l41->regmap, false); + + ret = cs35l41_wait_boot_done(cs35l41); + if (ret) + return ret; + + regcache_cache_only(cs35l41->regmap, true); + ret = pm_runtime_force_resume(dev); if (ret) { dev_err(dev, "System Resume Failed: Unable to runtime resume: %d\n", ret); @@ -908,6 +968,7 @@ static int cs35l41_runtime_suspend(struct device *dev) static int cs35l41_runtime_resume(struct device *dev) { struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); + unsigned int regid, reg_revid; int ret = 0; dev_dbg(cs35l41->dev, "Runtime Resume\n"); @@ -929,6 +990,10 @@ static int cs35l41_runtime_resume(struct device *dev) } } + ret = cs35l41_verify_id(cs35l41, ®id, ®_revid); + if (ret) + goto err; + /* Test key needs to be unlocked to allow the OTP settings to re-apply */ cs35l41_test_key_unlock(cs35l41->dev, cs35l41->regmap); ret = regcache_sync(cs35l41->regmap); @@ -941,6 +1006,8 @@ static int cs35l41_runtime_resume(struct device *dev) if (cs35l41->hw_cfg.bst_type == CS35L41_EXT_BOOST) cs35l41_init_boost(cs35l41->dev, cs35l41->regmap, &cs35l41->hw_cfg); + dev_dbg(cs35l41->dev, "CS35L41 Resumed (%x), Revision: %02X\n", regid, reg_revid); + err: mutex_unlock(&cs35l41->fw_mutex); @@ -1660,7 +1727,7 @@ static int cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41, const char *hid, i int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int irq, struct regmap *regmap) { - unsigned int int_sts, regid, reg_revid, mtl_revid, chipid, int_status; + unsigned int regid, reg_revid; struct cs35l41_hda *cs35l41; int ret; @@ -1701,41 +1768,13 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i usleep_range(2000, 2100); - ret = regmap_read_poll_timeout(cs35l41->regmap, CS35L41_IRQ1_STATUS4, int_status, - int_status & CS35L41_OTP_BOOT_DONE, 1000, 100000); - if (ret) { - dev_err_probe(cs35l41->dev, ret, "Failed waiting for OTP_BOOT_DONE\n"); - goto err; - } - - ret = regmap_read(cs35l41->regmap, CS35L41_IRQ1_STATUS3, &int_sts); - if (ret || (int_sts & CS35L41_OTP_BOOT_ERR)) { - dev_err_probe(cs35l41->dev, ret, "OTP Boot status %x error\n", - int_sts & CS35L41_OTP_BOOT_ERR); - ret = -EIO; - goto err; - } - - ret = regmap_read(cs35l41->regmap, CS35L41_DEVID, ®id); - if (ret) { - dev_err_probe(cs35l41->dev, ret, "Get Device ID failed\n"); - goto err; - } - - ret = regmap_read(cs35l41->regmap, CS35L41_REVID, ®_revid); - if (ret) { - dev_err_probe(cs35l41->dev, ret, "Get Revision ID failed\n"); + ret = cs35l41_wait_boot_done(cs35l41); + if (ret) goto err; - } - - mtl_revid = reg_revid & CS35L41_MTLREVID_MASK; - chipid = (mtl_revid % 2) ? CS35L41R_CHIP_ID : CS35L41_CHIP_ID; - if (regid != chipid) { - dev_err(cs35l41->dev, "CS35L41 Device ID (%X). Expected ID %X\n", regid, chipid); - ret = -ENODEV; + ret = cs35l41_verify_id(cs35l41, ®id, ®_revid); + if (ret) goto err; - } ret = cs35l41_test_key_unlock(cs35l41->dev, cs35l41->regmap); if (ret) -- 2.34.1
To ensure the chip has correctly reset during probe and system suspend, we need to force a software reset, in case of systems where the hardware reset is not available. The software reset register was labelled as volatile but not readable, however, it is readable, (just returns 0x0). Adding it to readable registers means it will be correctly treated as volatile, and thus will not be cached. Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> --- include/sound/cs35l41.h | 1 + sound/pci/hda/cs35l41_hda.c | 5 +++++ sound/soc/codecs/cs35l41-lib.c | 1 + 3 files changed, 7 insertions(+) diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h index 2fe8c6b0d4cf..80df80fe31e2 100644 --- a/include/sound/cs35l41.h +++ b/include/sound/cs35l41.h @@ -735,6 +735,7 @@ #define CS35L41_REVID_B2 0xB2 #define CS35L41_HALO_CORE_RESET 0x00000200 +#define CS35L41_SOFTWARE_RESET 0x5A000000 #define CS35L41_FS1_WINDOW_MASK 0x000007FF #define CS35L41_FS2_WINDOW_MASK 0x00FFF800 diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index e787788c1be2..9746c64ff0dd 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -901,6 +901,9 @@ static int cs35l41_system_resume(struct device *dev) regcache_cache_only(cs35l41->regmap, false); + regmap_write(cs35l41->regmap, CS35L41_SFT_RESET, CS35L41_SOFTWARE_RESET); + usleep_range(2000, 2100); + ret = cs35l41_wait_boot_done(cs35l41); if (ret) return ret; @@ -1766,6 +1769,8 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i gpiod_set_value_cansleep(cs35l41->reset_gpio, 1); } + usleep_range(2000, 2100); + regmap_write(cs35l41->regmap, CS35L41_SFT_RESET, CS35L41_SOFTWARE_RESET); usleep_range(2000, 2100); ret = cs35l41_wait_boot_done(cs35l41); diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c index 2ec5fdc875b1..ddedb7e63cb6 100644 --- a/sound/soc/codecs/cs35l41-lib.c +++ b/sound/soc/codecs/cs35l41-lib.c @@ -74,6 +74,7 @@ static bool cs35l41_readable_reg(struct device *dev, unsigned int reg) case CS35L41_FABID: case CS35L41_RELID: case CS35L41_OTPID: + case CS35L41_SFT_RESET: case CS35L41_TEST_KEY_CTL: case CS35L41_USER_KEY_CTL: case CS35L41_OTP_CTRL0: -- 2.34.1
Given the part is about to reset due to system suspend, and we are already in hibernate, there is no need to wake up the amp, just to get it ready to be reset. We just need to ensure cs_dsp is ready for reset by resetting the states. Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> --- sound/pci/hda/cs35l41_hda.c | 33 ++++----------------------------- 1 file changed, 4 insertions(+), 29 deletions(-) diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index 9746c64ff0dd..69303888be1a 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -760,41 +760,16 @@ int cs35l41_verify_id(struct cs35l41_hda *cs35l41, unsigned int *regid, unsigned static int cs35l41_ready_for_reset(struct cs35l41_hda *cs35l41) { - int ret = 0; - mutex_lock(&cs35l41->fw_mutex); if (cs35l41->firmware_running) { - - regcache_cache_only(cs35l41->regmap, false); - - ret = cs35l41_exit_hibernate(cs35l41->dev, cs35l41->regmap); - if (ret) { - dev_warn(cs35l41->dev, "Unable to exit Hibernate."); - goto err; - } - - /* Test key needs to be unlocked to allow the OTP settings to re-apply */ - cs35l41_test_key_unlock(cs35l41->dev, cs35l41->regmap); - ret = regcache_sync(cs35l41->regmap); - cs35l41_test_key_lock(cs35l41->dev, cs35l41->regmap); - if (ret) { - dev_err(cs35l41->dev, "Failed to restore register cache: %d\n", ret); - goto err; - } - - if (cs35l41->hw_cfg.bst_type == CS35L41_EXT_BOOST) - cs35l41_init_boost(cs35l41->dev, cs35l41->regmap, &cs35l41->hw_cfg); - - cs35l41_shutdown_dsp(cs35l41); - cs35l41_safe_reset(cs35l41->regmap, cs35l41->hw_cfg.bst_type); + cs35l41->cs_dsp.running = false; + cs35l41->cs_dsp.booted = false; + cs35l41->firmware_running = false; } -err: - regcache_cache_only(cs35l41->regmap, true); regcache_mark_dirty(cs35l41->regmap); - mutex_unlock(&cs35l41->fw_mutex); - return ret; + return 0; } static int cs35l41_system_suspend_prep(struct device *dev) -- 2.34.1
CSPL firmware should be in RUNNING or PAUSED state after loading. If not, the firmware has not been loaded correctly, and we can unload it and pass the error up. Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> --- sound/pci/hda/cs35l41_hda.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index 69303888be1a..496ff6a9d300 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -994,6 +994,7 @@ static int cs35l41_runtime_resume(struct device *dev) static int cs35l41_smart_amp(struct cs35l41_hda *cs35l41) { + unsigned int fw_status; __be32 halo_sts; int ret; @@ -1027,6 +1028,23 @@ static int cs35l41_smart_amp(struct cs35l41_hda *cs35l41) goto clean_dsp; } + ret = regmap_read(cs35l41->regmap, CS35L41_DSP_MBOX_2, &fw_status); + if (ret < 0) { + dev_err(cs35l41->dev, + "Failed to read firmware status: %d\n", ret); + goto clean_dsp; + } + + switch (fw_status) { + case CSPL_MBOX_STS_RUNNING: + case CSPL_MBOX_STS_PAUSED: + break; + default: + dev_err(cs35l41->dev, "Firmware status is invalid: %u\n", + fw_status); + goto clean_dsp; + } + ret = cs35l41_set_cspl_mbox_cmd(cs35l41->dev, cs35l41->regmap, CSPL_MBOX_CMD_PAUSE); if (ret) { dev_err(cs35l41->dev, "Error waiting for DSP to pause: %u\n", ret); -- 2.34.1
The existing code checks for the correct state transition after sending a command. However, it is possible for the message box to return -1, which indicates an error, if an error has occurred in the firmware. We can detect if the error has occurred, and return a different error. In addition, there is no recovering from a CSPL error, so the retry mechanism is not needed in this case, and we can return immediately. Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> --- include/sound/cs35l41.h | 2 ++ sound/soc/codecs/cs35l41-lib.c | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h index 80df80fe31e2..043f8ac65dbf 100644 --- a/include/sound/cs35l41.h +++ b/include/sound/cs35l41.h @@ -816,6 +816,8 @@ struct cs35l41_otp_map_element_t { }; enum cs35l41_cspl_mbox_status { + CSPL_MBOX_STS_ERROR = U32_MAX, + CSPL_MBOX_STS_ERROR2 = 0x00ffffff, // firmware not always sign-extending 24-bit value CSPL_MBOX_STS_RUNNING = 0, CSPL_MBOX_STS_PAUSED = 1, CSPL_MBOX_STS_RDY_FOR_REINIT = 2, diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c index ddedb7e63cb6..4569e4f7cf7e 100644 --- a/sound/soc/codecs/cs35l41-lib.c +++ b/sound/soc/codecs/cs35l41-lib.c @@ -1474,6 +1474,11 @@ int cs35l41_set_cspl_mbox_cmd(struct device *dev, struct regmap *regmap, continue; } + if (sts == CSPL_MBOX_STS_ERROR || sts == CSPL_MBOX_STS_ERROR2) { + dev_err(dev, "CSPL Error Detected\n"); + return -EINVAL; + } + if (!cs35l41_check_cspl_mbox_sts(cmd, sts)) dev_dbg(dev, "[%u] cmd %u returned invalid sts %u", i, cmd, sts); else -- 2.34.1
On Thu, Oct 26, 2023 at 04:05:58PM +0100, Stefan Binding wrote:
The existing code checks for the correct state transition after sending a command. However, it is possible for the message box to return -1, which indicates an error, if an error has occurred in the firmware. We can detect if the error has occurred, and return a different error. In addition, there is no recovering from a CSPL error, so the retry mechanism is not needed in this case, and we can return immediately.
Acked-by: Mark Brown <broonie@kernel.org>
On Thu, 26 Oct 2023 17:05:50 +0200, Stefan Binding wrote:
There is a report of a single laptop which uses CS35L41 HDA having an issue with System Suspend. This particular laptop uses S3 (Deep) Sleep. The reported issue states that when the laptop resumes from a system suspend, audio no longer works.
The root cause of this issue is due to the CS35L41 being returned to us in an unexpected state after a suspend/resume cycle. When the driver resumes, it expects the parts to have been reset, which leads to issues with audio and firmware loading.
To prevent this issue, and the possibility of similar issues, patches 2-5 force the driver to reset during probe, system suspend, and system resume, which ensures that the part is always in the correct state. Patches 6-8 are improvements in the suspend and firmware loading code, which makes it easier to detect issues in the future, as well as simplifiying the suspend code.
Patch 1 is a fix for an incorrect configuration for the HP Zbook Fury 17, which is the laptop which had the original issue.
Stefan Binding (8): ALSA: hda: cs35l41: Use reset label to get GPIO for HP Zbook Fury 17 G9 ALSA: hda: cs35l41: Assert reset before system suspend ALSA: hda: cs35l41: Assert Reset prior to de-asserting in probe and system resume ALSA: hda: cs35l41: Run boot process during resume callbacks ALSA: hda: cs35l41: Force a software reset after hardware reset ALSA: hda: cs35l41: Do not unload firmware before reset in system suspend ALSA: hda: cs35l41: Check CSPL state after loading firmware ASoC: cs35l41: Detect CSPL errors when sending CSPL commands
Applied to for-next branch now. Thanks. Takashi
participants (3)
-
Mark Brown -
Stefan Binding -
Takashi Iwai