Re: [PATCH 10/18] spi: qup: Fix DMA mode interrupt handling

[Sricharan R]

Hi Andy,

On 6/15/2017 1:36 AM, Andy Gross wrote:
> On Wed, Jun 14, 2017 at 12:51:11PM +0530, Sricharan R wrote:
>> Hi Varada,
>>
>> On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote:
>>> This is needed for v1, where the i/o completion is not
>>> handled in the dma driver.
>>>
>>> Signed-off-by: Andy Gross 
>>> Signed-off-by: Varadarajan Narayanan 
>>> ---
>>>  drivers/spi/spi-qup.c | 15 +--
>>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
>>> index 872de28..bd53e82 100644
>>> --- a/drivers/spi/spi-qup.c
>>> +++ b/drivers/spi/spi-qup.c
>>> @@ -510,9 +510,9 @@ static irqreturn_t spi_qup_qup_irq(int irq, void 
>>> *dev_id)
>>>  
>>> writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
>>> writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
>>> -   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
>>>  
>>> if (!xfer) {
>>> +   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
>>
>>  This does look correct to remove acknowledging the QUP in normal case and
>>   do it conditionally only when xfer = NULL.
> 
> This is to probably mask the issue of getting erroneous/spurious IRQs.
> 

 hmm, now the QUP_OPERATIONAL is not written to acknowledge the interrupts in
 normal case seems to be wrong.

>>
>>> dev_err_ratelimited(controller->dev, "unexpected irq %08x %08x 
>>> %08x\n",
>>> qup_err, spi_err, opflags);
>>> return IRQ_HANDLED;
>>> @@ -540,7 +540,15 @@ static irqreturn_t spi_qup_qup_irq(int irq, void 
>>> *dev_id)
>>> error = -EIO;
>>> }
>>>  
>>> -   if (!spi_qup_is_dma_xfer(controller->mode)) {
>>> +   if (spi_qup_is_dma_xfer(controller->mode)) {
>>> +   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
>>> +   if (opflags & QUP_OP_IN_SERVICE_FLAG &&
>>> +   opflags & QUP_OP_MAX_INPUT_DONE_FLAG)
>>> +   complete(>rxc);
>>> +   if (opflags & QUP_OP_OUT_SERVICE_FLAG &&
>>> +   opflags & QUP_OP_MAX_OUTPUT_DONE_FLAG)
>>> +   complete(>txc);
>>> +   } else {
>>
>>  Is this because in patch #8 that we do not populate the dma callback
>>  for v1. If that is done, this should not be required at all, as the
>>  complete would be signalled from the dma callback.
> 
> I believe that is true.  There shouldn't be any IRQs for DMA enabled
> transactions (at least BAM-dma).

 yeah, the above hunk looks like is ADM specific, not sure why ADM cannot
 work with dma callbacks.

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 10/18] spi: qup: Fix DMA mode interrupt handling

[Sricharan R]

Hi Andy,

On 6/15/2017 1:36 AM, Andy Gross wrote:
> On Wed, Jun 14, 2017 at 12:51:11PM +0530, Sricharan R wrote:
>> Hi Varada,
>>
>> On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote:
>>> This is needed for v1, where the i/o completion is not
>>> handled in the dma driver.
>>>
>>> Signed-off-by: Andy Gross 
>>> Signed-off-by: Varadarajan Narayanan 
>>> ---
>>>  drivers/spi/spi-qup.c | 15 +--
>>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
>>> index 872de28..bd53e82 100644
>>> --- a/drivers/spi/spi-qup.c
>>> +++ b/drivers/spi/spi-qup.c
>>> @@ -510,9 +510,9 @@ static irqreturn_t spi_qup_qup_irq(int irq, void 
>>> *dev_id)
>>>  
>>> writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
>>> writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
>>> -   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
>>>  
>>> if (!xfer) {
>>> +   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
>>
>>  This does look correct to remove acknowledging the QUP in normal case and
>>   do it conditionally only when xfer = NULL.
> 
> This is to probably mask the issue of getting erroneous/spurious IRQs.
> 

 hmm, now the QUP_OPERATIONAL is not written to acknowledge the interrupts in
 normal case seems to be wrong.

>>
>>> dev_err_ratelimited(controller->dev, "unexpected irq %08x %08x 
>>> %08x\n",
>>> qup_err, spi_err, opflags);
>>> return IRQ_HANDLED;
>>> @@ -540,7 +540,15 @@ static irqreturn_t spi_qup_qup_irq(int irq, void 
>>> *dev_id)
>>> error = -EIO;
>>> }
>>>  
>>> -   if (!spi_qup_is_dma_xfer(controller->mode)) {
>>> +   if (spi_qup_is_dma_xfer(controller->mode)) {
>>> +   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
>>> +   if (opflags & QUP_OP_IN_SERVICE_FLAG &&
>>> +   opflags & QUP_OP_MAX_INPUT_DONE_FLAG)
>>> +   complete(>rxc);
>>> +   if (opflags & QUP_OP_OUT_SERVICE_FLAG &&
>>> +   opflags & QUP_OP_MAX_OUTPUT_DONE_FLAG)
>>> +   complete(>txc);
>>> +   } else {
>>
>>  Is this because in patch #8 that we do not populate the dma callback
>>  for v1. If that is done, this should not be required at all, as the
>>  complete would be signalled from the dma callback.
> 
> I believe that is true.  There shouldn't be any IRQs for DMA enabled
> transactions (at least BAM-dma).

 yeah, the above hunk looks like is ADM specific, not sure why ADM cannot
 work with dma callbacks.

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 10/18] spi: qup: Fix DMA mode interrupt handling

[Andy Gross]

On Wed, Jun 14, 2017 at 12:51:11PM +0530, Sricharan R wrote:
> Hi Varada,
> 
> On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote:
> > This is needed for v1, where the i/o completion is not
> > handled in the dma driver.
> > 
> > Signed-off-by: Andy Gross 
> > Signed-off-by: Varadarajan Narayanan 
> > ---
> >  drivers/spi/spi-qup.c | 15 +--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
> > index 872de28..bd53e82 100644
> > --- a/drivers/spi/spi-qup.c
> > +++ b/drivers/spi/spi-qup.c
> > @@ -510,9 +510,9 @@ static irqreturn_t spi_qup_qup_irq(int irq, void 
> > *dev_id)
> >  
> > writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
> > writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> > -   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> >  
> > if (!xfer) {
> > +   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> 
>  This does look correct to remove acknowledging the QUP in normal case and
>   do it conditionally only when xfer = NULL.

This is to probably mask the issue of getting erroneous/spurious IRQs.

> 
> > dev_err_ratelimited(controller->dev, "unexpected irq %08x %08x 
> > %08x\n",
> > qup_err, spi_err, opflags);
> > return IRQ_HANDLED;
> > @@ -540,7 +540,15 @@ static irqreturn_t spi_qup_qup_irq(int irq, void 
> > *dev_id)
> > error = -EIO;
> > }
> >  
> > -   if (!spi_qup_is_dma_xfer(controller->mode)) {
> > +   if (spi_qup_is_dma_xfer(controller->mode)) {
> > +   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> > +   if (opflags & QUP_OP_IN_SERVICE_FLAG &&
> > +   opflags & QUP_OP_MAX_INPUT_DONE_FLAG)
> > +   complete(>rxc);
> > +   if (opflags & QUP_OP_OUT_SERVICE_FLAG &&
> > +   opflags & QUP_OP_MAX_OUTPUT_DONE_FLAG)
> > +   complete(>txc);
> > +   } else {
> 
>  Is this because in patch #8 that we do not populate the dma callback
>  for v1. If that is done, this should not be required at all, as the
>  complete would be signalled from the dma callback.

I believe that is true.  There shouldn't be any IRQs for DMA enabled
transactions (at least BAM-dma).

Re: [PATCH 10/18] spi: qup: Fix DMA mode interrupt handling

[Andy Gross]

On Wed, Jun 14, 2017 at 12:51:11PM +0530, Sricharan R wrote:
> Hi Varada,
> 
> On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote:
> > This is needed for v1, where the i/o completion is not
> > handled in the dma driver.
> > 
> > Signed-off-by: Andy Gross 
> > Signed-off-by: Varadarajan Narayanan 
> > ---
> >  drivers/spi/spi-qup.c | 15 +--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
> > index 872de28..bd53e82 100644
> > --- a/drivers/spi/spi-qup.c
> > +++ b/drivers/spi/spi-qup.c
> > @@ -510,9 +510,9 @@ static irqreturn_t spi_qup_qup_irq(int irq, void 
> > *dev_id)
> >  
> > writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
> > writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> > -   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> >  
> > if (!xfer) {
> > +   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> 
>  This does look correct to remove acknowledging the QUP in normal case and
>   do it conditionally only when xfer = NULL.

This is to probably mask the issue of getting erroneous/spurious IRQs.

> 
> > dev_err_ratelimited(controller->dev, "unexpected irq %08x %08x 
> > %08x\n",
> > qup_err, spi_err, opflags);
> > return IRQ_HANDLED;
> > @@ -540,7 +540,15 @@ static irqreturn_t spi_qup_qup_irq(int irq, void 
> > *dev_id)
> > error = -EIO;
> > }
> >  
> > -   if (!spi_qup_is_dma_xfer(controller->mode)) {
> > +   if (spi_qup_is_dma_xfer(controller->mode)) {
> > +   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> > +   if (opflags & QUP_OP_IN_SERVICE_FLAG &&
> > +   opflags & QUP_OP_MAX_INPUT_DONE_FLAG)
> > +   complete(>rxc);
> > +   if (opflags & QUP_OP_OUT_SERVICE_FLAG &&
> > +   opflags & QUP_OP_MAX_OUTPUT_DONE_FLAG)
> > +   complete(>txc);
> > +   } else {
> 
>  Is this because in patch #8 that we do not populate the dma callback
>  for v1. If that is done, this should not be required at all, as the
>  complete would be signalled from the dma callback.

I believe that is true.  There shouldn't be any IRQs for DMA enabled
transactions (at least BAM-dma).

Re: [PATCH 10/18] spi: qup: Fix DMA mode interrupt handling

[Sricharan R]

Hi Varada,

On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote:
> This is needed for v1, where the i/o completion is not
> handled in the dma driver.
> 
> Signed-off-by: Andy Gross 
> Signed-off-by: Varadarajan Narayanan 
> ---
>  drivers/spi/spi-qup.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
> index 872de28..bd53e82 100644
> --- a/drivers/spi/spi-qup.c
> +++ b/drivers/spi/spi-qup.c
> @@ -510,9 +510,9 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
>  
>   writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
>   writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> - writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
>  
>   if (!xfer) {
> + writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);

 This does look correct to remove acknowledging the QUP in normal case and
  do it conditionally only when xfer = NULL.

>   dev_err_ratelimited(controller->dev, "unexpected irq %08x %08x 
> %08x\n",
>   qup_err, spi_err, opflags);
>   return IRQ_HANDLED;
> @@ -540,7 +540,15 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
>   error = -EIO;
>   }
>  
> - if (!spi_qup_is_dma_xfer(controller->mode)) {
> + if (spi_qup_is_dma_xfer(controller->mode)) {
> + writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> + if (opflags & QUP_OP_IN_SERVICE_FLAG &&
> + opflags & QUP_OP_MAX_INPUT_DONE_FLAG)
> + complete(>rxc);
> + if (opflags & QUP_OP_OUT_SERVICE_FLAG &&
> + opflags & QUP_OP_MAX_OUTPUT_DONE_FLAG)
> + complete(>txc);
> + } else {

 Is this because in patch #8 that we do not populate the dma callback
 for v1. If that is done, this should not be required at all, as the
 complete would be signalled from the dma callback.

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 10/18] spi: qup: Fix DMA mode interrupt handling

[Sricharan R]

Hi Varada,

On 6/14/2017 11:22 AM, Varadarajan Narayanan wrote:
> This is needed for v1, where the i/o completion is not
> handled in the dma driver.
> 
> Signed-off-by: Andy Gross 
> Signed-off-by: Varadarajan Narayanan 
> ---
>  drivers/spi/spi-qup.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
> index 872de28..bd53e82 100644
> --- a/drivers/spi/spi-qup.c
> +++ b/drivers/spi/spi-qup.c
> @@ -510,9 +510,9 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
>  
>   writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
>   writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> - writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
>  
>   if (!xfer) {
> + writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);

 This does look correct to remove acknowledging the QUP in normal case and
  do it conditionally only when xfer = NULL.

>   dev_err_ratelimited(controller->dev, "unexpected irq %08x %08x 
> %08x\n",
>   qup_err, spi_err, opflags);
>   return IRQ_HANDLED;
> @@ -540,7 +540,15 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
>   error = -EIO;
>   }
>  
> - if (!spi_qup_is_dma_xfer(controller->mode)) {
> + if (spi_qup_is_dma_xfer(controller->mode)) {
> + writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> + if (opflags & QUP_OP_IN_SERVICE_FLAG &&
> + opflags & QUP_OP_MAX_INPUT_DONE_FLAG)
> + complete(>rxc);
> + if (opflags & QUP_OP_OUT_SERVICE_FLAG &&
> + opflags & QUP_OP_MAX_OUTPUT_DONE_FLAG)
> + complete(>txc);
> + } else {

 Is this because in patch #8 that we do not populate the dma callback
 for v1. If that is done, this should not be required at all, as the
 complete would be signalled from the dma callback.

Regards,
 Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH 10/18] spi: qup: Fix DMA mode interrupt handling

[Varadarajan Narayanan]

This is needed for v1, where the i/o completion is not
handled in the dma driver.

Signed-off-by: Andy Gross 
Signed-off-by: Varadarajan Narayanan 
---
 drivers/spi/spi-qup.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
index 872de28..bd53e82 100644
--- a/drivers/spi/spi-qup.c
+++ b/drivers/spi/spi-qup.c
@@ -510,9 +510,9 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
 
writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
-   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
 
if (!xfer) {
+   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
dev_err_ratelimited(controller->dev, "unexpected irq %08x %08x 
%08x\n",
qup_err, spi_err, opflags);
return IRQ_HANDLED;
@@ -540,7 +540,15 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
error = -EIO;
}
 
-   if (!spi_qup_is_dma_xfer(controller->mode)) {
+   if (spi_qup_is_dma_xfer(controller->mode)) {
+   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
+   if (opflags & QUP_OP_IN_SERVICE_FLAG &&
+   opflags & QUP_OP_MAX_INPUT_DONE_FLAG)
+   complete(>rxc);
+   if (opflags & QUP_OP_OUT_SERVICE_FLAG &&
+   opflags & QUP_OP_MAX_OUTPUT_DONE_FLAG)
+   complete(>txc);
+   } else {
if (opflags & QUP_OP_IN_SERVICE_FLAG)
spi_qup_read(controller, xfer);
 
@@ -553,6 +561,9 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
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)
complete(>done);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

[PATCH 10/18] spi: qup: Fix DMA mode interrupt handling

[Varadarajan Narayanan]

This is needed for v1, where the i/o completion is not
handled in the dma driver.

Signed-off-by: Andy Gross 
Signed-off-by: Varadarajan Narayanan 
---
 drivers/spi/spi-qup.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
index 872de28..bd53e82 100644
--- a/drivers/spi/spi-qup.c
+++ b/drivers/spi/spi-qup.c
@@ -510,9 +510,9 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
 
writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
-   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
 
if (!xfer) {
+   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
dev_err_ratelimited(controller->dev, "unexpected irq %08x %08x 
%08x\n",
qup_err, spi_err, opflags);
return IRQ_HANDLED;
@@ -540,7 +540,15 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
error = -EIO;
}
 
-   if (!spi_qup_is_dma_xfer(controller->mode)) {
+   if (spi_qup_is_dma_xfer(controller->mode)) {
+   writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
+   if (opflags & QUP_OP_IN_SERVICE_FLAG &&
+   opflags & QUP_OP_MAX_INPUT_DONE_FLAG)
+   complete(>rxc);
+   if (opflags & QUP_OP_OUT_SERVICE_FLAG &&
+   opflags & QUP_OP_MAX_OUTPUT_DONE_FLAG)
+   complete(>txc);
+   } else {
if (opflags & QUP_OP_IN_SERVICE_FLAG)
spi_qup_read(controller, xfer);
 
@@ -553,6 +561,9 @@ static irqreturn_t spi_qup_qup_irq(int irq, void *dev_id)
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)
complete(>done);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation