Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <43c99449-8a4b-4db5-8b1e-bc7ae920db76@os.amperecomputing.com>
Date:   Mon, 11 Dec 2023 11:08:52 +0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 1/2] i2c: aspeed: Handle the coalesced stop conditions
 with the start conditions.
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-2-quan@os.amperecomputing.com>
 <79ce9162faeb113ecb13efeb58d95f8a71e1a060.camel@codeconstruct.com.au>
From:   Quan Nguyen <quan@os.amperecomputing.com>
In-Reply-To: <79ce9162faeb113ecb13efeb58d95f8a71e1a060.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?MW53TDdEUGpQbE9IRVg4VDI1cVlERVBZZVhTNmVuN2NKTzY0T0FPYXgxcDd1?=
 =?utf-8?B?L2tCMENRYzhQMDRLWGF4N3FiaVVDZ1JwTFR4SnVERWwxL2hDbUl4SGtXNXJY?=
 =?utf-8?B?eFJya09SK3ZPVHVwZVpHU3IzSy9hdDBXN0RZWmxzVmxyYjBvYXlqSnYwemxi?=
 =?utf-8?B?azJTQk5IT1dVUlh6a1kzcmlWazJXOHhncnpqQ3FDQmVKQStSU1pCdmZBZ2w4?=
 =?utf-8?B?N0VJN0pnMkdVNUUvekh1OURFVEIyVyt1OGJyZnBZUFUvaFpwZWNQQW9rc3NC?=
 =?utf-8?B?ZFFHNEYyUXE4ejZjeGZtU1FIQWJVRVhncysvYkU1RTR2WmYrUUtNNEU4U2NN?=
 =?utf-8?B?SGhhNW53K05STSswYTVBcVN4aDdudDhDMU9EWldlWEdmdFl2N1psTHZ6eGRs?=
 =?utf-8?B?R1I5aDFaazVRUHZJeTRtbEJUWThIb210UDF3NTg5SG9YT050WS9VYkJ5WFdI?=
 =?utf-8?B?amhHMnQ4MGZNS2l1dWRmSnZEcFFzb1dvSnpIdWdmM1F6NU5BbVJPTUw0WHc3?=
 =?utf-8?B?bkl4b0J0dm1ObEVjNi96QnBKcFRZYnVWcm9tbGQyQjVtMDFnaUdLMHk0VXpJ?=
 =?utf-8?B?Z3JwTHE0OTl2NHlNYk9PN1l1a1hlWDJCVjZLUHhTYml2MDE3bml5elQxNGht?=
 =?utf-8?B?M1FCMWwyYW9JMytjYXYxa05wdWhOMzRuQm8vd1JmcGdVaHp6Q0JLREgrM2dB?=
 =?utf-8?B?K2M2QStPNW54UzVlb1U0UHMwNnhoL011VFYxRXFHNEFOZHFZNElqanM5dWFJ?=
 =?utf-8?B?TFNzT1BuMkJOdnhkSCt2T1dzc25KRXpFQ1lCVzJhbmtqVmp2QW1UTmlzeUJL?=
 =?utf-8?B?Tzl4Ym00SVAvZ3dwTzlsWm80UlBRNVNmblJIQVh0UHN3dTF0RFFMd0drcFBS?=
 =?utf-8?B?UXVvdE1RVlZjaGFlbmVNWngzVlg4TnRIWndlY1pOTy9JNDRKKzUydUF4RDEv?=
 =?utf-8?B?d3c5RHdVQ3lQUjExd3UvbVdJekFwVVcvMUlGVHYvZWhJaE5uNU9SemVDdmh6?=
 =?utf-8?B?TzBxUjB1ODF4a29YdTJ5LzZMOGZKSFBMem56aXplK3VvNnRFMGdvMWNsZGlN?=
 =?utf-8?B?Q0ZmaDRJOGx0UVdVVFNYdFR6N0tySENPam0zcHlHbGhRQ1dYSkFMS2hMTHk4?=
 =?utf-8?B?dk1BQXJERDMrdjRxOFk4Y2pmYTVIaWFGTVpieklCSkhIUEh1NGpUWm1DcnQ3?=
 =?utf-8?B?T3RuSmVnUzRVY3A1UnlCeGt3QzQ2bVBidnFqK1ZWbXFGQUF6SlJBU0xGTE5R?=
 =?utf-8?B?V0ZPbDBnOStUaXdVSnNjUGdvRzFxYlJPMzh0YnZCWUNqWGNvdDFFcUMybWxz?=
 =?utf-8?B?dWhtRU5vNGpxa1MwS3lSU25VZ3F3Y0dxdjJXRTZZcElmY21rR0JydEdWbEJl?=
 =?utf-8?B?bnVITVpWM3ZJVFMzQ1dJTUtHNkxXQ0p4NE5PbXh4U1UxeERSQ0xxaDRFYzUy?=
 =?utf-8?B?N0xJeldMYmJxZlN3M0VWeEdrNDVYdHBpWjlwV0NnSUd2MEJUdXBhY1l5UVVI?=
 =?utf-8?B?TWFabnZOdkx5TGh1MzFyRnlOY1p0ZU05WDJsck02eWIzbnY1K1JaMW5DNUJz?=
 =?utf-8?B?QVkvSG5QdjMwWEQ0WEpYQ05vMGVRVllnL2poM00zVzUxVXQ2SjR6RXpKRldU?=
 =?utf-8?B?N0IxTjRHNi93UndQdTBDcVJQZnMxdUdCcm5zbVRuUDhrdUk5dGdnR0d1VjIw?=
 =?utf-8?B?SDhpQ1krdW03QkFqM2V1K1ZQUUR2TmVBZWF1L1E2a2RhWjg3UlA3Z1dwY1lB?=
 =?utf-8?B?eGFHemoxUDFyQ3Q3TDBFRitGeDA0NDhpbjdZMTc1RDFUWjFvY3B0bzBCU0dJ?=
 =?utf-8?B?U05UZW9CbU5MOGR4SlFkOGp4UWIvMnVCWEl2NlFxNmNablYxQVdzNS9kRlFj?=
 =?utf-8?B?RWhmcFk1WHFLeEk1TWJBcHdDd05oUnRoMzhlZHNpcWZoK1F3dktmak91cUlD?=
 =?utf-8?B?blNITGEzMGJoczc1ZEdtWWtJNHB5d2FqaTdkbWhaTHUySHR6M0dWa1huaFFS?=
 =?utf-8?B?bWV2SUVLdEFaYXQ4S1lxQUN5V3ZRcllOYUlUdXI2c09oTzd2R3l3WUsxY2Yy?=
 =?utf-8?B?d3R5ZWlGQy82WDk1VXUzQ2JwTzUrM0FvNTdDVTdPQ3prc2hIVWZQNG1WUDdP?=
 =?utf-8?B?TkxRZmZLQTIvenljdXpDemdKUkxMY1F4S0xZVUpEdzM2ZFVPNWlqM2dqZm02?=
 =?utf-8?Q?A8WVQ9QdXcDGjin1k7gnI1U=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 82ef1e47-897c-46eb-4f9a-08dbf9fee9f7
