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

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

Hi Christophe,

On Fri,  9 May 2025 09:48:44 +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 and QMC_BD_[R/T]X_UB when a completion
> function is passed in.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  drivers/soc/fsl/qe/qmc.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soc/fsl/qe/qmc.c b/drivers/soc/fsl/qe/qmc.c
> index 36c0ccc06151..0a704fd0b1a0 100644
> --- a/drivers/soc/fsl/qe/qmc.c
> +++ b/drivers/soc/fsl/qe/qmc.c
> @@ -474,7 +474,9 @@ int qmc_chan_write_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
>  	xfer_desc->context = context;
>  
>  	/* Activate the descriptor */
> -	ctrl |= (QMC_BD_TX_R | QMC_BD_TX_UB);
> +	ctrl |= QMC_BD_TX_R;
> +	if (complete)
> +		ctrl |= QMC_BD_TX_I | QMC_BD_TX_UB;

Be careful, you don't set the UB bit for all descriptor anymore.
Your goal, is to have interrupts only on some descriptors (those where I
bit is set).

This can lead to issue in the function handling the interrupt.
This function, qmc_chan_write_done(), do the processing according to the
following:
        /*
	 * 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
	 */
https://elixir.bootlin.com/linux/v6.15-rc5/source/drivers/soc/fsl/qe/qmc.c#L507

It considers R=0 / UB=0 as a free BD and R=1 / UB=0 as a case that should
not happen.

Both cases are no more correct with your modification.

Have the feeling that UB bit still has to be set even if I bit is not set
in order to have qmc_chan_write_done() looking at all descriptors.

Suppose:
 desc 0, no interrupt
 desc 1, no interrupt
 desc 2, interrupt

When the interrupt for desc 2 is handled, desc 0 and desc 1 are seen with
R=0 and UB=0. As desc 0 is considered as free by qmc_chan_write_done(), it
will never look at desc 2.

>  	wmb(); /* Be sure to flush the descriptor before control update */
>  	qmc_write16(&bd->cbd_sc, ctrl);
>  
> @@ -586,7 +588,9 @@ int qmc_chan_read_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length,
>  		  QMC_BD_RX_AB | QMC_BD_RX_CR);
>  
>  	/* Activate the descriptor */
> -	ctrl |= (QMC_BD_RX_E | QMC_BD_RX_UB);
> +	ctrl |= QMC_BD_RX_E;
> +	if (complete)
> +		ctrl |= QMC_BD_RX_I | QMC_BD_RX_UB;

Exact same comment.


Best regards,
Hervé