Re: [PATCH 2/6] i2c: spacemit: remove stop function to avoid bus error

From: Aurelien Jarno <aurelien@aurel32.net>
To: Troy Mitchell <troy.mitchell@linux.spacemit.com>
Cc: Andi Shyti <andi.shyti@kernel.org>, Yixun Lan <dlan@gentoo.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 2/6] i2c: spacemit: remove stop function to avoid bus error
Date: Mon, 22 Sep 2025 22:55:57 +0200	[thread overview]
Message-ID: <aNG33WyGENMWI3Wl@aurel32.net> (raw)
In-Reply-To: <20250827-k1-i2c-atomic-v1-2-e59bea02d680@linux.spacemit.com>

On 2025-08-27 15:39, Troy Mitchell wrote:
> Previously, STOP handling was split into two separate steps:
>   1) clear TB/STOP/START/ACK bits
>   2) issue STOP by calling spacemit_i2c_stop()
> 
> This left a small window where the control register was updated
> twice, which can confuse the controller. While this race has not
> been observed with interrupt-driven transfers, it reliably causes
> bus errors in PIO mode.
> 
> Inline the STOP sequence into the IRQ handler and ensure that
> control register bits are updated atomically in a single writel().
> 
> Fixes: 5ea558473fa31 ("i2c: spacemit: add support for SpacemiT K1 SoC")
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
>  drivers/i2c/busses/i2c-k1.c | 28 +++++++++-------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index ee08811f4087c8e709d25dd314854ed643cc5a47..d342752030d077953adf84a2886211de96e843c4 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -267,19 +267,6 @@ static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
>  	writel(val, i2c->base + SPACEMIT_ICR);
>  }
>  
> -static void spacemit_i2c_stop(struct spacemit_i2c_dev *i2c)
> -{
> -	u32 val;
> -
> -	val = readl(i2c->base + SPACEMIT_ICR);
> -	val |= SPACEMIT_CR_STOP | SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB;
> -
> -	if (i2c->read)
> -		val |= SPACEMIT_CR_ACKNAK;
> -
> -	writel(val, i2c->base + SPACEMIT_ICR);
> -}
> -
>  static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>  {
>  	unsigned long time_left;
> @@ -412,7 +399,6 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
>  
>  	val = readl(i2c->base + SPACEMIT_ICR);
>  	val &= ~(SPACEMIT_CR_TB | SPACEMIT_CR_ACKNAK | SPACEMIT_CR_STOP | SPACEMIT_CR_START);
> -	writel(val, i2c->base + SPACEMIT_ICR);
>  
>  	switch (i2c->state) {
>  	case SPACEMIT_STATE_START:
> @@ -429,14 +415,18 @@ static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
>  	}
>  
>  	if (i2c->state != SPACEMIT_STATE_IDLE) {
> +		val |= SPACEMIT_CR_TB;
> +		if (i2c->is_pio)
> +			val |= SPACEMIT_CR_ALDIE;
> +

This needs to be moved to the last patch introducing PIO support.

>  		if (spacemit_i2c_is_last_msg(i2c)) {
>  			/* trigger next byte with stop */
> -			spacemit_i2c_stop(i2c);
> -		} else {
> -			/* trigger next byte */
> -			val |= SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB;
> -			writel(val, i2c->base + SPACEMIT_ICR);
> +			val |= SPACEMIT_CR_STOP;
> +
> +			if (i2c->read)
> +				val |= SPACEMIT_CR_ACKNAK;
>  		}
> +		writel(val, i2c->base + SPACEMIT_ICR);
>  	}
>  
>  err_out:

Otherwise sounds good.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                     http://aurel32.net