[PATCH V1 1/3] scsi: ufs: set the device reference clock setting

[PATCH V1 1/3] scsi: ufs: set the device reference clock setting

[Sayali Lokhande]

From: Subhash Jadavani <subhashj@codeaurora.org>

UFS host supplies the reference clock to UFS device and UFS device
specification allows host to provide one of the 4 frequencies (19.2 MHz,
26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the
device reference clock frequency setting in the device based on what
frequency it is supplying to UFS device.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
[cang@codeaurora.org: Resolved trivial merge conflicts]
Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
---
 drivers/scsi/ufs/ufs.h    |  9 +++++++
 drivers/scsi/ufs/ufshcd.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h |  1 +
 3 files changed, 72 insertions(+)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 14e5bf7..e15deb0 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -378,6 +378,15 @@ enum query_opcode {
 	UPIU_QUERY_OPCODE_TOGGLE_FLAG	= 0x8,
 };
 
+/* bRefClkFreq attribute values */
+enum ref_clk_freq {
+	REF_CLK_FREQ_19_2_MHZ	= 0x0,
+	REF_CLK_FREQ_26_MHZ	= 0x1,
+	REF_CLK_FREQ_38_4_MHZ	= 0x2,
+	REF_CLK_FREQ_52_MHZ	= 0x3,
+	REF_CLK_FREQ_MAX	= REF_CLK_FREQ_52_MHZ,
+};
+
 /* Query response result code */
 enum {
 	QUERY_RESULT_SUCCESS                    = 0x00,
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c5b1bf1..3669bc4 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6297,6 +6297,63 @@ static void ufshcd_def_desc_sizes(struct ufs_hba *hba)
 }
 
 /**
+ * ufshcd_set_dev_ref_clk - set the device bRefClkFreq
+ * @hba: per-adapter instance
+ * @ref_clk_freq: refrerence clock frequency to be set
+ *
+ * Read the current value of the bRefClkFreq attribute from device and update it
+ * if host is supplying different reference clock frequency than one mentioned
+ * in bRefClkFreq attribute.
+ *
+ * Returns zero on success, non-zero error value on failure.
+ */
+static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba, u32 ref_clk_freq)
+{
+	int err = 0;
+	int ref_clk = -1;
+	static const char * const ref_clk_freqs[] = {"19.2 MHz", "26 MHz",
+						     "38.4 MHz", "52 MHz"};
+
+	hba->dev_ref_clk_freq = ref_clk_freq;
+	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0, &ref_clk);
+
+	if (err) {
+		dev_err(hba->dev, "%s: failed reading bRefClkFreq. err = %d\n",
+			 __func__, err);
+		goto out;
+	}
+
+	if ((ref_clk < 0) || (ref_clk > REF_CLK_FREQ_52_MHZ)) {
+		dev_err(hba->dev, "%s: invalid ref_clk setting = %d\n",
+			 __func__, ref_clk);
+		err = -EINVAL;
+		goto out;
+	}
+
+	if (ref_clk == hba->dev_ref_clk_freq)
+		goto out; /* nothing to update */
+
+	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0,
+			&hba->dev_ref_clk_freq);
+
+	if (err)
+		dev_err(hba->dev, "%s: bRefClkFreq setting to %s failed\n",
+			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
+	else
+		/*
+		 * It is good to print this out here to debug any later failures
+		 * related to gear switch.
+		 */
+		dev_info(hba->dev, "%s: bRefClkFreq setting to %s succeeded\n",
+			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
+
+out:
+	return err;
+}
+
+/**
  * ufshcd_probe_hba - probe hba to detect device and initialize
  * @hba: per-adapter instance
  *
@@ -6361,6 +6418,11 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
 			"%s: Failed getting max supported power mode\n",
 			__func__);
 	} else {
+		/*
+		 * Set the right value to bRefClkFreq before attempting to
+		 * switch to HS gears.
+		 */
+		ufshcd_set_dev_ref_clk(hba, REF_CLK_FREQ_19_2_MHZ);
 		ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
 		if (ret) {
 			dev_err(hba->dev, "%s: Failed setting power mode, err = %d\n",
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 8110dcd..b026ad8 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -548,6 +548,7 @@ struct ufs_hba {
 	void *priv;
 	unsigned int irq;
 	bool is_irq_enabled;
+	u32 dev_ref_clk_freq;
 
 	/* Interrupt aggregation support is broken */
 	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V1 1/3] scsi: ufs: set the device reference clock setting

[Adrian Hunter]

On 01/06/18 13:42, Sayali Lokhande wrote:
> From: Subhash Jadavani <subhashj@codeaurora.org>
> 
> UFS host supplies the reference clock to UFS device and UFS device
> specification allows host to provide one of the 4 frequencies (19.2 MHz,
> 26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the
> device reference clock frequency setting in the device based on what
> frequency it is supplying to UFS device.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> [cang@codeaurora.org: Resolved trivial merge conflicts]
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs.h    |  9 +++++++
>  drivers/scsi/ufs/ufshcd.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.h |  1 +
>  3 files changed, 72 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 14e5bf7..e15deb0 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -378,6 +378,15 @@ enum query_opcode {
>  	UPIU_QUERY_OPCODE_TOGGLE_FLAG	= 0x8,
>  };
>  
> +/* bRefClkFreq attribute values */
> +enum ref_clk_freq {
> +	REF_CLK_FREQ_19_2_MHZ	= 0x0,
> +	REF_CLK_FREQ_26_MHZ	= 0x1,
> +	REF_CLK_FREQ_38_4_MHZ	= 0x2,
> +	REF_CLK_FREQ_52_MHZ	= 0x3,
> +	REF_CLK_FREQ_MAX	= REF_CLK_FREQ_52_MHZ,
> +};
> +
>  /* Query response result code */
>  enum {
>  	QUERY_RESULT_SUCCESS                    = 0x00,
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c5b1bf1..3669bc4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6297,6 +6297,63 @@ static void ufshcd_def_desc_sizes(struct ufs_hba *hba)
>  }
>  
>  /**
> + * ufshcd_set_dev_ref_clk - set the device bRefClkFreq
> + * @hba: per-adapter instance
> + * @ref_clk_freq: refrerence clock frequency to be set
> + *
> + * Read the current value of the bRefClkFreq attribute from device and update it
> + * if host is supplying different reference clock frequency than one mentioned
> + * in bRefClkFreq attribute.
> + *
> + * Returns zero on success, non-zero error value on failure.
> + */
> +static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba, u32 ref_clk_freq)
> +{
> +	int err = 0;
> +	int ref_clk = -1;
> +	static const char * const ref_clk_freqs[] = {"19.2 MHz", "26 MHz",
> +						     "38.4 MHz", "52 MHz"};
> +
> +	hba->dev_ref_clk_freq = ref_clk_freq;
> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0, &ref_clk);
> +
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed reading bRefClkFreq. err = %d\n",
> +			 __func__, err);
> +		goto out;
> +	}
> +
> +	if ((ref_clk < 0) || (ref_clk > REF_CLK_FREQ_52_MHZ)) {

If you used u32 ref_clk then you wouldn't have to check < 0, also you should
use REF_CLK_FREQ_MAX not REF_CLK_FREQ_52_MHZ, but really why is this check
needed anyway?

> +		dev_err(hba->dev, "%s: invalid ref_clk setting = %d\n",
> +			 __func__, ref_clk);
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (ref_clk == hba->dev_ref_clk_freq)
> +		goto out; /* nothing to update */
> +
> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0,
> +			&hba->dev_ref_clk_freq);
> +
> +	if (err)
> +		dev_err(hba->dev, "%s: bRefClkFreq setting to %s failed\n",
> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
> +	else
> +		/*
> +		 * It is good to print this out here to debug any later failures
> +		 * related to gear switch.
> +		 */
> +		dev_info(hba->dev, "%s: bRefClkFreq setting to %s succeeded\n",
> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);

Why not make this dev_dbg and print always even when there is no update.

> +
> +out:
> +	return err;
> +}
> +
> +/**
>   * ufshcd_probe_hba - probe hba to detect device and initialize
>   * @hba: per-adapter instance
>   *
> @@ -6361,6 +6418,11 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>  			"%s: Failed getting max supported power mode\n",
>  			__func__);
>  	} else {
> +		/*
> +		 * Set the right value to bRefClkFreq before attempting to
> +		 * switch to HS gears.
> +		 */
> +		ufshcd_set_dev_ref_clk(hba, REF_CLK_FREQ_19_2_MHZ);
>  		ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
>  		if (ret) {
>  			dev_err(hba->dev, "%s: Failed setting power mode, err = %d\n",
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 8110dcd..b026ad8 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -548,6 +548,7 @@ struct ufs_hba {
>  	void *priv;
>  	unsigned int irq;
>  	bool is_irq_enabled;
> +	u32 dev_ref_clk_freq;
>  
>  	/* Interrupt aggregation support is broken */
>  	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
> 

RE: [PATCH V1 1/3] scsi: ufs: set the device reference clock setting

[sayali]

Hi Adrain,

Updated my comments inline. Please check.

Thanks,
Sayali
-----Original Message-----
From: Adrian Hunter [mailto:adrian.hunter@intel.com] 
Sent: Friday, June 01, 2018 5:59 PM
To: Sayali Lokhande <sayalil@codeaurora.org>; subhashj@codeaurora.org; cang@codeaurora.org; vivek.gautam@codeaurora.org; rnayak@codeaurora.org; vinholikatti@gmail.com; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; asutoshd@codeaurora.org; evgreen@chromium.org
Cc: linux-scsi@vger.kernel.org; open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V1 1/3] scsi: ufs: set the device reference clock setting

On 01/06/18 13:42, Sayali Lokhande wrote:
> From: Subhash Jadavani <subhashj@codeaurora.org>
> 
> UFS host supplies the reference clock to UFS device and UFS device 
> specification allows host to provide one of the 4 frequencies (19.2 
> MHz,
> 26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the 
> device reference clock frequency setting in the device based on what 
> frequency it is supplying to UFS device.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> [cang@codeaurora.org: Resolved trivial merge conflicts]
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs.h    |  9 +++++++
>  drivers/scsi/ufs/ufshcd.c | 62 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.h |  1 +
>  3 files changed, 72 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index 
> 14e5bf7..e15deb0 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -378,6 +378,15 @@ enum query_opcode {
>  	UPIU_QUERY_OPCODE_TOGGLE_FLAG	= 0x8,
>  };
>  
> +/* bRefClkFreq attribute values */
> +enum ref_clk_freq {
> +	REF_CLK_FREQ_19_2_MHZ	= 0x0,
> +	REF_CLK_FREQ_26_MHZ	= 0x1,
> +	REF_CLK_FREQ_38_4_MHZ	= 0x2,
> +	REF_CLK_FREQ_52_MHZ	= 0x3,
> +	REF_CLK_FREQ_MAX	= REF_CLK_FREQ_52_MHZ,
> +};
> +
>  /* Query response result code */
>  enum {
>  	QUERY_RESULT_SUCCESS                    = 0x00,
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c 
> index c5b1bf1..3669bc4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6297,6 +6297,63 @@ static void ufshcd_def_desc_sizes(struct 
> ufs_hba *hba)  }
>  
>  /**
> + * ufshcd_set_dev_ref_clk - set the device bRefClkFreq
> + * @hba: per-adapter instance
> + * @ref_clk_freq: refrerence clock frequency to be set
> + *
> + * Read the current value of the bRefClkFreq attribute from device 
> +and update it
> + * if host is supplying different reference clock frequency than one 
> +mentioned
> + * in bRefClkFreq attribute.
> + *
> + * Returns zero on success, non-zero error value on failure.
> + */
> +static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba, u32 
> +ref_clk_freq) {
> +	int err = 0;
> +	int ref_clk = -1;
> +	static const char * const ref_clk_freqs[] = {"19.2 MHz", "26 MHz",
> +						     "38.4 MHz", "52 MHz"};
> +
> +	hba->dev_ref_clk_freq = ref_clk_freq;
> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0, &ref_clk);
> +
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed reading bRefClkFreq. err = %d\n",
> +			 __func__, err);
> +		goto out;
> +	}
> +
> +	if ((ref_clk < 0) || (ref_clk > REF_CLK_FREQ_52_MHZ)) {

If you used u32 ref_clk then you wouldn't have to check < 0, also you should use REF_CLK_FREQ_MAX not REF_CLK_FREQ_52_MHZ, but really why is this check needed anyway?
[Sayali] Here ref_clk is defined as integer with value -1. We are then reading bRefClkFreq attribute from device into ref_clk and hence the sanity check is required before we update/write bRefClkFreq attribute.

> +		dev_err(hba->dev, "%s: invalid ref_clk setting = %d\n",
> +			 __func__, ref_clk);
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (ref_clk == hba->dev_ref_clk_freq)
> +		goto out; /* nothing to update */
> +
> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0,
> +			&hba->dev_ref_clk_freq);
> +
> +	if (err)
> +		dev_err(hba->dev, "%s: bRefClkFreq setting to %s failed\n",
> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
> +	else
> +		/*
> +		 * It is good to print this out here to debug any later failures
> +		 * related to gear switch.
> +		 */
> +		dev_info(hba->dev, "%s: bRefClkFreq setting to %s succeeded\n",
> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);

Why not make this dev_dbg and print always even when there is no update.
[Sayali] Agreed. Will update in next patchsets.

> +
> +out:
> +	return err;
> +}
> +
> +/**
>   * ufshcd_probe_hba - probe hba to detect device and initialize
>   * @hba: per-adapter instance
>   *
> @@ -6361,6 +6418,11 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>  			"%s: Failed getting max supported power mode\n",
>  			__func__);
>  	} else {
> +		/*
> +		 * Set the right value to bRefClkFreq before attempting to
> +		 * switch to HS gears.
> +		 */
> +		ufshcd_set_dev_ref_clk(hba, REF_CLK_FREQ_19_2_MHZ);
>  		ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
>  		if (ret) {
>  			dev_err(hba->dev, "%s: Failed setting power mode, err = %d\n", 
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h 
> index 8110dcd..b026ad8 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -548,6 +548,7 @@ struct ufs_hba {
>  	void *priv;
>  	unsigned int irq;
>  	bool is_irq_enabled;
> +	u32 dev_ref_clk_freq;
>  
>  	/* Interrupt aggregation support is broken */
>  	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
> 

Re: [PATCH V1 1/3] scsi: ufs: set the device reference clock setting

[Adrian Hunter]

On 01/06/18 16:11, sayali wrote:
> Hi Adrain,
> 
> Updated my comments inline. Please check.
> 
> Thanks,
> Sayali
> -----Original Message-----
> From: Adrian Hunter [mailto:adrian.hunter@intel.com] 
> Sent: Friday, June 01, 2018 5:59 PM
> To: Sayali Lokhande <sayalil@codeaurora.org>; subhashj@codeaurora.org; cang@codeaurora.org; vivek.gautam@codeaurora.org; rnayak@codeaurora.org; vinholikatti@gmail.com; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; asutoshd@codeaurora.org; evgreen@chromium.org
> Cc: linux-scsi@vger.kernel.org; open list <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH V1 1/3] scsi: ufs: set the device reference clock setting
> 
> On 01/06/18 13:42, Sayali Lokhande wrote:
>> From: Subhash Jadavani <subhashj@codeaurora.org>
>>
>> UFS host supplies the reference clock to UFS device and UFS device 
>> specification allows host to provide one of the 4 frequencies (19.2 
>> MHz,
>> 26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the 
>> device reference clock frequency setting in the device based on what 
>> frequency it is supplying to UFS device.
>>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> [cang@codeaurora.org: Resolved trivial merge conflicts]
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
>> ---
>>  drivers/scsi/ufs/ufs.h    |  9 +++++++
>>  drivers/scsi/ufs/ufshcd.c | 62 
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/scsi/ufs/ufshcd.h |  1 +
>>  3 files changed, 72 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index 
>> 14e5bf7..e15deb0 100644
>> --- a/drivers/scsi/ufs/ufs.h
>> +++ b/drivers/scsi/ufs/ufs.h
>> @@ -378,6 +378,15 @@ enum query_opcode {
>>  	UPIU_QUERY_OPCODE_TOGGLE_FLAG	= 0x8,
>>  };
>>  
>> +/* bRefClkFreq attribute values */
>> +enum ref_clk_freq {
>> +	REF_CLK_FREQ_19_2_MHZ	= 0x0,
>> +	REF_CLK_FREQ_26_MHZ	= 0x1,
>> +	REF_CLK_FREQ_38_4_MHZ	= 0x2,
>> +	REF_CLK_FREQ_52_MHZ	= 0x3,
>> +	REF_CLK_FREQ_MAX	= REF_CLK_FREQ_52_MHZ,
>> +};
>> +
>>  /* Query response result code */
>>  enum {
>>  	QUERY_RESULT_SUCCESS                    = 0x00,
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c 
>> index c5b1bf1..3669bc4 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -6297,6 +6297,63 @@ static void ufshcd_def_desc_sizes(struct 
>> ufs_hba *hba)  }
>>  
>>  /**
>> + * ufshcd_set_dev_ref_clk - set the device bRefClkFreq
>> + * @hba: per-adapter instance
>> + * @ref_clk_freq: refrerence clock frequency to be set
>> + *
>> + * Read the current value of the bRefClkFreq attribute from device 
>> +and update it
>> + * if host is supplying different reference clock frequency than one 
>> +mentioned
>> + * in bRefClkFreq attribute.
>> + *
>> + * Returns zero on success, non-zero error value on failure.
>> + */
>> +static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba, u32 
>> +ref_clk_freq) {
>> +	int err = 0;
>> +	int ref_clk = -1;
>> +	static const char * const ref_clk_freqs[] = {"19.2 MHz", "26 MHz",
>> +						     "38.4 MHz", "52 MHz"};
>> +
>> +	hba->dev_ref_clk_freq = ref_clk_freq;
>> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0, &ref_clk);
>> +
>> +	if (err) {
>> +		dev_err(hba->dev, "%s: failed reading bRefClkFreq. err = %d\n",
>> +			 __func__, err);
>> +		goto out;
>> +	}
>> +
>> +	if ((ref_clk < 0) || (ref_clk > REF_CLK_FREQ_52_MHZ)) {
> 
> If you used u32 ref_clk then you wouldn't have to check < 0, also you should use REF_CLK_FREQ_MAX not REF_CLK_FREQ_52_MHZ, but really why is this check needed anyway?
> [Sayali] Here ref_clk is defined as integer with value -1. We are then reading bRefClkFreq attribute from device into ref_clk and hence the sanity check is required before we update/write bRefClkFreq attribute.

But why sanity check a value that is going to be overwritten?

> 
>> +		dev_err(hba->dev, "%s: invalid ref_clk setting = %d\n",
>> +			 __func__, ref_clk);
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	if (ref_clk == hba->dev_ref_clk_freq)
>> +		goto out; /* nothing to update */
>> +
>> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0,
>> +			&hba->dev_ref_clk_freq);
>> +
>> +	if (err)
>> +		dev_err(hba->dev, "%s: bRefClkFreq setting to %s failed\n",
>> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
>> +	else
>> +		/*
>> +		 * It is good to print this out here to debug any later failures
>> +		 * related to gear switch.
>> +		 */
>> +		dev_info(hba->dev, "%s: bRefClkFreq setting to %s succeeded\n",
>> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
> 
> Why not make this dev_dbg and print always even when there is no update.
> [Sayali] Agreed. Will update in next patchsets.
> 
>> +
>> +out:
>> +	return err;
>> +}
>> +
>> +/**
>>   * ufshcd_probe_hba - probe hba to detect device and initialize
>>   * @hba: per-adapter instance
>>   *
>> @@ -6361,6 +6418,11 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>>  			"%s: Failed getting max supported power mode\n",
>>  			__func__);
>>  	} else {
>> +		/*
>> +		 * Set the right value to bRefClkFreq before attempting to
>> +		 * switch to HS gears.
>> +		 */
>> +		ufshcd_set_dev_ref_clk(hba, REF_CLK_FREQ_19_2_MHZ);
>>  		ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
>>  		if (ret) {
>>  			dev_err(hba->dev, "%s: Failed setting power mode, err = %d\n", 
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h 
>> index 8110dcd..b026ad8 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -548,6 +548,7 @@ struct ufs_hba {
>>  	void *priv;
>>  	unsigned int irq;
>>  	bool is_irq_enabled;
>> +	u32 dev_ref_clk_freq;
>>  
>>  	/* Interrupt aggregation support is broken */
>>  	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
>>
> 
> 
> 

RE: [PATCH V1 1/3] scsi: ufs: set the device reference clock setting

[sayali]

-----Original Message-----
From: Adrian Hunter [mailto:adrian.hunter@intel.com] 
Sent: Friday, June 01, 2018 6:48 PM
To: sayali <sayalil@codeaurora.org>; subhashj@codeaurora.org; cang@codeaurora.org; vivek.gautam@codeaurora.org; rnayak@codeaurora.org; vinholikatti@gmail.com; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; asutoshd@codeaurora.org; evgreen@chromium.org
Cc: linux-scsi@vger.kernel.org; 'open list' <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V1 1/3] scsi: ufs: set the device reference clock setting

On 01/06/18 16:11, sayali wrote:
> Hi Adrain,
> 
> Updated my comments inline. Please check.
> 
> Thanks,
> Sayali
> -----Original Message-----
> From: Adrian Hunter [mailto:adrian.hunter@intel.com]
> Sent: Friday, June 01, 2018 5:59 PM
> To: Sayali Lokhande <sayalil@codeaurora.org>; subhashj@codeaurora.org; 
> cang@codeaurora.org; vivek.gautam@codeaurora.org; 
> rnayak@codeaurora.org; vinholikatti@gmail.com; 
> jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; 
> asutoshd@codeaurora.org; evgreen@chromium.org
> Cc: linux-scsi@vger.kernel.org; open list 
> <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH V1 1/3] scsi: ufs: set the device reference clock 
> setting
> 
> On 01/06/18 13:42, Sayali Lokhande wrote:
>> From: Subhash Jadavani <subhashj@codeaurora.org>
>>
>> UFS host supplies the reference clock to UFS device and UFS device 
>> specification allows host to provide one of the 4 frequencies (19.2 
>> MHz,
>> 26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the 
>> device reference clock frequency setting in the device based on what 
>> frequency it is supplying to UFS device.
>>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> [cang@codeaurora.org: Resolved trivial merge conflicts]
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
>> ---
>>  drivers/scsi/ufs/ufs.h    |  9 +++++++
>>  drivers/scsi/ufs/ufshcd.c | 62
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/scsi/ufs/ufshcd.h |  1 +
>>  3 files changed, 72 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index
>> 14e5bf7..e15deb0 100644
>> --- a/drivers/scsi/ufs/ufs.h
>> +++ b/drivers/scsi/ufs/ufs.h
>> @@ -378,6 +378,15 @@ enum query_opcode {
>>  	UPIU_QUERY_OPCODE_TOGGLE_FLAG	= 0x8,
>>  };
>>  
>> +/* bRefClkFreq attribute values */
>> +enum ref_clk_freq {
>> +	REF_CLK_FREQ_19_2_MHZ	= 0x0,
>> +	REF_CLK_FREQ_26_MHZ	= 0x1,
>> +	REF_CLK_FREQ_38_4_MHZ	= 0x2,
>> +	REF_CLK_FREQ_52_MHZ	= 0x3,
>> +	REF_CLK_FREQ_MAX	= REF_CLK_FREQ_52_MHZ,
>> +};
>> +
>>  /* Query response result code */
>>  enum {
>>  	QUERY_RESULT_SUCCESS                    = 0x00,
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c 
>> index c5b1bf1..3669bc4 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -6297,6 +6297,63 @@ static void ufshcd_def_desc_sizes(struct 
>> ufs_hba *hba)  }
>>  
>>  /**
>> + * ufshcd_set_dev_ref_clk - set the device bRefClkFreq
>> + * @hba: per-adapter instance
>> + * @ref_clk_freq: refrerence clock frequency to be set
>> + *
>> + * Read the current value of the bRefClkFreq attribute from device 
>> +and update it
>> + * if host is supplying different reference clock frequency than one 
>> +mentioned
>> + * in bRefClkFreq attribute.
>> + *
>> + * Returns zero on success, non-zero error value on failure.
>> + */
>> +static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba, u32
>> +ref_clk_freq) {
>> +	int err = 0;
>> +	int ref_clk = -1;
>> +	static const char * const ref_clk_freqs[] = {"19.2 MHz", "26 MHz",
>> +						     "38.4 MHz", "52 MHz"};
>> +
>> +	hba->dev_ref_clk_freq = ref_clk_freq;
>> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0, &ref_clk);
>> +
>> +	if (err) {
>> +		dev_err(hba->dev, "%s: failed reading bRefClkFreq. err = %d\n",
>> +			 __func__, err);
>> +		goto out;
>> +	}
>> +
>> +	if ((ref_clk < 0) || (ref_clk > REF_CLK_FREQ_52_MHZ)) {
> 
> If you used u32 ref_clk then you wouldn't have to check < 0, also you should use REF_CLK_FREQ_MAX not REF_CLK_FREQ_52_MHZ, but really why is this check needed anyway?
> [Sayali] Here ref_clk is defined as integer with value -1. We are then reading bRefClkFreq attribute from device into ref_clk and hence the sanity check is required before we update/write bRefClkFreq attribute.

But why sanity check a value that is going to be overwritten?
[Sayali] Agreed. I need not check the ref_clk(which is the current setting), but still I should add check for new setting " ref_clk_freq" that is passed and going to be written (like  ref_clk_freq < REF_CLK_FREQ_MAX). Will update in next patchset.

> 
>> +		dev_err(hba->dev, "%s: invalid ref_clk setting = %d\n",
>> +			 __func__, ref_clk);
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	if (ref_clk == hba->dev_ref_clk_freq)
>> +		goto out; /* nothing to update */
>> +
>> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0,
>> +			&hba->dev_ref_clk_freq);
>> +
>> +	if (err)
>> +		dev_err(hba->dev, "%s: bRefClkFreq setting to %s failed\n",
>> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
>> +	else
>> +		/*
>> +		 * It is good to print this out here to debug any later failures
>> +		 * related to gear switch.
>> +		 */
>> +		dev_info(hba->dev, "%s: bRefClkFreq setting to %s succeeded\n",
>> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
> 
> Why not make this dev_dbg and print always even when there is no update.
> [Sayali] Agreed. Will update in next patchsets.
> 
>> +
>> +out:
>> +	return err;
>> +}
>> +
>> +/**
>>   * ufshcd_probe_hba - probe hba to detect device and initialize
>>   * @hba: per-adapter instance
>>   *
>> @@ -6361,6 +6418,11 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>>  			"%s: Failed getting max supported power mode\n",
>>  			__func__);
>>  	} else {
>> +		/*
>> +		 * Set the right value to bRefClkFreq before attempting to
>> +		 * switch to HS gears.
>> +		 */
>> +		ufshcd_set_dev_ref_clk(hba, REF_CLK_FREQ_19_2_MHZ);
>>  		ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
>>  		if (ret) {
>>  			dev_err(hba->dev, "%s: Failed setting power mode, err = %d\n", 
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h 
>> index 8110dcd..b026ad8 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -548,6 +548,7 @@ struct ufs_hba {
>>  	void *priv;
>>  	unsigned int irq;
>>  	bool is_irq_enabled;
>> +	u32 dev_ref_clk_freq;
>>  
>>  	/* Interrupt aggregation support is broken */
>>  	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
>>
> 
> 
> 

Re: [PATCH V1 1/3] scsi: ufs: set the device reference clock setting

[Kyuho Choi]

Hi Sayali,

On 6/1/18, Sayali Lokhande <sayalil@codeaurora.org> wrote:
> From: Subhash Jadavani <subhashj@codeaurora.org>
>
> UFS host supplies the reference clock to UFS device and UFS device
> specification allows host to provide one of the 4 frequencies (19.2 MHz,
> 26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the
> device reference clock frequency setting in the device based on what
> frequency it is supplying to UFS device.
>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> [cang@codeaurora.org: Resolved trivial merge conflicts]
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs.h    |  9 +++++++
>  drivers/scsi/ufs/ufshcd.c | 62
> +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.h |  1 +
>  3 files changed, 72 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 14e5bf7..e15deb0 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -378,6 +378,15 @@ enum query_opcode {
>  	UPIU_QUERY_OPCODE_TOGGLE_FLAG	= 0x8,
>  };
>
> +/* bRefClkFreq attribute values */
> +enum ref_clk_freq {
> +	REF_CLK_FREQ_19_2_MHZ	= 0x0,
> +	REF_CLK_FREQ_26_MHZ	= 0x1,
> +	REF_CLK_FREQ_38_4_MHZ	= 0x2,
> +	REF_CLK_FREQ_52_MHZ	= 0x3,
> +	REF_CLK_FREQ_MAX	= REF_CLK_FREQ_52_MHZ,
> +};
> +
>  /* Query response result code */
>  enum {
>  	QUERY_RESULT_SUCCESS                    = 0x00,
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c5b1bf1..3669bc4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6297,6 +6297,63 @@ static void ufshcd_def_desc_sizes(struct ufs_hba
> *hba)
>  }
>
>  /**
> + * ufshcd_set_dev_ref_clk - set the device bRefClkFreq
> + * @hba: per-adapter instance
> + * @ref_clk_freq: refrerence clock frequency to be set
> + *
> + * Read the current value of the bRefClkFreq attribute from device and
> update it
> + * if host is supplying different reference clock frequency than one
> mentioned
> + * in bRefClkFreq attribute.
> + *
> + * Returns zero on success, non-zero error value on failure.
> + */
> +static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba, u32 ref_clk_freq)
> +{
> +	int err = 0;
> +	int ref_clk = -1;
> +	static const char * const ref_clk_freqs[] = {"19.2 MHz", "26 MHz",
> +						     "38.4 MHz", "52 MHz"};
> +
> +	hba->dev_ref_clk_freq = ref_clk_freq;
> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0, &ref_clk);
> +
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed reading bRefClkFreq. err = %d\n",
> +			 __func__, err);
> +		goto out;
> +	}
> +
> +	if ((ref_clk < 0) || (ref_clk > REF_CLK_FREQ_52_MHZ)) {
> +		dev_err(hba->dev, "%s: invalid ref_clk setting = %d\n",
> +			 __func__, ref_clk);
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (ref_clk == hba->dev_ref_clk_freq)
> +		goto out; /* nothing to update */
> +
> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0,
> +			&hba->dev_ref_clk_freq);
> +
> +	if (err)
> +		dev_err(hba->dev, "%s: bRefClkFreq setting to %s failed\n",
> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
> +	else
> +		/*
> +		 * It is good to print this out here to debug any later failures
> +		 * related to gear switch.
> +		 */
> +		dev_info(hba->dev, "%s: bRefClkFreq setting to %s succeeded\n",
> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
> +
> +out:
> +	return err;
> +}
> +
> +/**
>   * ufshcd_probe_hba - probe hba to detect device and initialize
>   * @hba: per-adapter instance
>   *
> @@ -6361,6 +6418,11 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>  			"%s: Failed getting max supported power mode\n",
>  			__func__);
>  	} else {
> +		/*
> +		 * Set the right value to bRefClkFreq before attempting to
> +		 * switch to HS gears.
> +		 */
> +		ufshcd_set_dev_ref_clk(hba, REF_CLK_FREQ_19_2_MHZ);

How it works in 26MHz refclk support host like exynos series?. As I
understood, there default refclk value are 26MHz for host and device.
In that case, your code maybe set device's refclk to 19.2MHz. It would
be need to set same refclk value both host and device for switch to
HS.

>  		ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
>  		if (ret) {
>  			dev_err(hba->dev, "%s: Failed setting power mode, err = %d\n",
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 8110dcd..b026ad8 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -548,6 +548,7 @@ struct ufs_hba {
>  	void *priv;
>  	unsigned int irq;
>  	bool is_irq_enabled;
> +	u32 dev_ref_clk_freq;
>
>  	/* Interrupt aggregation support is broken */
>  	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
>

BR,
Kyuho Choi

RE: [PATCH V1 1/3] scsi: ufs: set the device reference clock setting

[sayali]

-----Original Message-----
From: Kyuho Choi [mailto:chlrbgh0@gmail.com] 
Sent: Saturday, June 02, 2018 11:04 AM
To: Sayali Lokhande <sayalil@codeaurora.org>
Cc: subhashj@codeaurora.org; cang@codeaurora.org; vivek.gautam@codeaurora.org; rnayak@codeaurora.org; vinholikatti@gmail.com; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; asutoshd@codeaurora.org; evgreen@chromium.org; linux-scsi@vger.kernel.org; open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V1 1/3] scsi: ufs: set the device reference clock setting

Hi Sayali,

On 6/1/18, Sayali Lokhande <sayalil@codeaurora.org> wrote:
> From: Subhash Jadavani <subhashj@codeaurora.org>
>
> UFS host supplies the reference clock to UFS device and UFS device 
> specification allows host to provide one of the 4 frequencies (19.2 
> MHz,
> 26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the 
> device reference clock frequency setting in the device based on what 
> frequency it is supplying to UFS device.
>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> [cang@codeaurora.org: Resolved trivial merge conflicts]
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs.h    |  9 +++++++
>  drivers/scsi/ufs/ufshcd.c | 62
> +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.h |  1 +
>  3 files changed, 72 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index 
> 14e5bf7..e15deb0 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -378,6 +378,15 @@ enum query_opcode {
>  	UPIU_QUERY_OPCODE_TOGGLE_FLAG	= 0x8,
>  };
>
> +/* bRefClkFreq attribute values */
> +enum ref_clk_freq {
> +	REF_CLK_FREQ_19_2_MHZ	= 0x0,
> +	REF_CLK_FREQ_26_MHZ	= 0x1,
> +	REF_CLK_FREQ_38_4_MHZ	= 0x2,
> +	REF_CLK_FREQ_52_MHZ	= 0x3,
> +	REF_CLK_FREQ_MAX	= REF_CLK_FREQ_52_MHZ,
> +};
> +
>  /* Query response result code */
>  enum {
>  	QUERY_RESULT_SUCCESS                    = 0x00,
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c 
> index c5b1bf1..3669bc4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6297,6 +6297,63 @@ static void ufshcd_def_desc_sizes(struct 
> ufs_hba
> *hba)
>  }
>
>  /**
> + * ufshcd_set_dev_ref_clk - set the device bRefClkFreq
> + * @hba: per-adapter instance
> + * @ref_clk_freq: refrerence clock frequency to be set
> + *
> + * Read the current value of the bRefClkFreq attribute from device 
> + and
> update it
> + * if host is supplying different reference clock frequency than one
> mentioned
> + * in bRefClkFreq attribute.
> + *
> + * Returns zero on success, non-zero error value on failure.
> + */
> +static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba, u32 
> +ref_clk_freq) {
> +	int err = 0;
> +	int ref_clk = -1;
> +	static const char * const ref_clk_freqs[] = {"19.2 MHz", "26 MHz",
> +						     "38.4 MHz", "52 MHz"};
> +
> +	hba->dev_ref_clk_freq = ref_clk_freq;
> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0, &ref_clk);
> +
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed reading bRefClkFreq. err = %d\n",
> +			 __func__, err);
> +		goto out;
> +	}
> +
> +	if ((ref_clk < 0) || (ref_clk > REF_CLK_FREQ_52_MHZ)) {
> +		dev_err(hba->dev, "%s: invalid ref_clk setting = %d\n",
> +			 __func__, ref_clk);
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (ref_clk == hba->dev_ref_clk_freq)
> +		goto out; /* nothing to update */
> +
> +	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +			QUERY_ATTR_IDN_REF_CLK_FREQ, 0, 0,
> +			&hba->dev_ref_clk_freq);
> +
> +	if (err)
> +		dev_err(hba->dev, "%s: bRefClkFreq setting to %s failed\n",
> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
> +	else
> +		/*
> +		 * It is good to print this out here to debug any later failures
> +		 * related to gear switch.
> +		 */
> +		dev_info(hba->dev, "%s: bRefClkFreq setting to %s succeeded\n",
> +			__func__, ref_clk_freqs[hba->dev_ref_clk_freq]);
> +
> +out:
> +	return err;
> +}
> +
> +/**
>   * ufshcd_probe_hba - probe hba to detect device and initialize
>   * @hba: per-adapter instance
>   *
> @@ -6361,6 +6418,11 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>  			"%s: Failed getting max supported power mode\n",
>  			__func__);
>  	} else {
> +		/*
> +		 * Set the right value to bRefClkFreq before attempting to
> +		 * switch to HS gears.
> +		 */
> +		ufshcd_set_dev_ref_clk(hba, REF_CLK_FREQ_19_2_MHZ);

How it works in 26MHz refclk support host like exynos series?. As I understood, there default refclk value are 26MHz for host and device.
In that case, your code maybe set device's refclk to 19.2MHz. It would be need to set same refclk value both host and device for switch to HS.
[Sayali] : As per HPG, we set ref_clk to 19.2 Mhz (for host and device). But as it can vary for others (like exynos as you pointed), I think its better to parse ref_clk frequency from device tree instead of passing hardcoded value. DT approach was present in my previous patchset, I will update/add it again in my next patch set.

>  		ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
>  		if (ret) {
>  			dev_err(hba->dev, "%s: Failed setting power mode, err = %d\n", 
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h 
> index 8110dcd..b026ad8 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -548,6 +548,7 @@ struct ufs_hba {
>  	void *priv;
>  	unsigned int irq;
>  	bool is_irq_enabled;
> +	u32 dev_ref_clk_freq;
>
>  	/* Interrupt aggregation support is broken */
>  	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
> Forum, a Linux Foundation Collaborative Project
>
>

BR,
Kyuho Choi

[PATCH V1 2/3] scsi: ufs: Add ufs provisioning support

[Sayali Lokhande]

A new api ufshcd_do_config_device() is added in driver
to suppoet UFS provisioning at runtime. Sysfs support
is added to trigger provisioning.
Device and Unit configurable parameters are parsed from
vendor specific provisioning data or file and
passed via sysfs at runtime to provision ufs device.

Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
---
 drivers/scsi/ufs/ufs.h    |  28 +++++++
 drivers/scsi/ufs/ufshcd.c | 200 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h |   1 +
 3 files changed, 229 insertions(+)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index e15deb0..1f99904 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -333,6 +333,7 @@ enum {
 	UFSHCD_AMP		= 3,
 };
 
+#define UFS_BLOCK_SIZE	4096
 #define POWER_DESC_MAX_SIZE			0x62
 #define POWER_DESC_MAX_ACTV_ICC_LVLS		16
 
@@ -425,6 +426,33 @@ enum {
 	MASK_TM_SERVICE_RESP		= 0xFF,
 };
 
+struct ufs_unit_desc {
+	u8     bLUEnable;              /* 1 for enabled LU */
+	u8     bBootLunID;             /* 0 for using this LU for boot */
+	u8     bLUWriteProtect;        /* 1 = power on WP, 2 = permanent WP */
+	u8     bMemoryType;            /* 0 for enhanced memory type */
+	u32    dNumAllocUnits;         /* Number of alloc unit for this LU */
+	u8     bDataReliability;       /* 0 for reliable write support */
+	u8     bLogicalBlockSize;      /* See section 13.2.3 of UFS standard */
+	u8     bProvisioningType;      /* 0 for thin provisioning */
+	u16    wContextCapabilities;   /* refer Unit Descriptor Description */
+};
+
+struct ufs_config_descr {
+	u8     bNumberLU;              /* Total number of active LUs */
+	u8     bBootEnable;            /* enabling device for partial init */
+	u8     bDescrAccessEn;         /* desc access during partial init */
+	u8     bInitPowerMode;         /* Initial device power mode */
+	u8     bHighPriorityLUN;       /* LUN of the high priority LU */
+	u8     bSecureRemovalType;     /* Erase config for data removal */
+	u8     bInitActiveICCLevel;    /* ICC level after reset */
+	u16    wPeriodicRTCUpdate;     /* 0 to set a priodic RTC update rate */
+	u32     bConfigDescrLock;      /* 1 to lock Configation Descriptor */
+	u32    qVendorConfigCode;      /* Vendor specific configuration code */
+	struct ufs_unit_desc unit[8];
+	u8	lun_to_grow;
+};
+
 /* Task management service response */
 enum {
 	UPIU_TASK_MANAGEMENT_FUNC_COMPL		= 0x00,
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3669bc4..3fd29e0 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -237,6 +237,7 @@ static int ufshcd_config_pwr_mode(struct ufs_hba *hba,
 		struct ufs_pa_layer_attr *desired_pwr_mode);
 static int ufshcd_change_power_mode(struct ufs_hba *hba,
 			     struct ufs_pa_layer_attr *pwr_mode);
+static int ufshcd_do_config_device(struct ufs_hba *hba);
 static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
 {
 	return tag >= 0 && tag < hba->nutrs;
@@ -3063,6 +3064,14 @@ static inline int ufshcd_read_power_desc(struct ufs_hba *hba,
 	return ufshcd_read_desc(hba, QUERY_DESC_IDN_POWER, 0, buf, size);
 }
 
+static inline int ufshcd_read_geometry_desc(struct ufs_hba *hba,
+					 u8 *buf,
+					 u32 size)
+{
+	return ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0, buf, size);
+}
+
+
 static int ufshcd_read_device_desc(struct ufs_hba *hba, u8 *buf, u32 size)
 {
 	return ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, buf, size);
@@ -6354,6 +6363,197 @@ static int ufshcd_set_dev_ref_clk(struct ufs_hba *hba, u32 ref_clk_freq)
 }
 
 /**
+ * ufshcd_do_config_device - API function for UFS provisioning
+ * hba: per-adapter instance
+ * Returns 0 for success, non-zero in case of failure.
+ */
+static int ufshcd_do_config_device(struct ufs_hba *hba)
+{
+	struct ufs_config_descr *cfg = &hba->cfgs;
+	int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
+	u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
+	int i, ret = 0;
+	int lun_to_grow = -1;
+	u64 qTotalRawDeviceCapacity;
+	u16 wEnhanced1CapAdjFac, wEnhanced2CapAdjFac;
+	u32 dEnhanced1MaxNAllocU, dEnhanced2MaxNAllocU;
+	size_t alloc_units, units_to_create = 0;
+	size_t capacity_to_alloc_factor;
+	size_t enhanced1_units = 0, enhanced2_units = 0;
+	size_t conversion_ratio = 1;
+	u8 *pt;
+	u32 blocks_per_alloc_unit = 1024;
+	int geo_len = hba->desc_size.geom_desc;
+	u8 geo_buf[hba->desc_size.geom_desc];
+	unsigned int max_partitions = 8;
+
+	WARN_ON(!hba || !cfg);
+	ufshcd_set_dev_ref_clk(hba, REF_CLK_FREQ_19_2_MHZ);
+
+	ret = ufshcd_read_geometry_desc(hba, geo_buf, geo_len);
+	if (ret) {
+		dev_err(hba->dev, "%s: Failed getting geometry_desc %d\n",
+			__func__, ret);
+		return ret;
+	}
+
+	/*
+	 * Get Geomtric parameters like total configurable memory
+	 * quantity (Offset 0x04 to 0x0b), Capacity Adjustment
+	 * Factors (Offset 0x30, 0x31, 0x36, 0x37), Max allocation
+	 * units (Offset 0x2c to 0x2f, 0x32 to 0x35) used to configure
+	 * the device logical units.
+	 */
+	qTotalRawDeviceCapacity =
+		(uint64_t)geo_buf[0x0b] | ((uint64_t)geo_buf[0x0a] << 8) |
+		((uint64_t)geo_buf[0x09] << 16) |
+		((uint64_t)geo_buf[0x08] << 24) |
+		((uint64_t)geo_buf[0x07] << 32) |
+		((uint64_t)geo_buf[0x06] << 40) |
+		((uint64_t)geo_buf[0x05] << 48) |
+		((uint64_t)geo_buf[0x04] << 56);
+	wEnhanced1CapAdjFac =
+		(uint16_t)geo_buf[0x31] | ((uint16_t)geo_buf[0x30] << 8);
+	wEnhanced2CapAdjFac =
+		(uint16_t)geo_buf[0x37] | ((uint16_t)geo_buf[0x36] << 8);
+	dEnhanced1MaxNAllocU =
+		(uint32_t)geo_buf[0x2f] | ((uint32_t)geo_buf[0x2e] << 8) |
+		((uint32_t)geo_buf[0x2d] << 16) |
+		((uint32_t)geo_buf[0x2c] << 24);
+	dEnhanced2MaxNAllocU =
+		(uint32_t)geo_buf[0x35] | ((uint32_t)geo_buf[0x34] << 8) |
+		((uint32_t)geo_buf[0x33] << 16) |
+		((uint32_t)geo_buf[0x32] << 24);
+
+	capacity_to_alloc_factor =
+		(blocks_per_alloc_unit * UFS_BLOCK_SIZE) / 512;
+
+	if (qTotalRawDeviceCapacity % capacity_to_alloc_factor != 0) {
+		dev_err(hba->dev,
+			"%s: Raw capacity(%llu) not multiple of alloc factor(%zu)\n",
+			__func__, qTotalRawDeviceCapacity,
+			capacity_to_alloc_factor);
+		return -EINVAL;
+	}
+	alloc_units = (qTotalRawDeviceCapacity / capacity_to_alloc_factor);
+	units_to_create = 0;
+	enhanced1_units = enhanced2_units = 0;
+
+	/*
+	 * Calculate number of allocation units to be assigned to a logical unit
+	 * considering the capacity adjustment factor of respective memory type.
+	 */
+	for (i = 0; i < (max_partitions - 1) &&
+		units_to_create <= alloc_units; i++) {
+		if ((cfg->unit[i].dNumAllocUnits % blocks_per_alloc_unit) == 0)
+			cfg->unit[i].dNumAllocUnits /= blocks_per_alloc_unit;
+		else
+			cfg->unit[i].dNumAllocUnits =
+			cfg->unit[i].dNumAllocUnits / blocks_per_alloc_unit + 1;
+
+		if (cfg->unit[i].bMemoryType == 0)
+			units_to_create += cfg->unit[i].dNumAllocUnits;
+		else if (cfg->unit[i].bMemoryType == 3) {
+			enhanced1_units += cfg->unit[i].dNumAllocUnits;
+			cfg->unit[i].dNumAllocUnits *=
+				(wEnhanced1CapAdjFac / 0x100);
+			units_to_create += cfg->unit[i].dNumAllocUnits;
+		} else if (cfg->unit[i].bMemoryType == 4) {
+			enhanced2_units += cfg->unit[i].dNumAllocUnits;
+			cfg->unit[i].dNumAllocUnits *=
+				(wEnhanced1CapAdjFac / 0x100);
+			units_to_create += cfg->unit[i].dNumAllocUnits;
+		} else {
+			dev_err(hba->dev, "%s: Unsupported memory type %d\n",
+				__func__, cfg->unit[i].bMemoryType);
+			return -EINVAL;
+		}
+	}
+	if (enhanced1_units > dEnhanced1MaxNAllocU) {
+		dev_err(hba->dev, "%s: size %zu exceeds max enhanced1 area size %u\n",
+			__func__, enhanced1_units, dEnhanced1MaxNAllocU);
+		return -ERANGE;
+	}
+	if (enhanced2_units > dEnhanced2MaxNAllocU) {
+		dev_err(hba->dev, "%s: size %zu exceeds max enhanced2 area size %u\n",
+			__func__, enhanced2_units, dEnhanced2MaxNAllocU);
+		return -ERANGE;
+	}
+	if (units_to_create > alloc_units) {
+		dev_err(hba->dev, "%s: Specified size %zu exceeds device size %zu\n",
+			__func__, units_to_create, alloc_units);
+		return -ERANGE;
+	}
+	lun_to_grow = cfg->lun_to_grow;
+	if (lun_to_grow != -1) {
+		if (cfg->unit[i].bMemoryType == 0)
+			conversion_ratio = 1;
+		else if (cfg->unit[i].bMemoryType == 3)
+			conversion_ratio = (wEnhanced1CapAdjFac / 0x100);
+		else if (cfg->unit[i].bMemoryType == 4)
+			conversion_ratio = (wEnhanced2CapAdjFac / 0x100);
+
+		cfg->unit[lun_to_grow].dNumAllocUnits +=
+		((alloc_units - units_to_create) / conversion_ratio);
+		dev_dbg(hba->dev, "%s: conversion_ratio %zu for lun %d\n",
+			__func__, conversion_ratio, i);
+	}
+
+	/* Fill in the buffer with configuration data */
+	pt = desc_buf;
+	*pt++ = 0x90;        // bLength
+	*pt++ = 0x01;        // bDescriptorType
+	*pt++ = 0;           // Reserved in UFS2.0 and onward
+	*pt++ = cfg->bBootEnable;
+	*pt++ = cfg->bDescrAccessEn;
+	*pt++ = cfg->bInitPowerMode;
+	*pt++ = cfg->bHighPriorityLUN;
+	*pt++ = cfg->bSecureRemovalType;
+	*pt++ = cfg->bInitActiveICCLevel;
+	*pt++ = (cfg->wPeriodicRTCUpdate >> 8) & 0xff;
+	*pt++ = cfg->wPeriodicRTCUpdate & 0xff;
+	pt = pt + 5; // Reserved fields set to 0
+
+	/* Fill in the buffer with per logical unit data */
+	for (i = 0; i < UFS_UPIU_MAX_GENERAL_LUN; i++) {
+		*pt++ = cfg->unit[i].bLUEnable;
+		*pt++ = cfg->unit[i].bBootLunID;
+		*pt++ = cfg->unit[i].bLUWriteProtect;
+		*pt++ = cfg->unit[i].bMemoryType;
+		*pt++ = (cfg->unit[i].dNumAllocUnits >> 24) & 0xff;
+		*pt++ = (cfg->unit[i].dNumAllocUnits >> 16) & 0xff;
+		*pt++ = (cfg->unit[i].dNumAllocUnits >> 8) & 0xff;
+		*pt++ = (cfg->unit[i].dNumAllocUnits) & 0xff;
+		*pt++ = cfg->unit[i].bDataReliability;
+		*pt++ = cfg->unit[i].bLogicalBlockSize;
+		*pt++ = cfg->unit[i].bProvisioningType;
+		*pt++ = (cfg->unit[i].wContextCapabilities >> 8) & 0xff;
+		*pt++ = cfg->unit[i].wContextCapabilities;
+		pt = pt + 3; // Reserved fields set to 0
+	}
+
+	ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_WRITE_DESC,
+		QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
+
+	if (ret) {
+		dev_err(hba->dev, "%s: Failed writing descriptor. desc_idn %d, opcode %x ret %d\n",
+		__func__, QUERY_DESC_IDN_CONFIGURATION,
+		UPIU_QUERY_OPCODE_WRITE_DESC, ret);
+		return ret;
+	}
+
+	if (cfg->bConfigDescrLock == 1) {
+		ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+		QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &cfg->bConfigDescrLock);
+		if (ret)
+			dev_err(hba->dev, "%s: Failed writing bConfigDescrLock %d\n",
+				__func__, ret);
+	}
+
+	return ret;
+}
+
+/**
  * ufshcd_probe_hba - probe hba to detect device and initialize
  * @hba: per-adapter instance
  *
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index b026ad8..982bfdf 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -549,6 +549,7 @@ struct ufs_hba {
 	unsigned int irq;
 	bool is_irq_enabled;
 	u32 dev_ref_clk_freq;
+	struct ufs_config_descr cfgs;
 
 	/* Interrupt aggregation support is broken */
 	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V1 2/3] scsi: ufs: Add ufs provisioning support

[Kyuho Choi]

Sayali,

On 6/1/18, Sayali Lokhande <sayalil@codeaurora.org> wrote:
> A new api ufshcd_do_config_device() is added in driver
> to suppoet UFS provisioning at runtime. Sysfs support
> is added to trigger provisioning.
> Device and Unit configurable parameters are parsed from
> vendor specific provisioning data or file and
> passed via sysfs at runtime to provision ufs device.
>
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs.h    |  28 +++++++
>  drivers/scsi/ufs/ufshcd.c | 200
> ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.h |   1 +
>  3 files changed, 229 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index e15deb0..1f99904 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -333,6 +333,7 @@ enum {
>  	UFSHCD_AMP		= 3,
>  };
>
> +#define UFS_BLOCK_SIZE	4096
>  #define POWER_DESC_MAX_SIZE			0x62
>  #define POWER_DESC_MAX_ACTV_ICC_LVLS		16
>
> @@ -425,6 +426,33 @@ enum {
>  	MASK_TM_SERVICE_RESP		= 0xFF,
>  };
>
> +struct ufs_unit_desc {
> +	u8     bLUEnable;              /* 1 for enabled LU */
> +	u8     bBootLunID;             /* 0 for using this LU for boot */
> +	u8     bLUWriteProtect;        /* 1 = power on WP, 2 = permanent WP */
> +	u8     bMemoryType;            /* 0 for enhanced memory type */
> +	u32    dNumAllocUnits;         /* Number of alloc unit for this LU */
> +	u8     bDataReliability;       /* 0 for reliable write support */
> +	u8     bLogicalBlockSize;      /* See section 13.2.3 of UFS standard */
> +	u8     bProvisioningType;      /* 0 for thin provisioning */
> +	u16    wContextCapabilities;   /* refer Unit Descriptor Description */
> +};
> +
> +struct ufs_config_descr {
> +	u8     bNumberLU;              /* Total number of active LUs */
> +	u8     bBootEnable;            /* enabling device for partial init */
> +	u8     bDescrAccessEn;         /* desc access during partial init */
> +	u8     bInitPowerMode;         /* Initial device power mode */
> +	u8     bHighPriorityLUN;       /* LUN of the high priority LU */
> +	u8     bSecureRemovalType;     /* Erase config for data removal */
> +	u8     bInitActiveICCLevel;    /* ICC level after reset */
> +	u16    wPeriodicRTCUpdate;     /* 0 to set a priodic RTC update rate */
> +	u32     bConfigDescrLock;      /* 1 to lock Configation Descriptor */
> +	u32    qVendorConfigCode;      /* Vendor specific configuration code */
> +	struct ufs_unit_desc unit[8];
> +	u8	lun_to_grow;
> +};
> +
>  /* Task management service response */
>  enum {
>  	UPIU_TASK_MANAGEMENT_FUNC_COMPL		= 0x00,
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 3669bc4..3fd29e0 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -237,6 +237,7 @@ static int ufshcd_config_pwr_mode(struct ufs_hba *hba,
>  		struct ufs_pa_layer_attr *desired_pwr_mode);
>  static int ufshcd_change_power_mode(struct ufs_hba *hba,
>  			     struct ufs_pa_layer_attr *pwr_mode);
> +static int ufshcd_do_config_device(struct ufs_hba *hba);
>  static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
>  {
>  	return tag >= 0 && tag < hba->nutrs;
> @@ -3063,6 +3064,14 @@ static inline int ufshcd_read_power_desc(struct
> ufs_hba *hba,
>  	return ufshcd_read_desc(hba, QUERY_DESC_IDN_POWER, 0, buf, size);
>  }
>
> +static inline int ufshcd_read_geometry_desc(struct ufs_hba *hba,
> +					 u8 *buf,
> +					 u32 size)
> +{
> +	return ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0, buf, size);
> +}
> +
> +
>  static int ufshcd_read_device_desc(struct ufs_hba *hba, u8 *buf, u32 size)
>  {
>  	return ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, buf, size);
> @@ -6354,6 +6363,197 @@ static int ufshcd_set_dev_ref_clk(struct ufs_hba
> *hba, u32 ref_clk_freq)
>  }
>
>  /**
> + * ufshcd_do_config_device - API function for UFS provisioning
> + * hba: per-adapter instance
> + * Returns 0 for success, non-zero in case of failure.
> + */
> +static int ufshcd_do_config_device(struct ufs_hba *hba)
> +{
> +	struct ufs_config_descr *cfg = &hba->cfgs;
> +	int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
> +	u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
> +	int i, ret = 0;
> +	int lun_to_grow = -1;
> +	u64 qTotalRawDeviceCapacity;
> +	u16 wEnhanced1CapAdjFac, wEnhanced2CapAdjFac;
> +	u32 dEnhanced1MaxNAllocU, dEnhanced2MaxNAllocU;
> +	size_t alloc_units, units_to_create = 0;
> +	size_t capacity_to_alloc_factor;
> +	size_t enhanced1_units = 0, enhanced2_units = 0;
> +	size_t conversion_ratio = 1;
> +	u8 *pt;
> +	u32 blocks_per_alloc_unit = 1024;
> +	int geo_len = hba->desc_size.geom_desc;
> +	u8 geo_buf[hba->desc_size.geom_desc];
> +	unsigned int max_partitions = 8;
> +
> +	WARN_ON(!hba || !cfg);
> +	ufshcd_set_dev_ref_clk(hba, REF_CLK_FREQ_19_2_MHZ);

Why you try to change the device refclk in here?. You need to check
about ufs power mode before change the ufs device's refclk. The refclk
change function wokring fine even when the ufs device in HS mode?. As
I understood, refclk change in HS are not guaranteed by MPHY spec.

> +
> +	ret = ufshcd_read_geometry_desc(hba, geo_buf, geo_len);
> +	if (ret) {
> +		dev_err(hba->dev, "%s: Failed getting geometry_desc %d\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Get Geomtric parameters like total configurable memory
> +	 * quantity (Offset 0x04 to 0x0b), Capacity Adjustment
> +	 * Factors (Offset 0x30, 0x31, 0x36, 0x37), Max allocation
> +	 * units (Offset 0x2c to 0x2f, 0x32 to 0x35) used to configure
> +	 * the device logical units.
> +	 */
> +	qTotalRawDeviceCapacity =
> +		(uint64_t)geo_buf[0x0b] | ((uint64_t)geo_buf[0x0a] << 8) |
> +		((uint64_t)geo_buf[0x09] << 16) |
> +		((uint64_t)geo_buf[0x08] << 24) |
> +		((uint64_t)geo_buf[0x07] << 32) |
> +		((uint64_t)geo_buf[0x06] << 40) |
> +		((uint64_t)geo_buf[0x05] << 48) |
> +		((uint64_t)geo_buf[0x04] << 56);
> +	wEnhanced1CapAdjFac =
> +		(uint16_t)geo_buf[0x31] | ((uint16_t)geo_buf[0x30] << 8);
> +	wEnhanced2CapAdjFac =
> +		(uint16_t)geo_buf[0x37] | ((uint16_t)geo_buf[0x36] << 8);
> +	dEnhanced1MaxNAllocU =
> +		(uint32_t)geo_buf[0x2f] | ((uint32_t)geo_buf[0x2e] << 8) |
> +		((uint32_t)geo_buf[0x2d] << 16) |
> +		((uint32_t)geo_buf[0x2c] << 24);
> +	dEnhanced2MaxNAllocU =
> +		(uint32_t)geo_buf[0x35] | ((uint32_t)geo_buf[0x34] << 8) |
> +		((uint32_t)geo_buf[0x33] << 16) |
> +		((uint32_t)geo_buf[0x32] << 24);
> +
> +	capacity_to_alloc_factor =
> +		(blocks_per_alloc_unit * UFS_BLOCK_SIZE) / 512;
> +
> +	if (qTotalRawDeviceCapacity % capacity_to_alloc_factor != 0) {
> +		dev_err(hba->dev,
> +			"%s: Raw capacity(%llu) not multiple of alloc factor(%zu)\n",
> +			__func__, qTotalRawDeviceCapacity,
> +			capacity_to_alloc_factor);
> +		return -EINVAL;
> +	}
> +	alloc_units = (qTotalRawDeviceCapacity / capacity_to_alloc_factor);
> +	units_to_create = 0;
> +	enhanced1_units = enhanced2_units = 0;
> +
> +	/*
> +	 * Calculate number of allocation units to be assigned to a logical unit
> +	 * considering the capacity adjustment factor of respective memory type.
> +	 */
> +	for (i = 0; i < (max_partitions - 1) &&
> +		units_to_create <= alloc_units; i++) {
> +		if ((cfg->unit[i].dNumAllocUnits % blocks_per_alloc_unit) == 0)
> +			cfg->unit[i].dNumAllocUnits /= blocks_per_alloc_unit;
> +		else
> +			cfg->unit[i].dNumAllocUnits =
> +			cfg->unit[i].dNumAllocUnits / blocks_per_alloc_unit + 1;
> +
> +		if (cfg->unit[i].bMemoryType == 0)
> +			units_to_create += cfg->unit[i].dNumAllocUnits;
> +		else if (cfg->unit[i].bMemoryType == 3) {
> +			enhanced1_units += cfg->unit[i].dNumAllocUnits;
> +			cfg->unit[i].dNumAllocUnits *=
> +				(wEnhanced1CapAdjFac / 0x100);
> +			units_to_create += cfg->unit[i].dNumAllocUnits;
> +		} else if (cfg->unit[i].bMemoryType == 4) {
> +			enhanced2_units += cfg->unit[i].dNumAllocUnits;
> +			cfg->unit[i].dNumAllocUnits *=
> +				(wEnhanced1CapAdjFac / 0x100);
> +			units_to_create += cfg->unit[i].dNumAllocUnits;
> +		} else {
> +			dev_err(hba->dev, "%s: Unsupported memory type %d\n",
> +				__func__, cfg->unit[i].bMemoryType);
> +			return -EINVAL;
> +		}
> +	}
> +	if (enhanced1_units > dEnhanced1MaxNAllocU) {
> +		dev_err(hba->dev, "%s: size %zu exceeds max enhanced1 area size %u\n",
> +			__func__, enhanced1_units, dEnhanced1MaxNAllocU);
> +		return -ERANGE;
> +	}
> +	if (enhanced2_units > dEnhanced2MaxNAllocU) {
> +		dev_err(hba->dev, "%s: size %zu exceeds max enhanced2 area size %u\n",
> +			__func__, enhanced2_units, dEnhanced2MaxNAllocU);
> +		return -ERANGE;
> +	}
> +	if (units_to_create > alloc_units) {
> +		dev_err(hba->dev, "%s: Specified size %zu exceeds device size %zu\n",
> +			__func__, units_to_create, alloc_units);
> +		return -ERANGE;
> +	}
> +	lun_to_grow = cfg->lun_to_grow;
> +	if (lun_to_grow != -1) {
> +		if (cfg->unit[i].bMemoryType == 0)
> +			conversion_ratio = 1;
> +		else if (cfg->unit[i].bMemoryType == 3)
> +			conversion_ratio = (wEnhanced1CapAdjFac / 0x100);
> +		else if (cfg->unit[i].bMemoryType == 4)
> +			conversion_ratio = (wEnhanced2CapAdjFac / 0x100);
> +
> +		cfg->unit[lun_to_grow].dNumAllocUnits +=
> +		((alloc_units - units_to_create) / conversion_ratio);
> +		dev_dbg(hba->dev, "%s: conversion_ratio %zu for lun %d\n",
> +			__func__, conversion_ratio, i);
> +	}
> +
> +	/* Fill in the buffer with configuration data */
> +	pt = desc_buf;
> +	*pt++ = 0x90;        // bLength
> +	*pt++ = 0x01;        // bDescriptorType
> +	*pt++ = 0;           // Reserved in UFS2.0 and onward
> +	*pt++ = cfg->bBootEnable;
> +	*pt++ = cfg->bDescrAccessEn;
> +	*pt++ = cfg->bInitPowerMode;
> +	*pt++ = cfg->bHighPriorityLUN;
> +	*pt++ = cfg->bSecureRemovalType;
> +	*pt++ = cfg->bInitActiveICCLevel;
> +	*pt++ = (cfg->wPeriodicRTCUpdate >> 8) & 0xff;
> +	*pt++ = cfg->wPeriodicRTCUpdate & 0xff;
> +	pt = pt + 5; // Reserved fields set to 0
> +
> +	/* Fill in the buffer with per logical unit data */
> +	for (i = 0; i < UFS_UPIU_MAX_GENERAL_LUN; i++) {
> +		*pt++ = cfg->unit[i].bLUEnable;
> +		*pt++ = cfg->unit[i].bBootLunID;
> +		*pt++ = cfg->unit[i].bLUWriteProtect;
> +		*pt++ = cfg->unit[i].bMemoryType;
> +		*pt++ = (cfg->unit[i].dNumAllocUnits >> 24) & 0xff;
> +		*pt++ = (cfg->unit[i].dNumAllocUnits >> 16) & 0xff;
> +		*pt++ = (cfg->unit[i].dNumAllocUnits >> 8) & 0xff;
> +		*pt++ = (cfg->unit[i].dNumAllocUnits) & 0xff;
> +		*pt++ = cfg->unit[i].bDataReliability;
> +		*pt++ = cfg->unit[i].bLogicalBlockSize;
> +		*pt++ = cfg->unit[i].bProvisioningType;
> +		*pt++ = (cfg->unit[i].wContextCapabilities >> 8) & 0xff;
> +		*pt++ = cfg->unit[i].wContextCapabilities;
> +		pt = pt + 3; // Reserved fields set to 0
> +	}
> +
> +	ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_WRITE_DESC,
> +		QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
> +
> +	if (ret) {
> +		dev_err(hba->dev, "%s: Failed writing descriptor. desc_idn %d, opcode %x
> ret %d\n",
> +		__func__, QUERY_DESC_IDN_CONFIGURATION,
> +		UPIU_QUERY_OPCODE_WRITE_DESC, ret);
> +		return ret;
> +	}
> +
> +	if (cfg->bConfigDescrLock == 1) {
> +		ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +		QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &cfg->bConfigDescrLock);
> +		if (ret)
> +			dev_err(hba->dev, "%s: Failed writing bConfigDescrLock %d\n",
> +				__func__, ret);
> +	}
> +
> +	return ret;
> +}
> +
> +/**
>   * ufshcd_probe_hba - probe hba to detect device and initialize
>   * @hba: per-adapter instance
>   *
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index b026ad8..982bfdf 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -549,6 +549,7 @@ struct ufs_hba {
>  	unsigned int irq;
>  	bool is_irq_enabled;
>  	u32 dev_ref_clk_freq;
> +	struct ufs_config_descr cfgs;
>
>  	/* Interrupt aggregation support is broken */
>  	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
>

RE: [PATCH V1 2/3] scsi: ufs: Add ufs provisioning support

[sayali]

-----Original Message-----
From: Kyuho Choi [mailto:chlrbgh0@gmail.com] 
Sent: Saturday, June 02, 2018 12:06 PM
To: Sayali Lokhande <sayalil@codeaurora.org>
Cc: subhashj@codeaurora.org; cang@codeaurora.org; vivek.gautam@codeaurora.org; rnayak@codeaurora.org; vinholikatti@gmail.com; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com; asutoshd@codeaurora.org; evgreen@chromium.org; linux-scsi@vger.kernel.org; open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V1 2/3] scsi: ufs: Add ufs provisioning support

Sayali,

On 6/1/18, Sayali Lokhande <sayalil@codeaurora.org> wrote:
> A new api ufshcd_do_config_device() is added in driver to suppoet UFS 
> provisioning at runtime. Sysfs support is added to trigger 
> provisioning.
> Device and Unit configurable parameters are parsed from vendor 
> specific provisioning data or file and passed via sysfs at runtime to 
> provision ufs device.
>
> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs.h    |  28 +++++++
>  drivers/scsi/ufs/ufshcd.c | 200
> ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.h |   1 +
>  3 files changed, 229 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index 
> e15deb0..1f99904 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -333,6 +333,7 @@ enum {
>  	UFSHCD_AMP		= 3,
>  };
>
> +#define UFS_BLOCK_SIZE	4096
>  #define POWER_DESC_MAX_SIZE			0x62
>  #define POWER_DESC_MAX_ACTV_ICC_LVLS		16
>
> @@ -425,6 +426,33 @@ enum {
>  	MASK_TM_SERVICE_RESP		= 0xFF,
>  };
>
> +struct ufs_unit_desc {
> +	u8     bLUEnable;              /* 1 for enabled LU */
> +	u8     bBootLunID;             /* 0 for using this LU for boot */
> +	u8     bLUWriteProtect;        /* 1 = power on WP, 2 = permanent WP */
> +	u8     bMemoryType;            /* 0 for enhanced memory type */
> +	u32    dNumAllocUnits;         /* Number of alloc unit for this LU */
> +	u8     bDataReliability;       /* 0 for reliable write support */
> +	u8     bLogicalBlockSize;      /* See section 13.2.3 of UFS standard */
> +	u8     bProvisioningType;      /* 0 for thin provisioning */
> +	u16    wContextCapabilities;   /* refer Unit Descriptor Description */
> +};
> +
> +struct ufs_config_descr {
> +	u8     bNumberLU;              /* Total number of active LUs */
> +	u8     bBootEnable;            /* enabling device for partial init */
> +	u8     bDescrAccessEn;         /* desc access during partial init */
> +	u8     bInitPowerMode;         /* Initial device power mode */
> +	u8     bHighPriorityLUN;       /* LUN of the high priority LU */
> +	u8     bSecureRemovalType;     /* Erase config for data removal */
> +	u8     bInitActiveICCLevel;    /* ICC level after reset */
> +	u16    wPeriodicRTCUpdate;     /* 0 to set a priodic RTC update rate */
> +	u32     bConfigDescrLock;      /* 1 to lock Configation Descriptor */
> +	u32    qVendorConfigCode;      /* Vendor specific configuration code */
> +	struct ufs_unit_desc unit[8];
> +	u8	lun_to_grow;
> +};
> +
>  /* Task management service response */  enum {
>  	UPIU_TASK_MANAGEMENT_FUNC_COMPL		= 0x00,
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c 
> index 3669bc4..3fd29e0 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -237,6 +237,7 @@ static int ufshcd_config_pwr_mode(struct ufs_hba *hba,
>  		struct ufs_pa_layer_attr *desired_pwr_mode);  static int 
> ufshcd_change_power_mode(struct ufs_hba *hba,
>  			     struct ufs_pa_layer_attr *pwr_mode);
> +static int ufshcd_do_config_device(struct ufs_hba *hba);
>  static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)  {
>  	return tag >= 0 && tag < hba->nutrs; @@ -3063,6 +3064,14 @@ static 
> inline int ufshcd_read_power_desc(struct ufs_hba *hba,
>  	return ufshcd_read_desc(hba, QUERY_DESC_IDN_POWER, 0, buf, size);  }
>
> +static inline int ufshcd_read_geometry_desc(struct ufs_hba *hba,
> +					 u8 *buf,
> +					 u32 size)
> +{
> +	return ufshcd_read_desc(hba, QUERY_DESC_IDN_GEOMETRY, 0, buf, size); 
> +}
> +
> +
>  static int ufshcd_read_device_desc(struct ufs_hba *hba, u8 *buf, u32 
> size)  {
>  	return ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, buf, size); 
> @@ -6354,6 +6363,197 @@ static int ufshcd_set_dev_ref_clk(struct 
> ufs_hba *hba, u32 ref_clk_freq)  }
>
>  /**
> + * ufshcd_do_config_device - API function for UFS provisioning
> + * hba: per-adapter instance
> + * Returns 0 for success, non-zero in case of failure.
> + */
> +static int ufshcd_do_config_device(struct ufs_hba *hba) {
> +	struct ufs_config_descr *cfg = &hba->cfgs;
> +	int buff_len = QUERY_DESC_CONFIGURATION_DEF_SIZE;
> +	u8 desc_buf[QUERY_DESC_CONFIGURATION_DEF_SIZE] = {0};
> +	int i, ret = 0;
> +	int lun_to_grow = -1;
> +	u64 qTotalRawDeviceCapacity;
> +	u16 wEnhanced1CapAdjFac, wEnhanced2CapAdjFac;
> +	u32 dEnhanced1MaxNAllocU, dEnhanced2MaxNAllocU;
> +	size_t alloc_units, units_to_create = 0;
> +	size_t capacity_to_alloc_factor;
> +	size_t enhanced1_units = 0, enhanced2_units = 0;
> +	size_t conversion_ratio = 1;
> +	u8 *pt;
> +	u32 blocks_per_alloc_unit = 1024;
> +	int geo_len = hba->desc_size.geom_desc;
> +	u8 geo_buf[hba->desc_size.geom_desc];
> +	unsigned int max_partitions = 8;
> +
> +	WARN_ON(!hba || !cfg);
> +	ufshcd_set_dev_ref_clk(hba, REF_CLK_FREQ_19_2_MHZ);

Why you try to change the device refclk in here?. You need to check about ufs power mode before change the ufs device's refclk. The refclk change function wokring fine even when the ufs device in HS mode?. As I understood, refclk change in HS are not guaranteed by MPHY spec.
[Sayali] Agreed. I need not again set ref_clk here before runtime provisioning every time, as I am setting ref_clk during probe in ufshcd_probe_hba() . 
It anyway does nothing here as ref_clk in device will be already set before, so it just returns. Will remove setting ref_clk from here in my next patchset. 

> +
> +	ret = ufshcd_read_geometry_desc(hba, geo_buf, geo_len);
> +	if (ret) {
> +		dev_err(hba->dev, "%s: Failed getting geometry_desc %d\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Get Geomtric parameters like total configurable memory
> +	 * quantity (Offset 0x04 to 0x0b), Capacity Adjustment
> +	 * Factors (Offset 0x30, 0x31, 0x36, 0x37), Max allocation
> +	 * units (Offset 0x2c to 0x2f, 0x32 to 0x35) used to configure
> +	 * the device logical units.
> +	 */
> +	qTotalRawDeviceCapacity =
> +		(uint64_t)geo_buf[0x0b] | ((uint64_t)geo_buf[0x0a] << 8) |
> +		((uint64_t)geo_buf[0x09] << 16) |
> +		((uint64_t)geo_buf[0x08] << 24) |
> +		((uint64_t)geo_buf[0x07] << 32) |
> +		((uint64_t)geo_buf[0x06] << 40) |
> +		((uint64_t)geo_buf[0x05] << 48) |
> +		((uint64_t)geo_buf[0x04] << 56);
> +	wEnhanced1CapAdjFac =
> +		(uint16_t)geo_buf[0x31] | ((uint16_t)geo_buf[0x30] << 8);
> +	wEnhanced2CapAdjFac =
> +		(uint16_t)geo_buf[0x37] | ((uint16_t)geo_buf[0x36] << 8);
> +	dEnhanced1MaxNAllocU =
> +		(uint32_t)geo_buf[0x2f] | ((uint32_t)geo_buf[0x2e] << 8) |
> +		((uint32_t)geo_buf[0x2d] << 16) |
> +		((uint32_t)geo_buf[0x2c] << 24);
> +	dEnhanced2MaxNAllocU =
> +		(uint32_t)geo_buf[0x35] | ((uint32_t)geo_buf[0x34] << 8) |
> +		((uint32_t)geo_buf[0x33] << 16) |
> +		((uint32_t)geo_buf[0x32] << 24);
> +
> +	capacity_to_alloc_factor =
> +		(blocks_per_alloc_unit * UFS_BLOCK_SIZE) / 512;
> +
> +	if (qTotalRawDeviceCapacity % capacity_to_alloc_factor != 0) {
> +		dev_err(hba->dev,
> +			"%s: Raw capacity(%llu) not multiple of alloc factor(%zu)\n",
> +			__func__, qTotalRawDeviceCapacity,
> +			capacity_to_alloc_factor);
> +		return -EINVAL;
> +	}
> +	alloc_units = (qTotalRawDeviceCapacity / capacity_to_alloc_factor);
> +	units_to_create = 0;
> +	enhanced1_units = enhanced2_units = 0;
> +
> +	/*
> +	 * Calculate number of allocation units to be assigned to a logical unit
> +	 * considering the capacity adjustment factor of respective memory type.
> +	 */
> +	for (i = 0; i < (max_partitions - 1) &&
> +		units_to_create <= alloc_units; i++) {
> +		if ((cfg->unit[i].dNumAllocUnits % blocks_per_alloc_unit) == 0)
> +			cfg->unit[i].dNumAllocUnits /= blocks_per_alloc_unit;
> +		else
> +			cfg->unit[i].dNumAllocUnits =
> +			cfg->unit[i].dNumAllocUnits / blocks_per_alloc_unit + 1;
> +
> +		if (cfg->unit[i].bMemoryType == 0)
> +			units_to_create += cfg->unit[i].dNumAllocUnits;
> +		else if (cfg->unit[i].bMemoryType == 3) {
> +			enhanced1_units += cfg->unit[i].dNumAllocUnits;
> +			cfg->unit[i].dNumAllocUnits *=
> +				(wEnhanced1CapAdjFac / 0x100);
> +			units_to_create += cfg->unit[i].dNumAllocUnits;
> +		} else if (cfg->unit[i].bMemoryType == 4) {
> +			enhanced2_units += cfg->unit[i].dNumAllocUnits;
> +			cfg->unit[i].dNumAllocUnits *=
> +				(wEnhanced1CapAdjFac / 0x100);
> +			units_to_create += cfg->unit[i].dNumAllocUnits;
> +		} else {
> +			dev_err(hba->dev, "%s: Unsupported memory type %d\n",
> +				__func__, cfg->unit[i].bMemoryType);
> +			return -EINVAL;
> +		}
> +	}
> +	if (enhanced1_units > dEnhanced1MaxNAllocU) {
> +		dev_err(hba->dev, "%s: size %zu exceeds max enhanced1 area size %u\n",
> +			__func__, enhanced1_units, dEnhanced1MaxNAllocU);
> +		return -ERANGE;
> +	}
> +	if (enhanced2_units > dEnhanced2MaxNAllocU) {
> +		dev_err(hba->dev, "%s: size %zu exceeds max enhanced2 area size %u\n",
> +			__func__, enhanced2_units, dEnhanced2MaxNAllocU);
> +		return -ERANGE;
> +	}
> +	if (units_to_create > alloc_units) {
> +		dev_err(hba->dev, "%s: Specified size %zu exceeds device size %zu\n",
> +			__func__, units_to_create, alloc_units);
> +		return -ERANGE;
> +	}
> +	lun_to_grow = cfg->lun_to_grow;
> +	if (lun_to_grow != -1) {
> +		if (cfg->unit[i].bMemoryType == 0)
> +			conversion_ratio = 1;
> +		else if (cfg->unit[i].bMemoryType == 3)
> +			conversion_ratio = (wEnhanced1CapAdjFac / 0x100);
> +		else if (cfg->unit[i].bMemoryType == 4)
> +			conversion_ratio = (wEnhanced2CapAdjFac / 0x100);
> +
> +		cfg->unit[lun_to_grow].dNumAllocUnits +=
> +		((alloc_units - units_to_create) / conversion_ratio);
> +		dev_dbg(hba->dev, "%s: conversion_ratio %zu for lun %d\n",
> +			__func__, conversion_ratio, i);
> +	}
> +
> +	/* Fill in the buffer with configuration data */
> +	pt = desc_buf;
> +	*pt++ = 0x90;        // bLength
> +	*pt++ = 0x01;        // bDescriptorType
> +	*pt++ = 0;           // Reserved in UFS2.0 and onward
> +	*pt++ = cfg->bBootEnable;
> +	*pt++ = cfg->bDescrAccessEn;
> +	*pt++ = cfg->bInitPowerMode;
> +	*pt++ = cfg->bHighPriorityLUN;
> +	*pt++ = cfg->bSecureRemovalType;
> +	*pt++ = cfg->bInitActiveICCLevel;
> +	*pt++ = (cfg->wPeriodicRTCUpdate >> 8) & 0xff;
> +	*pt++ = cfg->wPeriodicRTCUpdate & 0xff;
> +	pt = pt + 5; // Reserved fields set to 0
> +
> +	/* Fill in the buffer with per logical unit data */
> +	for (i = 0; i < UFS_UPIU_MAX_GENERAL_LUN; i++) {
> +		*pt++ = cfg->unit[i].bLUEnable;
> +		*pt++ = cfg->unit[i].bBootLunID;
> +		*pt++ = cfg->unit[i].bLUWriteProtect;
> +		*pt++ = cfg->unit[i].bMemoryType;
> +		*pt++ = (cfg->unit[i].dNumAllocUnits >> 24) & 0xff;
> +		*pt++ = (cfg->unit[i].dNumAllocUnits >> 16) & 0xff;
> +		*pt++ = (cfg->unit[i].dNumAllocUnits >> 8) & 0xff;
> +		*pt++ = (cfg->unit[i].dNumAllocUnits) & 0xff;
> +		*pt++ = cfg->unit[i].bDataReliability;
> +		*pt++ = cfg->unit[i].bLogicalBlockSize;
> +		*pt++ = cfg->unit[i].bProvisioningType;
> +		*pt++ = (cfg->unit[i].wContextCapabilities >> 8) & 0xff;
> +		*pt++ = cfg->unit[i].wContextCapabilities;
> +		pt = pt + 3; // Reserved fields set to 0
> +	}
> +
> +	ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_WRITE_DESC,
> +		QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
> +
> +	if (ret) {
> +		dev_err(hba->dev, "%s: Failed writing descriptor. desc_idn %d, 
> +opcode %x
> ret %d\n",
> +		__func__, QUERY_DESC_IDN_CONFIGURATION,
> +		UPIU_QUERY_OPCODE_WRITE_DESC, ret);
> +		return ret;
> +	}
> +
> +	if (cfg->bConfigDescrLock == 1) {
> +		ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +		QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &cfg->bConfigDescrLock);
> +		if (ret)
> +			dev_err(hba->dev, "%s: Failed writing bConfigDescrLock %d\n",
> +				__func__, ret);
> +	}
> +
> +	return ret;
> +}
> +
> +/**
>   * ufshcd_probe_hba - probe hba to detect device and initialize
>   * @hba: per-adapter instance
>   *
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h 
> index b026ad8..982bfdf 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -549,6 +549,7 @@ struct ufs_hba {
>  	unsigned int irq;
>  	bool is_irq_enabled;
>  	u32 dev_ref_clk_freq;
> +	struct ufs_config_descr cfgs;
>
>  	/* Interrupt aggregation support is broken */
>  	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
> Forum, a Linux Foundation Collaborative Project
>
>

Re: [PATCH V1 2/3] scsi: ufs: Add ufs provisioning support

[Bart Van Assche]

On Fri, 2018-06-01 at 16:12 +0530, Sayali Lokhande wrote:
> +	qTotalRawDeviceCapacity =
> +		(uint64_t)geo_buf[0x0b] | ((uint64_t)geo_buf[0x0a] << 8) |
> +		((uint64_t)geo_buf[0x09] << 16) |
> +		((uint64_t)geo_buf[0x08] << 24) |
> +		((uint64_t)geo_buf[0x07] << 32) |
> +		((uint64_t)geo_buf[0x06] << 40) |
> +		((uint64_t)geo_buf[0x05] << 48) |
> +		((uint64_t)geo_buf[0x04] << 56);
> +	wEnhanced1CapAdjFac =
> +		(uint16_t)geo_buf[0x31] | ((uint16_t)geo_buf[0x30] << 8);

Please use get_unaligned_be*() instead of open-coding these functions.

> +		*pt++ = (cfg->unit[i].dNumAllocUnits >> 24) & 0xff;
> +		*pt++ = (cfg->unit[i].dNumAllocUnits >> 16) & 0xff;
> +		*pt++ = (cfg->unit[i].dNumAllocUnits >> 8) & 0xff;
> +		*pt++ = (cfg->unit[i].dNumAllocUnits) & 0xff;

Please use put_unaligned_be() instead of open-coding this function.

Thanks,

Bart.

RE: [PATCH V1 2/3] scsi: ufs: Add ufs provisioning support

[sayali]

-----Original Message-----
From: Bart Van Assche [mailto:Bart.VanAssche@wdc.com] 
Sent: Monday, June 04, 2018 2:48 PM
To: vinholikatti@gmail.com; asutoshd@codeaurora.org; sayalil@codeaurora.org; evgreen@chromium.org; cang@codeaurora.org; martin.petersen@oracle.com; subhashj@codeaurora.org; vivek.gautam@codeaurora.org; rnayak@codeaurora.org; jejb@linux.vnet.ibm.com
Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH V1 2/3] scsi: ufs: Add ufs provisioning support

On Fri, 2018-06-01 at 16:12 +0530, Sayali Lokhande wrote:
> +	qTotalRawDeviceCapacity =
> +		(uint64_t)geo_buf[0x0b] | ((uint64_t)geo_buf[0x0a] << 8) |
> +		((uint64_t)geo_buf[0x09] << 16) |
> +		((uint64_t)geo_buf[0x08] << 24) |
> +		((uint64_t)geo_buf[0x07] << 32) |
> +		((uint64_t)geo_buf[0x06] << 40) |
> +		((uint64_t)geo_buf[0x05] << 48) |
> +		((uint64_t)geo_buf[0x04] << 56);
> +	wEnhanced1CapAdjFac =
> +		(uint16_t)geo_buf[0x31] | ((uint16_t)geo_buf[0x30] << 8);

Please use get_unaligned_be*() instead of open-coding these functions.
[Sayali]Agreed. Will update.

> +		*pt++ = (cfg->unit[i].dNumAllocUnits >> 24) & 0xff;
> +		*pt++ = (cfg->unit[i].dNumAllocUnits >> 16) & 0xff;
> +		*pt++ = (cfg->unit[i].dNumAllocUnits >> 8) & 0xff;
> +		*pt++ = (cfg->unit[i].dNumAllocUnits) & 0xff;

Please use put_unaligned_be() instead of open-coding this function.
[Sayali]Agreed. Will update.

Thanks,

Bart.

[PATCH V1 3/3] scsi: ufs: Add sysfs support for ufs provision

[Sayali Lokhande]

Add sysfs support to trigger ufs provisioning at runtime.
Usage : echo <desc_buf> > /sys/bus/platform/drivers/*/
	config_descriptor/ufs_provision
To check provisioning status:
cat /sys/bus/platform/drivers/*/config_descriptor/ufs_provision
1- > Success (Reboot device to check updated provisioning)

Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
---
 Documentation/ABI/testing/sysfs-driver-ufs |  16 ++++
 drivers/scsi/ufs/ufs-sysfs.c               |  25 ++++++
 drivers/scsi/ufs/ufs.h                     |   2 +
 drivers/scsi/ufs/ufshcd.c                  | 128 +++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h                  |   5 ++
 5 files changed, 176 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index 016724e..43419b5 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -883,3 +883,19 @@ Contact:	Subhash Jadavani <subhashj@codeaurora.org>
 Description:	This entry shows the target state of an UFS UIC link
 		for the chosen system power management level.
 		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/config_descriptor/ufs_provision
+Date:		February 2018
+Contact:	Sayali Lokhande <sayalil@codeaurora.org>
+Description:	This file shows the status of runtime ufs provisioning.
+		This can be used to provision ufs device if bConfigDescrLock is 0.
+		Configuration buffer needs to be written in space separated format
+		specificied as:
+		echo <bNumberLU> <bBootEnable> <bDescrAccessEn> <bInitPowerMode>
+		<bHighPriorityLUN> <bSecureRemovalType> <bInitActiveICCLevel>
+		<wPeriodicRTCUpdate> <bConfigDescrLock> <LUNum> <bLUEnable>
+		<bBootLunID> <size_in_kb> <bDataReliability> <bLUWriteProtect>
+		<bMemoryType> <bLogicalBlockSize> <bProvisioningType>
+		<wContextCapabilities> > ufs_provision
+		To check updated configuration check unit_descriptor and
+		device_descriptor sysfs fields.
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 8d9332b..8b68813 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -252,6 +252,30 @@ static ssize_t ufs_sysfs_read_desc_param(struct ufs_hba *hba,
 	return ret;
 }
 
+static ssize_t ufs_provision_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return ufshcd_desc_config_show(dev, attr, buf);
+}
+
+static ssize_t ufs_provision_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	return ufshcd_desc_config_store(dev, attr, buf, count);
+}
+
+DEVICE_ATTR_RW(ufs_provision);
+
+static struct attribute *ufs_sysfs_config_descriptor[] = {
+	&dev_attr_ufs_provision.attr,
+	NULL,
+};
+
+static const struct attribute_group ufs_sysfs_config_descriptor_group = {
+	.name = "config_descriptor",
+	.attrs = ufs_sysfs_config_descriptor,
+};
+
 #define UFS_DESC_PARAM(_name, _puname, _duname, _size)			\
 static ssize_t _name##_show(struct device *dev,				\
 	struct device_attribute *attr, char *buf)			\
@@ -713,6 +737,7 @@ static DEVICE_ATTR_RO(_name)
 static const struct attribute_group *ufs_sysfs_groups[] = {
 	&ufs_sysfs_default_group,
 	&ufs_sysfs_device_descriptor_group,
+	&ufs_sysfs_config_descriptor_group,
 	&ufs_sysfs_interconnect_descriptor_group,
 	&ufs_sysfs_geometry_descriptor_group,
 	&ufs_sysfs_health_descriptor_group,
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 1f99904..0b497fc 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -427,6 +427,7 @@ enum {
 };
 
 struct ufs_unit_desc {
+	u8	   LUNum;
 	u8     bLUEnable;              /* 1 for enabled LU */
 	u8     bBootLunID;             /* 0 for using this LU for boot */
 	u8     bLUWriteProtect;        /* 1 = power on WP, 2 = permanent WP */
@@ -451,6 +452,7 @@ struct ufs_config_descr {
 	u32    qVendorConfigCode;      /* Vendor specific configuration code */
 	struct ufs_unit_desc unit[8];
 	u8	lun_to_grow;
+	u8 num_luns;
 };
 
 /* Task management service response */
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3fd29e0..122c119 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1580,6 +1580,131 @@ void ufshcd_release(struct ufs_hba *hba)
 }
 EXPORT_SYMBOL_GPL(ufshcd_release);
 
+ssize_t ufshcd_desc_config_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "provision_enabled = %d\n",
+		hba->provision_enabled);
+}
+
+ssize_t ufshcd_desc_config_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct ufs_config_descr *cfg = &hba->cfgs;
+	char *strbuf;
+	char *strbuf_copy;
+	int desc_buf[count];
+	int *pt;
+	char *token;
+	int i, ret;
+	int value, commit = 0;
+	int num_luns = 0;
+	int KB_per_block = 4;
+
+	/* reserve one byte for null termination */
+	strbuf = kmalloc(count + 1, GFP_KERNEL);
+	if (!strbuf)
+		return -ENOMEM;
+
+	strbuf_copy = strbuf;
+	strlcpy(strbuf, buf, count + 1);
+	memset(desc_buf, 0, count);
+
+	/* Just return if bConfigDescrLock is already set */
+	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+		QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0, &cfg->bConfigDescrLock);
+	if (ret) {
+		dev_err(hba->dev, "%s: Failed reading bConfigDescrLock %d, cannot re-provision device!\n",
+			__func__, ret);
+		hba->provision_enabled = 0;
+		goto out;
+	}
+	if (cfg->bConfigDescrLock == 1) {
+		dev_err(hba->dev, "%s: bConfigDescrLock already set to %u, cannot re-provision device!\n",
+		__func__, cfg->bConfigDescrLock);
+		hba->provision_enabled = 0;
+		goto out;
+	}
+
+	for (i = 0; i < count; i++) {
+		token = strsep(&strbuf, " ");
+		if (!token && i) {
+			num_luns = desc_buf[i-1];
+			dev_dbg(hba->dev, "%s: token %s, num_luns %d\n",
+				__func__, token, num_luns);
+			if (num_luns > 8) {
+				dev_err(hba->dev, "%s: Invalid num_luns %d\n",
+				__func__, num_luns);
+				hba->provision_enabled = 0;
+				goto out;
+			}
+			break;
+		}
+
+		ret = kstrtoint(token, 0, &value);
+		if (ret) {
+			dev_err(hba->dev, "%s: kstrtoint failed %d %s\n",
+				__func__, ret, token);
+			break;
+		}
+		desc_buf[i] = value;
+		dev_dbg(hba->dev, " desc_buf[%d] 0x%x", i, desc_buf[i]);
+	}
+
+	/* Fill in the descriptors with parsed configuration data */
+	pt = desc_buf;
+	cfg->bNumberLU = *pt++;
+	cfg->bBootEnable = *pt++;
+	cfg->bDescrAccessEn = *pt++;
+	cfg->bInitPowerMode = *pt++;
+	cfg->bHighPriorityLUN = *pt++;
+	cfg->bSecureRemovalType = *pt++;
+	cfg->bInitActiveICCLevel = *pt++;
+	cfg->wPeriodicRTCUpdate = *pt++;
+	cfg->bConfigDescrLock = *pt++;
+	dev_dbg(hba->dev, "%s: %u %u %u %u %u %u %u %u %u\n", __func__,
+	cfg->bNumberLU, cfg->bBootEnable, cfg->bDescrAccessEn,
+	cfg->bInitPowerMode, cfg->bHighPriorityLUN, cfg->bSecureRemovalType,
+	cfg->bInitActiveICCLevel, cfg->wPeriodicRTCUpdate,
+	cfg->bConfigDescrLock);
+
+	for (i = 0; i < num_luns; i++) {
+		cfg->unit[i].LUNum = *pt++;
+		cfg->unit[i].bLUEnable = *pt++;
+		cfg->unit[i].bBootLunID = *pt++;
+		/* dNumAllocUnits = size_in_kb/KB_per_block */
+		cfg->unit[i].dNumAllocUnits = (u32)(*pt++ / KB_per_block);
+		cfg->unit[i].bDataReliability = *pt++;
+		cfg->unit[i].bLUWriteProtect = *pt++;
+		cfg->unit[i].bMemoryType = *pt++;
+		cfg->unit[i].bLogicalBlockSize = *pt++;
+		cfg->unit[i].bProvisioningType = *pt++;
+		cfg->unit[i].wContextCapabilities = *pt++;
+	}
+
+	cfg->lun_to_grow = *pt++;
+	commit = *pt++;
+	cfg->num_luns = *pt;
+	dev_dbg(hba->dev, "%s: lun_to_grow %u, commit %u num_luns %u\n",
+		__func__, cfg->lun_to_grow, commit, cfg->num_luns);
+	if (commit == 1) {
+		ret = ufshcd_do_config_device(hba);
+		if (!ret) {
+			hba->provision_enabled = 1;
+			dev_err(hba->dev,
+			"%s: UFS Provisioning completed,num_luns %u, reboot now !\n",
+			__func__, cfg->num_luns);
+		}
+	} else
+		dev_err(hba->dev, "%s: Invalid commit %u\n", __func__, commit);
+out:
+	kfree(strbuf_copy);
+	return count;
+}
+
 static ssize_t ufshcd_clkgate_delay_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
@@ -6532,6 +6657,9 @@ static int ufshcd_do_config_device(struct ufs_hba *hba)
 		pt = pt + 3; // Reserved fields set to 0
 	}
 
+	for (i = 0; i < buff_len; i++)
+		dev_dbg(hba->dev, " desc_buf[%d] 0x%x", i, desc_buf[i]);
+
 	ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_WRITE_DESC,
 		QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
 
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 982bfdf..1b8304f 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -651,6 +651,7 @@ struct ufs_hba {
 	struct ufs_pwr_mode_info max_pwr_info;
 
 	struct ufs_clk_gating clk_gating;
+	bool provision_enabled;
 	/* Control to enable/disable host capabilities */
 	u32 caps;
 	/* Allow dynamic clk gating */
@@ -867,6 +868,10 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, int desc_index,
 
 int ufshcd_hold(struct ufs_hba *hba, bool async);
 void ufshcd_release(struct ufs_hba *hba);
+ssize_t ufshcd_desc_config_show(struct device *dev,
+		struct device_attribute *attr, char *buf);
+ssize_t ufshcd_desc_config_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count);
 
 int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
 	int *desc_length);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH V1 3/3] scsi: ufs: Add sysfs support for ufs provision

