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]

Date:   Wed, 4 Nov 2020 16:29:50 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Matthias Kaehlcke <mka@...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,

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.


> 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.  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?


> +       };
> +
>         /* 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.


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

-Doug