Re: [RFT PATCH] regulator: core: Require regulator drivers to check uV for get_optimum_mode()

From: Andrew Halaney <ahalaney@redhat.com>
To: Douglas Anderson <dianders@chromium.org>
Cc: Mark Brown <broonie@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [RFT PATCH] regulator: core: Require regulator drivers to check uV for get_optimum_mode()
Date: Tue, 23 Aug 2022 17:14:29 -0500	[thread overview]
Message-ID: <20220823221429.3bte2tgtyniur4wb@halaneylaptop> (raw)
In-Reply-To: <20220823131629.RFT.1.I137e6bef4f6d517be7b081be926059321102fd3d@changeid>

Hey Doug,

On Tue, Aug 23, 2022 at 01:16:34PM -0700, Douglas Anderson wrote:
> The get_optimum_mode() for regulator drivers is passed the input
> voltage and output voltage as well as the current. This is because, in
> theory, the optimum mode can depend on all three things.
> 
> It turns out that for all regulator drivers in mainline only the
> current is looked at when implementing get_optimum_mode(). None of the
> drivers take the input or output voltage into account. Despite the
> fact that none of the drivers take the input or output voltage into
> account, though, the regulator framework will error out before calling
> into get_optimum_mode() if it doesn't know the input or output
> voltage.
> 
> The above behavior turned out to be a probelm for some boards when we
> landed commit efb0cb50c427 ("regulator: qcom-rpmh: Implement
> get_optimum_mode(), not set_load()"). Before that change we'd have no
> problems running drms_uA_update() for RPMH regulators even if a
> regulator's input or output voltage was unknown. After that change
> drms_uA_update() started to fail. This is because typically boards
> using RPMH regulators don't model the input supplies of RPMH
> regulators. Input supplies for RPMH regulators nearly always come from
> the output of other RPMH regulators (or always-on regulators) and RPMH
> firmware is initialized with this knowledge and handles enabling (and
> adjusting the voltage of) input supplies. While we could model the
> parent/child relationship of the regulators in Linux, many boards
> don't bother since it adds extra overhead.
> 
> Let's change the regulator core to make things work again. Now if we
> fail to get the input or output voltage we'll still call into
> get_optimum_mode() and we'll just pass error codes in for input_uV
> and/or output_uV parameters.
> 
> Since no existing regulator drivers even look at input_uV and
> output_uV we don't need to add this error handling anywhere right
> now. We'll add some comments in the core so that it's obvious that (if
> regulator drivers care) it's up to them to add the checks.
> 
> Reported-by: Andrew Halaney <ahalaney@redhat.com>
> Fixes: efb0cb50c427 ("regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  drivers/regulator/core.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 5b5da14976c2..0bc4b9b0a885 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -979,10 +979,13 @@ static int drms_uA_update(struct regulator_dev *rdev)
>  	} else {
>  		/* get output voltage */
>  		output_uV = regulator_get_voltage_rdev(rdev);
> -		if (output_uV <= 0) {
> -			rdev_err(rdev, "invalid output voltage found\n");
> -			return -EINVAL;
> -		}
> +
> +		/*
> +		 * Don't return an error; if regulator driver cares about
> +		 * output_uV then it's up to the driver to validate.
> +		 */
> +		if (output_uV <= 0)
> +			rdev_dbg(rdev, "invalid output voltage found\n");
>  
>  		/* get input voltage */
>  		input_uV = 0;
> @@ -990,10 +993,13 @@ static int drms_uA_update(struct regulator_dev *rdev)
>  			input_uV = regulator_get_voltage(rdev->supply);
>  		if (input_uV <= 0)
>  			input_uV = rdev->constraints->input_uV;
> -		if (input_uV <= 0) {
> -			rdev_err(rdev, "invalid input voltage found\n");
> -			return -EINVAL;
> -		}
> +
> +		/*
> +		 * Don't return an error; if regulator driver cares about
> +		 * input_uV then it's up to the driver to validate.
> +		 */
> +		if (input_uV <= 0)
> +			rdev_dbg(rdev, "invalid input voltage found\n");
>  
>  		/* now get the optimum mode for our new total regulator load */
>  		mode = rdev->desc->ops->get_optimum_mode(rdev, input_uV,
> -- 
> 2.37.2.609.g9ff673ca1a-goog
> 

I think this makes sense, but unfortunately it doesn't entirely fix my
issue introduced by efb0cb50c427 ("regulator: qcom-rpmh: Implement get_optimum_mode(), not set_load()"),
I'm seeing this now:

    [    1.240757] vreg_l17c: mode operation not allowed
    [    1.245586] vreg_l17c: failed to get optimum mode @ 800000 uA 0 -> 2504000 uV: -EPERM
    [    1.253631] ufshcd-qcom 1d84000.ufs: ufshcd_enable_vreg: vcc enable failed, err=-1

which if I understand correctly is because my devicetree isn't setting
regulator-allowed-modes. It appears most of the qcom one's don't set
that (and a good number set regulator-allow-set-load, which I think is
necessary for the RPMH regulator to get this far), so I think others
will be in the same boat as me.

Just for clarity, I'm running with this dtb right now:

    https://lore.kernel.org/all/20220812165453.11608-4-quic_ppareek@quicinc.com/

Thanks,
Andrew