| Message ID | 20201103103749.1.I0ed4abdd2b2916fbedf76be254bc3457fb8b9655@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: qcom: sc7180-trogdor: Make pp3300_a the default supply for pp3300_hub | expand |
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
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
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
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.
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";
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(-)