[PATCH] ASoC: cs42l42: Implement system suspend

[PATCH] ASoC: cs42l42: Implement system suspend

[Richard Fitzgerald]

Add system suspend functions to handle clean power-down on suspend and
restoring state on resume.

The jack state could change during suspend. Plug->unplug and unplug->plug
are straightforward because this looks no different from any other plug
state change. However, the jack could be unplugged and a different type
of jack plugged, and on resume the plug state would not have changed.
Some code changes are needed to the jack handling so that on resume a
plugged state will be re-evaluated instead of filtered out. In this case
there would not have been a previous unplug event to clear the reported
jack state so the full state of all jack type and button flags must be
reported.

Note that during system suspend any jack plug/unplug and button events
will not be reported or generate a system wakeup. If the plug state or
headset type has changed it will be reported after resume.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/cs42l42.c | 212 ++++++++++++++++++++++++++++++++++++++++++---
 sound/soc/codecs/cs42l42.h |   8 +-
 2 files changed, 209 insertions(+), 11 deletions(-)

diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c
index 43d98bdb5b5b..5abac0dc8e8b 100644
--- a/sound/soc/codecs/cs42l42.c
+++ b/sound/soc/codecs/cs42l42.c
@@ -47,7 +47,6 @@ static const struct reg_default cs42l42_reg_defaults[] = {
 	{ CS42L42_I2C_STRETCH,			0x03 },
 	{ CS42L42_I2C_TIMEOUT,			0xB7 },
 	{ CS42L42_PWR_CTL1,			0xFF },
-	{ CS42L42_PWR_CTL2,			0x84 },
 	{ CS42L42_PWR_CTL3,			0x20 },
 	{ CS42L42_RSENSE_CTL1,			0x40 },
 	{ CS42L42_RSENSE_CTL2,			0x00 },
@@ -331,6 +330,11 @@ static bool cs42l42_volatile_register(struct device *dev, unsigned int reg)
 	case CS42L42_DEVID_CD:
 	case CS42L42_DEVID_E:
 	case CS42L42_MCLK_STATUS:
+	/*
+	 * PWR_CTL2 must be volatile so it can be used as a canary bit to
+	 * detect a reset during system suspend.
+	 */
+	case CS42L42_PWR_CTL2:
 	case CS42L42_OSC_SWITCH_STATUS:
 	case CS42L42_TRSENSE_STATUS:
 	case CS42L42_HS_DET_STATUS:
@@ -550,7 +554,7 @@ static int cs42l42_set_jack(struct snd_soc_component *component, struct snd_soc_
 	struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
 
 	/* Prevent race with interrupt handler */
-	mutex_lock(&cs42l42->jack_detect_mutex);
+	mutex_lock(&cs42l42->irq_lock);
 	cs42l42->jack = jk;
 
 	if (jk) {
@@ -566,7 +570,7 @@ static int cs42l42_set_jack(struct snd_soc_component *component, struct snd_soc_
 			break;
 		}
 	}
-	mutex_unlock(&cs42l42->jack_detect_mutex);
+	mutex_unlock(&cs42l42->irq_lock);
 
 	return 0;
 }
@@ -1613,6 +1617,12 @@ static irqreturn_t cs42l42_irq_thread(int irq, void *data)
 	unsigned int i;
 	int report = 0;
 
+	mutex_lock(&cs42l42->irq_lock);
+
+	if (cs42l42->suspended) {
+		mutex_unlock(&cs42l42->irq_lock);
+		return IRQ_NONE;
+	}
 
 	/* Read sticky registers to clear interurpt */
 	for (i = 0; i < ARRAY_SIZE(stickies); i++) {
@@ -1635,9 +1645,12 @@ static irqreturn_t cs42l42_irq_thread(int irq, void *data)
 		CS42L42_M_DETECT_FT_MASK |
 		CS42L42_M_HSBIAS_HIZ_MASK);
 
-	mutex_lock(&cs42l42->jack_detect_mutex);
-
-	/* Check auto-detect status */
+	/*
+	 * Check auto-detect status. During system suspend the jack could be
+	 * unplugged and a different type plugged. On resume there will not
+	 * have been a previous unplug event to clear the reported flags, so
+	 * here the full state of all flags must be reported.
+	 */
 	if ((~masks[5]) & irq_params_table[5].mask) {
 		if (stickies[5] & CS42L42_HSDET_AUTO_DONE_MASK) {
 			cs42l42_process_hs_type_detect(cs42l42);
@@ -1645,11 +1658,15 @@ static irqreturn_t cs42l42_irq_thread(int irq, void *data)
 			case CS42L42_PLUG_CTIA:
 			case CS42L42_PLUG_OMTP:
 				snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADSET,
-						    SND_JACK_HEADSET);
+						    SND_JACK_HEADSET |
+						    SND_JACK_BTN_0 | SND_JACK_BTN_1 |
+						    SND_JACK_BTN_2 | SND_JACK_BTN_3);
 				break;
 			case CS42L42_PLUG_HEADPHONE:
 				snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADPHONE,
-						    SND_JACK_HEADPHONE);
+						    SND_JACK_HEADSET |
+						    SND_JACK_BTN_0 | SND_JACK_BTN_1 |
+						    SND_JACK_BTN_2 | SND_JACK_BTN_3);
 				break;
 			default:
 				break;
