Hi Krzysztof,
> -----Original Message-----
> From: Krzysztof Adamski <krzysztof.adamski@xxxxxxxxx>
> Sent: Wednesday, June 29, 2022 5:49 PM
> To: 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
>
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
>
>
> W dniu 24.06.2022 o 14:05, Manikanta Guntupalli pisze:
> [...]
> > +static void xiic_std_fill_tx_fifo(struct xiic_i2c *i2c) {
> > + u8 fifo_space = xiic_tx_fifo_space(i2c);
> > + u16 data = 0;
> > + int len = xiic_tx_space(i2c);
> > +
> > + dev_dbg(i2c->adap.dev.parent, "%s entry, len: %d, fifo space: %d\n",
> > + __func__, len, fifo_space);
> > +
> > + if (len > fifo_space)
> > + len = fifo_space;
> > + else if (len && !(i2c->repeated_start))
> > + len--;
> > +
> > + while (len--) {
> > + data = i2c->tx_msg->buf[i2c->tx_pos++];
> > + xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data);
> > + }
> > +}
> This function looks very similar to the original xiic_fill_tx_fifo. The only
> difference is that it does not decrease the len in case of repeated_start (btw,
> why?), and it does not set the DYN_STOP bit. But this could be done
> conditionally based on i2c->dynamic, instead. No need for this duplication, in
> my opinion.
Agreed with your comments, we will fix in V2.
> [...]
> > @@ -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.
> > + /* 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.
> > +
> > + 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.
> 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.
> > +
> > + 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.
> [...]
>
> Krzysztof
Thanks,
Manikanta.
