[PATCH V2 0/2] Add post change sequence for link start notify

[PATCH V2 0/2] Add post change sequence for link start notify

[palash.kambar]

From: Palash Kambar <palash.kambar@oss.qualcomm.com>

This patch series introduces two updates to the UFS subsystem aimed at
improving link stability and power efficiency on platforms using the
Qualcomm UFS host controller.

During link startup, the number of connected TX/RX lanes discovered may be
fewer than the lanes specified in the device tree. The current UFS core
driver configures all DT-defined lanes unconditionally, which can lead to
mismatches during power mode changes. Patch 1/2 ensures to fail on this.

Additionally, certain Qualcomm platforms support Auto Hibern8 (AH8), where
the UFS controller autonomously de-asserts clk_req signals to the GCC
during Hibern8 state. Enabling this mechanism allows the clock controller
to gate unused clocks, providing meaningful power savings. Patch 2/2 adds
support for enabling this feature as recommended by the Hardware
Programming Guidelines.

---
changes from V1
1) Addressed Shawn Lin's comments to fix comment to connected lanes.
2) Addressed Bart's comments to remove warning and trigger failure 
   incase of lane mismatch.
---

Palash Kambar (2):
  ufs: core: Configure only active lanes during link
  ufs: ufs-qcom: Enable Auto Hibern8 clock request support

 drivers/ufs/core/ufshcd.c   | 39 +++++++++++++++++++++++++++++++++++++
 drivers/ufs/host/ufs-qcom.c | 11 +++++++++++
 drivers/ufs/host/ufs-qcom.h | 11 +++++++++++
 3 files changed, 61 insertions(+)

-- 
2.34.1

[PATCH V2 1/2] ufs: core: Configure only active lanes during link

[palash.kambar]

From: Palash Kambar <palash.kambar@oss.qualcomm.com>

The number of connected lanes detected during UFS link startup can be
fewer than the lanes specified in the device tree. The current driver
logic attempts to configure all lanes defined in the device tree,
regardless of their actual availability. This mismatch may cause
failures during power mode changes.

Hence, add check to identify only the lanes that were successfully
discovered during link startup, to warn on power mode change errors
caused by mismatched lane counts.

Signed-off-by: Palash Kambar <palash.kambar@oss.qualcomm.com>
---
 drivers/ufs/core/ufshcd.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 31950fc51a4c..cc291cae79f0 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5035,6 +5035,40 @@ void ufshcd_update_evt_hist(struct ufs_hba *hba, u32 id, u32 val)
 }
 EXPORT_SYMBOL_GPL(ufshcd_update_evt_hist);
 
+static int ufshcd_validate_link_params(struct ufs_hba *hba)
+{
+	int ret = 0;
+	int val = 0;
+
+	ret = ufshcd_dme_get(hba,
+			     UIC_ARG_MIB(PA_CONNECTEDTXDATALANES), &val);
+	if (ret)
+		goto out;
+
+	if (val != hba->lanes_per_direction) {
+		dev_err(hba->dev, "Tx lane mismatch [config,reported] [%d,%d]\n",
+			hba->lanes_per_direction, val);
+		ret = -ENOLINK;
+		goto out;
+	}
+
+	val = 0;
+
+	ret = ufshcd_dme_get(hba,
+			     UIC_ARG_MIB(PA_CONNECTEDRXDATALANES), &val);
+	if (ret)
+		goto out;
+
+	if (val != hba->lanes_per_direction) {
+		dev_err(hba->dev, "Rx lane mismatch [config,reported] [%d,%d]\n",
+			hba->lanes_per_direction, val);
+		ret = -ENOLINK;
+	}
+
+out:
+	return ret;
+}
+
 /**
  * ufshcd_link_startup - Initialize unipro link startup
  * @hba: per adapter instance
@@ -5108,6 +5142,11 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
 			goto out;
 	}
 
+	/* Check successfully detected lanes */
+	ret = ufshcd_validate_link_params(hba);
+	if (ret)
+		goto out;
+
 	/* Include any host controller configuration via UIC commands */
 	ret = ufshcd_vops_link_startup_notify(hba, POST_CHANGE);
 	if (ret)
-- 
2.34.1

Re: [PATCH V2 1/2] ufs: core: Configure only active lanes during link

[Shawn Lin]

Hi Palash

在 2026/03/27 星期五 17:03, palash.kambar@oss.qualcomm.com 写道:
> From: Palash Kambar <palash.kambar@oss.qualcomm.com>
> 
> The number of connected lanes detected during UFS link startup can be
> fewer than the lanes specified in the device tree. The current driver
> logic attempts to configure all lanes defined in the device tree,
> regardless of their actual availability. This mismatch may cause
> failures during power mode changes.
> 
> Hence, add check to identify only the lanes that were successfully
> discovered during link startup, to warn on power mode change errors
> caused by mismatched lane counts.

