Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Mon, 27 Nov 2023 23:25:22 +0100
From:   Andi Shyti <andi.shyti@kernel.org>
To:     Cosmo Chou <chou.cosmo@gmail.com>
Cc:     brendan.higgins@linux.dev, benh@kernel.crashing.org,
        joel@jms.id.au, andrew@codeconstruct.com.au, linux@roeck-us.net,
        wsa@kernel.org, jae.hyun.yoo@linux.intel.com,
        linux-i2c@vger.kernel.org, openbmc@lists.ozlabs.org,
        linux-arm-kernel@lists.infradead.org,
        linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        cosmo.chou@quantatw.com
Subject: Re: [PATCH] i2c: aspeed: Acknowledge Tx ack late when in
 SLAVE_READ_PROCESSED
Message-ID: <20231127222522.wjdd6btbug6shd7y@zenone.zhora.eu>
References: <20231120091746.2866232-1-chou.cosmo@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231120091746.2866232-1-chou.cosmo@gmail.com>
Precedence: bulk

Hi Cosmo,

On Mon, Nov 20, 2023 at 05:17:46PM +0800, Cosmo Chou wrote:
> commit 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early
> in interrupt handler") moved most interrupt acknowledgments to the
> start of the interrupt handler to avoid race conditions. However,
> slave Tx ack status shouldn't be cleared before SLAVE_READ_PROCESSED
> is handled.
> 
> Acknowledge Tx ack status after handling SLAVE_READ_PROCESSED to fix
> the problem that the next byte is not sent correctly.
> 
> Fixes: 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler")
> Signed-off-by: Cosmo Chou <chou.cosmo@gmail.com>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 28e2a5fc4528..c2d74e4b7e50 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -337,6 +337,12 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>  		break;
>  	}
>  
> +	/* Ack Tx ack */
> +	if (irq_handled & ASPEED_I2CD_INTR_TX_ACK) {
> +		writel(ASPEED_I2CD_INTR_TX_ACK, bus->base + ASPEED_I2C_INTR_STS_REG);
> +		readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> +	}
> +
>  	return irq_handled;
>  }
>  #endif /* CONFIG_I2C_SLAVE */
> @@ -602,13 +608,18 @@ static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>  static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>  {
>  	struct aspeed_i2c_bus *bus = dev_id;
> -	u32 irq_received, irq_remaining, irq_handled;
> +	u32 irq_received, irq_remaining, irq_handled, irq_acked;
>  
>  	spin_lock(&bus->lock);
>  	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>  	/* Ack all interrupts except for Rx done */
> -	writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> -	       bus->base + ASPEED_I2C_INTR_STS_REG);
> +	irq_acked = irq_received & ~ASPEED_I2CD_INTR_RX_DONE;
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	/* shouldn't ack Slave Tx Ack before it's handled */
> +	if (bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED)
> +		irq_acked &= ~ASPEED_I2CD_INTR_TX_ACK;
> +#endif
> +	writel(irq_acked, bus->base + ASPEED_I2C_INTR_STS_REG);

which branch are you? You don't look like being in the latest.
Please update your branch.

Andi

>  	readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>  	irq_received &= ASPEED_I2CD_INTR_RECV_MASK;
>  	irq_remaining = irq_received;
> -- 
> 2.34.1
>