Re: [PATCH 12/12] ASoC: cs42l42: Add support for Soundwire interrupts

On 22/08/2022 12:33, Pierre-Louis Bossart wrote:

On 8/19/22 14:52, Richard Fitzgerald wrote:

...

This adds support for using the Soundwire interrupt mechanism to handle CS42L42 chip interrupts.

Soundwire interrupts are used if a hard INT line is not declared.

This sounds really weird.

The register access would still use the SoundWire read/writes, which raises a number of opens since the interrupt cannot be handled until the bus is resumed/operational, so there would be no speed-up at all. Unless

I didn't say it was for speed. The cs42l42 has various quirks. One is that the interrupt controller can still assert hard INT while in Soundwire clock-stop but cannot trigger a Soundwire wake. We could remove INT support from this first submission if you prefer and add it when/if necessary.

I completely missed something, this seems like wasting one pin with no benefits?

The pin has no other function than INT so it's there whether you use it or not.

...

Wake-from-clock-stop is not used. The CS42L42 has limited wake capability, but clock-stop is already disabled when a snd_soc_jack is registered to prevent the host controller issuing a bus-reset on exit from clock stop mode, which would clear the interrupt status and break jack and button detection.

Same open as in previous patch on why this is needed.

As said, the CS42L42 has limited ability to issue a wake from clock stop and even with its limited capability things won't work well if it gets bus-reset before the source can be handled.

...

Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com

sound/soc/codecs/cs42l42-sdw.c | 90 +++++++++++++++++++++++++++++++++- sound/soc/codecs/cs42l42.h | 3 ++ 2 files changed, 92 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/cs42l42-sdw.c b/sound/soc/codecs/cs42l42-sdw.c index ed69a0a44d8c..1bdeed93587d 100644 --- a/sound/soc/codecs/cs42l42-sdw.c +++ b/sound/soc/codecs/cs42l42-sdw.c @@ -14,6 +14,7 @@ #include <linux/soundwire/sdw.h> #include <linux/soundwire/sdw_registers.h> #include <linux/soundwire/sdw_type.h> +#include <linux/workqueue.h> #include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/soc.h> @@ -26,6 +27,8 @@ /* Register addresses are offset when sent over Soundwire */ #define CS42L42_SDW_ADDR_OFFSET 0x8000

+#define CS42L42_SDW_GEN_INT_STATUS_1 0xc0 +#define CS42L42_SDW_GEN_INT_MASK_1 0xc1 #define CS42L42_SDW_MEM_ACCESS_STATUS 0xd0 #define CS42L42_SDW_MEM_READ_DATA 0xd8

@@ -33,6 +36,11 @@ #define CS42L42_SDW_CMD_IN_PROGRESS BIT(2) #define CS42L42_SDW_RDATA_RDY BIT(0)

+#define CS42L42_SDW_M_SCP_IMP_DEF1 BIT(0) +#define CS42L42_GEN_INT_CASCADE SDW_SCP_INT1_IMPL_DEF

+#define CS42L42_SDW_INT_MASK_CODEC_IRQ BIT(0)

• #define CS42L42_DELAYED_READ_POLL_US 1 #define CS42L42_DELAYED_READ_TIMEOUT_US 100

@@ -306,6 +314,13 @@ static void cs42l42_sdw_init(struct sdw_slave *peripheral) /* Disable internal logic that makes clock-stop conditional */ regmap_clear_bits(cs42l42->regmap, CS42L42_PWR_CTL3, CS42L42_SW_CLK_STP_STAT_SEL_MASK);

• /* Enable Soundwire interrupts */
• if (!cs42l42->irq) {

• dev_dbg(cs42l42->dev, "Using Soundwire interrupts\n");
  

• sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_MASK_1,
  

• 		CS42L42_SDW_INT_MASK_CODEC_IRQ);
  

• }
• /*
  • pm_runtime is needed to control bus manager suspend, and to
  • recover from an unattach_request when the manager suspends.

@@ -319,6 +334,49 @@ static void cs42l42_sdw_init(struct sdw_slave *peripheral) pm_runtime_idle(cs42l42->dev); }

+static int cs42l42_sdw_interrupt(struct sdw_slave *peripheral,

• 		 struct sdw_slave_intr_status *status)
  

+{

• struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);
• /* Soundwire core holds our pm_runtime when calling this function. */
• dev_dbg(cs42l42->dev, "int control_port=0x%x\n", status->control_port);
• if ((status->control_port & CS42L42_GEN_INT_CASCADE) == 0)

