[PATCH 0/5] cx23885: Ryzen/Xeon DMA/interrupt fixes

[PATCH 0/5] cx23885: Ryzen/Xeon DMA/interrupt fixes

[Brad Love]

Various problems have been reported to Hauppauge related
to usage of QuadHD products on Ryzen platforms and some
Xeon platforms. The most serious issue causes adapters on
the card to stop working. When the four tuners are in use it
is possible to experience RiSc engine OP CODE errors, at which
point the DMA engine for that port stalls and becomes inoperable
until a reboot. Unloading and reloading the driver does not
fix the condition. This DMA stall is handled by checking
TC_REQ and TC_REQ_SET registers in a lot of various spots.
If both of these registers are found to have the same bits
set, then their contents must be written back to 'un-freeze'
the DMA engine and let operation continue. The problem can
be quite hard to reproduce depending on motherboard and cpu.
The attached patch has been reported to fix all various parties
reporting issues with an assortment of Ryzen motherboards and
CPU combinations. It has been reported that some manufacturers
have released BIOS updates to address the PCIe instability.
This patch has been confirmed to have no adverse affets on
the reporting systems.


In the investigation of the issue a couple other problems
were found:
- On Ryzen/Xeon platforms it is possible to skip interrupts
- PCI and TS masks were ignored during irq


Brad Love (5):
  cx23885: Handle additional bufs on interrupt
  cx23885: Use PCI and TS masks in irq functions
  cx23885: Ryzen DMA related RiSC engine stall fixes
  cx23885: Expand registers printed during dma tsport reg dump
  cx23885: Add some missing register documentation

 drivers/media/pci/cx23885/cx23885-core.c | 144 +++++++++++++++++++++++++++----
 drivers/media/pci/cx23885/cx23885-reg.h  |  14 +++
 2 files changed, 139 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH 1/5] cx23885: Handle additional bufs on interrupt

[Brad Love]

On Ryzen systems interrupts are occasionally missed:

cx23885: cx23885_wakeup: [ffff99b384b83c00/28] wakeup reg=5406 buf=5405
cx23885: cx23885_wakeup: [ffff99b40bf79400/31] wakeup reg=9537 buf=9536

This patch loops up to five times on wakeup, marking any buffers
found done.

Since the count register is u16, but the vb2 counter is u32, some modulo
arithmetic is used to accommodate wraparound and ensure current active
buffer is the buffer expected.

Signed-off-by: Brad Love <brad@nextdimension.cc>
---
 drivers/media/pci/cx23885/cx23885-core.c | 37 +++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/media/pci/cx23885/cx23885-core.c b/drivers/media/pci/cx23885/cx23885-core.c
