diff mbox series

[v1,2/2] scsi: ufs: Support reading UFS's Vcc voltage from device tree

Message ID 69db325a09d5c3fa7fc260db031b1e498b601c25.1598939393.git.nguyenb@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series Supports Reading UFS's Vcc Voltage Levels from DT | expand

Commit Message

Bao D. Nguyen Sept. 1, 2020, 6 a.m. UTC 
The UFS specifications supports a range of Vcc operating voltage
from 2.4-3.6V depending on the device and manufacturers.
Allows selecting the UFS Vcc voltage level by setting the
UFS's entry vcc-voltage-level in the device tree. If UFS's
vcc-voltage-level setting is not found in the device tree,
use default values provided by the driver.

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-pltfrm.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Avri Altman Sept. 13, 2020, 9:37 a.m. UTC | #1 
> 
> The UFS specifications supports a range of Vcc operating voltage
> from 2.4-3.6V depending on the device and manufacturers.
> Allows selecting the UFS Vcc voltage level by setting the
> UFS's entry vcc-voltage-level in the device tree. If UFS's
> vcc-voltage-level setting is not found in the device tree,
> use default values provided by the driver.
> 
> 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-pltfrm.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
> index 3db0af6..48f429c 100644
> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
> @@ -104,10 +104,11 @@ static int ufshcd_parse_clock_info(struct ufs_hba
> *hba)
>  static int ufshcd_populate_vreg(struct device *dev, const char *name,
>                 struct ufs_vreg **out_vreg)
>  {
> -       int ret = 0;
> +       int len, ret = 0;
>         char prop_name[MAX_PROP_SIZE];
>         struct ufs_vreg *vreg = NULL;
>         struct device_node *np = dev->of_node;
> +       const __be32 *prop;
> 
>         if (!np) {
>                 dev_err(dev, "%s: non DT initialization\n", __func__);
> @@ -138,8 +139,16 @@ static int ufshcd_populate_vreg(struct device *dev,
> const char *name,
>                         vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV;
>                         vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV;
>                 } else {
> -                       vreg->min_uV = UFS_VREG_VCC_MIN_UV;
> -                       vreg->max_uV = UFS_VREG_VCC_MAX_UV;
> +                       prop = of_get_property(np, "vcc-voltage-level", &len);
> +                       if (!prop || (len != (2 * sizeof(__be32)))) {
> +                               dev_warn(dev, "%s vcc-voltage-level property.\n",
> +                                       prop ? "invalid format" : "no");
> +                               vreg->min_uV = UFS_VREG_VCC_MIN_UV;
> +                               vreg->max_uV = UFS_VREG_VCC_MAX_UV;
> +                       } else {
> +                               vreg->min_uV = be32_to_cpup(&prop[0]);
> +                               vreg->max_uV = be32_to_cpup(&prop[1]);
> +                       }
>                 }
>         } else if (!strcmp(name, "vccq")) {
>                 vreg->min_uV = UFS_VREG_VCCQ_MIN_UV;
> --
Maybe instead call ufshcd_populate_vreg with the new name,
To not break the function flow, and just add another else if ?
Bao D. Nguyen Sept. 14, 2020, 6:43 p.m. UTC | #2 
On 2020-09-13 02:37, Avri Altman wrote:
>> 
>> The UFS specifications supports a range of Vcc operating voltage
>> from 2.4-3.6V depending on the device and manufacturers.
>> Allows selecting the UFS Vcc voltage level by setting the
>> UFS's entry vcc-voltage-level in the device tree. If UFS's
>> vcc-voltage-level setting is not found in the device tree,
>> use default values provided by the driver.
>> 
>> 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-pltfrm.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c 
>> b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> index 3db0af6..48f429c 100644
>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> @@ -104,10 +104,11 @@ static int ufshcd_parse_clock_info(struct 
>> ufs_hba
>> *hba)
>>  static int ufshcd_populate_vreg(struct device *dev, const char *name,
>>                 struct ufs_vreg **out_vreg)
>>  {
>> -       int ret = 0;
>> +       int len, ret = 0;
>>         char prop_name[MAX_PROP_SIZE];
>>         struct ufs_vreg *vreg = NULL;
>>         struct device_node *np = dev->of_node;
>> +       const __be32 *prop;
>> 
>>         if (!np) {
>>                 dev_err(dev, "%s: non DT initialization\n", __func__);
>> @@ -138,8 +139,16 @@ static int ufshcd_populate_vreg(struct device 
>> *dev,
>> const char *name,
>>                         vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV;
>>                         vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV;
>>                 } else {
>> -                       vreg->min_uV = UFS_VREG_VCC_MIN_UV;
>> -                       vreg->max_uV = UFS_VREG_VCC_MAX_UV;
>> +                       prop = of_get_property(np, 
>> "vcc-voltage-level", &len);
>> +                       if (!prop || (len != (2 * sizeof(__be32)))) {
>> +                               dev_warn(dev, "%s vcc-voltage-level 
>> property.\n",
>> +                                       prop ? "invalid format" : 
>> "no");
>> +                               vreg->min_uV = UFS_VREG_VCC_MIN_UV;
>> +                               vreg->max_uV = UFS_VREG_VCC_MAX_UV;
>> +                       } else {
>> +                               vreg->min_uV = be32_to_cpup(&prop[0]);
>> +                               vreg->max_uV = be32_to_cpup(&prop[1]);
>> +                       }
>>                 }
>>         } else if (!strcmp(name, "vccq")) {
>>                 vreg->min_uV = UFS_VREG_VCCQ_MIN_UV;
>> --
> Maybe instead call ufshcd_populate_vreg with the new name,
> To not break the function flow, and just add another else if ?
Could you please clarify your comments? Are you suggesting to create a 
new function?
Thank you.
Avri Altman Sept. 15, 2020, 6:51 a.m. UTC | #3 
> > Maybe instead call ufshcd_populate_vreg with the new name,
> > To not break the function flow, and just add another else if ?
> Could you please clarify your comments? Are you suggesting to create a
> new function?
> Thank you.
No, just call ufshcd_populate_vreg with the new name, e.g. something like:

diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 3db0af6..9798d4c 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -141,6 +141,8 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name,
                        vreg->min_uV = UFS_VREG_VCC_MIN_UV;
                        vreg->max_uV = UFS_VREG_VCC_MAX_UV;
                }
+       } else if (!strcmp(name, "vcc-voltage-level")) {
+               /* add your changes here */
        } else if (!strcmp(name, "vccq")) {
                vreg->min_uV = UFS_VREG_VCCQ_MIN_UV;
                vreg->max_uV = UFS_VREG_VCCQ_MAX_UV;
@@ -177,8 +179,12 @@ static int ufshcd_parse_regulator_info(struct ufs_hba *hba)
                goto out;
 
        err = ufshcd_populate_vreg(dev, "vcc", &info->vcc);
-       if (err)
-               goto out;
+       if (err) {
+               err = ufshcd_populate_vreg(dev, "vcc-voltage-level",
+                                          &info->vcc);
+               if (err)
+                       goto out;
+       }
 
        err = ufshcd_populate_vreg(dev, "vccq", &info->vccq);
        if (err)
Bjorn Andersson Sept. 15, 2020, 1:46 p.m. UTC | #4 
On Tue 01 Sep 01:00 CDT 2020, Bao D. Nguyen wrote:

> The UFS specifications supports a range of Vcc operating voltage
> from 2.4-3.6V depending on the device and manufacturers.
> Allows selecting the UFS Vcc voltage level by setting the
> UFS's entry vcc-voltage-level in the device tree. If UFS's
> vcc-voltage-level setting is not found in the device tree,
> use default values provided by the driver.
> 

As stated in the reply to patch 1, this is not the right approach.  As
long as you have static min/max values requested by the driver you
should rely on the board's constraints for the regulator levels and
instead remove the min_uV/max_uV code from the driver.

Thanks,
Bjorn

> 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-pltfrm.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
> index 3db0af6..48f429c 100644
> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
> @@ -104,10 +104,11 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>  static int ufshcd_populate_vreg(struct device *dev, const char *name,
>  		struct ufs_vreg **out_vreg)
>  {
> -	int ret = 0;
> +	int len, ret = 0;
>  	char prop_name[MAX_PROP_SIZE];
>  	struct ufs_vreg *vreg = NULL;
>  	struct device_node *np = dev->of_node;
> +	const __be32 *prop;
>  
>  	if (!np) {
>  		dev_err(dev, "%s: non DT initialization\n", __func__);
> @@ -138,8 +139,16 @@ static int ufshcd_populate_vreg(struct device *dev, const char *name,
>  			vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV;
>  			vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV;
>  		} else {
> -			vreg->min_uV = UFS_VREG_VCC_MIN_UV;
> -			vreg->max_uV = UFS_VREG_VCC_MAX_UV;
> +			prop = of_get_property(np, "vcc-voltage-level", &len);
> +			if (!prop || (len != (2 * sizeof(__be32)))) {
> +				dev_warn(dev, "%s vcc-voltage-level property.\n",
> +					prop ? "invalid format" : "no");
> +				vreg->min_uV = UFS_VREG_VCC_MIN_UV;
> +				vreg->max_uV = UFS_VREG_VCC_MAX_UV;
> +			} else {
> +				vreg->min_uV = be32_to_cpup(&prop[0]);
> +				vreg->max_uV = be32_to_cpup(&prop[1]);
> +			}
>  		}
>  	} else if (!strcmp(name, "vccq")) {
>  		vreg->min_uV = UFS_VREG_VCCQ_MIN_UV;
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 3db0af6..48f429c 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -104,10 +104,11 @@  static int ufshcd_parse_clock_info(struct ufs_hba *hba)
 static int ufshcd_populate_vreg(struct device *dev, const char *name,
 		struct ufs_vreg **out_vreg)
 {
-	int ret = 0;
+	int len, ret = 0;
 	char prop_name[MAX_PROP_SIZE];
 	struct ufs_vreg *vreg = NULL;
 	struct device_node *np = dev->of_node;
+	const __be32 *prop;
 
 	if (!np) {
 		dev_err(dev, "%s: non DT initialization\n", __func__);
@@ -138,8 +139,16 @@  static int ufshcd_populate_vreg(struct device *dev, const char *name,
 			vreg->min_uV = UFS_VREG_VCC_1P8_MIN_UV;
 			vreg->max_uV = UFS_VREG_VCC_1P8_MAX_UV;
 		} else {
-			vreg->min_uV = UFS_VREG_VCC_MIN_UV;
-			vreg->max_uV = UFS_VREG_VCC_MAX_UV;
+			prop = of_get_property(np, "vcc-voltage-level", &len);
+			if (!prop || (len != (2 * sizeof(__be32)))) {
+				dev_warn(dev, "%s vcc-voltage-level property.\n",
+					prop ? "invalid format" : "no");
+				vreg->min_uV = UFS_VREG_VCC_MIN_UV;
+				vreg->max_uV = UFS_VREG_VCC_MAX_UV;
+			} else {
+				vreg->min_uV = be32_to_cpup(&prop[0]);
+				vreg->max_uV = be32_to_cpup(&prop[1]);
+			}
 		}
 	} else if (!strcmp(name, "vccq")) {
 		vreg->min_uV = UFS_VREG_VCCQ_MIN_UV;