2021-03-21 23:43:08

by Nitin Rawat

[permalink] [raw]
Subject: [PATCH V2 0/3] scsi: ufs: Add a vops to configure VCC voltage level

UFS specification allows different VCC configurations for UFS devices,
for example,
(1)2.70V - 3.60V (For UFS 2.x devices)
(2)2.40V - 2.70V (For UFS 3.x devices)
For platforms supporting both ufs 2.x (2.7v-3.6v) and
ufs 3.x (2.4v-2.7v), the voltage requirements (VCC) is 2.4v-3.6v.
So to support this, we need to start the ufs device initialization with
the common VCC voltage(2.7v) and after reading the device descriptor we
need to switch to the correct range(vcc min and vcc max) of VCC voltage
as per UFS device type since 2.7v is the marginal voltage as per specs
for both type of devices.

Once VCC regulator supply has been intialised to 2.7v and UFS device
type is read from device descriptor, we follows below steps to
change the VCC voltage values.

1. Set the device to SLEEP state.
2. Disable the Vcc Regulator.
3. Set the vcc voltage according to the device type and reenable
the regulator.
4. Set the device mode back to ACTIVE.

The above changes are done in vendor specific file by
adding a vops which will be needed for platform
supporting both ufs 2.x and ufs 3.x devices.

v1 -> v2
Added suggested-by on patch2 (scsi: ufs: add a vops to configure VCC voltage level)

Nitin Rawat (3):
scsi: ufs: export api for use in vendor file
scsi: ufs: add a vops to configure VCC voltage level
scsi: ufs-qcom: configure VCC voltage level in vendor file

drivers/scsi/ufs/ufs-qcom.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
drivers/scsi/ufs/ufshcd.c | 13 +++++++++---
drivers/scsi/ufs/ufshcd.h | 14 +++++++++++++
3 files changed, 75 insertions(+), 3 deletions(-)

--
2.7.4


2021-03-21 23:43:19

by Nitin Rawat

[permalink] [raw]
Subject: [PATCH V2 1/3] scsi: ufs: export api for use in vendor file

Exporting functions ufshcd_set_dev_pwr_mode, ufshcd_disable_vreg
and ufshcd_enable_vreg so that vendor drivers can make use of
them in setting vendor specific regulator setting
in vendor specific file.

Signed-off-by: Nitin Rawat <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 9 ++++++---
drivers/scsi/ufs/ufshcd.h | 4 ++++
2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 80620c8..633ca8e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8081,7 +8081,7 @@ static int ufshcd_config_vreg(struct device *dev,
return ret;
}

-static int ufshcd_enable_vreg(struct device *dev, struct ufs_vreg *vreg)
+int ufshcd_enable_vreg(struct device *dev, struct ufs_vreg *vreg)
{
int ret = 0;

@@ -8100,8 +8100,9 @@ static int ufshcd_enable_vreg(struct device *dev, struct ufs_vreg *vreg)
out:
return ret;
}
+EXPORT_SYMBOL(ufshcd_enable_vreg);

-static int ufshcd_disable_vreg(struct device *dev, struct ufs_vreg *vreg)
+int ufshcd_disable_vreg(struct device *dev, struct ufs_vreg *vreg)
{
int ret = 0;

@@ -8121,6 +8122,7 @@ static int ufshcd_disable_vreg(struct device *dev, struct ufs_vreg *vreg)
out:
return ret;
}
+EXPORT_SYMBOL(ufshcd_disable_vreg);

static int ufshcd_setup_vreg(struct ufs_hba *hba, bool on)
{
@@ -8445,7 +8447,7 @@ ufshcd_send_request_sense(struct ufs_hba *hba, struct scsi_device *sdp)
* Returns 0 if requested power mode is set successfully
* Returns non-zero if failed to set the requested power mode
*/
-static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
+int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
enum ufs_dev_pwr_mode pwr_mode)
{
unsigned char cmd[6] = { START_STOP };
@@ -8503,6 +8505,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
hba->host->eh_noresume = 0;
return ret;
}
+EXPORT_SYMBOL(ufshcd_set_dev_pwr_mode);

static int ufshcd_link_state_transition(struct ufs_hba *hba,
enum uic_link_state req_link_state,
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 18e56c1..0db796a 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -997,6 +997,10 @@ extern int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
u32 *mib_val, u8 peer);
extern int ufshcd_config_pwr_mode(struct ufs_hba *hba,
struct ufs_pa_layer_attr *desired_pwr_mode);
+extern int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
+ enum ufs_dev_pwr_mode pwr_mode);
+extern int ufshcd_enable_vreg(struct device *dev, struct ufs_vreg *vreg);
+extern int ufshcd_disable_vreg(struct device *dev, struct ufs_vreg *vreg);

/* UIC command interfaces for DME primitives */
#define DME_LOCAL 0
--
2.7.4

2021-03-21 23:44:53

by Nitin Rawat

[permalink] [raw]
Subject: [PATCH V2 2/3] scsi: ufs: add a vops to configure VCC voltage level

Add a vops to configure VCC voltage VCC voltage level
for platform supporting both ufs2.x and ufs 3.x devices.

Suggested-by: Stanley Chu <[email protected]>
Suggested-by: Asutosh Das <[email protected]>
Suggested-by: Bjorn Andersson <[email protected]>
Signed-off-by: Nitin Rawat <[email protected]>
Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 4 ++++
drivers/scsi/ufs/ufshcd.h | 10 ++++++++++
2 files changed, 14 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 633ca8e..5bfe987 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7763,6 +7763,10 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
goto out;

ufshcd_clear_ua_wluns(hba);
+ if (ufshcd_vops_setup_vcc_regulators(hba))
+ dev_err(hba->dev,
+ "%s: Failed to set the VCC regulator values, continue with 2.7v\n",
+ __func__);

