scsi: ufs: PM fixes Intel host controllers

PATCH Superseded Adrian Hunter 2 participants 6 messages Oct 22, 2025

Subsystems (2)

SCSI UFS host controller

lore

Share & tools

lore

Adrian Hunter

author

2025-10-22 18:08 UTC #0 lore raw

cover scsi: ufs: PM fixes Intel host controllers

Here are fixes related to power management on Intel host controllers, primarily ones based on Intel Alder Lake.

Patches are based on 6.18/scsi-fixes

Adrian Hunter (4):
scsi: ufs: ufs-pci: Fix S0ix/S3 for Intel controllers
scsi: ufs: core: Add a quirk to suppress link_startup_again
scsi: ufs: ufs-pci: Set UFSHCD_QUIRK_NO_LINK_STARTUP_AGAIN for Intel ADL
scsi: ufs: core: Fix invalid probe error return value

 drivers/ufs/core/ufshcd.c     |  7 +++--
 drivers/ufs/host/ufshcd-pci.c | 70 +++++++++++++++++++++++++++++++++++++++++--
 include/ufs/ufshcd.h          |  7 +++++
 3 files changed, 78 insertions(+), 6 deletions(-)

base-commit: d54c676d4fe0543d1642ab7a68ffdd31e8639a5d

Regards
Adrian

Adrian Hunter

author

2025-10-22 18:08 UTC #1 lore raw

PATCH 1/4 scsi: ufs: ufs-pci: Fix S0ix/S3 for Intel controllers

Intel platforms with UFS, can support Suspend-to-Idle (S0ix) and Suspend-to-RAM (S3). For S0ix the link state should be HIBERNATE. For S3, state is lost, so the link state must be OFF. Driver policy, expressed by spm_lvl, can be 3 (link HIBERNATE, device SLEEP) for S0ix but must be changed to 5 (link OFF, device POWEROFF) for S3.

Fix support for S0ix/S3 by switching spm_lvl as needed. During suspend ->prepare(), if the suspend target state is not Suspend-to-Idle, ensure the spm_lvl is at least 5 to ensure that resume will be possible from deep sleep states. During suspend ->complete(), restore the spm_lvl to its original value that is suitable for S0ix.

This fix is first needed in Intel Alder Lake based controllers.

Fixes 7dc9fb47bc9a ("scsi: ufs: ufs-pci: Add support for Intel ADL")

Cc [email protected]

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/ufs/host/ufshcd-pci.c | 67 +++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 2 deletions(-)

drivers/ufs/host/ufshcd-pci.c +65 -2

index b87e03777395..89f88b693850 100644
@@ -15,6 +15,7 @@
 #include <linux/pci.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm_qos.h>
+#include <linux/suspend.h>
 #include <linux/debugfs.h>
 #include <linux/uuid.h>
 #include <linux/acpi.h>

@@ -31,6 +32,7 @@ struct intel_host {
 	u32		dsm_fns;
 	u32		active_ltr;
 	u32		idle_ltr;
+	int		saved_spm_lvl;
 	struct dentry	*debugfs_root;
 	struct gpio_desc *reset_gpio;
 };

@@ -347,6 +349,7 @@ static int ufs_intel_common_init(struct ufs_hba *hba)
 	host = devm_kzalloc(hba->dev, sizeof(*host), GFP_KERNEL);
 	if (!host)
 		return -ENOMEM;
+	host->saved_spm_lvl = -1;
 	ufshcd_set_variant(hba, host);
 	intel_dsm_init(host, hba->dev);
 	if (INTEL_DSM_SUPPORTED(host, RESET)) {

@@ -538,6 +541,66 @@ static int ufshcd_pci_restore(struct device *dev)
 
 	return ufshcd_system_resume(dev);
 }
