| Message ID | be3ef9bf-438b-4e8a-62c4-f75458e8ecc0@free.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clean up UFSHC driver | expand |
On Mon, Feb 11, 2019 at 02:32:15PM +0100, Marc Gonzalez wrote: > Unfortunately, this optimization breaks UFS on systems where vccq > powers not only the Flash chip, but the host controller as well, > such as APQ8098 MEDIABOX or MTP8998: ... > In my opinion, the rationale for the original patch is questionable. > If neither the UFSHC, nor the Flash chip, require any load from vccq, > then that power rail should simply not be specified at all in the DT. If the supply is physically connected it should be valid to represent this in DT regardless of how or if the supply gets used at runtime. However it does sound like this support needs to be better thought through to make sure we have represented the supplies to the flash chip and the controller separately - it seems like right now there's no tracking of the supplies needed for the controller and the assumption is that only the flash chip needs managing which is breaking things.
On 11/02/2019 18:23, Mark Brown wrote: > On Mon, Feb 11, 2019 at 02:32:15PM +0100, Marc Gonzalez wrote: > >> Unfortunately, this optimization breaks UFS on systems where vccq >> powers not only the Flash chip, but the host controller as well, >> such as APQ8098 MEDIABOX or MTP8998. >> >> In my opinion, the rationale for the original patch is questionable. >> If neither the UFSHC, nor the Flash chip, require any load from vccq, >> then that power rail should simply not be specified at all in the DT. > > If the supply is physically connected it should be valid to represent > this in DT regardless of how or if the supply gets used at runtime. > However it does sound like this support needs to be better thought > through to make sure we have represented the supplies to the flash chip > and the controller separately - it seems like right now there's no > tracking of the supplies needed for the controller and the assumption is > that only the flash chip needs managing which is breaking things. As far as I'm aware, the conflation of host controller with their respective storage medium occurs across several techs: UFS, NAND, SDHC, EMMC. There might be room for improvement, but I don't think these considerations should hold up this series, which fixes something that is broken *today*. UFS reviewers (Alim, Avri, Pedro), can I get at least one Acked-by to remove this small power optimization that breaks UFS on my system? Regards.
> > Commit 60f0187031c0 ("disable vccq if it's not needed by UFS device") > introduced a small power optimization as a driver quirk: ignore the > vccq load specified in the UFSHC DT node when said host controller > is connected to specific Flash chips (Samsung and Hynix currently). > > Unfortunately, this optimization breaks UFS on systems where vccq > powers not only the Flash chip, but the host controller as well, > such as APQ8098 MEDIABOX or MTP8998: > > [ 3.929877] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04 > for idn 13 failed, index 0, err = -11 > [ 5.433815] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04 > for idn 13 failed, index 0, err = -11 > [ 6.937771] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04 > for idn 13 failed, index 0, err = -11 > [ 6.937866] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr_retry: query > attribute, idn 13, failed with error -11 after 3 retires > [ 6.946412] ufshcd-qcom 1da4000.ufshc: ufshcd_disable_auto_bkops: > failed to enable exception event -11 > [ 6.957972] ufshcd-qcom 1da4000.ufshc: dme-peer-get: attr-id 0x1587 > failed 3 retries > [ 6.967181] ufshcd-qcom 1da4000.ufshc: dme-peer-get: attr-id 0x1586 > failed 3 retries > [ 6.975025] ufshcd-qcom 1da4000.ufshc: ufshcd_get_max_pwr_mode: > invalid max pwm tx gear read = 0 > [ 6.982755] ufshcd-qcom 1da4000.ufshc: ufshcd_probe_hba: Failed getting > max supported power mode > [ 8.505770] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag > query for idn 3 failed, err = -11 > [ 10.009807] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag > query for idn 3 failed, err = -11 > [ 11.513766] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag > query for idn 3 failed, err = -11 > [ 11.513861] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag_retry: query > attribute, opcode 5, idn 3, failed with error -11 after 3 retires > [ 13.049807] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor: > opcode 0x01 for idn 8 failed, index 0, err = -11 > [ 14.553768] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor: > opcode 0x01 for idn 8 failed, index 0, err = -11 > [ 16.057767] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor: > opcode 0x01 for idn 8 failed, index 0, err = -11 > [ 16.057872] ufshcd-qcom 1da4000.ufshc: ufshcd_read_desc_param: Failed > reading descriptor. desc_id 8, desc_index 0, param_offset 0, ret -11 > [ 16.067109] ufshcd-qcom 1da4000.ufshc: ufshcd_init_icc_levels: Failed > reading power descriptor.len = 98 ret = -11 > [ 37.073787] ufshcd-qcom 1da4000.ufshc: link startup failed 1 > > In my opinion, the rationale for the original patch is questionable. > If neither the UFSHC, nor the Flash chip, require any load from vccq, > then that power rail should simply not be specified at all in the DT. > > Working around that fact in the driver is detrimental, as evidenced > by the failure to initialize the host controller on MSM8998. > > Revert the original patch, and clean up loose ends in the next patch. > > Relevant patches: > > 60f0187031c05e04cbadffb62f557d0ff3564490 > c58ab7aab71e2c783087115f0ce1623c2fdcf0b2 > 46c1cf706076500cdcde3445be97233793eec7f1 > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> Acked-by: Avri Altman <avri.altman@wdc.com>
Hi Marc, > Commit 60f0187031c0 ("disable vccq if it's not needed by UFS device") > introduced a small power optimization as a driver quirk: ignore the > vccq load specified in the UFSHC DT node when said host controller > is connected to specific Flash chips (Samsung and Hynix currently). [...] > Revert the original patch, and clean up loose ends in the next patch. This commit isn't a revert. Why not?
On 26/02/2019 15:44, Martin K. Petersen wrote: > Hi Marc, > >> Commit 60f0187031c0 ("disable vccq if it's not needed by UFS device") >> introduced a small power optimization as a driver quirk: ignore the >> vccq load specified in the UFSHC DT node when said host controller >> is connected to specific Flash chips (Samsung and Hynix currently). > > [...] > >> Revert the original patch, and clean up loose ends in the next patch. > > This commit isn't a revert. Why not? What do you mean?
Marc, >>> Revert the original patch, and clean up loose ends in the next patch. >> >> This commit isn't a revert. Why not? > > What do you mean? Your commit states it reverts the original patch but the submission is not a git revert. If there are reasons why simply reverting the original commit didn't work, I'd like to see them documented in the commit message.
[ Drop codeaurora.org devs ] On 26/02/2019 15:52, Martin K. Petersen wrote: >>>> Revert the original patch, and clean up loose ends in the next patch. >>> >>> This commit isn't a revert. Why not? >> >> What do you mean? > > Your commit states it reverts the original patch but the submission is > not a git revert. If there are reasons why simply reverting the original > commit didn't work, I'd like to see them documented in the commit > message. Martin, I indeed started off from 'git revert' $ git revert 60f0187031c0 warning: inexact rename detection was skipped due to too many files. warning: you may want to set your merge.renamelimit variable to at least 18258 and retry the command. error: could not revert 60f0187031c0... scsi: ufs: disable vccq if it's not needed by UFS device hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' hint: and commit the result with 'git commit' So I had to resolve the conflict in ufshcd_probe_hba() The line: ufs_advertise_fixup_device(hba); was modified by commit 93fdd5ac64bbe80dac6416f048405362d7ef0945 It's not clear to me if you want me to 1) document that there was a conflict 2) change the title of the patch 3) both 4) something else altogether Regards.
Hi Marc, > I indeed started off from 'git revert' > > $ git revert 60f0187031c0 > warning: inexact rename detection was skipped due to too many files. > warning: you may want to set your merge.renamelimit variable to at > least 18258 and retry the command. > error: could not revert 60f0187031c0... scsi: ufs: disable vccq if > it's not needed by UFS device > hint: after resolving the conflicts, mark the corrected paths > hint: with 'git add <paths>' or 'git rm <paths>' > hint: and commit the result with 'git commit' > > So I had to resolve the conflict in ufshcd_probe_hba() > > The line: > > ufs_advertise_fixup_device(hba); > > was modified by commit 93fdd5ac64bbe80dac6416f048405362d7ef0945 If it's a resolvable delta, a proper git revert is preferred. Please document any conflicts in the commit message and list the relevant commits that introduced them. If you find yourself in a situation where reverting simply isn't feasible, I'd expect the commit to state "This should have been a revert but I'd have to boil the oceans to resolve the conflicts because XYZ..."
On 26/02/2019 17:26, Martin K. Petersen wrote: >> I indeed started off from 'git revert' >> >> $ git revert 60f0187031c0 >> warning: inexact rename detection was skipped due to too many files. >> warning: you may want to set your merge.renamelimit variable to at >> least 18258 and retry the command. >> error: could not revert 60f0187031c0... scsi: ufs: disable vccq if >> it's not needed by UFS device >> hint: after resolving the conflicts, mark the corrected paths >> hint: with 'git add <paths>' or 'git rm <paths>' >> hint: and commit the result with 'git commit' >> >> So I had to resolve the conflict in ufshcd_probe_hba() >> >> The line: >> >> ufs_advertise_fixup_device(hba); >> >> was modified by commit 93fdd5ac64bbe80dac6416f048405362d7ef0945 > > If it's a resolvable delta, a proper git revert is preferred. Please > document any conflicts in the commit message and list the relevant > commits that introduced them. > > If you find yourself in a situation where reverting simply isn't > feasible, I'd expect the commit to state "This should have been a revert > but I'd have to boil the oceans to resolve the conflicts because XYZ..." OK, I'll do my best, but I've never been in this situation before. According to git blame, ufshcd_probe_hba() has been heavily modified around the ufshcd_set_vccq_rail_unused() call to be reverted: a4b0e8a4e92b1 (Potomski, MichalX 2017-02-23 09:05:30 +0000 6820) /* Init check for device descriptor sizes */ a4b0e8a4e92b1 (Potomski, MichalX 2017-02-23 09:05:30 +0000 6821) ufshcd_init_desc_sizes(hba); a4b0e8a4e92b1 (Potomski, MichalX 2017-02-23 09:05:30 +0000 6822) 93fdd5ac64bbe (Tomas Winkler 2017-01-05 10:45:12 +0200 6823) ret = ufs_get_device_desc(hba, &card); 93fdd5ac64bbe (Tomas Winkler 2017-01-05 10:45:12 +0200 6824) if (ret) { 93fdd5ac64bbe (Tomas Winkler 2017-01-05 10:45:12 +0200 6825) dev_err(hba->dev, "%s: Failed getting device info. err = %d\n", 93fdd5ac64bbe (Tomas Winkler 2017-01-05 10:45:12 +0200 6826) __func__, ret); 93fdd5ac64bbe (Tomas Winkler 2017-01-05 10:45:12 +0200 6827) goto out; 93fdd5ac64bbe (Tomas Winkler 2017-01-05 10:45:12 +0200 6828) } 93fdd5ac64bbe (Tomas Winkler 2017-01-05 10:45:12 +0200 6829) 93fdd5ac64bbe (Tomas Winkler 2017-01-05 10:45:12 +0200 6830) ufs_fixup_device_setup(hba, &card); 371131065de99 (Yaniv Gardi 2016-03-10 17:37:16 +0200 6831) ufshcd_tune_unipro_params(hba); 60f0187031c05 (Yaniv Gardi 2016-03-10 17:37:11 +0200 6832) 60f0187031c05 (Yaniv Gardi 2016-03-10 17:37:11 +0200 6833) ret = ufshcd_set_vccq_rail_unused(hba, 60f0187031c05 (Yaniv Gardi 2016-03-10 17:37:11 +0200 6834) (hba->dev_quirks & UFS_DEVICE_NO_VCCQ) ? true : false); 60f0187031c05 (Yaniv Gardi 2016-03-10 17:37:11 +0200 6835) if (ret) 60f0187031c05 (Yaniv Gardi 2016-03-10 17:37:11 +0200 6836) goto out; 60f0187031c05 (Yaniv Gardi 2016-03-10 17:37:11 +0200 6837) 57d104c153d3d (Subhash Jadavani 2014-09-25 15:32:30 +0300 6838) /* UFS device is also active now */ 57d104c153d3d (Subhash Jadavani 2014-09-25 15:32:30 +0300 6839) ufshcd_set_ufs_dev_active(hba); 66ec6d59407ba (Sujit Reddy Thumma 2013-07-30 00:35:59 +0530 6840) ufshcd_force_reset_auto_bkops(hba); 57d104c153d3d (Subhash Jadavani 2014-09-25 15:32:30 +0300 6841) hba->wlun_dev_clr_ua = true; So the patch to revert is 60f0187031c05. 57d104c153d3d and 66ec6d59407ba are older, so no problem there. Basically, we just need to preserve the lines from 371131065de99, 93fdd5ac64bbe, a4b0e8a4e92b1 and we're good to go. The conflict looks like this: <<<<<<< HEAD /* Init check for device descriptor sizes */ ufshcd_init_desc_sizes(hba); ret = ufs_get_device_desc(hba, &card); if (ret) { dev_err(hba->dev, "%s: Failed getting device info. err = %d\n", __func__, ret); goto out; } ufs_fixup_device_setup(hba, &card); ufshcd_tune_unipro_params(hba); ret = ufshcd_set_vccq_rail_unused(hba, (hba->dev_quirks & UFS_DEVICE_NO_VCCQ) ? true : false); if (ret) goto out; ======= ufs_advertise_fixup_device(hba); >>>>>>> parent of 60f0187031c0... scsi: ufs: disable vccq if it's not needed by UFS device The resolution is simple: we keep the HEAD version, and simply remove ufshcd_set_vccq_rail_unused() and its error-handling. Is that the information you were after? Do you prefer the patch subject to be: Revert "scsi: ufs: disable vccq if it's not needed by UFS device" instead of: scsi: ufs: Do not disable vccq in UFSHC driver And I should leave in the automatically added This reverts commit 60f0187031c05e04cbadffb62f557d0ff3564490. while adding some information from the manual resolution? Regards.
Marc, > The resolution is simple: we keep the HEAD version, and simply remove > ufshcd_set_vccq_rail_unused() and its error-handling. > > > Is that the information you were after? Yes. > Do you prefer the patch subject to be: > Revert "scsi: ufs: disable vccq if it's not needed by UFS device" > instead of: > scsi: ufs: Do not disable vccq in UFSHC driver > > And I should leave in the automatically added > This reverts commit 60f0187031c05e04cbadffb62f557d0ff3564490. > while adding some information from the manual resolution? Exactly. Thanks!
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index 6d176815e6ce..21e4ccb5ba6e 100644 --- a/drivers/scsi/ufs/ufs.h +++ b/drivers/scsi/ufs/ufs.h @@ -514,7 +514,6 @@ struct ufs_vreg { struct regulator *reg; const char *name; bool enabled; - bool unused; int min_uV; int max_uV; int min_uA; diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 5ca4581e60d5..a5bfcf04fdba 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -251,7 +251,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba); static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on, bool skip_ref_clk); static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on); -static int ufshcd_set_vccq_rail_unused(struct ufs_hba *hba, bool unused); static int ufshcd_uic_hibern8_exit(struct ufs_hba *hba); static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba); static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba); @@ -6830,11 +6829,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba) ufs_fixup_device_setup(hba, &card); ufshcd_tune_unipro_params(hba); - ret = ufshcd_set_vccq_rail_unused(hba, - (hba->dev_quirks & UFS_DEVICE_NO_VCCQ) ? true : false); - if (ret) - goto out; - /* UFS device is also active now */ ufshcd_set_ufs_dev_active(hba); ufshcd_force_reset_auto_bkops(hba); @@ -7018,24 +7012,13 @@ static int ufshcd_config_vreg_load(struct device *dev, struct ufs_vreg *vreg, static inline int ufshcd_config_vreg_lpm(struct ufs_hba *hba, struct ufs_vreg *vreg) { - if (!vreg) - return 0; - else if (vreg->unused) - return 0; - else - return ufshcd_config_vreg_load(hba->dev, vreg, - UFS_VREG_LPM_LOAD_UA); + return ufshcd_config_vreg_load(hba->dev, vreg, UFS_VREG_LPM_LOAD_UA); } static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba, struct ufs_vreg *vreg) { - if (!vreg) - return 0; - else if (vreg->unused) - return 0; - else - return ufshcd_config_vreg_load(hba->dev, vreg, vreg->max_uA); + return ufshcd_config_vreg_load(hba->dev, vreg, vreg->max_uA); } static int ufshcd_config_vreg(struct device *dev, @@ -7073,9 +7056,7 @@ static int ufshcd_enable_vreg(struct device *dev, struct ufs_vreg *vreg) { int ret = 0; - if (!vreg) - goto out; - else if (vreg->enabled || vreg->unused) + if (!vreg || vreg->enabled) goto out; ret = ufshcd_config_vreg(dev, vreg, true); @@ -7095,9 +7076,7 @@ static int ufshcd_disable_vreg(struct device *dev, struct ufs_vreg *vreg) { int ret = 0; - if (!vreg) - goto out; - else if (!vreg->enabled || vreg->unused) + if (!vreg || !vreg->enabled) goto out; ret = regulator_disable(vreg->reg); @@ -7203,36 +7182,6 @@ static int ufshcd_init_hba_vreg(struct ufs_hba *hba) return 0; } -static int ufshcd_set_vccq_rail_unused(struct ufs_hba *hba, bool unused) -{ - int ret = 0; - struct ufs_vreg_info *info = &hba->vreg_info; - - if (!info) - goto out; - else if (!info->vccq) - goto out; - - if (unused) { - /* shut off the rail here */ - ret = ufshcd_toggle_vreg(hba->dev, info->vccq, false); - /* - * Mark this rail as no longer used, so it doesn't get enabled - * later by mistake - */ - if (!ret) - info->vccq->unused = true; - } else { - /* - * rail should have been already enabled hence just make sure - * that unused flag is cleared. - */ - info->vccq->unused = false; - } -out: - return ret; -} - static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on, bool skip_ref_clk) {
Commit 60f0187031c0 ("disable vccq if it's not needed by UFS device") introduced a small power optimization as a driver quirk: ignore the vccq load specified in the UFSHC DT node when said host controller is connected to specific Flash chips (Samsung and Hynix currently). Unfortunately, this optimization breaks UFS on systems where vccq powers not only the Flash chip, but the host controller as well, such as APQ8098 MEDIABOX or MTP8998: [ 3.929877] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04 for idn 13 failed, index 0, err = -11 [ 5.433815] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04 for idn 13 failed, index 0, err = -11 [ 6.937771] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04 for idn 13 failed, index 0, err = -11 [ 6.937866] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr_retry: query attribute, idn 13, failed with error -11 after 3 retires [ 6.946412] ufshcd-qcom 1da4000.ufshc: ufshcd_disable_auto_bkops: failed to enable exception event -11 [ 6.957972] ufshcd-qcom 1da4000.ufshc: dme-peer-get: attr-id 0x1587 failed 3 retries [ 6.967181] ufshcd-qcom 1da4000.ufshc: dme-peer-get: attr-id 0x1586 failed 3 retries [ 6.975025] ufshcd-qcom 1da4000.ufshc: ufshcd_get_max_pwr_mode: invalid max pwm tx gear read = 0 [ 6.982755] ufshcd-qcom 1da4000.ufshc: ufshcd_probe_hba: Failed getting max supported power mode [ 8.505770] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag query for idn 3 failed, err = -11 [ 10.009807] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag query for idn 3 failed, err = -11 [ 11.513766] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag query for idn 3 failed, err = -11 [ 11.513861] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag_retry: query attribute, opcode 5, idn 3, failed with error -11 after 3 retires [ 13.049807] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor: opcode 0x01 for idn 8 failed, index 0, err = -11 [ 14.553768] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor: opcode 0x01 for idn 8 failed, index 0, err = -11 [ 16.057767] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor: opcode 0x01 for idn 8 failed, index 0, err = -11 [ 16.057872] ufshcd-qcom 1da4000.ufshc: ufshcd_read_desc_param: Failed reading descriptor. desc_id 8, desc_index 0, param_offset 0, ret -11 [ 16.067109] ufshcd-qcom 1da4000.ufshc: ufshcd_init_icc_levels: Failed reading power descriptor.len = 98 ret = -11 [ 37.073787] ufshcd-qcom 1da4000.ufshc: link startup failed 1 In my opinion, the rationale for the original patch is questionable. If neither the UFSHC, nor the Flash chip, require any load from vccq, then that power rail should simply not be specified at all in the DT. Working around that fact in the driver is detrimental, as evidenced by the failure to initialize the host controller on MSM8998. Revert the original patch, and clean up loose ends in the next patch. Relevant patches: 60f0187031c05e04cbadffb62f557d0ff3564490 c58ab7aab71e2c783087115f0ce1623c2fdcf0b2 46c1cf706076500cdcde3445be97233793eec7f1 Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> --- drivers/scsi/ufs/ufs.h | 1 - drivers/scsi/ufs/ufshcd.c | 59 +++------------------------------------ 2 files changed, 4 insertions(+), 56 deletions(-)