2018-09-03 09:43:26

by Shubhrajyoti Datta

[permalink] [raw]
Subject: [PATCH] i2c: xiic: Make the start and the byte count write atomic

From: Shubhrajyoti Datta <[email protected]>

Disable interrupts while configuring the transfer and enable them back.

We have below as the programming sequence
1. start and slave address
2. byte count and stop

In some customer platform there was a lot of interrupts between 1 and 2
and after slave address (around 7 clock cyles) if 2 is not executed
then the transaction is nacked.

To fix this case make the 2 writes atomic.

Signed-off-by: Shubhrajyoti Datta <[email protected]>
Signed-off-by: Michal Simek <[email protected]>
---
drivers/i2c/busses/i2c-xiic.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 9a71e50..4e3b2a4 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -532,6 +532,7 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
{
u8 rx_watermark;
struct i2c_msg *msg = i2c->rx_msg = i2c->tx_msg;
+ unsigned long flags;

/* Clear and enable Rx full interrupt. */
xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK | XIIC_INTR_TX_ERROR_MASK);
@@ -547,6 +548,7 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
rx_watermark = IIC_RX_FIFO_DEPTH;
xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rx_watermark - 1);

+ local_irq_save(flags);
if (!(msg->flags & I2C_M_NOSTART))
/* write the address */
xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
@@ -556,6 +558,7 @@ static void xiic_start_recv(struct xiic_i2c *i2c)

xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0));
+ local_irq_restore(flags);
if (i2c->nmsgs == 1)
/* very last, enable bus not busy as well */
xiic_irq_clr_en(i2c, XIIC_INTR_BNB_MASK);
--
2.1.1



2018-09-04 16:13:11

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATCH] i2c: xiic: Make the start and the byte count write atomic

On Mon, Sep 03, 2018 at 03:11:11PM +0530, [email protected] wrote:
> From: Shubhrajyoti Datta <[email protected]>
>
> Disable interrupts while configuring the transfer and enable them back.
>
> We have below as the programming sequence
> 1. start and slave address
> 2. byte count and stop
>
> In some customer platform there was a lot of interrupts between 1 and 2
> and after slave address (around 7 clock cyles) if 2 is not executed
> then the transaction is nacked.
>
> To fix this case make the 2 writes atomic.
>
> Signed-off-by: Shubhrajyoti Datta <[email protected]>
> Signed-off-by: Michal Simek <[email protected]>

I assume simply changing the order of the register writes won't fix it?

I also assume this is stable material?


Attachments:
(No filename) (807.00 B)
signature.asc (849.00 B)
Download all attachments

2018-09-06 09:37:20

by Shubhrajyoti Datta

[permalink] [raw]
Subject: Re: [PATCH] i2c: xiic: Make the start and the byte count write atomic

Hi,

On Tue, Sep 4, 2018 at 9:41 PM Wolfram Sang <[email protected]> wrote:
>
> On Mon, Sep 03, 2018 at 03:11:11PM +0530, [email protected] wrote:
> > From: Shubhrajyoti Datta <[email protected]>
> >
> > Disable interrupts while configuring the transfer and enable them back.
> >
> > We have below as the programming sequence
> > 1. start and slave address
> > 2. byte count and stop
> >
> > In some customer platform there was a lot of interrupts between 1 and 2
> > and after slave address (around 7 clock cyles) if 2 is not executed
> > then the transaction is nacked.
> >
> > To fix this case make the 2 writes atomic.
> >
> > Signed-off-by: Shubhrajyoti Datta <[email protected]>
> > Signed-off-by: Michal Simek <[email protected]>
>
> I assume simply changing the order of the register writes won't fix it?
No that is not possible.
>
> I also assume this is stable material?
>
Yes let me know if you want me to resend with the stable tag?

2018-09-06 21:15:27

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATCH] i2c: xiic: Make the start and the byte count write atomic

On Mon, Sep 03, 2018 at 03:11:11PM +0530, [email protected] wrote:
> From: Shubhrajyoti Datta <[email protected]>
>
> Disable interrupts while configuring the transfer and enable them back.
>
> We have below as the programming sequence
> 1. start and slave address
> 2. byte count and stop
>
> In some customer platform there was a lot of interrupts between 1 and 2
> and after slave address (around 7 clock cyles) if 2 is not executed
> then the transaction is nacked.
>
> To fix this case make the 2 writes atomic.
>
> Signed-off-by: Shubhrajyoti Datta <[email protected]>
> Signed-off-by: Michal Simek <[email protected]>

Added a newline for better readability, added the stable tag and applied
to for-current, thanks!

BTW we have no maintainer for this driver. Would you or Michal be
interested in doing that? I don't maintain the whole driver directory
anymore.


Attachments:
(No filename) (941.00 B)
signature.asc (849.00 B)
Download all attachments

2018-09-07 06:13:39

by Michal Simek

[permalink] [raw]
Subject: Re: [PATCH] i2c: xiic: Make the start and the byte count write atomic

On 6.9.2018 20:52, Wolfram Sang wrote:
> On Mon, Sep 03, 2018 at 03:11:11PM +0530, [email protected] wrote:
>> From: Shubhrajyoti Datta <[email protected]>
>>
>> Disable interrupts while configuring the transfer and enable them back.
>>
>> We have below as the programming sequence
>> 1. start and slave address
>> 2. byte count and stop
>>
>> In some customer platform there was a lot of interrupts between 1 and 2
>> and after slave address (around 7 clock cyles) if 2 is not executed
>> then the transaction is nacked.
>>
>> To fix this case make the 2 writes atomic.
>>
>> Signed-off-by: Shubhrajyoti Datta <[email protected]>
>> Signed-off-by: Michal Simek <[email protected]>
>
> Added a newline for better readability, added the stable tag and applied
> to for-current, thanks!
>
> BTW we have no maintainer for this driver. Would you or Michal be
> interested in doing that? I don't maintain the whole driver directory
> anymore.

Normally we do handle it via Zynq fragment to have single point of
contact. I have sent a patch for that. Please take it via i2c tree.

Thanks,
Michal