@@ -1705,7 +1722,7 @@ static irqreturn_t cs42l42_irq_thread(int irq, void *data)
 		}
 	}
 
-	mutex_unlock(&cs42l42->jack_detect_mutex);
+	mutex_unlock(&cs42l42->irq_lock);
 
 	return IRQ_HANDLED;
 }
@@ -2053,8 +2070,9 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client,
 		return -ENOMEM;
 
 	cs42l42->dev = &i2c_client->dev;
+	cs42l42->irq = i2c_client->irq;
 	i2c_set_clientdata(i2c_client, cs42l42);
-	mutex_init(&cs42l42->jack_detect_mutex);
+	mutex_init(&cs42l42->irq_lock);
 
 	cs42l42->regmap = devm_regmap_init_i2c(i2c_client, &cs42l42_regmap);
 	if (IS_ERR(cs42l42->regmap)) {
@@ -2210,6 +2228,179 @@ static int cs42l42_i2c_remove(struct i2c_client *i2c_client)
 	return 0;
 }
 
+/* Suspend sequence from datasheet */
+static const struct reg_sequence __maybe_unused cs42l42_suspend_seq[] = {
+	REG_SEQ0(CS42L42_MIC_DET_CTL1,		0x9f),
+	REG_SEQ0(CS42L42_ADC_OVFL_INT_MASK,	0x01),
+	REG_SEQ0(CS42L42_MIXER_INT_MASK,	0x0F),
+	REG_SEQ0(CS42L42_SRC_INT_MASK,		0x0F),
+	REG_SEQ0(CS42L42_ASP_RX_INT_MASK,	0x1F),
+	REG_SEQ0(CS42L42_ASP_TX_INT_MASK,	0x0F),
+	REG_SEQ0(CS42L42_CODEC_INT_MASK,	0x03),
+	REG_SEQ0(CS42L42_SRCPL_INT_MASK,	0x7F),
+	REG_SEQ0(CS42L42_VPMON_INT_MASK,	0x01),
+	REG_SEQ0(CS42L42_PLL_LOCK_INT_MASK,	0x01),
+	REG_SEQ0(CS42L42_TSRS_PLUG_INT_MASK,	0x0F),
+	REG_SEQ0(CS42L42_WAKE_CTL,		0xE1),
+	REG_SEQ0(CS42L42_DET_INT1_MASK,		0xE0),
+	REG_SEQ0(CS42L42_DET_INT2_MASK,		0xFF),
+	REG_SEQ0(CS42L42_MIXER_CHA_VOL,		0x3f),
+	REG_SEQ0(CS42L42_MIXER_ADC_VOL,		0x3f),
+	REG_SEQ0(CS42L42_MIXER_CHB_VOL,		0x3f),
+	REG_SEQ0(CS42L42_HP_CTL,		0x0f),
+	REG_SEQ0(CS42L42_ASP_RX_DAI0_EN,	0x00),
+	REG_SEQ0(CS42L42_ASP_CLK_CFG,		0x00),
+	REG_SEQ0(CS42L42_HSDET_CTL2,		0x00),
+	REG_SEQ0(CS42L42_PWR_CTL1,		0xfe),
+	REG_SEQ0(CS42L42_PWR_CTL2,		0x8c),
+	REG_SEQ0(CS42L42_DAC_CTL2,		0x02),
+	REG_SEQ0(CS42L42_HS_CLAMP_DISABLE,	0x00),
+	REG_SEQ0(CS42L42_MISC_DET_CTL,		0x03),
+	REG_SEQ0(CS42L42_TIPSENSE_CTL,		0x02),
+	REG_SEQ0(CS42L42_HSBIAS_SC_AUTOCTL,	0x03),
+	REG_SEQ0(CS42L42_PWR_CTL1,		0xff),
+};
+
+static const struct reg_sequence __maybe_unused cs42l42_suspend_post_seq[] = {
+	REG_SEQ0(CS42L42_PWR_CTL2,		0x9c),
+};
+
+static int __maybe_unused cs42l42_suspend(struct device *dev)
+{
+	struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
+	unsigned int reg;
+	int ret;
+
+	if (cs42l42->irq >= 0)
+		disable_irq(cs42l42->irq);
+
+	/*
+	 * Wait for threaded irq handler to be idle and stop it processing
+	 * future interrupts. This ensures a safe disable if the interrupt
+	 * is shared.
+	 */
+	mutex_lock(&cs42l42->irq_lock);
+	cs42l42->suspended = true;
+
+	/* Clear any previous PDN_DONE */
+	regmap_read(cs42l42->regmap, CS42L42_CODEC_STATUS, &reg);
+
+	/* Preserve cached values so they can be restored. */
+	regmap_multi_reg_write_bypassed(cs42l42->regmap,
+					cs42l42_suspend_seq,
+					ARRAY_SIZE(cs42l42_suspend_seq));
+
+	/* The cached address page register value is now stale */
+	regcache_drop_region(cs42l42->regmap, CS42L42_PAGE_REGISTER, CS42L42_PAGE_REGISTER);
+
+	/* Wait for power-down complete */
+	msleep(CS42L42_PDN_DONE_TIME_MS);
+	ret = regmap_read_poll_timeout(cs42l42->regmap, CS42L42_CODEC_STATUS, reg,
+				       (reg & CS42L42_PDN_DONE_MASK),
+				       CS42L42_PDN_DONE_POLL_US,
+				       CS42L42_PDN_DONE_TIMEOUT_US);
+	if (ret)
+		dev_warn(dev, "Failed to get PDN_DONE: %d\n", ret);
+
+	regmap_multi_reg_write_bypassed(cs42l42->regmap,
+					cs42l42_suspend_post_seq,
+					ARRAY_SIZE(cs42l42_suspend_post_seq));
+	msleep(CS42L42_FILT_DISCHARGE_TIME_MS);
+
+	/* The cached address page register value is now stale */
+	regcache_drop_region(cs42l42->regmap, CS42L42_PAGE_REGISTER, CS42L42_PAGE_REGISTER);
+
+	regcache_cache_only(cs42l42->regmap, true);
+
+	gpiod_set_value_cansleep(cs42l42->reset_gpio, 0);
+	regulator_bulk_disable(ARRAY_SIZE(cs42l42->supplies), cs42l42->supplies);
+
+	mutex_unlock(&cs42l42->irq_lock);
+
+	dev_dbg(dev, "System suspended\n");
+
+	return 0;
+}
+
+static int __maybe_unused cs42l42_resume(struct device *dev)
+{
+	struct cs42l42_private *cs42l42 = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	mutex_lock(&cs42l42->irq_lock);
+
+	/*
+	 * If jack was unplugged and re-plugged during suspend it could have
+	 * changed type but the tip-sense state hasn't changed. Force a
+	 * plugged state to be re-evaluated if it is still plugged.
+	 */
+	if (cs42l42->plug_state != CS42L42_TS_UNPLUG)
+		cs42l42->plug_state = CS42L42_TS_TRANS;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(cs42l42->supplies), cs42l42->supplies);
+	if (ret != 0) {
+		dev_err(dev, "Failed to enable supplies: %d\n", ret);
+		return ret;
+	}
+
+	gpiod_set_value_cansleep(cs42l42->reset_gpio, 1);
+	usleep_range(CS42L42_BOOT_TIME_US, CS42L42_BOOT_TIME_US * 2);
+
+	regcache_cache_only(cs42l42->regmap, false);
+
+	if (cs42l42->reset_gpio) {
+		/* Registers have reset to defaults */
+		regcache_mark_dirty(cs42l42->regmap);
+	} else {
+		/*
+		 * Only call regcache_mark_dirty() if cs42l42 reset, so
+		 * regcache_sync() writes all non-default cached values.
+		 * If it didn't reset we don't want to filter out syncing
+		 * dirty cache entries that have default value.
+		 * DISCHARGE_FILT==1 after suspend. If the cs42l42 reset
+		 * it will now be 0.
+		 */
+		ret = regmap_read(cs42l42->regmap, CS42L42_PWR_CTL2, &val);
+		if (ret) {
+			dev_err(dev, "failed to read PWR_CTL2: %d\n", ret);
+			goto err;
+		}
+
+		if ((val & CS42L42_DISCHARGE_FILT_MASK) == 0) {
+			dev_dbg(dev, "Has reset in suspend\n");
+			regcache_mark_dirty(cs42l42->regmap);
+		} else {
+			dev_dbg(dev, "Did not reset in suspend\n");
+		}
+	}
+
+	/* Sync LATCH_TO_VP first so the VP domain registers sync correctly */
+	regcache_sync_region(cs42l42->regmap, CS42L42_MIC_DET_CTL1, CS42L42_MIC_DET_CTL1);
+	regcache_sync(cs42l42->regmap);
+
+	cs42l42->suspended = false;
+	mutex_unlock(&cs42l42->irq_lock);
+
+	if (cs42l42->irq >= 0)
+		enable_irq(cs42l42->irq);
+
+	dev_dbg(dev, "System resumed\n");
+
+	return 0;
+err:
+	gpiod_set_value_cansleep(cs42l42->reset_gpio, 0);
+	regulator_bulk_disable(ARRAY_SIZE(cs42l42->supplies), cs42l42->supplies);
+
+	mutex_unlock(&cs42l42->irq_lock);
+
+	return ret;
+}
+
+static const struct dev_pm_ops cs42l42_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(cs42l42_suspend, cs42l42_resume)
+};
+
 #ifdef CONFIG_OF
 static const struct of_device_id cs42l42_of_match[] = {
 	{ .compatible = "cirrus,cs42l42", },
@@ -2236,6 +2427,7 @@ MODULE_DEVICE_TABLE(i2c, cs42l42_id);
 static struct i2c_driver cs42l42_i2c_driver = {
 	.driver = {
 		.name = "cs42l42",
+		.pm = &cs42l42_pm_ops,
 		.of_match_table = of_match_ptr(cs42l42_of_match),
 		.acpi_match_table = ACPI_PTR(cs42l42_acpi_match),
 		},
diff --git a/sound/soc/codecs/cs42l42.h b/sound/soc/codecs/cs42l42.h
index 9fff183dce8e..b497ca1618c2 100644
--- a/sound/soc/codecs/cs42l42.h
+++ b/sound/soc/codecs/cs42l42.h
@@ -826,6 +826,10 @@
 #define CS42L42_PLL_LOCK_POLL_US	250
 #define CS42L42_PLL_LOCK_TIMEOUT_US	1250
 #define CS42L42_HP_ADC_EN_TIME_US	20000
+#define CS42L42_PDN_DONE_POLL_US	1000
+#define CS42L42_PDN_DONE_TIMEOUT_US	200000
+#define CS42L42_PDN_DONE_TIME_MS	100
+#define CS42L42_FILT_DISCHARGE_TIME_MS	46
 
 static const char *const cs42l42_supply_names[CS42L42_NUM_SUPPLIES] = {
 	"VA",
@@ -842,7 +846,8 @@ struct  cs42l42_private {
 	struct gpio_desc *reset_gpio;
 	struct completion pdn_done;
 	struct snd_soc_jack *jack;
-	struct mutex jack_detect_mutex;
+	int irq;
+	struct mutex irq_lock;
 	int pll_config;
 	int bclk;
 	u32 sclk;
@@ -860,6 +865,7 @@ struct  cs42l42_private {
 	u8 hs_bias_sense_en;
 	u8 stream_use;
 	bool hp_adc_up_pending;
+	bool suspended;
 };
 
 #endif /* __CS42L42_H__ */
-- 
2.11.0

Re: [PATCH] ASoC: cs42l42: Implement system suspend

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2829 bytes --]

On Wed, Dec 01, 2021 at 03:36:48PM +0000, Richard Fitzgerald wrote:
> Add system suspend functions to handle clean power-down on suspend and
> restoring state on resume.
> 
> The jack state could change during suspend. Plug->unplug and unplug->plug
> are straightforward because this looks no different from any other plug
> state change. However, the jack could be unplugged and a different type

This fiddling about with the jack detect feels like it should be at
least one separate change, possibly multiple changes - the reporting of
button states feels like it should be a good cleanup/fix separately for
example.

> of jack plugged, and on resume the plug state would not have changed.
> Some code changes are needed to the jack handling so that on resume a
> plugged state will be re-evaluated instead of filtered out. In this case

It would be helpful to elaborate on what these code changes might be.

> +	/*
> +	 * PWR_CTL2 must be volatile so it can be used as a canary bit to
> +	 * detect a reset during system suspend.
> +	 */
> +	case CS42L42_PWR_CTL2:

This feels a bit hackish - is the cost of doing the cache sync really so
expensive that it's worth the effort of trying to skip it?

> +	if (cs42l42->suspended) {
> +		mutex_unlock(&cs42l42->irq_lock);
> +		return IRQ_NONE;
> +	}

Given that you're using disable_irq() to force the interrupt off (which
is a bit rude but often the best one can do) how might we be getting an
interrupt while suspended?  This seems to indicate an error condition.

>  			case CS42L42_PLUG_OMTP:
>  				snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADSET,
> -						    SND_JACK_HEADSET);
> +						    SND_JACK_HEADSET |
> +						    SND_JACK_BTN_0 | SND_JACK_BTN_1 |
> +						    SND_JACK_BTN_2 | SND_JACK_BTN_3);
>  				break;
>  			case CS42L42_PLUG_HEADPHONE:
>  				snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADPHONE,
> -						    SND_JACK_HEADPHONE);
> +						    SND_JACK_HEADSET |
> +						    SND_JACK_BTN_0 | SND_JACK_BTN_1 |
> +						    SND_JACK_BTN_2 | SND_JACK_BTN_3);

