Re: [PATCH v1] ufs: introcude UFSHCD_QUIRK_GET_VS_RESULT quirk

[Subhash Jadavani <subhashj@codeaurora.org>]

On 2016-11-07 23:48, Kiwoong Kim wrote:
> Some UFS host controllers may not report a result of
> UIC command completion properly.
> In such cases, they should get the result from UIC directly
> if their architectures support that.
> 
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 27 +++++++++++++++++++++++----
>  drivers/scsi/ufs/ufshcd.h | 20 ++++++++++++++++++++
>  2 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d4a5a9c..8e19631 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1011,7 +1011,10 @@ static inline bool
> ufshcd_ready_for_uic_cmd(struct ufs_hba *hba)
>   */
>  static inline u8 ufshcd_get_upmcrs(struct ufs_hba *hba)
>  {
> -	return (ufshcd_readl(hba, REG_CONTROLLER_STATUS) >> 8) & 0x7;
> +	if (hba->quirks & UFSHCD_QUIRK_GET_VS_RESULT)
> +		return (u8)ufshcd_vops_get_vs_info(hba, VS_OP_03);
> +	else
> +		return (ufshcd_readl(hba, REG_CONTROLLER_STATUS) >> 8) & 0x7;
>  }
> 
>  /**
> @@ -1038,6 +1041,17 @@ ufshcd_dispatch_uic_cmd(struct ufs_hba *hba,
> struct uic_command *uic_cmd)
>  		      REG_UIC_COMMAND);
>  }
> 
> +static inline enum vs_opcode ufshcd_get_vs_opcode(struct uic_command 
> *uic_cmd)
> +{
> +	if (uic_cmd->command == UIC_CMD_DME_LINK_STARTUP)
> +		return VS_OP_00;
> +	else if ((uic_cmd->command == UIC_CMD_DME_HIBER_ENTER))
> +		return VS_OP_01;
> +	else if ((uic_cmd->command == UIC_CMD_DME_HIBER_EXIT))
> +		return VS_OP_02;
> +	else
> +		return VS_OP_INVALID;
> +}
>  /**
>   * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
>   * @hba: per adapter instance
> @@ -1051,11 +1065,16 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba,
> struct uic_command *uic_cmd)
>  {
>  	int ret;
>  	unsigned long flags;
> +	enum vs_opcode vs_op = ufshcd_get_vs_opcode(uic_cmd);

Please move this after we check the quirk below.

> 
>  	if (wait_for_completion_timeout(&uic_cmd->done,
> -					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
> -		ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
> -	else
> +					msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
> +		if (hba->quirks & UFSHCD_QUIRK_GET_VS_RESULT &&
> +					vs_op != VS_OP_INVALID)
> +			ret = ufshcd_vops_get_vs_info(hba, vs_op);
> +		else
> +			ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
> +	} else
>  		ret = -ETIMEDOUT;
> 
>  	spin_lock_irqsave(hba->host->host_lock, flags);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 13504b4..8cf3991 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -245,6 +245,14 @@ struct ufs_pwr_mode_info {
>  	struct ufs_pa_layer_attr info;
>  };
> 
> +enum vs_opcode {
> +	VS_OP_00 = 0x00,
> +	VS_OP_01,
> +	VS_OP_02,
> +	VS_OP_03,
> +	VS_OP_INVALID = 0xFF,
> +};
> +

How do we interpret these codes? and how is this useful for non-exynos 
UFS host controller? Please make it generic which can be used by other 
controllers in future (if required).

>  /**
>   * struct ufs_hba_variant_ops - variant specific callbacks
>   * @name: variant name
> @@ -297,6 +305,7 @@ struct ufs_hba_variant_ops {
>  	int     (*resume)(struct ufs_hba *, enum ufs_pm_op);
>  	void	(*dbg_register_dump)(struct ufs_hba *hba);
>  	int	(*phy_initialization)(struct ufs_hba *);
> +	int	(*get_vs_info)(struct ufs_hba *hba, enum vs_opcode);
>  };
> 
>  /* clock gating state  */
> @@ -484,6 +493,8 @@ struct ufs_hba {
>  	 */
>  	#define UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION		UFS_BIT(5)
> 
> +	#define UFSHCD_QUIRK_GET_VS_RESULT			UFS_BIT(6)

Name is weird, can we name it to reflect the actual purpose of this 
quirk? something like UFSHCD_QUIRK_BROKEN_UIC_COMPL ?

> +
>  	unsigned int quirks;	/* Deviations from standard UFSHCI spec. */
> 
>  	/* Device deviations from standard UFS device spec. */
> @@ -853,4 +864,13 @@ static inline void
> ufshcd_vops_dbg_register_dump(struct ufs_hba *hba)
>  		hba->vops->dbg_register_dump(hba);
>  }
> 
> +static inline int ufshcd_vops_get_vs_info(struct ufs_hba *hba,


You may name it as *get_uic_coml_info() ?

> +					enum vs_opcode op)
> +{
> +	if (hba->vops && hba->vops->get_vs_info)
> +		return hba->vops->get_vs_info(hba, op);
> +
> +	return -ENOTSUPP;
> +}
> +
>  #endif /* End of Header */

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project