Message ID | 20220829164952.2672848-1-dianders@chromium.org |
---|---|
Headers | show |
Series | arm64: dts: qcom: Fix broken regulator spec on RPMH boards | expand |
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
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