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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <x5pkfdxwnpqv66d4y3bucpd6vpxbsahdt2mdj6mdlb43emfkxn@dktn4wpuosgr>
Date: Wed, 13 Aug 2025 15:25:40 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Palash Kambar <quic_pkambar@...cinc.com>
Cc: James.Bottomley@...senpartnership.com, martin.petersen@...cle.com, 
	linux-arm-msm@...r.kernel.org, linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org, 
	quic_nitirawa@...cinc.com
Subject: Re: [PATCH v3] ufs: ufs-qcom: Align programming sequence of Shared
 ICE for  UFS controller v5

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?

> 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@...cinc.com>
> 
> ---
> 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)

> +
>  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:
		/*
		 * 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.

- Mani

-- 
மணிவண்ணன் சதாசிவம்