| Message ID | 20220907204924.173030-1-ahalaney@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| 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
Hi, On Thu, Sep 8, 2022 at 7:29 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.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@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". 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
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 08/09/2022 16:38, Doug Anderson wrote: > > 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. OK, makes sense. Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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 >
Gah, ignore all the below. I drafted something up, went to something else, reviewed and sent to realize this was addressed already by others. Thanks, Andrew On Thu, Sep 08, 2022 at 09:50:38AM -0500, Andrew Halaney wrote: > 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, 7 Sep 2022 15:49:24 -0500, 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). > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next Thanks! [1/1] regulator: dt-bindings: qcom,rpmh: Indicate regulator-allow-set-load dependencies commit: 08865c2150392f67769a9d6e0b02800be226a990 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
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? 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(+)