Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Message-ID: <3f37c359-1c71-421f-b7d9-054696735adc@os.amperecomputing.com>
Date:   Wed, 29 Nov 2023 16:03:11 +0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 RESEND 1/2] i2c: aspeed: Fix unhandled Tx done with NAK
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: <20231128075236.2724038-1-quan@os.amperecomputing.com>
 <20231128075236.2724038-2-quan@os.amperecomputing.com>
 <20231129003542.jfhhotebweb3uwyb@zenone.zhora.eu>
From:   Quan Nguyen <quan@os.amperecomputing.com>
In-Reply-To: <20231129003542.jfhhotebweb3uwyb@zenone.zhora.eu>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TTdCVVhQc3VKKzIzQ015UUY3N0I5YWMrd002am55d2RaL3JpZ2twcEtXRWxr?=
 =?utf-8?B?ZVBWdlduaHVGWTRGNVo1SDJmR1Zxa2VOTys1NmVGbzNxRzlEdzdFSEJldWt3?=
 =?utf-8?B?eG55cWpWZzVMeDM0OWFRQlRaT254L0p6aUFzQWVHbFY4Rzd2cXRJTzZ1eS9D?=
 =?utf-8?B?Wm1OblVMSXgweE1nT21yOVI3T2t6Z00vYjFZeDJPOFh0N1R1T1pMNmpyOE90?=
 =?utf-8?B?b0VQdUpLK3VFdVcxWG4xODdSM1FVMm5aRldnNk43aW02aW5INFZSMGVITWRV?=
 =?utf-8?B?M01ZdDdlT0k2THpiMWFsQXlnNlhZc2RHNk5GR3AwQ1VaRGV6YkF5QWZwak16?=
 =?utf-8?B?eGFCbm1VWk9ja01uejdMd01PNFJKTFRoWEJCRHFTQm5wTkJvYWs0WDlXNVJn?=
 =?utf-8?B?TyttVzluS3hXVzl2OTFtTWlaY1REcHZBNVFVT3pLQ1hHVnZWWHZOWnhwdEI2?=
 =?utf-8?B?cGJzdFljUXNRbDV3cWtnWVB5UUNIQVFrZDEwcG85OWdtMXRNTjR2T3dOSG1q?=
 =?utf-8?B?WnQ4YlgrNUZJSXNDMGlsaDRhMFk5KzNPTmd2U1gxN1ZVeWFtU1Z2ZUtWZU9Z?=
 =?utf-8?B?WVVvYUI0YlRES21vaS9wYjdnVzYwVG9HVUNDeGVacDBZWWhOS1FYc1dScDBL?=
 =?utf-8?B?a2FlUmlFdW1TTDltYmIwNDdFY2xHNzhITGMvY2FKUVlFbmlibVpQNmlaTlhE?=
 =?utf-8?B?bTRMcW5DS1V6UTFidGwrcllwdXVlU0RQZVFuSFNkUlMwMWNTaVNJS1ZYc3hE?=
 =?utf-8?B?aW1XMHlLUlJMYTd4NVYrRFZXT2RhcDFFU1BFK1E2Q3JveGhiV0QzUTVtUStx?=
 =?utf-8?B?S09pYWlkc0kvRXJ3ZDJLRmtrRHkzZE40dkY5WmRvSlhhNDg5aktJSTQrQWFq?=
 =?utf-8?B?aWxFQVN1VDZuVFU5OUNuWTB1N0lkSzR2eVNubHA4VWFYUnJobHI3a2Z0MUVR?=
 =?utf-8?B?aWFIczk5KzZFU2pnZjNmVEdrYlJMaFhrU2lOK25JanNxWVhEdStYc3B6WUN1?=
 =?utf-8?B?L3BnOXNHcEVlWXdkMEhzd200dWpVMzlMcHZVRmhqbVdwY0NHQU9YL1hpQXll?=
 =?utf-8?B?Y3FvYnJqd3k3cUpSTDl2YjcrSWVSYk5qUGNMWHFyKzl0c2hBRTF0a0haTnpO?=
 =?utf-8?B?Vmh1QTQwVTBudnRKWEVpTkw1S0tZRlB2OUwxaXhLaWZtK2RaV01FSW9NRmky?=
 =?utf-8?B?dFpLSUY1R1Z1SXpEZURRODZXREJWMGJ5QVprMGxxRUhUWDlLQ2p5ZXFyU0pK?=
 =?utf-8?B?TTgwOEV4RXJFd2NJRHVQcHMxRDFzWU1zT3VKQXJLZTBQTDJZcXNCczN1YTM0?=
 =?utf-8?B?bjJ0UW9Xcjh4Nms1WmdCVFpsM1Q0VEl3RkFVUENQTnE1NE94QnIzckJHeHhC?=
 =?utf-8?B?Q0ZxY0VaL0RwTGd0bkduUWh0YitDSm9adUJQTnk1MG4weDNSMXc3L2FhLzZm?=
 =?utf-8?B?YXBsMkFCaWFWNWpUdllKdDRSSnNPeFZGTStXMmw5NnNHekI2TXZ3dzEvNmpa?=
 =?utf-8?B?azRjYnl2NWxqZTA5M0grVFQzUG9UQzJVemdMaFlnT1Q5Y0dLSTVEOC9JdkZQ?=
 =?utf-8?B?eWZhd3BhS0FQeVVDcWI2WVJycUlTSTZraHBZSVFSZDFVbUpOMVJEWVh2WVhr?=
 =?utf-8?B?aXpPSCtlWHRqR0J2ZWV3LzhoMUoweFBSZStsejhyZlVQS0J0b01CZWNxbmlt?=
 =?utf-8?B?VHB1bGRheGE5eTBGeDY4SU9RUjlXeDBCNGFNRUdQT09HRlQ2WkFURWVaZUd1?=
 =?utf-8?B?V29zNmZzT0RvRG9iNXU3ZGpSUXl5SXJodUJ6ejJiN3V3Y25PZDZObTRHRytT?=
 =?utf-8?B?TTlFSi9IL2Nzc0RHVC94aDF2NEdMaG1qbjhWSEdtVEc3TXpnck5CRXB1a1FO?=
 =?utf-8?B?ZHk4TTZxUlhsZWNFREcvL050QnJETE03RkdQdkd5aGNxalE3d2RwYVAvUjN6?=
 =?utf-8?B?Q2hyMWNCbUt0L2xKQ0FtWFZMcGpoZDNrbElkUzM2dnpPTkxFMWdBYkpQQ01z?=
 =?utf-8?B?aXp3UlppNWs5WDArMnRUazBMdWRrcTlMVG8rYWVGd3dDeDFqM1FWdCsranE3?=
 =?utf-8?B?K2k1aCtkdnlXTndCQi9SajVCOUxFVnpuSXVGdGlhT0pHN0VvbG91Z0FwOFNn?=
 =?utf-8?B?WU1JS1dzMmNQYzZIWkRkbkQxZ29jMndoNzB5WWp6cXhIUHBraVhnZ1N5MGNo?=
 =?utf-8?Q?Je991DuIKzOHT/km23nElfo=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 91b6c609-64d2-4d94-3c8d-08dbf0ba06fc