[sayali]

Hi Evan,

I am working on upstreaming runtime UFS provisioning patches and have posted
my latest (V1) patches for review.
I have noticed that you are also working on the same and would appreciate if
you could also review my patches.
I believe it will save us all some time and duplicate effort. Let me know if
you have any concern or comments on this. 

Thanks,
Sayali

-----Original Message-----
From: Sayali Lokhande [mailto:sayalil@codeaurora.org] 
Sent: Friday, June 01, 2018 4:13 PM
To: subhashj@codeaurora.org; cang@codeaurora.org;
vivek.gautam@codeaurora.org; rnayak@codeaurora.org; vinholikatti@gmail.com;
jejb@linux.vnet.ibm.com; martin.petersen@oracle.com;
asutoshd@codeaurora.org; evgreen@chromium.org
Cc: linux-scsi@vger.kernel.org; Sayali Lokhande <sayalil@codeaurora.org>;
Stanislav Nijnikov <stanislav.nijnikov@wdc.com>; Greg Kroah-Hartman
<gregkh@linuxfoundation.org>; Adrian Hunter <adrian.hunter@intel.com>; open
list <linux-kernel@vger.kernel.org>
Subject: [PATCH V1 3/3] scsi: ufs: Add sysfs support for ufs provision

Add sysfs support to trigger ufs provisioning at runtime.
Usage : echo <desc_buf> > /sys/bus/platform/drivers/*/
	config_descriptor/ufs_provision
To check provisioning status:
cat /sys/bus/platform/drivers/*/config_descriptor/ufs_provision
1- > Success (Reboot device to check updated provisioning)

Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
---
 Documentation/ABI/testing/sysfs-driver-ufs |  16 ++++
 drivers/scsi/ufs/ufs-sysfs.c               |  25 ++++++
 drivers/scsi/ufs/ufs.h                     |   2 +
 drivers/scsi/ufs/ufshcd.c                  | 128
