Re: [PATCH 11/18] spi: qup: properly detect extra interrupts
Hi Andy, On 6/15/2017 1:29 AM, Andy Gross wrote: > On Wed, Jun 14, 2017 at 12:57:25PM +0530, Sricharan R wrote: >> Hi Varada, >> >> On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote: >>> It's possible for a SPI transaction to complete and get another >>> interrupt and have it processed on the same spi_transfer before the >>> transfer_one can set it to NULL. >>> >>> This masks unexpected interrupts, so let's set the spi_transfer to >>> NULL in the interrupt once the transaction is done. So we can >>> properly detect these bad interrupts and print warning messages. >>> >>> Signed-off-by: Matthew McClintock>>> Signed-off-by: Varadarajan Narayanan >>> --- >>> drivers/spi/spi-qup.c | 20 +++- >>> 1 file changed, 11 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c >>> index bd53e82..1a2a9d9 100644 >>> --- a/drivers/spi/spi-qup.c >>> +++ b/drivers/spi/spi-qup.c >>> @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void >>> *dev_id) >>> struct spi_qup *controller = dev_id; >>> struct spi_transfer *xfer; >>> u32 opflags, qup_err, spi_err; >>> - unsigned long flags; >>> int error = 0; >>> + bool done = 0; >>> >>> - spin_lock_irqsave(>lock, flags); >>> + spin_lock(>lock); >>> xfer = controller->xfer; >>> controller->xfer = NULL; >>> - spin_unlock_irqrestore(>lock, flags); >>> + spin_unlock(>lock); >> >> Why change the locking here ? >> >>> >>> qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); >>> spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); >>> @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void >>> *dev_id) >>> spi_qup_write(controller, xfer); >>> } >>> >>> - spin_lock_irqsave(>lock, flags); >>> - controller->error = error; >>> - controller->xfer = xfer; >>> - spin_unlock_irqrestore(>lock, flags); >>> - >>> /* re-read opflags as flags may have changed due to actions above */ >>> opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); >>> >>> if ((controller->rx_bytes == xfer->len && >>> (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) >>> + done = true; >>> + >>> + spin_lock(>lock); >>> + controller->error = error; >>> + controller->xfer = done ? NULL : xfer; >>> + spin_unlock(>lock); >>> + >>> + if (done) >>> complete(>done); >>> >> Its not clear, why the driver is setting the controller->xfer = NULL >> and restoring it inside the irq. This patch seems to fix things on >> top of that. > > I think the original intent was to make sure that the irqhandler knew that > there > was no outstanding transaction. This begs the question of why that would ever > be necessary. I think it would suffice to rework all of that to remove that > behavior and perhaps enable/disable the irq as we need to during transactions. > > I've never been a fan of the controller->xfer being set to NULL. Agree, that part needs fixing in the original code. Also patch #10 changing the behavior to acknowledge the interrupts only when its spurious does not look correct. Trying to fix the original should simplify or avoid the spurious case itself. Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 11/18] spi: qup: properly detect extra interrupts
Hi Andy, On 6/15/2017 1:29 AM, Andy Gross wrote: > On Wed, Jun 14, 2017 at 12:57:25PM +0530, Sricharan R wrote: >> Hi Varada, >> >> On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote: >>> It's possible for a SPI transaction to complete and get another >>> interrupt and have it processed on the same spi_transfer before the >>> transfer_one can set it to NULL. >>> >>> This masks unexpected interrupts, so let's set the spi_transfer to >>> NULL in the interrupt once the transaction is done. So we can >>> properly detect these bad interrupts and print warning messages. >>> >>> Signed-off-by: Matthew McClintock >>> Signed-off-by: Varadarajan Narayanan >>> --- >>> drivers/spi/spi-qup.c | 20 +++- >>> 1 file changed, 11 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c >>> index bd53e82..1a2a9d9 100644 >>> --- a/drivers/spi/spi-qup.c >>> +++ b/drivers/spi/spi-qup.c >>> @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void >>> *dev_id) >>> struct spi_qup *controller = dev_id; >>> struct spi_transfer *xfer; >>> u32 opflags, qup_err, spi_err; >>> - unsigned long flags; >>> int error = 0; >>> + bool done = 0; >>> >>> - spin_lock_irqsave(>lock, flags); >>> + spin_lock(>lock); >>> xfer = controller->xfer; >>> controller->xfer = NULL; >>> - spin_unlock_irqrestore(>lock, flags); >>> + spin_unlock(>lock); >> >> Why change the locking here ? >> >>> >>> qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); >>> spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); >>> @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void >>> *dev_id) >>> spi_qup_write(controller, xfer); >>> } >>> >>> - spin_lock_irqsave(>lock, flags); >>> - controller->error = error; >>> - controller->xfer = xfer; >>> - spin_unlock_irqrestore(>lock, flags); >>> - >>> /* re-read opflags as flags may have changed due to actions above */ >>> opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); >>> >>> if ((controller->rx_bytes == xfer->len && >>> (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) >>> + done = true; >>> + >>> + spin_lock(>lock); >>> + controller->error = error; >>> + controller->xfer = done ? NULL : xfer; >>> + spin_unlock(>lock); >>> + >>> + if (done) >>> complete(>done); >>> >> Its not clear, why the driver is setting the controller->xfer = NULL >> and restoring it inside the irq. This patch seems to fix things on >> top of that. > > I think the original intent was to make sure that the irqhandler knew that > there > was no outstanding transaction. This begs the question of why that would ever > be necessary. I think it would suffice to rework all of that to remove that > behavior and perhaps enable/disable the irq as we need to during transactions. > > I've never been a fan of the controller->xfer being set to NULL. Agree, that part needs fixing in the original code. Also patch #10 changing the behavior to acknowledge the interrupts only when its spurious does not look correct. Trying to fix the original should simplify or avoid the spurious case itself. Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 11/18] spi: qup: properly detect extra interrupts
On Wed, Jun 14, 2017 at 2:59 PM, Andy Grosswrote: > On Wed, Jun 14, 2017 at 12:57:25PM +0530, Sricharan R wrote: >> Hi Varada, >> >> On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote: >> > It's possible for a SPI transaction to complete and get another >> > interrupt and have it processed on the same spi_transfer before the >> > transfer_one can set it to NULL. >> > >> > This masks unexpected interrupts, so let's set the spi_transfer to >> > NULL in the interrupt once the transaction is done. So we can >> > properly detect these bad interrupts and print warning messages. >> > >> > Signed-off-by: Matthew McClintock >> > Signed-off-by: Varadarajan Narayanan >> > --- >> > drivers/spi/spi-qup.c | 20 +++- >> > 1 file changed, 11 insertions(+), 9 deletions(-) >> > >> > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c >> > index bd53e82..1a2a9d9 100644 >> > --- a/drivers/spi/spi-qup.c >> > +++ b/drivers/spi/spi-qup.c >> > @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void >> > *dev_id) >> > struct spi_qup *controller = dev_id; >> > struct spi_transfer *xfer; >> > u32 opflags, qup_err, spi_err; >> > - unsigned long flags; >> > int error = 0; >> > + bool done = 0; >> > >> > - spin_lock_irqsave(>lock, flags); >> > + spin_lock(>lock); >> > xfer = controller->xfer; >> > controller->xfer = NULL; >> > - spin_unlock_irqrestore(>lock, flags); >> > + spin_unlock(>lock); >> >> Why change the locking here ? >> >> > >> > qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); >> > spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); >> > @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void >> > *dev_id) >> > spi_qup_write(controller, xfer); >> > } >> > >> > - spin_lock_irqsave(>lock, flags); >> > - controller->error = error; >> > - controller->xfer = xfer; >> > - spin_unlock_irqrestore(>lock, flags); >> > - >> > /* re-read opflags as flags may have changed due to actions above */ >> > opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); >> > >> > if ((controller->rx_bytes == xfer->len && >> > (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) >> > + done = true; >> > + >> > + spin_lock(>lock); >> > + controller->error = error; >> > + controller->xfer = done ? NULL : xfer; >> > + spin_unlock(>lock); >> > + >> > + if (done) >> > complete(>done); >> > >> Its not clear, why the driver is setting the controller->xfer = NULL >> and restoring it inside the irq. This patch seems to fix things on >> top of that. > > I think the original intent was to make sure that the irqhandler knew that > there > was no outstanding transaction. This begs the question of why that would ever > be necessary. I think it would suffice to rework all of that to remove that > behavior and perhaps enable/disable the irq as we need to during transactions. > > I've never been a fan of the controller->xfer being set to NULL. Also, this patch presumably fixes an issue seen on Dakota SoCs, doesn't add or remote the NULL bits. -M
Re: [PATCH 11/18] spi: qup: properly detect extra interrupts
On Wed, Jun 14, 2017 at 2:59 PM, Andy Gross wrote: > On Wed, Jun 14, 2017 at 12:57:25PM +0530, Sricharan R wrote: >> Hi Varada, >> >> On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote: >> > It's possible for a SPI transaction to complete and get another >> > interrupt and have it processed on the same spi_transfer before the >> > transfer_one can set it to NULL. >> > >> > This masks unexpected interrupts, so let's set the spi_transfer to >> > NULL in the interrupt once the transaction is done. So we can >> > properly detect these bad interrupts and print warning messages. >> > >> > Signed-off-by: Matthew McClintock >> > Signed-off-by: Varadarajan Narayanan >> > --- >> > drivers/spi/spi-qup.c | 20 +++- >> > 1 file changed, 11 insertions(+), 9 deletions(-) >> > >> > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c >> > index bd53e82..1a2a9d9 100644 >> > --- a/drivers/spi/spi-qup.c >> > +++ b/drivers/spi/spi-qup.c >> > @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void >> > *dev_id) >> > struct spi_qup *controller = dev_id; >> > struct spi_transfer *xfer; >> > u32 opflags, qup_err, spi_err; >> > - unsigned long flags; >> > int error = 0; >> > + bool done = 0; >> > >> > - spin_lock_irqsave(>lock, flags); >> > + spin_lock(>lock); >> > xfer = controller->xfer; >> > controller->xfer = NULL; >> > - spin_unlock_irqrestore(>lock, flags); >> > + spin_unlock(>lock); >> >> Why change the locking here ? >> >> > >> > qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); >> > spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); >> > @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void >> > *dev_id) >> > spi_qup_write(controller, xfer); >> > } >> > >> > - spin_lock_irqsave(>lock, flags); >> > - controller->error = error; >> > - controller->xfer = xfer; >> > - spin_unlock_irqrestore(>lock, flags); >> > - >> > /* re-read opflags as flags may have changed due to actions above */ >> > opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); >> > >> > if ((controller->rx_bytes == xfer->len && >> > (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) >> > + done = true; >> > + >> > + spin_lock(>lock); >> > + controller->error = error; >> > + controller->xfer = done ? NULL : xfer; >> > + spin_unlock(>lock); >> > + >> > + if (done) >> > complete(>done); >> > >> Its not clear, why the driver is setting the controller->xfer = NULL >> and restoring it inside the irq. This patch seems to fix things on >> top of that. > > I think the original intent was to make sure that the irqhandler knew that > there > was no outstanding transaction. This begs the question of why that would ever > be necessary. I think it would suffice to rework all of that to remove that > behavior and perhaps enable/disable the irq as we need to during transactions. > > I've never been a fan of the controller->xfer being set to NULL. Also, this patch presumably fixes an issue seen on Dakota SoCs, doesn't add or remote the NULL bits. -M
Re: [PATCH 11/18] spi: qup: properly detect extra interrupts
On Wed, Jun 14, 2017 at 12:57:25PM +0530, Sricharan R wrote: > Hi Varada, > > On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote: > > It's possible for a SPI transaction to complete and get another > > interrupt and have it processed on the same spi_transfer before the > > transfer_one can set it to NULL. > > > > This masks unexpected interrupts, so let's set the spi_transfer to > > NULL in the interrupt once the transaction is done. So we can > > properly detect these bad interrupts and print warning messages. > > > > Signed-off-by: Matthew McClintock> > Signed-off-by: Varadarajan Narayanan > > --- > > drivers/spi/spi-qup.c | 20 +++- > > 1 file changed, 11 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c > > index bd53e82..1a2a9d9 100644 > > --- a/drivers/spi/spi-qup.c > > +++ b/drivers/spi/spi-qup.c > > @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void > > *dev_id) > > struct spi_qup *controller = dev_id; > > struct spi_transfer *xfer; > > u32 opflags, qup_err, spi_err; > > - unsigned long flags; > > int error = 0; > > + bool done = 0; > > > > - spin_lock_irqsave(>lock, flags); > > + spin_lock(>lock); > > xfer = controller->xfer; > > controller->xfer = NULL; > > - spin_unlock_irqrestore(>lock, flags); > > + spin_unlock(>lock); > > Why change the locking here ? > > > > > qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); > > spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); > > @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void > > *dev_id) > > spi_qup_write(controller, xfer); > > } > > > > - spin_lock_irqsave(>lock, flags); > > - controller->error = error; > > - controller->xfer = xfer; > > - spin_unlock_irqrestore(>lock, flags); > > - > > /* re-read opflags as flags may have changed due to actions above */ > > opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); > > > > if ((controller->rx_bytes == xfer->len && > > (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) > > + done = true; > > + > > + spin_lock(>lock); > > + controller->error = error; > > + controller->xfer = done ? NULL : xfer; > > + spin_unlock(>lock); > > + > > + if (done) > > complete(>done); > > > Its not clear, why the driver is setting the controller->xfer = NULL > and restoring it inside the irq. This patch seems to fix things on > top of that. I think the original intent was to make sure that the irqhandler knew that there was no outstanding transaction. This begs the question of why that would ever be necessary. I think it would suffice to rework all of that to remove that behavior and perhaps enable/disable the irq as we need to during transactions. I've never been a fan of the controller->xfer being set to NULL. Regards, Andy
Re: [PATCH 11/18] spi: qup: properly detect extra interrupts
On Wed, Jun 14, 2017 at 12:57:25PM +0530, Sricharan R wrote: > Hi Varada, > > On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote: > > It's possible for a SPI transaction to complete and get another > > interrupt and have it processed on the same spi_transfer before the > > transfer_one can set it to NULL. > > > > This masks unexpected interrupts, so let's set the spi_transfer to > > NULL in the interrupt once the transaction is done. So we can > > properly detect these bad interrupts and print warning messages. > > > > Signed-off-by: Matthew McClintock > > Signed-off-by: Varadarajan Narayanan > > --- > > drivers/spi/spi-qup.c | 20 +++- > > 1 file changed, 11 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c > > index bd53e82..1a2a9d9 100644 > > --- a/drivers/spi/spi-qup.c > > +++ b/drivers/spi/spi-qup.c > > @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void > > *dev_id) > > struct spi_qup *controller = dev_id; > > struct spi_transfer *xfer; > > u32 opflags, qup_err, spi_err; > > - unsigned long flags; > > int error = 0; > > + bool done = 0; > > > > - spin_lock_irqsave(>lock, flags); > > + spin_lock(>lock); > > xfer = controller->xfer; > > controller->xfer = NULL; > > - spin_unlock_irqrestore(>lock, flags); > > + spin_unlock(>lock); > > Why change the locking here ? > > > > > qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); > > spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); > > @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void > > *dev_id) > > spi_qup_write(controller, xfer); > > } > > > > - spin_lock_irqsave(>lock, flags); > > - controller->error = error; > > - controller->xfer = xfer; > > - spin_unlock_irqrestore(>lock, flags); > > - > > /* re-read opflags as flags may have changed due to actions above */ > > opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); > > > > if ((controller->rx_bytes == xfer->len && > > (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) > > + done = true; > > + > > + spin_lock(>lock); > > + controller->error = error; > > + controller->xfer = done ? NULL : xfer; > > + spin_unlock(>lock); > > + > > + if (done) > > complete(>done); > > > Its not clear, why the driver is setting the controller->xfer = NULL > and restoring it inside the irq. This patch seems to fix things on > top of that. I think the original intent was to make sure that the irqhandler knew that there was no outstanding transaction. This begs the question of why that would ever be necessary. I think it would suffice to rework all of that to remove that behavior and perhaps enable/disable the irq as we need to during transactions. I've never been a fan of the controller->xfer being set to NULL. Regards, Andy
Re: [PATCH 11/18] spi: qup: properly detect extra interrupts
Hi Varada, On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote: > It's possible for a SPI transaction to complete and get another > interrupt and have it processed on the same spi_transfer before the > transfer_one can set it to NULL. > > This masks unexpected interrupts, so let's set the spi_transfer to > NULL in the interrupt once the transaction is done. So we can > properly detect these bad interrupts and print warning messages. > > Signed-off-by: Matthew McClintock> Signed-off-by: Varadarajan Narayanan > --- > drivers/spi/spi-qup.c | 20 +++- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c > index bd53e82..1a2a9d9 100644 > --- a/drivers/spi/spi-qup.c > +++ b/drivers/spi/spi-qup.c > @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void > *dev_id) > struct spi_qup *controller = dev_id; > struct spi_transfer *xfer; > u32 opflags, qup_err, spi_err; > - unsigned long flags; > int error = 0; > + bool done = 0; > > - spin_lock_irqsave(>lock, flags); > + spin_lock(>lock); > xfer = controller->xfer; > controller->xfer = NULL; > - spin_unlock_irqrestore(>lock, flags); > + spin_unlock(>lock); Why change the locking here ? > > qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); > spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); > @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void > *dev_id) > spi_qup_write(controller, xfer); > } > > - spin_lock_irqsave(>lock, flags); > - controller->error = error; > - controller->xfer = xfer; > - spin_unlock_irqrestore(>lock, flags); > - > /* re-read opflags as flags may have changed due to actions above */ > opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); > > if ((controller->rx_bytes == xfer->len && > (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) > + done = true; > + > + spin_lock(>lock); > + controller->error = error; > + controller->xfer = done ? NULL : xfer; > + spin_unlock(>lock); > + > + if (done) > complete(>done); > Its not clear, why the driver is setting the controller->xfer = NULL and restoring it inside the irq. This patch seems to fix things on top of that. Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 11/18] spi: qup: properly detect extra interrupts
Hi Varada, On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote: > It's possible for a SPI transaction to complete and get another > interrupt and have it processed on the same spi_transfer before the > transfer_one can set it to NULL. > > This masks unexpected interrupts, so let's set the spi_transfer to > NULL in the interrupt once the transaction is done. So we can > properly detect these bad interrupts and print warning messages. > > Signed-off-by: Matthew McClintock > Signed-off-by: Varadarajan Narayanan > --- > drivers/spi/spi-qup.c | 20 +++- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c > index bd53e82..1a2a9d9 100644 > --- a/drivers/spi/spi-qup.c > +++ b/drivers/spi/spi-qup.c > @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void > *dev_id) > struct spi_qup *controller = dev_id; > struct spi_transfer *xfer; > u32 opflags, qup_err, spi_err; > - unsigned long flags; > int error = 0; > + bool done = 0; > > - spin_lock_irqsave(>lock, flags); > + spin_lock(>lock); > xfer = controller->xfer; > controller->xfer = NULL; > - spin_unlock_irqrestore(>lock, flags); > + spin_unlock(>lock); Why change the locking here ? > > qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); > spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); > @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void > *dev_id) > spi_qup_write(controller, xfer); > } > > - spin_lock_irqsave(>lock, flags); > - controller->error = error; > - controller->xfer = xfer; > - spin_unlock_irqrestore(>lock, flags); > - > /* re-read opflags as flags may have changed due to actions above */ > opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); > > if ((controller->rx_bytes == xfer->len && > (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) > + done = true; > + > + spin_lock(>lock); > + controller->error = error; > + controller->xfer = done ? NULL : xfer; > + spin_unlock(>lock); > + > + if (done) > complete(>done); > Its not clear, why the driver is setting the controller->xfer = NULL and restoring it inside the irq. This patch seems to fix things on top of that. Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
[PATCH 11/18] spi: qup: properly detect extra interrupts
It's possible for a SPI transaction to complete and get another interrupt and have it processed on the same spi_transfer before the transfer_one can set it to NULL. This masks unexpected interrupts, so let's set the spi_transfer to NULL in the interrupt once the transaction is done. So we can properly detect these bad interrupts and print warning messages. Signed-off-by: Matthew McClintockSigned-off-by: Varadarajan Narayanan --- drivers/spi/spi-qup.c | 20 +++- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c index bd53e82..1a2a9d9 100644 --- a/drivers/spi/spi-qup.c +++ b/drivers/spi/spi-qup.c @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id) struct spi_qup *controller = dev_id; struct spi_transfer *xfer; u32 opflags, qup_err, spi_err; - unsigned long flags; int error = 0; + bool done = 0; - spin_lock_irqsave(>lock, flags); + spin_lock(>lock); xfer = controller->xfer; controller->xfer = NULL; - spin_unlock_irqrestore(>lock, flags); + spin_unlock(>lock); qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id) spi_qup_write(controller, xfer); } - spin_lock_irqsave(>lock, flags); - controller->error = error; - controller->xfer = xfer; - spin_unlock_irqrestore(>lock, flags); - /* re-read opflags as flags may have changed due to actions above */ opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); if ((controller->rx_bytes == xfer->len && (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) + done = true; + + spin_lock(>lock); + controller->error = error; + controller->xfer = done ? NULL : xfer; + spin_unlock(>lock); + + if (done) complete(>done); return IRQ_HANDLED; @@ -765,7 +768,6 @@ static int spi_qup_transfer_one(struct spi_master *master, exit: spi_qup_set_state(controller, QUP_STATE_RESET); spin_lock_irqsave(>lock, flags); - controller->xfer = NULL; if (!ret) ret = controller->error; spin_unlock_irqrestore(>lock, flags); -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
[PATCH 11/18] spi: qup: properly detect extra interrupts
It's possible for a SPI transaction to complete and get another interrupt and have it processed on the same spi_transfer before the transfer_one can set it to NULL. This masks unexpected interrupts, so let's set the spi_transfer to NULL in the interrupt once the transaction is done. So we can properly detect these bad interrupts and print warning messages. Signed-off-by: Matthew McClintock Signed-off-by: Varadarajan Narayanan --- drivers/spi/spi-qup.c | 20 +++- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c index bd53e82..1a2a9d9 100644 --- a/drivers/spi/spi-qup.c +++ b/drivers/spi/spi-qup.c @@ -496,13 +496,13 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id) struct spi_qup *controller = dev_id; struct spi_transfer *xfer; u32 opflags, qup_err, spi_err; - unsigned long flags; int error = 0; + bool done = 0; - spin_lock_irqsave(>lock, flags); + spin_lock(>lock); xfer = controller->xfer; controller->xfer = NULL; - spin_unlock_irqrestore(>lock, flags); + spin_unlock(>lock); qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS); spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS); @@ -556,16 +556,19 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id) spi_qup_write(controller, xfer); } - spin_lock_irqsave(>lock, flags); - controller->error = error; - controller->xfer = xfer; - spin_unlock_irqrestore(>lock, flags); - /* re-read opflags as flags may have changed due to actions above */ opflags = readl_relaxed(controller->base + QUP_OPERATIONAL); if ((controller->rx_bytes == xfer->len && (opflags & QUP_OP_MAX_INPUT_DONE_FLAG)) || error) + done = true; + + spin_lock(>lock); + controller->error = error; + controller->xfer = done ? NULL : xfer; + spin_unlock(>lock); + + if (done) complete(>done); return IRQ_HANDLED; @@ -765,7 +768,6 @@ static int spi_qup_transfer_one(struct spi_master *master, exit: spi_qup_set_state(controller, QUP_STATE_RESET); spin_lock_irqsave(>lock, flags); - controller->xfer = NULL; if (!ret) ret = controller->error; spin_unlock_irqrestore(>lock, flags); -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation