| Message ID | 20190326103146.24795-11-tomi.valkeinen@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/bridge: tc358767: DP support | expand |
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)
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
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 ?
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
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)
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(-)