+++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h                  |   5 ++
 5 files changed, 176 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs
b/Documentation/ABI/testing/sysfs-driver-ufs
index 016724e..43419b5 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -883,3 +883,19 @@ Contact:	Subhash Jadavani <subhashj@codeaurora.org>
 Description:	This entry shows the target state of an UFS UIC link
 		for the chosen system power management level.
 		The file is read only.
+
+What:
/sys/bus/platform/drivers/ufshcd/*/config_descriptor/ufs_provision
+Date:		February 2018
+Contact:	Sayali Lokhande <sayalil@codeaurora.org>
+Description:	This file shows the status of runtime ufs provisioning.
+		This can be used to provision ufs device if bConfigDescrLock
is 0.
+		Configuration buffer needs to be written in space separated
format
+		specificied as:
+		echo <bNumberLU> <bBootEnable> <bDescrAccessEn>
<bInitPowerMode>
+		<bHighPriorityLUN> <bSecureRemovalType>
<bInitActiveICCLevel>
+		<wPeriodicRTCUpdate> <bConfigDescrLock> <LUNum> <bLUEnable>
+		<bBootLunID> <size_in_kb> <bDataReliability>
<bLUWriteProtect>
+		<bMemoryType> <bLogicalBlockSize> <bProvisioningType>
+		<wContextCapabilities> > ufs_provision
+		To check updated configuration check unit_descriptor and
+		device_descriptor sysfs fields.
diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 8d9332b..8b68813 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -252,6 +252,30 @@ static ssize_t ufs_sysfs_read_desc_param(struct ufs_hba
*hba,
 	return ret;
 }
 
+static ssize_t ufs_provision_show(struct device *dev,
+		struct device_attribute *attr, char *buf) {
+	return ufshcd_desc_config_show(dev, attr, buf); }
+
+static ssize_t ufs_provision_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t
count) {
+	return ufshcd_desc_config_store(dev, attr, buf, count); }
+
+DEVICE_ATTR_RW(ufs_provision);
+
+static struct attribute *ufs_sysfs_config_descriptor[] = {
+	&dev_attr_ufs_provision.attr,
+	NULL,
+};
+
+static const struct attribute_group ufs_sysfs_config_descriptor_group = {
+	.name = "config_descriptor",
+	.attrs = ufs_sysfs_config_descriptor,
+};
+
 #define UFS_DESC_PARAM(_name, _puname, _duname, _size)			\
 static ssize_t _name##_show(struct device *dev,
\
 	struct device_attribute *attr, char *buf)			\
@@ -713,6 +737,7 @@ static DEVICE_ATTR_RO(_name)  static const struct
attribute_group *ufs_sysfs_groups[] = {
 	&ufs_sysfs_default_group,
 	&ufs_sysfs_device_descriptor_group,
+	&ufs_sysfs_config_descriptor_group,
 	&ufs_sysfs_interconnect_descriptor_group,
 	&ufs_sysfs_geometry_descriptor_group,
 	&ufs_sysfs_health_descriptor_group,
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index
1f99904..0b497fc 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -427,6 +427,7 @@ enum {
 };
 
 struct ufs_unit_desc {
+	u8	   LUNum;
 	u8     bLUEnable;              /* 1 for enabled LU */
 	u8     bBootLunID;             /* 0 for using this LU for boot */
 	u8     bLUWriteProtect;        /* 1 = power on WP, 2 = permanent WP
*/
@@ -451,6 +452,7 @@ struct ufs_config_descr {
 	u32    qVendorConfigCode;      /* Vendor specific configuration code
*/
 	struct ufs_unit_desc unit[8];
 	u8	lun_to_grow;
+	u8 num_luns;
 };
 
 /* Task management service response */
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
3fd29e0..122c119 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1580,6 +1580,131 @@ void ufshcd_release(struct ufs_hba *hba)  }
EXPORT_SYMBOL_GPL(ufshcd_release);
 
