Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Sep 2022 09:53:13 -0500
From:   Andrew Halaney <ahalaney@redhat.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     agross@kernel.org, andersson@kernel.org,
        konrad.dybcio@somainline.org, lgirdwood@gmail.com,
        broonie@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        dianders@chromium.org, johan@kernel.org,
        Johan Hovold <johan+kernel@kernel.org>
Subject: Re: [PATCH v3] regulator: dt-bindings: qcom,rpmh: Indicate
 regulator-allow-set-load dependencies
Message-ID: <20220908145313.36coao263bntngn6@halaneylaptop>
References: <20220907204924.173030-1-ahalaney@redhat.com>
 <abad381a-dfe7-9337-ff35-f657bf373d44@linaro.org>
 <20220908145038.o4olsfencb54uqiz@halaneylaptop>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220908145038.o4olsfencb54uqiz@halaneylaptop>
Precedence: bulk

Gah, ignore all the below. I drafted something up, went to something
else, reviewed and sent to realize this was addressed already by others.

Thanks,
Andrew

On Thu, Sep 08, 2022 at 09:50:38AM -0500, Andrew Halaney wrote:
> 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@kernel.org>
> > > Reviewed-by: Johan Hovold <johan+kernel@kernel.org>
> > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > > Signed-off-by: Andrew Halaney <ahalaney@redhat.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
> >