Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <f71fc275-5afd-5ce6-83e7-938fc2580fb0@amperemail.onmicrosoft.com>
Date:   Mon, 16 Oct 2023 09:47:06 +0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v1] i2c: designware: Disable TX_EMPTY irq while waiting
 for block length byte
To:     Serge Semin <fancer.lancer@gmail.com>,
        Tam Nguyen <tamnguyenchi@os.amperecomputing.com>
Cc:     linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org,
        patches@amperecomputing.com, jarkko.nikula@linux.intel.com,
        andriy.shevchenko@linux.intel.com, mika.westerberg@linux.intel.com,
        jsd@semihalf.com, chuong@os.amperecomputing.com,
        darren@os.amperecomputing.com, stable@vger.kernel.org
References: <20230929035356.6435-1-tamnguyenchi@os.amperecomputing.com>
 <avd7jhwexehgbvi6euzdwvf5zvqqgjx4ozo6uxu2qpmlarvva3@sgkce3rvovwk>
Content-Language: en-US
From:   Tam Chi Nguyen <tamnguyenchi@amperemail.onmicrosoft.com>
In-Reply-To: <avd7jhwexehgbvi6euzdwvf5zvqqgjx4ozo6uxu2qpmlarvva3@sgkce3rvovwk>
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?d1ROMDFMbkJwSHpDWGNRRzZFYU1GK0N2MjAzSzdpbFFsZk9qeXJScVdrOHlO?=
 =?utf-8?B?UVNFamd1UENIc1puREtrQ0NpU29kK29TbjNqQ0FGMytYWlpmblkwbDFvRDlH?=
 =?utf-8?B?bWZoWjVjeXh1WExrbG8rU01MNG95SWVIZjJpV0JiWm1aZEJwTDlzd2dxcGE5?=
 =?utf-8?B?NVBKNXZ3Q21rM0grSEVmbjNtT1FDYWRFWFg2eWYvSGwyT1h0L3R5RGhuMThC?=
 =?utf-8?B?TlpjYWNjYitXbkdDbEROcER6eG9ZaUViYXIyNXN5dXpjV2xLU3B2ODd5Yzla?=
 =?utf-8?B?VXcvcjAvKzN5MEZIZldnUVJaeVhNdGxDeHJENUZ5UkpsOTFCSkNGcHVHZjY1?=
 =?utf-8?B?Vjlwam5RUzByU2RPVEhTNlJkK0twejJnN1ZQSDVUb1dXY0NRaGV0alQrcERl?=
 =?utf-8?B?RTE1Qmg5RVh5UlN5RjFmZ3F5dWsvU1VCVVJYTGlTR0xPend1eFJCUENQNTJQ?=
 =?utf-8?B?TWg1eUozZXFEN1N0TG1xOXR3amIvV0pOWGpqK0hyQSs2TEJFR3AzblBkVmh2?=
 =?utf-8?B?NVRKM3VEQUluU2RIZi9XREJDVEluMDArOWo4L0dQVXJvNHVwV0tjeU9LOWlY?=
 =?utf-8?B?VFZNaFpPbGRrenE5TUE2RE5aYndPZHZJM0dhVTRNbTF5a1poT0I5Zm9RaUlT?=
 =?utf-8?B?UDE5Z0R6ajBvNTVqNFVGd0tBOVRqR0huOXVvOE5HVDBtcm13VEtNTXUrWGZ4?=
 =?utf-8?B?Yk1CZ3lUTDJLOUJtT3A2eU5OdkU3Y2ZmeFFvbmRYU0x4TWRUcnBtZzFWd2Vw?=
 =?utf-8?B?R05YK2RMK1NJK3VGOFhFR0hwNU5Lc1B6ZkxmMUxXNThjcmhnMkcwTGtVUVNX?=
 =?utf-8?B?WlFUWVB5aU03aEJsbTVpMUlPREYvc2x3VEowNEFRNjN1MGxiQWJiZ2IyRDdj?=
 =?utf-8?B?akFjMVZPNnZsa2haekg3ZnhtQTE5QXFFYlBuVnYrYXJTMlpLWDNDc20waFFk?=
 =?utf-8?B?UHpMT00zQk1NWmhZbVM0OFpEbXRqc2ZRMGwvbTBieE9TZFJQQWsxUTVpSWdN?=
 =?utf-8?B?Qy8zM2Y3TDdCYTdBcjJCd3ROZExGanpvdWtzbjZtd1FJeGYzaURKdmNhMHlL?=
 =?utf-8?B?dTdVMmtwekg2RFg0dkFISUExYjlTNFFFOUlBRnVqdnhpM3RWdUV6U0pRdDcv?=
 =?utf-8?B?VVEvTWdvZStHMDU4Y0NDMk9ZQnlEK2tKS2ZpRnk3Z25keW41VWJhaVJ4TG5w?=
 =?utf-8?B?Nm1FT3VwME85NUpUNS9wQ2JtZWVBVm5YckZ3blBsMmJkMEw1UHRMQ2FZQ2RP?=
 =?utf-8?B?SURTM2JmTjNaYms5ajk4WWNtNmoxSjNlcjBuWkRFV3N2MGFmWE9FeXBIWWEr?=
 =?utf-8?B?a1o0RVdhN1dtYzRVRzVSQ3B4ZnBDTEMyK2NhV29acko1WklPVzZpOEdKRkRs?=
 =?utf-8?B?emw5TTRES2hNdWNyZlRmMS9JZU1QejV2SWw1VGV6cmQvMHpKbU4vdTBtc3E0?=
 =?utf-8?B?V1NwbVNyS2JBMXZUWjg0cjRxMUtsKzhPTGVDbExoOXh3WGtOVjhRSm9KTGh0?=
 =?utf-8?B?cm13TEhZN052Vy9qM0RZSlhRUUlFa3J5QWFObkU0ci96R3dhWFN0a0UySG54?=
 =?utf-8?B?SlY2WlVBL2ZIYmIxcHRBOGVWZTdkWUduSXIwZTNuZ3N1Y0p4ZjZOSXRWM2VS?=
 =?utf-8?B?bVJXZGpPNzFzRzVXMEpxMWUzZG1Jc0RCZlVCR3VPeTVIQUVXeW1obXdVeG81?=
 =?utf-8?B?elVUM1NiM2lEclJCT2xnWEVFZ0VjUTNIaGRBdDR3ZzFXcnM0YXBOc2QxOGE2?=
 =?utf-8?B?SmR2Nk16VTVYcE84QnRjQTZjc3BlZ0l4amJzUHJIRmtFc2k5RHpKeXQzaFZ1?=
 =?utf-8?B?RTJEOXpqZDA1RlpYa1Y5ajdXdUhCb2hPSG9MUTY0NkVMZk1QTk1vSTgvRjdM?=
 =?utf-8?B?SkhjMXI2aWdIdFB6WUJGY3dhanNCdkpWUkJrSHgxK3dhcGhWa2w3ZHFNY0ND?=
 =?utf-8?B?b1pEMlFHSmlQQXRXM3VQNUJ0U0lRMDY0blJmRTZVd1hiMERlV2ZVQUlxSWZT?=
 =?utf-8?B?cGFnSktXQ3pjNWFUdzZISTM3ZWR0Zm5KS2YrM2UrZ1hhVi9pZEVaN0RmOTM1?=
 =?utf-8?B?Zjk5SmNBRHEvbzd1RTNXdGEvK1VCUUh5dXFsK05VZEEvcWlROEUwWFk0Zjhw?=
 =?utf-8?B?NFFqQ1V2dk43WGpZZnZEN295VVlyU25lYTVvTEMwSUhJbmZqRFk4NEtmNndq?=
 =?utf-8?Q?IFNiStFnLiavgtuO1GT51wrGeKt3LQXxO7gI3xYYmS1T?=