+ssize_t ufshcd_desc_config_show(struct device *dev,
+		struct device_attribute *attr, char *buf) {
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "provision_enabled = %d\n",
+		hba->provision_enabled);
+}
+
+ssize_t ufshcd_desc_config_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t
count) {
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct ufs_config_descr *cfg = &hba->cfgs;
+	char *strbuf;
+	char *strbuf_copy;
+	int desc_buf[count];
+	int *pt;
+	char *token;
+	int i, ret;
+	int value, commit = 0;
+	int num_luns = 0;
+	int KB_per_block = 4;
+
+	/* reserve one byte for null termination */
+	strbuf = kmalloc(count + 1, GFP_KERNEL);
+	if (!strbuf)
+		return -ENOMEM;
+
+	strbuf_copy = strbuf;
+	strlcpy(strbuf, buf, count + 1);
+	memset(desc_buf, 0, count);
+
+	/* Just return if bConfigDescrLock is already set */
+	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+		QUERY_ATTR_IDN_CONF_DESC_LOCK, 0, 0,
&cfg->bConfigDescrLock);
+	if (ret) {
+		dev_err(hba->dev, "%s: Failed reading bConfigDescrLock %d,
cannot re-provision device!\n",
+			__func__, ret);
+		hba->provision_enabled = 0;
+		goto out;
+	}
+	if (cfg->bConfigDescrLock == 1) {
+		dev_err(hba->dev, "%s: bConfigDescrLock already set to %u,
cannot re-provision device!\n",
+		__func__, cfg->bConfigDescrLock);
+		hba->provision_enabled = 0;
+		goto out;
+	}
+
+	for (i = 0; i < count; i++) {
+		token = strsep(&strbuf, " ");
+		if (!token && i) {
+			num_luns = desc_buf[i-1];
+			dev_dbg(hba->dev, "%s: token %s, num_luns %d\n",
+				__func__, token, num_luns);
+			if (num_luns > 8) {
+				dev_err(hba->dev, "%s: Invalid num_luns
%d\n",
+				__func__, num_luns);
+				hba->provision_enabled = 0;
+				goto out;
+			}
+			break;
+		}
+
+		ret = kstrtoint(token, 0, &value);
+		if (ret) {
+			dev_err(hba->dev, "%s: kstrtoint failed %d %s\n",
+				__func__, ret, token);
+			break;
+		}
+		desc_buf[i] = value;
+		dev_dbg(hba->dev, " desc_buf[%d] 0x%x", i, desc_buf[i]);
+	}
+
+	/* Fill in the descriptors with parsed configuration data */
+	pt = desc_buf;
+	cfg->bNumberLU = *pt++;
+	cfg->bBootEnable = *pt++;
+	cfg->bDescrAccessEn = *pt++;
+	cfg->bInitPowerMode = *pt++;
+	cfg->bHighPriorityLUN = *pt++;
+	cfg->bSecureRemovalType = *pt++;
+	cfg->bInitActiveICCLevel = *pt++;
+	cfg->wPeriodicRTCUpdate = *pt++;
+	cfg->bConfigDescrLock = *pt++;
+	dev_dbg(hba->dev, "%s: %u %u %u %u %u %u %u %u %u\n", __func__,
+	cfg->bNumberLU, cfg->bBootEnable, cfg->bDescrAccessEn,
+	cfg->bInitPowerMode, cfg->bHighPriorityLUN, cfg->bSecureRemovalType,
+	cfg->bInitActiveICCLevel, cfg->wPeriodicRTCUpdate,
+	cfg->bConfigDescrLock);
+
+	for (i = 0; i < num_luns; i++) {
+		cfg->unit[i].LUNum = *pt++;
+		cfg->unit[i].bLUEnable = *pt++;
+		cfg->unit[i].bBootLunID = *pt++;
+		/* dNumAllocUnits = size_in_kb/KB_per_block */
+		cfg->unit[i].dNumAllocUnits = (u32)(*pt++ / KB_per_block);
+		cfg->unit[i].bDataReliability = *pt++;
+		cfg->unit[i].bLUWriteProtect = *pt++;
+		cfg->unit[i].bMemoryType = *pt++;
+		cfg->unit[i].bLogicalBlockSize = *pt++;
+		cfg->unit[i].bProvisioningType = *pt++;
+		cfg->unit[i].wContextCapabilities = *pt++;
+	}
+
+	cfg->lun_to_grow = *pt++;
+	commit = *pt++;
+	cfg->num_luns = *pt;
+	dev_dbg(hba->dev, "%s: lun_to_grow %u, commit %u num_luns %u\n",
+		__func__, cfg->lun_to_grow, commit, cfg->num_luns);
+	if (commit == 1) {
+		ret = ufshcd_do_config_device(hba);
+		if (!ret) {
+			hba->provision_enabled = 1;
+			dev_err(hba->dev,
+			"%s: UFS Provisioning completed,num_luns %u, reboot
now !\n",
+			__func__, cfg->num_luns);
+		}
+	} else
+		dev_err(hba->dev, "%s: Invalid commit %u\n", __func__,
commit);
+out:
+	kfree(strbuf_copy);
+	return count;
+}
+
 static ssize_t ufshcd_clkgate_delay_show(struct device *dev,
 		struct device_attribute *attr, char *buf)  { @@ -6532,6
+6657,9 @@ static int ufshcd_do_config_device(struct ufs_hba *hba)
 		pt = pt + 3; // Reserved fields set to 0
 	}
 
