Re: [PATCH 01/12] i2c: xiic: Add standard mode support for > 255 byte read transfers

[Krzysztof Adamski]

Hi Manikanta,

W dniu 13.07.2022 o 09:54, Guntupalli, Manikanta pisze:
> Hi Krzysztof,
> > [...]
> > > @@ -579,31 +681,99 @@ static int xiic_busy(struct xiic_i2c *i2c)
> > >    static void xiic_start_recv(struct xiic_i2c *i2c)
> > >    {
> > >        u16 rx_watermark;
> > > +     u8 cr = 0, rfd_set = 0;
> > >        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);
> > > +     dev_dbg(i2c->adap.dev.parent, "%s entry, ISR: 0x%x, CR: 0x%x\n",
> > > +             __func__, xiic_getreg32(i2c, XIIC_IISR_OFFSET),
> > > +             xiic_getreg8(i2c, XIIC_CR_REG_OFFSET));
> > >
> > > -     /* we want to get all but last byte, because the TX_ERROR IRQ is used
> > > -      * to inidicate error ACK on the address, and negative ack on the last
> > > -      * received byte, so to not mix them receive all but last.
> > > -      * In the case where there is only one byte to receive
> > > -      * we can check if ERROR and RX full is set at the same time
> > > -      */
> > > -     rx_watermark = msg->len;
> > > -     if (rx_watermark > IIC_RX_FIFO_DEPTH)
> > > -             rx_watermark = IIC_RX_FIFO_DEPTH;
> > > -     xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, (u8)(rx_watermark - 1));
> > Do we really want to write 255 to RFD if msg->len == 0? That will set the
> > compare value in the RX_FIFO_PIRQ register to max value (15) but I don't
> > understand why we would like to do this.
> > Also, bits 31:4 are reserved so I think we should not try to touch them.
> Here comparing and taking minimum value of msg->len and  IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, while writing into register performed -1, so maximum value writing into RX_FIFO_PIRQ is 15.
I'm not sure you got my point - my point was that if the provided 
msg->len is 0, then rx_watermark will also be 0 and thus we will set the 
XIIC_RFD_REG_OFFSET to value 255 which seems illegal.
> > > +     /* Disable Tx interrupts */
> > > +     xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK |
> > > + XIIC_INTR_TX_EMPTY_MASK);
> > >
> > > -     if (!(msg->flags & I2C_M_NOSTART))
> > > -             /* write the address */
> > > -             xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
> > > -                     i2c_8bit_addr_from_msg(msg) | XIIC_TX_DYN_START_MASK);
> > >
> > > -     xiic_irq_clr_en(i2c, XIIC_INTR_BNB_MASK);
> > > +     if (i2c->dynamic) {
> > > +             u8 bytes;
> > > +             u16 val;
> > >
> > > -     xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
> > > -             msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0));
> > > +             /* Clear and enable Rx full interrupt. */
> > > +             xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK |
> > > +                             XIIC_INTR_TX_ERROR_MASK);
> > > +
> > > +             /*
> > > +              * We want to get all but last byte, because the TX_ERROR IRQ
> > > +              * is used to indicate error ACK on the address, and
> > > +              * negative ack on the last received byte, so to not mix
> > > +              * them receive all but last.
> > > +              * In the case where there is only one byte to receive
> > > +              * we can check if ERROR and RX full is set at the same time
> > > +              */
> > > +             rx_watermark = msg->len;
> > > +             bytes = min_t(u8, rx_watermark, IIC_RX_FIFO_DEPTH);
> > > +             bytes--;
> > Again, do we really want to write 255 to RFD if msg->len == 0?
> Here comparing and taking minimum value of msg->len and  IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, before writing into register performed decrement, so maximum value writing into RX_FIFO_PIRQ is 15.
You are not correct - in case the msg->len is 0, the value written into 
the register will be 255 (if you decrement 0, you will get 255 in case 
of u8). Unless there is some check making sure msg->len is never 0, that 
I do not see.
> > > +
> > > +             xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, bytes);
> > > +
> > > +             local_irq_save(flags);
> > > +             if (!(msg->flags & I2C_M_NOSTART))
> > > +                     /* write the address */
> > > +                     xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
> > > +                                   i2c_8bit_addr_from_msg(msg) |
> > > +                                   XIIC_TX_DYN_START_MASK);
> > When reviewing this patch, I tried to understand how the controller knows if
> > it should work in dynamic or in stanard mode.
> For receive operation with byte count greater than 255, we will switch to standard mode else always in the dynamic mode only.
>
> > My understanding is that in
> > order to start the dynamic mode logic, we have to set the DYN_START bit in
> > the TX FIFO when we write an address there. Is this correct?
> After deciding dynamic mode based on above logic, we need to  set the DYN_START bit along with address in the TX FIFO to start the transfer.
>
> > But we don't do
> > that if I2C_M_NOSTART flag is set so how is this supposed to work with this
> > flag? I mean, does the controller really supports doing I2C_M_NOSTART in
> > dynamic mode?
> I2C controller supports I2C_M_NOSTART in dynamic mode.
>
> > Or does it support it at all? After all, when we skip this, we will still write to
> > the TX_FIFO register 5 lines below. How is the controller supposed to know
> > that the len that we write there is *not* actually an address?
> Below notes mentioned in I2C_M_NOSTART section of kernel documentation(link mentioned below),
> " If you set the I2C_M_NOSTART variable for the first partial message,
>     we do not generate Addr, but we do generate the startbit S. This will
>     probably confuse all other clients on your bus, so don't try this."
>   
> https://www.kernel.org/doc/Documentation/i2c/i2c-protocol
>
> So I2C_M_NOSTART  flag need to use in second i2c_msg or in later i2c_msgs, but not in first i2c_msg. During first i2c_msg dynamic mode starts, so again no need to set start bit TX_FIFO.