+
+static int ufs_intel_suspend_prepare(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct intel_host *host = ufshcd_get_variant(hba);
+	int err;
+
+	/*
+	 * Only s2idle (S0ix) retains link state.  Force power-off
+	 * (UFS_PM_LVL_5) for any other case.
+	 */
+	if (pm_suspend_target_state != PM_SUSPEND_TO_IDLE && hba->spm_lvl < UFS_PM_LVL_5) {
+		host->saved_spm_lvl = hba->spm_lvl;
+		hba->spm_lvl = UFS_PM_LVL_5;
+	}
+
+	err = ufshcd_suspend_prepare(dev);
+
+	if (err < 0 && host->saved_spm_lvl != -1) {
+		hba->spm_lvl = host->saved_spm_lvl;
+		host->saved_spm_lvl = -1;
+	}
+
+	return err;
+}
+
+static void ufs_intel_resume_complete(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct intel_host *host = ufshcd_get_variant(hba);
+
+	ufshcd_resume_complete(dev);
+
+	if (host->saved_spm_lvl != -1) {
+		hba->spm_lvl = host->saved_spm_lvl;
+		host->saved_spm_lvl = -1;
+	}
+}
+
+static int ufshcd_pci_suspend_prepare(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	if (!strcmp(hba->vops->name, "intel-pci"))
+		return ufs_intel_suspend_prepare(dev);
+
+	return ufshcd_suspend_prepare(dev);
+}
+
+static void ufshcd_pci_resume_complete(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	if (!strcmp(hba->vops->name, "intel-pci")) {
+		ufs_intel_resume_complete(dev);
+		return;
+	}
+
+	ufshcd_resume_complete(dev);
+}
 #endif
 
 /**

@@ -611,8 +674,8 @@ static const struct dev_pm_ops ufshcd_pci_pm_ops = {
 	.thaw		= ufshcd_system_resume,
 	.poweroff	= ufshcd_system_suspend,
 	.restore	= ufshcd_pci_restore,
-	.prepare	= ufshcd_suspend_prepare,
-	.complete	= ufshcd_resume_complete,
+	.prepare	= ufshcd_pci_suspend_prepare,
+	.complete	= ufshcd_pci_resume_complete,
 #endif
 };

Signature · 1 line

2.48.1

Adrian Hunter

author

2025-10-22 18:08 UTC #2 lore raw

PATCH 2/4 scsi: ufs: core: Add a quirk to suppress link_startup_again

In reply to Adrian Hunter

ufshcd_link_startup() has a facility (link_startup_again) to issue DME_LINKSTARTUP a 2nd time even though the 1st time was successful.

Some older hardware benefits from that, however the behaviour is non-standard, and has been found to cause link startup to be unreliable for some Intel Alder Lake based host controllers.

Add UFSHCD_QUIRK_NO_LINK_STARTUP_AGAIN to suppress link_startup_again, in preparation for setting the quirk for affected controllers.

Fixes 7dc9fb47bc9a ("scsi: ufs: ufs-pci: Add support for Intel ADL")

Cc [email protected]

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/ufs/core/ufshcd.c | 3 ++-
 include/ufs/ufshcd.h      | 7 +++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

drivers/ufs/core/ufshcd.c +2 -1

index 9ca27de4767a..632b508d0edb 100644
@@ -5066,7 +5066,8 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
 	 * If UFS device isn't active then we will have to issue link startup
 	 * 2 times to make sure the device state move to active.
 	 */
-	if (!ufshcd_is_ufs_dev_active(hba))
+	if (!(hba->quirks & UFSHCD_QUIRK_NO_LINK_STARTUP_AGAIN) &&
+	    !ufshcd_is_ufs_dev_active(hba))
 		link_startup_again = true;
 
 link_startup:

include/ufs/ufshcd.h +7 -0