+	for (i = 0; i < buff_len; i++)
+		dev_dbg(hba->dev, " desc_buf[%d] 0x%x", i, desc_buf[i]);
+
 	ret = ufshcd_query_descriptor_retry(hba,
UPIU_QUERY_OPCODE_WRITE_DESC,
 		QUERY_DESC_IDN_CONFIGURATION, 0, 0, desc_buf, &buff_len);
 
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index
982bfdf..1b8304f 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -651,6 +651,7 @@ struct ufs_hba {
 	struct ufs_pwr_mode_info max_pwr_info;
 
 	struct ufs_clk_gating clk_gating;
+	bool provision_enabled;
 	/* Control to enable/disable host capabilities */
 	u32 caps;
 	/* Allow dynamic clk gating */
@@ -867,6 +868,10 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, int
desc_index,
 
 int ufshcd_hold(struct ufs_hba *hba, bool async);  void
ufshcd_release(struct ufs_hba *hba);
+ssize_t ufshcd_desc_config_show(struct device *dev,
+		struct device_attribute *attr, char *buf); ssize_t 
+ufshcd_desc_config_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t
count);
 
 int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn
desc_id,
 	int *desc_length);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project

Re: [PATCH V1 3/3] scsi: ufs: Add sysfs support for ufs provision

