* [PATCH] arm64: dts: qcom: sc7180-trogdor: Make pp3300_a the default supply for pp3300_hub
@ 2020-11-03 18:38 Matthias Kaehlcke
2020-11-05 0:29 ` Doug Anderson
0 siblings, 1 reply; 5+ messages in thread
From: Matthias Kaehlcke @ 2020-11-03 18:38 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Rob Herring
Cc: linux-arm-msm, devicetree, Douglas Anderson, linux-kernel,
Matthias Kaehlcke
The trogdor design has two options for supplying the pp3300_hub power rail,
it can be supplied by pp3300_l7c or pp3300_a. Initially pp3300_l7c was
used, newer revisions (will) use pp3300_a as supply.
Add a DT node for the pp3300_a path which includes a power switch that is
controlled by a GPIO. Make this path the default and keep trogdor rev1,
lazor rev0 and rev1 on pp3300_l7c.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
.../boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 4 +++
.../boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 4 +++
.../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts | 4 +++
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 30 ++++++++++++++++++-
4 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
index ae4c23a4fe65..08bf30b761fc 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
@@ -22,3 +22,7 @@ &sn65dsi86_out {
*/
lane-polarities = <1 0>;
};
+
+&usb_hub {
+ vdd-supply = <&pp3300_hub_7c>;
+};
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
index 3151ae31c1cc..9f7a44d78a1a 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
@@ -13,3 +13,7 @@ / {
model = "Google Lazor (rev1+)";
compatible = "google,lazor", "qcom,sc7180";
};
+
+&usb_hub {
+ vdd-supply = <&pp3300_hub_7c>;
+};
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
index 0a281c24841c..e1840fe07cd0 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
@@ -62,6 +62,10 @@ &sdhc_2 {
status = "okay";
};
+&usb_hub {
+ vdd-supply = <&pp3300_hub_7c>;
+};
+
/* PINCTRL - board-specific pinctrl */
&tlmm {
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index bf875589d364..2d64e75a2d6d 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -174,6 +174,21 @@ pp3300_fp_tp: pp3300-fp-tp-regulator {
vin-supply = <&pp3300_a>;
};
+ pp3300_hub: pp3300-hub {
+ compatible = "regulator-fixed";
+ regulator-name = "pp3300_hub";
+
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+
+ gpio = <&tlmm 84 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ pinctrl-names = "default";
+ pinctrl-0 = <&en_pp3300_hub>;
+
+ vin-supply = <&pp3300_a>;
+ };
+
/* BOARD-SPECIFIC TOP LEVEL NODES */
backlight: backlight {
@@ -469,7 +484,7 @@ ppvar_l6c: ldo6 {
regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
};
- pp3300_hub:
+ pp3300_hub_7c:
pp3300_l7c: ldo7 {
regulator-min-microvolt = <3304000>;
regulator-max-microvolt = <3304000>;
@@ -1151,6 +1166,19 @@ pinconf {
};
};
+ en_pp3300_hub: en-pp3300-hub {
+ pinmux {
+ pins = "gpio84";
+ function = "gpio";
+ };
+
+ pinconf {
+ pins = "gpio84";
+ drive-strength = <2>;
+ bias-disable;
+ };
+ };
+
en_pp3300_dx_edp: en-pp3300-dx-edp {
pinmux {
pins = "gpio30";
--
2.29.1.341.ge80a0c044ae-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] arm64: dts: qcom: sc7180-trogdor: Make pp3300_a the default supply for pp3300_hub
2020-11-03 18:38 [PATCH] arm64: dts: qcom: sc7180-trogdor: Make pp3300_a the default supply for pp3300_hub Matthias Kaehlcke
@ 2020-11-05 0:29 ` Doug Anderson
2020-11-05 1:55 ` Matthias Kaehlcke
0 siblings, 1 reply; 5+ messages in thread
From: Doug Anderson @ 2020-11-05 0:29 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Andy Gross, Bjorn Andersson, Rob Herring, linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML
Hi,
On Tue, Nov 3, 2020 at 10:38 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> The trogdor design has two options for supplying the pp3300_hub power rail,
> it can be supplied by pp3300_l7c or pp3300_a. Initially pp3300_l7c was
> used, newer revisions (will) use pp3300_a as supply.
>
> Add a DT node for the pp3300_a path which includes a power switch that is
> controlled by a GPIO. Make this path the default and keep trogdor rev1,
> lazor rev0 and rev1 on pp3300_l7c.
It might not hurt to mention that even on early hardware that GPIO84
was allocated to this purpose but that it was a stuff option for what
actually provided power to the hub. This explains why it's OK to add
the fixed regulator (just with no clients) even on old hardware. If
GPIO84 had been used for something else on old hardware this would
have been bad.
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index bf875589d364..2d64e75a2d6d 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -174,6 +174,21 @@ pp3300_fp_tp: pp3300-fp-tp-regulator {
> vin-supply = <&pp3300_a>;
> };
>
> + pp3300_hub: pp3300-hub {
> + compatible = "regulator-fixed";
> + regulator-name = "pp3300_hub";
> +
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> +
> + gpio = <&tlmm 84 GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> + pinctrl-names = "default";
> + pinctrl-0 = <&en_pp3300_hub>;
> +
> + vin-supply = <&pp3300_a>;
You're leaving things in a bit of an inconsistent state here. The
"pp3300_hub_7c" is always_on / boot_on. This new one isn't. I know
this is slightly more complicated due to the fact that downstream we
have a way to control the hub power but didn't quite get that resolved
upstream, but the way you have it now, on new hardware upstream will
power off the hub but also keep "pp3300_hub_7c" powered on for no
reason. Seems like that should be fixed?
> + };
> +
> /* BOARD-SPECIFIC TOP LEVEL NODES */
>
> backlight: backlight {
> @@ -469,7 +484,7 @@ ppvar_l6c: ldo6 {
> regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> };
>
> - pp3300_hub:
> + pp3300_hub_7c:
nit: If it were me, I probably wouldn't have bothered introducing the
"pp3300_hub_7c" alias since it's not a real thing in the schematic. I
would have just had the older revisions refer to "pp3300_l7c". If you
really love the "pp3300_hub_7c", though, I won't stand in your way.
> pp3300_l7c: ldo7 {
> regulator-min-microvolt = <3304000>;
> regulator-max-microvolt = <3304000>;
> @@ -1151,6 +1166,19 @@ pinconf {
> };
> };
>
> + en_pp3300_hub: en-pp3300-hub {
> + pinmux {
> + pins = "gpio84";
> + function = "gpio";
> + };
> +
> + pinconf {
> + pins = "gpio84";
> + drive-strength = <2>;
> + bias-disable;
> + };
> + };
> +
> en_pp3300_dx_edp: en-pp3300-dx-edp {
"hub" sorts after "dx", so the ordering is slightly wrong here.
-Doug
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] arm64: dts: qcom: sc7180-trogdor: Make pp3300_a the default supply for pp3300_hub
2020-11-05 0:29 ` Doug Anderson
@ 2020-11-05 1:55 ` Matthias Kaehlcke
2020-11-05 15:57 ` Doug Anderson
0 siblings, 1 reply; 5+ messages in thread
From: Matthias Kaehlcke @ 2020-11-05 1:55 UTC (permalink / raw)
To: Doug Anderson
Cc: Andy Gross, Bjorn Andersson, Rob Herring, linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML
Hi Doug,
On Wed, Nov 04, 2020 at 04:29:50PM -0800, Doug Anderson wrote:
> Hi,
>
> On Tue, Nov 3, 2020 at 10:38 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > The trogdor design has two options for supplying the pp3300_hub power rail,
> > it can be supplied by pp3300_l7c or pp3300_a. Initially pp3300_l7c was
> > used, newer revisions (will) use pp3300_a as supply.
> >
> > Add a DT node for the pp3300_a path which includes a power switch that is
> > controlled by a GPIO. Make this path the default and keep trogdor rev1,
> > lazor rev0 and rev1 on pp3300_l7c.
>
> It might not hurt to mention that even on early hardware that GPIO84
> was allocated to this purpose but that it was a stuff option for what
> actually provided power to the hub. This explains why it's OK to add
> the fixed regulator (just with no clients) even on old hardware. If
> GPIO84 had been used for something else on old hardware this would
> have been bad.
ok
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > index bf875589d364..2d64e75a2d6d 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > @@ -174,6 +174,21 @@ pp3300_fp_tp: pp3300-fp-tp-regulator {
> > vin-supply = <&pp3300_a>;
> > };
> >
> > + pp3300_hub: pp3300-hub {
> > + compatible = "regulator-fixed";
> > + regulator-name = "pp3300_hub";
> > +
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > +
> > + gpio = <&tlmm 84 GPIO_ACTIVE_HIGH>;
> > + enable-active-high;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&en_pp3300_hub>;
> > +
> > + vin-supply = <&pp3300_a>;
>
> You're leaving things in a bit of an inconsistent state here. The
> "pp3300_hub_7c" is always_on / boot_on. This new one isn't.
Actually the new "pp3300_hub" it is also on at boot, the Chrome OS bootloader
asserts the GPIO.
> I know this is slightly more complicated due to the fact that downstream we
> have a way to control the hub power but didn't quite get that resolved
> upstream, but the way you have it now, on new hardware upstream will
> power off the hub but also keep "pp3300_hub_7c" powered on for no
> reason. Seems like that should be fixed?
Our EEs told me that it would be ok in terms of power to keep "pp3300_hub_7c"
powered, since there would be no significant power consumption without load.
In any case unused RPMH regulators are switched off by the kernel ~30s after
boot, so I think we are ok:
[ 31.202219] ldo7: disabling
The above is from the l7c regulator on a Lazor rev2.
> > + };
> > +
> > /* BOARD-SPECIFIC TOP LEVEL NODES */
> >
> > backlight: backlight {
> > @@ -469,7 +484,7 @@ ppvar_l6c: ldo6 {
> > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> > };
> >
> > - pp3300_hub:
> > + pp3300_hub_7c:
>
> nit: If it were me, I probably wouldn't have bothered introducing the
> "pp3300_hub_7c" alias since it's not a real thing in the schematic. I
> would have just had the older revisions refer to "pp3300_l7c". If you
> really love the "pp3300_hub_7c", though, I won't stand in your way.
true, it's not really needed, I'll get rid of it in the next version.
> > pp3300_l7c: ldo7 {
> > regulator-min-microvolt = <3304000>;
> > regulator-max-microvolt = <3304000>;
> > @@ -1151,6 +1166,19 @@ pinconf {
> > };
> > };
> >
> > + en_pp3300_hub: en-pp3300-hub {
> > + pinmux {
> > + pins = "gpio84";
> > + function = "gpio";
> > + };
> > +
> > + pinconf {
> > + pins = "gpio84";
> > + drive-strength = <2>;
> > + bias-disable;
> > + };
> > + };
> > +
> > en_pp3300_dx_edp: en-pp3300-dx-edp {
>
> "hub" sorts after "dx", so the ordering is slightly wrong here.
ack, will change
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] arm64: dts: qcom: sc7180-trogdor: Make pp3300_a the default supply for pp3300_hub
2020-11-05 1:55 ` Matthias Kaehlcke
@ 2020-11-05 15:57 ` Doug Anderson
2020-11-05 18:28 ` Matthias Kaehlcke
0 siblings, 1 reply; 5+ messages in thread
From: Doug Anderson @ 2020-11-05 15:57 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Andy Gross, Bjorn Andersson, Rob Herring, linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML
Hi,
On Wed, Nov 4, 2020 at 5:55 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > > index bf875589d364..2d64e75a2d6d 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > > @@ -174,6 +174,21 @@ pp3300_fp_tp: pp3300-fp-tp-regulator {
> > > vin-supply = <&pp3300_a>;
> > > };
> > >
> > > + pp3300_hub: pp3300-hub {
> > > + compatible = "regulator-fixed";
> > > + regulator-name = "pp3300_hub";
> > > +
> > > + regulator-min-microvolt = <3300000>;
> > > + regulator-max-microvolt = <3300000>;
> > > +
> > > + gpio = <&tlmm 84 GPIO_ACTIVE_HIGH>;
> > > + enable-active-high;
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&en_pp3300_hub>;
> > > +
> > > + vin-supply = <&pp3300_a>;
> >
> > You're leaving things in a bit of an inconsistent state here. The
> > "pp3300_hub_7c" is always_on / boot_on. This new one isn't.
>
> Actually the new "pp3300_hub" it is also on at boot, the Chrome OS bootloader
> asserts the GPIO.
>
> > I know this is slightly more complicated due to the fact that downstream we
> > have a way to control the hub power but didn't quite get that resolved
> > upstream, but the way you have it now, on new hardware upstream will
> > power off the hub but also keep "pp3300_hub_7c" powered on for no
> > reason. Seems like that should be fixed?
>
> Our EEs told me that it would be ok in terms of power to keep "pp3300_hub_7c"
> powered, since there would be no significant power consumption without load.
>
> In any case unused RPMH regulators are switched off by the kernel ~30s after
> boot, so I think we are ok:
>
> [ 31.202219] ldo7: disabling
>
> The above is from the l7c regulator on a Lazor rev2.
I assume this is with the downstream codebase, though? With what you
have posted upstream I don't think ldo7 will ever get disabled because
it's marked "always-on"?
Similarly, with what you've posted upstream I think your new
"pp3300_hub" _will_ get disabled ~30 seconds after boot because it's
not marked "always-on" and it has no clients.
-Doug
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] arm64: dts: qcom: sc7180-trogdor: Make pp3300_a the default supply for pp3300_hub
2020-11-05 15:57 ` Doug Anderson
@ 2020-11-05 18:28 ` Matthias Kaehlcke
0 siblings, 0 replies; 5+ messages in thread
From: Matthias Kaehlcke @ 2020-11-05 18:28 UTC (permalink / raw)
To: Doug Anderson
Cc: Andy Gross, Bjorn Andersson, Rob Herring, linux-arm-msm,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML
On Thu, Nov 05, 2020 at 07:57:42AM -0800, Doug Anderson wrote:
> Hi,
>
> On Wed, Nov 4, 2020 at 5:55 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > > > index bf875589d364..2d64e75a2d6d 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > > > @@ -174,6 +174,21 @@ pp3300_fp_tp: pp3300-fp-tp-regulator {
> > > > vin-supply = <&pp3300_a>;
> > > > };
> > > >
> > > > + pp3300_hub: pp3300-hub {
> > > > + compatible = "regulator-fixed";
> > > > + regulator-name = "pp3300_hub";
> > > > +
> > > > + regulator-min-microvolt = <3300000>;
> > > > + regulator-max-microvolt = <3300000>;
> > > > +
> > > > + gpio = <&tlmm 84 GPIO_ACTIVE_HIGH>;
> > > > + enable-active-high;
> > > > + pinctrl-names = "default";
> > > > + pinctrl-0 = <&en_pp3300_hub>;
> > > > +
> > > > + vin-supply = <&pp3300_a>;
> > >
> > > You're leaving things in a bit of an inconsistent state here. The
> > > "pp3300_hub_7c" is always_on / boot_on. This new one isn't.
> >
> > Actually the new "pp3300_hub" it is also on at boot, the Chrome OS bootloader
> > asserts the GPIO.
> >
> > > I know this is slightly more complicated due to the fact that downstream we
> > > have a way to control the hub power but didn't quite get that resolved
> > > upstream, but the way you have it now, on new hardware upstream will
> > > power off the hub but also keep "pp3300_hub_7c" powered on for no
> > > reason. Seems like that should be fixed?
> >
> > Our EEs told me that it would be ok in terms of power to keep "pp3300_hub_7c"
> > powered, since there would be no significant power consumption without load.
> >
> > In any case unused RPMH regulators are switched off by the kernel ~30s after
> > boot, so I think we are ok:
> >
> > [ 31.202219] ldo7: disabling
> >
> > The above is from the l7c regulator on a Lazor rev2.
>
> I assume this is with the downstream codebase, though? With what you
> have posted upstream I don't think ldo7 will ever get disabled because
> it's marked "always-on"?
>
> Similarly, with what you've posted upstream I think your new
> "pp3300_hub" _will_ get disabled ~30 seconds after boot because it's
> not marked "always-on" and it has no clients.
Ah, now I see what you mean, thanks for the clarification. I associated
the ~30 seconds disabling with the RPMH regulators, but you're right, it's
generic regulator behavior (regulator_late_cleanup()).
So yeah, it seems some reshuffling of "always-on" and "boot-on" properties
is needed.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-11-05 18:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-03 18:38 [PATCH] arm64: dts: qcom: sc7180-trogdor: Make pp3300_a the default supply for pp3300_hub Matthias Kaehlcke
2020-11-05 0:29 ` Doug Anderson
2020-11-05 1:55 ` Matthias Kaehlcke
2020-11-05 15:57 ` Doug Anderson
2020-11-05 18:28 ` Matthias Kaehlcke
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).