Message-ID: <edrf3bobjnknwydzeitfwns7lehgf65p5prcohmc7eexhzoami@ywlamyweunmn>
Date: Mon, 11 Aug 2025 13:46:55 +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 v1] ufs: ufs-qcom: Align programming sequence of Shared
 ICE for UFS controller v5

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

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

Powered by blists - more mailing lists