[PATCHv2,10/22] drm/bridge: tc358767: add link disable function

Message ID 20190326103146.24795-11-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.
Currently we have tc_main_link_setup(), which configures and enabled the
link, but we have no counter-part for disabling the link.

Add tc_main_link_disable, and rename tc_main_link_setup to
tc_main_link_enable.

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

Comments

Andrzej Hajda April 15, 2019, 8:36 a.m. | #1
On 26.03.2019 11:31, Tomi Valkeinen wrote:
> Currently we have tc_main_link_setup(), which configures and enabled the
enables
> link, but we have no counter-part for disabling the link.
>
> Add tc_main_link_disable, and rename tc_main_link_setup to
> tc_main_link_enable.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index f8039149a4e8..fe5fd7db0247 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -822,7 +822,7 @@ static int tc_link_training(struct tc_data *tc, int pattern)
>  	return ret;
>  }
>  
> -static int tc_main_link_setup(struct tc_data *tc)
> +static int tc_main_link_enable(struct tc_data *tc)
>  {
>  	struct drm_dp_aux *aux = &tc->aux;
>  	struct device *dev = tc->dev;
> @@ -837,6 +837,8 @@ static int tc_main_link_setup(struct tc_data *tc)
>  	if (!tc->mode)
>  		return -EINVAL;
>  
> +	dev_dbg(tc->dev, "link enable\n");
> +
>  	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,
> @@ -1005,6 +1007,20 @@ static int tc_main_link_setup(struct tc_data *tc)
>  	return ret;
>  }
>  
> +static int tc_main_link_disable(struct tc_data *tc)
> +{
> +	int ret;
> +
> +	dev_dbg(tc->dev, "link disable\n");
> +
> +	tc_write(DP0_SRCCTRL, 0);
> +	tc_write(DP0CTL, 0);


The same register is set in stream_disable, is it correct? Looks
suspicious, disabling stream and link should be different things.

Regards

Andrzej


> +
> +	return 0;
> +err:
> +	return ret;
> +}
> +
>  static int tc_stream_enable(struct tc_data *tc)
>  {
>  	int ret;
> @@ -1083,15 +1099,16 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	int ret;
>  
> -	ret = tc_main_link_setup(tc);
> +	ret = tc_main_link_enable(tc);
>  	if (ret < 0) {
> -		dev_err(tc->dev, "main link setup error: %d\n", ret);
> +		dev_err(tc->dev, "main link enable error: %d\n", ret);
>  		return;
>  	}
>  
>  	ret = tc_stream_enable(tc);
>  	if (ret < 0) {
>  		dev_err(tc->dev, "main link stream start error: %d\n", ret);
> +		tc_main_link_disable(tc);
>  		return;
>  	}
>  
> @@ -1108,6 +1125,10 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
>  	ret = tc_stream_disable(tc);
>  	if (ret < 0)
>  		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
> +
> +	ret = tc_main_link_disable(tc);
> +	if (ret < 0)
> +		dev_err(tc->dev, "main link disable error: %d\n", ret);
>  }
>  
>  static void tc_bridge_post_disable(struct drm_bridge *bridge)
Tomi Valkeinen April 15, 2019, 11:39 a.m. | #2
On 15/04/2019 11:36, Andrzej Hajda wrote:

