| Message ID | 20220726102024.1.Icc838fe7bf0ef54a014ab2fee8af311654f5342a@changeid (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load() | expand |
On Tue, Jul 26, 2022 at 10:20:29AM -0700, Douglas Anderson wrote: > Since we don't actually pass the load to the firmware, switch to using > get_optimum_mode() instead of open-coding it. > > This is intended to have no effect other than cleanup. Thanks, this looks sensible. Unless there's issues I'll apply at -rc1.
On Tue, 26 Jul 2022 10:20:29 -0700, Douglas Anderson wrote: > Since we don't actually pass the load to the firmware, switch to using > get_optimum_mode() instead of open-coding it. > > This is intended to have no effect other than cleanup. > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next Thanks! [1/1] regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load() commit: efb0cb50c42734f868908a97f0d93e9208da1f0e All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hey Douglas, On Tue, Jul 26, 2022 at 10:20:29AM -0700, Douglas Anderson wrote: > Since we don't actually pass the load to the firmware, switch to using > get_optimum_mode() instead of open-coding it. > > This is intended to have no effect other than cleanup. I hate to post something without looking into it very deeply, but with this statement about no effect I figured I'd report what I'm noticing before diving deeper. I'm currently playing with the dts patches in [0], and without this patch (and a hack patch applied that is unrelated to this) the board boots fine. With this patch applied I get the messages reported in [1] (which I think is still a clean up that should be applied) and the board fails to boot due to regulator_enable() failing at the first invocation I see. Is that something you expect? Here's the ultimate failure message for regulator_enable(): [ 1.213419] ufshcd-qcom 1d84000.ufs: Adding to iommu group 0 [ 1.219492] ufshcd-qcom 1d84000.ufs: ufshcd_populate_vreg: Unable to find vdd-hba-supply regulator, assuming enabled [ 1.230287] ufshcd-qcom 1d84000.ufs: ufshcd_populate_vreg: Unable to find vccq2-supply regulator, assuming enabled [ 1.241079] vreg_l17c: invalid input voltage found [ 1.246002] ufshcd-qcom 1d84000.ufs: ufshcd_enable_vreg: vcc enable failed, err=-22 [ 1.253952] ufshcd-qcom 1d84000.ufs: Initialization failed [ 1.259622] ufshcd-qcom 1d84000.ufs: ufshcd_pltfrm_init() failed -22 [ 1.266151] ufshcd-qcom: probe of 1d84000.ufs failed with error -22 [0] https://lore.kernel.org/all/20220722143711.17563-1-quic_ppareek@quicinc.com/ [1] https://lore.kernel.org/all/166118500944.215002.10320899094541954077.b4-ty@kernel.org/ Thanks, Andrew > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/regulator/qcom-rpmh-regulator.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c > index 561de6b2e6e3..b2debde79361 100644 > --- a/drivers/regulator/qcom-rpmh-regulator.c > +++ b/drivers/regulator/qcom-rpmh-regulator.c > @@ -306,9 +306,10 @@ static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev) > } > > /** > - * rpmh_regulator_vrm_set_load() - set the regulator mode based upon the load > - * current requested > + * rpmh_regulator_vrm_get_optimum_mode() - get the mode based on the load > * @rdev: Regulator device pointer for the rpmh-regulator > + * @input_uV: Input voltage > + * @output_uV: Output voltage > * @load_uA: Aggregated load current in microamps > * > * This function is used in the regulator_ops for VRM type RPMh regulator > @@ -316,17 +317,15 @@ static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev) > * > * Return: 0 on success, errno on failure > */ > -static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA) > +static unsigned int rpmh_regulator_vrm_get_optimum_mode( > + struct regulator_dev *rdev, int input_uV, int output_uV, int load_uA) > { > struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); > - unsigned int mode; > > if (load_uA >= vreg->hw_data->hpm_min_load_uA) > - mode = REGULATOR_MODE_NORMAL; > + return REGULATOR_MODE_NORMAL; > else > - mode = REGULATOR_MODE_IDLE; > - > - return rpmh_regulator_vrm_set_mode(rdev, mode); > + return REGULATOR_MODE_IDLE; > } > > static int rpmh_regulator_vrm_set_bypass(struct regulator_dev *rdev, > @@ -375,7 +374,7 @@ static const struct regulator_ops rpmh_regulator_vrm_drms_ops = { > .list_voltage = regulator_list_voltage_linear_range, > .set_mode = rpmh_regulator_vrm_set_mode, > .get_mode = rpmh_regulator_vrm_get_mode, > - .set_load = rpmh_regulator_vrm_set_load, > + .get_optimum_mode = rpmh_regulator_vrm_get_optimum_mode, > }; > > static const struct regulator_ops rpmh_regulator_vrm_bypass_ops = { > -- > 2.37.1.359.gd136c6c3e2-goog >
On Mon, Aug 22, 2022 at 02:31:53PM -0500, Andrew Halaney wrote: > Hey Douglas, > > On Tue, Jul 26, 2022 at 10:20:29AM -0700, Douglas Anderson wrote: > > Since we don't actually pass the load to the firmware, switch to using > > get_optimum_mode() instead of open-coding it. > > > > This is intended to have no effect other than cleanup. > > I hate to post something without looking into it very deeply, but with > this statement about no effect I figured I'd report what I'm noticing > before diving deeper. > > I'm currently playing with the dts patches in [0], and without this > patch (and a hack patch applied that is unrelated to this) the board boots > fine. With this patch applied I get the messages reported in [1] (which I > think is still a clean up that should be applied) and the board fails to > boot due to regulator_enable() failing at the first invocation I see. Sorry for more spam, but just to be clear, in the above I'm saying that if I revert this patch the error message below goes away and the regulator_enable() call succeeds allowing the board to boot. Re-reading what I wrote it sounds like a jumbled mess. > > Is that something you expect? > > Here's the ultimate failure message for regulator_enable(): > > [ 1.213419] ufshcd-qcom 1d84000.ufs: Adding to iommu group 0 > [ 1.219492] ufshcd-qcom 1d84000.ufs: ufshcd_populate_vreg: Unable to find vdd-hba-supply regulator, assuming enabled > [ 1.230287] ufshcd-qcom 1d84000.ufs: ufshcd_populate_vreg: Unable to find vccq2-supply regulator, assuming enabled > [ 1.241079] vreg_l17c: invalid input voltage found > [ 1.246002] ufshcd-qcom 1d84000.ufs: ufshcd_enable_vreg: vcc enable failed, err=-22 > [ 1.253952] ufshcd-qcom 1d84000.ufs: Initialization failed > [ 1.259622] ufshcd-qcom 1d84000.ufs: ufshcd_pltfrm_init() failed -22 > [ 1.266151] ufshcd-qcom: probe of 1d84000.ufs failed with error -22 > > [0] https://lore.kernel.org/all/20220722143711.17563-1-quic_ppareek@quicinc.com/ > [1] https://lore.kernel.org/all/166118500944.215002.10320899094541954077.b4-ty@kernel.org/ > > Thanks, > Andrew > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > drivers/regulator/qcom-rpmh-regulator.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c > > index 561de6b2e6e3..b2debde79361 100644 > > --- a/drivers/regulator/qcom-rpmh-regulator.c > > +++ b/drivers/regulator/qcom-rpmh-regulator.c > > @@ -306,9 +306,10 @@ static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev) > > } > > > > /** > > - * rpmh_regulator_vrm_set_load() - set the regulator mode based upon the load > > - * current requested > > + * rpmh_regulator_vrm_get_optimum_mode() - get the mode based on the load > > * @rdev: Regulator device pointer for the rpmh-regulator > > + * @input_uV: Input voltage > > + * @output_uV: Output voltage > > * @load_uA: Aggregated load current in microamps > > * > > * This function is used in the regulator_ops for VRM type RPMh regulator > > @@ -316,17 +317,15 @@ static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev) > > * > > * Return: 0 on success, errno on failure > > */ > > -static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA) > > +static unsigned int rpmh_regulator_vrm_get_optimum_mode( > > + struct regulator_dev *rdev, int input_uV, int output_uV, int load_uA) > > { > > struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); > > - unsigned int mode; > > > > if (load_uA >= vreg->hw_data->hpm_min_load_uA) > > - mode = REGULATOR_MODE_NORMAL; > > + return REGULATOR_MODE_NORMAL; > > else > > - mode = REGULATOR_MODE_IDLE; > > - > > - return rpmh_regulator_vrm_set_mode(rdev, mode); > > + return REGULATOR_MODE_IDLE; > > } > > > > static int rpmh_regulator_vrm_set_bypass(struct regulator_dev *rdev, > > @@ -375,7 +374,7 @@ static const struct regulator_ops rpmh_regulator_vrm_drms_ops = { > > .list_voltage = regulator_list_voltage_linear_range, > > .set_mode = rpmh_regulator_vrm_set_mode, > > .get_mode = rpmh_regulator_vrm_get_mode, > > - .set_load = rpmh_regulator_vrm_set_load, > > + .get_optimum_mode = rpmh_regulator_vrm_get_optimum_mode, > > }; > > > > static const struct regulator_ops rpmh_regulator_vrm_bypass_ops = { > > -- > > 2.37.1.359.gd136c6c3e2-goog > >
Hi, On Mon, Aug 22, 2022 at 12:32 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > Hey Douglas, > > On Tue, Jul 26, 2022 at 10:20:29AM -0700, Douglas Anderson wrote: > > Since we don't actually pass the load to the firmware, switch to using > > get_optimum_mode() instead of open-coding it. > > > > This is intended to have no effect other than cleanup. > > I hate to post something without looking into it very deeply, but with > this statement about no effect I figured I'd report what I'm noticing > before diving deeper. > > I'm currently playing with the dts patches in [0], and without this > patch (and a hack patch applied that is unrelated to this) the board boots > fine. With this patch applied I get the messages reported in [1] (which I > think is still a clean up that should be applied) and the board fails to > boot due to regulator_enable() failing at the first invocation I see. > > Is that something you expect? > > Here's the ultimate failure message for regulator_enable(): > > [ 1.213419] ufshcd-qcom 1d84000.ufs: Adding to iommu group 0 > [ 1.219492] ufshcd-qcom 1d84000.ufs: ufshcd_populate_vreg: Unable to find vdd-hba-supply regulator, assuming enabled > [ 1.230287] ufshcd-qcom 1d84000.ufs: ufshcd_populate_vreg: Unable to find vccq2-supply regulator, assuming enabled > [ 1.241079] vreg_l17c: invalid input voltage found > [ 1.246002] ufshcd-qcom 1d84000.ufs: ufshcd_enable_vreg: vcc enable failed, err=-22 > [ 1.253952] ufshcd-qcom 1d84000.ufs: Initialization failed > [ 1.259622] ufshcd-qcom 1d84000.ufs: ufshcd_pltfrm_init() failed -22 > [ 1.266151] ufshcd-qcom: probe of 1d84000.ufs failed with error -22 > > [0] https://lore.kernel.org/all/20220722143711.17563-1-quic_ppareek@quicinc.com/ > [1] https://lore.kernel.org/all/166118500944.215002.10320899094541954077.b4-ty@kernel.org/ Ah, I see what's happening. When we were using set_load() all the regulator core did was call into RPMH and tell it the load. ...but with the get_optimum_mode() the regulator core needs to also pass the input and output voltages to RPMH as part of the API call (even though RPMH ignores them). That means that if the input voltages or output voltages are not known then it will error out before it even calls to RPMH. This all might be made worse by the fact that RPMH regulators boot up not knowing what their voltage is (see "vreg->voltage_selector = -ENOTRECOVERABLE" in rpmh_regulator_init_vreg()). Ah, I guess it's failing to get the "input" voltage though. I guess officially you could fix that by specifying the input supplies? I know we never bothered doing that on any trogdor devices, though. If I remember correctly all of the magic init that the firmware does for RPMH already magically makes the input supplies get enabled / voltage tuned properly so we decided that wasn't a benefit to modeling them in Linux... ...hmmm, so I tried to figure out why I didn't see a failure and I realized that in trogdor we never actually set `regulator-allow-set-load` so basically we're not running this code at all, so when I boot tested it on my hardware it wasn't terribly useful. :( I guess at this point I'll wait for Mark to give his suggestion for what to do. Options I'm aware of: a) ${SUBJECT} patch was written as a cleanup as per Mark's request and we could just revert it. b) We could make it so that failures to get the input/output voltages doesn't count as an error when going through the get_optimum_mode() path. -Doug
On Mon, Aug 22, 2022 at 01:13:55PM -0700, Doug Anderson wrote: > I guess at this point I'll wait for Mark to give his suggestion for > what to do. Options I'm aware of: > a) ${SUBJECT} patch was written as a cleanup as per Mark's request and > we could just revert it. > b) We could make it so that failures to get the input/output voltages > doesn't count as an error when going through the get_optimum_mode() > path. We could push the checks for a valid voltage down into the drivers, a lot of things aren't particularly sensitive to the output voltage here and are much more sensitive to the current draw. Depending on people's attitudes to DT stability for Qualcomm platforms we could also fix the affected DTs as well, though that doesn't stop us handling this in the core too.
Hi, On Tue, Aug 23, 2022 at 6:06 AM Mark Brown <broonie@kernel.org> wrote: > > On Mon, Aug 22, 2022 at 01:13:55PM -0700, Doug Anderson wrote: > > > I guess at this point I'll wait for Mark to give his suggestion for > > what to do. Options I'm aware of: > > > a) ${SUBJECT} patch was written as a cleanup as per Mark's request and > > we could just revert it. > > > b) We could make it so that failures to get the input/output voltages > > doesn't count as an error when going through the get_optimum_mode() > > path. > > We could push the checks for a valid voltage down into the drivers, a > lot of things aren't particularly sensitive to the output voltage here > and are much more sensitive to the current draw. Depending on people's > attitudes to DT stability for Qualcomm platforms we could also fix the > affected DTs as well, though that doesn't stop us handling this in the > core too. OK, patch posted with this approach. ("regulator: core: Require regulator drivers to check uV for get_optimum_mode()") [1] [1] https://lore.kernel.org/r/20220823131629.RFT.1.I137e6bef4f6d517be7b081be926059321102fd3d@changeid
diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c index 561de6b2e6e3..b2debde79361 100644 --- a/drivers/regulator/qcom-rpmh-regulator.c +++ b/drivers/regulator/qcom-rpmh-regulator.c @@ -306,9 +306,10 @@ static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev) } /** - * rpmh_regulator_vrm_set_load() - set the regulator mode based upon the load - * current requested + * rpmh_regulator_vrm_get_optimum_mode() - get the mode based on the load * @rdev: Regulator device pointer for the rpmh-regulator + * @input_uV: Input voltage + * @output_uV: Output voltage * @load_uA: Aggregated load current in microamps * * This function is used in the regulator_ops for VRM type RPMh regulator @@ -316,17 +317,15 @@ static unsigned int rpmh_regulator_vrm_get_mode(struct regulator_dev *rdev) * * Return: 0 on success, errno on failure */ -static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA) +static unsigned int rpmh_regulator_vrm_get_optimum_mode( + struct regulator_dev *rdev, int input_uV, int output_uV, int load_uA) { struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); - unsigned int mode; if (load_uA >= vreg->hw_data->hpm_min_load_uA) - mode = REGULATOR_MODE_NORMAL; + return REGULATOR_MODE_NORMAL; else - mode = REGULATOR_MODE_IDLE; - - return rpmh_regulator_vrm_set_mode(rdev, mode); + return REGULATOR_MODE_IDLE; } static int rpmh_regulator_vrm_set_bypass(struct regulator_dev *rdev, @@ -375,7 +374,7 @@ static const struct regulator_ops rpmh_regulator_vrm_drms_ops = { .list_voltage = regulator_list_voltage_linear_range, .set_mode = rpmh_regulator_vrm_set_mode, .get_mode = rpmh_regulator_vrm_get_mode, - .set_load = rpmh_regulator_vrm_set_load, + .get_optimum_mode = rpmh_regulator_vrm_get_optimum_mode, }; static const struct regulator_ops rpmh_regulator_vrm_bypass_ops = {
Since we don't actually pass the load to the firmware, switch to using get_optimum_mode() instead of open-coding it. This is intended to have no effect other than cleanup. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/regulator/qcom-rpmh-regulator.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)