On 08/09/2022 16:23, Doug Anderson wrote: > Hi, > > On Thu, Sep 8, 2022 at 3:25 AM Krzysztof Kozlowski > <krzysztof.kozlowski@xxxxxxxxxx> 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@xxxxxxxxxx> >>> Reviewed-by: Johan Hovold <johan+kernel@xxxxxxxxxx> >>> Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx> >>> Signed-off-by: Andrew Halaney <ahalaney@xxxxxxxxxx> >>> --- >>> >>> v2: https://lore.kernel.org/linux-arm-msm/20220906201959.69920-1-ahalaney@xxxxxxxxxx/ >>> 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
