[PATCH] spi: bcm2835: Enable shared interrupt support

[PATCH] spi: bcm2835: Enable shared interrupt support

[Nicolas Saenz Julienne]

From: Martin Sperl <kernel@martin.sperl.org>

bcm2711, Rasberry Pi 4's SoC, shares one interrupt for multiple
instances of the bcm2835 SPI controller. So this enables shared
interrupt support for them.

The early bail out in the interrupt routine avoids messing with buffers
of transfers being done by other means. Otherwise, the driver can handle
receiving interrupts asserted by other controllers during an IRQ based
transfer.

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/spi/spi-bcm2835.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 11c235879bb7..ca1085c812f2 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -379,6 +379,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 */
@@ -1340,8 +1344,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_dma_release;
-- 
2.26.2

Re: [PATCH] spi: bcm2835: Enable shared interrupt support

[Florian Fainelli]

On 5/28/20 11:58 AM, Nicolas Saenz Julienne wrote:
> From: Martin Sperl <kernel@martin.sperl.org>
> 
> bcm2711, Rasberry Pi 4's SoC, shares one interrupt for multiple
> instances of the bcm2835 SPI controller. So this enables shared
> interrupt support for them.
> 
> The early bail out in the interrupt routine avoids messing with buffers
> of transfers being done by other means. Otherwise, the driver can handle
> receiving interrupts asserted by other controllers during an IRQ based
> transfer.
> 
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks Nicolas, we have been using this patch on 7211 (which is very
similar to 2711):

Tested-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH] spi: bcm2835: Enable shared interrupt support

[Mark Brown]

On Thu, 28 May 2020 20:58:04 +0200, Nicolas Saenz Julienne wrote:
> bcm2711, Rasberry Pi 4's SoC, shares one interrupt for multiple
> instances of the bcm2835 SPI controller. So this enables shared
> interrupt support for them.
> 
> The early bail out in the interrupt routine avoids messing with buffers
> of transfers being done by other means. Otherwise, the driver can handle
> receiving interrupts asserted by other controllers during an IRQ based
> transfer.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: bcm2835: Enable shared interrupt support
      commit: ecfbd3cf3b8bb73ac6a80ddf430b5912fd4402a6

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH] spi: bcm2835: Enable shared interrupt support

[Lukas Wunner]

On Thu, May 28, 2020 at 08:58:04PM +0200, Nicolas Saenz Julienne wrote:
> --- a/drivers/spi/spi-bcm2835.c
> +++ b/drivers/spi/spi-bcm2835.c
> @@ -379,6 +379,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 */

This definitely looks wrong.  The check whether the interrupt is enabled
should be moved *before* the conditional calls to bcm2835_rd_fifo_blind()
and bcm2835_wr_fifo_blind(), i.e. to the top of the function.

Otherwise if an interrupt is raised by another SPI controller,
this function may perform read/write accesses to the FIFO and
interfere with an ongoing transfer in DMA or poll mode.

Also, instead of performing an MMIO read, just use the "cs" variable
which was assigned at the top of the function.

The code comment should explain that the check is necessary because the
interrupt may be shared with other controllers on the BCM2711.

Finally, it would be nice if the check would be optimized away when
compiling for pre-RasPi4 products, maybe something like:

+	if (IS_ENABLED(CONFIG_ARM_LPAE) && !(cs & BCM2835_SPI_CS_INTR))
+		return IRQ_NONE;

Thanks,

Lukas

Re: [PATCH] spi: bcm2835: Enable shared interrupt support

[Florian Fainelli]

On 5/29/20 10:43 AM, Lukas Wunner wrote:
> On Thu, May 28, 2020 at 08:58:04PM +0200, Nicolas Saenz Julienne wrote:
>> --- a/drivers/spi/spi-bcm2835.c
>> +++ b/drivers/spi/spi-bcm2835.c
>> @@ -379,6 +379,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 */
> 
> This definitely looks wrong.  The check whether the interrupt is enabled
> should be moved *before* the conditional calls to bcm2835_rd_fifo_blind()
> and bcm2835_wr_fifo_blind(), i.e. to the top of the function.
> 
> Otherwise if an interrupt is raised by another SPI controller,
> this function may perform read/write accesses to the FIFO and
> interfere with an ongoing transfer in DMA or poll mode.
> 
> Also, instead of performing an MMIO read, just use the "cs" variable
> which was assigned at the top of the function.
> 
> The code comment should explain that the check is necessary because the
> interrupt may be shared with other controllers on the BCM2711.
> 
> Finally, it would be nice if the check would be optimized away when
> compiling for pre-RasPi4 products, maybe something like:
> 
> +	if (IS_ENABLED(CONFIG_ARM_LPAE) && !(cs & BCM2835_SPI_CS_INTR))
> +		return IRQ_NONE;

Rather than keying this off ARM_LPAE or any other option, this should be
keyed off a compatible string, that way we can even conditionally pass
IRQF_SHARED to the interrupt handler if we care so much about performance.
-- 
Florian

Re: [PATCH] spi: bcm2835: Enable shared interrupt support

[Lukas Wunner]

