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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250928025400-GYB1330052@gentoo.org>
Date: Sun, 28 Sep 2025 10:54:00 +0800
From: Yixun Lan <dlan@...too.org>
To: 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

Hi Troy,

On 09:17 Sun 28 Sep     , Troy Mitchell wrote:
> 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.
No, this is not something I understand..
> 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.
checking 18.1.4.1 ICR REGISTER, there is Master Stop Deteced Enable - MSDE (Bit[26]) and
Master Stop Deteced Interrupt Enable - MSDIE (BIT[25])
you can disable MSDIE but still enable MSDE, and check status of MSD (BIT[26]) of ISR REGISTER?
it should still work with interrupt disabled, otherwise I'd consider the hardware is broken.

-- 
Yixun Lan (dlan)