[PATCH] firmware: xilinx: ufs: move PHY/SRAM ready polling into the firmware backend

[PATCH] firmware: xilinx: ufs: move PHY/SRAM ready polling into the firmware backend

[Michal Simek]

The Versal Gen 2 UFS driver polls the firmware for M-PHY TX/RX
configuration readiness and SRAM initialisation completion with two
open-coded do/while loops. Each iteration is a full firmware round-trip
(PM_IOCTL/IOCTL_READ_REG of a protected PMC_IOU_SLCR register), so the
loop can issue up to a million EEMI calls, and it hard-codes the wait
policy inside the controller driver.

Introduce coarse blocking helpers, zynqmp_pm_wait_mphy_tx_rx_config_ready()
and zynqmp_pm_wait_sram_init_done(), that take a caller-supplied timeout
budget and contain the poll loop. The loop is EEMI-specific (legacy
firmware only exposes the per-read status primitive) so it lives in the
firmware driver, keeping the UFS driver backend-agnostic: a future
backend can offload the wait to the platform in a single call without
touching the controller driver again. The existing per-read primitives stay
exported, so the current EEMI interface is unchanged.

The timeout budget remains owned by the UFS driver (the consumer that
knows the hardware) and is passed down, so EEMI and any future backend
stay consistent.

Signed-off-by: Michal Simek <michal.simek@amd.com>
---

 drivers/firmware/xilinx/zynqmp-ufs.c     | 70 ++++++++++++++++++++++--
 drivers/ufs/host/ufs-amd-versal2.c       | 46 ++++------------
 include/linux/firmware/xlnx-zynqmp-ufs.h |  8 +--
 3 files changed, 82 insertions(+), 42 deletions(-)

diff --git a/drivers/firmware/xilinx/zynqmp-ufs.c b/drivers/firmware/xilinx/zynqmp-ufs.c
index 85da8a822f3a..81ccf61a037c 100644
--- a/drivers/firmware/xilinx/zynqmp-ufs.c
+++ b/drivers/firmware/xilinx/zynqmp-ufs.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2025 Advanced Micro Devices, Inc.
  */
 
+#include <linux/delay.h>
 #include <linux/firmware/xlnx-zynqmp.h>
 #include <linux/module.h>
 
@@ -33,7 +34,7 @@
  *
  * Return:	Returns 0 on success or error value on failure.
  */
