[RFC PATCH v3 0/2] ufs: support various values per device

[RFC PATCH v3 0/2] ufs: support various values per device

[Kiwoong Kim]

v2 -> v3
set default fdeviceinit timeout to 1 second
set the interval among query requests to check if it's cleared 5ms

v1 -> v2
change ufs_dev_values to const
remove macros to enhance readability

Kiwoong Kim (2):
  ufs: support various values per device
  ufs: change the way to complete fDeviceInit

 drivers/scsi/ufs/ufs_quirks.h | 13 ++++++++
 drivers/scsi/ufs/ufshcd.c     | 75 ++++++++++++++++++++++++++++++++++++-------
 drivers/scsi/ufs/ufshcd.h     |  1 +
 3 files changed, 77 insertions(+), 12 deletions(-)

-- 
2.7.4

[RFC PATCH v3 1/2] ufs: support various values per device

[Kiwoong Kim]

Respective UFS devices have their own characteristics and
many of them could be a form of numbers, such as timeout
and a number of retires. This introduces the way to set
those things per specific device vendor or specific device.

I wrote this like the style of ufs_fixups stuffs.

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
---
 drivers/scsi/ufs/ufs_quirks.h | 13 +++++++++++++
 drivers/scsi/ufs/ufshcd.c     | 39 +++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h     |  1 +
 3 files changed, 53 insertions(+)

diff --git a/drivers/scsi/ufs/ufs_quirks.h b/drivers/scsi/ufs/ufs_quirks.h
index 2a00414..f074093 100644
--- a/drivers/scsi/ufs/ufs_quirks.h
+++ b/drivers/scsi/ufs/ufs_quirks.h
@@ -29,6 +29,19 @@ struct ufs_dev_fix {
 	unsigned int quirk;
 };
 
+enum dev_val_type {
+	DEV_VAL_FDEVICEINIT	= 0x0,
+	DEV_VAL_NUM,
+};
+
+struct ufs_dev_value {
+	u16 wmanufacturerid;
+	u8 *model;
+	u32 key;
+	u32 val;
+	bool enable;
+};
+
 #define END_FIX { }
 
 /* add specific device quirk */
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 52abe82..b26f182 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -207,6 +207,21 @@ static struct ufs_dev_fix ufs_fixups[] = {
 	END_FIX
 };
 
+static const struct ufs_dev_value ufs_dev_values[] = {
+	{0, 0, 0, 0, false},
+};
+
+static inline bool
+ufs_get_dev_specific_value(struct ufs_hba *hba,
+			   enum dev_val_type type, u32 *val)
+{
+	if (!ufs_dev_values[type].enable)
+		return false;
+
+	*val = hba->dev_value[type];
+	return true;
+}
+
 static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba);
 static void ufshcd_async_scan(void *data, async_cookie_t cookie);
 static int ufshcd_reset_and_restore(struct ufs_hba *hba);
@@ -6923,11 +6938,35 @@ void ufshcd_fixup_dev_quirks(struct ufs_hba *hba, struct ufs_dev_fix *fixups)
 }
 EXPORT_SYMBOL_GPL(ufshcd_fixup_dev_quirks);
 
