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

[Sricharan]

Hi,

> -----Original Message-----
> From: linux-arm-kernel [mailto:linux-arm-kernel-
> bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Abhishek Sahu
> Sent: Wednesday, May 11, 2016 11:04 PM
> To: Sricharan <sricharan@xxxxxxxxxxxxxx>
> Cc: architt@xxxxxxxxxxxxxx; wsa@xxxxxxxxxxxxx; linux-arm-
> msm@xxxxxxxxxxxxxxx; ntelkar@xxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; dmaengine@xxxxxxxxxxxxxxx; linux-
> i2c@xxxxxxxxxxxxxxx; agross@xxxxxxxxxxxxxx; andy.gross@xxxxxxxxxx; linux-
> arm-kernel@xxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxx>
> >> ---
> >>  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