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

[Doug Anderson]

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.

-Doug