+static void ufshcd_set_dev_values(struct ufs_hba *hba,
+				  const struct ufs_dev_value *value)
+{
+	struct ufs_dev_value *f;
+	struct ufs_dev_info *dev_info = &hba->dev_info;
+
+	if (!value)
+		return;
+
+	for (f = (struct ufs_dev_value *)value; f->val; f++) {
+		if ((f->wmanufacturerid == dev_info->wmanufacturerid ||
+					f->wmanufacturerid == UFS_ANY_VENDOR) &&
+				((dev_info->model &&
+				  STR_PRFX_EQUAL(f->model, dev_info->model)) ||
+				 !strcmp(f->model, UFS_ANY_MODEL))) {
+			f->enable = true;
+			hba->dev_value[f->key] = f->val;
+		}
+	}
+}
+
 static void ufs_fixup_device_setup(struct ufs_hba *hba)
 {
 	/* fix by general quirk table */
 	ufshcd_fixup_dev_quirks(hba, ufs_fixups);
 
+	/* set device specific values */
+	ufshcd_set_dev_values(hba, ufs_dev_values);
+
 	/* allow vendors to fix quirks */
 	ufshcd_vops_fixup_dev_quirks(hba);
 }
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index c774012..f221ca7 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -670,6 +670,7 @@ struct ufs_hba {
 
 	/* Device deviations from standard UFS device spec. */
 	unsigned int dev_quirks;
+	u32 dev_value[DEV_VAL_NUM];
 
 	struct blk_mq_tag_set tmf_tag_set;
 	struct request_queue *tmf_queue;
-- 
2.7.4

RE: [RFC PATCH v3 1/2] ufs: support various values per device

[Avri Altman]

 
> 
> Respective UFS devices have their own characteristics and
> many of them could be a form of numbers, such as timeout
> and a number of retires. This introduces the way to set
> those things per specific device vendor or specific device.
> 
> I wrote this like the style of ufs_fixups stuffs.
> 
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
This patch legitimize quirks of all kinds and shapes.
I am not sure that we should allow it.


> ---
>  drivers/scsi/ufs/ufs_quirks.h | 13 +++++++++++++
>  drivers/scsi/ufs/ufshcd.c     | 39
> +++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.h     |  1 +
>  3 files changed, 53 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs_quirks.h b/drivers/scsi/ufs/ufs_quirks.h
> index 2a00414..f074093 100644
> --- a/drivers/scsi/ufs/ufs_quirks.h
> +++ b/drivers/scsi/ufs/ufs_quirks.h
> @@ -29,6 +29,19 @@ struct ufs_dev_fix {
>         unsigned int quirk;
>  };
> 
> +enum dev_val_type {
> +       DEV_VAL_FDEVICEINIT     = 0x0,

            /* keep last */
> +       DEV_VAL_NUM,
> +};
> +
> +struct ufs_dev_value {
> +       u16 wmanufacturerid;
> +       u8 *model;
> +       u32 key;
> +       u32 val;
> +       bool enable;
> +};
> +
>  #define END_FIX { }
> 
>  /* add specific device quirk */
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 52abe82..b26f182 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -207,6 +207,21 @@ static struct ufs_dev_fix ufs_fixups[] = {
>         END_FIX
>  };
> 
> +static const struct ufs_dev_value ufs_dev_values[] = {
> +       {0, 0, 0, 0, false},
> +};
> +
> +static inline bool
> +ufs_get_dev_specific_value(struct ufs_hba *hba,
> +                          enum dev_val_type type, u32 *val)
> +{
If (ARRAY_SIZE(ufs_dev_values) <= type)
    return false;


Thanks,
Avri

RE: [RFC PATCH v3 1/2] ufs: support various values per device

[Grant Jung]

> > Respective UFS devices have their own characteristics and many of them
> > could be a form of numbers, such as timeout and a number of retires.
> > This introduces the way to set those things per specific device vendor
> > or specific device.
> >
> > I wrote this like the style of ufs_fixups stuffs.
> >
> > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> This patch legitimize quirks of all kinds and shapes.
> I am not sure that we should allow it.
> 
> 
> > ---
> >  drivers/scsi/ufs/ufs_quirks.h | 13 +++++++++++++
> >  drivers/scsi/ufs/ufshcd.c     | 39
> > +++++++++++++++++++++++++++++++++++++++
> >  drivers/scsi/ufs/ufshcd.h     |  1 +
> >  3 files changed, 53 insertions(+)
> >
> > diff --git a/drivers/scsi/ufs/ufs_quirks.h
> > b/drivers/scsi/ufs/ufs_quirks.h index 2a00414..f074093 100644
> > --- a/drivers/scsi/ufs/ufs_quirks.h
> > +++ b/drivers/scsi/ufs/ufs_quirks.h
> > @@ -29,6 +29,19 @@ struct ufs_dev_fix {
> >         unsigned int quirk;
> >  };
> >
> > +enum dev_val_type {
> > +       DEV_VAL_FDEVICEINIT     = 0x0,
> 
>             /* keep last */
> > +       DEV_VAL_NUM,
> > +};
> > +
> > +struct ufs_dev_value {
> > +       u16 wmanufacturerid;
> > +       u8 *model;
> > +       u32 key;
> > +       u32 val;
> > +       bool enable;
> > +};
> > +
> >  #define END_FIX { }
> >
> >  /* add specific device quirk */
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 52abe82..b26f182 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -207,6 +207,21 @@ static struct ufs_dev_fix ufs_fixups[] = {
> >         END_FIX
> >  };
> >
> > +static const struct ufs_dev_value ufs_dev_values[] = {
> > +       {0, 0, 0, 0, false},
> > +};
> > +
> > +static inline bool
> > +ufs_get_dev_specific_value(struct ufs_hba *hba,
> > +                          enum dev_val_type type, u32 *val) {
> If (ARRAY_SIZE(ufs_dev_values) <= type)
>     return false;
> 
> 
> Thanks,
> Avri

There is no specification for fdeviceinit timeout value like eMMC CMD1 which is 1s.
Usually this value is small but can be increased under some abnormal situation like SPO(fdeviceinit after Sudden Power Off).
I think that 1000 retries take less than 1 second but it is inaccurate and not enough. Some UFS vendor wants 1.5s.
Moreover, the latency of resuming ufs driver can be dependent on this value when vcc and vccq is off during suspend.
So it's bad to set with big value to apply all UFS devices.
 
I wonder quirk is needed for that.

BR
Grant

RE: [RFC PATCH v3 1/2] ufs: support various values per device

[Kiwoong Kim]

> > Respective UFS devices have their own characteristics and many of them
> > could be a form of numbers, such as timeout and a number of retires.
> > This introduces the way to set those things per specific device vendor
> > or specific device.
> >
> > I wrote this like the style of ufs_fixups stuffs.
> >
> > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> This patch legitimize quirks of all kinds and shapes.
> I am not sure that we should allow it.
If you're concerning the name 'quirk' literally, I can change the way
to use another values by device tree or whatever.
Or do you concern introducing various values?

> 
> 
> > ---
> >  drivers/scsi/ufs/ufs_quirks.h | 13 +++++++++++++
> >  drivers/scsi/ufs/ufshcd.c     | 39
> > +++++++++++++++++++++++++++++++++++++++
> >  drivers/scsi/ufs/ufshcd.h     |  1 +
> >  3 files changed, 53 insertions(+)
> >
> > diff --git a/drivers/scsi/ufs/ufs_quirks.h
> > b/drivers/scsi/ufs/ufs_quirks.h index 2a00414..f074093 100644
> > --- a/drivers/scsi/ufs/ufs_quirks.h
> > +++ b/drivers/scsi/ufs/ufs_quirks.h
> > @@ -29,6 +29,19 @@ struct ufs_dev_fix {
> >         unsigned int quirk;
> >  };
> >
> > +enum dev_val_type {
> > +       DEV_VAL_FDEVICEINIT     = 0x0,
> 
>             /* keep last */
> > +       DEV_VAL_NUM,
> > +};
> > +
> > +struct ufs_dev_value {
> > +       u16 wmanufacturerid;
> > +       u8 *model;
> > +       u32 key;
> > +       u32 val;
> > +       bool enable;
> > +};
> > +
> >  #define END_FIX { }
> >
> >  /* add specific device quirk */
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 52abe82..b26f182 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -207,6 +207,21 @@ static struct ufs_dev_fix ufs_fixups[] = {
> >         END_FIX
> >  };
> >
> > +static const struct ufs_dev_value ufs_dev_values[] = {
> > +       {0, 0, 0, 0, false},
> > +};
> > +
> > +static inline bool
> > +ufs_get_dev_specific_value(struct ufs_hba *hba,
> > +                          enum dev_val_type type, u32 *val) {
> If (ARRAY_SIZE(ufs_dev_values) <= type)
>     return false;
> 
> 
> Thanks,
> Avri
Got it.

Thanks.
Kiwoong Kim

Re: [RFC PATCH v3 1/2] ufs: support various values per device

[Bart Van Assche]

On 2020-07-02 22:30, Kiwoong Kim wrote:
> +static const struct ufs_dev_value ufs_dev_values[] = {
> +	{0, 0, 0, 0, false},
> +};

A minor stylistic request: please change "{0, 0, 0, 0, false}" into "{
}". The C language requires that structure members that have not been
specified are zero-initialized.

Thanks,

Bart.

RE: [RFC PATCH v3 1/2] ufs: support various values per device

[Kiwoong Kim]

> On 2020-07-02 22:30, Kiwoong Kim wrote:
> > +static const struct ufs_dev_value ufs_dev_values[] = {
> > +	{0, 0, 0, 0, false},
> > +};
> 
> A minor stylistic request: please change "{0, 0, 0, 0, false}" into "{ }".
> The C language requires that structure members that have not been
> specified are zero-initialized.
> 
> Thanks,
> 
> Bart.

Got it and I experienced the tool chain to show warning messages for not specifying details in there.

Thanks.
Kiwoong Kim

Re: [RFC PATCH v3 1/2] ufs: support various values per device

[Bart Van Assche]

On 2020-07-05 23:43, Kiwoong Kim wrote:
>> On 2020-07-02 22:30, Kiwoong Kim wrote:
>>> +static const struct ufs_dev_value ufs_dev_values[] = {
>>> +	{0, 0, 0, 0, false},
>>> +};
>>
>> A minor stylistic request: please change "{0, 0, 0, 0, false}" into "{ }".
>> The C language requires that structure members that have not been
>> specified are zero-initialized.
> 
> Got it and I experienced the tool chain to show warning messages for not specifying details in there.

That's unexpected. Which warning was shown and which toolchain did
display that warning?

Thanks,

Bart.

[RFC PATCH v3 2/2] ufs: change the way to complete fDeviceInit

[Kiwoong Kim]

Currently, UFS driver checks if fDeviceInit
is cleared at several times, not period. This patch
is to wait its completion with the period, not retrying.
Many device vendors usually provides the specification on
it with just period, not a combination of a number of retrying
and period. So it could be proper to regard to the information
coming from device vendors.

I first added one device specific value regarding the information.

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
---
 drivers/scsi/ufs/ufshcd.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b26f182..6c08ed2 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -208,6 +208,7 @@ static struct ufs_dev_fix ufs_fixups[] = {
 };
 
 static const struct ufs_dev_value ufs_dev_values[] = {
+	{UFS_VENDOR_TOSHIBA, UFS_ANY_MODEL, DEV_VAL_FDEVICEINIT, 2000, false},
 	{0, 0, 0, 0, false},
 };
 
@@ -4162,9 +4163,12 @@ EXPORT_SYMBOL_GPL(ufshcd_config_pwr_mode);
  */
 static int ufshcd_complete_dev_init(struct ufs_hba *hba)
 {
-	int i;
+	u32 dev_init_compl_in_ms = 1000;
+	unsigned long timeout;
 	int err;
 	bool flag_res = true;
+	bool is_dev_val;
+	u32 val;
 
 	err = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
 		QUERY_FLAG_IDN_FDEVICEINIT, 0, NULL);
@@ -4175,20 +4179,28 @@ static int ufshcd_complete_dev_init(struct ufs_hba *hba)
 		goto out;
 	}
 
-	/* poll for max. 1000 iterations for fDeviceInit flag to clear */
-	for (i = 0; i < 1000 && !err && flag_res; i++)
-		err = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
-			QUERY_FLAG_IDN_FDEVICEINIT, 0, &flag_res);
+	/* Poll fDeviceInit flag to be cleared */
+	is_dev_val = ufs_get_dev_specific_value(hba, DEV_VAL_FDEVICEINIT, &val);
+	dev_init_compl_in_ms = (is_dev_val) ? val : 500;
+	timeout = jiffies + msecs_to_jiffies(dev_init_compl_in_ms);
+	do {
+		err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG,
+					QUERY_FLAG_IDN_FDEVICEINIT, 0, &flag_res);
+		if (!flag_res)
+			break;
+		usleep_range(5, 10);
+	} while (time_before(jiffies, timeout));
 
