[PATCH v1 0/3] ASoC: cs42l42: Fix handling of hard reset
These patches fix 3 problems with hard reset: 1. Ensure a minimum reset pulse width 2. Deal with ACPI overriding the requested default GPIO state 3. Avoid a race condition when hard-resetting a SoundWire peripheral that is already enumerated
Richard Fitzgerald (3): ASoC: cs42l42: Ensure a reset pulse meets minimum pulse width. ASoC: cs42l42: Don't rely on GPIOD_OUT_LOW to set RESET initially low ASoC: cs42l42: Avoid stale SoundWire ATTACH after hard reset
sound/soc/codecs/cs42l42-sdw.c | 20 ++++++++++++++++++++ sound/soc/codecs/cs42l42.c | 21 ++++++++++++++++++++- sound/soc/codecs/cs42l42.h | 1 + 3 files changed, 41 insertions(+), 1 deletion(-)
From: Richard Fitzgerald rf@opensource.cirrus.com
The CS42L42 can accept very short reset pulses of a few microseconds but there's no reason to force a very short pulse. Allow a wide range for the usleep_range() so it can be relaxed about the choice of timing source.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Signed-off-by: Stefan Binding sbinding@opensource.cirrus.com --- sound/soc/codecs/cs42l42.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c index a0de0329406a..56d2857a4f01 100644 --- a/sound/soc/codecs/cs42l42.c +++ b/sound/soc/codecs/cs42l42.c @@ -2320,6 +2320,10 @@ int cs42l42_common_probe(struct cs42l42_private *cs42l42,
if (cs42l42->reset_gpio) { dev_dbg(cs42l42->dev, "Found reset GPIO\n"); + + /* Ensure minimum reset pulse width */ + usleep_range(10, 500); + gpiod_set_value_cansleep(cs42l42->reset_gpio, 1); } usleep_range(CS42L42_BOOT_TIME_US, CS42L42_BOOT_TIME_US * 2);
From: Richard Fitzgerald rf@opensource.cirrus.com
The ACPI setting for a GPIO default state has higher priority than the flag passed to devm_gpiod_get_optional() so ACPI can override the GPIOD_OUT_LOW. Explicitly set the GPIO low when hard resetting.
Although GPIOD_OUT_LOW can't be relied on this doesn't seem like a reason to stop passing it to devm_gpiod_get_optional(). So we still pass it to state our intent, but can deal with it having no effect.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Signed-off-by: Stefan Binding sbinding@opensource.cirrus.com --- sound/soc/codecs/cs42l42.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c index 56d2857a4f01..dc93861ddfb0 100644 --- a/sound/soc/codecs/cs42l42.c +++ b/sound/soc/codecs/cs42l42.c @@ -2321,6 +2321,12 @@ int cs42l42_common_probe(struct cs42l42_private *cs42l42, if (cs42l42->reset_gpio) { dev_dbg(cs42l42->dev, "Found reset GPIO\n");
+ /* + * ACPI can override the default GPIO state we requested + * so ensure that we start with RESET low. + */ + gpiod_set_value_cansleep(cs42l42->reset_gpio, 0); + /* Ensure minimum reset pulse width */ usleep_range(10, 500);
From: Richard Fitzgerald rf@opensource.cirrus.com
In SoundWire mode leave hard RESET asserted when exiting probe, and wait for an UNATTACHED notification before deasserting RESET.
If the boot state of the reset GPIO was deasserted it is possible that the SoundWire core had already enumerated the CS42L42 before cs42l42_sdw_probe() is called. When cs42l42_common_probe() hard resets the CS42L42 it triggers a race condition:
1) After cs42l42_sdw_probe() returns the thread that called it will call cs42l42_sdw_update_status() to report the last status recorded by the SoundWire core.
2) The SoundWire bus master will see a PING with the CS42L42 now reporting as unenumerated and will trigger the core SoundWire code to start enumerating CS42L42.
These two threads are racing against each other. If (1) happens before (2) a stale ATTACHED notification will be reported to the cs42l42 driver when in fact the status of cs42l42 is now unattached.
To avoid this race condition:
- Leave RESET asserted on exit from cs42l42_sdw_probe(). This ensures that an UNATTACHED notification must be sent to the cs42l42 driver. If cs42l42 was already enumerated it will be seen to drop off the bus, causing an UNATTACH notification. If it was never enumerated the status is already UNATTACHED and this will be reported by thread (1).
- When the UNATTACH notification is received, release RESET. This will cause CS42L42 to be enumerated and eventually report an ATTACHED notification.
- The ATTACHED notification is now valid.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Signed-off-by: Stefan Binding sbinding@opensource.cirrus.com --- sound/soc/codecs/cs42l42-sdw.c | 20 ++++++++++++++++++++ sound/soc/codecs/cs42l42.c | 11 ++++++++++- sound/soc/codecs/cs42l42.h | 1 + 3 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/cs42l42-sdw.c b/sound/soc/codecs/cs42l42-sdw.c index eeab07c850f9..974bae4abfad 100644 --- a/sound/soc/codecs/cs42l42-sdw.c +++ b/sound/soc/codecs/cs42l42-sdw.c @@ -344,6 +344,16 @@ static int cs42l42_sdw_update_status(struct sdw_slave *peripheral, switch (status) { case SDW_SLAVE_ATTACHED: dev_dbg(cs42l42->dev, "ATTACHED\n"); + + /* + * The SoundWire core can report stale ATTACH notifications + * if we hard-reset CS42L42 in probe() but it had already been + * enumerated. Reject the ATTACH if we haven't yet seen an + * UNATTACH report for the device being in reset. + */ + if (cs42l42->sdw_waiting_first_unattach) + break; + /* * Initialise codec, this only needs to be done once. * When resuming from suspend, resume callback will handle re-init of codec, @@ -354,6 +364,16 @@ static int cs42l42_sdw_update_status(struct sdw_slave *peripheral, break; case SDW_SLAVE_UNATTACHED: dev_dbg(cs42l42->dev, "UNATTACHED\n"); + + if (cs42l42->sdw_waiting_first_unattach) { + /* + * SoundWire core has seen that CS42L42 is not on + * the bus so release RESET and wait for ATTACH. + */ + cs42l42->sdw_waiting_first_unattach = false; + gpiod_set_value_cansleep(cs42l42->reset_gpio, 1); + } + break; default: break; diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c index dc93861ddfb0..2961340f15e2 100644 --- a/sound/soc/codecs/cs42l42.c +++ b/sound/soc/codecs/cs42l42.c @@ -2330,7 +2330,16 @@ int cs42l42_common_probe(struct cs42l42_private *cs42l42, /* Ensure minimum reset pulse width */ usleep_range(10, 500);
- gpiod_set_value_cansleep(cs42l42->reset_gpio, 1); + /* + * On SoundWire keep the chip in reset until we get an UNATTACH + * notification from the SoundWire core. This acts as a + * synchronization point to reject stale ATTACH notifications + * if the chip was already enumerated before we reset it. + */ + if (cs42l42->sdw_peripheral) + cs42l42->sdw_waiting_first_unattach = true; + else + gpiod_set_value_cansleep(cs42l42->reset_gpio, 1); } usleep_range(CS42L42_BOOT_TIME_US, CS42L42_BOOT_TIME_US * 2);
diff --git a/sound/soc/codecs/cs42l42.h b/sound/soc/codecs/cs42l42.h index 4bd7b85a5747..7785125b73ab 100644 --- a/sound/soc/codecs/cs42l42.h +++ b/sound/soc/codecs/cs42l42.h @@ -53,6 +53,7 @@ struct cs42l42_private { u8 stream_use; bool hp_adc_up_pending; bool suspended; + bool sdw_waiting_first_unattach; bool init_done; };
On Wed, 13 Sep 2023 16:00:09 +0100, Stefan Binding wrote:
These patches fix 3 problems with hard reset:
- Ensure a minimum reset pulse width
- Deal with ACPI overriding the requested default GPIO state
- Avoid a race condition when hard-resetting a SoundWire peripheral that is already enumerated
Richard Fitzgerald (3): ASoC: cs42l42: Ensure a reset pulse meets minimum pulse width. ASoC: cs42l42: Don't rely on GPIOD_OUT_LOW to set RESET initially low ASoC: cs42l42: Avoid stale SoundWire ATTACH after hard reset
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/3] ASoC: cs42l42: Ensure a reset pulse meets minimum pulse width. commit: 41dac81b56c82c51a6d00fda5f3af7691ffee2d7 [2/3] ASoC: cs42l42: Don't rely on GPIOD_OUT_LOW to set RESET initially low commit: a479b44ac0a0ac25cd48e5356200078924d78022 [3/3] ASoC: cs42l42: Avoid stale SoundWire ATTACH after hard reset commit: 2d066c6a78654c179f95c9beda1985d4c6befa4e
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (2)
-
Mark Brown
-
Stefan Binding