This patch set picks up several patches sent during the past months
related with NXP TDA998x HDMI transmitter driver. The patches have
been tested on Marvell Dove (Armada DRM) on several HDMI and DVI modes
with audio playing on S/PDIF.
I bumped all patches to v2, although only patches 2, 5, and 6 have
changed. I also fixed a typo in commit line of patch 8. As Darren
Etheridge already gave his Tested-by and tilcdc related patches have
not changed, I added it to all patches for v2, too.
I have not added Russell King's Tested-by, as I hope he will have a
close look at what I changed after his review comments.
Darren Etheridge (1):
drm/tilcdc fixup mode to workaround sync for tda998x
Russell King (5):
drm/i2c: tda998x: fix EDID reading on TDA19988 devices
drm/i2c: tda998x: ensure VIP output mux is properly set
drm/i2c: tda998x: fix npix/nline programming
drm/i2c: tda998x: prepare for video input configuration
drm/i2c: tda998x: add video and audio input configuration
Sebastian Hesselbarth (2):
drm/i2c: tda998x: fix sync generation and calculation
drm/i2c: tda998x: prepare for broken sync workaround
drivers/gpu/drm/i2c/tda998x_drv.c | 481 +++++++++++++++++++++++++++------
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 7 +-
drivers/gpu/drm/tilcdc/tilcdc_slave.c | 27 +-
include/drm/i2c/tda998x.h | 30 ++
4 files changed, 467 insertions(+), 78 deletions(-)
create mode 100644 include/drm/i2c/tda998x.h
---
Cc: David Airlie <[email protected]>
Cc: Darren Etheridge <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
--
1.7.10.4
From: Russell King <[email protected]>
TDA19988 devices need their RAM enabled in order to read EDID
information. Add support for this.
Signed-off-by: Russell King <[email protected]>
Signed-off-by: Rob Clark <[email protected]>
Tested-by: Darren Etheridge <[email protected]>
Tested-by: Sebastian Hesselbarth <[email protected]>
---
Cc: David Airlie <[email protected]>
Cc: Darren Etheridge <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/gpu/drm/i2c/tda998x_drv.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index e68b58a..d71c408 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -229,6 +229,8 @@ struct tda998x_priv {
/* Page 12h: HDCP and OTP */
#define REG_TX3 REG(0x12, 0x9a) /* read/write */
+#define REG_TX4 REG(0x12, 0x9b) /* read/write */
+# define TX4_PD_RAM (1 << 1)
#define REG_TX33 REG(0x12, 0xb8) /* read/write */
# define TX33_HDMI (1 << 1)
@@ -673,6 +675,7 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
static uint8_t *
do_get_edid(struct drm_encoder *encoder)
{
+ struct tda998x_priv *priv = to_tda998x_priv(encoder);
int j = 0, valid_extensions = 0;
uint8_t *block, *new;
bool print_bad_edid = drm_debug & DRM_UT_KMS;
@@ -680,6 +683,9 @@ do_get_edid(struct drm_encoder *encoder)
if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
return NULL;
+ if (priv->rev == TDA19988)
+ reg_clear(encoder, REG_TX4, TX4_PD_RAM);
+
/* base block fetch */
if (read_edid_block(encoder, block, 0))
goto fail;
@@ -689,7 +695,7 @@ do_get_edid(struct drm_encoder *encoder)
/* if there's no extensions, we're done */
if (block[0x7e] == 0)
- return block;
+ goto done;
new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
if (!new)
@@ -716,9 +722,15 @@ do_get_edid(struct drm_encoder *encoder)
block = new;
}
+done:
+ if (priv->rev == TDA19988)
+ reg_set(encoder, REG_TX4, TX4_PD_RAM);
+
return block;
fail:
+ if (priv->rev == TDA19988)
+ reg_set(encoder, REG_TX4, TX4_PD_RAM);
dev_warn(encoder->dev->dev, "failed to read EDID\n");
kfree(block);
return NULL;
--
1.7.10.4
Some LCD controller cannot provide valid VESA style sync, i.e. coincident
HS/VS edges. First, this patch adds hskew passed from the adjusted_mode to
reference pixel calculation to allow those controllers to add an offset
relative to the expected reference pixel.
Signed-off-by: Darren Etheridge <[email protected]>
Signed-off-by: Sebastian Hesselbarth <[email protected]>
---
Changelog:
for v1:
- reword comment
Cc: David Airlie <[email protected]>
Cc: Darren Etheridge <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/gpu/drm/i2c/tda998x_drv.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 92fcb3d..c2bd711 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -782,6 +782,14 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
de_pix_s = mode->htotal - mode->hdisplay;
ref_pix = 3 + hs_pix_s;
+ /*
+ * Attached LCD controllers may generate broken sync. Allow
+ * those to adjust the position of the rising VS edge by adding
+ * HSKEW to ref_pix.
+ */
+ if (adjusted_mode->flags & DRM_MODE_FLAG_HSKEW)
+ ref_pix += adjusted_mode->hskew;
+
if ((mode->flags & DRM_MODE_FLAG_INTERLACE) == 0) {
ref_line = 1 + mode->vsync_start - mode->vdisplay;
vwin1_line_s = mode->vtotal - mode->vdisplay - 1;
--
1.7.10.4
This fixes the wrong sync generation and sync calculation of TDA998x
for HS/VS-based sync detection.
Signed-off-by: Sebastian Hesselbarth <[email protected]>
Tested-by: Darren Etheridge <[email protected]>
---
Changelog:
v1->v2:
- revert calculation of hs/de_pix_s/e (Reported by Russell King)
Cc: David Airlie <[email protected]>
Cc: Darren Etheridge <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/gpu/drm/i2c/tda998x_drv.c | 181 +++++++++++++++++++++++--------------
1 file changed, 115 insertions(+), 66 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 2b64dfa..92fcb3d 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -140,8 +140,12 @@ struct tda998x_priv {
#define REG_VS_LINE_END_1_LSB REG(0x00, 0xae) /* write */
#define REG_VS_PIX_END_1_MSB REG(0x00, 0xaf) /* write */
#define REG_VS_PIX_END_1_LSB REG(0x00, 0xb0) /* write */
+#define REG_VS_LINE_STRT_2_MSB REG(0x00, 0xb1) /* write */
+#define REG_VS_LINE_STRT_2_LSB REG(0x00, 0xb2) /* write */
#define REG_VS_PIX_STRT_2_MSB REG(0x00, 0xb3) /* write */
#define REG_VS_PIX_STRT_2_LSB REG(0x00, 0xb4) /* write */
+#define REG_VS_LINE_END_2_MSB REG(0x00, 0xb5) /* write */
+#define REG_VS_LINE_END_2_LSB REG(0x00, 0xb6) /* write */
#define REG_VS_PIX_END_2_MSB REG(0x00, 0xb7) /* write */
#define REG_VS_PIX_END_2_LSB REG(0x00, 0xb8) /* write */
#define REG_HS_PIX_START_MSB REG(0x00, 0xb9) /* write */
@@ -152,21 +156,29 @@ struct tda998x_priv {
#define REG_VWIN_START_1_LSB REG(0x00, 0xbe) /* write */
#define REG_VWIN_END_1_MSB REG(0x00, 0xbf) /* write */
#define REG_VWIN_END_1_LSB REG(0x00, 0xc0) /* write */
+#define REG_VWIN_START_2_MSB REG(0x00, 0xc1) /* write */
+#define REG_VWIN_START_2_LSB REG(0x00, 0xc2) /* write */
+#define REG_VWIN_END_2_MSB REG(0x00, 0xc3) /* write */
+#define REG_VWIN_END_2_LSB REG(0x00, 0xc4) /* write */
#define REG_DE_START_MSB REG(0x00, 0xc5) /* write */
#define REG_DE_START_LSB REG(0x00, 0xc6) /* write */
#define REG_DE_STOP_MSB REG(0x00, 0xc7) /* write */
#define REG_DE_STOP_LSB REG(0x00, 0xc8) /* write */
#define REG_TBG_CNTRL_0 REG(0x00, 0xca) /* write */
+# define TBG_CNTRL_0_TOP_TGL (1 << 0)
+# define TBG_CNTRL_0_TOP_SEL (1 << 1)
+# define TBG_CNTRL_0_DE_EXT (1 << 2)
+# define TBG_CNTRL_0_TOP_EXT (1 << 3)
# define TBG_CNTRL_0_FRAME_DIS (1 << 5)
# define TBG_CNTRL_0_SYNC_MTHD (1 << 6)
# define TBG_CNTRL_0_SYNC_ONCE (1 << 7)
#define REG_TBG_CNTRL_1 REG(0x00, 0xcb) /* write */
-# define TBG_CNTRL_1_VH_TGL_0 (1 << 0)
-# define TBG_CNTRL_1_VH_TGL_1 (1 << 1)
-# define TBG_CNTRL_1_VH_TGL_2 (1 << 2)
-# define TBG_CNTRL_1_VHX_EXT_DE (1 << 3)
-# define TBG_CNTRL_1_VHX_EXT_HS (1 << 4)
-# define TBG_CNTRL_1_VHX_EXT_VS (1 << 5)
+# define TBG_CNTRL_1_H_TGL (1 << 0)
+# define TBG_CNTRL_1_V_TGL (1 << 1)
+# define TBG_CNTRL_1_TGL_EN (1 << 2)
+# define TBG_CNTRL_1_X_EXT (1 << 3)
+# define TBG_CNTRL_1_H_EXT (1 << 4)
+# define TBG_CNTRL_1_V_EXT (1 << 5)
# define TBG_CNTRL_1_DWIN_DIS (1 << 6)
#define REG_ENABLE_SPACE REG(0x00, 0xd6) /* write */
#define REG_HVF_CNTRL_0 REG(0x00, 0xe4) /* write */
@@ -735,43 +747,70 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
struct drm_display_mode *adjusted_mode)
{
struct tda998x_priv *priv = to_tda998x_priv(encoder);
- uint16_t hs_start, hs_end, line_start, line_end;
- uint16_t vwin_start, vwin_end, de_start, de_end;
- uint16_t ref_pix, ref_line, pix_start2;
+ uint16_t ref_pix, ref_line, n_pix, n_line;
+ uint16_t hs_pix_s, hs_pix_e;
+ uint16_t vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
+ uint16_t vs2_pix_s, vs2_pix_e, vs2_line_s, vs2_line_e;
+ uint16_t vwin1_line_s, vwin1_line_e;
+ uint16_t vwin2_line_s, vwin2_line_e;
+ uint16_t de_pix_s, de_pix_e;
uint8_t reg, div, rep;
- hs_start = mode->hsync_start - mode->hdisplay;
- hs_end = mode->hsync_end - mode->hdisplay;
- line_start = 1;
- line_end = 1 + mode->vsync_end - mode->vsync_start;
- vwin_start = mode->vtotal - mode->vsync_start;
- vwin_end = vwin_start + mode->vdisplay;
- de_start = mode->htotal - mode->hdisplay;
- de_end = mode->htotal;
-
- pix_start2 = 0;
- if (mode->flags & DRM_MODE_FLAG_INTERLACE)
- pix_start2 = (mode->htotal / 2) + hs_start;
-
- /* TODO how is this value calculated? It is 2 for all common
- * formats in the tables in out of tree nxp driver (assuming
- * I've properly deciphered their byzantine table system)
+ /*
+ * Internally TDA998x is using ITU-R BT.656 style sync but
+ * we get VESA style sync. TDA998x is using a reference pixel
+ * relative to ITU to sync to the input frame and for output
+ * sync generation. Currently, we are using reference detection
+ * from HS/VS, i.e. REFPIX/REFLINE denote frame start sync point
+ * which is position of rising VS with coincident rising HS.
+ *
+ * Now there is some issues to take care of:
+ * - HDMI data islands require sync-before-active
+ * - TDA998x register values must be > 0 to be enabled
+ * - REFLINE needs an additional offset of +1
+ * - REFPIX needs an addtional offset of +1 for UYUV and +3 for RGB
+ *
+ * So we add +1 to all horizontal and vertical register values,
+ * plus an additional +3 for REFPIX as we are using RGB input only.
*/
- ref_line = 2;
-
- /* this might changes for other color formats from the CRTC: */
- ref_pix = 3 + hs_start;
+ n_pix = mode->htotal;
+ n_line = mode->vtotal;
+
+ hs_pix_e = mode->hsync_end - mode->hdisplay;
+ hs_pix_s = mode->hsync_start - mode->hdisplay;
+ de_pix_e = mode->htotal;
+ de_pix_s = mode->htotal - mode->hdisplay;
+ ref_pix = 3 + hs_pix_s;
+
+ if ((mode->flags & DRM_MODE_FLAG_INTERLACE) == 0) {
+ ref_line = 1 + mode->vsync_start - mode->vdisplay;
+ vwin1_line_s = mode->vtotal - mode->vdisplay - 1;
+ vwin1_line_e = vwin1_line_s + mode->vdisplay;
+ vs1_pix_s = vs1_pix_e = hs_pix_s;
+ vs1_line_s = mode->vsync_start - mode->vdisplay;
+ vs1_line_e = vs1_line_s +
+ mode->vsync_end - mode->vsync_start;
+ vwin2_line_s = vwin2_line_e = 0;
+ vs2_pix_s = vs2_pix_e = 0;
+ vs2_line_s = vs2_line_e = 0;
+ } else {
+ ref_line = 1 + (mode->vsync_start - mode->vdisplay)/2;
+ vwin1_line_s = (mode->vtotal - mode->vdisplay)/2;
+ vwin1_line_e = vwin1_line_s + mode->vdisplay/2;
+ vs1_pix_s = vs1_pix_e = hs_pix_s;
+ vs1_line_s = (mode->vsync_start - mode->vdisplay)/2;
+ vs1_line_e = vs1_line_s +
+ (mode->vsync_end - mode->vsync_start)/2;
+ vwin2_line_s = vwin1_line_s + mode->vtotal/2;
+ vwin2_line_e = vwin2_line_s + mode->vdisplay/2;
+ vs2_pix_s = vs2_pix_e = hs_pix_s + mode->htotal/2;
+ vs2_line_s = vs1_line_s + mode->vtotal/2 ;
+ vs2_line_e = vs2_line_s +
+ (mode->vsync_end - mode->vsync_start)/2;
+ }
div = 148500 / mode->clock;
- DBG("clock=%d, div=%u", mode->clock, div);
- DBG("hs_start=%u, hs_end=%u, line_start=%u, line_end=%u",
- hs_start, hs_end, line_start, line_end);
- DBG("vwin_start=%u, vwin_end=%u, de_start=%u, de_end=%u",
- vwin_start, vwin_end, de_start, de_end);
- DBG("ref_line=%u, ref_pix=%u, pix_start2=%u",
- ref_line, ref_pix, pix_start2);
-
/* mute the audio FIFO: */
reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
@@ -802,9 +841,6 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
reg_write(encoder, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(div) |
PLL_SERIAL_2_SRL_PR(rep));
- reg_write16(encoder, REG_VS_PIX_STRT_2_MSB, pix_start2);
- reg_write16(encoder, REG_VS_PIX_END_2_MSB, pix_start2);
-
/* set color matrix bypass flag: */
reg_set(encoder, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP);
@@ -813,46 +849,59 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
reg_clear(encoder, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_MTHD);
+ /*
+ * Sync on rising HSYNC/VSYNC
+ */
reg_write(encoder, REG_VIP_CNTRL_3, 0);
reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_SYNC_HS);
+
+ /*
+ * TDA19988 requires high-active sync at input stage,
+ * so invert low-active sync provided by master encoder here
+ */
+ if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+ reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
if (mode->flags & DRM_MODE_FLAG_NVSYNC)
reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL);
+ /*
+ * Always generate sync polarity relative to input sync and
+ * revert input stage toggled sync at output stage
+ */
+ reg = TBG_CNTRL_1_TGL_EN;
if (mode->flags & DRM_MODE_FLAG_NHSYNC)
- reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
+ reg |= TBG_CNTRL_1_H_TGL;
+ if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+ reg |= TBG_CNTRL_1_V_TGL;
+ reg_write(encoder, REG_TBG_CNTRL_1, reg);
reg_write(encoder, REG_VIDFORMAT, 0x00);
- reg_write16(encoder, REG_NPIX_MSB, mode->htotal);
- reg_write16(encoder, REG_NLINE_MSB, mode->vtotal);
- reg_write16(encoder, REG_VS_LINE_STRT_1_MSB, line_start);
- reg_write16(encoder, REG_VS_LINE_END_1_MSB, line_end);
- reg_write16(encoder, REG_VS_PIX_STRT_1_MSB, hs_start);
- reg_write16(encoder, REG_VS_PIX_END_1_MSB, hs_start);
- reg_write16(encoder, REG_HS_PIX_START_MSB, hs_start);
- reg_write16(encoder, REG_HS_PIX_STOP_MSB, hs_end);
- reg_write16(encoder, REG_VWIN_START_1_MSB, vwin_start);
- reg_write16(encoder, REG_VWIN_END_1_MSB, vwin_end);
- reg_write16(encoder, REG_DE_START_MSB, de_start);
- reg_write16(encoder, REG_DE_STOP_MSB, de_end);
+ reg_write16(encoder, REG_REFPIX_MSB, ref_pix);
+ reg_write16(encoder, REG_REFLINE_MSB, ref_line);
+ reg_write16(encoder, REG_NPIX_MSB, n_pix);
+ reg_write16(encoder, REG_NLINE_MSB, n_line);
+ reg_write16(encoder, REG_VS_LINE_STRT_1_MSB, vs1_line_s);
+ reg_write16(encoder, REG_VS_PIX_STRT_1_MSB, vs1_pix_s);
+ reg_write16(encoder, REG_VS_LINE_END_1_MSB, vs1_line_e);
+ reg_write16(encoder, REG_VS_PIX_END_1_MSB, vs1_pix_e);
+ reg_write16(encoder, REG_VS_LINE_STRT_2_MSB, vs2_line_s);
+ reg_write16(encoder, REG_VS_PIX_STRT_2_MSB, vs2_pix_s);
+ reg_write16(encoder, REG_VS_LINE_END_2_MSB, vs2_line_e);
+ reg_write16(encoder, REG_VS_PIX_END_2_MSB, vs2_pix_e);
+ reg_write16(encoder, REG_HS_PIX_START_MSB, hs_pix_s);
+ reg_write16(encoder, REG_HS_PIX_STOP_MSB, hs_pix_e);
+ reg_write16(encoder, REG_VWIN_START_1_MSB, vwin1_line_s);
+ reg_write16(encoder, REG_VWIN_END_1_MSB, vwin1_line_e);
+ reg_write16(encoder, REG_VWIN_START_2_MSB, vwin2_line_s);
+ reg_write16(encoder, REG_VWIN_END_2_MSB, vwin2_line_e);
+ reg_write16(encoder, REG_DE_START_MSB, de_pix_s);
+ reg_write16(encoder, REG_DE_STOP_MSB, de_pix_e);
if (priv->rev == TDA19988) {
/* let incoming pixels fill the active space (if any) */
reg_write(encoder, REG_ENABLE_SPACE, 0x01);
}
- reg_write16(encoder, REG_REFPIX_MSB, ref_pix);
- reg_write16(encoder, REG_REFLINE_MSB, ref_line);
-
- reg = TBG_CNTRL_1_DWIN_DIS | /* HDCP off */
- TBG_CNTRL_1_VH_TGL_2;
- /*
- * It is questionable whether this is correct - the nxp driver
- * does not set VH_TGL_2 and the below for all display modes.
- */
- if (mode->flags & (DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC))
- reg |= TBG_CNTRL_1_VH_TGL_0;
- reg_set(encoder, REG_TBG_CNTRL_1, reg);
-
/* must be last register set: */
reg_clear(encoder, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE);
--
1.7.10.4
From: Darren Etheridge <[email protected]>
Add a fixup function that will flip the hsync priority and
add a hskew value that is used to shift the tda998x to the
right by a variable number of pixels depending on the mode.
This works around an issue with the sync timings that tilcdc
is outputing.
Signed-off-by: Darren Etheridge <[email protected]>
Tested-by: Sebastian Hesselbarth <[email protected]>
---
Changelog:
v1->v2:
- fix typo in commit line (s/workaound/workaround)
Cc: David Airlie <[email protected]>
Cc: Darren Etheridge <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 7 ++++++-
drivers/gpu/drm/tilcdc/tilcdc_slave.c | 27 ++++++++++++++++++++++++++-
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 7418dcd..6d05240 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -379,7 +379,12 @@ static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
else
tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);
- if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+ /*
+ * use value from adjusted_mode here as this might have been
+ * changed as part of the fixup for slave encoders to solve the
+ * issue where tilcdc timings are not VESA compliant
+ */
+ if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
else
tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
index 0bf5999..f6e9c26 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
@@ -73,13 +73,38 @@ static void slave_encoder_prepare(struct drm_encoder *encoder)
tilcdc_crtc_set_panel_info(encoder->crtc, &slave_info);
}
+static bool slave_encoder_fixup(struct drm_encoder *encoder,
+ const struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode)
+{
+ /*
+ * tilcdc does not generate VESA-complient sync but aligns
+ * VS on the second edge of HS instead of first edge.
+ * We use adjusted_mode, to fixup sync by aligning both rising
+ * edges and add HSKEW offset to let the slave encoder fix it up.
+ */
+ adjusted_mode->hskew = mode->hsync_end - mode->hsync_start;
+ adjusted_mode->flags |= DRM_MODE_FLAG_HSKEW;
+
+ if (mode->flags & DRM_MODE_FLAG_NHSYNC) {
+ adjusted_mode->flags |= DRM_MODE_FLAG_PHSYNC;
+ adjusted_mode->flags &= ~DRM_MODE_FLAG_NHSYNC;
+ } else {
+ adjusted_mode->flags |= DRM_MODE_FLAG_NHSYNC;
+ adjusted_mode->flags &= ~DRM_MODE_FLAG_PHSYNC;
+ }
+
+ return drm_i2c_encoder_mode_fixup(encoder, mode, adjusted_mode);
+}
+
+
static const struct drm_encoder_funcs slave_encoder_funcs = {
.destroy = slave_encoder_destroy,
};
static const struct drm_encoder_helper_funcs slave_encoder_helper_funcs = {
.dpms = drm_i2c_encoder_dpms,
- .mode_fixup = drm_i2c_encoder_mode_fixup,
+ .mode_fixup = slave_encoder_fixup,
.prepare = slave_encoder_prepare,
.commit = drm_i2c_encoder_commit,
.mode_set = drm_i2c_encoder_mode_set,
--
1.7.10.4
From: Russell King <[email protected]>
This patch adds tda998x specific parameters to allow it to be configured
for different boards using it. Also, this implements rudimentary audio
support for S/PDIF attached controllers.
Signed-off-by: Russell King <[email protected]>
Signed-off-by: Sebastian Hesselbarth <[email protected]>
Tested-by: Darren Etheridge <[email protected]>
---
Changelog:
for v1:
- set AUDIO_DIV to SERCLK/16 for modes with pixclk >100MHz
- also calculate CTS
v1->v2:
- Remove CTS calculation as it isn't used in current TDA998x setup
(Reported by Russell King)
- Remove debug left-over (Reported by Russell King)
- Use correct tda998x include (Reported by Russell King)
Cc: David Airlie <[email protected]>
Cc: Darren Etheridge <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/gpu/drm/i2c/tda998x_drv.c | 268 +++++++++++++++++++++++++++++++++++--
include/drm/i2c/tda998x.h | 30 +++++
2 files changed, 290 insertions(+), 8 deletions(-)
create mode 100644 include/drm/i2c/tda998x.h
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 527d11b..2b64dfa 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -23,7 +23,7 @@
#include <drm/drm_crtc_helper.h>
#include <drm/drm_encoder_slave.h>
#include <drm/drm_edid.h>
-
+#include <drm/i2c/tda998x.h>
#define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
@@ -32,9 +32,11 @@ struct tda998x_priv {
uint16_t rev;
uint8_t current_page;
int dpms;
+ bool is_hdmi_sink;
u8 vip_cntrl_0;
u8 vip_cntrl_1;
u8 vip_cntrl_2;
+ struct tda998x_encoder_params params;
};
#define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
@@ -71,10 +73,13 @@ struct tda998x_priv {
# define I2C_MASTER_DIS_MM (1 << 0)
# define I2C_MASTER_DIS_FILT (1 << 1)
# define I2C_MASTER_APP_STRT_LAT (1 << 2)
+#define REG_FEAT_POWERDOWN REG(0x00, 0x0e) /* read/write */
+# define FEAT_POWERDOWN_SPDIF (1 << 3)
#define REG_INT_FLAGS_0 REG(0x00, 0x0f) /* read/write */
#define REG_INT_FLAGS_1 REG(0x00, 0x10) /* read/write */
#define REG_INT_FLAGS_2 REG(0x00, 0x11) /* read/write */
# define INT_FLAGS_2_EDID_BLK_RD (1 << 1)
+#define REG_ENA_ACLK REG(0x00, 0x16) /* read/write */
#define REG_ENA_VP_0 REG(0x00, 0x18) /* read/write */
#define REG_ENA_VP_1 REG(0x00, 0x19) /* read/write */
#define REG_ENA_VP_2 REG(0x00, 0x1a) /* read/write */
@@ -113,6 +118,7 @@ struct tda998x_priv {
#define REG_VIP_CNTRL_5 REG(0x00, 0x25) /* write */
# define VIP_CNTRL_5_CKCASE (1 << 0)
# define VIP_CNTRL_5_SP_CNT(x) (((x) & 3) << 1)
+#define REG_MUX_AP REG(0x00, 0x26) /* read/write */
#define REG_MUX_VP_VIP_OUT REG(0x00, 0x27) /* read/write */
#define REG_MAT_CONTRL REG(0x00, 0x80) /* write */
# define MAT_CONTRL_MAT_SC(x) (((x) & 3) << 0)
@@ -175,6 +181,12 @@ struct tda998x_priv {
# define HVF_CNTRL_1_PAD(x) (((x) & 3) << 4)
# define HVF_CNTRL_1_SEMI_PLANAR (1 << 6)
#define REG_RPT_CNTRL REG(0x00, 0xf0) /* write */
+#define REG_I2S_FORMAT REG(0x00, 0xfc) /* read/write */
+# define I2S_FORMAT(x) (((x) & 3) << 0)
+#define REG_AIP_CLKSEL REG(0x00, 0xfd) /* write */
+# define AIP_CLKSEL_FS(x) (((x) & 3) << 0)
+# define AIP_CLKSEL_CLK_POL(x) (((x) & 1) << 2)
+# define AIP_CLKSEL_AIP(x) (((x) & 7) << 3)
/* Page 02h: PLL settings */
@@ -198,6 +210,12 @@ struct tda998x_priv {
#define REG_PLL_SCGR1 REG(0x02, 0x09) /* read/write */
#define REG_PLL_SCGR2 REG(0x02, 0x0a) /* read/write */
#define REG_AUDIO_DIV REG(0x02, 0x0e) /* read/write */
+# define AUDIO_DIV_SERCLK_1 0
+# define AUDIO_DIV_SERCLK_2 1
+# define AUDIO_DIV_SERCLK_4 2
+# define AUDIO_DIV_SERCLK_8 3
+# define AUDIO_DIV_SERCLK_16 4
+# define AUDIO_DIV_SERCLK_32 5
#define REG_SEL_CLK REG(0x02, 0x11) /* read/write */
# define SEL_CLK_SEL_CLK1 (1 << 0)
# define SEL_CLK_SEL_VRF_CLK(x) (((x) & 3) << 1)
@@ -216,6 +234,11 @@ struct tda998x_priv {
/* Page 10h: information frames and packets */
+#define REG_IF1_HB0 REG(0x10, 0x20) /* read/write */
+#define REG_IF2_HB0 REG(0x10, 0x40) /* read/write */
+#define REG_IF3_HB0 REG(0x10, 0x60) /* read/write */
+#define REG_IF4_HB0 REG(0x10, 0x80) /* read/write */
+#define REG_IF5_HB0 REG(0x10, 0xa0) /* read/write */
/* Page 11h: audio settings and content info packets */
@@ -225,10 +248,33 @@ struct tda998x_priv {
# define AIP_CNTRL_0_LAYOUT (1 << 2)
# define AIP_CNTRL_0_ACR_MAN (1 << 5)
# define AIP_CNTRL_0_RST_CTS (1 << 6)
+#define REG_CA_I2S REG(0x11, 0x01) /* read/write */
+# define CA_I2S_CA_I2S(x) (((x) & 31) << 0)
+# define CA_I2S_HBR_CHSTAT (1 << 6)
+#define REG_LATENCY_RD REG(0x11, 0x04) /* read/write */
+#define REG_ACR_CTS_0 REG(0x11, 0x05) /* read/write */
+#define REG_ACR_CTS_1 REG(0x11, 0x06) /* read/write */
+#define REG_ACR_CTS_2 REG(0x11, 0x07) /* read/write */
+#define REG_ACR_N_0 REG(0x11, 0x08) /* read/write */
+#define REG_ACR_N_1 REG(0x11, 0x09) /* read/write */
+#define REG_ACR_N_2 REG(0x11, 0x0a) /* read/write */
+#define REG_CTS_N REG(0x11, 0x0c) /* read/write */
+# define CTS_N_K(x) (((x) & 7) << 0)
+# define CTS_N_M(x) (((x) & 3) << 4)
#define REG_ENC_CNTRL REG(0x11, 0x0d) /* read/write */
# define ENC_CNTRL_RST_ENC (1 << 0)
# define ENC_CNTRL_RST_SEL (1 << 1)
# define ENC_CNTRL_CTL_CODE(x) (((x) & 3) << 2)
+#define REG_DIP_FLAGS REG(0x11, 0x0e) /* read/write */
+# define DIP_FLAGS_ACR (1 << 0)
+# define DIP_FLAGS_GC (1 << 1)
+#define REG_DIP_IF_FLAGS REG(0x11, 0x0f) /* read/write */
+# define DIP_IF_FLAGS_IF1 (1 << 1)
+# define DIP_IF_FLAGS_IF2 (1 << 2)
+# define DIP_IF_FLAGS_IF3 (1 << 3)
+# define DIP_IF_FLAGS_IF4 (1 << 4)
+# define DIP_IF_FLAGS_IF5 (1 << 5)
+#define REG_CH_STAT_B(x) REG(0x11, 0x14 + (x)) /* read/write */
/* Page 12h: HDCP and OTP */
@@ -344,6 +390,23 @@ fail:
return ret;
}
+static void
+reg_write_range(struct drm_encoder *encoder, uint16_t reg, uint8_t *p, int cnt)
+{
+ struct i2c_client *client = drm_i2c_encoder_get_client(encoder);
+ uint8_t buf[cnt+1];
+ int ret;
+
+ buf[0] = REG2ADDR(reg);
+ memcpy(&buf[1], p, cnt);
+
+ set_page(encoder, reg);
+
+ ret = i2c_master_send(client, buf, cnt + 1);
+ if (ret < 0)
+ dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg);
+}
+
static uint8_t
reg_read(struct drm_encoder *encoder, uint16_t reg)
{
@@ -412,7 +475,7 @@ tda998x_reset(struct drm_encoder *encoder)
reg_write(encoder, REG_SERIALIZER, 0x00);
reg_write(encoder, REG_BUFFER_OUT, 0x00);
reg_write(encoder, REG_PLL_SCG1, 0x00);
- reg_write(encoder, REG_AUDIO_DIV, 0x03);
+ reg_write(encoder, REG_AUDIO_DIV, AUDIO_DIV_SERCLK_8);
reg_write(encoder, REG_SEL_CLK, SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK);
reg_write(encoder, REG_PLL_SCGN1, 0xfa);
reg_write(encoder, REG_PLL_SCGN2, 0x00);
@@ -424,11 +487,184 @@ tda998x_reset(struct drm_encoder *encoder)
reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24);
}
+static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
+{
+ uint8_t sum = 0;
+
+ while (bytes--)
+ sum += *buf++;
+ return (255 - sum) + 1;
+}
+
+#define HB(x) (x)
+#define PB(x) (HB(2) + 1 + (x))
+
+static void
+tda998x_write_if(struct drm_encoder *encoder, uint8_t bit, uint16_t addr,
+ uint8_t *buf, size_t size)
+{
+ buf[PB(0)] = tda998x_cksum(buf, size);
+
+ reg_clear(encoder, REG_DIP_IF_FLAGS, bit);
+ reg_write_range(encoder, addr, buf, size);
+ reg_set(encoder, REG_DIP_IF_FLAGS, bit);
+}
+
+static void
+tda998x_write_aif(struct drm_encoder *encoder, struct tda998x_encoder_params *p)
+{
+ uint8_t buf[PB(5) + 1];
+
+ buf[HB(0)] = 0x84;
+ buf[HB(1)] = 0x01;
+ buf[HB(2)] = 10;
+ buf[PB(0)] = 0;
+ buf[PB(1)] = p->audio_frame[1] & 0x07; /* CC */
+ buf[PB(2)] = p->audio_frame[2] & 0x1c; /* SF */
+ buf[PB(4)] = p->audio_frame[4];
+ buf[PB(5)] = p->audio_frame[5] & 0xf8; /* DM_INH + LSV */
+
+ tda998x_write_if(encoder, DIP_IF_FLAGS_IF4, REG_IF4_HB0, buf,
+ sizeof(buf));
+}
+
+static void
+tda998x_write_avi(struct drm_encoder *encoder, struct drm_display_mode *mode)
+{
+ uint8_t buf[PB(13) + 1];
+
+ memset(buf, 0, sizeof(buf));
+ buf[HB(0)] = 0x82;
+ buf[HB(1)] = 0x02;
+ buf[HB(2)] = 13;
+ buf[PB(4)] = drm_match_cea_mode(mode);
+
+ tda998x_write_if(encoder, DIP_IF_FLAGS_IF2, REG_IF2_HB0, buf,
+ sizeof(buf));
+}
+
+static void tda998x_audio_mute(struct drm_encoder *encoder, bool on)
+{
+ if (on) {
+ reg_set(encoder, REG_SOFTRESET, SOFTRESET_AUDIO);
+ reg_clear(encoder, REG_SOFTRESET, SOFTRESET_AUDIO);
+ reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
+ } else {
+ reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
+ }
+}
+
+static void
+tda998x_configure_audio(struct drm_encoder *encoder,
+ struct drm_display_mode *mode, struct tda998x_encoder_params *p)
+{
+ uint8_t buf[6], clksel_aip, clksel_fs, ca_i2s, cts_n, adiv;
+ uint32_t n;
+
+ /* Enable audio ports */
+ reg_write(encoder, REG_ENA_AP, p->audio_cfg);
+ reg_write(encoder, REG_ENA_ACLK, p->audio_clk_cfg);
+
+ /* Set audio input source */
+ switch (p->audio_format) {
+ case AFMT_SPDIF:
+ reg_write(encoder, REG_MUX_AP, 0x40);
+ clksel_aip = AIP_CLKSEL_AIP(0);
+ /* FS64SPDIF */
+ clksel_fs = AIP_CLKSEL_FS(2);
+ cts_n = CTS_N_M(3) | CTS_N_K(3);
+ ca_i2s = 0;
+ break;
+
+ case AFMT_I2S:
+ reg_write(encoder, REG_MUX_AP, 0x64);
+ clksel_aip = AIP_CLKSEL_AIP(1);
+ /* ACLK */
+ clksel_fs = AIP_CLKSEL_FS(0);
+ cts_n = CTS_N_M(3) | CTS_N_K(3);
+ ca_i2s = CA_I2S_CA_I2S(0);
+ break;
+ }
+
+ reg_write(encoder, REG_AIP_CLKSEL, clksel_aip);
+ reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT);
+
+ /* Enable automatic CTS generation */
+ reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_ACR_MAN);
+ reg_write(encoder, REG_CTS_N, cts_n);
+
+ /*
+ * Audio input somehow depends on HDMI line rate which is
+ * related to pixclk. Testing showed that modes with pixclk
+ * >100MHz need a larger divider while <40MHz need the default.
+ * There is no detailed info in the datasheet, so we just
+ * assume 100MHz requires larger divider.
+ */
+ if (mode->clock > 100000)
+ adiv = AUDIO_DIV_SERCLK_16;
+ else
+ adiv = AUDIO_DIV_SERCLK_8;
+ reg_write(encoder, REG_AUDIO_DIV, adiv);
+
+ /*
+ * This is the approximate value of N, which happens to be
+ * the recommended values for non-coherent clocks.
+ */
+ n = 128 * p->audio_sample_rate / 1000;
+
+ /* Write the CTS and N values */
+ buf[0] = 0x44;
+ buf[1] = 0x42;
+ buf[2] = 0x01;
+ buf[3] = n;
+ buf[4] = n >> 8;
+ buf[5] = n >> 16;
+ reg_write_range(encoder, REG_ACR_CTS_0, buf, 6);
+
+ /* Set CTS clock reference */
+ reg_write(encoder, REG_AIP_CLKSEL, clksel_aip | clksel_fs);
+
+ /* Reset CTS generator */
+ reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
+ reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
+
+ /* Write the channel status */
+ buf[0] = 0x04;
+ buf[1] = 0x00;
+ buf[2] = 0x00;
+ buf[3] = 0xf1;
+ reg_write_range(encoder, REG_CH_STAT_B(0), buf, 4);
+
+ tda998x_audio_mute(encoder, true);
+ mdelay(20);
+ tda998x_audio_mute(encoder, false);
+
+ /* Write the audio information packet */
+ tda998x_write_aif(encoder, p);
+}
+
/* DRM encoder functions */
static void
tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
{
+ struct tda998x_priv *priv = to_tda998x_priv(encoder);
+ struct tda998x_encoder_params *p = params;
+
+ priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
+ (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) |
+ VIP_CNTRL_0_SWAP_B(p->swap_b) |
+ (p->mirr_b ? VIP_CNTRL_0_MIRR_B : 0);
+ priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(p->swap_c) |
+ (p->mirr_c ? VIP_CNTRL_1_MIRR_C : 0) |
+ VIP_CNTRL_1_SWAP_D(p->swap_d) |
+ (p->mirr_d ? VIP_CNTRL_1_MIRR_D : 0);
+ priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(p->swap_e) |
+ (p->mirr_e ? VIP_CNTRL_2_MIRR_E : 0) |
+ VIP_CNTRL_2_SWAP_F(p->swap_f) |
+ (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
+
+ priv->params = *p;
}
static void
@@ -445,8 +681,7 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
switch (mode) {
case DRM_MODE_DPMS_ON:
- /* enable audio and video ports */
- reg_write(encoder, REG_ENA_AP, 0xff);
+ /* enable video ports, audio will be enabled later */
reg_write(encoder, REG_ENA_VP_0, 0xff);
reg_write(encoder, REG_ENA_VP_1, 0xff);
reg_write(encoder, REG_ENA_VP_2, 0xff);
@@ -608,17 +843,32 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
reg_write16(encoder, REG_REFPIX_MSB, ref_pix);
reg_write16(encoder, REG_REFLINE_MSB, ref_line);
- reg = TBG_CNTRL_1_VHX_EXT_DE |
- TBG_CNTRL_1_VHX_EXT_HS |
- TBG_CNTRL_1_VHX_EXT_VS |
- TBG_CNTRL_1_DWIN_DIS | /* HDCP off */
+ reg = TBG_CNTRL_1_DWIN_DIS | /* HDCP off */
TBG_CNTRL_1_VH_TGL_2;
+ /*
+ * It is questionable whether this is correct - the nxp driver
+ * does not set VH_TGL_2 and the below for all display modes.
+ */
if (mode->flags & (DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC))
reg |= TBG_CNTRL_1_VH_TGL_0;
reg_set(encoder, REG_TBG_CNTRL_1, reg);
/* must be last register set: */
reg_clear(encoder, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE);
+
+ /* Only setup the info frames if the sink is HDMI */
+ if (priv->is_hdmi_sink) {
+ /* We need to turn HDMI HDCP stuff on to get audio through */
+ reg_clear(encoder, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS);
+ reg_write(encoder, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(1));
+ reg_set(encoder, REG_TX33, TX33_HDMI);
+
+ tda998x_write_avi(encoder, adjusted_mode);
+
+ if (priv->params.audio_cfg)
+ tda998x_configure_audio(encoder, adjusted_mode,
+ &priv->params);
+ }
}
static enum drm_connector_status
@@ -744,12 +994,14 @@ static int
tda998x_encoder_get_modes(struct drm_encoder *encoder,
struct drm_connector *connector)
{
+ struct tda998x_priv *priv = to_tda998x_priv(encoder);
struct edid *edid = (struct edid *)do_get_edid(encoder);
int n = 0;
if (edid) {
drm_mode_connector_update_edid_property(connector, edid);
n = drm_add_edid_modes(connector, edid);
+ priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid);
kfree(edid);
}
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
new file mode 100644
index 0000000..3e419d9
--- /dev/null
+++ b/include/drm/i2c/tda998x.h
@@ -0,0 +1,30 @@
+#ifndef __DRM_I2C_TDA998X_H__
+#define __DRM_I2C_TDA998X_H__
+
+struct tda998x_encoder_params {
+ u8 swap_b:3;
+ u8 mirr_b:1;
+ u8 swap_a:3;
+ u8 mirr_a:1;
+ u8 swap_d:3;
+ u8 mirr_d:1;
+ u8 swap_c:3;
+ u8 mirr_c:1;
+ u8 swap_f:3;
+ u8 mirr_f:1;
+ u8 swap_e:3;
+ u8 mirr_e:1;
+
+ u8 audio_cfg;
+ u8 audio_clk_cfg;
+ u8 audio_frame[6];
+
+ enum {
+ AFMT_SPDIF,
+ AFMT_I2S
+ } audio_format;
+
+ unsigned audio_sample_rate;
+};
+
+#endif
--
1.7.10.4
From: Russell King <[email protected]>
The video-input-port (VIP) is highly configurable. This prepares
current driver to allow to configure VIP configuration, as some
boards connect lcd controller and TDA998x "pin-swapped" and depend
on VIP to swap the pins by register configuration.
Signed-off-by: Russell King <[email protected]>
Tested-by: Darren Etheridge <[email protected]>
Tested-by: Sebastian Hesselbarth <[email protected]>
---
Cc: David Airlie <[email protected]>
Cc: Darren Etheridge <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/gpu/drm/i2c/tda998x_drv.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a701411..527d11b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -32,6 +32,9 @@ struct tda998x_priv {
uint16_t rev;
uint8_t current_page;
int dpms;
+ u8 vip_cntrl_0;
+ u8 vip_cntrl_1;
+ u8 vip_cntrl_2;
};
#define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
@@ -448,12 +451,9 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
reg_write(encoder, REG_ENA_VP_1, 0xff);
reg_write(encoder, REG_ENA_VP_2, 0xff);
/* set muxing after enabling ports: */
- reg_write(encoder, REG_VIP_CNTRL_0,
- VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3));
- reg_write(encoder, REG_VIP_CNTRL_1,
- VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1));
- reg_write(encoder, REG_VIP_CNTRL_2,
- VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5));
+ reg_write(encoder, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
+ reg_write(encoder, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
+ reg_write(encoder, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
break;
case DRM_MODE_DPMS_OFF:
/* disable audio and video ports */
@@ -823,6 +823,10 @@ tda998x_encoder_init(struct i2c_client *client,
if (!priv)
return -ENOMEM;
+ priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
+ priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
+ priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
+
priv->current_page = 0;
priv->cec = i2c_new_dummy(client->adapter, 0x34);
priv->dpms = DRM_MODE_DPMS_OFF;
--
1.7.10.4
From: Russell King <[email protected]>
The npix/nline registers are supposed to be programmed with the total
number of pixels/lines, not the displayed pixels/lines, and not minus
one either.
Signed-off-by: Russell King <[email protected]>
Tested-by: Darren Etheridge <[email protected]>
Tested-by: Sebastian Hesselbarth <[email protected]>
---
Cc: David Airlie <[email protected]>
Cc: Darren Etheridge <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/gpu/drm/i2c/tda998x_drv.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index cb9b13a..a701411 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -587,8 +587,8 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
reg_write(encoder, REG_VIDFORMAT, 0x00);
- reg_write16(encoder, REG_NPIX_MSB, mode->hdisplay - 1);
- reg_write16(encoder, REG_NLINE_MSB, mode->vdisplay - 1);
+ reg_write16(encoder, REG_NPIX_MSB, mode->htotal);
+ reg_write16(encoder, REG_NLINE_MSB, mode->vtotal);
reg_write16(encoder, REG_VS_LINE_STRT_1_MSB, line_start);
reg_write16(encoder, REG_VS_LINE_END_1_MSB, line_end);
reg_write16(encoder, REG_VS_PIX_STRT_1_MSB, hs_start);
--
1.7.10.4
From: Russell King <[email protected]>
When switching between various drivers for this device, it's possible
that some critical registers are left containing values which affect
the device operation. One such case encountered is the VIP output
mux register. This defaults to 0x24 on powerup, but other drivers may
set this to 0x12. This results in incorrect colours.
Fix this by ensuring that the register is always set to the power on
default setting.
Signed-off-by: Russell King <[email protected]>
Tested-by: Darren Etheridge <[email protected]>
Tested-by: Sebastian Hesselbarth <[email protected]>
---
Changelog:
v1->v2:
- move reg_write to tda998x_reset as last patch was based on an
old version (Reported by Russell King)
Cc: David Airlie <[email protected]>
Cc: Darren Etheridge <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Russell King <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/gpu/drm/i2c/tda998x_drv.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index d71c408..cb9b13a 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -110,6 +110,7 @@ struct tda998x_priv {
#define REG_VIP_CNTRL_5 REG(0x00, 0x25) /* write */
# define VIP_CNTRL_5_CKCASE (1 << 0)
# define VIP_CNTRL_5_SP_CNT(x) (((x) & 3) << 1)
+#define REG_MUX_VP_VIP_OUT REG(0x00, 0x27) /* read/write */
#define REG_MAT_CONTRL REG(0x00, 0x80) /* write */
# define MAT_CONTRL_MAT_SC(x) (((x) & 3) << 0)
# define MAT_CONTRL_MAT_BP (1 << 2)
@@ -415,6 +416,9 @@ tda998x_reset(struct drm_encoder *encoder)
reg_write(encoder, REG_PLL_SCGR1, 0x5b);
reg_write(encoder, REG_PLL_SCGR2, 0x00);
reg_write(encoder, REG_PLL_SCG2, 0x10);
+
+ /* Write the default value MUX register */
+ reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24);
}
/* DRM encoder functions */
--
1.7.10.4
On Wed, Aug 14, 2013 at 09:43:25PM +0200, Sebastian Hesselbarth wrote:
> This patch set picks up several patches sent during the past months
> related with NXP TDA998x HDMI transmitter driver. The patches have
> been tested on Marvell Dove (Armada DRM) on several HDMI and DVI modes
> with audio playing on S/PDIF.
>
> I bumped all patches to v2, although only patches 2, 5, and 6 have
> changed. I also fixed a typo in commit line of patch 8. As Darren
> Etheridge already gave his Tested-by and tilcdc related patches have
> not changed, I added it to all patches for v2, too.
>
> I have not added Russell King's Tested-by, as I hope he will have a
> close look at what I changed after his review comments.
>
> Darren Etheridge (1):
> drm/tilcdc fixup mode to workaround sync for tda998x
>
> Russell King (5):
> drm/i2c: tda998x: fix EDID reading on TDA19988 devices
> drm/i2c: tda998x: ensure VIP output mux is properly set
> drm/i2c: tda998x: fix npix/nline programming
> drm/i2c: tda998x: prepare for video input configuration
> drm/i2c: tda998x: add video and audio input configuration
>
> Sebastian Hesselbarth (2):
> drm/i2c: tda998x: fix sync generation and calculation
> drm/i2c: tda998x: prepare for broken sync workaround
>
> drivers/gpu/drm/i2c/tda998x_drv.c | 481 +++++++++++++++++++++++++++------
> drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 7 +-
> drivers/gpu/drm/tilcdc/tilcdc_slave.c | 27 +-
> include/drm/i2c/tda998x.h | 30 ++
> 4 files changed, 467 insertions(+), 78 deletions(-)
> create mode 100644 include/drm/i2c/tda998x.h
>
> ---
> Cc: David Airlie <[email protected]>
> Cc: Darren Etheridge <[email protected]>
> Cc: Rob Clark <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
Okay, I'm happy with this set. Patches 1-7 can get a:
Tested-by: Russell King <[email protected]>
Thanks.
On Thu, Aug 15, 2013 at 5:43 AM, Sebastian Hesselbarth
<[email protected]> wrote:
> From: Russell King <[email protected]>
>
> This patch adds tda998x specific parameters to allow it to be configured
> for different boards using it. Also, this implements rudimentary audio
> support for S/PDIF attached controllers.
>
> Signed-off-by: Russell King <[email protected]>
> Signed-off-by: Sebastian Hesselbarth <[email protected]>
> Tested-by: Darren Etheridge <[email protected]>
> ---
I've merged the series,
this one generates a warning though:
CC [M] drivers/gpu/drm/i2c/tda998x_drv.o
/home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:
In function ?tda998x_encoder_mode_set?:
/home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:637:11:
warning: ?clksel_fs? may be used uninitialized in this function
[-Wmaybe-uninitialized]
/home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:573:30:
note: ?clksel_fs? was declared here
/home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:637:11:
warning: ?clksel_aip? may be used uninitialized in this function
[-Wmaybe-uninitialized]
/home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:573:18:
note: ?clksel_aip? was declared here
It doesn't seem like a real problem, since the function is unlikely to
be called any way to make that case happen.
Dave.
On Mon, Aug 19, 2013 at 09:23:17AM +1000, Dave Airlie wrote:
> On Thu, Aug 15, 2013 at 5:43 AM, Sebastian Hesselbarth
> <[email protected]> wrote:
> > From: Russell King <[email protected]>
> >
> > This patch adds tda998x specific parameters to allow it to be configured
> > for different boards using it. Also, this implements rudimentary audio
> > support for S/PDIF attached controllers.
> >
> > Signed-off-by: Russell King <[email protected]>
> > Signed-off-by: Sebastian Hesselbarth <[email protected]>
> > Tested-by: Darren Etheridge <[email protected]>
> > ---
>
> I've merged the series,
Thanks.
> this one generates a warning though:
> CC [M] drivers/gpu/drm/i2c/tda998x_drv.o
> /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:
> In function ‘tda998x_encoder_mode_set’:
> /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:637:11:
> warning: ‘clksel_fs’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:573:30:
> note: ‘clksel_fs’ was declared here
> /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:637:11:
> warning: ‘clksel_aip’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:573:18:
> note: ‘clksel_aip’ was declared here
>
> It doesn't seem like a real problem, since the function is unlikely to
> be called any way to make that case happen.
Ok, I'll squash those warnings by a slight rearrangement of the code.
On Wed Aug 14 12:43:29 PDT 2013, Sebastian Hesselbarth wrote:
> From: Russell King <rmk+kernel at arm.linux.org.uk>
>
> The video-input-port (VIP) is highly configurable. This prepares
> current driver to allow to configure VIP configuration, as some
> boards connect lcd controller and TDA998x "pin-swapped" and depend
> on VIP to swap the pins by register configuration.
>
> Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
> Tested-by: Darren Etheridge <detheridge at ti.com>
> Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth at gmail.com>
[snip]
AFAIK, the TI boards have no "pin-swapped", nor has the Cubox (there is
no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
of the Dove lcd for RGB or YUV formats).
Which board needs a special VIP configuration?
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
On Wed Aug 14 12:43:30 PDT 2013, Sebastian Hesselbarth wrote:
> From: Russell King <rmk+kernel at arm.linux.org.uk>
>
> This patch adds tda998x specific parameters to allow it to be configured
> for different boards using it. Also, this implements rudimentary audio
> support for S/PDIF attached controllers.
It seems that this patch mixes two different functions:
- configuration
- audio
About configuration, why don't you use the standard way to set the
device parameters, i.e. board info and DT?
> Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth at gmail.com>
> Tested-by: Darren Etheridge <detheridge at ti.com>
> ---
> Changelog:
> for v1:
> - set AUDIO_DIV to SERCLK/16 for modes with pixclk >100MHz
> - also calculate CTS
> v1->v2:
> - Remove CTS calculation as it isn't used in current TDA998x setup
> (Reported by Russell King)
> - Remove debug left-over (Reported by Russell King)
> - Use correct tda998x include (Reported by Russell King)
>
> Cc: David Airlie <airlied at linux.ie>
> Cc: Darren Etheridge <detheridge at ti.com>
> Cc: Rob Clark <robdclark at gmail.com>
> Cc: Russell King <rmk+kernel at arm.linux.org.uk>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: dri-devel at lists.freedesktop.org
> Cc: linux-kernel at vger.kernel.org
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 268 +++++++++++++++++++++++++++++++++++--
> include/drm/i2c/tda998x.h | 30 +++++
> 2 files changed, 290 insertions(+), 8 deletions(-)
> create mode 100644 include/drm/i2c/tda998x.h
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 527d11b..2b64dfa 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
[snip]
> @@ -344,6 +390,23 @@ fail:
> return ret;
> }
>
> +static void
> +reg_write_range(struct drm_encoder *encoder, uint16_t reg, uint8_t *p, int cnt)
> +{
> + struct i2c_client *client = drm_i2c_encoder_get_client(encoder);
> + uint8_t buf[cnt+1];
> + int ret;
> +
> + buf[0] = REG2ADDR(reg);
> + memcpy(&buf[1], p, cnt);
> +
> + set_page(encoder, reg);
> +
> + ret = i2c_master_send(client, buf, cnt + 1);
> + if (ret < 0)
> + dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg);
> +}
> +
It seems simpler to reserve one byte in the caller buffer for the
register address and avoid a memcpy.
> static uint8_t
> reg_read(struct drm_encoder *encoder, uint16_t reg)
> {
> @@ -412,7 +475,7 @@ tda998x_reset(struct drm_encoder *encoder)
> reg_write(encoder, REG_SERIALIZER, 0x00);
> reg_write(encoder, REG_BUFFER_OUT, 0x00);
> reg_write(encoder, REG_PLL_SCG1, 0x00);
> - reg_write(encoder, REG_AUDIO_DIV, 0x03);
> + reg_write(encoder, REG_AUDIO_DIV, AUDIO_DIV_SERCLK_8);
> reg_write(encoder, REG_SEL_CLK, SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK);
> reg_write(encoder, REG_PLL_SCGN1, 0xfa);
> reg_write(encoder, REG_PLL_SCGN2, 0x00);
> @@ -424,11 +487,184 @@ tda998x_reset(struct drm_encoder *encoder)
> reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24);
> }
>
> +static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
> +{
> + uint8_t sum = 0;
> +
> + while (bytes--)
> + sum += *buf++;
> + return (255 - sum) + 1;
> +}
Simpler:
static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
{
int sum = 0; /* avoid byte computation */
while (bytes--)
sum -= *buf++; /* avoid a substraction */
return sum;
}
> +
> +#define HB(x) (x)
> +#define PB(x) (HB(2) + 1 + (x))
> +
> +static void
> +tda998x_write_if(struct drm_encoder *encoder, uint8_t bit, uint16_t addr,
> + uint8_t *buf, size_t size)
> +{
> + buf[PB(0)] = tda998x_cksum(buf, size);
> +
> + reg_clear(encoder, REG_DIP_IF_FLAGS, bit);
> + reg_write_range(encoder, addr, buf, size);
> + reg_set(encoder, REG_DIP_IF_FLAGS, bit);
> +}
> +
> +static void
> +tda998x_write_aif(struct drm_encoder *encoder, struct tda998x_encoder_params *p)
> +{
> + uint8_t buf[PB(5) + 1];
> +
> + buf[HB(0)] = 0x84;
> + buf[HB(1)] = 0x01;
> + buf[HB(2)] = 10;
Why don't you use the constants which are defined in hdmi.h?
> + buf[PB(0)] = 0;
> + buf[PB(1)] = p->audio_frame[1] & 0x07; /* CC */
> + buf[PB(2)] = p->audio_frame[2] & 0x1c; /* SF */
> + buf[PB(4)] = p->audio_frame[4];
> + buf[PB(5)] = p->audio_frame[5] & 0xf8; /* DM_INH + LSV */
> +
> + tda998x_write_if(encoder, DIP_IF_FLAGS_IF4, REG_IF4_HB0, buf,
> + sizeof(buf));
> +}
> +
> +static void
> +tda998x_write_avi(struct drm_encoder *encoder, struct drm_display_mode *mode)
> +{
> + uint8_t buf[PB(13) + 1];
> +
> + memset(buf, 0, sizeof(buf));
> + buf[HB(0)] = 0x82;
> + buf[HB(1)] = 0x02;
> + buf[HB(2)] = 13;
> + buf[PB(4)] = drm_match_cea_mode(mode);
> +
> + tda998x_write_if(encoder, DIP_IF_FLAGS_IF2, REG_IF2_HB0, buf,
> + sizeof(buf));
> +}
> +
> +static void tda998x_audio_mute(struct drm_encoder *encoder, bool on)
> +{
> + if (on) {
> + reg_set(encoder, REG_SOFTRESET, SOFTRESET_AUDIO);
> + reg_clear(encoder, REG_SOFTRESET, SOFTRESET_AUDIO);
> + reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
> + } else {
> + reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
> + }
> +}
> +
> +static void
> +tda998x_configure_audio(struct drm_encoder *encoder,
> + struct drm_display_mode *mode, struct tda998x_encoder_params *p)
> +{
> + uint8_t buf[6], clksel_aip, clksel_fs, ca_i2s, cts_n, adiv;
> + uint32_t n;
> +
> + /* Enable audio ports */
> + reg_write(encoder, REG_ENA_AP, p->audio_cfg);
> + reg_write(encoder, REG_ENA_ACLK, p->audio_clk_cfg);
> +
> + /* Set audio input source */
> + switch (p->audio_format) {
> + case AFMT_SPDIF:
> + reg_write(encoder, REG_MUX_AP, 0x40);
> + clksel_aip = AIP_CLKSEL_AIP(0);
> + /* FS64SPDIF */
> + clksel_fs = AIP_CLKSEL_FS(2);
> + cts_n = CTS_N_M(3) | CTS_N_K(3);
> + ca_i2s = 0;
> + break;
> +
> + case AFMT_I2S:
> + reg_write(encoder, REG_MUX_AP, 0x64);
> + clksel_aip = AIP_CLKSEL_AIP(1);
> + /* ACLK */
> + clksel_fs = AIP_CLKSEL_FS(0);
> + cts_n = CTS_N_M(3) | CTS_N_K(3);
> + ca_i2s = CA_I2S_CA_I2S(0);
> + break;
> + }
> +
> + reg_write(encoder, REG_AIP_CLKSEL, clksel_aip);
> + reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT);
> +
> + /* Enable automatic CTS generation */
> + reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_ACR_MAN);
> + reg_write(encoder, REG_CTS_N, cts_n);
> +
> + /*
> + * Audio input somehow depends on HDMI line rate which is
> + * related to pixclk. Testing showed that modes with pixclk
> + * >100MHz need a larger divider while <40MHz need the default.
> + * There is no detailed info in the datasheet, so we just
> + * assume 100MHz requires larger divider.
> + */
> + if (mode->clock > 100000)
> + adiv = AUDIO_DIV_SERCLK_16;
> + else
> + adiv = AUDIO_DIV_SERCLK_8;
> + reg_write(encoder, REG_AUDIO_DIV, adiv);
> +
> + /*
> + * This is the approximate value of N, which happens to be
> + * the recommended values for non-coherent clocks.
> + */
> + n = 128 * p->audio_sample_rate / 1000;
> +
> + /* Write the CTS and N values */
> + buf[0] = 0x44;
> + buf[1] = 0x42;
> + buf[2] = 0x01;
The CTS value is strange, but that does not matter: its generation is
automatic (see above).
> + buf[3] = n;
> + buf[4] = n >> 8;
> + buf[5] = n >> 16;
> + reg_write_range(encoder, REG_ACR_CTS_0, buf, 6);
> +
> + /* Set CTS clock reference */
> + reg_write(encoder, REG_AIP_CLKSEL, clksel_aip | clksel_fs);
> +
> + /* Reset CTS generator */
> + reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
> + reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
> +
> + /* Write the channel status */
> + buf[0] = 0x04;
> + buf[1] = 0x00;
> + buf[2] = 0x00;
> + buf[3] = 0xf1;
> + reg_write_range(encoder, REG_CH_STAT_B(0), buf, 4);
[snip]
>From what I understood in the NXP driver, buf[3] depends on the sample
rate: 0xf1 for 44.1kHz and 0xd1 for 48kHz.
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
On Wed, Aug 21, 2013 at 08:26:46PM +0200, Jean-Francois Moine wrote:
> On Wed Aug 14 12:43:29 PDT 2013, Sebastian Hesselbarth wrote:
> > From: Russell King <rmk+kernel at arm.linux.org.uk>
> >
> > The video-input-port (VIP) is highly configurable. This prepares
> > current driver to allow to configure VIP configuration, as some
> > boards connect lcd controller and TDA998x "pin-swapped" and depend
> > on VIP to swap the pins by register configuration.
> >
> > Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
> > Tested-by: Darren Etheridge <detheridge at ti.com>
> > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth at gmail.com>
> [snip]
>
> AFAIK, the TI boards have no "pin-swapped", nor has the Cubox (there is
> no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
> of the Dove lcd for RGB or YUV formats).
>
> Which board needs a special VIP configuration?
If you run the NXP driver, and then run this driver, things get messed
up - which has already been covered months ago when this patch was first
brought up.
It's there to ensure that the TDA998x is correctly configured no matter
what it's previous state is, and prevent the thing being fragile as hell.
No, reset doesn't restore its settings, only a power cycle does.
On Wed, Aug 21, 2013 at 08:33:55PM +0200, Jean-Francois Moine wrote:
> On Wed Aug 14 12:43:30 PDT 2013, Sebastian Hesselbarth wrote:
> > +static void
> > +reg_write_range(struct drm_encoder *encoder, uint16_t reg, uint8_t *p, int cnt)
> > +{
> > + struct i2c_client *client = drm_i2c_encoder_get_client(encoder);
> > + uint8_t buf[cnt+1];
> > + int ret;
> > +
> > + buf[0] = REG2ADDR(reg);
> > + memcpy(&buf[1], p, cnt);
> > +
> > + set_page(encoder, reg);
> > +
> > + ret = i2c_master_send(client, buf, cnt + 1);
> > + if (ret < 0)
> > + dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg);
> > +}
> > +
>
> It seems simpler to reserve one byte in the caller buffer for the
> register address and avoid a memcpy.
And by doing so create an obtuse API. No thanks.
> > +static void
> > +tda998x_write_aif(struct drm_encoder *encoder, struct tda998x_encoder_params *p)
> > +{
> > + uint8_t buf[PB(5) + 1];
> > +
> > + buf[HB(0)] = 0x84;
> > + buf[HB(1)] = 0x01;
> > + buf[HB(2)] = 10;
>
> Why don't you use the constants which are defined in hdmi.h?
Because I was not aware of them.
> > + /* Write the CTS and N values */
> > + buf[0] = 0x44;
> > + buf[1] = 0x42;
> > + buf[2] = 0x01;
>
> The CTS value is strange, but that does not matter: its generation is
> automatic (see above).
Correct - however not strange, if you've read the HDMI specification.
> > + buf[3] = n;
> > + buf[4] = n >> 8;
> > + buf[5] = n >> 16;
> > + reg_write_range(encoder, REG_ACR_CTS_0, buf, 6);
> > +
> > + /* Set CTS clock reference */
> > + reg_write(encoder, REG_AIP_CLKSEL, clksel_aip | clksel_fs);
> > +
> > + /* Reset CTS generator */
> > + reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
> > + reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
> > +
> > + /* Write the channel status */
> > + buf[0] = 0x04;
> > + buf[1] = 0x00;
> > + buf[2] = 0x00;
> > + buf[3] = 0xf1;
> > + reg_write_range(encoder, REG_CH_STAT_B(0), buf, 4);
> [snip]
>
> From what I understood in the NXP driver, buf[3] depends on the sample
> rate: 0xf1 for 44.1kHz and 0xd1 for 48kHz.
Correct, but the driver has no way to know this. The above reflects how
the NXP driver on the Cubox implements this (which we know works for
multiple different sample rates). This is because we use SPDIF, which
encodes the sample rate in the channel status information, which is then
presumably passed through by the TDA998x. I wouldn't know, I don't have
a HDMI debugger.
What I do know is that it works for multiple sample rates.
On Wed, 21 Aug 2013 23:36:05 +0100
Russell King - ARM Linux <[email protected]> wrote:
> > AFAIK, the TI boards have no "pin-swapped", nor has the Cubox (there is
> > no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
> > of the Dove lcd for RGB or YUV formats).
> >
> > Which board needs a special VIP configuration?
>
> If you run the NXP driver, and then run this driver, things get messed
> up - which has already been covered months ago when this patch was first
> brought up.
>
> It's there to ensure that the TDA998x is correctly configured no matter
> what it's previous state is, and prevent the thing being fragile as hell.
The NXP driver will never go to the mainline, so, I don't see the
problem. If you want to use it to test some other drivers, you should
better patch it instead of adding useless code in the TDA998x driver.
> No, reset doesn't restore its settings, only a power cycle does.
Sorry, all VIP control registers may be changed at any time and the
change appears immediately (thank you for the /sys i2c_read/write).
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
On Thu, Aug 22, 2013 at 08:53:13AM +0200, Jean-Francois Moine wrote:
> On Wed, 21 Aug 2013 23:36:05 +0100
> Russell King - ARM Linux <[email protected]> wrote:
>
> > > AFAIK, the TI boards have no "pin-swapped", nor has the Cubox (there is
> > > no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
> > > of the Dove lcd for RGB or YUV formats).
> > >
> > > Which board needs a special VIP configuration?
> >
> > If you run the NXP driver, and then run this driver, things get messed
> > up - which has already been covered months ago when this patch was first
> > brought up.
> >
> > It's there to ensure that the TDA998x is correctly configured no matter
> > what it's previous state is, and prevent the thing being fragile as hell.
>
> The NXP driver will never go to the mainline, so, I don't see the
> problem. If you want to use it to test some other drivers, you should
> better patch it instead of adding useless code in the TDA998x driver.
Sorry, you're wrong.
On Thu, Aug 22, 2013 at 2:53 AM, Jean-Francois Moine <[email protected]> wrote:
> On Wed, 21 Aug 2013 23:36:05 +0100
> Russell King - ARM Linux <[email protected]> wrote:
>
>> > AFAIK, the TI boards have no "pin-swapped", nor has the Cubox (there is
>> > no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
>> > of the Dove lcd for RGB or YUV formats).
>> >
>> > Which board needs a special VIP configuration?
>>
>> If you run the NXP driver, and then run this driver, things get messed
>> up - which has already been covered months ago when this patch was first
>> brought up.
>>
>> It's there to ensure that the TDA998x is correctly configured no matter
>> what it's previous state is, and prevent the thing being fragile as hell.
>
> The NXP driver will never go to the mainline, so, I don't see the
> problem. If you want to use it to test some other drivers, you should
> better patch it instead of adding useless code in the TDA998x driver.
I don't think it really matters for the end user if NXP isn't
mainline. If they are jumping between vendor kernel and mainline, and
inheriting some state left over from the NXP driver in vendor kernel,
it makes debugging very confusing. It would be less of an issue if a
warm reset actually reset the tda998x part, but that is not the case,
it is better to rely less on the hw state when the driver is loaded,
IMHO.
BR,
-R
>> No, reset doesn't restore its settings, only a power cycle does.
>
> Sorry, all VIP control registers may be changed at any time and the
> change appears immediately (thank you for the /sys i2c_read/write).
>
> --
> Ken ar c'henta? | ** Breizh ha Linux atav! **
> Jef | http://moinejf.free.fr/
On Thu, Aug 22, 2013 at 07:33:43AM -0400, Rob Clark wrote:
> On Thu, Aug 22, 2013 at 2:53 AM, Jean-Francois Moine <[email protected]> wrote:
> > On Wed, 21 Aug 2013 23:36:05 +0100
> > Russell King - ARM Linux <[email protected]> wrote:
> >
> >> > AFAIK, the TI boards have no "pin-swapped", nor has the Cubox (there is
> >> > no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
> >> > of the Dove lcd for RGB or YUV formats).
> >> >
> >> > Which board needs a special VIP configuration?
> >>
> >> If you run the NXP driver, and then run this driver, things get messed
> >> up - which has already been covered months ago when this patch was first
> >> brought up.
> >>
> >> It's there to ensure that the TDA998x is correctly configured no matter
> >> what it's previous state is, and prevent the thing being fragile as hell.
> >
> > The NXP driver will never go to the mainline, so, I don't see the
> > problem. If you want to use it to test some other drivers, you should
> > better patch it instead of adding useless code in the TDA998x driver.
>
> I don't think it really matters for the end user if NXP isn't
> mainline. If they are jumping between vendor kernel and mainline, and
> inheriting some state left over from the NXP driver in vendor kernel,
> it makes debugging very confusing. It would be less of an issue if a
> warm reset actually reset the tda998x part, but that is not the case,
> it is better to rely less on the hw state when the driver is loaded,
> IMHO.
Absolutely right, thanks for backing up what I've said. I've done exactly
that - switching between the NXP driver and the mainline driver to debug
other problems, and not having the TDA998x setup correctly just makes the
job much harder and time consuming.
I keep both drivers available in my internal git tree so that I can switch
between them when necessary.
On Wed, Aug 14, 2013 at 09:43:30PM +0200, Sebastian Hesselbarth wrote:
> From: Russell King <[email protected]>
>
> This patch adds tda998x specific parameters to allow it to be configured
> for different boards using it. Also, this implements rudimentary audio
> support for S/PDIF attached controllers.
>
> Signed-off-by: Russell King <[email protected]>
> Signed-off-by: Sebastian Hesselbarth <[email protected]>
> Tested-by: Darren Etheridge <[email protected]>
> ---
It looks like there's been a bug introduced in this patch (which wasn't
in my original).
> @@ -445,8 +681,7 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
>
> switch (mode) {
> case DRM_MODE_DPMS_ON:
> - /* enable audio and video ports */
> - reg_write(encoder, REG_ENA_AP, 0xff);
> + /* enable video ports, audio will be enabled later */
> reg_write(encoder, REG_ENA_VP_0, 0xff);
> reg_write(encoder, REG_ENA_VP_1, 0xff);
> reg_write(encoder, REG_ENA_VP_2, 0xff);
I also disabled the writing to REG_ENA_AP in the DPMS off path as well,
which clears this register.
That seems to be missing from this patch, and it means that when the
display is placed into DPMS-off mode, the audio inputs are disabled,
never to be re-enabled. There is no need to disable the audio input
in DPMS-off mode.
8<=============
From: Russell King <[email protected]>
Subject: [PATCH] drm/i2c: Fix broken TDA998x audio
In patch "drm/i2c: tda998x: add video and audio input configuration" in
its original version, disabling the audio input port was removed. The
version which was submitted for merging had this change deleted, which
results in audio being non-functional. Fix this by removing the write.
While here, update the comment too.
Signed-off-by: Russell King <[email protected]>
---
drivers/gpu/drm/i2c/tda998x_drv.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 5742cfc..59878af 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -705,8 +705,7 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
reg_write(encoder, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
break;
case DRM_MODE_DPMS_OFF:
- /* disable audio and video ports */
- reg_write(encoder, REG_ENA_AP, 0x00);
+ /* disable video ports */
reg_write(encoder, REG_ENA_VP_0, 0x00);
reg_write(encoder, REG_ENA_VP_1, 0x00);
reg_write(encoder, REG_ENA_VP_2, 0x00);
Ping?
On Mon, Sep 02, 2013 at 03:50:57PM +0100, Russell King - ARM Linux wrote:
> On Wed, Aug 14, 2013 at 09:43:30PM +0200, Sebastian Hesselbarth wrote:
> > From: Russell King <[email protected]>
> >
> > This patch adds tda998x specific parameters to allow it to be configured
> > for different boards using it. Also, this implements rudimentary audio
> > support for S/PDIF attached controllers.
> >
> > Signed-off-by: Russell King <[email protected]>
> > Signed-off-by: Sebastian Hesselbarth <[email protected]>
> > Tested-by: Darren Etheridge <[email protected]>
> > ---
>
> It looks like there's been a bug introduced in this patch (which wasn't
> in my original).
>
> > @@ -445,8 +681,7 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
> >
> > switch (mode) {
> > case DRM_MODE_DPMS_ON:
> > - /* enable audio and video ports */
> > - reg_write(encoder, REG_ENA_AP, 0xff);
> > + /* enable video ports, audio will be enabled later */
> > reg_write(encoder, REG_ENA_VP_0, 0xff);
> > reg_write(encoder, REG_ENA_VP_1, 0xff);
> > reg_write(encoder, REG_ENA_VP_2, 0xff);
>
> I also disabled the writing to REG_ENA_AP in the DPMS off path as well,
> which clears this register.
>
> That seems to be missing from this patch, and it means that when the
> display is placed into DPMS-off mode, the audio inputs are disabled,
> never to be re-enabled. There is no need to disable the audio input
> in DPMS-off mode.
>
> 8<=============
> From: Russell King <[email protected]>
> Subject: [PATCH] drm/i2c: Fix broken TDA998x audio
>
> In patch "drm/i2c: tda998x: add video and audio input configuration" in
> its original version, disabling the audio input port was removed. The
> version which was submitted for merging had this change deleted, which
> results in audio being non-functional. Fix this by removing the write.
> While here, update the comment too.
>
> Signed-off-by: Russell King <[email protected]>
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 5742cfc..59878af 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -705,8 +705,7 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
> reg_write(encoder, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
> break;
> case DRM_MODE_DPMS_OFF:
> - /* disable audio and video ports */
> - reg_write(encoder, REG_ENA_AP, 0x00);
> + /* disable video ports */
> reg_write(encoder, REG_ENA_VP_0, 0x00);
> reg_write(encoder, REG_ENA_VP_1, 0x00);
> reg_write(encoder, REG_ENA_VP_2, 0x00);