-	if (err)
+	if (err) {
 		dev_err(hba->dev,
-			"%s reading fDeviceInit flag failed with error %d\n",
-			__func__, err);
-	else if (flag_res)
+				"%s reading fDeviceInit flag failed with error %d\n",
+				__func__, err);
+	} else if (flag_res) {
 		dev_err(hba->dev,
-			"%s fDeviceInit was not cleared by the device\n",
-			__func__);
-
+				"%s fDeviceInit was not cleared by the device\n",
+				__func__);
+		err = -EBUSY;
+	}
 out:
 	return err;
 }
-- 
2.7.4

RE: [RFC PATCH v3 2/2] ufs: change the way to complete fDeviceInit

[Avri Altman]

 
> 
> Currently, UFS driver checks if fDeviceInit
> is cleared at several times, not period. This patch
> is to wait its completion with the period, not retrying.
> Many device vendors usually provides the specification on
> it with just period, not a combination of a number of retrying
> and period. So it could be proper to regard to the information
> coming from device vendors.
> 
> I first added one device specific value regarding the information.
> 
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
I still think that this patch alone is fine, and you don't need its predecessor.
The spec requires polling, so this is a form of a more-effective-polling: so be it.


> ---
>  drivers/scsi/ufs/ufshcd.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index b26f182..6c08ed2 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -208,6 +208,7 @@ static struct ufs_dev_fix ufs_fixups[] = {
>  };
> 
>  static const struct ufs_dev_value ufs_dev_values[] = {
> +       {UFS_VENDOR_TOSHIBA, UFS_ANY_MODEL, DEV_VAL_FDEVICEINIT, 2000,
> false},
>         {0, 0, 0, 0, false},
>  };
> 
> @@ -4162,9 +4163,12 @@ EXPORT_SYMBOL_GPL(ufshcd_config_pwr_mode);
>   */
>  static int ufshcd_complete_dev_init(struct ufs_hba *hba)
>  {
> -       int i;
> +       u32 dev_init_compl_in_ms = 1000;
> +       unsigned long timeout;
>         int err;
>         bool flag_res = true;
> +       bool is_dev_val;
> +       u32 val;
> 
>         err = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
>                 QUERY_FLAG_IDN_FDEVICEINIT, 0, NULL);
> @@ -4175,20 +4179,28 @@ static int ufshcd_complete_dev_init(struct
> ufs_hba *hba)
>                 goto out;
>         }
> 
> -       /* poll for max. 1000 iterations for fDeviceInit flag to clear */
> -       for (i = 0; i < 1000 && !err && flag_res; i++)
> -               err = ufshcd_query_flag_retry(hba,
> UPIU_QUERY_OPCODE_READ_FLAG,
> -                       QUERY_FLAG_IDN_FDEVICEINIT, 0, &flag_res);
> +       /* Poll fDeviceInit flag to be cleared */
> +       is_dev_val = ufs_get_dev_specific_value(hba, DEV_VAL_FDEVICEINIT,
> &val);
> +       dev_init_compl_in_ms = (is_dev_val) ? val : 500;
If you want dev_init_compl_in_ms to take its default 1,000, you should:
dev_init_compl_in_ms = (!is_dev_val) ? : val;

