On 6/29/20 2:52 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?
>
> From the commit description of "i2c: xiic: Make the start and the byte count write atomic"(ae7304c3ea28a3ba47a7a8312c76c654ef24967e),
>
> [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.
Is that a hardware property of the XIIC IP, that the transaction is
NACKed after 7 clock cycles of the XIIC IP ?
> To fix this case make the 2 writes atomic.]
But using local_irq_save()/local_irq_restore() will not make two
separate register writes atomic, they will still be two separate
consecutive register writes.
Also note that another CPU core can very well be generating a lot of
traffic on the bus between current CPU core and the XIIC IP, so the
first write from current CPU core to the XIIC IP would arrive at the
XIIC IP, then the bus will be busy for a long time with other requiests
(more than 7 cycles), and then the second write to the XIIC IP will
arrive at the XIIC IP. The local_irq_save()/local_irq_restore() can not
prevent this scenario, but this is very real on a system under load.
> So, this local_irq_save()/local_irq_restore() is not related to exclusive access in the driver (xiic_process vs xiic_start),
> but to supply the byte count to controller before it completes transmitting START and slave address.
But then shouldn't the XIIC IP be fixed / extended with support for such
an atomic update instead ?
>> Thanks
>>
>>>> if (!(msg->flags & I2C_M_NOSTART))
>>>> /* write the address */
>>>> xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, @@ -569,7 +567,6
>>
>> [...]
