[PATCH] regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()

[PATCH] regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()

[Douglas Anderson]

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(-)

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

Re: [PATCH] regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 327 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()

[Mark Brown]

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

Re: [PATCH] regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()

[Andrew Halaney]

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
> 

Re: [PATCH] regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()

[Andrew Halaney]

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
> > 

Re: [PATCH] regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()

[Doug Anderson]

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

Re: [PATCH] regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()

[Doug Anderson]

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