| Message ID | 1ca1a52df36ad3393c0487537832cf7f0a7e1260.1593412974.git.kwmad.kim@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ufs: support various values per device | expand |
> 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 > 7b6f13a..27afdf0 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 = 500; I think default timeout value is too small. Most UFS vendors which are Samsung, Kioxia, SKHynix, Micron and WD want to set more than 1 seconds for a worst case of fdeviceinit. We need to add many quirks for every ufs vendors if the default value is 500ms. > + 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(1000, 2000); How about think to increase the value of usleep() to 5 ~ 10ms. I think 1 ~ 2ms is too small. > + } 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 Thanks for this patch. We are changing this code and value for all projects. Fdeviceinit fail is one of most frequently happened defects. So it's important to set with proper value. BR Grant
> > 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 > > 7b6f13a..27afdf0 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 = 500; > > I think default timeout value is too small. Most UFS vendors which are > Samsung, Kioxia, SKHynix, Micron and WD want to set more than 1 seconds > for a worst case of fdeviceinit. We need to add many quirks for every ufs > vendors if the default value is 500ms. > > > + 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(1000, 2000); > > How about think to increase the value of usleep() to 5 ~ 10ms. I think 1 ~ > 2ms is too small. > > > + } 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 > > Thanks for this patch. We are changing this code and value for all > projects. > Fdeviceinit fail is one of most frequently happened defects. So it's > important to set with proper value. > > BR > Grant Got it. Thanks. Kiwoong Kim
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 7b6f13a..27afdf0 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 = 500; + 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(1000, 2000); + } 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; }
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(-)