Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Message-ID: <8822a211-678f-49e5-8e6b-50b46dfc61b3@os.amperecomputing.com>
Date:   Mon, 11 Dec 2023 11:06:45 +0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 2/2] i2c: aspeed: Acknowledge Tx done with and without
 ACK irq late
Content-Language: en-CA
To:     Andi Shyti <andi.shyti@kernel.org>
Cc:     Brendan Higgins <brendan.higgins@linux.dev>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Joel Stanley <joel@jms.id.au>,
        Andrew Jeffery <andrew@codeconstruct.com.au>,
        Wolfram Sang <wsa@kernel.org>,
        Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>,
        Guenter Roeck <linux@roeck-us.net>, 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 <chou.cosmo@gmail.com>,
        Open Source Submission <patches@amperecomputing.com>,
        Phong Vo <phong@os.amperecomputing.com>,
        "Thang Q . Nguyen" <thang@os.amperecomputing.com>
References: <20231208033142.1673232-1-quan@os.amperecomputing.com>
 <20231208033142.1673232-3-quan@os.amperecomputing.com>
 <20231209204455.jxize3muvx7hhpos@zenone.zhora.eu>
From:   Quan Nguyen <quan@os.amperecomputing.com>
In-Reply-To: <20231209204455.jxize3muvx7hhpos@zenone.zhora.eu>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UGtYbEIrVjE1MVZhQ0s0dHRTejhnck8rOVNhdWg4aGVNdzRVVDk2TjhKM2Z3?=
 =?utf-8?B?ZFIveW5UTUNORVpuQTQ4ZHNXMGJWK09GNERRUE41NTFSeHdLQS8wbzhSNUFG?=
 =?utf-8?B?U1dscUFDcmlNKzdqSTRPYjNLSkRXR1VKRFlqcXlwUWFBdi9qWmhtUGVEM3dN?=
 =?utf-8?B?NkJrUWZSME5Da0k2cXJzcXlWQlc2ZFdFT0NCR0tXN3lBTU1EVTJoNVpZeEN0?=
 =?utf-8?B?c2NDSEcyZGpTMnR1dzg5RU4rU1ArbTBUWUtkWCtwMUxDMENSd0J0dXRnbWlJ?=
 =?utf-8?B?c01IOGt3NTRoZkN5RHQ1aGQ4dGlFWkgrbjZxY3NYZTdJTjhLZTJ2SFFGOWVG?=
 =?utf-8?B?bHN4ZDRTV2gxeHJqTm1YREdsZG5xMDdOS2thallZeUFUWGU0N0tDbVJrSXYy?=
 =?utf-8?B?VWZIRm1rbmJpZFJwRHBhUW9KaEl1bVFsN1F1a1A5UkRDblVLQ3h1TDRYeTNo?=
 =?utf-8?B?eHR3a2VGdmhjem9GUW11KzRNeW9ZYTdhT0t0WDZVajhMU0s5K1RPQjA2Mzcv?=
 =?utf-8?B?OGZKTkIwNkxCME96VVZWZU1UWjhVR1JqM3VQS0I5MHVFODRKRm9ZWkIzb0Jn?=
 =?utf-8?B?YmtSb2ZzREdlbjdMeDVoRlBseGQxY1lJTTRKbFBUbDFTWlNyMkVHTlhiWFhI?=
 =?utf-8?B?Tlk5WjdyRUVSME1SNzZ0K09BZFZxU21LOENoTHRSTHBnZEg2WnVUaFZDRmVp?=
 =?utf-8?B?TFBCSUlSeWtHU1FIUnNwaEQxL2RFeW12ZWdHbTMyUE02Q2QxdjZuWXF2TlQ3?=
 =?utf-8?B?TXZuaGE2MzZSbkN3YzA2UEpoQURlZ2FxeEFaZk9adlFyNDY4dWNlS1dnY2VR?=
 =?utf-8?B?QUFWWlFheTRQbmpGcXhpN2JGVzF1b1BDRUJGNHpmRmZkOTFNbEN3QWdJQTBo?=
 =?utf-8?B?anhmSHhZVVNlc01YNWYvcy9sZTRFMTNwVENYQWthVXBFSVJyd2czTzNtTDd2?=
 =?utf-8?B?ZEdMTDQ0SkJCMEk3Z1VhaHl6MHdqOVFpdElBeHpoMm1IL2huRHozQjdkL2lu?=
 =?utf-8?B?Ny9uUW55aUU0S3pxM09qMFZoSytJMkQzd2lyampZajJkSGR1M1U5K3Btc2x4?=
 =?utf-8?B?SmsxWnRHTjRwd24zN04zQ0l0NnVXNCtpKzZIUWFBTHg5NG1SejVEc0VteTRx?=
 =?utf-8?B?dzhVZGlwS29vWUJ4S25CS0VvU2c1R05rV2wrN1BJQlNxUVp2K0JCQ1c0YXpp?=
 =?utf-8?B?ZmZ6NXhnWm9lYVR4VGpRLzM3ZGNQSVh1SWx1dmdnbU5OVnlDaGhsUHdvYjh4?=
 =?utf-8?B?Z2NieUdGaUxLVFdlaUU5d3JyWWlWa3VsaVRZQmtaNTdrRkNOV1lFWHJCWXpZ?=
 =?utf-8?B?bUg1UmdEenNlSDlHck9SUDVPWXBsL1lNUURsYXoyMG01aStFVTB1L0Z6c09q?=
 =?utf-8?B?ekZKemNNZDlITFFaUG0zcVhyOCs5a2ZmTW9xaFhCMjNCc1pTczFsTk9Fblo3?=
 =?utf-8?B?eDI2UHVRY3lUZ29WSGlYN1ltanNGSlJlUVhtdXNHaFI3TkY2bkVwcWJaamxp?=
 =?utf-8?B?RGJUUjNDYXJ3ZFJYWFp5S1dlK3d4TVQ5bjlDMzFlT3BOeEJyd0NkZEplQnVI?=
 =?utf-8?B?cXJJOURrYXF3b0hQUHBnUVdUR1ZYRXI5cks2d1Bpb1NhUDJXakVIVjMvNFd4?=
 =?utf-8?B?eUVEbWdod3VLUXo5TGRzN0lJUGo0SkFFa21BSy83S25GM1BZM3hVd0VTVGc4?=
 =?utf-8?B?cXp3WnB1WDViNEN1dE9lV1IvMWM2RzVJcnJjRjF5WU9YQTM3ODU2WGl2THZW?=
 =?utf-8?B?clB1L2dUSXFoWEdya1FPUVVZaW9xcXhubmZTK2NnUzdsTUphQitkbFpFM0tB?=
 =?utf-8?B?aDNseVlPa0FCaTRHVFJwbzJsN1pnNGgzTFFmdmdtb3RSbWs5SzUvNG95ZXI3?=
 =?utf-8?B?WGxraGJ6N0F0NFY3OE0rS1pYSkFyNEdGZU5mV2hGRnFPcUZneUV6U0NCb0ls?=
 =?utf-8?B?MFYxMXduVnlyT2xSN0VoaENnc3hPZ01ESTdiczAwaTliSnVsejBDRGRKYU1x?=
 =?utf-8?B?MUJwSW1pL3EvTWQ3elBYTUg1R3hWd29HSVA4bWcwVUo4UnlmY2VTdUZlYmJL?=
 =?utf-8?B?MTV1R2ZHeFFIaHRSVnUxeVR3Z25NdlYwcHJwTzAzd2NrL0Y1QnRsMGVieXIz?=
 =?utf-8?B?YitKSVVzMFllR1V0b2MxZ21abWFXdUxKM0JXdzVlMkx3MUxxaDdVMHFqM3lJ?=
 =?utf-8?Q?KHvCfwAbU3a32V/5UsDqdD8=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 60100480-8bac-4dbf-ecea-08dbf9fe9aca
