[RFC,v3,2/2] ufs: change the way to complete fDeviceInit
diff mbox series

Message ID 08bc1641fdce941175596fe106fd5c02161683bf.1593753896.git.kwmad.kim@samsung.com
State Changes Requested
Headers show
Series
  • ufs: support various values per device
Related show

Commit Message

Kiwoong Kim July 3, 2020, 5:30 a.m. UTC
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(-)

Comments

Avri Altman July 5, 2020, 10:53 a.m. UTC | #1
> 
> 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);
Kiwoong Kim July 15, 2020, 8:09 a.m. UTC | #2
> > 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
Avri Altman July 19, 2020, 6:46 a.m. UTC | #3
> 
> > > 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

Patch
diff mbox series

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;
 }