X-MS-Exchange-CrossTenant-AuthSource: SN4PR01MB7455.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Nov 2023 09:03:17.6830
 (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: eEQgCjP2gtjBO9UmqiNNu4OHStKzPlwd5Lr2cIPKcaixNWCtdo1UsVkdVm2X9HOGlD5qiCBf48/68BsbiraDx737osVtJfmuraWl9QHthxQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR01MB6302
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 agentk.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 (agentk.vger.email [0.0.0.0]); Wed, 29 Nov 2023 01:03:34 -0800 (PST)


On 29/11/2023 07:35, Andi Shyti wrote:
> Hi Quan,
> 
> On Tue, Nov 28, 2023 at 02:52:35PM +0700, Quan Nguyen wrote:
>> Under normal conditions, after the last byte is sent by the Slave, the
>> TX_NAK interrupt is raised.  However, it is also observed that
>> sometimes the Master issues the next transaction too quickly while the
>> Slave IRQ handler is not yet invoked and the TX_NAK interrupt for the
>> last byte of the previous READ_PROCESSED state has not been ack’ed.
>> This TX_NAK interrupt is then raised together with SLAVE_MATCH interrupt
>> and RX_DONE interrupt of the next coming transaction from Master. The
>> Slave IRQ handler currently handles the SLAVE_MATCH and RX_DONE, but
>> ignores the TX_NAK, causing complaints such as
>> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. Expected
>> 0x00000086, but was 0x00000084"
>>
>> This commit adds code to handle this case by emitting a SLAVE_STOP event
>> for the TX_NAK before processing the RX_DONE for the coming transaction
>> from the Master.
>>
>> Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>> 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 | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 28e2a5fc4528..79476b46285b 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -253,6 +253,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>>   
>>   	/* Slave was requested, restart state machine. */
>>   	if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
>> +		if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
>> +		    bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
>> +			irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>> +			i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
>> +		}
> 
> this is a duplicate of a later "if (...)" satement. What is the
> need for having them both?
> 
Thanks Andi for the review.

I assumed the if statement you mentioned is here in [1]. If so, then 
that is not duplicate.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-aspeed.c?h=v6.7-rc3#n287


The if statement is to process the case when Slave sending data to 
Master but being NAK, the I2C_SLAVE_STOP event will emit later in 
switch-case statement. But it is only for the case INTR_TX_NAK without 
INTR_SLAVE_MATCH.

The new code is for the case of INTR_TX_NAK with INTR_SLAVE_MATCH. What 
it does is to detect if there is a mix of INTR_TX_NAK of previous i2c 
transaction and the start of new i2c transaction, indicate by 
INTR_SLAVE_MATCH which is only raised when Slave found its address 
matched on the first byte it received. If so, the new code will try to 
emit the I2C_SLAVE_STOP first to complete the previous transaction and 
process the rest as a new request.

So if this was the case (with INTR_SLAVE_MATCH), the INTR_RX_DONE should 
always raise with INTR_SLAVE_MATCH because Slave did receive the data 
which matched with its Slave address. And this will be translated into 
either I2C_SLAVE_[READ|WRITE]_REQUESTED and that make the if statement 
you mentioned [1] evaluate to false and skip.

So, in short, the new code is trying to handle the case of INTR_TX_NAK 
with INTR_SLAVE_MATCH first before let the rest process as normal.

Thanks,
- Quan