linux-kernel - Re: [PATCH v2 2/2] 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: <CAD=FV=Xi-Fiay983L4WWVA07WWZvL0DSK4cazBwb9B3brVgM-g@mail.gmail.com>
Date:   Thu, 5 Nov 2020 17:05:38 -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 v2 2/2] arm64: dts: qcom: sc7180-trogdor: Make pp3300_a
 the default supply for pp3300_hub

Hi,

On Thu, Nov 5, 2020 at 4:37 PM Matthias Kaehlcke <mka@...omium.org> wrote:
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
> index 0a281c24841c..6603f2102233 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
> @@ -58,10 +58,23 @@ ap_ts: touchscreen@10 {
>         };
>  };
>
> +&pp3300_hub {
> +       /* pp3300_l7c is used to power the USB hub */
> +       /delete-property/regulator-always-on;
> +};
> +
> +&pp3300_l7c {
> +       regulator-always-on;

Personally I always end up pairing "always-on" and "boot-on", but that
might just be superstition from many kernel versions ago when there
were weird quirks.  The way you have it now you will sometimes have
"boot-on" but not "always-on".  Probably what you have is fine,
though.


> +};
> +
>  &sdhc_2 {
>         status = "okay";
>  };
>
> +&usb_hub {
> +        vdd-supply = <&pp3300_l7c>;
> +};
> +
>  /* PINCTRL - board-specific pinctrl */
>
>  &tlmm {
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index bf875589d364..50e733412a7f 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -174,6 +174,25 @@ 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>;
> +
> +               /* AP turns on with en_pp3300_hub; always on for AP */

Delete the above comment.  It's obvious based on the properties in
this node.  Other similar comments are useful because they describe
how the _EC_ turns on regulators and why a regulator that has an
enable still looks like an "always-on" regulator to the AP (because
it's always on whenever the AP is on).

If you want to add a comment, you could say:

/* Always on until we have a way to specify it can go off in suspend */


> @@ -469,7 +488,6 @@ ppvar_l6c: ldo6 {
>                         regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
>                 };
>
> -               pp3300_hub:
>                 pp3300_l7c: ldo7 {
>                         regulator-min-microvolt = <3304000>;
>                         regulator-max-microvolt = <3304000>;

Shouldn't you delete the "regulator-always-on;" from ldo7 since you're
adding it for all the older revs?