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

[PATCH v1 0/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.

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

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

[Kiwoong Kim]

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>

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.
---
 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..7b6f13a 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);
 
+void ufshcd_set_dev_values(struct ufs_hba *hba, struct ufs_dev_value *value)
+{
+	struct ufs_dev_value *f;
+	struct ufs_dev_info *dev_info = &hba->dev_info;
+
+	if (!value)
+		return;
+
+	for (f = 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;
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(ufshcd_set_dev_values);
+
 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: [PATCH v1 1/2] ufs: support various values per device

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2538 bytes --]

Hi Kiwoong,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on scsi/for-next]
[also build test WARNING on mkp-scsi/for-next v5.8-rc3 next-20200701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kiwoong-Kim/ufs-support-various-values-per-device/20200702-102146
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/scsi/ufs/ufshcd.c:6951:6: warning: no previous prototype for 'ufshcd_set_dev_values' [-Wmissing-prototypes]
    6951 | void ufshcd_set_dev_values(struct ufs_hba *hba, struct ufs_dev_value *value)
         |      ^~~~~~~~~~~~~~~~~~~~~
   drivers/scsi/ufs/ufshcd.c: In function 'ufs_fixup_device_setup':
>> drivers/scsi/ufs/ufshcd.c:6978:29: warning: passing argument 2 of 'ufshcd_set_dev_values' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    6978 |  ufshcd_set_dev_values(hba, ufs_dev_values);
         |                             ^~~~~~~~~~~~~~
   drivers/scsi/ufs/ufshcd.c:6951:71: note: expected 'struct ufs_dev_value *' but argument is of type 'const struct ufs_dev_value *'
    6951 | void ufshcd_set_dev_values(struct ufs_hba *hba, struct ufs_dev_value *value)
         |                                                 ~~~~~~~~~~~~~~~~~~~~~~^~~~~

vim +6978 drivers/scsi/ufs/ufshcd.c

  6971	
  6972	static void ufs_fixup_device_setup(struct ufs_hba *hba)
  6973	{
  6974		/* fix by general quirk table */
  6975		ufshcd_fixup_dev_quirks(hba, ufs_fixups);
  6976	
  6977		/* set device specific values */
> 6978		ufshcd_set_dev_values(hba, ufs_dev_values);
  6979	
  6980		/* allow vendors to fix quirks */
  6981		ufshcd_vops_fixup_dev_quirks(hba);
  6982	}
  6983	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67090 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1957 bytes --]

Hi Kiwoong,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on scsi/for-next]
[also build test WARNING on mkp-scsi/for-next v5.8-rc3 next-20200702]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kiwoong-Kim/ufs-support-various-values-per-device/20200702-102146
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: sparc64-randconfig-s032-20200701 (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.2-3-gfa153962-dirty
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=sparc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/scsi/ufs/ufshcd.c:211:13: sparse: sparse: Using plain integer as NULL pointer
>> drivers/scsi/ufs/ufshcd.c:6978:36: sparse: sparse: incorrect type in argument 2 (different modifiers) @@     expected struct ufs_dev_value *value @@     got struct ufs_dev_value const * @@
>> drivers/scsi/ufs/ufshcd.c:6978:36: sparse:     expected struct ufs_dev_value *value
>> drivers/scsi/ufs/ufshcd.c:6978:36: sparse:     got struct ufs_dev_value const *

vim +211 drivers/scsi/ufs/ufshcd.c

   209	
   210	static const struct ufs_dev_value ufs_dev_values[] = {
 > 211		{0, 0, 0, 0, false},
   212	};
   213	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32836 bytes --]

[PATCH v1 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 7b6f13a..717afed 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