[16/23] gpio: nomadik: support shared GPIO IRQs
Commit Message
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(-)
Comments
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
@@ -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;
}
@@ -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;