This unconditionally clears the button status - will something notice if
the buttons are pressed?

> +	} else {
> +		/*
> +		 * Only call regcache_mark_dirty() if cs42l42 reset, so
> +		 * regcache_sync() writes all non-default cached values.
> +		 * If it didn't reset we don't want to filter out syncing
> +		 * dirty cache entries that have default value.
> +		 * DISCHARGE_FILT==1 after suspend. If the cs42l42 reset
> +		 * it will now be 0.
> +		 */

Something needs to tell regmap that the cache is dirty otherwise it
won't sync anything, including the non-default register values?  There's
currently an assumption coded in there that the cache is dirty because
the device was reset and everything has default values.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: cs42l42: Implement system suspend

[Richard Fitzgerald]

On 01/12/2021 16:32, Mark Brown wrote:
> On Wed, Dec 01, 2021 at 03:36:48PM +0000, Richard Fitzgerald wrote:
>> Add system suspend functions to handle clean power-down on suspend and
>> restoring state on resume.
>>
>> The jack state could change during suspend. Plug->unplug and unplug->plug
>> are straightforward because this looks no different from any other plug
>> state change. However, the jack could be unplugged and a different type
> 
> This fiddling about with the jack detect feels like it should be at
> least one separate change, possibly multiple changes - the reporting of
> button states feels like it should be a good cleanup/fix separately for
> example.

I'll split them out.

> 
>> of jack plugged, and on resume the plug state would not have changed.
>> Some code changes are needed to the jack handling so that on resume a
>> plugged state will be re-evaluated instead of filtered out. In this case
> 
> It would be helpful to elaborate on what these code changes might be.
> 
>> +	/*
>> +	 * PWR_CTL2 must be volatile so it can be used as a canary bit to
>> +	 * detect a reset during system suspend.
>> +	 */
>> +	case CS42L42_PWR_CTL2:
> 
> This feels a bit hackish

I can't think of a better way of detecting whether the chip reset. If
we don't have control of the reset GPIO we can't force a reset. So it
depends whether power turned off (also whether it dropped below the
Vmin voltage at which a reset is triggered). The regulator framework
can't tell us if it really turned off power and on ACPI systems the
power is all controlled magically.

 >     - is the cost of doing the cache sync really so> expensive that 
it's worth the effort of trying to skip it?
> 

It's not cost, it's about getting the correct values in the registers.
If we call regcache_mark_dirty() it tells regmap that all the hardware
registers have reset to default. Then regcache_sync() will skip writing
cache values that are the register default value, assuming that the
register already has this value. If the chip didn't reset, the registers
could retain non-default values while the cache contains a dirty
default value, in that case the register needs updating to match the
cache.

I tried to persuade myself that a cache value couldn't possibly change
at any time from suspend() being called to resume disabling cache-only
so I could safely accept default values being skipped. But that
assumption made me very uncomfortable - I don't like leaving potential
bugs in when its simple enough to prevent them.

>> +	if (cs42l42->suspended) {
>> +		mutex_unlock(&cs42l42->irq_lock);
>> +		return IRQ_NONE;
>> +	}
> 
> Given that you're using disable_irq() to force the interrupt off (which
> is a bit rude but often the best one can do) how might we be getting an
> interrupt while suspended?  This seems to indicate an error condition.
> 

