Re: [PATCH v3] ufs: ufs-qcom: Align programming sequence of Shared ICE for UFS controller v5

[Palash Kambar]

On 8/13/2025 3:25 PM, Manivannan Sadhasivam wrote:
> On Tue, Aug 12, 2025 at 02:47:14PM GMT, Palash Kambar wrote:
>> Disable of AES core in Shared ICE is not supported during power
>> collapse for UFS Host Controller V5.0.
>>
> 
> Could you please add more info on the issue observed?

Sure Mani.

> 
>> Hence follow below steps to reset the ICE upon exiting power collapse
>> and align with Hw programming guide.
>>
>> a. Write 0x18 to UFS_MEM_ICE_CFG
>> b. Write 0x0 to UFS_MEM_ICE_CFG
>>
> 
> Please be explicit about the fields you are writing to.
> 
>> Signed-off-by: Palash Kambar <quic_pkambar@xxxxxxxxxxx>
>>
>> ---
>> changes from V1:
>> 1) Incorporated feedback from Konrad and Manivannan by adding a delay
>>    between ICE reset assertion and deassertion.
>> 2) Removed magic numbers and replaced them with meaningful constants.
>>
>> changes from V2:
>> 1) Addressed Manivannan's comment and moved change to ufs_qcom_resume.
>> ---
>>  drivers/ufs/host/ufs-qcom.c | 14 ++++++++++++++
>>  drivers/ufs/host/ufs-qcom.h |  2 +-
>>  2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 444a09265ded..60bf5e60b747 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -38,6 +38,9 @@
>>  #define DEEMPHASIS_3_5_dB	0x04
>>  #define NO_DEEMPHASIS		0x0
>>  
>> +#define UFS_ICE_RESET_ASSERT_VALUE	0x18
>> +#define UFS_ICE_RESET_DEASSERT_VALUE	0x00
> 
> Please define the actual bits as per the documentation, not the value you are
> writing. Here, you are changing two fields:
> 
> ICE_SYNC_RST_SEL BIT(3)
> ICE_SYNC_RST_SW BIT(4)

ok Mani.

>> +
>>  enum {
>>  	TSTBUS_UAWM,
>>  	TSTBUS_UARM,
>> @@ -756,6 +759,17 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>>  	if (err)
>>  		return err;
>>  
>> +	if ((!ufs_qcom_is_link_active(hba)) &&
>> +	    host->hw_ver.major == 5 &&
>> +	    host->hw_ver.minor == 0 &&
>> +	    host->hw_ver.step == 0) {
>> +		ufshcd_writel(hba, UFS_ICE_RESET_ASSERT_VALUE, UFS_MEM_ICE);
>> +		ufshcd_readl(hba, UFS_MEM_ICE);
>> +		usleep_range(50, 100);
> 
> Please add a comment above the delay to make it clear that the delay is not as
> per the doc:

Sure.

> 		/*
> 		 * HW documentation doesn't recommend any delay between the
> 		 * reset set and clear. But we are enforcing an arbitrary delay
> 		 * to give flops enough time to settle in.
> 		 */
> 
>> +		ufshcd_writel(hba, UFS_ICE_RESET_DEASSERT_VALUE, UFS_MEM_ICE);
>> +		ufshcd_readl(hba, UFS_MEM_ICE);
>> +	}
>> +
>>  	return ufs_qcom_ice_resume(host);
>>  }
>>  
>> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
>> index 6840b7526cf5..cc1324ce05c7 100644
>> --- a/drivers/ufs/host/ufs-qcom.h
>> +++ b/drivers/ufs/host/ufs-qcom.h
>> @@ -60,7 +60,7 @@ enum {
>>  	UFS_AH8_CFG				= 0xFC,
>>  
>>  	UFS_RD_REG_MCQ				= 0xD00,
>> -
>> +	UFS_MEM_ICE				= 0x2600,
> 
> As the internal doc, this register is called UFS_MEM_ICE_CFG.

Ok will update the register name.


> - Mani
>