Re: [PATCH V1 2/3] ufs: ufs-qcom: Align programming sequence for UFS controller v6.2

On 1/22/2026 8:39 PM, Konrad Dybcio wrote:
> On 1/22/26 3:13 PM, Nitin Rawat wrote:
>> UFS controller v6.2 requires bit 31 in the spare configuration register
>> to be set for high-speed link startup mode, as per the Hardware
>> Programming Guide (HPG).

Hi Konrad,

I've revived this patch. Sorry for the delayed response. Thanks for your 
previous comments on this patch.

> 
> Please stick a "Qualcomm" before mentioning UFS controller v6.2, I
> don't think that is immediately obvious without looking at the code..
> 
>> The spare register value is read during host driver initialization but
>> gets cleared after UFS reset. To align with the UFS v6.2 programming
>> sequence, preserve the spare register value during initialization and
>> restore it during link startup to ensure proper high-speed mode
> 
> I believe you're supposed to write the value yourself, depending on the
> state of the controller, it's 0 at reset.

The link startup mode (HS LSS - high-speed link startup, or LS LSS - 
low-speed link startup) is decided during the boot stage based on a 
bootconfig GPIO. This selection is carried forward through the secondary 
stage bootloaders and finally to HLOS via this register.

This register is only configured by the bootloader — no kernel code 
writes other fields to it. The kernel reads it during initialization to 
capture the configured link startup mode, and restores it after UFS 
reset (which clears the register to 0) so that the bootloader's 
selection is preserved for link startup.



> 
>> Signed-off-by: Nitin Rawat <[email protected]>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 11 ++++++++---
>>   drivers/ufs/host/ufs-qcom.h |  1 +
>>   2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index c43bb75d208c..ab5aed241913 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -686,6 +686,7 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, bool is_pre_scale_up, unsign
>>   static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
>>   					enum ufs_notify_change_status status)
>>   {
>> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>   	int err = 0;
>>
>>   	switch (status) {
>> @@ -708,6 +709,10 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
>>   		 */
>>   		err = ufshcd_disable_host_tx_lcc(hba);
>>
>> +		/* Update REG_UFS_DEBUG_SPARE_CFG to set HS-LSS mode in link startup */
> 
> "HS/LS"?

I've Change wording ("HS/LS" instead of "HS-LSS") in next patchset.

> 
>> +		if (host->hw_ver.major == 0x6 && host->hw_ver.minor == 0x2)
>> +			ufshcd_writel(hba, host->spare_cfg,
>> +				      REG_UFS_DEBUG_SPARE_CFG);
> 
> Is that a "only on v6.2", or "starting with v6.2"?

I've Change version check from "only v6.2" to "starting with v6.2" in 
next patchset.


> 
> Also, I see that this register has more than just this one field, with
> the previous question in mind, I think a rmw would be desired here

I'm using ufshcd_writel since this register is only configured by the 
bootloader and no kernel code writes other fields to it, ufshcd_writel 
is sufficient as there are no kernel written bits to preserve via rmw.

Thanks,
Nitin

> 
> Konrad