linux-kernel - Re: [PATCH v5 4/4] arm64: dts: qcom: Add Acer Aspire 1

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:   Fri, 7 Apr 2023 09:23:31 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Nikita Travkin <nikita@...n.ru>
Cc:     agross@...nel.org, andersson@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, konrad.dybcio@...aro.org,
        quic_srivasam@...cinc.com, judyhsiao@...omium.org,
        mka@...omium.org, cros-qcom-dts-watchers@...omium.org,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v5 4/4] arm64: dts: qcom: Add Acer Aspire 1

Hi,

I didn't do too thorough of a review, but I noticed your comment about
the panel power and took a look...

On Fri, Apr 7, 2023 at 8:14 AM Nikita Travkin <nikita@...n.ru> wrote:
>
> +       reg_lcm_3p3: panel-regulator {
> +               compatible = "regulator-fixed";
> +               regulator-name = "lcm_3p3";
> +               regulator-min-microvolt = <3300000>;
> +               regulator-max-microvolt = <3300000>;
> +
> +               /*
> +                * HACK: Display fails with
> +                *
> +                * *ERROR* Unexpected max rate (0x0); assuming 5.4 GHz
> +                * *ERROR* Link training failed, link is off (-5)
> +                *
> +                * if the power to the panel was ever cut
> +                */
> +               regulator-always-on;

I'm curious if `off-on-delay-us = <500000>;` would help you avoid the
hack. The eDP driver should already enforce stuff like this but I
think in some esoteric -EPROBE_DEFER cases it can end up violating
things. Any chance that's what you hit?

Oh, or maybe it's HPD. See below. Even if it's HPD, having an
'off-on-delay-us' specified here isn't a bad idea.

> +&i2c10 {
> +       clock-frequency = <400000>;
> +       status = "okay";
> +
> +       sn65dsi86_bridge: bridge@2c {
> +               compatible = "ti,sn65dsi86";
> +               reg = <0x2c>;
> +               gpio-controller;
> +               #gpio-cells = <2>;
> +               #pwm-cells = <1>;
> +
> +               interrupt-parent = <&tlmm>;
> +               interrupts = <11 IRQ_TYPE_LEVEL_HIGH>;
> +
> +               enable-gpios = <&tlmm 51 GPIO_ACTIVE_HIGH>;
> +               suspend-gpios = <&tlmm 22 GPIO_ACTIVE_LOW>;
> +
> +               pinctrl-0 = <&bridge_en_default>,
> +                           <&edp_bridge_irq_default>,
> +                           <&bridge_suspend_default>;
> +               pinctrl-names = "default";
> +
> +               vpll-supply = <&reg_brij_1p8>;
> +               vccio-supply = <&reg_brij_1p8>;
> +               vcca-supply = <&reg_brij_1p2>;
> +               vcc-supply = <&reg_brij_1p2>;
> +
> +               clocks = <&rpmhcc RPMH_LN_BB_CLK3>;
> +               clock-names = "refclk";

You want "no-hpd;" here somewhere. See below.


> +
> +               ports {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +
> +                       port@0 {
> +                               reg = <0>;
> +
> +                               sn65dsi86_in: endpoint {
> +                                       remote-endpoint = <&dsi0_out>;
> +                               };
> +                       };
> +
> +                       port@1 {
> +                               reg = <1>;
> +
> +                               sn65dsi86_out: endpoint {
> +                                       data-lanes = <0 1>;
> +                                       remote-endpoint = <&panel_in_edp>;
> +                               };
> +                       };
> +               };
> +
> +               aux-bus {
> +                       panel: panel {
> +                               compatible = "edp-panel";
> +                               power-supply = <&reg_lcm_3p3>;
> +                               backlight = <&backlight>;

I think you want:

no-hpd;
hpd-absent-delay-ms = <200>;

...and yes, you end up with "no-hpd" in both the panel node and the
ti-sn65dsi86 node. See sdm845-cheza.

HPD might very well be hooked up on your board, but the current Linux
ti-sn65dsi86 driver does not look at its own HPD line because it's
actually slower than just pretending that HPD isn't there. On trogdor
boards we ended up routing HPD to a GPIO.

I guess your other option would be to implement HPD support in
ti-sn65dsi86. That would probably be an overall slower boot for you,
but is technically more correct. In the past people have posted up
patches to get ti-sn65dsi86 working as a full DP port and they added
HPD support for that, but none of those patch series ever go to the
point of landing...