On 6/26/20 2:13 PM, Raviteja Narayanam wrote:
Hi,
[...]
>> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index
>> 0777e577720db..6db71c0fb6583 100644
>> --- a/drivers/i2c/busses/i2c-xiic.c
>> +++ b/drivers/i2c/busses/i2c-xiic.c
>> @@ -543,7 +543,6 @@ 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); @@ -559,7 +558,6 @@ 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);
>
> It is added as part of (i2c: xiic: Make the start and the byte count write atomic - ae7304c3ea28a3ba47a7a8312c76c654ef24967e) commit
> to make the below 2 register writes atomic so that the controller doesn't produce a wrong transaction.
Two consecutive register writes are not atomic, they are posted as two
consecutive writes on the bus and will reach the hardware IP as two
separate writes with arbitrary delay between them.
I think the intention of the patch ae7304c3ea28 ("i2c: xiic: Make the
start and the byte count write atomic") is to make sure that there will
be no read/write register access to the XIIC IP while those registers in
local_irq_save()/local_irq_restore() block are written, and that makes
sense.
However, local_irq_save()/local_irq_restore() is local to one CPU core,
it does not prevent another CPU core from posting register read/write
access to the XIIC IP (for example from xiic_process() threaded
interrupt handler, which might just be running on that other CPU core).
The &i2c->lock mutex is locked by both the xiic_start() and
xiic_process(), and therefore guarantees that no other thread can post
read/write register access to the XIIC IP while xiic_start() (and thus
xiic_recv_start(), which is called from it) is running.
And so, I think this local_irq_save()/local_irq_restore() can be removed.
> (If there is a delay between the 2 register
> writes, controller is messing up with the transaction). It is not intended for locking between this function and isr. So, this can't be removed.
But the bus can insert arbitrary delay between two consecutive register
writes to the XIIC IP, so if the timing between the two register writes
is really critical, then I don't see how to guarantee the timing
requirement. Do you happen to have more details on what really happens
on the hardware level here , maybe some errata document for the XIIC IP?
Thanks
>> if (!(msg->flags & I2C_M_NOSTART))
>> /* write the address */
>> xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, @@ -569,7 +567,6
[...]