I may have misunderstood here, but the documentation says that
enables/disables are nested. The interrupt starts out enabled right
after calling request_threaded_irq(), so I expected that all users of
the shared interrupt would have to call disable_irq() for it to actually
get disabled (I put the call in on that basis that it does no harm). If
disable_irq() forces the irq off even if it's shared then I'll remove it
because as you say that would be rude.

>>   			case CS42L42_PLUG_OMTP:
>>   				snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADSET,
>> -						    SND_JACK_HEADSET);
>> +						    SND_JACK_HEADSET |
>> +						    SND_JACK_BTN_0 | SND_JACK_BTN_1 |
>> +						    SND_JACK_BTN_2 | SND_JACK_BTN_3);
>>   				break;
>>   			case CS42L42_PLUG_HEADPHONE:
>>   				snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADPHONE,
>> -						    SND_JACK_HEADPHONE);
>> +						    SND_JACK_HEADSET |
>> +						    SND_JACK_BTN_0 | SND_JACK_BTN_1 |
>> +						    SND_JACK_BTN_2 | SND_JACK_BTN_3);
> 
> This unconditionally clears the button status - will something notice if
> the buttons are pressed?
> 
>> +	} else {
>> +		/*
>> +		 * Only call regcache_mark_dirty() if cs42l42 reset, so
>> +		 * regcache_sync() writes all non-default cached values.
>> +		 * If it didn't reset we don't want to filter out syncing
>> +		 * dirty cache entries that have default value.
>> +		 * DISCHARGE_FILT==1 after suspend. If the cs42l42 reset
>> +		 * it will now be 0.
>> +		 */
> 
> Something needs to tell regmap that the cache is dirty otherwise it

Regmap knows if it has dirty values that it hasn't written out to the
hardware.

> won't sync anything, including the non-default register values?  There's

That's fine. If the registers have not been reset they still have the
value of every clean cache entry. Every dirty cache entry will be
written out by regcache_sync().

> currently an assumption coded in there that the cache is dirty because
> the device was reset and everything has default values.
> 

Regmap only assumes that if regcache_mark_dirty() is called.

Re: [PATCH] ASoC: cs42l42: Implement system suspend

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 7233 bytes --]

