Re: [PATCH] drm/bridge: tc358767: Expose test mode functionality via debugfs
From: Tomi Valkeinen
Date: Tue Aug 27 2019 - 01:46:36 EST
Hi,
On 26/08/2019 21:25, Andrey Smirnov wrote:
Presently, the driver code artificially limits test pattern mode to a
single pattern with fixed color selection. It being a kernel module
parameter makes switching "test patter" <-> "proper output" modes
on-the-fly clunky and outright impossible if the driver is built into
the kernel.
To improve the situation a bit, convert current test pattern code to
use debugfs instead by exposing "TestCtl" register. This way old
"tc_test_pattern=1" functionality can be emulated via:
echo -n 0x78146312 > tstctl
and switch back to regular mode can be done with:
echo -n 0x78146310 > tstctl
It might be worth explaining the format in the commit msg or in a
comment in the driver.
Note that switching to any of the test patterns, will NOT trigger link
re-establishment whereas switching to normal operation WILL. This is
done so:
a) we can isolate and verify (e)DP link functionality by switching to
one of the test patters
b) trigger a link re-establishment by switching back to normal mode
Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
Cc: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
Cc: Cory Tusar <cory.tusar@xxxxxxxx>
Cc: Chris Healy <cphealy@xxxxxxxxx>
Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
Cc: linux-kernel@xxxxxxxxxxxxxxx
---
drivers/gpu/drm/bridge/tc358767.c | 137 ++++++++++++++++++++++--------
1 file changed, 101 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 6308d93ad91d..7a795b613ed0 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -17,6 +17,7 @@
#include <linux/bitfield.h>
#include <linux/clk.h>
+#include <linux/debugfs.h>
#include <linux/device.h>
#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
@@ -222,11 +223,10 @@
#define COLOR_B GENMASK(15, 8)
#define ENI2CFILTER BIT(4)
#define COLOR_BAR_MODE GENMASK(1, 0)
+#define COLOR_BAR_MODE_NORMAL 0
#define COLOR_BAR_MODE_BARS 2
-#define PLL_DBG 0x0a04
-static bool tc_test_pattern;
-module_param_named(test, tc_test_pattern, bool, 0644);
+#define PLL_DBG 0x0a04
struct tc_edp_link {
struct drm_dp_link base;
@@ -789,16 +789,6 @@ static int tc_set_video_mode(struct tc_data *tc,
if (ret)
return ret;
- /* Test pattern settings */
- ret = regmap_write(tc->regmap, TSTCTL,
- FIELD_PREP(COLOR_R, 120) |
- FIELD_PREP(COLOR_G, 20) |
- FIELD_PREP(COLOR_B, 99) |
- ENI2CFILTER |
- FIELD_PREP(COLOR_BAR_MODE, COLOR_BAR_MODE_BARS));
- if (ret)
- return ret;
-
/* DP Main Stream Attributes */
vid_sync_dly = hsync_len + left_margin + mode->hdisplay;
ret = regmap_write(tc->regmap, DP0_VIDSYNCDELAY,
@@ -1150,14 +1140,6 @@ static int tc_stream_enable(struct tc_data *tc)
dev_dbg(tc->dev, "enable video stream\n");
- /* PXL PLL setup */
- if (tc_test_pattern) {
- ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
- 1000 * tc->mode.clock);
- if (ret)
- return ret;
- }
-
ret = tc_set_video_mode(tc, &tc->mode);
if (ret)
return ret;
@@ -1186,12 +1168,8 @@ static int tc_stream_enable(struct tc_data *tc)
if (ret)
return ret;
/* Set input interface */
- value = DP0_AUDSRC_NO_INPUT;
- if (tc_test_pattern)
- value |= DP0_VIDSRC_COLOR_BAR;
- else
- value |= DP0_VIDSRC_DPI_RX;
- ret = regmap_write(tc->regmap, SYSCTRL, value);
+ ret = regmap_write(tc->regmap, SYSCTRL,
+ DP0_AUDSRC_NO_INPUT | DP0_VIDSRC_DPI_RX);
if (ret)
return ret;
@@ -1220,39 +1198,44 @@ static void tc_bridge_pre_enable(struct drm_bridge *bridge)
drm_panel_prepare(tc->panel);
}
-static void tc_bridge_enable(struct drm_bridge *bridge)
+static int __tc_bridge_enable(struct tc_data *tc)
{
- struct tc_data *tc = bridge_to_tc(bridge);
int ret;
ret = tc_get_display_props(tc);
if (ret < 0) {
dev_err(tc->dev, "failed to read display props: %d\n", ret);
- return;
+ return ret;
}
ret = tc_main_link_enable(tc);
if (ret < 0) {
dev_err(tc->dev, "main link enable error: %d\n", ret);
- return;
+ return ret;
}
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;
}
- drm_panel_enable(tc->panel);
+ return ret;
}
Maybe it's just me, but I rather have the last if() block do a "return
ret"; and have "return 0;" at the end of the function to keep all the if
blocks consistent.
-static void tc_bridge_disable(struct drm_bridge *bridge)
+static void tc_bridge_enable(struct drm_bridge *bridge)
{
struct tc_data *tc = bridge_to_tc(bridge);
- int ret;
- drm_panel_disable(tc->panel);
+ if (__tc_bridge_enable(tc) < 0)
+ return;
+
+ drm_panel_enable(tc->panel);
+}
+
+static int __tc_bridge_disable(struct tc_data *tc)
+{
+ int ret;
ret = tc_stream_disable(tc);
if (ret < 0)
@@ -1261,6 +1244,16 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
ret = tc_main_link_disable(tc);
if (ret < 0)
dev_err(tc->dev, "main link disable error: %d\n", ret);
+
+ return ret;
+}
+
+static void tc_bridge_disable(struct drm_bridge *bridge)
+{
+ struct tc_data *tc = bridge_to_tc(bridge);
+
+ drm_panel_disable(tc->panel);
+ __tc_bridge_disable(tc);
}
Maybe have this enable/disable change as a separate patch?
static void tc_bridge_post_disable(struct drm_bridge *bridge)
@@ -1372,6 +1365,77 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne
return connector_status_disconnected;
}
+static int tc_tstctl_set(void *data, u64 val)
+{
+ struct tc_data *tc = data;
+ int ret;
+
+ if (FIELD_GET(COLOR_BAR_MODE, val) == COLOR_BAR_MODE_NORMAL) {
+ ret = regmap_write(tc->regmap, SYSCTRL, DP0_VIDSRC_DPI_RX);
+ if (ret) {
+ dev_err(tc->dev,
+ "failed to select dpi video stream\n");
+ return ret;
+ }
+
+ ret = regmap_write(tc->regmap, TSTCTL, val | ENI2CFILTER);
+ if (ret) {
+ dev_err(tc->dev, "failed to set TSTCTL\n");
+ return ret;
+ }
+
+ ret = tc_pxl_pll_dis(tc);
+ if (ret) {
+ dev_err(tc->dev, "failed to disable PLL\n");
+ return ret;
+ }
+
+ /*
+ * Re-establish DP link
+ */
+ ret = __tc_bridge_disable(tc);
+ if (ret)
+ return ret;
+
+ ret = __tc_bridge_enable(tc);
+ if (ret)
+ return ret;
+ } else {
+ ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
+ 1000 * tc->mode.clock);
+ if (ret) {
+ dev_err(tc->dev, "failed to enable PLL\n");
+ return ret;
+ }
+
+ ret = regmap_write(tc->regmap, TSTCTL, val | ENI2CFILTER);
+ if (ret) {
+ dev_err(tc->dev, "failed to set TSTCTL\n");
+ return ret;
+ }
+
+ ret = regmap_write(tc->regmap, SYSCTRL, DP0_VIDSRC_COLOR_BAR);
+ if (ret) {
+ dev_err(tc->dev, "failed to color bar video stream\n");
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(tc_tstctl_fops, NULL, tc_tstctl_set, "%llu\n");
+
+static int tc_late_register(struct drm_connector *connector)
+{
+ if (connector->debugfs_entry)
+ debugfs_create_file_unsafe("tstctl", 0644,
+ connector->debugfs_entry,
+ connector_to_tc(connector),
+ &tc_tstctl_fops);
+ return 0;
+}
I very recently wanted to add quick debugfs functionality to a bridge
too, but as there didn't seem an easy way to do it, I just lazily added
the debugfs files to the debugfs root...
I'm not sure if adding bridge's debugfs files to connector's directory
is a good idea. What if we have two bridges, both have the same debugfs
file? Or even if there's no conflict, there's also no way to know which
bridge a particular debugfs file belongs to.
Maybe we need some DRM infrastructure for bridge debugfs?
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki