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

From: Troy Mitchell <troy.mitchell@linux.spacemit.com>
To: Yixun Lan <dlan@gentoo.org>,
	Troy Mitchell <troy.mitchell@linux.spacemit.com>
Cc: Andi Shyti <andi.shyti@kernel.org>,
	Alex Elder <elder@riscstar.com>,
	Troy Mitchell <troymitchell988@gmail.com>,
	linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-riscv@lists.infradead.org, spacemit@lists.linux.dev
Subject: Re: [PATCH v2 6/6] i2c: spacemit: introduce pio for k1
Date: Sun, 28 Sep 2025 16:09:44 +0800	[thread overview]
Message-ID: <11BACC490411452F+aNjtSOiG31hX_hrl@kernel.org> (raw)
In-Reply-To: <20250928025400-GYB1330052@gentoo.org>

On Sun, Sep 28, 2025 at 10:54:00AM +0800, Yixun Lan wrote:
> 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@linux.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.

Okay..maybe I misunderstood before.
I'll remove disable/enable the controller IRQ around PIO transfers.

BTW, the K1 I2C patch [1] for ILCR as the SCL clock hasn't gotten
any replies in ages. Could you take a look?

Link:
https://lore.kernel.org/all/20250814-k1-i2c-ilcr-v3-1-317723e74bcd@linux.spacemit.com/ [1]
> 
> -- 
> Yixun Lan (dlan)
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv