stk1160: Give the chip some time to retrieve data from AC97 codec.

PATCH In review Marcel Hasler 2 participants 5 messages Oct 28, 2016 – Nov 26, 2016 1 message not in the archive

Subsystems (2)

Media (V4L/DVB) STK1160 USB video capture driver

lore

Share & tools

lore

Marcel Hasler

author

2016-10-28 08:52 UTC #0 lore raw

PATCH 1/1 stk1160: Give the chip some time to retrieve data from AC97 codec.

The STK1160 needs some time to transfer data from the AC97 registers into its own. On some systems reading the chip's own registers to soon will return wrong values. The "proper" way to handle this would be to poll STK1160_AC97CTL_0 after every read or write command until the command bit has been cleared, but this may not be worth the hassle.

Signed-off-by Marcel Hasler <[email protected]>

---
 drivers/media/usb/stk1160/stk1160-ac97.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/media/usb/stk1160/stk1160-ac97.c +4 -0

index 31bdd60d..caa65a8 100644
@@ -20,6 +20,7 @@
  *
  */
 
+#include <linux/delay.h>
 #include <linux/module.h>
 
 #include "stk1160.h"

@@ -61,6 +62,9 @@ static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg)
 	 */
 	stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b);
 
+	/* Give the chip some time to transfer data */
+	usleep_range(20, 40);
+
 	/* Retrieve register value */
 	stk1160_read_reg(dev, STK1160_AC97_CMD, &vall);
 	stk1160_read_reg(dev, STK1160_AC97_CMD + 1, &valh);

Signature · 1 line

2.10.1

A message that is not in the archive (sent off-list or not yet received): CAOJOY2MwyX++KbGLBXpf5nKihmrP+Qx5JgYJ==q-41t-znVwKQ@mail.gmail.com

Marcel Hasler

author

2016-10-28 10:40 UTC #2 lore raw

PATCH 1/1 stk1160: Give the chip some time to retrieve data from AC97 codec.

This patch might need some explaining. I actually noticed this problem early on while trying to fix the sound problem, but it was only this morning that I realized the (trivial) cause of it.

I first noticed something strange going on when I read the AC97 registers from /proc/asound/cardX/codec97#0/ac97#0-0+regs using the current version of the driver. Every time I read that file I would get slightly different values, not only for one register but for several of them. Also, every time I plugged in the device and opened alsamixer I would be presented with a different set of mixer controls. So obviously something was going wrong while talking to the AC97 chip.

When analyzing the USB trace I took from Windows (on VirtualBox) I found long delays (2 ms) between control packets and wondered whether those might be set by the driver on purpose. So I tried adding delays in stk1160_[read|write]_reg, and sure enough, the problem disappeared.

In retrospective I suspect those long delays to really be the result of virtualization overhead. I actually tried getting a native trace using USBpcap, but unfortunately its timer resolution is so low that it's impossible to get any useful data.

Once I realized what the actual problem was I removed the delays in stk1160_[read|write]_reg and instead experimented with different delays in stk1160_read_ac97 and found 20 us to be perfectly sufficient to get reliable reads.

Now the strange thing about this problem is that it occurs on both of my notebooks, but not on my desktop computer. I can only speculate about the reason for this. My theory is that is has something to do with the way different USB host controllers handle/buffer outgoing control packets. Both of my notebooks are recent models by Acer (a normal notebook and a cloudbook) and most likely use the same host controller. My desktop motherboard on the other hand is a bit older.

So I wonder, have you experienced this problem on your own systems?

Best regards
Marcel

2016-10-28 10:52 GMT+02:00 Marcel Hasler <[email protected]>:

Quoted 35 lines

The STK1160 needs some time to transfer data from the AC97 registers into its own. On some systems reading the chip's own registers to soon will return wrong values. The "proper" way to handle this would be to poll STK1160_AC97CTL_0 after every read or write command until the command bit has been cleared, but this may not be worth the hassle.
Signed-off-by Marcel Hasler <[email protected]>
---
 drivers/media/usb/stk1160/stk1160-ac97.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/media/usb/stk1160/stk1160-ac97.c +4 -1
index 31bdd60d..caa65a8 100644
@@ -20,6 +20,7 @@
  *
  */
 
+#include <linux/delay.h>
 #include <linux/module.h>
 
 #include "stk1160.h"
@@ -61,6 +62,9 @@ static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg)
         */
        stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b);
 
+       /* Give the chip some time to transfer data */
+       usleep_range(20, 40);
+
        /* Retrieve register value */
        stk1160_read_reg(dev, STK1160_AC97_CMD, &vall);
        stk1160_read_reg(dev, STK1160_AC97_CMD + 1, &valh);
--
2.10.1

Ezequiel Garcia

maintainer

2016-11-20 17:37 UTC #3 lore raw

In reply to Marcel Hasler

Quoted 23 lines · Marcel Hasler

The STK1160 needs some time to transfer data from the AC97 registers into its own. On some systems reading the chip's own registers to soon will return wrong values. The "proper" way to handle this would be to poll STK1160_AC97CTL_0 after every read or write command until the command bit has been cleared, but this may not be worth the hassle.
Signed-off-by Marcel Hasler <[email protected]>
---
 drivers/media/usb/stk1160/stk1160-ac97.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/media/usb/stk1160/stk1160-ac97.c +1 -0
index 31bdd60d..caa65a8 100644
@@ -20,6 +20,7 @@
  *
  */
 
+#include <linux/delay.h>
 #include <linux/module.h>
 
 #include "stk1160.h"

drivers/media/usb/stk1160/stk1160-ac97.c static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg) · line 67

