RE: [PATCH 3/3] i2c: cadence: Add atomic transfer support for controller version 1.4

[Guntupalli, Manikanta]

Hi Andi,
Thank you for taking the time to review our patch. We appreciate your detailed feedback and will address and fix all the review comments, except for the last one, which we believe may not be applicable.

Please find our inline replies to your comments below for further clarification.

> -----Original Message-----
> From: Andi Shyti <andi.shyti@xxxxxxxxxx>
> Sent: Tuesday, September 10, 2024 6:46 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@xxxxxxx>
> Cc: git (AMD-Xilinx) <git@xxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> i2c@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Simek, Michal
> <michal.simek@xxxxxxx>; Pandey, Radhey Shyam
> <radhey.shyam.pandey@xxxxxxx>; Goud, Srinivas <srinivas.goud@xxxxxxx>;
> Datta, Shubhrajyoti <shubhrajyoti.datta@xxxxxxx>; manion05gk@xxxxxxxxx
> Subject: Re: [PATCH 3/3] i2c: cadence: Add atomic transfer support for controller
> version 1.4
> 
> Hi Manikata,
> 
> Sorry for the delay in reviewing this patch. Looks good, just a few notes below.
> 
> ...
> 
> > +static bool cdns_i2c_error_check(struct cdns_i2c *id) {
> > +	unsigned int isr_status;
> > +
> > +	id->err_status = 0;
> > +
> > +	isr_status = cdns_i2c_readreg(CDNS_I2C_ISR_OFFSET);
> > +	cdns_i2c_writereg(isr_status & CDNS_I2C_IXR_ERR_INTR_MASK,
> > +CDNS_I2C_ISR_OFFSET);
> > +
> > +	id->err_status = isr_status & CDNS_I2C_IXR_ERR_INTR_MASK;
> > +	if (id->err_status)
> > +		return true;
> > +
> > +	return false;
> 
> return !!id->err_status;
We will fix.
> 
> > +}
> > +
> > +static void cdns_i2c_mrecv_atomic(struct cdns_i2c *id) {
> > +	bool updatetx;
> 
> Please move the udatex declaration inside the while loop.
We will fix.
> 
> > +	while (id->recv_count > 0) {
> > +		/*
> > +		 * Check if transfer size register needs to be updated again for a
> > +		 * large data receive operation.
> > +		 */
> > +		updatetx = id->recv_count > id->curr_recv_count;
> > +
> > +		while (id->curr_recv_count > 0) {
> > +			if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) &
> CDNS_I2C_SR_RXDV) {
> > +				*id->p_recv_buf++ =
> cdns_i2c_readreg(CDNS_I2C_DATA_OFFSET);
> 
> Can you please expand this operation to be a bit more clearer, without asking
> people to check on operation precedence?
We will fix.
> 
> > +				id->recv_count--;
> > +				id->curr_recv_count--;
> > +
> > +				/*
> > +				 * Clear hold bit that was set for FIFO control
> > +				 * if RX data left is less than or equal to
> > +				 * FIFO DEPTH unless repeated start is selected
> 
> mmhhh... the lack of punctuation makes this comment difficult to understand.
Sorry, we will update.
> 
> > +				 */
> > +				if (id->recv_count <= id->fifo_depth && !id-
> >bus_hold_flag)
> > +					cdns_i2c_clear_bus_hold(id);
> > +			}
> > +			if (cdns_i2c_error_check(id))
> > +				return;
> > +			if (cdns_is_holdquirk(id, updatetx))
> > +				break;
> > +		}
> > +
> > +		/*
> > +		 * The controller sends NACK to the slave when transfer size
> 
> /slave/target/
We will update.
> 
> > +		 * register reaches zero without considering the HOLD bit.
> > +		 * This workaround is implemented for large data transfers to
> > +		 * maintain transfer size non-zero while performing a large
> > +		 * receive operation.
> > +		 */
> > +		if (cdns_is_holdquirk(id, updatetx)) {
> > +			/* wait while fifo is full */
> > +			while (cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET) !=
> > +			       (id->curr_recv_count - id->fifo_depth))
> > +				;
> > +
> > +			/*
> > +			 * Check number of bytes to be received against maximum
> > +			 * transfer size and update register accordingly.
> > +			 */
> > +			if (((int)(id->recv_count) - id->fifo_depth) >
> 
> The cast is not needed here.
We will fix.
> 
> > +			    id->transfer_size) {
> > +				cdns_i2c_writereg(id->transfer_size,
> > +						  CDNS_I2C_XFER_SIZE_OFFSET);
> > +				id->curr_recv_count = id->transfer_size +
> > +						      id->fifo_depth;
> > +			} else {
> > +				cdns_i2c_writereg(id->recv_count -
> > +						  id->fifo_depth,
> > +						  CDNS_I2C_XFER_SIZE_OFFSET);
> > +				id->curr_recv_count = id->recv_count;
> > +			}
> > +		}
> > +	}
> > +
> > +	/* Clear hold (if not repeated start) */
> > +	if (!id->recv_count && !id->bus_hold_flag)
> > +		cdns_i2c_clear_bus_hold(id);
> > +}
> > +
> >  /**
> >   * cdns_i2c_mrecv - Prepare and start a master receive operation
> >   * @id:		pointer to the i2c device structure
> > @@ -715,7 +804,34 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
> >  		cdns_i2c_writereg(addr, CDNS_I2C_ADDR_OFFSET);
> >  	}
> >
> > -	cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK,
> CDNS_I2C_IER_OFFSET);
> > +	if (!id->atomic)
> > +		cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK,
> CDNS_I2C_IER_OFFSET);
> > +	else
> > +		cdns_i2c_mrecv_atomic(id);
> > +}
> > +
> > +static void cdns_i2c_msend_rem_atomic(struct cdns_i2c *id) {
> > +	unsigned int avail_bytes;
> > +	unsigned int bytes_to_send;
> 
> Please move these inside the while.
We will update.
> 
> > +
> > +	while (id->send_count) {
> > +		avail_bytes = id->fifo_depth -
> cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET);
> > +		if (id->send_count > avail_bytes)
> > +			bytes_to_send = avail_bytes;
> > +		else
> > +			bytes_to_send = id->send_count;
> > +
> > +		while (bytes_to_send--) {
> > +			cdns_i2c_writereg((*id->p_send_buf++),
> CDNS_I2C_DATA_OFFSET);
> > +			id->send_count--;
> > +		}
> > +		if (cdns_i2c_error_check(id))
> > +			return;
> > +	}
> > +
> > +	if (!id->send_count && !id->bus_hold_flag)
> > +		cdns_i2c_clear_bus_hold(id);
> >  }
> >
> >  /**
> > @@ -778,7 +894,12 @@ static void cdns_i2c_msend(struct cdns_i2c *id)
> >  	cdns_i2c_writereg(id->p_msg->addr & CDNS_I2C_ADDR_MASK,
> >  						CDNS_I2C_ADDR_OFFSET);
> >
> > -	cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK,
> CDNS_I2C_IER_OFFSET);
> > +	if (!id->atomic) {
> > +		cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK,
> CDNS_I2C_IER_OFFSET);
> > +	} else {
> > +		if (id->send_count > 0)
> 
> If you do:
> 
> 	} else if (id->send_count > 0) {
> 
> we save a level of indentation.
We will fix.
> 
> > +			cdns_i2c_msend_rem_atomic(id);
> > +	}
> >  }
> >
> >  /**
> > @@ -818,7 +939,8 @@ static int cdns_i2c_process_msg(struct cdns_i2c
> > *id, struct i2c_msg *msg,
> >
> >  	id->p_msg = msg;
> >  	id->err_status = 0;
> > -	reinit_completion(&id->xfer_done);
> > +	if (!id->atomic)
> > +		reinit_completion(&id->xfer_done);
> >
> >  	/* Check for the TEN Bit mode on each msg */
> >  	reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
> > @@ -841,13 +963,24 @@ static int cdns_i2c_process_msg(struct cdns_i2c *id,
> struct i2c_msg *msg,
> >  	/* Minimal time to execute this message */
> >  	msg_timeout = msecs_to_jiffies((1000 * msg->len * BITS_PER_BYTE) / id-
> >i2c_clk);
> >  	/* Plus some wiggle room */
> > -	msg_timeout += msecs_to_jiffies(500);
> > +	if (!id->atomic)
> > +		msg_timeout += msecs_to_jiffies(500);
> > +	else
> > +		msg_timeout += msecs_to_jiffies(2000);
> 
> You explained this in the commit log, can you add it in a comment, as well?
We will update.
> 
> >
> >  	if (msg_timeout < adap->timeout)
> >  		msg_timeout = adap->timeout;
> >
> > -	/* Wait for the signal of completion */
> > -	time_left = wait_for_completion_timeout(&id->xfer_done, msg_timeout);
> > +	if (!id->atomic) {
> > +		/* Wait for the signal of completion */
> > +		time_left = wait_for_completion_timeout(&id->xfer_done,
> msg_timeout);
> > +	} else {
> > +		/* 0 is success, -ETIMEDOUT is error */
> > +		time_left = !readl_poll_timeout_atomic(id->membase +
> CDNS_I2C_ISR_OFFSET,
> > +						       reg, (reg & CDNS_I2C_IXR_COMP),
> > +						       CDNS_I2C_POLL_US_ATOMIC,
> msg_timeout);
> > +	}
> 
> You can merge this if/else with the one above, to save some code.
Thank you for your suggestion to merge the if/else blocks to streamline the code. We have considered this approach; however, merging them would necessitate duplicating the following lines in both the if and else blocks:
     if (msg_timeout < adap->timeout)
                msg_timeout = adap->timeout;


Thanks,
Manikanta.