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

[Shawn Lin <shawn.lin@rock-chips.com>]

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)