The logic of your patch is clear, but I believe there is a slight
inconsistency between the commit message and the current code
implementation. The patch currently returns -ENOLINK immediately when a
lane mismatch is detected. This causes the Link Startup process to
terminate instantly, preventing the UFS device from completing
initialization. Consequently, ufshcd_change_power_mode() will never be
executed, there is nothing about warning on power mode change errors.

How about "to prevents potential failures in subsequent power mode
changes by failing the initialization early"  or something similart?

> 
> Signed-off-by: Palash Kambar <palash.kambar@oss.qualcomm.com>
> ---
>   drivers/ufs/core/ufshcd.c | 39 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 31950fc51a4c..cc291cae79f0 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5035,6 +5035,40 @@ void ufshcd_update_evt_hist(struct ufs_hba *hba, u32 id, u32 val)
>   }
>   EXPORT_SYMBOL_GPL(ufshcd_update_evt_hist);
>   
> +static int ufshcd_validate_link_params(struct ufs_hba *hba)
> +{
> +	int ret = 0;
> +	int val = 0;
> +
> +	ret = ufshcd_dme_get(hba,
> +			     UIC_ARG_MIB(PA_CONNECTEDTXDATALANES), &val);
> +	if (ret)
> +		goto out;
> +
> +	if (val != hba->lanes_per_direction) {
> +		dev_err(hba->dev, "Tx lane mismatch [config,reported] [%d,%d]\n",
> +			hba->lanes_per_direction, val);
> +		ret = -ENOLINK;
> +		goto out;
> +	}
> +
> +	val = 0;
> +

ufshcd_dme_get() returns 0 on success, non-zero value on failure.
Perhaps you could remove this "val = 0".

> +	ret = ufshcd_dme_get(hba,
> +			     UIC_ARG_MIB(PA_CONNECTEDRXDATALANES), &val);
> +	if (ret)
> +		goto out;
> +
> +	if (val != hba->lanes_per_direction) {
> +		dev_err(hba->dev, "Rx lane mismatch [config,reported] [%d,%d]\n",
> +			hba->lanes_per_direction, val);
> +		ret = -ENOLINK;
> +	}
> +
> +out:
> +	return ret;
> +}
> +
>   /**
>    * ufshcd_link_startup - Initialize unipro link startup
>    * @hba: per adapter instance
> @@ -5108,6 +5142,11 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
>   			goto out;
>   	}
>   
> +	/* Check successfully detected lanes */
> +	ret = ufshcd_validate_link_params(hba);
> +	if (ret)
> +		goto out;
> +
>   	/* Include any host controller configuration via UIC commands */
>   	ret = ufshcd_vops_link_startup_notify(hba, POST_CHANGE);
>   	if (ret)

Re: [PATCH V2 1/2] ufs: core: Configure only active lanes during link

[Palash Kambar]

On 3/27/2026 3:01 PM, Shawn Lin wrote:
> Hi Palash
> 
> 在 2026/03/27 星期五 17:03, palash.kambar@oss.qualcomm.com 写道:
>> From: Palash Kambar <palash.kambar@oss.qualcomm.com>
>>
>> The number of connected lanes detected during UFS link startup can be
>> fewer than the lanes specified in the device tree. The current driver
>> logic attempts to configure all lanes defined in the device tree,
>> regardless of their actual availability. This mismatch may cause
>> failures during power mode changes.
>>
>> Hence, add check to identify only the lanes that were successfully
>> discovered during link startup, to warn on power mode change errors
>> caused by mismatched lane counts.
> 
> The logic of your patch is clear, but I believe there is a slight
> inconsistency between the commit message and the current code
> implementation. The patch currently returns -ENOLINK immediately when a
> lane mismatch is detected. This causes the Link Startup process to
> terminate instantly, preventing the UFS device from completing
> initialization. Consequently, ufshcd_change_power_mode() will never be
> executed, there is nothing about warning on power mode change errors.
> 
> How about "to prevents potential failures in subsequent power mode
> changes by failing the initialization early"  or something similart?
> 

Sure Shawn, will update the commit text.