On Fri, May 29, 2020 at 10:46:01AM -0700, Florian Fainelli wrote:
> On 5/29/20 10:43 AM, Lukas Wunner wrote:
> > On Thu, May 28, 2020 at 08:58:04PM +0200, Nicolas Saenz Julienne wrote:
> >> --- a/drivers/spi/spi-bcm2835.c
> >> +++ b/drivers/spi/spi-bcm2835.c
> >> @@ -379,6 +379,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 */
[...]
> > Finally, it would be nice if the check would be optimized away when
> > compiling for pre-RasPi4 products, maybe something like:
> > 
> > +	if (IS_ENABLED(CONFIG_ARM_LPAE) && !(cs & BCM2835_SPI_CS_INTR))
> > +		return IRQ_NONE;
> 
> Rather than keying this off ARM_LPAE or any other option, this should be
> keyed off a compatible string, that way we can even conditionally pass
> IRQF_SHARED to the interrupt handler if we care so much about performance.

But a compatible string can't be checked at compile time, can it?

The point is that at the least the Foundation compiles and ships a separate
kernel for each of the three platforms BCM2835, BCM2837, BCM2711.  It's
unnecessary to check whether an interrupt was actually raised if we *know*
in advance that it's not shared (as is the case with kernels compiled for
BCM2835 and BCM2837).

Thanks,

Lukas

Re: [PATCH] spi: bcm2835: Enable shared interrupt support

[Florian Fainelli]

On 5/29/20 10:53 AM, Lukas Wunner wrote:
> On Fri, May 29, 2020 at 10:46:01AM -0700, Florian Fainelli wrote:
>> On 5/29/20 10:43 AM, Lukas Wunner wrote:
>>> On Thu, May 28, 2020 at 08:58:04PM +0200, Nicolas Saenz Julienne wrote:
>>>> --- a/drivers/spi/spi-bcm2835.c
>>>> +++ b/drivers/spi/spi-bcm2835.c
>>>> @@ -379,6 +379,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 */
> [...]
>>> Finally, it would be nice if the check would be optimized away when
>>> compiling for pre-RasPi4 products, maybe something like:
>>>
>>> +	if (IS_ENABLED(CONFIG_ARM_LPAE) && !(cs & BCM2835_SPI_CS_INTR))
>>> +		return IRQ_NONE;
>>
>> Rather than keying this off ARM_LPAE or any other option, this should be
>> keyed off a compatible string, that way we can even conditionally pass
>> IRQF_SHARED to the interrupt handler if we care so much about performance.
> 
> But a compatible string can't be checked at compile time, can it?

No, but you can have a different interrupt handler that it set at
runtime if you want to completely eliminate this comparison.

My point is that CONFIG_ARM_LPAE is just too brittle, there is nothing
that prevents you from using a non-LPAE kernel on the Pi 4, even PCIe
could be made to work if using super section mappings to map the PCIe
outbound space. Even on models with over 4GB of DRAM, if you are willing
to lose some of it, it can work.

> 
> The point is that at the least the Foundation compiles and ships a separate
> kernel for each of the three platforms BCM2835, BCM2837, BCM2711.  It's
> unnecessary to check whether an interrupt was actually raised if we *know*
> in advance that it's not shared (as is the case with kernels compiled for
> BCM2835 and BCM2837).

I am fine with any solution that does not involve keying off
CONFIG_ARM_LPAE to discriminate 2711 from any other chip.
-- 
Florian

Re: [PATCH] spi: bcm2835: Enable shared interrupt support

[Lukas Wunner]

On Fri, May 29, 2020 at 11:03:48AM -0700, Florian Fainelli wrote:
> On 5/29/20 10:53 AM, Lukas Wunner wrote:
> > On Fri, May 29, 2020 at 10:46:01AM -0700, Florian Fainelli wrote:
> >> On 5/29/20 10:43 AM, Lukas Wunner wrote:
> >>> Finally, it would be nice if the check would be optimized away when
> >>> compiling for pre-RasPi4 products, maybe something like:
> >>>
> >>> +	if (IS_ENABLED(CONFIG_ARM_LPAE) && !(cs & BCM2835_SPI_CS_INTR))
> >>> +		return IRQ_NONE;
> >>
> >> Rather than keying this off ARM_LPAE or any other option, this should be
> >> keyed off a compatible string, that way we can even conditionally pass
> >> IRQF_SHARED to the interrupt handler if we care so much about performance.
> > 
> > But a compatible string can't be checked at compile time, can it?
> 
> No, but you can have a different interrupt handler that it set at
> runtime if you want to completely eliminate this comparison.

Good idea.  In fact the IRQ handler for platforms with shared interrupts
could just be a wrapper which performs the BCM2835_SPI_CS_INTR check
then tail-calls the existing IRQ handler.  The compiler would just
inline it and everything would be fine.


> My point is that CONFIG_ARM_LPAE is just too brittle, there is nothing
> that prevents you from using a non-LPAE kernel on the Pi 4, even PCIe
> could be made to work if using super section mappings to map the PCIe
> outbound space. Even on models with over 4GB of DRAM, if you are willing
> to lose some of it, it can work.

Agreed.

Thanks,

Lukas

Re: [PATCH] spi: bcm2835: Enable shared interrupt support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 313 bytes --]

On Fri, May 29, 2020 at 07:43:58PM +0200, Lukas Wunner wrote:

> This definitely looks wrong.  The check whether the interrupt is enabled
> should be moved *before* the conditional calls to bcm2835_rd_fifo_blind()
> and bcm2835_wr_fifo_blind(), i.e. to the top of the function.

Hrm, right - I'll drop the patch.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]