[PATCH V2] gpu: drm: tegra: Adjust dsi phy timing

[PATCH V2] gpu: drm: tegra: Adjust dsi phy timing

[David Ung]

Fix DSI PHY HS Trail setting on Tegra.
It is found that HS Trail is off -12% during compliance testing

Signed-off-by: David Ung <davidu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/gpu/drm/tegra/mipi-phy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tegra/mipi-phy.c b/drivers/gpu/drm/tegra/mipi-phy.c
index 486d19d..974bc68 100644
--- a/drivers/gpu/drm/tegra/mipi-phy.c
+++ b/drivers/gpu/drm/tegra/mipi-phy.c
@@ -34,7 +34,7 @@ int mipi_dphy_timing_get_default(struct mipi_dphy_timing *timing,
 	timing->hszero = 145 + 5 * period;
 	timing->hssettle = 85 + 6 * period;
 	timing->hsskip = 40;
-	timing->hstrail = max(8 * period, 60 + 4 * period);
+	timing->hstrail = 3 * period * 8 + max(8 * period, 60 + 4 * period);
 	timing->init = 100000;
 	timing->lpx = 60;
 	timing->taget = 5 * timing->lpx;
-- 
1.8.1.5

Re: [PATCH V2] gpu: drm: tegra: Adjust dsi phy timing

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

On Fri, Dec 05, 2014 at 01:48:42PM -0800, Sean Paul wrote:
> On Tue, Dec 2, 2014 at 5:40 PM, David Ung <davidu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > Fix DSI PHY HS Trail setting on Tegra.
> > It is found that HS Trail is off -12% during compliance testing
> >
> >
> [this time from the correct email address]
> 
> Our timing tests pass now.
> 
> Tested-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Can describe what kinds of timing tests these are? I obviously lack the
kind of equipment for this, but I'd still like to know how you get these
results.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2] gpu: drm: tegra: Adjust dsi phy timing

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1923 bytes --]

On Fri, Dec 05, 2014 at 02:28:18PM -0800, Sean Paul wrote:
> On Fri, Dec 5, 2014 at 2:17 PM, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> wrote:
> 
> > On Fri, Dec 05, 2014 at 01:48:42PM -0800, Sean Paul wrote:
> > > On Tue, Dec 2, 2014 at 5:40 PM, David Ung <davidu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > >
> > > > Fix DSI PHY HS Trail setting on Tegra.
> > > > It is found that HS Trail is off -12% during compliance testing
> > > >
> > > >
> > > [this time from the correct email address]
> > >
> > > Our timing tests pass now.
> > >
> > > Tested-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >
> > Can describe what kinds of timing tests these are? I obviously lack the
> > kind of equipment for this, but I'd still like to know how you get these
> > results.
> >
> >
> Hi Thierry,
> The test that was failing was the THS-TRAIL test. You can find the details
> at [1], test is 8.1.13. The time was 7.7ns too short (or 12%, as David
> stated).
> 
> I'm not sure how David arrived at the value he did to extend HS-TRAIL, but
> that particular test is now passing.

From reading the document that you linked to and comparing to the D-PHY
specification (version 1.2, Table 14, Page 40), the proper formula for
the HSTrail parameter would be:

	max(n * 8 * period, 60 + n * 4 * period)

I suspect that the additional 3 * period * 8 in this patch is derived
from the note about "n", which is 1 for forward-direction high-speed
mode and 4 for reverse-direction high-speed mode. Translating this to
the code, the correct line would really be:

	timing->hstrail = max(4 * 8 * period, 60 + 4 * 4 * period);

Since the "n" parameter appears in the second term, too. Also this is
not Tegra-specific after all, so mipi_dphy_timing_get_default() indeed
is the right place for it.

How about the attached patch?

Thierry

[-- Attachment #1.2: 0001-drm-tegra-dsi-Adjust-D-PHY-timing.patch --]
[-- Type: text/x-diff, Size: 2822 bytes --]

From 3399b7247fdc9609daea9919c444326fb9ddbdac Mon Sep 17 00:00:00 2001
From: David Ung <davidu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Fri, 5 Dec 2014 15:30:05 -0800
Subject: [PATCH] drm/tegra: dsi: Adjust D-PHY timing

Compliance testing shows that HS Trail is off by -12%. Increase the HS
Trail time to make this test pass.

Signed-off-by: David Ung <davidu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org: update specification references, add comment]
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/gpu/drm/tegra/mipi-phy.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/tegra/mipi-phy.c b/drivers/gpu/drm/tegra/mipi-phy.c
index 486d19d589c8..ba2ae6511957 100644
--- a/drivers/gpu/drm/tegra/mipi-phy.c
+++ b/drivers/gpu/drm/tegra/mipi-phy.c
@@ -12,9 +12,9 @@
 #include "mipi-phy.h"
 
 /*
- * Default D-PHY timings based on MIPI D-PHY specification. Derived from
- * the valid ranges specified in Section 5.9 of the D-PHY specification
- * with minor adjustments.
+ * Default D-PHY timings based on MIPI D-PHY specification. Derived from the
+ * valid ranges specified in Section 6.9, Table 14, Page 40 of the D-PHY
+ * specification (v1.2) with minor adjustments.
  */
 int mipi_dphy_timing_get_default(struct mipi_dphy_timing *timing,
 				 unsigned long period)
