Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#11130 - Rewrite ascodec_as3514.c to use interrupts

Attached to Project: Rockbox
Opened by Tobias Diedrich (ranma) - Friday, 19 March 2010, 19:53 GMT+2
Last edited by Tobias Diedrich (ranma) - Tuesday, 23 March 2010, 10:06 GMT+2
Task Type Patches
Category Operating System/Drivers
Status Closed
Assigned To Tobias Diedrich (ranma)
Player Type Sansa c200
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This patch rewrites ascodec_as3514.c to use interrupts.
It gets rid of the ascodec mutex and instead introduces a mutex in adc-as3514.c.
Requests are handled using a simple request queue, and allows asynchronous request to be started from interrupt context.
The (20+sizeof(struct wakeup)) bytes request struct is either allocated on the stack (synchronous read/write) or has to be allocated statically by the caller (asynchronous read).

The asynchronous write is used in system-as3525.c (which is run with interrupts disabled).
asynchronous reads are used by another pending patch of mine implementing AUDIO_IRQ (int 9) handling for usb_plug detection without needing a thread.
   i2c_irq.patch (13.5 KiB) 
 rockbox-pwm/firmware/target/arm/as3525/ascodec-as3525.c |  273 ++++++++++++----
 rockbox-pwm/firmware/target/arm/as3525/system-as3525.c  |    8 
 rockbox-pwm/firmware/target/arm/adc-as3514.c            |    8 
 rockbox-pwm/firmware/target/arm/as3525/ascodec-target.h |   60 +++
 4 files changed, 288 insertions(+), 61 deletions(-)

This task depends upon

Closed by  Tobias Diedrich (ranma)
Tuesday, 23 March 2010, 10:06 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  Committed
Comment by Rafaël Carré (funman) - Friday, 19 March 2010, 20:08 GMT+2
Good work!

You changed ascodec_lock() into a adc specific lock in adc-as3514.c, but this file is also used by e200v1/c200v1 : doesn't it cause a problem for those devices?
Comment by Tobias Diedrich (ranma) - Friday, 19 March 2010, 21:23 GMT+2
Good question. I think you're right. Updated patch attached.
   ascodec_irq.patch (12.8 KiB) 
 rockbox-irqwork/firmware/target/arm/as3525/ascodec-as3525.c |  276 ++++++++++--
 rockbox-irqwork/firmware/target/arm/as3525/system-as3525.c  |    8 
 rockbox-irqwork/firmware/target/arm/as3525/ascodec-target.h |   51 ++
 3 files changed, 288 insertions(+), 47 deletions(-)

Comment by Rafaël Carré (funman) - Saturday, 20 March 2010, 16:41 GMT+2
Tested on Clip+ : OK
Comment by Michael Chicoine (mc2739) - Saturday, 20 March 2010, 23:14 GMT+2
Tested on e260v2 with r25262:

When I attach the USB cable while in the Rockbox menu I intermittently get a black screen lockup that requires a long poweroff (10 secs) to recover.

All other functions appear normal.
Comment by Tobias Diedrich (ranma) - Sunday, 21 March 2010, 06:22 GMT+2
> Michael Chicoine (mc2739)
Only with this patch? Or with 11130+11131 applied?
Since 11130 doesn't change the usb detection I wonder why it should cause a lockup on usb plugin...
Hmm, backlight is controlled using as3514 on e200 though and system_reboot() calls _backlight_off() (why? If it reboots the backlight will be turned back on very soon anyway...). So if system_reboot() is called with interrupts disabled it would hang. Though it's should be called from the usb thread, which means interrupts should be enabled and I don't see why suddenly at that point the interrupt wouldn't fire if it has been working fine that far...
Comment by Tobias Diedrich (ranma) - Sunday, 21 March 2010, 08:36 GMT+2
> Michael Chicoine (mc2739)
Can you try this?
It will fall back to polling if irqs are disabled.
   ascodec_irq.patch (13.2 KiB) 
 rockbox-irqwork/firmware/target/arm/as3525/ascodec-as3525.c |  309 ++++++++++--
 rockbox-irqwork/firmware/target/arm/as3525/system-as3525.c  |    1 
 rockbox-irqwork/firmware/target/arm/as3525/ascodec-target.h |   51 +
 3 files changed, 314 insertions(+), 47 deletions(-)

Comment by Michael Chicoine (mc2739) - Sunday, 21 March 2010, 15:19 GMT+2
I tested this patch by itself first, and then tested 11131 along with this patch. The lockup occurred in both cases.

So far, with minimal testing, the new patch looks good. I have not had any lockups, yet. I will continue testing throughout the day. I will also test it with 11131 also applied.
Comment by Michael Chicoine (mc2739) - Monday, 22 March 2010, 12:31 GMT+2
After further testing, I have not had any lockups with the latest patch. No other problems noted.

Loading...