Hi Marek,
> -----Original Message-----
> From: Marek Vasut <marex@xxxxxxx>
> Sent: Tuesday, June 30, 2020 3:20 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/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 ?
Yes, it is from 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.
Yeah, such possibility is there. local_irq_save()/local_irq_restore() can't be the right solution
as you said.
>
> > 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 ?
I have started communicating with the hardware team on why the IP behavior is like this.
But, that will take more time to get to a conclusion. We can continue this discussion here.
So, how about we remove this patch from the current series and go ahead with the rest of patches?
In that case please send a v2 with the minimal changes suggested in other patches (4 and 5)?
Thanks
Raviteja N
>
> >> Thanks
> >>
> >>>> if (!(msg->flags & I2C_M_NOSTART))
> >>>> /* write the address */
> >>>> xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, @@ -569,7 +567,6
> >>
> >> [...]
