Message ID | 1443209283-20781-3-git-send-email-grygorii.strashko@ti.com |
---|---|
State | New |
Headers | show |
On Fri, Sep 25, 2015 at 12:28 PM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > This patch converts TI OMAP GPIO driver to use generic irq handler > instead of chained IRQ handler. This way OMAP GPIO driver will be > compatible with RT kernel where it will be forced thread IRQ handler > while in non-RT kernel it still will be executed in HW IRQ context. > As part of this change the IRQ wakeup configuration is applied to > GPIO Bank IRQ as it now will be under control of IRQ PM Core during > suspend. > > There are also additional benefits: > - on-RT kernel there will be no complains any more about PM runtime usage > in atomic context "BUG: sleeping function called from invalid context"; > - GPIO bank IRQs will appear in /proc/interrupts and its usage statistic > will be visible; > - GPIO bank IRQs could be configured through IRQ proc_fs interface and, > as result, could be a part of IRQ balancing process if needed; > - GPIO bank IRQs will be under control of IRQ PM Core during > suspend to RAM. > > Disadvantage: > - additional runtime overhed as call chain till > omap_gpio_irq_handler() will be longer now > - necessity to use wa_lock in omap_gpio_irq_handler() to W/A warning > in handle_irq_event_percpu() > WARNING: CPU: 1 PID: 35 at kernel/irq/handle.c:149 handle_irq_event_percpu+0x51c/0x638() > > This patch doesn't fully follows recommendations provided by Sebastian > Andrzej Siewior [1], because It's required to go through and check all > GPIO IRQ pin states as fast as possible and pass control to handle_level_irq > or handle_edge_irq. handle_level_irq or handle_edge_irq will perform actions > specific for IRQ triggering type and wakeup corresponding registered > threaded IRQ handler (at least it's expected to be threaded). > IRQs can be lost if handle_nested_irq() will be used, because excecution > time of some pin specific GPIO IRQ handler can be very significant and > require accessing ext. devices (I2C). > > Idea of such kind reworking was also discussed in [2]. > > [1] http://www.spinics.net/lists/linux-omap/msg120665.html > [2] http://www.spinics.net/lists/linux-omap/msg119516.html > > Tested-by: Tony Lindgren <tony@atomide.com> > Tested-by: Austin Schuh <austin@peloton-tech.com> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> Patch applied. I'm thinking that we need some recommendations on how to write IRQ handlers in order to be RT-compatible. Can you help me lining up the requirements in Documentation/gpio/driver.txt? I will write an RFC patch and let you write some additional text to it in response then we can iterate it a bit. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/02/2015 03:17 PM, Linus Walleij wrote: > On Fri, Sep 25, 2015 at 12:28 PM, Grygorii Strashko > <grygorii.strashko@ti.com> wrote: > >> This patch converts TI OMAP GPIO driver to use generic irq handler >> instead of chained IRQ handler. This way OMAP GPIO driver will be >> compatible with RT kernel where it will be forced thread IRQ handler >> while in non-RT kernel it still will be executed in HW IRQ context. >> As part of this change the IRQ wakeup configuration is applied to >> GPIO Bank IRQ as it now will be under control of IRQ PM Core during >> suspend. >> >> There are also additional benefits: >> - on-RT kernel there will be no complains any more about PM runtime usage >> in atomic context "BUG: sleeping function called from invalid context"; >> - GPIO bank IRQs will appear in /proc/interrupts and its usage statistic >> will be visible; >> - GPIO bank IRQs could be configured through IRQ proc_fs interface and, >> as result, could be a part of IRQ balancing process if needed; >> - GPIO bank IRQs will be under control of IRQ PM Core during >> suspend to RAM. >> >> Disadvantage: >> - additional runtime overhed as call chain till >> omap_gpio_irq_handler() will be longer now >> - necessity to use wa_lock in omap_gpio_irq_handler() to W/A warning >> in handle_irq_event_percpu() >> WARNING: CPU: 1 PID: 35 at kernel/irq/handle.c:149 handle_irq_event_percpu+0x51c/0x638() >> >> This patch doesn't fully follows recommendations provided by Sebastian >> Andrzej Siewior [1], because It's required to go through and check all >> GPIO IRQ pin states as fast as possible and pass control to handle_level_irq >> or handle_edge_irq. handle_level_irq or handle_edge_irq will perform actions >> specific for IRQ triggering type and wakeup corresponding registered >> threaded IRQ handler (at least it's expected to be threaded). >> IRQs can be lost if handle_nested_irq() will be used, because excecution >> time of some pin specific GPIO IRQ handler can be very significant and >> require accessing ext. devices (I2C). >> >> Idea of such kind reworking was also discussed in [2]. >> >> [1] http://www.spinics.net/lists/linux-omap/msg120665.html >> [2] http://www.spinics.net/lists/linux-omap/msg119516.html >> >> Tested-by: Tony Lindgren <tony@atomide.com> >> Tested-by: Austin Schuh <austin@peloton-tech.com> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > > Patch applied. > Thanks. > I'm thinking that we need some recommendations on how to write > IRQ handlers in order to be RT-compatible. Can you help me lining > up the requirements in Documentation/gpio/driver.txt? > > I will write an RFC patch and let you write some additional text > to it in response then we can iterate it a bit. Sure. I'll try to help.
Grygorii, can you please provide a patch set against 4.1-RT? That stuff rejects left and right. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Thomas, On 10/12/2015 10:52 AM, Thomas Gleixner wrote: > Grygorii, > > can you please provide a patch set against 4.1-RT? That stuff rejects > left and right. > This is really not easy thing to do :( and I don't know how to do it the best way. This patches are based on top of big set of other patches. As result we've back-ported mostly all GPIO patches from upstream kernel to TI's 4.1 kernel to keep things consistent and avoid complicated conflicts during back-porting of fixes. The branch linux-4.1.y-rt is continuously merged in TI's 4.1 rt-kernel. So, on top of linux-4.1.y there is below set of patches in TI's kernel now: *bc6b549 gpio: omap: convert to use generic irq handler *d8b79f8 gpio: omap: move pm runtime in irq_chip.irq_bus_lock/sync_unlock c5d9d80 gpio: omap: fix static checker warning 8e3f97d gpio: omap: Fix GPIO numbering for deferred probe a5fafaa gpio: omap: Fix gpiochip_add() handling for deferred probe *a79afac gpio: omap: fix clk_prepare/unprepare usage *e967fb8 gpio: omap: protect regs access in omap_gpio_irq_handler 7f66a45 gpio: omap: fix omap2_set_gpio_debounce 225b622 gpio: omap: switch to use platform_get_irq 4ad92d9 gpio: omap: remove wrong irq_domain_remove usage in probe f76691a gpio: omap: Fix missing raw locks conversion *97e1a2c gpio: omap: use raw locks for locking 7a74d02 ARM: OMAP2: Drop the concept of certain power domains not being able to lose context. 76281a5 gpio: omap: prevent module from being unloaded while in use 7aa88c9 gpio: omap: add missed spin_unlock_irqrestore in omap_gpio_irq_type 79d3bc2 gpio: omap: rework omap_gpio_irq_startup to handle current pin state properly 81dcc40 gpio: omap: rework omap_gpio_request to touch only gpio specific registers e44665c gpio: omap: rework omap_x_irq_shutdown to touch only irqs specific registers 23c487f gpio: omap: fix error handling in omap_gpio_irq_type b328dfc gpio: omap: fix omap_gpio_free to not clean up irq configuration 2966641 gpio: omap: Allow building as a loadable module I'd very appreciate for any advice of how to better proceed with your request. - I can try to apply and re-send only patches marked by '*' - I can prepare branch with all above patches - smth. else
Grygorii, On Tue, 13 Oct 2015, Grygorii Strashko wrote: > I'd very appreciate for any advice of how to better proceed with your request. > - I can try to apply and re-send only patches marked by '*' > - I can prepare branch with all above patches Please provide a branch on top of 4.1.10 which contains the whole lot. I have a look at it and decide then how to go from there. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Thomas, On 10/13/2015 09:33 PM, Thomas Gleixner wrote: > Grygorii, > > On Tue, 13 Oct 2015, Grygorii Strashko wrote: >> I'd very appreciate for any advice of how to better proceed with your request. >> - I can try to apply and re-send only patches marked by '*' >> - I can prepare branch with all above patches > > Please provide a branch on top of 4.1.10 which contains the whole > lot. I have a look at it and decide then how to go from there. I've prepared branch linux-4.1.10-ti-gpio: in git@git.ti.com:~gragst/ti-linux-kernel/gragsts-ti-linux-kernel.git based on top of git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git branch : linux-4.1.y commit : 27f1b7f Linux 4.1.10 Also, I've tried to merge it in linux-4.1.y-rt and found that merge can be done without merge conflicts if below patch is reverted: " Revert "gpio: omap: use raw locks for locking" This reverts commit 26dc4f70eb09ff7d36b1e6cd720e65b3c69098ae "
* Grygorii Strashko | 2015-10-15 19:33:43 [+0300]: >Hi Thomas, > >On 10/13/2015 09:33 PM, Thomas Gleixner wrote: >>Grygorii, >> >>On Tue, 13 Oct 2015, Grygorii Strashko wrote: >>>I'd very appreciate for any advice of how to better proceed with your request. >>> - I can try to apply and re-send only patches marked by '*' >>> - I can prepare branch with all above patches >> >>Please provide a branch on top of 4.1.10 which contains the whole >>lot. I have a look at it and decide then how to go from there. > >I've prepared branch linux-4.1.10-ti-gpio: >in git@git.ti.com:~gragst/ti-linux-kernel/gragsts-ti-linux-kernel.git could you please provide a git URL for that? >based on top of > git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git > branch : linux-4.1.y > commit : 27f1b7f Linux 4.1.10 Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/11/2015 06:57 PM, Sebastian Andrzej Siewior wrote: > * Grygorii Strashko | 2015-10-15 19:33:43 [+0300]: > >> Hi Thomas, >> >> On 10/13/2015 09:33 PM, Thomas Gleixner wrote: >>> Grygorii, >>> >>> On Tue, 13 Oct 2015, Grygorii Strashko wrote: >>>> I'd very appreciate for any advice of how to better proceed with your request. >>>> - I can try to apply and re-send only patches marked by '*' >>>> - I can prepare branch with all above patches >>> >>> Please provide a branch on top of 4.1.10 which contains the whole >>> lot. I have a look at it and decide then how to go from there. >> >> I've prepared branch linux-4.1.10-ti-gpio: >> in git@git.ti.com:~gragst/ti-linux-kernel/gragsts-ti-linux-kernel.git > > could you please provide a git URL for that? git://git.ti.com/~gragst/ti-linux-kernel/gragsts-ti-linux-kernel.git >> based on top of >> git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git >> branch : linux-4.1.y >> commit : 27f1b7f Linux 4.1.10 >
* Grygorii Strashko | 2015-12-12 10:30:10 [+0200]:
>git://git.ti.com/~gragst/ti-linux-kernel/gragsts-ti-linux-kernel.git
sucked in, thanks.
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index a254691..376827f 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -59,6 +59,7 @@ struct gpio_bank { u32 level_mask; u32 toggle_mask; raw_spinlock_t lock; + raw_spinlock_t wa_lock; struct gpio_chip chip; struct clk *dbck; u32 mod_usage; @@ -649,8 +650,13 @@ static int omap_gpio_wake_enable(struct irq_data *d, unsigned int enable) { struct gpio_bank *bank = omap_irq_data_get_bank(d); unsigned offset = d->hwirq; + int ret; + + ret = omap_set_gpio_wakeup(bank, offset, enable); + if (!ret) + ret = irq_set_irq_wake(bank->irq, enable); - return omap_set_gpio_wakeup(bank, offset, enable); + return ret; } static int omap_gpio_request(struct gpio_chip *chip, unsigned offset) @@ -704,26 +710,21 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset) * line's interrupt handler has been run, we may miss some nested * interrupts. */ -static void omap_gpio_irq_handler(struct irq_desc *desc) +static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank) { void __iomem *isr_reg = NULL; u32 isr; unsigned int bit; - struct gpio_bank *bank; - int unmasked = 0; - struct irq_chip *irqchip = irq_desc_get_chip(desc); - struct gpio_chip *chip = irq_desc_get_handler_data(desc); + struct gpio_bank *bank = gpiobank; + unsigned long wa_lock_flags; unsigned long lock_flags; - chained_irq_enter(irqchip, desc); - - bank = container_of(chip, struct gpio_bank, chip); isr_reg = bank->base + bank->regs->irqstatus; - pm_runtime_get_sync(bank->dev); - if (WARN_ON(!isr_reg)) goto exit; + pm_runtime_get_sync(bank->dev); + while (1) { u32 isr_saved, level_mask = 0; u32 enabled; @@ -745,13 +746,6 @@ static void omap_gpio_irq_handler(struct irq_desc *desc) raw_spin_unlock_irqrestore(&bank->lock, lock_flags); - /* if there is only edge sensitive GPIO pin interrupts - configured, we could unmask GPIO bank interrupt immediately */ - if (!level_mask && !unmasked) { - unmasked = 1; - chained_irq_exit(irqchip, desc); - } - if (!isr) break; @@ -772,18 +766,18 @@ static void omap_gpio_irq_handler(struct irq_desc *desc) raw_spin_unlock_irqrestore(&bank->lock, lock_flags); + raw_spin_lock_irqsave(&bank->wa_lock, wa_lock_flags); + generic_handle_irq(irq_find_mapping(bank->chip.irqdomain, bit)); + + raw_spin_unlock_irqrestore(&bank->wa_lock, + wa_lock_flags); } } - /* if bank has any level sensitive GPIO pin interrupt - configured, we must unmask the bank interrupt only after - handler(s) are executed in order to avoid spurious bank - interrupt */ exit: - if (!unmasked) - chained_irq_exit(irqchip, desc); pm_runtime_put(bank->dev); + return IRQ_HANDLED; } static unsigned int omap_gpio_irq_startup(struct irq_data *d) @@ -1135,7 +1129,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc) } ret = gpiochip_irqchip_add(&bank->chip, irqc, - irq_base, omap_gpio_irq_handler, + irq_base, handle_bad_irq, IRQ_TYPE_NONE); if (ret) { @@ -1144,10 +1138,14 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc) return -ENODEV; } - gpiochip_set_chained_irqchip(&bank->chip, irqc, - bank->irq, omap_gpio_irq_handler); + gpiochip_set_chained_irqchip(&bank->chip, irqc, bank->irq, NULL); - return 0; + ret = devm_request_irq(bank->dev, bank->irq, omap_gpio_irq_handler, + 0, dev_name(bank->dev), bank); + if (ret) + gpiochip_remove(&bank->chip); + + return ret; } static const struct of_device_id omap_gpio_match[]; @@ -1229,6 +1227,7 @@ static int omap_gpio_probe(struct platform_device *pdev) bank->set_dataout = omap_set_gpio_dataout_mask; raw_spin_lock_init(&bank->lock); + raw_spin_lock_init(&bank->wa_lock); /* Static mapping, never released */ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);