> -----Original Message-----
> From: Marek Vasut <marex@xxxxxxx>
> Sent: Monday, June 29, 2020 5:11 AM
> To: Raviteja Narayanam <rna@xxxxxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx
> Cc: Michal Simek <michals@xxxxxxxxxx>; Shubhrajyoti Datta
> <shubhraj@xxxxxxxxxx>; Wolfram Sang <wsa@xxxxxxxxxx>
> Subject: Re: [PATCH 2/5] i2c: xiic: Drop broken interrupt handler
>
> 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?
>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.
To fix this case make the 2 writes atomic.]
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.
>
> Thanks
>
> >> if (!(msg->flags & I2C_M_NOSTART))
> >> /* write the address */
> >> xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, @@ -569,7 +567,6
>
> [...]
