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@xxxxxxxxxxxx> 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