Re: [PATCH v3] regulator: dt-bindings: qcom,rpmh: Indicate regulator-allow-set-load dependencies

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 08/09/2022 16:23, Doug Anderson wrote:
> 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.

So how does set load works for this case? You mentioned
"allow-set-mode", but we talk about "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...

Best regards,
Krzysztof