Quoted 6 lines

drivers/media/usb/stk1160/stk1160-ac97.c +3 -0

@@ -61,6 +62,9 @@ static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg)
         */
        stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b);
 
+       /* Give the chip some time to transfer data */
+       usleep_range(20, 40);
+

I don't recall any issues with this. In any case, we only read the registers for debugging purposes, so it's not a big deal.

Maybe it would be better to expand the comment a little bit, using your commit log:

""
The "proper" way to
handle this would be to poll STK1160_AC97CTL_0 after every read or write command until the command bit has been cleared, but this may not be worth the hassle. ""

This way, if the sleep proves problematic in the future, the "proper way" is already documented.

Quoted 6 lines

drivers/media/usb/stk1160/stk1160-ac97.c +0 -1

        /* Retrieve register value */
        stk1160_read_reg(dev, STK1160_AC97_CMD, &vall);
        stk1160_read_reg(dev, STK1160_AC97_CMD + 1, &valh);
--

2.10.1

Signature · 2 lines

Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Marcel Hasler

author

2016-11-26 14:00 UTC #4 lore raw

2016-11-20 18:37 GMT+01:00 Ezequiel Garcia <[email protected]>:

Quoted 44 lines · Marcel Hasler

Quoted 23 lines
The STK1160 needs some time to transfer data from the AC97 registers into its own. On some systems reading the chip's own registers to soon will return wrong values. The "proper" way to handle this would be to poll STK1160_AC97CTL_0 after every read or write command until the command bit has been cleared, but this may not be worth the hassle.
Signed-off-by Marcel Hasler <[email protected]>
---
 drivers/media/usb/stk1160/stk1160-ac97.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/media/usb/stk1160/stk1160-ac97.c +1 -0
index 31bdd60d..caa65a8 100644
@@ -20,6 +20,7 @@
  *
  */
 
+#include <linux/delay.h>
 #include <linux/module.h>
 
 #include "stk1160.h"
drivers/media/usb/stk1160/stk1160-ac97.c static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg) · line 67
Quoted 6 lines
drivers/media/usb/stk1160/stk1160-ac97.c +3 -0
@@ -61,6 +62,9 @@ static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg)
         */
        stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b);
 
+       /* Give the chip some time to transfer data */
+       usleep_range(20, 40);
+
I don't recall any issues with this. In any case, we only read the registers for debugging purposes, so it's not a big deal.
Maybe it would be better to expand the comment a little bit, using your commit log:
""
The "proper" way to
handle this would be to poll STK1160_AC97CTL_0 after every read or write command until the command bit has been cleared, but this may not be worth the hassle. ""

Quoted 3 lines

This way, if the sleep proves problematic in the future, the "proper way" is already documented.

Of course, since the register dump isn't needed anymore, this function could also be dropped. But then again, I think it would make sense to keep it, even if it's just for documentation. There might be use for it in the future. I'll add a comment as suggested. Let me know whether I should remove the dump, I guess there's no need to keep it. I'll then prepare a new patchset with all four patches as soon as I can.

Quoted 9 lines

Quoted 6 lines

drivers/media/usb/stk1160/stk1160-ac97.c +0 -1

        /* Retrieve register value */
        stk1160_read_reg(dev, STK1160_AC97_CMD, &vall);
        stk1160_read_reg(dev, STK1160_AC97_CMD + 1, &valh);
--

2.10.1

Quoted 3 lines

--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Best regards
Marcel

Marcel Hasler

author

2016-11-26 14:07 UTC #5 lore raw

In reply to Ezequiel Garcia

2016-11-20 18:37 GMT+01:00 Ezequiel Garcia <[email protected]>:

Quoted 27 lines · Marcel Hasler

Quoted 26 lines
The STK1160 needs some time to transfer data from the AC97 registers into its own. On some systems reading the chip's own registers to soon will return wrong values. The "proper" way to handle this would be to poll STK1160_AC97CTL_0 after every read or write command until the command bit has been cleared, but this may not be worth the hassle.
Signed-off-by Marcel Hasler <[email protected]>
---
 drivers/media/usb/stk1160/stk1160-ac97.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/media/usb/stk1160/stk1160-ac97.c +1 -0
index 31bdd60d..caa65a8 100644
@@ -20,6 +20,7 @@
  *
  */
 
+#include <linux/delay.h>
 #include <linux/module.h>
 
 #include "stk1160.h"
@@ -61,6 +62,9 @@ static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg)
         */
        stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b);
 

Quoted 7 lines

drivers/media/usb/stk1160/stk1160-ac97.c static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg) · line 67
Quoted 3 lines
drivers/media/usb/stk1160/stk1160-ac97.c +3 -0
+       /* Give the chip some time to transfer data */
+       usleep_range(20, 40);
+
I don't recall any issues with this. In any case, we only read the registers for debugging purposes, so it's not a big deal.

I actually just re-tested this, as I recently replaced my computer's main board. I didn't happen with my old one, but it does with my new one, just as with both of my notebooks.

Quoted 25 lines

Maybe it would be better to expand the comment a little bit, using your commit log:
""
The "proper" way to
handle this would be to poll STK1160_AC97CTL_0 after every read or write command until the command bit has been cleared, but this may not be worth the hassle. ""
This way, if the sleep proves problematic in the future, the "proper way" is already documented.
Quoted 6 lines
drivers/media/usb/stk1160/stk1160-ac97.c +0 -1
        /* Retrieve register value */
        stk1160_read_reg(dev, STK1160_AC97_CMD, &vall);
        stk1160_read_reg(dev, STK1160_AC97_CMD + 1, &valh);
--
2.10.1
--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar