[alsa-devel] [PATCH] ad1848 and cs4231 busy loop replacement
From: Krzysztof Helt krzysztof.h1@wp.pl
This patch replaces busy loop with msleep and removes incorrect debug message.
Signed-off-by: Krzysztof Helt krzysztof.h1@wp.pl ---
This should fix Rene's problem with the timeout message he got in his new Aztech driver.
The problem is that the driver wait for busy bit to go high but it may not happen if the auto-calibration is not set. The result is incorrect debug message (that timeout happened). It is also possible (in theory) to get this message in the ad1848-lib on a slow CPU.
The patch just wait a period around the period needed for the auto-calibration. If there is no auto-calibration set, the driver just loses few miliseconds. This may happen only during driver start as both drivers set auto-calibration. If the auto-calibration is set we give CPU more time to do something else then move to next waiting loop (for the auto-calibration bit to get cleared).
The msleep delay is calculated as rounded down time of waiting number of cycles (from ad1848k and cs4231a specs) with the highest frequency (48kHz).
Rene, please test this patch on the cs4231 codec.
diff -urp linux-2.6.22/sound/isa/ad1848/ad1848_lib.c linux-2.6.23/sound/isa/ad1848/ad1848_lib.c --- linux-2.6.22/sound/isa/ad1848/ad1848_lib.c 2007-09-08 23:53:42.958067597 +0200 +++ linux-2.6.23/sound/isa/ad1848/ad1848_lib.c 2007-09-08 23:57:31.255077502 +0200 @@ -229,17 +229,12 @@ static void snd_ad1848_mce_down(struct s spin_unlock_irqrestore(&chip->reg_lock, flags); return; } + /* calibration process */ + msleep(7); + + snd_printd("(2) jiffies = %li\n", jiffies);
- for (timeout = 500; timeout > 0 && (snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) == 0; timeout--); - if ((snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) == 0) { - snd_printd("mce_down - auto calibration time out (1)\n"); - spin_unlock_irqrestore(&chip->reg_lock, flags); - return; - } -#if 0 - printk("(2) timeout = %i, jiffies = %li\n", timeout, jiffies); -#endif time = HZ / 4; while (snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) { spin_unlock_irqrestore(&chip->reg_lock, flags); diff -urp linux-2.6.22/sound/isa/cs423x/cs4231_lib.c linux-2.6.23/sound/isa/cs423x/cs4231_lib.c --- linux-2.6.22/sound/isa/cs423x/cs4231_lib.c 2007-09-08 23:53:29.005272473 +0200 +++ linux-2.6.23/sound/isa/cs423x/cs4231_lib.c 2007-09-08 23:56:52.824887490 +0200 @@ -334,19 +334,12 @@ void snd_cs4231_mce_down(struct snd_cs42 !(chip->hardware & (CS4231_HW_CS4231_MASK | CS4231_HW_CS4232_MASK))) { return; } - snd_cs4231_busy_wait(chip);
/* calibration process */ + msleep(2); + + snd_printd("(2) jiffies = %li\n", jiffies);
- for (timeout = 500; timeout > 0 && (snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) == 0; timeout--) - udelay(10); - if ((snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) == 0) { - snd_printd("cs4231_mce_down - auto calibration time out (1)\n"); - return; - } -#if 0 - printk("(2) timeout = %i, jiffies = %li\n", timeout, jiffies); -#endif /* in 10 ms increments, check condition, up to 250 ms */ timeout = 25; while (snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) {
On 09/09/2007 12:12 AM, Krzysztof Helt wrote:
This should fix Rene's problem with the timeout message he got in his new Aztech driver.
The problem is that the driver wait for busy bit to go high but it may not happen if the auto-calibration is not set. The result is incorrect debug message (that timeout happened). It is also possible (in theory) to get this message in the ad1848-lib on a slow CPU.
The patch just wait a period around the period needed for the auto-calibration. If there is no auto-calibration set, the driver just loses few miliseconds. This may happen only during driver start as both drivers set auto-calibration. If the auto-calibration is set we give CPU more time to do something else then move to next waiting loop (for the auto-calibration bit to get cleared).
Yes, thank you, this appears to be a proper analysis. Unfortunately, I don't believe the patch itself is correct though:
The msleep delay is calculated as rounded down time of waiting number of cycles (from ad1848k and cs4231a specs) with the highest frequency (48kHz).
As you say, the next loop is waiting for the auto-calibration bit to go down again, so here we should be waiting (the maximum time needed) for it to come up after dropping the MCE bit. You do a msleep(7), which I assume is derived (somehow...) from the 384 cycles mentioned in the spec, at 48kHz, but that's the time the auto-calibration bit will stay up after _having_ come up.
On page 24 of the AD1848K spec (2nd column, near top) we have:
* Clear the Mode Change Enable (MCE) bit
* The Autocalibrate-In-Progrss (ACI) bit will transition from LO to HI within 5 sample periods. [ ... ]
Which seems to be saying that we should be waiting for only 5 cycles at that point. With the slowest (!) possible sampling rate of 5.5125 kHz that would mean 907 us, or sligtly under 1 ms.
For the CS4231, the datasheet implies ACI should be up immediately upon dropping MCE (when auto-calibrating). On page 20 of the CS4231A sheet:
3) Return from Mode Change Enable by resetting the MCE bit [ ... ] 4) Wait until ACI cleared to proceed
As such, I've tested the below on CS4231A and there it seems to be working fine. Also delaying for 1ms in cs2431 would be fine by me as well...
Rene, please test this patch on the cs4231 codec.
Given that the only difference is that I wait less, yours would've worked as well -- thanks for looking into this! And could you viceversa test this on AD1848 and perhaps resubmit if you agree?
Rene.
diff --git a/sound/isa/ad1848/ad1848_lib.c b/sound/isa/ad1848/ad1848_lib.c index 8094282..4d951ac 100644 --- a/sound/isa/ad1848/ad1848_lib.c +++ b/sound/isa/ad1848/ad1848_lib.c @@ -227,16 +227,14 @@ static void snd_ad1848_mce_down(struct snd_ad1848 *chip) spin_unlock_irqrestore(&chip->reg_lock, flags); return; } - /* calibration process */
- for (timeout = 500; timeout > 0 && (snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) == 0; timeout--); - if ((snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) == 0) { - snd_printd("mce_down - auto calibration time out (1)\n"); - spin_unlock_irqrestore(&chip->reg_lock, flags); - return; - } + /* + * If auto-calibrating, CALIB_IN_PROGRESS will be up within 5 sample + * periods which at the slowest rate of 5.5125 kHz means ~ 907 us. + */ + mdelay(1); #if 0 - printk("(2) timeout = %i, jiffies = %li\n", timeout, jiffies); + printk("(2) jiffies = %li\n", jiffies); #endif time = HZ / 4; while (snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) { diff --git a/sound/isa/cs423x/cs4231_lib.c b/sound/isa/cs423x/cs4231_lib.c index 914d77b..bd1affe 100644 --- a/sound/isa/cs423x/cs4231_lib.c +++ b/sound/isa/cs423x/cs4231_lib.c @@ -344,18 +344,8 @@ void snd_cs4231_mce_down(struct snd_cs4231 *chip) !(chip->hardware & (CS4231_HW_CS4231_MASK | CS4231_HW_CS4232_MASK))) { return; } - snd_cs4231_busy_wait(chip); - - /* calibration process */ - - for (timeout = 500; timeout > 0 && (snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) == 0; timeout--) - udelay(10); - if ((snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) == 0) { - snd_printd("cs4231_mce_down - auto calibration time out (1)\n"); - return; - } #if 0 - printk("(2) timeout = %i, jiffies = %li\n", timeout, jiffies); + printk("(2) jiffies = %li\n", jiffies); #endif /* in 10 ms increments, check condition, up to 250 ms */ timeout = 25;
On 09/09/2007 11:09 PM, Rene Herman wrote:
Given that the only difference is that I wait less, yours would've worked as well -- thanks for looking into this! And could you viceversa test this on AD1848 and perhaps resubmit if you agree?
Oh, and I forgot to say (sigh) that I use a mdelay() and not an msleep() in the ad1848 one due to the 1 ms being short enough and didn't keep the snd_printd() because if one, then all please...
If you yourself insist on keeping a change like that split off, then doing that as a second patch ontop is best I guess but I suspect Takashi wouldn't mind trivial stuff like that anymore than I would -- we're not rule-driven robots...
Rene.
On Sun, 09 Sep 2007 23:19:56 +0200 Rene Herman rene.herman@gmail.com wrote:
On 09/09/2007 11:09 PM, Rene Herman wrote:
Given that the only difference is that I wait less, yours would've worked as well -- thanks for looking into this! And could you viceversa test this on AD1848 and perhaps resubmit if you agree?
Oh, and I forgot to say (sigh) that I use a mdelay() and not an msleep() in the ad1848 one due to the 1 ms being short enough and didn't keep the snd_printd() because if one, then all please...
I tested CS4231 on 450MHz sparc with 1000Hz kernel (I don't have PC card yet). The waiting loop was changed as in my second patch (the 10ms blocks to schedule_timeout()). I played 48kHz wav (alsa "Front_Center.wav").
The waiting loop was passed 340 times after mce was down with msleep(1). The delay between snd_printd "2 jiffies" and "8 jiffies" was 3 jiffies.
The waiting loop was passed 34 times after mce was down with msleep(2). The delay between snd_printd "2 jiffies" and "8 jiffies" was 3 jiffies too.
It took about ten times less loop passes if the driver waits 2ms. The 2ms seems like the optimal value for the CS4231.
Regards, Krzysztof
Rene wrote:
As you say, the next loop is waiting for the auto-calibration bit to go down again, so here we should be waiting (the maximum time needed) for it to come up after dropping the MCE bit. You do a msleep(7), which I assume is derived (somehow...) from the 384 cycles mentioned in the spec, at 48kHz, but that's the time the auto-calibration bit will stay up after _having_ come up.
On page 24 of the AD1848K spec (2nd column, near top) we have:
Clear the Mode Change Enable (MCE) bit
The Autocalibrate-In-Progrss (ACI) bit will transition from LO to HI within 5 sample periods. [ ... ]
Which seems to be saying that we should be waiting for only 5 cycles at that point. With the slowest (!) possible sampling rate of 5.5125 kHz that would mean 907 us, or sligtly under 1 ms.
It does not matter. If you have to wait for the start of the auto-calibration then for the end of auto-calibration you may as well wait for the end of the auto-calibration only (unless you want to follow the auto-calibration to the very single clock pulse ;-). The delay here is simple to assure that auto-calibration started (and probably is already under way). It may be longer then the start but it should not be longer than whole auto-calibration. I simply tried to do as long msleep as possible to allow other tasks using CPU and decrease number of waiting loop iterations.
I tested on the AD1848 before I posted it. The 7ms delay was never enough for auto-calibration to complete. Even at 48kHz, tens of waiting loops iterations happened on my SC-6000. Tens of waiting loops is much less than 1 jiffy, and msleep(2) is 2 jiffies. Och, and my AD1848 returns 0xA revision which means it is AD1848K despite it is marked as AD1848 only.
fine. Also delaying for 1ms in cs2431 would be fine by me as well...
I tested this on SPARC cs4231. The 2ms and 1ms gives the same results which may be attributed to kernel timer resolution (2 jiffies for msleep(1) and msleep(2)). It was enough for the whole autocalibration (no waiting loop iterations at 48kHz). I got 2.8ms delay at 48kHz when I calculated it from the specs. I will retest it with kernel set to 1000 Hz to confirm if the 2ms is too long (it should not be we are below kernel timer resolution).
I will post SPARC cs4231 patch (with sleep and improved waiting) when Takashi will say if the kernel or alsa version of the driver is correct.
Rene, please test this patch on the cs4231 codec.
Given that the only difference is that I wait less, yours would've worked as well -- thanks for looking into this! And could you viceversa test this on AD1848 and perhaps resubmit if you agree?
Thanks, Krzysztof
On 09/10/2007 09:08 AM, Krzysztof Helt wrote:
Rene wrote:
Which seems to be saying that we should be waiting for only 5 cycles at that point. With the slowest (!) possible sampling rate of 5.5125 kHz that would mean 907 us, or sligtly under 1 ms.
It does not matter. If you have to wait for the start of the auto-calibration then for the end of auto-calibration you may as well wait for the end of the auto-calibration only (unless you want to follow the auto-calibration to the very single clock pulse ;-). The delay here is simple to assure that auto-calibration started (and probably is already under way). It may be longer then the start but it should not be longer than whole auto-calibration.
I simply tried to do as long msleep as possible to allow other tasks using CPU and decrease number of waiting loop iterations.
Well, no, sorry, but I consider that to be completely breaking the logic of the code. One line after this bit, we're doing the "wait for calibration to finish" loop (and already schedule ourselves away while doing so, for 250ms no less) meaning that at this point we should only wait long enough to be guaranteed that the ACI bit reflects reality -- those 5 cycles.
Your: wait unconditionally until calibration _nearly_ done, then go wait for it for 250 ms to be really done.
Mine: wait unconditionally until calibration has started, then go wait for it for 250 ms to finish.
Really -- please just do the 1 ms delay for ad1848 and either no delay or the same 1 ms for cs4231 if you want to keep them in sync (and the comment explaining why it's 1 ms). I don't particularly care about mdelay versus msleep here (although a mere 1 ms fits mdelay better) but your setup is pointlessly obscuring the code-flow.
Rene.
On 9/10/07, Rene Herman rene.herman@gmail.com wrote:
On 09/10/2007 09:08 AM, Krzysztof Helt wrote:
Rene wrote:
Well, no, sorry, but I consider that to be completely breaking the logic of the code.
No I changed the logic of this code not to wait for specifically for callibration "start" but into calibration "under way".
One line after this bit, we're doing the "wait for calibration to finish" loop (and already schedule ourselves away while doing so, for 250ms no less) meaning that at this point we should only wait long enough to be guaranteed that the ACI bit reflects reality -- those 5 cycles.
Your: wait unconditionally until calibration _nearly_ done, then go wait for it for 250 ms to be really done.
Mine: wait unconditionally until calibration has started, then go wait for it for 250 ms to finish.
This discussion get me amused. Look at the code:
Mine:
msleep(7); /* or msleep(2) for CS4231 */
/*waiting loop 250ms*/ while ...
Yours: msleep(1); /* or msleep(1) for CS4231 */
/*waiting loop 250ms */ while ...
So the only difference is 6 (or 1) ms and this time will be spent in the loop anyway. Are we arguing 1ms (for CS4231) in 250ms waiting loop? If you really want it can be substracted from the waiting loop 250ms to be exactly the same as it was.
Really -- please just do the 1 ms delay for ad1848 and either no delay or the same 1 ms for cs4231 if you want to keep them in sync
I don't understand "keep them in sync". After the patch they behave the same but with different delay (they have different speed). It can be 2ms (as for faster one) for both but I wanted to avoid doing many loop iterations if possible.
Regards, Krzysztof
On 09/10/2007 02:42 PM, Krzysztof Helt wrote:
On 9/10/07, Rene Herman rene.herman@gmail.com wrote:
Well, no, sorry, but I consider that to be completely breaking the logic of the code.
No I changed the logic of this code not to wait for specifically for callibration "start" but into calibration "under way".
No, waiting for calibration the be under way is what my 0/1 ms does. You are waiting for it to be nearly done, which is complete nonsense. One line below we are waiting for 250 ms (generally with _one_ pass through the loop -- we only wake up through signals) anyway!
The no delay at all from cs4231 is the logic -- when we've dropped MCE, ACI comes up (when auto-calibrating) and we only wait for it to finish. For ad1848, ACI up may take 5 cycles from MCE down so we delay 1 ms so we know we're testing correctly.
Your: wait unconditionally until calibration _nearly_ done, then go wait for it for 250 ms to be really done.
Mine: wait unconditionally until calibration has started, then go wait for it for 250 ms to finish.
[ ... ]
So the only difference is 6 (or 1) ms and this time will be spent in the loop anyway. Are we arguing 1ms (for CS4231) in 250ms waiting loop?
No, we are arguing maintaining code. Do not obscure the code flow for no reason. Fix your logic or (for what it's worth) I am going to NAK the change.
I don't understand "keep them in sync".
In sync source-code wise. While the no delay from cs4231 may be the rule, ad1848 needs a small delay so if you'd wanted to keep them looking the same I wouldn't care about a 1 ms delay for cs4231 as well. If you don't, fine as well.
Rene.
On Mon, 10 Sep 2007 15:13:24 +0200 Rene Herman rene.herman@gmail.com wrote:
On 09/10/2007 02:42 PM, Krzysztof Helt wrote:
On 9/10/07, Rene Herman rene.herman@gmail.com wrote:
Well, no, sorry, but I consider that to be completely breaking the logic of the code.
No I changed the logic of this code not to wait for specifically for callibration "start" but into calibration "under way".
No, waiting for calibration the be under way is what my 0/1 ms does. You are waiting for it to be nearly done, which is complete nonsense. One line below we are waiting for 250 ms (generally with _one_ pass through the loop -- we only wake up through signals) anyway!
I don't buy your argument that it is complete nonsense. I want some merit argument like: your code uses more memory/cpu, your code is longer, your code drives hardware into undefined states, your code push hardware outside hardware limits, your code is slower/adds more delay.
Also, I don't think mimicking the hardware behaviour in driver code is correct paradigm. I think that correct way is to _drive_ the hardware. If the hardware must be taken from state A to state C and goes through state B , the driver may wait only for the C completely ignoring the B if the B is irrelevant (no need to set or get something from the hardware until the C).
Another thing I believe is that driver should be "CPU friendly" means it should gives back control to the CPU every time it knows the hardware is busy and nothing can be done for some known period of time. That's why I don't want to use all these m/udelay functions. They make CPU busy. Sometimes they are must have, sometimes not. If one knows that it must wait at least 7ms why it should wait 1ms than makes another 6ms inside the loop?
Actually, if you look into the lower delay argument, the waiting loop after the change is inefficient. The whole calibration takes 2-3ms at the highest rate, but one has to wait in 10ms blocks. In order to lower this delay one must change it to some finer way of returning time to the CPU (cpu_relax() or schedule_timeout() - like ad1848). If it is done, the loop will make thousands of iterations every few miliseconds. Taking your argument that loop should be passed in the least possible iterations (you wrote "preferably one pass"), the only way is to wait longer before entering the loop.
No, we are arguing maintaining code. Do not obscure the code flow for no reason. Fix your logic or (for what it's worth) I am going to NAK the change.
The code does not be obscured with one simple comment before msleep: /* wait till calibration is nearly done */
Also comments like "my way or highway" (NAK) won't make you many friends.
Krzysztof
On 09/10/2007 07:05 PM, Krzysztof Helt wrote:
Rene Herman rene.herman@gmail.com wrote:
No, waiting for calibration the be under way is what my 0/1 ms does. You are waiting for it to be nearly done, which is complete nonsense. One line below we are waiting for 250 ms (generally with _one_ pass through the loop -- we only wake up through signals) anyway!
I don't buy your argument that it is complete nonsense.
You don't need to buy it -- I presented it to you for free on a fucking platter 4 times now. You are making me waste my time. Answer this or just bury this useles nonsense discussion: why do you insist on delaying for 7 ms when the specification says (and the hardware is confirming) that 1 ms is enough?
You are delaying 7 ms and then 250 ms when we are auto-calibrating. I delay 1 ms and then 250 ms when we are auto-calibrating. Why do you delay longer than the hardware needs and make everyone who ever reads this code wonder why the hell that odd delay is in there? And make everyone who ever will be re-writing or porting this code drag it along on the notion that someone, somewhere probably once had a purpose for it?
Fix it up with a comment? Why? There's still not a single point to it.
Why 7? Or 2? Or whatever more time than needed for the calibration to start?
Also comments like "my way or highway" (NAK) won't make you many friends.
Good, I don't need more friends. You are doing the generic newbie thing where you assume you obviously must know everything better than everyone else. Just shelve that dumb crap.
You also may want to take into account that I'm taking the time to review your code and dig trough datasheets while I had in fact different things to do and it's proving utterly useless.
Fixed patch attached.
Rene.
diff --git a/sound/isa/ad1848/ad1848_lib.c b/sound/isa/ad1848/ad1848_lib.c index 8094282..dcd4435 100644 --- a/sound/isa/ad1848/ad1848_lib.c +++ b/sound/isa/ad1848/ad1848_lib.c @@ -227,16 +227,15 @@ static void snd_ad1848_mce_down(struct snd_ad1848 *chip) spin_unlock_irqrestore(&chip->reg_lock, flags); return; } - /* calibration process */
- for (timeout = 500; timeout > 0 && (snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) == 0; timeout--); - if ((snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) == 0) { - snd_printd("mce_down - auto calibration time out (1)\n"); - spin_unlock_irqrestore(&chip->reg_lock, flags); - return; - } + /* + * Wait for (possible -- during init auto-calibration may not be set) + * calibration process to start. Needs upto 5 sample periods on AD1848 + * which at the slowest possible rate of 5.5125 kHz means 907 us. + */ + msleep(1); #if 0 - printk("(2) timeout = %i, jiffies = %li\n", timeout, jiffies); + printk("(2) jiffies = %li\n", jiffies); #endif time = HZ / 4; while (snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) { diff --git a/sound/isa/cs423x/cs4231_lib.c b/sound/isa/cs423x/cs4231_lib.c index 914d77b..9e8e0f1 100644 --- a/sound/isa/cs423x/cs4231_lib.c +++ b/sound/isa/cs423x/cs4231_lib.c @@ -346,16 +346,14 @@ void snd_cs4231_mce_down(struct snd_cs4231 *chip) } snd_cs4231_busy_wait(chip);
- /* calibration process */ - - for (timeout = 500; timeout > 0 && (snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) == 0; timeout--) - udelay(10); - if ((snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) == 0) { - snd_printd("cs4231_mce_down - auto calibration time out (1)\n"); - return; - } + /* + * Wait for (possible -- during init auto-calibration may not be set) + * calibration process to start. Needs upto 5 sample periods on AD1848 + * which at the slowest possible rate of 5.5125 kHz means 907 us. + */ + msleep(1); #if 0 - printk("(2) timeout = %i, jiffies = %li\n", timeout, jiffies); + printk("(2) jiffies = %li\n", jiffies); #endif /* in 10 ms increments, check condition, up to 250 ms */ timeout = 25;
participants (2)
-
Krzysztof Helt -
Rene Herman