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;
MIME-Version: 1.0
References: <20220907204924.173030-1-ahalaney@redhat.com> <abad381a-dfe7-9337-ff35-f657bf373d44@linaro.org>
In-Reply-To: <abad381a-dfe7-9337-ff35-f657bf373d44@linaro.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Thu, 8 Sep 2022 07:23:33 -0700
Message-ID: <CAD=FV=VmnKtx9smitqvNgmiCs-UCnLGFgbPnKd41QWeo1t3c9g@mail.gmail.com>
Subject: Re: [PATCH v3] regulator: dt-bindings: qcom,rpmh: Indicate
 regulator-allow-set-load dependencies
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     Andrew Halaney <ahalaney@redhat.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, Johan Hovold <johan@kernel.org>,
        Johan Hovold <johan+kernel@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Thu, Sep 8, 2022 at 3:25 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> 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).
>
> drms_uA_update() later calls regulator_mode_constrain() which checks if
> mode changing is allowed (REGULATOR_CHANGE_MODE).
>
> 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.

RPMh is special compared to RPM because in RPMh the hardware exposes
"modes" to the OS and in RPM the hardware doesn't. Specifically:

In RPM, the OS (Linux) has no idea what mode the regulator is running
at and what modes are valid. The OS just tells the RPM hardware "I'm
requesting a load of X uA. Thanks!" So "regulator-allow-set-mode"
basically says "yeah, let the OS talk to RPM about loads for this
regulator.

In RPMh, the OS knows all about the modes. For each regulator it's the
OS's job to know how much load the regulator can handle before it
needs to change modes. So the OS adds up all the load requests from
all the users of the regulator and then translates that to a mode. The
OS knows all about the modes possible for the regulator and limiting
them to a subset is a concept that is sensible.

This is why, for instance, there can be an "initial mode" specified
for RPMh but not for RPM. The OS doesn't ever know what mode a RPM
regulator is in but it does for RPMh.

-Doug