X-OriginatorOrg: amperemail.onmicrosoft.com
X-MS-Exchange-CrossTenant-Network-Message-Id: be8b9337-c4bc-4e99-4d62-08dbcdf237e5
X-MS-Exchange-CrossTenant-AuthSource: SA1PR01MB8131.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Oct 2023 02:47:20.8180
 (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: erlWbtVBEFxg7V5jbX9+EKkc4ZXH3Lf+1j8JeFYKz3+ukTsw3t9L6g6LTvZazDS2Ujo+xUyMP30/0NXKxyFXSMZhnDxEoe08FS+dU3gQPWYxxaZ2dM/nrwjuLcS2Ox/B
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR01MB8571
X-Spam-Status: No, score=-3.6 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED,
	FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.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 (fry.vger.email [0.0.0.0]); Sun, 15 Oct 2023 19:47:39 -0700 (PDT)


On 9/29/2023 23:08, Serge Semin wrote:
> On Fri, Sep 29, 2023 at 10:53:56AM +0700, Tam Nguyen wrote:
>> During SMBus block data read process, we have seen high interrupt rate
>> because of TX_EMPTY irq status while waiting for block length byte (the
>> first data byte after the address phase). The interrupt handler does not
>> do anything because the internal state is kept as STATUS_WRITE_IN_PROGRESS.
>> Hence, we should disable TX_EMPTY irq until I2C DW receives first data
>> byte from I2C device, then re-enable it.
>>
>> It takes 0.789 ms for host to receive data length from slave.
>> Without the patch, i2c_dw_isr is called 99 times by TX_EMPTY interrupt.
>> And it is none after applying the patch.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Chuong Tran <chuong@os.amperecomputing.com>
>> Signed-off-by: Tam Nguyen <tamnguyenchi@os.amperecomputing.com>
>> ---
>>   drivers/i2c/busses/i2c-designware-master.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
>> index 55ea91a63382..2152b1f9b27c 100644
>> --- a/drivers/i2c/busses/i2c-designware-master.c
>> +++ b/drivers/i2c/busses/i2c-designware-master.c
>> @@ -462,6 +462,13 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>>   		if (buf_len > 0 || flags & I2C_M_RECV_LEN) {
>>   			/* more bytes to be written */
>>   			dev->status |= STATUS_WRITE_IN_PROGRESS;
>> +			/*
>> +			 * In I2C_FUNC_SMBUS_BLOCK_DATA case, there is no data
>> +			 * to send before receiving data length from slave.
>> +			 * Disable TX_EMPTY while waiting for data length byte
>> +			 */
>> +			if (flags & I2C_M_RECV_LEN)
>> +				intr_mask &= ~DW_IC_INTR_TX_EMPTY;
> Is it possible to reduce the indentations level? Like this:
>
> 		/*
> 		 * Because we don't know the buffer length in the
> 		 * I2C_FUNC_SMBUS_BLOCK_DATA case, we can't stop the
> 		 * transaction here. Also disable the TX_EMPTY IRQ
> 		 * while waiting for the data length byte to avoid the
> 		 * bogus interrupts flood.
> 		 */
> 		if (flags & I2C_M_RECV_LEN) {
>    			dev->status |= STATUS_WRITE_IN_PROGRESS;
> 			intr_mask &= ~DW_IC_INTR_TX_EMPTY;
> 			break;
> 		} else if (buf_len > 0) {
>    			/* more bytes to be written */
>    			dev->status |= STATUS_WRITE_IN_PROGRESS;
> 			break;
> 		} else {
> 			dev->status &= ~STATUS_WRITE_IN_PROGRESS;
> 		}
Thank you very much for your advice. I will update in V2.
>>   			break;
>>   		} else
>>   			dev->status &= ~STATUS_WRITE_IN_PROGRESS;
>> @@ -485,6 +492,7 @@ i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len)
>>   {
>>   	struct i2c_msg *msgs = dev->msgs;
>>   	u32 flags = msgs[dev->msg_read_idx].flags;
>> +	u32 intr_mask;
>>   
>>   	/*
>>   	 * Adjust the buffer length and mask the flag
>> @@ -495,6 +503,11 @@ i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len)
>>   	msgs[dev->msg_read_idx].len = len;
>>   	msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN;
>>   
>> +	/* Re-enable TX_EMPTY interrupt. */
>> +	regmap_read(dev->map, DW_IC_INTR_MASK, &intr_mask);
>> +	intr_mask |= DW_IC_INTR_TX_EMPTY;
>> +	regmap_write(dev->map, DW_IC_INTR_MASK, intr_mask);
> 1. What about just:
> 	regmap_update_bits(dev->map, DW_IC_INTR_MASK, DW_IC_INTR_TX_EMPTY,
> 			   DW_IC_INTR_TX_EMPTY);
Right, much better with this update. I'll update it in v2.
> 2. The in-situ comment is pointless because the statement already
> implies the IRQ re-enabling. I suggest to add more details of _why_
> the IRQ needs to be re-enabled (what is supposed to be done after it's
> re-enabled?).

Got that. I will add more context here. Thank for the advice.

Best regards,
Tam Nguyen

> -Serge(y)
>
>> +
>>   	return len;
>>   }
>>   
>> -- 
>> 2.25.1
>>