[RFC,15/18] spi: bcm2835: enable shared interrupt support
diff mbox series

Message ID 1563398164-2679-2-git-send-email-wahrenst@gmx.net
State New
Headers show
Series
  • Untitled series #147121
Related show

Commit Message

Stefan Wahren July 17, 2019, 9:16 p.m. UTC
From: Martin Sperl <kernel@martin.sperl.org>

The new BCM2838 share one interrupt for multiple instances of the BCM2835
SPI controller. So this enables shared interrupt support for them.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/spi/spi-bcm2835.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

--
2.7.4

Comments

Mark Brown July 18, 2019, 12:42 p.m. UTC | #1
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?
Stefan Wahren July 18, 2019, 5:53 p.m. UTC | #2
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
Florian Fainelli July 18, 2019, 6:05 p.m. UTC | #3
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?
Stefan Wahren July 18, 2019, 6:21 p.m. UTC | #4
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
Mark Brown July 18, 2019, 6:52 p.m. UTC | #5
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.
Mark Brown July 24, 2019, 5:15 p.m. UTC | #6
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.

Patch
diff mbox series

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;