[PATCH 0/2] i2c: qup: Fixed the DMA transfer errors

[PATCH 0/2] i2c: qup: Fixed the DMA transfer errors

[Abhishek Sahu]

These patches contain the I2C QUP (Qualcomm Universal Peripheral) DMA
related fixes. The first patch cleared the error bits in ISR, which
resolves the kernel hang for I2C bus errors and DMA transfer. The
second patch fixed the DMA segments length, which resolves the issue
for I2C transfer failure over 255 bytes.

Abhishek Sahu (2):
  drivers: i2c: qup: Cleared the error bits in ISR
  drivers: i2c: qup: Fixed the DMA segments length

 drivers/i2c/busses/i2c-qup.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/2] i2c: qup: Cleared the error bits in ISR

[Abhishek Sahu]

1. Current QCOM I2C driver hangs when sending data to address 0x03-0x07
in some scenarios. The QUP controller generates invalid write in this
case, since these addresses are reserved for different bus formats.

2. Also, the error handling is done by I2C QUP ISR in the case of DMA
mode. The state need to be RESET in case of any error for clearing the
available data in FIFO, which otherwise leaves the BAM DMA controller
in hang state.

This patch fixes the above two issues by clearing the error bits from
I2C and QUP status in ISR in case of I2C error, QUP error and resets
the QUP state to clear the FIFO data.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
 drivers/i2c/busses/i2c-qup.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index 23eaabb..8c2f1bc 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -213,14 +213,16 @@ static irqreturn_t qup_i2c_interrupt(int irq, void *dev)
 	bus_err &= I2C_STATUS_ERROR_MASK;
 	qup_err &= QUP_STATUS_ERROR_FLAGS;
 
-	if (qup_err) {
-		/* Clear Error interrupt */
+	/* Clear the error bits in QUP_ERROR_FLAGS */
+	if (qup_err)
 		writel(qup_err, qup->base + QUP_ERROR_FLAGS);
-		goto done;
-	}
 
-	if (bus_err) {
-		/* Clear Error interrupt */
+	/* Clear the error bits in QUP_I2C_STATUS */
+	if (bus_err)
+		writel(bus_err, qup->base + QUP_I2C_STATUS);
+
+	/* Reset the QUP State in case of error */
+	if (qup_err || bus_err) {
 		writel(QUP_RESET_STATE, qup->base + QUP_STATE);
 		goto done;
 	}
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/2] i2c: qup: Cleared the error bits in ISR

[Sricharan]

Hi,