-int zynqmp_pm_is_mphy_tx_rx_config_ready(bool *is_ready)
+static int zynqmp_pm_is_mphy_tx_rx_config_ready(bool *is_ready)
 {
 	u32 regval;
 	int ret;
@@ -53,7 +54,6 @@ int zynqmp_pm_is_mphy_tx_rx_config_ready(bool *is_ready)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(zynqmp_pm_is_mphy_tx_rx_config_ready);
 
 /**
  * zynqmp_pm_is_sram_init_done - check SRAM initialization
@@ -61,7 +61,7 @@ EXPORT_SYMBOL_GPL(zynqmp_pm_is_mphy_tx_rx_config_ready);
  *
  * Return:	Returns 0 on success or error value on failure.
  */
-int zynqmp_pm_is_sram_init_done(bool *is_done)
+static int zynqmp_pm_is_sram_init_done(bool *is_done)
 {
 	u32 regval;
 	int ret;
@@ -81,7 +81,69 @@ int zynqmp_pm_is_sram_init_done(bool *is_done)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(zynqmp_pm_is_sram_init_done);
+
+/**
+ * zynqmp_pm_wait_mphy_tx_rx_config_ready - wait for M-PHY TX-RX config ready
+ * @timeout_us:	Caller-supplied timeout budget in microseconds
+ *
+ * Poll the M-PHY TX-RX configuration-ready status until it settles or the
+ * timeout elapses. The poll loop is EEMI-specific (legacy firmware only offers
+ * the per-read status primitive), so it lives here in the firmware driver
+ * rather than in the UFS controller driver; an SCMI-based backend can instead
+ * offload the wait to the platform in a single call. The timeout budget is
+ * owned by the caller (UFS driver), keeping the policy with the consumer.
+ *
+ * Return:	Returns 0 once ready, -ETIMEDOUT on timeout, or error value.
+ */
+int zynqmp_pm_wait_mphy_tx_rx_config_ready(u32 timeout_us)
+{
+	bool is_ready;
+	int ret;
+
+	while (timeout_us--) {
+		ret = zynqmp_pm_is_mphy_tx_rx_config_ready(&is_ready);
+		if (ret)
+			return ret;
+
+		if (!is_ready)
+			return 0;
+
+		usleep_range(1, 5);
+	}
+
+	return -ETIMEDOUT;
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_wait_mphy_tx_rx_config_ready);
+
+/**
+ * zynqmp_pm_wait_sram_init_done - wait for SRAM initialization to complete
+ * @timeout_us:	Caller-supplied timeout budget in microseconds
+ *
+ * Poll the SRAM initialization-done status until it is set or the timeout
+ * elapses. As with the M-PHY wait, the poll loop is EEMI-specific and kept in
+ * the firmware driver so the UFS controller driver stays backend-agnostic.
+ *
+ * Return:	Returns 0 once done, -ETIMEDOUT on timeout, or error value.
+ */
+int zynqmp_pm_wait_sram_init_done(u32 timeout_us)
+{
+	bool is_done;
+	int ret;
+
+	while (timeout_us--) {
+		ret = zynqmp_pm_is_sram_init_done(&is_done);
+		if (ret)
+			return ret;
+
+		if (is_done)
+			return 0;
+
+		usleep_range(1, 5);
+	}
+
+	return -ETIMEDOUT;
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_wait_sram_init_done);
 
 /**
  * zynqmp_pm_set_sram_bypass - Set SRAM bypass Control
diff --git a/drivers/ufs/host/ufs-amd-versal2.c b/drivers/ufs/host/ufs-amd-versal2.c
index 2154d6286817..dff0c2c95486 100644
--- a/drivers/ufs/host/ufs-amd-versal2.c
+++ b/drivers/ufs/host/ufs-amd-versal2.c
@@ -225,8 +225,6 @@ static int ufs_versal2_setup_phy(struct ufs_hba *hba)
 static int ufs_versal2_phy_init(struct ufs_hba *hba)
 {
 	struct ufs_versal2_host *host = ufshcd_get_variant(hba);
-	u32 time_left;
-	bool is_ready;
 	int ret;
 	static const struct ufshcd_dme_attr_val rmmi_attrs[] = {
 		{ UIC_ARG_MIB(CBREFCLKCTRL2), CBREFREFCLK_GATE_OVR_EN, DME_LOCAL },
@@ -235,23 +233,15 @@ static int ufs_versal2_phy_init(struct ufs_hba *hba)
 		{ UIC_ARG_MIB(VS_MPHYCFGUPDT), 1, DME_LOCAL }
 	};
 
-	/* Wait for Tx/Rx config_rdy */
-	time_left = TIMEOUT_MICROSEC;
-	do {
-		time_left--;
-		ret = zynqmp_pm_is_mphy_tx_rx_config_ready(&is_ready);
-		if (ret)
-			return ret;
-
-		if (!is_ready)
-			break;
-
-		usleep_range(1, 5);
-	} while (time_left);
-
-	if (!time_left) {
+	/*
+	 * Wait for Tx/Rx config_rdy. The poll loop lives in the firmware
+	 * backend (EEMI today, SCMI in future) so this driver stays
+	 * backend-agnostic; the timeout budget stays here with the consumer.
+	 */
+	ret = zynqmp_pm_wait_mphy_tx_rx_config_ready(TIMEOUT_MICROSEC);
+	if (ret) {
 		dev_err(hba->dev, "Tx/Rx configuration signal busy.\n");
-		return -ETIMEDOUT;
+		return ret;
 	}
 
 	ret = ufshcd_dwc_dme_set_attrs(hba, rmmi_attrs, ARRAY_SIZE(rmmi_attrs));
@@ -264,23 +254,11 @@ static int ufs_versal2_phy_init(struct ufs_hba *hba)
 		return ret;
 	}
 
-	/* Wait for SRAM init done */
-	time_left = TIMEOUT_MICROSEC;
-	do {
-		time_left--;
-		ret = zynqmp_pm_is_sram_init_done(&is_ready);
-		if (ret)
-			return ret;
-
-		if (is_ready)
-			break;
-
-		usleep_range(1, 5);
-	} while (time_left);
-
-	if (!time_left) {
+	/* Wait for SRAM init done (poll handled by the firmware backend). */
+	ret = zynqmp_pm_wait_sram_init_done(TIMEOUT_MICROSEC);
+	if (ret) {
 		dev_err(hba->dev, "SRAM initialization failed.\n");
-		return -ETIMEDOUT;
+		return ret;
 	}
 
 	ret = ufs_versal2_setup_phy(hba);
diff --git a/include/linux/firmware/xlnx-zynqmp-ufs.h b/include/linux/firmware/xlnx-zynqmp-ufs.h
index d3538dd5822a..00383dd835f2 100644
--- a/include/linux/firmware/xlnx-zynqmp-ufs.h
+++ b/include/linux/firmware/xlnx-zynqmp-ufs.h
@@ -9,17 +9,17 @@
 #define __FIRMWARE_XLNX_ZYNQMP_UFS_H__
 
 #if IS_REACHABLE(CONFIG_ZYNQMP_FIRMWARE)
-int zynqmp_pm_is_mphy_tx_rx_config_ready(bool *is_ready);
-int zynqmp_pm_is_sram_init_done(bool *is_done);
+int zynqmp_pm_wait_mphy_tx_rx_config_ready(u32 timeout_us);
+int zynqmp_pm_wait_sram_init_done(u32 timeout_us);
 int zynqmp_pm_set_sram_bypass(void);
 int zynqmp_pm_get_ufs_calibration_values(u32 *val);
 #else
-static inline int zynqmp_pm_is_mphy_tx_rx_config_ready(bool *is_ready)
+static inline int zynqmp_pm_wait_mphy_tx_rx_config_ready(u32 timeout_us)
 {
 	return -ENODEV;
 }
 
-static inline int zynqmp_pm_is_sram_init_done(bool *is_done)
+static inline int zynqmp_pm_wait_sram_init_done(u32 timeout_us)
 {
 	return -ENODEV;
 }
---
base-commit: 848acc8ffe1b7cd5f1bf427b93069becfebc2c9d
branch: xnext/ufs

-- 
2.43.0

Re: [PATCH] firmware: xilinx: ufs: move PHY/SRAM ready polling into the firmware backend

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The patch makes per-read primitives static and un-exports them, contradicting the commit message claim that they stay exported.
- [Medium] The refactoring regresses error handling by grouping lower-level firmware transport failures under hardware-specific timeout error messages.
--

--- Patch [1]: [PATCH] firmware: xilinx: ufs: move PHY/SRAM ready polling into the firmware backend ---
commit e824b853b832156de7cffafd5b473161a8e6545d
Author: Michal Simek <michal.simek@amd.com>

firmware: xilinx: ufs: move PHY/SRAM ready polling into the firmware backend

This commit moves the open-coded do/while polling loops for UFS PHY and SRAM 
initialization into coarse blocking helpers in the firmware driver. It 
introduces zynqmp_pm_wait_mphy_tx_rx_config_ready() and 
zynqmp_pm_wait_sram_init_done() to keep the UFS controller backend-agnostic.

> Introduce coarse blocking helpers, zynqmp_pm_wait_mphy_tx_rx_config_ready()
> and zynqmp_pm_wait_sram_init_done(), that take a caller-supplied timeout
> budget and contain the poll loop. The loop is EEMI-specific (legacy
> firmware only exposes the per-read status primitive) so it lives in the
> firmware driver, keeping the UFS driver backend-agnostic: a future
> backend can offload the wait to the platform in a single call without
> touching the controller driver again. The existing per-read primitives stay
> exported, so the current EEMI interface is unchanged.

[Severity: Medium]
The commit message indicates that "The existing per-read primitives stay
exported, so the current EEMI interface is unchanged." However, the code
modifications below make these functions static and remove their prototypes.
Is it intentional to un-export these primitives, contrary to the commit
message?

> diff --git a/drivers/firmware/xilinx/zynqmp-ufs.c b/drivers/firmware/xilinx/zynqmp-ufs.c
> index 85da8a822f3a5..81ccf61a037ce 100644
> --- a/drivers/firmware/xilinx/zynqmp-ufs.c
> +++ b/drivers/firmware/xilinx/zynqmp-ufs.c

[ ... ]

> @@ -33,7 +34,7 @@
>   *
>   * Return:	Returns 0 on success or error value on failure.
>   */
> -int zynqmp_pm_is_mphy_tx_rx_config_ready(bool *is_ready)
> +static int zynqmp_pm_is_mphy_tx_rx_config_ready(bool *is_ready)
>  {
>  	u32 regval;
>  	int ret;
> @@ -53,7 +54,6 @@ int zynqmp_pm_is_mphy_tx_rx_config_ready(bool *is_ready)
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(zynqmp_pm_is_mphy_tx_rx_config_ready);

[ ... ]

> @@ -61,7 +61,7 @@ EXPORT_SYMBOL_GPL(zynqmp_pm_is_mphy_tx_rx_config_ready);
>   *
>   * Return:	Returns 0 on success or error value on failure.
>   */
> -int zynqmp_pm_is_sram_init_done(bool *is_done)
> +static int zynqmp_pm_is_sram_init_done(bool *is_done)
>  {
>  	u32 regval;
>  	int ret;
> @@ -81,7 +81,69 @@ int zynqmp_pm_is_sram_init_done(bool *is_done)
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(zynqmp_pm_is_sram_init_done);

[ ... ]

> diff --git a/drivers/ufs/host/ufs-amd-versal2.c b/drivers/ufs/host/ufs-amd-versal2.c
> index 2154d62868171..dff0c2c954865 100644
> --- a/drivers/ufs/host/ufs-amd-versal2.c
> +++ b/drivers/ufs/host/ufs-amd-versal2.c

[ ... ]

> @@ -235,23 +233,15 @@ static int ufs_versal2_phy_init(struct ufs_hba *hba)
>  		{ UIC_ARG_MIB(VS_MPHYCFGUPDT), 1, DME_LOCAL }
>  	};
>  
> -	/* Wait for Tx/Rx config_rdy */
> -	time_left = TIMEOUT_MICROSEC;
> -	do {
> -		time_left--;
> -		ret = zynqmp_pm_is_mphy_tx_rx_config_ready(&is_ready);
> -		if (ret)
> -			return ret;
> -
> -		if (!is_ready)
> -			break;
> -
> -		usleep_range(1, 5);
> -	} while (time_left);
> -
> -	if (!time_left) {
> +	/*
> +	 * Wait for Tx/Rx config_rdy. The poll loop lives in the firmware
> +	 * backend (EEMI today, SCMI in future) so this driver stays
> +	 * backend-agnostic; the timeout budget stays here with the consumer.
> +	 */
> +	ret = zynqmp_pm_wait_mphy_tx_rx_config_ready(TIMEOUT_MICROSEC);
> +	if (ret) {
>  		dev_err(hba->dev, "Tx/Rx configuration signal busy.\n");
> -		return -ETIMEDOUT;
> +		return ret;
>  	}

[Severity: Medium]
Will this change result in misleading error logs if a firmware transport
error occurs?

In the original code, if zynqmp_pm_is_mphy_tx_rx_config_ready() failed with a
transport error like -EIO, the function returned silently. The
"Tx/Rx configuration signal busy" error was only logged when the timeout
actually expired.

With this update, if zynqmp_pm_wait_mphy_tx_rx_config_ready() returns -EIO,
it seems we will incorrectly log "Tx/Rx configuration signal busy." instead
of propagating the transport failure silently.

>  	ret = ufshcd_dwc_dme_set_attrs(hba, rmmi_attrs, ARRAY_SIZE(rmmi_attrs));
> @@ -264,23 +254,11 @@ static int ufs_versal2_phy_init(struct ufs_hba *hba)
>  		return ret;
>  	}
>  
> -	/* Wait for SRAM init done */
> -	time_left = TIMEOUT_MICROSEC;
> -	do {
> -		time_left--;
> -		ret = zynqmp_pm_is_sram_init_done(&is_ready);
> -		if (ret)
> -			return ret;
> -
> -		if (is_ready)
> -			break;
> -
> -		usleep_range(1, 5);
> -	} while (time_left);
> -
> -	if (!time_left) {
> +	/* Wait for SRAM init done (poll handled by the firmware backend). */
> +	ret = zynqmp_pm_wait_sram_init_done(TIMEOUT_MICROSEC);
> +	if (ret) {
>  		dev_err(hba->dev, "SRAM initialization failed.\n");
> -		return -ETIMEDOUT;
> +		return ret;
>  	}

[Severity: Medium]
Similarly, if zynqmp_pm_wait_sram_init_done() returns a generic firmware
transport error, will it now misleadingly print "SRAM initialization failed."
rather than bubbling up the underlying error?

[ ... ]

> diff --git a/include/linux/firmware/xlnx-zynqmp-ufs.h b/include/linux/firmware/xlnx-zynqmp-ufs.h
> index d3538dd5822a5..00383dd835f24 100644
> --- a/include/linux/firmware/xlnx-zynqmp-ufs.h
> +++ b/include/linux/firmware/xlnx-zynqmp-ufs.h
> @@ -9,17 +9,17 @@
>  #define __FIRMWARE_XLNX_ZYNQMP_UFS_H__
>  
>  #if IS_REACHABLE(CONFIG_ZYNQMP_FIRMWARE)
> -int zynqmp_pm_is_mphy_tx_rx_config_ready(bool *is_ready);
> -int zynqmp_pm_is_sram_init_done(bool *is_done);

[Severity: Medium]
Is it intentional to remove these prototypes from the public header, despite
the commit message mentioning that the per-read primitives stay exported?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/eaeaaea8ed76069943e0706a0917139ff6569929.1785855749.git.michal.simek@amd.com?part=1