Re: [PATCH v4] i2c: spacemit: introduce pio for k1

Troy Mitchell
2025-12-15T05:44:13Z

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