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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201105015501.GA3079843@google.com>
Date:   Wed, 4 Nov 2020 17:55:01 -0800
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] arm64: dts: qcom: sc7180-trogdor: Make pp3300_a the
 default supply for pp3300_hub

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