Hi Krzysztof,

-----Original Message-----
From: Krzysztof Adamski <redacted>
Sent: Wednesday, July 20, 2022 3:56 PM
To: Guntupalli, Manikanta <redacted>; Manikanta
Guntupalli [off-list ref]; michal.simek@xilinx.com;
Simek, Michal [off-list ref]; linux-arm-
kernel@lists.infradead.org; linux-i2c@vger.kernel.org; linux-
kernel@vger.kernel.org; git (AMD-Xilinx) [off-list ref]
Cc: Raviteja Narayanam <redacted>
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:

quoted

Hi Krzysztof,

quoted

[...]

quoted

@@ -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);

quoted

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

quoted

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.

quoted

quoted

quoted

+     /* 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);

quoted

quoted

quoted

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

quoted

quoted

quoted

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

quoted

quoted

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.

quoted

quoted

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.

quoted

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?

quoted

quoted

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,

quoted

    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.

quoted

quoted

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.

quoted

quoted

quoted

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

quoted

Again, only if msg->len is not 0, which is not ensured.

Agreed, we will fix in V2.

Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel