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

[Herve Codina <herve.codina@bootlin.com>]

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é