| Message ID | 1563398164-2679-2-git-send-email-wahrenst@gmx.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Wed, Jul 17, 2019 at 11:16:01PM +0200, Stefan Wahren wrote: > + /* check if we got interrupt enabled */ > + if (!(bcm2835_rd(bs, BCM2835_SPI_CS) & BCM2835_SPI_CS_INTR)) > + return IRQ_NONE; > + Is that checking if the interrupt is enabled or if it is asserted?
Hi Mark, Am 18.07.19 um 14:42 schrieb Mark Brown: > On Wed, Jul 17, 2019 at 11:16:01PM +0200, Stefan Wahren wrote: > >> + /* check if we got interrupt enabled */ >> + if (!(bcm2835_rd(bs, BCM2835_SPI_CS) & BCM2835_SPI_CS_INTR)) >> + return IRQ_NONE; >> + > Is that checking if the interrupt is enabled or if it is asserted? the BCM2835 doesn't provide a SPI register, which shows that the interrupt has been asserted. So i think, Martin tried to adapt the workaround from spi-bcm2835-aux which has the same problem. Stefan
On 7/18/19 10:53 AM, Stefan Wahren wrote: > Hi Mark, > > Am 18.07.19 um 14:42 schrieb Mark Brown: >> On Wed, Jul 17, 2019 at 11:16:01PM +0200, Stefan Wahren wrote: >> >>> + /* check if we got interrupt enabled */ >>> + if (!(bcm2835_rd(bs, BCM2835_SPI_CS) & BCM2835_SPI_CS_INTR)) >>> + return IRQ_NONE; >>> + >> Is that checking if the interrupt is enabled or if it is asserted? > > the BCM2835 doesn't provide a SPI register, which shows that the > interrupt has been asserted. > > So i think, Martin tried to adapt the workaround from spi-bcm2835-aux > which has the same problem. I was about to submit a change to address that since we also have that shared interrupt on BCM7211: https://github.com/ffainelli/linux/commit/15d96d82bd42991dc71369128131312d5338f65c Martin's patch is more efficient in terms of amount of register accesses, but I am bit worried (based on the register description) that the INTR bit is only asserted with the read FIFO crossing a certain condition and that a TX only transfer may not be captured by that condition. Maybe we can just check spi_controller::idling to determine if that specific instance generated an interrupt?
Hi Florian, Am 18.07.19 um 20:05 schrieb Florian Fainelli: > On 7/18/19 10:53 AM, Stefan Wahren wrote: >> Hi Mark, >> > I was about to submit a change to address that since we also have that > shared interrupt on BCM7211: > > https://github.com/ffainelli/linux/commit/15d96d82bd42991dc71369128131312d5338f65c > > Martin's patch is more efficient in terms of amount of register > accesses, but I am bit worried (based on the register description) that > the INTR bit is only asserted with the read FIFO crossing a certain > condition and that a TX only transfer may not be captured by that condition. > > Maybe we can just check spi_controller::idling to determine if that > specific instance generated an interrupt? sorry, i'm not that SPI expert. I suggest to drop this non-essential patch from the series and discuss this separate. Stefan
On Thu, Jul 18, 2019 at 07:53:43PM +0200, Stefan Wahren wrote: > Am 18.07.19 um 14:42 schrieb Mark Brown: > > On Wed, Jul 17, 2019 at 11:16:01PM +0200, Stefan Wahren wrote: > >> + /* check if we got interrupt enabled */ > >> + if (!(bcm2835_rd(bs, BCM2835_SPI_CS) & BCM2835_SPI_CS_INTR)) > >> + return IRQ_NONE; > > Is that checking if the interrupt is enabled or if it is asserted? > the BCM2835 doesn't provide a SPI register, which shows that the > interrupt has been asserted. > So i think, Martin tried to adapt the workaround from spi-bcm2835-aux > which has the same problem. OK, I don't know what that workaround was or exactly what this is checking but if it's just checking if the interrupt was enabled then there's going to be cases where this gets called while interrupts are enabled but due to another device asserting the interrupt. If the driver can cope with that and this is just an optimization then fine but if it's relying on this there's an issue.
On Thu, Jul 18, 2019 at 08:21:36PM +0200, Stefan Wahren wrote: > Am 18.07.19 um 20:05 schrieb Florian Fainelli: > > On 7/18/19 10:53 AM, Stefan Wahren wrote: > > Martin's patch is more efficient in terms of amount of register > > accesses, but I am bit worried (based on the register description) that > > the INTR bit is only asserted with the read FIFO crossing a certain > > condition and that a TX only transfer may not be captured by that condition. It looks like the driver sets the bit for TX only transfers so that's probably fine but I might be missing something. > > Maybe we can just check spi_controller::idling to determine if that > > specific instance generated an interrupt? > sorry, i'm not that SPI expert. I suggest to drop this non-essential > patch from the series and discuss this separate. I'm not opposed to the patch, I'm just concerned based on the combination of the description and the code that it might not be doing what's expected. If it's mostly just an optimization that provides a fast path in the case the interrupt is shared rather than a correctness thing then it's fine. A comment in the commit message or the code about this being an optimization would be a good idea though.
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c index 6f243a9..50969ae 100644 --- a/drivers/spi/spi-bcm2835.c +++ b/drivers/spi/spi-bcm2835.c @@ -346,6 +346,10 @@ static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id) if (bs->tx_len && cs & BCM2835_SPI_CS_DONE) bcm2835_wr_fifo_blind(bs, BCM2835_SPI_FIFO_SIZE); + /* check if we got interrupt enabled */ + if (!(bcm2835_rd(bs, BCM2835_SPI_CS) & BCM2835_SPI_CS_INTR)) + return IRQ_NONE; + /* Read as many bytes as possible from FIFO */ bcm2835_rd_fifo(bs); /* Write as many bytes as possible to FIFO */ @@ -1028,8 +1032,8 @@ static int bcm2835_spi_probe(struct platform_device *pdev) bcm2835_wr(bs, BCM2835_SPI_CS, BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX); - err = devm_request_irq(&pdev->dev, bs->irq, bcm2835_spi_interrupt, 0, - dev_name(&pdev->dev), ctlr); + err = devm_request_irq(&pdev->dev, bs->irq, bcm2835_spi_interrupt, + IRQF_SHARED, dev_name(&pdev->dev), ctlr); if (err) { dev_err(&pdev->dev, "could not request IRQ: %d\n", err); goto out_clk_disable;