Re: [PATCH v3 03/10] ufs: host: Add common Hibern8 TX FSM polling helper

[Frank Li <Frank.li@oss.nxp.com>]

On Wed, Sep 16, 2026 at 10:44:04AM +0200, Larisa Grigore wrote:
> Factor out the common logic used to poll TX_FSM_STATE until the TX lanes
> enter Hibern8 into ufshcd_check_hibern8().
>
> The HiSilicon and Qualcomm drivers currently implement similar checks
> using the UniPro TX_FSM_STATE attribute after link/PHY bring-up. Move
> this logic to a common helper in ufshcd so it can be shared across host
> controller drivers.
>
> Switch the HiSilicon and Qualcomm drivers to use the new helper and drop
> their local implementations. While at it, normalize the "TX lane failed to
> reach Hibern8" error path to return -ETIMEDOUT instead of the raw
> TX_FSM_STATE value (qcom) or -1 (hisi), so all callers get a proper errno.
> This is an intentional, harmless change of the returned error value; the
> pass/fail behaviour at the call sites is unchanged.
>
> Unlike the initial implementation, the timeout error is now reported only
> once by ufshcd_dme_check_tx_hibern8() after its final check, instead of
> per lane.
>
> This also prepares for reusing the same UniPro-specific Hibern8 check in
> a subsequent commit.
>
> Signed-off-by: Larisa Grigore <larisa.grigore@oss.nxp.com>
> ---
>  drivers/ufs/core/ufshcd.c   | 86 +++++++++++++++++++++++++++++++++++++
>  drivers/ufs/host/ufs-hisi.c | 48 +--------------------
>  drivers/ufs/host/ufs-qcom.c | 42 +-----------------
>  include/ufs/ufshcd.h        |  3 ++
>  4 files changed, 91 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 2ba244cf40ac..f34b7fe54a16 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -4448,6 +4448,92 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
>  }
>  EXPORT_SYMBOL_GPL(ufshcd_dme_get_attr);
>
> +/**
> + * ufshcd_poll_tx_hibern8_lanes - Check TX_FSM_STATE of all TX lanes once
> + * @hba: host controller instance
> + * @num_lanes: number of TX lanes to check
> + *
> + * Read TX_FSM_STATE for every lane and verify it reached Hibern8.
> + *
> + * Return: 0 if all lanes are in Hibern8, -EAGAIN if any lane is not (yet)
> + * in Hibern8, or a negative errno (e.g. -ETIMEDOUT from a hard UIC command
> + * timeout) if the attribute read fails.
> + */
> +static int ufshcd_poll_tx_hibern8_lanes(struct ufs_hba *hba,
> +					unsigned int num_lanes)
> +{
> +	u32 tx_fsm_val = 0;
> +	unsigned int i;
> +	int err;
> +
> +	for (i = 0; i < num_lanes; i++) {
> +		err = ufshcd_dme_get(hba,
> +				UIC_ARG_MIB_SEL(TX_FSM_STATE,
> +					UIC_ARG_MPHY_TX_GEN_SEL_INDEX(i)),
> +				&tx_fsm_val);
> +		if (err) {
> +			dev_err(hba->dev,
> +				"%s: unable to get TX_FSM_STATE for lane %u, err %d\n",
> +				__func__, i, err);
> +			return err;
> +		}
> +
> +		if (tx_fsm_val != TX_STATE_HIBERN8)
> +			return -EAGAIN;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * ufshcd_dme_check_tx_hibern8 - Check if all TX lanes entered Hibern8 state
> + * @hba: host controller instance
> + * @num_lanes: number of TX lanes to check
> + * @timeout_ms: timeout in milliseconds for all lanes
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int ufshcd_dme_check_tx_hibern8(struct ufs_hba *hba, unsigned int num_lanes,
> +				unsigned int timeout_ms)
> +{
> +	unsigned long timeout;
> +	int err;
> +
> +	if (!num_lanes)
> +		return -EINVAL;
> +
> +	timeout = jiffies + msecs_to_jiffies(timeout_ms);
> +
> +	do {
> +		err = ufshcd_poll_tx_hibern8_lanes(hba, num_lanes);
> +		/*
> +		 * -EAGAIN means the lanes are simply not in HIBERN8 yet, so
> +		 * keep polling. Any other error is a real failure and must
> +		 * be returned immediately instead of being retried.
> +		 */
> +		if (err != -EAGAIN)
> +			return err;
> +
> +		/* sleep for max. 200us */
> +		usleep_range(100, 200);
> +	} while (time_before(jiffies, timeout));
> +
> +	/*
> +	 * We might have been scheduled out for long during polling, so do
> +	 * one final check before reporting timeout.
> +	 */
> +	err = ufshcd_poll_tx_hibern8_lanes(hba, num_lanes);
> +	if (err == -EAGAIN) {
> +		dev_err(hba->dev,
> +			"%s: timeout waiting for TX lanes to enter HIBERN8\n",
> +			__func__);
> +		err = -ETIMEDOUT;
> +	}


why not use read read_poll_timeout()

read_poll_timeout(ufshcd_poll_tx_hibern8_lanes, err, err != -EAGAIN, timeout_ms,
		  0, hba, numlanes);


Frank

> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_dme_check_tx_hibern8);
> +
>  /**
>   * ufshcd_dme_rmw - get modify set a DME attribute
>   * @hba: per adapter instance
> diff --git a/drivers/ufs/host/ufs-hisi.c b/drivers/ufs/host/ufs-hisi.c
> index bd223bda1ce2..b84075dfe00d 100644
> --- a/drivers/ufs/host/ufs-hisi.c
> +++ b/drivers/ufs/host/ufs-hisi.c
> @@ -22,50 +22,6 @@
>  #include <ufs/ufshci.h>
>  #include <ufs/ufs_quirks.h>
>
> -static int ufs_hisi_check_hibern8(struct ufs_hba *hba)
> -{
> -	int err = 0;
> -	u32 tx_fsm_val_0 = 0;
> -	u32 tx_fsm_val_1 = 0;
> -	unsigned long timeout = jiffies + msecs_to_jiffies(HBRN8_POLL_TOUT_MS);
> -
> -	do {
> -		err = ufshcd_dme_get(hba, UIC_ARG_MIB_SEL(TX_FSM_STATE, 0),
> -				      &tx_fsm_val_0);
> -		err |= ufshcd_dme_get(hba,
> -		    UIC_ARG_MIB_SEL(TX_FSM_STATE, 1), &tx_fsm_val_1);
> -		if (err || (tx_fsm_val_0 == TX_STATE_HIBERN8 &&
> -			tx_fsm_val_1 == TX_STATE_HIBERN8))
> -			break;
> -
> -		/* sleep for max. 200us */
> -		usleep_range(100, 200);
> -	} while (time_before(jiffies, timeout));
> -
> -	/*
> -	 * we might have scheduled out for long during polling so
> -	 * check the state again.
> -	 */
> -	if (time_after(jiffies, timeout)) {
> -		err = ufshcd_dme_get(hba, UIC_ARG_MIB_SEL(TX_FSM_STATE, 0),
> -				     &tx_fsm_val_0);
> -		err |= ufshcd_dme_get(hba,
> -		 UIC_ARG_MIB_SEL(TX_FSM_STATE, 1), &tx_fsm_val_1);
> -	}
> -
> -	if (err) {
> -		dev_err(hba->dev, "%s: unable to get TX_FSM_STATE, err %d\n",
> -			__func__, err);
> -	} else if (tx_fsm_val_0 != TX_STATE_HIBERN8 ||
> -			 tx_fsm_val_1 != TX_STATE_HIBERN8) {
> -		err = -1;
> -		dev_err(hba->dev, "%s: invalid TX_FSM_STATE, lane0 = %d, lane1 = %d\n",
> -			__func__, tx_fsm_val_0, tx_fsm_val_1);
> -	}
> -
> -	return err;
> -}
> -
>  static void ufs_hisi_clk_init(struct ufs_hba *hba)
>  {
>  	struct ufs_hisi_host *host = ufshcd_get_variant(hba);
> @@ -224,9 +180,7 @@ static int ufs_hisi_link_startup_pre_change(struct ufs_hba *hba)
>
>  	/* Unipro VS_mphy_disable */
>  	ufshcd_dme_set(hba, UIC_ARG_MIB_SEL(0xD0C1, 0x0), 0x0);
> -	err = ufs_hisi_check_hibern8(hba);
> -	if (err)
> -		dev_err(hba->dev, "ufs_hisi_check_hibern8 error\n");
> +	err = ufshcd_dme_check_tx_hibern8(hba, 2, HBRN8_POLL_TOUT_MS);
>
>  	if (!(host->caps & UFS_HISI_CAP_PHY10nm))
>  		ufshcd_writel(hba, UFS_HCLKDIV_NORMAL_VALUE, UFS_REG_HCLKDIV);
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 1e492dac8d93..815ca82a0878 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -382,46 +382,6 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
>  	return 0;
>  }
>
> -static int ufs_qcom_check_hibern8(struct ufs_hba *hba)
> -{
> -	int err;
> -	u32 tx_fsm_val;
> -	unsigned long timeout = jiffies + msecs_to_jiffies(HBRN8_POLL_TOUT_MS);
> -
> -	do {
> -		err = ufshcd_dme_get(hba,
> -				UIC_ARG_MIB_SEL(TX_FSM_STATE,
> -					UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)),
> -				&tx_fsm_val);
> -		if (err || tx_fsm_val == TX_STATE_HIBERN8)
> -			break;
> -
> -		/* sleep for max. 200us */
> -		usleep_range(100, 200);
> -	} while (time_before(jiffies, timeout));
> -
> -	/*
> -	 * we might have scheduled out for long during polling so
> -	 * check the state again.
> -	 */
> -	if (time_after(jiffies, timeout))
> -		err = ufshcd_dme_get(hba,
> -				UIC_ARG_MIB_SEL(TX_FSM_STATE,
> -					UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)),
> -				&tx_fsm_val);
> -
> -	if (err) {
> -		dev_err(hba->dev, "%s: unable to get TX_FSM_STATE, err %d\n",
> -				__func__, err);
> -	} else if (tx_fsm_val != TX_STATE_HIBERN8) {
> -		err = tx_fsm_val;
> -		dev_err(hba->dev, "%s: invalid TX_FSM_STATE = %d\n",
> -				__func__, err);
> -	}
> -
> -	return err;
> -}
> -
>  static void ufs_qcom_select_unipro_mode(struct ufs_qcom_host *host)
>  {
>  	ufshcd_rmwl(host->hba, QUNIPRO_SEL, QUNIPRO_SEL, REG_UFS_CFG1);
> @@ -607,7 +567,7 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
>  		break;
>  	case POST_CHANGE:
>  		/* check if UFS PHY moved from DISABLED to HIBERN8 */
> -		err = ufs_qcom_check_hibern8(hba);
> +		err = ufshcd_dme_check_tx_hibern8(hba, 1, HBRN8_POLL_TOUT_MS);
>  		ufs_qcom_enable_hw_clk_gating(hba);
>  		ufs_qcom_ice_enable(host);
>  		ufs_qcom_config_ice_allocator(host);
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index dfd302f2dc7c..0d9d0a26eb20 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -1556,6 +1556,9 @@ extern int ufshcd_dme_set_attr(struct ufs_hba *hba, u32 attr_sel,
>  			       u8 attr_set, u32 mib_val, u8 peer);
>  extern int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
>  			       u32 *mib_val, u8 peer);
> +extern int ufshcd_dme_check_tx_hibern8(struct ufs_hba *hba,
> +				       unsigned int num_lanes,
> +				       unsigned int timeout_ms);
>  extern int ufshcd_change_power_mode(struct ufs_hba *hba,
>  				    struct ufs_pa_layer_attr *pwr_mode,
>  				    enum ufshcd_pmc_policy pmc_policy);
> --
> 2.43.0
>
>