cx23885[v2]: Fix IR interrupt storm.

PATCH nibble.max 3 participants 3 messages Jul 19, 2013 Media (V4L/DVB)

Share & tools

nibble.max

author

2013-07-19 03:37 UTC #0 lore raw

Hello Luis,
The internel interrupts are rounted as follow:
flatiron(include ADC)--->HammerHead(include IR inside)--->Pecos(PCIe)
The flatiron interrupt is enabled when chip power up.
When HammerHead interrupt is enalbe in Pecos, the most of interrupts are coming from flatiron.
The more accurate code is that reading back these left and right registers(0x1f, 0x23), set its bit-7 to "1" , then write back. So that it does not touch other bits.
BR,
Max

Quoted 70 lines

Hi all,
This path is meant to be up-streamed.
Andy has a nice explanation for the interrupt storm when enabling the IR interrupt:
The flatiron core (the audio adc) signals the end of its self-test with an interrupt. Since the flatiron irq seems OR-wired with the IR irq the result is this interrupt storm. This i2c tranfers will clear the flatiron interrupts - the left and right channels self-tests.
Also as suggested by Andy I moved the i2c transfers to the cx23885 av core interrupt handling worker. If any spurious interrupt happens we silence them.
The flatiron has some dedicated register read/write functions but are not exported so Antti just suggested to call the i2c_transfer directly.
Tested in the TBS6981 Dual DVB-S2 card.
PS: I've found this i2c_transfers in TBS media tree, more precisely in the cx23885-i2c.c file.
Regards,
Luis
Signed-off-by Luis Alves <[email protected]>
---
drivers/media/pci/cx23885/cx23885-av.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
drivers/media/pci/cx23885/cx23885-av.c +17 -0
index e958a01..d33570f 100644
@@ -29,8 +29,25 @@ void cx23885_av_work_handler(struct work_struct *work)
 	struct cx23885_dev *dev =
 			   container_of(work, struct cx23885_dev, cx25840_work);
 	bool handled;
+	char buffer[2];
+	struct i2c_msg msg = {
+		.addr = 0x98 >> 1,
+		.flags = 0,
+		.len = 2,
+		.buf = buffer,
+	};
 
 	v4l2_subdev_call(dev->sd_cx25840, core, interrupt_service_routine,
 			 PCI_MSK_AV_CORE, &handled);
+
+	if (!handled) {
+		/* clear any pending flatiron interrupts */
+		buffer[0] = 0x1f;
+		buffer[1] = 0x80;
+		i2c_transfer(&dev->i2c_bus[2].i2c_adap, &msg, 1);
+		buffer[0] = 0x23;
+		i2c_transfer(&dev->i2c_bus[2].i2c_adap, &msg, 1);
+	}
+
 	cx23885_irq_enable(dev, PCI_MSK_AV_CORE);
 }
Signature · 6 lines
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis Alves

2013-07-19 08:53 UTC #1 lore raw

Hi Max,

Should have thought on that! I'll change it to preserve all other bits.

Thanks,
Luis

Quoted 81 lines · nibble.max

Hello Luis,
The internel interrupts are rounted as follow:
flatiron(include ADC)--->HammerHead(include IR inside)--->Pecos(PCIe)
The flatiron interrupt is enabled when chip power up.
When HammerHead interrupt is enalbe in Pecos, the most of interrupts are coming from flatiron.
The more accurate code is that reading back these left and right registers(0x1f, 0x23), set its bit-7 to "1" , then write back. So that it does not touch other bits.
BR,
Max
Quoted 70 lines
Hi all,
This path is meant to be up-streamed.
Andy has a nice explanation for the interrupt storm when enabling the IR interrupt:
The flatiron core (the audio adc) signals the end of its self-test with an interrupt. Since the flatiron irq seems OR-wired with the IR irq the result is this interrupt storm. This i2c tranfers will clear the flatiron interrupts - the left and right channels self-tests.
Also as suggested by Andy I moved the i2c transfers to the cx23885 av core interrupt handling worker. If any spurious interrupt happens we silence them.
The flatiron has some dedicated register read/write functions but are not exported so Antti just suggested to call the i2c_transfer directly.
Tested in the TBS6981 Dual DVB-S2 card.
PS: I've found this i2c_transfers in TBS media tree, more precisely in the cx23885-i2c.c file.
Regards,
Luis
Signed-off-by Luis Alves <[email protected]>
---
drivers/media/pci/cx23885/cx23885-av.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
drivers/media/pci/cx23885/cx23885-av.c +17 -1
index e958a01..d33570f 100644
@@ -29,8 +29,25 @@ void cx23885_av_work_handler(struct work_struct *work)
      struct cx23885_dev *dev =
                         container_of(work, struct cx23885_dev, cx25840_work);
      bool handled;
+      char buffer[2];
+      struct i2c_msg msg = {
+              .addr = 0x98 >> 1,
+              .flags = 0,
+              .len = 2,
+              .buf = buffer,
+      };
 
      v4l2_subdev_call(dev->sd_cx25840, core, interrupt_service_routine,
                       PCI_MSK_AV_CORE, &handled);
