[PATCH v2 0/4] ASoC: fsl: fsl_qmc_audio: Reduce amount of interrupts

[PATCH v2 0/4] ASoC: fsl: fsl_qmc_audio: Reduce amount of interrupts

[Christophe Leroy]

This series reduces significantly the amount of interrupts on
fsl_qmc_audio device.

Patches 1 and 2 are preparatory patches.
Patch 3 is the main change
Patch 4 is a cleanup which is enabled by previous patch

Changes in v2:
- Don't remove UB bit (Patch 1, comment from Herve Codina)
- Make sure audio channels are ordered on TDM bus (Patch 2, new patch, comment from Herve Codina)
- Drop struct qmc_dai_chan  (patch 4, new patch)

Backgroup (copied from patch 3):

In non-interleaved mode, several QMC channels are used in sync.
More details can be found in commit 188d9cae5438 ("ASoC: fsl:
fsl_qmc_audio: Add support for non-interleaved mode.")
At the time being, an interrupt is requested on each channel to
perform capture/playback completion, allthough the completion is
really performed only once all channels have completed their work.

This leads to a lot more interrupts than really needed. Looking at
/proc/interrupts shows ~3800 interrupts per second when using
4 capture and 4 playback devices with 5ms periods while
only 1600 (200 x 4 + 200 x 4) periods are processed during one second.

The QMC channels work in sync, the one started first is the one
finishing first and the one started last is the one finishing last,
so when the last one finishes it is guaranteed that the other ones are
finished as well. Therefore only request completion processing on the
last QMC channel.

On my board with the above exemple, on a kernel started with
'threadirqs' option, the QMC irq thread uses 16% CPU time with this
patch while it uses 26% CPU time without this patch.

Christophe Leroy (4):
  soc: fsl: qmc: Only set completion interrupt when needed
  ASoc: fsl: fsl_qmc_audio: Ensure audio channels are ordered in TDM bus
  ASoC: fsl: fsl_qmc_audio: Only request completion on last channel
  ASoc: fsl: fsl_qmc_audio: Drop struct qmc_dai_chan

 drivers/soc/fsl/qe/qmc.c      |  44 +++++++++---
 sound/soc/fsl/fsl_qmc_audio.c | 125 +++++++++++++++-------------------
 2 files changed, 87 insertions(+), 82 deletions(-)

-- 
2.49.0

[PATCH v2 1/4] soc: fsl: qmc: Only set completion interrupt when needed

[Christophe Leroy]

When no post-completion processing is expected, don't waste time
handling useless interrupts.

Only set QMC_BD_[R/T]X_I when a completion function is passed in,
and perform seamless completion on submit for interruptless buffers.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: Keep the UB flag to mark not completed buffers and seamlessly flag them as completed during next submit.
---
 drivers/soc/fsl/qe/qmc.c | 44 ++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/drivers/soc/fsl/qe/qmc.c b/drivers/soc/fsl/qe/qmc.c
index 36c0ccc06151f..8f76b9a5e385d 100644
--- a/drivers/soc/fsl/qe/qmc.c
+++ b/drivers/soc/fsl/qe/qmc.c
@@ -461,9 +461,16 @@ int qmc_chan_write_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
 
 	ctrl = qmc_read16(&bd->cbd_sc);
 	if (ctrl & (QMC_BD_TX_R | QMC_BD_TX_UB)) {
-		/* We are full ... */
-		ret = -EBUSY;
-		goto end;
+		if (!(ctrl & QMC_BD_TX_I) && bd == chan->txbd_done) {
+			if (ctrl & QMC_BD_TX_W)
+				chan->txbd_done = chan->txbds;
+			else
+				chan->txbd_done++;
+		} else {
+			/* We are full ... */
+			ret = -EBUSY;
+			goto end;
+		}
 	}
 
 	qmc_write16(&bd->cbd_datlen, length);
@@ -475,6 +482,10 @@ int qmc_chan_write_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
 
 	/* Activate the descriptor */
 	ctrl |= (QMC_BD_TX_R | QMC_BD_TX_UB);
+	if (complete)
+		ctrl |= QMC_BD_TX_I;
+	else
+		ctrl &= ~QMC_BD_TX_I;
 	wmb(); /* Be sure to flush the descriptor before control update */
 	qmc_write16(&bd->cbd_sc, ctrl);
 
@@ -569,9 +580,16 @@ int qmc_chan_read_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
 
 	ctrl = qmc_read16(&bd->cbd_sc);
 	if (ctrl & (QMC_BD_RX_E | QMC_BD_RX_UB)) {
-		/* We are full ... */
-		ret = -EBUSY;
-		goto end;
+		if (!(ctrl & QMC_BD_RX_I) && bd == chan->rxbd_done) {
+			if (ctrl & QMC_BD_RX_W)
+				chan->rxbd_done = chan->rxbds;
+			else
+				chan->rxbd_done++;
+		} else {
+			/* We are full ... */
+			ret = -EBUSY;
+			goto end;
+		}
 	}
 
 	qmc_write16(&bd->cbd_datlen, 0); /* data length is updated by the QMC */
@@ -587,6 +605,10 @@ int qmc_chan_read_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
 
 	/* Activate the descriptor */
 	ctrl |= (QMC_BD_RX_E | QMC_BD_RX_UB);
+	if (complete)
+		ctrl |= QMC_BD_RX_I;
+	else
+		ctrl &= ~QMC_BD_RX_I;
 	wmb(); /* Be sure to flush data before descriptor activation */
 	qmc_write16(&bd->cbd_sc, ctrl);
 
@@ -1482,19 +1504,19 @@ static int qmc_setup_chan(struct qmc *qmc, struct qmc_chan *chan)
 
 	/* Init Rx BDs and set Wrap bit on last descriptor */
 	BUILD_BUG_ON(QMC_NB_RXBDS == 0);
-	val = QMC_BD_RX_I;
 	for (i = 0; i < QMC_NB_RXBDS; i++) {
 		bd = chan->rxbds + i;
-		qmc_write16(&bd->cbd_sc, val);
+		qmc_write16(&bd->cbd_sc, 0);
 	}
 	bd = chan->rxbds + QMC_NB_RXBDS - 1;
-	qmc_write16(&bd->cbd_sc, val | QMC_BD_RX_W);
+	qmc_write16(&bd->cbd_sc, QMC_BD_RX_W);
 
 	/* Init Tx BDs and set Wrap bit on last descriptor */
 	BUILD_BUG_ON(QMC_NB_TXBDS == 0);
-	val = QMC_BD_TX_I;
 	if (chan->mode == QMC_HDLC)
-		val |= QMC_BD_TX_L | QMC_BD_TX_TC;
+		val = QMC_BD_TX_L | QMC_BD_TX_TC;
+	else
+		val = 0;
 	for (i = 0; i < QMC_NB_TXBDS; i++) {
 		bd = chan->txbds + i;
 		qmc_write16(&bd->cbd_sc, val);
-- 
2.49.0

[PATCH v2 2/4] ASoc: fsl: fsl_qmc_audio: Ensure audio channels are ordered in TDM bus

[Christophe Leroy]

To reduce complexity of interrupt handling in following patch, ensure
audio channels are configured in the same order as timeslots on the
TDM bus. If we need a given ordering of audio sources in the audio
frame, it is possible to re-order codecs on the TDM bus, no need to
mix up timeslots in channels.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: New
---
 sound/soc/fsl/fsl_qmc_audio.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/sound/soc/fsl/fsl_qmc_audio.c b/sound/soc/fsl/fsl_qmc_audio.c
index 5614a8b909edf..0be29ccc1ff7b 100644
--- a/sound/soc/fsl/fsl_qmc_audio.c
+++ b/sound/soc/fsl/fsl_qmc_audio.c
@@ -791,12 +791,17 @@ static int qmc_audio_dai_parse(struct qmc_audio *qmc_audio, struct device_node *
 			       struct qmc_dai *qmc_dai,
 			       struct snd_soc_dai_driver *qmc_soc_dai_driver)
 {
+	struct qmc_chan_ts_info ts_info;
 	struct qmc_chan_info info;
 	unsigned long rx_fs_rate;
 	unsigned long tx_fs_rate;
+	int prev_last_rx_ts = 0;
+	int prev_last_tx_ts = 0;
 	unsigned int nb_tx_ts;
 	unsigned int nb_rx_ts;
 	unsigned int i;
+	int last_rx_ts;
+	int last_tx_ts;
 	int count;
 	u32 val;
 	int ret;
@@ -879,6 +884,30 @@ static int qmc_audio_dai_parse(struct qmc_audio *qmc_audio, struct device_node *
 				return -EINVAL;
 			}
 		}
+
+		ret = qmc_chan_get_ts_info(qmc_dai->qmc_chans[i], &ts_info);
+		if (ret) {
+			dev_err(qmc_audio->dev, "dai %d get QMC %d channel TS info failed %d\n",
+				qmc_dai->id, i, ret);
+			return ret;
+		}
+
+		last_rx_ts = fls64(ts_info.rx_ts_mask);
+		last_tx_ts = fls64(ts_info.rx_ts_mask);
+
+		if (prev_last_rx_ts > last_rx_ts) {
+			dev_err(qmc_audio->dev, "dai %d QMC chan %d unordered channels (RX timeslot %lu before %lu)\n",
+				qmc_dai->id, i, prev_last_rx_ts, last_rx_ts);
+			return -EINVAL;
+		}
+		if (prev_last_tx_ts > last_tx_ts) {
+			dev_err(qmc_audio->dev, "dai %d QMC chan %d unordered channels (TX timeslot %lu before %lu)\n",
+				qmc_dai->id, i, prev_last_tx_ts, last_tx_ts);
+			return -EINVAL;
+		}
+
+		prev_last_rx_ts = last_rx_ts;
+		prev_last_tx_ts = last_tx_ts;
 	}
 
 	qmc_dai->nb_chans_avail = count;
-- 
2.49.0

[PATCH v2 3/4] ASoC: fsl: fsl_qmc_audio: Only request completion on last channel

[Christophe Leroy]

In non-interleaved mode, several QMC channels are used in sync.
More details can be found in commit 188d9cae5438 ("ASoC: fsl:
fsl_qmc_audio: Add support for non-interleaved mode.")
At the time being, an interrupt is requested on each channel to
perform capture/playback completion, allthough the completion is
really performed only once all channels have completed their work.

This leads to a lot more interrupts than really needed. Looking at
/proc/interrupts shows ~3800 interrupts per second when using
4 capture and 4 playback devices with 5ms periods while
only 1600 (200 x 4 + 200 x 4) periods are processed during one second.

The QMC channels work in sync, the one started first is the one
finishing first and the one started last is the one finishing last,
so when the last one finishes it is guaranteed that the other ones are
finished as well. Therefore only request completion processing on the
last QMC channel.

On my board with the above exemple, on a kernel started with
'threadirqs' option, the QMC irq thread uses 16% CPU time with this
patch while it uses 26% CPU time without this patch.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 sound/soc/fsl/fsl_qmc_audio.c | 46 +++++------------------------------
 1 file changed, 6 insertions(+), 40 deletions(-)

diff --git a/sound/soc/fsl/fsl_qmc_audio.c b/sound/soc/fsl/fsl_qmc_audio.c
index 0be29ccc1ff7b..92aa813aa3bee 100644
--- a/sound/soc/fsl/fsl_qmc_audio.c
+++ b/sound/soc/fsl/fsl_qmc_audio.c
@@ -57,7 +57,6 @@ struct qmc_dai_prtd {
 	size_t ch_dma_offset;
 
 	unsigned int channels;
-	DECLARE_BITMAP(chans_pending, 64);
 	struct snd_pcm_substream *substream;
 };
 
@@ -126,17 +125,14 @@ static int qmc_audio_pcm_write_submit(struct qmc_dai_prtd *prtd)
 	int ret;
 
 	for (i = 0; i < prtd->channels; i++) {
-		bitmap_set(prtd->chans_pending, i, 1);
-
 		ret = qmc_chan_write_submit(prtd->qmc_dai->chans[i].qmc_chan,
 					    prtd->ch_dma_addr_current + i * prtd->ch_dma_offset,
 					    prtd->ch_dma_size,
-					    qmc_audio_pcm_write_complete,
-					    &prtd->qmc_dai->chans[i]);
+					    i == prtd->channels - 1 ? qmc_audio_pcm_write_complete :
+								      NULL, prtd);
 		if (ret) {
 			dev_err(prtd->qmc_dai->dev, "write_submit %u failed %d\n",
 				i, ret);
-			bitmap_clear(prtd->chans_pending, i, 1);
 			return ret;
 		}
 	}
@@ -146,20 +142,7 @@ static int qmc_audio_pcm_write_submit(struct qmc_dai_prtd *prtd)
 
 static void qmc_audio_pcm_write_complete(void *context)
 {
-	struct qmc_dai_chan *chan = context;
-	struct qmc_dai_prtd *prtd;
-
-	prtd = chan->prtd_tx;
-
-	/* Mark the current channel as completed */
-	bitmap_clear(prtd->chans_pending, chan - prtd->qmc_dai->chans, 1);
-
-	/*
-	 * All QMC channels involved must have completed their transfer before
-	 * submitting a new one.
-	 */
-	if (!bitmap_empty(prtd->chans_pending, 64))
-		return;
+	struct qmc_dai_prtd *prtd = context;
 
 	prtd->buffer_ended += prtd->period_size;
 	if (prtd->buffer_ended >= prtd->buffer_size)
@@ -182,17 +165,14 @@ static int qmc_audio_pcm_read_submit(struct qmc_dai_prtd *prtd)
 	int ret;
 
 	for (i = 0; i < prtd->channels; i++) {
-		bitmap_set(prtd->chans_pending, i, 1);
-
 		ret = qmc_chan_read_submit(prtd->qmc_dai->chans[i].qmc_chan,
 					   prtd->ch_dma_addr_current + i * prtd->ch_dma_offset,
 					   prtd->ch_dma_size,
-					   qmc_audio_pcm_read_complete,
-					   &prtd->qmc_dai->chans[i]);
+					   i == prtd->channels - 1 ? qmc_audio_pcm_read_complete :
+								     NULL, prtd);
 		if (ret) {
 			dev_err(prtd->qmc_dai->dev, "read_submit %u failed %d\n",
 				i, ret);
-			bitmap_clear(prtd->chans_pending, i, 1);
 			return ret;
 		}
 	}
@@ -202,26 +182,13 @@ static int qmc_audio_pcm_read_submit(struct qmc_dai_prtd *prtd)
 
 static void qmc_audio_pcm_read_complete(void *context, size_t length, unsigned int flags)
 {
-	struct qmc_dai_chan *chan = context;
-	struct qmc_dai_prtd *prtd;
-
-	prtd = chan->prtd_rx;
-
-	/* Mark the current channel as completed */
-	bitmap_clear(prtd->chans_pending, chan - prtd->qmc_dai->chans, 1);
+	struct qmc_dai_prtd *prtd = context;
 
 	if (length != prtd->ch_dma_size) {
 		dev_err(prtd->qmc_dai->dev, "read complete length = %zu, exp %zu\n",
 			length, prtd->ch_dma_size);
 	}
 
-	/*
-	 * All QMC channels involved must have completed their transfer before
-	 * submitting a new one.
-	 */
-	if (!bitmap_empty(prtd->chans_pending, 64))
-		return;
-
 	prtd->buffer_ended += prtd->period_size;
 	if (prtd->buffer_ended >= prtd->buffer_size)
 		prtd->buffer_ended = 0;
@@ -249,7 +216,6 @@ static int qmc_audio_pcm_trigger(struct snd_soc_component *component,
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
-		bitmap_zero(prtd->chans_pending, 64);
 		prtd->buffer_ended = 0;
 		prtd->ch_dma_addr_current = prtd->ch_dma_addr_start;
 
-- 
2.49.0

[PATCH v2 4/4] ASoc: fsl: fsl_qmc_audio: Drop struct qmc_dai_chan

[Christophe Leroy]

prtd_tx and prtd_rx members are not used anymore and only qmc_chan
member remains so struct qmc_dai_chan has become pointless.

Use qmc_chan directly and drop struct qmc_dai_chan.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: New
---
 sound/soc/fsl/fsl_qmc_audio.c | 50 +++++++++++++----------------------
 1 file changed, 19 insertions(+), 31 deletions(-)

diff --git a/sound/soc/fsl/fsl_qmc_audio.c b/sound/soc/fsl/fsl_qmc_audio.c
index 92aa813aa3bee..6719da1c81f1d 100644
--- a/sound/soc/fsl/fsl_qmc_audio.c
+++ b/sound/soc/fsl/fsl_qmc_audio.c
@@ -17,12 +17,6 @@
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 
-struct qmc_dai_chan {
-	struct qmc_dai_prtd *prtd_tx;
-	struct qmc_dai_prtd *prtd_rx;
-	struct qmc_chan *qmc_chan;
-};
-
 struct qmc_dai {
 	char *name;
 	int id;
@@ -33,7 +27,7 @@ struct qmc_dai {
 	unsigned int nb_chans_avail;
 	unsigned int nb_chans_used_tx;
 	unsigned int nb_chans_used_rx;
-	struct qmc_dai_chan *chans;
+	struct qmc_chan **qmc_chans;
 };
 
 struct qmc_audio {
@@ -125,7 +119,7 @@ static int qmc_audio_pcm_write_submit(struct qmc_dai_prtd *prtd)
 	int ret;
 
 	for (i = 0; i < prtd->channels; i++) {
-		ret = qmc_chan_write_submit(prtd->qmc_dai->chans[i].qmc_chan,
+		ret = qmc_chan_write_submit(prtd->qmc_dai->qmc_chans[i],
 					    prtd->ch_dma_addr_current + i * prtd->ch_dma_offset,
 					    prtd->ch_dma_size,
 					    i == prtd->channels - 1 ? qmc_audio_pcm_write_complete :
@@ -165,7 +159,7 @@ static int qmc_audio_pcm_read_submit(struct qmc_dai_prtd *prtd)
 	int ret;
 
 	for (i = 0; i < prtd->channels; i++) {
-		ret = qmc_chan_read_submit(prtd->qmc_dai->chans[i].qmc_chan,
+		ret = qmc_chan_read_submit(prtd->qmc_dai->qmc_chans[i],
 					   prtd->ch_dma_addr_current + i * prtd->ch_dma_offset,
 					   prtd->ch_dma_size,
 					   i == prtd->channels - 1 ? qmc_audio_pcm_read_complete :
@@ -206,7 +200,6 @@ static int qmc_audio_pcm_trigger(struct snd_soc_component *component,
 				 struct snd_pcm_substream *substream, int cmd)
 {
 	struct qmc_dai_prtd *prtd = substream->runtime->private_data;
-	unsigned int i;
 	int ret;
 
 	if (!prtd->qmc_dai) {
@@ -220,9 +213,6 @@ static int qmc_audio_pcm_trigger(struct snd_soc_component *component,
 		prtd->ch_dma_addr_current = prtd->ch_dma_addr_start;
 
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-			for (i = 0; i < prtd->channels; i++)
-				prtd->qmc_dai->chans[i].prtd_tx = prtd;
-
 			/* Submit first chunk ... */
 			ret = qmc_audio_pcm_write_submit(prtd);
 			if (ret)
@@ -238,9 +228,6 @@ static int qmc_audio_pcm_trigger(struct snd_soc_component *component,
 			if (ret)
 				return ret;
 		} else {
-			for (i = 0; i < prtd->channels; i++)
-				prtd->qmc_dai->chans[i].prtd_rx = prtd;
-
 			/* Submit first chunk ... */
 			ret = qmc_audio_pcm_read_submit(prtd);
 			if (ret)
@@ -610,9 +597,9 @@ static int qmc_dai_hw_params(struct snd_pcm_substream *substream,
 		chan_param.mode = QMC_TRANSPARENT;
 		chan_param.transp.max_rx_buf_size = params_period_bytes(params) / nb_chans_used;
 		for (i = 0; i < nb_chans_used; i++) {
-			ret = qmc_chan_set_param(qmc_dai->chans[i].qmc_chan, &chan_param);
+			ret = qmc_chan_set_param(qmc_dai->qmc_chans[i], &chan_param);
 			if (ret) {
-				dev_err(dai->dev, "chans[%u], set param failed %d\n",
+				dev_err(dai->dev, "qmc_chans[%u], set param failed %d\n",
 					i, ret);
 				return ret;
 			}
@@ -654,7 +641,7 @@ static int qmc_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		for (i = 0; i < nb_chans_used; i++) {
-			ret = qmc_chan_start(qmc_dai->chans[i].qmc_chan, direction);
+			ret = qmc_chan_start(qmc_dai->qmc_chans[i], direction);
 			if (ret)
 				goto err_stop;
 		}
@@ -663,13 +650,13 @@ static int qmc_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_STOP:
 		/* Stop and reset all QMC channels and return the first error encountered */
 		for (i = 0; i < nb_chans_used; i++) {
-			ret_tmp = qmc_chan_stop(qmc_dai->chans[i].qmc_chan, direction);
+			ret_tmp = qmc_chan_stop(qmc_dai->qmc_chans[i], direction);
 			if (!ret)
 				ret = ret_tmp;
 			if (ret_tmp)
 				continue;
 
-			ret_tmp = qmc_chan_reset(qmc_dai->chans[i].qmc_chan, direction);
+			ret_tmp = qmc_chan_reset(qmc_dai->qmc_chans[i], direction);
 			if (!ret)
 				ret = ret_tmp;
 		}
@@ -681,7 +668,7 @@ static int qmc_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 		/* Stop all QMC channels and return the first error encountered */
 		for (i = 0; i < nb_chans_used; i++) {
-			ret_tmp = qmc_chan_stop(qmc_dai->chans[i].qmc_chan, direction);
+			ret_tmp = qmc_chan_stop(qmc_dai->qmc_chans[i], direction);
 			if (!ret)
 				ret = ret_tmp;
 		}
@@ -697,8 +684,8 @@ static int qmc_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 
 err_stop:
 	while (i--) {
-		qmc_chan_stop(qmc_dai->chans[i].qmc_chan, direction);
-		qmc_chan_reset(qmc_dai->chans[i].qmc_chan, direction);
+		qmc_chan_stop(qmc_dai->qmc_chans[i], direction);
+		qmc_chan_reset(qmc_dai->qmc_chans[i], direction);
 	}
 	return ret;
 }
@@ -794,19 +781,20 @@ static int qmc_audio_dai_parse(struct qmc_audio *qmc_audio, struct device_node *
 		return dev_err_probe(qmc_audio->dev, -EINVAL,
 				     "dai %d no QMC channel defined\n", qmc_dai->id);
 
-	qmc_dai->chans = devm_kcalloc(qmc_audio->dev, count, sizeof(*qmc_dai->chans), GFP_KERNEL);
-	if (!qmc_dai->chans)
+	qmc_dai->qmc_chans = devm_kcalloc(qmc_audio->dev, count, sizeof(*qmc_dai->qmc_chans),
+					  GFP_KERNEL);
+	if (!qmc_dai->qmc_chans)
 		return -ENOMEM;
 
 	for (i = 0; i < count; i++) {
-		qmc_dai->chans[i].qmc_chan = devm_qmc_chan_get_byphandles_index(qmc_audio->dev, np,
-										"fsl,qmc-chan", i);
-		if (IS_ERR(qmc_dai->chans[i].qmc_chan)) {
-			return dev_err_probe(qmc_audio->dev, PTR_ERR(qmc_dai->chans[i].qmc_chan),
+		qmc_dai->qmc_chans[i] = devm_qmc_chan_get_byphandles_index(qmc_audio->dev, np,
+									   "fsl,qmc-chan", i);
+		if (IS_ERR(qmc_dai->qmc_chans[i])) {
+			return dev_err_probe(qmc_audio->dev, PTR_ERR(qmc_dai->qmc_chans[i]),
 					     "dai %d get QMC channel %d failed\n", qmc_dai->id, i);
 		}
 
-		ret = qmc_chan_get_info(qmc_dai->chans[i].qmc_chan, &info);
+		ret = qmc_chan_get_info(qmc_dai->qmc_chans[i], &info);
 		if (ret) {
 			dev_err(qmc_audio->dev, "dai %d get QMC %d channel info failed %d\n",
 				qmc_dai->id, i, ret);
-- 
2.49.0

Re: [PATCH v2 1/4] soc: fsl: qmc: Only set completion interrupt when needed

[Herve Codina]

Hi Christophe,

On Tue, 12 Aug 2025 12:50:55 +0200
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> When no post-completion processing is expected, don't waste time
> handling useless interrupts.
> 
> Only set QMC_BD_[R/T]X_I when a completion function is passed in,
> and perform seamless completion on submit for interruptless buffers.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> v2: Keep the UB flag to mark not completed buffers and seamlessly flag them as completed during next submit.
> ---
>  drivers/soc/fsl/qe/qmc.c | 44 ++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/soc/fsl/qe/qmc.c b/drivers/soc/fsl/qe/qmc.c
> index 36c0ccc06151f..8f76b9a5e385d 100644
> --- a/drivers/soc/fsl/qe/qmc.c
> +++ b/drivers/soc/fsl/qe/qmc.c
> @@ -461,9 +461,16 @@ int qmc_chan_write_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
>  
>  	ctrl = qmc_read16(&bd->cbd_sc);
>  	if (ctrl & (QMC_BD_TX_R | QMC_BD_TX_UB)) {
> -		/* We are full ... */
> -		ret = -EBUSY;
> -		goto end;
> +		if (!(ctrl & QMC_BD_TX_I) && bd == chan->txbd_done) {
> +			if (ctrl & QMC_BD_TX_W)
> +				chan->txbd_done = chan->txbds;
> +			else
> +				chan->txbd_done++;
> +		} else {
> +			/* We are full ... */
> +			ret = -EBUSY;
> +			goto end;
> +		}
>  	}
>  
>  	qmc_write16(&bd->cbd_datlen, length);
> @@ -475,6 +482,10 @@ int qmc_chan_write_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
>  
>  	/* Activate the descriptor */
>  	ctrl |= (QMC_BD_TX_R | QMC_BD_TX_UB);
> +	if (complete)
> +		ctrl |= QMC_BD_TX_I;
> +	else
> +		ctrl &= ~QMC_BD_TX_I;
>  	wmb(); /* Be sure to flush the descriptor before control update */
>  	qmc_write16(&bd->cbd_sc, ctrl);
>  

You try to purge one descriptor for which the transfer is done but you do that
when you have no more free descriptors.

You end up with all descriptor "used". I think a better way to do that is
to purge all "done" descriptor configured to work without interrupts until a
descriptor with interrupt is found.

What do you think about the following (draft code not compiled and not tested) ?

/* must be called with tx_lock taken */
static void qmc_chan_write_purge(struct qmc_chan *chan)
{
	cbd_t __iomem *bd;
	u16 ctrl;

	/*
         * Purge descriptors configured to work without interrupts
	 * (I bit == 0) those descriptors have no completion
	 * callbacks.
	 *
	 * R bit  UB bit
	 *   0       0  : The BD is free
	 *   1       1  : The BD is in used, waiting for transfer
	 *   0       1  : The BD is in used, waiting for completion
	 *   1       0  : Should not append
	 *
	 * We purge those descriptors which are in the state "waiting for
	 * completion" up to the first one configured to work with an interrupt.
	 * 
	 */
	bd = chan->txbd_done;

	ctrl = qmc_read16(&bd->cbd_sc);
	while (!(ctrl & (QMC_BD_TX_R | QMC_BD_TX_I)) {
		if (!(ctrl & QMC_BD_TX_UB))
			return;

		qmc_write16(&bd->cbd_sc, ctrl & ~QMC_BD_TX_UB);

		if (ctrl & QMC_BD_TX_W)
			chan->txbd_done = chan->txbds;
		else
			chan->txbd_done++;

		bd = chan->txbd_done;
		ctrl = qmc_read16(&bd->cbd_sc);
	}
}

Then, qmc_chan_write_submit() calls the purge function as a first operation.
This will look to something like the following:

int qmc_chan_write_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
			  void (*complete)(void *context), void *context)
{
	struct qmc_xfer_desc *xfer_desc;
	unsigned long flags;
	cbd_t __iomem *bd;
	u16 ctrl;
	int ret;

	/*
	 * R bit  UB bit
	 *   0       0  : The BD is free
	 *   1       1  : The BD is in used, waiting for transfer
	 *   0       1  : The BD is in used, waiting for completion
	 *   1       0  : Should not append
	 */

	spin_lock_irqsave(&chan->tx_lock, flags);

	qmc_chan_write_purge(chan);

	bd = chan->txbd_free;
	...

	if (complete)
		ctrl |= QMC_BD_TX_I;
	else
		ctrl &= ~QMC_BD_TX_I;
	...
}


> @@ -569,9 +580,16 @@ int qmc_chan_read_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
> 
...

Exact same comment and proposal for read part.

>  
> @@ -1482,19 +1504,19 @@ static int qmc_setup_chan(struct qmc *qmc, struct qmc_chan *chan)
>  
>  	/* Init Rx BDs and set Wrap bit on last descriptor */
>  	BUILD_BUG_ON(QMC_NB_RXBDS == 0);
> -	val = QMC_BD_RX_I;
>  	for (i = 0; i < QMC_NB_RXBDS; i++) {
>  		bd = chan->rxbds + i;
> -		qmc_write16(&bd->cbd_sc, val);
> +		qmc_write16(&bd->cbd_sc, 0);
>  	}
>  	bd = chan->rxbds + QMC_NB_RXBDS - 1;
> -	qmc_write16(&bd->cbd_sc, val | QMC_BD_RX_W);
> +	qmc_write16(&bd->cbd_sc, QMC_BD_RX_W);
>  
>  	/* Init Tx BDs and set Wrap bit on last descriptor */
>  	BUILD_BUG_ON(QMC_NB_TXBDS == 0);
> -	val = QMC_BD_TX_I;
>  	if (chan->mode == QMC_HDLC)
> -		val |= QMC_BD_TX_L | QMC_BD_TX_TC;
> +		val = QMC_BD_TX_L | QMC_BD_TX_TC;
> +	else
> +		val = 0;
>  	for (i = 0; i < QMC_NB_TXBDS; i++) {
>  		bd = chan->txbds + i;
>  		qmc_write16(&bd->cbd_sc, val);

Ok for modifications in qmc_setup_chan().

Best regards,
Hervé

Re: [PATCH v2 2/4] ASoc: fsl: fsl_qmc_audio: Ensure audio channels are ordered in TDM bus

[Herve Codina]

Hi Christophe,

On Tue, 12 Aug 2025 12:50:56 +0200
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

...

> @@ -879,6 +884,30 @@ static int qmc_audio_dai_parse(struct qmc_audio *qmc_audio, struct device_node *
>  				return -EINVAL;
>  			}
>  		}
> +
> +		ret = qmc_chan_get_ts_info(qmc_dai->qmc_chans[i], &ts_info);
> +		if (ret) {
> +			dev_err(qmc_audio->dev, "dai %d get QMC %d channel TS info failed %d\n",
> +				qmc_dai->id, i, ret);
> +			return ret;
> +		}
> +
> +		last_rx_ts = fls64(ts_info.rx_ts_mask);
> +		last_tx_ts = fls64(ts_info.rx_ts_mask);
                                              
tx_ts_mask instead of rx_ts_mask for last_tx_ts.

Best regards,
Hervé

Re: [PATCH v2 3/4] ASoC: fsl: fsl_qmc_audio: Only request completion on last channel

[Herve Codina]

On Tue, 12 Aug 2025 12:50:57 +0200
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> In non-interleaved mode, several QMC channels are used in sync.
> More details can be found in commit 188d9cae5438 ("ASoC: fsl:
> fsl_qmc_audio: Add support for non-interleaved mode.")
> At the time being, an interrupt is requested on each channel to
> perform capture/playback completion, allthough the completion is
> really performed only once all channels have completed their work.
> 
> This leads to a lot more interrupts than really needed. Looking at
> /proc/interrupts shows ~3800 interrupts per second when using
> 4 capture and 4 playback devices with 5ms periods while
> only 1600 (200 x 4 + 200 x 4) periods are processed during one second.
> 
> The QMC channels work in sync, the one started first is the one
> finishing first and the one started last is the one finishing last,
> so when the last one finishes it is guaranteed that the other ones are
> finished as well. Therefore only request completion processing on the
> last QMC channel.
> 
> On my board with the above exemple, on a kernel started with
> 'threadirqs' option, the QMC irq thread uses 16% CPU time with this
> patch while it uses 26% CPU time without this patch.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  sound/soc/fsl/fsl_qmc_audio.c | 46 +++++------------------------------
>  1 file changed, 6 insertions(+), 40 deletions(-)
> 

Acked-by: Herve Codina <herve.codina@bootlin.com>

Best regards,
Hervé



-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 4/4] ASoc: fsl: fsl_qmc_audio: Drop struct qmc_dai_chan

[Herve Codina]

Hi Christophe,

On Tue, 12 Aug 2025 12:50:58 +0200
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> prtd_tx and prtd_rx members are not used anymore and only qmc_chan
> member remains so struct qmc_dai_chan has become pointless.
> 
> Use qmc_chan directly and drop struct qmc_dai_chan.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> v2: New
> ---
>  sound/soc/fsl/fsl_qmc_audio.c | 50 +++++++++++++----------------------
>  1 file changed, 19 insertions(+), 31 deletions(-)

Acked-by: Herve Codina <herve.codina@bootlin.com>

Best regards,
Hervé

Re: [PATCH v2 2/4] ASoc: fsl: fsl_qmc_audio: Ensure audio channels are ordered in TDM bus

[kernel test robot]

Hi Christophe,

kernel test robot noticed the following build warnings:

[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on linus/master v6.17-rc1 next-20250813]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christophe-Leroy/soc-fsl-qmc-Only-set-completion-interrupt-when-needed/20250812-193812
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
patch link:    https://lore.kernel.org/r/8d01cf4599664188c92a515922d68c9834263384.1754993232.git.christophe.leroy%40csgroup.eu
patch subject: [PATCH v2 2/4] ASoc: fsl: fsl_qmc_audio: Ensure audio channels are ordered in TDM bus
config: powerpc64-randconfig-r073-20250813 (https://download.01.org/0day-ci/archive/20250814/202508140002.NBrWOZJL-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 3769ce013be2879bf0b329c14a16f5cb766f26ce)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250814/202508140002.NBrWOZJL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508140002.NBrWOZJL-lkp@intel.com/

All warnings (new ones prefixed by >>):

   sound/soc/fsl/fsl_qmc_audio.c:888:39: error: no member named 'qmc_chans' in 'struct qmc_dai'
     888 |                 ret = qmc_chan_get_ts_info(qmc_dai->qmc_chans[i], &ts_info);
         |                                            ~~~~~~~  ^
>> sound/soc/fsl/fsl_qmc_audio.c:900:21: warning: format specifies type 'unsigned long' but the argument has type 'int' [-Wformat]
     899 |                         dev_err(qmc_audio->dev, "dai %d QMC chan %d unordered channels (RX timeslot %lu before %lu)\n",
         |                                                                                                     ~~~
         |                                                                                                     %d
     900 |                                 qmc_dai->id, i, prev_last_rx_ts, last_rx_ts);
         |                                                 ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:154:65: note: expanded from macro 'dev_err'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ~~~    ^~~~~~~~~~~
   sound/soc/fsl/fsl_qmc_audio.c:900:38: warning: format specifies type 'unsigned long' but the argument has type 'int' [-Wformat]
     899 |                         dev_err(qmc_audio->dev, "dai %d QMC chan %d unordered channels (RX timeslot %lu before %lu)\n",
         |                                                                                                                ~~~
         |                                                                                                                %d
     900 |                                 qmc_dai->id, i, prev_last_rx_ts, last_rx_ts);
         |                                                                  ^~~~~~~~~~
   include/linux/dev_printk.h:154:65: note: expanded from macro 'dev_err'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ~~~    ^~~~~~~~~~~
   sound/soc/fsl/fsl_qmc_audio.c:905:21: warning: format specifies type 'unsigned long' but the argument has type 'int' [-Wformat]
     904 |                         dev_err(qmc_audio->dev, "dai %d QMC chan %d unordered channels (TX timeslot %lu before %lu)\n",
         |                                                                                                     ~~~
         |                                                                                                     %d
     905 |                                 qmc_dai->id, i, prev_last_tx_ts, last_tx_ts);
         |                                                 ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:154:65: note: expanded from macro 'dev_err'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ~~~    ^~~~~~~~~~~
   sound/soc/fsl/fsl_qmc_audio.c:905:38: warning: format specifies type 'unsigned long' but the argument has type 'int' [-Wformat]
     904 |                         dev_err(qmc_audio->dev, "dai %d QMC chan %d unordered channels (TX timeslot %lu before %lu)\n",
         |                                                                                                                ~~~
         |                                                                                                                %d
     905 |                                 qmc_dai->id, i, prev_last_tx_ts, last_tx_ts);
         |                                                                  ^~~~~~~~~~
   include/linux/dev_printk.h:154:65: note: expanded from macro 'dev_err'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ~~~    ^~~~~~~~~~~
   4 warnings and 1 error generated.


vim +900 sound/soc/fsl/fsl_qmc_audio.c

   789	
   790	static int qmc_audio_dai_parse(struct qmc_audio *qmc_audio, struct device_node *np,
   791				       struct qmc_dai *qmc_dai,
   792				       struct snd_soc_dai_driver *qmc_soc_dai_driver)
   793	{
   794		struct qmc_chan_ts_info ts_info;
   795		struct qmc_chan_info info;
   796		unsigned long rx_fs_rate;
   797		unsigned long tx_fs_rate;
   798		int prev_last_rx_ts = 0;
   799		int prev_last_tx_ts = 0;
   800		unsigned int nb_tx_ts;
   801		unsigned int nb_rx_ts;
   802		unsigned int i;
   803		int last_rx_ts;
   804		int last_tx_ts;
   805		int count;
   806		u32 val;
   807		int ret;
   808	
   809		qmc_dai->dev = qmc_audio->dev;
   810	
   811		ret = of_property_read_u32(np, "reg", &val);
   812		if (ret) {
   813			dev_err(qmc_audio->dev, "%pOF: failed to read reg\n", np);
   814			return ret;
   815		}
   816		qmc_dai->id = val;
   817	
   818		qmc_dai->name = devm_kasprintf(qmc_audio->dev, GFP_KERNEL, "%s.%d",
   819					       np->parent->name, qmc_dai->id);
   820		if (!qmc_dai->name)
   821			return -ENOMEM;
   822	
   823		count = qmc_chan_count_phandles(np, "fsl,qmc-chan");
   824		if (count < 0)
   825			return dev_err_probe(qmc_audio->dev, count,
   826					     "dai %d get number of QMC channel failed\n", qmc_dai->id);
   827		if (!count)
   828			return dev_err_probe(qmc_audio->dev, -EINVAL,
   829					     "dai %d no QMC channel defined\n", qmc_dai->id);
   830	
   831		qmc_dai->chans = devm_kcalloc(qmc_audio->dev, count, sizeof(*qmc_dai->chans), GFP_KERNEL);
   832		if (!qmc_dai->chans)
   833			return -ENOMEM;
   834	
   835		for (i = 0; i < count; i++) {
   836			qmc_dai->chans[i].qmc_chan = devm_qmc_chan_get_byphandles_index(qmc_audio->dev, np,
   837											"fsl,qmc-chan", i);
   838			if (IS_ERR(qmc_dai->chans[i].qmc_chan)) {
   839				return dev_err_probe(qmc_audio->dev, PTR_ERR(qmc_dai->chans[i].qmc_chan),
   840						     "dai %d get QMC channel %d failed\n", qmc_dai->id, i);
   841			}
   842	
   843			ret = qmc_chan_get_info(qmc_dai->chans[i].qmc_chan, &info);
   844			if (ret) {
   845				dev_err(qmc_audio->dev, "dai %d get QMC %d channel info failed %d\n",
   846					qmc_dai->id, i, ret);
   847				return ret;
   848			}
   849	
   850			if (info.mode != QMC_TRANSPARENT) {
   851				dev_err(qmc_audio->dev, "dai %d QMC chan %d mode %d is not QMC_TRANSPARENT\n",
   852					qmc_dai->id, i, info.mode);
   853				return -EINVAL;
   854			}
   855	
   856			/*
   857			 * All channels must have the same number of Tx slots and the
   858			 * same numbers of Rx slots.
   859			 */
   860			if (i == 0) {
   861				nb_tx_ts = info.nb_tx_ts;
   862				nb_rx_ts = info.nb_rx_ts;
   863				tx_fs_rate = info.tx_fs_rate;
   864				rx_fs_rate = info.rx_fs_rate;
   865			} else {
   866				if (nb_tx_ts != info.nb_tx_ts) {
   867					dev_err(qmc_audio->dev, "dai %d QMC chan %d inconsistent number of Tx timeslots (%u instead of %u)\n",
   868						qmc_dai->id, i, info.nb_tx_ts, nb_tx_ts);
   869					return -EINVAL;
   870				}
   871				if (nb_rx_ts != info.nb_rx_ts) {
   872					dev_err(qmc_audio->dev, "dai %d QMC chan %d inconsistent number of Rx timeslots (%u instead of %u)\n",
   873						qmc_dai->id, i, info.nb_rx_ts, nb_rx_ts);
   874					return -EINVAL;
   875				}
   876				if (tx_fs_rate != info.tx_fs_rate) {
   877					dev_err(qmc_audio->dev, "dai %d QMC chan %d inconsistent Tx frame sample rate (%lu instead of %lu)\n",
   878						qmc_dai->id, i, info.tx_fs_rate, tx_fs_rate);
   879					return -EINVAL;
   880				}
   881				if (rx_fs_rate != info.rx_fs_rate) {
   882					dev_err(qmc_audio->dev, "dai %d QMC chan %d inconsistent Rx frame sample rate (%lu instead of %lu)\n",
   883						qmc_dai->id, i, info.rx_fs_rate, rx_fs_rate);
   884					return -EINVAL;
   885				}
   886			}
   887	
 > 888			ret = qmc_chan_get_ts_info(qmc_dai->qmc_chans[i], &ts_info);
   889			if (ret) {
   890				dev_err(qmc_audio->dev, "dai %d get QMC %d channel TS info failed %d\n",
   891					qmc_dai->id, i, ret);
   892				return ret;
   893			}
   894	
   895			last_rx_ts = fls64(ts_info.rx_ts_mask);
   896			last_tx_ts = fls64(ts_info.rx_ts_mask);
   897	
   898			if (prev_last_rx_ts > last_rx_ts) {
   899				dev_err(qmc_audio->dev, "dai %d QMC chan %d unordered channels (RX timeslot %lu before %lu)\n",
 > 900					qmc_dai->id, i, prev_last_rx_ts, last_rx_ts);
   901				return -EINVAL;
   902			}
   903			if (prev_last_tx_ts > last_tx_ts) {
   904				dev_err(qmc_audio->dev, "dai %d QMC chan %d unordered channels (TX timeslot %lu before %lu)\n",
   905					qmc_dai->id, i, prev_last_tx_ts, last_tx_ts);
   906				return -EINVAL;
   907			}
   908	
   909			prev_last_rx_ts = last_rx_ts;
   910			prev_last_tx_ts = last_tx_ts;
   911		}
   912	
   913		qmc_dai->nb_chans_avail = count;
   914		qmc_dai->nb_tx_ts = nb_tx_ts * count;
   915		qmc_dai->nb_rx_ts = nb_rx_ts * count;
   916	
   917		qmc_soc_dai_driver->id = qmc_dai->id;
   918		qmc_soc_dai_driver->name = qmc_dai->name;
   919	
   920		qmc_soc_dai_driver->playback.channels_min = 0;
   921		qmc_soc_dai_driver->playback.channels_max = 0;
   922		if (nb_tx_ts) {
   923			qmc_soc_dai_driver->playback.channels_min = 1;
   924			qmc_soc_dai_driver->playback.channels_max = count > 1 ? count : nb_tx_ts;
   925		}
   926		qmc_soc_dai_driver->playback.formats = qmc_audio_formats(nb_tx_ts,
   927									 count > 1);
   928	
   929		qmc_soc_dai_driver->capture.channels_min = 0;
   930		qmc_soc_dai_driver->capture.channels_max = 0;
   931		if (nb_rx_ts) {
   932			qmc_soc_dai_driver->capture.channels_min = 1;
   933			qmc_soc_dai_driver->capture.channels_max = count > 1 ? count : nb_rx_ts;
   934		}
   935		qmc_soc_dai_driver->capture.formats = qmc_audio_formats(nb_rx_ts,
   936									count > 1);
   937	
   938		qmc_soc_dai_driver->playback.rates = snd_pcm_rate_to_rate_bit(tx_fs_rate);
   939		qmc_soc_dai_driver->playback.rate_min = tx_fs_rate;
   940		qmc_soc_dai_driver->playback.rate_max = tx_fs_rate;
   941		qmc_soc_dai_driver->capture.rates = snd_pcm_rate_to_rate_bit(rx_fs_rate);
   942		qmc_soc_dai_driver->capture.rate_min = rx_fs_rate;
   943		qmc_soc_dai_driver->capture.rate_max = rx_fs_rate;
   944	
   945		qmc_soc_dai_driver->ops = &qmc_dai_ops;
   946	
   947		return 0;
   948	}
   949	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 1/4] soc: fsl: qmc: Only set completion interrupt when needed

[Herve Codina]

Hi Christophe,

On Wed, 13 Aug 2025 12:06:51 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> Hi Christophe,
> 
> On Tue, 12 Aug 2025 12:50:55 +0200
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
> > When no post-completion processing is expected, don't waste time
> > handling useless interrupts.
> > 
> > Only set QMC_BD_[R/T]X_I when a completion function is passed in,
> > and perform seamless completion on submit for interruptless buffers.
> > 
> > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> > ---
> > v2: Keep the UB flag to mark not completed buffers and seamlessly flag them as completed during next submit.
> > ---
> >  drivers/soc/fsl/qe/qmc.c | 44 ++++++++++++++++++++++++++++++----------
> >  1 file changed, 33 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/soc/fsl/qe/qmc.c b/drivers/soc/fsl/qe/qmc.c
> > index 36c0ccc06151f..8f76b9a5e385d 100644
> > --- a/drivers/soc/fsl/qe/qmc.c
> > +++ b/drivers/soc/fsl/qe/qmc.c
> > @@ -461,9 +461,16 @@ int qmc_chan_write_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
> >  
> >  	ctrl = qmc_read16(&bd->cbd_sc);
> >  	if (ctrl & (QMC_BD_TX_R | QMC_BD_TX_UB)) {
> > -		/* We are full ... */
> > -		ret = -EBUSY;
> > -		goto end;
> > +		if (!(ctrl & QMC_BD_TX_I) && bd == chan->txbd_done) {
> > +			if (ctrl & QMC_BD_TX_W)
> > +				chan->txbd_done = chan->txbds;
> > +			else
> > +				chan->txbd_done++;
> > +		} else {
> > +			/* We are full ... */
> > +			ret = -EBUSY;
> > +			goto end;
> > +		}
> >  	}
> >  
> >  	qmc_write16(&bd->cbd_datlen, length);
> > @@ -475,6 +482,10 @@ int qmc_chan_write_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
> >  
> >  	/* Activate the descriptor */
> >  	ctrl |= (QMC_BD_TX_R | QMC_BD_TX_UB);
> > +	if (complete)
> > +		ctrl |= QMC_BD_TX_I;
> > +	else
> > +		ctrl &= ~QMC_BD_TX_I;
> >  	wmb(); /* Be sure to flush the descriptor before control update */
> >  	qmc_write16(&bd->cbd_sc, ctrl);
> >    
> 
> You try to purge one descriptor for which the transfer is done but you do that
> when you have no more free descriptors.
> 
> You end up with all descriptor "used". I think a better way to do that is
> to purge all "done" descriptor configured to work without interrupts until a
> descriptor with interrupt is found.

I have looked again at your code and looking for a free descriptor only when it
is needed is sufficient. You can forget my previous proposal.

Back to your code, I think you need to be sure that the descriptor you want to
re-use is really available and so you need to check the 'R' bit to be sure
that we are not with 'R' = 1 and 'UB' = 1 which means "BD is used, waiting for
a transfer".

For instance:

	if (ctrl & (QMC_BD_TX_R | QMC_BD_TX_UB)) {
		if (!(ctrl & (QMC_BD_TX_I | QMC_BD_TX_R) &&
		    bd == chan->txbd_done) {
			if (ctrl & QMC_BD_TX_W)
				chan->txbd_done = chan->txbds;
			else
				chan->txbd_done++;
		} else {
			/* We are full ... */
			ret = -EBUSY;
			goto end;
		}
	}

Best regards,
Hervé

Re: [PATCH v2 2/4] ASoc: fsl: fsl_qmc_audio: Ensure audio channels are ordered in TDM bus

[Herve Codina]

On Tue, 12 Aug 2025 12:50:56 +0200
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> To reduce complexity of interrupt handling in following patch, ensure
> audio channels are configured in the same order as timeslots on the
> TDM bus. If we need a given ordering of audio sources in the audio
> frame, it is possible to re-order codecs on the TDM bus, no need to
> mix up timeslots in channels.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> v2: New
> ---
>  sound/soc/fsl/fsl_qmc_audio.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/sound/soc/fsl/fsl_qmc_audio.c b/sound/soc/fsl/fsl_qmc_audio.c
> index 5614a8b909edf..0be29ccc1ff7b 100644
> --- a/sound/soc/fsl/fsl_qmc_audio.c
> +++ b/sound/soc/fsl/fsl_qmc_audio.c
> @@ -791,12 +791,17 @@ static int qmc_audio_dai_parse(struct qmc_audio *qmc_audio, struct device_node *
>  			       struct qmc_dai *qmc_dai,
>  			       struct snd_soc_dai_driver *qmc_soc_dai_driver)
>  {
> +	struct qmc_chan_ts_info ts_info;
>  	struct qmc_chan_info info;
>  	unsigned long rx_fs_rate;
>  	unsigned long tx_fs_rate;
> +	int prev_last_rx_ts = 0;
> +	int prev_last_tx_ts = 0;
>  	unsigned int nb_tx_ts;
>  	unsigned int nb_rx_ts;
>  	unsigned int i;
> +	int last_rx_ts;
> +	int last_tx_ts;
>  	int count;
>  	u32 val;
>  	int ret;
> @@ -879,6 +884,30 @@ static int qmc_audio_dai_parse(struct qmc_audio *qmc_audio, struct device_node *
>  				return -EINVAL;
>  			}
>  		}
> +
> +		ret = qmc_chan_get_ts_info(qmc_dai->qmc_chans[i], &ts_info);

qmc_chan_get_ts_info() need a struct qmc_chan as first parameter

qmc_dai->qmc_chans[i].qmc_chan instead of qmc_dai->qmc_chans[i].

The use of qmc_dai->qmc_chans[i] without .qmc_chan have to be done on patch 4 (cleanup patch).

Best regards,
Hervé

Re: [PATCH v2 1/4] soc: fsl: qmc: Only set completion interrupt when needed

[Christophe Leroy]

Hi Hervé,

Le 14/08/2025 à 09:34, Herve Codina a écrit :
> Hi Christophe,
> 
> On Wed, 13 Aug 2025 12:06:51 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
>> Hi Christophe,
>>
>> On Tue, 12 Aug 2025 12:50:55 +0200
>> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
>>
>>> When no post-completion processing is expected, don't waste time
>>> handling useless interrupts.
>>>
>>> Only set QMC_BD_[R/T]X_I when a completion function is passed in,
>>> and perform seamless completion on submit for interruptless buffers.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>> v2: Keep the UB flag to mark not completed buffers and seamlessly flag them as completed during next submit.
>>> ---
>>>   drivers/soc/fsl/qe/qmc.c | 44 ++++++++++++++++++++++++++++++----------
>>>   1 file changed, 33 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/soc/fsl/qe/qmc.c b/drivers/soc/fsl/qe/qmc.c
>>> index 36c0ccc06151f..8f76b9a5e385d 100644
>>> --- a/drivers/soc/fsl/qe/qmc.c
>>> +++ b/drivers/soc/fsl/qe/qmc.c
>>> @@ -461,9 +461,16 @@ int qmc_chan_write_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
>>>   
>>>   	ctrl = qmc_read16(&bd->cbd_sc);
>>>   	if (ctrl & (QMC_BD_TX_R | QMC_BD_TX_UB)) {
>>> -		/* We are full ... */
>>> -		ret = -EBUSY;
>>> -		goto end;
>>> +		if (!(ctrl & QMC_BD_TX_I) && bd == chan->txbd_done) {
>>> +			if (ctrl & QMC_BD_TX_W)
>>> +				chan->txbd_done = chan->txbds;
>>> +			else
>>> +				chan->txbd_done++;
>>> +		} else {
>>> +			/* We are full ... */
>>> +			ret = -EBUSY;
>>> +			goto end;
>>> +		}
>>>   	}
>>>   
>>>   	qmc_write16(&bd->cbd_datlen, length);
>>> @@ -475,6 +482,10 @@ int qmc_chan_write_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
>>>   
>>>   	/* Activate the descriptor */
>>>   	ctrl |= (QMC_BD_TX_R | QMC_BD_TX_UB);
>>> +	if (complete)
>>> +		ctrl |= QMC_BD_TX_I;
>>> +	else
>>> +		ctrl &= ~QMC_BD_TX_I;
>>>   	wmb(); /* Be sure to flush the descriptor before control update */
>>>   	qmc_write16(&bd->cbd_sc, ctrl);
>>>     
>>
>> You try to purge one descriptor for which the transfer is done but you do that
>> when you have no more free descriptors.
>>
>> You end up with all descriptor "used". I think a better way to do that is
>> to purge all "done" descriptor configured to work without interrupts until a
>> descriptor with interrupt is found.
> 
> I have looked again at your code and looking for a free descriptor only when it
> is needed is sufficient. You can forget my previous proposal.
> 
> Back to your code, I think you need to be sure that the descriptor you want to
> re-use is really available and so you need to check the 'R' bit to be sure
> that we are not with 'R' = 1 and 'UB' = 1 which means "BD is used, waiting for
> a transfer".
> 

Fixed in v3.

Thanks
Christophe

Re: [PATCH v2 2/4] ASoc: fsl: fsl_qmc_audio: Ensure audio channels are ordered in TDM bus

[Christophe Leroy]

Le 14/08/2025 à 09:45, Herve Codina a écrit :
> On Tue, 12 Aug 2025 12:50:56 +0200
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
>> To reduce complexity of interrupt handling in following patch, ensure
>> audio channels are configured in the same order as timeslots on the
>> TDM bus. If we need a given ordering of audio sources in the audio
>> frame, it is possible to re-order codecs on the TDM bus, no need to
>> mix up timeslots in channels.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> v2: New
>> ---
>>   sound/soc/fsl/fsl_qmc_audio.c | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/sound/soc/fsl/fsl_qmc_audio.c b/sound/soc/fsl/fsl_qmc_audio.c
>> index 5614a8b909edf..0be29ccc1ff7b 100644
>> --- a/sound/soc/fsl/fsl_qmc_audio.c
>> +++ b/sound/soc/fsl/fsl_qmc_audio.c
>> @@ -791,12 +791,17 @@ static int qmc_audio_dai_parse(struct qmc_audio *qmc_audio, struct device_node *
>>   			       struct qmc_dai *qmc_dai,
>>   			       struct snd_soc_dai_driver *qmc_soc_dai_driver)
>>   {
>> +	struct qmc_chan_ts_info ts_info;
>>   	struct qmc_chan_info info;
>>   	unsigned long rx_fs_rate;
>>   	unsigned long tx_fs_rate;
>> +	int prev_last_rx_ts = 0;
>> +	int prev_last_tx_ts = 0;
>>   	unsigned int nb_tx_ts;
>>   	unsigned int nb_rx_ts;
>>   	unsigned int i;
>> +	int last_rx_ts;
>> +	int last_tx_ts;
>>   	int count;
>>   	u32 val;
>>   	int ret;
>> @@ -879,6 +884,30 @@ static int qmc_audio_dai_parse(struct qmc_audio *qmc_audio, struct device_node *
>>   				return -EINVAL;
>>   			}
>>   		}
>> +
>> +		ret = qmc_chan_get_ts_info(qmc_dai->qmc_chans[i], &ts_info);
> 
> qmc_chan_get_ts_info() need a struct qmc_chan as first parameter
> 
> qmc_dai->qmc_chans[i].qmc_chan instead of qmc_dai->qmc_chans[i].
> 
> The use of qmc_dai->qmc_chans[i] without .qmc_chan have to be done on patch 4 (cleanup patch).

Fixed in v3

Thanks
Christophe

Re: [PATCH v2 2/4] ASoc: fsl: fsl_qmc_audio: Ensure audio channels are ordered in TDM bus

[Christophe Leroy]

Le 13/08/2025 à 12:06, Herve Codina a écrit :
> Hi Christophe,
> 
> On Tue, 12 Aug 2025 12:50:56 +0200
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
> ...
> 
>> @@ -879,6 +884,30 @@ static int qmc_audio_dai_parse(struct qmc_audio *qmc_audio, struct device_node *
>>   				return -EINVAL;
>>   			}
>>   		}
>> +
>> +		ret = qmc_chan_get_ts_info(qmc_dai->qmc_chans[i], &ts_info);
>> +		if (ret) {
>> +			dev_err(qmc_audio->dev, "dai %d get QMC %d channel TS info failed %d\n",
>> +				qmc_dai->id, i, ret);
>> +			return ret;
>> +		}
>> +
>> +		last_rx_ts = fls64(ts_info.rx_ts_mask);
>> +		last_tx_ts = fls64(ts_info.rx_ts_mask);
>                                                
> tx_ts_mask instead of rx_ts_mask for last_tx_ts.
> 

Fixed in v3

Thanks
Christophe