Re: [PATCH 1/6] scsi: ufs: ufs-qcom: Setup host power mode during init

[Manivannan Sadhasivam <mani@kernel.org>]

On Sun, Sep 10, 2023 at 10:59:22PM -0700, Can Guo wrote:
> Setup host power mode and its limitations during UFS host driver init to
> avoid repetitive work during every power mode change.
> 
> Co-developed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 27 ++++++++++++++++++---------
>  drivers/ufs/host/ufs-qcom.h |  1 +
>  2 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index c3215d3..710f079 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -443,7 +443,11 @@ static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba)
>  static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> +	struct ufs_dev_params *host_pwr_cap = &host->host_pwr_cap;
>  	struct phy *phy = host->generic_phy;
> +	enum phy_mode mode = host_pwr_cap->hs_rate == PA_HS_MODE_B ?
> +							PHY_MODE_UFS_HS_B :
> +							PHY_MODE_UFS_HS_A;

I do not see anyone passing PA_HS_MODE_A in this patch, so this change is not
required now. If you are doing this as a preparatory work, please do it in a
separate patch.

>  	int ret;
>  
>  	/* Reset UFS Host Controller and PHY */
> @@ -460,7 +464,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>  		return ret;
>  	}
>  
> -	phy_set_mode_ext(phy, PHY_MODE_UFS_HS_B, host->phy_gear);
> +	phy_set_mode_ext(phy, mode, host->phy_gear);

Same as above.

>  
>  	/* power on phy - start serdes and phy's power and clocks */
>  	ret = phy_power_on(phy);
> @@ -884,7 +888,6 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
>  				struct ufs_pa_layer_attr *dev_req_params)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> -	struct ufs_dev_params ufs_qcom_cap;
>  	int ret = 0;
>  
>  	if (!dev_req_params) {
> @@ -894,13 +897,7 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
>  
>  	switch (status) {
>  	case PRE_CHANGE:
> -		ufshcd_init_pwr_dev_param(&ufs_qcom_cap);
> -		ufs_qcom_cap.hs_rate = UFS_QCOM_LIMIT_HS_RATE;
> -
> -		/* This driver only supports symmetic gear setting i.e., hs_tx_gear == hs_rx_gear */
> -		ufs_qcom_cap.hs_tx_gear = ufs_qcom_cap.hs_rx_gear = ufs_qcom_get_hs_gear(hba);
> -
> -		ret = ufshcd_get_pwr_dev_param(&ufs_qcom_cap,
> +		ret = ufshcd_get_pwr_dev_param(&host->host_pwr_cap,
>  					       dev_max_params,
>  					       dev_req_params);
>  		if (ret) {
> @@ -1037,6 +1034,17 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba)
>  		hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH;
>  }
>  
> +static void ufs_qcom_set_pwr_mode_limits(struct ufs_hba *hba)

It's good that you are moving the setting to init() as they are static, but I'm
worried about the different naming conventions used all over the place.

The intention here is to set host parameters and then get the agreed one between
host and the device. But different names are being used. The structure itself is
named as "ufs_dev_params" even though it targets host and the vendor drivers are
naming it as "ufs_<vendor>_cap" or "host_cap". And now you've given a new name,
"host_pwr_cap", which makes things even worse.

So we should rename the struct itself as "ufs_host_params" and all the vendor
drivers should stick to "host_params".

If you do not want to do it in this series, please use existing "host_cap",
since you are embedding it within "ufs_qcom_host" struct.

Also, it'd be good to make this change for other vendor drivers as well (moving
to init()).

- Mani

> +{
> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> +	struct ufs_dev_params *host_pwr_cap = &host->host_pwr_cap;
> +
> +	ufshcd_init_pwr_dev_param(host_pwr_cap);
> +
> +	/* This driver only supports symmetic gear setting i.e., hs_tx_gear == hs_rx_gear */
> +	host_pwr_cap->hs_tx_gear = host_pwr_cap->hs_rx_gear = ufs_qcom_get_hs_gear(hba);
> +}
> +
>  static void ufs_qcom_set_caps(struct ufs_hba *hba)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> @@ -1259,6 +1267,7 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>  	if (err)
>  		goto out_variant_clear;
>  
> +	ufs_qcom_set_pwr_mode_limits(hba);
>  	ufs_qcom_set_caps(hba);
>  	ufs_qcom_advertise_quirks(hba);
>  
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index fa54248..4db64d9 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -227,6 +227,7 @@ struct ufs_qcom_host {
>  
>  	struct gpio_desc *device_reset;
>  
> +	struct ufs_dev_params host_pwr_cap;
>  	u32 phy_gear;
>  
>  	bool esi_enabled;
> -- 
> 2.7.4
> 

-- 
மணிவண்ணன் சதாசிவம்