linux-kernel - Re: [PATCH v5 2/2] 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: <68BEF1A67DD3F4D0+aVHhhzvaM49Mm-0d@kernel.org>
Date: Mon, 29 Dec 2025 10:03:51 +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>,
	Aurelien Jarno <aurelien@...el32.net>,
	Michael Opdenacker <michael.opdenacker@...tcommit.com>,
	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 v5 2/2] i2c: spacemit: introduce pio for k1

On Sun, Dec 28, 2025 at 05:24:26PM -0600, Alex Elder wrote:
> On 12/25/25 9:31 PM, Troy Mitchell wrote:
> > This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> > enabling the use of I2C in atomic context.
> > 
> > When i2c xfer_atomic is invoked, use_pio is set accordingly.
> > 
> > Since an atomic context is required, all interrupts are disabled when
> > operating in PIO mode. Even with interrupts disabled, the bits in the
> > ISR (Interrupt Status Register) will still be set, so error handling can
> > be performed by polling the relevant status bits in the ISR.
> > 
> > Signed-off-by: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
> 
> This generally looks good and what I say below doesn't
> really ask for functional changes.
> 
> I have some suggestions on comments to improve readability
> of the code.  I still have a few questions related to delays
> and timeouts, and when you enable TX and RX interrupts.
> These are more about explaining/justifying what's going on,
> though in some cases they might imply an improvement that
> could be made.
> 
[...]
> > +		 *
> > +		 * 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.
> 
> I don't think the TX FIFO empty interrupt is "erroneous" in
NO FIFO NOW. Data Byte Register(DBR).
But the comments below still suitable.

> > +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;
> > +
> > +	do {
> > +		i2c->status = readl(i2c->base + SPACEMIT_ISR);
> > +
> > +		spacemit_i2c_clear_int_status(i2c, i2c->status);
> > +
> > +		if (!(i2c->status & mask)) {
> > +			udelay(10);
> 
> You are looking only for TX FIFO empty and RX FIFO full
> interrupts.  In this situation I *think* you have several
> possible interrupt conditions occurring.  Some questions:
> - Would observing one of the other possibly conditions
>   at this point be an error?
> - If so, is it OK to simply ignore (and acknowledge) these?
actualy, we can.
but I think it's better to check error here.
> - Why is the 10 microsecond delay required?
To ensure hardware stability, even in interrupt mode, the bit is set
first before the interrupt occurs.
> - Is it reasonable to delay if you see the RXHF condition?
The delay is only taken when none of the expected bits are observed.
> 
> > +			continue;
> > +		}
> > +
> > +		spacemit_i2c_handle_state(i2c);
> > +
> > +
> 
> Delete the extra blank lines here.
> 
> > +	} while (i2c->unprocessed && ktime_compare(ktime_get(), timeout) < 0);
> > +
> > +	if (i2c->unprocessed)
> > +		return 0;
> > +
> > +	if (i2c->read)
> > +		return 1;
> > +
> > +	/*
> > +	 * If this is the last byte to write of the current message,
> > +	 * we have to wait here. Otherwise, control will proceed directly
> > +	 * to start(), which would overwrite the current data.
> > +	 */
> > +	ret = readl_poll_timeout_atomic(i2c->base + SPACEMIT_ISR,
> > +					i2c->status, i2c->status & SPACEMIT_SR_ITE,
> > +					30, 1000);
> 
> Why is 1000 microseconds the correct timeout period here?
1000us is sufficient for the hardware to respond; if it still doesn't
work by then, it's considered a hardware timeout.
> 
> > +	if (ret)
> > +		return 0;
> > +
> > +	/*
> > +	 * For writes: in interrupt mode, an ITE (write-empty) interrupt is triggered
> > +	 * after the last byte, and the MSD-related handling takes place there.
> > +	 * In PIO mode, however, we need to explicitly call err_check() to emulate this
> > +	 * step, otherwise the next transfer will fail.
> > +	 */
> > +	if (i2c->msg_idx == i2c->msg_num - 1) {
> > +		mask = SPACEMIT_SR_MSD | SPACEMIT_SR_ERR;
> > +		/*
> > +		 * In some cases, MSD may not arrive immediately;
> > +		 * wait here to handle that.
> > +		 */
> > +		ret = readl_poll_timeout_atomic(i2c->base + SPACEMIT_ISR,
> > +						i2c->status, i2c->status & mask,
> > +						30, 1000);
> 
> Same question in this case.  Also, symbolic constants for
> timeouts are often better.
See above. Thanks. I'll define it.

                    - Troy