On Wed, Dec 01, 2021 at 06:04:00PM +0000, Richard Fitzgerald wrote:
> On 01/12/2021 16:32, Mark Brown wrote:
> > On Wed, Dec 01, 2021 at 03:36:48PM +0000, Richard Fitzgerald wrote:

> > > +	/*
> > > +	 * PWR_CTL2 must be volatile so it can be used as a canary bit to
> > > +	 * detect a reset during system suspend.
> > > +	 */
> > > +	case CS42L42_PWR_CTL2:

> > This feels a bit hackish

> I can't think of a better way of detecting whether the chip reset. If
> we don't have control of the reset GPIO we can't force a reset. So it
> depends whether power turned off (also whether it dropped below the
> Vmin voltage at which a reset is triggered). The regulator framework
> can't tell us if it really turned off power and on ACPI systems the
> power is all controlled magically.

Right, hence the second part of the question - the general thing for
drivers has just been to assume that the power might've gone out over
suspend and behave as though it did unless we were using the device as a
wake source.  We could if required extend the existing regulator API
notification stuff to generate notifications, though they'd not work
when it's compiled out.

> >     - is the cost of doing the cache sync really so> expensive that it's
> worth the effort of trying to skip it?

> It's not cost, it's about getting the correct values in the registers.
> If we call regcache_mark_dirty() it tells regmap that all the hardware
> registers have reset to default. Then regcache_sync() will skip writing
> cache values that are the register default value, assuming that the
> register already has this value. If the chip didn't reset, the registers
> could retain non-default values while the cache contains a dirty
> default value, in that case the register needs updating to match the
> cache.