> +       timeout = jiffies + msecs_to_jiffies(dev_init_compl_in_ms);
> +       do {
> +               err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG,
> +                                       QUERY_FLAG_IDN_FDEVICEINIT, 0, &flag_res);
> +               if (!flag_res)
> +                       break;
> +               usleep_range(5, 10);
Per Grant's comment:
usleep_range(5000, 10000);

RE: [RFC PATCH v3 2/2] ufs: change the way to complete fDeviceInit

[Kiwoong Kim]

> > Currently, UFS driver checks if fDeviceInit is cleared at several
> > times, not period. This patch is to wait its completion with the
> > period, not retrying.
> > Many device vendors usually provides the specification on it with just
> > period, not a combination of a number of retrying and period. So it
> > could be proper to regard to the information coming from device
> > vendors.
> >
> > I first added one device specific value regarding the information.
> >
> > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> I still think that this patch alone is fine, and you don't need its
> predecessor.
> The spec requires polling, so this is a form of a more-effective-polling:
> so be it.

If what you're mentioning 'effective' means being able to just wait for
some long time upon completion of being cleared, it's not proper in real
products because fDeviceInit latency usually has the biggest overhead of
steps run during boot and some companies often try to manage its latency
as KPI. The method like a combination of retrying and small delay make them
harder to make it.
Or if I understand what you meant, please let me know.