But what happens if somebody passes the I2C_M_NOSTART bit in the first 
message? There seems to be no check for that. I know the documentation 
says that this should be avioded but it also states what should happen 
(S bit is generated by no address is sent) and as far as I understand 
this is not what will happen in i2c-xiic. What will happen is that the 
address *will* be generated and it will be the first byte from the buf.

> > That being said, we do not annouce the I2C_FUNC_NOSTART support so
> > maybe we should not care at all and just remove the code handling the
> > I2C_M_NOSTART flag?
> Since it supports I2C_M_NOSTART flag, need to keep code handling flag.
But if it supports it, that should be annouced in xiic_func.
> > > +
> > > +             xiic_irq_clr_en(i2c, XIIC_INTR_BNB_MASK);
> > > +
> > > +             /* If last message, include dynamic stop bit with length */
> > > +             val = (i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0;
> > > +             val |= msg->len;
> > > +
> > > +             xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, val);
> > > +             local_irq_restore(flags);
> > > +     } else {
> > > +             cr = xiic_getreg8(i2c, XIIC_CR_REG_OFFSET);
> > > +
> > > +             /* Set Receive fifo depth */
> > > +             rx_watermark = msg->len;
> > > +             if (rx_watermark > IIC_RX_FIFO_DEPTH) {
> > > +                     rfd_set = IIC_RX_FIFO_DEPTH - 1;
> > > +             } else if ((rx_watermark == 1) || (rx_watermark == 0)) {
> > > +                     rfd_set = rx_watermark - 1;
> > Again, do we really want to write 255 to RFD if msg->len == 0?
> Here comparing and taking minimum value of msg->len and  IIC_RX_FIFO_DEPTH. The value of IIC_RX_FIFO_DEPTH is 16, before writing into register performed -1, so maximum value writing into RX_FIFO_PIRQ is 15.
Again, only if msg->len is not 0, which is not ensured.

Krzysztof