From: Dan T. <log...@gm...> - 2006-02-19 16:15:43
|
Hi all, I'm trying to write a driver for a simple three colour LED which I've hooked up to three GPIO lines. I want to use a timer to make it flash, so I've written a very basic module to see if I can get it working. I've managed to get the interrupt handler to fire at the appropriate frequency, but I've noticed a few weird side effects when I leave the module loaded for a while. The most annoying is that the console (via the serial port) locks up after a couple of minutes. The speed at which it locks up seems to be proportional to the frequency of the timer. Code is attached. Any ideas? Dan --- #include <linux/module.h> #include <linux/config.h> #include <linux/init.h> #include <linux/fs.h> #include <linux/spinlock.h> #include <linux/sysctl.h> #include <linux/interrupt.h> #include <asm/uaccess.h> #include <asm/arch/hardware.h> #include <asm/arch/pxa-regs.h> #include "led.h" #define DEVICE_NAME "led" // GPIOs and whathaveyou static int gGPIORed =3D 61; static int gGPIOGreen =3D 62; static int gGPIOBlue =3D 60; static unsigned long gFreq =3D 10; static unsigned short gCount; //********************************************************************** // START TIMER static void start_timer(unsigned long freq) { unsigned long period; unsigned long oscr; unsigned long osmr2; period =3D 3686400 / freq; oscr =3D OSCR; osmr2 =3D OSMR2; OSMR2 =3D oscr + period; } //********************************************************************** // TOGGLE GPIO static void toggle_gpio(int gpio) { =09if(GPLR(gpio) & GPIO_bit(gpio)) =09=09GPDR(gpio) &=3D ~GPIO_bit(gpio); =09else =09=09GPDR(gpio) |=3D GPIO_bit(gpio); } //********************************************************************** // ON TIMER static irqreturn_t on_timer( int irq, void *dev_id, struct pt_regs *regs ) { // We're called with interrupts disabled. Not sure what these three lines do! (void)irq; (void)dev_id; (void)regs; // Clear the match OSSR |=3D OSSR_M2; =09if(gCount =3D=3D 0) =09toggle_gpio(gGPIOBlue); =09if(gCount =3D=3D 1) =09toggle_gpio(gGPIORed); =09if(gCount =3D=3D 2) =09toggle_gpio(gGPIOGreen); =09if(gCount++ >=3D 3) =09=09gCount =3D 0; =09start_timer(gFreq); return IRQ_HANDLED; } //********************************************************************** // FILE OPERATIONS struct file_operations led_fops =3D { owner: THIS_MODULE, // ioctl: whatever, // open: led_open, // release: led_release }; //********************************************************************** // INIT static int __init led_init( void ) { int rc; =09gCount =3D 0; =09//printk("led: init called\n"); // Register our device with Linux if (( rc =3D register_chrdev( LED_MAJOR, DEVICE_NAME, &led_fops )) < 0 = ) { printk( KERN_WARNING "led: register_chrdev failed for major %d\n", LED_MAJOR ); return rc; } =09pxa_gpio_mode( gGPIORed | GPIO_OUT ); =09clear_gpio(gGPIORed); =09pxa_gpio_mode( gGPIOGreen | GPIO_OUT ); =09clear_gpio(gGPIOGreen); =09pxa_gpio_mode( gGPIOBlue | GPIO_OUT ); =09clear_gpio(gGPIOBlue); set_irq_type( IRQ_OST2, IRQT_RISING ); if (( rc =3D request_irq( IRQ_OST2, on_timer, SA_INTERRUPT, "led", NULL )) !=3D 0 ) { return rc; } // Enable interrupt OIER |=3D OIER_E2; =09start_timer(gFreq); return 0; } //********************************************************************** // EXIT static void __exit led_exit( void ) { free_irq( IRQ_OST2, NULL ); OIER &=3D ~OIER_E2; unregister_chrdev( LED_MAJOR, DEVICE_NAME ); } //********************************************************************** // MODULE SETUP module_init(led_init); module_exit(led_exit); MODULE_AUTHOR("Dan Taylor"); MODULE_DESCRIPTION("Three Colour LED Driver"); MODULE_LICENSE("GPL"); -- Dan Taylor BSc MIEE Software Development Engineer, JTL Systems Ltd PhD Student, Heriot Watt University, UK http://www.logicalgenetics.com |
From: Dave H. <dhy...@gm...> - 2006-02-19 19:39:52
|
Hi Dan, > The most annoying is that the console (via the serial port) locks up > after a couple of minutes. The speed at which it locks up seems to be > proportional to the frequency of the timer. I see that you got some good answers on the linux-arm-kernel list. For everyone else's benefit, I'll include the summary here (credit goes to Russell King and Robon Farine). toggle_gpio was toggling the direction and not the value associated with a GPIO pin. The real problem is this line in on_timer: > // Clear the match > OSSR |=3D OSSR_M2; which should be > OSSR =3D OSSR_M2; This is a subtle distinction and careful reading of the datasheet is required. Many hardware devices use write only registers, and doing the |=3D is a serious mistake. I'm not familiar enough with the PXA255 to have seen that problem by just looking at the code, but having written many device drivers, I'm familiar with the type of problem (and I've been nailed by it too :) The whole read/modify/write issue is also a potentially gnarly issue. I should rereview my robostix driver to make sure I'm not doing anything bad :) And, finally, set_irq_type is obsolete and should be replaced with the appropriate flags being passed into request_irq (this particular issue has been discussed several times on the arm-linux-kernel list, and you can read all about it in the archives). -- Dave Hylands Vancouver, BC, Canada http://www.DaveHylands.com/ |
From: Dan T. <log...@gm...> - 2006-02-20 00:53:33
|
Yep, that was the problem... Replacing |=3D with =3D made it work fine and I'm in the process of sorting all the other stuff out now. Hadn't quite got my head around the race hazards and the "write only" register thing and had attacked the problem like I would in user space. It's hard work getting to grips with some of these issues, but very good fun too :o) Just one point of note, the set_irq_type call was "inspired" by the sample button driver on the Gumstix wikki. I don't mean to sound ungrateful, because that particular example has been invaluable to me during my baptism of fire in the world of device drivers, but it might need looking at in future - or maybe just marking with a comment? Anyway, I'm going to spend the rest of the evening gazing at my flashing LED like a loving parent. Cheers, Dan On 19/02/06, Dave Hylands <dhy...@gm...> wrote: > Hi Dan, > > > The most annoying is that the console (via the serial port) locks up > > after a couple of minutes. The speed at which it locks up seems to be > > proportional to the frequency of the timer. > > I see that you got some good answers on the linux-arm-kernel list. > > For everyone else's benefit, I'll include the summary here (credit > goes to Russell King and Robon Farine). > > toggle_gpio was toggling the direction and not the value associated > with a GPIO pin. > > The real problem is this line in on_timer: > > > // Clear the match > > OSSR |=3D OSSR_M2; > > which should be > > > OSSR =3D OSSR_M2; > > This is a subtle distinction and careful reading of the datasheet is > required. Many hardware devices use write only registers, and doing > the |=3D is a serious mistake. > > I'm not familiar enough with the PXA255 to have seen that problem by > just looking at the code, but having written many device drivers, I'm > familiar with the type of problem (and I've been nailed by it too :) > > The whole read/modify/write issue is also a potentially gnarly issue. > I should rereview my robostix driver to make sure I'm not doing > anything bad :) > > And, finally, set_irq_type is obsolete and should be replaced with the > appropriate flags being passed into request_irq (this particular issue > has been discussed several times on the arm-linux-kernel list, and you > can read all about it in the archives). > > -- > Dave Hylands > Vancouver, BC, Canada > http://www.DaveHylands.com/ > > > ------------------------------------------------------- > This SF.net email is sponsored by: Splunk Inc. Do you grep through log fi= les > for problems? Stop! Download the new AJAX search engine that makes > searching your log files as easy as surfing the web. DOWNLOAD SPLUNK! > http://sel.as-us.falkag.net/sel?cmdlnk&kid=103432&bid#0486&dat=121642 > _______________________________________________ > gumstix-users mailing list > gum...@li... > https://lists.sourceforge.net/lists/listinfo/gumstix-users > -- Dan Taylor BSc MIEE Software Development Engineer, JTL Systems Ltd PhD Student, Heriot Watt University, UK http://www.logicalgenetics.com |
From: Dave H. <dhy...@gm...> - 2006-02-20 01:11:22
|
Hi Dan, > Replacing |=3D with =3D made it work fine and I'm in the process of > sorting all the other stuff out now. Hadn't quite got my head around > the race hazards and the "write only" register thing and had attacked > the problem like I would in user space. It's hard work getting to > grips with some of these issues, but very good fun too :o) Yeah - that's why I like doing driver development. > Just one point of note, the set_irq_type call was "inspired" by the > sample button driver on the Gumstix wikki. I don't mean to sound > ungrateful, because that particular example has been invaluable to me > during my baptism of fire in the world of device drivers, but it might > need looking at in future - or maybe just marking with a comment? I'll update the example. At the time I wrote it, set_irq_type was still the way to do it. Incorrect usage of set_irq_type isn't really a problem on the gumstix for the GPIO example. It's only really an issue if two drivers are both trying to use the same interrupt, in which case calling set_irq_type before calling request_irq is bad form. The newer method of passing the flags into request_irq is the best way of doing it. > Anyway, I'm going to spend the rest of the evening gazing at my > flashing LED like a loving parent. Yeah - it's always cool when stuff works the way its supposed to :) -- Dave Hylands Vancouver, BC, Canada http://www.DaveHylands.com/ |