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

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

[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 <mahasler@gmail.com>
---
 drivers/media/usb/stk1160/stk1160-ac97.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
index 31bdd60d..caa65a8 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -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

Fwd: [PATCH] stk1160: Give the chip some time to retrieve data from AC97 codec.

[Marcel Hasler]

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 <mahasler@gmail.com>:
> 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 <mahasler@gmail.com>
> ---
>  drivers/media/usb/stk1160/stk1160-ac97.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
> index 31bdd60d..caa65a8 100644
> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
> @@ -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
>

Re: [PATCH] stk1160: Give the chip some time to retrieve data from AC97 codec.

[Ezequiel Garcia]

On 28 October 2016 at 05:52, Marcel Hasler <mahasler@gmail.com> wrote:
> 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 <mahasler@gmail.com>
> ---
>  drivers/media/usb/stk1160/stk1160-ac97.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
> index 31bdd60d..caa65a8 100644
> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
> @@ -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);
> +

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.

>         /* 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

Re: [PATCH] stk1160: Give the chip some time to retrieve data from AC97 codec.

[Marcel Hasler]

2016-11-20 18:37 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
> On 28 October 2016 at 05:52, Marcel Hasler <mahasler@gmail.com> wrote:
>> 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 <mahasler@gmail.com>
>> ---
>>  drivers/media/usb/stk1160/stk1160-ac97.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
>> index 31bdd60d..caa65a8 100644
>> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
>> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
>> @@ -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);
>> +
>
> 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.
>

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.

>>         /* 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

Best regards
Marcel

Re: [PATCH] stk1160: Give the chip some time to retrieve data from AC97 codec.

[Marcel Hasler]

2016-11-20 18:37 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
> On 28 October 2016 at 05:52, Marcel Hasler <mahasler@gmail.com> wrote:
>> 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 <mahasler@gmail.com>
>> ---
>>  drivers/media/usb/stk1160/stk1160-ac97.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c
>> index 31bdd60d..caa65a8 100644
>> --- a/drivers/media/usb/stk1160/stk1160-ac97.c
>> +++ b/drivers/media/usb/stk1160/stk1160-ac97.c
>> @@ -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);
>> +
>
> 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.

> 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.
>
>>         /* 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