From a1e2ec72102d623a9000b28356f56ca031cdcbbd Mon Sep 17 00:00:00 2001 From: Seppo Ingalsuo Date: Tue, 8 Jun 2021 14:02:05 +0300 Subject: [PATCH 1/3] Drivers: Intel: DMIC: Change active_fifos_mask to uint32_t This patch does not change any functionality. The mask is no more signed value and cache_to_uncache() needs a multiple of 32 bit type. Signed-off-by: Seppo Ingalsuo --- src/drivers/intel/dmic.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/drivers/intel/dmic.c b/src/drivers/intel/dmic.c index 70fe5a08528a..0715b4a97efd 100644 --- a/src/drivers/intel/dmic.c +++ b/src/drivers/intel/dmic.c @@ -136,7 +136,7 @@ static const uint32_t coef_base_b[4] = {PDM0_COEFFICIENT_B, PDM1_COEFFICIENT_B, /* Global configuration request for DMIC, need to use uncached address to access them */ static SHARED_DATA struct sof_ipc_dai_dmic_params *dmic_prm[DMIC_HW_FIFOS]; -static SHARED_DATA int dmic_active_fifos_mask; +static SHARED_DATA uint32_t dmic_active_fifos_mask; /* this ramps volume changes over time */ static enum task_state dmic_work(void *data) @@ -1292,7 +1292,7 @@ static int dmic_set_config(struct dai *dai, struct ipc_config_dai *common_config static void dmic_start(struct dai *dai) { struct sof_ipc_dai_dmic_params **uncached_dmic_prm = cache_to_uncache(&dmic_prm[0]); - int *uncached_dmic_active_fifos_mask = cache_to_uncache(&dmic_active_fifos_mask); + uint32_t *uncached_dmic_active_fifos_mask = cache_to_uncache(&dmic_active_fifos_mask); struct dmic_pdata *dmic = dai_get_drvdata(dai); int i; int mic_a; @@ -1442,7 +1442,7 @@ static void dmic_stop_fifo_packers(struct dai *dai, int fifo_index) static void dmic_stop(struct dai *dai) { struct dmic_pdata *dmic = dai_get_drvdata(dai); - int *uncached_dmic_active_fifos_mask = cache_to_uncache(&dmic_active_fifos_mask); + uint32_t *uncached_dmic_active_fifos_mask = cache_to_uncache(&dmic_active_fifos_mask); int i; dai_dbg(dai, "dmic_stop()"); @@ -1633,7 +1633,7 @@ static int dmic_probe(struct dai *dai) static int dmic_remove(struct dai *dai) { struct sof_ipc_dai_dmic_params **uncached_dmic_prm = cache_to_uncache(&dmic_prm[0]); - int uncached_dmic_active_fifos_mask = *cache_to_uncache(&dmic_active_fifos_mask); + uint32_t uncached_dmic_active_fifos_mask = *cache_to_uncache(&dmic_active_fifos_mask); struct dmic_pdata *dmic = dai_get_drvdata(dai); int i; From 1955ec3a108e8a2951e9f248b45abdf7f3cf9a1a Mon Sep 17 00:00:00 2001 From: Seppo Ingalsuo Date: Tue, 8 Jun 2021 14:13:21 +0300 Subject: [PATCH 2/3] Drivers: Intel: DMIC: Fix FW panic caused by too eager dmic_remove() This patch fixes an FW panic that could be triggered by multiple DMIC DAI capture when pausing, stopping, and resuming. A sequence like this crashed in last operation start capture on dmic0, dmic1 pause 0 stop 1 resume 0 It was caused by dmic_remove() for dmic1 that issued pm_runtime_put_sync() for DMIC clock and power, and freed the dmic_prm[] that resume for 0 needed. commit 282fe224d83b ("dmic: fix pause/release error leading to invalid dmic_active_fifos") solved the panic but left the HW start/stop sequence details incorrect. commit 73e6a6fab19e ("Driver: Intel: DMIC: Remove state check from active FIFO mask update") re-introduced the FW panic issue in multiple DAI capture. This should be a proper quick fix for pause handling. A successive patch will clean up the DMIC component data and do this in a nicer looking way. Signed-off-by: Seppo Ingalsuo --- src/drivers/intel/dmic.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/drivers/intel/dmic.c b/src/drivers/intel/dmic.c index 0715b4a97efd..538bf0d03f0a 100644 --- a/src/drivers/intel/dmic.c +++ b/src/drivers/intel/dmic.c @@ -137,6 +137,7 @@ static const uint32_t coef_base_b[4] = {PDM0_COEFFICIENT_B, PDM1_COEFFICIENT_B, /* Global configuration request for DMIC, need to use uncached address to access them */ static SHARED_DATA struct sof_ipc_dai_dmic_params *dmic_prm[DMIC_HW_FIFOS]; static SHARED_DATA uint32_t dmic_active_fifos_mask; +static SHARED_DATA uint32_t dmic_pause_mask; /* this ramps volume changes over time */ static enum task_state dmic_work(void *data) @@ -1293,6 +1294,7 @@ static void dmic_start(struct dai *dai) { struct sof_ipc_dai_dmic_params **uncached_dmic_prm = cache_to_uncache(&dmic_prm[0]); uint32_t *uncached_dmic_active_fifos_mask = cache_to_uncache(&dmic_active_fifos_mask); + uint32_t *uncached_dmic_pause_mask = cache_to_uncache(&dmic_pause_mask); struct dmic_pdata *dmic = dai_get_drvdata(dai); int i; int mic_a; @@ -1401,8 +1403,9 @@ static void dmic_start(struct dai *dai) CIC_CONTROL_SOFT_RESET_BIT, 0); } - /* Set bit dai->index */ + /* Set bit dai->index for active FIFO, and clear pause bit */ *uncached_dmic_active_fifos_mask |= BIT(dai->index); + *uncached_dmic_pause_mask &= ~BIT(dai->index); dmic->state = COMP_STATE_ACTIVE; spin_unlock(&dai->lock); @@ -1439,10 +1442,11 @@ static void dmic_stop_fifo_packers(struct dai *dai, int fifo_index) } /* stop the DMIC for capture */ -static void dmic_stop(struct dai *dai) +static void dmic_stop(struct dai *dai, bool stop_is_pause) { struct dmic_pdata *dmic = dai_get_drvdata(dai); uint32_t *uncached_dmic_active_fifos_mask = cache_to_uncache(&dmic_active_fifos_mask); + uint32_t *uncached_dmic_pause_mask = cache_to_uncache(&dmic_pause_mask); int i; dai_dbg(dai, "dmic_stop()"); @@ -1455,8 +1459,14 @@ static void dmic_stop(struct dai *dai) dai_info(dai, "dmic_stop(), dmic_active_fifos_mask = 0x%x", *uncached_dmic_active_fifos_mask); - /* Clear bit dai->index */ + /* Clear bit dai->index for active FIFO. If stop for pause, set pause mask bit. + * If stop is not for pausing, it is safe to clear the pause bit. + */ *uncached_dmic_active_fifos_mask &= ~BIT(dai->index); + if (stop_is_pause) + *uncached_dmic_pause_mask |= BIT(dai->index); + else + *uncached_dmic_pause_mask &= ~BIT(dai->index); for (i = 0; i < DMIC_HW_CONTROLLERS; i++) { /* Don't stop CIC yet if one FIFO remains active */ @@ -1529,11 +1539,11 @@ static int dmic_trigger(struct dai *dai, int cmd, int direction) break; case COMP_TRIGGER_STOP: dmic->state = COMP_STATE_PREPARE; - dmic_stop(dai); + dmic_stop(dai, false); break; case COMP_TRIGGER_PAUSE: dmic->state = COMP_STATE_PAUSED; - dmic_stop(dai); + dmic_stop(dai, true); break; case COMP_TRIGGER_RESUME: dmic_context_restore(dai); @@ -1634,6 +1644,7 @@ static int dmic_remove(struct dai *dai) { struct sof_ipc_dai_dmic_params **uncached_dmic_prm = cache_to_uncache(&dmic_prm[0]); uint32_t uncached_dmic_active_fifos_mask = *cache_to_uncache(&dmic_active_fifos_mask); + uint32_t uncached_dmic_pause_mask = *cache_to_uncache(&dmic_pause_mask); struct dmic_pdata *dmic = dai_get_drvdata(dai); int i; @@ -1649,9 +1660,9 @@ static int dmic_remove(struct dai *dai) interrupt_unregister(dmic->irq, dai); /* The next end tasks must be passed if another DAI FIFO still runs */ - dai_info(dai, "dmic_remove(), dmic_active_fifos_mask = 0x%x", - uncached_dmic_active_fifos_mask); - if (uncached_dmic_active_fifos_mask) + dai_info(dai, "dmic_remove(), dmic_active_fifos_mask = 0x%x, dmic_pause_mask = 0x%x", + uncached_dmic_active_fifos_mask, uncached_dmic_pause_mask); + if (uncached_dmic_active_fifos_mask || uncached_dmic_pause_mask) return 0; pm_runtime_put_sync(DMIC_CLK, dai->index); From 96e144e8972a993cada512a068ffb79cf3611e5c Mon Sep 17 00:00:00 2001 From: Seppo Ingalsuo Date: Tue, 8 Jun 2021 19:40:05 +0300 Subject: [PATCH 3/3] Drivers: Intel: DMIC: Spinlocks notes add This patch adds comments to code parts about spinlocks applied elsewhere to avoid confusion. Signed-off-by: Seppo Ingalsuo --- src/drivers/intel/dmic.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/drivers/intel/dmic.c b/src/drivers/intel/dmic.c index 538bf0d03f0a..b0a77f513d90 100644 --- a/src/drivers/intel/dmic.c +++ b/src/drivers/intel/dmic.c @@ -914,6 +914,9 @@ static int configure_registers(struct dai *dai, return ret; } + /* Note about accessing dmic_active_fifos_mask: the dai spinlock has been set in + * the calling function dmic_set_config(). + */ for (i = 0; i < DMIC_HW_CONTROLLERS; i++) { if (uncached_dmic_active_fifos_mask == 0) { /* CIC */ @@ -1659,7 +1662,10 @@ static int dmic_remove(struct dai *dai) interrupt_disable(dmic->irq, dai); interrupt_unregister(dmic->irq, dai); - /* The next end tasks must be passed if another DAI FIFO still runs */ + /* The next end tasks must be passed if another DAI FIFO still runs. + * Note: dai_put() function that calls remove() applies the spinlock + * so it is not needed here to protect access to mask bits. + */ dai_info(dai, "dmic_remove(), dmic_active_fifos_mask = 0x%x, dmic_pause_mask = 0x%x", uncached_dmic_active_fifos_mask, uncached_dmic_pause_mask); if (uncached_dmic_active_fifos_mask || uncached_dmic_pause_mask)