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

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

Posted by Palash Kambar 2 months ago

Disable of AES core in Shared ICE is not supported during power
collapse for UFS Host Controller V5.0.

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

Signed-off-by: Palash Kambar <quic_pkambar@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 24 ++++++++++++++++++++++++
 drivers/ufs/host/ufs-qcom.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 444a09265ded..2744614bbc32 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -744,6 +744,8 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
 	if (ufs_qcom_is_link_off(hba) && host->device_reset)
 		ufs_qcom_device_reset_ctrl(hba, true);
 
+	host->vdd_hba_pc = true;
+
 	return ufs_qcom_ice_suspend(host);
 }
 
@@ -759,6 +761,27 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	return ufs_qcom_ice_resume(host);
 }
 
+static void ufs_qcom_hibern8_notify(struct ufs_hba *hba,
+				    enum uic_cmd_dme uic_cmd,
+				    enum ufs_notify_change_status status)
+{
+	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+
+	/* Apply shared ICE WA */
+	if (uic_cmd == UIC_CMD_DME_HIBER_EXIT &&
+	    status == POST_CHANGE &&
+	    host->hw_ver.major == 0x5 &&
+	    host->hw_ver.minor == 0x0 &&
+	    host->hw_ver.step == 0x0 &&
+	    host->vdd_hba_pc) {
+		host->vdd_hba_pc = false;
+		ufshcd_writel(hba, 0x18, UFS_MEM_ICE);
+		ufshcd_readl(hba, UFS_MEM_ICE);
+		ufshcd_writel(hba, 0x0, UFS_MEM_ICE);
+		ufshcd_readl(hba, UFS_MEM_ICE);
+	}
+}
+
 static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
 {
 	if (host->dev_ref_clk_ctrl_mmio &&
@@ -2258,6 +2281,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
 	.hce_enable_notify      = ufs_qcom_hce_enable_notify,
 	.link_startup_notify    = ufs_qcom_link_startup_notify,
 	.pwr_change_notify	= ufs_qcom_pwr_change_notify,
+	.hibern8_notify		= ufs_qcom_hibern8_notify,
 	.apply_dev_quirks	= ufs_qcom_apply_dev_quirks,
 	.fixup_dev_quirks       = ufs_qcom_fixup_dev_quirks,
 	.suspend		= ufs_qcom_suspend,
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 6840b7526cf5..8a8bd44a8e22 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -60,6 +60,7 @@ enum {
 	UFS_AH8_CFG				= 0xFC,
 
 	UFS_RD_REG_MCQ				= 0xD00,
+	UFS_MEM_ICE				= 0x2600,
 
 	REG_UFS_MEM_ICE_CONFIG			= 0x260C,
 	REG_UFS_MEM_ICE_NUM_CORE		= 0x2664,
@@ -290,6 +291,7 @@ struct ufs_qcom_host {
 	u32 phy_gear;
 
 	bool esi_enabled;
+	bool vdd_hba_pc;
 	unsigned long active_cmds;
 };
 
-- 
2.34.1

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

Posted by Manivannan Sadhasivam 1 month, 4 weeks ago

On Wed, Aug 06, 2025 at 12:04:09PM GMT, Palash Kambar wrote:
> Disable of AES core in Shared ICE is not supported during power
> collapse for UFS Host Controller V5.0.
> 
> 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
> 
> Signed-off-by: Palash Kambar <quic_pkambar@quicinc.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 24 ++++++++++++++++++++++++
>  drivers/ufs/host/ufs-qcom.h |  2 ++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 444a09265ded..2744614bbc32 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -744,6 +744,8 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
>  	if (ufs_qcom_is_link_off(hba) && host->device_reset)
>  		ufs_qcom_device_reset_ctrl(hba, true);
>  
> +	host->vdd_hba_pc = true;

What does this variable correspond to?

> +
>  	return ufs_qcom_ice_suspend(host);
>  }
>  
> @@ -759,6 +761,27 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  	return ufs_qcom_ice_resume(host);
>  }
>  
> +static void ufs_qcom_hibern8_notify(struct ufs_hba *hba,
> +				    enum uic_cmd_dme uic_cmd,
> +				    enum ufs_notify_change_status status)
> +{
> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> +
> +	/* Apply shared ICE WA */

Are you really sure it is *shared ICE*?

> +	if (uic_cmd == UIC_CMD_DME_HIBER_EXIT &&
> +	    status == POST_CHANGE &&
> +	    host->hw_ver.major == 0x5 &&
> +	    host->hw_ver.minor == 0x0 &&
> +	    host->hw_ver.step == 0x0 &&
> +	    host->vdd_hba_pc) {
> +		host->vdd_hba_pc = false;
> +		ufshcd_writel(hba, 0x18, UFS_MEM_ICE);

Define the actual bits instead of writing magic values.

> +		ufshcd_readl(hba, UFS_MEM_ICE);
> +		ufshcd_writel(hba, 0x0, UFS_MEM_ICE);
> +		ufshcd_readl(hba, UFS_MEM_ICE);

Why do you need readl()? Writes to device memory won't get reordered.

> +	}
> +}
> +
>  static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
>  {
>  	if (host->dev_ref_clk_ctrl_mmio &&
> @@ -2258,6 +2281,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
>  	.hce_enable_notify      = ufs_qcom_hce_enable_notify,
>  	.link_startup_notify    = ufs_qcom_link_startup_notify,
>  	.pwr_change_notify	= ufs_qcom_pwr_change_notify,
> +	.hibern8_notify		= ufs_qcom_hibern8_notify,

This callback is not called anywhere. Regardeless of that, why can't you use
ufs_qcom_clk_scale_notify()?

- Mani

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

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

Posted by Konrad Dybcio 1 month, 4 weeks ago

On 8/6/25 1:14 PM, Manivannan Sadhasivam wrote:
> On Wed, Aug 06, 2025 at 12:04:09PM GMT, Palash Kambar wrote:
>> Disable of AES core in Shared ICE is not supported during power
>> collapse for UFS Host Controller V5.0.
>>
>> 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
>>
>> Signed-off-by: Palash Kambar <quic_pkambar@quicinc.com>
>> ---

[...]

> 
>> +		ufshcd_readl(hba, UFS_MEM_ICE);
>> +		ufshcd_writel(hba, 0x0, UFS_MEM_ICE);
>> +		ufshcd_readl(hba, UFS_MEM_ICE);
> 
> Why do you need readl()? Writes to device memory won't get reordered.

I'm not sure if we need a delay between them, otherwise they'll happen
within a couple cycles of each other which may not be enough since this
is a synchronous reset and the clock period is 20-50ns when running at
XO (19.2 / 38.4 MHz) rate

Konrad

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

Posted by Manivannan Sadhasivam 1 month, 4 weeks ago

On Wed, Aug 06, 2025 at 02:56:43PM GMT, Konrad Dybcio wrote:
> On 8/6/25 1:14 PM, Manivannan Sadhasivam wrote:
> > On Wed, Aug 06, 2025 at 12:04:09PM GMT, Palash Kambar wrote:
> >> Disable of AES core in Shared ICE is not supported during power
> >> collapse for UFS Host Controller V5.0.
> >>
> >> 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
> >>
> >> Signed-off-by: Palash Kambar <quic_pkambar@quicinc.com>
> >> ---
> 
> [...]
> 
> > 
> >> +		ufshcd_readl(hba, UFS_MEM_ICE);
> >> +		ufshcd_writel(hba, 0x0, UFS_MEM_ICE);
> >> +		ufshcd_readl(hba, UFS_MEM_ICE);
> > 
> > Why do you need readl()? Writes to device memory won't get reordered.
> 
> I'm not sure if we need a delay between them, otherwise they'll happen
> within a couple cycles of each other which may not be enough since this
> is a synchronous reset and the clock period is 20-50ns when running at
> XO (19.2 / 38.4 MHz) rate
> 

IIUC, the second register write is just reenabling the mask, so there is no
delay required between these two writes. If that's not true, and if there is a
delay required, then do:

	ufshcd_writel(0x18);
	ufshcd_readl();
	usleep()/msleep();
	ufshcd_write(0x0);

- Mani

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

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

Posted by Konrad Dybcio 1 month, 4 weeks ago

On 8/6/25 7:39 PM, Manivannan Sadhasivam wrote:
> On Wed, Aug 06, 2025 at 02:56:43PM GMT, Konrad Dybcio wrote:
>> On 8/6/25 1:14 PM, Manivannan Sadhasivam wrote:
>>> On Wed, Aug 06, 2025 at 12:04:09PM GMT, Palash Kambar wrote:
>>>> Disable of AES core in Shared ICE is not supported during power
>>>> collapse for UFS Host Controller V5.0.
>>>>
>>>> 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
>>>>
>>>> Signed-off-by: Palash Kambar <quic_pkambar@quicinc.com>
>>>> ---
>>
>> [...]
>>
>>>
>>>> +		ufshcd_readl(hba, UFS_MEM_ICE);
>>>> +		ufshcd_writel(hba, 0x0, UFS_MEM_ICE);
>>>> +		ufshcd_readl(hba, UFS_MEM_ICE);
>>>
>>> Why do you need readl()? Writes to device memory won't get reordered.
>>
>> I'm not sure if we need a delay between them, otherwise they'll happen
>> within a couple cycles of each other which may not be enough since this
>> is a synchronous reset and the clock period is 20-50ns when running at
>> XO (19.2 / 38.4 MHz) rate
>>
> 
> IIUC, the second register write is just reenabling the mask, so there is no

Yes

> delay required between these two writes. If that's not true, and if there is a

Yes

> delay required, then do:

We don't know if it's required, the HPG makes no effort to clarify it,
but I would expect it's probably not instantaneous


> 
> 	ufshcd_writel(0x18);
> 	ufshcd_readl();
> 	usleep()/msleep();
> 	ufshcd_write(0x0);

You'd still need the second readl to make sure the de-assertion has
gone through before proceeding

Konrad

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

Posted by Palash Kambar 1 month, 4 weeks ago


On 8/6/2025 11:09 PM, Manivannan Sadhasivam wrote:
> On Wed, Aug 06, 2025 at 02:56:43PM GMT, Konrad Dybcio wrote:
>> On 8/6/25 1:14 PM, Manivannan Sadhasivam wrote:
>>> On Wed, Aug 06, 2025 at 12:04:09PM GMT, Palash Kambar wrote:
>>>> Disable of AES core in Shared ICE is not supported during power
>>>> collapse for UFS Host Controller V5.0.
>>>>
>>>> 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
>>>>
>>>> Signed-off-by: Palash Kambar <quic_pkambar@quicinc.com>
>>>> ---
>>
>> [...]
>>
>>>
>>>> +		ufshcd_readl(hba, UFS_MEM_ICE);
>>>> +		ufshcd_writel(hba, 0x0, UFS_MEM_ICE);
>>>> +		ufshcd_readl(hba, UFS_MEM_ICE);
>>>
>>> Why do you need readl()? Writes to device memory won't get reordered.
>>
>> I'm not sure if we need a delay between them, otherwise they'll happen
>> within a couple cycles of each other which may not be enough since this
>> is a synchronous reset and the clock period is 20-50ns when running at
>> XO (19.2 / 38.4 MHz) rate
>>
> 
> IIUC, the second register write is just reenabling the mask, so there is no
> delay required between these two writes. If that's not true, and if there is a
> delay required, then do:
> 
> 	ufshcd_writel(0x18);
> 	ufshcd_readl();
> 	usleep()/msleep();
> 	ufshcd_write(0x0);
> 

 I will address this in next patch set.

-Palash K

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

Posted by Palash Kambar 1 month, 4 weeks ago


On 8/6/2025 4:44 PM, Manivannan Sadhasivam wrote:
> On Wed, Aug 06, 2025 at 12:04:09PM GMT, Palash Kambar wrote:
>> Disable of AES core in Shared ICE is not supported during power
>> collapse for UFS Host Controller V5.0.
>>
>> 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
>>
>> Signed-off-by: Palash Kambar <quic_pkambar@quicinc.com>
>> ---
>>  drivers/ufs/host/ufs-qcom.c | 24 ++++++++++++++++++++++++
>>  drivers/ufs/host/ufs-qcom.h |  2 ++
>>  2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 444a09265ded..2744614bbc32 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -744,6 +744,8 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
>>  	if (ufs_qcom_is_link_off(hba) && host->device_reset)
>>  		ufs_qcom_device_reset_ctrl(hba, true);
>>  
>> +	host->vdd_hba_pc = true;
> 
> What does this variable correspond to?
Hi Manivannan,

It corresponds to power collapse, will rename it for better readability.

> 
>> +
>>  	return ufs_qcom_ice_suspend(host);
>>  }
>>  
>> @@ -759,6 +761,27 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>>  	return ufs_qcom_ice_resume(host);
>>  }
>>  
>> +static void ufs_qcom_hibern8_notify(struct ufs_hba *hba,
>> +				    enum uic_cmd_dme uic_cmd,
>> +				    enum ufs_notify_change_status status)
>> +{
>> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> +
>> +	/* Apply shared ICE WA */
> 
> Are you really sure it is *shared ICE*?

 Yes Manivannan, I am.

> 
>> +	if (uic_cmd == UIC_CMD_DME_HIBER_EXIT &&
>> +	    status == POST_CHANGE &&
>> +	    host->hw_ver.major == 0x5 &&
>> +	    host->hw_ver.minor == 0x0 &&
>> +	    host->hw_ver.step == 0x0 &&
>> +	    host->vdd_hba_pc) {
>> +		host->vdd_hba_pc = false;
>> +		ufshcd_writel(hba, 0x18, UFS_MEM_ICE);
> 
> Define the actual bits instead of writing magic values.

Sure.

> 
>> +		ufshcd_readl(hba, UFS_MEM_ICE);
>> +		ufshcd_writel(hba, 0x0, UFS_MEM_ICE);
>> +		ufshcd_readl(hba, UFS_MEM_ICE);
> 
> Why do you need readl()? Writes to device memory won't get reordered.

Since these are hardware register, there is a potential for reordering.

> 
>> +	}
>> +}
>> +
>>  static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
>>  {
>>  	if (host->dev_ref_clk_ctrl_mmio &&
>> @@ -2258,6 +2281,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
>>  	.hce_enable_notify      = ufs_qcom_hce_enable_notify,
>>  	.link_startup_notify    = ufs_qcom_link_startup_notify,
>>  	.pwr_change_notify	= ufs_qcom_pwr_change_notify,
>> +	.hibern8_notify		= ufs_qcom_hibern8_notify,
> 
> This callback is not called anywhere. Regardeless of that, why can't you use
> ufs_qcom_clk_scale_notify()?
> 

According to the HPG guidelines, as part of this workaround, we are required to reset the ICE controller during the Hibern8 exit sequence when the UFS controller resumes from power collapse. Therefore, this reset logic has been added to the H8 exit notifier callback.

The ufs_clk_scale_notify function is invoked whenever clock scaling (up or down) occurs, regardless of whether a power collapse has taken place. Hence, the ICE controller reset was specifically added to the H8 exit notifier to ensure it is executed only in the appropriate context.


Regards,
Palash K

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

Posted by Manivannan Sadhasivam 1 month, 4 weeks ago

On Wed, Aug 06, 2025 at 06:11:09PM GMT, Palash Kambar wrote:
> 
> 
> On 8/6/2025 4:44 PM, Manivannan Sadhasivam wrote:
> > On Wed, Aug 06, 2025 at 12:04:09PM GMT, Palash Kambar wrote:
> >> Disable of AES core in Shared ICE is not supported during power
> >> collapse for UFS Host Controller V5.0.
> >>
> >> 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
> >>
> >> Signed-off-by: Palash Kambar <quic_pkambar@quicinc.com>
> >> ---
> >>  drivers/ufs/host/ufs-qcom.c | 24 ++++++++++++++++++++++++
> >>  drivers/ufs/host/ufs-qcom.h |  2 ++
> >>  2 files changed, 26 insertions(+)
> >>
> >> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> >> index 444a09265ded..2744614bbc32 100644
> >> --- a/drivers/ufs/host/ufs-qcom.c
> >> +++ b/drivers/ufs/host/ufs-qcom.c
> >> @@ -744,6 +744,8 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
> >>  	if (ufs_qcom_is_link_off(hba) && host->device_reset)
> >>  		ufs_qcom_device_reset_ctrl(hba, true);
> >>  
> >> +	host->vdd_hba_pc = true;
> > 
> > What does this variable correspond to?
> Hi Manivannan,
> 
> It corresponds to power collapse, will rename it for better readability.
> 

What is 'power collapse' from UFS perspective?

> > 
> >> +
> >>  	return ufs_qcom_ice_suspend(host);
> >>  }
> >>  
> >> @@ -759,6 +761,27 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> >>  	return ufs_qcom_ice_resume(host);
> >>  }
> >>  
> >> +static void ufs_qcom_hibern8_notify(struct ufs_hba *hba,
> >> +				    enum uic_cmd_dme uic_cmd,
> >> +				    enum ufs_notify_change_status status)
> >> +{
> >> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> >> +
> >> +	/* Apply shared ICE WA */
> > 
> > Are you really sure it is *shared ICE*?
> 
>  Yes Manivannan, I am.
> 

Well, there are two kind of registers defined in the internal doc that I can
see: UFS_MEM_ICE and UFS_MEM_SHARED_ICE. And hence the question.

> > 
> >> +	if (uic_cmd == UIC_CMD_DME_HIBER_EXIT &&
> >> +	    status == POST_CHANGE &&
> >> +	    host->hw_ver.major == 0x5 &&
> >> +	    host->hw_ver.minor == 0x0 &&
> >> +	    host->hw_ver.step == 0x0 &&
> >> +	    host->vdd_hba_pc) {
> >> +		host->vdd_hba_pc = false;
> >> +		ufshcd_writel(hba, 0x18, UFS_MEM_ICE);
> > 
> > Define the actual bits instead of writing magic values.
> 
> Sure.
> 
> > 
> >> +		ufshcd_readl(hba, UFS_MEM_ICE);
> >> +		ufshcd_writel(hba, 0x0, UFS_MEM_ICE);
> >> +		ufshcd_readl(hba, UFS_MEM_ICE);
> > 
> > Why do you need readl()? Writes to device memory won't get reordered.
> 
> Since these are hardware register, there is a potential for reordering.
> 

Really? Who said that? Please cite the reference.

> > 
> >> +	}
> >> +}
> >> +
> >>  static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
> >>  {
> >>  	if (host->dev_ref_clk_ctrl_mmio &&
> >> @@ -2258,6 +2281,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
> >>  	.hce_enable_notify      = ufs_qcom_hce_enable_notify,
> >>  	.link_startup_notify    = ufs_qcom_link_startup_notify,
> >>  	.pwr_change_notify	= ufs_qcom_pwr_change_notify,
> >> +	.hibern8_notify		= ufs_qcom_hibern8_notify,
> > 
> > This callback is not called anywhere. Regardeless of that, why can't you use
> > ufs_qcom_clk_scale_notify()?
> > 
> 
> According to the HPG guidelines, as part of this workaround, we are required to reset the ICE controller during the Hibern8 exit sequence when the UFS controller resumes from power collapse. Therefore, this reset logic has been added to the H8 exit notifier callback.
> 

Please wrap the replies to 80 column.

Well, we do call ufshcd_uic_hibern8_exit() from these callbacks. So why can't
you reset the ICE after calling ufshcd_uic_hibern8_exit() here?

> The ufs_clk_scale_notify function is invoked whenever clock scaling (up or down) occurs, regardless of whether a power collapse has taken place. Hence, the ICE controller reset was specifically added to the H8 exit notifier to ensure it is executed only in the appropriate context.
> 

Please define what 'power collapse' mean here. And as I said before, you are
not at all calling this newly introduced callback.

- Mani

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

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

Posted by Palash Kambar 1 month, 4 weeks ago


On 8/6/2025 11:19 PM, Manivannan Sadhasivam wrote:
> On Wed, Aug 06, 2025 at 06:11:09PM GMT, Palash Kambar wrote:
>>
>>
>> On 8/6/2025 4:44 PM, Manivannan Sadhasivam wrote:
>>> On Wed, Aug 06, 2025 at 12:04:09PM GMT, Palash Kambar wrote:
>>>> Disable of AES core in Shared ICE is not supported during power
>>>> collapse for UFS Host Controller V5.0.
>>>>
>>>> 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
>>>>
>>>> Signed-off-by: Palash Kambar <quic_pkambar@quicinc.com>
>>>> ---
>>>>  drivers/ufs/host/ufs-qcom.c | 24 ++++++++++++++++++++++++
>>>>  drivers/ufs/host/ufs-qcom.h |  2 ++
>>>>  2 files changed, 26 insertions(+)
>>>>
>>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>>> index 444a09265ded..2744614bbc32 100644
>>>> --- a/drivers/ufs/host/ufs-qcom.c
>>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>>> @@ -744,6 +744,8 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
>>>>  	if (ufs_qcom_is_link_off(hba) && host->device_reset)
>>>>  		ufs_qcom_device_reset_ctrl(hba, true);
>>>>  
>>>> +	host->vdd_hba_pc = true;
>>>
>>> What does this variable correspond to?
>> Hi Manivannan,
>>
>> It corresponds to power collapse, will rename it for better readability.
>>
> 
> What is 'power collapse' from UFS perspective?

As part of UFS controller power collapse, UFS controller and PHY enters HIBERNATE_STATE
during idle periods .The UFS controller is power-collapsed with essential registers 
retained (for ex ICE), while PHY maintains M-PHY compliant signaling. Upon data transfer
requests, software restores power and exits HIBERNATE_STATE without requiring re-initialization, 
as configurations and ICE encryption keys are preserved.

> 
>>>
>>>> +
>>>>  	return ufs_qcom_ice_suspend(host);
>>>>  }
>>>>  
>>>> @@ -759,6 +761,27 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>>>>  	return ufs_qcom_ice_resume(host);
>>>>  }
>>>>  
>>>> +static void ufs_qcom_hibern8_notify(struct ufs_hba *hba,
>>>> +				    enum uic_cmd_dme uic_cmd,
>>>> +				    enum ufs_notify_change_status status)
>>>> +{
>>>> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>>> +
>>>> +	/* Apply shared ICE WA */
>>>
>>> Are you really sure it is *shared ICE*?
>>
>>  Yes Manivannan, I am.
>>
> 
> Well, there are two kind of registers defined in the internal doc that I can
> see: UFS_MEM_ICE and UFS_MEM_SHARED_ICE. And hence the question.
> 
>>>
>>>> +	if (uic_cmd == UIC_CMD_DME_HIBER_EXIT &&
>>>> +	    status == POST_CHANGE &&
>>>> +	    host->hw_ver.major == 0x5 &&
>>>> +	    host->hw_ver.minor == 0x0 &&
>>>> +	    host->hw_ver.step == 0x0 &&
>>>> +	    host->vdd_hba_pc) {
>>>> +		host->vdd_hba_pc = false;
>>>> +		ufshcd_writel(hba, 0x18, UFS_MEM_ICE);
>>>
>>> Define the actual bits instead of writing magic values.
>>
>> Sure.
>>
>>>
>>>> +		ufshcd_readl(hba, UFS_MEM_ICE);
>>>> +		ufshcd_writel(hba, 0x0, UFS_MEM_ICE);
>>>> +		ufshcd_readl(hba, UFS_MEM_ICE);
>>>
>>> Why do you need readl()? Writes to device memory won't get reordered.
>>
>> Since these are hardware register, there is a potential for reordering.
>>
> 
> Really? Who said that? Please cite the reference.
> 
>>>
>>>> +	}
>>>> +}
>>>> +
>>>>  static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
>>>>  {
>>>>  	if (host->dev_ref_clk_ctrl_mmio &&
>>>> @@ -2258,6 +2281,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
>>>>  	.hce_enable_notify      = ufs_qcom_hce_enable_notify,
>>>>  	.link_startup_notify    = ufs_qcom_link_startup_notify,
>>>>  	.pwr_change_notify	= ufs_qcom_pwr_change_notify,
>>>> +	.hibern8_notify		= ufs_qcom_hibern8_notify,
>>>
>>> This callback is not called anywhere. Regardeless of that, why can't you use
>>> ufs_qcom_clk_scale_notify()?
>>>
>>
>> According to the HPG guidelines, as part of this workaround, we are required to reset the ICE controller during the Hibern8 exit sequence when the UFS controller resumes from power collapse. Therefore, this reset logic has been added to the H8 exit notifier callback.
>>
> 
> Please wrap the replies to 80 column.
> 
> Well, we do call ufshcd_uic_hibern8_exit() from these callbacks. So why can't
> you reset the ICE after calling ufshcd_uic_hibern8_exit() here?

As per HPG guidance, the ICE Reset workaround is required only after the
controller undergoes a power collapse. In the UFS subsystem, power collapse
is managed via the GDSC (GCC_UFS_MEM_PHY_GDSC), which is part of GenPD
(power domains). Since GenPD is tied to runtime suspend operations, we are
setting the power collapse flag during runtime suspend and checking this
flag during hibernate exit.


> 
>> The ufs_clk_scale_notify function is invoked whenever clock scaling (up or down) occurs, regardless of whether a power collapse has taken place. Hence, the ICE controller reset was specifically added to the H8 exit notifier to ensure it is executed only in the appropriate context.
>>
> 
> Please define what 'power collapse' mean here. And as I said before, you are
> not at all calling this newly introduced callback.

This is not a newly introduced callback, Mani. We are registering for
an already existing notifier. You may refer to ufshcd.c, where this
notifier is invoked.

> 
> - Mani
>

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

Posted by Manivannan Sadhasivam 1 month, 3 weeks ago

On Thu, Aug 07, 2025 at 03:50:58PM GMT, Palash Kambar wrote:
> 
> 
> On 8/6/2025 11:19 PM, Manivannan Sadhasivam wrote:
> > On Wed, Aug 06, 2025 at 06:11:09PM GMT, Palash Kambar wrote:
> >>
> >>
> >> On 8/6/2025 4:44 PM, Manivannan Sadhasivam wrote:
> >>> On Wed, Aug 06, 2025 at 12:04:09PM GMT, Palash Kambar wrote:
> >>>> Disable of AES core in Shared ICE is not supported during power
> >>>> collapse for UFS Host Controller V5.0.
> >>>>
> >>>> 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
> >>>>
> >>>> Signed-off-by: Palash Kambar <quic_pkambar@quicinc.com>
> >>>> ---
> >>>>  drivers/ufs/host/ufs-qcom.c | 24 ++++++++++++++++++++++++
> >>>>  drivers/ufs/host/ufs-qcom.h |  2 ++
> >>>>  2 files changed, 26 insertions(+)
> >>>>
> >>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> >>>> index 444a09265ded..2744614bbc32 100644
> >>>> --- a/drivers/ufs/host/ufs-qcom.c
> >>>> +++ b/drivers/ufs/host/ufs-qcom.c
> >>>> @@ -744,6 +744,8 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
> >>>>  	if (ufs_qcom_is_link_off(hba) && host->device_reset)
> >>>>  		ufs_qcom_device_reset_ctrl(hba, true);
> >>>>  
> >>>> +	host->vdd_hba_pc = true;
> >>>
> >>> What does this variable correspond to?
> >> Hi Manivannan,
> >>
> >> It corresponds to power collapse, will rename it for better readability.
> >>
> > 
> > What is 'power collapse' from UFS perspective?
> 
> As part of UFS controller power collapse, UFS controller and PHY enters HIBERNATE_STATE
> during idle periods .The UFS controller is power-collapsed with essential registers 
> retained (for ex ICE), while PHY maintains M-PHY compliant signaling. Upon data transfer
> requests, software restores power and exits HIBERNATE_STATE without requiring re-initialization, 
> as configurations and ICE encryption keys are preserved.
> 

AFAIK, Hibern8 is a UFS *link* specific feature, not controller specific. In
other peripherals, power collapse means powering off the controller entirely and
then relying on the hardware logic to retain the register states. I believe the
same behavior applies to UFS also.

In that case, I would expect you to check for the power collapse in
ufs_qcom_resume() using some logic and toggle the relevant bits in UFS_MEM_ICE.

The current logic you proposed doesn't really make sure that the controller is
power collapsed. You just assume that ufs_qcom_suspend() would allow the
controller to enter power collapse state, but it won't. If the user has opted
for 'spm_lvl' to be '0', then I don't think the controller can enter power
collapse state.

> > 
> >>>
> >>>> +
> >>>>  	return ufs_qcom_ice_suspend(host);
> >>>>  }
> >>>>  
> >>>> @@ -759,6 +761,27 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> >>>>  	return ufs_qcom_ice_resume(host);
> >>>>  }
> >>>>  
> >>>> +static void ufs_qcom_hibern8_notify(struct ufs_hba *hba,
> >>>> +				    enum uic_cmd_dme uic_cmd,
> >>>> +				    enum ufs_notify_change_status status)
> >>>> +{
> >>>> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> >>>> +
> >>>> +	/* Apply shared ICE WA */
> >>>
> >>> Are you really sure it is *shared ICE*?
> >>
> >>  Yes Manivannan, I am.
> >>
> > 
> > Well, there are two kind of registers defined in the internal doc that I can
> > see: UFS_MEM_ICE and UFS_MEM_SHARED_ICE. And hence the question.
> > 
> >>>
> >>>> +	if (uic_cmd == UIC_CMD_DME_HIBER_EXIT &&
> >>>> +	    status == POST_CHANGE &&
> >>>> +	    host->hw_ver.major == 0x5 &&
> >>>> +	    host->hw_ver.minor == 0x0 &&
> >>>> +	    host->hw_ver.step == 0x0 &&
> >>>> +	    host->vdd_hba_pc) {
> >>>> +		host->vdd_hba_pc = false;
> >>>> +		ufshcd_writel(hba, 0x18, UFS_MEM_ICE);
> >>>
> >>> Define the actual bits instead of writing magic values.
> >>
> >> Sure.
> >>
> >>>
> >>>> +		ufshcd_readl(hba, UFS_MEM_ICE);
> >>>> +		ufshcd_writel(hba, 0x0, UFS_MEM_ICE);
> >>>> +		ufshcd_readl(hba, UFS_MEM_ICE);
> >>>
> >>> Why do you need readl()? Writes to device memory won't get reordered.
> >>
> >> Since these are hardware register, there is a potential for reordering.
> >>
> > 
> > Really? Who said that? Please cite the reference.
> > 
> >>>
> >>>> +	}
> >>>> +}
> >>>> +
> >>>>  static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
> >>>>  {
> >>>>  	if (host->dev_ref_clk_ctrl_mmio &&
> >>>> @@ -2258,6 +2281,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
> >>>>  	.hce_enable_notify      = ufs_qcom_hce_enable_notify,
> >>>>  	.link_startup_notify    = ufs_qcom_link_startup_notify,
> >>>>  	.pwr_change_notify	= ufs_qcom_pwr_change_notify,
> >>>> +	.hibern8_notify		= ufs_qcom_hibern8_notify,
> >>>
> >>> This callback is not called anywhere. Regardeless of that, why can't you use
> >>> ufs_qcom_clk_scale_notify()?
> >>>
> >>
> >> According to the HPG guidelines, as part of this workaround, we are required to reset the ICE controller during the Hibern8 exit sequence when the UFS controller resumes from power collapse. Therefore, this reset logic has been added to the H8 exit notifier callback.
> >>
> > 
> > Please wrap the replies to 80 column.
> > 
> > Well, we do call ufshcd_uic_hibern8_exit() from these callbacks. So why can't
> > you reset the ICE after calling ufshcd_uic_hibern8_exit() here?
> 
> As per HPG guidance, the ICE Reset workaround is required only after the
> controller undergoes a power collapse. In the UFS subsystem, power collapse
> is managed via the GDSC (GCC_UFS_MEM_PHY_GDSC), which is part of GenPD
> (power domains). Since GenPD is tied to runtime suspend operations, we are
> setting the power collapse flag during runtime suspend and checking this
> flag during hibernate exit.
> 
> 
> > 
> >> The ufs_clk_scale_notify function is invoked whenever clock scaling (up or down) occurs, regardless of whether a power collapse has taken place. Hence, the ICE controller reset was specifically added to the H8 exit notifier to ensure it is executed only in the appropriate context.
> >>
> > 
> > Please define what 'power collapse' mean here. And as I said before, you are
> > not at all calling this newly introduced callback.
> 
> This is not a newly introduced callback, Mani. We are registering for
> an already existing notifier. You may refer to ufshcd.c, where this
> notifier is invoked.
> 

Okay, my bad. You could've mentioned something about this callback in the commit
message to make others aware that you are reusing an existing callback.

- Mani

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

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

Posted by Palash Kambar 1 month, 3 weeks ago


On 8/11/2025 1:46 PM, Manivannan Sadhasivam wrote:
> On Thu, Aug 07, 2025 at 03:50:58PM GMT, Palash Kambar wrote:
>>
>>
>> On 8/6/2025 11:19 PM, Manivannan Sadhasivam wrote:
>>> On Wed, Aug 06, 2025 at 06:11:09PM GMT, Palash Kambar wrote:
>>>>
>>>>
>>>> On 8/6/2025 4:44 PM, Manivannan Sadhasivam wrote:
>>>>> On Wed, Aug 06, 2025 at 12:04:09PM GMT, Palash Kambar wrote:
>>>>>> Disable of AES core in Shared ICE is not supported during power
>>>>>> collapse for UFS Host Controller V5.0.
>>>>>>
>>>>>> 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
>>>>>>
>>>>>> Signed-off-by: Palash Kambar <quic_pkambar@quicinc.com>
>>>>>> ---
>>>>>>  drivers/ufs/host/ufs-qcom.c | 24 ++++++++++++++++++++++++
>>>>>>  drivers/ufs/host/ufs-qcom.h |  2 ++
>>>>>>  2 files changed, 26 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>>>>> index 444a09265ded..2744614bbc32 100644
>>>>>> --- a/drivers/ufs/host/ufs-qcom.c
>>>>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>>>>> @@ -744,6 +744,8 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
>>>>>>  	if (ufs_qcom_is_link_off(hba) && host->device_reset)
>>>>>>  		ufs_qcom_device_reset_ctrl(hba, true);
>>>>>>  
>>>>>> +	host->vdd_hba_pc = true;
>>>>>
>>>>> What does this variable correspond to?
>>>> Hi Manivannan,
>>>>
>>>> It corresponds to power collapse, will rename it for better readability.
>>>>
>>>
>>> What is 'power collapse' from UFS perspective?
>>
>> As part of UFS controller power collapse, UFS controller and PHY enters HIBERNATE_STATE
>> during idle periods .The UFS controller is power-collapsed with essential registers 
>> retained (for ex ICE), while PHY maintains M-PHY compliant signaling. Upon data transfer
>> requests, software restores power and exits HIBERNATE_STATE without requiring re-initialization, 
>> as configurations and ICE encryption keys are preserved.
>>
> 
> AFAIK, Hibern8 is a UFS *link* specific feature, not controller specific. In
> other peripherals, power collapse means powering off the controller entirely and
> then relying on the hardware logic to retain the register states. I believe the
> same behavior applies to UFS also.
> 
> In that case, I would expect you to check for the power collapse in
> ufs_qcom_resume() using some logic and toggle the relevant bits in UFS_MEM_ICE.
> 
> The current logic you proposed doesn't really make sure that the controller is
> power collapsed. You just assume that ufs_qcom_suspend() would allow the
> controller to enter power collapse state, but it won't. If the user has opted
> for 'spm_lvl' to be '0', then I don't think the controller can enter power
> collapse state.

Sure Mani, will modify as per your suggestion.

- Palash K> 
>>>
>>>>>
>>>>>> +
>>>>>>  	return ufs_qcom_ice_suspend(host);
>>>>>>  }
>>>>>>  
>>>>>> @@ -759,6 +761,27 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>>>>>>  	return ufs_qcom_ice_resume(host);
>>>>>>  }
>>>>>>  
>>>>>> +static void ufs_qcom_hibern8_notify(struct ufs_hba *hba,
>>>>>> +				    enum uic_cmd_dme uic_cmd,
>>>>>> +				    enum ufs_notify_change_status status)
>>>>>> +{
>>>>>> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>>>>> +
>>>>>> +	/* Apply shared ICE WA */
>>>>>
>>>>> Are you really sure it is *shared ICE*?
>>>>
>>>>  Yes Manivannan, I am.
>>>>
>>>
>>> Well, there are two kind of registers defined in the internal doc that I can
>>> see: UFS_MEM_ICE and UFS_MEM_SHARED_ICE. And hence the question.
>>>
>>>>>
>>>>>> +	if (uic_cmd == UIC_CMD_DME_HIBER_EXIT &&
>>>>>> +	    status == POST_CHANGE &&
>>>>>> +	    host->hw_ver.major == 0x5 &&
>>>>>> +	    host->hw_ver.minor == 0x0 &&
>>>>>> +	    host->hw_ver.step == 0x0 &&
>>>>>> +	    host->vdd_hba_pc) {
>>>>>> +		host->vdd_hba_pc = false;
>>>>>> +		ufshcd_writel(hba, 0x18, UFS_MEM_ICE);
>>>>>
>>>>> Define the actual bits instead of writing magic values.
>>>>
>>>> Sure.
>>>>
>>>>>
>>>>>> +		ufshcd_readl(hba, UFS_MEM_ICE);
>>>>>> +		ufshcd_writel(hba, 0x0, UFS_MEM_ICE);
>>>>>> +		ufshcd_readl(hba, UFS_MEM_ICE);
>>>>>
>>>>> Why do you need readl()? Writes to device memory won't get reordered.
>>>>
>>>> Since these are hardware register, there is a potential for reordering.
>>>>
>>>
>>> Really? Who said that? Please cite the reference.
>>>
>>>>>
>>>>>> +	}
>>>>>> +}
>>>>>> +
>>>>>>  static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
>>>>>>  {
>>>>>>  	if (host->dev_ref_clk_ctrl_mmio &&
>>>>>> @@ -2258,6 +2281,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
>>>>>>  	.hce_enable_notify      = ufs_qcom_hce_enable_notify,
>>>>>>  	.link_startup_notify    = ufs_qcom_link_startup_notify,
>>>>>>  	.pwr_change_notify	= ufs_qcom_pwr_change_notify,
>>>>>> +	.hibern8_notify		= ufs_qcom_hibern8_notify,
>>>>>
>>>>> This callback is not called anywhere. Regardeless of that, why can't you use
>>>>> ufs_qcom_clk_scale_notify()?
>>>>>
>>>>
>>>> According to the HPG guidelines, as part of this workaround, we are required to reset the ICE controller during the Hibern8 exit sequence when the UFS controller resumes from power collapse. Therefore, this reset logic has been added to the H8 exit notifier callback.
>>>>
>>>
>>> Please wrap the replies to 80 column.
>>>
>>> Well, we do call ufshcd_uic_hibern8_exit() from these callbacks. So why can't
>>> you reset the ICE after calling ufshcd_uic_hibern8_exit() here?
>>
>> As per HPG guidance, the ICE Reset workaround is required only after the
>> controller undergoes a power collapse. In the UFS subsystem, power collapse
>> is managed via the GDSC (GCC_UFS_MEM_PHY_GDSC), which is part of GenPD
>> (power domains). Since GenPD is tied to runtime suspend operations, we are
>> setting the power collapse flag during runtime suspend and checking this
>> flag during hibernate exit.
>>
>>
>>>
>>>> The ufs_clk_scale_notify function is invoked whenever clock scaling (up or down) occurs, regardless of whether a power collapse has taken place. Hence, the ICE controller reset was specifically added to the H8 exit notifier to ensure it is executed only in the appropriate context.
>>>>
>>>
>>> Please define what 'power collapse' mean here. And as I said before, you are
>>> not at all calling this newly introduced callback.
>>
>> This is not a newly introduced callback, Mani. We are registering for
>> an already existing notifier. You may refer to ufshcd.c, where this
>> notifier is invoked.
>>
> 
> Okay, my bad. You could've mentioned something about this callback in the commit
> message to make others aware that you are reusing an existing callback.
> 
> - Mani
>