| Message ID | 1483618013-10721-1-git-send-email-a.hajda@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Andrzej, On Thu, Jan 05, 2017 at 01:06:53PM +0100, Andrzej Hajda wrote: > In case of arbitration lost adequate interrupt sometimes is not signaled. As > a result transfer timeouts and is not retried, as it should. To avoid such > cases code is added to check transaction status in case of every interrupt. > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > --- > Hi, > > I am not I2C specialist, this patch is a result of debugging issues > with SiI8620 MHL driver on TM2 device. I guess the main problem is > with SiI8620 chip, but exynos5 i2c is also guilty - with i2c-gpio driver > it works correctly - there is only one arbitration lost at 1st transaction > (which is properly handled - transfer is repeated succesfully). In case > of exynos5 there are much more arbitration losts. Lowering bus speed > decreases frequency of losts but do not eliminate them. > Any help/hint, what could cause such issues? I'm having the same issue on TM2 and your patch fixes it. I'm not sure, though, that's the right fix. In any case I will look into it. Andi -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 05, 2017 at 01:06:53PM +0100, Andrzej Hajda wrote: > In case of arbitration lost adequate interrupt sometimes is not signaled. As > a result transfer timeouts and is not retried, as it should. To avoid such > cases code is added to check transaction status in case of every interrupt. > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> Looks good to me, but I'd like some review from one of the Samsung people? > --- > Hi, > > I am not I2C specialist, this patch is a result of debugging issues > with SiI8620 MHL driver on TM2 device. I guess the main problem is > with SiI8620 chip, but exynos5 i2c is also guilty - with i2c-gpio driver > it works correctly - there is only one arbitration lost at 1st transaction > (which is properly handled - transfer is repeated succesfully). In case > of exynos5 there are much more arbitration losts. Lowering bus speed > decreases frequency of losts but do not eliminate them. > Any help/hint, what could cause such issues? > > I guess increasing adap.retries should also decrease fail probability, > currently for exynos5 it is set to 3. > > Regards > Andrzej > --- > drivers/i2c/busses/i2c-exynos5.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c > index bea6071..a01c1ae 100644 > --- a/drivers/i2c/busses/i2c-exynos5.c > +++ b/drivers/i2c/busses/i2c-exynos5.c > @@ -130,12 +130,32 @@ > /* I2C_TRANS_STATUS register bits */ > #define HSI2C_MASTER_BUSY (1u << 17) > #define HSI2C_SLAVE_BUSY (1u << 16) > + > +/* I2C_TRANS_STATUS register bits for Exynos5 variant */ > #define HSI2C_TIMEOUT_AUTO (1u << 4) > #define HSI2C_NO_DEV (1u << 3) > #define HSI2C_NO_DEV_ACK (1u << 2) > #define HSI2C_TRANS_ABORT (1u << 1) > #define HSI2C_TRANS_DONE (1u << 0) > > +/* I2C_TRANS_STATUS register bits for Exynos7 variant */ > +#define HSI2C_MASTER_ST_MASK 0xf > +#define HSI2C_MASTER_ST_IDLE 0x0 > +#define HSI2C_MASTER_ST_START 0x1 > +#define HSI2C_MASTER_ST_RESTART 0x2 > +#define HSI2C_MASTER_ST_STOP 0x3 > +#define HSI2C_MASTER_ST_MASTER_ID 0x4 > +#define HSI2C_MASTER_ST_ADDR0 0x5 > +#define HSI2C_MASTER_ST_ADDR1 0x6 > +#define HSI2C_MASTER_ST_ADDR2 0x7 > +#define HSI2C_MASTER_ST_ADDR_SR 0x8 > +#define HSI2C_MASTER_ST_READ 0x9 > +#define HSI2C_MASTER_ST_WRITE 0xa > +#define HSI2C_MASTER_ST_NO_ACK 0xb > +#define HSI2C_MASTER_ST_LOSE 0xc > +#define HSI2C_MASTER_ST_WAIT 0xd > +#define HSI2C_MASTER_ST_WAIT_CMD 0xe > + > /* I2C_ADDR register bits */ > #define HSI2C_SLV_ADDR_SLV(x) ((x & 0x3ff) << 0) > #define HSI2C_SLV_ADDR_MAS(x) ((x & 0x3ff) << 10) > @@ -437,6 +457,7 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id) > > int_status = readl(i2c->regs + HSI2C_INT_STATUS); > writel(int_status, i2c->regs + HSI2C_INT_STATUS); > + trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS); > > /* handle interrupt related to the transfer status */ > if (i2c->variant->hw == HSI2C_EXYNOS7) { > @@ -460,8 +481,13 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id) > i2c->state = -ETIMEDOUT; > goto stop; > } > + > + switch (trans_status & HSI2C_MASTER_ST_MASK) { > + case HSI2C_MASTER_ST_LOSE: > + i2c->state = -EAGAIN; > + goto stop; > + } > } else if (int_status & HSI2C_INT_I2C) { > - trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS); > if (trans_status & HSI2C_NO_DEV_ACK) { > dev_dbg(i2c->dev, "No ACK from device\n"); > i2c->state = -ENXIO; > -- > 2.7.4 >
On Wed, Jan 25, 2017 at 09:55:54PM +0100, Wolfram Sang wrote: > On Thu, Jan 05, 2017 at 01:06:53PM +0100, Andrzej Hajda wrote: > > In case of arbitration lost adequate interrupt sometimes is not signaled. As > > a result transfer timeouts and is not retried, as it should. To avoid such > > cases code is added to check transaction status in case of every interrupt. > > > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > > Looks good to me, but I'd like some review from one of the Samsung > people? Ooops, you are from Samsung as well :) Still, I'd like one more ack or review from someone with the HW. Thanks, Wolfram
Hi, > > > In case of arbitration lost adequate interrupt sometimes is not signaled. As > > > a result transfer timeouts and is not retried, as it should. To avoid such > > > cases code is added to check transaction status in case of every interrupt. > > > > > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > > > > Looks good to me, but I'd like some review from one of the Samsung > > people? > > Ooops, you are from Samsung as well :) Still, I'd like one more ack or > review from someone with the HW. I am using this patch already: Reviewed-by: Andi Shyti <andi.shyti@samsung.com> Tested-by: Andi Shyti <andi.shyti@samsung.com> Shouldn't we cc for the stable Kernel? Andi -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 05, 2017 at 01:06:53PM +0100, Andrzej Hajda wrote: > In case of arbitration lost adequate interrupt sometimes is not signaled. As > a result transfer timeouts and is not retried, as it should. To avoid such > cases code is added to check transaction status in case of every interrupt. > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > --- > + > + switch (trans_status & HSI2C_MASTER_ST_MASK) { > + case HSI2C_MASTER_ST_LOSE: > + i2c->state = -EAGAIN; > + goto stop; > + } Why not using if() instead of switch() as in the rest of the driver? And there is arbitration lost checking already with int_status & HSI2C_INT_TRANS_ABORT. Any guess why it doesn't trigger?
On 09.02.2017 17:27, Wolfram Sang wrote: > On Thu, Jan 05, 2017 at 01:06:53PM +0100, Andrzej Hajda wrote: >> In case of arbitration lost adequate interrupt sometimes is not signaled. As >> a result transfer timeouts and is not retried, as it should. To avoid such >> cases code is added to check transaction status in case of every interrupt. >> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >> --- >> + >> + switch (trans_status & HSI2C_MASTER_ST_MASK) { >> + case HSI2C_MASTER_ST_LOSE: >> + i2c->state = -EAGAIN; >> + goto stop; >> + } > Why not using if() instead of switch() as in the rest of the driver? Following reasons (not serious ones): 1. With if() the line should be split anyway to keep 80 chars per line limit, and the result looks less readable, so no gain: if ((trans_status & HSI2C_MASTER_ST_MASK) == HSI2C_MASTER_ST_LOSE) { i2c->state = -EAGAIN; goto stop; } 2. It is ready to handle other status values as well, without repeating long if() clause, in fact during tests I have added more values, but finally they went out. 3. In the rest of the functions if() is used because code tests if some bits are set, here we test if some bit field have specific value, slightly different thing. Of course I can change to if() if you prefer it. > And there is arbitration lost checking already with int_status & > HSI2C_INT_TRANS_ABORT. Any guess why it doesn't trigger? > Nope. This patch is just a result of comparing register values during good and bad transfer. I have looked for similar issues over the net, but without ultimate answer, some hints are that master can check status of SDA line too early. Anyway it works correctly with gpio bit-banging driver, it suggests there could be something wrong in hsi2c logic. Regards Andrzej -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10.02.2017 08:39, Andrzej Hajda wrote: > On 09.02.2017 17:27, Wolfram Sang wrote: >> On Thu, Jan 05, 2017 at 01:06:53PM +0100, Andrzej Hajda wrote: >>> In case of arbitration lost adequate interrupt sometimes is not signaled. As >>> a result transfer timeouts and is not retried, as it should. To avoid such >>> cases code is added to check transaction status in case of every interrupt. >>> >>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>> --- >>> + >>> + switch (trans_status & HSI2C_MASTER_ST_MASK) { >>> + case HSI2C_MASTER_ST_LOSE: >>> + i2c->state = -EAGAIN; >>> + goto stop; >>> + } >> Why not using if() instead of switch() as in the rest of the driver? > Following reasons (not serious ones): > 1. With if() the line should be split anyway to keep 80 chars per line > limit, and the result looks less readable, so no gain: > if ((trans_status & HSI2C_MASTER_ST_MASK) == > HSI2C_MASTER_ST_LOSE) { > i2c->state = -EAGAIN; > goto stop; > } > 2. It is ready to handle other status values as well, without repeating > long if() clause, in fact during tests I have added more values, but > finally they went out. > 3. In the rest of the functions if() is used because code tests if some > bits are set, here we test if some bit field have specific value, > slightly different thing. > > Of course I can change to if() if you prefer it. > >> And there is arbitration lost checking already with int_status & >> HSI2C_INT_TRANS_ABORT. Any guess why it doesn't trigger? >> > Nope. This patch is just a result of comparing register values during > good and bad transfer. > I have looked for similar issues over the net, but without ultimate > answer, some hints are that master can check status of SDA line too early. > Anyway it works correctly with gpio bit-banging driver, it suggests > there could be something wrong in hsi2c logic. I have just found in spec of newer version of the chip configuration bit NO_ARB_FASTSDA. Citation: > This bit deactivates the modified logic of the > abnormal operation when the SCL is falling. Then, > other I2C device quickly lowers the SDA line, and > SDA has fallen before the first PCLK period > elapses. > 0 = Skip the master arbitration checking during the > first PCLK period after SCL falls > 1 = Restore original logic I have no hardware with newer chip to really test if this solves the issue, but I suppose it could, the only problem is that this bit is not present in current chip version, Exynos5433. Regards Andrzej > > Regards > Andrzej > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 10, 2017 at 08:39:58AM +0100, Andrzej Hajda wrote: > On 09.02.2017 17:27, Wolfram Sang wrote: > > On Thu, Jan 05, 2017 at 01:06:53PM +0100, Andrzej Hajda wrote: > >> In case of arbitration lost adequate interrupt sometimes is not signaled. As > >> a result transfer timeouts and is not retried, as it should. To avoid such > >> cases code is added to check transaction status in case of every interrupt. > >> > >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > >> --- > >> + > >> + switch (trans_status & HSI2C_MASTER_ST_MASK) { > >> + case HSI2C_MASTER_ST_LOSE: > >> + i2c->state = -EAGAIN; > >> + goto stop; > >> + } > > Why not using if() instead of switch() as in the rest of the driver? > > Following reasons (not serious ones): I see. It is probably a taste thing, but a switch() with just one case in it looks like something has been forgotten :) We can change it back to switch() if ever more states need to be added. Given that I am not super strict with the 80 char limit anyhow and it will miss the limit by a few chars only, I'd prefer the if statement actually. Other than this, I am OK with applying the patch for 4.11. Can you resend with the above change? Thanks, Wolfram
diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c index bea6071..a01c1ae 100644 --- a/drivers/i2c/busses/i2c-exynos5.c +++ b/drivers/i2c/busses/i2c-exynos5.c @@ -130,12 +130,32 @@ /* I2C_TRANS_STATUS register bits */ #define HSI2C_MASTER_BUSY (1u << 17) #define HSI2C_SLAVE_BUSY (1u << 16) + +/* I2C_TRANS_STATUS register bits for Exynos5 variant */ #define HSI2C_TIMEOUT_AUTO (1u << 4) #define HSI2C_NO_DEV (1u << 3) #define HSI2C_NO_DEV_ACK (1u << 2) #define HSI2C_TRANS_ABORT (1u << 1) #define HSI2C_TRANS_DONE (1u << 0) +/* I2C_TRANS_STATUS register bits for Exynos7 variant */ +#define HSI2C_MASTER_ST_MASK 0xf +#define HSI2C_MASTER_ST_IDLE 0x0 +#define HSI2C_MASTER_ST_START 0x1 +#define HSI2C_MASTER_ST_RESTART 0x2 +#define HSI2C_MASTER_ST_STOP 0x3 +#define HSI2C_MASTER_ST_MASTER_ID 0x4 +#define HSI2C_MASTER_ST_ADDR0 0x5 +#define HSI2C_MASTER_ST_ADDR1 0x6 +#define HSI2C_MASTER_ST_ADDR2 0x7 +#define HSI2C_MASTER_ST_ADDR_SR 0x8 +#define HSI2C_MASTER_ST_READ 0x9 +#define HSI2C_MASTER_ST_WRITE 0xa +#define HSI2C_MASTER_ST_NO_ACK 0xb +#define HSI2C_MASTER_ST_LOSE 0xc +#define HSI2C_MASTER_ST_WAIT 0xd +#define HSI2C_MASTER_ST_WAIT_CMD 0xe + /* I2C_ADDR register bits */ #define HSI2C_SLV_ADDR_SLV(x) ((x & 0x3ff) << 0) #define HSI2C_SLV_ADDR_MAS(x) ((x & 0x3ff) << 10) @@ -437,6 +457,7 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id) int_status = readl(i2c->regs + HSI2C_INT_STATUS); writel(int_status, i2c->regs + HSI2C_INT_STATUS); + trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS); /* handle interrupt related to the transfer status */ if (i2c->variant->hw == HSI2C_EXYNOS7) { @@ -460,8 +481,13 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id) i2c->state = -ETIMEDOUT; goto stop; } + + switch (trans_status & HSI2C_MASTER_ST_MASK) { + case HSI2C_MASTER_ST_LOSE: + i2c->state = -EAGAIN; + goto stop; + } } else if (int_status & HSI2C_INT_I2C) { - trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS); if (trans_status & HSI2C_NO_DEV_ACK) { dev_dbg(i2c->dev, "No ACK from device\n"); i2c->state = -ENXIO;
In case of arbitration lost adequate interrupt sometimes is not signaled. As a result transfer timeouts and is not retried, as it should. To avoid such cases code is added to check transaction status in case of every interrupt. Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> --- Hi, I am not I2C specialist, this patch is a result of debugging issues with SiI8620 MHL driver on TM2 device. I guess the main problem is with SiI8620 chip, but exynos5 i2c is also guilty - with i2c-gpio driver it works correctly - there is only one arbitration lost at 1st transaction (which is properly handled - transfer is repeated succesfully). In case of exynos5 there are much more arbitration losts. Lowering bus speed decreases frequency of losts but do not eliminate them. Any help/hint, what could cause such issues? I guess increasing adap.retries should also decrease fail probability, currently for exynos5 it is set to 3. Regards Andrzej --- drivers/i2c/busses/i2c-exynos5.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)