> 
> 
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 36 ++++++++++++++++++++++++------------
> >  1 file changed, 24 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index b26f182..6c08ed2 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -208,6 +208,7 @@ static struct ufs_dev_fix ufs_fixups[] = {  };
> >
> >  static const struct ufs_dev_value ufs_dev_values[] = {
> > +       {UFS_VENDOR_TOSHIBA, UFS_ANY_MODEL, DEV_VAL_FDEVICEINIT, 2000,
> > false},
> >         {0, 0, 0, 0, false},
> >  };
> >
> > @@ -4162,9 +4163,12 @@ EXPORT_SYMBOL_GPL(ufshcd_config_pwr_mode);
> >   */
> >  static int ufshcd_complete_dev_init(struct ufs_hba *hba)  {
> > -       int i;
> > +       u32 dev_init_compl_in_ms = 1000;
> > +       unsigned long timeout;
> >         int err;
> >         bool flag_res = true;
> > +       bool is_dev_val;
> > +       u32 val;
> >
> >         err = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
> >                 QUERY_FLAG_IDN_FDEVICEINIT, 0, NULL); @@ -4175,20
> > +4179,28 @@ static int ufshcd_complete_dev_init(struct ufs_hba *hba)
> >                 goto out;
> >         }
> >
> > -       /* poll for max. 1000 iterations for fDeviceInit flag to clear */
> > -       for (i = 0; i < 1000 && !err && flag_res; i++)
> > -               err = ufshcd_query_flag_retry(hba,
> > UPIU_QUERY_OPCODE_READ_FLAG,
> > -                       QUERY_FLAG_IDN_FDEVICEINIT, 0, &flag_res);
> > +       /* Poll fDeviceInit flag to be cleared */
> > +       is_dev_val = ufs_get_dev_specific_value(hba,
> > + DEV_VAL_FDEVICEINIT,
> > &val);
> > +       dev_init_compl_in_ms = (is_dev_val) ? val : 500;
> If you want dev_init_compl_in_ms to take its default 1,000, you should:
> dev_init_compl_in_ms = (!is_dev_val) ? : val;
Got it.

