| Message ID | 20220907204924.173030-1-ahalaney@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] regulator: dt-bindings: qcom,rpmh: Indicate regulator-allow-set-load dependencies | expand |
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. Best regards, Krzysztof
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
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
On Thu, Sep 08, 2022 at 04:29:29PM +0200, Krzysztof Kozlowski wrote: > 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... It works perfectly fine, if the driver has a set_load() operation then we call that and don't do anything to do with modes.
On 08/09/2022 16:38, Mark Brown wrote: > On Thu, Sep 08, 2022 at 04:29:29PM +0200, Krzysztof Kozlowski wrote: > >> 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... > > It works perfectly fine, if the driver has a set_load() operation then > we call that and don't do anything to do with modes. Ah, I see now set_load() in drms_uA_update() in first if-arm. Makes sense I guess. Best regards, Krzysztof
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 >
On Wed, Dec 28, 2022 at 11:37:06AM +0100, 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 > > Andrew, > > This patch was merged therefore we started seeing such warnings. Any > plans to actually fix them? Didn't Doug already do that? https://lore.kernel.org/all/20220829164952.2672848-1-dianders@chromium.org/ Johan
On 28/12/2022 11:58, Johan Hovold wrote: > On Wed, Dec 28, 2022 at 11:37:06AM +0100, 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 >> >> Andrew, >> >> This patch was merged therefore we started seeing such warnings. Any >> plans to actually fix them? > > Didn't Doug already do that? > > https://lore.kernel.org/all/20220829164952.2672848-1-dianders@chromium.org/ You're right, thanks. I keep seeing the error on sm8350-sony-xperia-sagami-pdx214 and I thought it is on every board. My bad. I'll fix the Xperia same way as HDK was fixed. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml index 9a36bee750af..92ff4d59ba20 100644 --- a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml +++ b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml @@ -99,12 +99,16 @@ properties: type: object $ref: "regulator.yaml#" description: BOB regulator node. + dependencies: + regulator-allow-set-load: ["regulator-allowed-modes"] patternProperties: "^(smps|ldo|lvs)[0-9]+$": type: object $ref: "regulator.yaml#" description: smps/ldo regulator nodes(s). + dependencies: + regulator-allow-set-load: ["regulator-allowed-modes"] required: - compatible
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 v1: https://lore.kernel.org/linux-arm-msm/20220902185148.635292-1-ahalaney@redhat.com/ Changes since v1: - Dropped first two patches in the series as they were user error (thanks Krzysztof for highlighting this!) - No change in the remaining patch .../devicetree/bindings/regulator/qcom,rpmh-regulator.yaml | 4 ++++ 1 file changed, 4 insertions(+)