> 1. Current QCOM I2C driver hangs when sending data to address 0x03-0x07
> in some scenarios. The QUP controller generates invalid write in this
case,
> since these addresses are reserved for different bus formats.
> 
> 2. Also, the error handling is done by I2C QUP ISR in the case of DMA
mode.
> The state need to be RESET in case of any error for clearing the available
> data in FIFO, which otherwise leaves the BAM DMA controller in hang state.
> 
> This patch fixes the above two issues by clearing the error bits from I2C
and
> QUP status in ISR in case of I2C error, QUP error and resets the QUP state
to
> clear the FIFO data.
> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
>  drivers/i2c/busses/i2c-qup.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index
> 23eaabb..8c2f1bc 100644
> --- a/drivers/i2c/busses/i2c-qup.c
> +++ b/drivers/i2c/busses/i2c-qup.c
> @@ -213,14 +213,16 @@ static irqreturn_t qup_i2c_interrupt(int irq, void
> *dev)
>  	bus_err &= I2C_STATUS_ERROR_MASK;
>  	qup_err &= QUP_STATUS_ERROR_FLAGS;
> 
> -	if (qup_err) {
> -		/* Clear Error interrupt */
> +	/* Clear the error bits in QUP_ERROR_FLAGS */
> +	if (qup_err)
>  		writel(qup_err, qup->base + QUP_ERROR_FLAGS);
> -		goto done;
> -	}
> 
> -	if (bus_err) {
> -		/* Clear Error interrupt */
> +	/* Clear the error bits in QUP_I2C_STATUS */
> +	if (bus_err)
> +		writel(bus_err, qup->base + QUP_I2C_STATUS);
> +
> +	/* Reset the QUP State in case of error */
> +	if (qup_err || bus_err) {
>  		writel(QUP_RESET_STATE, qup->base + QUP_STATE);
>  		goto done;
>  	}
       In qup_i2c_xfer and qup_i2c_xfer_v2 state is set to RESET at the end,
when
       there is no error. So would it be fine if we do it there
unconditionally ?

Regards,
 Sricharan

[PATCH 1/2] i2c: qup: Cleared the error bits in ISR

[Abhishek Sahu]

On 2016-05-11 21:27, Sricharan wrote:
> Hi,
> 
>> 1. Current QCOM I2C driver hangs when sending data to address 
>> 0x03-0x07
>> in some scenarios. The QUP controller generates invalid write in this
>> case,
>> since these addresses are reserved for different bus formats.
>> 
>> 2. Also, the error handling is done by I2C QUP ISR in the case of DMA
>> mode.
>> The state need to be RESET in case of any error for clearing the 
>> available
>> data in FIFO, which otherwise leaves the BAM DMA controller in hang 
>> state.
>> 
>> This patch fixes the above two issues by clearing the error bits from 
>> I2C
>> and
>> QUP status in ISR in case of I2C error, QUP error and resets the QUP 
>> state
>> to
>> clear the FIFO data.
>> 
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> ---
>>  drivers/i2c/busses/i2c-qup.c | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/i2c/busses/i2c-qup.c 
>> b/drivers/i2c/busses/i2c-qup.c
>> index
>> 23eaabb..8c2f1bc 100644
>> --- a/drivers/i2c/busses/i2c-qup.c
>> +++ b/drivers/i2c/busses/i2c-qup.c
>> @@ -213,14 +213,16 @@ static irqreturn_t qup_i2c_interrupt(int irq, 
>> void
>> *dev)
>>  	bus_err &= I2C_STATUS_ERROR_MASK;
>>  	qup_err &= QUP_STATUS_ERROR_FLAGS;
>> 
>> -	if (qup_err) {
>> -		/* Clear Error interrupt */
>> +	/* Clear the error bits in QUP_ERROR_FLAGS */
>> +	if (qup_err)
>>  		writel(qup_err, qup->base + QUP_ERROR_FLAGS);
>> -		goto done;
>> -	}
>> 
>> -	if (bus_err) {
>> -		/* Clear Error interrupt */
>> +	/* Clear the error bits in QUP_I2C_STATUS */
>> +	if (bus_err)
>> +		writel(bus_err, qup->base + QUP_I2C_STATUS);
>> +
>> +	/* Reset the QUP State in case of error */
>> +	if (qup_err || bus_err) {
>>  		writel(QUP_RESET_STATE, qup->base + QUP_STATE);
>>  		goto done;
>>  	}
>        In qup_i2c_xfer and qup_i2c_xfer_v2 state is set to RESET at the 
> end, when
>        there is no error. So would it be fine if we do it there 
> unconditionally ?
> 
> Regards,
>  Sricharan

RESET the QUP state wouldn't create any issue in the case of multiple 
calls.
The existing code also RESET the QUP state for bus_err but it is not 
clearing
status bits.

-- 
Abhishek Sahu

[PATCH 1/2] i2c: qup: Cleared the error bits in ISR

[Sricharan]

Hi,

> -----Original Message-----
> From: linux-arm-kernel [mailto:linux-arm-kernel-
> bounces at lists.infradead.org] On Behalf Of Abhishek Sahu
> Sent: Wednesday, May 11, 2016 11:04 PM
> To: Sricharan <sricharan@codeaurora.org>
> Cc: architt at codeaurora.org; wsa at the-dreams.de; linux-arm-
> msm at vger.kernel.org; ntelkar at codeaurora.org; linux-
> kernel at vger.kernel.org; dmaengine at vger.kernel.org; linux-
> i2c at vger.kernel.org; agross at codeaurora.org; andy.gross at linaro.org; linux-
> arm-kernel at lists.infradead.org
> Subject: RE: [PATCH 1/2] i2c: qup: Cleared the error bits in ISR
> 
> On 2016-05-11 21:27, Sricharan wrote:
> > Hi,
> >
> >> 1. Current QCOM I2C driver hangs when sending data to address
> >> 0x03-0x07
> >> in some scenarios. The QUP controller generates invalid write in this
> >> case, since these addresses are reserved for different bus formats.
> >>
> >> 2. Also, the error handling is done by I2C QUP ISR in the case of DMA
> >> mode.
> >> The state need to be RESET in case of any error for clearing the
> >> available data in FIFO, which otherwise leaves the BAM DMA controller
> >> in hang state.
> >>
> >> This patch fixes the above two issues by clearing the error bits from
> >> I2C and QUP status in ISR in case of I2C error, QUP error and resets
> >> the QUP state to clear the FIFO data.
> >>
> >> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> >> ---
> >>  drivers/i2c/busses/i2c-qup.c | 14 ++++++++------
> >>  1 file changed, 8 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-qup.c
> >> b/drivers/i2c/busses/i2c-qup.c index 23eaabb..8c2f1bc 100644
> >> --- a/drivers/i2c/busses/i2c-qup.c
> >> +++ b/drivers/i2c/busses/i2c-qup.c
> >> @@ -213,14 +213,16 @@ static irqreturn_t qup_i2c_interrupt(int irq,
> >> void
> >> *dev)
> >>  	bus_err &= I2C_STATUS_ERROR_MASK;
> >>  	qup_err &= QUP_STATUS_ERROR_FLAGS;
> >>
> >> -	if (qup_err) {
> >> -		/* Clear Error interrupt */
> >> +	/* Clear the error bits in QUP_ERROR_FLAGS */
> >> +	if (qup_err)
> >>  		writel(qup_err, qup->base + QUP_ERROR_FLAGS);
> >> -		goto done;
> >> -	}
> >>
> >> -	if (bus_err) {
> >> -		/* Clear Error interrupt */
> >> +	/* Clear the error bits in QUP_I2C_STATUS */
> >> +	if (bus_err)
> >> +		writel(bus_err, qup->base + QUP_I2C_STATUS);
> >> +
> >> +	/* Reset the QUP State in case of error */
> >> +	if (qup_err || bus_err) {
> >>  		writel(QUP_RESET_STATE, qup->base + QUP_STATE);
> >>  		goto done;
> >>  	}
> >        In qup_i2c_xfer and qup_i2c_xfer_v2 state is set to RESET at
> > the end, when
> >        there is no error. So would it be fine if we do it there
> > unconditionally ?
> >
> > Regards,
> >  Sricharan
> 
> RESET the QUP state wouldn't create any issue in the case of multiple
calls.
> The existing code also RESET the QUP state for bus_err but it is not
clearing
> status bits.
    So is there a difference that you see by setting it in isr for qup
errors ?
    Otherwise, its better to set it in one place unconditionally instead of
doing
    it in two places ?

Regards,
 Sricharan

[PATCH 1/2] i2c: qup: Cleared the error bits in ISR

[Andy Gross]

On Wed, May 11, 2016 at 11:04:17PM +0530, Abhishek Sahu wrote:

<snip>

> >       In qup_i2c_xfer and qup_i2c_xfer_v2 state is set to RESET at the
> >end, when
> >       there is no error. So would it be fine if we do it there
> >unconditionally ?
> >
> >Regards,
> > Sricharan
> 
> RESET the QUP state wouldn't create any issue in the case of multiple calls.
> The existing code also RESET the QUP state for bus_err but it is not
> clearing
> status bits.

It'd be better to not reset the QUP inside the ISR at all.  I think the better
solution is making the reset occur in the xfer function.  So just clear the bits
like you should in the isr, and defer reset till later.

[PATCH 1/2] i2c: qup: Cleared the error bits in ISR

[Abhishek Sahu]

On Thu, May 12, 2016 at 12:13:47AM -0500, Andy Gross wrote:
> On Wed, May 11, 2016 at 11:04:17PM +0530, Abhishek Sahu wrote:
> 
> <snip>
> 
> > >       In qup_i2c_xfer and qup_i2c_xfer_v2 state is set to RESET at the
> > >end, when
> > >       there is no error. So would it be fine if we do it there
> > >unconditionally ?
> > >
> > >Regards,
> > > Sricharan
> > 
> > RESET the QUP state wouldn't create any issue in the case of multiple calls.
> > The existing code also RESET the QUP state for bus_err but it is not
> > clearing
> > status bits.
> 
> It'd be better to not reset the QUP inside the ISR at all.  I think the better
> solution is making the reset occur in the xfer function.  So just clear the bits
> like you should in the isr, and defer reset till later.

This is a HW workaround for QUP where we need to RESET the core in
ISR. When interrupt is level triggerd, more than one interrupt may
fire in error cases and QUP will generate these interrupts after
completion of current interrupt. Thus we reset the core immediately
in the ISR to ward off the next interrupt.

-- 
Abhishek Sahu

[PATCH 1/2] i2c: qup: Cleared the error bits in ISR

[Andy Gross]

On Thu, May 12, 2016 at 11:48:43AM +0530, Abhishek Sahu wrote:
> On Thu, May 12, 2016 at 12:13:47AM -0500, Andy Gross wrote:
> > On Wed, May 11, 2016 at 11:04:17PM +0530, Abhishek Sahu wrote:
> > 
> > <snip>
> > 
> > > >       In qup_i2c_xfer and qup_i2c_xfer_v2 state is set to RESET at the
> > > >end, when
> > > >       there is no error. So would it be fine if we do it there
> > > >unconditionally ?
> > > >
> > > >Regards,
> > > > Sricharan
> > > 
> > > RESET the QUP state wouldn't create any issue in the case of multiple calls.
> > > The existing code also RESET the QUP state for bus_err but it is not
> > > clearing
> > > status bits.
> > 
> > It'd be better to not reset the QUP inside the ISR at all.  I think the better
> > solution is making the reset occur in the xfer function.  So just clear the bits
> > like you should in the isr, and defer reset till later.
> 
> This is a HW workaround for QUP where we need to RESET the core in
> ISR. When interrupt is level triggerd, more than one interrupt may
> fire in error cases and QUP will generate these interrupts after
> completion of current interrupt. Thus we reset the core immediately
> in the ISR to ward off the next interrupt.

I wonder if that was just an expedient solution.  And it seems like it could be
a little racy as a poll is not being done to make sure the QUP state machine
transitions to reset.  Is there any documentation/errata on this?  Or is this
just from commit comments?


Regards,
Andy

[PATCH 1/2] i2c: qup: Cleared the error bits in ISR

[Abhishek Sahu]

On Thu, May 12, 2016 at 12:58:30PM -0500, Andy Gross wrote:
> On Thu, May 12, 2016 at 11:48:43AM +0530, Abhishek Sahu wrote:
> > On Thu, May 12, 2016 at 12:13:47AM -0500, Andy Gross wrote:
> > > On Wed, May 11, 2016 at 11:04:17PM +0530, Abhishek Sahu wrote:
> > > 
> > > <snip>
> > > 
> > > > >       In qup_i2c_xfer and qup_i2c_xfer_v2 state is set to RESET at the
> > > > >end, when
> > > > >       there is no error. So would it be fine if we do it there
> > > > >unconditionally ?
> > > > >
> > > > >Regards,
> > > > > Sricharan
> > > > 
> > > > RESET the QUP state wouldn't create any issue in the case of multiple calls.
> > > > The existing code also RESET the QUP state for bus_err but it is not
> > > > clearing
> > > > status bits.
> > > 
> > > It'd be better to not reset the QUP inside the ISR at all.  I think the better
> > > solution is making the reset occur in the xfer function.  So just clear the bits
> > > like you should in the isr, and defer reset till later.
> > 
> > This is a HW workaround for QUP where we need to RESET the core in
> > ISR. When interrupt is level triggerd, more than one interrupt may
> > fire in error cases and QUP will generate these interrupts after
> > completion of current interrupt. Thus we reset the core immediately
> > in the ISR to ward off the next interrupt.
> 
> I wonder if that was just an expedient solution.  And it seems like it could be
> a little racy as a poll is not being done to make sure the QUP state machine
> transitions to reset.  Is there any documentation/errata on this?  Or is this
> just from commit comments?
> 
> 
> Regards,
> Andy

Hi Andy,

Thanks for your comments. We have only added the patch for clearing
the error bits. This resetting of QUP is present in the base code
itself. Since this QUP is being used by all the QCOM chips that's why
we have not changed the existing QUP reset logic. We will look
into reset polling logic separately and will post the patch for the
same.

We got the input for QUP RESET by referring our working internal
drivers and we also faced the BAM hang issue in base upstream
driver, if we don't do this RESET. If we schedule any transfer
with DMA mode then the DMA Engine (BAM) will never get any interrupt
for these errors, since the QUP interrupt handles the error cases.
The BAM will generate the interrupt only when it will receive the
requested amount of data. This reset will clear the QUP FIFO and
we will get the BAM transfer completion interrupt by the existing
logic present in the base bam transfer function.

-- 
Abhishek Sahu

[PATCH 1/2] i2c: qup: Cleared the error bits in ISR

[Andy Gross]

On 12 May 2016 at 14:32, Abhishek Sahu <absahu@codeaurora.org> wrote:
> On Thu, May 12, 2016 at 12:58:30PM -0500, Andy Gross wrote:
>> On Thu, May 12, 2016 at 11:48:43AM +0530, Abhishek Sahu wrote:
>> > On Thu, May 12, 2016 at 12:13:47AM -0500, Andy Gross wrote:
>> > > On Wed, May 11, 2016 at 11:04:17PM +0530, Abhishek Sahu wrote:
>> > >
>> > > <snip>
>> > >
>> > > > >       In qup_i2c_xfer and qup_i2c_xfer_v2 state is set to RESET at the
>> > > > >end, when
>> > > > >       there is no error. So would it be fine if we do it there
>> > > > >unconditionally ?
>> > > > >
>> > > > >Regards,
>> > > > > Sricharan
>> > > >
>> > > > RESET the QUP state wouldn't create any issue in the case of multiple calls.
>> > > > The existing code also RESET the QUP state for bus_err but it is not
>> > > > clearing
>> > > > status bits.
>> > >
>> > > It'd be better to not reset the QUP inside the ISR at all.  I think the better
>> > > solution is making the reset occur in the xfer function.  So just clear the bits
>> > > like you should in the isr, and defer reset till later.
>> >
>> > This is a HW workaround for QUP where we need to RESET the core in
>> > ISR. When interrupt is level triggerd, more than one interrupt may
>> > fire in error cases and QUP will generate these interrupts after
>> > completion of current interrupt. Thus we reset the core immediately
>> > in the ISR to ward off the next interrupt.
>>
>> I wonder if that was just an expedient solution.  And it seems like it could be
>> a little racy as a poll is not being done to make sure the QUP state machine
>> transitions to reset.  Is there any documentation/errata on this?  Or is this
>> just from commit comments?
>>
>>
>> Regards,
>> Andy
>
> Hi Andy,
>
> Thanks for your comments. We have only added the patch for clearing
> the error bits. This resetting of QUP is present in the base code
> itself. Since this QUP is being used by all the QCOM chips that's why
> we have not changed the existing QUP reset logic. We will look
> into reset polling logic separately and will post the patch for the
> same.

Yeah, it is present in the current code.  I hadn't seen any errata on
this issue, so that is why I was curious.  In many cases, the code in
the mainline driver was carried forward from the downstream CAF
driver.  I know the two don't look that similar in some regards, but
in most cases the actions taken are the same, even if it is not
obvious.

> We got the input for QUP RESET by referring our working internal
> drivers and we also faced the BAM hang issue in base upstream
> driver, if we don't do this RESET. If we schedule any transfer
> with DMA mode then the DMA Engine (BAM) will never get any interrupt
> for these errors, since the QUP interrupt handles the error cases.
> The BAM will generate the interrupt only when it will receive the
> requested amount of data. This reset will clear the QUP FIFO and
> we will get the BAM transfer completion interrupt by the existing
> logic present in the base bam transfer function.

Right.  If the QUP has an issue and isn't processing data, and you
have a BAM transaction pending for this, you have to not only ack the
qup error, but also terminate the BAM transaction.

What test case are you using to test out the error path?  Something
other than NACKs?

Regards,

Andy

[PATCH 1/2] i2c: qup: Cleared the error bits in ISR

[Abhishek Sahu]

On Thu, May 12, 2016 at 03:05:39PM -0500, Andy Gross wrote:
> On 12 May 2016 at 14:32, Abhishek Sahu <absahu@codeaurora.org> wrote:
> > On Thu, May 12, 2016 at 12:58:30PM -0500, Andy Gross wrote:
> >> On Thu, May 12, 2016 at 11:48:43AM +0530, Abhishek Sahu wrote:
> >> > On Thu, May 12, 2016 at 12:13:47AM -0500, Andy Gross wrote:
> >> > > On Wed, May 11, 2016 at 11:04:17PM +0530, Abhishek Sahu wrote:
> >> > >
> >> > > <snip>
> >> > >
> >> > > > >       In qup_i2c_xfer and qup_i2c_xfer_v2 state is set to RESET at the
> >> > > > >end, when
> >> > > > >       there is no error. So would it be fine if we do it there
> >> > > > >unconditionally ?
> >> > > > >
> >> > > > >Regards,
> >> > > > > Sricharan
> >> > > >
> >> > > > RESET the QUP state wouldn't create any issue in the case of multiple calls.
> >> > > > The existing code also RESET the QUP state for bus_err but it is not
> >> > > > clearing
> >> > > > status bits.
> >> > >
> >> > > It'd be better to not reset the QUP inside the ISR at all.  I think the better
> >> > > solution is making the reset occur in the xfer function.  So just clear the bits
> >> > > like you should in the isr, and defer reset till later.
> >> >
> >> > This is a HW workaround for QUP where we need to RESET the core in
> >> > ISR. When interrupt is level triggerd, more than one interrupt may
> >> > fire in error cases and QUP will generate these interrupts after
> >> > completion of current interrupt. Thus we reset the core immediately
> >> > in the ISR to ward off the next interrupt.
> >>
> >> I wonder if that was just an expedient solution.  And it seems like it could be
> >> a little racy as a poll is not being done to make sure the QUP state machine
> >> transitions to reset.  Is there any documentation/errata on this?  Or is this
> >> just from commit comments?
> >>
> >>
> >> Regards,
> >> Andy
> >
> > Hi Andy,
> >
> > Thanks for your comments. We have only added the patch for clearing
> > the error bits. This resetting of QUP is present in the base code
> > itself. Since this QUP is being used by all the QCOM chips that's why
> > we have not changed the existing QUP reset logic. We will look
> > into reset polling logic separately and will post the patch for the
> > same.
> 
> Yeah, it is present in the current code.  I hadn't seen any errata on
> this issue, so that is why I was curious.  In many cases, the code in
> the mainline driver was carried forward from the downstream CAF
> driver.  I know the two don't look that similar in some regards, but
> in most cases the actions taken are the same, even if it is not
> obvious.
> 
> > We got the input for QUP RESET by referring our working internal
> > drivers and we also faced the BAM hang issue in base upstream
> > driver, if we don't do this RESET. If we schedule any transfer
> > with DMA mode then the DMA Engine (BAM) will never get any interrupt
> > for these errors, since the QUP interrupt handles the error cases.
> > The BAM will generate the interrupt only when it will receive the
> > requested amount of data. This reset will clear the QUP FIFO and
> > we will get the BAM transfer completion interrupt by the existing
> > logic present in the base bam transfer function.
> 
> Right.  If the QUP has an issue and isn't processing data, and you
> have a BAM transaction pending for this, you have to not only ack the
> qup error, but also terminate the BAM transaction.
> 
> What test case are you using to test out the error path?  Something
> other than NACKs?
> 
> Regards,
> 
> Andy

Hi Andy,

We run the command i2cdetect for address 0x3 to 0x77. The QUP generates
write error for address 0x3 to 0x7 apart from other bus errors since
these are reserved addresses. I was getting the crash in non DMA mode
and BAM hang in DMA mode before putting the fix.

Also we have connected the I2C TPM and run the following script
overnight for both DMA and Non DMA mode.  The script checks for
all transfer length and we are generating multiple NACK and
Non NACK error before each valid transfer.

a=1

while [ $a -lt 4096 ]
do
   echo $a
   i2cdetect -y -a -r 0 0x03 0x77
   tpm-manager get_random $a
   i2cdetect -y -a -r 1 0x03 0x77
   a=`expr $a + 1`
done

--
Abhishek Sahu

[PATCH 1/2] i2c: qup: Cleared the error bits in ISR

[Wolfram Sang]

> We run the command i2cdetect for address 0x3 to 0x77. The QUP generates
> write error for address 0x3 to 0x7 apart from other bus errors since
> these are reserved addresses. I was getting the crash in non DMA mode
> and BAM hang in DMA mode before putting the fix.
> 
> Also we have connected the I2C TPM and run the following script
> overnight for both DMA and Non DMA mode.  The script checks for
> all transfer length and we are generating multiple NACK and
> Non NACK error before each valid transfer.
> 
> a=1
> 
> while [ $a -lt 4096 ]
> do
>    echo $a
>    i2cdetect -y -a -r 0 0x03 0x77
>    tpm-manager get_random $a
>    i2cdetect -y -a -r 1 0x03 0x77
>    a=`expr $a + 1`
> done

So, what is the outcome of this discussion?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160618/51eabe63/attachment.sig>

[PATCH 1/2] i2c: qup: Cleared the error bits in ISR

[Abhishek Sahu]

On 2016-06-18 22:02, Wolfram Sang wrote:
>> We run the command i2cdetect for address 0x3 to 0x77. The QUP 
>> generates
>> write error for address 0x3 to 0x7 apart from other bus errors since
>> these are reserved addresses. I was getting the crash in non DMA mode
>> and BAM hang in DMA mode before putting the fix.
>> 
>> Also we have connected the I2C TPM and run the following script
>> overnight for both DMA and Non DMA mode.  The script checks for
>> all transfer length and we are generating multiple NACK and
>> Non NACK error before each valid transfer.
>> 
>> a=1
>> 
>> while [ $a -lt 4096 ]
>> do
>>    echo $a
>>    i2cdetect -y -a -r 0 0x03 0x77
>>    tpm-manager get_random $a
>>    i2cdetect -y -a -r 1 0x03 0x77
>>    a=`expr $a + 1`
>> done
> 
> So, what is the outcome of this discussion?

Discussion was regarding resetting the QUP state in ISR and it is the 
part of
existing code itself. The current patch only added the error checking 
for bus
errors so we can go ahead with this patch. I have shared the script 
which we
are using for testing error path.

[PATCH 1/2] i2c: qup: Cleared the error bits in ISR

[Wolfram Sang]

On Mon, May 09, 2016 at 06:14:30PM +0530, Abhishek Sahu wrote:
> 1. Current QCOM I2C driver hangs when sending data to address 0x03-0x07
> in some scenarios. The QUP controller generates invalid write in this
> case, since these addresses are reserved for different bus formats.
> 
> 2. Also, the error handling is done by I2C QUP ISR in the case of DMA
> mode. The state need to be RESET in case of any error for clearing the
> available data in FIFO, which otherwise leaves the BAM DMA controller
> in hang state.
> 
> This patch fixes the above two issues by clearing the error bits from
> I2C and QUP status in ISR in case of I2C error, QUP error and resets
> the QUP state to clear the FIFO data.
> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>

Applied to for-next, thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160715/091568a5/attachment.sig>

[PATCH 2/2] i2c: qup: Fixed the DMA segments length

[Abhishek Sahu]

1. The current QCOM I2C driver code is failing for transfer length
greater than 255. This is happening due to improper segments length
as the I2C DMA segments can be maximum of 256 bytes.

2. The transfer length tlen was being initialized with 0 for 256
bytes, which is being passed for DMA mappings resulting in improper
DMA mapping length.

This patch fixes the above said problems by initializing the block
count with the values calculated in qup_i2c_set_blk_data and calculating
the remaining length for last DMA segment. Also, the block data length
need to be decremented after each transfer. Additionally, this patch
corrects the tlen assignment for DMA mapping.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
 drivers/i2c/busses/i2c-qup.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index 8c2f1bc..6d6b7dc 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -651,23 +651,24 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
 	u8 *tags;
 
 	while (idx < num) {
-		blocks = (msg->len + limit) / limit;
-		rem = msg->len % limit;
 		tx_len = 0, len = 0, i = 0;
 
 		qup->is_last = (idx == (num - 1));
 
 		qup_i2c_set_blk_data(qup, msg);
 
+		blocks = qup->blk.count;
+		rem = msg->len - (blocks - 1) * limit;
+
 		if (msg->flags & I2C_M_RD) {
 			rx_nents += (blocks * 2) + 1;
 			tx_nents += 1;
 
 			while (qup->blk.pos < blocks) {
-				/* length set to '0' implies 256 bytes */
-				tlen = (i == (blocks - 1)) ? rem : 0;
+				tlen = (i == (blocks - 1)) ? rem : limit;
 				tags = &qup->start_tag.start[off + len];
 				len += qup_i2c_set_tags(tags, qup, msg, 1);
+				qup->blk.data_len -= tlen;
 
 				/* scratch buf to read the start and len tags */
 				ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
@@ -706,9 +707,10 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
 			tx_nents += (blocks * 2);
 
 			while (qup->blk.pos < blocks) {
-				tlen = (i == (blocks - 1)) ? rem : 0;
+				tlen = (i == (blocks - 1)) ? rem : limit;
 				tags = &qup->start_tag.start[off + tx_len];
 				len = qup_i2c_set_tags(tags, qup, msg, 1);
+				qup->blk.data_len -= tlen;
 
 				ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],
 						     tags,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/2] i2c: qup: Fixed the DMA segments length

[Sricharan]

Hi,

> 1. The current QCOM I2C driver code is failing for transfer length greater
> than 255. This is happening due to improper segments length as the I2C DMA
> segments can be maximum of 256 bytes.
> 
> 2. The transfer length tlen was being initialized with 0 for 256 bytes,
which is
> being passed for DMA mappings resulting in improper DMA mapping length.
> 
> This patch fixes the above said problems by initializing the block count
with
> the values calculated in qup_i2c_set_blk_data and calculating the
remaining
> length for last DMA segment. Also, the block data length need to be
> decremented after each transfer. Additionally, this patch corrects the
tlen
> assignment for DMA mapping.
> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
>  drivers/i2c/busses/i2c-qup.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index
> 8c2f1bc..6d6b7dc 100644
> --- a/drivers/i2c/busses/i2c-qup.c
> +++ b/drivers/i2c/busses/i2c-qup.c
> @@ -651,23 +651,24 @@ static int qup_i2c_bam_do_xfer(struct
> qup_i2c_dev *qup, struct i2c_msg *msg,
>  	u8 *tags;
> 
>  	while (idx < num) {
> -		blocks = (msg->len + limit) / limit;
> -		rem = msg->len % limit;
>  		tx_len = 0, len = 0, i = 0;
> 
>  		qup->is_last = (idx == (num - 1));
> 
>  		qup_i2c_set_blk_data(qup, msg);
> 
> +		blocks = qup->blk.count;
> +		rem = msg->len - (blocks - 1) * limit;
> +
      Same if we had blocks = (msg->len + limit - 1) / limit instead of the
above ?
     Otherwise, 
     Reviewed-by: Sricharan at codeaurora.org

Regards,
 Sricharan

[PATCH 2/2] i2c: qup: Fixed the DMA segments length

[Abhishek Sahu]

On 2016-05-11 21:48, Sricharan wrote:
> Hi,
> 
>> 1. The current QCOM I2C driver code is failing for transfer length 
>> greater
>> than 255. This is happening due to improper segments length as the I2C 
>> DMA
>> segments can be maximum of 256 bytes.
>> 
>> 2. The transfer length tlen was being initialized with 0 for 256 
>> bytes,
>> which is
>> being passed for DMA mappings resulting in improper DMA mapping 
>> length.
>> 
>> This patch fixes the above said problems by initializing the block 
>> count
>> with
>> the values calculated in qup_i2c_set_blk_data and calculating the
> remaining
>> length for last DMA segment. Also, the block data length need to be
>> decremented after each transfer. Additionally, this patch corrects the
>> tlen
>> assignment for DMA mapping.
>> 
>> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>> ---
>>  drivers/i2c/busses/i2c-qup.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/i2c/busses/i2c-qup.c 
>> b/drivers/i2c/busses/i2c-qup.c
>> index
>> 8c2f1bc..6d6b7dc 100644
>> --- a/drivers/i2c/busses/i2c-qup.c
>> +++ b/drivers/i2c/busses/i2c-qup.c
>> @@ -651,23 +651,24 @@ static int qup_i2c_bam_do_xfer(struct
>> qup_i2c_dev *qup, struct i2c_msg *msg,
>>  	u8 *tags;
>> 
>>  	while (idx < num) {
>> -		blocks = (msg->len + limit) / limit;
>> -		rem = msg->len % limit;
>>  		tx_len = 0, len = 0, i = 0;
>> 
>>  		qup->is_last = (idx == (num - 1));
>> 
>>  		qup_i2c_set_blk_data(qup, msg);
>> 
>> +		blocks = qup->blk.count;
>> +		rem = msg->len - (blocks - 1) * limit;
>> +
>       Same if we had blocks = (msg->len + limit - 1) / limit instead of 
> the
> above ?
Yes. We calculating this inside qup_i2c_set_blk_data and assigning the 
same value to
qup->blk.count so I am using this to avoid redundant arithmetic 
operations.
>      Otherwise,
>      Reviewed-by: Sricharan at codeaurora.org
> 
> Regards,
>  Sricharan

-- 
Abhishek Sahu

[PATCH 2/2] i2c: qup: Fixed the DMA segments length

[Wolfram Sang]

On Mon, May 09, 2016 at 06:14:31PM +0530, Abhishek Sahu wrote:
> 1. The current QCOM I2C driver code is failing for transfer length
> greater than 255. This is happening due to improper segments length
> as the I2C DMA segments can be maximum of 256 bytes.
> 
> 2. The transfer length tlen was being initialized with 0 for 256
> bytes, which is being passed for DMA mappings resulting in improper
> DMA mapping length.
> 
> This patch fixes the above said problems by initializing the block
> count with the values calculated in qup_i2c_set_blk_data and calculating
> the remaining length for last DMA segment. Also, the block data length
> need to be decremented after each transfer. Additionally, this patch
> corrects the tlen assignment for DMA mapping.
> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>

Applied to for-next, thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160715/563cf6af/attachment.sig>