[PATCHv2,11/22] drm/bridge: tc358767: ensure DP is disabled before LT

Message ID 20190326103146.24795-12-tomi.valkeinen@ti.com
State Superseded
Headers show
Series
  • drm/bridge: tc358767: DP support
Related show

Commit Message

Tomi Valkeinen March 26, 2019, 10:31 a.m.
Link training will sometimes fail if the DP link is, for some whatever
reason, enabled when tc_main_link_enable() is called.

Ensure that the DP link is disabled by setting DP0CTL to 0 as the first
thing.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Andrzej Hajda April 15, 2019, 8:49 a.m. | #1
On 26.03.2019 11:31, Tomi Valkeinen wrote:
> Link training will sometimes fail if the DP link is, for some whatever
> reason, enabled when tc_main_link_enable() is called.


Only tc_stream_enable enables it, does it mean that link training can
happen after tc_stream_enable?

It suggests that driver/device preforms strange things, is it true? Or
just overprotection?


Regards

Andrzej


>
> Ensure that the DP link is disabled by setting DP0CTL to 0 as the first
> thing.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index fe5fd7db0247..f628575c9de9 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -839,6 +839,8 @@ static int tc_main_link_enable(struct tc_data *tc)
>  
>  	dev_dbg(tc->dev, "link enable\n");
>  
> +	tc_write(DP0CTL, 0);
> +



>  	tc_write(DP0_SRCCTRL, tc_srcctrl(tc));
>  	/* SSCG and BW27 on DP1 must be set to the same as on DP0 */
>  	tc_write(DP1_SRCCTRL,
Tomi Valkeinen April 15, 2019, 11:26 a.m. | #2
On 15/04/2019 11:49, Andrzej Hajda wrote:
> On 26.03.2019 11:31, Tomi Valkeinen wrote:
>> Link training will sometimes fail if the DP link is, for some whatever
>> reason, enabled when tc_main_link_enable() is called.
> 
> 
> Only tc_stream_enable enables it, does it mean that link training can
> happen after tc_stream_enable?
> 
> It suggests that driver/device preforms strange things, is it true? Or
> just overprotection?

Just protection. I did try all kinds of things when trying to get the
link setup stable and having DP0CTL enabled before link training was one
of the problems I encountered.

In theory DP0CTL should always be disabled when we call
tc_main_link_enable, but I thought it best leave it there in case we
accidentally leave DP0CTL enabled via some error path or such.

Maybe we should have a WARN there if DP0CTL is enabled (and then clear
it), so that we might find those error cases.

 Tomi
Laurent Pinchart April 20, 2019, 9:41 p.m. | #3
Hi Tomi,

On Mon, Apr 15, 2019 at 02:26:20PM +0300, Tomi Valkeinen wrote:
> On 15/04/2019 11:49, Andrzej Hajda wrote:
> > On 26.03.2019 11:31, Tomi Valkeinen wrote:
> >> Link training will sometimes fail if the DP link is, for some whatever
> >> reason, enabled when tc_main_link_enable() is called.
> > 
> > Only tc_stream_enable enables it, does it mean that link training can
> > happen after tc_stream_enable?
> > 
> > It suggests that driver/device preforms strange things, is it true? Or
> > just overprotection?
> 
> Just protection. I did try all kinds of things when trying to get the
> link setup stable and having DP0CTL enabled before link training was one
> of the problems I encountered.
> 
> In theory DP0CTL should always be disabled when we call
> tc_main_link_enable, but I thought it best leave it there in case we
> accidentally leave DP0CTL enabled via some error path or such.
> 
> Maybe we should have a WARN there if DP0CTL is enabled (and then clear
> it), so that we might find those error cases.

I'd prefer a warning, as incorrect error paths (or such) could also
create lots of other issues. I don't think we should protect against one
particular issue that is supposed never to happen and then consider that
we're safe.

Patch

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index fe5fd7db0247..f628575c9de9 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -839,6 +839,8 @@  static int tc_main_link_enable(struct tc_data *tc)
 
 	dev_dbg(tc->dev, "link enable\n");
 
+	tc_write(DP0CTL, 0);
+
 	tc_write(DP0_SRCCTRL, tc_srcctrl(tc));
 	/* SSCG and BW27 on DP1 must be set to the same as on DP0 */
 	tc_write(DP1_SRCCTRL,