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

[Guntupalli, Manikanta]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Adamski <krzysztof.adamski@xxxxxxxxx>
> Sent: Wednesday, July 20, 2022 3:56 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@xxxxxxx>; Manikanta
> Guntupalli <manikanta.guntupalli@xxxxxxxxxx>; michal.simek@xxxxxxxxxx;
> Simek, Michal <michal.simek@xxxxxxx>; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>
> Cc: Raviteja Narayanam <raviteja.narayanam@xxxxxxxxxx>
> Subject: Re: [PATCH 01/12] i2c: xiic: Add standard mode support for > 255
> byte read transfers
> 
> 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.

We will fix in V2.

> >>> +     /* 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.

Agreed, we will fix in V2.

> >
> >>> +
> >>> +             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.
> 

As per kernel documentation it's recommended to use I2C_M_NOSTART flag from second message onwards, but not mandatory. 
Dynamic mode won't start if someone passes I2C_M_NOSTART flag in first message.

> >
> >> 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.

Since we are not announcing I2C_FUNC_NOSTART, we will remove  I2C_M_NOSTART flag handling checks in V2.

> >
> >>> +
> >>> +             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.
> 

Agreed, we will fix in V2.

> Krzysztof