Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220417181538.57fa1303@blackhole>
In-Reply-To: <20220417181538.57fa1303@blackhole>
From:   Vasily Khoruzhick <anarsoul@gmail.com>
Date:   Sun, 17 Apr 2022 11:52:14 -0700
Message-ID: <CA+E=qVeX2aU0hiDMxLXzVk-YiMsqKKFKpm=cc=72joMhZmNV1g@mail.gmail.com>
Subject: Re: [PATCH] drm/bridge: fix anx6345 power up sequence
To:     Torsten Duwe <duwe@lst.de>
Cc:     Andrzej Hajda <a.hajda@samsung.com>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Robert Foss <robert.foss@linaro.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Thierry Reding <treding@nvidia.com>,
        Lyude Paul <lyude@redhat.com>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Harald Geyer <harald@ccbib.org>, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, Apr 17, 2022 at 9:15 AM Torsten Duwe <duwe@lst.de> wrote:
>
> Align the power-up sequence with the known-good procedure documented in [1]:
> un-swap dvdd12 and dvdd25, and allow a little extra time for them to settle
> before de-asserting reset.

Hi Torsten,

Interesting find! I tried to fix the issue several times by playing
with the delays to no avail.

What's interesting, ANX6345 datasheet allows DVDD12 to come up either
earlier or later than DVDD25 with the delay of T1 (2ms typical)
between them, and actually bringing up DVDD12 first works fine in
u-boot.

The datasheet also requires reset to be deasserted no earlier than T2
(2-5ms) after all the rails are stable.

Another thing it mentions is that the system clock must be stable for
T3 (1-3ms) before reset is deasserted, T3 is already a part of T2,
however it cannot be gated on Pinebook, see [1], page 15

The change looks good to me, but I'll need some time to actually test
it. If you don't hear from me for longer than a week please ping me.

[1] https://files.pine64.org/doc/pinebook/pinebook_mainboard_schematic_3.0.pdf

Regards,
Vasily

> Fixes: 6aa192698089b ("drm/bridge: Add Analogix anx6345 support")
>
> [1] https://github.com/OLIMEX/DIY-LAPTOP/blob/master/
> HARDWARE/A64-TERES/TERES-PCB1-A64-MAIN/Rev.C/TERES_PCB1-A64-MAIN_Rev.C.pdf
> (page 5, blue comment down left)
>
> Reported-by: Harald Geyer <harald@ccbib.org>
> Signed-off-by: Torsten Duwe <duwe@lst.de>
> Cc: stable@vger.kernel.org
> ---
>
> This fixes the problem that e.g. X screensaver turns the screen black,
> and it stays black until the next reboot; definitely on the Teres-I,
> probably on the pinebook64, too.

You should probably move this comment up to be included in the commit message.


>
> --- a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> @@ -309,27 +309,27 @@ static void anx6345_poweron(struct anx63
>         gpiod_set_value_cansleep(anx6345->gpiod_reset, 1);
>         usleep_range(1000, 2000);
>
> -       err = regulator_enable(anx6345->dvdd12);
> +       err = regulator_enable(anx6345->dvdd25);
>         if (err) {
> -               DRM_ERROR("Failed to enable dvdd12 regulator: %d\n",
> +               DRM_ERROR("Failed to enable dvdd25 regulator: %d\n",
>                           err);
>                 return;
>         }
>
> -       /* T1 - delay between VDD12 and VDD25 should be 0-2ms */
> +       /* T1 - delay between VDD25 and VDD12 should be 0-2ms */
>         usleep_range(1000, 2000);
>
> -       err = regulator_enable(anx6345->dvdd25);
> +       err = regulator_enable(anx6345->dvdd12);
>         if (err) {
> -               DRM_ERROR("Failed to enable dvdd25 regulator: %d\n",
> +               DRM_ERROR("Failed to enable dvdd12 regulator: %d\n",
>                           err);
>                 return;
>         }
>
>         /* T2 - delay between RESETN and all power rail stable,
> -        * should be 2-5ms
> +        * should be at least 2-5ms, 10ms to be safe.
>          */
> -       usleep_range(2000, 5000);
> +       usleep_range(9000, 11000);
>
>         gpiod_set_value_cansleep(anx6345->gpiod_reset, 0);
>