index 019fac4..b279758 100644
--- a/drivers/media/pci/cx23885/cx23885-core.c
+++ b/drivers/media/pci/cx23885/cx23885-core.c
@@ -422,19 +422,30 @@ static void cx23885_wakeup(struct cx23885_tsport *port,
 			   struct cx23885_dmaqueue *q, u32 count)
 {
 	struct cx23885_buffer *buf;
-
-	if (list_empty(&q->active))
-		return;
-	buf = list_entry(q->active.next,
-			 struct cx23885_buffer, queue);
-
-	buf->vb.vb2_buf.timestamp = ktime_get_ns();
-	buf->vb.sequence = q->count++;
-	dprintk(1, "[%p/%d] wakeup reg=%d buf=%d\n", buf,
-		buf->vb.vb2_buf.index,
-		count, q->count);
-	list_del(&buf->queue);
-	vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
+	int count_delta;
+	int max_buf_done = 5; /* service maximum five buffers */
+
+	do {
+		if (list_empty(&q->active))
+			return;
+		buf = list_entry(q->active.next,
+				 struct cx23885_buffer, queue);
+
+		buf->vb.vb2_buf.timestamp = ktime_get_ns();
+		buf->vb.sequence = q->count++;
+		if (count != (q->count % 65536)) {
+			dprintk(1, "[%p/%d] wakeup reg=%d buf=%d\n", buf,
+				buf->vb.vb2_buf.index, count, q->count);
+		} else {
+			dprintk(7, "[%p/%d] wakeup reg=%d buf=%d\n", buf,
+				buf->vb.vb2_buf.index, count, q->count);
+		}
+		list_del(&buf->queue);
+		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
+		max_buf_done--;
+		/* count register is 16 bits so apply modulo appropriately */
+		count_delta = ((int)count - (int)(q->count % 65536));
+	} while ((count_delta > 0) && (max_buf_done > 0));
 }
 
 int cx23885_sram_channel_setup(struct cx23885_dev *dev,
-- 
2.7.4

[PATCH 2/5] cx23885: Use PCI and TS masks in irq functions

[Brad Love]

Currently mask is read for pci_status/ts1_status/ts2_status, but
otherwise ignored. The masks are now used to determine whether
action is warranted.

Signed-off-by: Brad Love <brad@nextdimension.cc>
---
 drivers/media/pci/cx23885/cx23885-core.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/cx23885/cx23885-core.c b/drivers/media/pci/cx23885/cx23885-core.c
index b279758..3f55319 100644
--- a/drivers/media/pci/cx23885/cx23885-core.c
+++ b/drivers/media/pci/cx23885/cx23885-core.c
@@ -1704,6 +1704,12 @@ static irqreturn_t cx23885_irq(int irq, void *dev_id)
 
 	pci_status = cx_read(PCI_INT_STAT);
 	pci_mask = cx23885_irq_get_mask(dev);
+	if ((pci_status & pci_mask) == 0) {
+		dprintk(7, "pci_status: 0x%08x  pci_mask: 0x%08x\n",
+			pci_status, pci_mask);
+		goto out;
+	}
+
 	vida_status = cx_read(VID_A_INT_STAT);
 	vida_mask = cx_read(VID_A_INT_MSK);
 	audint_status = cx_read(AUDIO_INT_INT_STAT);
@@ -1713,7 +1719,9 @@ static irqreturn_t cx23885_irq(int irq, void *dev_id)
 	ts2_status = cx_read(VID_C_INT_STAT);
 	ts2_mask = cx_read(VID_C_INT_MSK);
 
-	if ((pci_status == 0) && (ts2_status == 0) && (ts1_status == 0))
+	if (((pci_status & pci_mask) == 0) &&
+		((ts2_status & ts2_mask) == 0) &&
+		((ts1_status & ts1_mask) == 0))
 		goto out;
 
 	vida_count = cx_read(VID_A_GPCNT);
@@ -1840,7 +1848,7 @@ static irqreturn_t cx23885_irq(int irq, void *dev_id)
 	}
 
 	if (handled)
-		cx_write(PCI_INT_STAT, pci_status);
+		cx_write(PCI_INT_STAT, pci_status & pci_mask);
 out:
 	return IRQ_RETVAL(handled);
 }
-- 
2.7.4

[PATCH 3/5] cx23885: Ryzen DMA related RiSC engine stall fixes

[Brad Love]

This bug affects all of Hauppauge QuadHD boards when used on all Ryzen
platforms and some XEON platforms. On these platforms it is possible to
error out the RiSC engine and cause it to stall, whereafter the only
way to reset the board to a working state is to reboot.

This is the fatal condition with current driver:

