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 <[email protected]>
Reviewed-by: Johan Hovold <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
Signed-off-by: Andrew Halaney <[email protected]>
---
v2: https://lore.kernel.org/linux-arm-msm/[email protected]/
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/[email protected]/
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(+)
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
--
2.37.2
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 <[email protected]>
> Reviewed-by: Johan Hovold <[email protected]>
> Reviewed-by: Douglas Anderson <[email protected]>
> Signed-off-by: Andrew Halaney <[email protected]>
> ---
>
> v2: https://lore.kernel.org/linux-arm-msm/[email protected]/
> 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
<[email protected]> 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 <[email protected]>
> > Reviewed-by: Johan Hovold <[email protected]>
> > Reviewed-by: Douglas Anderson <[email protected]>
> > Signed-off-by: Andrew Halaney <[email protected]>
> > ---
> >
> > v2: https://lore.kernel.org/linux-arm-msm/[email protected]/
> > 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
Hi,
On Thu, Sep 8, 2022 at 7:29 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 08/09/2022 16:23, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Sep 8, 2022 at 3:25 AM Krzysztof Kozlowski
> > <[email protected]> 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 <[email protected]>
> >>> Reviewed-by: Johan Hovold <[email protected]>
> >>> Reviewed-by: Douglas Anderson <[email protected]>
> >>> Signed-off-by: Andrew Halaney <[email protected]>
> >>> ---
> >>>
> >>> v2: https://lore.kernel.org/linux-arm-msm/[email protected]/
> >>> 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:23, Doug Anderson wrote:
> Hi,
>
> On Thu, Sep 8, 2022 at 3:25 AM Krzysztof Kozlowski
> <[email protected]> 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 <[email protected]>
>>> Reviewed-by: Johan Hovold <[email protected]>
>>> Reviewed-by: Douglas Anderson <[email protected]>
>>> Signed-off-by: Andrew Halaney <[email protected]>
>>> ---
>>>
>>> v2: https://lore.kernel.org/linux-arm-msm/[email protected]/
>>> 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
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 <[email protected]>
> > > Reviewed-by: Johan Hovold <[email protected]>
> > > Reviewed-by: Douglas Anderson <[email protected]>
> > > Signed-off-by: Andrew Halaney <[email protected]>
> > > ---
> > >
> > > v2: https://lore.kernel.org/linux-arm-msm/[email protected]/
> > > 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 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 <[email protected]>
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 <[email protected]>
> > Reviewed-by: Johan Hovold <[email protected]>
> > Reviewed-by: Douglas Anderson <[email protected]>
> > Signed-off-by: Andrew Halaney <[email protected]>
> > ---
> >
> > v2: https://lore.kernel.org/linux-arm-msm/[email protected]/
> > 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/[email protected]/
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/[email protected]/
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