Re: [PATCH v3 4/7] ARM: mediatek: Add sysirq interrupt polarity support

Re: [PATCH v3 4/7] ARM: mediatek: Add sysirq interrupt polarity support

[Arnd Bergmann]

On Thursday 09 October 2014 22:29:37 Joe. C wrote:

> +static int __init mtk_sysirq_of_init(struct device_node *node,
> +				   struct device_node *parent)
> +{
> +	struct device_node *parent_node;
> +	struct irq_domain *domain, *domain_parent = NULL;
> +	struct mt_sysirq_chip_data *chip_data;
> +	int ret = 0;
> +
> +	parent_node = of_irq_find_parent(node);
> +	if (parent_node) {
> +		domain_parent = irq_find_host(parent_node);
> +		of_node_put(parent_node);
> +	}

Just a small comment: I think the 'parent' argument to the function
already contains the device node you are looking up here, so
no need for the second variable or _find_parent call.

This means you can also drop the  = NULL assignment for the
domain.

	Arnd

Re: [PATCH v3 4/7] ARM: mediatek: Add sysirq interrupt polarity support

[Matthias Brugger]

On 09/10/14 16:29, Joe.C wrote:
> From: "Joe.C" <yingjoe.chen@mediatek.com>
> 
> Mediatek SoCs have interrupt polarity in sysirq which allows
> to swap the polarity for given interrupts. Add this support
> using hierarchy irq domain.
> 
> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> ---
>  arch/arm/mach-mediatek/Kconfig      |   1 +
>  drivers/irqchip/Makefile            |   1 +
>  drivers/irqchip/irq-mt65xx-sysirq.c | 170 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 172 insertions(+)
>  create mode 100644 drivers/irqchip/irq-mt65xx-sysirq.c
> 
> diff --git a/arch/arm/mach-mediatek/Kconfig b/arch/arm/mach-mediatek/Kconfig
> index 2c043a2..7093859 100644
> --- a/arch/arm/mach-mediatek/Kconfig
> +++ b/arch/arm/mach-mediatek/Kconfig
> @@ -2,5 +2,6 @@ config ARCH_MEDIATEK
>  	bool "Mediatek MT6589 SoC" if ARCH_MULTI_V7
>  	select ARM_GIC
>  	select MTK_TIMER
> +	select IRQ_DOMAIN_HIERARCHY
>  	help
>  	  Support for Mediatek Cortex-A7 Quad-Core-SoC MT6589.
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 73052ba..809c9d5 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -34,3 +34,4 @@ obj-$(CONFIG_XTENSA)			+= irq-xtensa-pic.o
>  obj-$(CONFIG_XTENSA_MX)			+= irq-xtensa-mx.o
>  obj-$(CONFIG_IRQ_CROSSBAR)		+= irq-crossbar.o
>  obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o
> +obj-$(CONFIG_ARCH_MEDIATEK)		+= irq-mt65xx-sysirq.o
> diff --git a/drivers/irqchip/irq-mt65xx-sysirq.c b/drivers/irqchip/irq-mt65xx-sysirq.c
> new file mode 100644
> index 0000000..9e0eee5
> --- /dev/null
> +++ b/drivers/irqchip/irq-mt65xx-sysirq.c
> @@ -0,0 +1,170 @@
> +/*
> + * Copyright (c) 2014 MediaTek Inc.
> + * Author: Joe.C <yingjoe.chen@mediatek.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "irqchip.h"
> +
> +#define MT6577_SYS_INTPOL_NUM	(224)
> +
> +struct mt_sysirq_chip_data {
> +	spinlock_t lock;
> +	void __iomem *intpol_base;
> +};
> +
> +
> +static int mt_sysirq_set_type(struct irq_data *data, unsigned int type)

Are the mt_sysirq_ prefixed functions special to mt65xx processors? In
this case you should prefix them mt65xx_sysirq_
If not, please rename to mtk_sysirq_ to have a consistency in the kernel.

> +{
> +	irq_hw_number_t hwirq = data->hwirq;
> +	struct mt_sysirq_chip_data *chip_data = data->chip_data;
> +	u32 offset, reg_index, value;
> +	unsigned long flags;
> +	int ret;
> +
> +	offset = hwirq & 0x1f;
> +	reg_index = hwirq >> 5;
> +
> +	spin_lock_irqsave(&chip_data->lock, flags);
> +	value = readl_relaxed(chip_data->intpol_base + reg_index * 4);
> +	if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) {
> +		if (type == IRQ_TYPE_LEVEL_LOW)
> +			type = IRQ_TYPE_LEVEL_HIGH;
> +		else
> +			type = IRQ_TYPE_EDGE_RISING;
> +		value |= (1 << offset);
> +	} else
> +		value &= ~(1 << offset);
> +	writel(value, chip_data->intpol_base + reg_index * 4);
> +
> +	data = data->parent_data;
> +	ret = data->chip->irq_set_type(data, type);
> +	spin_unlock_irqrestore(&chip_data->lock, flags);
> +	return ret;
> +}
> +
> +static struct irq_chip mt_sysirq_chip = {
> +	.name			= "MT_SYSIRQ",
> +	.irq_mask		= irq_chip_mask_parent,
> +	.irq_unmask		= irq_chip_unmask_parent,
> +	.irq_eoi		= irq_chip_eoi_parent,
> +	.irq_set_type		= mt_sysirq_set_type,
> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> +};
> +
> +static int mt_sysirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				unsigned int nr_irqs, void *arg)

Same here.

> +{
> +	int i, ret;
> +	irq_hw_number_t hwirq;
> +	struct of_phandle_args *irq_data = arg;
> +	unsigned int type;
> +
> +	if (irq_data->args_count != 3)
> +		return -EINVAL;
> +
> +	hwirq = irq_data->args[1];
> +	if (irq_find_mapping(domain, hwirq) > 0)
> +		return -EEXIST;
> +
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +					&mt_sysirq_chip, domain->host_data);
> +
> +	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
> +	if (ret < 0)
> +		return ret;
> +
> +	type = irq_data->args[2] & IRQ_TYPE_SENSE_MASK;
> +
> +	/* Set type if specified and different than the current one */
> +	if (type != IRQ_TYPE_NONE &&
> +	    type != irq_get_trigger_type(virq))
> +		irq_set_irq_type(virq, type);
> +
> +	return 0;
> +}
> +
> +static void mt_sysirq_domain_free(struct irq_domain *domain, unsigned int virq,
> +				unsigned int nr_irqs)

