[PATCH] drm/bridge: fix anx6345 power up sequence

[PATCH] drm/bridge: fix anx6345 power up sequence

[Torsten Duwe]

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

--- 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);
 

Re: [PATCH] drm/bridge: fix anx6345 power up sequence

[Vasily Khoruzhick]

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);
>

Re: [PATCH] drm/bridge: fix anx6345 power up sequence

[Vasily Khoruzhick]

On Sun, Apr 17, 2022 at 11:52 AM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>
> 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.

Your change doesn't fix the issue for me. Running "xrandr --output
eDP-1 --off; xrandr --output eDP-1 --auto" in a loop triggers the
issue pretty quickly even with the patch.

Re: [PATCH] drm/bridge: fix anx6345 power up sequence

[Torsten Duwe]

On Mon, 18 Apr 2022 17:25:57 -0700
Vasily Khoruzhick <anarsoul@gmail.com> wrote:

> On Sun, Apr 17, 2022 at 11:52 AM Vasily Khoruzhick
> <anarsoul@gmail.com> wrote:

> > 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.
> 
> Your change doesn't fix the issue for me. Running "xrandr --output
> eDP-1 --off; xrandr --output eDP-1 --auto" in a loop triggers the
> issue pretty quickly even with the patch.

Nope, even that works fine here. Side question: how do you initially
power on the eDP bridge? Could there be any leftovers from that
mechanism? I use a hacked-up U-Boot with a procedure similar to the
kernel driver as fixed by this change.

But the main question is: does this patch in any way worsen the
situation on the pinebook?

	Torsten

Re: [PATCH] drm/bridge: fix anx6345 power up sequence

[Vasily Khoruzhick]

On Thu, Apr 28, 2022 at 8:58 AM Torsten Duwe <duwe@lst.de> wrote:
>
> On Mon, 18 Apr 2022 17:25:57 -0700
> Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>
> > On Sun, Apr 17, 2022 at 11:52 AM Vasily Khoruzhick
> > <anarsoul@gmail.com> wrote:
>
> > > 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.
> >
> > Your change doesn't fix the issue for me. Running "xrandr --output
> > eDP-1 --off; xrandr --output eDP-1 --auto" in a loop triggers the
> > issue pretty quickly even with the patch.
>
> Nope, even that works fine here. Side question: how do you initially
> power on the eDP bridge? Could there be any leftovers from that
> mechanism? I use a hacked-up U-Boot with a procedure similar to the
> kernel driver as fixed by this change.
>
> But the main question is: does this patch in any way worsen the
> situation on the pinebook?

I don't think it worsens anything, but according to the datasheet the
change makes no sense. Could you try increasing T2 instead of changing
the power sequence?

>         Torsten

Re: [PATCH] drm/bridge: fix anx6345 power up sequence

[Torsten Duwe]

On Wed, 18 May 2022 09:53:58 -0700
Vasily Khoruzhick <anarsoul@gmail.com> wrote:

> On Thu, Apr 28, 2022 at 8:58 AM Torsten Duwe <duwe@lst.de> wrote:

> > power on the eDP bridge? Could there be any leftovers from that
> > mechanism? I use a hacked-up U-Boot with a procedure similar to the
> > kernel driver as fixed by this change.

I was asking because I recall an ugly hack in some ATF code to power up
the chip correctly. Did you patch ATF, and maybe call functions of it
at runtime?

> >
> > But the main question is: does this patch in any way worsen the
> > situation on the pinebook?
> 
> I don't think it worsens anything, but according to the datasheet the
> change makes no sense. Could you try increasing T2 instead of changing
> the power sequence?

According to the datasheet, there is also T3, I realise now. The
diagram talks about "System Clock", but both Teres and Pinebook have a
passive resonator circuit there. Correct me if I'm wrong, but without
chip power, there is little to resonate. What if that driving clock
circuit is powered by Vdd25? Maybe the earlier provision of 2V5 is
enough for Teres' Q4, but Pinebook X4 takes even longer? The start-up
times can be in the range of milliseconds.

	Torsten

Re: [PATCH] drm/bridge: fix anx6345 power up sequence

[Vasily Khoruzhick]

On Thu, May 19, 2022 at 6:40 AM Torsten Duwe <duwe@lst.de> wrote:
>
> On Wed, 18 May 2022 09:53:58 -0700
> Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>
> > On Thu, Apr 28, 2022 at 8:58 AM Torsten Duwe <duwe@lst.de> wrote:
>
> > > power on the eDP bridge? Could there be any leftovers from that
> > > mechanism? I use a hacked-up U-Boot with a procedure similar to the
> > > kernel driver as fixed by this change.
>
> I was asking because I recall an ugly hack in some ATF code to power up
> the chip correctly. Did you patch ATF, and maybe call functions of it
> at runtime?

Initially it's powered on by ATF on system power up. ATF parses DTB
and finds the regulators that it needs to enable and enables them.
It's done in ATF because u-boot SPL didn't have enough space to fit in
the AXP803 driver. It's only done at startup and once linux takes
over, ATF doesn't touch these regulators.

> > >
> > > But the main question is: does this patch in any way worsen the
> > > situation on the pinebook?
> >
> > I don't think it worsens anything, but according to the datasheet the
> > change makes no sense. Could you try increasing T2 instead of changing
> > the power sequence?
>
> According to the datasheet, there is also T3, I realise now. The
> diagram talks about "System Clock", but both Teres and Pinebook have a
> passive resonator circuit there. Correct me if I'm wrong, but without
> chip power, there is little to resonate. What if that driving clock
> circuit is powered by Vdd25? Maybe the earlier provision of 2V5 is
> enough for Teres' Q4, but Pinebook X4 takes even longer? The start-up
> times can be in the range of milliseconds.

That's plausible, but can you please try just increasing the delays
without changing the power sequence?

>         Torsten