Message-ID: <7413B6F44E526F60+aT-fu7gTS6YjEEKy@kernel.org>
Date: Mon, 15 Dec 2025 13:42:19 +0800
From: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
To: Alex Elder <elder@...cstar.com>,
	Troy Mitchell <troy.mitchell@...ux.spacemit.com>,
	Andi Shyti <andi.shyti@...nel.org>, Yixun Lan <dlan@...too.org>,
	Troy Mitchell <troymitchell988@...il.com>
Cc: linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-riscv@...ts.infradead.org, spacemit@...ts.linux.dev
Subject: Re: [PATCH v4] i2c: spacemit: introduce pio for k1

On Fri, Nov 07, 2025 at 09:50:26AM -0600, Alex Elder wrote:
> On 10/9/25 4:59 AM, Troy Mitchell wrote:
> > This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> > enabling the use of I2C in atomic context.
> 
> (Sorry I haven't commented on your earlier versions.  They
> included other changes to prepare for this; I'm looking at
> this patch in isolation and haven't reviewed the others.)
> 
> An aside:  I notice the #includes are indented an additional
> space in this source file;  perhaps you can get rid of those
> (in a separate patch) at some point.
Uh? Where?
I didn't do that in this patch.
> 
> You really need to provide more information about how this
> is implemented.  This patch makes non-trivial changes to
> the logic.
I'll provide more infomation in the next version.

[...]

> >   static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> >   {
> > -	u32 val;
> > -
> > -	/*
> > -	 * Unmask interrupt bits for all xfer mode:
> > -	 * bus error, arbitration loss detected.
> > -	 * For transaction complete signal, we use master stop
> > -	 * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE.
> > -	 */
> > -	val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
> > -
> > -	/*
> > -	 * Unmask interrupt bits for interrupt xfer mode:
> > -	 * When IDBR receives a byte, an interrupt is triggered.
> > -	 *
> > -	 * For the tx empty interrupt, it will be enabled in the
> > -	 * i2c_start function.
> > -	 * Otherwise, it will cause an erroneous empty interrupt before i2c_start.
> > -	 */
> > -	val |= SPACEMIT_CR_DRFIE;
> > +	u32 val = 0;
> > +
> > +	if (!i2c->use_pio) {
> 
> Why does this block of initialization only need to be done
> when not using PIO?  What happens in the PIO case?  I think
> the answer is that you don't want any interrupts when using
> PIO.  But... what if these conditions occur in PIO mode?
We always read ISR register in the pio_xfer().
Disabling the interrupt for a error bit only prevents the interrupt from
firing; it does not prevent the error bit itself from being set in the
ISR.
And we reused handle_state() so the error check is still there.

> 
> > +		/*
> > +		 * Unmask interrupt bits for all xfer mode:
> > +		 * bus error, arbitration loss detected.
> > +		 * For transaction complete signal, we use master stop
> > +		 * interrupt, so we don't need to unmask SPACEMIT_CR_TXDONEIE.
> > +		 */
> > +		val |= SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
> > +
> > +		/*
> > +		 * Unmask interrupt bits for interrupt xfer mode:
> > +		 * When IDBR receives a byte, an interrupt is triggered.
> > +		 *
> > +		 * For the tx empty interrupt, it will be enabled in the
> > +		 * i2c_start function.
> > +		 * Otherwise, it will cause an erroneous empty interrupt before i2c_start.
> > +		 */
> > +		val |= SPACEMIT_CR_DRFIE;
> 
> It's worth explaining (somewhere, possibly in the patch
> header) that this is different from what was done before.
> As a reviewer I didn't realize this at first; I thought
> you were simply making a block of code conditional on
> the value of use_pio.
See above. And I'll put them into commit message.

> 
> > +		/* unmask master stop interrupt bit */
> > +		val |= SPACEMIT_CR_MSDIE;
> > +	}
> 
> I would find it easier to follow if you set up the
> common bit assignments to the SPACEMIT_ICR register
> first, and then do the non-PIO specific ones afterward.
I don't think so. I think it should be configured on demand; otherwise
we will need to do the same thing again when FIFO support or high-speed
transfer modes are added in the future.
Moreover, in the PIO path we would still have to disable the FIFO and
high-speed bits, which makes the approach counterproductive.

[...]

> > +static int spacemit_i2c_wait_pio_xfer(struct spacemit_i2c_dev *i2c)
> > +{
> > +	u32 mask, msec = jiffies_to_msecs(i2c->adapt.timeout);
> > +	ktime_t timeout = ktime_add_ms(ktime_get(), msec);
> > +	int ret;
> > +
> > +	mask = SPACEMIT_SR_IRF | SPACEMIT_SR_ITE;
> 
> I think this can be a do..while block instead.  Both conditions
> will certainly be true initially.
> 
> > +	while (i2c->unprocessed && ktime_compare(ktime_get(), timeout) < 0) {
> > +		udelay(10);
> 
> Why do you delay *first*?  I think you should read the status
> and only if you're not done, insert the delay.
> 
> > +		i2c->status = readl(i2c->base + SPACEMIT_ISR);
> > +
> > +		spacemit_i2c_clear_int_status(i2c, i2c->status);
> > +
> > +		if (!(i2c->status & mask))
> > +			continue;
> > +
> > +		spacemit_i2c_handle_state(i2c);
> > +
> > +		/*
> > +		 * This is the last byte to write of the current message.
> 
> You mean *if* this is the last byte to write in the message, ...
Yes I lost *if*.

[...]
> 
> > @@ -445,7 +547,10 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> >   	}
> >   	if (i2c->state != SPACEMIT_STATE_IDLE) {
> > -		val |= SPACEMIT_CR_TB | SPACEMIT_CR_ALDIE;
> > +		val |= SPACEMIT_CR_TB;
> > +		if (i2c->use_pio)
> > +			val |= SPACEMIT_CR_ALDIE;
> > +
> >   		if (spacemit_i2c_is_last_msg(i2c)) {
> >   			/* trigger next byte with stop */
> > @@ -459,6 +564,23 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> >   err_out:
> >   	spacemit_i2c_err_check(i2c);
> > +}
> > +
> > +static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> 
> Introducing spacemit_i2c_handle_state() as a simple separate
> initial patch would simplify review.  (Not a big deal, but
> something to think about for future patch series.)
I will. Thanks.
> 
> > +{
> > +	struct spacemit_i2c_dev *i2c = devid;
> > +	u32 status;
> > +
> > +	status = readl(i2c->base + SPACEMIT_ISR);
> > +	if (!status)
> > +		return IRQ_HANDLED;
> > +
> > +	i2c->status = status;
> > +
> > +	spacemit_i2c_clear_int_status(i2c, status);
> > +
> > +	spacemit_i2c_handle_state(i2c);
> > +
> >   	return IRQ_HANDLED;
> >   }
> > @@ -467,6 +589,11 @@ static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
> >   	unsigned long timeout;
> >   	int idx = 0, cnt = 0;
> 
> Why is the timeout a fixed constant for PIO, but a fairly
> precise calculation based on message length otherwise?
We have talked about here [1]

                - Troy

Link: https://lore.kernel.org/all/2ADE8BFFC8EF40BC+aNH0fplBawrkLp3Z@LT-Guozexi/ [1]
> 
> 					-Alex
> 
> > +	if (i2c->use_pio) {
> > +		i2c->adapt.timeout = msecs_to_jiffies(SPACEMIT_WAIT_TIMEOUT);
> > +		return;
> > +	}
> > +
> >   	for (; idx < i2c->msg_num; idx++)
> >   		cnt += (i2c->msgs + idx)->len + 1;
> > @@ -479,11 +606,14 @@ static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
> >   	i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 10) / i2c->msg_num;
> >   }
> > -static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> > +static inline int
> > +spacemit_i2c_xfer_common(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num, bool use_pio)
> >   {
> >   	struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
> >   	int ret;
> > +	i2c->use_pio = use_pio;
> > +
> >   	i2c->msgs = msgs;
> >   	i2c->msg_num = num;
> > @@ -511,6 +641,16 @@ static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, in
> >   	return ret < 0 ? ret : num;
> >   }
> > +static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> > +{
> > +	return spacemit_i2c_xfer_common(adapt, msgs, num, false);
> > +}
> > +
> > +static int spacemit_i2c_pio_xfer_atomic(struct i2c_adapter *adapt, struct i2c_msg *msgs, int num)
> > +{
> > +	return spacemit_i2c_xfer_common(adapt, msgs, num, true);
> > +}
> > +
> >   static u32 spacemit_i2c_func(struct i2c_adapter *adap)
> >   {
> >   	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> > @@ -518,6 +658,7 @@ static u32 spacemit_i2c_func(struct i2c_adapter *adap)
> >   static const struct i2c_algorithm spacemit_i2c_algo = {
> >   	.xfer = spacemit_i2c_xfer,
> > +	.xfer_atomic = spacemit_i2c_pio_xfer_atomic,
> >   	.functionality = spacemit_i2c_func,
> >   };
> > 
> > ---
> > base-commit: bc574b64121525b24d52e9bab747184181c808dc
> > change-id: 20250814-k1-i2c-atomic-f1a90cd34364
> > 
> > Best regards,
> 
> 

Powered by blists - more mailing lists