Same here.

> +{
> +	int i;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		irq_set_handler(virq + i, NULL);
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, 0, NULL, NULL);
> +	}
> +	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
> +}
> +
> +static struct irq_domain_ops sysirq_domain_ops = {
> +	.alloc = mt_sysirq_domain_alloc,
> +	.free = mt_sysirq_domain_free,
> +};
> +
> +static int __init mtk_sysirq_of_init(struct device_node *node,
> +				   struct device_node *parent)
> +{
> +	struct device_node *parent_node;
> +	struct irq_domain *domain, *domain_parent = NULL;
> +	struct mt_sysirq_chip_data *chip_data;
> +	int ret = 0;
> +
> +	parent_node = of_irq_find_parent(node);
> +	if (parent_node) {
> +		domain_parent = irq_find_host(parent_node);
> +		of_node_put(parent_node);
> +	}
> +
> +	if (!domain_parent) {
> +		pr_err("mtk_sysirq: interrupt-parent not found\n");
> +		return -EINVAL;
> +	}
> +
> +	chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
> +	if (!chip_data)
> +		return -ENOMEM;
> +
> +	chip_data->intpol_base = of_io_request_and_map(node, 0, "intpol");
> +	if (!chip_data->intpol_base) {
> +		pr_err("mtk_sysirq: unable to map sysirq register\n");
> +		ret = -ENOMEM;
> +		goto out_free;
> +	}
> +
> +	domain = irq_domain_add_linear(node, MT6577_SYS_INTPOL_NUM,
> +				&sysirq_domain_ops, chip_data);
> +	if (!domain) {
> +		ret = -ENOMEM;
> +		goto out_unmap;
> +	}
> +	domain->parent = domain_parent;
> +	spin_lock_init(&chip_data->lock);
> +
> +	return 0;
> +
> +out_unmap:
> +	iounmap(chip_data->intpol_base);
> +out_free:
> +	kfree(chip_data);
> +	return ret;
> +}
> +IRQCHIP_DECLARE(mtk_sysirq, "mediatek,mt6577-sysirq", mtk_sysirq_of_init);

If this is compatible to all mt65xx from mt6577 upwards, we should
rename the file to irq-mt6577-sysirq.c to have consistency in the naming.

> 

Re: [PATCH v3 4/7] ARM: mediatek: Add sysirq interrupt polarity support

[Matthias Brugger]