(note BTW that at the point I queried the performance thing I'd not got
as far as the magic bypassed write sequences)

So this is clearly a being done at the wrong level - it is needlessly
complex, non-idiomatic and making fragile and non-obvious assumptions
about how the cache code operates.  The issue you've got is that the
cache code is presenting interfaces for the common case where you'd only
want to resync the cache in cases where the device has been reset but
you've added code which bypasses the cache and pulls the device out of
sync with the cache.  You need to teach regmap about that, not try to
hack around things in the driver code.  But...

> I tried to persuade myself that a cache value couldn't possibly change
> at any time from suspend() being called to resume disabling cache-only
> so I could safely accept default values being skipped. But that
> assumption made me very uncomfortable - I don't like leaving potential
> bugs in when its simple enough to prevent them.

...that's based on the assumption that this is all about the magic write
sequences you're using for shutdown potentially conflicting with default
values you've got in the cache?  If it's not those then the assumption
is that either the device was reset in which case it has reset values or
the device was not reset and held it's previous value, in which case the
cache sync is redundant.  If we don't trust the device to reset cleanly
for some reason then it's probably a bad idea to tell regmap about
default values in the first place since even on initial boot we might
actually be doing a soft reboot and the device could be holding values
from whatever was running beforehand.

This clearly isn't simple, and other than those shutdown sequences or
potential issues with the device not resetting cleanly this should be
done by extending regmap so it explicitly knows what's going on.  If it
is those shutdown sequences then you're probably looking for something
like doing a _sync_region() on the registers the sequences touch.
Possibly a _sync_region() variant that writes everything requested, or
just make sync_region() not skip defaults.  Or just remove all the
defaults from the driver, the sync will be a bit more expensive but
that's hopefully fine.  If it's not those shutdown sequences it should
still be handled genericly but I'd really like to understand the
scenario you're concerned about here, especially the fact that any issue
will show up in this single register that the code is checking.