/* Initialize devfreq after UFS device is detected */
if (ufshcd_is_clkscaling_supported(hba)) {
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 0db796a..8f0945d 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -324,6 +324,7 @@ struct ufs_pwr_mode_info {
* @device_reset: called to issue a reset pulse on the UFS device
* @program_key: program or evict an inline encryption key
* @event_notify: called to notify important events
+ * @setup_vcc_regulators : update vcc regulator level
*/
struct ufs_hba_variant_ops {
const char *name;
@@ -360,6 +361,7 @@ struct ufs_hba_variant_ops {
const union ufs_crypto_cfg_entry *cfg, int slot);
void (*event_notify)(struct ufs_hba *hba,
enum ufs_event_type evt, void *data);
+ int (*setup_vcc_regulators)(struct ufs_hba *hba);
};

/* clock gating state */
@@ -1269,6 +1271,14 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
hba->vops->config_scaling_param(hba, profile, data);
}

+static inline int ufshcd_vops_setup_vcc_regulators(struct ufs_hba *hba)
+{
+ if (hba->vops && hba->vops->setup_vcc_regulators)
+ return hba->vops->setup_vcc_regulators(hba);
+
+ return 0;
+}
+
extern struct ufs_pm_lvl_states ufs_pm_lvl_states[];

/*
--
2.7.4

2021-03-21 23:44:54

by Nitin Rawat

[permalink] [raw]
Subject: [PATCH V2 3/3] scsi: ufs-qcom: configure VCC voltage level in vendor file

As a part of vops handler, VCC voltage is updated
as per the ufs device probed after reading the device
descriptor. We follow below steps to configure voltage
level.

1. Set the device to SLEEP state.
2. Disable the Vcc Regulator.
3. Set the vcc voltage according to the device type and reenable
the regulator.
4. Set the device mode back to ACTIVE.

Signed-off-by: Nitin Rawat <[email protected]>
Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
---
drivers/scsi/ufs/ufs-qcom.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index f97d7b0..ca35f5c 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -21,6 +21,17 @@
#define UFS_QCOM_DEFAULT_DBG_PRINT_EN \
(UFS_QCOM_DBG_PRINT_REGS_EN | UFS_QCOM_DBG_PRINT_TEST_BUS_EN)

+#define ANDROID_BOOT_DEV_MAX 30
+static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
+
+/* Min and Max VCC voltage values for ufs 2.x and
+ * ufs 3.x devices
+ */
+#define UFS_3X_VREG_VCC_MIN_UV 2540000 /* uV */
+#define UFS_3X_VREG_VCC_MAX_UV 2700000 /* uV */
+#define UFS_2X_VREG_VCC_MIN_UV 2950000 /* uV */
+#define UFS_2X_VREG_VCC_MAX_UV 2960000 /* uV */
+
enum {
TSTBUS_UAWM,
TSTBUS_UARM,
@@ -1293,6 +1304,45 @@ static void ufs_qcom_print_hw_debug_reg_all(struct ufs_hba *hba,
print_fn(hba, reg, 9, "UFS_DBG_RD_REG_TMRLUT ", priv);
}

+ /**
+ * ufs_qcom_setup_vcc_regulators - Update VCC voltage
+ * @hba: host controller instance
+ * Update VCC voltage based on UFS device(ufs 2.x or
+ * ufs 3.x probed)
+ */
+static int ufs_qcom_setup_vcc_regulators(struct ufs_hba *hba)
+{
+ struct ufs_dev_info *dev_info = &hba->dev_info;
+ struct ufs_vreg *vreg = hba->vreg_info.vcc;
+ int ret;
+
+ /* Put the device in sleep before lowering VCC level */
+ ret = ufshcd_set_dev_pwr_mode(hba, UFS_SLEEP_PWR_MODE);
+
+ /* Switch off VCC before switching it ON at 2.5v or 2.96v */
+ ret = ufshcd_disable_vreg(hba->dev, vreg);
+
+ /* add ~2ms delay before renabling VCC at lower voltage */
+ usleep_range(2000, 2100);
+
+ /* set VCC min and max voltage according to ufs device type */
+ if (dev_info->wspecversion >= 0x300) {
+ vreg->min_uV = UFS_3X_VREG_VCC_MIN_UV;
+ vreg->max_uV = UFS_3X_VREG_VCC_MAX_UV;
+ }
+
+ else {
+ vreg->min_uV = UFS_2X_VREG_VCC_MIN_UV;
+ vreg->max_uV = UFS_2X_VREG_VCC_MAX_UV;
+ }
+
+ ret = ufshcd_enable_vreg(hba->dev, vreg);
+
+ /* Bring the device in active now */
+ ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
+ return ret;
+}
+
static void ufs_qcom_enable_test_bus(struct ufs_qcom_host *host)
{
if (host->dbg_print_en & UFS_QCOM_DBG_PRINT_TEST_BUS_EN) {
@@ -1490,6 +1540,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
.device_reset = ufs_qcom_device_reset,
.config_scaling_param = ufs_qcom_config_scaling_param,
.program_key = ufs_qcom_ice_program_key,
+ .setup_vcc_regulators = ufs_qcom_setup_vcc_regulators,
};

/**
--
2.7.4

2021-03-23 15:31:33

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH V2 3/3] scsi: ufs-qcom: configure VCC voltage level in vendor file

On Sun 21 Mar 16:57 CDT 2021, Nitin Rawat wrote:

> As a part of vops handler, VCC voltage is updated
> as per the ufs device probed after reading the device
> descriptor. We follow below steps to configure voltage
> level.
>
> 1. Set the device to SLEEP state.
> 2. Disable the Vcc Regulator.
> 3. Set the vcc voltage according to the device type and reenable
> the regulator.
> 4. Set the device mode back to ACTIVE.
>

When we discussed this a while back this was described as a requirement
from the device specification, you only operate on objects "owned" by
ufshcd and you invoke ufshcd operations to perform the actions.

So why is this a ufs-qcom patch and not something in ufshcd?

Regards,
Bjorn

> Signed-off-by: Nitin Rawat <[email protected]>
> Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
> ---
> drivers/scsi/ufs/ufs-qcom.c | 51 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index f97d7b0..ca35f5c 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -21,6 +21,17 @@
> #define UFS_QCOM_DEFAULT_DBG_PRINT_EN \
> (UFS_QCOM_DBG_PRINT_REGS_EN | UFS_QCOM_DBG_PRINT_TEST_BUS_EN)
>
> +#define ANDROID_BOOT_DEV_MAX 30
> +static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
> +
> +/* Min and Max VCC voltage values for ufs 2.x and
> + * ufs 3.x devices
> + */
> +#define UFS_3X_VREG_VCC_MIN_UV 2540000 /* uV */
> +#define UFS_3X_VREG_VCC_MAX_UV 2700000 /* uV */
> +#define UFS_2X_VREG_VCC_MIN_UV 2950000 /* uV */
> +#define UFS_2X_VREG_VCC_MAX_UV 2960000 /* uV */
> +
> enum {
> TSTBUS_UAWM,
> TSTBUS_UARM,
> @@ -1293,6 +1304,45 @@ static void ufs_qcom_print_hw_debug_reg_all(struct ufs_hba *hba,
> print_fn(hba, reg, 9, "UFS_DBG_RD_REG_TMRLUT ", priv);
> }
>
> + /**
> + * ufs_qcom_setup_vcc_regulators - Update VCC voltage
> + * @hba: host controller instance
> + * Update VCC voltage based on UFS device(ufs 2.x or
> + * ufs 3.x probed)
> + */
> +static int ufs_qcom_setup_vcc_regulators(struct ufs_hba *hba)
> +{
> + struct ufs_dev_info *dev_info = &hba->dev_info;
> + struct ufs_vreg *vreg = hba->vreg_info.vcc;
> + int ret;
> +
> + /* Put the device in sleep before lowering VCC level */
> + ret = ufshcd_set_dev_pwr_mode(hba, UFS_SLEEP_PWR_MODE);
> +
> + /* Switch off VCC before switching it ON at 2.5v or 2.96v */
> + ret = ufshcd_disable_vreg(hba->dev, vreg);
> +
> + /* add ~2ms delay before renabling VCC at lower voltage */
> + usleep_range(2000, 2100);
> +
> + /* set VCC min and max voltage according to ufs device type */
> + if (dev_info->wspecversion >= 0x300) {
> + vreg->min_uV = UFS_3X_VREG_VCC_MIN_UV;
> + vreg->max_uV = UFS_3X_VREG_VCC_MAX_UV;
> + }
> +
> + else {
> + vreg->min_uV = UFS_2X_VREG_VCC_MIN_UV;
> + vreg->max_uV = UFS_2X_VREG_VCC_MAX_UV;
> + }
> +
> + ret = ufshcd_enable_vreg(hba->dev, vreg);
> +
> + /* Bring the device in active now */
> + ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
> + return ret;
> +}
> +
> static void ufs_qcom_enable_test_bus(struct ufs_qcom_host *host)
> {
> if (host->dbg_print_en & UFS_QCOM_DBG_PRINT_TEST_BUS_EN) {
> @@ -1490,6 +1540,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
> .device_reset = ufs_qcom_device_reset,
> .config_scaling_param = ufs_qcom_config_scaling_param,
> .program_key = ufs_qcom_ice_program_key,
> + .setup_vcc_regulators = ufs_qcom_setup_vcc_regulators,
> };
>
> /**
> --
> 2.7.4
>

2021-03-25 03:32:16

by Nitin Rawat

[permalink] [raw]
Subject: Re: [PATCH V2 3/3] scsi: ufs-qcom: configure VCC voltage level in vendor file

On 2021-03-23 20:58, Bjorn Andersson wrote:
> On Sun 21 Mar 16:57 CDT 2021, Nitin Rawat wrote:
>
>> As a part of vops handler, VCC voltage is updated
>> as per the ufs device probed after reading the device
>> descriptor. We follow below steps to configure voltage
>> level.
>>
>> 1. Set the device to SLEEP state.
>> 2. Disable the Vcc Regulator.
>> 3. Set the vcc voltage according to the device type and reenable
>> the regulator.
>> 4. Set the device mode back to ACTIVE.
>>
>
> When we discussed this a while back this was described as a requirement
> from the device specification, you only operate on objects "owned" by
> ufshcd and you invoke ufshcd operations to perform the actions.
>
> So why is this a ufs-qcom patch and not something in ufshcd?
>
> Regards,
> Bjorn
>
>> Signed-off-by: Nitin Rawat <[email protected]>
>> Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
>> ---
>> drivers/scsi/ufs/ufs-qcom.c | 51
>> +++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 51 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
>> index f97d7b0..ca35f5c 100644
>> --- a/drivers/scsi/ufs/ufs-qcom.c
>> +++ b/drivers/scsi/ufs/ufs-qcom.c
>> @@ -21,6 +21,17 @@
>> #define UFS_QCOM_DEFAULT_DBG_PRINT_EN \
>> (UFS_QCOM_DBG_PRINT_REGS_EN | UFS_QCOM_DBG_PRINT_TEST_BUS_EN)
>>
>> +#define ANDROID_BOOT_DEV_MAX 30
>> +static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
>> +
>> +/* Min and Max VCC voltage values for ufs 2.x and
>> + * ufs 3.x devices
>> + */
>> +#define UFS_3X_VREG_VCC_MIN_UV 2540000 /* uV */
>> +#define UFS_3X_VREG_VCC_MAX_UV 2700000 /* uV */
>> +#define UFS_2X_VREG_VCC_MIN_UV 2950000 /* uV */
>> +#define UFS_2X_VREG_VCC_MAX_UV 2960000 /* uV */
>> +
>> enum {
>> TSTBUS_UAWM,
>> TSTBUS_UARM,
>> @@ -1293,6 +1304,45 @@ static void
>> ufs_qcom_print_hw_debug_reg_all(struct ufs_hba *hba,
>> print_fn(hba, reg, 9, "UFS_DBG_RD_REG_TMRLUT ", priv);
>> }
>>
>> + /**
>> + * ufs_qcom_setup_vcc_regulators - Update VCC voltage
>> + * @hba: host controller instance
>> + * Update VCC voltage based on UFS device(ufs 2.x or
>> + * ufs 3.x probed)
>> + */
>> +static int ufs_qcom_setup_vcc_regulators(struct ufs_hba *hba)
>> +{
>> + struct ufs_dev_info *dev_info = &hba->dev_info;
>> + struct ufs_vreg *vreg = hba->vreg_info.vcc;
>> + int ret;
>> +
>> + /* Put the device in sleep before lowering VCC level */
>> + ret = ufshcd_set_dev_pwr_mode(hba, UFS_SLEEP_PWR_MODE);
>> +
>> + /* Switch off VCC before switching it ON at 2.5v or 2.96v */
>> + ret = ufshcd_disable_vreg(hba->dev, vreg);
>> +
>> + /* add ~2ms delay before renabling VCC at lower voltage */
>> + usleep_range(2000, 2100);
>> +
>> + /* set VCC min and max voltage according to ufs device type */
>> + if (dev_info->wspecversion >= 0x300) {
>> + vreg->min_uV = UFS_3X_VREG_VCC_MIN_UV;
>> + vreg->max_uV = UFS_3X_VREG_VCC_MAX_UV;
>> + }
>> +
>> + else {
>> + vreg->min_uV = UFS_2X_VREG_VCC_MIN_UV;
>> + vreg->max_uV = UFS_2X_VREG_VCC_MAX_UV;
>> + }
>> +
>> + ret = ufshcd_enable_vreg(hba->dev, vreg);
>> +
>> + /* Bring the device in active now */
>> + ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
>> + return ret;
>> +}
>> +
>> static void ufs_qcom_enable_test_bus(struct ufs_qcom_host *host)
>> {
>> if (host->dbg_print_en & UFS_QCOM_DBG_PRINT_TEST_BUS_EN) {
>> @@ -1490,6 +1540,7 @@ static const struct ufs_hba_variant_ops
>> ufs_hba_qcom_vops = {
>> .device_reset = ufs_qcom_device_reset,
>> .config_scaling_param = ufs_qcom_config_scaling_param,
>> .program_key = ufs_qcom_ice_program_key,
>> + .setup_vcc_regulators = ufs_qcom_setup_vcc_regulators,
>> };
>>
>> /**
>> --
>> 2.7.4
>>

Hi Bjorn,
Thanks for your review.
But As per the earlier discussion regarding handling of vcc voltage
for platform supporting both ufs 2.x and ufs 3.x , it was finally
concluded to
use "vops and let vendors handle it, until specs or someone
has a better suggestion". Please correct me in case i am wrong.

https://www.mail-archive.com/[email protected]/msg2399116.html

Regards,
Nitin

2021-03-31 18:03:03

by Asutosh Das (asd)

[permalink] [raw]
Subject: Re: [PATCH V2 2/3] scsi: ufs: add a vops to configure VCC voltage level

On 3/21/2021 2:57 PM, Nitin Rawat wrote:
> Add a vops to configure VCC voltage VCC voltage level
> for platform supporting both ufs2.x and ufs 3.x devices.
>
> Suggested-by: Stanley Chu <[email protected]>
> Suggested-by: Asutosh Das <[email protected]>
> Suggested-by: Bjorn Andersson <[email protected]>
> Signed-off-by: Nitin Rawat <[email protected]>
> Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 4 ++++
> drivers/scsi/ufs/ufshcd.h | 10 ++++++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 633ca8e..5bfe987 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7763,6 +7763,10 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
> goto out;
>
> ufshcd_clear_ua_wluns(hba);
> + if (ufshcd_vops_setup_vcc_regulators(hba))
This would be invoked even for platforms that don't support both 2.x and
3.x and don't need to set the voltages in the driver.
I guess platforms that support both 2.x and 3.x and can't set the
regulator voltages from dts due to different voltage requirements of 2.x
and 3.x, should request the driver to set the voltages. And the driver
may do so after determining the device version.

> + dev_err(hba->dev,
> + "%s: Failed to set the VCC regulator values, continue with 2.7v\n",
> + __func__);
>
> /* Initialize devfreq after UFS device is detected */
> if (ufshcd_is_clkscaling_supported(hba)) {
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 0db796a..8f0945d 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -324,6 +324,7 @@ struct ufs_pwr_mode_info {
> * @device_reset: called to issue a reset pulse on the UFS device
> * @program_key: program or evict an inline encryption key
> * @event_notify: called to notify important events
> + * @setup_vcc_regulators : update vcc regulator level
> */
> struct ufs_hba_variant_ops {
> const char *name;
> @@ -360,6 +361,7 @@ struct ufs_hba_variant_ops {
> const union ufs_crypto_cfg_entry *cfg, int slot);
> void (*event_notify)(struct ufs_hba *hba,
> enum ufs_event_type evt, void *data);
> + int (*setup_vcc_regulators)(struct ufs_hba *hba);
> };
>
> /* clock gating state */
> @@ -1269,6 +1271,14 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
> hba->vops->config_scaling_param(hba, profile, data);
> }
>
> +static inline int ufshcd_vops_setup_vcc_regulators(struct ufs_hba *hba)
> +{
> + if (hba->vops && hba->vops->setup_vcc_regulators)
> + return hba->vops->setup_vcc_regulators(hba);
> +
> + return 0;
> +}
> +
> extern struct ufs_pm_lvl_states ufs_pm_lvl_states[];
>
> /*
> --
> 2.7.4
>


--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

2021-03-31 18:23:05

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH V2 3/3] scsi: ufs-qcom: configure VCC voltage level in vendor file

On Wed 24 Mar 16:55 CDT 2021, [email protected] wrote:

> On 2021-03-23 20:58, Bjorn Andersson wrote:
> > On Sun 21 Mar 16:57 CDT 2021, Nitin Rawat wrote:
> >
> > > As a part of vops handler, VCC voltage is updated
> > > as per the ufs device probed after reading the device
> > > descriptor. We follow below steps to configure voltage
> > > level.
> > >
> > > 1. Set the device to SLEEP state.
> > > 2. Disable the Vcc Regulator.
> > > 3. Set the vcc voltage according to the device type and reenable
> > > the regulator.
> > > 4. Set the device mode back to ACTIVE.
> > >
> >
> > When we discussed this a while back this was described as a requirement
> > from the device specification, you only operate on objects "owned" by
> > ufshcd and you invoke ufshcd operations to perform the actions.
> >
> > So why is this a ufs-qcom patch and not something in ufshcd?
> >
> > Regards,
> > Bjorn
> >
> > > Signed-off-by: Nitin Rawat <[email protected]>
> > > Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
> > > ---
> > > drivers/scsi/ufs/ufs-qcom.c | 51
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 51 insertions(+)
> > >
> > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> > > index f97d7b0..ca35f5c 100644
> > > --- a/drivers/scsi/ufs/ufs-qcom.c
> > > +++ b/drivers/scsi/ufs/ufs-qcom.c
> > > @@ -21,6 +21,17 @@
> > > #define UFS_QCOM_DEFAULT_DBG_PRINT_EN \
> > > (UFS_QCOM_DBG_PRINT_REGS_EN | UFS_QCOM_DBG_PRINT_TEST_BUS_EN)
> > >
> > > +#define ANDROID_BOOT_DEV_MAX 30
> > > +static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
> > > +
> > > +/* Min and Max VCC voltage values for ufs 2.x and
> > > + * ufs 3.x devices
> > > + */
> > > +#define UFS_3X_VREG_VCC_MIN_UV 2540000 /* uV */
> > > +#define UFS_3X_VREG_VCC_MAX_UV 2700000 /* uV */
> > > +#define UFS_2X_VREG_VCC_MIN_UV 2950000 /* uV */
> > > +#define UFS_2X_VREG_VCC_MAX_UV 2960000 /* uV */
> > > +
> > > enum {
> > > TSTBUS_UAWM,
> > > TSTBUS_UARM,
> > > @@ -1293,6 +1304,45 @@ static void
> > > ufs_qcom_print_hw_debug_reg_all(struct ufs_hba *hba,
> > > print_fn(hba, reg, 9, "UFS_DBG_RD_REG_TMRLUT ", priv);
> > > }
> > >
> > > + /**
> > > + * ufs_qcom_setup_vcc_regulators - Update VCC voltage
> > > + * @hba: host controller instance
> > > + * Update VCC voltage based on UFS device(ufs 2.x or
> > > + * ufs 3.x probed)
> > > + */
> > > +static int ufs_qcom_setup_vcc_regulators(struct ufs_hba *hba)
> > > +{
> > > + struct ufs_dev_info *dev_info = &hba->dev_info;
> > > + struct ufs_vreg *vreg = hba->vreg_info.vcc;
> > > + int ret;
> > > +
> > > + /* Put the device in sleep before lowering VCC level */
> > > + ret = ufshcd_set_dev_pwr_mode(hba, UFS_SLEEP_PWR_MODE);
> > > +
> > > + /* Switch off VCC before switching it ON at 2.5v or 2.96v */
> > > + ret = ufshcd_disable_vreg(hba->dev, vreg);
> > > +
> > > + /* add ~2ms delay before renabling VCC at lower voltage */
> > > + usleep_range(2000, 2100);
> > > +
> > > + /* set VCC min and max voltage according to ufs device type */
> > > + if (dev_info->wspecversion >= 0x300) {
> > > + vreg->min_uV = UFS_3X_VREG_VCC_MIN_UV;
> > > + vreg->max_uV = UFS_3X_VREG_VCC_MAX_UV;
> > > + }
> > > +
> > > + else {
> > > + vreg->min_uV = UFS_2X_VREG_VCC_MIN_UV;
> > > + vreg->max_uV = UFS_2X_VREG_VCC_MAX_UV;
> > > + }
> > > +
> > > + ret = ufshcd_enable_vreg(hba->dev, vreg);
> > > +
> > > + /* Bring the device in active now */
> > > + ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
> > > + return ret;
> > > +}
> > > +
> > > static void ufs_qcom_enable_test_bus(struct ufs_qcom_host *host)
> > > {
> > > if (host->dbg_print_en & UFS_QCOM_DBG_PRINT_TEST_BUS_EN) {
> > > @@ -1490,6 +1540,7 @@ static const struct ufs_hba_variant_ops
> > > ufs_hba_qcom_vops = {
> > > .device_reset = ufs_qcom_device_reset,
> > > .config_scaling_param = ufs_qcom_config_scaling_param,
> > > .program_key = ufs_qcom_ice_program_key,
> > > + .setup_vcc_regulators = ufs_qcom_setup_vcc_regulators,
> > > };
> > >
> > > /**
> > > --
> > > 2.7.4
> > >
>
> Hi Bjorn,
> Thanks for your review.
> But As per the earlier discussion regarding handling of vcc voltage
> for platform supporting both ufs 2.x and ufs 3.x , it was finally concluded
> to
> use "vops and let vendors handle it, until specs or someone
> has a better suggestion". Please correct me in case i am wrong.
>

I was under the impression that this would result in something custom
per platform, but what I'm objecting to now that I see the code is that
this is completely generic.

And the concerns we discussed regarding these regulators being shared
with other devices is not considered in this implementation. But in
practice I don't see how you could support 2.x, 3.x and rail sharing at
the same time.

Regards,
Bjorn

> https://www.mail-archive.com/[email protected]/msg2399116.html
>
> Regards,
> Nitin

2021-04-01 17:54:14

by Nitin Rawat

[permalink] [raw]
Subject: Re: [PATCH V2 3/3] scsi: ufs-qcom: configure VCC voltage level in vendor file

On 2021-03-31 23:49, Bjorn Andersson wrote:
> On Wed 24 Mar 16:55 CDT 2021, [email protected] wrote:
>
>> On 2021-03-23 20:58, Bjorn Andersson wrote:
>> > On Sun 21 Mar 16:57 CDT 2021, Nitin Rawat wrote:
>> >
>> > > As a part of vops handler, VCC voltage is updated
>> > > as per the ufs device probed after reading the device
>> > > descriptor. We follow below steps to configure voltage
>> > > level.
>> > >
>> > > 1. Set the device to SLEEP state.
>> > > 2. Disable the Vcc Regulator.
>> > > 3. Set the vcc voltage according to the device type and reenable
>> > > the regulator.
>> > > 4. Set the device mode back to ACTIVE.
>> > >
>> >
>> > When we discussed this a while back this was described as a requirement
>> > from the device specification, you only operate on objects "owned" by
>> > ufshcd and you invoke ufshcd operations to perform the actions.
>> >
>> > So why is this a ufs-qcom patch and not something in ufshcd?
>> >
>> > Regards,
>> > Bjorn
>> >
>> > > Signed-off-by: Nitin Rawat <[email protected]>
>> > > Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
>> > > ---
>> > > drivers/scsi/ufs/ufs-qcom.c | 51
>> > > +++++++++++++++++++++++++++++++++++++++++++++
>> > > 1 file changed, 51 insertions(+)
>> > >
>> > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
>> > > index f97d7b0..ca35f5c 100644
>> > > --- a/drivers/scsi/ufs/ufs-qcom.c
>> > > +++ b/drivers/scsi/ufs/ufs-qcom.c
>> > > @@ -21,6 +21,17 @@
>> > > #define UFS_QCOM_DEFAULT_DBG_PRINT_EN \
>> > > (UFS_QCOM_DBG_PRINT_REGS_EN | UFS_QCOM_DBG_PRINT_TEST_BUS_EN)
>> > >
>> > > +#define ANDROID_BOOT_DEV_MAX 30
>> > > +static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
>> > > +
>> > > +/* Min and Max VCC voltage values for ufs 2.x and
>> > > + * ufs 3.x devices
>> > > + */
>> > > +#define UFS_3X_VREG_VCC_MIN_UV 2540000 /* uV */
>> > > +#define UFS_3X_VREG_VCC_MAX_UV 2700000 /* uV */
>> > > +#define UFS_2X_VREG_VCC_MIN_UV 2950000 /* uV */
>> > > +#define UFS_2X_VREG_VCC_MAX_UV 2960000 /* uV */
>> > > +
>> > > enum {
>> > > TSTBUS_UAWM,
>> > > TSTBUS_UARM,
>> > > @@ -1293,6 +1304,45 @@ static void
>> > > ufs_qcom_print_hw_debug_reg_all(struct ufs_hba *hba,
>> > > print_fn(hba, reg, 9, "UFS_DBG_RD_REG_TMRLUT ", priv);
>> > > }
>> > >
>> > > + /**
>> > > + * ufs_qcom_setup_vcc_regulators - Update VCC voltage
>> > > + * @hba: host controller instance
>> > > + * Update VCC voltage based on UFS device(ufs 2.x or
>> > > + * ufs 3.x probed)
>> > > + */
>> > > +static int ufs_qcom_setup_vcc_regulators(struct ufs_hba *hba)
>> > > +{
>> > > + struct ufs_dev_info *dev_info = &hba->dev_info;
>> > > + struct ufs_vreg *vreg = hba->vreg_info.vcc;
>> > > + int ret;
>> > > +
>> > > + /* Put the device in sleep before lowering VCC level */
>> > > + ret = ufshcd_set_dev_pwr_mode(hba, UFS_SLEEP_PWR_MODE);
>> > > +
>> > > + /* Switch off VCC before switching it ON at 2.5v or 2.96v */
>> > > + ret = ufshcd_disable_vreg(hba->dev, vreg);
>> > > +
>> > > + /* add ~2ms delay before renabling VCC at lower voltage */
>> > > + usleep_range(2000, 2100);
>> > > +
>> > > + /* set VCC min and max voltage according to ufs device type */
>> > > + if (dev_info->wspecversion >= 0x300) {
>> > > + vreg->min_uV = UFS_3X_VREG_VCC_MIN_UV;
>> > > + vreg->max_uV = UFS_3X_VREG_VCC_MAX_UV;
>> > > + }
>> > > +
>> > > + else {
>> > > + vreg->min_uV = UFS_2X_VREG_VCC_MIN_UV;
>> > > + vreg->max_uV = UFS_2X_VREG_VCC_MAX_UV;
>> > > + }
>> > > +
>> > > + ret = ufshcd_enable_vreg(hba->dev, vreg);
>> > > +
>> > > + /* Bring the device in active now */
>> > > + ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
>> > > + return ret;
>> > > +}
>> > > +
>> > > static void ufs_qcom_enable_test_bus(struct ufs_qcom_host *host)
>> > > {
>> > > if (host->dbg_print_en & UFS_QCOM_DBG_PRINT_TEST_BUS_EN) {
>> > > @@ -1490,6 +1540,7 @@ static const struct ufs_hba_variant_ops
>> > > ufs_hba_qcom_vops = {
>> > > .device_reset = ufs_qcom_device_reset,
>> > > .config_scaling_param = ufs_qcom_config_scaling_param,
>> > > .program_key = ufs_qcom_ice_program_key,
>> > > + .setup_vcc_regulators = ufs_qcom_setup_vcc_regulators,
>> > > };
>> > >
>> > > /**
>> > > --
>> > > 2.7.4
>> > >
>>
>> Hi Bjorn,
>> Thanks for your review.
>> But As per the earlier discussion regarding handling of vcc voltage
>> for platform supporting both ufs 2.x and ufs 3.x , it was finally
>> concluded
>> to
>> use "vops and let vendors handle it, until specs or someone
>> has a better suggestion". Please correct me in case i am wrong.
>>
>
> I was under the impression that this would result in something custom
> per platform, but what I'm objecting to now that I see the code is that
> this is completely generic.
>
> And the concerns we discussed regarding these regulators being shared
> with other devices is not considered in this implementation. But in
> practice I don't see how you could support 2.x, 3.x and rail sharing at
> the same time.
>
> Regards,
> Bjorn
>
>> https://www.mail-archive.com/[email protected]/msg2399116.html
>>
>> Regards,
>> Nitin

Hi Bjorn,
Thanks for your feedback.
Regarding your query for regulator being shared with other device,
Imho, the soc/pmic designer should share only those device
with ufs regulator which has the same voltage range (2.4-3.6v).
If that is not considered by the pmic designer,
wouldn't that would be a board design issue ???

And I agree with you that - the code looks generic but
since the below steps is not part of the specs,
I had to keep it in vendor specific file for which I
had to export few api from ufshcd.c to use in vendor
specific files.

1. Set the device to SLEEP state.
2. Disable the Vcc Regulator.
3. Set the vcc voltage according to the device type and reenable
the regulator.
4. Set the device mode back to ACTIVE.

Please correct me if my understanding is not correct.

Regards,
Nitin

2021-04-01 17:57:19

by Nitin Rawat

[permalink] [raw]
Subject: Re: [PATCH V2 2/3] scsi: ufs: add a vops to configure VCC voltage level

On 2021-03-31 23:30, Asutosh Das (asd) wrote:
> On 3/21/2021 2:57 PM, Nitin Rawat wrote:
>> Add a vops to configure VCC voltage VCC voltage level
>> for platform supporting both ufs2.x and ufs 3.x devices.
>>
>> Suggested-by: Stanley Chu <[email protected]>
>> Suggested-by: Asutosh Das <[email protected]>
>> Suggested-by: Bjorn Andersson <[email protected]>
>> Signed-off-by: Nitin Rawat <[email protected]>
>> Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 4 ++++
>> drivers/scsi/ufs/ufshcd.h | 10 ++++++++++
>> 2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 633ca8e..5bfe987 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -7763,6 +7763,10 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
>> goto out;
>>
>> ufshcd_clear_ua_wluns(hba);
>> + if (ufshcd_vops_setup_vcc_regulators(hba))
> This would be invoked even for platforms that don't support both 2.x
> and 3.x and don't need to set the voltages in the driver.
> I guess platforms that support both 2.x and 3.x and can't set the
> regulator voltages from dts due to different voltage requirements of
> 2.x and 3.x, should request the driver to set the voltages. And the
> driver may do so after determining the device version.
>
>> + dev_err(hba->dev,
>> + "%s: Failed to set the VCC regulator values, continue with
>> 2.7v\n",
>> + __func__);
>>
>> /* Initialize devfreq after UFS device is detected */
>> if (ufshcd_is_clkscaling_supported(hba)) {
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index 0db796a..8f0945d 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -324,6 +324,7 @@ struct ufs_pwr_mode_info {
>> * @device_reset: called to issue a reset pulse on the UFS device
>> * @program_key: program or evict an inline encryption key
>> * @event_notify: called to notify important events
>> + * @setup_vcc_regulators : update vcc regulator level
>> */
>> struct ufs_hba_variant_ops {
>> const char *name;
>> @@ -360,6 +361,7 @@ struct ufs_hba_variant_ops {
>> const union ufs_crypto_cfg_entry *cfg, int slot);
>> void (*event_notify)(struct ufs_hba *hba,
>> enum ufs_event_type evt, void *data);
>> + int (*setup_vcc_regulators)(struct ufs_hba *hba);
>> };
>>
>> /* clock gating state */
>> @@ -1269,6 +1271,14 @@ static inline void
>> ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
>> hba->vops->config_scaling_param(hba, profile, data);
>> }
>>
>> +static inline int ufshcd_vops_setup_vcc_regulators(struct ufs_hba
>> *hba)
>> +{
>> + if (hba->vops && hba->vops->setup_vcc_regulators)
>> + return hba->vops->setup_vcc_regulators(hba);
>> +
>> + return 0;
>> +}
>> +
>> extern struct ufs_pm_lvl_states ufs_pm_lvl_states[];
>>
>> /*
>> --
>> 2.7.4
>>

Hi Asutosh,
Thanks for the suggestion. I will check and try to accommodate your
suggestion.
Regards,
Nitin

2021-04-01 17:57:37

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH V2 3/3] scsi: ufs-qcom: configure VCC voltage level in vendor file

On Thu 01 Apr 09:58 CDT 2021, [email protected] wrote:

> On 2021-03-31 23:49, Bjorn Andersson wrote:
> > On Wed 24 Mar 16:55 CDT 2021, [email protected] wrote:
> >
> > > On 2021-03-23 20:58, Bjorn Andersson wrote:
> > > > On Sun 21 Mar 16:57 CDT 2021, Nitin Rawat wrote:
> > > >
> > > > > As a part of vops handler, VCC voltage is updated
> > > > > as per the ufs device probed after reading the device
> > > > > descriptor. We follow below steps to configure voltage
> > > > > level.
> > > > >
> > > > > 1. Set the device to SLEEP state.
> > > > > 2. Disable the Vcc Regulator.
> > > > > 3. Set the vcc voltage according to the device type and reenable
> > > > > the regulator.
> > > > > 4. Set the device mode back to ACTIVE.
> > > > >
> > > >
> > > > When we discussed this a while back this was described as a requirement
> > > > from the device specification, you only operate on objects "owned" by
> > > > ufshcd and you invoke ufshcd operations to perform the actions.
> > > >
> > > > So why is this a ufs-qcom patch and not something in ufshcd?
> > > >
> > > > Regards,
> > > > Bjorn
> > > >
> > > > > Signed-off-by: Nitin Rawat <[email protected]>
> > > > > Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
> > > > > ---
> > > > > drivers/scsi/ufs/ufs-qcom.c | 51
> > > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 51 insertions(+)
> > > > >
> > > > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> > > > > index f97d7b0..ca35f5c 100644
> > > > > --- a/drivers/scsi/ufs/ufs-qcom.c
> > > > > +++ b/drivers/scsi/ufs/ufs-qcom.c
> > > > > @@ -21,6 +21,17 @@
> > > > > #define UFS_QCOM_DEFAULT_DBG_PRINT_EN \
> > > > > (UFS_QCOM_DBG_PRINT_REGS_EN | UFS_QCOM_DBG_PRINT_TEST_BUS_EN)
> > > > >
> > > > > +#define ANDROID_BOOT_DEV_MAX 30
> > > > > +static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
> > > > > +
> > > > > +/* Min and Max VCC voltage values for ufs 2.x and
> > > > > + * ufs 3.x devices
> > > > > + */
> > > > > +#define UFS_3X_VREG_VCC_MIN_UV 2540000 /* uV */
> > > > > +#define UFS_3X_VREG_VCC_MAX_UV 2700000 /* uV */
> > > > > +#define UFS_2X_VREG_VCC_MIN_UV 2950000 /* uV */
> > > > > +#define UFS_2X_VREG_VCC_MAX_UV 2960000 /* uV */
> > > > > +
> > > > > enum {
> > > > > TSTBUS_UAWM,
> > > > > TSTBUS_UARM,
> > > > > @@ -1293,6 +1304,45 @@ static void
> > > > > ufs_qcom_print_hw_debug_reg_all(struct ufs_hba *hba,
> > > > > print_fn(hba, reg, 9, "UFS_DBG_RD_REG_TMRLUT ", priv);
> > > > > }
> > > > >
> > > > > + /**
> > > > > + * ufs_qcom_setup_vcc_regulators - Update VCC voltage
> > > > > + * @hba: host controller instance
> > > > > + * Update VCC voltage based on UFS device(ufs 2.x or
> > > > > + * ufs 3.x probed)
> > > > > + */
> > > > > +static int ufs_qcom_setup_vcc_regulators(struct ufs_hba *hba)
> > > > > +{
> > > > > + struct ufs_dev_info *dev_info = &hba->dev_info;
> > > > > + struct ufs_vreg *vreg = hba->vreg_info.vcc;
> > > > > + int ret;
> > > > > +
> > > > > + /* Put the device in sleep before lowering VCC level */
> > > > > + ret = ufshcd_set_dev_pwr_mode(hba, UFS_SLEEP_PWR_MODE);
> > > > > +
> > > > > + /* Switch off VCC before switching it ON at 2.5v or 2.96v */
> > > > > + ret = ufshcd_disable_vreg(hba->dev, vreg);
> > > > > +
> > > > > + /* add ~2ms delay before renabling VCC at lower voltage */
> > > > > + usleep_range(2000, 2100);
> > > > > +
> > > > > + /* set VCC min and max voltage according to ufs device type */
> > > > > + if (dev_info->wspecversion >= 0x300) {
> > > > > + vreg->min_uV = UFS_3X_VREG_VCC_MIN_UV;
> > > > > + vreg->max_uV = UFS_3X_VREG_VCC_MAX_UV;
> > > > > + }
> > > > > +
> > > > > + else {
> > > > > + vreg->min_uV = UFS_2X_VREG_VCC_MIN_UV;
> > > > > + vreg->max_uV = UFS_2X_VREG_VCC_MAX_UV;
> > > > > + }
> > > > > +
> > > > > + ret = ufshcd_enable_vreg(hba->dev, vreg);
> > > > > +
> > > > > + /* Bring the device in active now */
> > > > > + ret = ufshcd_set_dev_pwr_mode(hba, UFS_ACTIVE_PWR_MODE);
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > static void ufs_qcom_enable_test_bus(struct ufs_qcom_host *host)
> > > > > {
> > > > > if (host->dbg_print_en & UFS_QCOM_DBG_PRINT_TEST_BUS_EN) {
> > > > > @@ -1490,6 +1540,7 @@ static const struct ufs_hba_variant_ops
> > > > > ufs_hba_qcom_vops = {
> > > > > .device_reset = ufs_qcom_device_reset,
> > > > > .config_scaling_param = ufs_qcom_config_scaling_param,
> > > > > .program_key = ufs_qcom_ice_program_key,
> > > > > + .setup_vcc_regulators = ufs_qcom_setup_vcc_regulators,
> > > > > };
> > > > >
> > > > > /**
> > > > > --
> > > > > 2.7.4
> > > > >
> > >
> > > Hi Bjorn,
> > > Thanks for your review.
> > > But As per the earlier discussion regarding handling of vcc voltage
> > > for platform supporting both ufs 2.x and ufs 3.x , it was finally
> > > concluded
> > > to
> > > use "vops and let vendors handle it, until specs or someone
> > > has a better suggestion". Please correct me in case i am wrong.
> > >
> >
> > I was under the impression that this would result in something custom
> > per platform, but what I'm objecting to now that I see the code is that
> > this is completely generic.
> >
> > And the concerns we discussed regarding these regulators being shared
> > with other devices is not considered in this implementation. But in
> > practice I don't see how you could support 2.x, 3.x and rail sharing at
> > the same time.
> >
> > Regards,
> > Bjorn
> >
> > > https://www.mail-archive.com/[email protected]/msg2399116.html
> > >
> > > Regards,
> > > Nitin
>
> Hi Bjorn,
> Thanks for your feedback.
> Regarding your query for regulator being shared with other device,
> Imho, the soc/pmic designer should share only those device
> with ufs regulator which has the same voltage range (2.4-3.6v).
> If that is not considered by the pmic designer,
> wouldn't that would be a board design issue ???
>

It's not only that the rail needs to stay within 2.4-3.6V, depending on
operating mode of this device it either need to be at 2.54-2.7V or
2.95-2.96V depending on wspecversion for this instance.

So either that other device need to be completely flexible in that range
and support the voltage jumping between them without notice, or such
design isn't possible.

And as you say, that would be something that the hardware designers
would need to handle for us.

> And I agree with you that - the code looks generic but
> since the below steps is not part of the specs,
> I had to keep it in vendor specific file for which I
> had to export few api from ufshcd.c to use in vendor
> specific files.
>
> 1. Set the device to SLEEP state.
> 2. Disable the Vcc Regulator.
> 3. Set the vcc voltage according to the device type and reenable
> the regulator.
> 4. Set the device mode back to ACTIVE.
>
> Please correct me if my understanding is not correct.
>

Are you saying that steps 1 to 4 here are not defined in the
specification and therefor Qualcomm specific? Do we expect other vendors
to not follow this sequence, or do they simply not have these voltage
constraints?

And again, isn't this the voltage for the attached UFS device? (Rather
than a Qualcomm thing)

Regards,
Bjorn

2021-04-01 18:07:49

by Nitin Rawat

[permalink] [raw]
Subject: Re: [PATCH V2 2/3] scsi: ufs: add a vops to configure VCC voltage level

On 2021-03-31 23:30, Asutosh Das (asd) wrote:
> On 3/21/2021 2:57 PM, Nitin Rawat wrote:
>> Add a vops to configure VCC voltage VCC voltage level
>> for platform supporting both ufs2.x and ufs 3.x devices.
>>
>> Suggested-by: Stanley Chu <[email protected]>
>> Suggested-by: Asutosh Das <[email protected]>
>> Suggested-by: Bjorn Andersson <[email protected]>
>> Signed-off-by: Nitin Rawat <[email protected]>
>> Signed-off-by: Veerabhadrarao Badiganti <[email protected]>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 4 ++++
>> drivers/scsi/ufs/ufshcd.h | 10 ++++++++++
>> 2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 633ca8e..5bfe987 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -7763,6 +7763,10 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
>> goto out;
>>
>> ufshcd_clear_ua_wluns(hba);
>> + if (ufshcd_vops_setup_vcc_regulators(hba))
> This would be invoked even for platforms that don't support both 2.x
> and 3.x and don't need to set the voltages in the driver.
> I guess platforms that support both 2.x and 3.x and can't set the
> regulator voltages from dts due to different voltage requirements of
> 2.x and 3.x, should request the driver to set the voltages. And the
> driver may do so after determining the device version.

Hi Asutosh,
Thanks for the suggestion. I will check and try to accommodate your
suggestion.
Regards,
Nitin
>
>> + dev_err(hba->dev,
>> + "%s: Failed to set the VCC regulator values, continue with
>> 2.7v\n",
>> + __func__);
>>
>> /* Initialize devfreq after UFS device is detected */
>> if (ufshcd_is_clkscaling_supported(hba)) {
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index 0db796a..8f0945d 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -324,6 +324,7 @@ struct ufs_pwr_mode_info {
>> * @device_reset: called to issue a reset pulse on the UFS device
>> * @program_key: program or evict an inline encryption key
>> * @event_notify: called to notify important events
>> + * @setup_vcc_regulators : update vcc regulator level
>> */
>> struct ufs_hba_variant_ops {
>> const char *name;
>> @@ -360,6 +361,7 @@ struct ufs_hba_variant_ops {
>> const union ufs_crypto_cfg_entry *cfg, int slot);
>> void (*event_notify)(struct ufs_hba *hba,
>> enum ufs_event_type evt, void *data);
>> + int (*setup_vcc_regulators)(struct ufs_hba *hba);
>> };
>>
>> /* clock gating state */
>> @@ -1269,6 +1271,14 @@ static inline void
>> ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
>> hba->vops->config_scaling_param(hba, profile, data);
>> }
>>
>> +static inline int ufshcd_vops_setup_vcc_regulators(struct ufs_hba
>> *hba)
>> +{
>> + if (hba->vops && hba->vops->setup_vcc_regulators)
>> + return hba->vops->setup_vcc_regulators(hba);
>> +
>> + return 0;
>> +}
>> +
>> extern struct ufs_pm_lvl_states ufs_pm_lvl_states[];
>>
>> /*
>> --
>> 2.7.4
>>