[PATCH 1/2] dt-bindings: drm/bridge: sn65dsi86: Add panel-hpd-delay

[PATCH 1/2] dt-bindings: drm/bridge: sn65dsi86: Add panel-hpd-delay

[Douglas Anderson]

The timing diagram of some eDP panels says that you're supposed to
wait for HPD to be asserted before the aux channel is operational.

In some cases, however, it's better to just hardcode the max delay
instead of trying to use HPD.  Why?

The sn65dsi86 datasheet says that it only reports the debounced
HPD signal to software.  It will tell software about HPD assertion
as quickly as 100 ms after it's asserted, but sometimes it might
take 400 ms because it's timed with a very inaccurate ring
oscillator.  In practice it was measured at 200 ms on at least
one system.

On a particular panel, HPD was asserted 84 ms after power was given.
This same panel specified that HPD would always be asserted within
200 ms of applying power.  Thus on this panel with the measured
84 ms to assert HPD + the 200 ms measured debounce we'd wait 284 ms
which is 84 ms longer than just hardcoding the sleep.

Let's allow boards to explicitly choose the hardcoded delay by adding
a device tree attribute for it.  A few notes:
a) This delay can't easily be in the panel bindings because the delay
   wouldn't actually be needed if the same panel were hooked somewhere
   else (someplace with more sane HPD behavior).
b) The nice thing about being able to set this delay is that it's also
   useful on boards where HPD wasn't hooked up at all (for whatever
   reason).

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 .../bindings/display/bridge/ti,sn65dsi86.txt          | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
index 0a3fbb53a16e..1a1ca0f7a1d8 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
@@ -33,6 +33,15 @@ Optional properties:
 
 - suspend-gpios: specification for GPIO1 pin on bridge (active low)
 
+- ti,panel-hpd-delay-ms: Assume that HPD from the panel will be asserted within
+			 this many milliseconds after giving power to the panel.
+			 With this number we can ignore the HPD signal from
+			 the panel and just hardcode a delay.  This is useful
+			 to do because the bridge chip only provides the
+			 debounced HPD signal to software and the timing of the
+			 debounce is very inaccurate so it's often faster to
+			 hardcode the max from the panel spec.
+
 Required nodes:
 This device has two video ports. Their connections are modelled using the
 OF graph bindings specified in Documentation/devicetree/bindings/graph.txt.
@@ -62,6 +71,8 @@ edp-bridge@2d {
 	clock-names = "refclk";
 	clocks = <&input_refclk>;
 
+	ti,panel-hpd-delay-ms = <200>;
+
 	ports {
 		#address-cells = <1>;
 		#size-cells = <0>;
-- 
2.19.1.568.g152ad8e336-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] dt-bindings: drm/bridge: sn65dsi86: Add panel-hpd-delay

[Douglas Anderson]

The timing diagram of some eDP panels says that you're supposed to
wait for HPD to be asserted before the aux channel is operational.

In some cases, however, it's better to just hardcode the max delay
instead of trying to use HPD.  Why?

The sn65dsi86 datasheet says that it only reports the debounced
HPD signal to software.  It will tell software about HPD assertion
as quickly as 100 ms after it's asserted, but sometimes it might
take 400 ms because it's timed with a very inaccurate ring
oscillator.  In practice it was measured at 200 ms on at least
one system.

On a particular panel, HPD was asserted 84 ms after power was given.
This same panel specified that HPD would always be asserted within
200 ms of applying power.  Thus on this panel with the measured
84 ms to assert HPD + the 200 ms measured debounce we'd wait 284 ms
which is 84 ms longer than just hardcoding the sleep.

Let's allow boards to explicitly choose the hardcoded delay by adding
a device tree attribute for it.  A few notes:
a) This delay can't easily be in the panel bindings because the delay
   wouldn't actually be needed if the same panel were hooked somewhere
   else (someplace with more sane HPD behavior).
b) The nice thing about being able to set this delay is that it's also
   useful on boards where HPD wasn't hooked up at all (for whatever
   reason).

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 .../bindings/display/bridge/ti,sn65dsi86.txt          | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
index 0a3fbb53a16e..1a1ca0f7a1d8 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
@@ -33,6 +33,15 @@ Optional properties:
 
 - suspend-gpios: specification for GPIO1 pin on bridge (active low)
 
+- ti,panel-hpd-delay-ms: Assume that HPD from the panel will be asserted within
+			 this many milliseconds after giving power to the panel.
+			 With this number we can ignore the HPD signal from
+			 the panel and just hardcode a delay.  This is useful
+			 to do because the bridge chip only provides the
+			 debounced HPD signal to software and the timing of the
+			 debounce is very inaccurate so it's often faster to
+			 hardcode the max from the panel spec.
+
 Required nodes:
 This device has two video ports. Their connections are modelled using the
 OF graph bindings specified in Documentation/devicetree/bindings/graph.txt.
