linux-kernel - Re: [PATCH v2 6/6] i2c: spacemit: introduce pio for k1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <F076AA5B04CF3445+aNiMlrTNTdI7H4PI@kernel.org>
Date: Sun, 28 Sep 2025 09:17:10 +0800
From: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
To: Yixun Lan <dlan@...too.org>,
	Troy Mitchell <troy.mitchell@...ux.spacemit.com>
Cc: Andi Shyti <andi.shyti@...nel.org>, Alex Elder <elder@...cstar.com>,
	Troy Mitchell <troymitchell988@...il.com>,
	linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-riscv@...ts.infradead.org, spacemit@...ts.linux.dev
Subject: Re: [PATCH v2 6/6] i2c: spacemit: introduce pio for k1

On Sat, Sep 27, 2025 at 06:56:16PM +0800, Yixun Lan wrote:
> On 10:02 Thu 25 Sep     , Troy Mitchell wrote:
> > This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> > enabling the use of I2C with interrupts disabled.
> > 
> > Signed-off-by: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
> > ---
> >  drivers/i2c/busses/i2c-k1.c | 164 +++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 140 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> > index 6b918770e612e098b8ad17418f420d87c94df166..e403eb7d6f329f4fe5c5242f94cc21094dff105c 100644
> > --- a/drivers/i2c/busses/i2c-k1.c
> > +++ b/drivers/i2c/busses/i2c-k1.c
> > @@ -97,6 +97,9 @@
> >  
> ..
> >  static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> >  {
> >  	unsigned long time_left;
> > @@ -310,10 +368,28 @@ static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> >  
> >  		reinit_completion(&i2c->complete);
> >  
> > -		spacemit_i2c_start(i2c);
> > +		if (i2c->is_pio) {
> > +			/* We disable the interrupt to avoid unintended spurious triggers. */
> the comment is suspicious, and seems wrong..
> > +			disable_irq(i2c->irq);
> > +
> I guess this doesn't disable interrupt in the hardware layer, it will still
> fire interrupt once enabled, so instead of calling disable_irq(), why not
> dealing with ISR setting of the controller? I mean those "IE bits"(interrupt
> enableing) of ICR REGISTER, disabling them should prevent the interrupt
> triggered?
For example, take MSD (master stop detect).
If we disable this interrupt, even the interrupt status bit will never be triggered.
Then how are we supposed to know when the transfer has completed?
That’s why we disable the global interrupt here, but still keep the pending bit.

> 
> > +			/*
> > +			 * The K1 I2C controller has a quirk:
> > +			 * when doing PIO transfers, the status register must be cleared
> > +			 * of all other bits before issuing a START.
> > +			 * Failing to do so will prevent the transfer from working properly.
> > +			 */
> > +			spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
> this also doesn't seem correct to me, the irq status bit should be cleared once
> interrupt occur,
> not dealing it here before sending msg, this seems a
> wrong procedure
I'll double check it, as I recall that if it’s not cleared here,
the second message will inevitably fail.

                - Troy
> 
> > +
> > +			spacemit_i2c_start(i2c);
> > +			time_left = spacemit_i2c_wait_pio_xfer(i2c);
> > +
> > +			enable_irq(i2c->irq);
> > +		} else {
> > +			spacemit_i2c_start(i2c);
> > +			time_left = wait_for_completion_timeout(&i2c->complete,
> > +								i2c->adapt.timeout);
> > +		}
> >  
> > -		time_left = wait_for_completion_timeout(&i2c->complete,
> > -							i2c->adapt.timeout);
> >  		if (!time_left) {
> >  			dev_err(i2c->dev, "msg completion timeout\n");
> >  			spacemit_i2c_conditionally_reset_bus(i2c);
> > @@ -341,6 +417,9 @@ static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
> 
> -- 
> Yixun Lan (dlan)
>