| Message ID | 5c9d6f76303bbe5188bf839b2ea5e5bf530e7281.1598923023.git.nguyenb@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1,1/1] scsi: ufshcd: Properly set the device Icc Level | expand |
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
> > 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
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
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 >
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 >>
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 > > >
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 >> > >
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 > > > > >
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
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 --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);