>>
>> Signed-off-by: Palash Kambar <palash.kambar@oss.qualcomm.com>
>> ---
>>   drivers/ufs/core/ufshcd.c | 39 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 31950fc51a4c..cc291cae79f0 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -5035,6 +5035,40 @@ void ufshcd_update_evt_hist(struct ufs_hba *hba, u32 id, u32 val)
>>   }
>>   EXPORT_SYMBOL_GPL(ufshcd_update_evt_hist);
>>   +static int ufshcd_validate_link_params(struct ufs_hba *hba)
>> +{
>> +    int ret = 0;
>> +    int val = 0;
>> +
>> +    ret = ufshcd_dme_get(hba,
>> +                 UIC_ARG_MIB(PA_CONNECTEDTXDATALANES), &val);
>> +    if (ret)
>> +        goto out;
>> +
>> +    if (val != hba->lanes_per_direction) {
>> +        dev_err(hba->dev, "Tx lane mismatch [config,reported] [%d,%d]\n",
>> +            hba->lanes_per_direction, val);
>> +        ret = -ENOLINK;
>> +        goto out;
>> +    }
>> +
>> +    val = 0;
>> +
> 
> ufshcd_dme_get() returns 0 on success, non-zero value on failure.
> Perhaps you could remove this "val = 0".
>

Hi Shawn, the "val" used here holds the value of attribute returned
I have used "ret" to hold the return from ufshcd_dme_get()

 
>> +    ret = ufshcd_dme_get(hba,
>> +                 UIC_ARG_MIB(PA_CONNECTEDRXDATALANES), &val);
>> +    if (ret)
>> +        goto out;
>> +
>> +    if (val != hba->lanes_per_direction) {
>> +        dev_err(hba->dev, "Rx lane mismatch [config,reported] [%d,%d]\n",
>> +            hba->lanes_per_direction, val);
>> +        ret = -ENOLINK;
>> +    }
>> +
>> +out:
>> +    return ret;
>> +}
>> +
>>   /**
>>    * ufshcd_link_startup - Initialize unipro link startup
>>    * @hba: per adapter instance
>> @@ -5108,6 +5142,11 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
>>               goto out;
>>       }
>>   +    /* Check successfully detected lanes */
>> +    ret = ufshcd_validate_link_params(hba);
>> +    if (ret)
>> +        goto out;
>> +
>>       /* Include any host controller configuration via UIC commands */
>>       ret = ufshcd_vops_link_startup_notify(hba, POST_CHANGE);
>>       if (ret)

Re: [PATCH V2 1/2] ufs: core: Configure only active lanes during link

[Bart Van Assche]

On 3/27/26 2:03 AM, palash.kambar@oss.qualcomm.com wrote:
> +static int ufshcd_validate_link_params(struct ufs_hba *hba)
> +{
> +	int ret = 0;
> +	int val = 0;

Both initializers are superfluous. Please remove at least the
initializer for "ret" since the first statement in this function assigns
a value to "ret".

> +	ret = ufshcd_dme_get(hba,
> +			     UIC_ARG_MIB(PA_CONNECTEDTXDATALANES), &val);

The formatting of the above statement does not follow the Linux kernel
coding style. Please format it as follows:

	ret = ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES),
			     &val);

A possible alternative to formatting code manually is to run something
like "git clang-format HEAD^" from the command line.

> +	val = 0;

This assignment is superfluous, isn't it?

> +	ret = ufshcd_dme_get(hba,
> +			     UIC_ARG_MIB(PA_CONNECTEDRXDATALANES), &val);

Please move the UIC_ARG_MIB() to the previous line.

Thanks,

Bart.

Re: [PATCH V2 1/2] ufs: core: Configure only active lanes during link

[Palash Kambar]

On 3/28/2026 2:47 AM, Bart Van Assche wrote:
> On 3/27/26 2:03 AM, palash.kambar@oss.qualcomm.com wrote:
>> +static int ufshcd_validate_link_params(struct ufs_hba *hba)
>> +{
>> +    int ret = 0;
>> +    int val = 0;
> 
> Both initializers are superfluous. Please remove at least the
> initializer for "ret" since the first statement in this function assigns
> a value to "ret".
> 
>> +    ret = ufshcd_dme_get(hba,
>> +                 UIC_ARG_MIB(PA_CONNECTEDTXDATALANES), &val);
> 
> The formatting of the above statement does not follow the Linux kernel
> coding style. Please format it as follows:
> 
>     ret = ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES),
>                  &val);
> 
> A possible alternative to formatting code manually is to run something
> like "git clang-format HEAD^" from the command line.
> 
>> +    val = 0;
> 
> This assignment is superfluous, isn't it?
> 
>> +    ret = ufshcd_dme_get(hba,
>> +                 UIC_ARG_MIB(PA_CONNECTEDRXDATALANES), &val);
> 
> Please move the UIC_ARG_MIB() to the previous line.
> 
> Thanks,
> 
> Bart.

Sure Bart, will address these comments.

[PATCH V2 2/2] ufs: ufs-qcom: Enable Auto Hibern8 clock request support

[palash.kambar]

From: Palash Kambar <palash.kambar@oss.qualcomm.com>

On platforms that support Auto Hibern8 (AH8), the UFS controller can
autonomously de-assert clk_req signals to the GCC when entering the
Hibern8 state. This allows GCC to gate unused clocks, improving
power efficiency.

