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.
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 | 14 ++++++++++---
drivers/scsi/ufs/ufshcd.h | 14 +++++++++++++
3 files changed, 76 insertions(+), 3 deletions(-)
--
2.7.4
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]>
Signed-off-by: Bao D. Nguyen <[email protected]>
Signed-off-by: Veerabhadrarao Badiganti <[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 9c691e4..000a03a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8091,7 +8091,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;
@@ -8110,8 +8110,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;
@@ -8131,6 +8132,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)
{
@@ -8455,7 +8457,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 };
@@ -8513,6 +8515,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 ee61f82..1410c95 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
>
> 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.
As for ufshcd_{enable,disable}_vreg - maybe inline ufshcd_toggle_vreg and use it instead?
>
> Signed-off-by: Nitin Rawat <[email protected]>
> Signed-off-by: Bao D. Nguyen <[email protected]>
> Signed-off-by: Veerabhadrarao Badiganti <[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 9c691e4..000a03a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8091,7 +8091,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;
>
> @@ -8110,8 +8110,9 @@ static int ufshcd_enable_vreg(struct device *dev,
> struct ufs_vreg *vreg)
> out:
> return ret;
> }
> +EXPORT_SYMBOL(ufshcd_enable_vreg);
Why do you need to export it across the kernel?
Isn't making it non-static suffices?
Do you need it for a loadable module?
>
> -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;
>
> @@ -8131,6 +8132,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)
> {
> @@ -8455,7 +8457,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 };
> @@ -8513,6 +8515,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 ee61f82..1410c95 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
>
> 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.
The flow should be generic - isn't it?
Why do you need the entire flow to be vendor-specific?
Why not just the parameters vendor-specific?
Thanks,
Avri
On 2021-01-31 19:29, Avri Altman wrote:
>>
>> 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.
> As for ufshcd_{enable,disable}_vreg - maybe inline ufshcd_toggle_vreg
> and use it instead?
>
>>
>> Signed-off-by: Nitin Rawat <[email protected]>
>> Signed-off-by: Bao D. Nguyen <[email protected]>
>> Signed-off-by: Veerabhadrarao Badiganti <[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 9c691e4..000a03a 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -8091,7 +8091,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;
>>
>> @@ -8110,8 +8110,9 @@ static int ufshcd_enable_vreg(struct device
>> *dev,
>> struct ufs_vreg *vreg)
>> out:
>> return ret;
>> }
>> +EXPORT_SYMBOL(ufshcd_enable_vreg);
> Why do you need to export it across the kernel?
> Isn't making it non-static suffices?
> Do you need it for a loadable module?
>
>>
>> -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;
>>
>> @@ -8131,6 +8132,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)
>> {
>> @@ -8455,7 +8457,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 };
>> @@ -8513,6 +8515,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 ee61f82..1410c95 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
Hi Avri,
Thanks for reviewing it.
ufs-qcom.c can be a loadable module, so just inlining won't suffice in
that case.
Hence export is needed.
Thanks,
Nitin
On 2021-01-31 19:32, Avri Altman wrote:
>>
>> 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.
> The flow should be generic - isn't it?
> Why do you need the entire flow to be vendor-specific?
> Why not just the parameters vendor-specific?
>
> Thanks,
> Avri
Hi Avri,
This vops change was done as per the below mail thread
discussion where it was decided to go with vops and
let vendors handle it, until specs provides more clarity.
https://www.spinics.net/lists/kernel/msg3754995.html
Regards,
Nitin
On 2021-02-01 14:01, [email protected] wrote:
> On 2021-01-31 19:32, Avri Altman wrote:
>>>
>>> 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.
>> The flow should be generic - isn't it?
>> Why do you need the entire flow to be vendor-specific?
>> Why not just the parameters vendor-specific?
>>
>> Thanks,
>> Avri
>
> Hi Avri,
> This vops change was done as per the below mail thread
> discussion where it was decided to go with vops and
> let vendors handle it, until specs provides more clarity.
>
> https://www.spinics.net/lists/kernel/msg3754995.html
>
> Regards,
> Nitin
Hi Avri,
Please let me know if you have any further comments on this.
Regards,
Nitin
> >> The flow should be generic - isn't it?
> >> Why do you need the entire flow to be vendor-specific?
> >> Why not just the parameters vendor-specific?
> >>
> >> Thanks,
> >> Avri
> >
> > Hi Avri,
> > This vops change was done as per the below mail thread
> > discussion where it was decided to go with vops and
> > let vendors handle it, until specs provides more clarity.
> >
> > https://www.spinics.net/lists/kernel/msg3754995.html
> >
> > Regards,
> > Nitin
>
> Hi Avri,
> Please let me know if you have any further comments on this.
No further comments.
Looks like you need an ack from Stanley or Bjorn who proposed this approach.
Thanks,
Avri
On 2021-02-08 17:52, Avri Altman wrote:
>> >> The flow should be generic - isn't it?
>> >> Why do you need the entire flow to be vendor-specific?
>> >> Why not just the parameters vendor-specific?
>> >>
>> >> Thanks,
>> >> Avri
>> >
>> > Hi Avri,
>> > This vops change was done as per the below mail thread
>> > discussion where it was decided to go with vops and
>> > let vendors handle it, until specs provides more clarity.
>> >
>> > https://www.spinics.net/lists/kernel/msg3754995.html
>> >
>> > Regards,
>> > Nitin
>>
>> Hi Avri,
>> Please let me know if you have any further comments on this.
> No further comments.
> Looks like you need an ack from Stanley or Bjorn who proposed this
> approach.
>
> Thanks,
> Avri
Hi Stanley/Bjorn,
Please can you review this patch and provide your input.
Regards,
Nitin