* [PATCH 01/10] video: exynos_dp: Change aux transaction failures
From: Sean Paul @ 2012-08-08 3:54 UTC (permalink / raw)
To: linux-fbdev
This patch adds the function name to aux transaction failure messages
so we can tell which transaction is failing. It also changes the level
of Aux Transaction fail messages from error to debug. We retry the
transactions a few times and will report errors if warranted outside of
this function.
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Bernie Thompson <bhthompson@chromium.org>
---
drivers/video/exynos/exynos_dp_reg.c | 21 ++++++++++++++-------
1 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
index ce401c8..a121bed 100644
--- a/drivers/video/exynos/exynos_dp_reg.c
+++ b/drivers/video/exynos/exynos_dp_reg.c
@@ -471,7 +471,8 @@ int exynos_dp_write_byte_to_dpcd(struct exynos_dp_device *dp,
if (retval = 0)
break;
else
- dev_err(dp->dev, "Aux Transaction fail!\n");
+ dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
+ __func__);
}
return retval;
@@ -511,7 +512,8 @@ int exynos_dp_read_byte_from_dpcd(struct exynos_dp_device *dp,
if (retval = 0)
break;
else
- dev_err(dp->dev, "Aux Transaction fail!\n");
+ dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
+ __func__);
}
/* Read data buffer */
@@ -575,7 +577,8 @@ int exynos_dp_write_bytes_to_dpcd(struct exynos_dp_device *dp,
if (retval = 0)
break;
else
- dev_err(dp->dev, "Aux Transaction fail!\n");
+ dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
+ __func__);
}
start_offset += cur_data_count;
@@ -632,7 +635,8 @@ int exynos_dp_read_bytes_from_dpcd(struct exynos_dp_device *dp,
if (retval = 0)
break;
else
- dev_err(dp->dev, "Aux Transaction fail!\n");
+ dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
+ __func__);
}
for (cur_data_idx = 0; cur_data_idx < cur_data_count;
@@ -677,7 +681,7 @@ int exynos_dp_select_i2c_device(struct exynos_dp_device *dp,
/* Start AUX transaction */
retval = exynos_dp_start_aux_transaction(dp);
if (retval != 0)
- dev_err(dp->dev, "Aux Transaction fail!\n");
+ dev_dbg(dp->dev, "%s: Aux Transaction fail!\n", __func__);
return retval;
}
@@ -717,7 +721,8 @@ int exynos_dp_read_byte_from_i2c(struct exynos_dp_device *dp,
if (retval = 0)
break;
else
- dev_err(dp->dev, "Aux Transaction fail!\n");
+ dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
+ __func__);
}
/* Read data */
@@ -777,7 +782,9 @@ int exynos_dp_read_bytes_from_i2c(struct exynos_dp_device *dp,
if (retval = 0)
break;
else
- dev_err(dp->dev, "Aux Transaction fail!\n");
+ dev_dbg(dp->dev,
+ "%s: Aux Transaction fail!\n",
+ __func__);
}
/* Check if Rx sends defer */
reg = readl(dp->reg_base + EXYNOS_DP_AUX_RX_COMM);
--
1.7.7.3
^ permalink raw reply related
* [PATCH 02/10] video: exynos_dp: Check DPCD return codes
From: Sean Paul @ 2012-08-08 3:54 UTC (permalink / raw)
To: linux-fbdev
Add return code checks to the DPCD transactions in the SW link training.
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Mandeep Singh Baines <msb@chromium.org>
---
drivers/video/exynos/exynos_dp_core.c | 65 ++++++++++++++++++++++-----------
1 files changed, 44 insertions(+), 21 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index c6c016a..1836e33 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -258,8 +258,9 @@ static void exynos_dp_set_lane_lane_pre_emphasis(struct exynos_dp_device *dp,
}
}
-static void exynos_dp_link_start(struct exynos_dp_device *dp)
+static int exynos_dp_link_start(struct exynos_dp_device *dp)
{
+ int ret;
u8 buf[5];
int lane;
int lane_count;
@@ -273,8 +274,10 @@ static void exynos_dp_link_start(struct exynos_dp_device *dp)
dp->link_train.cr_loop[lane] = 0;
/* Set sink to D0 (Sink Not Ready) mode. */
- exynos_dp_write_byte_to_dpcd(dp, DPCD_ADDR_SINK_POWER_STATE,
+ ret = exynos_dp_write_byte_to_dpcd(dp, DPCD_ADDR_SINK_POWER_STATE,
DPCD_SET_POWER_STATE_D0);
+ if (ret)
+ return ret;
/* Set link rate and count as you want to establish*/
exynos_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
@@ -283,8 +286,10 @@ static void exynos_dp_link_start(struct exynos_dp_device *dp)
/* Setup RX configuration */
buf[0] = dp->link_train.link_rate;
buf[1] = dp->link_train.lane_count;
- exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_LINK_BW_SET,
+ ret = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_LINK_BW_SET,
2, buf);
+ if (ret)
+ return ret;
/* Set TX pre-emphasis to minimum */
for (lane = 0; lane < lane_count; lane++)
@@ -303,9 +308,13 @@ static void exynos_dp_link_start(struct exynos_dp_device *dp)
for (lane = 0; lane < lane_count; lane++)
buf[lane] = DPCD_PRE_EMPHASIS_PATTERN2_LEVEL0 |
DPCD_VOLTAGE_SWING_PATTERN1_LEVEL0;
- exynos_dp_write_bytes_to_dpcd(dp,
+ ret = exynos_dp_write_bytes_to_dpcd(dp,
DPCD_ADDR_TRAINING_LANE0_SET,
lane_count, buf);
+ if (ret)
+ return ret;
+
+ return ret;
}
static unsigned char exynos_dp_get_lane_status(u8 link_status[6], int lane)
@@ -475,6 +484,7 @@ static int exynos_dp_check_max_cr_loop(struct exynos_dp_device *dp,
static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
{
+ int ret;
u8 data;
u8 link_status[6];
int lane;
@@ -488,8 +498,11 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
usleep_range(100, 101);
- exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
+ ret = exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
6, link_status);
+ if (ret)
+ return ret;
+
lane_count = dp->link_train.lane_count;
if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
@@ -503,18 +516,22 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
buf[0] = DPCD_SCRAMBLING_DISABLED |
DPCD_TRAINING_PATTERN_2;
- exynos_dp_write_byte_to_dpcd(dp,
+ ret = exynos_dp_write_byte_to_dpcd(dp,
DPCD_ADDR_TRAINING_PATTERN_SET,
buf[0]);
+ if (ret)
+ return ret;
for (lane = 0; lane < lane_count; lane++) {
exynos_dp_set_lane_link_training(dp,
dp->link_train.training_lane[lane],
lane);
buf[lane] = dp->link_train.training_lane[lane];
- exynos_dp_write_byte_to_dpcd(dp,
+ ret = exynos_dp_write_byte_to_dpcd(dp,
DPCD_ADDR_TRAINING_LANE0_SET + lane,
buf[lane]);
+ if (ret)
+ return ret;
}
dp->link_train.lt_state = EQUALIZER_TRAINING;
} else {
@@ -551,18 +568,21 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
dp->link_train.training_lane[lane],
lane);
buf[lane] = dp->link_train.training_lane[lane];
- exynos_dp_write_byte_to_dpcd(dp,
+ ret = exynos_dp_write_byte_to_dpcd(dp,
DPCD_ADDR_TRAINING_LANE0_SET + lane,
buf[lane]);
+ if (ret)
+ return ret;
}
}
}
- return 0;
+ return ret;
}
static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
{
+ int ret;
u8 link_status[6];
int lane;
int lane_count;
@@ -573,8 +593,10 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
usleep_range(400, 401);
- exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
+ ret = exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
6, link_status);
+ if (ret)
+ return ret;
lane_count = dp->link_train.lane_count;
if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
@@ -614,9 +636,11 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
dp->link_train.training_lane[lane],
lane);
buf[lane] = dp->link_train.training_lane[lane];
- exynos_dp_write_byte_to_dpcd(dp,
+ ret = exynos_dp_write_byte_to_dpcd(dp,
DPCD_ADDR_TRAINING_LANE0_SET + lane,
buf[lane]);
+ if (ret)
+ return ret;
}
}
}
@@ -624,7 +648,7 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
exynos_dp_reduce_link_rate(dp);
}
- return 0;
+ return ret;
}
static void exynos_dp_get_max_rx_bandwidth(struct exynos_dp_device *dp,
@@ -692,28 +716,25 @@ static void exynos_dp_init_training(struct exynos_dp_device *dp,
static int exynos_dp_sw_link_training(struct exynos_dp_device *dp)
{
- int retval = 0;
- int training_finished;
+ int ret = 0, training_finished = 0;
/* Turn off unnecessary lane */
if (dp->link_train.lane_count = 1)
exynos_dp_set_analog_power_down(dp, CH1_BLOCK, 1);
- training_finished = 0;
-
dp->link_train.lt_state = START;
/* Process here */
- while (!training_finished) {
+ while (!ret && !training_finished) {
switch (dp->link_train.lt_state) {
case START:
- exynos_dp_link_start(dp);
+ ret = exynos_dp_link_start(dp);
break;
case CLOCK_RECOVERY:
- exynos_dp_process_clock_recovery(dp);
+ ret = exynos_dp_process_clock_recovery(dp);
break;
case EQUALIZER_TRAINING:
- exynos_dp_process_equalizer_training(dp);
+ ret = exynos_dp_process_equalizer_training(dp);
break;
case FINISHED:
training_finished = 1;
@@ -722,8 +743,10 @@ static int exynos_dp_sw_link_training(struct exynos_dp_device *dp)
return -EREMOTEIO;
}
}
+ if (ret)
+ dev_err(dp->dev, "eDP link training failed (%d)\n", ret);
- return retval;
+ return ret;
}
static int exynos_dp_set_link_train(struct exynos_dp_device *dp,
--
1.7.7.3
^ permalink raw reply related
* [PATCH 03/10] video: exynos_dp: Clean up SW link training
From: Sean Paul @ 2012-08-08 3:54 UTC (permalink / raw)
To: linux-fbdev
Clean up some of the SW training code to make it more clear and reduce
duplicate code.
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Mandeep Singh Baines <msb@chromium.org>
---
drivers/video/exynos/exynos_dp_core.c | 180 ++++++++++++---------------------
1 files changed, 67 insertions(+), 113 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index 1836e33..3deded2 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -260,10 +260,8 @@ static void exynos_dp_set_lane_lane_pre_emphasis(struct exynos_dp_device *dp,
static int exynos_dp_link_start(struct exynos_dp_device *dp)
{
- int ret;
- u8 buf[5];
- int lane;
- int lane_count;
+ int ret, lane, lane_count;
+ u8 buf[4];
lane_count = dp->link_train.lane_count;
@@ -286,8 +284,7 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
/* Setup RX configuration */
buf[0] = dp->link_train.link_rate;
buf[1] = dp->link_train.lane_count;
- ret = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_LINK_BW_SET,
- 2, buf);
+ ret = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_LINK_BW_SET, 2, buf);
if (ret)
return ret;
@@ -300,16 +297,15 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
exynos_dp_set_training_pattern(dp, TRAINING_PTN1);
/* Set RX training pattern */
- buf[0] = DPCD_SCRAMBLING_DISABLED |
- DPCD_TRAINING_PATTERN_1;
- exynos_dp_write_byte_to_dpcd(dp,
- DPCD_ADDR_TRAINING_PATTERN_SET, buf[0]);
+ ret = exynos_dp_write_byte_to_dpcd(dp, DPCD_ADDR_TRAINING_PATTERN_SET,
+ DPCD_SCRAMBLING_DISABLED | DPCD_TRAINING_PATTERN_1);
+ if (ret)
+ return ret;
for (lane = 0; lane < lane_count; lane++)
buf[lane] = DPCD_PRE_EMPHASIS_PATTERN2_LEVEL0 |
DPCD_VOLTAGE_SWING_PATTERN1_LEVEL0;
- ret = exynos_dp_write_bytes_to_dpcd(dp,
- DPCD_ADDR_TRAINING_LANE0_SET,
+ ret = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_TRAINING_LANE0_SET,
lane_count, buf);
if (ret)
return ret;
@@ -484,67 +480,32 @@ static int exynos_dp_check_max_cr_loop(struct exynos_dp_device *dp,
static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
{
- int ret;
- u8 data;
- u8 link_status[6];
- int lane;
- int lane_count;
- u8 buf[5];
-
- u8 adjust_request[2];
- u8 voltage_swing;
- u8 pre_emphasis;
- u8 training_lane;
+ int ret, lane, lane_count;
+ u8 voltage_swing, pre_emphasis, training_lane, link_status[6];
+ u8 *adjust_request;
usleep_range(100, 101);
- ret = exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
- 6, link_status);
+ ret = exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS, 6,
+ link_status);
if (ret)
return ret;
lane_count = dp->link_train.lane_count;
+ adjust_request = link_status + 4;
if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
/* set training pattern 2 for EQ */
exynos_dp_set_training_pattern(dp, TRAINING_PTN2);
- adjust_request[0] = link_status[4];
- adjust_request[1] = link_status[5];
-
- exynos_dp_get_adjust_train(dp, adjust_request);
-
- buf[0] = DPCD_SCRAMBLING_DISABLED |
- DPCD_TRAINING_PATTERN_2;
ret = exynos_dp_write_byte_to_dpcd(dp,
DPCD_ADDR_TRAINING_PATTERN_SET,
- buf[0]);
+ DPCD_SCRAMBLING_DISABLED | DPCD_TRAINING_PATTERN_2);
if (ret)
return ret;
- for (lane = 0; lane < lane_count; lane++) {
- exynos_dp_set_lane_link_training(dp,
- dp->link_train.training_lane[lane],
- lane);
- buf[lane] = dp->link_train.training_lane[lane];
- ret = exynos_dp_write_byte_to_dpcd(dp,
- DPCD_ADDR_TRAINING_LANE0_SET + lane,
- buf[lane]);
- if (ret)
- return ret;
- }
dp->link_train.lt_state = EQUALIZER_TRAINING;
} else {
- exynos_dp_read_byte_from_dpcd(dp,
- DPCD_ADDR_ADJUST_REQUEST_LANE0_1,
- &data);
- adjust_request[0] = data;
-
- exynos_dp_read_byte_from_dpcd(dp,
- DPCD_ADDR_ADJUST_REQUEST_LANE2_3,
- &data);
- adjust_request[1] = data;
-
for (lane = 0; lane < lane_count; lane++) {
training_lane = exynos_dp_get_lane_link_training(
dp, lane);
@@ -560,36 +521,31 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
if (exynos_dp_check_max_cr_loop(dp, voltage_swing) != 0) {
exynos_dp_reduce_link_rate(dp);
- } else {
- exynos_dp_get_adjust_train(dp, adjust_request);
-
- for (lane = 0; lane < lane_count; lane++) {
- exynos_dp_set_lane_link_training(dp,
- dp->link_train.training_lane[lane],
- lane);
- buf[lane] = dp->link_train.training_lane[lane];
- ret = exynos_dp_write_byte_to_dpcd(dp,
- DPCD_ADDR_TRAINING_LANE0_SET + lane,
- buf[lane]);
- if (ret)
- return ret;
- }
+ return ret;
}
}
+ exynos_dp_get_adjust_train(dp, adjust_request);
+
+ for (lane = 0; lane < lane_count; lane++) {
+ exynos_dp_set_lane_link_training(dp,
+ dp->link_train.training_lane[lane], lane);
+ ret = exynos_dp_write_byte_to_dpcd(dp,
+ DPCD_ADDR_TRAINING_LANE0_SET + lane,
+ dp->link_train.training_lane[lane]);
+ if (ret)
+ return ret;
+ }
+
return ret;
}
static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
{
- int ret;
+ int ret, lane, lane_count;
u8 link_status[6];
- int lane;
- int lane_count;
- u8 buf[5];
u32 reg;
-
- u8 adjust_request[2];
+ u8 *adjust_request;
usleep_range(400, 401);
@@ -597,55 +553,53 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
6, link_status);
if (ret)
return ret;
+
+ adjust_request = link_status + 4;
lane_count = dp->link_train.lane_count;
- if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
- adjust_request[0] = link_status[4];
- adjust_request[1] = link_status[5];
+ if (exynos_dp_clock_recovery_ok(link_status, lane_count)) {
+ exynos_dp_reduce_link_rate(dp);
+ return ret;
+ }
+ if (exynos_dp_channel_eq_ok(link_status, lane_count) = 0) {
+ /* traing pattern Set to Normal */
+ exynos_dp_training_pattern_dis(dp);
- if (exynos_dp_channel_eq_ok(link_status, lane_count) = 0) {
- /* traing pattern Set to Normal */
- exynos_dp_training_pattern_dis(dp);
+ dev_info(dp->dev, "Link Training success!\n");
- dev_info(dp->dev, "Link Training success!\n");
+ exynos_dp_get_link_bandwidth(dp, ®);
+ dp->link_train.link_rate = reg;
+ dev_dbg(dp->dev, "final bandwidth = %.2x\n",
+ dp->link_train.link_rate);
- exynos_dp_get_link_bandwidth(dp, ®);
- dp->link_train.link_rate = reg;
- dev_dbg(dp->dev, "final bandwidth = %.2x\n",
- dp->link_train.link_rate);
+ exynos_dp_get_lane_count(dp, ®);
+ dp->link_train.lane_count = reg;
+ dev_dbg(dp->dev, "final lane count = %.2x\n",
+ dp->link_train.lane_count);
+ /* set enhanced mode if available */
+ exynos_dp_set_enhanced_mode(dp);
- exynos_dp_get_lane_count(dp, ®);
- dp->link_train.lane_count = reg;
- dev_dbg(dp->dev, "final lane count = %.2x\n",
- dp->link_train.lane_count);
- /* set enhanced mode if available */
- exynos_dp_set_enhanced_mode(dp);
+ dp->link_train.lt_state = FINISHED;
+ } else {
+ /* not all locked */
+ dp->link_train.eq_loop++;
- dp->link_train.lt_state = FINISHED;
+ if (dp->link_train.eq_loop > MAX_EQ_LOOP) {
+ exynos_dp_reduce_link_rate(dp);
} else {
- /* not all locked */
- dp->link_train.eq_loop++;
-
- if (dp->link_train.eq_loop > MAX_EQ_LOOP) {
- exynos_dp_reduce_link_rate(dp);
- } else {
- exynos_dp_get_adjust_train(dp, adjust_request);
-
- for (lane = 0; lane < lane_count; lane++) {
- exynos_dp_set_lane_link_training(dp,
- dp->link_train.training_lane[lane],
- lane);
- buf[lane] = dp->link_train.training_lane[lane];
- ret = exynos_dp_write_byte_to_dpcd(dp,
- DPCD_ADDR_TRAINING_LANE0_SET + lane,
- buf[lane]);
- if (ret)
- return ret;
- }
+ exynos_dp_get_adjust_train(dp, adjust_request);
+
+ for (lane = 0; lane < lane_count; lane++) {
+ exynos_dp_set_lane_link_training(dp,
+ dp->link_train.training_lane[lane],
+ lane);
+ ret = exynos_dp_write_byte_to_dpcd(dp,
+ DPCD_ADDR_TRAINING_LANE0_SET + lane,
+ dp->link_train.training_lane[lane]);
+ if (ret)
+ return ret;
}
}
- } else {
- exynos_dp_reduce_link_rate(dp);
}
return ret;
--
1.7.7.3
^ permalink raw reply related
* [PATCH 04/10] video: exynos_dp: Get pll lock before pattern set
From: Sean Paul @ 2012-08-08 3:54 UTC (permalink / raw)
To: linux-fbdev
According to the exynos datasheet (Figure 49-10), we should wait for PLL
lock before programming the training pattern when doing software eDP
link training.
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Mandeep Singh Baines <msb@chromium.org>
---
drivers/video/exynos/exynos_dp_core.c | 14 +++++++++++++-
1 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index 3deded2..207bd7e 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -23,6 +23,8 @@
#include "exynos_dp_core.h"
+#define PLL_MAX_TRIES 100
+
static int exynos_dp_init_dp(struct exynos_dp_device *dp)
{
exynos_dp_reset(dp);
@@ -260,7 +262,7 @@ static void exynos_dp_set_lane_lane_pre_emphasis(struct exynos_dp_device *dp,
static int exynos_dp_link_start(struct exynos_dp_device *dp)
{
- int ret, lane, lane_count;
+ int ret, lane, lane_count, pll_tries;
u8 buf[4];
lane_count = dp->link_train.lane_count;
@@ -293,6 +295,16 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
exynos_dp_set_lane_lane_pre_emphasis(dp,
PRE_EMPHASIS_LEVEL_0, lane);
+ /* Wait for PLL lock */
+ pll_tries = 0;
+ while (exynos_dp_get_pll_lock_status(dp) = PLL_UNLOCKED) {
+ if (pll_tries = PLL_MAX_TRIES)
+ return -ETIMEDOUT;
+
+ pll_tries++;
+ udelay(100);
+ }
+
/* Set training pattern 1 */
exynos_dp_set_training_pattern(dp, TRAINING_PTN1);
--
1.7.7.3
^ permalink raw reply related
* [PATCH 05/10] video: exynos_dp: Remove sink control to D0
From: Sean Paul @ 2012-08-08 3:54 UTC (permalink / raw)
To: linux-fbdev
Don't reset the sink power to D0. Removing this for three reasons:
1) It's not required in the SW link training documentation
2) The comment is incorrect, D0 is normal operation, not "power down"
3) It seems to change things in the link training that causes glitches
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
drivers/video/exynos/exynos_dp_core.c | 6 ------
1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index 207bd7e..1c998d9 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -273,12 +273,6 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
for (lane = 0; lane < lane_count; lane++)
dp->link_train.cr_loop[lane] = 0;
- /* Set sink to D0 (Sink Not Ready) mode. */
- ret = exynos_dp_write_byte_to_dpcd(dp, DPCD_ADDR_SINK_POWER_STATE,
- DPCD_SET_POWER_STATE_D0);
- if (ret)
- return ret;
-
/* Set link rate and count as you want to establish*/
exynos_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
exynos_dp_set_lane_count(dp, dp->link_train.lane_count);
--
1.7.7.3
^ permalink raw reply related
* [PATCH 06/10] video: exynos_dp: Fix get_pll_lock_status return value
From: Sean Paul @ 2012-08-08 3:54 UTC (permalink / raw)
To: linux-fbdev
Fix the return value of exynos_dp_get_pll_lock_status to
reflect what it actually returns.
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Olof Johansson <olofj@chromium.org>
---
drivers/video/exynos/exynos_dp_core.h | 2 +-
drivers/video/exynos/exynos_dp_reg.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
index 8526e54..6431c65 100644
--- a/drivers/video/exynos/exynos_dp_core.h
+++ b/drivers/video/exynos/exynos_dp_core.h
@@ -43,7 +43,7 @@ void exynos_dp_init_interrupt(struct exynos_dp_device *dp);
void exynos_dp_reset(struct exynos_dp_device *dp);
void exynos_dp_swreset(struct exynos_dp_device *dp);
void exynos_dp_config_interrupt(struct exynos_dp_device *dp);
-u32 exynos_dp_get_pll_lock_status(struct exynos_dp_device *dp);
+enum pll_status exynos_dp_get_pll_lock_status(struct exynos_dp_device *dp);
void exynos_dp_set_pll_power_down(struct exynos_dp_device *dp, bool enable);
void exynos_dp_set_analog_power_down(struct exynos_dp_device *dp,
enum analog_power_block block,
diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
index a121bed..d7b1494 100644
--- a/drivers/video/exynos/exynos_dp_reg.c
+++ b/drivers/video/exynos/exynos_dp_reg.c
@@ -179,7 +179,7 @@ void exynos_dp_config_interrupt(struct exynos_dp_device *dp)
writel(reg, dp->reg_base + EXYNOS_DP_INT_STA_MASK);
}
-u32 exynos_dp_get_pll_lock_status(struct exynos_dp_device *dp)
+enum pll_status exynos_dp_get_pll_lock_status(struct exynos_dp_device *dp)
{
u32 reg;
--
1.7.7.3
^ permalink raw reply related
* [PATCH 07/10] video: exynos_dp: Improve EDID error handling
From: Sean Paul @ 2012-08-08 3:54 UTC (permalink / raw)
To: linux-fbdev
EDID error handling has 2 problems:
- It doesn't fail as early as it can
- The retry counts for i2c and aux transactions are huge
This patch fails if the initial i2c transaction fails, and reduces the
aux and i2c retry counts down to 3.
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Olof Johansson <olofj@chromium.org>
---
drivers/video/exynos/exynos_dp_core.c | 18 ++++++++++--------
drivers/video/exynos/exynos_dp_reg.c | 9 ++++-----
2 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index 1c998d9..2882362 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -89,9 +89,11 @@ static int exynos_dp_read_edid(struct exynos_dp_device *dp)
*/
/* Read Extension Flag, Number of 128-byte EDID extension blocks */
- exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
+ retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
EDID_EXTENSION_FLAG,
&extend_block);
+ if (retval)
+ return retval;
if (extend_block > 0) {
dev_dbg(dp->dev, "EDID data includes a single extension!\n");
@@ -177,21 +179,21 @@ static int exynos_dp_handle_edid(struct exynos_dp_device *dp)
{
u8 buf[12];
int i;
- int retval;
+ int ret;
/* Read DPCD DPCD_ADDR_DPCD_REV~RECEIVE_PORT1_CAP_1 */
- exynos_dp_read_bytes_from_dpcd(dp,
- DPCD_ADDR_DPCD_REV,
- 12, buf);
+ ret = exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_DPCD_REV, 12, buf);
+ if (ret)
+ return ret;
/* Read EDID */
for (i = 0; i < 3; i++) {
- retval = exynos_dp_read_edid(dp);
- if (retval = 0)
+ ret = exynos_dp_read_edid(dp);
+ if (!ret)
break;
}
- return retval;
+ return ret;
}
static void exynos_dp_enable_rx_to_enhanced_mode(struct exynos_dp_device *dp,
diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
index d7b1494..389e0f0 100644
--- a/drivers/video/exynos/exynos_dp_reg.c
+++ b/drivers/video/exynos/exynos_dp_reg.c
@@ -547,7 +547,7 @@ int exynos_dp_write_bytes_to_dpcd(struct exynos_dp_device *dp,
else
cur_data_count = count - start_offset;
- for (i = 0; i < 10; i++) {
+ for (i = 0; i < 3; i++) {
/* Select DPCD device address */
reg = AUX_ADDR_7_0(reg_addr + start_offset);
writel(reg, dp->reg_base + EXYNOS_DP_AUX_ADDR_7_0);
@@ -612,7 +612,7 @@ int exynos_dp_read_bytes_from_dpcd(struct exynos_dp_device *dp,
cur_data_count = count - start_offset;
/* AUX CH Request Transaction process */
- for (i = 0; i < 10; i++) {
+ for (i = 0; i < 3; i++) {
/* Select DPCD device address */
reg = AUX_ADDR_7_0(reg_addr + start_offset);
writel(reg, dp->reg_base + EXYNOS_DP_AUX_ADDR_7_0);
@@ -695,7 +695,7 @@ int exynos_dp_read_byte_from_i2c(struct exynos_dp_device *dp,
int i;
int retval;
- for (i = 0; i < 10; i++) {
+ for (i = 0; i < 3; i++) {
/* Clear AUX CH data buffer */
reg = BUF_CLR;
writel(reg, dp->reg_base + EXYNOS_DP_BUFFER_DATA_CTL);
@@ -703,7 +703,6 @@ int exynos_dp_read_byte_from_i2c(struct exynos_dp_device *dp,
/* Select EDID device */
retval = exynos_dp_select_i2c_device(dp, device_addr, reg_addr);
if (retval != 0) {
- dev_err(dp->dev, "Select EDID device fail!\n");
continue;
}
@@ -745,7 +744,7 @@ int exynos_dp_read_bytes_from_i2c(struct exynos_dp_device *dp,
int retval = 0;
for (i = 0; i < count; i += 16) {
- for (j = 0; j < 100; j++) {
+ for (j = 0; j < 3; j++) {
/* Clear AUX CH data buffer */
reg = BUF_CLR;
writel(reg, dp->reg_base + EXYNOS_DP_BUFFER_DATA_CTL);
--
1.7.7.3
^ permalink raw reply related
* [PATCH 08/10] video: exynos_dp: Move hotplug into a workqueue
From: Sean Paul @ 2012-08-08 3:54 UTC (permalink / raw)
To: linux-fbdev
Move the hotplug related code from probe and resume into a workqueue.
This allows us to initialize the DP driver (and resume it) when there
is no monitor connected.
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Olof Johansson <olofj@chromium.org>
---
drivers/video/exynos/exynos_dp_core.c | 94 +++++++++++++++++----------------
drivers/video/exynos/exynos_dp_core.h | 1 +
2 files changed, 50 insertions(+), 45 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index 2882362..68ad494 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -834,6 +834,45 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
return IRQ_HANDLED;
}
+static void exynos_dp_hotplug(struct work_struct *work)
+{
+ struct exynos_dp_device *dp;
+ int ret;
+
+ dp = container_of(work, struct exynos_dp_device, hotplug_work);
+
+ ret = exynos_dp_detect_hpd(dp);
+ if (ret) {
+ dev_err(dp->dev, "unable to detect hpd\n");
+ return;
+ }
+
+ ret = exynos_dp_handle_edid(dp);
+ if (ret) {
+ dev_err(dp->dev, "unable to handle edid\n");
+ return;
+ }
+
+ ret = exynos_dp_set_link_train(dp, dp->video_info->lane_count,
+ dp->video_info->link_rate);
+ if (ret) {
+ dev_err(dp->dev, "unable to do link train\n");
+ return;
+ }
+
+ exynos_dp_enable_scramble(dp, 1);
+ exynos_dp_enable_rx_to_enhanced_mode(dp, 1);
+ exynos_dp_enable_enhanced_mode(dp, 1);
+
+ exynos_dp_set_lane_count(dp, dp->video_info->lane_count);
+ exynos_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
+
+ exynos_dp_init_video(dp);
+ ret = exynos_dp_config_video(dp, dp->video_info);
+ if (ret)
+ dev_err(&dp->dev, "unable to config video\n");
+}
+
static int __devinit exynos_dp_probe(struct platform_device *pdev)
{
struct resource *res;
@@ -886,6 +925,8 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
goto err_clock;
}
+ INIT_WORK(&dp->hotplug_work, exynos_dp_hotplug);
+
ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
"exynos-dp", dp);
if (ret) {
@@ -899,36 +940,8 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
exynos_dp_init_dp(dp);
- ret = exynos_dp_detect_hpd(dp);
- if (ret) {
- dev_err(&pdev->dev, "unable to detect hpd\n");
- goto err_clock;
- }
-
- exynos_dp_handle_edid(dp);
-
- ret = exynos_dp_set_link_train(dp, dp->video_info->lane_count,
- dp->video_info->link_rate);
- if (ret) {
- dev_err(&pdev->dev, "unable to do link train\n");
- goto err_clock;
- }
-
- exynos_dp_enable_scramble(dp, 1);
- exynos_dp_enable_rx_to_enhanced_mode(dp, 1);
- exynos_dp_enable_enhanced_mode(dp, 1);
-
- exynos_dp_set_lane_count(dp, dp->video_info->lane_count);
- exynos_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
-
- exynos_dp_init_video(dp);
- ret = exynos_dp_config_video(dp, dp->video_info);
- if (ret) {
- dev_err(&pdev->dev, "unable to config video\n");
- goto err_clock;
- }
-
platform_set_drvdata(pdev, dp);
+ schedule_work(&dp->hotplug_work);
return 0;
@@ -943,6 +956,9 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
struct exynos_dp_device *dp = platform_get_drvdata(pdev);
+ if (work_pending(&dp->hotplug_work))
+ flush_work_sync(&dp->hotplug_work);
+
if (pdata && pdata->phy_exit)
pdata->phy_exit();
@@ -959,6 +975,9 @@ static int exynos_dp_suspend(struct device *dev)
struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
struct exynos_dp_device *dp = platform_get_drvdata(pdev);
+ if (work_pending(&dp->hotplug_work))
+ flush_work_sync(&dp->hotplug_work);
+
if (pdata && pdata->phy_exit)
pdata->phy_exit();
@@ -979,22 +998,7 @@ static int exynos_dp_resume(struct device *dev)
clk_enable(dp->clock);
exynos_dp_init_dp(dp);
-
- exynos_dp_detect_hpd(dp);
- exynos_dp_handle_edid(dp);
-
- exynos_dp_set_link_train(dp, dp->video_info->lane_count,
- dp->video_info->link_rate);
-
- exynos_dp_enable_scramble(dp, 1);
- exynos_dp_enable_rx_to_enhanced_mode(dp, 1);
- exynos_dp_enable_enhanced_mode(dp, 1);
-
- exynos_dp_set_lane_count(dp, dp->video_info->lane_count);
- exynos_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
-
- exynos_dp_init_video(dp);
- exynos_dp_config_video(dp, dp->video_info);
+ schedule_work(&dp->hotplug_work);
return 0;
}
diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
index 6431c65..cf1010b 100644
--- a/drivers/video/exynos/exynos_dp_core.h
+++ b/drivers/video/exynos/exynos_dp_core.h
@@ -32,6 +32,7 @@ struct exynos_dp_device {
struct video_info *video_info;
struct link_train link_train;
+ struct work_struct hotplug_work;
};
/* exynos_dp_reg.c */
--
1.7.7.3
^ permalink raw reply related
* [PATCH 09/10] video: exynos_dp: Fix bug when checking dp->irq
From: Sean Paul @ 2012-08-08 3:54 UTC (permalink / raw)
To: linux-fbdev
Fix a bug where we check !dp->irq instead of the correct check for
-ENXIO.
BUG=None
TEST=None
Change-Id: Ibcdd16fb71eb41e4380ac8911ad6063634ff80e2
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-on: https://gerrit.chromium.org/gerrit/29144
Reviewed-by: Olof Johansson <olofj@chromium.org>
---
drivers/video/exynos/exynos_dp_core.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index 68ad494..eb5816f 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -919,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
}
dp->irq = platform_get_irq(pdev, 0);
- if (!dp->irq) {
+ if (dp->irq = -ENXIO) {
dev_err(&pdev->dev, "failed to get irq\n");
ret = -ENODEV;
goto err_clock;
--
1.7.7.3
^ permalink raw reply related
* [PATCH 10/10] video: exynos_dp: Enable hotplug interrupts
From: Sean Paul @ 2012-08-08 3:54 UTC (permalink / raw)
To: linux-fbdev
Enable hotplug interrupts and move the hotplug scheduling into the
interrupt handler. This allows us to introduce a screen at any time
while we're running.
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Olof Johansson <olofj@chromium.org>
---
drivers/video/exynos/exynos_dp_core.c | 40 +++++++++++++++++++++++++-------
drivers/video/exynos/exynos_dp_core.h | 9 +++++++
drivers/video/exynos/exynos_dp_reg.c | 35 ++++++++++++++++++++++++++++-
3 files changed, 74 insertions(+), 10 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index eb5816f..5279027 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -47,10 +47,6 @@ static int exynos_dp_detect_hpd(struct exynos_dp_device *dp)
{
int timeout_loop = 0;
- exynos_dp_init_hpd(dp);
-
- usleep_range(200, 210);
-
while (exynos_dp_get_plug_in_status(dp) != 0) {
timeout_loop++;
if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
@@ -829,8 +825,32 @@ static void exynos_dp_enable_scramble(struct exynos_dp_device *dp, bool enable)
static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
{
struct exynos_dp_device *dp = arg;
-
- dev_err(dp->dev, "exynos_dp_irq_handler\n");
+ enum dp_irq_type irq_type;
+
+ irq_type = exynos_dp_get_irq_type(dp);
+ switch (irq_type) {
+ case DP_IRQ_TYPE_HP_CABLE_IN:
+ dev_dbg(dp->dev, "Received irq - cable in\n");
+ schedule_work(&dp->hotplug_work);
+ exynos_dp_clear_hotplug_interrupts(dp);
+ break;
+ case DP_IRQ_TYPE_HP_CABLE_OUT:
+ dev_dbg(dp->dev, "Received irq - cable out\n");
+ exynos_dp_clear_hotplug_interrupts(dp);
+ break;
+ case DP_IRQ_TYPE_HP_CHANGE:
+ /*
+ * We get these change notifications once in a while, but there
+ * is nothing we can do with them. Just ignore it for now and
+ * only handle cable changes.
+ */
+ dev_dbg(dp->dev, "Received irq - hotplug change; ignoring.\n");
+ exynos_dp_clear_hotplug_interrupts(dp);
+ break;
+ default:
+ dev_err(dp->dev, "Received irq - unknown type!\n");
+ break;
+ }
return IRQ_HANDLED;
}
@@ -843,7 +863,7 @@ static void exynos_dp_hotplug(struct work_struct *work)
ret = exynos_dp_detect_hpd(dp);
if (ret) {
- dev_err(dp->dev, "unable to detect hpd\n");
+ /* Cable has been disconnected, we're done */
return;
}
@@ -941,7 +961,6 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
exynos_dp_init_dp(dp);
platform_set_drvdata(pdev, dp);
- schedule_work(&dp->hotplug_work);
return 0;
@@ -975,6 +994,8 @@ static int exynos_dp_suspend(struct device *dev)
struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
struct exynos_dp_device *dp = platform_get_drvdata(pdev);
+ disable_irq(dp->irq);
+
if (work_pending(&dp->hotplug_work))
flush_work_sync(&dp->hotplug_work);
@@ -998,7 +1019,8 @@ static int exynos_dp_resume(struct device *dev)
clk_enable(dp->clock);
exynos_dp_init_dp(dp);
- schedule_work(&dp->hotplug_work);
+
+ enable_irq(dp->irq);
return 0;
}
diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
index cf1010b..a5795dd 100644
--- a/drivers/video/exynos/exynos_dp_core.h
+++ b/drivers/video/exynos/exynos_dp_core.h
@@ -13,6 +13,13 @@
#ifndef _EXYNOS_DP_CORE_H
#define _EXYNOS_DP_CORE_H
+enum dp_irq_type {
+ DP_IRQ_TYPE_HP_CABLE_IN,
+ DP_IRQ_TYPE_HP_CABLE_OUT,
+ DP_IRQ_TYPE_HP_CHANGE,
+ DP_IRQ_TYPE_UNKNOWN,
+};
+
struct link_train {
int eq_loop;
int cr_loop[4];
@@ -51,6 +58,8 @@ void exynos_dp_set_analog_power_down(struct exynos_dp_device *dp,
bool enable);
void exynos_dp_init_analog_func(struct exynos_dp_device *dp);
void exynos_dp_init_hpd(struct exynos_dp_device *dp);
+enum dp_irq_type exynos_dp_get_irq_type(struct exynos_dp_device *dp);
+void exynos_dp_clear_hotplug_interrupts(struct exynos_dp_device *dp);
void exynos_dp_reset_aux(struct exynos_dp_device *dp);
void exynos_dp_init_aux(struct exynos_dp_device *dp);
int exynos_dp_get_plug_in_status(struct exynos_dp_device *dp);
diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
index 389e0f0..5906fc7 100644
--- a/drivers/video/exynos/exynos_dp_reg.c
+++ b/drivers/video/exynos/exynos_dp_reg.c
@@ -327,7 +327,7 @@ void exynos_dp_init_analog_func(struct exynos_dp_device *dp)
writel(reg, dp->reg_base + EXYNOS_DP_FUNC_EN_2);
}
-void exynos_dp_init_hpd(struct exynos_dp_device *dp)
+void exynos_dp_clear_hotplug_interrupts(struct exynos_dp_device *dp)
{
u32 reg;
@@ -336,10 +336,43 @@ void exynos_dp_init_hpd(struct exynos_dp_device *dp)
reg = INT_HPD;
writel(reg, dp->reg_base + EXYNOS_DP_INT_STA);
+}
+
+void exynos_dp_init_hpd(struct exynos_dp_device *dp)
+{
+ u32 reg;
+
+ exynos_dp_clear_hotplug_interrupts(dp);
reg = readl(dp->reg_base + EXYNOS_DP_SYS_CTL_3);
reg &= ~(F_HPD | HPD_CTRL);
writel(reg, dp->reg_base + EXYNOS_DP_SYS_CTL_3);
+
+ /* Unmask hotplug interrupts */
+ reg = HOTPLUG_CHG | HPD_LOST | PLUG;
+ writel(reg, dp->reg_base + EXYNOS_DP_COMMON_INT_MASK_4);
+
+ reg = INT_HPD;
+ writel(reg, dp->reg_base + EXYNOS_DP_INT_STA_MASK);
+}
+
+enum dp_irq_type exynos_dp_get_irq_type(struct exynos_dp_device *dp)
+{
+ u32 reg;
+
+ /* Parse hotplug interrupt status register */
+ reg = readl(dp->reg_base + EXYNOS_DP_COMMON_INT_STA_4);
+
+ if (reg & PLUG)
+ return DP_IRQ_TYPE_HP_CABLE_IN;
+
+ if (reg & HPD_LOST)
+ return DP_IRQ_TYPE_HP_CABLE_OUT;
+
+ if (reg & HOTPLUG_CHG)
+ return DP_IRQ_TYPE_HP_CHANGE;
+
+ return DP_IRQ_TYPE_UNKNOWN;
}
void exynos_dp_reset_aux(struct exynos_dp_device *dp)
--
1.7.7.3
^ permalink raw reply related
* Re: [RFC 08/17] OMAPDSS: DSI: Maintain own copy of timings in driver data
From: Archit Taneja @ 2012-08-08 6:09 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, sumit.semwal, rob
In-Reply-To: <1344348462.7216.77.camel@lappyti>
On Tuesday 07 August 2012 07:37 PM, Tomi Valkeinen wrote:
> On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
>> The DSI driver currently relies on the timings in omap_dss_device struct to
>> configure the DISPC and DSI blocks accordingly. This makes the DSI interface
>> driver dependent on the omap_dss_device struct.
>>
>> Make the DPI driver data maintain it's own timings field. The panel driver is
> ^^^
> DSI
>
>> expected to call omapdss_dsi_set_timings() to set these timings before the panel
>> is enabled.
>>
>> Signed-off-by: Archit Taneja <archit@ti.com>d
>> ---
>> drivers/video/omap2/displays/panel-taal.c | 2 ++
>> drivers/video/omap2/dss/dsi.c | 27 ++++++++++++++++++++++-----
>> include/video/omapdss.h | 2 ++
>> 3 files changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/video/omap2/displays/panel-taal.c b/drivers/video/omap2/displays/panel-taal.c
>> index 3f5acc7..4775580 100644
>> --- a/drivers/video/omap2/displays/panel-taal.c
>> +++ b/drivers/video/omap2/displays/panel-taal.c
>> @@ -1060,6 +1060,8 @@ static int taal_power_on(struct omap_dss_device *dssdev)
>> goto err0;
>> };
>>
>> + omapdss_dsi_set_timings(dssdev, &td->panel_config->timings);
>> +
>> r = omapdss_dsi_display_enable(dssdev);
>> if (r) {
>> dev_err(&dssdev->dev, "failed to enable DSI\n");
>
> Video timings for command mode panel are meaningless. If we need to pass
> the resolution of the panel, perhaps we should have a separate function
> for that.
>
> However, with a quick glance at dsi.c, we don't even use the
> dssdev->panel.timings for cmd mode panel. But we do use
> dssdev->get_resolution() in a few places. Those calls could be replaced
> by storing the panel size in dsi.c, given with omapdss_dsi_set_size() or
> such. We could use the timings field in dsi.c to store them, though.
I am a bit unclear about resolution when it comes to command mode panels.
For command mode panels, we can perform rotation at the panel side. That
is, the panel refreshes itself by fetching pixels from it's buffer in a
rotated way. Is that right?
If the original resolution is 864x480, and we set rotation at panel side
to make the rotation 480x864, the DISPC manager size should also be
configured at 480x864 right?
We seem to be setting the manager timings only once when DSI is enabled.
After that, setting rotation doesn't impact manager size.
I am asking this to understand if we need to keep resolution as a
separate parameter than timings. That is, timings represents the initial
width and height of the panel, and resolution represents the current
width and height of the panel.
Archit
^ permalink raw reply
* Re: [RFC 13/17] OMAPDSS: SDI: Create a function to set timings
From: Archit Taneja @ 2012-08-08 6:10 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, sumit.semwal, rob
In-Reply-To: <1344349218.7216.80.camel@lappyti>
On Tuesday 07 August 2012 07:50 PM, Tomi Valkeinen wrote:
> On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
>> Create function omapdss_sdi_set_timings(), this can be used by a SDI panel
>> driver without disabling/enabling the SDI interface. This is similar to the
>> set_timings op of the DPI interface driver. It calls sdi_set_mode() which only
>> configures the DISPC timings and DSS/DISPC clock dividers.
>
> I don't think this works, as the SDI PLL uses pclk-free, and if pclk
> changes, PLL lock probably breaks.
>
> OMAP3430 TRM explains the sequence how to configure settings on the fly,
> but that's not very simple. Just turning the output off and on is much
> easier.
Right, I'll make set_timings() just disable and enable SDI like before.
Archit
^ permalink raw reply
* Re: [RFC 08/17] OMAPDSS: DSI: Maintain own copy of timings in driver data
From: Tomi Valkeinen @ 2012-08-08 6:15 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap, sumit.semwal, rob
In-Reply-To: <5021FFDC.6080006@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2871 bytes --]
On Wed, 2012-08-08 at 11:27 +0530, Archit Taneja wrote:
> I am a bit unclear about resolution when it comes to command mode panels.
Right, it's a bit confusing. And I'm not 100% sure how to manage the
rotation.
> For command mode panels, we can perform rotation at the panel side. That
> is, the panel refreshes itself by fetching pixels from it's buffer in a
> rotated way. Is that right?
Yes. Well, actually I think the panel stores the pixels in rotated
manner when it receives them from OMAP, but it's practically the same.
One thing to realize is that this kind of rotation is a bit limited:
because there's only one buffer, OMAP will write pixels to the buffers
at the same time as the panel shows them. When rotating, this leads to
tearing.
If the panel has double buffer, that solves the problem, but I haven't
seen such panels. Another option is to update the panel in two parts,
like N9 does, but that's timing sensitive and a bit tricky.
> If the original resolution is 864x480, and we set rotation at panel side
> to make the rotation 480x864, the DISPC manager size should also be
> configured at 480x864 right?
Yep. When we use the panel rotation, from OMAP's point of view the panel
resolution has changed.
> We seem to be setting the manager timings only once when DSI is enabled.
> After that, setting rotation doesn't impact manager size.
Hmm, previously the mgr size was set before each update. I wonder if
that code has been dropped, probably because we removed the support for
partial updates at one point. Without partial updates, the size stays
the same, except obviously with rotation. I think I just forgot about
rotation at that time.
> I am asking this to understand if we need to keep resolution as a
> separate parameter than timings. That is, timings represents the initial
> width and height of the panel, and resolution represents the current
> width and height of the panel.
I'm not sure. I think that OMAP doesn't really need to know about the
initial resolution. It doesn't really matter from OMAP's point of view.
I think I originally kept timings and resolution separately, and the
idea was that timings represent the panel's timings, i.e. how it updates
the screen from its own memory. And resolution represents the usable
resolution, from OMAP's point of view.
While I haven't seen such a cmd mode panel, there could be a command
sent to the panel to configure its timings. For this we need real
timings, not the rotated resolution.
However, even in that case the DISPC doesn't need to know about those
timings, they would be handled by the panel driver (which could,
perhaps, reconfigure the DSI bus speed to match the new timings). So I
think that inside omapdss, we don't need separate timings and resolution
for DSI cmd mode panels.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 00/17] OMAPDSS: Change way of passing timings from panel driver to interface
From: Archit Taneja @ 2012-08-08 6:17 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, sumit.semwal, rob
In-Reply-To: <1344349948.7216.82.camel@lappyti>
On Tuesday 07 August 2012 08:02 PM, Tomi Valkeinen wrote:
> On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
>> This series tries to make interface drivers less dependent on omap_dss_device
>> which represents a panel/device connected to that interface. The current way of
>> configuring an interface is to populate the panel's omap_dss_device instance
>> with parameters common to the panel and the interface, they are either populated
>> in the board file, or in the panel driver. Panel timings, number of lanes
>> connected to interface, and pixel format are examples of such parameters, these
>> are then extracted by the interface driver to configure itself.
>
> The series looks good. I had only a few comments to make, but obviously
> this needs quite a bit of testing. I'll try it out.
One thing I'm not sure about is whether these new functions should be
aware of the state of the output. For example, if we call set_timings()
with DSI video mode which is already enabled, the timings won't really
take any impact.
Similar issues would occur when we try to make other ops like
set_data_lines() or set_pixel_format(). These need to be called before
the output is enabled. I was wondering if we would need to add
intelligence here to make panel drivers less likely to make mistakes.
Archit
^ permalink raw reply
* RE: [PATCH V4 0/2] arm: samsung: Move FIMD headers to include/video/
From: Kukjin Kim @ 2012-08-08 6:20 UTC (permalink / raw)
To: 'Leela Krishna Amudala', linux-arm-kernel,
linux-samsung-soc
Cc: dri-devel, linux-fbdev, ben-linux, m.szyprowski,
'Florian Tobias Schandinat'
In-Reply-To: <1344342872-7904-1-git-send-email-l.krishna@samsung.com>
Leela Krishna Amudala wrote:
>
> This patchset moves the contents of regs-fb-v4.h and regs-fb.h from arch
> side
> to include/video/samsung_fimd.h
>
> This patchset is created and rebased against master branch of torvalds
> tree.
> Tested on smdk5250 board, build tested for other boards.
>
> Changes from version 3:
> - Squashed 1st, 3rd, 4th and 5th patches from version 3
>
> Changes from version 2:
> - Changed the EXYNOS5_xxx macro name to FIMD_V8_xxx
> - Moved the header inclusion under same kind
>
> Changes from version 1:
> - Split the patches as per Sylwester comments
> - Changed FIMD_V8_xxx macro to EXYNOS5_xxx
>
> Leela Krishna Amudala (2):
> include/video: move fimd register headers from platform to
> include/video
> include/video: Add register offsets for FIMD version 8
>
> arch/arm/mach-exynos/mach-nuri.c | 2 +-
> arch/arm/mach-exynos/mach-origen.c | 2 +-
> arch/arm/mach-exynos/mach-smdk4x12.c | 2 +-
> arch/arm/mach-exynos/mach-smdkv310.c | 2 +-
> arch/arm/mach-exynos/mach-universal_c210.c | 2 +-
> arch/arm/mach-exynos/setup-fimd0.c | 2 +-
> arch/arm/mach-s3c24xx/mach-smdk2416.c | 2 +-
> arch/arm/mach-s3c64xx/mach-anw6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-crag6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-hmt.c | 2 +-
> arch/arm/mach-s3c64xx/mach-mini6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-ncp.c | 2 +-
> arch/arm/mach-s3c64xx/mach-real6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-smartq5.c | 2 +-
> arch/arm/mach-s3c64xx/mach-smartq7.c | 2 +-
> arch/arm/mach-s3c64xx/mach-smdk6410.c | 2 +-
> arch/arm/mach-s5p64x0/mach-smdk6440.c | 2 +-
> arch/arm/mach-s5p64x0/mach-smdk6450.c | 2 +-
> arch/arm/mach-s5pc100/mach-smdkc100.c | 2 +-
> arch/arm/mach-s5pv210/mach-aquila.c | 2 +-
> arch/arm/mach-s5pv210/mach-goni.c | 2 +-
> arch/arm/mach-s5pv210/mach-smdkv210.c | 2 +-
> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159
----------------
> ----
> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 2 +-
> drivers/video/s3c-fb.c | 2 +-
> .../plat/regs-fb.h => include/video/samsung_fimd.h | 152
> +++++++++++++++++--
> 26 files changed, 165 insertions(+), 194 deletions(-)
> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
> rename arch/arm/plat-samsung/include/plat/regs-fb.h =>
> include/video/samsung_fimd.h (73%)
Looks OK to me. Will apply into topic branch, 'v3.7-for-florian' for
Florian's pulling.
If there is no objection for a couple of days, I will merge this into
for-next and send to Florian.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply
* Re: [RFC 00/17] OMAPDSS: Change way of passing timings from panel driver to interface
From: Tomi Valkeinen @ 2012-08-08 6:25 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap, sumit.semwal, rob
In-Reply-To: <502201B7.9030205@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2279 bytes --]
On Wed, 2012-08-08 at 11:35 +0530, Archit Taneja wrote:
> On Tuesday 07 August 2012 08:02 PM, Tomi Valkeinen wrote:
> > On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
> >> This series tries to make interface drivers less dependent on omap_dss_device
> >> which represents a panel/device connected to that interface. The current way of
> >> configuring an interface is to populate the panel's omap_dss_device instance
> >> with parameters common to the panel and the interface, they are either populated
> >> in the board file, or in the panel driver. Panel timings, number of lanes
> >> connected to interface, and pixel format are examples of such parameters, these
> >> are then extracted by the interface driver to configure itself.
> >
> > The series looks good. I had only a few comments to make, but obviously
> > this needs quite a bit of testing. I'll try it out.
>
> One thing I'm not sure about is whether these new functions should be
> aware of the state of the output. For example, if we call set_timings()
> with DSI video mode which is already enabled, the timings won't really
> take any impact.
>
> Similar issues would occur when we try to make other ops like
> set_data_lines() or set_pixel_format(). These need to be called before
> the output is enabled. I was wondering if we would need to add
> intelligence here to make panel drivers less likely to make mistakes.
Hmm, true. It'd be nice if the functions returned -EBUSY if the
operation cannot be done while the output is enabled.
We have the dssdev->state, but we should get rid of that (or leave it to
panel drivers). It'd be good if the output drivers know whether the
output is enabled or not. I think this data is already tracked by
apply.c. It's about ovl managers, but I think that's practically the
same as output.
Calling dss_mgr_enable() will set mp->enabled = true, which could be
returned via dss_mgr_is_enabled() or such.
Then again, it wouldn't be many lines of codes to track the enable-state
in each output driver. So if we have any suspicions that mp->enabled
doesn't quite work for, say, dsi, we could just add a private "enabled"
member to dsi. But I don't right away see why dss_mgr_is_enabled()
wouldn't work.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 08/17] OMAPDSS: DSI: Maintain own copy of timings in driver data
From: Archit Taneja @ 2012-08-08 6:41 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, sumit.semwal, rob
In-Reply-To: <1344406508.17575.12.camel@lappyti>
On Wednesday 08 August 2012 11:45 AM, Tomi Valkeinen wrote:
> On Wed, 2012-08-08 at 11:27 +0530, Archit Taneja wrote:
>
>> I am a bit unclear about resolution when it comes to command mode panels.
>
> Right, it's a bit confusing. And I'm not 100% sure how to manage the
> rotation.
>
>> For command mode panels, we can perform rotation at the panel side. That
>> is, the panel refreshes itself by fetching pixels from it's buffer in a
>> rotated way. Is that right?
>
> Yes. Well, actually I think the panel stores the pixels in rotated
> manner when it receives them from OMAP, but it's practically the same.
>
> One thing to realize is that this kind of rotation is a bit limited:
> because there's only one buffer, OMAP will write pixels to the buffers
> at the same time as the panel shows them. When rotating, this leads to
> tearing.
>
> If the panel has double buffer, that solves the problem, but I haven't
> seen such panels. Another option is to update the panel in two parts,
> like N9 does, but that's timing sensitive and a bit tricky.
>
>> If the original resolution is 864x480, and we set rotation at panel side
>> to make the rotation 480x864, the DISPC manager size should also be
>> configured at 480x864 right?
>
> Yep. When we use the panel rotation, from OMAP's point of view the panel
> resolution has changed.
>
>> We seem to be setting the manager timings only once when DSI is enabled.
>> After that, setting rotation doesn't impact manager size.
>
> Hmm, previously the mgr size was set before each update. I wonder if
> that code has been dropped, probably because we removed the support for
> partial updates at one point. Without partial updates, the size stays
> the same, except obviously with rotation. I think I just forgot about
> rotation at that time.
I tried out rotation on Taal, and it only works for 180 degrees(and 0 of
course), 90 and 270 result in no output. I'll add a
dss_mgr_set_timings() in omap_dsi_update, that should sort of fix it,
but someone would need to reconfigure the connected overlays too before
trying out an update.
>
>> I am asking this to understand if we need to keep resolution as a
>> separate parameter than timings. That is, timings represents the initial
>> width and height of the panel, and resolution represents the current
>> width and height of the panel.
>
> I'm not sure. I think that OMAP doesn't really need to know about the
> initial resolution. It doesn't really matter from OMAP's point of view.
>
> I think I originally kept timings and resolution separately, and the
> idea was that timings represent the panel's timings, i.e. how it updates
> the screen from its own memory. And resolution represents the usable
> resolution, from OMAP's point of view.
>
> While I haven't seen such a cmd mode panel, there could be a command
> sent to the panel to configure its timings. For this we need real
> timings, not the rotated resolution.
>
> However, even in that case the DISPC doesn't need to know about those
> timings, they would be handled by the panel driver (which could,
> perhaps, reconfigure the DSI bus speed to match the new timings). So I
> think that inside omapdss, we don't need separate timings and resolution
> for DSI cmd mode panels.
Ok.
Archit
^ permalink raw reply
* Re: [RFC 00/17] OMAPDSS: Change way of passing timings from panel driver to interface
From: Archit Taneja @ 2012-08-08 6:59 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, sumit.semwal, rob
In-Reply-To: <1344407158.17575.21.camel@lappyti>
On Wednesday 08 August 2012 11:55 AM, Tomi Valkeinen wrote:
> On Wed, 2012-08-08 at 11:35 +0530, Archit Taneja wrote:
>> On Tuesday 07 August 2012 08:02 PM, Tomi Valkeinen wrote:
>>> On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
>>>> This series tries to make interface drivers less dependent on omap_dss_device
>>>> which represents a panel/device connected to that interface. The current way of
>>>> configuring an interface is to populate the panel's omap_dss_device instance
>>>> with parameters common to the panel and the interface, they are either populated
>>>> in the board file, or in the panel driver. Panel timings, number of lanes
>>>> connected to interface, and pixel format are examples of such parameters, these
>>>> are then extracted by the interface driver to configure itself.
>>>
>>> The series looks good. I had only a few comments to make, but obviously
>>> this needs quite a bit of testing. I'll try it out.
>>
>> One thing I'm not sure about is whether these new functions should be
>> aware of the state of the output. For example, if we call set_timings()
>> with DSI video mode which is already enabled, the timings won't really
>> take any impact.
>>
>> Similar issues would occur when we try to make other ops like
>> set_data_lines() or set_pixel_format(). These need to be called before
>> the output is enabled. I was wondering if we would need to add
>> intelligence here to make panel drivers less likely to make mistakes.
>
> Hmm, true. It'd be nice if the functions returned -EBUSY if the
> operation cannot be done while the output is enabled.
>
> We have the dssdev->state, but we should get rid of that (or leave it to
> panel drivers). It'd be good if the output drivers know whether the
> output is enabled or not. I think this data is already tracked by
> apply.c. It's about ovl managers, but I think that's practically the
> same as output.
>
> Calling dss_mgr_enable() will set mp->enabled = true, which could be
> returned via dss_mgr_is_enabled() or such.
>
> Then again, it wouldn't be many lines of codes to track the enable-state
> in each output driver. So if we have any suspicions that mp->enabled
> doesn't quite work for, say, dsi, we could just add a private "enabled"
> member to dsi. But I don't right away see why dss_mgr_is_enabled()
> wouldn't work.
>
I think we had discussed previously that it may not the best idea to see
if a manager is enabled via mp->enabled as it's always possible that it
changes afterwards. Same for any other parameter in APPLY's private
data. This was the reason why we passed privtate data to DISPC functions
rather than creating apply helper functions which return the value of a
private data. For example, we pass manager timings to dispc_ovl_setup(),
instead of DISPC using a function like dss_mgr_get_timings().
I also don't see why dss_mgr_is_enabled() wouldn't work. The only places
where the manager's state will change are the output's enable and
disable ops. The mutex maintained by the output would ensure
sequential-ity between the output's enable() and set_timings() op, and
hence ensure the manager's state we see is fine.
If we manage the 'enabled' state for each output interface, we would be
a bit more consistent with respect to other parameters. For example,
timings is maintained by both manager and the output. Also, if we need
to separate out manager configurations from outputs in the future, it
would probably be better for the output to query it's own state rather
than depending on the manager, which could be configured either earlier
or later.
Archit
^ permalink raw reply
* Re: [RFC 08/17] OMAPDSS: DSI: Maintain own copy of timings in driver data
From: Tomi Valkeinen @ 2012-08-08 7:10 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap, sumit.semwal, rob
In-Reply-To: <5022073B.6070109@ti.com>
[-- Attachment #1: Type: text/plain, Size: 743 bytes --]
On Wed, 2012-08-08 at 11:59 +0530, Archit Taneja wrote:
> I tried out rotation on Taal, and it only works for 180 degrees(and 0 of
> course), 90 and 270 result in no output. I'll add a
> dss_mgr_set_timings() in omap_dsi_update, that should sort of fix it,
> but someone would need to reconfigure the connected overlays too before
> trying out an update.
Right, but that's something omapdss/panel cannot do, it must be done by
the user. The same problem is there with changing, say, DPI mode also.
Btw, can you separate smaller cleanups/fixes to another patch series, to
make this series even slightly smaller? I think at least the first
patches in this series are quite separate, and the rotation fix is also.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 00/17] OMAPDSS: Change way of passing timings from panel driver to interface
From: Tomi Valkeinen @ 2012-08-08 7:27 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap, sumit.semwal, rob
In-Reply-To: <50220B94.5040303@ti.com>
[-- Attachment #1: Type: text/plain, Size: 5740 bytes --]
On Wed, 2012-08-08 at 12:17 +0530, Archit Taneja wrote:
> On Wednesday 08 August 2012 11:55 AM, Tomi Valkeinen wrote:
> > On Wed, 2012-08-08 at 11:35 +0530, Archit Taneja wrote:
> >> On Tuesday 07 August 2012 08:02 PM, Tomi Valkeinen wrote:
> >>> On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
> >>>> This series tries to make interface drivers less dependent on omap_dss_device
> >>>> which represents a panel/device connected to that interface. The current way of
> >>>> configuring an interface is to populate the panel's omap_dss_device instance
> >>>> with parameters common to the panel and the interface, they are either populated
> >>>> in the board file, or in the panel driver. Panel timings, number of lanes
> >>>> connected to interface, and pixel format are examples of such parameters, these
> >>>> are then extracted by the interface driver to configure itself.
> >>>
> >>> The series looks good. I had only a few comments to make, but obviously
> >>> this needs quite a bit of testing. I'll try it out.
> >>
> >> One thing I'm not sure about is whether these new functions should be
> >> aware of the state of the output. For example, if we call set_timings()
> >> with DSI video mode which is already enabled, the timings won't really
> >> take any impact.
> >>
> >> Similar issues would occur when we try to make other ops like
> >> set_data_lines() or set_pixel_format(). These need to be called before
> >> the output is enabled. I was wondering if we would need to add
> >> intelligence here to make panel drivers less likely to make mistakes.
> >
> > Hmm, true. It'd be nice if the functions returned -EBUSY if the
> > operation cannot be done while the output is enabled.
> >
> > We have the dssdev->state, but we should get rid of that (or leave it to
> > panel drivers). It'd be good if the output drivers know whether the
> > output is enabled or not. I think this data is already tracked by
> > apply.c. It's about ovl managers, but I think that's practically the
> > same as output.
> >
> > Calling dss_mgr_enable() will set mp->enabled = true, which could be
> > returned via dss_mgr_is_enabled() or such.
> >
> > Then again, it wouldn't be many lines of codes to track the enable-state
> > in each output driver. So if we have any suspicions that mp->enabled
> > doesn't quite work for, say, dsi, we could just add a private "enabled"
> > member to dsi. But I don't right away see why dss_mgr_is_enabled()
> > wouldn't work.
> >
>
> I think we had discussed previously that it may not the best idea to see
> if a manager is enabled via mp->enabled as it's always possible that it
> changes afterwards. Same for any other parameter in APPLY's private
> data. This was the reason why we passed privtate data to DISPC functions
> rather than creating apply helper functions which return the value of a
> private data. For example, we pass manager timings to dispc_ovl_setup(),
> instead of DISPC using a function like dss_mgr_get_timings().
I think that's slightly different problem. The dispc case has an issue
with locking. If dispc_ovl_setup() is called with the apply's spinlock
taken, neither dispc_ovl_setup() nor dss_mgr_get_timings() can take the
lock. But if dispc_ovl_setup() is called from somewhere else, it should
take the lock. Also, if dispc_ovl_setup() would call a function in
apply, it'd be calling "upwards" to a higher level component.
With the output driver calling apply, none of those problems is present,
I believe.
> I also don't see why dss_mgr_is_enabled() wouldn't work. The only places
> where the manager's state will change are the output's enable and
> disable ops. The mutex maintained by the output would ensure
> sequential-ity between the output's enable() and set_timings() op, and
> hence ensure the manager's state we see is fine.
>
> If we manage the 'enabled' state for each output interface, we would be
> a bit more consistent with respect to other parameters. For example,
> timings is maintained by both manager and the output. Also, if we need
> to separate out manager configurations from outputs in the future, it
> would probably be better for the output to query it's own state rather
> than depending on the manager, which could be configured either earlier
> or later.
Two things that came to my mind:
If the output driver uses dss_mgr_is_enabled(), if both DPI and DSI
output drivers use the same manager, they'd both see themselves as
enabled. Of course only one can work at a time, so I'm not sure if
that's a practical problem. And if we had some kind of link between the
mgr and the output driver this would not be an issue.
The second thing is that we're not strictly required to have DISPC
connected to DSI or RFBI. We could use CPU/sDMA to output the image.
This is quite theoretical, though.
So, I think using dss_mgr_is_enabled() would work, but I'm still not
100% sure...
Well, perhaps the code should be such that dss_mgr_is_enabled() is used
to see if the mgr is enabled, not if the output is enabled. What I mean
with this is that if, say, set_data_lines() calls dispc to set the data
lines, we are really interested in if the dispc's mgr is enabled, not if
the DSI is enabled.
And if some other function changes DSI configuration (but doesn't touch
dispc), then we're not really interested in if the mgr is enabled, but
if the DSI is enabled.
That's a bit more complex than using only dss_mgr_is_enabled() or using
only output specific enable-flag, but I think it's more correct. In
DPI's case only dss_mgr_is_enabled() is probably needed. For DSI we may
need a separate private enable-flag.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 08/17] OMAPDSS: DSI: Maintain own copy of timings in driver data
From: Archit Taneja @ 2012-08-08 8:06 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, sumit.semwal, rob
In-Reply-To: <1344409847.17575.40.camel@lappyti>
On Wednesday 08 August 2012 12:40 PM, Tomi Valkeinen wrote:
> On Wed, 2012-08-08 at 11:59 +0530, Archit Taneja wrote:
>
>> I tried out rotation on Taal, and it only works for 180 degrees(and 0 of
>> course), 90 and 270 result in no output. I'll add a
>> dss_mgr_set_timings() in omap_dsi_update, that should sort of fix it,
>> but someone would need to reconfigure the connected overlays too before
>> trying out an update.
>
> Right, but that's something omapdss/panel cannot do, it must be done by
> the user. The same problem is there with changing, say, DPI mode also.
>
> Btw, can you separate smaller cleanups/fixes to another patch series, to
> make this series even slightly smaller? I think at least the first
> patches in this series are quite separate, and the rotation fix is also.
Right, I'll do that.
Archit
^ permalink raw reply
* Re: [RFC 00/17] OMAPDSS: Change way of passing timings from panel driver to interface
From: Archit Taneja @ 2012-08-08 8:11 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, sumit.semwal, rob
In-Reply-To: <1344410835.17575.54.camel@lappyti>
On Wednesday 08 August 2012 12:57 PM, Tomi Valkeinen wrote:
> On Wed, 2012-08-08 at 12:17 +0530, Archit Taneja wrote:
>> On Wednesday 08 August 2012 11:55 AM, Tomi Valkeinen wrote:
>>> On Wed, 2012-08-08 at 11:35 +0530, Archit Taneja wrote:
>>>> On Tuesday 07 August 2012 08:02 PM, Tomi Valkeinen wrote:
>>>>> On Wed, 2012-08-01 at 16:01 +0530, Archit Taneja wrote:
>>>>>> This series tries to make interface drivers less dependent on omap_dss_device
>>>>>> which represents a panel/device connected to that interface. The current way of
>>>>>> configuring an interface is to populate the panel's omap_dss_device instance
>>>>>> with parameters common to the panel and the interface, they are either populated
>>>>>> in the board file, or in the panel driver. Panel timings, number of lanes
>>>>>> connected to interface, and pixel format are examples of such parameters, these
>>>>>> are then extracted by the interface driver to configure itself.
>>>>>
>>>>> The series looks good. I had only a few comments to make, but obviously
>>>>> this needs quite a bit of testing. I'll try it out.
>>>>
>>>> One thing I'm not sure about is whether these new functions should be
>>>> aware of the state of the output. For example, if we call set_timings()
>>>> with DSI video mode which is already enabled, the timings won't really
>>>> take any impact.
>>>>
>>>> Similar issues would occur when we try to make other ops like
>>>> set_data_lines() or set_pixel_format(). These need to be called before
>>>> the output is enabled. I was wondering if we would need to add
>>>> intelligence here to make panel drivers less likely to make mistakes.
>>>
>>> Hmm, true. It'd be nice if the functions returned -EBUSY if the
>>> operation cannot be done while the output is enabled.
>>>
>>> We have the dssdev->state, but we should get rid of that (or leave it to
>>> panel drivers). It'd be good if the output drivers know whether the
>>> output is enabled or not. I think this data is already tracked by
>>> apply.c. It's about ovl managers, but I think that's practically the
>>> same as output.
>>>
>>> Calling dss_mgr_enable() will set mp->enabled = true, which could be
>>> returned via dss_mgr_is_enabled() or such.
>>>
>>> Then again, it wouldn't be many lines of codes to track the enable-state
>>> in each output driver. So if we have any suspicions that mp->enabled
>>> doesn't quite work for, say, dsi, we could just add a private "enabled"
>>> member to dsi. But I don't right away see why dss_mgr_is_enabled()
>>> wouldn't work.
>>>
>>
>> I think we had discussed previously that it may not the best idea to see
>> if a manager is enabled via mp->enabled as it's always possible that it
>> changes afterwards. Same for any other parameter in APPLY's private
>> data. This was the reason why we passed privtate data to DISPC functions
>> rather than creating apply helper functions which return the value of a
>> private data. For example, we pass manager timings to dispc_ovl_setup(),
>> instead of DISPC using a function like dss_mgr_get_timings().
>
> I think that's slightly different problem. The dispc case has an issue
> with locking. If dispc_ovl_setup() is called with the apply's spinlock
> taken, neither dispc_ovl_setup() nor dss_mgr_get_timings() can take the
> lock. But if dispc_ovl_setup() is called from somewhere else, it should
> take the lock. Also, if dispc_ovl_setup() would call a function in
> apply, it'd be calling "upwards" to a higher level component.
>
> With the output driver calling apply, none of those problems is present,
> I believe.
>
>> I also don't see why dss_mgr_is_enabled() wouldn't work. The only places
>> where the manager's state will change are the output's enable and
>> disable ops. The mutex maintained by the output would ensure
>> sequential-ity between the output's enable() and set_timings() op, and
>> hence ensure the manager's state we see is fine.
>>
>> If we manage the 'enabled' state for each output interface, we would be
>> a bit more consistent with respect to other parameters. For example,
>> timings is maintained by both manager and the output. Also, if we need
>> to separate out manager configurations from outputs in the future, it
>> would probably be better for the output to query it's own state rather
>> than depending on the manager, which could be configured either earlier
>> or later.
>
> Two things that came to my mind:
>
> If the output driver uses dss_mgr_is_enabled(), if both DPI and DSI
> output drivers use the same manager, they'd both see themselves as
> enabled. Of course only one can work at a time, so I'm not sure if
> that's a practical problem. And if we had some kind of link between the
> mgr and the output driver this would not be an issue.
>
> The second thing is that we're not strictly required to have DISPC
> connected to DSI or RFBI. We could use CPU/sDMA to output the image.
> This is quite theoretical, though.
>
> So, I think using dss_mgr_is_enabled() would work, but I'm still not
> 100% sure...
>
> Well, perhaps the code should be such that dss_mgr_is_enabled() is used
> to see if the mgr is enabled, not if the output is enabled. What I mean
> with this is that if, say, set_data_lines() calls dispc to set the data
> lines, we are really interested in if the dispc's mgr is enabled, not if
> the DSI is enabled.
>
> And if some other function changes DSI configuration (but doesn't touch
> dispc), then we're not really interested in if the mgr is enabled, but
> if the DSI is enabled.
>
> That's a bit more complex than using only dss_mgr_is_enabled() or using
> only output specific enable-flag, but I think it's more correct. In
> DPI's case only dss_mgr_is_enabled() is probably needed. For DSI we may
> need a separate private enable-flag.
Okay, one thing which I want to align on is that most of these functions
don't really do the actual configurations. That is, they'll just update
the private data, and the actual configuration will only happen on enable.
We would want set_timings() op to have a direct impact. But we wouldn't
want the same for setting the data lines, that could be clubbed with
other configurations at enable. That's okay, right?
Archit
^ permalink raw reply
* Re: [RFC 00/17] OMAPDSS: Change way of passing timings from panel driver to interface
From: Tomi Valkeinen @ 2012-08-08 8:13 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap, sumit.semwal, rob
In-Reply-To: <50221C4D.8000503@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1252 bytes --]
On Wed, 2012-08-08 at 13:29 +0530, Archit Taneja wrote:
> Okay, one thing which I want to align on is that most of these functions
> don't really do the actual configurations. That is, they'll just update
> the private data, and the actual configuration will only happen on enable.
>
> We would want set_timings() op to have a direct impact. But we wouldn't
> want the same for setting the data lines, that could be clubbed with
> other configurations at enable. That's okay, right?
I'm not sure we want/need set_timings to have direct impact. Changing
the timings on the fly has some problems, like the output size changing
to smaller than the overlays, and we perhaps may need to adjust the
clock dividers (dispc's, DSI PLL's or PRCM's).
It feels just much easier and safer to require that the mgr is disabled
when these changes are made. And as far as I can see, there shouldn't be
any need to change the timings via the shadow registers, as quickly as
possible and during vblank...
This makes me also think that if the output related settings can only be
changed when the output is off, the apply mechanism is not really needed
at all for these. Not that it causes any harm, but just a point I
realized.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 00/17] OMAPDSS: Change way of passing timings from panel driver to interface
From: Tomi Valkeinen @ 2012-08-08 8:48 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap, sumit.semwal, rob
In-Reply-To: <50222575.6050807@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2794 bytes --]
On Wed, 2012-08-08 at 14:08 +0530, Archit Taneja wrote:
> On Wednesday 08 August 2012 01:43 PM, Tomi Valkeinen wrote:
> > On Wed, 2012-08-08 at 13:29 +0530, Archit Taneja wrote:
> >
> >> Okay, one thing which I want to align on is that most of these functions
> >> don't really do the actual configurations. That is, they'll just update
> >> the private data, and the actual configuration will only happen on enable.
> >>
> >> We would want set_timings() op to have a direct impact. But we wouldn't
> >> want the same for setting the data lines, that could be clubbed with
> >> other configurations at enable. That's okay, right?
> >
> > I'm not sure we want/need set_timings to have direct impact. Changing
> > the timings on the fly has some problems, like the output size changing
> > to smaller than the overlays, and we perhaps may need to adjust the
> > clock dividers (dispc's, DSI PLL's or PRCM's).
> >
> > It feels just much easier and safer to require that the mgr is disabled
> > when these changes are made. And as far as I can see, there shouldn't be
> > any need to change the timings via the shadow registers, as quickly as
> > possible and during vblank...
>
> That makes sense. But currently set_timings for DPI has a direct impact.
> HDMI/VENC/SDI take the easier route of disabling and enabling the interface.
>
> I agree it's safer and easier to make sure things are disabled first,
> but maybe it's good to have the capability set hdmi timings on the fly
> in the future, it would make the switch faster, same goes for reading edid.
When do we need to switch mode quickly? Reading edid should not require
disabling the output for sure.
HDMI is a bit broken currently, though. I think we first enable the
whole stuff, including video output using VGA, then we read EDID, then
we change the mode.
We should just enable enough of HDMI to be able to read EDID, and start
the video output with the correct mode. This needs some restructuring of
the driver, though. I tried it once quickly, but it turned out not to be
trivial.
> What I meant to ask was whether we should do the same for something like
> dpi_set_data_lines(), that is, disable dpi, update the data_lines
> private data with a new value, and enable dpi again.
Hmm, I think it's better to leave disabling and enabling the output to
the panel driver. So when the panel driver wants to use
dpi_set_data_lines(), it needs to first disable the DPI output. If it
doesn't, dpi_set_data_lines() returns -EBUSY.
Otherwise if the panel driver does something like:
dpi_set_foo()
dpi_set_bar()
Both of those could first disable output, change setting, enable output.
Instead the panel should first disable, then call those, and then
enable.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 00/17] OMAPDSS: Change way of passing timings from panel driver to interface
From: Archit Taneja @ 2012-08-08 8:50 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, sumit.semwal, rob
In-Reply-To: <1344413580.4932.5.camel@deskari>
On Wednesday 08 August 2012 01:43 PM, Tomi Valkeinen wrote:
> On Wed, 2012-08-08 at 13:29 +0530, Archit Taneja wrote:
>
>> Okay, one thing which I want to align on is that most of these functions
>> don't really do the actual configurations. That is, they'll just update
>> the private data, and the actual configuration will only happen on enable.
>>
>> We would want set_timings() op to have a direct impact. But we wouldn't
>> want the same for setting the data lines, that could be clubbed with
>> other configurations at enable. That's okay, right?
>
> I'm not sure we want/need set_timings to have direct impact. Changing
> the timings on the fly has some problems, like the output size changing
> to smaller than the overlays, and we perhaps may need to adjust the
> clock dividers (dispc's, DSI PLL's or PRCM's).
>
> It feels just much easier and safer to require that the mgr is disabled
> when these changes are made. And as far as I can see, there shouldn't be
> any need to change the timings via the shadow registers, as quickly as
> possible and during vblank...
That makes sense. But currently set_timings for DPI has a direct impact.
HDMI/VENC/SDI take the easier route of disabling and enabling the interface.
I agree it's safer and easier to make sure things are disabled first,
but maybe it's good to have the capability set hdmi timings on the fly
in the future, it would make the switch faster, same goes for reading edid.
What I meant to ask was whether we should do the same for something like
dpi_set_data_lines(), that is, disable dpi, update the data_lines
private data with a new value, and enable dpi again.
>
> This makes me also think that if the output related settings can only be
> changed when the output is off, the apply mechanism is not really needed
> at all for these. Not that it causes any harm, but just a point I
> realized.
Hmm, unfortunately you are right. It's still good to have all the DISPC
writes only in APPLY though, and it gives us the option to do some
operation on the fly if needed in the future.
Archit
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox