From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gregory CLEMENT Subject: Re: [PATCH v4 5/7] pinctrl: aramda-37xx: Add irqchip support Date: Wed, 26 Apr 2017 11:23:49 +0200 Message-ID: <87zif38qu2.fsf@free-electrons.com> References: <70ffe3343c13d01737bf74e5de4898d0c0be07a0.1491405475.git-series.gregory.clement@free-electrons.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: (Linus Walleij's message of "Mon, 24 Apr 2017 14:14:36 +0200") List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Linus Walleij Cc: Thomas Petazzoni , Andrew Lunn , Jason Cooper , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Nadav Haklai , "linux-gpio@vger.kernel.org" , Rob Herring , Neta Zur Hershkovits , Victor Gu , Hua Jing , Marcin Wojtas , Wilson Ding , "linux-arm-kernel@lists.infradead.org" , Sebastian Hesselbarth List-Id: linux-gpio@vger.kernel.org Hi Linus, On lun., avril 24 2017, Linus Walleij wrote: > 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. It is done in patch 2 "arm64: marvell: enable the Armada 37xx pinctrl driver". > > 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; > (...) Yes sure I cand do it. > >> +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). If a new IRQ appears during the loop, then the irq handler will be called again because the cause of this new IRQ won't have been acked yet. So I think we're fine here. > >> +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 */ > OK >> + 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")) > OK Gregory -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1433747AbdDZJcF (ORCPT ); Wed, 26 Apr 2017 05:32:05 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:42359 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1956206AbdDZJXv (ORCPT ); Wed, 26 Apr 2017 05:23:51 -0400 From: Gregory CLEMENT To: Linus Walleij 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 Subject: Re: [PATCH v4 5/7] pinctrl: aramda-37xx: Add irqchip support References: <70ffe3343c13d01737bf74e5de4898d0c0be07a0.1491405475.git-series.gregory.clement@free-electrons.com> Date: Wed, 26 Apr 2017 11:23:49 +0200 In-Reply-To: (Linus Walleij's message of "Mon, 24 Apr 2017 14:14:36 +0200") Message-ID: <87zif38qu2.fsf@free-electrons.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Linus, On lun., avril 24 2017, Linus Walleij wrote: > 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. It is done in patch 2 "arm64: marvell: enable the Armada 37xx pinctrl driver". > > 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; > (...) Yes sure I cand do it. > >> +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). If a new IRQ appears during the loop, then the irq handler will be called again because the cause of this new IRQ won't have been acked yet. So I think we're fine here. > >> +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 */ > OK >> + 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")) > OK Gregory -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: gregory.clement@free-electrons.com (Gregory CLEMENT) Date: Wed, 26 Apr 2017 11:23:49 +0200 Subject: [PATCH v4 5/7] pinctrl: aramda-37xx: Add irqchip support In-Reply-To: (Linus Walleij's message of "Mon, 24 Apr 2017 14:14:36 +0200") References: <70ffe3343c13d01737bf74e5de4898d0c0be07a0.1491405475.git-series.gregory.clement@free-electrons.com> Message-ID: <87zif38qu2.fsf@free-electrons.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Linus, On lun., avril 24 2017, Linus Walleij wrote: > 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. It is done in patch 2 "arm64: marvell: enable the Armada 37xx pinctrl driver". > > 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; > (...) Yes sure I cand do it. > >> +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). If a new IRQ appears during the loop, then the irq handler will be called again because the cause of this new IRQ won't have been acked yet. So I think we're fine here. > >> +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 */ > OK >> + 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")) > OK Gregory -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com