| Message ID | 20190818180115.31114-14-olteanv@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NXP DSPI driver cleanup | expand |
On Sun, Aug 18, 2019 at 09:01:14PM +0300, Vladimir Oltean wrote: > There is no point in checking which interrupt source was triggered in > SPI_SR, since only EOQ and TCFQ modes trigger interrupts anyway (see the > writes to SPI_RSER). In DMA mode, the RSER is configured for RFDF_DIRS=1 > and TFFF_DIRS=1, aka FIFO events generate eDMA requests and not CPU > interrupts. It's also good to check interrupt sources in case the interrupt line might be shared, that means that it's possible that the interrupt handler will be called when there's no interrupt at all from the IP. It also helps with robustness in case there's some system error though that's (hopefully!) a lot less common. This driver does set IRQF_SHARED so it does support that, I don't know how many systems could do it but it seems a shame to remove the support from the driver.
On Tue, 20 Aug 2019 at 16:02, Mark Brown <broonie@kernel.org> wrote: > > On Sun, Aug 18, 2019 at 09:01:14PM +0300, Vladimir Oltean wrote: > > There is no point in checking which interrupt source was triggered in > > SPI_SR, since only EOQ and TCFQ modes trigger interrupts anyway (see the > > writes to SPI_RSER). In DMA mode, the RSER is configured for RFDF_DIRS=1 > > and TFFF_DIRS=1, aka FIFO events generate eDMA requests and not CPU > > interrupts. > > It's also good to check interrupt sources in case the interrupt line > might be shared, that means that it's possible that the interrupt > handler will be called when there's no interrupt at all from the IP. It > also helps with robustness in case there's some system error though > that's (hopefully!) a lot less common. This driver does set IRQF_SHARED > so it does support that, I don't know how many systems could do it but > it seems a shame to remove the support from the driver. Ok, I hadn't thought of that, I'll add the check back.
On Wed, 21 Aug 2019 at 01:43, Vladimir Oltean <olteanv@gmail.com> wrote: > > On Tue, 20 Aug 2019 at 16:02, Mark Brown <broonie@kernel.org> wrote: > > > > On Sun, Aug 18, 2019 at 09:01:14PM +0300, Vladimir Oltean wrote: > > > There is no point in checking which interrupt source was triggered in > > > SPI_SR, since only EOQ and TCFQ modes trigger interrupts anyway (see the > > > writes to SPI_RSER). In DMA mode, the RSER is configured for RFDF_DIRS=1 > > > and TFFF_DIRS=1, aka FIFO events generate eDMA requests and not CPU > > > interrupts. > > > > It's also good to check interrupt sources in case the interrupt line > > might be shared, that means that it's possible that the interrupt > > handler will be called when there's no interrupt at all from the IP. It > > also helps with robustness in case there's some system error though > > that's (hopefully!) a lot less common. This driver does set IRQF_SHARED > > so it does support that, I don't know how many systems could do it but > > it seems a shame to remove the support from the driver. > > Ok, I hadn't thought of that, I'll add the check back. But shouldn't it be returning IRQ_NONE in that case? Right now it's returning IRQ_HANDLED.
On Wed, Aug 21, 2019 at 01:49:15AM +0300, Vladimir Oltean wrote: > But shouldn't it be returning IRQ_NONE in that case? Right now it's > returning IRQ_HANDLED. Yes, that's a bug that should be fixed.
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c index 790cb02fc181..2993d15f640e 100644 --- a/drivers/spi/spi-fsl-dspi.c +++ b/drivers/spi/spi-fsl-dspi.c @@ -658,47 +658,45 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id) regmap_read(dspi->regmap, SPI_SR, &spi_sr); regmap_write(dspi->regmap, SPI_SR, spi_sr); + /* Get transfer counter (in number of SPI transfers). It was + * reset to 0 when transfer(s) were started. + */ + regmap_read(dspi->regmap, SPI_TCR, &spi_tcr); + spi_tcnt = SPI_TCR_GET_TCNT(spi_tcr); + /* Update total number of bytes that were transferred */ + msg->actual_length += spi_tcnt * dspi->bytes_per_word; + + trans_mode = dspi->devtype_data->trans_mode; + switch (trans_mode) { + case DSPI_EOQ_MODE: + dspi_eoq_read(dspi); + break; + case DSPI_TCFQ_MODE: + dspi_tcfq_read(dspi); + break; + default: + dev_err(&dspi->pdev->dev, "unsupported trans_mode %u\n", + trans_mode); + return IRQ_HANDLED; + } - if (spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)) { - /* Get transfer counter (in number of SPI transfers). It was - * reset to 0 when transfer(s) were started. - */ - regmap_read(dspi->regmap, SPI_TCR, &spi_tcr); - spi_tcnt = SPI_TCR_GET_TCNT(spi_tcr); - /* Update total number of bytes that were transferred */ - msg->actual_length += spi_tcnt * dspi->bytes_per_word; - - trans_mode = dspi->devtype_data->trans_mode; - switch (trans_mode) { - case DSPI_EOQ_MODE: - dspi_eoq_read(dspi); - break; - case DSPI_TCFQ_MODE: - dspi_tcfq_read(dspi); - break; - default: - dev_err(&dspi->pdev->dev, "unsupported trans_mode %u\n", - trans_mode); - return IRQ_HANDLED; - } + if (!dspi->len) { + dspi->waitflags = 1; + wake_up_interruptible(&dspi->waitq); + return IRQ_HANDLED; + } - if (!dspi->len) { - dspi->waitflags = 1; - wake_up_interruptible(&dspi->waitq); - } else { - switch (trans_mode) { - case DSPI_EOQ_MODE: - dspi_eoq_write(dspi); - break; - case DSPI_TCFQ_MODE: - dspi_tcfq_write(dspi); - break; - default: - dev_err(&dspi->pdev->dev, - "unsupported trans_mode %u\n", - trans_mode); - } - } + switch (trans_mode) { + case DSPI_EOQ_MODE: + dspi_eoq_write(dspi); + break; + case DSPI_TCFQ_MODE: + dspi_tcfq_write(dspi); + break; + default: + dev_err(&dspi->pdev->dev, + "unsupported trans_mode %u\n", + trans_mode); } return IRQ_HANDLED;
There is no point in checking which interrupt source was triggered in SPI_SR, since only EOQ and TCFQ modes trigger interrupts anyway (see the writes to SPI_RSER). In DMA mode, the RSER is configured for RFDF_DIRS=1 and TFFF_DIRS=1, aka FIFO events generate eDMA requests and not CPU interrupts. Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- drivers/spi/spi-fsl-dspi.c | 76 +++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 39 deletions(-)