From 62c21db8dbcb8179cd28466609d6efa5e0eeadbf Mon Sep 17 00:00:00 2001 From: David Brownell Date: Wed, 8 Oct 2008 17:22:30 +0300 Subject: [PATCH] twl4030-pwrirq simplification, cleanup I can't test the pwrbutton stuff, but rtc and musb behave... With one issue: the usb transceiver glue misbehaves on startup if the device is connected. (Again.) I looked at this a bit, and I think the issue is probably caused by not actually using key USB registers ... IRQ trigger mode state transitions are at best a fragile proxy for the real state. (This is similar to the GPIO patch I just sent, but simpler except for the impact on the few drivers thinking oddly about IRQs. Those patches cover the key SIH modules, and a similar one affects the PIH in twl4030-core.) - Dave ================================ Streamline the "power irq" code, and some of the mechanisms it uses: - Support IRQ masking, not just unmasking; simpler code. - Use the standard handle_edge_irq() handler for these edge IRQs. - Use generic_handle_irq() instead of a manual expansion thereof. - Provide a missing spinlock for the shared data. In short, use more of the standard genirq code ... more correctly. Also, update the drivers using the "power IRQ" accordingly: - Don't request IRQF_DISABLED if the handler makes I2C calls; and defend against lockdep forcing it on us. - Let the irq_chip handle IRQ mask/unmask; it handles mutual exclusion between drivers, among other things. - (Unrelated) remove useless MODULE_ALIAS in pwrbutton. The USB transceiver driver still places some dodgey games with IRQ enable/disable, and IRQ trigger flags. At least some of them seem like they'd be simplified by using USB transceiver registers ... notably, startup code, which doesn't seem to check state before it enters an irq-driven mode. For the moment, these drivers still (wrongly) try to configure IRQ trigger modes themselves ... again, that's something that an irq_chip handles (but not yet, for this one). NOTE: tested (MUSB and RTC only) along with the init/retry hack to make twl4030-pwrirq work around the i2c-omap timeout problems. --- drivers/i2c/chips/twl4030-pwrbutton.c | 33 ++++----- drivers/i2c/chips/twl4030-pwrirq.c | 98 +++++++++------------------ drivers/i2c/chips/twl4030-usb.c | 57 ++++++++-------- drivers/rtc/rtc-twl4030.c | 8 +++ 4 files changed, 80 insertions(+), 116 deletions(-) diff --git a/drivers/i2c/chips/twl4030-pwrbutton.c b/drivers/i2c/chips/twl4030-pwrbutton.c index 9a361b3b6d1..cd4d06e8c96 100644 --- a/drivers/i2c/chips/twl4030-pwrbutton.c +++ b/drivers/i2c/chips/twl4030-pwrbutton.c @@ -49,6 +49,14 @@ static irqreturn_t powerbutton_irq(int irq, void *dev_id) int err; u8 value; +#ifdef CONFIG_LOCKDEP + /* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which + * we don't want and can't tolerate. Although it might be + * friendlier not to borrow this thread context... + */ + local_irq_enable(); +#endif + err = twl4030_i2c_read_u8(TWL4030_MODULE_PM_MASTER, &value, STS_HW_CONDITIONS); if (!err) { @@ -67,6 +75,7 @@ static int __init twl4030_pwrbutton_init(void) int err = 0; u8 value; + /* PWRBTN == PWRON */ if (request_irq(TWL4030_PWRIRQ_PWRBTN, powerbutton_irq, 0, "PwrButton", NULL) < 0) { printk(KERN_ERR "Unable to allocate IRQ for power button\n"); @@ -92,22 +101,9 @@ static int __init twl4030_pwrbutton_init(void) goto free_irq_and_out; } - err = twl4030_i2c_read_u8(TWL4030_MODULE_INT, &value, PWR_IMR1); - if (err) { - printk(KERN_WARNING "I2C error %d while reading TWL4030" - " INT PWR_IMR1 register\n", err); - - goto free_input_dev; - } - - err = twl4030_i2c_write_u8(TWL4030_MODULE_INT, - value & (~PWR_PWRON_IRQ), PWR_IMR1); - if (err) { - printk(KERN_WARNING "I2C error %d while writing TWL4030" - " INT PWR_IMR1 register\n", err); - goto free_input_dev; - } - + /* FIXME just pass IRQF_EDGE_FALLING | IRQF_EDGE_RISING + * to request_irq(), once MODULE_INT supports them... + */ err = twl4030_i2c_read_u8(TWL4030_MODULE_INT, &value, PWR_EDR1); if (err) { printk(KERN_WARNING "I2C error %d while reading TWL4030" @@ -136,19 +132,16 @@ free_irq_and_out: out: return err; } +module_init(twl4030_pwrbutton_init); static void __exit twl4030_pwrbutton_exit(void) { free_irq(TWL4030_PWRIRQ_PWRBTN, NULL); input_unregister_device(powerbutton_dev); input_free_device(powerbutton_dev); - } - -module_init(twl4030_pwrbutton_init); module_exit(twl4030_pwrbutton_exit); -MODULE_ALIAS("i2c:twl4030-pwrbutton"); MODULE_DESCRIPTION("Triton2 Power Button"); MODULE_LICENSE("GPL"); MODULE_AUTHOR("Peter De Schrijver"); diff --git a/drivers/i2c/chips/twl4030-pwrirq.c b/drivers/i2c/chips/twl4030-pwrirq.c index 1ef3b7eaa9a..ce66dd4424d 100644 --- a/drivers/i2c/chips/twl4030-pwrirq.c +++ b/drivers/i2c/chips/twl4030-pwrirq.c @@ -29,21 +29,35 @@ #include +static DEFINE_SPINLOCK(pwr_lock); static u8 twl4030_pwrirq_mask; -static u8 twl4030_pwrirq_pending_unmask; static struct task_struct *twl4030_pwrirq_unmask_thread; static void twl4030_pwrirq_ack(unsigned int irq) {} -static void twl4030_pwrirq_disableint(unsigned int irq) {} +static void twl4030_pwrirq_disableint(unsigned int irq) +{ + unsigned long flags; + + spin_lock_irqsave(&pwr_lock, flags); + twl4030_pwrirq_mask |= 1 << (irq - TWL4030_PWR_IRQ_BASE); + if (twl4030_pwrirq_unmask_thread + && twl4030_pwrirq_unmask_thread->state != TASK_RUNNING) + wake_up_process(twl4030_pwrirq_unmask_thread); + spin_unlock_irqrestore(&pwr_lock, flags); +} static void twl4030_pwrirq_enableint(unsigned int irq) { - twl4030_pwrirq_pending_unmask |= 1 << (irq - TWL4030_PWR_IRQ_BASE); - if (twl4030_pwrirq_unmask_thread && - twl4030_pwrirq_unmask_thread->state != TASK_RUNNING) + unsigned long flags; + + spin_lock_irqsave(&pwr_lock, flags); + twl4030_pwrirq_mask &= ~(1 << (irq - TWL4030_PWR_IRQ_BASE)); + if (twl4030_pwrirq_unmask_thread + && twl4030_pwrirq_unmask_thread->state != TASK_RUNNING) wake_up_process(twl4030_pwrirq_unmask_thread); + spin_unlock_irqrestore(&pwr_lock, flags); } static struct irq_chip twl4030_pwrirq_chip = { @@ -53,48 +67,6 @@ static struct irq_chip twl4030_pwrirq_chip = { .unmask = twl4030_pwrirq_enableint, }; -static void do_twl4030_pwrmodule_irq(unsigned int irq, irq_desc_t *desc) -{ - struct irqaction *action; - const unsigned int cpu = smp_processor_id(); - - desc->status |= IRQ_LEVEL; - - if (!desc->depth) { - kstat_cpu(cpu).irqs[irq]++; - - action = desc->action; - if (action) { - int ret; - int status = 0; - int retval = 0; - - do { - ret = action->handler(irq, action->dev_id); - if (ret == IRQ_HANDLED) - status |= action->flags; - retval |= ret; - action = action->next; - } while (action); - - if (status & IRQF_SAMPLE_RANDOM) - add_interrupt_randomness(irq); - - if (retval != IRQ_HANDLED) - printk(KERN_ERR "ISR for TWL4030 power module" - " irq %d can't handle interrupt\n", - irq); - } else { - local_irq_disable(); - twl4030_pwrirq_mask |= 1 << (irq - TWL4030_PWR_IRQ_BASE); - local_irq_enable(); - twl4030_i2c_write_u8(TWL4030_MODULE_INT, - twl4030_pwrirq_mask, - TWL4030_INT_PWR_IMR1); - } - } -} - static void do_twl4030_pwrirq(unsigned int irq, irq_desc_t *desc) { const unsigned int cpu = smp_processor_id(); @@ -113,6 +85,7 @@ static void do_twl4030_pwrirq(unsigned int irq, irq_desc_t *desc) local_irq_enable(); ret = twl4030_i2c_read_u8(TWL4030_MODULE_INT, &pwr_isr, TWL4030_INT_PWR_ISR1); + local_irq_disable(); if (ret) { printk(KERN_WARNING "I2C error %d while reading TWL4030" @@ -122,13 +95,10 @@ static void do_twl4030_pwrirq(unsigned int irq, irq_desc_t *desc) for (module_irq = TWL4030_PWR_IRQ_BASE; pwr_isr != 0; module_irq++, pwr_isr >>= 1) { - if (pwr_isr & 1) { - irq_desc_t *d = irq_desc + module_irq; - d->handle_irq(module_irq, d); - } + if (pwr_isr & 1) + generic_handle_irq(module_irq); } - local_irq_disable(); desc->chip->unmask(irq); } } @@ -138,22 +108,19 @@ static int twl4030_pwrirq_thread(void *data) current->flags |= PF_NOFREEZE; while (!kthread_should_stop()) { - u8 local_unmask; - - local_irq_disable(); - local_unmask = twl4030_pwrirq_pending_unmask; - twl4030_pwrirq_pending_unmask = 0; - local_irq_enable(); + u8 local_mask; - twl4030_pwrirq_mask &= ~local_unmask; + spin_lock_irq(&pwr_lock); + local_mask = twl4030_pwrirq_mask; + spin_unlock_irq(&pwr_lock); - twl4030_i2c_write_u8(TWL4030_MODULE_INT, twl4030_pwrirq_mask, + twl4030_i2c_write_u8(TWL4030_MODULE_INT, local_mask, TWL4030_INT_PWR_IMR1); - local_irq_disable(); - if (!twl4030_pwrirq_pending_unmask) + spin_lock_irq(&pwr_lock); + if (local_mask == twl4030_pwrirq_mask) set_current_state(TASK_INTERRUPTIBLE); - local_irq_enable(); + spin_unlock_irq(&pwr_lock); schedule(); } @@ -166,7 +133,6 @@ static int __init twl4030_pwrirq_init(void) int i, err; twl4030_pwrirq_mask = 0xff; - twl4030_pwrirq_pending_unmask = 0; err = twl4030_i2c_write_u8(TWL4030_MODULE_INT, twl4030_pwrirq_mask, TWL4030_INT_PWR_IMR1); @@ -191,8 +157,8 @@ static int __init twl4030_pwrirq_init(void) } for (i = TWL4030_PWR_IRQ_BASE; i < TWL4030_PWR_IRQ_END; i++) { - set_irq_chip(i, &twl4030_pwrirq_chip); - set_irq_handler(i, do_twl4030_pwrmodule_irq); + set_irq_chip_and_handler(i, &twl4030_pwrirq_chip, + handle_edge_irq); set_irq_flags(i, IRQF_VALID); } diff --git a/drivers/i2c/chips/twl4030-usb.c b/drivers/i2c/chips/twl4030-usb.c index 61c831cfdbf..7852dca4a6d 100644 --- a/drivers/i2c/chips/twl4030-usb.c +++ b/drivers/i2c/chips/twl4030-usb.c @@ -237,14 +237,9 @@ #define GPIO_USB_4PIN_ULPI_2430C (3 << 0) /* In module TWL4030_MODULE_INT */ -#define REG_PWR_ISR1 0x00 -#define REG_PWR_IMR1 0x01 -#define USB_PRES (1 << 2) #define REG_PWR_EDR1 0x05 #define USB_PRES_FALLING (1 << 4) #define USB_PRES_RISING (1 << 5) -#define REG_PWR_SIH_CTRL 0x07 -#define COR (1 << 2) /* bits in OTG_CTRL */ #define OTG_XCEIV_OUTPUTS \ @@ -274,6 +269,7 @@ struct twl4030_usb { unsigned vbus:1; int irq; u8 asleep; + bool irq_enabled; }; /* internal define on top of container_of */ @@ -417,6 +413,8 @@ static void usb_irq_enable(struct twl4030_usb *twl, int rising, int falling) { u8 val; + /* FIXME use set_irq_type(...) when that (soon) works */ + /* edge setup */ WARN_ON(twl4030_i2c_read_u8(TWL4030_MODULE_INT, &val, REG_PWR_EDR1) < 0); @@ -429,34 +427,18 @@ static void usb_irq_enable(struct twl4030_usb *twl, int rising, int falling) WARN_ON(twl4030_i2c_write_u8_verify(twl, TWL4030_MODULE_INT, val, REG_PWR_EDR1) < 0); - /* un-mask interrupt */ - WARN_ON(twl4030_i2c_read_u8(TWL4030_MODULE_INT, - &val, REG_PWR_IMR1) < 0); - - val &= ~USB_PRES; - - WARN_ON(twl4030_i2c_write_u8_verify(twl, TWL4030_MODULE_INT, - val, REG_PWR_IMR1) < 0); + if (!twl->irq_enabled) { + enable_irq(twl->irq); + twl->irq_enabled = true; + } } static void usb_irq_disable(struct twl4030_usb *twl) { - u8 val; - - /* undo edge setup */ - WARN_ON(twl4030_i2c_read_u8(TWL4030_MODULE_INT, - &val, REG_PWR_EDR1) < 0); - val &= ~(USB_PRES_RISING | USB_PRES_FALLING); - WARN_ON(twl4030_i2c_write_u8_verify(twl, TWL4030_MODULE_INT, - val, REG_PWR_EDR1) < 0); - - /* mask interrupt */ - WARN_ON(twl4030_i2c_read_u8(TWL4030_MODULE_INT, - &val, REG_PWR_IMR1) < 0); - val |= USB_PRES; - - WARN_ON(twl4030_i2c_write_u8_verify(twl, TWL4030_MODULE_INT, - val, REG_PWR_IMR1) < 0); + if (twl->irq_enabled) { + disable_irq(twl->irq); + twl->irq_enabled = false; + } } static void twl4030_phy_power(struct twl4030_usb *twl, int on) @@ -565,6 +547,21 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl) struct twl4030_usb *twl = _twl; u8 val; +#ifdef CONFIG_LOCKDEP + /* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which + * we don't want and can't tolerate. Although it might be + * friendlier not to borrow this thread context... + */ + local_irq_enable(); +#endif + + /* FIXME stop accessing PWR_EDR1 ... if nothing else, we + * know which edges we told the IRQ to trigger on. And + * there seem to be OTG_specific registers and irqs that + * provide the right info without guessing like this: + * USB_INT_STS, ID_STATUS, STS_HW_CONDITIONS, etc. + */ + /* action based on cable attach or detach */ WARN_ON(twl4030_i2c_read_u8(TWL4030_MODULE_INT, &val, REG_PWR_EDR1) < 0); @@ -697,7 +694,7 @@ static int __init twl4030_usb_probe(struct platform_device *pdev) /* init irq workqueue before request_irq */ INIT_WORK(&twl->irq_work, twl4030_usb_irq_work); - usb_irq_disable(twl); + twl->irq_enabled = true; status = request_irq(twl->irq, twl4030_usb_irq, 0, "twl4030_usb", twl); if (status < 0) { dev_dbg(&pdev->dev, "can't get IRQ %d, err %d\n", diff --git a/drivers/rtc/rtc-twl4030.c b/drivers/rtc/rtc-twl4030.c index 53c67c636ce..41f66dba4f4 100644 --- a/drivers/rtc/rtc-twl4030.c +++ b/drivers/rtc/rtc-twl4030.c @@ -347,6 +347,14 @@ static irqreturn_t twl4030_rtc_interrupt(int irq, void *rtc) int res; u8 rd_reg; +#ifdef CONFIG_LOCKDEP + /* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which + * we don't want and can't tolerate. Although it might be + * friendlier not to borrow this thread context... + */ + local_irq_enable(); +#endif + res = twl4030_rtc_read_u8(&rd_reg, REG_RTC_STATUS_REG); if (res) goto out; -- 2.41.1