[Evan Green]

On Fri, Jun 1, 2018 at 3:49 AM sayali <sayalil@codeaurora.org> wrote:
>
> Hi Evan,
>
> I am working on upstreaming runtime UFS provisioning patches and have posted
> my latest (V1) patches for review.
> I have noticed that you are also working on the same and would appreciate if
> you could also review my patches.
> I believe it will save us all some time and duplicate effort. Let me know if
> you have any concern or comments on this.
>
> Thanks,
> Sayali
>

Hi Sayali,
I am currently travelling for work, but will make an effort to review
your patches soon. It looks like we did a bit of duplicate work. Maybe
the maintainers or community at large could let us know which approach
they prefer, or if they want both.
-Evan

Re: [PATCH V1 3/3] scsi: ufs: Add sysfs support for ufs provision

[Greg Kroah-Hartman]

On Tue, Jun 05, 2018 at 08:16:50AM +0000, Stanislav Nijnikov wrote:
> Hi Sayali,
> 
> I think that passing an array of values in a string is not proper way
> to work with a sysfs entry. There are binary attributes to do such
> things.

No, don't do that, sysfs is for "one value per file", and binary
attributes are for "hardware value pass-through" type stuff.  Unless
this is "raw" data straight from the hardware, binary does not work, and
neither does a normal sysfs file either.

