From: Magnus Damm <[email protected]>
This patch is V2 of the Emma Mobile GPIO driver. This
driver is designed to be reusable between multiple SoCs
that share the same basic building block, but so far it
has only been used on Emma Mobile EV2.
Each driver instance handles 32 GPIOs with individually
maskable IRQs. The driver operates on two I/O memory
ranges and the 32 GPIOs are hooked up to two interrupts.
In the case of Emma Mobile EV2 this GPIO building block
is used as main external interrupt controller hooking up
159 GPIOS as 159 interrupts via 5 driver instances and
10 interrupts to the GIC and the Cortex-A9 Dual.
Signed-off-by: Magnus Damm <[email protected]>
---
Changes since V1:
- use inline for private data functions using container_of()
- use BIT(n) instead of 1 << n
- added legacy irq domain support for static mappings
- use irqd_to_hwirq() instead of own offset calculation
- convert irqchip callbacks to not care about virq
- rework IRQ handler to read interrupt status inside loop
Changes not made:
- ioread/iowrite are still used over readl/writel
- no request_mem_region
- no devm_ alloc/ioremap
- kept the kconfig dependencies as in V1
Many thanks to Linus Walleij and Arnd Bergmann for
their help with the code review!
Incremental DT feature patch will be posted shortly.
drivers/gpio/Kconfig | 6
drivers/gpio/Makefile | 1
drivers/gpio/gpio-em.c | 418 +++++++++++++++++++++++++++++++++
include/linux/platform_data/gpio-em.h | 10
4 files changed, 435 insertions(+)
--- 0001/drivers/gpio/Kconfig
+++ work/drivers/gpio/Kconfig 2012-05-15 14:50:42.000000000 +0900
@@ -103,6 +103,12 @@ config GPIO_IT8761E
help
Say yes here to support GPIO functionality of IT8761E super I/O chip.
+config GPIO_EM
+ tristate "Emma Mobile GPIO"
+ depends on ARM
+ help
+ Say yes here to support GPIO on Renesas Emma Mobile SoCs.
+
config GPIO_EP93XX
def_bool y
depends on ARCH_EP93XX
--- 0001/drivers/gpio/Makefile
+++ work/drivers/gpio/Makefile 2012-05-15 14:50:42.000000000 +0900
@@ -16,6 +16,7 @@ obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
obj-$(CONFIG_ARCH_DAVINCI) += gpio-davinci.o
+obj-$(CONFIG_GPIO_EM) += gpio-em.o
obj-$(CONFIG_GPIO_EP93XX) += gpio-ep93xx.o
obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
--- /dev/null
+++ work/drivers/gpio/gpio-em.c 2012-05-16 00:07:20.000000000 +0900
@@ -0,0 +1,418 @@
+/*
+ * Emma Mobile GPIO Support - GIO
+ *
+ * Copyright (C) 2012 Magnus Damm
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/interrupt.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/bitops.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/platform_data/gpio-em.h>
+
+struct em_gio_priv {
+ void __iomem *base0;
+ void __iomem *base1;
+ unsigned int irq_base;
+ spinlock_t sense_lock;
+ struct platform_device *pdev;
+ struct gpio_chip gpio_chip;
+ struct irq_chip irq_chip;
+ struct irq_domain *irq_domain;
+};
+
+#define GIO_E1 0x00
+#define GIO_E0 0x04
+#define GIO_EM 0x04
+#define GIO_OL 0x08
+#define GIO_OH 0x0c
+#define GIO_I 0x10
+#define GIO_IIA 0x14
+#define GIO_IEN 0x18
+#define GIO_IDS 0x1c
+#define GIO_IIM 0x1c
+#define GIO_RAW 0x20
+#define GIO_MST 0x24
+#define GIO_IIR 0x28
+
+#define GIO_IDT0 0x40
+#define GIO_IDT1 0x44
+#define GIO_IDT2 0x48
+#define GIO_IDT3 0x4c
+#define GIO_RAWBL 0x50
+#define GIO_RAWBH 0x54
+#define GIO_IRBL 0x58
+#define GIO_IRBH 0x5c
+
+#define GIO_IDT(n) (GIO_IDT0 + ((n) * 4))
+
+static inline unsigned long em_gio_read(struct em_gio_priv *p, int offs)
+{
+ if (offs < GIO_IDT0)
+ return ioread32(p->base0 + offs);
+ else
+ return ioread32(p->base1 + (offs - GIO_IDT0));
+}
+
+static inline void em_gio_write(struct em_gio_priv *p, int offs,
+ unsigned long value)
+{
+ if (offs < GIO_IDT0)
+ iowrite32(value, p->base0 + offs);
+ else
+ iowrite32(value, p->base1 + (offs - GIO_IDT0));
+}
+
+static inline struct em_gio_priv *irq_to_priv(struct irq_data *d)
+{
+ struct irq_chip *chip = irq_data_get_irq_chip(d);
+ return container_of(chip, struct em_gio_priv, irq_chip);
+}
+
+static void em_gio_irq_disable(struct irq_data *d)
+{
+ struct em_gio_priv *p = irq_to_priv(d);
+
+ em_gio_write(p, GIO_IDS, BIT(irqd_to_hwirq(d)));
+}
+
+static void em_gio_irq_enable(struct irq_data *d)
+{
+ struct em_gio_priv *p = irq_to_priv(d);
+
+ em_gio_write(p, GIO_IEN, BIT(irqd_to_hwirq(d)));
+}
+
+#define GIO_ASYNC(x) (x + 8)
+
+static unsigned char em_gio_sense_table[IRQ_TYPE_SENSE_MASK + 1] = {
+ [IRQ_TYPE_EDGE_RISING] = GIO_ASYNC(0x00),
+ [IRQ_TYPE_EDGE_FALLING] = GIO_ASYNC(0x01),
+ [IRQ_TYPE_LEVEL_HIGH] = GIO_ASYNC(0x02),
+ [IRQ_TYPE_LEVEL_LOW] = GIO_ASYNC(0x03),
+ [IRQ_TYPE_EDGE_BOTH] = GIO_ASYNC(0x04),
+};
+
+static int em_gio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+ unsigned char value = em_gio_sense_table[type & IRQ_TYPE_SENSE_MASK];
+ struct em_gio_priv *p = irq_to_priv(d);
+ unsigned int reg, offset, shift;
+ unsigned long flags;
+ unsigned long tmp;
+
+ if (!value)
+ return -EINVAL;
+
+ offset = irqd_to_hwirq(d);
+
+ pr_debug("gio: sense irq = %d, mode = %d\n", offset, value);
+
+ /* 8 x 4 bit fields in 4 IDT registers */
+ reg = GIO_IDT(offset >> 3);
+ shift = (offset & 0x07) << 4;
+
+ spin_lock_irqsave(&p->sense_lock, flags);
+
+ /* disable the interrupt in IIA */
+ tmp = em_gio_read(p, GIO_IIA);
+ tmp &= ~BIT(offset);
+ em_gio_write(p, GIO_IIA, tmp);
+
+ /* change the sense setting in IDT */
+ tmp = em_gio_read(p, reg);
+ tmp &= ~(0xf << shift);
+ tmp |= value << shift;
+ em_gio_write(p, reg, tmp);
+
+ /* clear pending interrupts */
+ em_gio_write(p, GIO_IIR, BIT(offset));
+
+ /* enable the interrupt in IIA */
+ tmp = em_gio_read(p, GIO_IIA);
+ tmp |= BIT(offset);
+ em_gio_write(p, GIO_IIA, tmp);
+
+ spin_unlock_irqrestore(&p->sense_lock, flags);
+
+ return 0;
+}
+
+static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
+{
+ struct em_gio_priv *p = dev_id;
+ unsigned long pending;
+ unsigned int offset, irqs_handled = 0;
+
+ while ((pending = em_gio_read(p, GIO_MST))) {
+ offset = __ffs(pending);
+ em_gio_write(p, GIO_IIR, BIT(offset));
+ generic_handle_irq(irq_find_mapping(p->irq_domain, offset));
+ irqs_handled++;
+ }
+
+ return irqs_handled ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static inline struct em_gio_priv *gpio_to_priv(struct gpio_chip *chip)
+{
+ return container_of(chip, struct em_gio_priv, gpio_chip);
+}
+
+static int em_gio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+ em_gio_write(gpio_to_priv(chip), GIO_E0, BIT(offset));
+ return 0;
+}
+
+static int em_gio_get(struct gpio_chip *chip, unsigned offset)
+{
+ return (int)(em_gio_read(gpio_to_priv(chip), GIO_I) & BIT(offset));
+}
+
+static void __em_gio_set(struct gpio_chip *chip, unsigned int reg,
+ unsigned shift, int value)
+{
+ /* upper 16 bits contains mask and lower 16 actual value */
+ em_gio_write(gpio_to_priv(chip), reg,
+ (1 << (shift + 16)) | (value << shift));
+}
+
+static void em_gio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+ /* output is split into two registers */
+ if (offset < 16)
+ __em_gio_set(chip, GIO_OL, offset, value);
+ else
+ __em_gio_set(chip, GIO_OH, offset - 16, value);
+}
+
+static int em_gio_direction_output(struct gpio_chip *chip, unsigned offset,
+ int value)
+{
+ /* write GPIO value to output before selecting output mode of pin */
+ em_gio_set(chip, offset, value);
+ em_gio_write(gpio_to_priv(chip), GIO_E1, BIT(offset));
+ return 0;
+}
+
+static int em_gio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+ return irq_find_mapping(gpio_to_priv(chip)->irq_domain, offset);
+}
+
+static int em_gio_irq_domain_map(struct irq_domain *h, unsigned int virq,
+ irq_hw_number_t hw)
+{
+ struct em_gio_priv *p = h->host_data;
+
+ pr_debug("gio: map hw irq = %d, virq = %d\n", (int)hw, virq);
+
+ irq_set_chip_data(virq, h->host_data);
+ irq_set_chip_and_handler(virq, &p->irq_chip, handle_level_irq);
+ set_irq_flags(virq, IRQF_VALID); /* kill me now */
+ return 0;
+}
+
+static struct irq_domain_ops em_gio_irq_domain_ops = {
+ .map = em_gio_irq_domain_map,
+};
+
+static int __devinit em_gio_irq_domain_init(struct em_gio_priv *p)
+{
+ struct platform_device *pdev = p->pdev;
+ struct gpio_em_config *pdata = pdev->dev.platform_data;
+
+ p->irq_base = irq_alloc_descs(pdata->irq_base, 0,
+ pdata->number_of_pins, numa_node_id());
+ if (IS_ERR_VALUE(p->irq_base)) {
+ dev_err(&pdev->dev, "cannot get irq_desc\n");
+ return -ENXIO;
+ }
+ pr_debug("gio: hw base = %d, nr = %d, sw base = %d\n",
+ pdata->gpio_base, pdata->number_of_pins, p->irq_base);
+
+ p->irq_domain = irq_domain_add_legacy(pdev->dev.of_node,
+ pdata->number_of_pins,
+ p->irq_base, 0,
+ &em_gio_irq_domain_ops, p);
+ if (!p->irq_domain) {
+ irq_free_descs(p->irq_base, pdata->number_of_pins);
+ return -ENXIO;
+ }
+
+ return 0;
+}
+
+static void __devexit em_gio_irq_domain_cleanup(struct em_gio_priv *p)
+{
+ struct gpio_em_config *pdata = p->pdev->dev.platform_data;
+
+ irq_free_descs(p->irq_base, pdata->number_of_pins);
+ /* FIXME: irq domain wants to be freed! */
+}
+
+static int __devinit em_gio_probe(struct platform_device *pdev)
+{
+ struct gpio_em_config *pdata = pdev->dev.platform_data;
+ struct em_gio_priv *p;
+ struct resource *io[2], *irq[2];
+ struct gpio_chip *gpio_chip;
+ struct irq_chip *irq_chip;
+ const char *name = dev_name(&pdev->dev);
+ int ret;
+
+ p = kzalloc(sizeof(*p), GFP_KERNEL);
+ if (!p) {
+ dev_err(&pdev->dev, "failed to allocate driver data\n");
+ ret = -ENOMEM;
+ goto err0;
+ }
+
+ p->pdev = pdev;
+ platform_set_drvdata(pdev, p);
+ spin_lock_init(&p->sense_lock);
+
+ io[0] = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ io[1] = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ irq[0] = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ irq[1] = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
+
+ if (!io[0] || !io[1] || !irq[0] || !irq[1] || !pdata) {
+ dev_err(&pdev->dev, "missing IRQ, IOMEM or configuration\n");
+ ret = -EINVAL;
+ goto err1;
+ }
+
+ p->base0 = ioremap_nocache(io[0]->start, resource_size(io[0]));
+ if (!p->base0) {
+ dev_err(&pdev->dev, "failed to remap low I/O memory\n");
+ ret = -ENXIO;
+ goto err1;
+ }
+
+ p->base1 = ioremap_nocache(io[1]->start, resource_size(io[1]));
+ if (!p->base1) {
+ dev_err(&pdev->dev, "failed to remap high I/O memory\n");
+ ret = -ENXIO;
+ goto err2;
+ }
+
+ gpio_chip = &p->gpio_chip;
+ gpio_chip->direction_input = em_gio_direction_input;
+ gpio_chip->get = em_gio_get;
+ gpio_chip->direction_output = em_gio_direction_output;
+ gpio_chip->set = em_gio_set;
+ gpio_chip->to_irq = em_gio_to_irq;
+ gpio_chip->label = name;
+ gpio_chip->owner = THIS_MODULE;
+ gpio_chip->base = pdata->gpio_base;
+ gpio_chip->ngpio = pdata->number_of_pins;
+
+ irq_chip = &p->irq_chip;
+ irq_chip->name = name;
+ irq_chip->irq_mask = em_gio_irq_disable;
+ irq_chip->irq_unmask = em_gio_irq_enable;
+ irq_chip->irq_enable = em_gio_irq_enable;
+ irq_chip->irq_disable = em_gio_irq_disable;
+ irq_chip->irq_set_type = em_gio_irq_set_type;
+ irq_chip->flags = IRQCHIP_SKIP_SET_WAKE;
+
+ ret = em_gio_irq_domain_init(p);
+ if (ret) {
+ dev_err(&pdev->dev, "cannot initialize irq domain\n");
+ goto err3;
+ }
+
+ if (request_irq(irq[0]->start, em_gio_irq_handler, 0, name, p)) {
+ dev_err(&pdev->dev, "failed to request low IRQ\n");
+ ret = -ENOENT;
+ goto err4;
+ }
+
+ if (request_irq(irq[1]->start, em_gio_irq_handler, 0, name, p)) {
+ dev_err(&pdev->dev, "failed to request high IRQ\n");
+ ret = -ENOENT;
+ goto err5;
+ }
+
+ ret = gpiochip_add(gpio_chip);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to add GPIO controller\n");
+ goto err6;
+ }
+ return 0;
+
+err6:
+ free_irq(irq[1]->start, pdev);
+err5:
+ free_irq(irq[0]->start, pdev);
+err4:
+ em_gio_irq_domain_cleanup(p);
+err3:
+ iounmap(p->base1);
+err2:
+ iounmap(p->base0);
+err1:
+ kfree(p);
+err0:
+ return ret;
+}
+
+static int __devexit em_gio_remove(struct platform_device *pdev)
+{
+ struct em_gio_priv *p = platform_get_drvdata(pdev);
+ struct resource *irq[2];
+ int ret;
+
+ ret = gpiochip_remove(&p->gpio_chip);
+ if (ret)
+ return ret;
+
+ irq[0] = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ irq[1] = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
+
+ free_irq(irq[1]->start, pdev);
+ free_irq(irq[0]->start, pdev);
+ em_gio_irq_domain_cleanup(p);
+ iounmap(p->base1);
+ iounmap(p->base0);
+ kfree(p);
+ return 0;
+}
+
+static struct platform_driver em_gio_device_driver = {
+ .probe = em_gio_probe,
+ .remove = __devexit_p(em_gio_remove),
+ .driver = {
+ .name = "em_gio",
+ }
+};
+
+module_platform_driver(em_gio_device_driver);
+
+MODULE_AUTHOR("Magnus Damm");
+MODULE_DESCRIPTION("Renesas Emma Mobile GIO Driver");
+MODULE_LICENSE("GPL v2");
--- /dev/null
+++ work/include/linux/platform_data/gpio-em.h 2012-05-15 14:50:43.000000000 +0900
@@ -0,0 +1,10 @@
+#ifndef __GPIO_EM_H__
+#define __GPIO_EM_H__
+
+struct gpio_em_config {
+ unsigned int gpio_base;
+ unsigned int irq_base;
+ unsigned int number_of_pins;
+};
+
+#endif /* __GPIO_EM_H__ */
On Wed, 2012-05-16 at 00:43 +0900, Magnus Damm wrote:
> This patch is V2 of the Emma Mobile GPIO driver.
Just some trivial comments
> +++ work/drivers/gpio/gpio-em.c 2012-05-16 00:07:20.000000000 +0900
[]
Adding pr_fmt before any #include prefixes all pr_<level> messages.
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
[]
> +static int em_gio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
[]
> + pr_debug("gio: sense irq = %d, mode = %d\n", offset, value);
pr_debug("sense irq = %d, mode = %d\n", offset, value);
[]
> +static void __em_gio_set(struct gpio_chip *chip, unsigned int reg,
> + unsigned shift, int value)
> +{
> + /* upper 16 bits contains mask and lower 16 actual value */
> + em_gio_write(gpio_to_priv(chip), reg,
> + (1 << (shift + 16)) | (value << shift));
> +}
comment doesn't seem to match code unless
value is a single bit, then value should
probably be renamed.
> +static int em_gio_irq_domain_map(struct irq_domain *h, unsigned int virq,
> + irq_hw_number_t hw)
> +{
> + struct em_gio_priv *p = h->host_data;
> +
> + pr_debug("gio: map hw irq = %d, virq = %d\n", (int)hw, virq);
pr_debug("map hw irq = %d, virq = %d\n", (int)hw, virq)
> +static struct irq_domain_ops em_gio_irq_domain_ops = {
const?
> +static int __devinit em_gio_irq_domain_init(struct em_gio_priv *p)
[]
> + pr_debug("gio: hw base = %d, nr = %d, sw base = %d\n",
> + pdata->gpio_base, pdata->number_of_pins, p->irq_base);
etc.
> +static int __devinit em_gio_probe(struct platform_device *pdev)
[]
> + p = kzalloc(sizeof(*p), GFP_KERNEL);
> + if (!p) {
> + dev_err(&pdev->dev, "failed to allocate driver data\n");
OOM messages aren't necessary as k.alloc failures
produce a dump_stack.
On Tue, May 15, 2012 at 5:43 PM, Magnus Damm <[email protected]> wrote:
> From: Magnus Damm <[email protected]>
>
> This patch is V2 of the Emma Mobile GPIO driver. This
> driver is designed to be reusable between multiple SoCs
> that share the same basic building block, but so far it
> has only been used on Emma Mobile EV2.
>
> Each driver instance handles 32 GPIOs with individually
> maskable IRQs. The driver operates on two I/O memory
> ranges and the 32 GPIOs are hooked up to two interrupts.
>
> In the case of Emma Mobile EV2 this GPIO building block
> is used as main external interrupt controller hooking up
> 159 GPIOS as 159 interrupts via 5 driver instances and
> 10 interrupts to the GIC and the Cortex-A9 Dual.
>
> Signed-off-by: Magnus Damm <[email protected]>
V2 is fair enough for me, so feel free to add:
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, May 16, 2012 at 12:43:33AM +0900, Magnus Damm wrote:
> +static int __devinit em_gio_irq_domain_init(struct em_gio_priv *p)
> +{
> + struct platform_device *pdev = p->pdev;
> + struct gpio_em_config *pdata = pdev->dev.platform_data;
> +
> + p->irq_base = irq_alloc_descs(pdata->irq_base, 0,
> + pdata->number_of_pins, numa_node_id());
> + if (IS_ERR_VALUE(p->irq_base)) {
> + dev_err(&pdev->dev, "cannot get irq_desc\n");
> + return -ENXIO;
> + }
> + pr_debug("gio: hw base = %d, nr = %d, sw base = %d\n",
> + pdata->gpio_base, pdata->number_of_pins, p->irq_base);
> +
> + p->irq_domain = irq_domain_add_legacy(pdev->dev.of_node,
> + pdata->number_of_pins,
> + p->irq_base, 0,
> + &em_gio_irq_domain_ops, p);
> + if (!p->irq_domain) {
> + irq_free_descs(p->irq_base, pdata->number_of_pins);
> + return -ENXIO;
> + }
> +
> + return 0;
> +}
> +
There's no reason to use a legacy domain here, this is a pretty
straightforward candidate for a linear map. You can use ->to_irq() for
the irq_create_mapping() invocation and then later look up the IRQ with
irq_linear_revmap().
irq_domain_add_legacy() exists for existing static ranges, which there is
really no reason to be adding in new board/platform support. You don't
have to worry about virq overlap since irq_create_mapping() already wraps
on top of irq_alloc_desc_xxx() for lookup.
> +static void __devexit em_gio_irq_domain_cleanup(struct em_gio_priv *p)
> +{
> + struct gpio_em_config *pdata = p->pdev->dev.platform_data;
> +
> + irq_free_descs(p->irq_base, pdata->number_of_pins);
> + /* FIXME: irq domain wants to be freed! */
> +}
> +
After which point you can iterate and dispose of the virq mappings with
irq_dispose_mapping(). The lack of irq domain teardown functionality does
need to be addressed though.
On Wed, May 16, 2012 at 4:29 PM, Paul Mundt <[email protected]> wrote:
> On Wed, May 16, 2012 at 12:43:33AM +0900, Magnus Damm wrote:
>> +static int __devinit em_gio_irq_domain_init(struct em_gio_priv *p)
>> +{
>> + ? ? struct platform_device *pdev = p->pdev;
>> + ? ? struct gpio_em_config *pdata = pdev->dev.platform_data;
>> +
>> + ? ? p->irq_base = irq_alloc_descs(pdata->irq_base, 0,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->number_of_pins, numa_node_id());
>> + ? ? if (IS_ERR_VALUE(p->irq_base)) {
>> + ? ? ? ? ? ? dev_err(&pdev->dev, "cannot get irq_desc\n");
>> + ? ? ? ? ? ? return -ENXIO;
>> + ? ? }
>> + ? ? pr_debug("gio: hw base = %d, nr = %d, sw base = %d\n",
>> + ? ? ? ? ? ? ?pdata->gpio_base, pdata->number_of_pins, p->irq_base);
>> +
>> + ? ? p->irq_domain = irq_domain_add_legacy(pdev->dev.of_node,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->number_of_pins,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? p->irq_base, 0,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &em_gio_irq_domain_ops, p);
>> + ? ? if (!p->irq_domain) {
>> + ? ? ? ? ? ? irq_free_descs(p->irq_base, pdata->number_of_pins);
>> + ? ? ? ? ? ? return -ENXIO;
>> + ? ? }
>> +
>> + ? ? return 0;
>> +}
>> +
> There's no reason to use a legacy domain here, this is a pretty
> straightforward candidate for a linear map. You can use ->to_irq() for
> the irq_create_mapping() invocation and then later look up the IRQ with
> irq_linear_revmap().
I believe I have my reasons for using the legacy domain, but I agree
that it is possible to use irq_create_mapping() and
irq_linear_revmap() in the case of linear mappings. Actually, it's
exactly what my DT prototype patch does. Please see this patch that
adds linear irq domain support in the DT case, and the patch
description also includes information about when the legacy irq domain
is used and when the linear one is:
http://groups.google.com/group/linux.kernel/browse_thread/thread/25a8d7e4d311ab59?pli=1
> irq_domain_add_legacy() exists for existing static ranges, which there is
> really no reason to be adding in new board/platform support. You don't
> have to worry about virq overlap since irq_create_mapping() already wraps
> on top of irq_alloc_desc_xxx() for lookup.
So I intentionally made use of the legacy domain in the non-DT case.
This because I want to let the SoC code set the static IRQ ranges via
platform data.
>
>> +static void __devexit em_gio_irq_domain_cleanup(struct em_gio_priv *p)
>> +{
>> + ? ? struct gpio_em_config *pdata = p->pdev->dev.platform_data;
>> +
>> + ? ? irq_free_descs(p->irq_base, pdata->number_of_pins);
>> + ? ? /* FIXME: irq domain wants to be freed! */
>> +}
>> +
> After which point you can iterate and dispose of the virq mappings with
> irq_dispose_mapping(). The lack of irq domain teardown functionality does
> need to be addressed though.
I can't see any GPIO driver doing that yet, but yes, that is possible.
Thanks,
/ magnus
On Wed, May 16, 2012 at 4:11 PM, Linus Walleij <[email protected]> wrote:
> On Tue, May 15, 2012 at 5:43 PM, Magnus Damm <[email protected]> wrote:
>
>> From: Magnus Damm <[email protected]>
>>
>> This patch is V2 of the Emma Mobile GPIO driver. This
>> driver is designed to be reusable between multiple SoCs
>> that share the same basic building block, but so far it
>> has only been used on Emma Mobile EV2.
>>
>> Each driver instance handles 32 GPIOs with individually
>> maskable IRQs. The driver operates on two I/O memory
>> ranges and the 32 GPIOs are hooked up to two interrupts.
>>
>> In the case of Emma Mobile EV2 this GPIO building block
>> is used as main external interrupt controller hooking up
>> 159 GPIOS as 159 interrupts via 5 driver instances and
>> 10 interrupts to the GIC and the Cortex-A9 Dual.
>>
>> Signed-off-by: Magnus Damm <[email protected]>
>
> V2 is fair enough for me, so feel free to add:
> Acked-by: Linus Walleij <[email protected]>
Thank you! Do you guys have any preferences how to merge this?
Can I include it together with the EMEV2 SoC bits perhaps? That may be
easy so we can keep track of the platform data header file dependency.
Cheers,
/ magnus
On Wed, May 16, 2012 at 12:15 PM, Magnus Damm <[email protected]> wrote:
> Do you guys have any preferences how to merge this?
>
> Can I include it together with the EMEV2 SoC bits perhaps? That may be
> easy so we can keep track of the platform data header file dependency.
For ux500 I made a special "gpio and pins" branch and sent through ARM SoC.
Yours,
Linus Walleij
On Wednesday 16 May 2012, Magnus Damm wrote:
> > irq_domain_add_legacy() exists for existing static ranges, which there is
> > really no reason to be adding in new board/platform support. You don't
> > have to worry about virq overlap since irq_create_mapping() already wraps
> > on top of irq_alloc_desc_xxx() for lookup.
>
> So I intentionally made use of the legacy domain in the non-DT case.
> This because I want to let the SoC code set the static IRQ ranges via
> platform data.
I think it's generally better to use just one code path for both cases,
if you need both DT and non-DT support, which means you would always
use irq_domain_add_legacy. Once you have the final patch to convert it
to DT, you can remove the legacy domain and just convert it to linear.
Arnd
On Wed, May 16, 2012 at 9:09 PM, Arnd Bergmann <[email protected]> wrote:
> On Wednesday 16 May 2012, Magnus Damm wrote:
>> > irq_domain_add_legacy() exists for existing static ranges, which there is
>> > really no reason to be adding in new board/platform support. You don't
>> > have to worry about virq overlap since irq_create_mapping() already wraps
>> > on top of irq_alloc_desc_xxx() for lookup.
>>
>> So I intentionally made use of the legacy domain in the non-DT case.
>> This because I want to let the SoC code set the static IRQ ranges via
>> platform data.
>
> I think it's generally better to use just one code path for both cases,
> if you need both DT and non-DT support, which means you would always
> use irq_domain_add_legacy. Once you have the final patch to convert it
> to DT, you can remove the legacy domain and just convert it to linear.
Sure, I don't mind that at all - it will make the DT bits even easier.
Thanks,
/ magnus
On Wednesday, May 16, 2012, Linus Walleij wrote:
> On Wed, May 16, 2012 at 12:15 PM, Magnus Damm <[email protected]> wrote:
>
> > Do you guys have any preferences how to merge this?
> >
> > Can I include it together with the EMEV2 SoC bits perhaps? That may be
> > easy so we can keep track of the platform data header file dependency.
>
> For ux500 I made a special "gpio and pins" branch and sent through ARM SoC.
The problem is we have a patch depending on the $subject one in the EMEV2
series and it would be better to keep them both together if that's not
a big deal.
Thanks,
Rafael
On Wed, May 16, 2012 at 1:05 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Wednesday, May 16, 2012, Linus Walleij wrote:
>> On Wed, May 16, 2012 at 12:15 PM, Magnus Damm <[email protected]> wrote:
>>
>> > Do you guys have any preferences how to merge this?
>> >
>> > Can I include it together with the EMEV2 SoC bits perhaps? That may be
>> > easy so we can keep track of the platform data header file dependency.
>>
>> For ux500 I made a special "gpio and pins" branch and sent through ARM SoC.
>
> The problem is we have a patch depending on the $subject one in the EMEV2
> series and it would be better to keep them both together if that's not
> a big deal.
Dependencies are fine, as long as they are not circular. You can
either pull in the gpio/pins branch into the EMEV2 branch, or base it
on it.
While we try to keep most dependencies one-way and common between
platforms, as long as the combinations don't grow exponentially we can
deal with a couple of reverse ones by adding a new version of the
topic branch further down the list of merges to do (see dt2 or soc2 in
arm-soc today).
-Olof
On Thursday, May 17, 2012, Olof Johansson wrote:
> On Wed, May 16, 2012 at 1:05 PM, Rafael J. Wysocki <[email protected]> wrote:
> > On Wednesday, May 16, 2012, Linus Walleij wrote:
> >> On Wed, May 16, 2012 at 12:15 PM, Magnus Damm <[email protected]> wrote:
> >>
> >> > Do you guys have any preferences how to merge this?
> >> >
> >> > Can I include it together with the EMEV2 SoC bits perhaps? That may be
> >> > easy so we can keep track of the platform data header file dependency.
> >>
> >> For ux500 I made a special "gpio and pins" branch and sent through ARM SoC.
> >
> > The problem is we have a patch depending on the $subject one in the EMEV2
> > series and it would be better to keep them both together if that's not
> > a big deal.
>
> Dependencies are fine, as long as they are not circular. You can
> either pull in the gpio/pins branch into the EMEV2 branch, or base it
> on it.
I guess I'll try to merge the gpio/pins into the EMEV2 branch.
Thanks,
Rafael
On Wed, May 16, 2012 at 3:37 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Thursday, May 17, 2012, Olof Johansson wrote:
>> On Wed, May 16, 2012 at 1:05 PM, Rafael J. Wysocki <[email protected]> wrote:
>> > On Wednesday, May 16, 2012, Linus Walleij wrote:
>> >> On Wed, May 16, 2012 at 12:15 PM, Magnus Damm <[email protected]> wrote:
>> >>
>> >> > Do you guys have any preferences how to merge this?
>> >> >
>> >> > Can I include it together with the EMEV2 SoC bits perhaps? That may be
>> >> > easy so we can keep track of the platform data header file dependency.
>> >>
>> >> For ux500 I made a special "gpio and pins" branch and sent through ARM SoC.
>> >
>> > The problem is we have a patch depending on the $subject one in the EMEV2
>> > series and it would be better to keep them both together if that's not
>> > a big deal.
>>
>> Dependencies are fine, as long as they are not circular. You can
>> either pull in the gpio/pins branch into the EMEV2 branch, or base it
>> on it.
>
> I guess I'll try to merge the gpio/pins into the EMEV2 branch.
By the way, I should have mentioned that if the dependencies are only
for building and not for context when applying patches, then it's
sufficient to let us know in the pull request so we merge the branches
in the right order when sending to Linus (so we maintain
bisectability).
-Olof
On Wed, May 16, 2012 at 12:09:03PM +0000, Arnd Bergmann wrote:
> On Wednesday 16 May 2012, Magnus Damm wrote:
> > > irq_domain_add_legacy() exists for existing static ranges, which there is
> > > really no reason to be adding in new board/platform support. You don't
> > > have to worry about virq overlap since irq_create_mapping() already wraps
> > > on top of irq_alloc_desc_xxx() for lookup.
> >
> > So I intentionally made use of the legacy domain in the non-DT case.
> > This because I want to let the SoC code set the static IRQ ranges via
> > platform data.
>
> I think it's generally better to use just one code path for both cases,
> if you need both DT and non-DT support, which means you would always
> use irq_domain_add_legacy. Once you have the final patch to convert it
> to DT, you can remove the legacy domain and just convert it to linear.
>
That's a bit short sighted. There are going to be plenty of cases where
we can tie in IRQ domains and will make use of it both with and without
DT, and there is very little cause for forcing manual irq_desc
allocation/freeing up the chain when the IRQ domain code can handle it
just fine.
The one area that I see being problematic with IRQ domains at the moment
is IORESOURCE_IRQ. In the 'legacy' cases this maps out to the hwirq,
while in the non-legacy cases it works out to a virq mapping that's
lazily inserted by of_irq_to_resource_table() in of_device_alloc().
In adding irq domain support for the sh intc subsystem I hacked up some
prototype code for doing an in-place update of IORESOURCE_IRQ resources
hanging off platform devices that does the hwirq->virq translation and it
seems to work fine, albeit hacky, and something I would rather avoid. On
the other hand, there's no need for that either if we support a 1:1 hwirq
to virq mapping where possible, which is fairly easy to do by just trying
to fetch a virq with irq_alloc_desc_at() before falling back on the
existing virq hinting logic in irq_create_mapping(). We can easily or a
flag in to the revmap type that denotes the 'static' nature of the domain
to inhibit irq_alloc_desc_from() usage in domains with 1:1 mappings.
In any event, it looks like irq domains needs some more work for the
non-DT case before it can really be useful. Creating arbitrary static
mappings to shoe-horn a driver in to DT + non-DT shape through a legacy
mapping seems pretty absurd, though.
I'll do some more hacking on it and see what I come up with, but I'm
certainly not going to be maintaining my own radix tree on the side and
wrapping in to a legacy domain for the sh intc case when all of the
support infrastructure is already extant in the irqdomain code.
Hi Joe,
On Wed, May 16, 2012 at 1:32 AM, Joe Perches <[email protected]> wrote:
> On Wed, 2012-05-16 at 00:43 +0900, Magnus Damm wrote:
>> This patch is V2 of the Emma Mobile GPIO driver.
>
> Just some trivial comments
Nice with feedback, thank you!
>> +++ work/drivers/gpio/gpio-em.c ? ? ? 2012-05-16 00:07:20.000000000 +0900
> []
>
> Adding pr_fmt before any #include prefixes all pr_<level> messages.
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Ok, good idea, will add.
>> +#include <linux/init.h>
>> +#include <linux/platform_device.h>
> []
>> +static int em_gio_irq_set_type(struct irq_data *d, unsigned int type)
>> +{
> []
>> + ? ? pr_debug("gio: sense irq = %d, mode = %d\n", offset, value);
>
> ? ? ? ?pr_debug("sense irq = %d, mode = %d\n", offset, value);
Right, with the above pr_fmt() line.
> []
>> +static void __em_gio_set(struct gpio_chip *chip, unsigned int reg,
>> + ? ? ? ? ? ? ? ? ? ? ?unsigned shift, int value)
>> +{
>> + ? ? /* upper 16 bits contains mask and lower 16 actual value */
>> + ? ? em_gio_write(gpio_to_priv(chip), reg,
>> + ? ? ? ? ? ? ? ? ?(1 << (shift + 16)) | (value << shift));
>> +}
>
> comment doesn't seem to match code unless
> value is a single bit, then value should
> probably be renamed.
Perhaps a bit odd, but the argument variable names match the GPIO API:
[include/linux/gpio.h]
..
static inline void gpio_set_value(unsigned int gpio, int value)
So I prefer to keep them as-is.
>> +static int em_gio_irq_domain_map(struct irq_domain *h, unsigned int virq,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?irq_hw_number_t hw)
>> +{
>> + ? ? struct em_gio_priv *p = h->host_data;
>> +
>> + ? ? pr_debug("gio: map hw irq = %d, virq = %d\n", (int)hw, virq);
>
> ? ? ? ?pr_debug("map hw irq = %d, virq = %d\n", (int)hw, virq)
>
>> +static struct irq_domain_ops em_gio_irq_domain_ops = {
>
> const?
Sure, why not?
>> +static int __devinit em_gio_probe(struct platform_device *pdev)
> []
>> + ? ? p = kzalloc(sizeof(*p), GFP_KERNEL);
>> + ? ? if (!p) {
>> + ? ? ? ? ? ? dev_err(&pdev->dev, "failed to allocate driver data\n");
>
> OOM messages aren't necessary as k.alloc failures
> produce a dump_stack.
Ok!
Thanks for your feedback, I will address them in a separate
incremental feature patch.
/ magnus
On Wed, 16 May 2012 15:54:16 -0700, Olof Johansson <[email protected]> wrote:
> On Wed, May 16, 2012 at 3:37 PM, Rafael J. Wysocki <[email protected]> wrote:
> > On Thursday, May 17, 2012, Olof Johansson wrote:
> >> On Wed, May 16, 2012 at 1:05 PM, Rafael J. Wysocki <[email protected]> wrote:
> >> > On Wednesday, May 16, 2012, Linus Walleij wrote:
> >> >> On Wed, May 16, 2012 at 12:15 PM, Magnus Damm <[email protected]> wrote:
> >> >>
> >> >> > Do you guys have any preferences how to merge this?
> >> >> >
> >> >> > Can I include it together with the EMEV2 SoC bits perhaps? That may be
> >> >> > easy so we can keep track of the platform data header file dependency.
> >> >>
> >> >> For ux500 I made a special "gpio and pins" branch and sent through ARM SoC.
> >> >
> >> > The problem is we have a patch depending on the $subject one in the EMEV2
> >> > series and it would be better to keep them both together if that's not
> >> > a big deal.
> >>
> >> Dependencies are fine, as long as they are not circular. You can
> >> either pull in the gpio/pins branch into the EMEV2 branch, or base it
> >> on it.
> >
> > I guess I'll try to merge the gpio/pins into the EMEV2 branch.
>
> By the way, I should have mentioned that if the dependencies are only
> for building and not for context when applying patches, then it's
> sufficient to let us know in the pull request so we merge the branches
> in the right order when sending to Linus (so we maintain
> bisectability).
Really? I don't think that works. The actually commit point will
always be unbuildable regardless of the merge order in mainline. If
there is a dependency then the dependency must be merged into the
working branch before applying the commit.
g.
On Wed, 16 May 2012 16:29:28 +0900, Paul Mundt <[email protected]> wrote:
> On Wed, May 16, 2012 at 12:43:33AM +0900, Magnus Damm wrote:
> > +static int __devinit em_gio_irq_domain_init(struct em_gio_priv *p)
> > +{
> > + struct platform_device *pdev = p->pdev;
> > + struct gpio_em_config *pdata = pdev->dev.platform_data;
> > +
> > + p->irq_base = irq_alloc_descs(pdata->irq_base, 0,
> > + pdata->number_of_pins, numa_node_id());
> > + if (IS_ERR_VALUE(p->irq_base)) {
> > + dev_err(&pdev->dev, "cannot get irq_desc\n");
> > + return -ENXIO;
> > + }
> > + pr_debug("gio: hw base = %d, nr = %d, sw base = %d\n",
> > + pdata->gpio_base, pdata->number_of_pins, p->irq_base);
> > +
> > + p->irq_domain = irq_domain_add_legacy(pdev->dev.of_node,
> > + pdata->number_of_pins,
> > + p->irq_base, 0,
> > + &em_gio_irq_domain_ops, p);
> > + if (!p->irq_domain) {
> > + irq_free_descs(p->irq_base, pdata->number_of_pins);
> > + return -ENXIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
> There's no reason to use a legacy domain here, this is a pretty
> straightforward candidate for a linear map. You can use ->to_irq() for
> the irq_create_mapping() invocation and then later look up the IRQ with
> irq_linear_revmap().
>
> irq_domain_add_legacy() exists for existing static ranges, which there is
> really no reason to be adding in new board/platform support. You don't
> have to worry about virq overlap since irq_create_mapping() already wraps
> on top of irq_alloc_desc_xxx() for lookup.
>
> > +static void __devexit em_gio_irq_domain_cleanup(struct em_gio_priv *p)
> > +{
> > + struct gpio_em_config *pdata = p->pdev->dev.platform_data;
> > +
> > + irq_free_descs(p->irq_base, pdata->number_of_pins);
> > + /* FIXME: irq domain wants to be freed! */
> > +}
> > +
> After which point you can iterate and dispose of the virq mappings with
> irq_dispose_mapping(). The lack of irq domain teardown functionality does
> need to be addressed though.
Yes, that falls firmly in the "that's a bug" category. I need to get
some time to fix it if someone doesn't beat me to it.
g.
--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
On Thu, 17 May 2012 09:41:25 +0900, Paul Mundt <[email protected]> wrote:
> On Wed, May 16, 2012 at 12:09:03PM +0000, Arnd Bergmann wrote:
> > On Wednesday 16 May 2012, Magnus Damm wrote:
> > > > irq_domain_add_legacy() exists for existing static ranges, which there is
> > > > really no reason to be adding in new board/platform support. You don't
> > > > have to worry about virq overlap since irq_create_mapping() already wraps
> > > > on top of irq_alloc_desc_xxx() for lookup.
> > >
> > > So I intentionally made use of the legacy domain in the non-DT case.
> > > This because I want to let the SoC code set the static IRQ ranges via
> > > platform data.
> >
> > I think it's generally better to use just one code path for both cases,
> > if you need both DT and non-DT support, which means you would always
> > use irq_domain_add_legacy. Once you have the final patch to convert it
> > to DT, you can remove the legacy domain and just convert it to linear.
> >
> That's a bit short sighted. There are going to be plenty of cases where
> we can tie in IRQ domains and will make use of it both with and without
> DT, and there is very little cause for forcing manual irq_desc
> allocation/freeing up the chain when the IRQ domain code can handle it
> just fine.
>
> The one area that I see being problematic with IRQ domains at the moment
> is IORESOURCE_IRQ. In the 'legacy' cases this maps out to the hwirq,
> while in the non-legacy cases it works out to a virq mapping that's
> lazily inserted by of_irq_to_resource_table() in of_device_alloc().
On a related note, I'd like to be rid of the registration-time
of_irq_to_resource_table() setup entirely and replace it with
something like a hook in platform_get_irq() to do a DT lookup when
possible. Overall I think that will work better with irq controllers
in modules and deferred probe.
> In adding irq domain support for the sh intc subsystem I hacked up some
> prototype code for doing an in-place update of IORESOURCE_IRQ resources
> hanging off platform devices that does the hwirq->virq translation and it
> seems to work fine, albeit hacky, and something I would rather avoid. On
> the other hand, there's no need for that either if we support a 1:1 hwirq
> to virq mapping where possible, which is fairly easy to do by just trying
> to fetch a virq with irq_alloc_desc_at() before falling back on the
> existing virq hinting logic in irq_create_mapping().
That's exactly what the current irq_domain code does. Search for
'hint' in the irqdomain code. It just cannot be relied upon in any
way when there is more than once irq_domain depending on which one
maps it first. Plus I'm planning to remove the 'hint' logic because I
think it just adds complexity that doesn't need to be there.
A big problem is I don't see a good place to hook into when drivers
ask for an irq number to be mapped so that the kernel knows when it
needs to allocate and remap an irq number. request_irq() is a
possibility, but it doesn't have any knowledge of whether the irq
number it is passed is a 'core static' irq number that should be 1:1
mapped to a specific hwirq, or if it was a dynamically allocated virq
number that has already been mapped. (or possibly a 1:1 number that
has been mapped by an earlier call).
hmmm...
Okay, how about this:
- get rid of the legacy map (I certainly won't cry to see it go)
- switch all legacy users to linear
- Add a new api to force-associate hwirqs and linux irqs... something like:
irq_domain_associate(struct irq_domain *d, int irq_base,
int hwirq_base, int size)
- This function will reserve irq numbers (without allocating irq_descs)
for the given range.
- When mapping, if an irq has been reserved, then allocate only that
irq_desc; otherwise do the normal irq_alloc_desc().
- In irq_request, if the irq_desc has not yet been allocated and
mapped, then do so.
This does require a number of changes in the irq_desc layer though
which aren't particularly hard but I haven't wanted to do.
Alternately (or perhaps as a stepping stone) irq_domain_associate()
could actually allocate and map irq_descs for the given range. It
isn't as memory efficient because unused irq_descs still get mapped,
but it is a whole lot simpler and easier to understand.
So, instead of irq controllers doing:
if (dt)
d = irq_domain_add_linear();
else
d = irq_domain_add_legacy();
They will do:
d = irq_domain_add_linear();
if (irq_base)
irq_domain_associate(d, irq_base, ...);
Thoughts?
> We can easily or a
> flag in to the revmap type that denotes the 'static' nature of the domain
> to inhibit irq_alloc_desc_from() usage in domains with 1:1 mappings.
>
> In any event, it looks like irq domains needs some more work for the
> non-DT case before it can really be useful. Creating arbitrary static
> mappings to shoe-horn a driver in to DT + non-DT shape through a legacy
> mapping seems pretty absurd, though.
>
> I'll do some more hacking on it and see what I come up with, but I'm
> certainly not going to be maintaining my own radix tree on the side and
> wrapping in to a legacy domain for the sh intc case when all of the
> support infrastructure is already extant in the irqdomain code.
indeed!
g.
--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
On Fri, May 18, 2012 at 3:56 PM, Grant Likely <[email protected]> wrote:
> On Wed, 16 May 2012 15:54:16 -0700, Olof Johansson <[email protected]> wrote:
>> On Wed, May 16, 2012 at 3:37 PM, Rafael J. Wysocki <[email protected]> wrote:
>> > On Thursday, May 17, 2012, Olof Johansson wrote:
>> >> On Wed, May 16, 2012 at 1:05 PM, Rafael J. Wysocki <[email protected]> wrote:
>> >> > On Wednesday, May 16, 2012, Linus Walleij wrote:
>> >> >> On Wed, May 16, 2012 at 12:15 PM, Magnus Damm <[email protected]> wrote:
>> >> >>
>> >> >> > Do you guys have any preferences how to merge this?
>> >> >> >
>> >> >> > Can I include it together with the EMEV2 SoC bits perhaps? That may be
>> >> >> > easy so we can keep track of the platform data header file dependency.
>> >> >>
>> >> >> For ux500 I made a special "gpio and pins" branch and sent through ARM SoC.
>> >> >
>> >> > The problem is we have a patch depending on the $subject one in the EMEV2
>> >> > series and it would be better to keep them both together if that's not
>> >> > a big deal.
>> >>
>> >> Dependencies are fine, as long as they are not circular. You can
>> >> either pull in the gpio/pins branch into the EMEV2 branch, or base it
>> >> on it.
>> >
>> > I guess I'll try to merge the gpio/pins into the EMEV2 branch.
>>
>> By the way, I should have mentioned that if the dependencies are only
>> for building and not for context when applying patches, then it's
>> sufficient to let us know in the pull request so we merge the branches
>> in the right order when sending to Linus (so we maintain
>> bisectability).
>
> Really? ?I don't think that works. The actually commit point will
> always be unbuildable regardless of the merge order in mainline. ?If
> there is a dependency then the dependency must be merged into the
> working branch before applying the commit.
Not if the prerequisite commit sits in a branch that is merged before
the dependent commit.
A git bisect should never end up in a situation where the second
commit is included but the first is not. Either that, or I have
completely misunderstood how it works.
Of course, this assumes that the dependency is one-way, and not mutual.
-Olof
On Fri, May 18, 2012 at 7:18 PM, Olof Johansson <[email protected]> wrote:
> On Fri, May 18, 2012 at 3:56 PM, Grant Likely <[email protected]> wrote:
>> On Wed, 16 May 2012 15:54:16 -0700, Olof Johansson <[email protected]> wrote:
>>> On Wed, May 16, 2012 at 3:37 PM, Rafael J. Wysocki <[email protected]> wrote:
>>> > On Thursday, May 17, 2012, Olof Johansson wrote:
>>> >> On Wed, May 16, 2012 at 1:05 PM, Rafael J. Wysocki <[email protected]> wrote:
>>> >> > On Wednesday, May 16, 2012, Linus Walleij wrote:
>>> >> >> On Wed, May 16, 2012 at 12:15 PM, Magnus Damm <[email protected]> wrote:
>>> >> >>
>>> >> >> > Do you guys have any preferences how to merge this?
>>> >> >> >
>>> >> >> > Can I include it together with the EMEV2 SoC bits perhaps? That may be
>>> >> >> > easy so we can keep track of the platform data header file dependency.
>>> >> >>
>>> >> >> For ux500 I made a special "gpio and pins" branch and sent through ARM SoC.
>>> >> >
>>> >> > The problem is we have a patch depending on the $subject one in the EMEV2
>>> >> > series and it would be better to keep them both together if that's not
>>> >> > a big deal.
>>> >>
>>> >> Dependencies are fine, as long as they are not circular. You can
>>> >> either pull in the gpio/pins branch into the EMEV2 branch, or base it
>>> >> on it.
>>> >
>>> > I guess I'll try to merge the gpio/pins into the EMEV2 branch.
>>>
>>> By the way, I should have mentioned that if the dependencies are only
>>> for building and not for context when applying patches, then it's
>>> sufficient to let us know in the pull request so we merge the branches
>>> in the right order when sending to Linus (so we maintain
>>> bisectability).
>>
>> Really? ?I don't think that works. The actually commit point will
>> always be unbuildable regardless of the merge order in mainline. ?If
>> there is a dependency then the dependency must be merged into the
>> working branch before applying the commit.
>
> Not if the prerequisite commit sits in a branch that is merged before
> the dependent commit.
>
> A git bisect should never end up in a situation where the second
> commit is included but the first is not. Either that, or I have
> completely misunderstood how it works.
>
> Of course, this assumes that the dependency is one-way, and not mutual.
I think either you've misunderstood, or I've misunderstood you. Sooo,
let's trot out an example:
Consider the following:
Mainline (git log --oneline --graph)
* cc88978 C
* 3fcf8cf B
* ecc927b A
Branch X:
* a3152ba X1
* 3fcf8cf B
* ecc927b A
Branch Y:
* 7ebda88 Y1
* 3fcf8cf B
* ecc927b A
Commits A and B are common on all three branches. Mainline has an
additional commit 'C', X has X1 and Y has Y1. Now; let's say commit
Y1 actually depends on commit X1, and let's say the merge order
reflects that. Mainline will now look like:
* 602fa8f E: Merge branch 'Y'
|\
| * 7ebda88 Y1
* | 14697ca D: Merge branch 'X'
|\ \
| * | a3152ba X1
| |/
* | cc88978 C
|/
* 3fcf8cf B
* ecc927b A
Right? Now if we do a git bisect:
$ git bisect bad
You need to start by "git bisect start"
Do you want me to do it for you [Y/n]? y
(master|BISECTING) ~/test/gittry$ git bisect bad HEAD
(master|BISECTING) ~/test/gittry$ git bisect good ecc927b
Bisecting: 3 revisions left to test after this (roughly 2 steps)
[a3152ba87f16b1b319869d0920e414a70cdaf2d9] X1
((a3152ba...)|BISECTING) ~/test/gittry$ git bisect good
Bisecting: 1 revision left to test after this (roughly 1 step)
[14697cab82f0ea655d0f8ee10f57f197fe69acb8] D: Merge branch 'X'
((14697ca...)|BISECTING) ~/test/gittry$ git bisect good
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[7ebda88aec6eeeda8e9264de3f22df5ac86d3c70] Y1
((7ebda88...)|BISECTING) ~/test/gittry$ git log --oneline
7ebda88 Y1
3fcf8cf B
ecc927b A
See? Y1 shows up without the X1 parent. With git, there is never a
straight line history, so every commit must be fully built on top of
all the commits it needs. (And did I understand what you were trying
to describe correctly?)
g.
On Fri, May 18, 2012 at 04:57:47PM -0600, Grant Likely wrote:
> On Wed, 16 May 2012 16:29:28 +0900, Paul Mundt <[email protected]> wrote:
> > After which point you can iterate and dispose of the virq mappings with
> > irq_dispose_mapping(). The lack of irq domain teardown functionality does
> > need to be addressed though.
>
> Yes, that falls firmly in the "that's a bug" category. I need to get
> some time to fix it if someone doesn't beat me to it.
>
I've got some code done for it already, along with a batch of other
changes. I'll have the series out for you shortly as soon as I'm done
with some additional testing.
On Fri, May 18, 2012 at 6:44 PM, Grant Likely <[email protected]> wrote:
> See? ?Y1 shows up without the X1 parent. ?With git, there is never a
> straight line history, so every commit must be fully built on top of
> all the commits it needs. ?(And did I understand what you were trying
> to describe correctly?)
You understood my intentions, but I had a misconception of how bisect
actually works.
"Fighting regressions with git bisect" visualizes pretty well why
you're right and I'm wrong as well. :)
-Olof
On Fri, May 18, 2012 at 05:25:39PM -0600, Grant Likely wrote:
> On Thu, 17 May 2012 09:41:25 +0900, Paul Mundt <[email protected]> wrote:
> > In adding irq domain support for the sh intc subsystem I hacked up some
> > prototype code for doing an in-place update of IORESOURCE_IRQ resources
> > hanging off platform devices that does the hwirq->virq translation and it
> > seems to work fine, albeit hacky, and something I would rather avoid. On
> > the other hand, there's no need for that either if we support a 1:1 hwirq
> > to virq mapping where possible, which is fairly easy to do by just trying
> > to fetch a virq with irq_alloc_desc_at() before falling back on the
> > existing virq hinting logic in irq_create_mapping().
>
> That's exactly what the current irq_domain code does. Search for
> 'hint' in the irqdomain code. It just cannot be relied upon in any
> way when there is more than once irq_domain depending on which one
> maps it first. Plus I'm planning to remove the 'hint' logic because I
> think it just adds complexity that doesn't need to be there.
>
There's a subtle functionality difference that the irqdomain code fails
to account for at present, which is the difference between the old
dynamic IRQ API (create_irq()/create_irq_nr() -- and no, not the stupid
signed vs unsigned thing). irq_alloc_desc_at() allocates at a fixed
position within the bitmap, and only at that position. If we're unable to
get the mapping we want, it's an error. irq_alloc_desc_from() simply uses
the hwirq as a hint for where to start looking, and will quite happily
hand you back whatever it finds. At present we have no way to tell the
irqdomain code to allocate a specific virq, which is what we're going to
need for converting static mappings over.
I do agree that getting rid of the hinting logic is a good idea though.
On SH we actually do our hinting the opposite, where we first populate
the IRQ map with all of our static hardware mappings, and then scan from
the beginning to find holes for when we need dynamic IRQs. This gives us
a pretty tightly packed IRQ map, which helps with lookup times, radix
tree utilization, and nr_irqs expansion for sparseirq (which we use
unconditionally on all sh + sh/r-mobile arm platforms).
> - get rid of the legacy map (I certainly won't cry to see it go)
> - switch all legacy users to linear
While this is a good direction to go, it's also important to keep in mind
that not all legacy users will have linear maps (though perhaps out of
the existing users it's one or the other). I've added a couple of API
calls to help with inserting extant mappings and establishing identity
mappings that should permit legacy users to adopt whatever mapping model
works the best for them.
Once the semantics have been agreed upon we can begin converting the
existing users before the _legacy proliferation gets too far along.
> - Add a new api to force-associate hwirqs and linux irqs... something like:
> irq_domain_associate(struct irq_domain *d, int irq_base,
> int hwirq_base, int size)
> - This function will reserve irq numbers (without allocating irq_descs)
> for the given range.
There's already an irq_reserve_irq{s,}() that can be used to inhibit
accidental mapping. We use this in the SH INTC case to ensure that all of
the possible hardware vectors are reserved before we permit dynamic IRQ
allocation throughout the remaining space (we have a flat vector space
where both static and dynamic mappings co-exist -- I plan to maintain
this behaviour in converting to IRQ domains, which should also make it
easy for DT/non-DT stuff to co-exist in the same vector space).
I've generalized irq_setup_virq() as irq_domain_associate() in order to
allow direct insertion of existing IRQs while still allowing the domain
to do whatever it likes with the mapping in its ->map() (ie, revmap
insertion).
> - When mapping, if an irq has been reserved, then allocate only that
> irq_desc; otherwise do the normal irq_alloc_desc().
This seems likely to duplicate state. We have no way to determine what
caused an IRQ to be reserved in the bitmap, or to test for reserved vs
normally allocated ones. I think we can avoid the need for this by simply
splitting in to irq_alloc_desc_at() for static and maintaining
irq_alloc_desc_from() for dynamic.
> - In irq_request, if the irq_desc has not yet been allocated and
> mapped, then do so.
>
> This does require a number of changes in the irq_desc layer though
> which aren't particularly hard but I haven't wanted to do.
>
> Alternately (or perhaps as a stepping stone) irq_domain_associate()
> could actually allocate and map irq_descs for the given range. It
> isn't as memory efficient because unused irq_descs still get mapped,
> but it is a whole lot simpler and easier to understand.
>
That's possible as well. I've taken a slightly different approach with my
patches that leave the irqdesc behaviour alone, but I'm not sure how well
it will mesh with the DT model.
On Thursday, May 17, 2012, Olof Johansson wrote:
> On Wed, May 16, 2012 at 3:37 PM, Rafael J. Wysocki <[email protected]> wrote:
> > On Thursday, May 17, 2012, Olof Johansson wrote:
> >> On Wed, May 16, 2012 at 1:05 PM, Rafael J. Wysocki <[email protected]> wrote:
> >> > On Wednesday, May 16, 2012, Linus Walleij wrote:
> >> >> On Wed, May 16, 2012 at 12:15 PM, Magnus Damm <[email protected]> wrote:
> >> >>
> >> >> > Do you guys have any preferences how to merge this?
> >> >> >
> >> >> > Can I include it together with the EMEV2 SoC bits perhaps? That may be
> >> >> > easy so we can keep track of the platform data header file dependency.
> >> >>
> >> >> For ux500 I made a special "gpio and pins" branch and sent through ARM SoC.
> >> >
> >> > The problem is we have a patch depending on the $subject one in the EMEV2
> >> > series and it would be better to keep them both together if that's not
> >> > a big deal.
> >>
> >> Dependencies are fine, as long as they are not circular. You can
> >> either pull in the gpio/pins branch into the EMEV2 branch, or base it
> >> on it.
> >
> > I guess I'll try to merge the gpio/pins into the EMEV2 branch.
>
> By the way, I should have mentioned that if the dependencies are only
> for building and not for context when applying patches, then it's
> sufficient to let us know in the pull request so we merge the branches
> in the right order when sending to Linus (so we maintain
> bisectability).
Well, eventually I have taken the GPIO patch from Magnus into the emev2 branch,
because that has been much more convenient to me (pull request sent already).
Thanks,
Rafael
On Sat, 19 May 2012 15:46:07 +0900, Paul Mundt <[email protected]> wrote:
> On Fri, May 18, 2012 at 05:25:39PM -0600, Grant Likely wrote:
> > On Thu, 17 May 2012 09:41:25 +0900, Paul Mundt <[email protected]> wrote:
> > > In adding irq domain support for the sh intc subsystem I hacked up some
> > > prototype code for doing an in-place update of IORESOURCE_IRQ resources
> > > hanging off platform devices that does the hwirq->virq translation and it
> > > seems to work fine, albeit hacky, and something I would rather avoid. On
> > > the other hand, there's no need for that either if we support a 1:1 hwirq
> > > to virq mapping where possible, which is fairly easy to do by just trying
> > > to fetch a virq with irq_alloc_desc_at() before falling back on the
> > > existing virq hinting logic in irq_create_mapping().
> >
> > That's exactly what the current irq_domain code does. Search for
> > 'hint' in the irqdomain code. It just cannot be relied upon in any
> > way when there is more than once irq_domain depending on which one
> > maps it first. Plus I'm planning to remove the 'hint' logic because I
> > think it just adds complexity that doesn't need to be there.
> >
> There's a subtle functionality difference that the irqdomain code fails
> to account for at present, which is the difference between the old
> dynamic IRQ API (create_irq()/create_irq_nr() -- and no, not the stupid
> signed vs unsigned thing). irq_alloc_desc_at() allocates at a fixed
> position within the bitmap, and only at that position. If we're unable to
> get the mapping we want, it's an error. irq_alloc_desc_from() simply uses
> the hwirq as a hint for where to start looking, and will quite happily
> hand you back whatever it finds. At present we have no way to tell the
> irqdomain code to allocate a specific virq, which is what we're going to
> need for converting static mappings over.
>
> I do agree that getting rid of the hinting logic is a good idea though.
> On SH we actually do our hinting the opposite, where we first populate
> the IRQ map with all of our static hardware mappings, and then scan from
> the beginning to find holes for when we need dynamic IRQs. This gives us
> a pretty tightly packed IRQ map, which helps with lookup times, radix
> tree utilization, and nr_irqs expansion for sparseirq (which we use
> unconditionally on all sh + sh/r-mobile arm platforms).
>
> > - get rid of the legacy map (I certainly won't cry to see it go)
> > - switch all legacy users to linear
>
> While this is a good direction to go, it's also important to keep in mind
> that not all legacy users will have linear maps (though perhaps out of
> the existing users it's one or the other). I've added a couple of API
> calls to help with inserting extant mappings and establishing identity
> mappings that should permit legacy users to adopt whatever mapping model
> works the best for them.
Mostly I'm thinking about simplification of the code. I think
irq_domain is a little too complex. I would like to be able to
simplify things my making the legacy map just a linear map that is
pre-populated. The majority of linear users seem to be in the 32-64
num_irqs range so I'm not concerned about wasting memory by allocating
a map for legacy users (and dropping the legacy mapping code reduced
.text by ~660 bytes in my test compile). :-)
>
> Once the semantics have been agreed upon we can begin converting the
> existing users before the _legacy proliferation gets too far along.
>
> > - Add a new api to force-associate hwirqs and linux irqs... something like:
> > irq_domain_associate(struct irq_domain *d, int irq_base,
> > int hwirq_base, int size)
> > - This function will reserve irq numbers (without allocating irq_descs)
> > for the given range.
>
> There's already an irq_reserve_irq{s,}() that can be used to inhibit
> accidental mapping. We use this in the SH INTC case to ensure that all of
> the possible hardware vectors are reserved before we permit dynamic IRQ
> allocation throughout the remaining space (we have a flat vector space
> where both static and dynamic mappings co-exist -- I plan to maintain
> this behaviour in converting to IRQ domains, which should also make it
> easy for DT/non-DT stuff to co-exist in the same vector space).
Yes, I was looking at irq_reserve_irqs() and thinking about how best
to fit it into irq_domain. I would like to be able to defer
allocation of irq_descs in the legacy or reserved use cases, but I'm
not sure the best way to do it. Baby steps I guess. Start with
making it work with reserved irq_descs and worry about lazy allocation
later.
> I've generalized irq_setup_virq() as irq_domain_associate() in order to
> allow direct insertion of existing IRQs while still allowing the domain
> to do whatever it likes with the mapping in its ->map() (ie, revmap
> insertion).
>
> > - When mapping, if an irq has been reserved, then allocate only that
> > irq_desc; otherwise do the normal irq_alloc_desc().
>
> This seems likely to duplicate state. We have no way to determine what
> caused an IRQ to be reserved in the bitmap, or to test for reserved vs
> normally allocated ones. I think we can avoid the need for this by simply
> splitting in to irq_alloc_desc_at() for static and maintaining
> irq_alloc_desc_from() for dynamic.
>
> > - In irq_request, if the irq_desc has not yet been allocated and
> > mapped, then do so.
> >
> > This does require a number of changes in the irq_desc layer though
> > which aren't particularly hard but I haven't wanted to do.
> >
> > Alternately (or perhaps as a stepping stone) irq_domain_associate()
> > could actually allocate and map irq_descs for the given range. It
> > isn't as memory efficient because unused irq_descs still get mapped,
> > but it is a whole lot simpler and easier to understand.
> >
> That's possible as well. I've taken a slightly different approach with my
> patches that leave the irqdesc behaviour alone, but I'm not sure how well
> it will mesh with the DT model.
I've looked at your patches I think it is still pretty similar to what
I'm suggesting above. Just the API needs to be quibbled over I think
g.
--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.