> > > +	if (cs42l42->suspended) {
> > > +		mutex_unlock(&cs42l42->irq_lock);
> > > +		return IRQ_NONE;
> > > +	}

> > Given that you're using disable_irq() to force the interrupt off (which
> > is a bit rude but often the best one can do) how might we be getting an
> > interrupt while suspended?  This seems to indicate an error condition.

> I may have misunderstood here, but the documentation says that
> enables/disables are nested. The interrupt starts out enabled right
> after calling request_threaded_irq(), so I expected that all users of
> the shared interrupt would have to call disable_irq() for it to actually
> get disabled (I put the call in on that basis that it does no harm). If
> disable_irq() forces the irq off even if it's shared then I'll remove it
> because as you say that would be rude.

Hrm, that may have changed since I last looked - if that's the case then
I guess it's best not to warn which was what I was thinking here.

> > > +		/*
> > > +		 * Only call regcache_mark_dirty() if cs42l42 reset, so
> > > +		 * regcache_sync() writes all non-default cached values.
> > > +		 * If it didn't reset we don't want to filter out syncing
> > > +		 * dirty cache entries that have default value.
> > > +		 * DISCHARGE_FILT==1 after suspend. If the cs42l42 reset
> > > +		 * it will now be 0.
> > > +		 */

> > Something needs to tell regmap that the cache is dirty otherwise it

> Regmap knows if it has dirty values that it hasn't written out to the
> hardware.

Well, apparently it doesn't since otherwise we could just do a resync.

> > won't sync anything, including the non-default register values?  There's

> That's fine. If the registers have not been reset they still have the
> value of every clean cache entry. Every dirty cache entry will be
> written out by regcache_sync().

What about the shutdown sequence - does that not touch cached registers?

> > currently an assumption coded in there that the cache is dirty because
> > the device was reset and everything has default values.

> Regmap only assumes that if regcache_mark_dirty() is called.

Right, but the point here is that coming out of suspend the driver is
expected to be unconditionally marking the cache as dirty and doing a
resync since we probably lost power.  The code is trying to avoid that
for reasons that are not at all apparent but I *think* are due to the
bypassed writes in shutdown(), especially if you say it's not a
performance thing.  We shouldn't need to worry about there being non
default states in the hardware.

The whole thing is just really confusing as it stands.  Like I say I
think this needs to express what it's trying to do clearly in both the
regmap operations it's doing and the code in general, at the minute it's
not clear looking at things what the goal is.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: cs42l42: Implement system suspend

