| Message ID | 0e60fccd7913b83ee53d2921ce8f297927e8b6f3.1490120798.git-series.gregory.clement@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 21, 2017 at 7:28 PM, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > The Armada 37xx SoCs can handle interrupt through GPIO. However it can > only manage the edge ones. > > The way the interrupt are managed are classical so we can use the generic > interrupt chip model. > > The only unusual "feature" is that many interrupts are connected to the > parent interrupt controller. But we do not take advantage of this and use > the chained irq with all of them. > > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> You need something in your Kconfig doing select GPIOLIB_IRQCHIP unless there is something I miss here. > +#define IRQ_EN 0x0 > +#define IRQ_POL 0x08 > +#define IRQ_STATUS 0x10 This just cries out to me that there is a register 0x0c and I bet it handles edges vs levels so you could also implement level IRQs. Am I right? > - aramda_37xx_update_reg(®, offset); > + armada_37xx_update_reg(®, offset); (...) > - aramda_37xx_update_reg(®, offset); > + armada_37xx_update_reg(®, offset); These spelling fixes, do not do them in this patch, fix the first patch adding them instead. It's super-confusing. Applies everywhere. > +static void armada_37xx_irq_handler(struct irq_desc *desc) > +{ > + struct gpio_chip *gc = irq_desc_get_handler_data(desc); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + struct armada_37xx_pinctrl *info = gpiochip_get_data(gc); > + struct irq_domain *d = gc->irqdomain; > + int i; > + > + chained_irq_enter(chip, desc); > + for (i = 0; i <= d->revmap_size / GPIO_PER_REG; i++) { > + u32 status; > + unsigned long flags; > + > + spin_lock_irqsave(&info->irq_lock, flags); > + status = readl_relaxed(info->base + IRQ_STATUS + 4 * i); > + /* Manage only the interrupt that was enabled */ > + status &= readl_relaxed(info->base + IRQ_EN + 4 * i); > + spin_unlock_irqrestore(&info->irq_lock, flags); > + while (status) { > + u32 hwirq = ffs(status) - 1; > + u32 virq = irq_linear_revmap(d, hwirq + > + i * GPIO_PER_REG); Use irq_find_mapping() instead please. > + generic_handle_irq(virq); > + status &= ~(1 << hwirq); Why not status &= ~BIT(hwirq); > + } > + } > + chained_irq_exit(chip, desc); Apart from that nice, it re-reads status on every iteration which is good. > +static int armada_37xx_irqchip_register(struct platform_device *pdev, > + struct armada_37xx_pinctrl *info) > +{ > + struct device_node *np = info->dev->of_node; > + int nrirqs = info->data->nr_pins; > + struct gpio_chip *gc = &info->gpio_chip; > + struct irq_chip *irqchip = &info->irq_chip; > + struct resource res; > + int ret, i, nr_irq_parent; > + > + for_each_child_of_node(info->dev->of_node, np) { > + if (of_find_property(np, "gpio-controller", NULL)) { > + ret = 0; > + break; > + } > + }; Now there is this thing again looping over the nodes. > + if (ret) > + return ret; ret may be used uninitialized here, if you loop over all nodes and do not find any "gpio-controller". The static code checks will just scream about this. (Please fix in the other patch as well if present there.) > + nr_irq_parent = of_irq_count(np); > + spin_lock_init(&info->irq_lock); > + > + if (!nr_irq_parent) { > + dev_err(&pdev->dev, "Invalid or no IRQ\n"); > + return 0; > + } What if it is > 1? That doesn't seem to work but will pass this check silently. > + ret = gpiochip_irqchip_add(gc, irqchip, 0, > + handle_level_irq, IRQ_TYPE_NONE); If you also set up the handler in .set_type() you can assign handle_bad_irq() here and let .set_type set the right handler as e.g. drivers/gpio/gpio-pl061.c. > + for (i = 0; i < nrirqs; i++) { > + struct irq_data *d = irq_get_irq_data(gc->irq_base + i); > + > + d->mask = 1 << (i % GPIO_PER_REG); > + } What is this? It looks like a big hack. At least put in a fat comment about what is going on and why. > + for (i = 0; i < nr_irq_parent; i++) { > + int irq = irq_of_parse_and_map(np, i); I think gpiochip_irqchip_add() will do this for you already, as it calls irq_create_mapping() for all offsets which will call irq_of_parse_and_map() am I right? > + > + if (irq < 0) > + continue; > + > + gpiochip_set_chained_irqchip(gc, irqchip, irq, > + armada_37xx_irq_handler); > + } So only this statement for each IRQ should be all right. I think this driver needs a bit of tinkering and refining. Yours, Linus Walleij
Hi Linus, On lun., mars 27 2017, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Mar 21, 2017 at 7:28 PM, Gregory CLEMENT > <gregory.clement@free-electrons.com> wrote: > >> The Armada 37xx SoCs can handle interrupt through GPIO. However it can >> only manage the edge ones. >> >> The way the interrupt are managed are classical so we can use the generic >> interrupt chip model. >> >> The only unusual "feature" is that many interrupts are connected to the >> parent interrupt controller. But we do not take advantage of this and use >> the chained irq with all of them. >> >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > > You need something in your Kconfig > doing select GPIOLIB_IRQCHIP unless there is > something I miss here. I forgot to add it, I will do it in the v4. > >> +#define IRQ_EN 0x0 >> +#define IRQ_POL 0x08 >> +#define IRQ_STATUS 0x10 > > This just cries out to me that there is a register 0x0c > and I bet it handles edges vs levels so you could also implement > level IRQs. Am I right? Unfortunately, no :( As far as I know there is now way to handle level. The 0xc register is the IRQ_POL for the GPIO above 32. > >> - aramda_37xx_update_reg(®, offset); >> + armada_37xx_update_reg(®, offset); > (...) >> - aramda_37xx_update_reg(®, offset); >> + armada_37xx_update_reg(®, offset); > > These spelling fixes, do not do them in this patch, fix the first > patch adding them > instead. It's super-confusing. Applies everywhere. It was a typo (too much 'a' in the same word) that I properly fixed in the v3. (But I still need to fix the title of the patch in the v4) >> +static void armada_37xx_irq_handler(struct irq_desc *desc) >> +{ >> + struct gpio_chip *gc = irq_desc_get_handler_data(desc); >> + struct irq_chip *chip = irq_desc_get_chip(desc); >> + struct armada_37xx_pinctrl *info = gpiochip_get_data(gc); >> + struct irq_domain *d = gc->irqdomain; >> + int i; >> + >> + chained_irq_enter(chip, desc); >> + for (i = 0; i <= d->revmap_size / GPIO_PER_REG; i++) { >> + u32 status; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&info->irq_lock, flags); >> + status = readl_relaxed(info->base + IRQ_STATUS + 4 * i); >> + /* Manage only the interrupt that was enabled */ >> + status &= readl_relaxed(info->base + IRQ_EN + 4 * i); >> + spin_unlock_irqrestore(&info->irq_lock, flags); >> + while (status) { >> + u32 hwirq = ffs(status) - 1; >> + u32 virq = irq_linear_revmap(d, hwirq + >> + i * GPIO_PER_REG); > > Use irq_find_mapping() instead please. As we are in the interrupt handler I chose to use this function because according to its documentation: "This is a fast path alternative to irq_find_mapping() that can be called directly by irq controller code to save a handful of instructions". The only restriction is "It is always safe to call, but won't find irqs mapped using the radix tree.". So I think that for this driver it is okay. > >> + generic_handle_irq(virq); >> + status &= ~(1 << hwirq); > > Why not status &= ~BIT(hwirq); OK > >> + } >> + } >> + chained_irq_exit(chip, desc); > > Apart from that nice, it re-reads status on every iteration which is > good. > >> +static int armada_37xx_irqchip_register(struct platform_device *pdev, >> + struct armada_37xx_pinctrl *info) >> +{ >> + struct device_node *np = info->dev->of_node; >> + int nrirqs = info->data->nr_pins; >> + struct gpio_chip *gc = &info->gpio_chip; >> + struct irq_chip *irqchip = &info->irq_chip; >> + struct resource res; >> + int ret, i, nr_irq_parent; >> + >> + for_each_child_of_node(info->dev->of_node, np) { >> + if (of_find_property(np, "gpio-controller", NULL)) { >> + ret = 0; >> + break; >> + } >> + }; > > Now there is this thing again looping over the nodes. As explained in the other patch we will only have one GPIO subnode. > >> + if (ret) >> + return ret; > > ret may be used uninitialized here, if you loop over all nodes > and do not find any "gpio-controller". > > The static code checks will just scream about this. > > (Please fix in the other patch as well if present there.) OK > >> + nr_irq_parent = of_irq_count(np); >> + spin_lock_init(&info->irq_lock); >> + >> + if (!nr_irq_parent) { >> + dev_err(&pdev->dev, "Invalid or no IRQ\n"); >> + return 0; >> + } > > What if it is > 1? That doesn't seem to work but will pass this > check silently. If we have nr_irq_parent > 1, it will work and it is actually expected. > >> + ret = gpiochip_irqchip_add(gc, irqchip, 0, >> + handle_level_irq, IRQ_TYPE_NONE); > > If you also set up the handler in .set_type() you can assign > handle_bad_irq() here and let .set_type set the right handler > as e.g. drivers/gpio/gpio-pl061.c. Well the hardware can only manage the edge trigger, so there is no benefit to modify it each time we want to change the kind of edge we want (raising or falling). But your comment make me realized that I used the wrong one, I will move to handle_edge_irq in the v4. > >> + for (i = 0; i < nrirqs; i++) { >> + struct irq_data *d = irq_get_irq_data(gc->irq_base + i); >> + >> + d->mask = 1 << (i % GPIO_PER_REG); >> + } > > What is this? It looks like a big hack. At least put in a fat > comment about what is going on and why. I can reuse a part of the commit log here: "The only unusual "feature" is that many interrupts are connected to the parent interrupt controller. But we do not take advantage of this and use the chained irq with all of them." > >> + for (i = 0; i < nr_irq_parent; i++) { >> + int irq = irq_of_parse_and_map(np, i); > > I think gpiochip_irqchip_add() will do this for you already, > as it calls irq_create_mapping() for all offsets which will call > irq_of_parse_and_map() am I right? After reading the code, it doesn't seem it is the case. At least there is no irq_of_parse_and_map() call from gpiochip_irqchip_add(). And waht we need here is to associate each IRQ to the same GPIO handler. I can still try without this line to confirm it. Gregory
On Tue, Mar 28, 2017 at 12:36 PM, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > On lun., mars 27 2017, Linus Walleij <linus.walleij@linaro.org> wrote: >>> + u32 virq = irq_linear_revmap(d, hwirq + >>> + i * GPIO_PER_REG); >> >> Use irq_find_mapping() instead please. > > As we are in the interrupt handler I chose to use this function because > according to its documentation: "This is a fast path alternative to > irq_find_mapping() that can be called directly by irq controller code to > save a handful of instructions". > > The only restriction is "It is always safe to call, but won't find irqs > mapped using the radix tree.". So I think that for this driver it is > okay. So you are relying on the core code in gpiolib to select a linear map. That is implicit semantics, and either all drivers using GPIOLIB_IRQCHIP should be changed to irq_linear_revmap() or all stay with irq_find_mapping(). In this case, I doubt it that you are actually so timing critical that it matters. Please use irq_find_mapping(). >>> + nr_irq_parent = of_irq_count(np); >>> + spin_lock_init(&info->irq_lock); >>> + >>> + if (!nr_irq_parent) { >>> + dev_err(&pdev->dev, "Invalid or no IRQ\n"); >>> + return 0; >>> + } >> >> What if it is > 1? That doesn't seem to work but will pass this >> check silently. > > If we have nr_irq_parent > 1, it will work and it is actually expected. Ah, I get it. Nice. >>> + ret = gpiochip_irqchip_add(gc, irqchip, 0, >>> + handle_level_irq, IRQ_TYPE_NONE); >> >> If you also set up the handler in .set_type() you can assign >> handle_bad_irq() here and let .set_type set the right handler >> as e.g. drivers/gpio/gpio-pl061.c. > > Well the hardware can only manage the edge trigger, so there is no > benefit to modify it each time we want to change the kind of edge we > want (raising or falling). But your comment make me realized that I used > the wrong one, I will move to handle_edge_irq in the v4. Ooops, yeah handle_edge_irq() is what calls the ACK callback. >>> + for (i = 0; i < nrirqs; i++) { >>> + struct irq_data *d = irq_get_irq_data(gc->irq_base + i); >>> + >>> + d->mask = 1 << (i % GPIO_PER_REG); >>> + } >> >> What is this? It looks like a big hack. At least put in a fat >> comment about what is going on and why. > > I can reuse a part of the commit log here: "The only unusual "feature" > is that many interrupts are connected to the parent interrupt > controller. But we do not take advantage of this and use the chained irq > with all of them." What you're doing is mocking around with core irqchip semantics. Is ->mask really supposed to be played around with from the outsid like this? Anyways: BIT(i % GPIO_PER_REG) is nicer. >>> + for (i = 0; i < nr_irq_parent; i++) { >>> + int irq = irq_of_parse_and_map(np, i); >> >> I think gpiochip_irqchip_add() will do this for you already, >> as it calls irq_create_mapping() for all offsets which will call >> irq_of_parse_and_map() am I right? > > After reading the code, it doesn't seem it is the case. At least there > is no irq_of_parse_and_map() call from gpiochip_irqchip_add(). And waht > we need here is to associate each IRQ to the same GPIO handler. > > I can still try without this line to confirm it. It has irq_create_mapping(gpiochip->irqdomain, offset); that get called for every IRQ, and that will eventually call irq_of_parse_and_map() if the IRQs are defined in the device tree. (IIRC) Yours, Linus Walleij
Hi Linus, On mar., mars 28 2017, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Mar 28, 2017 at 12:36 PM, Gregory CLEMENT > <gregory.clement@free-electrons.com> wrote: >> On lun., mars 27 2017, Linus Walleij <linus.walleij@linaro.org> wrote: > >>>> + u32 virq = irq_linear_revmap(d, hwirq + >>>> + i * GPIO_PER_REG); >>> >>> Use irq_find_mapping() instead please. >> >> As we are in the interrupt handler I chose to use this function because >> according to its documentation: "This is a fast path alternative to >> irq_find_mapping() that can be called directly by irq controller code to >> save a handful of instructions". >> >> The only restriction is "It is always safe to call, but won't find irqs >> mapped using the radix tree.". So I think that for this driver it is >> okay. > > So you are relying on the core code in gpiolib to select a linear > map. That is implicit semantics, and either all drivers using > GPIOLIB_IRQCHIP should be changed to irq_linear_revmap() > or all stay with irq_find_mapping(). > > In this case, I doubt it that you are actually so timing critical that > it matters. Please use irq_find_mapping(). OK >>>> + ret = gpiochip_irqchip_add(gc, irqchip, 0, >>>> + handle_level_irq, IRQ_TYPE_NONE); >>> >>> If you also set up the handler in .set_type() you can assign >>> handle_bad_irq() here and let .set_type set the right handler >>> as e.g. drivers/gpio/gpio-pl061.c. >> >> Well the hardware can only manage the edge trigger, so there is no >> benefit to modify it each time we want to change the kind of edge we >> want (raising or falling). But your comment make me realized that I used >> the wrong one, I will move to handle_edge_irq in the v4. > > Ooops, yeah handle_edge_irq() is what calls the ACK callback. > >>>> + for (i = 0; i < nrirqs; i++) { >>>> + struct irq_data *d = irq_get_irq_data(gc->irq_base + i); >>>> + >>>> + d->mask = 1 << (i % GPIO_PER_REG); >>>> + } >>> >>> What is this? It looks like a big hack. At least put in a fat >>> comment about what is going on and why. >> >> I can reuse a part of the commit log here: "The only unusual "feature" >> is that many interrupts are connected to the parent interrupt >> controller. But we do not take advantage of this and use the chained irq >> with all of them." > > What you're doing is mocking around with core irqchip semantics. > Is ->mask really supposed to be played around with from the outsid > like this? According to the documentation mask is a "precomputed bitmask for accessing the chip registers" and it is exactly the way it is used in this driver. Moreover, currently ->mask is only used by the generic irqchip framework and not by the core of the irqchip subsystem. So the mask initialization is not done from the oustide but at the same level as the generic irqchip which is not used here. > Anyways: BIT(i % GPIO_PER_REG) is nicer. OK > >>>> + for (i = 0; i < nr_irq_parent; i++) { >>>> + int irq = irq_of_parse_and_map(np, i); >>> >>> I think gpiochip_irqchip_add() will do this for you already, >>> as it calls irq_create_mapping() for all offsets which will call >>> irq_of_parse_and_map() am I right? >> >> After reading the code, it doesn't seem it is the case. At least there >> is no irq_of_parse_and_map() call from gpiochip_irqchip_add(). And waht >> we need here is to associate each IRQ to the same GPIO handler. >> >> I can still try without this line to confirm it. > > It has irq_create_mapping(gpiochip->irqdomain, offset); that get > called for every IRQ, and that will eventually call irq_of_parse_and_map() > if the IRQs are defined in the device tree. (IIRC) When I followed the functions called I never find a call to irq_of_parse_and_map(), the closer things related to device tree I found was: "virq = irq_domain_alloc_descs(-1, 1, hwirq, of_node_to_nid(of_node), NULL);" http://elixir.free-electrons.com/source/kernel/irq/irqdomain.c?v=4.11-rc4#L507 Gregory
On Tue, Mar 28, 2017 at 4:19 PM, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > On mar., mars 28 2017, Linus Walleij <linus.walleij@linaro.org> wrote: >> What you're doing is mocking around with core irqchip semantics. >> Is ->mask really supposed to be played around with from the outsid >> like this? > > According to the documentation mask is a "precomputed bitmask for > accessing the chip registers" and it is exactly the way it is used in > this driver. > > Moreover, currently ->mask is only used by the generic irqchip framework > and not by the core of the irqchip subsystem. So the mask initialization > is not done from the oustide but at the same level as the generic > irqchip which is not used here. OK excellent, sorry for my ignorance. We should rather point to your driver as a good example of how to do this. Care to add some few lines of comment as to what is going on for others that need to do the same? Actually it would even be good to have something in Documentation/gpio/driver.txt >> It has irq_create_mapping(gpiochip->irqdomain, offset); that get >> called for every IRQ, and that will eventually call irq_of_parse_and_map() >> if the IRQs are defined in the device tree. (IIRC) > > When I followed the functions called I never find a call to > irq_of_parse_and_map(), the closer things related to device tree I found > was: > "virq = irq_domain_alloc_descs(-1, 1, hwirq, of_node_to_nid(of_node), > NULL);" > http://elixir.free-electrons.com/source/kernel/irq/irqdomain.c?v=4.11-rc4#L507 I don't know if I'm rambling or what. I'm pretty sure it gets called, maybe even earlier, like when the DT is parsed for the platform. We have so many drivers not seemingly needing this, but if your driver needs it, all others may need to be fixed too. Can you put a print in irq_of_parse_and_map() and see what happens? Yours, Linus Walleij
Hi Linus, On mer., mars 29 2017, Linus Walleij <linus.walleij@linaro.org> wrote: >>> It has irq_create_mapping(gpiochip->irqdomain, offset); that get >>> called for every IRQ, and that will eventually call irq_of_parse_and_map() >>> if the IRQs are defined in the device tree. (IIRC) >> >> When I followed the functions called I never find a call to >> irq_of_parse_and_map(), the closer things related to device tree I found >> was: >> "virq = irq_domain_alloc_descs(-1, 1, hwirq, of_node_to_nid(of_node), >> NULL);" >> http://elixir.free-electrons.com/source/kernel/irq/irqdomain.c?v=4.11-rc4#L507 > > I don't know if I'm rambling or what. I'm pretty sure it gets called, maybe > even earlier, like when the DT is parsed for the platform. We have so many > drivers not seemingly needing this, but if your driver needs it, all others > may need to be fixed too. > > Can you put a print in irq_of_parse_and_map() and see what happens? So if I don't call it explicitly in my driver, then this function is never called for the gpio. Gregory
diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c index e41e2d02aca7..5d3af27f1ba9 100644 --- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c @@ -13,7 +13,9 @@ #include <linux/gpio/driver.h> #include <linux/mfd/syscon.h> #include <linux/of.h> +#include <linux/of_address.h> #include <linux/of_device.h> +#include <linux/of_irq.h> #include <linux/pinctrl/pinconf-generic.h> #include <linux/pinctrl/pinconf.h> #include <linux/pinctrl/pinctrl.h> @@ -30,6 +32,11 @@ #define OUTPUT_CTL 0x20 #define SELECTION 0x30 +#define IRQ_EN 0x0 +#define IRQ_POL 0x08 +#define IRQ_STATUS 0x10 +#define IRQ_WKUP 0x18 + static int global_pin; #define NB_FUNCS 2 @@ -76,9 +83,12 @@ struct armada_37xx_pmx_func { struct armada_37xx_pinctrl { struct regmap *regmap; + void __iomem *base; struct armada_37xx_pin_data *data; struct device *dev; struct gpio_chip gpio_chip; + struct irq_chip irq_chip; + spinlock_t irq_lock; struct pinctrl_desc pctl; struct pinctrl_dev *pctl_dev; struct armada_37xx_pin_group *groups; @@ -316,7 +326,7 @@ static int armada_37xx_pmx_set(struct pinctrl_dev *pctldev, return armada_37xx_pmx_set_by_name(pctldev, name, grp); } -static inline void aramda_37xx_update_reg(unsigned int *reg, +static inline void armada_37xx_update_reg(unsigned int *reg, unsigned int offset) { /* We never have more than 2 registers */ @@ -326,6 +336,14 @@ static inline void aramda_37xx_update_reg(unsigned int *reg, } } +static inline void armada_37xx_irq_update_reg(unsigned int *reg, + struct irq_data *d) +{ + int offset = irqd_to_hwirq(d); + + armada_37xx_update_reg(reg, offset); +} + static int armada_37xx_gpio_direction_input(struct gpio_chip *chip, unsigned int offset) { @@ -333,7 +351,7 @@ static int armada_37xx_gpio_direction_input(struct gpio_chip *chip, unsigned int reg = OUTPUT_EN; unsigned int mask; - aramda_37xx_update_reg(®, offset); + armada_37xx_update_reg(®, offset); mask = BIT(offset); return regmap_update_bits(info->regmap, reg, mask, 0); @@ -346,7 +364,7 @@ static int armada_37xx_gpio_get_direction(struct gpio_chip *chip, unsigned int reg = OUTPUT_EN; unsigned int val, mask; - aramda_37xx_update_reg(®, offset); + armada_37xx_update_reg(®, offset); mask = BIT(offset); regmap_read(info->regmap, reg, &val); @@ -361,7 +379,7 @@ static int armada_37xx_gpio_direction_output(struct gpio_chip *chip, unsigned int reg = OUTPUT_EN; unsigned int mask; - aramda_37xx_update_reg(®, offset); + armada_37xx_update_reg(®, offset); mask = BIT(offset); return regmap_update_bits(info->regmap, reg, mask, mask); @@ -373,7 +391,7 @@ static int armada_37xx_gpio_get(struct gpio_chip *chip, unsigned int offset) unsigned int reg = INPUT_VAL; unsigned int val, mask; - aramda_37xx_update_reg(®, offset); + armada_37xx_update_reg(®, offset); mask = BIT(offset); regmap_read(info->regmap, reg, &val); @@ -388,7 +406,7 @@ static void armada_37xx_gpio_set(struct gpio_chip *chip, unsigned int offset, unsigned int reg = OUTPUT_VAL; unsigned int mask, val; - aramda_37xx_update_reg(®, offset); + armada_37xx_update_reg(®, offset); mask = BIT(offset); val = value ? mask : 0; @@ -449,6 +467,194 @@ static const struct gpio_chip armada_37xx_gpiolib_chip = { .owner = THIS_MODULE, }; +void armada_37xx_irq_ack(struct irq_data *d) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + struct armada_37xx_pinctrl *info = gpiochip_get_data(chip); + u32 reg = IRQ_STATUS, mask = d->mask; + unsigned long flags; + + armada_37xx_irq_update_reg(®, d); + spin_lock_irqsave(&info->irq_lock, flags); + writel(mask, info->base + reg); + spin_unlock_irqrestore(&info->irq_lock, flags); +} + +void armada_37xx_irq_mask(struct irq_data *d) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + struct armada_37xx_pinctrl *info = gpiochip_get_data(chip); + u32 val, reg = IRQ_EN, mask = d->mask; + unsigned long flags; + + armada_37xx_irq_update_reg(®, d); + spin_lock_irqsave(&info->irq_lock, flags); + val = readl(info->base + reg); + writel(val & ~mask, info->base + reg); + spin_unlock_irqrestore(&info->irq_lock, flags); +} + +void armada_37xx_irq_unmask(struct irq_data *d) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + struct armada_37xx_pinctrl *info = gpiochip_get_data(chip); + u32 val, reg = IRQ_EN, mask = d->mask; + unsigned long flags; + + armada_37xx_irq_update_reg(®, d); + spin_lock_irqsave(&info->irq_lock, flags); + val = readl(info->base + reg); + writel(val | mask, info->base + reg); + spin_unlock_irqrestore(&info->irq_lock, flags); +} + +static int armada_37xx_irq_set_wake(struct irq_data *d, unsigned int on) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + struct armada_37xx_pinctrl *info = gpiochip_get_data(chip); + u32 val, reg = IRQ_WKUP, mask = d->mask; + unsigned long flags; + + armada_37xx_irq_update_reg(®, d); + spin_lock_irqsave(&info->irq_lock, flags); + val = readl(info->base + reg); + if (on) + val |= mask; + else + val &= ~mask; + writel(val, info->base + reg); + spin_unlock_irqrestore(&info->irq_lock, flags); + + return 0; +} + +static int armada_37xx_irq_set_type(struct irq_data *d, unsigned int type) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + struct armada_37xx_pinctrl *info = gpiochip_get_data(chip); + u32 val, reg = IRQ_POL, mask = d->mask; + unsigned long flags; + + spin_lock_irqsave(&info->irq_lock, flags); + armada_37xx_irq_update_reg(®, d); + val = readl(info->base + reg); + switch (type) { + case IRQ_TYPE_EDGE_RISING: + val &= ~mask; + break; + case IRQ_TYPE_EDGE_FALLING: + val |= mask; + break; + default: + spin_unlock_irqrestore(&info->irq_lock, flags); + return -EINVAL; + } + writel(val, info->base + reg); + spin_unlock_irqrestore(&info->irq_lock, flags); + + return 0; +} + + +static void armada_37xx_irq_handler(struct irq_desc *desc) +{ + struct gpio_chip *gc = irq_desc_get_handler_data(desc); + struct irq_chip *chip = irq_desc_get_chip(desc); + struct armada_37xx_pinctrl *info = gpiochip_get_data(gc); + struct irq_domain *d = gc->irqdomain; + int i; + + chained_irq_enter(chip, desc); + for (i = 0; i <= d->revmap_size / GPIO_PER_REG; i++) { + u32 status; + unsigned long flags; + + spin_lock_irqsave(&info->irq_lock, flags); + status = readl_relaxed(info->base + IRQ_STATUS + 4 * i); + /* Manage only the interrupt that was enabled */ + status &= readl_relaxed(info->base + IRQ_EN + 4 * i); + spin_unlock_irqrestore(&info->irq_lock, flags); + while (status) { + u32 hwirq = ffs(status) - 1; + u32 virq = irq_linear_revmap(d, hwirq + + i * GPIO_PER_REG); + + generic_handle_irq(virq); + status &= ~(1 << hwirq); + } + } + chained_irq_exit(chip, desc); +} + +static int armada_37xx_irqchip_register(struct platform_device *pdev, + struct armada_37xx_pinctrl *info) +{ + struct device_node *np = info->dev->of_node; + int nrirqs = info->data->nr_pins; + struct gpio_chip *gc = &info->gpio_chip; + struct irq_chip *irqchip = &info->irq_chip; + struct resource res; + int ret, i, nr_irq_parent; + + for_each_child_of_node(info->dev->of_node, np) { + if (of_find_property(np, "gpio-controller", NULL)) { + ret = 0; + break; + } + }; + if (ret) + return ret; + + nr_irq_parent = of_irq_count(np); + spin_lock_init(&info->irq_lock); + + if (!nr_irq_parent) { + dev_err(&pdev->dev, "Invalid or no IRQ\n"); + return 0; + } + + if (of_address_to_resource(info->dev->of_node, 1, &res)) { + dev_err(info->dev, "cannot find IO resource\n"); + return -ENOENT; + } + + info->base = devm_ioremap_resource(info->dev, &res); + if (IS_ERR(info->base)) + return PTR_ERR(info->base); + + irqchip->irq_ack = armada_37xx_irq_ack; + irqchip->irq_mask = armada_37xx_irq_mask; + irqchip->irq_unmask = armada_37xx_irq_unmask; + irqchip->irq_set_wake = armada_37xx_irq_set_wake; + irqchip->irq_set_type = armada_37xx_irq_set_type; + irqchip->name = info->data->name; + + ret = gpiochip_irqchip_add(gc, irqchip, 0, + handle_level_irq, IRQ_TYPE_NONE); + if (ret) { + dev_info(&pdev->dev, "could not add irqchip\n"); + return ret; + } + + for (i = 0; i < nrirqs; i++) { + struct irq_data *d = irq_get_irq_data(gc->irq_base + i); + + d->mask = 1 << (i % GPIO_PER_REG); + } + + for (i = 0; i < nr_irq_parent; i++) { + int irq = irq_of_parse_and_map(np, i); + + if (irq < 0) + continue; + + gpiochip_set_chained_irqchip(gc, irqchip, irq, + armada_37xx_irq_handler); + } + + return 0; +} + static int armada_37xx_gpiochip_register(struct platform_device *pdev, struct armada_37xx_pinctrl *info) { @@ -477,6 +683,9 @@ static int armada_37xx_gpiochip_register(struct platform_device *pdev, ret = gpiochip_add_data(gc, info); if (ret) return ret; + ret = armada_37xx_irqchip_register(pdev, info); + if (ret) + return ret; return 0; } @@ -518,7 +727,7 @@ static int armada_37xx_fill_group(struct armada_37xx_pinctrl *info, int base) grp->pins = devm_kzalloc(info->dev, (grp->npins + grp->extra_npins) * - sizeof(*grp->pins), GFP_KERNEL); + sizeof(*grp->pins), GFP_KERNEL); if (!grp->pins) return -ENOMEM;
The Armada 37xx SoCs can handle interrupt through GPIO. However it can only manage the edge ones. The way the interrupt are managed are classical so we can use the generic interrupt chip model. The only unusual "feature" is that many interrupts are connected to the parent interrupt controller. But we do not take advantage of this and use the chained irq with all of them. Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> --- drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 223 ++++++++++++++++++++- 1 file changed, 216 insertions(+), 7 deletions(-)