2014-10-13 15:43 GMT+02:00 Matthias Brugger <matthias.bgg@gmail.com>:
>
>
> On 09/10/14 16:29, Joe.C wrote:
>> From: "Joe.C" <yingjoe.chen@mediatek.com>
>>
>> Mediatek SoCs have interrupt polarity in sysirq which allows
>> to swap the polarity for given interrupts. Add this support
>> using hierarchy irq domain.
>>
>> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
>> ---
>>  arch/arm/mach-mediatek/Kconfig      |   1 +
>>  drivers/irqchip/Makefile            |   1 +
>>  drivers/irqchip/irq-mt65xx-sysirq.c | 170 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 172 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-mt65xx-sysirq.c
>>
>> diff --git a/arch/arm/mach-mediatek/Kconfig b/arch/arm/mach-mediatek/Kconfig
>> index 2c043a2..7093859 100644
>> --- a/arch/arm/mach-mediatek/Kconfig
>> +++ b/arch/arm/mach-mediatek/Kconfig
>> @@ -2,5 +2,6 @@ config ARCH_MEDIATEK
>>       bool "Mediatek MT6589 SoC" if ARCH_MULTI_V7
>>       select ARM_GIC
>>       select MTK_TIMER
>> +     select IRQ_DOMAIN_HIERARCHY
>>       help
>>         Support for Mediatek Cortex-A7 Quad-Core-SoC MT6589.
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 73052ba..809c9d5 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -34,3 +34,4 @@ obj-$(CONFIG_XTENSA)                        += irq-xtensa-pic.o
>>  obj-$(CONFIG_XTENSA_MX)                      += irq-xtensa-mx.o
>>  obj-$(CONFIG_IRQ_CROSSBAR)           += irq-crossbar.o
>>  obj-$(CONFIG_BRCMSTB_L2_IRQ)         += irq-brcmstb-l2.o
>> +obj-$(CONFIG_ARCH_MEDIATEK)          += irq-mt65xx-sysirq.o
>> diff --git a/drivers/irqchip/irq-mt65xx-sysirq.c b/drivers/irqchip/irq-mt65xx-sysirq.c
>> new file mode 100644
>> index 0000000..9e0eee5
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-mt65xx-sysirq.c
>> @@ -0,0 +1,170 @@
>> +/*
>> + * Copyright (c) 2014 MediaTek Inc.
>> + * Author: Joe.C <yingjoe.chen@mediatek.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * 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.
>> + */
>> +
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +#include <linux/io.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +
>> +#include "irqchip.h"
>> +
>> +#define MT6577_SYS_INTPOL_NUM        (224)
>> +
>> +struct mt_sysirq_chip_data {
>> +     spinlock_t lock;
>> +     void __iomem *intpol_base;
>> +};
>> +
>> +
>> +static int mt_sysirq_set_type(struct irq_data *data, unsigned int type)
>
> Are the mt_sysirq_ prefixed functions special to mt65xx processors? In
> this case you should prefix them mt65xx_sysirq_
> If not, please rename to mtk_sysirq_ to have a consistency in the kernel.
>
>> +{
>> +     irq_hw_number_t hwirq = data->hwirq;
>> +     struct mt_sysirq_chip_data *chip_data = data->chip_data;
>> +     u32 offset, reg_index, value;
>> +     unsigned long flags;
>> +     int ret;
>> +
>> +     offset = hwirq & 0x1f;
>> +     reg_index = hwirq >> 5;
>> +
>> +     spin_lock_irqsave(&chip_data->lock, flags);
>> +     value = readl_relaxed(chip_data->intpol_base + reg_index * 4);
>> +     if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) {
>> +             if (type == IRQ_TYPE_LEVEL_LOW)
>> +                     type = IRQ_TYPE_LEVEL_HIGH;
>> +             else
>> +                     type = IRQ_TYPE_EDGE_RISING;
>> +             value |= (1 << offset);
>> +     } else
>> +             value &= ~(1 << offset);
>> +     writel(value, chip_data->intpol_base + reg_index * 4);
>> +
>> +     data = data->parent_data;
>> +     ret = data->chip->irq_set_type(data, type);
>> +     spin_unlock_irqrestore(&chip_data->lock, flags);
>> +     return ret;
>> +}
>> +
>> +static struct irq_chip mt_sysirq_chip = {
>> +     .name                   = "MT_SYSIRQ",
>> +     .irq_mask               = irq_chip_mask_parent,
>> +     .irq_unmask             = irq_chip_unmask_parent,
>> +     .irq_eoi                = irq_chip_eoi_parent,
>> +     .irq_set_type           = mt_sysirq_set_type,
>> +     .irq_retrigger          = irq_chip_retrigger_hierarchy,
>> +     .irq_set_affinity       = irq_chip_set_affinity_parent,
>> +};
>> +
>> +static int mt_sysirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> +                             unsigned int nr_irqs, void *arg)
>
> Same here.
>
>> +{
>> +     int i, ret;
>> +     irq_hw_number_t hwirq;
>> +     struct of_phandle_args *irq_data = arg;
>> +     unsigned int type;
>> +
>> +     if (irq_data->args_count != 3)
>> +             return -EINVAL;
>> +
>> +     hwirq = irq_data->args[1];
>> +     if (irq_find_mapping(domain, hwirq) > 0)
>> +             return -EEXIST;
>> +
>> +     for (i = 0; i < nr_irqs; i++)
>> +             irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>> +                                     &mt_sysirq_chip, domain->host_data);
>> +
>> +     ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     type = irq_data->args[2] & IRQ_TYPE_SENSE_MASK;
>> +
>> +     /* Set type if specified and different than the current one */
>> +     if (type != IRQ_TYPE_NONE &&
>> +         type != irq_get_trigger_type(virq))
>> +             irq_set_irq_type(virq, type);
>> +
>> +     return 0;
>> +}
>> +
>> +static void mt_sysirq_domain_free(struct irq_domain *domain, unsigned int virq,
>> +                             unsigned int nr_irqs)
>
> Same here.
>
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < nr_irqs; i++) {
>> +             irq_set_handler(virq + i, NULL);
>> +             irq_domain_set_hwirq_and_chip(domain, virq + i, 0, NULL, NULL);
>> +     }
>> +     irq_domain_free_irqs_parent(domain, virq, nr_irqs);
>> +}
>> +
>> +static struct irq_domain_ops sysirq_domain_ops = {
>> +     .alloc = mt_sysirq_domain_alloc,
>> +     .free = mt_sysirq_domain_free,
>> +};
>> +
>> +static int __init mtk_sysirq_of_init(struct device_node *node,
>> +                                struct device_node *parent)
>> +{
>> +     struct device_node *parent_node;
>> +     struct irq_domain *domain, *domain_parent = NULL;
>> +     struct mt_sysirq_chip_data *chip_data;
>> +     int ret = 0;
>> +
>> +     parent_node = of_irq_find_parent(node);
>> +     if (parent_node) {
>> +             domain_parent = irq_find_host(parent_node);
>> +             of_node_put(parent_node);
>> +     }
>> +
>> +     if (!domain_parent) {
>> +             pr_err("mtk_sysirq: interrupt-parent not found\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
>> +     if (!chip_data)
>> +             return -ENOMEM;
>> +
>> +     chip_data->intpol_base = of_io_request_and_map(node, 0, "intpol");
>> +     if (!chip_data->intpol_base) {
>> +             pr_err("mtk_sysirq: unable to map sysirq register\n");
>> +             ret = -ENOMEM;
>> +             goto out_free;
>> +     }
>> +
>> +     domain = irq_domain_add_linear(node, MT6577_SYS_INTPOL_NUM,
>> +                             &sysirq_domain_ops, chip_data);
>> +     if (!domain) {
>> +             ret = -ENOMEM;
>> +             goto out_unmap;
>> +     }
>> +     domain->parent = domain_parent;
>> +     spin_lock_init(&chip_data->lock);
>> +
>> +     return 0;
>> +
>> +out_unmap:
>> +     iounmap(chip_data->intpol_base);
>> +out_free:
>> +     kfree(chip_data);
>> +     return ret;
>> +}
>> +IRQCHIP_DECLARE(mtk_sysirq, "mediatek,mt6577-sysirq", mtk_sysirq_of_init);
>
> If this is compatible to all mt65xx from mt6577 upwards, we should
> rename the file to irq-mt6577-sysirq.c to have consistency in the naming.

After reviewing the patches, I see that this is compatible to mt6589
as well as to mt81xx, right?
In this case please rename the file to irq-mtk-sysirq.c
Although I think al the mt_ prefixed functions should be renamed using
mtk_ prefix.

Thanks,
Matthias

>
>>



-- 
motzblog.wordpress.com

Re: [PATCH v3 6/7] dt-bindings: add irq domain parent binding

[Mark Rutland]

Hi,

On Thu, Oct 09, 2014 at 03:29:39PM +0100, Joe.C wrote:
> From: "Joe.C" <yingjoe.chen@mediatek.com>
> 
> Add "arm,irq-domain-hierarchy" optional property.
> 
> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> ---
>  Documentation/devicetree/bindings/arm/gic.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> index c7d2fa1..f0bb759 100644
> --- a/Documentation/devicetree/bindings/arm/gic.txt
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -68,6 +68,8 @@ Example:
>  		      <0xfff10100 0x100>;
>  	};
>  
> +- arm,irq-domain-hierarchy : When this property exists, gic will be used as
> +  parent for hierarchy irq domain.

IRQ domains are a Linux internal detail, and their description doesn't
belong in the DT. So this property does not look like it should be
described in the DT.

I don't see a description of any relationship between devices here, so
it's not clear to me what this property is intended to imply.

Why do you think you need this?

Thanks,
Mark.

Re: [PATCH v3 3/7] irqchip: gic: Support hierarchy irq domain.

[Marc Zyngier]

On 09/10/14 15:29, Joe.C wrote:
> From: "Joe.C" <yingjoe.chen@mediatek.com>
> 
> Add support to use gic as a parent for stacked irq domain.
> 
> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> ---
>  drivers/irqchip/irq-gic.c | 56 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index dda6dbc..17f5aa6 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -767,19 +767,17 @@ void __init gic_init_physaddr(struct device_node *node)
>  static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
>  				irq_hw_number_t hw)
>  {
> +	irq_domain_set_hwirq_and_chip(d, irq, hw, &gic_chip, d->host_data);
>  	if (hw < 32) {
>  		irq_set_percpu_devid(irq);
> -		irq_set_chip_and_handler(irq, &gic_chip,
> -					 handle_percpu_devid_irq);
> +		irq_set_handler(irq, handle_percpu_devid_irq);
>  		set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN);
>  	} else {
> -		irq_set_chip_and_handler(irq, &gic_chip,
> -					 handle_fasteoi_irq);
> +		irq_set_handler(irq, handle_fasteoi_irq);
>  		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
>  
>  		gic_routable_irq_domain_ops->map(d, irq, hw);
>  	}
> -	irq_set_chip_data(irq, d->host_data);
>  	return 0;
>  }
>  
> @@ -795,8 +793,6 @@ static int gic_irq_domain_xlate(struct irq_domain *d,
>  {
>  	unsigned long ret = 0;
>  
> -	if (d->of_node != controller)
> -		return -EINVAL;
>  	if (intsize < 3)
>  		return -EINVAL;
>  
> @@ -839,6 +835,46 @@ static struct notifier_block gic_cpu_notifier = {
>  };
>  #endif
>  
> +
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> +static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				unsigned int nr_irqs, void *arg)
> +{
> +	int i, ret;
> +	irq_hw_number_t hwirq;
> +	unsigned int type = IRQ_TYPE_NONE;
> +	struct of_phandle_args *irq_data = arg;
> +
> +	ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args,
> +				   irq_data->args_count, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < nr_irqs; i++)
> +		gic_irq_domain_map(domain, virq+i, hwirq+i);
> +
> +	return 0;
> +}
> +
> +static void gic_irq_domain_free(struct irq_domain *domain, unsigned int virq,
> +				unsigned int nr_irqs)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		irq_set_handler(virq + i, NULL);
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, 0, NULL, NULL);
> +	}
> +}
> +
> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
> +	.alloc = gic_irq_domain_alloc,
> +	.free = gic_irq_domain_free,
> +};
> +#else
> +#define gic_irq_domain_hierarchy_ops 0
> +#endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */
> +
>  static const struct irq_domain_ops gic_irq_domain_ops = {
>  	.map = gic_irq_domain_map,
>  	.unmap = gic_irq_domain_unmap,
> @@ -952,7 +988,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>  
>  	gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
>  
> -	if (of_property_read_u32(node, "arm,routable-irqs",
> +	if (IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) &&
> +		of_find_property(node, "arm,irq-domain-hierarchy", NULL))
> +		gic->domain = irq_domain_add_linear(node, gic_irqs,
> +					&gic_irq_domain_hierarchy_ops, gic);

I really think that looking for a property is the wrong thing to do. If
"node" is non-NULL, then we're pretty sure that we're initializing from
DT, and that a pure linear domain should be the right thing, leaving the
legacy stuff for the few non-DT platforms that are still around.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 3/7] irqchip: gic: Support hierarchy irq domain.

[Arnd Bergmann]

On Thursday 09 October 2014 17:59:56 Marc Zyngier wrote:
> > @@ -952,7 +988,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> >  
> >       gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
> >  
> > -     if (of_property_read_u32(node, "arm,routable-irqs",
> > +     if (IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) &&
> > +             of_find_property(node, "arm,irq-domain-hierarchy", NULL))
> > +             gic->domain = irq_domain_add_linear(node, gic_irqs,
> > +                                     &gic_irq_domain_hierarchy_ops, gic);
> 
> I really think that looking for a property is the wrong thing to do. If
> "node" is non-NULL, then we're pretty sure that we're initializing from
> DT, and that a pure linear domain should be the right thing, leaving the
> legacy stuff for the few non-DT platforms that are still around.

FWIW, these are the remaining ones that use gic:

arch/arm/mach-cns3xxx/core.c:   gic_init(0, 29, IOMEM(CNS3XXX_TC11MP_GIC_DIST_BASE_VIRT),
arch/arm/mach-realview/realview_eb.c:           gic_init(0, 29, __io_address(REALVIEW_EB11MP_GIC_DIST_BASE),
arch/arm/mach-realview/realview_eb.c:           gic_init(1, 96, __io_address(REALVIEW_EB_GIC_DIST_BASE),
arch/arm/mach-realview/realview_eb.c:           gic_init(0, 29, __io_address(REALVIEW_EB_GIC_DIST_BASE),
arch/arm/mach-realview/realview_pb1176.c:       gic_init(0, IRQ_DC1176_GIC_START,
arch/arm/mach-realview/realview_pb1176.c:       gic_init(1, IRQ_PB1176_GIC_START,
arch/arm/mach-realview/realview_pb11mp.c:       gic_init(0, 29, __io_address(REALVIEW_TC11MP_GIC_DIST_BASE),
arch/arm/mach-realview/realview_pb11mp.c:       gic_init(1, IRQ_PB11MP_GIC_START,
arch/arm/mach-realview/realview_pba8.c: gic_init(0, IRQ_PBA8_GIC_START,
arch/arm/mach-realview/realview_pbx.c:          gic_init(0, 29, __io_address(REALVIEW_PBX_TILE_GIC_DIST_BASE),
arch/arm/mach-realview/realview_pbx.c:          gic_init(0, IRQ_PBX_GIC_START,
arch/arm/mach-shmobile/intc-sh73a0.c:   gic_init(0, 29, gic_dist_base, gic_cpu_base);
arch/arm/mach-tegra/irq.c:              gic_init(0, 29, distbase,
arch/arm/mach-vexpress/ct-ca9x4.c:      gic_init(0, 29, ioremap(A9_MPCORE_GIC_DIST, SZ_4K),

Linus Walleij is working on realview, Pawel already has a patch for vexpress, the tegra one
can just be removed and the Renesas team will deal with shmobile.

This basically leaves cns3xxx, which now has a new maintainer but I suspect we won't
have DT-only support for it too soon anyway.

	Arnd

Re: [PATCH v3 3/7] irqchip: gic: Support hierarchy irq domain.

[Marc Zyngier]

[-- Attachment #1: Type: text/plain, Size: 1612 bytes --]

On 13/10/14 11:43, Joe.C wrote:
> On Thu, 2014-10-09 at 17:59 +0100, Marc Zyngier wrote:
>> On 09/10/14 15:29, Joe.C wrote
>>> @@ -952,7 +988,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>>>  
>>>  	gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
>>>  
>>> -	if (of_property_read_u32(node, "arm,routable-irqs",
>>> +	if (IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) &&
>>> +		of_find_property(node, "arm,irq-domain-hierarchy", NULL))
>>> +		gic->domain = irq_domain_add_linear(node, gic_irqs,
>>> +					&gic_irq_domain_hierarchy_ops, gic);
>>
>> I really think that looking for a property is the wrong thing to do. If
>> "node" is non-NULL, then we're pretty sure that we're initializing from
>> DT, and that a pure linear domain should be the right thing, leaving the
>> legacy stuff for the few non-DT platforms that are still around.
>>
>> Thanks,
>>
>> 	M.
> 
> The only reason I introduce "arm,irq-domain-hierarchy" property is
> trying to keep original behavior when hierarchy irq domain is not used.
> Without this, when a board init GIC with DT, all driver will have to use
> devicetree. I'm not sure we want to break things like this.

I don't think we want to support a "middle of the road" setup, where the
GIC is probed by DT, but some devices have hardcoded interrupts.

> I will remove this and just use linear for all DT in my next version.

I came up with the attached patch, which allows me to boot my test
platform (together with the other fix I posted earlier).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-fixup-irqchip-gic-Support-hierarchy-irq-domain.patch --]
[-- Type: text/x-diff;  name=0001-fixup-irqchip-gic-Support-hierarchy-irq-domain.patch, Size: 3828 bytes --]

>From 97d4ea1f0922fb47dd1b09cd2694b7fa5b519db9 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Mon, 13 Oct 2014 10:57:28 +0100
Subject: [PATCH] fixup! irqchip: gic: Support hierarchy irq domain.

---
 drivers/irqchip/Kconfig   |  1 +
 drivers/irqchip/irq-gic.c | 53 ++++++++++++++++++++++-------------------------
 2 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index b8632bf..2a48e0a 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -5,6 +5,7 @@ config IRQCHIP
 config ARM_GIC
 	bool
 	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
 	select MULTI_IRQ_HANDLER
 
 config GIC_NON_BANKED
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 17f5aa6..a99c211 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -835,8 +835,6 @@ static struct notifier_block gic_cpu_notifier = {
 };
 #endif
 
-
-#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
 static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 				unsigned int nr_irqs, void *arg)
 {
@@ -870,10 +868,8 @@ static void gic_irq_domain_free(struct irq_domain *domain, unsigned int virq,
 static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
 	.alloc = gic_irq_domain_alloc,
 	.free = gic_irq_domain_free,
+	.xlate = gic_irq_domain_xlate,
 };
-#else
-#define gic_irq_domain_hierarchy_ops 0
-#endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */
 
 static const struct irq_domain_ops gic_irq_domain_ops = {
 	.map = gic_irq_domain_map,
@@ -965,18 +961,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 		gic_cpu_map[i] = 0xff;
 
 	/*
-	 * For primary GICs, skip over SGIs.
-	 * For secondary GICs, skip over PPIs, too.
-	 */
-	if (gic_nr == 0 && (irq_start & 31) > 0) {
-		hwirq_base = 16;
-		if (irq_start != -1)
-			irq_start = (irq_start & ~31) + 16;
-	} else {
-		hwirq_base = 32;
-	}
-
-	/*
 	 * Find out how many interrupts are supported.
 	 * The GIC only supports up to 1020 interrupt sources.
 	 */
@@ -986,14 +970,31 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 		gic_irqs = 1020;
 	gic->gic_irqs = gic_irqs;
 
-	gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
+	if (node) {		/* DT case */
+		const struct irq_domain_ops *ops = &gic_irq_domain_hierarchy_ops;
+
+		if (!of_property_read_u32(node, "arm,routable-irqs",
+					  &nr_routable_irqs)) {
+			ops = &gic_irq_domain_ops;
+			gic_irqs = nr_routable_irqs;
+		}
+
+		gic->domain = irq_domain_add_linear(node, gic_irqs, ops, gic);
+	} else {		/* Non-DT case */
+		/*
+		 * For primary GICs, skip over SGIs.
+		 * For secondary GICs, skip over PPIs, too.
+		 */
+		if (gic_nr == 0 && (irq_start & 31) > 0) {
+			hwirq_base = 16;
+			if (irq_start != -1)
+				irq_start = (irq_start & ~31) + 16;
+		} else {
+			hwirq_base = 32;
+		}
+
+		gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
 
-	if (IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) &&
-		of_find_property(node, "arm,irq-domain-hierarchy", NULL))
-		gic->domain = irq_domain_add_linear(node, gic_irqs,
-					&gic_irq_domain_hierarchy_ops, gic);
-	else if (of_property_read_u32(node, "arm,routable-irqs",
-				 &nr_routable_irqs)) {
 		irq_base = irq_alloc_descs(irq_start, 16, gic_irqs,
 					   numa_node_id());
 		if (IS_ERR_VALUE(irq_base)) {
@@ -1004,10 +1005,6 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 
 		gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base,
 					hwirq_base, &gic_irq_domain_ops, gic);
-	} else {
-		gic->domain = irq_domain_add_linear(node, nr_routable_irqs,
-						    &gic_irq_domain_ops,
-						    gic);
 	}
 
 	if (WARN_ON(!gic->domain))
-- 
2.0.4

Re: [PATCH v3 3/7] irqchip: gic: Support hierarchy irq domain.

[Arnd Bergmann]

On Monday 13 October 2014 13:10:32 Marc Zyngier wrote:
> On 13/10/14 11:43, Joe.C wrote:
> > On Thu, 2014-10-09 at 17:59 +0100, Marc Zyngier wrote:
> >> On 09/10/14 15:29, Joe.C wrote
> >>> @@ -952,7 +988,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> >>>  
> >>>     gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
> >>>  
> >>> -   if (of_property_read_u32(node, "arm,routable-irqs",
> >>> +   if (IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) &&
> >>> +           of_find_property(node, "arm,irq-domain-hierarchy", NULL))
> >>> +           gic->domain = irq_domain_add_linear(node, gic_irqs,
> >>> +                                   &gic_irq_domain_hierarchy_ops, gic);
> >>
> >> I really think that looking for a property is the wrong thing to do. If
> >> "node" is non-NULL, then we're pretty sure that we're initializing from
> >> DT, and that a pure linear domain should be the right thing, leaving the
> >> legacy stuff for the few non-DT platforms that are still around.
> >>
> >> Thanks,
> >>
> >>      M.
> > 
> > The only reason I introduce "arm,irq-domain-hierarchy" property is
> > trying to keep original behavior when hierarchy irq domain is not used.
> > Without this, when a board init GIC with DT, all driver will have to use
> > devicetree. I'm not sure we want to break things like this.
> 
> I don't think we want to support a "middle of the road" setup, where the
> GIC is probed by DT, but some devices have hardcoded interrupts.

Agreed. We should work on making GIC DT-only by converting the few remaining
users instead, and certainly should not add any new board files that
might use the domain hierarchy code.

	Arnd

Re: [PATCH v3 3/7] irqchip: gic: Support hierarchy irq domain.

[Marc Zyngier]

On 09/10/14 15:29, Joe.C wrote:
> From: "Joe.C" <yingjoe.chen@mediatek.com>
> 
> Add support to use gic as a parent for stacked irq domain.
> 
> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> ---
>  drivers/irqchip/irq-gic.c | 56 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index dda6dbc..17f5aa6 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -767,19 +767,17 @@ void __init gic_init_physaddr(struct device_node *node)
>  static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
>  				irq_hw_number_t hw)
>  {
> +	irq_domain_set_hwirq_and_chip(d, irq, hw, &gic_chip, d->host_data);
>  	if (hw < 32) {
>  		irq_set_percpu_devid(irq);
> -		irq_set_chip_and_handler(irq, &gic_chip,
> -					 handle_percpu_devid_irq);
> +		irq_set_handler(irq, handle_percpu_devid_irq);
>  		set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN);
>  	} else {
> -		irq_set_chip_and_handler(irq, &gic_chip,
> -					 handle_fasteoi_irq);
> +		irq_set_handler(irq, handle_fasteoi_irq);
>  		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
>  
>  		gic_routable_irq_domain_ops->map(d, irq, hw);
>  	}
> -	irq_set_chip_data(irq, d->host_data);
>  	return 0;
>  }
>  
> @@ -795,8 +793,6 @@ static int gic_irq_domain_xlate(struct irq_domain *d,
>  {
>  	unsigned long ret = 0;
>  
> -	if (d->of_node != controller)
> -		return -EINVAL;
>  	if (intsize < 3)
>  		return -EINVAL;
>  
> @@ -839,6 +835,46 @@ static struct notifier_block gic_cpu_notifier = {
>  };
>  #endif
>  
> +
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> +static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				unsigned int nr_irqs, void *arg)
> +{
> +	int i, ret;
> +	irq_hw_number_t hwirq;
> +	unsigned int type = IRQ_TYPE_NONE;
> +	struct of_phandle_args *irq_data = arg;
> +
> +	ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args,
> +				   irq_data->args_count, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < nr_irqs; i++)
> +		gic_irq_domain_map(domain, virq+i, hwirq+i);
> +
> +	return 0;
> +}
> +
> +static void gic_irq_domain_free(struct irq_domain *domain, unsigned int virq,
> +				unsigned int nr_irqs)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		irq_set_handler(virq + i, NULL);
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, 0, NULL, NULL);
> +	}
> +}
> +
> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
> +	.alloc = gic_irq_domain_alloc,
> +	.free = gic_irq_domain_free,
> +};
> +#else
> +#define gic_irq_domain_hierarchy_ops 0
> +#endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */
> +
>  static const struct irq_domain_ops gic_irq_domain_ops = {
>  	.map = gic_irq_domain_map,
>  	.unmap = gic_irq_domain_unmap,
> @@ -952,7 +988,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>  
>  	gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
>  
> -	if (of_property_read_u32(node, "arm,routable-irqs",
> +	if (IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) &&
> +		of_find_property(node, "arm,irq-domain-hierarchy", NULL))
> +		gic->domain = irq_domain_add_linear(node, gic_irqs,
> +					&gic_irq_domain_hierarchy_ops, gic);
> +	else if (of_property_read_u32(node, "arm,routable-irqs",
>  				 &nr_routable_irqs)) {
>  		irq_base = irq_alloc_descs(irq_start, 16, gic_irqs,
>  					   numa_node_id());
> 

So I've been playing with this over the weekend (with quite a few
tweaks), and I'm hitting a not-so-nice effect of the automatic platform
device creation from the device tree.

What happens is the following:
- Kernel starts
- GIC gets initialized with a linear domain supporting hierarchy
- per-cpu timers are up and running
- platform devices get created from the device tree:
  - irq_of_parse_and_map()
  - irq_create_of_mapping()
  - irq_domain_alloc_irqs()

Here, we start re-allocating interrupts that have already been allocated
(the timer interrupts). This has a side effect of nuking the
percpu_dev_id, and everything explodes on the next timer tick. Grmbl.

The main issue here is that we have a single path that:
- translates the interrupt from DT to HW
- configures the interrupts
and that we use it more than once.

The non-hierarchy path works because the the "map" operation takes place
only once, and virtual interrupts are allocated upfront.

When we switch to this more dynamic way of doing things, the fact that
the virqs only available at allocation time is screwing us up.

I can see a way out of this, which would involve having a way of
detecting that a hwirq has already been allocated (which requires having
the xlate callback instantiated). Something like this (not even
compile-tested):

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index dd8d3ab..6a45821 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -479,6 +479,21 @@ unsigned int irq_create_of_mapping(struct
of_phandle_args *irq_data)
 	}

 	if (irq_domain_is_hierarchy(domain)) {
+		if (domain->ops->xlate) {
+			/*
+			 * If we've already configured this interrupt,
+			 * don't do it again, or hell will break loose.
+			 */
+			if (domain->ops->xlate(domain, irq_data->np,
+					       irq_data->args,
+					       irq_data->args_count,
+					       &hwirq, &type))
+				return 0;
+
+			virq = irq_find_mapping(domain, hwirq);
+			if (virq)
+				return virq;
+		}
 		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, irq_data);
 		return virq <= 0 ? 0 : virq;
 	}

Thoughts?

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 3/7] irqchip: gic: Support hierarchy irq domain.

[Marc Zyngier]

On 13/10/14 09:56, Marc Zyngier wrote:
> On 09/10/14 15:29, Joe.C wrote:
>> From: "Joe.C" <yingjoe.chen@mediatek.com>
>>
>> Add support to use gic as a parent for stacked irq domain.
>>
>> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
>> ---
>>  drivers/irqchip/irq-gic.c | 56 ++++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 48 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index dda6dbc..17f5aa6 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -767,19 +767,17 @@ void __init gic_init_physaddr(struct device_node *node)
>>  static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
>>  				irq_hw_number_t hw)
>>  {
>> +	irq_domain_set_hwirq_and_chip(d, irq, hw, &gic_chip, d->host_data);
>>  	if (hw < 32) {
>>  		irq_set_percpu_devid(irq);
>> -		irq_set_chip_and_handler(irq, &gic_chip,
>> -					 handle_percpu_devid_irq);
>> +		irq_set_handler(irq, handle_percpu_devid_irq);
>>  		set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN);
>>  	} else {
>> -		irq_set_chip_and_handler(irq, &gic_chip,
>> -					 handle_fasteoi_irq);
>> +		irq_set_handler(irq, handle_fasteoi_irq);
>>  		set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
>>  
>>  		gic_routable_irq_domain_ops->map(d, irq, hw);
>>  	}
>> -	irq_set_chip_data(irq, d->host_data);
>>  	return 0;
>>  }
>>  
>> @@ -795,8 +793,6 @@ static int gic_irq_domain_xlate(struct irq_domain *d,
>>  {
>>  	unsigned long ret = 0;
>>  
>> -	if (d->of_node != controller)
>> -		return -EINVAL;
>>  	if (intsize < 3)
>>  		return -EINVAL;
>>  
>> @@ -839,6 +835,46 @@ static struct notifier_block gic_cpu_notifier = {
>>  };
>>  #endif
>>  
>> +
>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>> +static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> +				unsigned int nr_irqs, void *arg)
>> +{
>> +	int i, ret;
>> +	irq_hw_number_t hwirq;
>> +	unsigned int type = IRQ_TYPE_NONE;
>> +	struct of_phandle_args *irq_data = arg;
>> +
>> +	ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args,
>> +				   irq_data->args_count, &hwirq, &type);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < nr_irqs; i++)
>> +		gic_irq_domain_map(domain, virq+i, hwirq+i);
>> +
>> +	return 0;
>> +}
>> +
>> +static void gic_irq_domain_free(struct irq_domain *domain, unsigned int virq,
>> +				unsigned int nr_irqs)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < nr_irqs; i++) {
>> +		irq_set_handler(virq + i, NULL);
>> +		irq_domain_set_hwirq_and_chip(domain, virq + i, 0, NULL, NULL);
>> +	}
>> +}
>> +
>> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
>> +	.alloc = gic_irq_domain_alloc,
>> +	.free = gic_irq_domain_free,
>> +};
>> +#else
>> +#define gic_irq_domain_hierarchy_ops 0
>> +#endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */
>> +
>>  static const struct irq_domain_ops gic_irq_domain_ops = {
>>  	.map = gic_irq_domain_map,
>>  	.unmap = gic_irq_domain_unmap,
>> @@ -952,7 +988,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>>  
>>  	gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */
>>  
>> -	if (of_property_read_u32(node, "arm,routable-irqs",
>> +	if (IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) &&
>> +		of_find_property(node, "arm,irq-domain-hierarchy", NULL))
>> +		gic->domain = irq_domain_add_linear(node, gic_irqs,
>> +					&gic_irq_domain_hierarchy_ops, gic);
>> +	else if (of_property_read_u32(node, "arm,routable-irqs",
>>  				 &nr_routable_irqs)) {
>>  		irq_base = irq_alloc_descs(irq_start, 16, gic_irqs,
>>  					   numa_node_id());
>>
> 
> So I've been playing with this over the weekend (with quite a few
> tweaks), and I'm hitting a not-so-nice effect of the automatic platform
> device creation from the device tree.
> 
> What happens is the following:
> - Kernel starts
> - GIC gets initialized with a linear domain supporting hierarchy
> - per-cpu timers are up and running
> - platform devices get created from the device tree:
>   - irq_of_parse_and_map()
>   - irq_create_of_mapping()
>   - irq_domain_alloc_irqs()
> 
> Here, we start re-allocating interrupts that have already been allocated
> (the timer interrupts). This has a side effect of nuking the
> percpu_dev_id, and everything explodes on the next timer tick. Grmbl.
> 
> The main issue here is that we have a single path that:
> - translates the interrupt from DT to HW
> - configures the interrupts
> and that we use it more than once.
> 
> The non-hierarchy path works because the the "map" operation takes place
> only once, and virtual interrupts are allocated upfront.
> 
> When we switch to this more dynamic way of doing things, the fact that
> the virqs only available at allocation time is screwing us up.
> 
> I can see a way out of this, which would involve having a way of
> detecting that a hwirq has already been allocated (which requires having
> the xlate callback instantiated). Something like this (not even
> compile-tested):
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index dd8d3ab..6a45821 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -479,6 +479,21 @@ unsigned int irq_create_of_mapping(struct
> of_phandle_args *irq_data)
>  	}
> 
>  	if (irq_domain_is_hierarchy(domain)) {
> +		if (domain->ops->xlate) {
> +			/*
> +			 * If we've already configured this interrupt,
> +			 * don't do it again, or hell will break loose.
> +			 */
> +			if (domain->ops->xlate(domain, irq_data->np,
> +					       irq_data->args,
> +					       irq_data->args_count,
> +					       &hwirq, &type))
> +				return 0;
> +
> +			virq = irq_find_mapping(domain, hwirq);
> +			if (virq)
> +				return virq;
> +		}
>  		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, irq_data);
>  		return virq <= 0 ? 0 : virq;
>  	}
> 
> Thoughts?

For the record, I just booted a pure DT system with the above patch, and
everything seems to be fine. Haven't completely convinced myself this is
the right fix though.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 3/7] irqchip: gic: Support hierarchy irq domain.

[Arnd Bergmann]

On Monday 13 October 2014 09:56:20 Marc Zyngier wrote:
>         if (irq_domain_is_hierarchy(domain)) {
> +               if (domain->ops->xlate) {
> +                       /*
> +                        * If we've already configured this interrupt,
> +                        * don't do it again, or hell will break loose.
> +                        */
> +                       if (domain->ops->xlate(domain, irq_data->np,
> +                                              irq_data->args,
> +                                              irq_data->args_count,
> +                                              &hwirq, &type))
> +                               return 0;
> +
> +                       virq = irq_find_mapping(domain, hwirq);
> +                       if (virq)
> +                               return virq;
> +               }
>                 virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, irq_data);
>                 return virq <= 0 ? 0 : virq;
>         }
> 
> Thoughts?

Using irq_find_mapping() first is probably the right approach, that
is what irq_create_mapping() does too, and I suppose we want those
to be symmetric.

mt_sysirq_domain_alloc() in patch 4 has the irq_find_domain check
in it, which I guess we can remove when it has moved to the common
code.

I don't see irq_domain_alloc_irqs() in linux-next or older kernels, where
does that get introduced?

	Arnd

Re: [PATCH v3 3/7] irqchip: gic: Support hierarchy irq domain.

[Marc Zyngier]

On 13/10/14 10:27, Arnd Bergmann wrote:
> On Monday 13 October 2014 09:56:20 Marc Zyngier wrote:
>>         if (irq_domain_is_hierarchy(domain)) {
>> +               if (domain->ops->xlate) {
>> +                       /*
>> +                        * If we've already configured this interrupt,
>> +                        * don't do it again, or hell will break loose.
>> +                        */
>> +                       if (domain->ops->xlate(domain, irq_data->np,
>> +                                              irq_data->args,
>> +                                              irq_data->args_count,
>> +                                              &hwirq, &type))
>> +                               return 0;
>> +
>> +                       virq = irq_find_mapping(domain, hwirq);
>> +                       if (virq)
>> +                               return virq;
>> +               }
>>                 virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, irq_data);
>>                 return virq <= 0 ? 0 : virq;
>>         }
>>
>> Thoughts?
> 
> Using irq_find_mapping() first is probably the right approach, that
> is what irq_create_mapping() does too, and I suppose we want those
> to be symmetric.

Ah, good point. I somehow missed that.

> mt_sysirq_domain_alloc() in patch 4 has the irq_find_domain check
> in it, which I guess we can remove when it has moved to the common
> code.
> 
> I don't see irq_domain_alloc_irqs() in linux-next or older kernels, where
> does that get introduced?

This is part of Jiang's domain hierarchy series:
https://patchwork.ozlabs.org/patch/388279/

which I plan to use to get rid of the ugly gic_extn hack that only Tegra
uses (but that everyone tries to abuse), and also for the GICv2m support.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 1/7] irqdomain: Fix irq_domain_alloc_irqs return check.

[Marc Zyngier]

On 09/10/14 15:29, Joe.C wrote:
> From: "Joe.C" <yingjoe.chen@mediatek.com>
> 
> Change virq type from unsigned int to int. Otherwise the return value
> check for irq_domain_alloc_irqs will always pass.
> 
> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
> ---
>  kernel/irq/irqdomain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 584be46..dd8d3ab 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -469,7 +469,7 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
>  	struct irq_domain *domain;
>  	irq_hw_number_t hwirq;
>  	unsigned int type = IRQ_TYPE_NONE;
> -	unsigned int virq;
> +	int virq;
>  
>  	domain = irq_data->np ? irq_find_host(irq_data->np) : irq_default_domain;
>  	if (!domain) {
> 

I'd expect this patch to get folded into Jiang's series.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 1/7] irqdomain: Fix irq_domain_alloc_irqs return check.

[Jiang Liu]

On 2014/10/13 20:11, Marc Zyngier wrote:
> On 09/10/14 15:29, Joe.C wrote:
>> From: "Joe.C" <yingjoe.chen@mediatek.com>
>>
>> Change virq type from unsigned int to int. Otherwise the return value
>> check for irq_domain_alloc_irqs will always pass.
>>
>> Signed-off-by: Joe.C <yingjoe.chen@mediatek.com>
>> ---
>>  kernel/irq/irqdomain.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index 584be46..dd8d3ab 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -469,7 +469,7 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
>>  	struct irq_domain *domain;
>>  	irq_hw_number_t hwirq;
>>  	unsigned int type = IRQ_TYPE_NONE;
>> -	unsigned int virq;
>> +	int virq;
>>  
>>  	domain = irq_data->np ? irq_find_host(irq_data->np) : irq_default_domain;
>>  	if (!domain) {
>>
> 
> I'd expect this patch to get folded into Jiang's series.
Hi Mark and Joe,
	Yes, my next version includes this fixup.
Regards!
Gerry
> 
> Thanks,
> 
> 	M.
>