| Message ID | 20200528185805.28991-1-nsaenzjulienne@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ecfbd3cf3b8bb73ac6a80ddf430b5912fd4402a6 |
| Headers | show |
| Series | spi: bcm2835: Enable shared interrupt support | expand |
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>
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
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
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.
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.
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
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.
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
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;