Message ID | 20220829164952.2672848-1-dianders@chromium.org (mailing list archive) |
---|---|
Headers | show |
Series | arm64: dts: qcom: Fix broken regulator spec on RPMH boards | expand |
On Mon, Aug 29, 2022 at 09:49:46AM -0700, Douglas Anderson wrote: > Prior to commit efb0cb50c427 ("regulator: qcom-rpmh: Implement > get_optimum_mode(), not set_load()") several boards were able to > change their regulator mode even though they had nothing listed in > "regulator-allowed-modes". After that commit (and fixes [1]) we'll be > stuck at the initial mode. Discussion of this (again, see [1]) has > resulted in the decision that the old dts files were wrong and should > be fixed to fully restore old functionality. > > This series attempts to fix everyone. I've kept each board in a > separate patch to make stable / backports work easier. Should you also update the bindings so that this can be caught during devicetree validation? That is, to always require "regulator-allowed-modes" when "regulator-allow-set-load" is specified. Perhaps at least for RPMh as it seemed you found some cases were this wasn't currently needed (even if that sounded like an Linux-specific implementation detail). Johan
Hi, On Tue, Aug 30, 2022 at 11:47 PM Johan Hovold <johan@kernel.org> wrote: > > On Mon, Aug 29, 2022 at 09:49:46AM -0700, Douglas Anderson wrote: > > Prior to commit efb0cb50c427 ("regulator: qcom-rpmh: Implement > > get_optimum_mode(), not set_load()") several boards were able to > > change their regulator mode even though they had nothing listed in > > "regulator-allowed-modes". After that commit (and fixes [1]) we'll be > > stuck at the initial mode. Discussion of this (again, see [1]) has > > resulted in the decision that the old dts files were wrong and should > > be fixed to fully restore old functionality. > > > > This series attempts to fix everyone. I've kept each board in a > > separate patch to make stable / backports work easier. > > Should you also update the bindings so that this can be caught during > devicetree validation? That is, to always require > "regulator-allowed-modes" when "regulator-allow-set-load" is specified. Yeah, it's probably a good idea. I'm happy to review a patch that does that. I'm already quite a few patches deep of submitting random cleanups because someone mentioned it in a code review. ;-) That's actually how I got in this mess to begin with. The RPMH change was in response to a request in a different code review. ...and that came about in a code review that was posted in response to a comment about how awkward setting regulator loads was... Need to get back to my day job. In any case, I think these dts patches are ready to land now. > Perhaps at least for RPMh as it seemed you found some cases were this > wasn't currently needed (even if that sounded like an Linux-specific > implementation detail). I think you're talking about the RPM vs. RPMH difference? It's actually not Linux specific. In RPM the API to the "hardware" (actually a remote processor) is to pass the load. In RPMH the API to the hardware is to pass a mode. This is why RPMH has "regulator-allowed-modes" and "regulator-initial-mode". Both RPM and RPMH have "regulator-allow-set-load" though... -Doug
On Wed, Aug 31, 2022 at 07:52:52AM -0700, Doug Anderson wrote: > Hi, > > On Tue, Aug 30, 2022 at 11:47 PM Johan Hovold <johan@kernel.org> wrote: > > > > On Mon, Aug 29, 2022 at 09:49:46AM -0700, Douglas Anderson wrote: > > > Prior to commit efb0cb50c427 ("regulator: qcom-rpmh: Implement > > > get_optimum_mode(), not set_load()") several boards were able to > > > change their regulator mode even though they had nothing listed in > > > "regulator-allowed-modes". After that commit (and fixes [1]) we'll be > > > stuck at the initial mode. Discussion of this (again, see [1]) has > > > resulted in the decision that the old dts files were wrong and should > > > be fixed to fully restore old functionality. > > > > > > This series attempts to fix everyone. I've kept each board in a > > > separate patch to make stable / backports work easier. > > > > Should you also update the bindings so that this can be caught during > > devicetree validation? That is, to always require > > "regulator-allowed-modes" when "regulator-allow-set-load" is specified. > > Yeah, it's probably a good idea. I'm happy to review a patch that does > that. I'm already quite a few patches deep of submitting random > cleanups because someone mentioned it in a code review. ;-) That's > actually how I got in this mess to begin with. The RPMH change was in > response to a request in a different code review. ...and that came > about in a code review that was posted in response to a comment about > how awkward setting regulator loads was... Need to get back to my day > job. I can take a stab at this during the week here I hope.. I owe Doug for the slew of patches and have wanted to peek at how all the dt-binding validation stuff works anyways. Thanks, Andrew > > In any case, I think these dts patches are ready to land now. > > > > Perhaps at least for RPMh as it seemed you found some cases were this > > wasn't currently needed (even if that sounded like an Linux-specific > > implementation detail). > > I think you're talking about the RPM vs. RPMH difference? It's > actually not Linux specific. In RPM the API to the "hardware" > (actually a remote processor) is to pass the load. In RPMH the API to > the hardware is to pass a mode. This is why RPMH has > "regulator-allowed-modes" and "regulator-initial-mode". Both RPM and > RPMH have "regulator-allow-set-load" though... > > -Doug >
On Wed, Aug 31, 2022 at 07:52:52AM -0700, Doug Anderson wrote: > Hi, > > On Tue, Aug 30, 2022 at 11:47 PM Johan Hovold <johan@kernel.org> wrote: > > > > On Mon, Aug 29, 2022 at 09:49:46AM -0700, Douglas Anderson wrote: > > > Prior to commit efb0cb50c427 ("regulator: qcom-rpmh: Implement > > > get_optimum_mode(), not set_load()") several boards were able to > > > change their regulator mode even though they had nothing listed in > > > "regulator-allowed-modes". After that commit (and fixes [1]) we'll be > > > stuck at the initial mode. Discussion of this (again, see [1]) has > > > resulted in the decision that the old dts files were wrong and should > > > be fixed to fully restore old functionality. > > > > > > This series attempts to fix everyone. I've kept each board in a > > > separate patch to make stable / backports work easier. > > > > Should you also update the bindings so that this can be caught during > > devicetree validation? That is, to always require > > "regulator-allowed-modes" when "regulator-allow-set-load" is specified. > > Yeah, it's probably a good idea. I'm happy to review a patch that does > that. I'm already quite a few patches deep of submitting random > cleanups because someone mentioned it in a code review. ;-) That's > actually how I got in this mess to begin with. The RPMH change was in > response to a request in a different code review. ...and that came > about in a code review that was posted in response to a comment about > how awkward setting regulator loads was... Need to get back to my day > job. Heh. > In any case, I think these dts patches are ready to land now. Yeah, as the old dtbs are now broken with newer kernels these are indeed needed. But regardless of the question of backwards compatibility, it seems that the bindings should at least reflect that the old DTs are now considered malformed. > > Perhaps at least for RPMh as it seemed you found some cases were this > > wasn't currently needed (even if that sounded like an Linux-specific > > implementation detail). > > I think you're talking about the RPM vs. RPMH difference? It's > actually not Linux specific. In RPM the API to the "hardware" > (actually a remote processor) is to pass the load. In RPMH the API to > the hardware is to pass a mode. This is why RPMH has > "regulator-allowed-modes" and "regulator-initial-mode". Both RPM and > RPMH have "regulator-allow-set-load" though... Ah, ok. And this was only an issue for Qualcomm DTs, which are the only users of "regulator-allow-set-load" in mainline. Johan
Hi, On Thu, Sep 1, 2022 at 8:52 AM Johan Hovold <johan@kernel.org> wrote: > > On Wed, Aug 31, 2022 at 07:52:52AM -0700, Doug Anderson wrote: > > Hi, > > > > On Tue, Aug 30, 2022 at 11:47 PM Johan Hovold <johan@kernel.org> wrote: > > > > > > On Mon, Aug 29, 2022 at 09:49:46AM -0700, Douglas Anderson wrote: > > > > Prior to commit efb0cb50c427 ("regulator: qcom-rpmh: Implement > > > > get_optimum_mode(), not set_load()") several boards were able to > > > > change their regulator mode even though they had nothing listed in > > > > "regulator-allowed-modes". After that commit (and fixes [1]) we'll be > > > > stuck at the initial mode. Discussion of this (again, see [1]) has > > > > resulted in the decision that the old dts files were wrong and should > > > > be fixed to fully restore old functionality. > > > > > > > > This series attempts to fix everyone. I've kept each board in a > > > > separate patch to make stable / backports work easier. > > > > > > Should you also update the bindings so that this can be caught during > > > devicetree validation? That is, to always require > > > "regulator-allowed-modes" when "regulator-allow-set-load" is specified. > > > > Yeah, it's probably a good idea. I'm happy to review a patch that does > > that. I'm already quite a few patches deep of submitting random > > cleanups because someone mentioned it in a code review. ;-) That's > > actually how I got in this mess to begin with. The RPMH change was in > > response to a request in a different code review. ...and that came > > about in a code review that was posted in response to a comment about > > how awkward setting regulator loads was... Need to get back to my day > > job. > > Heh. > > > In any case, I think these dts patches are ready to land now. > > Yeah, as the old dtbs are now broken with newer kernels these are indeed > needed. With the latest patches in the regulator tree things shouldn't be _too_ broken even without the dts files. Essentially things will get stuck at their initial mode (HPM). So without these patches things should all still boot but could possibly end up at a higher power state. -Doug
On Thu, Sep 01, 2022 at 05:44:03PM -0700, Doug Anderson wrote: > Hi, > > On Thu, Sep 1, 2022 at 8:52 AM Johan Hovold <johan@kernel.org> wrote: > > > > On Wed, Aug 31, 2022 at 07:52:52AM -0700, Doug Anderson wrote: > > > Hi, > > > > > > On Tue, Aug 30, 2022 at 11:47 PM Johan Hovold <johan@kernel.org> wrote: > > > > > > > > On Mon, Aug 29, 2022 at 09:49:46AM -0700, Douglas Anderson wrote: > > > > > Prior to commit efb0cb50c427 ("regulator: qcom-rpmh: Implement > > > > > get_optimum_mode(), not set_load()") several boards were able to > > > > > change their regulator mode even though they had nothing listed in > > > > > "regulator-allowed-modes". After that commit (and fixes [1]) we'll be > > > > > stuck at the initial mode. Discussion of this (again, see [1]) has > > > > > resulted in the decision that the old dts files were wrong and should > > > > > be fixed to fully restore old functionality. > > > > > > > > > > This series attempts to fix everyone. I've kept each board in a > > > > > separate patch to make stable / backports work easier. > > > > > > > > Should you also update the bindings so that this can be caught during > > > > devicetree validation? That is, to always require > > > > "regulator-allowed-modes" when "regulator-allow-set-load" is specified. > > > > > > Yeah, it's probably a good idea. I'm happy to review a patch that does > > > that. I'm already quite a few patches deep of submitting random > > > cleanups because someone mentioned it in a code review. ;-) That's > > > actually how I got in this mess to begin with. The RPMH change was in > > > response to a request in a different code review. ...and that came > > > about in a code review that was posted in response to a comment about > > > how awkward setting regulator loads was... Need to get back to my day > > > job. > > > > Heh. > > > > > In any case, I think these dts patches are ready to land now. > > > > Yeah, as the old dtbs are now broken with newer kernels these are indeed > > needed. > > With the latest patches in the regulator tree things shouldn't be > _too_ broken even without the dts files. Essentially things will get > stuck at their initial mode (HPM). So without these patches things > should all still boot but could possibly end up at a higher power > state. Ok, and there's also a warning during boot when that happens so that it's not a silent regression? Johan
On Wed, Aug 31, 2022 at 02:00:18PM -0500, Andrew Halaney wrote: > On Wed, Aug 31, 2022 at 07:52:52AM -0700, Doug Anderson wrote: > > Hi, > > > > On Tue, Aug 30, 2022 at 11:47 PM Johan Hovold <johan@kernel.org> wrote: > > > > > > On Mon, Aug 29, 2022 at 09:49:46AM -0700, Douglas Anderson wrote: > > > > Prior to commit efb0cb50c427 ("regulator: qcom-rpmh: Implement > > > > get_optimum_mode(), not set_load()") several boards were able to > > > > change their regulator mode even though they had nothing listed in > > > > "regulator-allowed-modes". After that commit (and fixes [1]) we'll be > > > > stuck at the initial mode. Discussion of this (again, see [1]) has > > > > resulted in the decision that the old dts files were wrong and should > > > > be fixed to fully restore old functionality. > > > > > > > > This series attempts to fix everyone. I've kept each board in a > > > > separate patch to make stable / backports work easier. > > > > > > Should you also update the bindings so that this can be caught during > > > devicetree validation? That is, to always require > > > "regulator-allowed-modes" when "regulator-allow-set-load" is specified. > > > > Yeah, it's probably a good idea. I'm happy to review a patch that does > > that. I'm already quite a few patches deep of submitting random > > cleanups because someone mentioned it in a code review. ;-) That's > > actually how I got in this mess to begin with. The RPMH change was in > > response to a request in a different code review. ...and that came > > about in a code review that was posted in response to a comment about > > how awkward setting regulator loads was... Need to get back to my day > > job. > > I can take a stab at this during the week here I hope.. I owe Doug for > the slew of patches and have wanted to peek at how all the dt-binding > validation stuff works anyways. > Here's my attempt after a couple hours of banging the head on the wall: https://lore.kernel.org/all/20220902185148.635292-1-ahalaney@redhat.com/ Thanks, Andrew
Bjorn, On Mon, Aug 29, 2022 at 9:50 AM Douglas Anderson <dianders@chromium.org> wrote: > > Prior to commit efb0cb50c427 ("regulator: qcom-rpmh: Implement > get_optimum_mode(), not set_load()") several boards were able to > change their regulator mode even though they had nothing listed in > "regulator-allowed-modes". After that commit (and fixes [1]) we'll be > stuck at the initial mode. Discussion of this (again, see [1]) has > resulted in the decision that the old dts files were wrong and should > be fixed to fully restore old functionality. > > This series attempts to fix everyone. I've kept each board in a > separate patch to make stable / backports work easier. > > Affected boards were found with: > rpmh_users=$(git grep -l -i rpmh -- arch/arm*/boot/dts/qcom) > set_modes=$(grep -l regulator-allow-set-load ${rpmh_users}) > but_no_allowed_modes=$(grep -l -v regulator-allowed-modes ${set_modes}) > > Fix was applied with: > for f in ${but_no_allowed_modes}; do > sed -i~ -e \ > 's/^\(\s*\)regulator-allow-set-load;/\1regulator-allow-set-load;\n\1regulator-allowed-modes =\n\1 <RPMH_REGULATOR_MODE_LPM\n\1 RPMH_REGULATOR_MODE_HPM>;/'\ > $f > done > > Then results were manually inspected. In one board I removed a > "regulator-allow-set-load" from a fixed regulator since that was > clearly wrong. > > v2 of this series adds tags and also rebases atop Johan's series [2] > as requested [3]. This ends up turning the series into a 6-part series > instead of a 7-part one. > > It should also be noted that as of the v2 posting that the related > regulator fixes have all landed in the regulator tree. > > [1] https://lore.kernel.org/r/20220824142229.RFT.v2.2.I6f77860e5cd98bf5c67208fa9edda4a08847c304@changeid > [2] https://lore.kernel.org/r/20220803121942.30236-1-johan+linaro@kernel.org/ > [3] https://lore.kernel.org/r/YwhwIX+Ib8epUYWS@hovoldconsulting.com/ > > Changes in v2: > - Added note about removing regulator-allow-set-load from vreg_s4a_1p8 > - Rebased atop ("...: sa8295p-adp: disallow regulator mode switches") > - Rebased atop ("...: sc8280xp-crd: disallow regulator mode switches") > > Douglas Anderson (6): > arm64: dts: qcom: sa8155p-adp: Specify which LDO modes are allowed > arm64: dts: qcom: sa8295p-adp: Specify which LDO modes are allowed > arm64: dts: qcom: sc8280xp-crd: Specify which LDO modes are allowed > arm64: dts: qcom: sm8150-xperia-kumano: Specify which LDO modes are > allowed > arm64: dts: qcom: sm8250-xperia-edo: Specify which LDO modes are > allowed > arm64: dts: qcom: sm8350-hdk: Specify which LDO modes are allowed > > arch/arm64/boot/dts/qcom/sa8155p-adp.dts | 13 ++++++++++++- > arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 12 ++++++++++++ > arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 6 ++++++ > .../boot/dts/qcom/sm8150-sony-xperia-kumano.dtsi | 6 ++++++ > .../arm64/boot/dts/qcom/sm8250-sony-xperia-edo.dtsi | 6 ++++++ > arch/arm64/boot/dts/qcom/sm8350-hdk.dts | 12 ++++++++++++ > 6 files changed, 54 insertions(+), 1 deletion(-) I think this series is ready to land if it's a good time now. It looks like you've already applied the series that this depends on [1] and this one is all reviewed and ready to go. Thanks! [1] https://lore.kernel.org/all/166181675980.322065.3918715363441736917.b4-ty@kernel.org/
Bjorn, On Thu, Sep 8, 2022 at 9:04 AM Doug Anderson <dianders@chromium.org> wrote: > > Bjorn, > > On Mon, Aug 29, 2022 at 9:50 AM Douglas Anderson <dianders@chromium.org> wrote: > > > > Prior to commit efb0cb50c427 ("regulator: qcom-rpmh: Implement > > get_optimum_mode(), not set_load()") several boards were able to > > change their regulator mode even though they had nothing listed in > > "regulator-allowed-modes". After that commit (and fixes [1]) we'll be > > stuck at the initial mode. Discussion of this (again, see [1]) has > > resulted in the decision that the old dts files were wrong and should > > be fixed to fully restore old functionality. > > > > This series attempts to fix everyone. I've kept each board in a > > separate patch to make stable / backports work easier. > > > > Affected boards were found with: > > rpmh_users=$(git grep -l -i rpmh -- arch/arm*/boot/dts/qcom) > > set_modes=$(grep -l regulator-allow-set-load ${rpmh_users}) > > but_no_allowed_modes=$(grep -l -v regulator-allowed-modes ${set_modes}) > > > > Fix was applied with: > > for f in ${but_no_allowed_modes}; do > > sed -i~ -e \ > > 's/^\(\s*\)regulator-allow-set-load;/\1regulator-allow-set-load;\n\1regulator-allowed-modes =\n\1 <RPMH_REGULATOR_MODE_LPM\n\1 RPMH_REGULATOR_MODE_HPM>;/'\ > > $f > > done > > > > Then results were manually inspected. In one board I removed a > > "regulator-allow-set-load" from a fixed regulator since that was > > clearly wrong. > > > > v2 of this series adds tags and also rebases atop Johan's series [2] > > as requested [3]. This ends up turning the series into a 6-part series > > instead of a 7-part one. > > > > It should also be noted that as of the v2 posting that the related > > regulator fixes have all landed in the regulator tree. > > > > [1] https://lore.kernel.org/r/20220824142229.RFT.v2.2.I6f77860e5cd98bf5c67208fa9edda4a08847c304@changeid > > [2] https://lore.kernel.org/r/20220803121942.30236-1-johan+linaro@kernel.org/ > > [3] https://lore.kernel.org/r/YwhwIX+Ib8epUYWS@hovoldconsulting.com/ > > > > Changes in v2: > > - Added note about removing regulator-allow-set-load from vreg_s4a_1p8 > > - Rebased atop ("...: sa8295p-adp: disallow regulator mode switches") > > - Rebased atop ("...: sc8280xp-crd: disallow regulator mode switches") > > > > Douglas Anderson (6): > > arm64: dts: qcom: sa8155p-adp: Specify which LDO modes are allowed > > arm64: dts: qcom: sa8295p-adp: Specify which LDO modes are allowed > > arm64: dts: qcom: sc8280xp-crd: Specify which LDO modes are allowed > > arm64: dts: qcom: sm8150-xperia-kumano: Specify which LDO modes are > > allowed > > arm64: dts: qcom: sm8250-xperia-edo: Specify which LDO modes are > > allowed > > arm64: dts: qcom: sm8350-hdk: Specify which LDO modes are allowed > > > > arch/arm64/boot/dts/qcom/sa8155p-adp.dts | 13 ++++++++++++- > > arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 12 ++++++++++++ > > arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 6 ++++++ > > .../boot/dts/qcom/sm8150-sony-xperia-kumano.dtsi | 6 ++++++ > > .../arm64/boot/dts/qcom/sm8250-sony-xperia-edo.dtsi | 6 ++++++ > > arch/arm64/boot/dts/qcom/sm8350-hdk.dts | 12 ++++++++++++ > > 6 files changed, 54 insertions(+), 1 deletion(-) > > I think this series is ready to land if it's a good time now. It looks > like you've already applied the series that this depends on [1] and > this one is all reviewed and ready to go. Thanks! > > [1] https://lore.kernel.org/all/166181675980.322065.3918715363441736917.b4-ty@kernel.org/ I saw you sent out a pull request, but it didn't seem to include these patches. Were they missing anything? Maybe you're still planning on a "Fixes" pull request? -Doug
On Mon, 29 Aug 2022 09:49:46 -0700, Douglas Anderson wrote: > Prior to commit efb0cb50c427 ("regulator: qcom-rpmh: Implement > get_optimum_mode(), not set_load()") several boards were able to > change their regulator mode even though they had nothing listed in > "regulator-allowed-modes". After that commit (and fixes [1]) we'll be > stuck at the initial mode. Discussion of this (again, see [1]) has > resulted in the decision that the old dts files were wrong and should > be fixed to fully restore old functionality. > > [...] Applied, thanks! [1/6] arm64: dts: qcom: sa8155p-adp: Specify which LDO modes are allowed commit: bd9f3dcf42d943b53190f99bcdbcfe98a56ac4cd [2/6] arm64: dts: qcom: sa8295p-adp: Specify which LDO modes are allowed commit: 09a1710b3e12e7ac8d871506bc395a9e8a0f63d6 [3/6] arm64: dts: qcom: sc8280xp-crd: Specify which LDO modes are allowed commit: a4543e21ae363f4f094fb3c3503d77029e0c5d90 [4/6] arm64: dts: qcom: sm8150-xperia-kumano: Specify which LDO modes are allowed commit: aa30e786202e4ed1df980442d305658441f65859 [5/6] arm64: dts: qcom: sm8250-xperia-edo: Specify which LDO modes are allowed commit: b7870d460c05ce31e2311036d91de1e2e0b32cea [6/6] arm64: dts: qcom: sm8350-hdk: Specify which LDO modes are allowed commit: 1ce8aaf6abdc35cde555924418b3d4516b4ec871 Best regards,