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