| Message ID | 20240214-mbly-gpio-v1-16-f88c0ccf372b@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rework Nomadik GPIO to add Mobileye EyeQ5 support | expand |
On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > Support a single IRQs used by multiple GPIO banks. Change the IRQ > handler type from a chained handler (as used by gpiolib > for ->parent_handler) to a threaded IRQ. > > Use a fake raw spinlock to ensure generic_handle_irq() is called in a > no-irq context. See Documentation/driver-api/gpio/driver.rst, "CHAINED > CASCADED GPIO IRQCHIPS" for additional information. > Any reason for not using preempt_disable()? Bart [snip]
Hello, On Mon Feb 19, 2024 at 4:48 PM CET, Bartosz Golaszewski wrote: > On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > > Support a single IRQs used by multiple GPIO banks. Change the IRQ > > handler type from a chained handler (as used by gpiolib > > for ->parent_handler) to a threaded IRQ. > > > > Use a fake raw spinlock to ensure generic_handle_irq() is called in a > > no-irq context. See Documentation/driver-api/gpio/driver.rst, "CHAINED > > CASCADED GPIO IRQCHIPS" for additional information. > > > > Any reason for not using preempt_disable()? I did what the doc recommended: > The generic_handle_irq() is expected to be called with IRQ disabled, > so the IRQ core will complain if it is called from an IRQ handler which is > forced to a thread. The "fake?" raw lock can be used to work around this > problem:: > > raw_spinlock_t wa_lock; > static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank) > unsigned long wa_lock_flags; > raw_spin_lock_irqsave(&bank->wa_lock, wa_lock_flags); > generic_handle_irq(irq_find_mapping(bank->chip.irq.domain, bit)); > raw_spin_unlock_irqrestore(&bank->wa_lock, wa_lock_flags); If you confirm I should be using preempt_disable() that's what I'll do in the next revision. I could even throw in a documentation patch if the advice is outdated. Thanks Bartosz, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Mon, Feb 19, 2024 at 4:54 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > Hello, > > On Mon Feb 19, 2024 at 4:48 PM CET, Bartosz Golaszewski wrote: > > On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > > > > Support a single IRQs used by multiple GPIO banks. Change the IRQ > > > handler type from a chained handler (as used by gpiolib > > > for ->parent_handler) to a threaded IRQ. > > > > > > Use a fake raw spinlock to ensure generic_handle_irq() is called in a > > > no-irq context. See Documentation/driver-api/gpio/driver.rst, "CHAINED > > > CASCADED GPIO IRQCHIPS" for additional information. > > > > > > > Any reason for not using preempt_disable()? > > I did what the doc recommended: > > > The generic_handle_irq() is expected to be called with IRQ disabled, > > so the IRQ core will complain if it is called from an IRQ handler which is > > forced to a thread. The "fake?" raw lock can be used to work around this > > problem:: > > > > raw_spinlock_t wa_lock; > > static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank) > > unsigned long wa_lock_flags; > > raw_spin_lock_irqsave(&bank->wa_lock, wa_lock_flags); > > generic_handle_irq(irq_find_mapping(bank->chip.irq.domain, bit)); > > raw_spin_unlock_irqrestore(&bank->wa_lock, wa_lock_flags); > > If you confirm I should be using preempt_disable() that's what I'll do > in the next revision. I could even throw in a documentation patch if > the advice is outdated. > > Thanks Bartosz, This was added 9 years ago: commit c307b002548590c5d8c32b964831de671ad4affe Author: Grygorii Strashko <grygorii.strashko@ti.com> Date: Tue Oct 20 17:22:15 2015 +0300 gpio: add a real time compliance notes Put in a compliance checklist. Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> I'm Cc'ing Grygorii - maybe he can remember if there was any reason for using a spinlock over preempt_disable(). But for now I'd go with the latter. Bart > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Mon, Feb 19, 2024 at 4:48 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Wed, Feb 14, 2024 at 5:24 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > > Support a single IRQs used by multiple GPIO banks. Change the IRQ > > handler type from a chained handler (as used by gpiolib > > for ->parent_handler) to a threaded IRQ. > > > > Use a fake raw spinlock to ensure generic_handle_irq() is called in a > > no-irq context. See Documentation/driver-api/gpio/driver.rst, "CHAINED > > CASCADED GPIO IRQCHIPS" for additional information. > > > > Any reason for not using preempt_disable()? I think this needs to be discussed with tglx if Grygorii is not available. Yours, Linus Walleij
diff --git a/drivers/gpio/gpio-nomadik.c b/drivers/gpio/gpio-nomadik.c index 25c8490aa1b6..5b1e3b3efcff 100644 --- a/drivers/gpio/gpio-nomadik.c +++ b/drivers/gpio/gpio-nomadik.c @@ -254,27 +254,31 @@ static void nmk_gpio_irq_shutdown(struct irq_data *d) clk_disable(nmk_chip->clk); } -static void nmk_gpio_irq_handler(struct irq_desc *desc) +static irqreturn_t nmk_gpio_irq_handler(int irq, void *dev_id) { - struct irq_chip *host_chip = irq_desc_get_chip(desc); - struct gpio_chip *chip = irq_desc_get_handler_data(desc); - struct nmk_gpio_chip *nmk_chip = gpiochip_get_data(chip); - u32 status; - - chained_irq_enter(host_chip, desc); + struct nmk_gpio_chip *nmk_chip = dev_id; + struct gpio_chip *chip = &nmk_chip->chip; + unsigned long mask = GENMASK(chip->ngpio - 1, 0); + unsigned long flags, status; + int bit; clk_enable(nmk_chip->clk); + status = readl(nmk_chip->addr + NMK_GPIO_IS); - clk_disable(nmk_chip->clk); - while (status) { - int bit = __ffs(status); + /* Ensure we cannot leave pending bits; this should never occur. */ + if (unlikely(status & ~mask)) + writel(status & ~mask, nmk_chip->addr + NMK_GPIO_IC); + + clk_disable(nmk_chip->clk); + for_each_set_bit(bit, &status, chip->ngpio) { + raw_spin_lock_irqsave(&nmk_chip->fake_lock, flags); generic_handle_domain_irq(chip->irq.domain, bit); - status &= ~BIT(bit); + raw_spin_unlock_irqrestore(&nmk_chip->fake_lock, flags); } - chained_irq_exit(host_chip, desc); + return IRQ_RETVAL((status & mask) != 0); } /* I/O Functions */ @@ -566,19 +570,20 @@ static const struct irq_chip nmk_irq_chip = { GPIOCHIP_IRQ_RESOURCE_HELPERS, }; -static int nmk_gpio_probe(struct platform_device *dev) +static int nmk_gpio_probe(struct platform_device *pdev) { - struct device_node *np = dev->dev.of_node; + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; struct nmk_gpio_chip *nmk_chip; - struct gpio_chip *chip; struct gpio_irq_chip *girq; bool supports_sleepmode; + struct gpio_chip *chip; int irq; int ret; - nmk_chip = nmk_gpio_populate_chip(np, dev); + nmk_chip = nmk_gpio_populate_chip(np, pdev); if (IS_ERR(nmk_chip)) { - dev_err(&dev->dev, "could not populate nmk chip struct\n"); + dev_err(dev, "could not populate nmk chip struct\n"); return PTR_ERR(nmk_chip); } @@ -586,9 +591,9 @@ static int nmk_gpio_probe(struct platform_device *dev) of_property_read_bool(np, "st,supports-sleepmode"); /* Correct platform device ID */ - dev->id = nmk_chip->bank; + pdev->id = nmk_chip->bank; - irq = platform_get_irq(dev, 0); + irq = platform_get_irq(pdev, 0); if (irq < 0) return irq; @@ -600,7 +605,7 @@ static int nmk_gpio_probe(struct platform_device *dev) spin_lock_init(&nmk_chip->lock); chip = &nmk_chip->chip; - chip->parent = &dev->dev; + chip->parent = dev; chip->request = gpiochip_generic_request; chip->free = gpiochip_generic_free; chip->get_direction = nmk_gpio_get_dir; @@ -614,17 +619,19 @@ static int nmk_gpio_probe(struct platform_device *dev) girq = &chip->irq; gpio_irq_chip_set_chip(girq, &nmk_irq_chip); - girq->parent_handler = nmk_gpio_irq_handler; - girq->num_parents = 1; - girq->parents = devm_kcalloc(&dev->dev, 1, - sizeof(*girq->parents), - GFP_KERNEL); - if (!girq->parents) - return -ENOMEM; - girq->parents[0] = irq; + girq->parent_handler = NULL; + girq->num_parents = 0; + girq->parents = NULL; girq->default_type = IRQ_TYPE_NONE; girq->handler = handle_edge_irq; + ret = devm_request_irq(dev, irq, nmk_gpio_irq_handler, IRQF_SHARED, + dev_name(dev), nmk_chip); + if (ret) { + dev_err(dev, "failed requesting IRQ\n"); + return ret; + } + clk_enable(nmk_chip->clk); nmk_chip->lowemi = readl_relaxed(nmk_chip->addr + NMK_GPIO_LOWEMI); clk_disable(nmk_chip->clk); @@ -633,9 +640,9 @@ static int nmk_gpio_probe(struct platform_device *dev) if (ret) return ret; - platform_set_drvdata(dev, nmk_chip); + platform_set_drvdata(pdev, nmk_chip); - dev_info(&dev->dev, "chip registered\n"); + dev_info(dev, "chip registered\n"); return 0; } diff --git a/include/linux/gpio/gpio-nomadik.h b/include/linux/gpio/gpio-nomadik.h index 0166ddb71f43..8f7bb756ad35 100644 --- a/include/linux/gpio/gpio-nomadik.h +++ b/include/linux/gpio/gpio-nomadik.h @@ -56,6 +56,7 @@ struct nmk_gpio_chip { unsigned int bank; void (*set_ioforce)(bool enable); spinlock_t lock; + raw_spinlock_t fake_lock; bool sleepmode; /* Keep track of configured edges */ u32 edge_rising;
Support a single IRQs used by multiple GPIO banks. Change the IRQ handler type from a chained handler (as used by gpiolib for ->parent_handler) to a threaded IRQ. Use a fake raw spinlock to ensure generic_handle_irq() is called in a no-irq context. See Documentation/driver-api/gpio/driver.rst, "CHAINED CASCADED GPIO IRQCHIPS" for additional information. The Mobileye EyeQ5 platform uses this GPIO controller and share an IRQ for its two banks. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- drivers/gpio/gpio-nomadik.c | 67 +++++++++++++++++++++------------------ include/linux/gpio/gpio-nomadik.h | 1 + 2 files changed, 38 insertions(+), 30 deletions(-)