* [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.
2014-11-19 14:14 [PATCH v7 0/4] ARM: mediatek: Add support for interrupt polarity Yingjoe Chen
@ 2014-11-19 14:14 ` Yingjoe Chen
2014-11-19 17:18 ` Marc Zyngier
2014-11-19 14:14 ` [PATCH v7 2/4] ARM: mediatek: Add sysirq interrupt polarity support Yingjoe Chen
` (2 subsequent siblings)
3 siblings, 1 reply; 19+ messages in thread
From: Yingjoe Chen @ 2014-11-19 14:14 UTC (permalink / raw)
To: Thomas Gleixner, Jiang Liu, Marc Zyngier
Cc: Mark Rutland, Boris BREZILLON, Russell King, Jason Cooper,
Pawel Moll, devicetree, hc.yen, srv_heupstream, yh.chen,
linux-kernel, Grant Likely, Yijing Wang, Rob Herring,
nathan.chung, yingjoe.chen, Matthias Brugger, Yingjoe Chen,
eddie.huang, Bjorn Helgaas, Sascha Hauer, linux-arm-kernel
Add support to use gic as a parent for stacked irq domain.
Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
drivers/irqchip/Kconfig | 1 +
drivers/irqchip/irq-gic.c | 78 ++++++++++++++++++++++++++++++++---------------
2 files changed, 55 insertions(+), 24 deletions(-)
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index b21f12f..7f34138 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 38493ff..464dd53 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -788,17 +788,16 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
{
if (hw < 32) {
irq_set_percpu_devid(irq);
- irq_set_chip_and_handler(irq, &gic_chip,
- handle_percpu_devid_irq);
+ irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
+ handle_percpu_devid_irq, NULL, NULL);
set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN);
} else {
- irq_set_chip_and_handler(irq, &gic_chip,
- handle_fasteoi_irq);
+ irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
+ handle_fasteoi_irq, NULL, NULL);
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;
}
@@ -858,6 +857,31 @@ static struct notifier_block gic_cpu_notifier = {
};
#endif
+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 const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
+ .xlate = gic_irq_domain_xlate,
+ .alloc = gic_irq_domain_alloc,
+ .free = irq_domain_free_irqs_top,
+};
+
static const struct irq_domain_ops gic_irq_domain_ops = {
.map = gic_irq_domain_map,
.unmap = gic_irq_domain_unmap,
@@ -948,18 +972,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.
*/
@@ -969,10 +981,32 @@ 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 (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)) {
@@ -983,10 +1017,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))
--
1.8.1.1.dirty
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.
2014-11-19 14:14 ` [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain Yingjoe Chen
@ 2014-11-19 17:18 ` Marc Zyngier
2014-11-20 3:57 ` Yingjoe Chen
2014-11-20 4:26 ` Jiang Liu
0 siblings, 2 replies; 19+ messages in thread
From: Marc Zyngier @ 2014-11-19 17:18 UTC (permalink / raw)
To: Yingjoe Chen
Cc: Thomas Gleixner, Jiang Liu, Mark Rutland, Boris BREZILLON,
Russell King, Jason Cooper, Pawel Moll, devicetree, hc.yen,
srv_heupstream, yh.chen, linux-kernel, grant.likely, Yijing Wang,
Rob Herring, nathan.chung, yingjoe.chen, Matthias Brugger,
eddie.huang, Bjorn Helgaas, Sascha Hauer,
linux- arm-kernel@lists.infradead.org
Hi Yingjoe,
On Wed, Nov 19 2014 at 2:14:08 pm GMT, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
> Add support to use gic as a parent for stacked irq domain.
>
> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
> drivers/irqchip/Kconfig | 1 +
> drivers/irqchip/irq-gic.c | 78 ++++++++++++++++++++++++++++++++---------------
> 2 files changed, 55 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index b21f12f..7f34138 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 38493ff..464dd53 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -788,17 +788,16 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
> {
> if (hw < 32) {
> irq_set_percpu_devid(irq);
> - irq_set_chip_and_handler(irq, &gic_chip,
> - handle_percpu_devid_irq);
> + irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
> + handle_percpu_devid_irq, NULL, NULL);
> set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN);
> } else {
> - irq_set_chip_and_handler(irq, &gic_chip,
> - handle_fasteoi_irq);
> + irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
> + handle_fasteoi_irq, NULL, NULL);
> 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;
> }
>
> @@ -858,6 +857,31 @@ static struct notifier_block gic_cpu_notifier = {
> };
> #endif
>
> +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);
nit: spacing around '+'.
> +
> + return 0;
> +}
> +
> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
> + .xlate = gic_irq_domain_xlate,
> + .alloc = gic_irq_domain_alloc,
> + .free = irq_domain_free_irqs_top,
I'm convinced that irq_domain_free_irqs_top is the wrong function to
call here, because you're calling it from the bottom, not the top-level
(it has no parent).
I cannot verify this with your code as I don't a working platform with
GICv2m, but if I enable something similar on GICv3, it dies a very
painful way:
Unable to handle kernel NULL pointer dereference at virtual address 00000018
pgd = ffffffc03d059000
[00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000
Internal error: Oops: 96000006 [#1] SMP
Modules linked in:
CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
PC is at irq_domain_free_irqs_recursive+0x1c/0x80
LR is at irq_domain_free_irqs_common+0x88/0x9c
pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
[...]
[<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
[<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
[<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <-- gic_domain.free()
[<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
[<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
[<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
[<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
[<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
[<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
[<ffffffc0000ef518>] msi_domain_free+0x70/0x88
[<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
[<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
[<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
[<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
[<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
[...]
and I cannot see how this could work on the standard GIC either.
Thomas, Jiang: could you please confirm or infirm my suspicions? My
understanding is that irq_domain_free_irqs_top can only be called from
the top-level domain.
> +};
> +
> static const struct irq_domain_ops gic_irq_domain_ops = {
> .map = gic_irq_domain_map,
> .unmap = gic_irq_domain_unmap,
> @@ -948,18 +972,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.
> */
> @@ -969,10 +981,32 @@ 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;
nit: please put this on the same line, even if it is longer than 80
characters.
> +
> + 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 (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)) {
> @@ -983,10 +1017,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))
Thanks,
M.
--
Jazz is not dead. It just smells funny.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.
2014-11-19 17:18 ` Marc Zyngier
@ 2014-11-20 3:57 ` Yingjoe Chen
2014-11-20 9:41 ` Yingjoe Chen
2014-11-20 4:26 ` Jiang Liu
1 sibling, 1 reply; 19+ messages in thread
From: Yingjoe Chen @ 2014-11-20 3:57 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Jiang Liu, Mark Rutland, Boris BREZILLON,
Russell King, Jason Cooper, Pawel Moll, devicetree, hc.yen,
srv_heupstream, yh.chen, linux-kernel, grant.likely, Yijing Wang,
Rob Herring, nathan.chung, yingjoe.chen, Matthias Brugger,
eddie.huang, Bjorn Helgaas, Sascha Hauer,
linux- arm-kernel@lists.infradead.org
Hi Marc,
On Wed, 2014-11-19 at 17:18 +0000, Marc Zyngier wrote:
> > +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);
>
> nit: spacing around '+'.
OK.
> > +
> > + return 0;
> > +}
> > +
> > +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
> > + .xlate = gic_irq_domain_xlate,
> > + .alloc = gic_irq_domain_alloc,
> > + .free = irq_domain_free_irqs_top,
>
> I'm convinced that irq_domain_free_irqs_top is the wrong function to
> call here, because you're calling it from the bottom, not the top-level
> (it has no parent).
Base on the name, I though this is helper function for top level
irq_domain?
> I cannot verify this with your code as I don't a working platform with
> GICv2m, but if I enable something similar on GICv3, it dies a very
> painful way:
>
> Unable to handle kernel NULL pointer dereference at virtual address 00000018
> pgd = ffffffc03d059000
> [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000
> Internal error: Oops: 96000006 [#1] SMP
> Modules linked in:
> CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
> task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
> PC is at irq_domain_free_irqs_recursive+0x1c/0x80
> LR is at irq_domain_free_irqs_common+0x88/0x9c
> pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
> [...]
> [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <-- gic_domain.free()
> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
> [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
> [<ffffffc0000ef518>] msi_domain_free+0x70/0x88
> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
> [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
> [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
> [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
> [...]
>
> and I cannot see how this could work on the standard GIC either.
I'm sorry, I just realize my testcase was too simple, irqs are populated
by device tree and never got freed. I'll add that and test it again.
> Thomas, Jiang: could you please confirm or infirm my suspicions? My
> understanding is that irq_domain_free_irqs_top can only be called from
> the top-level domain.
>
> > +};
> > +
> > static const struct irq_domain_ops gic_irq_domain_ops = {
> > .map = gic_irq_domain_map,
> > .unmap = gic_irq_domain_unmap,
> > @@ -948,18 +972,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.
> > */
> > @@ -969,10 +981,32 @@ 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;
>
> nit: please put this on the same line, even if it is longer than 80
> characters.
ok.
Joe.C
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.
2014-11-20 3:57 ` Yingjoe Chen
@ 2014-11-20 9:41 ` Yingjoe Chen
2014-11-20 10:29 ` Marc Zyngier
0 siblings, 1 reply; 19+ messages in thread
From: Yingjoe Chen @ 2014-11-20 9:41 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Jiang Liu, Mark Rutland, Boris BREZILLON,
Russell King, Jason Cooper, Pawel Moll, devicetree, hc.yen,
srv_heupstream, yh.chen, linux-kernel, grant.likely, Yijing Wang,
Rob Herring, nathan.chung, yingjoe.chen, Matthias Brugger,
eddie.huang, Bjorn Helgaas, Sascha Hauer,
linux- arm-kernel@lists.infradead.org
Hi Marc,
On Thu, 2014-11-20 at 11:57 +0800, Yingjoe Chen wrote:
> On Wed, 2014-11-19 at 17:18 +0000, Marc Zyngier wrote:
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
> > > + .xlate = gic_irq_domain_xlate,
> > > + .alloc = gic_irq_domain_alloc,
> > > + .free = irq_domain_free_irqs_top,
> >
> > I'm convinced that irq_domain_free_irqs_top is the wrong function to
> > call here, because you're calling it from the bottom, not the top-level
> > (it has no parent).
>
> Base on the name, I though this is helper function for top level
> irq_domain?
>
> > I cannot verify this with your code as I don't a working platform with
> > GICv2m, but if I enable something similar on GICv3, it dies a very
> > painful way:
> >
> > Unable to handle kernel NULL pointer dereference at virtual address 00000018
> > pgd = ffffffc03d059000
> > [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000
> > Internal error: Oops: 96000006 [#1] SMP
> > Modules linked in:
> > CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
> > task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
> > PC is at irq_domain_free_irqs_recursive+0x1c/0x80
> > LR is at irq_domain_free_irqs_common+0x88/0x9c
> > pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
> > [...]
> > [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
> > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <-- gic_domain.free()
> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> > [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
> > [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
> > [<ffffffc0000ef518>] msi_domain_free+0x70/0x88
> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> > [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
> > [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
> > [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
> > [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
> > [...]
> >
> > and I cannot see how this could work on the standard GIC either.
>
> I'm sorry, I just realize my testcase was too simple, irqs are populated
> by device tree and never got freed. I'll add that and test it again.
On a second thoughts, unlike the MSI cases, gic_irq_domain_hierarchy_ops
is only used when we use DT, so we probably will never use the free
function. Is it OK to remove the free support here?
Joe.C
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.
2014-11-20 9:41 ` Yingjoe Chen
@ 2014-11-20 10:29 ` Marc Zyngier
0 siblings, 0 replies; 19+ messages in thread
From: Marc Zyngier @ 2014-11-20 10:29 UTC (permalink / raw)
To: Yingjoe Chen
Cc: Thomas Gleixner, Jiang Liu, Mark Rutland, Boris BREZILLON,
Russell King, Jason Cooper, Pawel Moll, devicetree, hc.yen,
srv_heupstream, yh.chen, linux-kernel, grant.likely, Yijing Wang,
Rob Herring, nathan.chung, yingjoe.chen, Matthias Brugger,
eddie.huang, Bjorn Helgaas, Sascha Hauer,
linux- arm-kernel@lists.infradead.org
Hi Joe,
On Thu, Nov 20 2014 at 9:41:51 am GMT, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
> Hi Marc,
>
> On Thu, 2014-11-20 at 11:57 +0800, Yingjoe Chen wrote:
>> On Wed, 2014-11-19 at 17:18 +0000, Marc Zyngier wrote:
>> > > +
>> > > + return 0;
>> > > +}
>> > > +
>> > > +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
>> > > + .xlate = gic_irq_domain_xlate,
>> > > + .alloc = gic_irq_domain_alloc,
>> > > + .free = irq_domain_free_irqs_top,
>> >
>> > I'm convinced that irq_domain_free_irqs_top is the wrong function to
>> > call here, because you're calling it from the bottom, not the top-level
>> > (it has no parent).
>>
>> Base on the name, I though this is helper function for top level
>> irq_domain?
>>
>> > I cannot verify this with your code as I don't a working platform with
>> > GICv2m, but if I enable something similar on GICv3, it dies a very
>> > painful way:
>> >
>> > Unable to handle kernel NULL pointer dereference at virtual address 00000018
>> > pgd = ffffffc03d059000
>> > [00000018] *pgd=0000000081356003, *pud=0000000081356003,
>> > *pmd=0000000000000000
>> > Internal error: Oops: 96000006 [#1] SMP
>> > Modules linked in:
>> > CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
>> > task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
>> > PC is at irq_domain_free_irqs_recursive+0x1c/0x80
>> > LR is at irq_domain_free_irqs_common+0x88/0x9c
>> > pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
>> > [...]
>> > [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
>> > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
>> > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <--
>> > gic_domain.free()
>> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>> > [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
>> > [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
>> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>> > [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
>> > [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
>> > [<ffffffc0000ef518>] msi_domain_free+0x70/0x88
>> > [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>> > [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
>> > [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
>> > [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
>> > [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
>> > [...]
>> >
>> > and I cannot see how this could work on the standard GIC either.
>>
>> I'm sorry, I just realize my testcase was too simple, irqs are populated
>> by device tree and never got freed. I'll add that and test it again.
>
> On a second thoughts, unlike the MSI cases, gic_irq_domain_hierarchy_ops
> is only used when we use DT, so we probably will never use the free
> function. Is it OK to remove the free support here?
Well, such thing is coming with GICv2m (SPIs are allocated out of
DT). You can drop it if you want, but I will then have to add it back
(which seems like a waste of time).
I'd prefer if you kept it in with the rest of the conversion.
Thanks,
M.
--
Jazz is not dead. It just smells funny.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.
2014-11-19 17:18 ` Marc Zyngier
2014-11-20 3:57 ` Yingjoe Chen
@ 2014-11-20 4:26 ` Jiang Liu
2014-11-20 10:07 ` Marc Zyngier
1 sibling, 1 reply; 19+ messages in thread
From: Jiang Liu @ 2014-11-20 4:26 UTC (permalink / raw)
To: Marc Zyngier, Yingjoe Chen
Cc: Thomas Gleixner, Mark Rutland, Boris BREZILLON, Russell King,
Jason Cooper, Pawel Moll, devicetree, hc.yen, srv_heupstream,
yh.chen, linux-kernel, grant.likely, Yijing Wang, Rob Herring,
nathan.chung, yingjoe.chen, Matthias Brugger, eddie.huang,
Bjorn Helgaas, Sascha Hauer,
linux- arm-kernel@lists.infradead.org
On 2014/11/20 1:18, Marc Zyngier wrote:
> Hi Yingjoe,
>
> On Wed, Nov 19 2014 at 2:14:08 pm GMT, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
>> Add support to use gic as a parent for stacked irq domain.
>>
>> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
>> ---
>> drivers/irqchip/Kconfig | 1 +
>> drivers/irqchip/irq-gic.c | 78 ++++++++++++++++++++++++++++++++---------------
>> 2 files changed, 55 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index b21f12f..7f34138 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 38493ff..464dd53 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -788,17 +788,16 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
>> {
>> if (hw < 32) {
>> irq_set_percpu_devid(irq);
>> - irq_set_chip_and_handler(irq, &gic_chip,
>> - handle_percpu_devid_irq);
>> + irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
>> + handle_percpu_devid_irq, NULL, NULL);
>> set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN);
>> } else {
>> - irq_set_chip_and_handler(irq, &gic_chip,
>> - handle_fasteoi_irq);
>> + irq_domain_set_info(d, irq, hw, &gic_chip, d->host_data,
>> + handle_fasteoi_irq, NULL, NULL);
>> 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;
>> }
>>
>> @@ -858,6 +857,31 @@ static struct notifier_block gic_cpu_notifier = {
>> };
>> #endif
>>
>> +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);
>
> nit: spacing around '+'.
>
>> +
>> + return 0;
>> +}
>> +
>> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
>> + .xlate = gic_irq_domain_xlate,
>> + .alloc = gic_irq_domain_alloc,
>> + .free = irq_domain_free_irqs_top,
>
> I'm convinced that irq_domain_free_irqs_top is the wrong function to
> call here, because you're calling it from the bottom, not the top-level
> (it has no parent).
>
> I cannot verify this with your code as I don't a working platform with
> GICv2m, but if I enable something similar on GICv3, it dies a very
> painful way:
>
> Unable to handle kernel NULL pointer dereference at virtual address 00000018
> pgd = ffffffc03d059000
> [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000
> Internal error: Oops: 96000006 [#1] SMP
> Modules linked in:
> CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
> task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
> PC is at irq_domain_free_irqs_recursive+0x1c/0x80
> LR is at irq_domain_free_irqs_common+0x88/0x9c
> pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
> [...]
> [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <-- gic_domain.free()
> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
> [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
> [<ffffffc0000ef518>] msi_domain_free+0x70/0x88
> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
> [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
> [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
> [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
> [...]
>
> and I cannot see how this could work on the standard GIC either.
>
> Thomas, Jiang: could you please confirm or infirm my suspicions? My
> understanding is that irq_domain_free_irqs_top can only be called from
> the top-level domain.
Hi Marc,
It indicates that irq_domain_free_irqs_top() is not a good name.
We have:
1) irq_domain_set_hwirq_and_chip() to set irq_chip and chip_data
2) irq_domain_set_info() to set irq_chip, chip_data, flow_handler and
handler_data;
3) irq_domain_reset_irq_data() resets irq_chip and chip_data.
4) irq_domain_free_irqs_common() resets irq_chip, chip_data and calls
parent domain's domain_ops.free() callback.
5) irq_domain_free_irqs_top() resets irq_chip, chip_data, flow handler,
handler_data and call parent domain's domain_ops.free() callback.
So there two possible improvements here:
1) Rename irq_domain_free_irqs_top() with better name, any suggestions?
It's named as is because it's always called by the outer-most
irqdomains on x86.
2) Change irq_domain_free_irqs_common() and irq_domain_free_irqs_top()
to call parent domain's domain_ops.free() callback only if parent
exists. By this way, they could be used for inner-most irqdomains.
If OK, I will respin a version 4 patch set based on tip/irq/irqdomain.
Thoughts?
Regards!
Gerry
>
>> +};
>> +
>> static const struct irq_domain_ops gic_irq_domain_ops = {
>> .map = gic_irq_domain_map,
>> .unmap = gic_irq_domain_unmap,
>> @@ -948,18 +972,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.
>> */
>> @@ -969,10 +981,32 @@ 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;
>
> nit: please put this on the same line, even if it is longer than 80
> characters.
>
>> +
>> + 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 (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)) {
>> @@ -983,10 +1017,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))
>
> Thanks,
>
> M.
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.
2014-11-20 4:26 ` Jiang Liu
@ 2014-11-20 10:07 ` Marc Zyngier
2014-11-21 15:51 ` Yingjoe Chen
0 siblings, 1 reply; 19+ messages in thread
From: Marc Zyngier @ 2014-11-20 10:07 UTC (permalink / raw)
To: Jiang Liu
Cc: Yingjoe Chen, Thomas Gleixner, Mark Rutland, Boris BREZILLON,
Russell King, Jason Cooper, Pawel Moll, devicetree, hc.yen,
srv_heupstream, yh.chen, linux-kernel, grant.likely, Yijing Wang,
Rob Herring, nathan.chung, yingjoe.chen, Matthias Brugger,
eddie.huang, Bjorn Helgaas, Sascha Hauer,
lin ux- arm-kernel@lists.infradead.org
On Thu, Nov 20 2014 at 4:26:10 am GMT, Jiang Liu <jiang.liu@linux.intel.com> wrote:
Hi Jiang,
> On 2014/11/20 1:18, Marc Zyngier wrote:
>> Hi Yingjoe,
>>
>> On Wed, Nov 19 2014 at 2:14:08 pm GMT, Yingjoe Chen
>> <yingjoe.chen@mediatek.com> wrote:
>>> +
>>> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
>>> + .xlate = gic_irq_domain_xlate,
>>> + .alloc = gic_irq_domain_alloc,
>>> + .free = irq_domain_free_irqs_top,
>>
>> I'm convinced that irq_domain_free_irqs_top is the wrong function to
>> call here, because you're calling it from the bottom, not the top-level
>> (it has no parent).
>>
>> I cannot verify this with your code as I don't a working platform with
>> GICv2m, but if I enable something similar on GICv3, it dies a very
>> painful way:
>>
>> Unable to handle kernel NULL pointer dereference at virtual address 00000018
>> pgd = ffffffc03d059000
>> [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000
>> Internal error: Oops: 96000006 [#1] SMP
>> Modules linked in:
>> CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
>> task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
>> PC is at irq_domain_free_irqs_recursive+0x1c/0x80
>> LR is at irq_domain_free_irqs_common+0x88/0x9c
>> pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
>> [...]
>> [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
>> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
>> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <-- gic_domain.free()
>> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>> [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
>> [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
>> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
>> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
>> [<ffffffc0000ef518>] msi_domain_free+0x70/0x88
>> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>> [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
>> [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
>> [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
>> [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
>> [...]
>>
>> and I cannot see how this could work on the standard GIC either.
>>
>> Thomas, Jiang: could you please confirm or infirm my suspicions? My
>> understanding is that irq_domain_free_irqs_top can only be called from
>> the top-level domain.
> Hi Marc,
> It indicates that irq_domain_free_irqs_top() is not a good name.
> We have:
> 1) irq_domain_set_hwirq_and_chip() to set irq_chip and chip_data
> 2) irq_domain_set_info() to set irq_chip, chip_data, flow_handler and
> handler_data;
> 3) irq_domain_reset_irq_data() resets irq_chip and chip_data.
> 4) irq_domain_free_irqs_common() resets irq_chip, chip_data and calls
> parent domain's domain_ops.free() callback.
> 5) irq_domain_free_irqs_top() resets irq_chip, chip_data, flow handler,
> handler_data and call parent domain's domain_ops.free() callback.
Yes, and this "call parent domain's free callback" is where the problem
lies. Here, it is called from the innermost domain, with no parent.
> So there two possible improvements here:
> 1) Rename irq_domain_free_irqs_top() with better name, any suggestions?
> It's named as is because it's always called by the outer-most
> irqdomains on x86.
> 2) Change irq_domain_free_irqs_common() and irq_domain_free_irqs_top()
> to call parent domain's domain_ops.free() callback only if parent
> exists. By this way, they could be used for inner-most irqdomains.
> If OK, I will respin a version 4 patch set based on tip/irq/irqdomain.
> Thoughts?
Checking the parent is probably a safe solution (this is not a hot path
anyway). I don't care much about the name though, and I the only thing I
can think of is irq_domain_free_irqs_reset_flow, which looks so bad it's
not even funny. I'll let the matter rest in your capable hands! ;-)
Thanks,
M.
--
Jazz is not dead. It just smells funny.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.
2014-11-20 10:07 ` Marc Zyngier
@ 2014-11-21 15:51 ` Yingjoe Chen
2014-11-24 19:31 ` Marc Zyngier
0 siblings, 1 reply; 19+ messages in thread
From: Yingjoe Chen @ 2014-11-21 15:51 UTC (permalink / raw)
To: Marc Zyngier, Jiang Liu, Thomas Gleixner
Cc: Mark Rutland, Boris BREZILLON, Russell King, Jason Cooper,
Pawel Moll, devicetree, hc.yen, srv_heupstream, yh.chen,
linux-kernel, grant.likely, Yijing Wang, Rob Herring,
nathan.chung, yingjoe.chen, Matthias Brugger, eddie.huang,
Bjorn Helgaas, Sascha Hauer,
lin ux- arm-kernel@lists.infradead.org
Hi,
On Thu, 2014-11-20 at 10:07 +0000, Marc Zyngier wrote:
> On Thu, Nov 20 2014 at 4:26:10 am GMT, Jiang Liu <jiang.liu@linux.intel.com> wrote:
>
> Hi Jiang,
>
> > On 2014/11/20 1:18, Marc Zyngier wrote:
> >> Hi Yingjoe,
> >>
> >> On Wed, Nov 19 2014 at 2:14:08 pm GMT, Yingjoe Chen
> >> <yingjoe.chen@mediatek.com> wrote:
> >>> +
> >>> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
> >>> + .xlate = gic_irq_domain_xlate,
> >>> + .alloc = gic_irq_domain_alloc,
> >>> + .free = irq_domain_free_irqs_top,
> >>
> >> I'm convinced that irq_domain_free_irqs_top is the wrong function to
> >> call here, because you're calling it from the bottom, not the top-level
> >> (it has no parent).
> >>
> >> I cannot verify this with your code as I don't a working platform with
> >> GICv2m, but if I enable something similar on GICv3, it dies a very
> >> painful way:
> >>
> >> Unable to handle kernel NULL pointer dereference at virtual address 00000018
> >> pgd = ffffffc03d059000
> >> [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000
> >> Internal error: Oops: 96000006 [#1] SMP
> >> Modules linked in:
> >> CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
> >> task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
> >> PC is at irq_domain_free_irqs_recursive+0x1c/0x80
> >> LR is at irq_domain_free_irqs_common+0x88/0x9c
> >> pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
> >> [...]
> >> [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
> >> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> >> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <-- gic_domain.free()
> >> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> >> [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
> >> [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
> >> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> >> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
> >> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
> >> [<ffffffc0000ef518>] msi_domain_free+0x70/0x88
> >> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
> >> [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
> >> [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
> >> [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
> >> [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
> >> [...]
> >>
> >> and I cannot see how this could work on the standard GIC either.
> >>
> >> Thomas, Jiang: could you please confirm or infirm my suspicions? My
> >> understanding is that irq_domain_free_irqs_top can only be called from
> >> the top-level domain.
> > Hi Marc,
> > It indicates that irq_domain_free_irqs_top() is not a good name.
> > We have:
> > 1) irq_domain_set_hwirq_and_chip() to set irq_chip and chip_data
> > 2) irq_domain_set_info() to set irq_chip, chip_data, flow_handler and
> > handler_data;
> > 3) irq_domain_reset_irq_data() resets irq_chip and chip_data.
> > 4) irq_domain_free_irqs_common() resets irq_chip, chip_data and calls
> > parent domain's domain_ops.free() callback.
> > 5) irq_domain_free_irqs_top() resets irq_chip, chip_data, flow handler,
> > handler_data and call parent domain's domain_ops.free() callback.
>
> Yes, and this "call parent domain's free callback" is where the problem
> lies. Here, it is called from the innermost domain, with no parent.
>
> > So there two possible improvements here:
> > 1) Rename irq_domain_free_irqs_top() with better name, any suggestions?
> > It's named as is because it's always called by the outer-most
> > irqdomains on x86.
> > 2) Change irq_domain_free_irqs_common() and irq_domain_free_irqs_top()
> > to call parent domain's domain_ops.free() callback only if parent
> > exists. By this way, they could be used for inner-most irqdomains.
> > If OK, I will respin a version 4 patch set based on tip/irq/irqdomain.
> > Thoughts?
>
> Checking the parent is probably a safe solution (this is not a hot path
> anyway). I don't care much about the name though, and I the only thing I
> can think of is irq_domain_free_irqs_reset_flow, which looks so bad it's
> not even funny. I'll let the matter rest in your capable hands! ;-)
I've applied Jiang's "irqdomain: Enhance irq_domain_free_irqs_common()
to support parentless irqdomain" patch and it did fix the crash.
Thanks Jiang, Marc
Joe.C
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain.
2014-11-21 15:51 ` Yingjoe Chen
@ 2014-11-24 19:31 ` Marc Zyngier
0 siblings, 0 replies; 19+ messages in thread
From: Marc Zyngier @ 2014-11-24 19:31 UTC (permalink / raw)
To: Yingjoe Chen, Jiang Liu, Thomas Gleixner
Cc: Mark Rutland, Boris BREZILLON, Russell King, Jason Cooper,
Pawel Moll, devicetree, hc.yen, srv_heupstream, yh.chen,
linux-kernel, grant.likely, Yijing Wang, Rob Herring,
nathan.chung, yingjoe.chen, Matthias Brugger, eddie.huang,
Bjorn Helgaas, Sascha Hauer,
lin ux- arm-kernel@lists.infradead.org
On 21/11/14 15:51, Yingjoe Chen wrote:
>
> Hi,
>
> On Thu, 2014-11-20 at 10:07 +0000, Marc Zyngier wrote:
>> On Thu, Nov 20 2014 at 4:26:10 am GMT, Jiang Liu <jiang.liu@linux.intel.com> wrote:
>>
>> Hi Jiang,
>>
>>> On 2014/11/20 1:18, Marc Zyngier wrote:
>>>> Hi Yingjoe,
>>>>
>>>> On Wed, Nov 19 2014 at 2:14:08 pm GMT, Yingjoe Chen
>>>> <yingjoe.chen@mediatek.com> wrote:
>>>>> +
>>>>> +static const struct irq_domain_ops gic_irq_domain_hierarchy_ops = {
>>>>> + .xlate = gic_irq_domain_xlate,
>>>>> + .alloc = gic_irq_domain_alloc,
>>>>> + .free = irq_domain_free_irqs_top,
>>>>
>>>> I'm convinced that irq_domain_free_irqs_top is the wrong function to
>>>> call here, because you're calling it from the bottom, not the top-level
>>>> (it has no parent).
>>>>
>>>> I cannot verify this with your code as I don't a working platform with
>>>> GICv2m, but if I enable something similar on GICv3, it dies a very
>>>> painful way:
>>>>
>>>> Unable to handle kernel NULL pointer dereference at virtual address 00000018
>>>> pgd = ffffffc03d059000
>>>> [00000018] *pgd=0000000081356003, *pud=0000000081356003, *pmd=0000000000000000
>>>> Internal error: Oops: 96000006 [#1] SMP
>>>> Modules linked in:
>>>> CPU: 4 PID: 1052 Comm: sh Not tainted 3.18.0-rc4+ #3311
>>>> task: ffffffc03e320000 ti: ffffffc001390000 task.ti: ffffffc001390000
>>>> PC is at irq_domain_free_irqs_recursive+0x1c/0x80
>>>> LR is at irq_domain_free_irqs_common+0x88/0x9c
>>>> pc : [<ffffffc0000ed790>] lr : [<ffffffc0000ede20>] pstate: 60000145
>>>> [...]
>>>> [<ffffffc0000ed790>] irq_domain_free_irqs_recursive+0x1c/0x80
>>>> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
>>>> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c <-- gic_domain.free()
>>>> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>>>> [<ffffffc0000ee468>] irq_domain_free_irqs_parent+0x14/0x20
>>>> [<ffffffc0003500b8>] its_irq_domain_free+0xc8/0x250
>>>> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>>>> [<ffffffc0000ede1c>] irq_domain_free_irqs_common+0x84/0x9c
>>>> [<ffffffc0000ede98>] irq_domain_free_irqs_top+0x64/0x7c
>>>> [<ffffffc0000ef518>] msi_domain_free+0x70/0x88
>>>> [<ffffffc0000ed798>] irq_domain_free_irqs_recursive+0x24/0x80
>>>> [<ffffffc0000ee3ac>] irq_domain_free_irqs+0x108/0x17c
>>>> [<ffffffc0000efb68>] msi_domain_free_irqs+0x28/0x4c
>>>> [<ffffffc000369cac>] free_msi_irqs+0xb4/0x1c0
>>>> [<ffffffc00036adec>] pci_disable_msix+0x3c/0x4c
>>>> [...]
>>>>
>>>> and I cannot see how this could work on the standard GIC either.
>>>>
>>>> Thomas, Jiang: could you please confirm or infirm my suspicions? My
>>>> understanding is that irq_domain_free_irqs_top can only be called from
>>>> the top-level domain.
>>> Hi Marc,
>>> It indicates that irq_domain_free_irqs_top() is not a good name.
>>> We have:
>>> 1) irq_domain_set_hwirq_and_chip() to set irq_chip and chip_data
>>> 2) irq_domain_set_info() to set irq_chip, chip_data, flow_handler and
>>> handler_data;
>>> 3) irq_domain_reset_irq_data() resets irq_chip and chip_data.
>>> 4) irq_domain_free_irqs_common() resets irq_chip, chip_data and calls
>>> parent domain's domain_ops.free() callback.
>>> 5) irq_domain_free_irqs_top() resets irq_chip, chip_data, flow handler,
>>> handler_data and call parent domain's domain_ops.free() callback.
>>
>> Yes, and this "call parent domain's free callback" is where the problem
>> lies. Here, it is called from the innermost domain, with no parent.
>>
>>> So there two possible improvements here:
>>> 1) Rename irq_domain_free_irqs_top() with better name, any suggestions?
>>> It's named as is because it's always called by the outer-most
>>> irqdomains on x86.
>>> 2) Change irq_domain_free_irqs_common() and irq_domain_free_irqs_top()
>>> to call parent domain's domain_ops.free() callback only if parent
>>> exists. By this way, they could be used for inner-most irqdomains.
>>> If OK, I will respin a version 4 patch set based on tip/irq/irqdomain.
>>> Thoughts?
>>
>> Checking the parent is probably a safe solution (this is not a hot path
>> anyway). I don't care much about the name though, and I the only thing I
>> can think of is irq_domain_free_irqs_reset_flow, which looks so bad it's
>> not even funny. I'll let the matter rest in your capable hands! ;-)
>
> I've applied Jiang's "irqdomain: Enhance irq_domain_free_irqs_common()
> to support parentless irqdomain" patch and it did fix the crash.
Excellent. I think this is pretty much getting there now. Any chance
you could repost this series with the various fixes?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v7 2/4] ARM: mediatek: Add sysirq interrupt polarity support
2014-11-19 14:14 [PATCH v7 0/4] ARM: mediatek: Add support for interrupt polarity Yingjoe Chen
2014-11-19 14:14 ` [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain Yingjoe Chen
@ 2014-11-19 14:14 ` Yingjoe Chen
2014-11-19 18:04 ` Mark Rutland
2014-11-19 14:14 ` [PATCH v7 3/4] ARM: mediatek: Add sysirq in mt6589/mt8135/mt8127 dtsi Yingjoe Chen
2014-11-19 14:14 ` [PATCH v7 4/4] dt-bindings: add bindings for mediatek sysirq Yingjoe Chen
3 siblings, 1 reply; 19+ messages in thread
From: Yingjoe Chen @ 2014-11-19 14:14 UTC (permalink / raw)
To: Thomas Gleixner, Jiang Liu, Marc Zyngier
Cc: Mark Rutland, Boris BREZILLON, Russell King, Jason Cooper,
Pawel Moll, devicetree, hc.yen, srv_heupstream, yh.chen,
linux-kernel, Grant Likely, Yijing Wang, Rob Herring,
nathan.chung, yingjoe.chen, Matthias Brugger, Yingjoe Chen,
eddie.huang, Bjorn Helgaas, Sascha Hauer, linux-arm-kernel
Mediatek SoCs have interrupt polarity support in sysirq which
allows to invert polarity for given interrupt. Add this support
using hierarchy irq domain.
Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-mtk-sysirq.c | 158 +++++++++++++++++++++++++++++++++++++++
2 files changed, 159 insertions(+)
create mode 100644 drivers/irqchip/irq-mtk-sysirq.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 173bb5f..4e0f254 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -38,3 +38,4 @@ obj-$(CONFIG_IRQ_CROSSBAR) += irq-crossbar.o
obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o \
irq-bcm7120-l2.o
obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o
+obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o
diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
new file mode 100644
index 0000000..93cb945
--- /dev/null
+++ b/drivers/irqchip/irq-mtk-sysirq.c
@@ -0,0 +1,158 @@
+/*
+ * 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 mtk_sysirq_chip_data {
+ spinlock_t lock;
+ void __iomem *intpol_base;
+};
+
+static int mtk_sysirq_set_type(struct irq_data *data, unsigned int type)
+{
+ irq_hw_number_t hwirq = data->hwirq;
+ struct mtk_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 mtk_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 = mtk_sysirq_set_type,
+ .irq_retrigger = irq_chip_retrigger_hierarchy,
+ .irq_set_affinity = irq_chip_set_affinity_parent,
+};
+
+static int mtk_sysirq_domain_xlate(struct irq_domain *d,
+ struct device_node *controller,
+ const u32 *intspec, unsigned int intsize,
+ unsigned long *out_hwirq,
+ unsigned int *out_type)
+{
+ if (intsize < 3)
+ return -EINVAL;
+
+ /* sysirq doesn't support PPI */
+ if (intspec[0])
+ return -EINVAL;
+
+ *out_hwirq = intspec[1];
+ *out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
+ return 0;
+}
+
+static int mtk_sysirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *arg)
+{
+ int i;
+ irq_hw_number_t hwirq;
+ struct of_phandle_args *irq_data = arg;
+ struct of_phandle_args gic_data = *irq_data;
+
+ if (irq_data->args_count < 3)
+ return -EINVAL;
+
+ hwirq = irq_data->args[1];
+ for (i = 0; i < nr_irqs; i++)
+ irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+ &mtk_sysirq_chip,
+ domain->host_data);
+
+ gic_data.np = domain->parent->of_node;
+ return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &gic_data);
+}
+
+static struct irq_domain_ops sysirq_domain_ops = {
+ .xlate = mtk_sysirq_domain_xlate,
+ .alloc = mtk_sysirq_domain_alloc,
+ .free = irq_domain_free_irqs_common,
+};
+
+static int __init mtk_sysirq_of_init(struct device_node *node,
+ struct device_node *parent)
+{
+ struct irq_domain *domain, *domain_parent;
+ struct mtk_sysirq_chip_data *chip_data;
+ int ret = 0;
+
+ domain_parent = irq_find_host(parent);
+ 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_hierarchy(domain_parent, 0,
+ MT6577_SYS_INTPOL_NUM, node,
+ &sysirq_domain_ops, chip_data);
+ if (!domain) {
+ ret = -ENOMEM;
+ goto out_unmap;
+ }
+ 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);
--
1.8.1.1.dirty
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v7 2/4] ARM: mediatek: Add sysirq interrupt polarity support
2014-11-19 14:14 ` [PATCH v7 2/4] ARM: mediatek: Add sysirq interrupt polarity support Yingjoe Chen
@ 2014-11-19 18:04 ` Mark Rutland
2014-11-21 15:36 ` Yingjoe Chen
0 siblings, 1 reply; 19+ messages in thread
From: Mark Rutland @ 2014-11-19 18:04 UTC (permalink / raw)
To: Yingjoe Chen
Cc: Thomas Gleixner, Jiang Liu, Marc Zyngier, Boris BREZILLON,
Russell King, Jason Cooper, Pawel Moll, devicetree, hc.yen,
srv_heupstream, yh.chen, linux-kernel, grant.likely, Yijing Wang,
Rob Herring, nathan.chung, yingjoe.chen, Matthias Brugger,
eddie.huang, Bjorn Helgaas, Sascha Hauer, linux-arm-kernel
On Wed, Nov 19, 2014 at 02:14:09PM +0000, Yingjoe Chen wrote:
> Mediatek SoCs have interrupt polarity support in sysirq which
> allows to invert polarity for given interrupt. Add this support
> using hierarchy irq domain.
>
> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-mtk-sysirq.c | 158 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 159 insertions(+)
> create mode 100644 drivers/irqchip/irq-mtk-sysirq.c
>
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 173bb5f..4e0f254 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -38,3 +38,4 @@ obj-$(CONFIG_IRQ_CROSSBAR) += irq-crossbar.o
> obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o \
> irq-bcm7120-l2.o
> obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o
> +obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o
> diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
> new file mode 100644
> index 0000000..93cb945
> --- /dev/null
> +++ b/drivers/irqchip/irq-mtk-sysirq.c
> @@ -0,0 +1,158 @@
> +/*
> + * 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 mtk_sysirq_chip_data {
> + spinlock_t lock;
> + void __iomem *intpol_base;
> +};
> +
> +static int mtk_sysirq_set_type(struct irq_data *data, unsigned int type)
> +{
> + irq_hw_number_t hwirq = data->hwirq;
> + struct mtk_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);
Nit: if one branch of an if statements is bracketed, both branches
should be. Please bracket the else case.
> + 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 mtk_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 = mtk_sysirq_set_type,
> + .irq_retrigger = irq_chip_retrigger_hierarchy,
> + .irq_set_affinity = irq_chip_set_affinity_parent,
> +};
> +
> +static int mtk_sysirq_domain_xlate(struct irq_domain *d,
> + struct device_node *controller,
> + const u32 *intspec, unsigned int intsize,
> + unsigned long *out_hwirq,
> + unsigned int *out_type)
> +{
> + if (intsize < 3)
> + return -EINVAL;
> +
> + /* sysirq doesn't support PPI */
> + if (intspec[0])
> + return -EINVAL;
> +
> + *out_hwirq = intspec[1];
> + *out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
All of this is very strongly tied to the GIC, but the binding made it
sound like this was more generic (it didn't specify the number of cells
or their meaning). It doesn't look like we validate that this is
attached to a GIC.
Do we expect this to only ever be attached to a GICv2?
It would be nice to either make this generic or to describe the format
in the binding.
Also, surely you expect intsize == 3 if you expect a GICv2? Likewise in
mtk_sysirq_domain_alloc.
Thanks,
Mark.
> + return 0;
> +}
> +
> +static int mtk_sysirq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> + unsigned int nr_irqs, void *arg)
> +{
> + int i;
> + irq_hw_number_t hwirq;
> + struct of_phandle_args *irq_data = arg;
> + struct of_phandle_args gic_data = *irq_data;
> +
> + if (irq_data->args_count < 3)
> + return -EINVAL;
> +
> + hwirq = irq_data->args[1];
> + for (i = 0; i < nr_irqs; i++)
> + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> + &mtk_sysirq_chip,
> + domain->host_data);
> +
> + gic_data.np = domain->parent->of_node;
> + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &gic_data);
> +}
> +
> +static struct irq_domain_ops sysirq_domain_ops = {
> + .xlate = mtk_sysirq_domain_xlate,
> + .alloc = mtk_sysirq_domain_alloc,
> + .free = irq_domain_free_irqs_common,
> +};
> +
> +static int __init mtk_sysirq_of_init(struct device_node *node,
> + struct device_node *parent)
> +{
> + struct irq_domain *domain, *domain_parent;
> + struct mtk_sysirq_chip_data *chip_data;
> + int ret = 0;
> +
> + domain_parent = irq_find_host(parent);
> + 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_hierarchy(domain_parent, 0,
> + MT6577_SYS_INTPOL_NUM, node,
> + &sysirq_domain_ops, chip_data);
> + if (!domain) {
> + ret = -ENOMEM;
> + goto out_unmap;
> + }
> + 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);
> --
> 1.8.1.1.dirty
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v7 2/4] ARM: mediatek: Add sysirq interrupt polarity support
2014-11-19 18:04 ` Mark Rutland
@ 2014-11-21 15:36 ` Yingjoe Chen
0 siblings, 0 replies; 19+ messages in thread
From: Yingjoe Chen @ 2014-11-21 15:36 UTC (permalink / raw)
To: Mark Rutland
Cc: Thomas Gleixner, Jiang Liu, Marc Zyngier, Boris BREZILLON,
Russell King, Jason Cooper, Pawel Moll, devicetree, hc.yen,
srv_heupstream, yh.chen, linux-kernel, grant.likely, Yijing Wang,
Rob Herring, nathan.chung, yingjoe.chen, Matthias Brugger,
eddie.huang, Bjorn Helgaas, Sascha Hauer, linux-arm-kernel
Hi Mark,
On Wed, 2014-11-19 at 18:04 +0000, Mark Rutland wrote:
> On Wed, Nov 19, 2014 at 02:14:09PM +0000, Yingjoe Chen wrote:
> > + 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);
>
> Nit: if one branch of an if statements is bracketed, both branches
> should be. Please bracket the else case.
OK, will change in next version.
> > + 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 mtk_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 = mtk_sysirq_set_type,
> > + .irq_retrigger = irq_chip_retrigger_hierarchy,
> > + .irq_set_affinity = irq_chip_set_affinity_parent,
> > +};
> > +
> > +static int mtk_sysirq_domain_xlate(struct irq_domain *d,
> > + struct device_node *controller,
> > + const u32 *intspec, unsigned int intsize,
> > + unsigned long *out_hwirq,
> > + unsigned int *out_type)
> > +{
> > + if (intsize < 3)
> > + return -EINVAL;
> > +
> > + /* sysirq doesn't support PPI */
> > + if (intspec[0])
> > + return -EINVAL;
> > +
> > + *out_hwirq = intspec[1];
> > + *out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
>
> All of this is very strongly tied to the GIC, but the binding made it
> sound like this was more generic (it didn't specify the number of cells
> or their meaning). It doesn't look like we validate that this is
> attached to a GIC.
>
> Do we expect this to only ever be attached to a GICv2?
>
> It would be nice to either make this generic or to describe the format
> in the binding.
This expect the parent to use the same interrupt cells format as GIC.
I'll fix the binding to make this more clear.
> Also, surely you expect intsize == 3 if you expect a GICv2? Likewise in
> mtk_sysirq_domain_alloc.
OK, will change that in next version.
Joe.C
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v7 3/4] ARM: mediatek: Add sysirq in mt6589/mt8135/mt8127 dtsi
2014-11-19 14:14 [PATCH v7 0/4] ARM: mediatek: Add support for interrupt polarity Yingjoe Chen
2014-11-19 14:14 ` [PATCH v7 1/4] irqchip: gic: Support hierarchy irq domain Yingjoe Chen
2014-11-19 14:14 ` [PATCH v7 2/4] ARM: mediatek: Add sysirq interrupt polarity support Yingjoe Chen
@ 2014-11-19 14:14 ` Yingjoe Chen
2014-11-19 17:49 ` Marc Zyngier
2014-11-19 14:14 ` [PATCH v7 4/4] dt-bindings: add bindings for mediatek sysirq Yingjoe Chen
3 siblings, 1 reply; 19+ messages in thread
From: Yingjoe Chen @ 2014-11-19 14:14 UTC (permalink / raw)
To: Thomas Gleixner, Jiang Liu, Marc Zyngier
Cc: Mark Rutland, Boris BREZILLON, Russell King, Jason Cooper,
Pawel Moll, devicetree, hc.yen, srv_heupstream, yh.chen,
linux-kernel, Grant Likely, Yijing Wang, Rob Herring,
nathan.chung, yingjoe.chen, Matthias Brugger, Yingjoe Chen,
eddie.huang, Bjorn Helgaas, Sascha Hauer, linux-arm-kernel
Add sysirq settings for mt6589/mt8135/mt8127
This also correct timer interrupt flag. The old setting works
because boot loader already set polarity for timer interrupt.
Without intpol support, the setting was not changed so gic
can get the irq correctly.
Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
arch/arm/boot/dts/mt6589.dtsi | 14 ++++++++++++--
arch/arm/boot/dts/mt8127.dtsi | 14 ++++++++++++--
arch/arm/boot/dts/mt8135.dtsi | 14 ++++++++++++--
3 files changed, 36 insertions(+), 6 deletions(-)
diff --git a/arch/arm/boot/dts/mt6589.dtsi b/arch/arm/boot/dts/mt6589.dtsi
index e3c7600..c91b2a9 100644
--- a/arch/arm/boot/dts/mt6589.dtsi
+++ b/arch/arm/boot/dts/mt6589.dtsi
@@ -19,7 +19,7 @@
/ {
compatible = "mediatek,mt6589";
- interrupt-parent = <&gic>;
+ interrupt-parent = <&sysirq>;
cpus {
#address-cells = <1>;
@@ -76,15 +76,25 @@
timer: timer@10008000 {
compatible = "mediatek,mt6577-timer";
reg = <0x10008000 0x80>;
- interrupts = <GIC_SPI 113 IRQ_TYPE_EDGE_RISING>;
+ interrupts = <GIC_SPI 113 IRQ_TYPE_LEVEL_LOW>;
clocks = <&system_clk>, <&rtc_clk>;
clock-names = "system-clk", "rtc-clk";
};
+ sysirq: interrupt-controller@10200100 {
+ compatible = "mediatek,mt6589-sysirq",
+ "mediatek,mt6577-sysirq";
+ interrupt-controller;
+ #interrupt-cells = <3>;
+ interrupt-parent = <&gic>;
+ reg = <0x10200100 0x1c>;
+ };
+
gic: interrupt-controller@10211000 {
compatible = "arm,cortex-a7-gic";
interrupt-controller;
#interrupt-cells = <3>;
+ interrupt-parent = <&gic>;
reg = <0x10211000 0x1000>,
<0x10212000 0x1000>,
<0x10214000 0x2000>,
diff --git a/arch/arm/boot/dts/mt8127.dtsi b/arch/arm/boot/dts/mt8127.dtsi
index c3ee060..49f5976 100644
--- a/arch/arm/boot/dts/mt8127.dtsi
+++ b/arch/arm/boot/dts/mt8127.dtsi
@@ -18,7 +18,7 @@
/ {
compatible = "mediatek,mt8127";
- interrupt-parent = <&gic>;
+ interrupt-parent = <&sysirq>;
cpus {
#address-cells = <1>;
@@ -82,15 +82,25 @@
compatible = "mediatek,mt8127-timer",
"mediatek,mt6577-timer";
reg = <0 0x10008000 0 0x80>;
- interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>;
+ interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_LOW>;
clocks = <&system_clk>, <&rtc_clk>;
clock-names = "system-clk", "rtc-clk";
};
+ sysirq: interrupt-controller@10200100 {
+ compatible = "mediatek,mt8127-sysirq",
+ "mediatek,mt6577-sysirq";
+ interrupt-controller;
+ #interrupt-cells = <3>;
+ interrupt-parent = <&gic>;
+ reg = <0 0x10200100 0 0x1c>;
+ };
+
gic: interrupt-controller@10211000 {
compatible = "arm,cortex-a7-gic";
interrupt-controller;
#interrupt-cells = <3>;
+ interrupt-parent = <&gic>;
reg = <0 0x10211000 0 0x1000>,
<0 0x10212000 0 0x1000>,
<0 0x10214000 0 0x2000>,
diff --git a/arch/arm/boot/dts/mt8135.dtsi b/arch/arm/boot/dts/mt8135.dtsi
index 5faae6e..60338d9 100644
--- a/arch/arm/boot/dts/mt8135.dtsi
+++ b/arch/arm/boot/dts/mt8135.dtsi
@@ -18,7 +18,7 @@
/ {
compatible = "mediatek,mt8135";
- interrupt-parent = <&gic>;
+ interrupt-parent = <&sysirq>;
cpu-map {
cluster0 {
@@ -105,15 +105,25 @@
compatible = "mediatek,mt8135-timer",
"mediatek,mt6577-timer";
reg = <0 0x10008000 0 0x80>;
- interrupts = <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>;
+ interrupts = <GIC_SPI 113 IRQ_TYPE_LEVEL_LOW>;
clocks = <&system_clk>, <&rtc_clk>;
clock-names = "system-clk", "rtc-clk";
};
+ sysirq: interrupt-controller@10200030 {
+ compatible = "mediatek,mt8135-sysirq",
+ "mediatek,mt6577-sysirq";
+ interrupt-controller;
+ #interrupt-cells = <3>;
+ interrupt-parent = <&gic>;
+ reg = <0 0x10200030 0 0x1c>;
+ };
+
gic: interrupt-controller@10211000 {
compatible = "arm,cortex-a15-gic";
interrupt-controller;
#interrupt-cells = <3>;
+ interrupt-parent = <&gic>;
reg = <0 0x10211000 0 0x1000>,
<0 0x10212000 0 0x1000>,
<0 0x10214000 0 0x2000>,
--
1.8.1.1.dirty
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v7 3/4] ARM: mediatek: Add sysirq in mt6589/mt8135/mt8127 dtsi
2014-11-19 14:14 ` [PATCH v7 3/4] ARM: mediatek: Add sysirq in mt6589/mt8135/mt8127 dtsi Yingjoe Chen
@ 2014-11-19 17:49 ` Marc Zyngier
2014-11-20 3:29 ` Yingjoe Chen
0 siblings, 1 reply; 19+ messages in thread
From: Marc Zyngier @ 2014-11-19 17:49 UTC (permalink / raw)
To: Yingjoe Chen
Cc: Thomas Gleixner, Jiang Liu, Mark Rutland, Boris BREZILLON,
Russell King, Jason Cooper, Pawel Moll, devicetree, hc.yen,
srv_heupstream, yh.chen, linux-kernel, grant.likely, Yijing Wang,
Rob Herring, nathan.chung, yingjoe.chen, Matthias Brugger,
eddie.huang, Bjorn Helgaas, Sascha Hauer,
linux- arm-kernel@lists.infradead.org
On Wed, Nov 19 2014 at 2:14:10 pm GMT, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
> Add sysirq settings for mt6589/mt8135/mt8127
> This also correct timer interrupt flag. The old setting works
> because boot loader already set polarity for timer interrupt.
> Without intpol support, the setting was not changed so gic
> can get the irq correctly.
>
> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
> arch/arm/boot/dts/mt6589.dtsi | 14 ++++++++++++--
> arch/arm/boot/dts/mt8127.dtsi | 14 ++++++++++++--
> arch/arm/boot/dts/mt8135.dtsi | 14 ++++++++++++--
> 3 files changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/boot/dts/mt6589.dtsi b/arch/arm/boot/dts/mt6589.dtsi
> index e3c7600..c91b2a9 100644
> --- a/arch/arm/boot/dts/mt6589.dtsi
> +++ b/arch/arm/boot/dts/mt6589.dtsi
> @@ -19,7 +19,7 @@
>
> / {
> compatible = "mediatek,mt6589";
> - interrupt-parent = <&gic>;
> + interrupt-parent = <&sysirq>;
This worries me a bit. Your sysirq cannot handle PPIs, and yet you make
it the top-level interrupt controller, without amending any PPI.
Does it mean you do not use *any* PPI? No per-cpu timer, nothing?
Thanks,
M.
--
Jazz is not dead. It just smells funny.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v7 3/4] ARM: mediatek: Add sysirq in mt6589/mt8135/mt8127 dtsi
2014-11-19 17:49 ` Marc Zyngier
@ 2014-11-20 3:29 ` Yingjoe Chen
0 siblings, 0 replies; 19+ messages in thread
From: Yingjoe Chen @ 2014-11-20 3:29 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Jiang Liu, Mark Rutland, Boris BREZILLON,
Russell King, Jason Cooper, Pawel Moll, devicetree, hc.yen,
srv_heupstream, yh.chen, linux-kernel, grant.likely, Yijing Wang,
Rob Herring, nathan.chung, yingjoe.chen, Matthias Brugger,
eddie.huang, Bjorn Helgaas, Sascha Hauer,
linux- arm-kernel@lists.infradead.org
On Wed, 2014-11-19 at 17:49 +0000, Marc Zyngier wrote:
> On Wed, Nov 19 2014 at 2:14:10 pm GMT, Yingjoe Chen <yingjoe.chen@mediatek.com> wrote:
> > Add sysirq settings for mt6589/mt8135/mt8127
> > This also correct timer interrupt flag. The old setting works
> > because boot loader already set polarity for timer interrupt.
> > Without intpol support, the setting was not changed so gic
> > can get the irq correctly.
> >
> > Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > ---
> > arch/arm/boot/dts/mt6589.dtsi | 14 ++++++++++++--
> > arch/arm/boot/dts/mt8127.dtsi | 14 ++++++++++++--
> > arch/arm/boot/dts/mt8135.dtsi | 14 ++++++++++++--
> > 3 files changed, 36 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/mt6589.dtsi b/arch/arm/boot/dts/mt6589.dtsi
> > index e3c7600..c91b2a9 100644
> > --- a/arch/arm/boot/dts/mt6589.dtsi
> > +++ b/arch/arm/boot/dts/mt6589.dtsi
> > @@ -19,7 +19,7 @@
> >
> > / {
> > compatible = "mediatek,mt6589";
> > - interrupt-parent = <&gic>;
> > + interrupt-parent = <&sysirq>;
>
> This worries me a bit. Your sysirq cannot handle PPIs, and yet you make
> it the top-level interrupt controller, without amending any PPI.
>
> Does it mean you do not use *any* PPI? No per-cpu timer, nothing?
Matthias had a patch to enable arch timer[1], but that is not merged
yet. Node using PPI interrupts must add their own interrupt-parent. This
works if we don't have node that use both PPI & SPI interrupts. For
timer, we could do this:
+ timer2: timer {
+ compatible = "arm,armv7-timer";
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) |
IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+ <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
+ clock-frequency = <13000000>;
+ };
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/277017.html
Joe.C
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v7 4/4] dt-bindings: add bindings for mediatek sysirq
2014-11-19 14:14 [PATCH v7 0/4] ARM: mediatek: Add support for interrupt polarity Yingjoe Chen
` (2 preceding siblings ...)
2014-11-19 14:14 ` [PATCH v7 3/4] ARM: mediatek: Add sysirq in mt6589/mt8135/mt8127 dtsi Yingjoe Chen
@ 2014-11-19 14:14 ` Yingjoe Chen
2014-11-19 18:07 ` Mark Rutland
3 siblings, 1 reply; 19+ messages in thread
From: Yingjoe Chen @ 2014-11-19 14:14 UTC (permalink / raw)
To: Thomas Gleixner, Jiang Liu, Marc Zyngier
Cc: Mark Rutland, Boris BREZILLON, Russell King, Jason Cooper,
Pawel Moll, devicetree, hc.yen, srv_heupstream, yh.chen,
linux-kernel, Grant Likely, Yijing Wang, Rob Herring,
nathan.chung, yingjoe.chen, Matthias Brugger, Yingjoe Chen,
eddie.huang, Bjorn Helgaas, Sascha Hauer, linux-arm-kernel
Add binding documentation for Mediatek SoC SYSIRQ.
Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
.../bindings/arm/mediatek/mediatek,sysirq.txt | 26 ++++++++++++++++++++++
1 file changed, 26 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
new file mode 100644
index 0000000..8669536
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
@@ -0,0 +1,26 @@
+Mediatek 65xx/81xx sysirq
+
+Mediatek SOCs sysirq support controllable irq inverter for each GIC SPI
+interrupt.
+
+Required properties:
+- compatible: should be one of:
+ "mediatek,mt8135-sysirq"
+ "mediatek,mt8127-sysirq"
+ "mediatek,mt6589-sysirq"
+ "mediatek,mt6582-sysirq"
+ "mediatek,mt6577-sysirq"
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Must use the same cells/format as parent controller.
+- interrupt-parent: phandle of irq domain parent for sysirq.
+- reg: Physical base address of the intpol registers and length of memory
+ mapped region.
+
+Example:
+ sysirq: interrupt-controller@10200100 {
+ compatible = "mediatek,mt6589-sysirq", "mediatek,mt6577-sysirq";
+ interrupt-controller;
+ #interrupt-cells = <3>;
+ interrupt-parent = <&gic>;
+ reg = <0 0x10200100 0 0x1c>;
+ };
--
1.8.1.1.dirty
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v7 4/4] dt-bindings: add bindings for mediatek sysirq
2014-11-19 14:14 ` [PATCH v7 4/4] dt-bindings: add bindings for mediatek sysirq Yingjoe Chen
@ 2014-11-19 18:07 ` Mark Rutland
2014-11-24 15:14 ` Yingjoe Chen
0 siblings, 1 reply; 19+ messages in thread
From: Mark Rutland @ 2014-11-19 18:07 UTC (permalink / raw)
To: Yingjoe Chen
Cc: Thomas Gleixner, Jiang Liu, Marc Zyngier, Boris BREZILLON,
Russell King, Jason Cooper, Pawel Moll, devicetree, hc.yen,
srv_heupstream, yh.chen, linux-kernel, grant.likely, Yijing Wang,
Rob Herring, nathan.chung, yingjoe.chen, Matthias Brugger,
eddie.huang, Bjorn Helgaas, Sascha Hauer, linux-arm-kernel
On Wed, Nov 19, 2014 at 02:14:11PM +0000, Yingjoe Chen wrote:
> Add binding documentation for Mediatek SoC SYSIRQ.
>
> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
> .../bindings/arm/mediatek/mediatek,sysirq.txt | 26 ++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
> new file mode 100644
> index 0000000..8669536
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
> @@ -0,0 +1,26 @@
> +Mediatek 65xx/81xx sysirq
> +
> +Mediatek SOCs sysirq support controllable irq inverter for each GIC SPI
> +interrupt.
> +
> +Required properties:
> +- compatible: should be one of:
> + "mediatek,mt8135-sysirq"
> + "mediatek,mt8127-sysirq"
> + "mediatek,mt6589-sysirq"
> + "mediatek,mt6582-sysirq"
> + "mediatek,mt6577-sysirq"
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Must use the same cells/format as parent controller.
> +- interrupt-parent: phandle of irq domain parent for sysirq.
I'm concerned that this sounds very general while the binding assumes
the GICv2 interrupt-specifier format. Either the driver needs to become
more general, or this needs to be tightened up.
It's also odd to say "irq domain parent", as that's purely a Linux
construct and has nothing to do with the HW.
Thanks,
Mark.
> +- reg: Physical base address of the intpol registers and length of memory
> + mapped region.
> +
> +Example:
> + sysirq: interrupt-controller@10200100 {
> + compatible = "mediatek,mt6589-sysirq", "mediatek,mt6577-sysirq";
> + interrupt-controller;
> + #interrupt-cells = <3>;
> + interrupt-parent = <&gic>;
> + reg = <0 0x10200100 0 0x1c>;
> + };
> --
> 1.8.1.1.dirty
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v7 4/4] dt-bindings: add bindings for mediatek sysirq
2014-11-19 18:07 ` Mark Rutland
@ 2014-11-24 15:14 ` Yingjoe Chen
0 siblings, 0 replies; 19+ messages in thread
From: Yingjoe Chen @ 2014-11-24 15:14 UTC (permalink / raw)
To: Mark Rutland
Cc: Thomas Gleixner, Jiang Liu, Marc Zyngier, Boris BREZILLON,
Russell King, Jason Cooper, Pawel Moll, devicetree, hc.yen,
srv_heupstream, yh.chen, linux-kernel, grant.likely, Yijing Wang,
Rob Herring, nathan.chung, yingjoe.chen, Matthias Brugger,
eddie.huang, Bjorn Helgaas, Sascha Hauer, linux-arm-kernel
Hi Mark,
On Wed, 2014-11-19 at 18:07 +0000, Mark Rutland wrote:
> On Wed, Nov 19, 2014 at 02:14:11PM +0000, Yingjoe Chen wrote:
> > Add binding documentation for Mediatek SoC SYSIRQ.
> >
> > Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > ---
> > .../bindings/arm/mediatek/mediatek,sysirq.txt | 26 ++++++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
> >
> > diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
> > new file mode 100644
> > index 0000000..8669536
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sysirq.txt
> > @@ -0,0 +1,26 @@
> > +Mediatek 65xx/81xx sysirq
> > +
> > +Mediatek SOCs sysirq support controllable irq inverter for each GIC SPI
> > +interrupt.
> > +
> > +Required properties:
> > +- compatible: should be one of:
> > + "mediatek,mt8135-sysirq"
> > + "mediatek,mt8127-sysirq"
> > + "mediatek,mt6589-sysirq"
> > + "mediatek,mt6582-sysirq"
> > + "mediatek,mt6577-sysirq"
> > +- interrupt-controller : Identifies the node as an interrupt controller
> > +- #interrupt-cells : Must use the same cells/format as parent controller.
> > +- interrupt-parent: phandle of irq domain parent for sysirq.
>
> I'm concerned that this sounds very general while the binding assumes
> the GICv2 interrupt-specifier format. Either the driver needs to become
> more general, or this needs to be tightened up.
>
> It's also odd to say "irq domain parent", as that's purely a Linux
> construct and has nothing to do with the HW.
The implementation expect the parent to use the same interrupt-cells
format as GIC. Based on the block diagram in the cover-letter, we could
say GIC is the irq parent of sysirq. So I'm planning to change to this,
hope this is OK.
- #interrupt-cells : Use the same format as specified by GIC in
Documentation/devicetree/bindings/arm/gic.txt
- interrupt-parent: phandle of irq parent for sysirq. The parent must
use the same interrupt-cells format as GIC.
Joe.C
^ permalink raw reply [flat|nested] 19+ messages in thread