index 9425cfd9d00e..4157fbaebff6 100644
@@ -688,6 +688,13 @@ enum ufshcd_quirks {
 	 * single doorbell mode.
 	 */
 	UFSHCD_QUIRK_BROKEN_LSDBS_CAP			= 1 << 25,
+
+	/*
+	 * This quirk indicates that DME_LINKSTARTUP should not be issued a 2nd
+	 * time (refer link_startup_again) after the 1st time was successful,
+	 * because it causes link startup to become unreliable.
+	 */
+	UFSHCD_QUIRK_NO_LINK_STARTUP_AGAIN		= 1 << 26,
 };
 
 enum ufshcd_caps {

Signature · 1 line

2.48.1

Bart Van Assche

reviewer

2025-10-22 19:35 UTC #3 lore raw

Re: [PATCH 2/4] scsi: ufs: core: Add a quirk to suppress link_startup_again

include/ufs/ufshcd.h enum ufshcd_quirks { · line 697

Quoted 6 lines · Adrian Hunter

include/ufs/ufshcd.h +6 -0

+	/*
+	 * This quirk indicates that DME_LINKSTARTUP should not be issued a 2nd
+	 * time (refer link_startup_again) after the 1st time was successful,
+	 * because it causes link startup to become unreliable.
+	 */
+	UFSHCD_QUIRK_NO_LINK_STARTUP_AGAIN		= 1 << 26,

UFSHCD_QUIRK_PERFORM_LINK_STARTUP_ONCE might be a better name since it doesn't include a negation.

Thanks,

Bart.

Adrian Hunter

author

2025-10-22 18:08 UTC #4 lore raw

PATCH 3/4 scsi: ufs: ufs-pci: Set UFSHCD_QUIRK_NO_LINK_STARTUP_AGAIN for Intel ADL

In reply to Adrian Hunter

Link startup becomes unreliable for Intel Alder Lake based host controllers when a 2nd DME_LINKSTARTUP is issued unnecessarily. Employ UFSHCD_QUIRK_NO_LINK_STARTUP_AGAIN to suppress that from happening.

Fixes 7dc9fb47bc9a ("scsi: ufs: ufs-pci: Add support for Intel ADL")

Cc [email protected]

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/ufs/host/ufshcd-pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/ufs/host/ufshcd-pci.c +2 -1

index 89f88b693850..171a3cc58512 100644
@@ -428,7 +428,8 @@ static int ufs_intel_lkf_init(struct ufs_hba *hba)
 static int ufs_intel_adl_init(struct ufs_hba *hba)
 {
 	hba->nop_out_timeout = 200;
-	hba->quirks |= UFSHCD_QUIRK_BROKEN_AUTO_HIBERN8;
+	hba->quirks |= UFSHCD_QUIRK_BROKEN_AUTO_HIBERN8 |
+		       UFSHCD_QUIRK_NO_LINK_STARTUP_AGAIN;
 	hba->caps |= UFSHCD_CAP_WB_EN;
 	return ufs_intel_common_init(hba);
 }

Signature · 1 line

2.48.1

Adrian Hunter

author

2025-10-22 18:08 UTC #5 lore raw

PATCH 4/4 scsi: ufs: core: Fix invalid probe error return value

In reply to Adrian Hunter

After DME Link Startup, the error return value is set to the MIPI UniPro GenericErrorCode which can be 0 (SUCCESS) or 1 (FAILURE). Upon failure during driver probe, the error code 1 is propagated back to the driver probe function which must return a negative value to indicate an error, but 1 is not negative, so the probe is considered to be successful even though it failed. Subsequently, removing the driver results in an oops because it is not in a valid state.

This happens because none of the callers of ufshcd_init() expect a non-negative error code.

Fix the return value and documentation to match actual usage.

Fixes 69f5eb78d4b0 ("scsi: ufs: core: Move the ufshcd_device_init(hba, true) call")

Cc [email protected]

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/ufs/core/ufshcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/ufs/core/ufshcd.c +2 -2

index 632b508d0edb..eafff8cfb2b6 100644
@@ -10662,7 +10662,7 @@ static int ufshcd_add_scsi_host(struct ufs_hba *hba)
  * @mmio_base: base register address
  * @irq: Interrupt line of device
  *
- * Return: 0 on success, non-zero value on failure.
+ * Return: 0 on success; < 0 on failure.
  */
 int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 {

@@ -10903,7 +10903,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	hba->is_irq_enabled = false;
 	ufshcd_hba_exit(hba);
 out_error:
-	return err;
+	return err > 0 ? -EIO : err;
 }
 EXPORT_SYMBOL_GPL(ufshcd_init);

Signature · 1 line

2.48.1