>> +static int tc_main_link_disable(struct tc_data *tc)
>> +{
>> +	int ret;
>> +
>> +	dev_dbg(tc->dev, "link disable\n");
>> +
>> +	tc_write(DP0_SRCCTRL, 0);
>> +	tc_write(DP0CTL, 0);
> 
> 
> The same register is set in stream_disable, is it correct? Looks
> suspicious, disabling stream and link should be different things.

Good point. It's probably not correct. With these patches, the link and
stream are always enabled and disabled together, so it shouldn't cause
any problems at the moment.

During my debugging and development, at some point I had a version where
I enabled the link right away when we got HPD high, mostly to help my
debugging. That's why I separated link and stream setup (although I
think that's a logical design in any case).

However, I never did try disabling only stream, and keeping the link up,
so it may well be non-functional. Or, well, it clearly is
non-functional, as we disable the link (DP0CTL) in tc_stream_disable()...

So this is mostly about just adding the architecture to have separate
link/stream handling, but the functionality is not there. I should
perhaps add a comment to the code to point this out.

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

Thank you for the patch.

On Mon, Apr 15, 2019 at 02:39:21PM +0300, Tomi Valkeinen wrote:
> On 15/04/2019 11:36, Andrzej Hajda wrote:
> 
> >> +static int tc_main_link_disable(struct tc_data *tc)
> >> +{
> >> +	int ret;
> >> +
> >> +	dev_dbg(tc->dev, "link disable\n");
> >> +
> >> +	tc_write(DP0_SRCCTRL, 0);
> >> +	tc_write(DP0CTL, 0);
> > 
> > The same register is set in stream_disable, is it correct? Looks
> > suspicious, disabling stream and link should be different things.
> 
> Good point. It's probably not correct. With these patches, the link and
> stream are always enabled and disabled together, so it shouldn't cause
> any problems at the moment.
> 
> During my debugging and development, at some point I had a version where
> I enabled the link right away when we got HPD high, mostly to help my
> debugging. That's why I separated link and stream setup (although I
> think that's a logical design in any case).
> 
> However, I never did try disabling only stream, and keeping the link up,
> so it may well be non-functional. Or, well, it clearly is
> non-functional, as we disable the link (DP0CTL) in tc_stream_disable()...
> 
> So this is mostly about just adding the architecture to have separate
> link/stream handling, but the functionality is not there. I should
> perhaps add a comment to the code to point this out.

This makes me a bit uneasy about the rework, as the
tc_main_link_enable() function doesn't enable the link (it doesn't set
the DP_EN bit in DP0CTL), and stream disabling separately from link
disabling is broken. Is this fixable ?
Tomi Valkeinen May 3, 2019, 9:36 a.m. | #4
On 21/04/2019 00:39, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Mon, Apr 15, 2019 at 02:39:21PM +0300, Tomi Valkeinen wrote:
>> On 15/04/2019 11:36, Andrzej Hajda wrote:
>>
>>>> +static int tc_main_link_disable(struct tc_data *tc)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	dev_dbg(tc->dev, "link disable\n");
>>>> +
>>>> +	tc_write(DP0_SRCCTRL, 0);
>>>> +	tc_write(DP0CTL, 0);
>>>
>>> The same register is set in stream_disable, is it correct? Looks
>>> suspicious, disabling stream and link should be different things.
>>
>> Good point. It's probably not correct. With these patches, the link and
>> stream are always enabled and disabled together, so it shouldn't cause
>> any problems at the moment.
>>
>> During my debugging and development, at some point I had a version where
>> I enabled the link right away when we got HPD high, mostly to help my
>> debugging. That's why I separated link and stream setup (although I
>> think that's a logical design in any case).
>>
>> However, I never did try disabling only stream, and keeping the link up,
>> so it may well be non-functional. Or, well, it clearly is
>> non-functional, as we disable the link (DP0CTL) in tc_stream_disable()...
>>
>> So this is mostly about just adding the architecture to have separate
>> link/stream handling, but the functionality is not there. I should
>> perhaps add a comment to the code to point this out.
> 
> This makes me a bit uneasy about the rework, as the
> tc_main_link_enable() function doesn't enable the link (it doesn't set
> the DP_EN bit in DP0CTL), and stream disabling separately from link
> disabling is broken. Is this fixable ?

tc_main_link_enable() does set DP_EN.

It's true that managing the link and stream separately isn't working,
but it wasn't there earlier either, and it still isn't, as the driver
never enables/disables the link/stream separately. We need more code and
logic to manage them separately, and this series just starts the work by
trying to organize the code better.

I'll look at tc_stream_disable(), as tc_write(DP0CTL, 0) there does not
look correct. Probably we can just clear VID_EN.

Afaics, this series should not introduce any issues, but fixes many
issues that were present. There's lots more to do, and there's another
series from Andrey that cleans up many things that I didn't touch in
this series. And after Andrey's cleanups, I think we can then start
looking at adding more logic like the separate link/stream enabling if
needed.

 Tomi

Patch

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index f8039149a4e8..fe5fd7db0247 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -822,7 +822,7 @@  static int tc_link_training(struct tc_data *tc, int pattern)
 	return ret;
 }
 
-static int tc_main_link_setup(struct tc_data *tc)
+static int tc_main_link_enable(struct tc_data *tc)
 {
 	struct drm_dp_aux *aux = &tc->aux;
 	struct device *dev = tc->dev;
@@ -837,6 +837,8 @@  static int tc_main_link_setup(struct tc_data *tc)
 	if (!tc->mode)
 		return -EINVAL;
 
+	dev_dbg(tc->dev, "link enable\n");
+
 	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,
@@ -1005,6 +1007,20 @@  static int tc_main_link_setup(struct tc_data *tc)
 	return ret;
 }
 
+static int tc_main_link_disable(struct tc_data *tc)
+{
+	int ret;
+
+	dev_dbg(tc->dev, "link disable\n");
+
+	tc_write(DP0_SRCCTRL, 0);
+	tc_write(DP0CTL, 0);
+
+	return 0;
+err:
+	return ret;
+}
+
 static int tc_stream_enable(struct tc_data *tc)
 {
 	int ret;
@@ -1083,15 +1099,16 @@  static void tc_bridge_enable(struct drm_bridge *bridge)
 	struct tc_data *tc = bridge_to_tc(bridge);
 	int ret;
 
-	ret = tc_main_link_setup(tc);
+	ret = tc_main_link_enable(tc);
 	if (ret < 0) {
-		dev_err(tc->dev, "main link setup error: %d\n", ret);
+		dev_err(tc->dev, "main link enable error: %d\n", ret);
 		return;
 	}
 
 	ret = tc_stream_enable(tc);
 	if (ret < 0) {
 		dev_err(tc->dev, "main link stream start error: %d\n", ret);
+		tc_main_link_disable(tc);
 		return;
 	}
 
@@ -1108,6 +1125,10 @@  static void tc_bridge_disable(struct drm_bridge *bridge)
 	ret = tc_stream_disable(tc);
 	if (ret < 0)
 		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
+
+	ret = tc_main_link_disable(tc);
+	if (ret < 0)
+		dev_err(tc->dev, "main link disable error: %d\n", ret);
 }
 
 static void tc_bridge_post_disable(struct drm_bridge *bridge)