Re: [PATCH] arm64: dts: qcom: sc7180-trogdor: Make pp3300_a the default supply for pp3300_hub

[Matthias Kaehlcke]

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.