From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Walleij Subject: Re: [PATCH v4 5/7] pinctrl: aramda-37xx: Add irqchip support Date: Mon, 24 Apr 2017 14:14:36 +0200 Message-ID: References: <70ffe3343c13d01737bf74e5de4898d0c0be07a0.1491405475.git-series.gregory.clement@free-electrons.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: Received: from mail-io0-f173.google.com ([209.85.223.173]:32997 "EHLO mail-io0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1044560AbdDXMOj (ORCPT ); Mon, 24 Apr 2017 08:14:39 -0400 Received: by mail-io0-f173.google.com with SMTP id k87so191381842ioi.0 for ; Mon, 24 Apr 2017 05:14:38 -0700 (PDT) In-Reply-To: <70ffe3343c13d01737bf74e5de4898d0c0be07a0.1491405475.git-series.gregory.clement@free-electrons.com> Sender: linux-gpio-owner@vger.kernel.org List-Id: linux-gpio@vger.kernel.org To: Gregory CLEMENT Cc: "linux-gpio@vger.kernel.org" , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Thomas Petazzoni , "linux-arm-kernel@lists.infradead.org" , Rob Herring , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Nadav Haklai , Victor Gu , Marcin Wojtas , Wilson Ding , Hua Jing , Neta Zur Hershkovits On Wed, Apr 5, 2017 at 5:18 PM, Gregory CLEMENT 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 There are some issues with this patch. First: You need to add select GPIOLIB_IRQCHIP to the Kconfig entry. It's only working in your setup because something else is selecting this for you, probably. At all places like this: > + u32 mask = d->mask; (...) > + if (on) > + val |= mask; > + else > + val &= ~mask; Isn't it simpler to just use d->mask directly in the code and skip the local variable? if (on) val |= d->mask; (...) > +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_find_mapping(d, hwirq + > + i * GPIO_PER_REG); > + > + generic_handle_irq(virq); > + status &= ~BIT(hwirq); > + } You hae a problem here is a new IRQ appears while you are inside of this loop. You need to re-read the status register for each iteration (and &= with the IRQ_EN I guess). > +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 = -ENODEV, i, nr_irq_parent; > + This warrants a comment: /* Check if we have at least one gpio-controller child node */ > + for_each_child_of_node(info->dev->of_node, np) { > + if (of_find_property(np, "gpio-controller", NULL)) { > + ret = 0; > + break; > + } Rewrite: if (of_property_read_bool(np, "gpio-controller")) > + }; > + 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_edge_irq, IRQ_TYPE_NONE); > + if (ret) { > + dev_info(&pdev->dev, "could not add irqchip\n"); > + return ret; > + } > + > + /* > + * 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 < nrirqs; i++) { > + struct irq_data *d = irq_get_irq_data(gc->irq_base + i); > + > + /* > + * The mask field is a "precomputed bitmask for > + * accessing the chip registers" which was introduced > + * for the generic irqchip framework. As we don't use > + * this framework, we can reuse this field for our own > + * usage. > + */ > + d->mask = BIT(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) > { > @@ -496,6 +714,9 @@ static int armada_37xx_gpiochip_register(struct platform_device *pdev, > ret = devm_gpiochip_add_data(&pdev->dev, gc, info); > if (ret) > return ret; > + ret = armada_37xx_irqchip_register(pdev, info); > + if (ret) > + return ret; > > return 0; > } > -- > git-series 0.9.1 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1046524AbdDXMOs (ORCPT ); Mon, 24 Apr 2017 08:14:48 -0400 Received: from mail-io0-f177.google.com ([209.85.223.177]:32997 "EHLO mail-io0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1044539AbdDXMOj (ORCPT ); Mon, 24 Apr 2017 08:14:39 -0400 MIME-Version: 1.0 In-Reply-To: <70ffe3343c13d01737bf74e5de4898d0c0be07a0.1491405475.git-series.gregory.clement@free-electrons.com> References: <70ffe3343c13d01737bf74e5de4898d0c0be07a0.1491405475.git-series.gregory.clement@free-electrons.com> From: Linus Walleij Date: Mon, 24 Apr 2017 14:14:36 +0200 Message-ID: Subject: Re: [PATCH v4 5/7] pinctrl: aramda-37xx: Add irqchip support To: Gregory CLEMENT Cc: "linux-gpio@vger.kernel.org" , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Thomas Petazzoni , "linux-arm-kernel@lists.infradead.org" , Rob Herring , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Nadav Haklai , Victor Gu , Marcin Wojtas , Wilson Ding , Hua Jing , Neta Zur Hershkovits Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 5, 2017 at 5:18 PM, Gregory CLEMENT 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 There are some issues with this patch. First: You need to add select GPIOLIB_IRQCHIP to the Kconfig entry. It's only working in your setup because something else is selecting this for you, probably. At all places like this: > + u32 mask = d->mask; (...) > + if (on) > + val |= mask; > + else > + val &= ~mask; Isn't it simpler to just use d->mask directly in the code and skip the local variable? if (on) val |= d->mask; (...) > +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_find_mapping(d, hwirq + > + i * GPIO_PER_REG); > + > + generic_handle_irq(virq); > + status &= ~BIT(hwirq); > + } You hae a problem here is a new IRQ appears while you are inside of this loop. You need to re-read the status register for each iteration (and &= with the IRQ_EN I guess). > +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 = -ENODEV, i, nr_irq_parent; > + This warrants a comment: /* Check if we have at least one gpio-controller child node */ > + for_each_child_of_node(info->dev->of_node, np) { > + if (of_find_property(np, "gpio-controller", NULL)) { > + ret = 0; > + break; > + } Rewrite: if (of_property_read_bool(np, "gpio-controller")) > + }; > + 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_edge_irq, IRQ_TYPE_NONE); > + if (ret) { > + dev_info(&pdev->dev, "could not add irqchip\n"); > + return ret; > + } > + > + /* > + * 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 < nrirqs; i++) { > + struct irq_data *d = irq_get_irq_data(gc->irq_base + i); > + > + /* > + * The mask field is a "precomputed bitmask for > + * accessing the chip registers" which was introduced > + * for the generic irqchip framework. As we don't use > + * this framework, we can reuse this field for our own > + * usage. > + */ > + d->mask = BIT(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) > { > @@ -496,6 +714,9 @@ static int armada_37xx_gpiochip_register(struct platform_device *pdev, > ret = devm_gpiochip_add_data(&pdev->dev, gc, info); > if (ret) > return ret; > + ret = armada_37xx_irqchip_register(pdev, info); > + if (ret) > + return ret; > > return 0; > } > -- > git-series 0.9.1 From mboxrd@z Thu Jan 1 00:00:00 1970 From: linus.walleij@linaro.org (Linus Walleij) Date: Mon, 24 Apr 2017 14:14:36 +0200 Subject: [PATCH v4 5/7] pinctrl: aramda-37xx: Add irqchip support In-Reply-To: <70ffe3343c13d01737bf74e5de4898d0c0be07a0.1491405475.git-series.gregory.clement@free-electrons.com> References: <70ffe3343c13d01737bf74e5de4898d0c0be07a0.1491405475.git-series.gregory.clement@free-electrons.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Apr 5, 2017 at 5:18 PM, Gregory CLEMENT 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 There are some issues with this patch. First: You need to add select GPIOLIB_IRQCHIP to the Kconfig entry. It's only working in your setup because something else is selecting this for you, probably. At all places like this: > + u32 mask = d->mask; (...) > + if (on) > + val |= mask; > + else > + val &= ~mask; Isn't it simpler to just use d->mask directly in the code and skip the local variable? if (on) val |= d->mask; (...) > +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_find_mapping(d, hwirq + > + i * GPIO_PER_REG); > + > + generic_handle_irq(virq); > + status &= ~BIT(hwirq); > + } You hae a problem here is a new IRQ appears while you are inside of this loop. You need to re-read the status register for each iteration (and &= with the IRQ_EN I guess). > +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 = -ENODEV, i, nr_irq_parent; > + This warrants a comment: /* Check if we have at least one gpio-controller child node */ > + for_each_child_of_node(info->dev->of_node, np) { > + if (of_find_property(np, "gpio-controller", NULL)) { > + ret = 0; > + break; > + } Rewrite: if (of_property_read_bool(np, "gpio-controller")) > + }; > + 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_edge_irq, IRQ_TYPE_NONE); > + if (ret) { > + dev_info(&pdev->dev, "could not add irqchip\n"); > + return ret; > + } > + > + /* > + * 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 < nrirqs; i++) { > + struct irq_data *d = irq_get_irq_data(gc->irq_base + i); > + > + /* > + * The mask field is a "precomputed bitmask for > + * accessing the chip registers" which was introduced > + * for the generic irqchip framework. As we don't use > + * this framework, we can reuse this field for our own > + * usage. > + */ > + d->mask = BIT(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) > { > @@ -496,6 +714,9 @@ static int armada_37xx_gpiochip_register(struct platform_device *pdev, > ret = devm_gpiochip_add_data(&pdev->dev, gc, info); > if (ret) > return ret; > + ret = armada_37xx_irqchip_register(pdev, info); > + if (ret) > + return ret; > > return 0; > } > -- > git-series 0.9.1