[Charles Keepax]

On Wed, Dec 01, 2021 at 10:08:26PM +0000, Mark Brown wrote:
> On Wed, Dec 01, 2021 at 06:04:00PM +0000, Richard Fitzgerald wrote:
> > On 01/12/2021 16:32, Mark Brown wrote:
> > > On Wed, Dec 01, 2021 at 03:36:48PM +0000, Richard Fitzgerald wrote:
> ...that's based on the assumption that this is all about the magic write
> sequences you're using for shutdown potentially conflicting with default
> values you've got in the cache?  If it's not those then the assumption
> is that either the device was reset in which case it has reset values or
> the device was not reset and held it's previous value, in which case the
> cache sync is redundant.

This isn't quite accurate though, as whilst the device was
suspended the user may have touched ALSA controls which will have
updated the state of the cache. Thus the cache requires a sync
regardless of if the hardware turned off.

I suspect we do need to have a think about the write sequences,
but there is also a more general issue here. The sequence looks
like this:

1) Device enters cache only mode.
2) User writes an ALSA control causing a register to return to
its default value.
3) Device exits cache only and does a cache sync.

This flow works if the device was reset but not if the registers
retained their value since the register written by the user will
be at default in the cache, but not on the hardware. The cache
sync code assumes the device returned to defaults.

It is often tricky to know if the device reset since the
regulator could be shared and even if they arn't capacitance can
play a part if the off time is very small. Usually we mandate a
hard reset or use a soft reset if the hard isn't available. I
think the soft reset has some issues on this chip but perhaps we
should look into that more.

> > > Given that you're using disable_irq() to force the interrupt off (which
> > > is a bit rude but often the best one can do) how might we be getting an
> > > interrupt while suspended?  This seems to indicate an error condition.
> 
> > I may have misunderstood here, but the documentation says that
> > enables/disables are nested. The interrupt starts out enabled right
> > after calling request_threaded_irq(), so I expected that all users of
> > the shared interrupt would have to call disable_irq() for it to actually
> > get disabled (I put the call in on that basis that it does no harm). If
> > disable_irq() forces the irq off even if it's shared then I'll remove it
> > because as you say that would be rude.
> 
> Hrm, that may have changed since I last looked - if that's the case then
> I guess it's best not to warn which was what I was thinking here.
> 

No I am pretty sure you are correct here calling disable_irq will
immediately disable the IRQ for everyone using it. The reference
counting is on the other side, ie. if I call disable_irq 5 times
I have to call enable_irq 5 times before it turns back on.

Thanks,
Charles

Re: [PATCH] ASoC: cs42l42: Implement system suspend

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

On Thu, Dec 02, 2021 at 09:53:33AM +0000, Charles Keepax wrote:
> On Wed, Dec 01, 2021 at 10:08:26PM +0000, Mark Brown wrote:

> > ...that's based on the assumption that this is all about the magic write
> > sequences you're using for shutdown potentially conflicting with default
> > values you've got in the cache?  If it's not those then the assumption
> > is that either the device was reset in which case it has reset values or
> > the device was not reset and held it's previous value, in which case the
> > cache sync is redundant.

> This isn't quite accurate though, as whilst the device was
> suspended the user may have touched ALSA controls which will have

> updated the state of the cache. Thus the cache requires a sync
> regardless of if the hardware turned off.

Right, an actual description of an actual issue.  Though how would they
have touched the ALSA controls during system suspend?  Userspace should
halted before we start suspending devices so there shouldn't be anything
new coming in from the application layer while the device is in cache
only mode.  The sound card as a whole should've been suspended first so
nothing should be coming in from there either.

> I suspect we do need to have a think about the write sequences,
> but there is also a more general issue here. The sequence looks
> like this:

> 1) Device enters cache only mode.
> 2) User writes an ALSA control causing a register to return to
> its default value.
> 3) Device exits cache only and does a cache sync.

This is a thing that happens for runtime suspend but for runtime suspend
we have good tracking of if the device lost power so we shouldn't just
be marking the cache as dirty unconditionally.  For system suspend there
shouldn't be a need to worry about userspace changing anything, and
anything coming in from the rest of the kernel should be very limited.

In any case this isn't something that should be hacked around in an
individual driver, like I say whatever the driver is trying to do needs
to be written in a clear and obvious fashion.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]