Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <79d8377f-b443-47cd-831c-4f27a4f59572@os.amperecomputing.com>
Date:   Mon, 11 Dec 2023 11:06:12 +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:     Andrew Jeffery <andrew@codeconstruct.com.au>,
        Brendan Higgins <brendan.higgins@linux.dev>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Joel Stanley <joel@jms.id.au>,
        Andi Shyti <andi.shyti@kernel.org>,
        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
Cc:     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>
 <dc36d61aedd2b2389eb366b27436f19d934fc93b.camel@codeconstruct.com.au>
From:   Quan Nguyen <quan@os.amperecomputing.com>
In-Reply-To: <dc36d61aedd2b2389eb366b27436f19d934fc93b.camel@codeconstruct.com.au>
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?UmF1WkVQNEp0TDV6cmh1a0ZDck5TUVV0U2pDbVQvRjBmNkpNWkRja2U0TnV1?=
 =?utf-8?B?SStDN1o4aUo0NXU4RUNoWElKY0JMVzdIUUJVZ0IxQXVsVzhFZy8rN1hJWFhB?=
 =?utf-8?B?eDFZWUxjL3poM3R3aFU0WTJSVjFEMlJzQ2xzd3RDZWo4MytmV0xuWDZpR3M1?=
 =?utf-8?B?YkVELzBxVWtCYkZRZjh0NWRHaHhPOGVFZFNmMCt1UmZFNExyVGZ6U25hQ3Jj?=
 =?utf-8?B?bFhEZTI3R1h2WnVDV1lSQnpYLzMrRXh0Uy80ckl3cFl3c0x6bGZCQVJycjk5?=
 =?utf-8?B?ZDdsU1RwRmhQa2FIbEJPY3RFZUhqL3RUQkRuR2IxbDl5QWtXYXo1UFJpaHNi?=
 =?utf-8?B?Y1RzWDZuWnA2ZUl2RHltYWtRb3hwelhRZS9oTTRtOEFQbDVjYmpIVXdpZDk4?=
 =?utf-8?B?Z1FrRmJKRndnS3RyQUJVUnhNY3dHNkpCR1VmZmV0QjZmNVEwaFZmaEhLRnl0?=
 =?utf-8?B?cEdqaWhFS04yY3QvcEdWelZoTDZKNm1Bczh3TEJtSEs1eXlHd2IvbjZHR2FU?=
 =?utf-8?B?OEliMzFWdE9ET1FMdVVJdVNJZzQzMmhQTVRyOXBUaWlLd3NOS2RmNEJoM3kv?=
 =?utf-8?B?a3Z3UC9aYWpXUXlHcjZ6TDd2MFlCRTgvT0trd3orMWl4bjVXV1VObk5xTlJB?=
 =?utf-8?B?MHA1MEFYT3hNMncxYmk2Vm1QbHNqbGprbnhYMDRqc2ZEVTVrSUpDSFZmTEFO?=
 =?utf-8?B?VDZSVzM5cmZwdjFqd2F3YXVPYVlFYU4vck02K1hPbFN2YWE5MHJUMTFYVXNX?=
 =?utf-8?B?NUhEMFBLM2xsQ3FBd0VJc1M4bk5yMmRhVUNrQUVkc05USXFkaEEyVXRQejdy?=
 =?utf-8?B?bXl1U0FvWWhlKzBueHhTUkNKMU1NQVhldUdpcGFKUEJNL3JBcEY1VzYzRXNr?=
 =?utf-8?B?VHVEbW5XaDJZTitzWE0yYStKU1VEMUFTNTVHNVN3UzhjWEdwUHdSQ280VlZr?=
 =?utf-8?B?L2RubCtZaHFkVzZWZlpZd2lBUlByQXVEYXlOOGdzVXQ4SE1zUjFHNkZrNGIw?=
 =?utf-8?B?SmV2ZDl3NG9WZDByeGc4UFp0K2pTYnlobUhZK21WcHliZElEVml1dTZVQkRM?=
 =?utf-8?B?d2V2SGJ2akdmcWJGNjMwS04rS0VTS3cvVVZaS2pyeEFoVkxCTTVSVWhadkpy?=
 =?utf-8?B?aFVDcVdxN1lIaW54UFJPWlZYUnFsRzVZUm9Qdm54Vm1sbld5YWp0MG5xS0hu?=
 =?utf-8?B?ZURmMW1jWU9XQTJaZUFaOVZZWWpXN1RmYlFzd3NWdVFKbzhyM1dDYTM4TGgx?=
 =?utf-8?B?aGdHUWVHU2ZhVjRTaXpMT0N3Tm15L2x6bkdhNXFGOGZua25IcVNYcmRkUGFi?=
 =?utf-8?B?eElzV2tqNXh6N3RNYXZWZ1JUTG91SEVwc09ZY05ZeDRzUjVwYlNXL0ZycERQ?=
 =?utf-8?B?TnlYcEpscXlDYVVVNXhOa2t1T0JDNUpvR3h2TEJqK0txNGIyYUlzM2VwQ3lN?=
 =?utf-8?B?Z1lyaXFuS3R5bjQ1UTQzcnpBdHY2TUpzbVRWclEwZUR5UGdjMFhPNVVXSlhD?=
 =?utf-8?B?MDhobDFKdWJScmE4MVFlYmRWeUZGOFVqU2FmbFUwcWlhc1d0bDZHcFVCbmYr?=
 =?utf-8?B?cjFOV01XR1IzR09TNzM5OW5WS3YyeVYwbTJZc0Q2dkJ2cTltZndScm9VRVFn?=
 =?utf-8?B?R21XVVZrRnp4RURZVEpQN3BuRzhNandrNjE0MzZPWVAzTXZ1WWhFUkJkU25R?=
 =?utf-8?B?aTFEd0tXV09ScklqMzFUei9rR1lCVzkyaEhSNVhuWXl0VlZ6UXUvSTVlSXJK?=
 =?utf-8?B?b0ZyLzZ6RnNRRGtaWkI3QndCUDl0bXB2ZHZIWCtGQmtKcjErK1I4a0NVRG1G?=
 =?utf-8?B?ZmNRdk9MMzhCbG83MTdEQVZoa2NwanJZUFRaR1hUdE0xZTFIckNCWXBWMVNm?=
 =?utf-8?B?ZHROdHJKZy9Uc3ZmVDBpOFBaODZkMWs5cGdCam1hTkZkL2lEK3lYNVNHM0Rp?=
 =?utf-8?B?dkd4WjhhcFhsbFFicFFTb2FqdzJZcmVILy9rQkRkVm92dFZqSlc1alF6dXZi?=
 =?utf-8?B?aU9vNW83aC9mNnFUVG5OU29HeVRhV2N6YkhUWjJDSzRYZ2hHUWJVMG9uUjd0?=
 =?utf-8?B?WjFHbTVuWHQ0dnhaQStOK041allzOS9lblg0NVNTTEFtMHN3VjBjRzhzZ0Rp?=
 =?utf-8?B?SGxoV2tFNjAwNDFoU2NEdVVHNVBlV3ZoTUtFaFgvd1Z1cE5BNXNaWmYrd0dC?=
 =?utf-8?Q?rNSqdQh6ikfuxG7UbMLOUPU=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 487bebe6-074a-4025-96b4-08dbf9fe8a91
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:24.7145
 (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: HDFiE9M2BVZdkr6PhzsZnjaRUXnXAaun9WJJnPKubPKKQT94EnPEqQlmBtiybO/M4nojlvoJ4bkWis9R1hIM6Gt+tyVCVn3M3sWxoZjFGVI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR01MB7821
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2,
        SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
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 (snail.vger.email [0.0.0.0]); Sun, 10 Dec 2023 20:06:46 -0800 (PST)


On 08/12/2023 11:00, Andrew Jeffery wrote:
> On Fri, 2023-12-08 at 10:31 +0700, Quan Nguyen wrote:
>> Commit 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in
>> interrupt handler") acknowledges most interrupts early before the slave
>> irq handler is executed, except for the "Receive Done Interrupt status"
>> which is acknowledged late in the interrupt.
>> However, it has been observed that the early acknowledgment of "Transmit
>> Done Interrupt Status" (with ACK or NACK) often causes the interrupt to
>> be raised in READ REQUEST state, that shows the
>> "Unexpected ACK on read request." complaint messages.
>>
>> Assuming that the "Transmit Done" interrupt should only be acknowledged
>> once it is truly processed, this commit fixes that issue by acknowledging
>> interrupts for both ACK and NACK cases late in the interrupt handler.
>>
>> Fixes: 2be6b47211e1 ("i2c: aspeed: Acknowledge most interrupts early in interrupt handler")
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>> v3:
>>    + Fix the unconditinal write when ack the irqs               [Andrew]
>>    + Refactor the code to enhance code readability                [Quan]
>>    + Fix grammar in commit message                                [Quan]
>>
>> v2:
>>    + Split to separate series                                     [Joel]
>>    + Added the Fixes line                                         [Joel]
>>    + Fixed multiline comment                                      [Joel]
>>    + Refactor irq clearing code                          [Joel, Guenter]
>>    + Revised commit message                                       [Joel]
>>    + Revised commit message                                       [Quan]
>>    + About a note to remind why the readl() should immediately follow the
>> writel() to fix the race condition when clearing irq status from commit
>> c926c87b8e36 ("i2c: aspeed: Avoid i2c interrupt status clear race
>> condition"), I think it looks straight forward in this patch and decided
>> not to add that note.                                            [Joel]
>>
>> v1:
>>    + First introduced in
>> https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/
>> ---
>>   drivers/i2c/busses/i2c-aspeed.c | 21 +++++++++++++--------
>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 1c2a4f4c4e1b..967a26dd4ffa 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -617,13 +617,19 @@ 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_ack_last;
> 
> `irq_ack_last` might be better as a macro, but you're probably saved by
> the optimiser anyway. If there's another reason to do a v4 or others
> are unhappy with it then consider fixing it, otherwise:
> 
> Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> 

I'll send out the v4 to use the defined macro instead as below:

Thanks Andi, for the suggestion on the macro name.

diff --git a/drivers/i2c/busses/i2c-aspeed.c 
b/drivers/i2c/busses/i2c-aspeed.c
index 5511fd46a65e..0f67218cf04a 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -93,6 +93,10 @@
  		 ASPEED_I2CD_INTR_RX_DONE | \
  		 ASPEED_I2CD_INTR_TX_NAK |  \
  		 ASPEED_I2CD_INTR_TX_ACK)
+#define ASPEED_I2CD_INTR_ACK_RX_TX	    \
+		(ASPEED_I2CD_INTR_RX_DONE | \
+		 ASPEED_I2CD_INTR_TX_ACK |  \
+		 ASPEED_I2CD_INTR_TX_NAK)

Thanks for the review,
- Quan