Received-SPF: pass (google.com: domain of linux-kernel+bounces-3184-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <7dfc99e5-4c76-413b-aabc-81b26e26249e@os.amperecomputing.com>
Date: Mon, 18 Dec 2023 15:45:56 +0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 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: <20231211102217.2436294-1-quan@os.amperecomputing.com>
 <20231211102217.2436294-3-quan@os.amperecomputing.com>
 <2eab42cde34723a195e7a0287db08b25f8388a3b.camel@codeconstruct.com.au>
 <54cba87a0df233b8762e43b742afe8e44a77a60c.camel@codeconstruct.com.au>
From: Quan Nguyen <quan@os.amperecomputing.com>
In-Reply-To: <54cba87a0df233b8762e43b742afe8e44a77a60c.camel@codeconstruct.com.au>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?bWxGYWV4MTM4ZVlyQmhTWW9NRlBSekxrclpvSmZjbzZNeEpSMGQ2aGl6Tm1q?=
 =?utf-8?B?Z1JyUnJVSk9WcW5HRjh0VUxIL281dzJQbVhCZnpJcnZHVU9mRE1PN1JVajlo?=
 =?utf-8?B?a0ZRNDZpR0xhSGJEM1FYL1U2cU9TUXlJYmVqMWY5b3dKby9ZbmZlbTVscDhP?=
 =?utf-8?B?S2tkSWVXV2Z1OGhTcmRqTktpeUFjR3FiMDZVNi8wUTU0Zm1rWlROQmJxVnNv?=
 =?utf-8?B?cm1xR21kZGJlY0tMQWJSS3lSWE80ZzdsdEpLT2FRVlBUZ00wODVoN0gxMFdv?=
 =?utf-8?B?eGpvR3BTMldzN1E0Wi9WRmFucEZqZWcwVDRUbEEzUTVJeEJxb3d3cVk0K0ty?=
 =?utf-8?B?bXNyY29NbXRnODBsQVpiNU5WdUF3MmJiYVFqSms5eVpRWDN1dUs3MzFEZXRm?=
 =?utf-8?B?a2c1Ym03T0x0aldNOE51dzB3c0RGT0lxc1JGWnVrT3BFSUVsb0JHRjl5MFFG?=
 =?utf-8?B?WW9jRUNyYjM0YUNsME90Z1pwWko2L2NsVE1VZXJTUEJnWXI3R0IrZDBSN0VW?=
 =?utf-8?B?RTJlbWtuNGd2bFpyRldyai9hbEJPRDQ0aDNoUFdqWW8wTWVhWTlEOUhBQzZi?=
 =?utf-8?B?QjNGQVBmR1Z0WWdQRyt0OEVLRmd6VHlNWlFlRmJGTHlPajVhenBhUUlXdHY5?=
 =?utf-8?B?ZE8zdnVVZGl1ZFlOZURIbUFqLzVJWCt2VXBZNjcvd1ArOFdIcExlN2k5dTZD?=
 =?utf-8?B?RHhOSVRmbmoybExWTXJFUFZsaWNNazFEUnJ0MmVPSUp4VU5DM0tTNk5uOUs4?=
 =?utf-8?B?cDVOQVB5UGJaaDdRT1c4WUZicVVYTlhnYzVIUzRnUG11cURlUDg1V1hWK2ND?=
 =?utf-8?B?Kyt3VHVPZERDcS9pTGlXK3VYNXkwYVI2eVU5K3JqWHZzM1ljRjduS3gzUXEx?=
 =?utf-8?B?TFFvR2JGcExrVkZNbjRNeFpUZ05BRFM3NEpKUkRwS0xvbE9RNmZ3UTVYRE8y?=
 =?utf-8?B?aXRqWU5kaEZLZ0h0VTh2R1VqbFI1R2xhaEhjSkV1WXB2TjhLZ2ExZGxMODJu?=
 =?utf-8?B?eG84ZjBQTzNJcUxXcU4veitqWmJDeDBRcXVOZHJVakcxS28vbkFTUUxwMms0?=
 =?utf-8?B?UFlDcnRDbFlPMDJucmZNMGZtaHExcjhNajFWUU1LY0t4eFJja3JqVFRYUWRS?=
 =?utf-8?B?aGFCa2dGOHZaVHNLVGhlRmlvQm5NUlA1RUJnVld3OFk2K0duaCtQRFZOKzFj?=
 =?utf-8?B?ZE9MZTZCcEV5ZGI2dEM2bGlGeExSWm5jT0lFSENnVHNrTFUvUXd1NFZjMG55?=
 =?utf-8?B?cVp2NFFIa2RaaElnWjdSWkoxVXA1dW9OKzVCNjNGZTI3MG9NMFVUc2N4ckxJ?=
 =?utf-8?B?anRXbndjWWtkbTB5WDQ2UStLSzZwZUZzTE5ZaE10VFJ4OGFyK3RJUGp4K1E2?=
 =?utf-8?B?TVU1MzdzOVVocUlOeGJneDE5U1AyRWNKamxvSS9QaHExZHFnNXhGVC9obHRr?=
 =?utf-8?B?bm1PdHE0SUFxSmFJRitlK2ZSNy85ZDRMNUNjdlAzWHozdC9GNFVxVEVrZ3lJ?=
 =?utf-8?B?Z1htRkQvMFBKYmlYT083OGxJdFdSeWRJWkR0bGJlQnRheGdVTExsRytNQnh6?=
 =?utf-8?B?cS92UzBTbHdmUVJIaXZLTjJxVkltUUFpRDJudW9WajJ2QklvdnVOL3E2UVJq?=
 =?utf-8?B?RjlnbTlOdkRRaDFsTzdyN1cyZXc4Q2t4d2R6Q1g1MmhMdi9QN1NOL25SNmlC?=
 =?utf-8?B?bms5UUNtVllTcURqRldIRGdxT3pOMWJXbTFJbHN2N2xmZTN2Nitrcmt6V1JS?=
 =?utf-8?B?TC93NlF1cUZSZDgyeGNjMU11emdZdndRTTlhOVZXbGNhVWZ2b05BUGJMQ1Jl?=
 =?utf-8?B?Ym5RMTZ0dkxUYzBBQ0txNmxTdUNZZmFPMHZ1SzFHaW0xaTBlMm5pdTZVb1ND?=
 =?utf-8?B?TVI1NFhkcURsOWdnNnpIRlhyYTZjUjhQU3VzVmEzN3Azay9OMXVGK3hkRmtT?=
 =?utf-8?B?a2tFT0pNalVhb0ZIVUw4a2g2d3h4TUhJWFpqa0Q2Tlo3NVRsd29iUWROWXJ4?=
 =?utf-8?B?TytsZ0NyUTQrbEFIb29adVNuRWdHa3BLalNDTEZKR3huUzRYR0U0TjhRTHBw?=
 =?utf-8?B?ZGo0bCtyRnUrcDMxNWpJTE9PQi84VnkranpDVTY3bTVkUW90b1EzbllRbHBU?=
 =?utf-8?B?VWVYWGdBSkI0aERoQlRaMCtudUg3eDVSRVFYejNTZ0NJQnJ4SWxmT1haU3ZS?=
 =?utf-8?B?a1E9PQ==?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bc451b3b-4694-4bd8-3c73-08dbffa5c800
X-MS-Exchange-CrossTenant-AuthSource: SN4PR01MB7455.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Dec 2023 08:46:09.5151
 (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: kCc6BkizIgUKHA/3mWWwwf6YfHn/CK5UhtFteX9eKTjXHzPPRX7kd0pIthFO8u6b51QjvoFX9MuD2BjPeZYXdHv+6STR9xz6fxs4NfcA7r4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN4PR01MB7440


On 15/12/2023 05:21, Andrew Jeffery wrote:
>> On Mon, 2023-12-11 at 17:22 +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>
>>
>> Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> 
> So I just booted this series on v6.7-rc5 under qemu v8.2.0-rc4 and
> found this:
> 
> ```
> $ qemu-system-arm \
> 	-M ast2600-evb \
> 	-kernel build.aspeed_g5/arch/arm/boot/zImage \
> 	-dtb build.aspeed_g5/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dtb \
> 	-initrd ~/src/buildroot.org/buildroot/output/images/rootfs.cpio.xz \
> 	-nographic 2>&1 \
> 	| ts -s
> ...
> 00:00:03 [    1.089187] Freeing initrd memory: 3308K
> 00:00:05 smbus: error: Unexpected send start condition in state 1
> 00:00:05 smbus: error: Unexpected write in state -1
> 00:00:06 [    3.685731] aspeed-i2c-bus 1e78a400.i2c-bus: i2c bus 7 registered, irq 48
> 00:00:06 [    3.688918] aspeed-i2c-bus 1e78a480.i2c-bus: i2c bus 8 registered, irq 49
> 00:00:06 [    3.692326] aspeed-i2c-bus 1e78a500.i2c-bus: i2c bus 9 registered, irq 50
> 00:00:06 [    3.693757] aspeed-i2c-bus 1e78a680.i2c-bus: i2c bus 12 registered, irq 51
> 00:00:06 [    3.695070] aspeed-i2c-bus 1e78a700.i2c-bus: i2c bus 13 registered, irq 52
> 00:00:06 [    3.696184] aspeed-i2c-bus 1e78a780.i2c-bus: i2c bus 14 registered, irq 53
> 00:00:06 [    3.697144] aspeed-i2c-bus 1e78a800.i2c-bus: i2c bus 15 registered, irq 54
> 00:00:06 [    3.699061] aspeed-video 1e700000.video: irq 55
> 00:00:06 [    3.699254] aspeed-video 1e700000.video: assigned reserved memory node video
> 00:00:06 [    3.702755] aspeed-video 1e700000.video: alloc mem size(24576) at 0xbc000000 for jpeg header
> 00:00:06 [    3.706139] Driver for 1-wire Dallas network protocol.
> 00:00:07 smbus: error: Unexpected send start condition in state -1
> 00:00:07 smbus: error: Unexpected write in state -1
> 00:00:10 smbus: error: Unexpected send start condition in state -1
> 00:00:10 smbus: error: Unexpected write in state -1
> 00:00:12 smbus: error: Unexpected send start condition in state -1
> 00:00:12 smbus: error: Unexpected write in state -1
> 00:00:14 smbus: error: Unexpected send start condition in state -1
> 00:00:14 smbus: error: Unexpected write in state -1
> 00:00:17 smbus: error: Unexpected send start condition in state -1
> 00:00:17 smbus: error: Unexpected write in state -1
> 00:00:18 [   14.080141] adt7475 7-002e: Error configuring attenuator bypass
> 00:00:19 smbus: error: Unexpected send start condition in state -1
> 00:00:19 smbus: error: Unexpected write in state -1
> 00:00:21 smbus: error: Unexpected send start condition in state -1
> 00:00:21 smbus: error: Unexpected write in state -1
> 00:00:24 smbus: error: Unexpected send start condition in state -1
> 00:00:24 smbus: error: Unexpected write in state -1
> ```
> 
> The smbus errors do not occur if I revert this patch.
> 
> Can you look into qemu to see if it's a bug in the aspeed i2c
> controller model's state machine?
> 

Thanks, Andrew, for testing these patches on qemu.

I'll try to look into it to see if anything can be improved, but I have 
to admit that I'm not so familiar with it. This is my first time trying 
it on qemu. Just did these tests on real HW with waveform captured 
sometimes.

So far I could be able to reproduce the issue and start playing around 
trying to understand the model.

Thanks,
- Quan