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.0Hi 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é
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é
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
© 2016 - 2025 Red Hat, Inc.