X-MS-Exchange-CrossTenant-AuthSource: SN4PR01MB7455.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Dec 2023 04:06:51.8837
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: nlGCZmGEW7+CN54hVieLI3Hr5t4zqOvk+qNR0K5FtG49ig1eXdNVlvWKzqH2nZvzU+fCL976A0epgfu/Ler1icknDY7XkNhAWFVDe5FCFzc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR01MB7821
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Sun, 10 Dec 2023 20:07:21 -0800 (PST)


On 10/12/2023 03:44, Andi Shyti wrote:
> Hi Quan,
> 
> [...]
> 
>> -	/* Ack all interrupts except for Rx done */
>> -	writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
>> -	       bus->base + ASPEED_I2C_INTR_STS_REG);
>> +
>> +	/*
>> +	 * Early acking of INTR_RX_DONE and INTR_TX_[ACK|NAK] would indicate HW to
>> +	 * start receiving or sending new data, and this may cause a race condition
>> +	 * as the irq handler has not yet handled these irqs but is being acked.
>> +	 * Let's ack them late at the end of the irq handler when those are truly processed.
>> +	 */
>> +	irq_ack_last = ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK;
>> +	writel(irq_received & ~irq_ack_last, bus->base + ASPEED_I2C_INTR_STS_REG);
> 
> I like Andrews suggestion of having irq_ack_last as a define that
> is already negated, instead of negating it in the writel, which
> makes it a bit difficult to read.
> 

Yes, but the it still need to negate again when do the write to late ack 
them later in the end of irq handler. So I'll keep the define as below 
in my v4:

+#define ASPEED_I2CD_INTR_ACK_RX_TX	    \
+		(ASPEED_I2CD_INTR_RX_DONE | \
+		 ASPEED_I2CD_INTR_TX_ACK |  \
+		 ASPEED_I2CD_INTR_TX_NAK)

The early ack will look like this:

+		writel(irq_received & ~ASPEED_I2CD_INTR_ACK_RX_TX,
+		       bus->base + ASPEED_I2C_INTR_STS_REG);
+		readl(bus->base + ASPEED_I2C_INTR_STS_REG);

And the late ack:

-	/* Ack Rx done */
-	if (irq_received & ASPEED_I2CD_INTR_RX_DONE) {
-		writel(ASPEED_I2CD_INTR_RX_DONE,
+	if (irq_received & ASPEED_I2CD_INTR_ACK_RX_TX) {
+		writel(irq_received & ASPEED_I2CD_INTR_ACK_RX_TX,
  		       bus->base + ASPEED_I2C_INTR_STS_REG);
  		readl(bus->base + ASPEED_I2C_INTR_STS_REG);
  	}

> Besides, ack_last, as a name is not very meaningful, I'd rather
> call it irq_ack_rx_tx (or something similar).
> 
> But I'm not going to block it for this, up to you if you want to
> send a new version.
> 
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> 

Thanks, Andi for the comments.

I will send out v4 to address those.

- Quan