So this needs to be reworked please.

thanks,

greg k-h

RE: [PATCH V1 3/3] scsi: ufs: Add sysfs support for ufs provision

[sayali]

Hi Greg / Stanislav,

Thank you for your comments. 
Updated my comments inline. Please check.

Thanks,
Sayali
-----Original Message-----
From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] 
Sent: Tuesday, June 05, 2018 2:12 PM
To: Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
Cc: Sayali Lokhande <sayalil@codeaurora.org>; subhashj@codeaurora.org;
cang@codeaurora.org; vivek.gautam@codeaurora.org; rnayak@codeaurora.org;
vinholikatti@gmail.com; jejb@linux.vnet.ibm.com; martin.petersen@oracle.com;
asutoshd@codeaurora.org; evgreen@chromium.org; Bart Van Assche
<Bart.VanAssche@wdc.com>; linux-scsi@vger.kernel.org; Adrian Hunter
<adrian.hunter@intel.com>; open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V1 3/3] scsi: ufs: Add sysfs support for ufs provision

On Tue, Jun 05, 2018 at 08:16:50AM +0000, Stanislav Nijnikov wrote:
> Hi Sayali,
> 
> I think that passing an array of values in a string is not proper way 
> to work with a sysfs entry. There are binary attributes to do such 
> things.

No, don't do that, sysfs is for "one value per file", and binary attributes
are for "hardware value pass-through" type stuff.  Unless this is "raw" data
straight from the hardware, binary does not work, and neither does a normal
sysfs file either.

So this needs to be reworked please.

[Sayali] As per Documentation/filesystems/sysfs.txt :
	"Attributes should be ASCII text files, preferably with only one
value per file. It is noted that it may not be efficient to contain only one
value per file, 	so it is socially acceptable to express an array of
values of the same type."
	So it seems sysfs can be used to pass more than one value given that
they are of same type (which is ensured as I am passing char string).
	Also I have noticed , in scsi_sysfs.c (store_scan() or scsi_scan()),
we do pass char buffer (more than one value) via sysfs .
	As sysfs is already being used to read descriptors, I thought of
using sysfs as write interface for configuration descriptor.
	Appreciate, if you could suggest me some other interfaces that can
be used here (allow passing more than one value) or
	do you think I need to pass each configurable parameter one by one
for provisioning ?

thanks,

greg k-h