* [PATCH v1] ufs: introcude UFSHCD_QUIRK_GET_VS_RESULT quirk
@ 2016-11-08 7:48 Kiwoong Kim
2016-11-08 19:16 ` Subhash Jadavani
0 siblings, 1 reply; 4+ messages in thread
From: Kiwoong Kim @ 2016-11-08 7:48 UTC (permalink / raw)
To: James E.J. Bottomley, linux-scsi, Martin K. Petersen,
vinholikatti
Cc: 추헌광
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);
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,
+};
+
/**
* 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)
+
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,
+ 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 */
--
2.1.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1] ufs: introcude UFSHCD_QUIRK_GET_VS_RESULT quirk
2016-11-08 7:48 [PATCH v1] ufs: introcude UFSHCD_QUIRK_GET_VS_RESULT quirk Kiwoong Kim
@ 2016-11-08 19:16 ` Subhash Jadavani
2016-11-09 6:46 ` Kiwoong Kim
0 siblings, 1 reply; 4+ messages in thread
From: Subhash Jadavani @ 2016-11-08 19:16 UTC (permalink / raw)
To: Kiwoong Kim
Cc: James E.J. Bottomley, linux-scsi, Martin K. Petersen,
vinholikatti, 추헌광, linux-scsi-owner
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH v1] ufs: introcude UFSHCD_QUIRK_GET_VS_RESULT quirk
2016-11-08 19:16 ` Subhash Jadavani
@ 2016-11-09 6:46 ` Kiwoong Kim
2016-11-09 19:04 ` Subhash Jadavani
0 siblings, 1 reply; 4+ messages in thread
From: Kiwoong Kim @ 2016-11-09 6:46 UTC (permalink / raw)
To: 'Subhash Jadavani'
Cc: 'James E.J. Bottomley', linux-scsi,
'Martin K. Petersen', vinholikatti, '???',
linux-scsi-owner, cpgs
> > 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.
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] ufs: introcude UFSHCD_QUIRK_GET_VS_RESULT quirk
2016-11-09 6:46 ` Kiwoong Kim
@ 2016-11-09 19:04 ` Subhash Jadavani
0 siblings, 0 replies; 4+ messages in thread
From: Subhash Jadavani @ 2016-11-09 19:04 UTC (permalink / raw)
To: Kiwoong Kim
Cc: 'James E.J. Bottomley', linux-scsi,
'Martin K. Petersen', vinholikatti, '???',
linux-scsi-owner, cpgs
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-11-09 19:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-08 7:48 [PATCH v1] ufs: introcude UFSHCD_QUIRK_GET_VS_RESULT quirk Kiwoong Kim
2016-11-08 19:16 ` Subhash Jadavani
2016-11-09 6:46 ` Kiwoong Kim
2016-11-09 19:04 ` Subhash Jadavani
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).