Message-ID: <CAD=FV=VMKGOLR5uupTk9Z2vyDfCksYjJLHF6DDM36rgh+=0xLA@mail.gmail.com>
Date:   Thu, 8 Sep 2022 07:38:32 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Andrew Halaney <ahalaney@...hat.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Johan Hovold <johan@...nel.org>,
        Johan Hovold <johan+kernel@...nel.org>
Subject: Re: [PATCH v3] regulator: dt-bindings: qcom,rpmh: Indicate
 regulator-allow-set-load dependencies

Hi,

On Thu, Sep 8, 2022 at 7:29 AM Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> wrote:
>
> On 08/09/2022 16:23, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Sep 8, 2022 at 3:25 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@...aro.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@...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).
> >>
> >> 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.
>
> So how does set load works for this case? You mentioned
> "allow-set-mode", but we talk about "allow-set-load".

Ah, sorry. I meant "allow-set-load".

> > 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.
>
> Sorry, I don't find it related. Whether RPM has modes or not, does not
> matter to this discussion unless it sets as well allow-set-load without
> the mode... and then how does it work? In current implementation it
> shouldn't...

>From looking at the source code of Linux:

* allow-set-load basically says whether the core regulator framework
even pays attention when drivers specify how much load they're using.

* On RPM then if allow-set-load is set then we'll sum up all of the
load requests from clients and pass it to hardware.

* On RPMH, if allow-set-load is set then we'll sum up all the load
requests, translate that to a mode, validate it against the set of
"allowable" modes, and if it's valid then pass it to hardware.

-Doug

Powered by blists - more mailing lists