Date:   Thu, 8 Sep 2022 09:50:38 -0500
From:   Andrew Halaney <ahalaney@...hat.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     agross@...nel.org, andersson@...nel.org,
        konrad.dybcio@...ainline.org, lgirdwood@...il.com,
        broonie@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        dianders@...omium.org, johan@...nel.org,
        Johan Hovold <johan+kernel@...nel.org>
Subject: Re: [PATCH v3] regulator: dt-bindings: qcom,rpmh: Indicate
 regulator-allow-set-load dependencies

On Thu, Sep 08, 2022 at 12:25:25PM +0200, Krzysztof Kozlowski wrote:
> On 07/09/2022 22:49, Andrew Halaney wrote:
> > For RPMH regulators it doesn't make sense to indicate
> > regulator-allow-set-load without saying what modes you can switch to,
> > so be sure to indicate a dependency on regulator-allowed-modes.
> > 
> > In general this is true for any regulators that are setting modes
> > instead of setting a load directly, for example RPMH regulators. A
> > counter example would be RPM based regulators, which set a load
> > change directly instead of a mode change. In the RPM case
> > regulator-allow-set-load alone is sufficient to describe the regulator
> > (the regulator can change its output current, here's the new load),
> > but in the RPMH case what valid operating modes exist must also be
> > stated to properly describe the regulator (the new load is this, what
> > is the optimum mode for this regulator with that load, let's change to
> > that mode now).
> > 
> > With this in place devicetree validation can catch issues like this:
> > 
> >     /mnt/extrassd/git/linux-next/arch/arm64/boot/dts/qcom/sm8350-hdk.dtb: pm8350-rpmh-regulators: ldo5: 'regulator-allowed-modes' is a dependency of 'regulator-allow-set-load'
> >             From schema: /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
> > 
> > Where the RPMH regulator hardware is described as being settable, but
> > there are no modes described to set it to!
> > 
> > Suggested-by: Johan Hovold <johan+kernel@...nel.org>
> > Reviewed-by: Johan Hovold <johan+kernel@...nel.org>
> > Reviewed-by: Douglas Anderson <dianders@...omium.org>
> > Signed-off-by: Andrew Halaney <ahalaney@...hat.com>
> > ---
> > 
> > v2: https://lore.kernel.org/linux-arm-msm/20220906201959.69920-1-ahalaney@redhat.com/
> > Changes since v2:
> >   - Updated commit message to explain how this is a property of the
> >     hardware, and why it only applies to certain regulators like RPMH
> >     (Johan + Krzysztof recommendation)
> >   - Added Johan + Douglas' R-B tags
> 
> You posted before we finished discussion so let me paste it here:
> 
> The bindings don't express it, but the regulator core explicitly asks
> for set_mode with set_load callbacks in drms_uA_update(), which depends
> on REGULATOR_CHANGE_DRMS (toggled with regulator-allow-set-load).

If I follow correctly it isn't asking for both, just either set_mode()
or set_load():

https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/regulator/core.c#L961
copy pasta below
	/*
	 * first check to see if we can set modes at all, otherwise just
	 * tell the consumer everything is OK.
	 */
	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_DRMS)) {
		rdev_dbg(rdev, "DRMS operation not allowed\n");
		return 0;
	}

	if (!rdev->desc->ops->get_optimum_mode &&
	    !rdev->desc->ops->set_load)
		return 0;

	if (!rdev->desc->ops->set_mode &&
	    !rdev->desc->ops->set_load)
		return -EINVAL;

I'm interpreting the if statements as:

    1. Can the core set the load (as you highlighted REGULATOR_CHANGE_DRMS is
       toggled by regulator-allow-set-load) for this hardware at all?

    2. Are we able to determine the best mode to switch to, or can we
       just set the load directly? If neither of those the core can't do
       much

    3. Can we set the mode we determined was
       optimum with get_optimum_mode()? If the hardware is settable, and
       the core can determine a new mode but there's no mode set_mode()
       to actually switch that's an error unless we can just call
       set_load() directly with our new current requirement

That's a long winded way of saying I don't think the core asks for
set_mode && set_load callbacks to be implemented (which is how I
interpreted your message above).

> 
> drms_uA_update() later calls regulator_mode_constrain() which checks if
> mode changing is allowed (REGULATOR_CHANGE_MODE).

If set_load() is implemented this is not checked and the load is set
directly before returning from drms_uA_update().
https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/regulator/core.c#L973

> 
> Therefore based on current implementation and meaning of
> set-load/allowed-modes properties, I would say that this applies to all
> regulators. I don't think that RPMh is special here.
> 

The above comments are why I don't think it applies to *all* regulators.
It does apply to any "mode based" regulator hardware though (which I
attempted to capture in the last paragraph in the commit message), but
unfortunately I do not know of a way to do the below pseudo check at a
regulator-wide binding level:

    if regulator-allow-set-load && !set_load() && set_mode() && !regulator-allowed-modes:
        complain_about_invalid_devicetree()

Basically, the bindings don't really indicate the ops hardware supports
so I can't think of a good way to key check the set_mode() and
!set_load() bits to catch this at a wider level, so I opted to just
attack the dt-binding at a hardware specific level since I can make
assumptions about what operations the hardware supports at that level.

So, with this approach I do only plug the hole up for RPMh users, other
set_mode() users are still at risk. Other than duplicating this to those
users I can't really think of a generic way to tackle this at the
regulator.yaml level since I don't see a good way to grab the ops
supported.

We could maybe add extra bindings to indicate what ops are
supported, i.e. regulator-set-load and regulator-set-mode, and then have
(hopefully this is possible in the dt-bindings) some if statements like:

    if (regulator-allow-set-load) {
        if (regulator-set-load)
            return 0;
        else if (regulator-set-mode && !regulator-allowed-modes)
            return -EINVAL;
        else
            return -EINVAL;
    }

But I'm not really sure how I feel about making each dt-binding specify
what ops their hardware supports.

Regardless I think the current patch helps out RPMh users.. but I'm open
to extending it if we can come up with a good way to do it!

Thanks,
Andrew

> Best regards,
> Krzysztof
> 

Powered by blists - more mailing lists