lkml.org 
[lkml]   [2018]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] drm/bridge: ti-sn65dsi86: Allow DT to set "HPD delay"
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);

\
 
 \ /
  Last update: 2018-10-24 20:33    [W:0.066 / U:0.612 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site