| Message ID | 20210915222847.10239-2-asmaa@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | gpio: mlxbf2: Introduce proper interrupt handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
> +static void mlxbf2_gpio_irq_enable(struct irq_data *irqd) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd); > + struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc); > + int offset = irqd_to_hwirq(irqd); > + unsigned long flags; > + u32 val; > + > + spin_lock_irqsave(&gs->gc.bgpio_lock, flags); > + val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE); > + val |= BIT(offset); > + writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE); > + > + /* Enable PHY interrupt by setting the priority level */ This should be an abstract driver for a collection of GPIO lines. Yes, one of these GPIOs is used for the PHY, but the GPIO driver does not care. So please remove this comment. > + val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0); > + val |= BIT(offset); > + writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0); What exactly does this do? It appears to clear the interrupt, if i understand mlxbf2_gpio_irq_handler(). I don't know the GPIO framework well enough to know if this is correct. It does mean if the interrupt signal is active but masked, and you enable it, you appear to loose the interrupt? Maybe you want the interrupt to fire as soon as it is enabled? > +static irqreturn_t mlxbf2_gpio_irq_handler(int irq, void *ptr) > +{ > + struct mlxbf2_gpio_context *gs = ptr; > + struct gpio_chip *gc = &gs->gc; > + unsigned long pending; > + u32 level; > + > + pending = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_CAUSE_EVTEN0); > + writel(pending, gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE); > + > + for_each_set_bit(level, &pending, gc->ngpio) { > + int gpio_irq = irq_find_mapping(gc->irq.domain, level); > + generic_handle_irq(gpio_irq); > + } > + > + return IRQ_RETVAL(pending); > +} > + > +static void mlxbf2_gpio_irq_mask(struct irq_data *irqd) { > + mlxbf2_gpio_irq_disable(irqd); > +} > + > +static void mlxbf2_gpio_irq_unmask(struct irq_data *irqd) { > + mlxbf2_gpio_irq_enable(irqd); > +} Do these two functions have any value? > +static int > +mlxbf2_gpio_irq_set_type(struct irq_data *irqd, unsigned int type) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd); > + struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc); > + int offset = irqd_to_hwirq(irqd); > + unsigned long flags; > + bool fall = false; > + bool rise = false; > + u32 val; > + > + switch (type & IRQ_TYPE_SENSE_MASK) { > + case IRQ_TYPE_EDGE_BOTH: > + case IRQ_TYPE_LEVEL_MASK: > + fall = true; > + rise = true; > + break; > + case IRQ_TYPE_EDGE_RISING: > + case IRQ_TYPE_LEVEL_HIGH: > + rise = true; > + break; > + case IRQ_TYPE_EDGE_FALLING: > + case IRQ_TYPE_LEVEL_LOW: > + fall = true; > + break; This looks wrong. You cannot map a level interrupt into an edge. It looks like your hardware only supports edges. If asked to do level, return -EINVAL. > + default: > + break; > + } > + > + /* The INT_N interrupt level is active low. > + * So enable cause fall bit to detect when GPIO > + * state goes low. > + */ I don't understand this comment. Andrew
> + /* Enable PHY interrupt by setting the priority level */ This should be an abstract driver for a collection of GPIO lines. Yes, one of these GPIOs is used for the PHY, but the GPIO driver does not care. So please remove this comment. Asmaa>> Done > + val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0); > + val |= BIT(offset); > + writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0); What exactly does this do? It appears to clear the interrupt, if i understand mlxbf2_gpio_irq_handler(). I don't know the GPIO framework well enough to know if this is correct. It does mean if the interrupt signal is active but masked, and you enable it, you appear to loose the interrupt? Maybe you want the interrupt to fire as soon as it is enabled? Asmaa>> YU_GPIO_CAUSE_OR_CLRCAUSE - Makes sure the interrupt is initially cleared. Otherwise, we will not receive further interrupts. YU_GPIO_CAUSE_OR_EVTEN0 - All interrupts are disabled by default. This register is what actually unmasks/enables the specific interrupt to start "firing". > +static void mlxbf2_gpio_irq_mask(struct irq_data *irqd) { > + mlxbf2_gpio_irq_disable(irqd); > +} > + > +static void mlxbf2_gpio_irq_unmask(struct irq_data *irqd) { > + mlxbf2_gpio_irq_enable(irqd); > +} Do these two functions have any value? Asmaa>> This code is actually not being called. enable/disable is what's being called. So I will remove it. > + switch (type & IRQ_TYPE_SENSE_MASK) { > + case IRQ_TYPE_EDGE_BOTH: > + case IRQ_TYPE_LEVEL_MASK: > + fall = true; > + rise = true; > + break; > + case IRQ_TYPE_EDGE_RISING: > + case IRQ_TYPE_LEVEL_HIGH: > + rise = true; > + break; > + case IRQ_TYPE_EDGE_FALLING: > + case IRQ_TYPE_LEVEL_LOW: > + fall = true; > + break; This looks wrong. You cannot map a level interrupt into an edge. It looks like your hardware only supports edges. If asked to do level, return -EINVAL. Asmaa>> done > + > + /* The INT_N interrupt level is active low. > + * So enable cause fall bit to detect when GPIO > + * state goes low. > + */ I don't understand this comment. Asmaa>> removed.
On Thu, Sep 16, 2021 at 03:48:51PM +0000, Asmaa Mnebhi wrote: > > + /* Enable PHY interrupt by setting the priority level */ > > This should be an abstract driver for a collection of GPIO lines. > Yes, one of these GPIOs is used for the PHY, but the GPIO driver does not care. So please remove this comment. > Asmaa>> Done > > > + val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0); > > + val |= BIT(offset); > > + writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0); > > What exactly does this do? It appears to clear the interrupt, if i understand mlxbf2_gpio_irq_handler(). I don't know the GPIO framework well enough to know if this is correct. It does mean if the interrupt signal is active but masked, and you enable it, you appear to loose the interrupt? Maybe you want the interrupt to fire as soon as it is enabled? > > Asmaa>> > YU_GPIO_CAUSE_OR_CLRCAUSE - Makes sure the interrupt is initially cleared. Otherwise, we will not receive further interrupts. If the interrupt status bit is set, as soon as you unmask the interrupt, the hardware should fire the interrupt. At least, that is how interrupt controllers usually work. A typical pattern is that the interrupt fires. You mask it, ack it, and then do what is needed to actually handle the interrupt. While doing the handling, the hardware can indicate the interrupt again. But since it is masked nothing happened. This avoids your interrupt handler going recursive. Once the handler has finished, the interrupt is unmasked. At this point it actually fires, triggering the interrupt handler again. Please also get your email client fixed. I wrap my emails at around 75 characters. Your mailer has destroyed it. Your text should also be wrapped at about 75 characters. Andrew
> > + val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0); > > + val |= BIT(offset); > > + writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0); > > What exactly does this do? It appears to clear the interrupt, if i understand > mlxbf2_gpio_irq_handler(). I don't know the GPIO framework well enough to know if this is > correct. It does mean if the interrupt signal is active but masked, and you enable it, you appear > to loose the interrupt? Maybe you want the interrupt to fire as soon as it is enabled? > > Asmaa>> > YU_GPIO_CAUSE_OR_CLRCAUSE - Makes sure the interrupt is initially cleared. Otherwise, we > will not receive further interrupts. > If the interrupt status bit is set, as soon as you unmask the interrupt, the hardware should fire > the interrupt. At least, that is how interrupt controllers usually work. > A typical pattern is that the interrupt fires. You mask it, ack it, and then do what is needed to > actually handle the interrupt. While doing the handling, the hardware can indicate the interrupt > again. But since it is masked nothing happened. This avoids your interrupt handler going > recursive. > Once the handler has finished, the interrupt is unmasked. At this point it actually fires, triggering > the interrupt handler again. Asmaa>> mlxbf2_gpio_irq_enable seems to be called only once when the driver is loaded. And I will actually remove mlxbf2_gpio_irq_ack because it is not being called at all. After further investigation, that function is called via chained_irq_enter which is itself invoked in the interrupt handler. It should have looked something like this: static irqreturn_t mlxbf2_gpio_irq_handler(int irq, void *ptr) { chained_irq_enter(gc->irq->chip, desc); // rest of the code here chained_irq_exit(gc->irq->chip, desc); } But in our case, we decided to directly request the irq instead of passing a flow-handler to gpiochip_set_chained_irqchip, because the irq has to be marked as shared (IRQF_SHARED). gpio-mt7621.c does something similar. Moreover, whenever an interrupt is fired by HW, it is automatically disabled/masked until it is explicitly cleared by Software. And this line takes care of it in mlxbf2_gpio_irq_handler: writel(pending, gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE); After a HW reset, all gpio interrupts are disabled by default by HW. The HW will not signal any gpio interrupt as long as all bits in YU_GPIO_CAUSE_OR_EVTEN0 are 0. In mlxbf2_gpio_irq_enable, we configure a specific gpio as an interrupt by writing 1 to YU_GPIO_CAUSE_OR_EVTEN0. I just wanted to make sure there is no trash value in YU_GPIO_CAUSE_OR_CLRCAUSE before enabling gpio interrupt support. So pending interrupts in YU_GPIO_CAUSE_OR_CLRCAUSE only matters if YU_GPIO_CAUSE_OR_EVTEN0 is set accordingly. Does this answer your question? > Please also get your email client fixed. I wrap my emails at around 75 characters. Your mailer > has destroyed it. Your text should also be wrapped at about 75 characters. Asmaa>> Sorry about that. I wrapped my outlook emails around 75 characters, I hope it works.
diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c index 177d03ef4529..f2850957ed77 100644 --- a/drivers/gpio/gpio-mlxbf2.c +++ b/drivers/gpio/gpio-mlxbf2.c @@ -1,9 +1,14 @@ // SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2020-2021 NVIDIA CORPORATION & AFFILIATES + */ + #include <linux/bitfield.h> #include <linux/bitops.h> #include <linux/device.h> #include <linux/gpio/driver.h> +#include <linux/interrupt.h> #include <linux/io.h> #include <linux/ioport.h> #include <linux/kernel.h> @@ -43,9 +48,14 @@ #define YU_GPIO_MODE0 0x0c #define YU_GPIO_DATASET 0x14 #define YU_GPIO_DATACLEAR 0x18 +#define YU_GPIO_CAUSE_RISE_EN 0x44 +#define YU_GPIO_CAUSE_FALL_EN 0x48 #define YU_GPIO_MODE1_CLEAR 0x50 #define YU_GPIO_MODE0_SET 0x54 #define YU_GPIO_MODE0_CLEAR 0x58 +#define YU_GPIO_CAUSE_OR_CAUSE_EVTEN0 0x80 +#define YU_GPIO_CAUSE_OR_EVTEN0 0x94 +#define YU_GPIO_CAUSE_OR_CLRCAUSE 0x98 struct mlxbf2_gpio_context_save_regs { u32 gpio_mode0; @@ -55,6 +65,7 @@ struct mlxbf2_gpio_context_save_regs { /* BlueField-2 gpio block context structure. */ struct mlxbf2_gpio_context { struct gpio_chip gc; + struct irq_chip irq_chip; /* YU GPIO blocks address */ void __iomem *gpio_io; @@ -218,15 +229,145 @@ static int mlxbf2_gpio_direction_output(struct gpio_chip *chip, return ret; } +static void mlxbf2_gpio_irq_enable(struct irq_data *irqd) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd); + struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc); + int offset = irqd_to_hwirq(irqd); + unsigned long flags; + u32 val; + + spin_lock_irqsave(&gs->gc.bgpio_lock, flags); + val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE); + val |= BIT(offset); + writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE); + + /* Enable PHY interrupt by setting the priority level */ + val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0); + val |= BIT(offset); + writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0); + spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags); +} + +static void mlxbf2_gpio_irq_disable(struct irq_data *irqd) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd); + struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc); + int offset = irqd_to_hwirq(irqd); + unsigned long flags; + u32 val; + + spin_lock_irqsave(&gs->gc.bgpio_lock, flags); + val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0); + val &= ~BIT(offset); + writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0); + spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags); +} + +static void mlxbf2_gpio_irq_ack(struct irq_data *irqd) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd); + struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc); + int offset = irqd_to_hwirq(irqd); + unsigned long flags; + u32 val; + + spin_lock_irqsave(&gs->gc.bgpio_lock, flags); + val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE); + val |= BIT(offset); + writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE); + spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags); +} + +static irqreturn_t mlxbf2_gpio_irq_handler(int irq, void *ptr) +{ + struct mlxbf2_gpio_context *gs = ptr; + struct gpio_chip *gc = &gs->gc; + unsigned long pending; + u32 level; + + pending = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_CAUSE_EVTEN0); + writel(pending, gs->gpio_io + YU_GPIO_CAUSE_OR_CLRCAUSE); + + for_each_set_bit(level, &pending, gc->ngpio) { + int gpio_irq = irq_find_mapping(gc->irq.domain, level); + generic_handle_irq(gpio_irq); + } + + return IRQ_RETVAL(pending); +} + +static void mlxbf2_gpio_irq_mask(struct irq_data *irqd) { + mlxbf2_gpio_irq_disable(irqd); +} + +static void mlxbf2_gpio_irq_unmask(struct irq_data *irqd) { + mlxbf2_gpio_irq_enable(irqd); +} + +static int +mlxbf2_gpio_irq_set_type(struct irq_data *irqd, unsigned int type) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd); + struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc); + int offset = irqd_to_hwirq(irqd); + unsigned long flags; + bool fall = false; + bool rise = false; + u32 val; + + switch (type & IRQ_TYPE_SENSE_MASK) { + case IRQ_TYPE_EDGE_BOTH: + case IRQ_TYPE_LEVEL_MASK: + fall = true; + rise = true; + break; + case IRQ_TYPE_EDGE_RISING: + case IRQ_TYPE_LEVEL_HIGH: + rise = true; + break; + case IRQ_TYPE_EDGE_FALLING: + case IRQ_TYPE_LEVEL_LOW: + fall = true; + break; + default: + break; + } + + /* The INT_N interrupt level is active low. + * So enable cause fall bit to detect when GPIO + * state goes low. + */ + spin_lock_irqsave(&gs->gc.bgpio_lock, flags); + if (fall) { + val = readl(gs->gpio_io + YU_GPIO_CAUSE_FALL_EN); + val |= BIT(offset); + writel(val, gs->gpio_io + YU_GPIO_CAUSE_FALL_EN); + } + + if (rise) { + val = readl(gs->gpio_io + YU_GPIO_CAUSE_RISE_EN); + val |= BIT(offset); + writel(val, gs->gpio_io + YU_GPIO_CAUSE_RISE_EN); + } + spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags); + + return 0; +} + /* BlueField-2 GPIO driver initialization routine. */ static int mlxbf2_gpio_probe(struct platform_device *pdev) { struct mlxbf2_gpio_context *gs; struct device *dev = &pdev->dev; + struct gpio_irq_chip *girq; struct gpio_chip *gc; unsigned int npins; - int ret; + const char *name; + int ret, irq; + + name = dev_name(dev); gs = devm_kzalloc(dev, sizeof(*gs), GFP_KERNEL); if (!gs) @@ -256,11 +397,47 @@ mlxbf2_gpio_probe(struct platform_device *pdev) NULL, 0); + if (ret) { + dev_err(dev, "bgpio_init failed\n"); + return ret; + } + gc->direction_input = mlxbf2_gpio_direction_input; gc->direction_output = mlxbf2_gpio_direction_output; gc->ngpio = npins; gc->owner = THIS_MODULE; + irq = platform_get_irq(pdev, 0); + if (irq >= 0) { + gs->irq_chip.name = name; + gs->irq_chip.irq_ack = mlxbf2_gpio_irq_ack; + gs->irq_chip.irq_mask = mlxbf2_gpio_irq_mask; + gs->irq_chip.irq_unmask = mlxbf2_gpio_irq_unmask; + gs->irq_chip.irq_set_type = mlxbf2_gpio_irq_set_type; + gs->irq_chip.irq_enable = mlxbf2_gpio_irq_enable; + gs->irq_chip.irq_disable = mlxbf2_gpio_irq_disable; + + girq = &gs->gc.irq; + girq->chip = &gs->irq_chip; + girq->handler = handle_simple_irq; + girq->default_type = IRQ_TYPE_NONE; + /* This will let us handle the parent IRQ in the driver */ + girq->num_parents = 0; + girq->parents = NULL; + girq->parent_handler = NULL; + + /* + * Directly request the irq here instead of passing + * a flow-handler because the irq is shared. + */ + ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler, + IRQF_SHARED, name, gs); + if (ret) { + dev_err(dev, "failed to request IRQ"); + return ret; + } + } + platform_set_drvdata(pdev, gs); ret = devm_gpiochip_add_data(dev, &gs->gc, gs); @@ -315,5 +492,5 @@ static struct platform_driver mlxbf2_gpio_driver = { module_platform_driver(mlxbf2_gpio_driver); MODULE_DESCRIPTION("Mellanox BlueField-2 GPIO Driver"); -MODULE_AUTHOR("Mellanox Technologies"); +MODULE_AUTHOR("Asmaa Mnebhi <asmaa@nvidia.com>"); MODULE_LICENSE("GPL v2");
Introduce standard IRQ handling in the gpio-mlxbf2.c driver. Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com> --- drivers/gpio/gpio-mlxbf2.c | 181 ++++++++++++++++++++++++++++++++++++- 1 file changed, 179 insertions(+), 2 deletions(-)