[  255.663598] cx23885: cx23885[0]: mpeg risc op code error
[  255.663607] cx23885: cx23885[0]: TS1 B - dma channel status dump
[  255.663612] cx23885: cx23885[0]:   cmds: init risc lo   : 0xffe54000
[  255.663615] cx23885: cx23885[0]:   cmds: init risc hi   : 0x00000000
[  255.663619] cx23885: cx23885[0]:   cmds: cdt base       : 0x00010870
[  255.663622] cx23885: cx23885[0]:   cmds: cdt size       : 0x0000000a
[  255.663625] cx23885: cx23885[0]:   cmds: iq base        : 0x00010630
[  255.663629] cx23885: cx23885[0]:   cmds: iq size        : 0x00000010
[  255.663632] cx23885: cx23885[0]:   cmds: risc pc lo     : 0xffe54018
[  255.663636] cx23885: cx23885[0]:   cmds: risc pc hi     : 0x00000000
[  255.663639] cx23885: cx23885[0]:   cmds: iq wr ptr      : 0x00004192
[  255.663642] cx23885: cx23885[0]:   cmds: iq rd ptr      : 0x0000418c
[  255.663645] cx23885: cx23885[0]:   cmds: cdt current    : 0x00010898
[  255.663649] cx23885: cx23885[0]:   cmds: pci target lo  : 0xf85ca340
[  255.663652] cx23885: cx23885[0]:   cmds: pci target hi  : 0x00000000
[  255.663655] cx23885: cx23885[0]:   cmds: line / byte    : 0x000c0000
[  255.663659] cx23885: cx23885[0]:   risc0:
[  255.663661] 0x1c0002f0 [ write sol eol count=752 ]
[  255.663666] cx23885: cx23885[0]:   risc1:
[  255.663667] 0xf85ca050 [ INVALID sol 22 20 19 18 resync 13 count=80 ]
[  255.663674] cx23885: cx23885[0]:   risc2:
[  255.663674] 0x00000000 [ INVALID count=0 ]
[  255.663678] cx23885: cx23885[0]:   risc3:
[  255.663679] 0x1c0002f0 [ write sol eol count=752 ]
[  255.663684] cx23885: cx23885[0]:   (0x00010630) iq 0:
[  255.663685] 0x1c0002f0 [ write sol eol count=752 ]
[  255.663690] cx23885: cx23885[0]:   iq 1: 0xf85ca630 [ arg #1 ]
[  255.663693] cx23885: cx23885[0]:   iq 2: 0x00000000 [ arg #2 ]
[  255.663696] cx23885: cx23885[0]:   (0x0001063c) iq 3:
[  255.663697] 0x1c0002f0 [ write sol eol count=752 ]
[  255.663702] cx23885: cx23885[0]:   iq 4: 0xf85ca920 [ arg #1 ]
[  255.663705] cx23885: cx23885[0]:   iq 5: 0x00000000 [ arg #2 ]
[  255.663709] cx23885: cx23885[0]:   (0x00010648) iq 6:
[  255.663709] 0xf85ca340 [ INVALID sol 22 20 19 18 resync 13 count=832 ]
[  255.663716] cx23885: cx23885[0]:   (0x0001064c) iq 7:
[  255.663717] 0x00000000 [ INVALID count=0 ]
[  255.663721] cx23885: cx23885[0]:   (0x00010650) iq 8:
[  255.663721] 0x00000000 [ INVALID count=0 ]
[  255.663725] cx23885: cx23885[0]:   (0x00010654) iq 9:
[  255.663726] 0x1c0002f0 [ write sol eol count=752 ]
[  255.663731] cx23885: cx23885[0]:   iq a: 0xf85c9780 [ arg #1 ]
[  255.663734] cx23885: cx23885[0]:   iq b: 0x00000000 [ arg #2 ]
[  255.663737] cx23885: cx23885[0]:   (0x00010660) iq c:
[  255.663738] 0x1c0002f0 [ write sol eol count=752 ]
[  255.663743] cx23885: cx23885[0]:   iq d: 0xf85c9a70 [ arg #1 ]
[  255.663746] cx23885: cx23885[0]:   iq e: 0x00000000 [ arg #2 ]
[  255.663749] cx23885: cx23885[0]:   (0x0001066c) iq f:
[  255.663750] 0x1c0002f0 [ write sol eol count=752 ]
[  255.663755] cx23885: cx23885[0]:   iq 10: 0xf4fa2920 [ arg #1 ]
[  255.663758] cx23885: cx23885[0]:   iq 11: 0x00000000 [ arg #2 ]
[  255.663759] cx23885: cx23885[0]: fifo: 0x00005000 -> 0x6000
[  255.663760] cx23885: cx23885[0]: ctrl: 0x00010630 -> 0x10690
[  255.663764] cx23885: cx23885[0]:   ptr1_reg: 0x00005980
[  255.663767] cx23885: cx23885[0]:   ptr2_reg: 0x000108a8
[  255.663770] cx23885: cx23885[0]:   cnt1_reg: 0x0000000b
[  255.663773] cx23885: cx23885[0]:   cnt2_reg: 0x00000003

Included is checks of the TC_REQ and TC_REQ_SET registers during states
of board initialization, reset, DMA start, and DMA stop. If both registers
are set, this indicates a stall in the RiSC engine, at which point the
bridge error is cleared.

A small delay is introduced in stop_dma as well, to allow transfers in
progress to finish.

After application all models work on Ryzen, occasionally yielding:

cx23885_clear_bridge_error: dma in progress detected 0x00000001 0x00000001, clearing

Signed-off-by: Brad Love <brad@nextdimension.cc>
---
 drivers/media/pci/cx23885/cx23885-core.c | 63 +++++++++++++++++++++++++++++++-
 drivers/media/pci/cx23885/cx23885-reg.h  | 14 +++++++
 2 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/cx23885/cx23885-core.c b/drivers/media/pci/cx23885/cx23885-core.c
index 3f55319..20a1fd2 100644
--- a/drivers/media/pci/cx23885/cx23885-core.c
+++ b/drivers/media/pci/cx23885/cx23885-core.c
@@ -601,6 +601,25 @@ static void cx23885_risc_disasm(struct cx23885_tsport *port,
 	}
 }
 
+static void cx23885_clear_bridge_error(struct cx23885_dev *dev)
+{
+	uint32_t reg1_val = cx_read(TC_REQ); /* read-only */
+	uint32_t reg2_val = cx_read(TC_REQ_SET);
+
+	if (reg1_val && reg2_val) {
+		cx_write(TC_REQ, reg1_val);
+		cx_write(TC_REQ_SET, reg2_val);
+		cx_read(VID_B_DMA);
+		cx_read(VBI_B_DMA);
+		cx_read(VID_C_DMA);
+		cx_read(VBI_C_DMA);
+
+		dev_info(&dev->pci->dev,
+			"dma in progress detected 0x%08x 0x%08x, clearing\n",
+			reg1_val, reg2_val);
+	}
+}
+
 static void cx23885_shutdown(struct cx23885_dev *dev)
 {
 	/* disable RISC controller */
@@ -633,6 +652,7 @@ static void cx23885_shutdown(struct cx23885_dev *dev)
 
 static void cx23885_reset(struct cx23885_dev *dev)
 {
+	uint32_t reg;
 	dprintk(1, "%s()\n", __func__);
 
 	cx23885_shutdown(dev);
@@ -646,6 +666,8 @@ static void cx23885_reset(struct cx23885_dev *dev)
 	cx_write(CLK_DELAY, cx_read(CLK_DELAY) & 0x80000000);
 	cx_write(PAD_CTRL, 0x00500300);
 
+	/* clear dma in progress */
+	cx23885_clear_bridge_error(dev);
 	mdelay(100);
 
 	cx23885_sram_channel_setup(dev, &dev->sram_channels[SRAM_CH01],
@@ -662,6 +684,11 @@ static void cx23885_reset(struct cx23885_dev *dev)
 	cx23885_sram_channel_setup(dev, &dev->sram_channels[SRAM_CH09], 128, 0);
 
 	cx23885_gpio_setup(dev);
+
+	cx23885_irq_get_mask(dev);
+
+	/* clear dma in progress */
+	cx23885_clear_bridge_error(dev);
 }
 
 
@@ -676,6 +703,8 @@ static int cx23885_pci_quirks(struct cx23885_dev *dev)
 	if (dev->bridge == CX23885_BRIDGE_885)
 		cx_clear(RDR_TLCTL0, 1 << 4);
 
+	/* clear dma in progress */
+	cx23885_clear_bridge_error(dev);
 	return 0;
 }
 
@@ -1352,6 +1381,9 @@ int cx23885_start_dma(struct cx23885_tsport *port,
 	dprintk(1, "%s() w: %d, h: %d, f: %d\n", __func__,
 		dev->width, dev->height, dev->field);
 
+	/* clear dma in progress */
+	cx23885_clear_bridge_error(dev);
+
 	/* Stop the fifo and risc engine for this port */
 	cx_clear(port->reg_dma_ctl, port->dma_ctl_val);
 
@@ -1432,16 +1464,26 @@ int cx23885_start_dma(struct cx23885_tsport *port,
 	case CX23885_BRIDGE_888:
 		/* enable irqs */
 		dprintk(1, "%s() enabling TS int's and DMA\n", __func__);
+		/* clear dma in progress */
+		cx23885_clear_bridge_error(dev);
 		cx_set(port->reg_ts_int_msk,  port->ts_int_msk_val);
 		cx_set(port->reg_dma_ctl, port->dma_ctl_val);
+
+		/* clear dma in progress */
+		cx23885_clear_bridge_error(dev);
 		cx23885_irq_add(dev, port->pci_irqmask);
 		cx23885_irq_enable_all(dev);
+
+		/* clear dma in progress */
+		cx23885_clear_bridge_error(dev);
 		break;
 	default:
 		BUG();
 	}
 
 	cx_set(DEV_CNTRL2, (1<<5)); /* Enable RISC controller */
+	/* clear dma in progress */
+	cx23885_clear_bridge_error(dev);
 
 	if (cx23885_boards[dev->board].portb == CX23885_MPEG_ENCODER)
 		cx23885_av_clk(dev, 1);
@@ -1449,6 +1491,11 @@ int cx23885_start_dma(struct cx23885_tsport *port,
 	if (debug > 4)
 		cx23885_tsport_reg_dump(port);
 
+	cx23885_irq_get_mask(dev);
+
+	/* clear dma in progress */
+	cx23885_clear_bridge_error(dev);
+
 	return 0;
 }
 
@@ -1456,15 +1503,28 @@ static int cx23885_stop_dma(struct cx23885_tsport *port)
 {
 	struct cx23885_dev *dev = port->dev;
 	u32 reg;
+	int delay = 0;
+	uint32_t reg1_val;
+	uint32_t reg2_val;
 
 	dprintk(1, "%s()\n", __func__);
 
 	/* Stop interrupts and DMA */
 	cx_clear(port->reg_ts_int_msk, port->ts_int_msk_val);
 	cx_clear(port->reg_dma_ctl, port->dma_ctl_val);
+	/* just in case wait for any dma to complete before allowing dealloc */
+	mdelay(20);
+	for (delay = 0; delay < 100; delay++) {
+		reg1_val = cx_read(TC_REQ);
+		reg2_val = cx_read(TC_REQ_SET);
+		if (reg1_val == 0 || reg2_val == 0)
+			break;
+		mdelay(1);
+	}
+	dev_dbg(&dev->pci->dev, "delay=%d reg1=0x%08x reg2=0x%08x\n",
+		delay, reg1_val, reg2_val);
 
 	if (cx23885_boards[dev->board].portb == CX23885_MPEG_ENCODER) {
-
 		reg = cx_read(PAD_CTRL);
 
 		/* Set TS1_OE */
@@ -1475,7 +1535,6 @@ static int cx23885_stop_dma(struct cx23885_tsport *port)
 		cx_write(PAD_CTRL, reg);
 		cx_write(port->reg_src_sel, 0);
 		cx_write(port->reg_gen_ctrl, 8);
-
 	}
 
 	if (cx23885_boards[dev->board].portb == CX23885_MPEG_ENCODER)
diff --git a/drivers/media/pci/cx23885/cx23885-reg.h b/drivers/media/pci/cx23885/cx23885-reg.h
index 2d3cbaf..08cec8d 100644
--- a/drivers/media/pci/cx23885/cx23885-reg.h
+++ b/drivers/media/pci/cx23885/cx23885-reg.h
@@ -288,6 +288,18 @@ Channel manager Data Structure entry = 20 DWORD
 #define AUDIO_EXT_INT_MSTAT	0x00040068
 #define AUDIO_EXT_INT_SSTAT	0x0004006C
 
+/* Bits [7:0] set in both TC_REQ and TC_REQ_SET
+ * indicate a stall in the RISC engine for a
+ * particular rider traffic class. This causes
+ * the 885 and 888 bridges (unknown about 887)
+ * to become inoperable. Setting bits in
+ * TC_REQ_SET resets the corresponding bits
+ * in TC_REQ (and TC_REQ_SET) allowing
+ * operation to continue.
+ */
+#define TC_REQ		0x00040090
+#define TC_REQ_SET	0x00040094
+
 #define RDR_CFG0	0x00050000
 #define RDR_CFG1	0x00050004
 #define RDR_CFG2	0x00050008
@@ -386,6 +398,8 @@ Channel manager Data Structure entry = 20 DWORD
 #define VID_B_PIXEL_FRMT	0x00130184
 
 /* Video C Interface */
+#define VID_C_DMA		0x00130200
+#define VBI_C_DMA		0x00130208
 #define VID_C_GPCNT		0x00130220
 #define VID_C_GPCNT_CTL		0x00130230
 #define VBI_C_GPCNT_CTL		0x00130234
-- 
2.7.4

[PATCH 4/5] cx23885: Expand registers in dma tsport reg dump

[Brad Love]

Include some additional useful registers in the output.

Signed-off-by: Brad Love <brad@nextdimension.cc>
---
 drivers/media/pci/cx23885/cx23885-core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/pci/cx23885/cx23885-core.c b/drivers/media/pci/cx23885/cx23885-core.c
index 20a1fd2..1150160 100644
--- a/drivers/media/pci/cx23885/cx23885-core.c
+++ b/drivers/media/pci/cx23885/cx23885-core.c
@@ -1369,6 +1369,18 @@ static void cx23885_tsport_reg_dump(struct cx23885_tsport *port)
 		port->reg_ts_clk_en, cx_read(port->reg_ts_clk_en));
 	dprintk(1, "%s() ts_int_msk(0x%08X)     0x%08x\n", __func__,
 		port->reg_ts_int_msk, cx_read(port->reg_ts_int_msk));
+	dprintk(1, "%s() ts_int_status(0x%08X)  0x%08x\n", __func__,
+		port->reg_ts_int_stat, cx_read(port->reg_ts_int_stat));
+	dprintk(1, "%s() PCI_INT_STAT           0x%08X\n", __func__,
+		cx_read(PCI_INT_STAT));
+	dprintk(1, "%s() VID_B_INT_MSTAT        0x%08X\n", __func__,
+		cx_read(VID_B_INT_MSTAT));
+	dprintk(1, "%s() VID_B_INT_SSTAT        0x%08X\n", __func__,
+		cx_read(VID_B_INT_SSTAT));
+	dprintk(1, "%s() VID_C_INT_MSTAT        0x%08X\n", __func__,
+		cx_read(VID_C_INT_MSTAT));
+	dprintk(1, "%s() VID_C_INT_SSTAT        0x%08X\n", __func__,
+		cx_read(VID_C_INT_SSTAT));
 }
 
 int cx23885_start_dma(struct cx23885_tsport *port,
-- 
2.7.4

[PATCH 5/5] cx23885: Add some missing register documentation

[Brad Love]

Document what these two register calls are doing.

Signed-off-by: Brad Love <brad@nextdimension.cc>
---
 drivers/media/pci/cx23885/cx23885-core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/pci/cx23885/cx23885-core.c b/drivers/media/pci/cx23885/cx23885-core.c
index 1150160..ca19e0d 100644
--- a/drivers/media/pci/cx23885/cx23885-core.c
+++ b/drivers/media/pci/cx23885/cx23885-core.c
@@ -1465,8 +1465,15 @@ int cx23885_start_dma(struct cx23885_tsport *port,
 		reg = reg | 0xa;
 		cx_write(PAD_CTRL, reg);
 
-		/* FIXME and these two registers should be documented. */
+		/* Sets MOE_CLK_DIS to disable MoE clock */
+		/* sets MCLK_DLY_SEL/BCLK_DLY_SEL to 1 buffer delay each */
 		cx_write(CLK_DELAY, cx_read(CLK_DELAY) | 0x80000011);
+
+		/* ALT_GPIO_ALT_SET: GPIO[0]
+		 * IR_ALT_TX_SEL: GPIO[1]
+		 * GPIO1_ALT_SEL: VIP_656_DATA[0]
+		 * GPIO0_ALT_SEL: VIP_656_CLK
+		 */
 		cx_write(ALT_PIN_OUT_SEL, 0x10100045);
 	}
 
-- 
2.7.4