On Mon, Oct 22, 2018 at 01:46:37PM -0700, 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 testing 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 (presumably 84 ms would
> have worked too).
>
> 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 hardcoding a 200 ms delay in the panel driver
> using the patch in this series ("drm/panel: simple: Support panels
> with HPD where HPD isn't connected").
>
> If we later find a panel that needs to use this bridge where we need
> HPD then we'll have to come up with some new code to handle it. Given
> the silly debouncing in the bridge chip, though, it seems unlikely.
>
> 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 <diand...@chromium.org>
Awesome commit message and comment, thanks for solving the mystery!
Reviewed-by: Sean Paul <s...@poorly.run>
> ---
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 29 +++++++++++++++------------
> 1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index f8a931cf3665..680566d97adc 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -458,18 +458,6 @@ static void ti_sn_bridge_enable(struct drm_bridge
> *bridge)
> unsigned int val;
> 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.
> - *
> - * 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.
> - */
> - msleep(70);
> -
> /* DSI_A lane config */
> val = CHA_DSI_LANES(4 - pdata->dsi->lanes);
> regmap_update_bits(pdata->regmap, SN_DSI_LANES_REG,
> @@ -536,7 +524,22 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge
> *bridge)
> /* configure bridge ref_clk */
> ti_sn_bridge_set_refclk_freq(pdata);
>
> - /* in case drm_panel is connected then HPD is not supported */
> + /*
> + * HPD on this bridge chip is a bit useless. This is an eDP bridge
> + * so the HPD is an internal signal that's only there to signal that
> + * the panel is done powering up. ...but the bridge chip debounces
> + * this signal by between 100 ms and 400 ms (depending on process,
> + * voltage, and temperate--I measured it at about 200 ms). One
> + * particular panel asserted HPD 84 ms after it was powered on meaning
> + * that we saw HPD 284 ms after power on. ...but the same panel said
> + * that instead of looking at HPD you could just hardcode a delay of
> + * 200 ms. We'll assume that the panel driver will have the hardcoded
> + * delay in its prepare and always disable HPD.
> + *
> + * If HPD somehow makes sense on some future panel we'll have to
> + * change this to be conditional on someone specifying that HPD should
> + * be used.
> + */
> regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> HPD_DISABLE);
>
> --
> 2.19.1.568.g152ad8e336-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