@@ -34,7 +34,20 @@ int mipi_dphy_timing_get_default(struct mipi_dphy_timing *timing,
 	timing->hszero = 145 + 5 * period;
 	timing->hssettle = 85 + 6 * period;
 	timing->hsskip = 40;
-	timing->hstrail = max(8 * period, 60 + 4 * period);
+
+	/*
+	 * The MIPI D-PHY specification (Section 6.9, v1.2, Table 14, Page 40)
+	 * contains this formula as:
+	 *
+	 *     T_HS-TRAIL = max(n * 8 * period, 60 + n * 4 * period)
+	 *
+	 * where n = 1 for forward-direction HS mode and n = 4 for reverse-
+	 * direction HS mode. There's only one setting and this function does
+	 * not parameterize on anything other that period, so this code will
+	 * assumes that reverse-direction HS mode is supported and uses n = 4.
+	 */
+	timing->hstrail = max(4 * 8 * period, 60 + 4 * 4 * period);
+
 	timing->init = 100000;
 	timing->lpx = 60;
 	timing->taget = 5 * timing->lpx;
@@ -46,8 +59,8 @@ int mipi_dphy_timing_get_default(struct mipi_dphy_timing *timing,
 }
 
 /*
- * Validate D-PHY timing according to MIPI Alliance Specification for D-PHY,
- * Section 5.9 "Global Operation Timing Parameters".
+ * Validate D-PHY timing according to MIPI D-PHY specification (v1.2, Section
+ * Section 6.9 "Global Operation Timing Parameters").
  */
 int mipi_dphy_timing_validate(struct mipi_dphy_timing *timing,
 			      unsigned long period)
-- 
2.1.3


[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V2] gpu: drm: tegra: Adjust dsi phy timing

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2024 bytes --]

The subject prefix should be "drm/tegra: dsi:". Also make sure to use
the proper case. "dsi" should be "DSI" and "phy" should be "PHY". Also
the DSI PHY is typically referred to as D-PHY, so a better subject would
be:

	drm/tegra: dsi: Adjust D-PHY timing

One more comment below:

On Tue, Dec 02, 2014 at 05:40:23PM -0800, David Ung wrote:
> Fix DSI PHY HS Trail setting on Tegra.
> It is found that HS Trail is off -12% during compliance testing
> 
> Signed-off-by: David Ung <davidu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/gpu/drm/tegra/mipi-phy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tegra/mipi-phy.c b/drivers/gpu/drm/tegra/mipi-phy.c
> index 486d19d..974bc68 100644
> --- a/drivers/gpu/drm/tegra/mipi-phy.c
> +++ b/drivers/gpu/drm/tegra/mipi-phy.c
> @@ -34,7 +34,7 @@ int mipi_dphy_timing_get_default(struct mipi_dphy_timing *timing,
>  	timing->hszero = 145 + 5 * period;
>  	timing->hssettle = 85 + 6 * period;
>  	timing->hsskip = 40;
> -	timing->hstrail = max(8 * period, 60 + 4 * period);
> +	timing->hstrail = 3 * period * 8 + max(8 * period, 60 + 4 * period);
>  	timing->init = 100000;
>  	timing->lpx = 60;
>  	timing->taget = 5 * timing->lpx;

Back when I wrote this code I envisioned that this could eventually be a
generic set of helpers that could be used across different DSI drivers.
As such, mipi_dphy_timing_get_default() obtains the default timings as
given by the D-PHY specification. Therefore you should be adjusting this
within the driver code rather than the default timings. And then make it
something like:

	/* comment explaining why this is needed */
	timing.hstrail += 3 * period * 8;

That said, most of this code is in flux right now, so I probably have to
manually apply the patch. It would still be good to respin with these
comments addressed so that I can adopt the commit message and comment
explaining why this is necessary or what the consequences are.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]