diff mbox series

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

Message ID 5c9d6f76303bbe5188bf839b2ea5e5bf530e7281.1598923023.git.nguyenb@codeaurora.org
State New
Headers show
Series [v1,1/1] scsi: ufshcd: Properly set the device Icc Level | expand

Commit Message

Bao D. Nguyen Sept. 1, 2020, 1:19 a.m. UTC
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(-)

Comments

Avri Altman Sept. 10, 2020, 10:02 a.m. UTC | #1
> 
> 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
Bao D. Nguyen Sept. 14, 2020, 4:34 p.m. UTC | #2
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
Bjorn Andersson Sept. 15, 2020, 2:54 a.m. UTC | #3
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
>
Bao D. Nguyen Sept. 17, 2020, 12:53 a.m. UTC | #4
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
>> > >
Bjorn Andersson Sept. 17, 2020, 2:29 a.m. UTC | #5
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
> > > > >
Bao D. Nguyen Oct. 26, 2020, 8:48 p.m. UTC | #6
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.
diff mbox series

Patch

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);