Re: [PATCH v1] ufs: introcude UFSHCD_QUIRK_GET_VS_RESULT quirk

[Subhash Jadavani <subhashj@codeaurora.org>]

On 2016-11-08 22:46, 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.
> 
> Okay, I'll apply what you said.
> 
>> 
>> >
>> >  	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).
> 
> As you guessed, this patch is to get some information
> by using vendor specific ways directly, which was got from standard
> UFSHCI SFRs for some hardware problems.
> 
> If other controllers have similar problems, but the problems are 
> acutally
> another cases, you should add the function call - 
> ufshcd_vops_get_vs_info
> - wherever you want to use this interface.
> 
> This patch is intended to cover all results of link startup, hibern8 
> and
> power mode change. Because I don't want to add an additional interface
> whenever a new controller has a new problem similar with what I 
> mentioned
> above.
> 
> Anyway, could you give me your opinion about it?
> Or, if you don't agree applying something just to fix some simple 
> problems,
> such as reading SFRs, I won't update this patch any more.


VS_OP_XY is not at all readable and not generic enough. If you can 
define some generic names for it, we should be ok with this change.

> 
> Thank you.
> 
>> 
>> >  /**
>> >   * 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 ?
> 
> Okay, I'll change the name with proper one.
> 
>> 
>> > +
>> >  	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() ?
> 
> Same with above.
> 
>> 
>> > +					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
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" 
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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