Enable the Clock Request feature by setting the UFS_HW_CLK_CTRL_EN
bit in the UFS_AH8_CFG register, as recommended in the Hardware
Programming Guidelines.

Signed-off-by: Palash Kambar <palash.kambar@oss.qualcomm.com>
---
 drivers/ufs/host/ufs-qcom.c | 11 +++++++++++
 drivers/ufs/host/ufs-qcom.h | 11 +++++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 8ebee0cc5313..0e653b34b00d 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -683,6 +683,14 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, bool is_pre_scale_up, unsign
 	return 0;
 }
 
+static void ufs_qcom_link_startup_post_change(struct ufs_hba *hba)
+{
+	if (ufshcd_is_auto_hibern8_supported(hba)) {
+		ufshcd_rmwl(hba, UFS_HW_CLK_CTRL_EN, UFS_HW_CLK_CTRL_EN,
+			    UFS_AH8_CFG);
+	}
+}
+
 static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 					enum ufs_notify_change_status status)
 {
@@ -708,6 +716,9 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 		 */
 		err = ufshcd_disable_host_tx_lcc(hba);
 
+		break;
+	case POST_CHANGE:
+		ufs_qcom_link_startup_post_change(hba);
 		break;
 	default:
 		break;
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 380d02333d38..f19def37c86f 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -228,6 +228,17 @@ enum {
  */
 #define NUM_TX_R1W1 13
 
+/* bit definitions for UFS_AH8_CFG register */
+#define CC_UFS_SYS_CLK_REQ_EN          BIT(2)
+#define CC_UFS_ICE_CORE_CLK_REQ_EN     BIT(3)
+#define CC_UFS_UNIPRO_CORE_CLK_REQ_EN  BIT(4)
+#define CC_UFS_AUXCLK_REQ_EN           BIT(5)
+
+#define UFS_HW_CLK_CTRL_EN	(CC_UFS_SYS_CLK_REQ_EN |\
+				CC_UFS_ICE_CORE_CLK_REQ_EN |\
+				CC_UFS_UNIPRO_CORE_CLK_REQ_EN |\
+				CC_UFS_AUXCLK_REQ_EN)
+
 static inline void
 ufs_qcom_get_controller_revision(struct ufs_hba *hba,
 				 u8 *major, u16 *minor, u16 *step)
-- 
2.34.1

Re: [PATCH V2 2/2] ufs: ufs-qcom: Enable Auto Hibern8 clock request support

[Bart Van Assche]

On 3/27/26 2:03 AM, palash.kambar@oss.qualcomm.com wrote:
> On platforms that support Auto Hibern8 (AH8), the UFS controller can
> autonomously de-assert clk_req signals to the GCC when entering the
> Hibern8 state. This allows GCC to gate unused clocks, improving
> power efficiency.

What is the "GCC"? Please expand acronyms that are neither defined in
the UFSHCI standard nor in the UFS standard.

> +static void ufs_qcom_link_startup_post_change(struct ufs_hba *hba)
> +{
> +	if (ufshcd_is_auto_hibern8_supported(hba)) {
> +		ufshcd_rmwl(hba, UFS_HW_CLK_CTRL_EN, UFS_HW_CLK_CTRL_EN,
> +			    UFS_AH8_CFG);
> +	}
> +}

 From the Linux kernel coding style "Do not unnecessarily use braces 
where a single statement will do." Please follow that rule.

Thanks,

Bart.

Re: [PATCH V2 2/2] ufs: ufs-qcom: Enable Auto Hibern8 clock request support

[Palash Kambar]

On 3/28/2026 2:52 AM, Bart Van Assche wrote:
> On 3/27/26 2:03 AM, palash.kambar@oss.qualcomm.com wrote:
>> On platforms that support Auto Hibern8 (AH8), the UFS controller can
>> autonomously de-assert clk_req signals to the GCC when entering the
>> Hibern8 state. This allows GCC to gate unused clocks, improving
>> power efficiency.
> 
> What is the "GCC"? Please expand acronyms that are neither defined in
> the UFSHCI standard nor in the UFS standard.
> 
>> +static void ufs_qcom_link_startup_post_change(struct ufs_hba *hba)
>> +{
>> +    if (ufshcd_is_auto_hibern8_supported(hba)) {
>> +        ufshcd_rmwl(hba, UFS_HW_CLK_CTRL_EN, UFS_HW_CLK_CTRL_EN,
>> +                UFS_AH8_CFG);
>> +    }
>> +}
> 
> From the Linux kernel coding style "Do not unnecessarily use braces where a single statement will do." Please follow that rule.
> 
> Thanks,
> 
> Bart.


Sure Bart, will address these comments.