X-MS-Exchange-CrossTenant-AuthSource: SN4PR01MB7455.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Dec 2023 04:09:04.7167
 (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: HpvyiMmHLT3NruOPdaRuYpgmh51jmVyA3C+uFWaYBVn2KY2ZYlN+s3FX2DY4uOFKcdRosQfYRk87WT6jbyQTnIYSp3X8WrstMcOAVMBsPP4=
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 howler.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 (howler.vger.email [0.0.0.0]); Sun, 10 Dec 2023 20:09:20 -0800 (PST)


On 08/12/2023 10:56, Andrew Jeffery wrote:
> On Fri, 2023-12-08 at 10:31 +0700, Quan Nguyen wrote:
>> Some masters may drive the transfers with low enough latency between
>> the nak/stop phase of the current command and the start/address phase
>> of the following command that the interrupts are coalesced by the
>> time we process them.
>> Handle the stop conditions before processing SLAVE_MATCH to fix the
>> complaints that sometimes occur below.
>>
>> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. Expected
>> 0x00000086, but was 0x00000084"
>>
>> Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>> v3:
>>    + Change to handle the coalesced stop condition with the start
>> conditions                                                            [Andrew]
>>    + Revised commit message                                              [Quan]
>>
>> v2:
>>    + Split to separate series                                            [Joel]
>>    + Added the Fixes line                                                [Joel]
>>    + Revised commit message                                              [Quan]
>>
>> v1:
>>    + First introduced in
>> https://lore.kernel.org/all/20210519074934.20712-1-quan@os.amperecomputing.com/
>> ---
>>   drivers/i2c/busses/i2c-aspeed.c | 47 ++++++++++++++++++++++-----------
>>   1 file changed, 31 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 28e2a5fc4528..1c2a4f4c4e1b 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -249,18 +249,45 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>>   	if (!slave)
>>   		return 0;
>>   
>> -	command = readl(bus->base + ASPEED_I2C_CMD_REG);
>> +	/*
>> +	 * Handle stop conditions early, prior to SLAVE_MATCH. Some masters may drive
>> +	 * transfers with low enough latency between the nak/stop phase of the current
>> +	 * command and the start/address phase of the following command that the
>> +	 * interrupts are coalesced by the time we process them.
>> +	 */
>> +	if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> +		irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
>> +		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> +	}
>>   
>> -	/* Slave was requested, restart state machine. */
>> +	if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
>> +	    bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
>> +		irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>> +		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> +	}
>> +
>> +	/* Propagate any stop conditions to the slave implementation. */
>> +	if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
>> +		i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
>> +		bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;
>> +	}
>> +	/*
> 
> If there's a reason to do a v4 then an extra empty line above the
> comment would be nice. But let's not get hung up on that if everyone
> else is happy.
> 
> Thanks for the fixes!
> 
> Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> 

Thanks Andrew,

I'll add your Reviewed-by in my v4 with that extra empty lime before the 
comment.

Thanks for the review
- Quan