| Message ID | 20210920212227.19358-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 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: > + fall = true; > + rise = true; > + break; > + case IRQ_TYPE_EDGE_RISING: > + rise = true; > + break; > + case IRQ_TYPE_EDGE_FALLING: > + fall = true; > + break; > + default: > + return -EINVAL; > + } What PHY are you using? I think every one i've looked at are level triggered, not edge. Using an edge interrupt might work 99% of the time, but when the timing is just wrong, you can loose an interrupt. Which might mean phylib thinks the link is down, when it fact it is up. You will need to unplug and replug to recover from that. Andrew
> +static int > +mlxbf2_gpio_irq_set_type(struct irq_data *irqd, unsigned int type) { > + > + switch (type & IRQ_TYPE_SENSE_MASK) { > + case IRQ_TYPE_EDGE_BOTH: > + fall = true; > + rise = true; > + break; > + case IRQ_TYPE_EDGE_RISING: > + rise = true; > + break; > + case IRQ_TYPE_EDGE_FALLING: > + fall = true; > + break; > + default: > + return -EINVAL; > + } > What PHY are you using? I think every one i've looked at are > level triggered, not edge. Using an edge interrupt might work 99% > of the time, but when the timing is just wrong, you can loose an interrupt. > Which might mean phylib thinks the link is down, when it fact it is up. > You will need to unplug and replug to recover from that. It is the micrel PHY KSZ9031 so it is an active low level interrupt. Here, IRQ_TYPE_EDGE* macros are mainly used to decide whether to write the YU_GPIO_CAUSE_FALL_EN register vs the YU_GPIO_CAUSE_RISE_EN register. These 2 registers are used in both LEVEL/EDGE interrupts. So I will add back the IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_LEVEL_HIGH as I did before to configure YU_GPIO_CAUSE_FALL_EN and YU_GPIO_CAUSE_HIGH_EN respectively. The PHY interrupt signal is physically Connected to an open drain GPIO pin so software only needs to set YU_GPIO_CAUSE_FALL_EN register in this case. Thanks. Asmaa
On Wed, Sep 22, 2021 at 02:16:40PM +0000, Asmaa Mnebhi wrote: > > > +static int > > +mlxbf2_gpio_irq_set_type(struct irq_data *irqd, unsigned int type) { > > + > > + switch (type & IRQ_TYPE_SENSE_MASK) { > > + case IRQ_TYPE_EDGE_BOTH: > > + fall = true; > > + rise = true; > > + break; > > + case IRQ_TYPE_EDGE_RISING: > > + rise = true; > > + break; > > + case IRQ_TYPE_EDGE_FALLING: > > + fall = true; > > + break; > > + default: > > + return -EINVAL; > > + } > > > What PHY are you using? I think every one i've looked at are > > level triggered, not edge. Using an edge interrupt might work 99% > > of the time, but when the timing is just wrong, you can loose an interrupt. > > Which might mean phylib thinks the link is down, when it fact it is up. > > You will need to unplug and replug to recover from that. > > It is the micrel PHY KSZ9031 so it is an active low level interrupt. > Here, IRQ_TYPE_EDGE* macros are mainly used to decide whether to write the > YU_GPIO_CAUSE_FALL_EN register vs the YU_GPIO_CAUSE_RISE_EN register. > These 2 registers are used in both LEVEL/EDGE interrupts. I assume you also have an YU_GPIO_CAUSE_LOW_EN and YU_GPIO_CAUSE_HIGH_EN registers? These registers need to be set for IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_LEVEL_HIGH. Andrew
> > +static int > > +mlxbf2_gpio_irq_set_type(struct irq_data *irqd, unsigned int type) > > +{ > > + > > + switch (type & IRQ_TYPE_SENSE_MASK) { > > + case IRQ_TYPE_EDGE_BOTH: > > + fall = true; > > + rise = true; > > + break; > > + case IRQ_TYPE_EDGE_RISING: > > + rise = true; > > + break; > > + case IRQ_TYPE_EDGE_FALLING: > > + fall = true; > > + break; > > + default: > > + return -EINVAL; > > + } > > > What PHY are you using? I think every one i've looked at are level > > triggered, not edge. Using an edge interrupt might work 99% of the > > time, but when the timing is just wrong, you can loose an interrupt. > > Which might mean phylib thinks the link is down, when it fact it is up. > > You will need to unplug and replug to recover from that. > > It is the micrel PHY KSZ9031 so it is an active low level interrupt. > Here, IRQ_TYPE_EDGE* macros are mainly used to decide whether to write > the YU_GPIO_CAUSE_FALL_EN register vs the YU_GPIO_CAUSE_RISE_EN register. > These 2 registers are used in both LEVEL/EDGE interrupts. > I assume you also have an YU_GPIO_CAUSE_LOW_EN and > YU_GPIO_CAUSE_HIGH_EN registers? These registers need to > be set for IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_LEVEL_HIGH. No we don't. I double checked with the HW team and they confirmed that YU_GPIO_CAUSE_FALL_EN and YU_GPIO_CAUSE_RISE_EN are used in Both level and edge interrupts cases. Thanks. Asnaa
> No we don't. I double checked with the HW team and they confirmed that > YU_GPIO_CAUSE_FALL_EN and YU_GPIO_CAUSE_RISE_EN are used in > Both level and edge interrupts cases. How? They are different things. I suggest you test this. Make sure a level interrupt real does fire on level. One simple test is use a resistor to force the interrupt pin low. Your machine should then die in an interrupt storm, until the kernel declares the interrupt broken and disables it. Andrew
diff --git a/drivers/gpio/gpio-mlxbf2.c b/drivers/gpio/gpio-mlxbf2.c index 177d03ef4529..3d89912a05b8 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,114 @@ 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); + + 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 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 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: + fall = true; + rise = true; + break; + case IRQ_TYPE_EDGE_RISING: + rise = true; + break; + case IRQ_TYPE_EDGE_FALLING: + fall = true; + break; + default: + return -EINVAL; + } + + 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 +366,44 @@ 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_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 +458,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 | 147 ++++++++++++++++++++++++++++++++++++- 1 file changed, 145 insertions(+), 2 deletions(-)