* Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level
2020-09-01 1:19 [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level Bao D. Nguyen
@ 2020-09-10 1:28 ` nguyenb
[not found] ` <0101017475a11d00-6def34a7-db5d-472c-9dcc-215a80510402-000000@us-west-2.amazonses.com>
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: nguyenb @ 2020-09-10 1:28 UTC (permalink / raw)
To: cang, asutoshd, martin.petersen, linux-scsi
Cc: linux-arm-msm, Alim Akhtar, Avri Altman, James E.J. Bottomley,
Stanley Chu, Bean Huo, Bart Van Assche, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread[parent not found: <0101017475a11d00-6def34a7-db5d-472c-9dcc-215a80510402-000000@us-west-2.amazonses.com>]
* RE: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level
[not found] ` <0101017475a11d00-6def34a7-db5d-472c-9dcc-215a80510402-000000@us-west-2.amazonses.com>
@ 2020-09-10 10:02 ` Avri Altman
2020-09-14 16:34 ` nguyenb
0 siblings, 1 reply; 11+ messages in thread
From: Avri Altman @ 2020-09-10 10:02 UTC (permalink / raw)
To: nguyenb, cang, asutoshd, martin.petersen, linux-scsi
Cc: linux-arm-msm, Alim Akhtar, James E.J. Bottomley, Stanley Chu,
Bean Huo, Bart Van Assche, linux-kernel
>
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level
2020-09-10 10:02 ` Avri Altman
@ 2020-09-14 16:34 ` nguyenb
0 siblings, 0 replies; 11+ messages in thread
From: nguyenb @ 2020-09-14 16:34 UTC (permalink / raw)
To: Avri Altman
Cc: cang, asutoshd, martin.petersen, linux-scsi, linux-arm-msm,
Alim Akhtar, James E.J. Bottomley, Stanley Chu, Bean Huo,
Bart Van Assche, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level
2020-09-01 1:19 [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level Bao D. Nguyen
2020-09-10 1:28 ` nguyenb
[not found] ` <0101017475a11d00-6def34a7-db5d-472c-9dcc-215a80510402-000000@us-west-2.amazonses.com>
@ 2020-09-15 2:54 ` Bjorn Andersson
2020-09-15 8:49 ` nguyenb
2020-09-29 2:35 ` nguyenb
2020-10-26 20:48 ` nguyenb
4 siblings, 1 reply; 11+ messages in thread
From: Bjorn Andersson @ 2020-09-15 2:54 UTC (permalink / raw)
To: Bao D. Nguyen
Cc: cang, asutoshd, martin.petersen, linux-scsi, linux-arm-msm,
Alim Akhtar, Avri Altman, James E.J. Bottomley, Stanley Chu,
Bean Huo, Bart Van Assche, open list
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level
2020-09-15 2:54 ` Bjorn Andersson
@ 2020-09-15 8:49 ` nguyenb
2020-09-15 13:37 ` Bjorn Andersson
0 siblings, 1 reply; 11+ messages in thread
From: nguyenb @ 2020-09-15 8:49 UTC (permalink / raw)
To: Bjorn Andersson
Cc: cang, asutoshd, martin.petersen, linux-scsi, linux-arm-msm,
Alim Akhtar, Avri Altman, James E.J. Bottomley, Stanley Chu,
Bean Huo, Bart Van Assche, open list
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
>>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level
2020-09-15 8:49 ` nguyenb
@ 2020-09-15 13:37 ` Bjorn Andersson
2020-09-17 0:53 ` nguyenb
0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Andersson @ 2020-09-15 13:37 UTC (permalink / raw)
To: nguyenb
Cc: cang, asutoshd, martin.petersen, linux-scsi, linux-arm-msm,
Alim Akhtar, Avri Altman, James E.J. Bottomley, Stanley Chu,
Bean Huo, Bart Van Assche, open list
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
> > >
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level
2020-09-15 13:37 ` Bjorn Andersson
@ 2020-09-17 0:53 ` nguyenb
2020-09-17 2:29 ` Bjorn Andersson
0 siblings, 1 reply; 11+ messages in thread
From: nguyenb @ 2020-09-17 0:53 UTC (permalink / raw)
To: Bjorn Andersson
Cc: cang, asutoshd, martin.petersen, linux-scsi, linux-arm-msm,
Alim Akhtar, Avri Altman, James E.J. Bottomley, Stanley Chu,
Bean Huo, Bart Van Assche, open list
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
>> > >
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level
2020-09-17 0:53 ` nguyenb
@ 2020-09-17 2:29 ` Bjorn Andersson
0 siblings, 0 replies; 11+ messages in thread
From: Bjorn Andersson @ 2020-09-17 2:29 UTC (permalink / raw)
To: nguyenb
Cc: cang, asutoshd, martin.petersen, linux-scsi, linux-arm-msm,
Alim Akhtar, Avri Altman, James E.J. Bottomley, Stanley Chu,
Bean Huo, Bart Van Assche, open list
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
> > > > >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level
2020-09-01 1:19 [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level Bao D. Nguyen
` (2 preceding siblings ...)
2020-09-15 2:54 ` Bjorn Andersson
@ 2020-09-29 2:35 ` nguyenb
2020-10-26 20:48 ` nguyenb
4 siblings, 0 replies; 11+ messages in thread
From: nguyenb @ 2020-09-29 2:35 UTC (permalink / raw)
To: cang, asutoshd, martin.petersen, linux-scsi
Cc: linux-arm-msm, Alim Akhtar, Avri Altman, James E.J. Bottomley,
Stanley Chu, Bean Huo, Bart Van Assche, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level
2020-09-01 1:19 [PATCH v1 1/1] scsi: ufshcd: Properly set the device Icc Level Bao D. Nguyen
` (3 preceding siblings ...)
2020-09-29 2:35 ` nguyenb
@ 2020-10-26 20:48 ` nguyenb
4 siblings, 0 replies; 11+ messages in thread
From: nguyenb @ 2020-10-26 20:48 UTC (permalink / raw)
To: cang, asutoshd, martin.petersen, linux-scsi
Cc: linux-arm-msm, Alim Akhtar, Avri Altman, James E.J. Bottomley,
Stanley Chu, Bean Huo, Bart Van Assche, linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread