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

[Krzysztof Adamski <krzysztof.adamski@nokia.com>]

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.
[...]
> @@ -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.

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

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

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?

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?
> +
> +		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?
[...]

Krzysztof