• return 0;
  

• /*

• * Clear and mask until it has been handled. The read of GEN_INT_STATUS_1
  

• * is required as per the Soundwire spec for interrupt status bits to clear.
  

Humm, this explanation is not very clear. What part of the spec are you referring to?

Section 11.1.2 "Interrupt Model" says that a read is necessary to make sure a condition is not missed while clearing the status with successful write. You need to write the status to clear, and re-read the status to see if another condition remains. That's not how I understand the code below, which does the write and read in the opposite order.

The spec is 1. The status must be read before it can be cleared 2. It must then be written to clear it 3. Any new events triggered since the read will not be cleared by the write since there hasn't been another read, and will remain pending.

Thus the sequence is 1. Read INT_STATUS to ack the interrupts we've seen and enable clearing them. 2. Write INT_STATUS to clear the interrupts acked by the previous read. 3. New interrupts since the read of INT_STATUS will not clear so we don't lose them. However since we do a pre-step of masking the input sources to GEN_INT_STATUS_1 no new interrupt can actually be triggered, in this register - this prevents an ALERT storm while waiting for the work function to run and clear the interrupt sources.

(BTW these are custom registers, not SW spec registers, so their behavior is implementation-defined, although GEN_INT_STATUS_1 does implement the normal SW read-write-to-clear behavior.)

...

• */
  

• sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_MASK_1, 0);
• sdw_read_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1);
• sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1, 0xFF);
• queue_work(system_power_efficient_wq, &cs42l42->sdw_irq_work);
• /* Prevent host controller suspending before we handle the interrupt */
• pm_runtime_get_noresume(cs42l42->dev);
• return 0;

+}

+static void cs42l42_sdw_irq_work(struct work_struct *work) +{

• struct cs42l42_private *cs42l42 = container_of(work,

• 				       struct cs42l42_private,
  

• 				       sdw_irq_work);
  

• cs42l42_irq_thread(-1, cs42l42);
• /* unmask interrupt */
• if (!cs42l42->sdw_irq_no_unmask)

• sdw_write_no_pm(cs42l42->sdw_peripheral, CS42L42_SDW_GEN_INT_MASK_1,
  

• 		CS42L42_SDW_INT_MASK_CODEC_IRQ);
  

• pm_runtime_put_autosuspend(cs42l42->dev);

+}

• static int cs42l42_sdw_read_prop(struct sdw_slave *peripheral) { struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);

@@ -334,6 +392,14 @@ static int cs42l42_sdw_read_prop(struct sdw_slave *peripheral) prop->quirks = SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY; prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;

• /*

• * CS42L42 doesn't have a SDW_SCP_INT1_IMPL_DEF mask bit but it must be
  

• * set in scp_int1_mask else the Soundwire framework won't notify us
  

• * when the IMPL_DEF interrupt is asserted.
  

• */
  

• if (!cs42l42->irq)

• prop->scp_int1_mask |= SDW_SCP_INT1_IMPL_DEF;
  

Sorry, I don't follow the explanation. If you don't have a bit defined for a specific interrupt, how would that interrupt be handled?

The IMP_DEF interrupt is always unmasked. It does not have a mask bit.

...

