[PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level

[PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level

[Bao D. Nguyen]

UFS version 3.0 and later devices require Vcc and Vccq power supplies
with Vccq2 being optional. While earlier UFS version 2.0 and 2.1
devices, the Vcc and Vccq2 are required with Vccq being optional.
Check the required power supplies used by the device
and set the device's supported Icc level properly.

Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 06e2439..fdd1d3e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6845,8 +6845,9 @@ static u32 ufshcd_find_max_sup_active_icc_level(struct ufs_hba *hba,
 {
 	u32 icc_level = 0;
 
-	if (!hba->vreg_info.vcc || !hba->vreg_info.vccq ||
-						!hba->vreg_info.vccq2) {
+	if (!hba->vreg_info.vcc ||
+		(!hba->vreg_info.vccq && hba->dev_info.wspecversion >= 0x300) ||
+		(!hba->vreg_info.vccq2 && hba->dev_info.wspecversion < 0x300)) {
 		dev_err(hba->dev,
 			"%s: Regulator capability was not set, actvIccLevel=%d",
 							__func__, icc_level);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level

[nguyenb]

On 2020-08-31 18:19, Bao D. Nguyen wrote:
> UFS version 3.0 and later devices require Vcc and Vccq power supplies
> with Vccq2 being optional. While earlier UFS version 2.0 and 2.1
> devices, the Vcc and Vccq2 are required with Vccq being optional.
> Check the required power supplies used by the device
> and set the device's supported Icc level properly.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 06e2439..fdd1d3e 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6845,8 +6845,9 @@ static u32
> ufshcd_find_max_sup_active_icc_level(struct ufs_hba *hba,
>  {
>  	u32 icc_level = 0;
> 
> -	if (!hba->vreg_info.vcc || !hba->vreg_info.vccq ||
> -						!hba->vreg_info.vccq2) {
> +	if (!hba->vreg_info.vcc ||
> +		(!hba->vreg_info.vccq && hba->dev_info.wspecversion >= 0x300) ||
> +		(!hba->vreg_info.vccq2 && hba->dev_info.wspecversion < 0x300)) {
>  		dev_err(hba->dev,
>  			"%s: Regulator capability was not set, actvIccLevel=%d",
>  							__func__, icc_level);

Hello, please help review the change and comment if any.

Thanks!
Bao

RE: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level

[Avri Altman]

> 
> On 2020-08-31 18:19, Bao D. Nguyen wrote:
> > UFS version 3.0 and later devices require Vcc and Vccq power supplies
> > with Vccq2 being optional. While earlier UFS version 2.0 and 2.1
> > devices, the Vcc and Vccq2 are required with Vccq being optional.
> > Check the required power supplies used by the device
> > and set the device's supported Icc level properly.
Practically you are correct - most flash vendors moved in UFS3.1 to 1.2 supply instead of 1.8.
However, the host should provide all 3 supplies to the device because - 
a) A flash vendor might want to still use 1.8 in its UFS3.1 device, and
b) We should allow a degenerated configurations, e.g. 3.1 devices, that are degenerated to 2.1 or 2.2

That said, I think we can entirely remove the check in the beginning of the function,
But not because the spec allows it, but because each supply is explicitly checked later on,
before reading its applicable max current entry in the power descriptor.

Thanks,
Avri

Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level

[nguyenb]

On 2020-09-10 03:02, Avri Altman wrote:
>> 
>> On 2020-08-31 18:19, Bao D. Nguyen wrote:
>> > UFS version 3.0 and later devices require Vcc and Vccq power supplies
>> > with Vccq2 being optional. While earlier UFS version 2.0 and 2.1
>> > devices, the Vcc and Vccq2 are required with Vccq being optional.
>> > Check the required power supplies used by the device
>> > and set the device's supported Icc level properly.
> Practically you are correct - most flash vendors moved in UFS3.1 to
> 1.2 supply instead of 1.8.
> However, the host should provide all 3 supplies to the device because -
> a) A flash vendor might want to still use 1.8 in its UFS3.1 device, and
> b) We should allow a degenerated configurations, e.g. 3.1 devices,
> that are degenerated to 2.1 or 2.2
Thank you for your comment.
The host can provide all 3 power supplies. However, the change is to 
ensure
we do not exit early and fail to properly set the Icc level because the 
optional power
supply is not provided.
> 
> That said, I think we can entirely remove the check in the beginning
> of the function,
> But not because the spec allows it, but because each supply is
> explicitly checked later on,
> before reading its applicable max current entry in the power 
> descriptor.
We need these checks to prevent NULL pointer access subsequently in this 
function.
> Thanks,
> Avri

Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level

[Bjorn Andersson]

On Tue 01 Sep 01:19 UTC 2020, Bao D. Nguyen wrote:

> UFS version 3.0 and later devices require Vcc and Vccq power supplies
> with Vccq2 being optional. While earlier UFS version 2.0 and 2.1
> devices, the Vcc and Vccq2 are required with Vccq being optional.
> Check the required power supplies used by the device
> and set the device's supported Icc level properly.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 06e2439..fdd1d3e 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6845,8 +6845,9 @@ static u32 ufshcd_find_max_sup_active_icc_level(struct ufs_hba *hba,
>  {
>  	u32 icc_level = 0;
>  
> -	if (!hba->vreg_info.vcc || !hba->vreg_info.vccq ||
> -						!hba->vreg_info.vccq2) {
> +	if (!hba->vreg_info.vcc ||

How did you test this?

devm_regulator_get() never returns NULL, so afaict this conditional will
never be taken with either the old or new version of the code.

Regards,
Bjorn

> +		(!hba->vreg_info.vccq && hba->dev_info.wspecversion >= 0x300) ||
> +		(!hba->vreg_info.vccq2 && hba->dev_info.wspecversion < 0x300)) {
>  		dev_err(hba->dev,
>  			"%s: Regulator capability was not set, actvIccLevel=%d",
>  							__func__, icc_level);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level

[nguyenb]

On 2020-09-14 19:54, Bjorn Andersson wrote:
> On Tue 01 Sep 01:19 UTC 2020, Bao D. Nguyen wrote:
> 
>> UFS version 3.0 and later devices require Vcc and Vccq power supplies
>> with Vccq2 being optional. While earlier UFS version 2.0 and 2.1
>> devices, the Vcc and Vccq2 are required with Vccq being optional.
>> Check the required power supplies used by the device
>> and set the device's supported Icc level properly.
>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 06e2439..fdd1d3e 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -6845,8 +6845,9 @@ static u32 
>> ufshcd_find_max_sup_active_icc_level(struct ufs_hba *hba,
>>  {
>>  	u32 icc_level = 0;
>> 
>> -	if (!hba->vreg_info.vcc || !hba->vreg_info.vccq ||
>> -						!hba->vreg_info.vccq2) {
>> +	if (!hba->vreg_info.vcc ||
> 
> How did you test this?
> 
> devm_regulator_get() never returns NULL, so afaict this conditional 
> will
> never be taken with either the old or new version of the code.
Thanks for your comment. The call flow is as follows:
ufshcd_pltfrm_init->ufshcd_parse_regulator_info->ufshcd_populate_vreg
In the ufshcd_populate_vreg() function, it looks for DT entries 
"%s-supply"
For UFS3.0+ devices, "vccq2-supply" is optional, so the vendor may 
choose not to provide vccq2-supply in the DT.
As a result, a NULL is returned to hba->vreg_info.vccq2.
Same for UFS2.0 and UFS2.1 devices, a NULL may be returned to 
hba->vreg_info.vccq if vccq-supply is not provided in the DT.
The current code only checks for !hba->vreg_info.vccq OR 
!hba->vreg_info.vccq2. It will skip the setting for icc_level
if either vccq or vccq2 is not provided in the DT.
> 
> Regards,
> Bjorn
> 
>> +		(!hba->vreg_info.vccq && hba->dev_info.wspecversion >= 0x300) ||
>> +		(!hba->vreg_info.vccq2 && hba->dev_info.wspecversion < 0x300)) {
>>  		dev_err(hba->dev,
>>  			"%s: Regulator capability was not set, actvIccLevel=%d",
>>  							__func__, icc_level);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level

[Bjorn Andersson]

On Tue 15 Sep 03:49 CDT 2020, nguyenb@codeaurora.org wrote:

> On 2020-09-14 19:54, Bjorn Andersson wrote:
> > On Tue 01 Sep 01:19 UTC 2020, Bao D. Nguyen wrote:
> > 
> > > UFS version 3.0 and later devices require Vcc and Vccq power supplies
> > > with Vccq2 being optional. While earlier UFS version 2.0 and 2.1
> > > devices, the Vcc and Vccq2 are required with Vccq being optional.
> > > Check the required power supplies used by the device
> > > and set the device's supported Icc level properly.
> > > 
> > > Signed-off-by: Can Guo <cang@codeaurora.org>
> > > Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> > > Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> > > ---
> > >  drivers/scsi/ufs/ufshcd.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > index 06e2439..fdd1d3e 100644
> > > --- a/drivers/scsi/ufs/ufshcd.c
> > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > @@ -6845,8 +6845,9 @@ static u32
> > > ufshcd_find_max_sup_active_icc_level(struct ufs_hba *hba,
> > >  {
> > >  	u32 icc_level = 0;
> > > 
> > > -	if (!hba->vreg_info.vcc || !hba->vreg_info.vccq ||
> > > -						!hba->vreg_info.vccq2) {
> > > +	if (!hba->vreg_info.vcc ||
> > 
> > How did you test this?
> > 
> > devm_regulator_get() never returns NULL, so afaict this conditional will
> > never be taken with either the old or new version of the code.
> Thanks for your comment. The call flow is as follows:
> ufshcd_pltfrm_init->ufshcd_parse_regulator_info->ufshcd_populate_vreg
> In the ufshcd_populate_vreg() function, it looks for DT entries "%s-supply"
> For UFS3.0+ devices, "vccq2-supply" is optional, so the vendor may choose
> not to provide vccq2-supply in the DT.
> As a result, a NULL is returned to hba->vreg_info.vccq2.
> Same for UFS2.0 and UFS2.1 devices, a NULL may be returned to
> hba->vreg_info.vccq if vccq-supply is not provided in the DT.
> The current code only checks for !hba->vreg_info.vccq OR
> !hba->vreg_info.vccq2. It will skip the setting for icc_level
> if either vccq or vccq2 is not provided in the DT.
> > 

Thanks for the pointers, I now see that the there will only be struct
ufs_vreg objects allocated for the items that has an associated
%s-supply.

FYI, the idiomatic way to handle optional regulators is to use
regulator_get_optional(), which will return -ENODEV for regulators not
specified.

Regards,
Bjorn

> > Regards,
> > Bjorn
> > 
> > > +		(!hba->vreg_info.vccq && hba->dev_info.wspecversion >= 0x300) ||
> > > +		(!hba->vreg_info.vccq2 && hba->dev_info.wspecversion < 0x300)) {
> > >  		dev_err(hba->dev,
> > >  			"%s: Regulator capability was not set, actvIccLevel=%d",
> > >  							__func__, icc_level);
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > > Forum,
> > > a Linux Foundation Collaborative Project
> > > 

Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level

[nguyenb]

On 2020-09-15 06:37, Bjorn Andersson wrote:
> On Tue 15 Sep 03:49 CDT 2020, nguyenb@codeaurora.org wrote:
> 
>> On 2020-09-14 19:54, Bjorn Andersson wrote:
>> > On Tue 01 Sep 01:19 UTC 2020, Bao D. Nguyen wrote:
>> >
>> > > UFS version 3.0 and later devices require Vcc and Vccq power supplies
>> > > with Vccq2 being optional. While earlier UFS version 2.0 and 2.1
>> > > devices, the Vcc and Vccq2 are required with Vccq being optional.
>> > > Check the required power supplies used by the device
>> > > and set the device's supported Icc level properly.
>> > >
>> > > Signed-off-by: Can Guo <cang@codeaurora.org>
>> > > Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>> > > Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
>> > > ---
>> > >  drivers/scsi/ufs/ufshcd.c | 5 +++--
>> > >  1 file changed, 3 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> > > index 06e2439..fdd1d3e 100644
>> > > --- a/drivers/scsi/ufs/ufshcd.c
>> > > +++ b/drivers/scsi/ufs/ufshcd.c
>> > > @@ -6845,8 +6845,9 @@ static u32
>> > > ufshcd_find_max_sup_active_icc_level(struct ufs_hba *hba,
>> > >  {
>> > >  	u32 icc_level = 0;
>> > >
>> > > -	if (!hba->vreg_info.vcc || !hba->vreg_info.vccq ||
>> > > -						!hba->vreg_info.vccq2) {
>> > > +	if (!hba->vreg_info.vcc ||
>> >
>> > How did you test this?
>> >
>> > devm_regulator_get() never returns NULL, so afaict this conditional will
>> > never be taken with either the old or new version of the code.
>> Thanks for your comment. The call flow is as follows:
>> ufshcd_pltfrm_init->ufshcd_parse_regulator_info->ufshcd_populate_vreg
>> In the ufshcd_populate_vreg() function, it looks for DT entries 
>> "%s-supply"
>> For UFS3.0+ devices, "vccq2-supply" is optional, so the vendor may 
>> choose
>> not to provide vccq2-supply in the DT.
>> As a result, a NULL is returned to hba->vreg_info.vccq2.
>> Same for UFS2.0 and UFS2.1 devices, a NULL may be returned to
>> hba->vreg_info.vccq if vccq-supply is not provided in the DT.
>> The current code only checks for !hba->vreg_info.vccq OR
>> !hba->vreg_info.vccq2. It will skip the setting for icc_level
>> if either vccq or vccq2 is not provided in the DT.
>> >
> 
> Thanks for the pointers, I now see that the there will only be struct
> ufs_vreg objects allocated for the items that has an associated
> %s-supply.
> 
> FYI, the idiomatic way to handle optional regulators is to use
> regulator_get_optional(), which will return -ENODEV for regulators not
> specified.
Thanks for the regulator_get_optional() suggestion. Do you have a strong 
opinion about
using regulator_get_optional() or would my proposal be ok? With 
regulator_get_optional(),
we need to make 3 calls and check each result while the current 
implementation is also reliable
simple quick check for NULL without any potential problem.

Thanks,
Bao
> 
> Regards,
> Bjorn
> 
>> > Regards,
>> > Bjorn
>> >
>> > > +		(!hba->vreg_info.vccq && hba->dev_info.wspecversion >= 0x300) ||
>> > > +		(!hba->vreg_info.vccq2 && hba->dev_info.wspecversion < 0x300)) {
>> > >  		dev_err(hba->dev,
>> > >  			"%s: Regulator capability was not set, actvIccLevel=%d",
>> > >  							__func__, icc_level);
>> > > --
>> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> > > Forum,
>> > > a Linux Foundation Collaborative Project
>> > >

Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level

[Bjorn Andersson]

On Wed 16 Sep 19:53 CDT 2020, nguyenb@codeaurora.org wrote:

> On 2020-09-15 06:37, Bjorn Andersson wrote:
> > On Tue 15 Sep 03:49 CDT 2020, nguyenb@codeaurora.org wrote:
> > 
> > > On 2020-09-14 19:54, Bjorn Andersson wrote:
> > > > On Tue 01 Sep 01:19 UTC 2020, Bao D. Nguyen wrote:
> > > >
> > > > > UFS version 3.0 and later devices require Vcc and Vccq power supplies
> > > > > with Vccq2 being optional. While earlier UFS version 2.0 and 2.1
> > > > > devices, the Vcc and Vccq2 are required with Vccq being optional.
> > > > > Check the required power supplies used by the device
> > > > > and set the device's supported Icc level properly.
> > > > >
> > > > > Signed-off-by: Can Guo <cang@codeaurora.org>
> > > > > Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> > > > > Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> > > > > ---
> > > > >  drivers/scsi/ufs/ufshcd.c | 5 +++--
> > > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > > > index 06e2439..fdd1d3e 100644
> > > > > --- a/drivers/scsi/ufs/ufshcd.c
> > > > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > > > @@ -6845,8 +6845,9 @@ static u32
> > > > > ufshcd_find_max_sup_active_icc_level(struct ufs_hba *hba,
> > > > >  {
> > > > >  	u32 icc_level = 0;
> > > > >
> > > > > -	if (!hba->vreg_info.vcc || !hba->vreg_info.vccq ||
> > > > > -						!hba->vreg_info.vccq2) {
> > > > > +	if (!hba->vreg_info.vcc ||
> > > >
> > > > How did you test this?
> > > >
> > > > devm_regulator_get() never returns NULL, so afaict this conditional will
> > > > never be taken with either the old or new version of the code.
> > > Thanks for your comment. The call flow is as follows:
> > > ufshcd_pltfrm_init->ufshcd_parse_regulator_info->ufshcd_populate_vreg
> > > In the ufshcd_populate_vreg() function, it looks for DT entries
> > > "%s-supply"
> > > For UFS3.0+ devices, "vccq2-supply" is optional, so the vendor may
> > > choose
> > > not to provide vccq2-supply in the DT.
> > > As a result, a NULL is returned to hba->vreg_info.vccq2.
> > > Same for UFS2.0 and UFS2.1 devices, a NULL may be returned to
> > > hba->vreg_info.vccq if vccq-supply is not provided in the DT.
> > > The current code only checks for !hba->vreg_info.vccq OR
> > > !hba->vreg_info.vccq2. It will skip the setting for icc_level
> > > if either vccq or vccq2 is not provided in the DT.
> > > >
> > 
> > Thanks for the pointers, I now see that the there will only be struct
> > ufs_vreg objects allocated for the items that has an associated
> > %s-supply.
> > 
> > FYI, the idiomatic way to handle optional regulators is to use
> > regulator_get_optional(), which will return -ENODEV for regulators not
> > specified.
> Thanks for the regulator_get_optional() suggestion. Do you have a strong
> opinion about
> using regulator_get_optional() or would my proposal be ok? With
> regulator_get_optional(),
> we need to make 3 calls and check each result while the current
> implementation is also reliable
> simple quick check for NULL without any potential problem.
> 

I think the changes to the conditional that you're proposing in this
patch is reasonable.

Regards,
Bjorn

> Thanks,
> Bao
> > 
> > Regards,
> > Bjorn
> > 
> > > > Regards,
> > > > Bjorn
> > > >
> > > > > +		(!hba->vreg_info.vccq && hba->dev_info.wspecversion >= 0x300) ||
> > > > > +		(!hba->vreg_info.vccq2 && hba->dev_info.wspecversion < 0x300)) {
> > > > >  		dev_err(hba->dev,
> > > > >  			"%s: Regulator capability was not set, actvIccLevel=%d",
> > > > >  							__func__, icc_level);
> > > > > --
> > > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > > > > Forum,
> > > > > a Linux Foundation Collaborative Project
> > > > >

Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level

[nguyenb]

On 2020-08-31 18:19, Bao D. Nguyen wrote:
> UFS version 3.0 and later devices require Vcc and Vccq power supplies
> with Vccq2 being optional. While earlier UFS version 2.0 and 2.1
> devices, the Vcc and Vccq2 are required with Vccq being optional.
> Check the required power supplies used by the device
> and set the device's supported Icc level properly.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 06e2439..fdd1d3e 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6845,8 +6845,9 @@ static u32
> ufshcd_find_max_sup_active_icc_level(struct ufs_hba *hba,
>  {
>  	u32 icc_level = 0;
> 
> -	if (!hba->vreg_info.vcc || !hba->vreg_info.vccq ||
> -						!hba->vreg_info.vccq2) {
> +	if (!hba->vreg_info.vcc ||
> +		(!hba->vreg_info.vccq && hba->dev_info.wspecversion >= 0x300) ||
> +		(!hba->vreg_info.vccq2 && hba->dev_info.wspecversion < 0x300)) {
>  		dev_err(hba->dev,
>  			"%s: Regulator capability was not set, actvIccLevel=%d",
>  							__func__, icc_level);

Hello,
Thank you for the comments on this change so far.
It's been idle for some time, so I would like to ping and see if there 
is any other comment.

Regards,
Bao

Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level

[nguyenb]

On 2020-08-31 18:19, Bao D. Nguyen wrote:
> UFS version 3.0 and later devices require Vcc and Vccq power supplies
> with Vccq2 being optional. While earlier UFS version 2.0 and 2.1
> devices, the Vcc and Vccq2 are required with Vccq being optional.
> Check the required power supplies used by the device
> and set the device's supported Icc level properly.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 06e2439..fdd1d3e 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6845,8 +6845,9 @@ static u32
> ufshcd_find_max_sup_active_icc_level(struct ufs_hba *hba,
>  {
>  	u32 icc_level = 0;
> 
> -	if (!hba->vreg_info.vcc || !hba->vreg_info.vccq ||
> -						!hba->vreg_info.vccq2) {
> +	if (!hba->vreg_info.vcc ||
> +		(!hba->vreg_info.vccq && hba->dev_info.wspecversion >= 0x300) ||
> +		(!hba->vreg_info.vccq2 && hba->dev_info.wspecversion < 0x300)) {
>  		dev_err(hba->dev,
>  			"%s: Regulator capability was not set, actvIccLevel=%d",
>  							__func__, icc_level);
Hello,
Could you please help review?
Thank you.