diff mbox series

[v3,4/4] arm64: dts: qcom: sc8280xp-x13s: keep less regulators always-on

Message ID 20240905122023.47251-5-brgl@bgdev.pl (mailing list archive)
State Superseded
Headers show
Series arm64: dts: qcom: sc8280xp: enable WLAN and Bluetooth | expand

Commit Message

Bartosz Golaszewski Sept. 5, 2024, 12:20 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Remove the regulator-always-on property from the ones that used to be
implicitly needed by the on-board WCN6855 now that its PMU is modeled in
device-tree.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 4 ----
 1 file changed, 4 deletions(-)

Comments

Johan Hovold Sept. 5, 2024, 2:04 p.m. UTC | #1 
On Thu, Sep 05, 2024 at 02:20:22PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Remove the regulator-always-on property from the ones that used to be
> implicitly needed by the on-board WCN6855 now that its PMU is modeled in
> device-tree.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> index 88b31550f9df..1a9dac16c952 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> @@ -479,7 +479,6 @@ vreg_s10b: smps10 {
>  			regulator-min-microvolt = <1800000>;
>  			regulator-max-microvolt = <1800000>;
>  			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> -			regulator-always-on;
>  		};

What makes you think s10b is only used by wcn6855?

You clearly did not check the schematics so make sure you verify the
rest as well before resending.

And if any of these are valid, I think this should be part of the
previous patch.

Johan
Bartosz Golaszewski Sept. 5, 2024, 6:23 p.m. UTC | #2 
On Thu, Sep 5, 2024 at 4:04 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, Sep 05, 2024 at 02:20:22PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Remove the regulator-always-on property from the ones that used to be
> > implicitly needed by the on-board WCN6855 now that its PMU is modeled in
> > device-tree.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> > index 88b31550f9df..1a9dac16c952 100644
> > --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> > @@ -479,7 +479,6 @@ vreg_s10b: smps10 {
> >                       regulator-min-microvolt = <1800000>;
> >                       regulator-max-microvolt = <1800000>;
> >                       regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> > -                     regulator-always-on;
> >               };
>
> What makes you think s10b is only used by wcn6855?
>

I didn't say that. It's used by many components but they seem to be
all described in DT. But I get it, the schematics show it in so many
places, it would be risky to not keep it on.

> You clearly did not check the schematics so make sure you verify the
> rest as well before resending.
>
> And if any of these are valid, I think this should be part of the
> previous patch.
>

At least vreg_s11b and vreg_s12b should be fine. I'm not sure when
I'll respin the series though, we need to first figure out whether to
upstream the calibration variant property and what its name should be.

Bart
Dmitry Baryshkov Sept. 5, 2024, 7:27 p.m. UTC | #3 
On Thu, Sep 05, 2024 at 08:23:39PM GMT, Bartosz Golaszewski wrote:
> On Thu, Sep 5, 2024 at 4:04 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Sep 05, 2024 at 02:20:22PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Remove the regulator-always-on property from the ones that used to be
> > > implicitly needed by the on-board WCN6855 now that its PMU is modeled in
> > > device-tree.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 4 ----
> > >  1 file changed, 4 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> > > index 88b31550f9df..1a9dac16c952 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> > > +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> > > @@ -479,7 +479,6 @@ vreg_s10b: smps10 {
> > >                       regulator-min-microvolt = <1800000>;
> > >                       regulator-max-microvolt = <1800000>;
> > >                       regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> > > -                     regulator-always-on;
> > >               };
> >
> > What makes you think s10b is only used by wcn6855?
> >
> 
> I didn't say that. It's used by many components but they seem to be
> all described in DT. But I get it, the schematics show it in so many
> places, it would be risky to not keep it on.

Well, that depends on the consumers. If all consumers are good and
voting, then it should be safe.

> > You clearly did not check the schematics so make sure you verify the
> > rest as well before resending.
> >
> > And if any of these are valid, I think this should be part of the
> > previous patch.
> >
> 
> At least vreg_s11b and vreg_s12b should be fine. I'm not sure when
> I'll respin the series though, we need to first figure out whether to
> upstream the calibration variant property and what its name should be.
Johan Hovold Sept. 6, 2024, 2:49 p.m. UTC | #4 
On Thu, Sep 05, 2024 at 10:27:24PM +0300, Dmitry Baryshkov wrote:
> On Thu, Sep 05, 2024 at 08:23:39PM GMT, Bartosz Golaszewski wrote:
> > On Thu, Sep 5, 2024 at 4:04 PM Johan Hovold <johan@kernel.org> wrote:
> > > On Thu, Sep 05, 2024 at 02:20:22PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > Remove the regulator-always-on property from the ones that used to be
> > > > implicitly needed by the on-board WCN6855 now that its PMU is modeled in
> > > > device-tree.

> > > What makes you think s10b is only used by wcn6855?
> > 
> > I didn't say that. It's used by many components but they seem to be
> > all described in DT. But I get it, the schematics show it in so many
> > places, it would be risky to not keep it on.
> 
> Well, that depends on the consumers. If all consumers are good and
> voting, then it should be safe.

Right. But in this case, not all consumers are described in DT (e.g.
ddr) and this is effectively an always-on rail.

Johan
Dmitry Baryshkov Sept. 6, 2024, 9:37 p.m. UTC | #5 
On Fri, Sep 06, 2024 at 04:49:31PM GMT, Johan Hovold wrote:
> On Thu, Sep 05, 2024 at 10:27:24PM +0300, Dmitry Baryshkov wrote:
> > On Thu, Sep 05, 2024 at 08:23:39PM GMT, Bartosz Golaszewski wrote:
> > > On Thu, Sep 5, 2024 at 4:04 PM Johan Hovold <johan@kernel.org> wrote:
> > > > On Thu, Sep 05, 2024 at 02:20:22PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > >
> > > > > Remove the regulator-always-on property from the ones that used to be
> > > > > implicitly needed by the on-board WCN6855 now that its PMU is modeled in
> > > > > device-tree.
> 
> > > > What makes you think s10b is only used by wcn6855?
> > > 
> > > I didn't say that. It's used by many components but they seem to be
> > > all described in DT. But I get it, the schematics show it in so many
> > > places, it would be risky to not keep it on.
> > 
> > Well, that depends on the consumers. If all consumers are good and
> > voting, then it should be safe.
> 
> Right. But in this case, not all consumers are described in DT (e.g.
> ddr) and this is effectively an always-on rail.

Ack.
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
index 88b31550f9df..1a9dac16c952 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -479,7 +479,6 @@  vreg_s10b: smps10 {
 			regulator-min-microvolt = <1800000>;
 			regulator-max-microvolt = <1800000>;
 			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
-			regulator-always-on;
 		};
 
 		vreg_s11b: smps11 {
@@ -487,7 +486,6 @@  vreg_s11b: smps11 {
 			regulator-min-microvolt = <1272000>;
 			regulator-max-microvolt = <1272000>;
 			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
-			regulator-always-on;
 		};
 
 		vreg_s12b: smps12 {
@@ -495,7 +493,6 @@  vreg_s12b: smps12 {
 			regulator-min-microvolt = <984000>;
 			regulator-max-microvolt = <984000>;
 			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
-			regulator-always-on;
 		};
 
 		vreg_l1b: ldo1 {
@@ -545,7 +542,6 @@  vreg_s1c: smps1 {
 			regulator-min-microvolt = <1880000>;
 			regulator-max-microvolt = <1900000>;
 			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
-			regulator-always-on;
 		};
 
 		vreg_l1c: ldo1 {