From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Wahren Subject: Re: [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared handler Date: Thu, 3 May 2018 20:31:37 +0200 (CEST) Message-ID: <205406626.92407.1525372297575@email.1und1.de> References: <20180503180945.3502-1-robh@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: Rob Herring , Florian Fainelli , Scott Branden , Marc Zyngier , Ray Jui , Alexander Graf , linux-spi@vger.kernel.org, Eric Anholt , Mark Brown , bcm-kernel-feedback-list@broadcom.com, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org To: Martin Sperl , =?UTF-8?Q?Noralf_Tr=C3=B8nnes?= , Phil Elwell Return-path: In-Reply-To: <20180503180945.3502-1-robh@kernel.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org List-Id: linux-spi.vger.kernel.org Hi, [added Martin, Noralf and Phil] > Rob Herring hat am 3. Mai 2018 um 20:09 geschrieben: > > > The BCM2835 AUX SPI has a shared interrupt line (with AUX UART). > Downstream fixes this with an AUX irqchip to demux the IRQ sources and a > DT change which breaks compatibility with older kernels. The AUX irqchip > was already rejected for upstream[1] and the DT change would break > working systems if the DTB is updated to a newer one. The latter issue > was brought to my attention by Alex Graf. > > The root cause however is a bug in the shared handler. Shared handlers > must check that interrupts are actually enabled before servicing the > interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled. > > [1] https://patchwork.kernel.org/patch/9781221/ > > Cc: Alexander Graf > Cc: Marc Zyngier > Cc: Mark Brown > Cc: Eric Anholt > Cc: Stefan Wahren > Cc: Florian Fainelli > Cc: Ray Jui > Cc: Scott Branden > Cc: bcm-kernel-feedback-list@broadcom.com > Cc: linux-spi@vger.kernel.org > Cc: linux-rpi-kernel@lists.infradead.org > Cc: linux-arm-kernel@lists.infradead.org > Signed-off-by: Rob Herring > --- > Compile tested only. I'll add something to the related github issue on > this and hopefully someone can test and confirm. > > I'm assuming the 8250 driver can handle shared irqs correctly. > > Rob > > drivers/spi/spi-bcm2835aux.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c > index 1431cb98fe40..3094d818cf06 100644 > --- a/drivers/spi/spi-bcm2835aux.c > +++ b/drivers/spi/spi-bcm2835aux.c > @@ -184,6 +184,11 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id) > struct bcm2835aux_spi *bs = spi_master_get_devdata(master); > irqreturn_t ret = IRQ_NONE; > > + /* IRQ may be shared, so return if our interrupts are disabled */ > + if (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_CNTL1) & > + (BCM2835_AUX_SPI_CNTL1_TXEMPTY | BCM2835_AUX_SPI_CNTL1_IDLE))) > + return ret; > + > /* check if we have data to read */ > while (bs->rx_len && > (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) & > -- > 2.17.0 does anyone have a setup to test this patch (i assume it requires a compute module)? @Rob: Thank you very much for this patch From mboxrd@z Thu Jan 1 00:00:00 1970 From: stefan.wahren@i2se.com (Stefan Wahren) Date: Thu, 3 May 2018 20:31:37 +0200 (CEST) Subject: [PATCH] spi: bcm2835aux: ensure interrupts are enabled for shared handler In-Reply-To: <20180503180945.3502-1-robh@kernel.org> References: <20180503180945.3502-1-robh@kernel.org> Message-ID: <205406626.92407.1525372297575@email.1und1.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi, [added Martin, Noralf and Phil] > Rob Herring hat am 3. Mai 2018 um 20:09 geschrieben: > > > The BCM2835 AUX SPI has a shared interrupt line (with AUX UART). > Downstream fixes this with an AUX irqchip to demux the IRQ sources and a > DT change which breaks compatibility with older kernels. The AUX irqchip > was already rejected for upstream[1] and the DT change would break > working systems if the DTB is updated to a newer one. The latter issue > was brought to my attention by Alex Graf. > > The root cause however is a bug in the shared handler. Shared handlers > must check that interrupts are actually enabled before servicing the > interrupt. Add a check that the TXEMPTY or IDLE interrupts are enabled. > > [1] https://patchwork.kernel.org/patch/9781221/ > > Cc: Alexander Graf > Cc: Marc Zyngier > Cc: Mark Brown > Cc: Eric Anholt > Cc: Stefan Wahren > Cc: Florian Fainelli > Cc: Ray Jui > Cc: Scott Branden > Cc: bcm-kernel-feedback-list at broadcom.com > Cc: linux-spi at vger.kernel.org > Cc: linux-rpi-kernel at lists.infradead.org > Cc: linux-arm-kernel at lists.infradead.org > Signed-off-by: Rob Herring > --- > Compile tested only. I'll add something to the related github issue on > this and hopefully someone can test and confirm. > > I'm assuming the 8250 driver can handle shared irqs correctly. > > Rob > > drivers/spi/spi-bcm2835aux.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c > index 1431cb98fe40..3094d818cf06 100644 > --- a/drivers/spi/spi-bcm2835aux.c > +++ b/drivers/spi/spi-bcm2835aux.c > @@ -184,6 +184,11 @@ static irqreturn_t bcm2835aux_spi_interrupt(int irq, void *dev_id) > struct bcm2835aux_spi *bs = spi_master_get_devdata(master); > irqreturn_t ret = IRQ_NONE; > > + /* IRQ may be shared, so return if our interrupts are disabled */ > + if (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_CNTL1) & > + (BCM2835_AUX_SPI_CNTL1_TXEMPTY | BCM2835_AUX_SPI_CNTL1_IDLE))) > + return ret; > + > /* check if we have data to read */ > while (bs->rx_len && > (!(bcm2835aux_rd(bs, BCM2835_AUX_SPI_STAT) & > -- > 2.17.0 does anyone have a setup to test this patch (i assume it requires a compute module)? @Rob: Thank you very much for this patch