* [PATCH] i2c: at91: handle TXRDY interrupt spam @ 2019-04-23 7:36 Raag Jadav 2019-04-29 9:00 ` Ludovic Desroches 0 siblings, 1 reply; 9+ messages in thread From: Raag Jadav @ 2019-04-23 7:36 UTC (permalink / raw) To: linux-i2c Cc: ludovic.desroches, nicolas.ferre, alexandre.belloni, linux-arm-kernel, linux-kernel, Raag Jadav 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 <raagjadav@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: at91: handle TXRDY interrupt spam 2019-04-23 7:36 [PATCH] i2c: at91: handle TXRDY interrupt spam Raag Jadav @ 2019-04-29 9:00 ` Ludovic Desroches 2019-04-29 22:33 ` Raag Jadav 0 siblings, 1 reply; 9+ messages in thread From: Ludovic Desroches @ 2019-04-29 9:00 UTC (permalink / raw) To: Raag Jadav Cc: linux-i2c, nicolas.ferre, alexandre.belloni, linux-arm-kernel, linux-kernel 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 <raagjadav@gmail.com> > --- > 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: at91: handle TXRDY interrupt spam 2019-04-29 9:00 ` Ludovic Desroches @ 2019-04-29 22:33 ` Raag Jadav 2019-05-02 14:01 ` Ludovic Desroches 0 siblings, 1 reply; 9+ messages in thread From: Raag Jadav @ 2019-04-29 22:33 UTC (permalink / raw) To: Ludovic Desroches Cc: linux-i2c, nicolas.ferre, alexandre.belloni, linux-arm-kernel, linux-kernel 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 <raagjadav@gmail.com> > > --- > > 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 > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: at91: handle TXRDY interrupt spam 2019-04-29 22:33 ` Raag Jadav @ 2019-05-02 14:01 ` Ludovic Desroches 2019-05-03 23:58 ` Raag Jadav 0 siblings, 1 reply; 9+ messages in thread From: Ludovic Desroches @ 2019-05-02 14:01 UTC (permalink / raw) To: Raag Jadav; +Cc: linux-kernel, alexandre.belloni, linux-i2c, linux-arm-kernel 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 <raagjadav@gmail.com> > > > --- > > > 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 > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: at91: handle TXRDY interrupt spam 2019-05-02 14:01 ` Ludovic Desroches @ 2019-05-03 23:58 ` Raag Jadav 2019-05-06 8:19 ` Eugen.Hristev 2019-05-14 8:42 ` Ludovic Desroches 0 siblings, 2 replies; 9+ messages in thread From: Raag Jadav @ 2019-05-03 23:58 UTC (permalink / raw) To: Ludovic Desroches Cc: linux-kernel, alexandre.belloni, linux-i2c, 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 <raagjadav@gmail.com> > > > > --- > > > > 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 > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: at91: handle TXRDY interrupt spam 2019-05-03 23:58 ` Raag Jadav @ 2019-05-06 8:19 ` Eugen.Hristev 2019-05-06 16:02 ` Raag Jadav 2019-05-14 8:52 ` Ludovic Desroches 2019-05-14 8:42 ` Ludovic Desroches 1 sibling, 2 replies; 9+ messages in thread From: Eugen.Hristev @ 2019-05-06 8:19 UTC (permalink / raw) To: raagjadav, Ludovic.Desroches Cc: alexandre.belloni, linux-kernel, linux-arm-kernel, linux-i2c 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 <david.engraf@sysgo.com> 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 <raagjadav@gmail.com> >>>>> --- >>>>> 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 >>> linux-arm-kernel@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>> > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: at91: handle TXRDY interrupt spam 2019-05-06 8:19 ` Eugen.Hristev @ 2019-05-06 16:02 ` Raag Jadav 2019-05-14 8:52 ` Ludovic Desroches 1 sibling, 0 replies; 9+ messages in thread From: Raag Jadav @ 2019-05-06 16:02 UTC (permalink / raw) To: Eugen.Hristev, ludovic.desroches Cc: alexandre.belloni, linux-kernel, linux-arm-kernel, linux-i2c On Mon, May 06, 2019 at 08:19:01AM +0000, Eugen.Hristev@microchip.com 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 <david.engraf@sysgo.com> > 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 <raagjadav@gmail.com> > >>>>> --- > >>>>> 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 > >>> linux-arm-kernel@lists.infradead.org > >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > >>> > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: at91: handle TXRDY interrupt spam 2019-05-06 8:19 ` Eugen.Hristev 2019-05-06 16:02 ` Raag Jadav @ 2019-05-14 8:52 ` Ludovic Desroches 1 sibling, 0 replies; 9+ messages in thread From: Ludovic Desroches @ 2019-05-14 8:52 UTC (permalink / raw) To: Eugen Hristev - M18282 Cc: Raag Jadav, alexandre.belloni, linux-kernel, linux-arm-kernel, linux-i2c 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 <david.engraf@sysgo.com> > 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 <raagjadav@gmail.com> > >>>>> --- > >>>>> 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 > >>> linux-arm-kernel@lists.infradead.org > >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > >>> > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: at91: handle TXRDY interrupt spam 2019-05-03 23:58 ` Raag Jadav 2019-05-06 8:19 ` Eugen.Hristev @ 2019-05-14 8:42 ` Ludovic Desroches 1 sibling, 0 replies; 9+ messages in thread From: Ludovic Desroches @ 2019-05-14 8:42 UTC (permalink / raw) To: Raag Jadav; +Cc: linux-kernel, alexandre.belloni, linux-i2c, 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 <raagjadav@gmail.com> > > > > > --- > > > > > 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 > > > linux-arm-kernel@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-05-14 8:52 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-23 7:36 [PATCH] i2c: at91: handle TXRDY interrupt spam Raag Jadav 2019-04-29 9:00 ` Ludovic Desroches 2019-04-29 22:33 ` Raag Jadav 2019-05-02 14:01 ` Ludovic Desroches 2019-05-03 23:58 ` Raag Jadav 2019-05-06 8:19 ` Eugen.Hristev 2019-05-06 16:02 ` Raag Jadav 2019-05-14 8:52 ` Ludovic Desroches 2019-05-14 8:42 ` Ludovic Desroches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).