> 
> > +       timeout = jiffies + msecs_to_jiffies(dev_init_compl_in_ms);
> > +       do {
> > +               err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG,
> > +                                       QUERY_FLAG_IDN_FDEVICEINIT, 0, &flag_res);
> > +               if (!flag_res)
> > +                       break;
> > +               usleep_range(5, 10);
> Per Grant's comment:
> usleep_range(5000, 10000); 
Got it.

Thanks.
Kiwoong Kim

RE: [RFC PATCH v3 2/2] ufs: change the way to complete fDeviceInit

[Avri Altman]

 
> 
> > > Currently, UFS driver checks if fDeviceInit is cleared at several
> > > times, not period. This patch is to wait its completion with the
> > > period, not retrying.
> > > Many device vendors usually provides the specification on it with just
> > > period, not a combination of a number of retrying and period. So it
> > > could be proper to regard to the information coming from device
> > > vendors.
> > >
> > > I first added one device specific value regarding the information.
> > >
> > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> > I still think that this patch alone is fine, and you don't need its
> > predecessor.
> > The spec requires polling, so this is a form of a more-effective-polling:
> > so be it.
> 
> If what you're mentioning 'effective' means being able to just wait for
> some long time upon completion of being cleared, it's not proper in real
> products because fDeviceInit latency usually has the biggest overhead of
> steps run during boot and some companies often try to manage its latency
> as KPI. The method like a combination of retrying and small delay make them
> harder to make it.
> Or if I understand what you meant, please let me know.
IMO, this patch is fine as it is - no need for the previous patch.
Just send it without the first patch.

Thanks,
Avri