@@ -62,6 +71,8 @@ edp-bridge@2d {
 	clock-names = "refclk";
 	clocks = <&input_refclk>;
 
+	ti,panel-hpd-delay-ms = <200>;
+
 	ports {
 		#address-cells = <1>;
 		#size-cells = <0>;
-- 
2.19.1.568.g152ad8e336-goog

[PATCH 2/2] drm/bridge: ti-sn65dsi86: Allow DT to set "HPD delay"

[Douglas Anderson]

Let's solve the mystery of commit bf1178c98930 ("drm/bridge:
ti-sn65dsi86: Add mystery delay to enable()").  Specifically the
reason we needed that mystery delay is that we weren't paying
attention to HPD.

Looking at the datasheet for the same panel that was tested for the
original commit, I see there's a timing "t3" that times from power on
to the aux channel being operational.  This time is specced as 0 - 200
ms.  The datasheet says that the aux channel is operational at exactly
the same time that HPD is asserted.

Scoping the signals on this board showed that HPD was asserted 84 ms
after power was asserted.  That very closely matches the magic 70 ms
delay that we had.  ...and actually, in my esting the 70 ms wasn't
quite enough of a delay and some percentage of the time the display
didn't come up until I bumped it to 100 ms.

To solve this, we tried to hook up the HPD signal in the bridge.
...but in doing so we found that that the bridge didn't report that
HPD was asserted until ~280 ms after we powered it (!).  This is
explained by looking at the sn65dsi86 datasheet section "8.4.5.1 HPD
(Hot Plug/Unplug Detection)".  Reading there we see that the bridge
isn't even intended to report HPD until 100 ms after it's asserted.
...but that would have left us at 184 ms.  The extra 100 ms
(presumably) comes from this part in the datasheet:

> The HPD state machine operates off an internal ring oscillator. The
> ring oscillator frequency will vary [ ... ]. The min/max range in
> the HPD State Diagram refers to the possible times based off
> variation in the ring oscillator frequency.

Given that the 280 ms we'll end up delaying if we hook up HPD is
_slower_ than the 200 ms we could just hardcode, for now we'll solve
the problem by just allowing boards to hardcode a value.  If someone
using this part finds that they can get things to work more quickly by
actually hooking up HPD that can always be a future patch.

One last note is that I tried to solve this through another way: In
ti_sn_bridge_enable() I tried to use various combinations of
dp_dpcd_writeb() and dp_dpcd_readb() to detect when the aux channel
was up.  In theory that would let me detect _exactly_ when I could
continue and do link training.  Unfortunately even if I did an aux
transfer w/out waiting I couldn't see any errors.  Possibly I could
keep looping over link training until it came back with success, but
that seemed a little overly hacky to me.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 45 ++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index f8a931cf3665..5deed667480c 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -93,6 +93,7 @@ struct ti_sn_bridge {
 	struct clk			*refclk;
 	struct drm_panel		*panel;
 	struct gpio_desc		*enable_gpio;
+	int				panel_hpd_delay_ms;
 	struct regulator_bulk_data	supplies[SN_REGULATOR_SUPPLY_NUM];
 };
 
@@ -459,16 +460,37 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	int ret;
 
 	/*
-	 * FIXME:
-	 * This 70ms was found necessary by experimentation. If it's not
-	 * present, link training fails. It seems like it can go anywhere from
-	 * pre_enable() up to semi-auto link training initiation below.
+	 * The timing diagram of some eDP panels says that you're supposed to
+	 * wait for HPD to be asserted before the aux channel is operational.
 	 *
-	 * Neither the datasheet for the bridge nor the panel tested mention a
-	 * delay of this magnitude in the timing requirements. So for now, add
-	 * the mystery delay until someone figures out a better fix.
+	 * While we could configure the bridge to report the HPD signal to us
+	 * and add a delay here until the HPD is asserted, it turns out that's
+	 * slower than just hardcoding the max delay from the panel in some
+	 * cases.  Why?
+	 *
+	 * The sn65dsi86 datasheet says that it only reports the debounced
+	 * HPD signal to software.  It will tell software about HPD assertion
+	 * as quickly as 100 ms after it's asserted, but sometimes it might
+	 * take 400 ms because it's timed with a very inaccurate ring
+	 * oscillator.  In practice it was measured at 200 ms on at least
+	 * one system.
+	 *
+	 * On a particular panel, HPD was asserted 84 ms after power was given.
+	 * This same panel specified that HPD would always be asserted within
+	 * 200 ms of applying power.  Thus on this panel with the measured
+	 * 84 ms to assert HPD + the 200 ms measured debounce we'd wait 284 ms
+	 * which is 84 ms longer than just hardcoding the sleep.
+	 *
+	 * For now we don't know of any cases where paying attention to HPD
+	 * is better than hardcoding the value.  Thus for now only support the
+	 * hardcoded delay and print a warning if it wasn't specified.  Later
+	 * one could imagine improving the driver to enable HPD support if
+	 * panel-hpd-delay-ms wasn't specified in the device tree.
 	 */
-	msleep(70);
+	if (pdata->panel_hpd_delay_ms >= 0)
+		msleep(pdata->panel_hpd_delay_ms);
+	else
+		DRM_WARN("HPD not supported; consider a hardcoded delay\n");
 
 	/* DSI_A lane config */
 	val = CHA_DSI_LANES(4 - pdata->dsi->lanes);
@@ -656,6 +678,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 {
 	struct ti_sn_bridge *pdata;
 	int ret;
+	u32 val;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
 		DRM_ERROR("device doesn't support I2C\n");
@@ -712,6 +735,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
+	if (!of_property_read_u32(pdata->dev->of_node,
+				  "ti,panel-hpd-delay-ms", &val))
+		pdata->panel_hpd_delay_ms = val;
+	else
+		pdata->panel_hpd_delay_ms = -1;
+
 	pm_runtime_enable(pdata->dev);
 
 	i2c_set_clientdata(client, pdata);
-- 
2.19.1.568.g152ad8e336-goog

Re: [PATCH 1/2] dt-bindings: drm/bridge: sn65dsi86: Add panel-hpd-delay

[Laurent Pinchart]

Hi Douglas,

Thank you for the patch.

On Friday, 19 October 2018 23:19:39 EEST Douglas Anderson wrote:
> The timing diagram of some eDP panels says that you're supposed to
> wait for HPD to be asserted before the aux channel is operational.
> 
> In some cases, however, it's better to just hardcode the max delay
> instead of trying to use HPD.  Why?
> 
> The sn65dsi86 datasheet says that it only reports the debounced
> HPD signal to software.  It will tell software about HPD assertion
> as quickly as 100 ms after it's asserted, but sometimes it might
> take 400 ms because it's timed with a very inaccurate ring
> oscillator.  In practice it was measured at 200 ms on at least
> one system.
> 
> On a particular panel, HPD was asserted 84 ms after power was given.
> This same panel specified that HPD would always be asserted within
> 200 ms of applying power.  Thus on this panel with the measured
> 84 ms to assert HPD + the 200 ms measured debounce we'd wait 284 ms
> which is 84 ms longer than just hardcoding the sleep.
> 
> Let's allow boards to explicitly choose the hardcoded delay by adding
> a device tree attribute for it.  A few notes:
> a) This delay can't easily be in the panel bindings because the delay
>    wouldn't actually be needed if the same panel were hooked somewhere
>    else (someplace with more sane HPD behavior).

The delay shouldn't be handled in the panel driver, but I think it's still a 
property of the panel, and should thus be specified in the panel DT node. The 
panel driver should parse it from DT (or, if the panel driver knows about the 
specific panel model, just hardcode it), and then report it to the bridge 
driver which can then decide, based on its knowledge if the bridge internal 
HPD processing delays, to just wait for a fixed delay or use regular HPD 
handling.

> b) The nice thing about being able to set this delay is that it's also
>    useful on boards where HPD wasn't hooked up at all (for whatever
>    reason).
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  .../bindings/display/bridge/ti,sn65dsi86.txt          | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt index
> 0a3fbb53a16e..1a1ca0f7a1d8 100644
> --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> @@ -33,6 +33,15 @@ Optional properties:
> 
>  - suspend-gpios: specification for GPIO1 pin on bridge (active low)
> 
> +- ti,panel-hpd-delay-ms: Assume that HPD from the panel will be asserted
> within
> +			 this many milliseconds after giving power to the panel.
> +			 With this number we can ignore the HPD signal from
> +			 the panel and just hardcode a delay.  This is useful
> +			 to do because the bridge chip only provides the
> +			 debounced HPD signal to software and the timing of the
> +			 debounce is very inaccurate so it's often faster to
> +			 hardcode the max from the panel spec.
> +
>  Required nodes:
>  This device has two video ports. Their connections are modelled using the
>  OF graph bindings specified in Documentation/devicetree/bindings/graph.txt.
> @@ -62,6 +71,8 @@ edp-bridge@2d {
>  	clock-names = "refclk";
>  	clocks = <&input_refclk>;
> 
> +	ti,panel-hpd-delay-ms = <200>;
> +
>  	ports {
>  		#address-cells = <1>;
>  		#size-cells = <0>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] dt-bindings: drm/bridge: sn65dsi86: Add panel-hpd-delay

[Doug Anderson]

Hi,

On Sun, Oct 21, 2018 at 2:07 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Douglas,
>
> Thank you for the patch.
>
> On Friday, 19 October 2018 23:19:39 EEST Douglas Anderson wrote:
> > The timing diagram of some eDP panels says that you're supposed to
> > wait for HPD to be asserted before the aux channel is operational.
> >
> > In some cases, however, it's better to just hardcode the max delay
> > instead of trying to use HPD.  Why?
> >
> > The sn65dsi86 datasheet says that it only reports the debounced
> > HPD signal to software.  It will tell software about HPD assertion
> > as quickly as 100 ms after it's asserted, but sometimes it might
> > take 400 ms because it's timed with a very inaccurate ring
> > oscillator.  In practice it was measured at 200 ms on at least
> > one system.
> >
> > On a particular panel, HPD was asserted 84 ms after power was given.
> > This same panel specified that HPD would always be asserted within
> > 200 ms of applying power.  Thus on this panel with the measured
> > 84 ms to assert HPD + the 200 ms measured debounce we'd wait 284 ms
> > which is 84 ms longer than just hardcoding the sleep.
> >
> > Let's allow boards to explicitly choose the hardcoded delay by adding
> > a device tree attribute for it.  A few notes:
> > a) This delay can't easily be in the panel bindings because the delay
> >    wouldn't actually be needed if the same panel were hooked somewhere
> >    else (someplace with more sane HPD behavior).
>
> The delay shouldn't be handled in the panel driver, but I think it's still a
> property of the panel, and should thus be specified in the panel DT node. The
> panel driver should parse it from DT (or, if the panel driver knows about the
> specific panel model, just hardcode it), and then report it to the bridge
> driver which can then decide, based on its knowledge if the bridge internal
> HPD processing delays, to just wait for a fixed delay or use regular HPD
> handling.

OK, that's fair.

I looked a little at trying to add this but it seems like it's a
pretty specific use case and to communicate between the panel and the
bridge I believe I'd need to add a bunch of members and/or functions
into the drm_connector structures that nobody would use except this
one case.  I also still need to add a device tree attribute to the
panel to say that HPD shouldn't be used.

Before I go about doing that, I have another idea that maybe you'd be
OK with.  I've written up patches for it to hopefully make it easier
to see what it looks like.  Instead of adding the device tree
attribute for "don't use HPD" to the bridge, I can just add that to
the panel.  The panel knows that if HPD isn't connected that it will
need to insert a delay.  No abstraction violations and no need to even
communicate to the bridge.  So basically anyone using this panel w/
this bridge chip should know that HPD isn't useful and not bother
hooking it up and then set this property.  ...and in fact, on most of
our boards HPD isn't actually connected.  We just started connecting
it on some of them to see if it was the best way to fix this delay...

Patches can be found at:

https://lore.kernel.org/patchwork/patch/1002547/
[1/6] dt-bindings: drm/panel: simple: Add no-hpd property

https://lore.kernel.org/patchwork/patch/1002548/
[2/6] drm/panel: simple: Support panels with HPD where HPD isn't connected

https://lore.kernel.org/patchwork/patch/1002552/
[3/6] drm/panel: simple: Add "no-hpd" delay for Innolux TV123WAM

https://lore.kernel.org/patchwork/patch/1002549/
[4/6] drm/bridge: ti-sn65dsi86: Remove the mystery delay

https://lore.kernel.org/patchwork/patch/1002551/
[5/6] drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1

https://lore.kernel.org/patchwork/patch/1002550/
[6/6] dt-bindings: drm/panel: simple: Innolux TV123WAM is actually P120ZDG-BF1


Thanks!

-Doug

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi86: Allow DT to set "HPD delay"

[Doug Anderson]

Hi,

On Fri, Oct 19, 2018 at 1:20 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> Let's solve the mystery of commit bf1178c98930 ("drm/bridge:
> ti-sn65dsi86: Add mystery delay to enable()").  Specifically the
> reason we needed that mystery delay is that we weren't paying
> attention to HPD.
>
> Looking at the datasheet for the same panel that was tested for the
> original commit, I see there's a timing "t3" that times from power on
> to the aux channel being operational.  This time is specced as 0 - 200
> ms.  The datasheet says that the aux channel is operational at exactly
> the same time that HPD is asserted.
>
> Scoping the signals on this board showed that HPD was asserted 84 ms
> after power was asserted.  That very closely matches the magic 70 ms
> delay that we had.  ...and actually, in my esting the 70 ms wasn't
> quite enough of a delay and some percentage of the time the display
> didn't come up until I bumped it to 100 ms.
>
> To solve this, we tried to hook up the HPD signal in the bridge.
> ...but in doing so we found that that the bridge didn't report that
> HPD was asserted until ~280 ms after we powered it (!).  This is
> explained by looking at the sn65dsi86 datasheet section "8.4.5.1 HPD
> (Hot Plug/Unplug Detection)".  Reading there we see that the bridge
> isn't even intended to report HPD until 100 ms after it's asserted.
> ...but that would have left us at 184 ms.  The extra 100 ms
> (presumably) comes from this part in the datasheet:
>
> > The HPD state machine operates off an internal ring oscillator. The
> > ring oscillator frequency will vary [ ... ]. The min/max range in
> > the HPD State Diagram refers to the possible times based off
> > variation in the ring oscillator frequency.
>
> Given that the 280 ms we'll end up delaying if we hook up HPD is
> _slower_ than the 200 ms we could just hardcode, for now we'll solve
> the problem by just allowing boards to hardcode a value.  If someone
> using this part finds that they can get things to work more quickly by
> actually hooking up HPD that can always be a future patch.
>
> One last note is that I tried to solve this through another way: In
> ti_sn_bridge_enable() I tried to use various combinations of
> dp_dpcd_writeb() and dp_dpcd_readb() to detect when the aux channel
> was up.  In theory that would let me detect _exactly_ when I could
> continue and do link training.  Unfortunately even if I did an aux
> transfer w/out waiting I couldn't see any errors.  Possibly I could
> keep looping over link training until it came back with success, but
> that seemed a little overly hacky to me.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 45 ++++++++++++++++++++++-----
>  1 file changed, 37 insertions(+), 8 deletions(-)

Adding breadcrumbs to point to the new version of this patch at
<https://lore.kernel.org/patchwork/patch/1002549/> AKA ("[4/6]
drm/bridge: ti-sn65dsi86: Remove the mystery delay").  I didn't call
that a v2 since it's a pretty different approach compared to this one,
but (assuming people are OK w/ it) it replaces this patch.

Thanks!

-Doug

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi86: Allow DT to set "HPD delay"

[spanda]

On 2018-10-20 01:49, Douglas Anderson wrote:
> Let's solve the mystery of commit bf1178c98930 ("drm/bridge:
> ti-sn65dsi86: Add mystery delay to enable()").  Specifically the
> reason we needed that mystery delay is that we weren't paying
> attention to HPD.
> 
> Looking at the datasheet for the same panel that was tested for the
> original commit, I see there's a timing "t3" that times from power on
> to the aux channel being operational.  This time is specced as 0 - 200
> ms.  The datasheet says that the aux channel is operational at exactly
> the same time that HPD is asserted.
> 
> Scoping the signals on this board showed that HPD was asserted 84 ms
> after power was asserted.  That very closely matches the magic 70 ms
> delay that we had.  ...and actually, in my esting the 70 ms wasn't
> quite enough of a delay and some percentage of the time the display
> didn't come up until I bumped it to 100 ms.
> 
> To solve this, we tried to hook up the HPD signal in the bridge.
> ...but in doing so we found that that the bridge didn't report that
> HPD was asserted until ~280 ms after we powered it (!).  This is
> explained by looking at the sn65dsi86 datasheet section "8.4.5.1 HPD
> (Hot Plug/Unplug Detection)".  Reading there we see that the bridge
> isn't even intended to report HPD until 100 ms after it's asserted.
> ...but that would have left us at 184 ms.  The extra 100 ms
> (presumably) comes from this part in the datasheet:
> 
>> The HPD state machine operates off an internal ring oscillator. The
>> ring oscillator frequency will vary [ ... ]. The min/max range in
>> the HPD State Diagram refers to the possible times based off
>> variation in the ring oscillator frequency.
> 
> Given that the 280 ms we'll end up delaying if we hook up HPD is
> _slower_ than the 200 ms we could just hardcode, for now we'll solve
> the problem by just allowing boards to hardcode a value.  If someone
> using this part finds that they can get things to work more quickly by
> actually hooking up HPD that can always be a future patch.
> 
> One last note is that I tried to solve this through another way: In
> ti_sn_bridge_enable() I tried to use various combinations of
> dp_dpcd_writeb() and dp_dpcd_readb() to detect when the aux channel
> was up.  In theory that would let me detect _exactly_ when I could
> continue and do link training.  Unfortunately even if I did an aux
> transfer w/out waiting I couldn't see any errors.  Possibly I could
> keep looping over link training until it came back with success, but
> that seemed a little overly hacky to me.
> 

Thanks for very detailed explanation.

> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 45 ++++++++++++++++++++++-----
>  1 file changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index f8a931cf3665..5deed667480c 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -93,6 +93,7 @@ struct ti_sn_bridge {
>  	struct clk			*refclk;
>  	struct drm_panel		*panel;
>  	struct gpio_desc		*enable_gpio;
> +	int				panel_hpd_delay_ms;
>  	struct regulator_bulk_data	supplies[SN_REGULATOR_SUPPLY_NUM];
>  };
> 
> @@ -459,16 +460,37 @@ static void ti_sn_bridge_enable(struct drm_bridge 
> *bridge)
>  	int ret;
> 
>  	/*
> -	 * FIXME:
> -	 * This 70ms was found necessary by experimentation. If it's not
> -	 * present, link training fails. It seems like it can go anywhere 
> from
> -	 * pre_enable() up to semi-auto link training initiation below.
> +	 * The timing diagram of some eDP panels says that you're supposed to
> +	 * wait for HPD to be asserted before the aux channel is operational.
>  	 *
> -	 * Neither the datasheet for the bridge nor the panel tested mention 
> a
> -	 * delay of this magnitude in the timing requirements. So for now, 
> add
> -	 * the mystery delay until someone figures out a better fix.
> +	 * While we could configure the bridge to report the HPD signal to us
> +	 * and add a delay here until the HPD is asserted, it turns out 
> that's
> +	 * slower than just hardcoding the max delay from the panel in some
> +	 * cases.  Why?
> +	 *
> +	 * The sn65dsi86 datasheet says that it only reports the debounced
> +	 * HPD signal to software.  It will tell software about HPD assertion
> +	 * as quickly as 100 ms after it's asserted, but sometimes it might
> +	 * take 400 ms because it's timed with a very inaccurate ring
> +	 * oscillator.  In practice it was measured at 200 ms on at least
> +	 * one system.
> +	 *
> +	 * On a particular panel, HPD was asserted 84 ms after power was 
> given.
> +	 * This same panel specified that HPD would always be asserted within
> +	 * 200 ms of applying power.  Thus on this panel with the measured
> +	 * 84 ms to assert HPD + the 200 ms measured debounce we'd wait 284 
> ms
> +	 * which is 84 ms longer than just hardcoding the sleep.
> +	 *
> +	 * For now we don't know of any cases where paying attention to HPD
> +	 * is better than hardcoding the value.  Thus for now only support 
> the
> +	 * hardcoded delay and print a warning if it wasn't specified.  Later
> +	 * one could imagine improving the driver to enable HPD support if
> +	 * panel-hpd-delay-ms wasn't specified in the device tree.
>  	 */
> -	msleep(70);
> +	if (pdata->panel_hpd_delay_ms >= 0)

Is Zero a valid option here, msleep(0) ?


> +		msleep(pdata->panel_hpd_delay_ms);
> +	else
> +		DRM_WARN("HPD not supported; consider a hardcoded delay\n");
> 
>  	/* DSI_A lane config */
>  	val = CHA_DSI_LANES(4 - pdata->dsi->lanes);
> @@ -656,6 +678,7 @@ static int ti_sn_bridge_probe(struct i2c_client 
> *client,
>  {
>  	struct ti_sn_bridge *pdata;
>  	int ret;
> +	u32 val;
> 
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>  		DRM_ERROR("device doesn't support I2C\n");
> @@ -712,6 +735,12 @@ static int ti_sn_bridge_probe(struct i2c_client 
> *client,
>  	if (ret)
>  		return ret;
> 
> +	if (!of_property_read_u32(pdata->dev->of_node,
> +				  "ti,panel-hpd-delay-ms", &val))
> +		pdata->panel_hpd_delay_ms = val;
> +	else
> +		pdata->panel_hpd_delay_ms = -1;

Same comment as above.
> +
>  	pm_runtime_enable(pdata->dev);
> 
>  	i2c_set_clientdata(client, pdata);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi86: Allow DT to set "HPD delay"

[spanda]

On 2018-10-20 01:49, Douglas Anderson wrote:
> Let's solve the mystery of commit bf1178c98930 ("drm/bridge:
> ti-sn65dsi86: Add mystery delay to enable()").  Specifically the
> reason we needed that mystery delay is that we weren't paying
> attention to HPD.
> 
> Looking at the datasheet for the same panel that was tested for the
> original commit, I see there's a timing "t3" that times from power on
> to the aux channel being operational.  This time is specced as 0 - 200
> ms.  The datasheet says that the aux channel is operational at exactly
> the same time that HPD is asserted.
> 
> Scoping the signals on this board showed that HPD was asserted 84 ms
> after power was asserted.  That very closely matches the magic 70 ms
> delay that we had.  ...and actually, in my esting the 70 ms wasn't
> quite enough of a delay and some percentage of the time the display
> didn't come up until I bumped it to 100 ms.
> 
> To solve this, we tried to hook up the HPD signal in the bridge.
> ...but in doing so we found that that the bridge didn't report that
> HPD was asserted until ~280 ms after we powered it (!).  This is
> explained by looking at the sn65dsi86 datasheet section "8.4.5.1 HPD
> (Hot Plug/Unplug Detection)".  Reading there we see that the bridge
> isn't even intended to report HPD until 100 ms after it's asserted.
> ...but that would have left us at 184 ms.  The extra 100 ms
> (presumably) comes from this part in the datasheet:
> 
>> The HPD state machine operates off an internal ring oscillator. The
>> ring oscillator frequency will vary [ ... ]. The min/max range in
>> the HPD State Diagram refers to the possible times based off
>> variation in the ring oscillator frequency.
> 
> Given that the 280 ms we'll end up delaying if we hook up HPD is
> _slower_ than the 200 ms we could just hardcode, for now we'll solve
> the problem by just allowing boards to hardcode a value.  If someone
> using this part finds that they can get things to work more quickly by
> actually hooking up HPD that can always be a future patch.
> 
> One last note is that I tried to solve this through another way: In
> ti_sn_bridge_enable() I tried to use various combinations of
> dp_dpcd_writeb() and dp_dpcd_readb() to detect when the aux channel
> was up.  In theory that would let me detect _exactly_ when I could
> continue and do link training.  Unfortunately even if I did an aux
> transfer w/out waiting I couldn't see any errors.  Possibly I could
> keep looping over link training until it came back with success, but
> that seemed a little overly hacky to me.
> 

Thanks for very detailed explanation.

> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 45 ++++++++++++++++++++++-----
>  1 file changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index f8a931cf3665..5deed667480c 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -93,6 +93,7 @@ struct ti_sn_bridge {
>  	struct clk			*refclk;
>  	struct drm_panel		*panel;
>  	struct gpio_desc		*enable_gpio;
> +	int				panel_hpd_delay_ms;
>  	struct regulator_bulk_data	supplies[SN_REGULATOR_SUPPLY_NUM];
>  };
> 
> @@ -459,16 +460,37 @@ static void ti_sn_bridge_enable(struct drm_bridge 
> *bridge)
>  	int ret;
> 
>  	/*
> -	 * FIXME:
> -	 * This 70ms was found necessary by experimentation. If it's not
> -	 * present, link training fails. It seems like it can go anywhere 
> from
> -	 * pre_enable() up to semi-auto link training initiation below.
> +	 * The timing diagram of some eDP panels says that you're supposed to
> +	 * wait for HPD to be asserted before the aux channel is operational.
>  	 *
> -	 * Neither the datasheet for the bridge nor the panel tested mention 
> a
> -	 * delay of this magnitude in the timing requirements. So for now, 
> add
> -	 * the mystery delay until someone figures out a better fix.
> +	 * While we could configure the bridge to report the HPD signal to us
> +	 * and add a delay here until the HPD is asserted, it turns out 
> that's
> +	 * slower than just hardcoding the max delay from the panel in some
> +	 * cases.  Why?
> +	 *
> +	 * The sn65dsi86 datasheet says that it only reports the debounced
> +	 * HPD signal to software.  It will tell software about HPD assertion
> +	 * as quickly as 100 ms after it's asserted, but sometimes it might
> +	 * take 400 ms because it's timed with a very inaccurate ring
> +	 * oscillator.  In practice it was measured at 200 ms on at least
> +	 * one system.
> +	 *
> +	 * On a particular panel, HPD was asserted 84 ms after power was 
> given.
> +	 * This same panel specified that HPD would always be asserted within
> +	 * 200 ms of applying power.  Thus on this panel with the measured
> +	 * 84 ms to assert HPD + the 200 ms measured debounce we'd wait 284 
> ms
> +	 * which is 84 ms longer than just hardcoding the sleep.
> +	 *
> +	 * For now we don't know of any cases where paying attention to HPD
> +	 * is better than hardcoding the value.  Thus for now only support 
> the
> +	 * hardcoded delay and print a warning if it wasn't specified.  Later
> +	 * one could imagine improving the driver to enable HPD support if
> +	 * panel-hpd-delay-ms wasn't specified in the device tree.
>  	 */
> -	msleep(70);
> +	if (pdata->panel_hpd_delay_ms >= 0)

Is Zero a valid option here, msleep(0) ?


> +		msleep(pdata->panel_hpd_delay_ms);
> +	else
> +		DRM_WARN("HPD not supported; consider a hardcoded delay\n");
> 
>  	/* DSI_A lane config */
>  	val = CHA_DSI_LANES(4 - pdata->dsi->lanes);
> @@ -656,6 +678,7 @@ static int ti_sn_bridge_probe(struct i2c_client 
> *client,
>  {
>  	struct ti_sn_bridge *pdata;
>  	int ret;
> +	u32 val;
> 
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>  		DRM_ERROR("device doesn't support I2C\n");
> @@ -712,6 +735,12 @@ static int ti_sn_bridge_probe(struct i2c_client 
> *client,
>  	if (ret)
>  		return ret;
> 
> +	if (!of_property_read_u32(pdata->dev->of_node,
> +				  "ti,panel-hpd-delay-ms", &val))
> +		pdata->panel_hpd_delay_ms = val;
> +	else
> +		pdata->panel_hpd_delay_ms = -1;

Same comment as above.
> +
>  	pm_runtime_enable(pdata->dev);
> 
>  	i2c_set_clientdata(client, pdata);

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi86: Allow DT to set "HPD delay"

[Doug Anderson]

Hi,

On Wed, Oct 24, 2018 at 11:32 AM <spanda@codeaurora.org> wrote:
>
> On 2018-10-20 01:49, Douglas Anderson wrote:
> > Let's solve the mystery of commit bf1178c98930 ("drm/bridge:
> > ti-sn65dsi86: Add mystery delay to enable()").  Specifically the
> > reason we needed that mystery delay is that we weren't paying
> > attention to HPD.
> >
> > Looking at the datasheet for the same panel that was tested for the
> > original commit, I see there's a timing "t3" that times from power on
> > to the aux channel being operational.  This time is specced as 0 - 200
> > ms.  The datasheet says that the aux channel is operational at exactly
> > the same time that HPD is asserted.
> >
> > Scoping the signals on this board showed that HPD was asserted 84 ms
> > after power was asserted.  That very closely matches the magic 70 ms
> > delay that we had.  ...and actually, in my esting the 70 ms wasn't
> > quite enough of a delay and some percentage of the time the display
> > didn't come up until I bumped it to 100 ms.
> >
> > To solve this, we tried to hook up the HPD signal in the bridge.
> > ...but in doing so we found that that the bridge didn't report that
> > HPD was asserted until ~280 ms after we powered it (!).  This is
> > explained by looking at the sn65dsi86 datasheet section "8.4.5.1 HPD
> > (Hot Plug/Unplug Detection)".  Reading there we see that the bridge
> > isn't even intended to report HPD until 100 ms after it's asserted.
> > ...but that would have left us at 184 ms.  The extra 100 ms
> > (presumably) comes from this part in the datasheet:
> >
> >> The HPD state machine operates off an internal ring oscillator. The
> >> ring oscillator frequency will vary [ ... ]. The min/max range in
> >> the HPD State Diagram refers to the possible times based off
> >> variation in the ring oscillator frequency.
> >
> > Given that the 280 ms we'll end up delaying if we hook up HPD is
> > _slower_ than the 200 ms we could just hardcode, for now we'll solve
> > the problem by just allowing boards to hardcode a value.  If someone
> > using this part finds that they can get things to work more quickly by
> > actually hooking up HPD that can always be a future patch.
> >
> > One last note is that I tried to solve this through another way: In
> > ti_sn_bridge_enable() I tried to use various combinations of
> > dp_dpcd_writeb() and dp_dpcd_readb() to detect when the aux channel
> > was up.  In theory that would let me detect _exactly_ when I could
> > continue and do link training.  Unfortunately even if I did an aux
> > transfer w/out waiting I couldn't see any errors.  Possibly I could
> > keep looping over link training until it came back with success, but
> > that seemed a little overly hacky to me.
> >
>
> Thanks for very detailed explanation.

Note: I already replied to myself and left breadcrumbs, but please
assume that ${SUBJECT} patch is abandoned and see my new series that
moves this delay into the panel.

-Doug