+
+      if (!handled) {
+              /* clear any pending flatiron interrupts */
+              buffer[0] = 0x1f;
+              buffer[1] = 0x80;
+              i2c_transfer(&dev->i2c_bus[2].i2c_adap, &msg, 1);
+              buffer[0] = 0x23;
+              i2c_transfer(&dev->i2c_bus[2].i2c_adap, &msg, 1);
+      }
+
      cx23885_irq_enable(dev, PCI_MSK_AV_CORE);
 }
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Andy Walls

2013-07-19 09:08 UTC #2 lore raw

Quoted 93 lines · Luis Alves

Hi Max,
Should have thought on that! I'll change it to preserve all other bits.
Thanks,
Luis
Quoted 5 lines · nibble.max
Hello Luis,
The internel interrupts are rounted as follow:
flatiron(include ADC)--->HammerHead(include IR inside)--->Pecos(PCIe) The flatiron interrupt is enabled when chip power up.
When HammerHead interrupt is enalbe in Pecos, the most of interrupts
are coming from flatiron.
Quoted 1 line
The more accurate code is that reading back these left and right
registers(0x1f, 0x23), set its bit-7 to "1" , then write back.
Quoted 22 lines
So that it does not touch other bits.
BR,
Max
Quoted 18 lines
Hi all,
This path is meant to be up-streamed.
Andy has a nice explanation for the interrupt storm when enabling the IR interrupt:
The flatiron core (the audio adc) signals the end of its self-test with an interrupt. Since the flatiron irq seems OR-wired with the IR irq the result is this interrupt storm. This i2c tranfers will clear the flatiron interrupts - the left and right channels self-tests.
Also as suggested by Andy I moved the i2c transfers to the cx23885 av core interrupt handling worker. If any spurious interrupt happens we silence them.
The flatiron has some dedicated register read/write functions but are not exported so Antti just suggested to call the i2c_transfer
directly.
Quoted 10 lines
Quoted 10 lines
Tested in the TBS6981 Dual DVB-S2 card.
PS: I've found this i2c_transfers in TBS media tree, more precisely in the cx23885-i2c.c file.
Regards,
Luis
Quoted 6 lines
Review
Quoted 6 lines
Signed-off-by Luis Alves <[email protected]>
---
drivers/media/pci/cx23885/cx23885-av.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
(quoted diff) +0 -0
b/drivers/media/pci/cx23885/cx23885-av.c
Quoted 4 lines
drivers/media/pci/cx23885/cx23885-av.c void cx23885_av_work_handler(struct work_struct
Quoted 4 lines
index e958a01..d33570f 100644
drivers/media/pci/cx23885/cx23885-av.c +0 -0
@@ -29,8 +29,25 @@ void cx23885_av_work_handler(struct work_struct
*work)
Quoted 2 lines
drivers/media/pci/cx23885/cx23885-av.c void cx23885_av_work_handler(struct work_struct · line 30
Quoted 2 lines
drivers/media/pci/cx23885/cx23885-av.c +0 -0
      struct cx23885_dev *dev =
                         container_of(work, struct cx23885_dev,
cx25840_work);
Quoted 10 lines
drivers/media/pci/cx23885/cx23885-av.c void cx23885_av_work_handler(struct work_struct · line 40
Quoted 10 lines
drivers/media/pci/cx23885/cx23885-av.c +7 -0
      bool handled;
+      char buffer[2];
+      struct i2c_msg msg = {
+              .addr = 0x98 >> 1,
+              .flags = 0,
+              .len = 2,
+              .buf = buffer,
+      };
 
      v4l2_subdev_call(dev->sd_cx25840, core,
interrupt_service_routine,
Quoted 16 lines
Quoted 16 lines
drivers/media/pci/cx23885/cx23885-av.c +10 -1
                       PCI_MSK_AV_CORE, &handled);
+
+      if (!handled) {
+              /* clear any pending flatiron interrupts */
+              buffer[0] = 0x1f;
+              buffer[1] = 0x80;
+              i2c_transfer(&dev->i2c_bus[2].i2c_adap, &msg, 1);
+              buffer[0] = 0x23;
+              i2c_transfer(&dev->i2c_bus[2].i2c_adap, &msg, 1);
+      }
+
      cx23885_irq_enable(dev, PCI_MSK_AV_CORE);
 }
--
1.7.9.5

Quoted 6 lines

Quoted 2 lines
Quoted 2 lines
--
To unsubscribe from this list: send the line "unsubscribe
linux-media" in
Quoted 3 lines
Quoted 2 lines
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Yup, I was going to suggest that.

At that point you'll want to put prototypes for cx23885_flatiron_read/write() in a header file and remove any static keyword in front of the function definitions in cx23885-video.c.

Using those functions will make the change easier to read.

Regards,
Andy