/* DP1 - capture */ ports[0].num = CS42L42_SDW_CAPTURE_PORT, ports[0].type = SDW_DPN_FULL, @@ -403,6 +469,7 @@ static int __maybe_unused cs42l42_sdw_clk_stop(struct sdw_slave *peripheral,

static const struct sdw_slave_ops cs42l42_sdw_ops = { .read_prop = cs42l42_sdw_read_prop,

• .interrupt_callback = cs42l42_sdw_interrupt, .update_status = cs42l42_sdw_update_status, .bus_config = cs42l42_sdw_bus_config, #ifdef DEBUG

@@ -473,6 +540,11 @@ static int __maybe_unused cs42l42_sdw_runtime_resume(struct device *dev) regcache_sync_region(cs42l42->regmap, CS42L42_MIC_DET_CTL1, CS42L42_MIC_DET_CTL1); regcache_sync(cs42l42->regmap);

• /* Re-enable Soundwire interrupts */
• if (!cs42l42->irq)

• sdw_write_no_pm(cs42l42->sdw_peripheral, CS42L42_SDW_GEN_INT_MASK_1,
  

• 		CS42L42_SDW_INT_MASK_CODEC_IRQ);
  

• return 0; }

@@ -495,6 +567,11 @@ static int __maybe_unused cs42l42_sdw_resume(struct device *dev)

cs42l42_resume_restore(dev);

• /* Re-enable Soundwire interrupts */
• if (!cs42l42->irq)

• sdw_write_no_pm(cs42l42->sdw_peripheral, CS42L42_SDW_GEN_INT_MASK_1,
  

• 		CS42L42_SDW_INT_MASK_CODEC_IRQ);
  

that would prevent the device from waking up the system while in suspend? How would the resume be triggered then, only by the manager? That doesn't seem like a working model for a headset codec.

I don't understand your concern here. When you bus-reset our peripheral on exit from clock-stop it will reset the custom implementation-defined interrupt masks. The core code won't restore them because it doesn't know about them. So we have to restore them otherwise we will never get any more jack or button ALERTs from the CS42L42. This is no different from doing a regcache_sync() to restore registers.

This seems also weird since I don't see where the interrupts are disabled on suspend, so this 're-enable' does not have a clear 'disable' dual operation.

We don't want to disable them. We need to recover from an Intel forced bus-reset. It would be simpler if we didn't have to consider a mode 0 clock stop possibly resetting the peripheral, but as Intel does that we must handle everything the core code can't restore automatically.

...

return 0; }

@@ -546,6 +623,7 @@ static int cs42l42_sdw_probe(struct sdw_slave *peripheral, const struct sdw_devi component_drv->dapm_routes = cs42l42_sdw_audio_map; component_drv->num_dapm_routes = ARRAY_SIZE(cs42l42_sdw_audio_map);

• INIT_WORK(&cs42l42->sdw_irq_work, cs42l42_sdw_irq_work); cs42l42->dev = dev; cs42l42->regmap = regmap; cs42l42->sdw_peripheral = peripheral;

@@ -562,8 +640,18 @@ static int cs42l42_sdw_remove(struct sdw_slave *peripheral) { struct cs42l42_private *cs42l42 = dev_get_drvdata(&peripheral->dev);

• /* Resume so that cs42l42_remove() can access registers */

• /* Resume so that we can access registers */ pm_runtime_get_sync(cs42l42->dev);
• /* Disable Soundwire interrupts */
• if (!cs42l42->irq) {

• cs42l42->sdw_irq_no_unmask = true;
  

• cancel_work_sync(&cs42l42->sdw_irq_work);
  

• sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_MASK_1, 0);
  

• sdw_read_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1);
  

• sdw_write_no_pm(peripheral, CS42L42_SDW_GEN_INT_STATUS_1, 0xFF);
  

• }
• cs42l42_common_remove(cs42l42); pm_runtime_put(cs42l42->dev); pm_runtime_disable(cs42l42->dev);

diff --git a/sound/soc/codecs/cs42l42.h b/sound/soc/codecs/cs42l42.h index 038db45d95b3..b29126d218c4 100644 --- a/sound/soc/codecs/cs42l42.h +++ b/sound/soc/codecs/cs42l42.h @@ -19,6 +19,7 @@ #include <linux/regmap.h> #include <linux/regulator/consumer.h> #include <linux/soundwire/sdw.h> +#include <linux/workqueue.h> #include <sound/jack.h> #include <sound/cs42l42.h> #include <sound/soc-component.h> @@ -32,6 +33,7 @@ struct cs42l42_private { struct completion pdn_done; struct snd_soc_jack *jack; struct sdw_slave *sdw_peripheral;

• struct work_struct sdw_irq_work; struct mutex irq_lock; int irq; int pll_config;

@@ -52,6 +54,7 @@ struct cs42l42_private { bool hp_adc_up_pending; bool suspended; bool init_done;

• bool sdw_irq_no_unmask; };

  extern const struct regmap_config cs42l42_regmap;