Performing i2c write operation while SDA or SCL line is held
or grounded by slave device, we go into infinite at91_twi_write_next_byte
loop with TXRDY interrupt spam.
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/i2c/busses/i2c-at91.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 3f3e8b3..b2f5fdb 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -72,6 +72,7 @@
#define AT91_TWI_TXCOMP BIT(0) /* Transmission Complete */
#define AT91_TWI_RXRDY BIT(1) /* Receive Holding Register Ready */
#define AT91_TWI_TXRDY BIT(2) /* Transmit Holding Register Ready */
+#define AT91_TWI_SVREAD BIT(3) /* Slave Read */
#define AT91_TWI_OVRE BIT(6) /* Overrun Error */
#define AT91_TWI_UNRE BIT(7) /* Underrun Error */
#define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
@@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
at91_disable_twi_interrupts(dev);
complete(&dev->cmd_complete);
} else if (irqstatus & AT91_TWI_TXRDY) {
- at91_twi_write_next_byte(dev);
+ if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
+ at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
+ else
+ at91_twi_write_next_byte(dev);
}
/* catch error flags */
--
2.7.4
Hello Raag,
On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
> External E-Mail
>
>
> Performing i2c write operation while SDA or SCL line is held
> or grounded by slave device, we go into infinite at91_twi_write_next_byte
> loop with TXRDY interrupt spam.
Sorry but I am not sure to have the full picture, the controller is in
slave or master mode?
SVREAD is only used in slave mode. When SVREAD is set, it means that a read
access is performed and your issue concerns the write operation.
Regards
Ludovic
>
> Signed-off-by: Raag Jadav <[email protected]>
> ---
> drivers/i2c/busses/i2c-at91.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 3f3e8b3..b2f5fdb 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -72,6 +72,7 @@
> #define AT91_TWI_TXCOMP BIT(0) /* Transmission Complete */
> #define AT91_TWI_RXRDY BIT(1) /* Receive Holding Register Ready */
> #define AT91_TWI_TXRDY BIT(2) /* Transmit Holding Register Ready */
> +#define AT91_TWI_SVREAD BIT(3) /* Slave Read */
> #define AT91_TWI_OVRE BIT(6) /* Overrun Error */
> #define AT91_TWI_UNRE BIT(7) /* Underrun Error */
> #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
> @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> at91_disable_twi_interrupts(dev);
> complete(&dev->cmd_complete);
> } else if (irqstatus & AT91_TWI_TXRDY) {
> - at91_twi_write_next_byte(dev);
> + if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
> + at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
> + else
> + at91_twi_write_next_byte(dev);
> }
>
> /* catch error flags */
> --
> 2.7.4
>
>
On Mon, Apr 29, 2019 at 11:00:05AM +0200, Ludovic Desroches wrote:
> Hello Raag,
>
> On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
> > External E-Mail
> >
> >
> > Performing i2c write operation while SDA or SCL line is held
> > or grounded by slave device, we go into infinite at91_twi_write_next_byte
> > loop with TXRDY interrupt spam.
>
> Sorry but I am not sure to have the full picture, the controller is in
> slave or master mode?
>
> SVREAD is only used in slave mode. When SVREAD is set, it means that a read
> access is performed and your issue concerns the write operation.
>
> Regards
>
> Ludovic
Yes, even though the datasheet suggests that SVREAD is irrelevant in master mode,
TXRDY and SVREAD are the only ones being set in status register upon reproducing the issue.
Couldn't think of a better way to handle such strange behaviour.
Any suggestions would be appreciated.
Cheers,
Raag
>
> >
> > Signed-off-by: Raag Jadav <[email protected]>
> > ---
> > drivers/i2c/busses/i2c-at91.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> > index 3f3e8b3..b2f5fdb 100644
> > --- a/drivers/i2c/busses/i2c-at91.c
> > +++ b/drivers/i2c/busses/i2c-at91.c
> > @@ -72,6 +72,7 @@
> > #define AT91_TWI_TXCOMP BIT(0) /* Transmission Complete */
> > #define AT91_TWI_RXRDY BIT(1) /* Receive Holding Register Ready */
> > #define AT91_TWI_TXRDY BIT(2) /* Transmit Holding Register Ready */
> > +#define AT91_TWI_SVREAD BIT(3) /* Slave Read */
> > #define AT91_TWI_OVRE BIT(6) /* Overrun Error */
> > #define AT91_TWI_UNRE BIT(7) /* Underrun Error */
> > #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
> > @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> > at91_disable_twi_interrupts(dev);
> > complete(&dev->cmd_complete);
> > } else if (irqstatus & AT91_TWI_TXRDY) {
> > - at91_twi_write_next_byte(dev);
> > + if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
> > + at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
> > + else
> > + at91_twi_write_next_byte(dev);
> > }
> >
> > /* catch error flags */
> > --
> > 2.7.4
> >
> >
On Tue, Apr 30, 2019 at 04:03:32AM +0530, Raag Jadav wrote:
> External E-Mail
>
>
> On Mon, Apr 29, 2019 at 11:00:05AM +0200, Ludovic Desroches wrote:
> > Hello Raag,
> >
> > On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
> > > External E-Mail
> > >
> > >
> > > Performing i2c write operation while SDA or SCL line is held
> > > or grounded by slave device, we go into infinite at91_twi_write_next_byte
> > > loop with TXRDY interrupt spam.
> >
> > Sorry but I am not sure to have the full picture, the controller is in
> > slave or master mode?
> >
> > SVREAD is only used in slave mode. When SVREAD is set, it means that a read
> > access is performed and your issue concerns the write operation.
> >
> > Regards
> >
> > Ludovic
>
> Yes, even though the datasheet suggests that SVREAD is irrelevant in master mode,
> TXRDY and SVREAD are the only ones being set in status register upon reproducing the issue.
> Couldn't think of a better way to handle such strange behaviour.
> Any suggestions would be appreciated.
I have the confirmation that you can't rely on the SVREAD flag when in
master mode. This flag should always have the same value.
I am trying to understand what could lead to your situation. Can you
give me more details. What kind of device it is? What does lead to this
situation? Does it happen randomly or not?
Regards
Ludovic
>
> Cheers,
> Raag
>
> >
> > >
> > > Signed-off-by: Raag Jadav <[email protected]>
> > > ---
> > > drivers/i2c/busses/i2c-at91.c | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> > > index 3f3e8b3..b2f5fdb 100644
> > > --- a/drivers/i2c/busses/i2c-at91.c
> > > +++ b/drivers/i2c/busses/i2c-at91.c
> > > @@ -72,6 +72,7 @@
> > > #define AT91_TWI_TXCOMP BIT(0) /* Transmission Complete */
> > > #define AT91_TWI_RXRDY BIT(1) /* Receive Holding Register Ready */
> > > #define AT91_TWI_TXRDY BIT(2) /* Transmit Holding Register Ready */
> > > +#define AT91_TWI_SVREAD BIT(3) /* Slave Read */
> > > #define AT91_TWI_OVRE BIT(6) /* Overrun Error */
> > > #define AT91_TWI_UNRE BIT(7) /* Underrun Error */
> > > #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
> > > @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> > > at91_disable_twi_interrupts(dev);
> > > complete(&dev->cmd_complete);
> > > } else if (irqstatus & AT91_TWI_TXRDY) {
> > > - at91_twi_write_next_byte(dev);
> > > + if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
> > > + at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
> > > + else
> > > + at91_twi_write_next_byte(dev);
> > > }
> > >
> > > /* catch error flags */
> > > --
> > > 2.7.4
> > >
> > >
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On Thu, May 02, 2019 at 04:01:16PM +0200, Ludovic Desroches wrote:
> On Tue, Apr 30, 2019 at 04:03:32AM +0530, Raag Jadav wrote:
> > External E-Mail
> >
> >
> > On Mon, Apr 29, 2019 at 11:00:05AM +0200, Ludovic Desroches wrote:
> > > Hello Raag,
> > >
> > > On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
> > > > External E-Mail
> > > >
> > > >
> > > > Performing i2c write operation while SDA or SCL line is held
> > > > or grounded by slave device, we go into infinite at91_twi_write_next_byte
> > > > loop with TXRDY interrupt spam.
> > >
> > > Sorry but I am not sure to have the full picture, the controller is in
> > > slave or master mode?
> > >
> > > SVREAD is only used in slave mode. When SVREAD is set, it means that a read
> > > access is performed and your issue concerns the write operation.
> > >
> > > Regards
> > >
> > > Ludovic
> >
> > Yes, even though the datasheet suggests that SVREAD is irrelevant in master mode,
> > TXRDY and SVREAD are the only ones being set in status register upon reproducing the issue.
> > Couldn't think of a better way to handle such strange behaviour.
> > Any suggestions would be appreciated.
>
> I have the confirmation that you can't rely on the SVREAD flag when in
> master mode. This flag should always have the same value.
>
> I am trying to understand what could lead to your situation. Can you
> give me more details. What kind of device it is? What does lead to this
> situation? Does it happen randomly or not?
One of the sama5d2 based board I worked on, was having trouble complete its boot
because of a faulty i2c device, which was randomly holding down the SDA line
on i2c write operation, not allowing the controller to complete its transmission,
causing a massive TXRDY interrupt spam, ultimately hanging the processor.
Another strange observation was that SVREAD was being set in the status register
along with TXRDY, every time I reproduced the issue.
You can reproduce it by simply grounding the SDA line and performing i2c write
on the bus.
Note that NACK, LOCK or TXCOMP are never set as the transmission never completes.
I'm not sure why slave bits are being set in master mode,
but it's been working reliably for me.
This patch doesn't recover the SDA line. It just prevents the processor from
getting hanged in case of i2c bus lockup.
Cheers,
Raag
>
> Regards
>
> Ludovic
>
> >
> > Cheers,
> > Raag
> >
> > >
> > > >
> > > > Signed-off-by: Raag Jadav <[email protected]>
> > > > ---
> > > > drivers/i2c/busses/i2c-at91.c | 6 +++++-
> > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> > > > index 3f3e8b3..b2f5fdb 100644
> > > > --- a/drivers/i2c/busses/i2c-at91.c
> > > > +++ b/drivers/i2c/busses/i2c-at91.c
> > > > @@ -72,6 +72,7 @@
> > > > #define AT91_TWI_TXCOMP BIT(0) /* Transmission Complete */
> > > > #define AT91_TWI_RXRDY BIT(1) /* Receive Holding Register Ready */
> > > > #define AT91_TWI_TXRDY BIT(2) /* Transmit Holding Register Ready */
> > > > +#define AT91_TWI_SVREAD BIT(3) /* Slave Read */
> > > > #define AT91_TWI_OVRE BIT(6) /* Overrun Error */
> > > > #define AT91_TWI_UNRE BIT(7) /* Underrun Error */
> > > > #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
> > > > @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> > > > at91_disable_twi_interrupts(dev);
> > > > complete(&dev->cmd_complete);
> > > > } else if (irqstatus & AT91_TWI_TXRDY) {
> > > > - at91_twi_write_next_byte(dev);
> > > > + if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
> > > > + at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
> > > > + else
> > > > + at91_twi_write_next_byte(dev);
> > > > }
> > > >
> > > > /* catch error flags */
> > > > --
> > > > 2.7.4
> > > >
> > > >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
On 04.05.2019 02:58, Raag Jadav wrote:
> On Thu, May 02, 2019 at 04:01:16PM +0200, Ludovic Desroches wrote:
>> On Tue, Apr 30, 2019 at 04:03:32AM +0530, Raag Jadav wrote:
>>> External E-Mail
>>>
>>>
>>> On Mon, Apr 29, 2019 at 11:00:05AM +0200, Ludovic Desroches wrote:
>>>> Hello Raag,
>>>>
>>>> On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
>>>>> External E-Mail
>>>>>
>>>>>
>>>>> Performing i2c write operation while SDA or SCL line is held
>>>>> or grounded by slave device, we go into infinite at91_twi_write_next_byte
>>>>> loop with TXRDY interrupt spam.
>>>>
>>>> Sorry but I am not sure to have the full picture, the controller is in
>>>> slave or master mode?
>>>>
>>>> SVREAD is only used in slave mode. When SVREAD is set, it means that a read
>>>> access is performed and your issue concerns the write operation.
>>>>
>>>> Regards
>>>>
>>>> Ludovic
>>>
>>> Yes, even though the datasheet suggests that SVREAD is irrelevant in master mode,
>>> TXRDY and SVREAD are the only ones being set in status register upon reproducing the issue.
>>> Couldn't think of a better way to handle such strange behaviour.
>>> Any suggestions would be appreciated.
>>
>> I have the confirmation that you can't rely on the SVREAD flag when in
>> master mode. This flag should always have the same value.
>>
>> I am trying to understand what could lead to your situation. Can you
>> give me more details. What kind of device it is? What does lead to this
>> situation? Does it happen randomly or not?
>
> One of the sama5d2 based board I worked on, was having trouble complete its boot
> because of a faulty i2c device, which was randomly holding down the SDA line
> on i2c write operation, not allowing the controller to complete its transmission,
> causing a massive TXRDY interrupt spam, ultimately hanging the processor.
>
> Another strange observation was that SVREAD was being set in the status register
> along with TXRDY, every time I reproduced the issue.
> You can reproduce it by simply grounding the SDA line and performing i2c write
> on the bus.
>
> Note that NACK, LOCK or TXCOMP are never set as the transmission never completes.
> I'm not sure why slave bits are being set in master mode,
> but it's been working reliably for me.
>
> This patch doesn't recover the SDA line. It just prevents the processor from
> getting hanged in case of i2c bus lockup.
Hello,
I have noticed the same hanging at some points... In my case it is
because of this patch:
commit e8f39e9fc0e0b7bce24922da925af820bacb8ef8
Author: David Engraf <[email protected]>
Date: Thu Apr 26 11:53:14 2018 +0200
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index bfd1fdf..3f3e8b3 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -518,8 +518,16 @@ static irqreturn_t atmel_twi_interrupt(int irq,
void *dev_id)
* the RXRDY interrupt first in order to not keep garbage data
in the
* Receive Holding Register for the next transfer.
*/
- if (irqstatus & AT91_TWI_RXRDY)
- at91_twi_read_next_byte(dev);
+ if (irqstatus & AT91_TWI_RXRDY) {
+ /*
+ * Read all available bytes at once by polling RXRDY
usable w/
+ * and w/o FIFO. With FIFO enabled we could also read
RXFL and
+ * avoid polling RXRDY.
+ */
+ do {
+ at91_twi_read_next_byte(dev);
+ } while (at91_twi_read(dev, AT91_TWI_SR) & AT91_TWI_RXRDY);
+ }
In my opinion having a do/while with an exit condition relying solely on
a bit read from hardware is unacceptable in IRQ context - kernel can
hang here.
A timeout would be a solution...
For me, reverting this patch solves hanging issues.
Hope this helps,
Eugen
>
> Cheers,
> Raag
>
>>
>> Regards
>>
>> Ludovic
>>
>>>
>>> Cheers,
>>> Raag
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Raag Jadav <[email protected]>
>>>>> ---
>>>>> drivers/i2c/busses/i2c-at91.c | 6 +++++-
>>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
>>>>> index 3f3e8b3..b2f5fdb 100644
>>>>> --- a/drivers/i2c/busses/i2c-at91.c
>>>>> +++ b/drivers/i2c/busses/i2c-at91.c
>>>>> @@ -72,6 +72,7 @@
>>>>> #define AT91_TWI_TXCOMP BIT(0) /* Transmission Complete */
>>>>> #define AT91_TWI_RXRDY BIT(1) /* Receive Holding Register Ready */
>>>>> #define AT91_TWI_TXRDY BIT(2) /* Transmit Holding Register Ready */
>>>>> +#define AT91_TWI_SVREAD BIT(3) /* Slave Read */
>>>>> #define AT91_TWI_OVRE BIT(6) /* Overrun Error */
>>>>> #define AT91_TWI_UNRE BIT(7) /* Underrun Error */
>>>>> #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
>>>>> @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
>>>>> at91_disable_twi_interrupts(dev);
>>>>> complete(&dev->cmd_complete);
>>>>> } else if (irqstatus & AT91_TWI_TXRDY) {
>>>>> - at91_twi_write_next_byte(dev);
>>>>> + if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
>>>>> + at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
>>>>> + else
>>>>> + at91_twi_write_next_byte(dev);
>>>>> }
>>>>>
>>>>> /* catch error flags */
>>>>> --
>>>>> 2.7.4
>>>>>
>>>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> [email protected]
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
On Mon, May 06, 2019 at 08:19:01AM +0000, [email protected] wrote:
>
>
> On 04.05.2019 02:58, Raag Jadav wrote:
>
> > On Thu, May 02, 2019 at 04:01:16PM +0200, Ludovic Desroches wrote:
> >> On Tue, Apr 30, 2019 at 04:03:32AM +0530, Raag Jadav wrote:
> >>> External E-Mail
> >>>
> >>>
> >>> On Mon, Apr 29, 2019 at 11:00:05AM +0200, Ludovic Desroches wrote:
> >>>> Hello Raag,
> >>>>
> >>>> On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
> >>>>> External E-Mail
> >>>>>
> >>>>>
> >>>>> Performing i2c write operation while SDA or SCL line is held
> >>>>> or grounded by slave device, we go into infinite at91_twi_write_next_byte
> >>>>> loop with TXRDY interrupt spam.
> >>>>
> >>>> Sorry but I am not sure to have the full picture, the controller is in
> >>>> slave or master mode?
> >>>>
> >>>> SVREAD is only used in slave mode. When SVREAD is set, it means that a read
> >>>> access is performed and your issue concerns the write operation.
> >>>>
> >>>> Regards
> >>>>
> >>>> Ludovic
> >>>
> >>> Yes, even though the datasheet suggests that SVREAD is irrelevant in master mode,
> >>> TXRDY and SVREAD are the only ones being set in status register upon reproducing the issue.
> >>> Couldn't think of a better way to handle such strange behaviour.
> >>> Any suggestions would be appreciated.
> >>
> >> I have the confirmation that you can't rely on the SVREAD flag when in
> >> master mode. This flag should always have the same value.
> >>
> >> I am trying to understand what could lead to your situation. Can you
> >> give me more details. What kind of device it is? What does lead to this
> >> situation? Does it happen randomly or not?
> >
> > One of the sama5d2 based board I worked on, was having trouble complete its boot
> > because of a faulty i2c device, which was randomly holding down the SDA line
> > on i2c write operation, not allowing the controller to complete its transmission,
> > causing a massive TXRDY interrupt spam, ultimately hanging the processor.
> >
> > Another strange observation was that SVREAD was being set in the status register
> > along with TXRDY, every time I reproduced the issue.
> > You can reproduce it by simply grounding the SDA line and performing i2c write
> > on the bus.
> >
> > Note that NACK, LOCK or TXCOMP are never set as the transmission never completes.
> > I'm not sure why slave bits are being set in master mode,
> > but it's been working reliably for me.
> >
> > This patch doesn't recover the SDA line. It just prevents the processor from
> > getting hanged in case of i2c bus lockup.
>
> Hello,
>
> I have noticed the same hanging at some points... In my case it is
> because of this patch:
>
> commit e8f39e9fc0e0b7bce24922da925af820bacb8ef8
> Author: David Engraf <[email protected]>
> Date: Thu Apr 26 11:53:14 2018 +0200
>
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index bfd1fdf..3f3e8b3 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -518,8 +518,16 @@ static irqreturn_t atmel_twi_interrupt(int irq,
> void *dev_id)
> * the RXRDY interrupt first in order to not keep garbage data
> in the
> * Receive Holding Register for the next transfer.
> */
> - if (irqstatus & AT91_TWI_RXRDY)
> - at91_twi_read_next_byte(dev);
> + if (irqstatus & AT91_TWI_RXRDY) {
> + /*
> + * Read all available bytes at once by polling RXRDY
> usable w/
> + * and w/o FIFO. With FIFO enabled we could also read
> RXFL and
> + * avoid polling RXRDY.
> + */
> + do {
> + at91_twi_read_next_byte(dev);
> + } while (at91_twi_read(dev, AT91_TWI_SR) & AT91_TWI_RXRDY);
> + }
>
>
> In my opinion having a do/while with an exit condition relying solely on
> a bit read from hardware is unacceptable in IRQ context - kernel can
> hang here.
> A timeout would be a solution...
>
> For me, reverting this patch solves hanging issues.
>
> Hope this helps,
>
> Eugen
Thank you for your input, but my issue concerns i2c write operation.
I haven't noticed any issue with i2c read yet.
But given the same scenario, it could be true for RXRDY as well.
Cheers,
Raag
>
> >
> > Cheers,
> > Raag
> >
> >>
> >> Regards
> >>
> >> Ludovic
> >>
> >>>
> >>> Cheers,
> >>> Raag
> >>>
> >>>>
> >>>>>
> >>>>> Signed-off-by: Raag Jadav <[email protected]>
> >>>>> ---
> >>>>> drivers/i2c/busses/i2c-at91.c | 6 +++++-
> >>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> >>>>> index 3f3e8b3..b2f5fdb 100644
> >>>>> --- a/drivers/i2c/busses/i2c-at91.c
> >>>>> +++ b/drivers/i2c/busses/i2c-at91.c
> >>>>> @@ -72,6 +72,7 @@
> >>>>> #define AT91_TWI_TXCOMP BIT(0) /* Transmission Complete */
> >>>>> #define AT91_TWI_RXRDY BIT(1) /* Receive Holding Register Ready */
> >>>>> #define AT91_TWI_TXRDY BIT(2) /* Transmit Holding Register Ready */
> >>>>> +#define AT91_TWI_SVREAD BIT(3) /* Slave Read */
> >>>>> #define AT91_TWI_OVRE BIT(6) /* Overrun Error */
> >>>>> #define AT91_TWI_UNRE BIT(7) /* Underrun Error */
> >>>>> #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
> >>>>> @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> >>>>> at91_disable_twi_interrupts(dev);
> >>>>> complete(&dev->cmd_complete);
> >>>>> } else if (irqstatus & AT91_TWI_TXRDY) {
> >>>>> - at91_twi_write_next_byte(dev);
> >>>>> + if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
> >>>>> + at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
> >>>>> + else
> >>>>> + at91_twi_write_next_byte(dev);
> >>>>> }
> >>>>>
> >>>>> /* catch error flags */
> >>>>> --
> >>>>> 2.7.4
> >>>>>
> >>>>>
> >>>
> >>> _______________________________________________
> >>> linux-arm-kernel mailing list
> >>> [email protected]
> >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>>
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> >
On Sat, May 04, 2019 at 05:28:51AM +0530, Raag Jadav wrote:
> On Thu, May 02, 2019 at 04:01:16PM +0200, Ludovic Desroches wrote:
> > On Tue, Apr 30, 2019 at 04:03:32AM +0530, Raag Jadav wrote:
> > > External E-Mail
> > >
> > >
> > > On Mon, Apr 29, 2019 at 11:00:05AM +0200, Ludovic Desroches wrote:
> > > > Hello Raag,
> > > >
> > > > On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
> > > > > External E-Mail
> > > > >
> > > > >
> > > > > Performing i2c write operation while SDA or SCL line is held
> > > > > or grounded by slave device, we go into infinite at91_twi_write_next_byte
> > > > > loop with TXRDY interrupt spam.
> > > >
> > > > Sorry but I am not sure to have the full picture, the controller is in
> > > > slave or master mode?
> > > >
> > > > SVREAD is only used in slave mode. When SVREAD is set, it means that a read
> > > > access is performed and your issue concerns the write operation.
> > > >
> > > > Regards
> > > >
> > > > Ludovic
> > >
> > > Yes, even though the datasheet suggests that SVREAD is irrelevant in master mode,
> > > TXRDY and SVREAD are the only ones being set in status register upon reproducing the issue.
> > > Couldn't think of a better way to handle such strange behaviour.
> > > Any suggestions would be appreciated.
> >
> > I have the confirmation that you can't rely on the SVREAD flag when in
> > master mode. This flag should always have the same value.
> >
> > I am trying to understand what could lead to your situation. Can you
> > give me more details. What kind of device it is? What does lead to this
> > situation? Does it happen randomly or not?
>
> One of the sama5d2 based board I worked on, was having trouble complete its boot
> because of a faulty i2c device, which was randomly holding down the SDA line
> on i2c write operation, not allowing the controller to complete its transmission,
> causing a massive TXRDY interrupt spam, ultimately hanging the processor.
>
> Another strange observation was that SVREAD was being set in the status register
> along with TXRDY, every time I reproduced the issue.
> You can reproduce it by simply grounding the SDA line and performing i2c write
> on the bus.
Thanks for the details, I'll discussed it with hw guys but expect some
dealy as I'll be off next 2 weeks.
Regards
Ludovic
>
> Note that NACK, LOCK or TXCOMP are never set as the transmission never completes.
> I'm not sure why slave bits are being set in master mode,
> but it's been working reliably for me.
>
> This patch doesn't recover the SDA line. It just prevents the processor from
> getting hanged in case of i2c bus lockup.
>
> Cheers,
> Raag
>
> >
> > Regards
> >
> > Ludovic
> >
> > >
> > > Cheers,
> > > Raag
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Raag Jadav <[email protected]>
> > > > > ---
> > > > > drivers/i2c/busses/i2c-at91.c | 6 +++++-
> > > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> > > > > index 3f3e8b3..b2f5fdb 100644
> > > > > --- a/drivers/i2c/busses/i2c-at91.c
> > > > > +++ b/drivers/i2c/busses/i2c-at91.c
> > > > > @@ -72,6 +72,7 @@
> > > > > #define AT91_TWI_TXCOMP BIT(0) /* Transmission Complete */
> > > > > #define AT91_TWI_RXRDY BIT(1) /* Receive Holding Register Ready */
> > > > > #define AT91_TWI_TXRDY BIT(2) /* Transmit Holding Register Ready */
> > > > > +#define AT91_TWI_SVREAD BIT(3) /* Slave Read */
> > > > > #define AT91_TWI_OVRE BIT(6) /* Overrun Error */
> > > > > #define AT91_TWI_UNRE BIT(7) /* Underrun Error */
> > > > > #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
> > > > > @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> > > > > at91_disable_twi_interrupts(dev);
> > > > > complete(&dev->cmd_complete);
> > > > > } else if (irqstatus & AT91_TWI_TXRDY) {
> > > > > - at91_twi_write_next_byte(dev);
> > > > > + if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
> > > > > + at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
> > > > > + else
> > > > > + at91_twi_write_next_byte(dev);
> > > > > }
> > > > >
> > > > > /* catch error flags */
> > > > > --
> > > > > 2.7.4
> > > > >
> > > > >
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > [email protected]
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > >
On Mon, May 06, 2019 at 10:19:01AM +0200, Eugen Hristev - M18282 wrote:
>
>
> On 04.05.2019 02:58, Raag Jadav wrote:
>
> > On Thu, May 02, 2019 at 04:01:16PM +0200, Ludovic Desroches wrote:
> >> On Tue, Apr 30, 2019 at 04:03:32AM +0530, Raag Jadav wrote:
> >>> External E-Mail
> >>>
> >>>
> >>> On Mon, Apr 29, 2019 at 11:00:05AM +0200, Ludovic Desroches wrote:
> >>>> Hello Raag,
> >>>>
> >>>> On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
> >>>>> External E-Mail
> >>>>>
> >>>>>
> >>>>> Performing i2c write operation while SDA or SCL line is held
> >>>>> or grounded by slave device, we go into infinite at91_twi_write_next_byte
> >>>>> loop with TXRDY interrupt spam.
> >>>>
> >>>> Sorry but I am not sure to have the full picture, the controller is in
> >>>> slave or master mode?
> >>>>
> >>>> SVREAD is only used in slave mode. When SVREAD is set, it means that a read
> >>>> access is performed and your issue concerns the write operation.
> >>>>
> >>>> Regards
> >>>>
> >>>> Ludovic
> >>>
> >>> Yes, even though the datasheet suggests that SVREAD is irrelevant in master mode,
> >>> TXRDY and SVREAD are the only ones being set in status register upon reproducing the issue.
> >>> Couldn't think of a better way to handle such strange behaviour.
> >>> Any suggestions would be appreciated.
> >>
> >> I have the confirmation that you can't rely on the SVREAD flag when in
> >> master mode. This flag should always have the same value.
> >>
> >> I am trying to understand what could lead to your situation. Can you
> >> give me more details. What kind of device it is? What does lead to this
> >> situation? Does it happen randomly or not?
> >
> > One of the sama5d2 based board I worked on, was having trouble complete its boot
> > because of a faulty i2c device, which was randomly holding down the SDA line
> > on i2c write operation, not allowing the controller to complete its transmission,
> > causing a massive TXRDY interrupt spam, ultimately hanging the processor.
> >
> > Another strange observation was that SVREAD was being set in the status register
> > along with TXRDY, every time I reproduced the issue.
> > You can reproduce it by simply grounding the SDA line and performing i2c write
> > on the bus.
> >
> > Note that NACK, LOCK or TXCOMP are never set as the transmission never completes.
> > I'm not sure why slave bits are being set in master mode,
> > but it's been working reliably for me.
> >
> > This patch doesn't recover the SDA line. It just prevents the processor from
> > getting hanged in case of i2c bus lockup.
>
> Hello,
>
> I have noticed the same hanging at some points... In my case it is
> because of this patch:
>
> commit e8f39e9fc0e0b7bce24922da925af820bacb8ef8
> Author: David Engraf <[email protected]>
> Date: Thu Apr 26 11:53:14 2018 +0200
>
Good to know.
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index bfd1fdf..3f3e8b3 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -518,8 +518,16 @@ static irqreturn_t atmel_twi_interrupt(int irq,
> void *dev_id)
> * the RXRDY interrupt first in order to not keep garbage data
> in the
> * Receive Holding Register for the next transfer.
> */
> - if (irqstatus & AT91_TWI_RXRDY)
> - at91_twi_read_next_byte(dev);
> + if (irqstatus & AT91_TWI_RXRDY) {
> + /*
> + * Read all available bytes at once by polling RXRDY
> usable w/
> + * and w/o FIFO. With FIFO enabled we could also read
> RXFL and
> + * avoid polling RXRDY.
> + */
> + do {
> + at91_twi_read_next_byte(dev);
> + } while (at91_twi_read(dev, AT91_TWI_SR) & AT91_TWI_RXRDY);
> + }
>
>
> In my opinion having a do/while with an exit condition relying solely on
> a bit read from hardware is unacceptable in IRQ context - kernel can
> hang here.
> A timeout would be a solution...
You're right with a faulty hardware it can lead to disaster. As you
mentionned issues with this patch, the end of loop condition is not good
as it can stay true indefinitely.
For sure a timeout is a solution but its value can be controversial.
Maybe there is a better combination of flags to check in the status
register. I'll see this point too.
Regards
Ludovic
>
> For me, reverting this patch solves hanging issues.
>
> Hope this helps,
>
> Eugen
>
> >
> > Cheers,
> > Raag
> >
> >>
> >> Regards
> >>
> >> Ludovic
> >>
> >>>
> >>> Cheers,
> >>> Raag
> >>>
> >>>>
> >>>>>
> >>>>> Signed-off-by: Raag Jadav <[email protected]>
> >>>>> ---
> >>>>> drivers/i2c/busses/i2c-at91.c | 6 +++++-
> >>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> >>>>> index 3f3e8b3..b2f5fdb 100644
> >>>>> --- a/drivers/i2c/busses/i2c-at91.c
> >>>>> +++ b/drivers/i2c/busses/i2c-at91.c
> >>>>> @@ -72,6 +72,7 @@
> >>>>> #define AT91_TWI_TXCOMP BIT(0) /* Transmission Complete */
> >>>>> #define AT91_TWI_RXRDY BIT(1) /* Receive Holding Register Ready */
> >>>>> #define AT91_TWI_TXRDY BIT(2) /* Transmit Holding Register Ready */
> >>>>> +#define AT91_TWI_SVREAD BIT(3) /* Slave Read */
> >>>>> #define AT91_TWI_OVRE BIT(6) /* Overrun Error */
> >>>>> #define AT91_TWI_UNRE BIT(7) /* Underrun Error */
> >>>>> #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
> >>>>> @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> >>>>> at91_disable_twi_interrupts(dev);
> >>>>> complete(&dev->cmd_complete);
> >>>>> } else if (irqstatus & AT91_TWI_TXRDY) {
> >>>>> - at91_twi_write_next_byte(dev);
> >>>>> + if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
> >>>>> + at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
> >>>>> + else
> >>>>> + at91_twi_write_next_byte(dev);
> >>>>> }
> >>>>>
> >>>>> /* catch error flags */
> >>>>> --
> >>>>> 2.7.4
> >>>>>
> >>>>>
> >>>
> >>> _______________________________________________
> >>> linux-arm-kernel mailing list
> >>> [email protected]
> >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>>
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> >