* [PATCH v2 0/3] Add Mediatek CIRQ interrupt controller
@ 2016-11-01 11:51 Youlin Pei
2016-11-01 11:52 ` [PATCH v2 1/3] binding: irqchip: mtk-cirq: Add binding document Youlin Pei
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Youlin Pei @ 2016-11-01 11:51 UTC (permalink / raw)
To: Marc Zyngier, Rob Herring, Matthias Brugger
Cc: youlin.pei, Thomas Gleixner, Jason Cooper, Mark Rutland,
Russell King, linux-kernel, devicetree, linux-arm-kernel,
linux-mediatek, srv_heupstream, hongkun.cao, yong.wu, erin.lo,
chieh-jay.liu
In Mediatek SOCs, the CIRQ is a low power interrupt controller designed to
works outside MCUSYS which comprises with Cortex-Ax cores,CCI and GIC.
The CIRQ controller is integrated in between MCUSYS and interrupt sources
as the second level interrupt controller. The external interrupts which
outside MCUSYS will feed through CIRQ then bypass to GIC.
In normal mode(where MCUSYS is active), CIRQ is disabled and interrupts
will directly issue to MCUSYS. When MCUSYS enters sleep mode, where GIC
is power downed. CIRQ will be enabled and monitor all edge trigger
interrupts(only edge trigger interrupts will be lost in this scenario).
When an edge interrupt is triggered, CIRQ will record the status and
generated a pulse signal to GIC when flush command is executed.
With CIRQ, MCUSYS can be completely turned off to improve the system
power consumption without losing interrupts.
change in v2:
1. fix coding style issue.
2. change the compatible string.
3. resolve IRQ offset at alloc time.
4. clear irq status in irq_eoi function.
5. rebase on 4.9-rc1.
v1:
http://lists.infradead.org/pipermail/linux-mediatek/2016-October/007213.html
Youlin Pei (3):
binding: irqchip: mtk-cirq: Add binding document
irqchip: mtk-cirq: Add mediatek mtk-cirq implement
ARM: dts: mt2701: Add mtk-cirq node for mt2701
.../interrupt-controller/mediatek,cirq.txt | 30 +++
arch/arm/boot/dts/mt2701.dtsi | 11 +-
drivers/irqchip/Makefile | 2 +-
drivers/irqchip/irq-mtk-cirq.c | 262 +++++++++++++++++++++
4 files changed, 303 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,cirq.txt
create mode 100644 drivers/irqchip/irq-mtk-cirq.c
--
1.9.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 1/3] binding: irqchip: mtk-cirq: Add binding document 2016-11-01 11:51 [PATCH v2 0/3] Add Mediatek CIRQ interrupt controller Youlin Pei @ 2016-11-01 11:52 ` Youlin Pei 2016-11-09 18:26 ` Rob Herring 2016-11-01 11:52 ` [PATCH v2 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement Youlin Pei 2016-11-01 11:52 ` [PATCH v2 3/3] ARM: dts: mt2701: Add mtk-cirq node for mt2701 Youlin Pei 2 siblings, 1 reply; 12+ messages in thread From: Youlin Pei @ 2016-11-01 11:52 UTC (permalink / raw) To: Marc Zyngier, Rob Herring, Matthias Brugger Cc: youlin.pei, Thomas Gleixner, Jason Cooper, Mark Rutland, Russell King, linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, srv_heupstream, hongkun.cao, yong.wu, erin.lo, chieh-jay.liu This commit adds the device tree binding document for the mediatek cirq. Signed-off-by: Youlin Pei <youlin.pei@mediatek.com> --- .../interrupt-controller/mediatek,cirq.txt | 30 ++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,cirq.txt diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,cirq.txt b/Documentation/devicetree/bindings/interrupt-controller/mediatek,cirq.txt new file mode 100644 index 0000000..84e8123 --- /dev/null +++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,cirq.txt @@ -0,0 +1,30 @@ +* Mediatek 27xx cirq + +In Mediatek SOCs, the CIRQ is a low power interrupt controller designed to +works outside MCUSYS which comprises with Cortex-Ax cores,CCI and GIC. +The external interrupts (outside MCUSYS) will feed through CIRQ and connect +to GIC in MCUSYS. When CIRQ is enabled, it will record the edge-sensitive +interrupts and generated a pulse signal to parent interrupt controller when +flush command is executed. With CIRQ, MCUSYS can be completely turned off +to improve the system power consumption without losing interrupts. + +Required properties: +- compatible: should be: "mediatek,mtk-cirq". +- interrupt-controller : Identifies the node as an interrupt controller. +- #interrupt-cells : Use the same format as specified by GIC in arm,gic.txt. +- interrupt-parent: phandle of irq parent for cirq. The parent must + use the same interrupt-cells format as GIC. +- reg: Physical base address of the cirq registers and length of memory + mapped region. +- mediatek,ext-irq-start: Identifies external irq start number in different + SOCs. + +Example: + cirq: interrupt-controller@10204000 { + compatible = "mediatek,mtk-cirq"; + interrupt-controller; + #interrupt-cells = <3>; + interrupt-parent = <&sysirq>; + reg = <0 0x10204000 0 0x4000>; + mediatek,ext-irq-start = <32>; + }; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] binding: irqchip: mtk-cirq: Add binding document 2016-11-01 11:52 ` [PATCH v2 1/3] binding: irqchip: mtk-cirq: Add binding document Youlin Pei @ 2016-11-09 18:26 ` Rob Herring 2016-11-15 3:09 ` Youlin Pei 0 siblings, 1 reply; 12+ messages in thread From: Rob Herring @ 2016-11-09 18:26 UTC (permalink / raw) To: Youlin Pei Cc: Marc Zyngier, Matthias Brugger, Thomas Gleixner, Jason Cooper, Mark Rutland, Russell King, linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, srv_heupstream, hongkun.cao, yong.wu, erin.lo, chieh-jay.liu On Tue, Nov 01, 2016 at 07:52:00PM +0800, Youlin Pei wrote: > This commit adds the device tree binding document for > the mediatek cirq. > > Signed-off-by: Youlin Pei <youlin.pei@mediatek.com> > --- > .../interrupt-controller/mediatek,cirq.txt | 30 ++++++++++++++++++++ > 1 file changed, 30 insertions(+) > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,cirq.txt > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,cirq.txt b/Documentation/devicetree/bindings/interrupt-controller/mediatek,cirq.txt > new file mode 100644 > index 0000000..84e8123 > --- /dev/null > +++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,cirq.txt > @@ -0,0 +1,30 @@ > +* Mediatek 27xx cirq > + > +In Mediatek SOCs, the CIRQ is a low power interrupt controller designed to > +works outside MCUSYS which comprises with Cortex-Ax cores,CCI and GIC. s/works/work/ > +The external interrupts (outside MCUSYS) will feed through CIRQ and connect > +to GIC in MCUSYS. When CIRQ is enabled, it will record the edge-sensitive > +interrupts and generated a pulse signal to parent interrupt controller when s/generated/generate/ > +flush command is executed. With CIRQ, MCUSYS can be completely turned off > +to improve the system power consumption without losing interrupts. > + > +Required properties: > +- compatible: should be: "mediatek,mtk-cirq". This should be SoC specific. This is fine as a fallback if the same block is in many SoCs, but mediatek and mtk is a bit redundant. > +- interrupt-controller : Identifies the node as an interrupt controller. > +- #interrupt-cells : Use the same format as specified by GIC in arm,gic.txt. > +- interrupt-parent: phandle of irq parent for cirq. The parent must > + use the same interrupt-cells format as GIC. > +- reg: Physical base address of the cirq registers and length of memory > + mapped region. > +- mediatek,ext-irq-start: Identifies external irq start number in different > + SOCs. Wouldn't this always be 32 if the GIC is the parent? If 32 is the common case, then use the SoC compatible to determine this value. > + > +Example: > + cirq: interrupt-controller@10204000 { > + compatible = "mediatek,mtk-cirq"; > + interrupt-controller; > + #interrupt-cells = <3>; > + interrupt-parent = <&sysirq>; > + reg = <0 0x10204000 0 0x4000>; > + mediatek,ext-irq-start = <32>; > + }; > -- > 1.7.9.5 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] binding: irqchip: mtk-cirq: Add binding document 2016-11-09 18:26 ` Rob Herring @ 2016-11-15 3:09 ` Youlin Pei 0 siblings, 0 replies; 12+ messages in thread From: Youlin Pei @ 2016-11-15 3:09 UTC (permalink / raw) To: Rob Herring Cc: Marc Zyngier, Matthias Brugger, Thomas Gleixner, Jason Cooper, Mark Rutland, Russell King, linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, srv_heupstream, hongkun.cao, yong.wu, erin.lo, chieh-jay.liu On Wed, 2016-11-09 at 12:26 -0600, Rob Herring wrote: > On Tue, Nov 01, 2016 at 07:52:00PM +0800, Youlin Pei wrote: > > This commit adds the device tree binding document for > > the mediatek cirq. > > > > Signed-off-by: Youlin Pei <youlin.pei@mediatek.com> > > --- > > .../interrupt-controller/mediatek,cirq.txt | 30 ++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,cirq.txt > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,cirq.txt b/Documentation/devicetree/bindings/interrupt-controller/mediatek,cirq.txt > > new file mode 100644 > > index 0000000..84e8123 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,cirq.txt > > @@ -0,0 +1,30 @@ > > +* Mediatek 27xx cirq > > + > > +In Mediatek SOCs, the CIRQ is a low power interrupt controller designed to > > +works outside MCUSYS which comprises with Cortex-Ax cores,CCI and GIC. > > s/works/work/ will fix in next version. > > > +The external interrupts (outside MCUSYS) will feed through CIRQ and connect > > +to GIC in MCUSYS. When CIRQ is enabled, it will record the edge-sensitive > > +interrupts and generated a pulse signal to parent interrupt controller when > > s/generated/generate/ will fix in next version. > > > +flush command is executed. With CIRQ, MCUSYS can be completely turned off > > +to improve the system power consumption without losing interrupts. > > + > > +Required properties: > > +- compatible: should be: "mediatek,mtk-cirq". > > This should be SoC specific. This is fine as a fallback if the same > block is in many SoCs, but mediatek and mtk is a bit redundant. > In next version, i will improve as following: - compatible: Should be one of - "mediatek,mt2701-cirq" for mt2701 CIRQ - "mediatek,mt8173-cirq" for mt8173 CIRQ - "mediatek,mt8135-cirq" for mt8135 CIRQ and "mediatek,cirq" as a fallback. > > +- interrupt-controller : Identifies the node as an interrupt controller. > > +- #interrupt-cells : Use the same format as specified by GIC in arm,gic.txt. > > +- interrupt-parent: phandle of irq parent for cirq. The parent must > > + use the same interrupt-cells format as GIC. > > +- reg: Physical base address of the cirq registers and length of memory > > + mapped region. > > +- mediatek,ext-irq-start: Identifies external irq start number in different > > + SOCs. > > Wouldn't this always be 32 if the GIC is the parent? If 32 is the common > case, then use the SoC compatible to determine this value. > ext-irq-start is not always 32. In different Socs, this value is different. Thanks a lot! > > + > > +Example: > > + cirq: interrupt-controller@10204000 { > > + compatible = "mediatek,mtk-cirq"; > > + interrupt-controller; > > + #interrupt-cells = <3>; > > + interrupt-parent = <&sysirq>; > > + reg = <0 0x10204000 0 0x4000>; > > + mediatek,ext-irq-start = <32>; > > + }; > > -- > > 1.7.9.5 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement 2016-11-01 11:51 [PATCH v2 0/3] Add Mediatek CIRQ interrupt controller Youlin Pei 2016-11-01 11:52 ` [PATCH v2 1/3] binding: irqchip: mtk-cirq: Add binding document Youlin Pei @ 2016-11-01 11:52 ` Youlin Pei 2016-11-01 20:49 ` Marc Zyngier 2016-11-01 11:52 ` [PATCH v2 3/3] ARM: dts: mt2701: Add mtk-cirq node for mt2701 Youlin Pei 2 siblings, 1 reply; 12+ messages in thread From: Youlin Pei @ 2016-11-01 11:52 UTC (permalink / raw) To: Marc Zyngier, Rob Herring, Matthias Brugger Cc: youlin.pei, Thomas Gleixner, Jason Cooper, Mark Rutland, Russell King, linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, srv_heupstream, hongkun.cao, yong.wu, erin.lo, chieh-jay.liu In Mediatek SOCs, the CIRQ is a low power interrupt controller designed to works outside MCUSYS which comprises with Cortex-Ax cores,CCI and GIC. The CIRQ controller is integrated in between MCUSYS( include Cortex-Ax, CCI and GIC ) and interrupt sources as the second level interrupt controller. The external interrupts which outside MCUSYS will feed through CIRQ then bypass to GIC. CIRQ can monitors all edge trigger interupts. When an edge interrupt is triggered, CIRQ can record the status and generate a pulse signal to GIC when flush command executed. When system enters sleep mode, MCUSYS will be turned off to improve power consumption, also GIC is power down. The edge trigger interrupts will be lost in this scenario without CIRQ. This commit provides the CIRQ irqchip implement. Signed-off-by: Youlin Pei <youlin.pei@mediatek.com> --- drivers/irqchip/Makefile | 2 +- drivers/irqchip/irq-mtk-cirq.c | 262 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 263 insertions(+), 1 deletion(-) create mode 100644 drivers/irqchip/irq-mtk-cirq.c diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index e4dbfc8..8f33580 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -60,7 +60,7 @@ obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o -obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o +obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o irq-mtk-cirq.o obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o obj-$(CONFIG_RENESAS_H8300H_INTC) += irq-renesas-h8300h.o obj-$(CONFIG_RENESAS_H8S_INTC) += irq-renesas-h8s.o diff --git a/drivers/irqchip/irq-mtk-cirq.c b/drivers/irqchip/irq-mtk-cirq.c new file mode 100644 index 0000000..fc43ef3 --- /dev/null +++ b/drivers/irqchip/irq-mtk-cirq.c @@ -0,0 +1,262 @@ +/* + * Copyright (c) 2016 MediaTek Inc. + * Author: Youlin.Pei <youlin.pei@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/irqchip.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/syscore_ops.h> + +#define CIRQ_ACK 0x40 +#define CIRQ_MASK_SET 0xc0 +#define CIRQ_MASK_CLR 0x100 +#define CIRQ_SENS_SET 0x180 +#define CIRQ_SENS_CLR 0x1c0 +#define CIRQ_POL_SET 0x240 +#define CIRQ_POL_CLR 0x280 +#define CIRQ_CONTROL 0x300 + +#define CIRQ_EN 0x1 +#define CIRQ_EDGE 0x2 +#define CIRQ_FLUSH 0x4 + +#define CIRQ_IRQ_NUM 0x200 + +struct mtk_cirq_chip_data { + void __iomem *base; + unsigned int ext_irq_start; +}; + +static struct mtk_cirq_chip_data *cirq_data; + +static void mtk_cirq_write_mask(struct irq_data *data, unsigned int offset) +{ + struct mtk_cirq_chip_data *chip_data = data->chip_data; + unsigned int cirq_num = data->hwirq; + u32 mask = 1 << (cirq_num % 32); + + writel(mask, chip_data->base + offset + (cirq_num / 32) * 4); +} + +static void mtk_cirq_mask(struct irq_data *data) +{ + mtk_cirq_write_mask(data, CIRQ_MASK_SET); + irq_chip_mask_parent(data); +} + +static void mtk_cirq_unmask(struct irq_data *data) +{ + mtk_cirq_write_mask(data, CIRQ_MASK_CLR); + irq_chip_unmask_parent(data); +} + +static void mtk_cirq_eoi(struct irq_data *data) +{ + mtk_cirq_write_mask(data, CIRQ_ACK); + irq_chip_eoi_parent(data); +} + +static int mtk_cirq_set_type(struct irq_data *data, unsigned int type) +{ + int ret; + + switch (type & IRQ_TYPE_SENSE_MASK) { + case IRQ_TYPE_EDGE_FALLING: + mtk_cirq_write_mask(data, CIRQ_POL_CLR); + mtk_cirq_write_mask(data, CIRQ_SENS_CLR); + break; + case IRQ_TYPE_EDGE_RISING: + mtk_cirq_write_mask(data, CIRQ_POL_SET); + mtk_cirq_write_mask(data, CIRQ_SENS_CLR); + break; + case IRQ_TYPE_LEVEL_LOW: + mtk_cirq_write_mask(data, CIRQ_POL_CLR); + mtk_cirq_write_mask(data, CIRQ_SENS_SET); + break; + case IRQ_TYPE_LEVEL_HIGH: + mtk_cirq_write_mask(data, CIRQ_POL_SET); + mtk_cirq_write_mask(data, CIRQ_SENS_SET); + break; + default: + break; + } + + data = data->parent_data; + ret = data->chip->irq_set_type(data, type); + return ret; +} + +static struct irq_chip mtk_cirq_chip = { + .name = "MT_CIRQ", + .irq_mask = mtk_cirq_mask, + .irq_unmask = mtk_cirq_unmask, + .irq_eoi = mtk_cirq_eoi, + .irq_set_type = mtk_cirq_set_type, + .irq_retrigger = irq_chip_retrigger_hierarchy, +#ifdef CONFIG_SMP + .irq_set_affinity = irq_chip_set_affinity_parent, +#endif +}; + +static int mtk_cirq_domain_translate(struct irq_domain *d, + struct irq_fwspec *fwspec, + unsigned long *hwirq, + unsigned int *type) +{ + if (is_of_node(fwspec->fwnode)) { + if (fwspec->param_count != 3) + return -EINVAL; + + /* No PPI should point to this domain */ + if (fwspec->param[0] != 0) + return -EINVAL; + + /* cirq support irq number check */ + if (fwspec->param[1] < cirq_data->ext_irq_start) + return -EINVAL; + + *hwirq = fwspec->param[1] - cirq_data->ext_irq_start; + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; + return 0; + } + + return -EINVAL; +} + +static int mtk_cirq_domain_alloc(struct irq_domain *domain, unsigned int virq, + unsigned int nr_irqs, void *arg) +{ + int i; + irq_hw_number_t hwirq; + struct irq_fwspec *fwspec = arg; + struct irq_fwspec parent_fwspec = *fwspec; + + if (fwspec->param_count != 3) + return -EINVAL; + + /* cirq doesn't support PPI */ + if (fwspec->param[0]) + return -EINVAL; + + if (fwspec->param[1] < cirq_data->ext_irq_start) + return -EINVAL; + + hwirq = fwspec->param[1] - cirq_data->ext_irq_start; + for (i = 0; i < nr_irqs; i++) + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, + &mtk_cirq_chip, + domain->host_data); + + parent_fwspec.fwnode = domain->parent->fwnode; + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, + &parent_fwspec); +} + +static const struct irq_domain_ops cirq_domain_ops = { + .translate = mtk_cirq_domain_translate, + .alloc = mtk_cirq_domain_alloc, + .free = irq_domain_free_irqs_common, +}; + +#ifdef CONFIG_PM_SLEEP +static int mtk_cirq_suspend(void) +{ + u32 value; + + /* set edge_only mode, record edge-triggerd interrupts */ + /* enable cirq */ + value = readl_relaxed(cirq_data->base + CIRQ_CONTROL); + value |= (CIRQ_EDGE | CIRQ_EN); + writel(value, cirq_data->base + CIRQ_CONTROL); + return 0; +} + +static void mtk_cirq_resume(void) +{ + u32 value; + + /* flush recored interrupts, will send signals to parent controller */ + value = readl_relaxed(cirq_data->base + CIRQ_CONTROL); + writel(value | CIRQ_FLUSH, cirq_data->base + CIRQ_CONTROL); + + /* disable cirq */ + value = readl_relaxed(cirq_data->base + CIRQ_CONTROL); + value &= ~(CIRQ_EDGE | CIRQ_EN); + writel(value, cirq_data->base + CIRQ_CONTROL); +} + +static struct syscore_ops mtk_cirq_syscore_ops = { + .suspend = mtk_cirq_suspend, + .resume = mtk_cirq_resume, +}; + +static void mtk_cirq_syscore_init(void) +{ + register_syscore_ops(&mtk_cirq_syscore_ops); +} +#else +static inline void mtk_cirq_syscore_init(void) {} +#endif + +static int __init mtk_cirq_of_init(struct device_node *node, + struct device_node *parent) +{ + struct irq_domain *domain, *domain_parent; + int ret; + + domain_parent = irq_find_host(parent); + if (!domain_parent) { + pr_err("mtk_cirq: interrupt-parent not found\n"); + return -EINVAL; + } + + cirq_data = kzalloc(sizeof(*cirq_data), GFP_KERNEL); + if (!cirq_data) + return -ENOMEM; + + cirq_data->base = of_iomap(node, 0); + if (!cirq_data->base) { + pr_err("mtk_cirq: unable to map cirq register\n"); + ret = -ENXIO; + goto out_free; + } + + ret = of_property_read_u32(node, "mediatek,ext-irq-start", + &cirq_data->ext_irq_start); + if (ret) + goto out_unmap; + + domain = irq_domain_add_hierarchy(domain_parent, 0, CIRQ_IRQ_NUM, node, + &cirq_domain_ops, cirq_data); + if (!domain) { + ret = -ENOMEM; + goto out_unmap; + } + + mtk_cirq_syscore_init(); + + return 0; + +out_unmap: + iounmap(cirq_data->base); +out_free: + kfree(cirq_data); + return ret; +} + +IRQCHIP_DECLARE(mtk_cirq, "mediatek,mtk-cirq", mtk_cirq_of_init); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement 2016-11-01 11:52 ` [PATCH v2 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement Youlin Pei @ 2016-11-01 20:49 ` Marc Zyngier 2016-11-04 4:42 ` Youlin Pei 0 siblings, 1 reply; 12+ messages in thread From: Marc Zyngier @ 2016-11-01 20:49 UTC (permalink / raw) To: Youlin Pei Cc: Rob Herring, Matthias Brugger, Thomas Gleixner, Jason Cooper, Mark Rutland, Russell King, linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, srv_heupstream, hongkun.cao, yong.wu, erin.lo, chieh-jay.liu On Tue, Nov 01 2016 at 11:52:01 AM, Youlin Pei <youlin.pei@mediatek.com> wrote: > In Mediatek SOCs, the CIRQ is a low power interrupt controller > designed to works outside MCUSYS which comprises with Cortex-Ax > cores,CCI and GIC. > > The CIRQ controller is integrated in between MCUSYS( include > Cortex-Ax, CCI and GIC ) and interrupt sources as the second > level interrupt controller. The external interrupts which outside > MCUSYS will feed through CIRQ then bypass to GIC. CIRQ can monitors > all edge trigger interupts. When an edge interrupt is triggered, > CIRQ can record the status and generate a pulse signal to GIC when > flush command executed. > > When system enters sleep mode, MCUSYS will be turned off to improve > power consumption, also GIC is power down. The edge trigger interrupts > will be lost in this scenario without CIRQ. > > This commit provides the CIRQ irqchip implement. > > Signed-off-by: Youlin Pei <youlin.pei@mediatek.com> > --- > drivers/irqchip/Makefile | 2 +- > drivers/irqchip/irq-mtk-cirq.c | 262 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 263 insertions(+), 1 deletion(-) > create mode 100644 drivers/irqchip/irq-mtk-cirq.c > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index e4dbfc8..8f33580 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -60,7 +60,7 @@ obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o > obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o > obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o > obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o > -obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o > +obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o irq-mtk-cirq.o > obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o > obj-$(CONFIG_RENESAS_H8300H_INTC) += irq-renesas-h8300h.o > obj-$(CONFIG_RENESAS_H8S_INTC) += irq-renesas-h8s.o > diff --git a/drivers/irqchip/irq-mtk-cirq.c b/drivers/irqchip/irq-mtk-cirq.c > new file mode 100644 > index 0000000..fc43ef3 > --- /dev/null > +++ b/drivers/irqchip/irq-mtk-cirq.c > @@ -0,0 +1,262 @@ > +/* > + * Copyright (c) 2016 MediaTek Inc. > + * Author: Youlin.Pei <youlin.pei@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/irqchip.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/syscore_ops.h> > + > +#define CIRQ_ACK 0x40 > +#define CIRQ_MASK_SET 0xc0 > +#define CIRQ_MASK_CLR 0x100 > +#define CIRQ_SENS_SET 0x180 > +#define CIRQ_SENS_CLR 0x1c0 > +#define CIRQ_POL_SET 0x240 > +#define CIRQ_POL_CLR 0x280 > +#define CIRQ_CONTROL 0x300 > + > +#define CIRQ_EN 0x1 > +#define CIRQ_EDGE 0x2 > +#define CIRQ_FLUSH 0x4 > + > +#define CIRQ_IRQ_NUM 0x200 > + > +struct mtk_cirq_chip_data { > + void __iomem *base; > + unsigned int ext_irq_start; > +}; > + > +static struct mtk_cirq_chip_data *cirq_data; Are you guaranteed that you'll only ever have a single CIRQ in any system? > + > +static void mtk_cirq_write_mask(struct irq_data *data, unsigned int offset) > +{ > + struct mtk_cirq_chip_data *chip_data = data->chip_data; > + unsigned int cirq_num = data->hwirq; > + u32 mask = 1 << (cirq_num % 32); > + > + writel(mask, chip_data->base + offset + (cirq_num / 32) * 4); Why can't you use the relaxed accessors? > +} > + > +static void mtk_cirq_mask(struct irq_data *data) > +{ > + mtk_cirq_write_mask(data, CIRQ_MASK_SET); > + irq_chip_mask_parent(data); > +} > + > +static void mtk_cirq_unmask(struct irq_data *data) > +{ > + mtk_cirq_write_mask(data, CIRQ_MASK_CLR); > + irq_chip_unmask_parent(data); > +} > + > +static void mtk_cirq_eoi(struct irq_data *data) > +{ > + mtk_cirq_write_mask(data, CIRQ_ACK); EOI and ACK have very different semantics. What is this write actually doing? Also, you're now doing an additional MMIO write on each interrupt EOI, doubling its cost. Do you really need to do actually signal the HW that we've EOIed an interrupt? I would have hoped that you'd be able to put it in "bypass" mode as long as you're not suspending... > + irq_chip_eoi_parent(data); > +} > + > +static int mtk_cirq_set_type(struct irq_data *data, unsigned int type) > +{ > + int ret; > + > + switch (type & IRQ_TYPE_SENSE_MASK) { > + case IRQ_TYPE_EDGE_FALLING: > + mtk_cirq_write_mask(data, CIRQ_POL_CLR); > + mtk_cirq_write_mask(data, CIRQ_SENS_CLR); > + break; > + case IRQ_TYPE_EDGE_RISING: > + mtk_cirq_write_mask(data, CIRQ_POL_SET); > + mtk_cirq_write_mask(data, CIRQ_SENS_CLR); > + break; > + case IRQ_TYPE_LEVEL_LOW: > + mtk_cirq_write_mask(data, CIRQ_POL_CLR); > + mtk_cirq_write_mask(data, CIRQ_SENS_SET); > + break; > + case IRQ_TYPE_LEVEL_HIGH: > + mtk_cirq_write_mask(data, CIRQ_POL_SET); > + mtk_cirq_write_mask(data, CIRQ_SENS_SET); > + break; > + default: > + break; > + } > + > + data = data->parent_data; > + ret = data->chip->irq_set_type(data, type); > + return ret; > +} > + > +static struct irq_chip mtk_cirq_chip = { > + .name = "MT_CIRQ", > + .irq_mask = mtk_cirq_mask, > + .irq_unmask = mtk_cirq_unmask, > + .irq_eoi = mtk_cirq_eoi, > + .irq_set_type = mtk_cirq_set_type, > + .irq_retrigger = irq_chip_retrigger_hierarchy, > +#ifdef CONFIG_SMP > + .irq_set_affinity = irq_chip_set_affinity_parent, > +#endif > +}; > + > +static int mtk_cirq_domain_translate(struct irq_domain *d, > + struct irq_fwspec *fwspec, > + unsigned long *hwirq, > + unsigned int *type) > +{ > + if (is_of_node(fwspec->fwnode)) { > + if (fwspec->param_count != 3) > + return -EINVAL; > + > + /* No PPI should point to this domain */ > + if (fwspec->param[0] != 0) > + return -EINVAL; > + > + /* cirq support irq number check */ > + if (fwspec->param[1] < cirq_data->ext_irq_start) > + return -EINVAL; > + > + *hwirq = fwspec->param[1] - cirq_data->ext_irq_start; What if the result is > CIRQ_IRQ_NUM? > + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > + return 0; > + } > + > + return -EINVAL; > +} > + > +static int mtk_cirq_domain_alloc(struct irq_domain *domain, unsigned int virq, > + unsigned int nr_irqs, void *arg) > +{ > + int i; > + irq_hw_number_t hwirq; > + struct irq_fwspec *fwspec = arg; > + struct irq_fwspec parent_fwspec = *fwspec; > + > + if (fwspec->param_count != 3) > + return -EINVAL; > + > + /* cirq doesn't support PPI */ > + if (fwspec->param[0]) > + return -EINVAL; > + > + if (fwspec->param[1] < cirq_data->ext_irq_start) > + return -EINVAL; > + > + hwirq = fwspec->param[1] - cirq_data->ext_irq_start; All this is a pure copy of mtk_cirq_domain_translate(). Please use it. > + for (i = 0; i < nr_irqs; i++) > + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, > + &mtk_cirq_chip, > + domain->host_data); This is a bit silly. This loop only exists for the benefit of MSI support, which we're not dealing with here. So please stick a if (WARN_ON(nr_irqs != 1)) return -EINVAL; and drop the loop. > + > + parent_fwspec.fwnode = domain->parent->fwnode; > + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, > + &parent_fwspec); > +} > + > +static const struct irq_domain_ops cirq_domain_ops = { > + .translate = mtk_cirq_domain_translate, > + .alloc = mtk_cirq_domain_alloc, > + .free = irq_domain_free_irqs_common, > +}; > + > +#ifdef CONFIG_PM_SLEEP > +static int mtk_cirq_suspend(void) > +{ > + u32 value; > + > + /* set edge_only mode, record edge-triggerd interrupts */ > + /* enable cirq */ > + value = readl_relaxed(cirq_data->base + CIRQ_CONTROL); > + value |= (CIRQ_EDGE | CIRQ_EN); > + writel(value, cirq_data->base + CIRQ_CONTROL); You're mixing relaxed and non-relaxed accessors. Why? > + return 0; > +} > + > +static void mtk_cirq_resume(void) > +{ > + u32 value; > + > + /* flush recored interrupts, will send signals to parent controller */ > + value = readl_relaxed(cirq_data->base + CIRQ_CONTROL); > + writel(value | CIRQ_FLUSH, cirq_data->base + CIRQ_CONTROL); Same remark. > + > + /* disable cirq */ > + value = readl_relaxed(cirq_data->base + CIRQ_CONTROL); > + value &= ~(CIRQ_EDGE | CIRQ_EN); > + writel(value, cirq_data->base + CIRQ_CONTROL); So from this, I infer that CIRQ is not enabled when the kernel is running (not suspended). It really makes me wonder why you need to do anything in the EOI callback. > +} > + > +static struct syscore_ops mtk_cirq_syscore_ops = { > + .suspend = mtk_cirq_suspend, > + .resume = mtk_cirq_resume, > +}; > + > +static void mtk_cirq_syscore_init(void) > +{ > + register_syscore_ops(&mtk_cirq_syscore_ops); > +} > +#else > +static inline void mtk_cirq_syscore_init(void) {} > +#endif > + > +static int __init mtk_cirq_of_init(struct device_node *node, > + struct device_node *parent) > +{ > + struct irq_domain *domain, *domain_parent; > + int ret; > + > + domain_parent = irq_find_host(parent); > + if (!domain_parent) { > + pr_err("mtk_cirq: interrupt-parent not found\n"); > + return -EINVAL; > + } > + > + cirq_data = kzalloc(sizeof(*cirq_data), GFP_KERNEL); > + if (!cirq_data) > + return -ENOMEM; > + > + cirq_data->base = of_iomap(node, 0); > + if (!cirq_data->base) { > + pr_err("mtk_cirq: unable to map cirq register\n"); > + ret = -ENXIO; > + goto out_free; > + } > + > + ret = of_property_read_u32(node, "mediatek,ext-irq-start", > + &cirq_data->ext_irq_start); > + if (ret) > + goto out_unmap; > + > + domain = irq_domain_add_hierarchy(domain_parent, 0, CIRQ_IRQ_NUM, node, > + &cirq_domain_ops, cirq_data); So you support at most 512 interrupts, and yet the GIC supports up to 987 SPIs. What happens for interrupt lines that out of the CIRQ range? Maybe having an explicit range in DT would be a good thing. That also brings back the question of having a single CIRQ in the system... > + if (!domain) { > + ret = -ENOMEM; > + goto out_unmap; > + } > + > + mtk_cirq_syscore_init(); > + > + return 0; > + > +out_unmap: > + iounmap(cirq_data->base); > +out_free: > + kfree(cirq_data); > + return ret; > +} > + > +IRQCHIP_DECLARE(mtk_cirq, "mediatek,mtk-cirq", mtk_cirq_of_init); Thanks, M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement 2016-11-01 20:49 ` Marc Zyngier @ 2016-11-04 4:42 ` Youlin Pei 2016-11-04 22:21 ` Marc Zyngier 0 siblings, 1 reply; 12+ messages in thread From: Youlin Pei @ 2016-11-04 4:42 UTC (permalink / raw) To: Marc Zyngier Cc: Rob Herring, Matthias Brugger, Thomas Gleixner, Jason Cooper, Mark Rutland, Russell King, linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, srv_heupstream, hongkun.cao, yong.wu, erin.lo, chieh-jay.liu On Tue, 2016-11-01 at 20:49 +0000, Marc Zyngier wrote: > On Tue, Nov 01 2016 at 11:52:01 AM, Youlin Pei <youlin.pei@mediatek.com> wrote: > > In Mediatek SOCs, the CIRQ is a low power interrupt controller > > designed to works outside MCUSYS which comprises with Cortex-Ax > > cores,CCI and GIC. > > > > The CIRQ controller is integrated in between MCUSYS( include > > Cortex-Ax, CCI and GIC ) and interrupt sources as the second > > level interrupt controller. The external interrupts which outside > > MCUSYS will feed through CIRQ then bypass to GIC. CIRQ can monitors > > all edge trigger interupts. When an edge interrupt is triggered, > > CIRQ can record the status and generate a pulse signal to GIC when > > flush command executed. > > > > When system enters sleep mode, MCUSYS will be turned off to improve > > power consumption, also GIC is power down. The edge trigger interrupts > > will be lost in this scenario without CIRQ. > > > > This commit provides the CIRQ irqchip implement. > > > > Signed-off-by: Youlin Pei <youlin.pei@mediatek.com> > > --- > > drivers/irqchip/Makefile | 2 +- > > drivers/irqchip/irq-mtk-cirq.c | 262 ++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 263 insertions(+), 1 deletion(-) > > create mode 100644 drivers/irqchip/irq-mtk-cirq.c > > > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > > index e4dbfc8..8f33580 100644 > > --- a/drivers/irqchip/Makefile > > +++ b/drivers/irqchip/Makefile > > @@ -60,7 +60,7 @@ obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o > > obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o > > obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o > > obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o > > -obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o > > +obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o irq-mtk-cirq.o > > obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o > > obj-$(CONFIG_RENESAS_H8300H_INTC) += irq-renesas-h8300h.o > > obj-$(CONFIG_RENESAS_H8S_INTC) += irq-renesas-h8s.o > > diff --git a/drivers/irqchip/irq-mtk-cirq.c b/drivers/irqchip/irq-mtk-cirq.c > > new file mode 100644 > > index 0000000..fc43ef3 > > --- /dev/null > > +++ b/drivers/irqchip/irq-mtk-cirq.c > > @@ -0,0 +1,262 @@ > > +/* > > + * Copyright (c) 2016 MediaTek Inc. > > + * Author: Youlin.Pei <youlin.pei@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/irqchip.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/syscore_ops.h> > > + > > +#define CIRQ_ACK 0x40 > > +#define CIRQ_MASK_SET 0xc0 > > +#define CIRQ_MASK_CLR 0x100 > > +#define CIRQ_SENS_SET 0x180 > > +#define CIRQ_SENS_CLR 0x1c0 > > +#define CIRQ_POL_SET 0x240 > > +#define CIRQ_POL_CLR 0x280 > > +#define CIRQ_CONTROL 0x300 > > + > > +#define CIRQ_EN 0x1 > > +#define CIRQ_EDGE 0x2 > > +#define CIRQ_FLUSH 0x4 > > + > > +#define CIRQ_IRQ_NUM 0x200 > > + > > +struct mtk_cirq_chip_data { > > + void __iomem *base; > > + unsigned int ext_irq_start; > > +}; > > + > > +static struct mtk_cirq_chip_data *cirq_data; > > Are you guaranteed that you'll only ever have a single CIRQ in any > system? In Mediatek's SOC, only hace a single CIRQ. > > > + > > +static void mtk_cirq_write_mask(struct irq_data *data, unsigned int offset) > > +{ > > + struct mtk_cirq_chip_data *chip_data = data->chip_data; > > + unsigned int cirq_num = data->hwirq; > > + u32 mask = 1 << (cirq_num % 32); > > + > > + writel(mask, chip_data->base + offset + (cirq_num / 32) * 4); > > Why can't you use the relaxed accessors? It seems that i use wrong function, i will change the writel to writel_relaxed in next version. > > > +} > > + > > +static void mtk_cirq_mask(struct irq_data *data) > > +{ > > + mtk_cirq_write_mask(data, CIRQ_MASK_SET); > > + irq_chip_mask_parent(data); > > +} > > + > > +static void mtk_cirq_unmask(struct irq_data *data) > > +{ > > + mtk_cirq_write_mask(data, CIRQ_MASK_CLR); > > + irq_chip_unmask_parent(data); > > +} > > + > > +static void mtk_cirq_eoi(struct irq_data *data) > > +{ > > + mtk_cirq_write_mask(data, CIRQ_ACK); > > EOI and ACK have very different semantics. What is this write actually > doing? Also, you're now doing an additional MMIO write on each interrupt > EOI, doubling its cost. Do you really need to do actually signal the HW > that we've EOIed an interrupt? I would have hoped that you'd be able to > put it in "bypass" mode as long as you're not suspending... > When external interrupt happened, CIRQ status register record the status even CIRQ is not enabled. when execute the flush command, CIRQ will resend the signals according to the status. So if don't clear the status, CIRQ will resend the wrong signals. the ACK write operation will clear the status. > > + irq_chip_eoi_parent(data); > > +} > > + > > +static int mtk_cirq_set_type(struct irq_data *data, unsigned int type) > > +{ > > + int ret; > > + > > + switch (type & IRQ_TYPE_SENSE_MASK) { > > + case IRQ_TYPE_EDGE_FALLING: > > + mtk_cirq_write_mask(data, CIRQ_POL_CLR); > > + mtk_cirq_write_mask(data, CIRQ_SENS_CLR); > > + break; > > + case IRQ_TYPE_EDGE_RISING: > > + mtk_cirq_write_mask(data, CIRQ_POL_SET); > > + mtk_cirq_write_mask(data, CIRQ_SENS_CLR); > > + break; > > + case IRQ_TYPE_LEVEL_LOW: > > + mtk_cirq_write_mask(data, CIRQ_POL_CLR); > > + mtk_cirq_write_mask(data, CIRQ_SENS_SET); > > + break; > > + case IRQ_TYPE_LEVEL_HIGH: > > + mtk_cirq_write_mask(data, CIRQ_POL_SET); > > + mtk_cirq_write_mask(data, CIRQ_SENS_SET); > > + break; > > + default: > > + break; > > + } > > + > > + data = data->parent_data; > > + ret = data->chip->irq_set_type(data, type); > > + return ret; > > +} > > + > > +static struct irq_chip mtk_cirq_chip = { > > + .name = "MT_CIRQ", > > + .irq_mask = mtk_cirq_mask, > > + .irq_unmask = mtk_cirq_unmask, > > + .irq_eoi = mtk_cirq_eoi, > > + .irq_set_type = mtk_cirq_set_type, > > + .irq_retrigger = irq_chip_retrigger_hierarchy, > > +#ifdef CONFIG_SMP > > + .irq_set_affinity = irq_chip_set_affinity_parent, > > +#endif > > +}; > > + > > +static int mtk_cirq_domain_translate(struct irq_domain *d, > > + struct irq_fwspec *fwspec, > > + unsigned long *hwirq, > > + unsigned int *type) > > +{ > > + if (is_of_node(fwspec->fwnode)) { > > + if (fwspec->param_count != 3) > > + return -EINVAL; > > + > > + /* No PPI should point to this domain */ > > + if (fwspec->param[0] != 0) > > + return -EINVAL; > > + > > + /* cirq support irq number check */ > > + if (fwspec->param[1] < cirq_data->ext_irq_start) > > + return -EINVAL; > > + > > + *hwirq = fwspec->param[1] - cirq_data->ext_irq_start; > > What if the result is > CIRQ_IRQ_NUM? will add CIRQ supported irq range in DT, and do range check here in next version. > > > + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > > + return 0; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int mtk_cirq_domain_alloc(struct irq_domain *domain, unsigned int virq, > > + unsigned int nr_irqs, void *arg) > > +{ > > + int i; > > + irq_hw_number_t hwirq; > > + struct irq_fwspec *fwspec = arg; > > + struct irq_fwspec parent_fwspec = *fwspec; > > + > > + if (fwspec->param_count != 3) > > + return -EINVAL; > > + > > + /* cirq doesn't support PPI */ > > + if (fwspec->param[0]) > > + return -EINVAL; > > + > > + if (fwspec->param[1] < cirq_data->ext_irq_start) > > + return -EINVAL; > > + > > + hwirq = fwspec->param[1] - cirq_data->ext_irq_start; > > All this is a pure copy of mtk_cirq_domain_translate(). Please use it. will fix in next version. > > + for (i = 0; i < nr_irqs; i++) > > + irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i, > > + &mtk_cirq_chip, > > + domain->host_data); > > This is a bit silly. This loop only exists for the benefit of MSI > support, which we're not dealing with here. So please stick a > > if (WARN_ON(nr_irqs != 1)) > return -EINVAL; > > and drop the loop. will fix in next version. > > > + > > + parent_fwspec.fwnode = domain->parent->fwnode; > > + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, > > + &parent_fwspec); > > +} > > + > > +static const struct irq_domain_ops cirq_domain_ops = { > > + .translate = mtk_cirq_domain_translate, > > + .alloc = mtk_cirq_domain_alloc, > > + .free = irq_domain_free_irqs_common, > > +}; > > + > > +#ifdef CONFIG_PM_SLEEP > > +static int mtk_cirq_suspend(void) > > +{ > > + u32 value; > > + > > + /* set edge_only mode, record edge-triggerd interrupts */ > > + /* enable cirq */ > > + value = readl_relaxed(cirq_data->base + CIRQ_CONTROL); > > + value |= (CIRQ_EDGE | CIRQ_EN); > > + writel(value, cirq_data->base + CIRQ_CONTROL); > > You're mixing relaxed and non-relaxed accessors. Why? will change writel to writel_relaxed in next version. > > > + return 0; > > +} > > + > > +static void mtk_cirq_resume(void) > > +{ > > + u32 value; > > + > > + /* flush recored interrupts, will send signals to parent controller */ > > + value = readl_relaxed(cirq_data->base + CIRQ_CONTROL); > > + writel(value | CIRQ_FLUSH, cirq_data->base + CIRQ_CONTROL); > > Same remark. will change writel to writel_relaxed in next version. > > > + > > + /* disable cirq */ > > + value = readl_relaxed(cirq_data->base + CIRQ_CONTROL); > > + value &= ~(CIRQ_EDGE | CIRQ_EN); > > + writel(value, cirq_data->base + CIRQ_CONTROL); > > So from this, I infer that CIRQ is not enabled when the kernel is > running (not suspended). It really makes me wonder why you need to do > anything in the EOI callback. > > > +} > > + > > +static struct syscore_ops mtk_cirq_syscore_ops = { > > + .suspend = mtk_cirq_suspend, > > + .resume = mtk_cirq_resume, > > +}; > > + > > +static void mtk_cirq_syscore_init(void) > > +{ > > + register_syscore_ops(&mtk_cirq_syscore_ops); > > +} > > +#else > > +static inline void mtk_cirq_syscore_init(void) {} > > +#endif > > + > > +static int __init mtk_cirq_of_init(struct device_node *node, > > + struct device_node *parent) > > +{ > > + struct irq_domain *domain, *domain_parent; > > + int ret; > > + > > + domain_parent = irq_find_host(parent); > > + if (!domain_parent) { > > + pr_err("mtk_cirq: interrupt-parent not found\n"); > > + return -EINVAL; > > + } > > + > > + cirq_data = kzalloc(sizeof(*cirq_data), GFP_KERNEL); > > + if (!cirq_data) > > + return -ENOMEM; > > + > > + cirq_data->base = of_iomap(node, 0); > > + if (!cirq_data->base) { > > + pr_err("mtk_cirq: unable to map cirq register\n"); > > + ret = -ENXIO; > > + goto out_free; > > + } > > + > > + ret = of_property_read_u32(node, "mediatek,ext-irq-start", > > + &cirq_data->ext_irq_start); > > + if (ret) > > + goto out_unmap; > > + > > + domain = irq_domain_add_hierarchy(domain_parent, 0, CIRQ_IRQ_NUM, node, > > + &cirq_domain_ops, cirq_data); > > So you support at most 512 interrupts, and yet the GIC supports up to > 987 SPIs. What happens for interrupt lines that out of the CIRQ range? > Maybe having an explicit range in DT would be a good thing. That also > brings back the question of having a single CIRQ in the system... > In next version, i will add an explicit CIRQ supported range in DT. Thanks a lot! > > + if (!domain) { > > + ret = -ENOMEM; > > + goto out_unmap; > > + } > > + > > + mtk_cirq_syscore_init(); > > + > > + return 0; > > + > > +out_unmap: > > + iounmap(cirq_data->base); > > +out_free: > > + kfree(cirq_data); > > + return ret; > > +} > > + > > +IRQCHIP_DECLARE(mtk_cirq, "mediatek,mtk-cirq", mtk_cirq_of_init); > > Thanks, > > M. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement 2016-11-04 4:42 ` Youlin Pei @ 2016-11-04 22:21 ` Marc Zyngier 2016-11-08 2:57 ` Youlin Pei 0 siblings, 1 reply; 12+ messages in thread From: Marc Zyngier @ 2016-11-04 22:21 UTC (permalink / raw) To: Youlin Pei Cc: Rob Herring, Matthias Brugger, Thomas Gleixner, Jason Cooper, Mark Rutland, Russell King, linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, srv_heupstream, hongkun.cao, yong.wu, erin.lo, chieh-jay.liu On Fri, Nov 04 2016 at 04:42:57 AM, Youlin Pei <youlin.pei@mediatek.com> wrote: > On Tue, 2016-11-01 at 20:49 +0000, Marc Zyngier wrote: >> On Tue, Nov 01 2016 at 11:52:01 AM, Youlin Pei <youlin.pei@mediatek.com> wrote: >> > In Mediatek SOCs, the CIRQ is a low power interrupt controller >> > designed to works outside MCUSYS which comprises with Cortex-Ax >> > cores,CCI and GIC. >> > >> > The CIRQ controller is integrated in between MCUSYS( include >> > Cortex-Ax, CCI and GIC ) and interrupt sources as the second >> > level interrupt controller. The external interrupts which outside >> > MCUSYS will feed through CIRQ then bypass to GIC. CIRQ can monitors >> > all edge trigger interupts. When an edge interrupt is triggered, >> > CIRQ can record the status and generate a pulse signal to GIC when >> > flush command executed. >> > >> > When system enters sleep mode, MCUSYS will be turned off to improve >> > power consumption, also GIC is power down. The edge trigger interrupts >> > will be lost in this scenario without CIRQ. >> > >> > This commit provides the CIRQ irqchip implement. >> > >> > Signed-off-by: Youlin Pei <youlin.pei@mediatek.com> >> > --- >> > drivers/irqchip/Makefile | 2 +- >> > drivers/irqchip/irq-mtk-cirq.c | 262 ++++++++++++++++++++++++++++++++++++++++ >> > 2 files changed, 263 insertions(+), 1 deletion(-) >> > create mode 100644 drivers/irqchip/irq-mtk-cirq.c >> > >> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile >> > index e4dbfc8..8f33580 100644 >> > --- a/drivers/irqchip/Makefile >> > +++ b/drivers/irqchip/Makefile >> > @@ -60,7 +60,7 @@ obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o >> > obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o >> > obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o >> > obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o >> > -obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o >> > +obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o irq-mtk-cirq.o >> > obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o >> > obj-$(CONFIG_RENESAS_H8300H_INTC) += irq-renesas-h8300h.o >> > obj-$(CONFIG_RENESAS_H8S_INTC) += irq-renesas-h8s.o >> > diff --git a/drivers/irqchip/irq-mtk-cirq.c b/drivers/irqchip/irq-mtk-cirq.c >> > new file mode 100644 >> > index 0000000..fc43ef3 >> > --- /dev/null >> > +++ b/drivers/irqchip/irq-mtk-cirq.c >> > @@ -0,0 +1,262 @@ >> > +/* >> > + * Copyright (c) 2016 MediaTek Inc. >> > + * Author: Youlin.Pei <youlin.pei@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/irqchip.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/syscore_ops.h> >> > + >> > +#define CIRQ_ACK 0x40 >> > +#define CIRQ_MASK_SET 0xc0 >> > +#define CIRQ_MASK_CLR 0x100 >> > +#define CIRQ_SENS_SET 0x180 >> > +#define CIRQ_SENS_CLR 0x1c0 >> > +#define CIRQ_POL_SET 0x240 >> > +#define CIRQ_POL_CLR 0x280 >> > +#define CIRQ_CONTROL 0x300 >> > + >> > +#define CIRQ_EN 0x1 >> > +#define CIRQ_EDGE 0x2 >> > +#define CIRQ_FLUSH 0x4 >> > + >> > +#define CIRQ_IRQ_NUM 0x200 >> > + >> > +struct mtk_cirq_chip_data { >> > + void __iomem *base; >> > + unsigned int ext_irq_start; >> > +}; >> > + >> > +static struct mtk_cirq_chip_data *cirq_data; >> >> Are you guaranteed that you'll only ever have a single CIRQ in any >> system? > > In Mediatek's SOC, only hace a single CIRQ. > >> >> > + >> > +static void mtk_cirq_write_mask(struct irq_data *data, unsigned int offset) >> > +{ >> > + struct mtk_cirq_chip_data *chip_data = data->chip_data; >> > + unsigned int cirq_num = data->hwirq; >> > + u32 mask = 1 << (cirq_num % 32); >> > + >> > + writel(mask, chip_data->base + offset + (cirq_num / 32) * 4); >> >> Why can't you use the relaxed accessors? > > It seems that i use wrong function, i will change the writel to > writel_relaxed in next version. > >> >> > +} >> > + >> > +static void mtk_cirq_mask(struct irq_data *data) >> > +{ >> > + mtk_cirq_write_mask(data, CIRQ_MASK_SET); >> > + irq_chip_mask_parent(data); >> > +} >> > + >> > +static void mtk_cirq_unmask(struct irq_data *data) >> > +{ >> > + mtk_cirq_write_mask(data, CIRQ_MASK_CLR); >> > + irq_chip_unmask_parent(data); >> > +} >> > + >> > +static void mtk_cirq_eoi(struct irq_data *data) >> > +{ >> > + mtk_cirq_write_mask(data, CIRQ_ACK); >> >> EOI and ACK have very different semantics. What is this write actually >> doing? Also, you're now doing an additional MMIO write on each interrupt >> EOI, doubling its cost. Do you really need to do actually signal the HW >> that we've EOIed an interrupt? I would have hoped that you'd be able to >> put it in "bypass" mode as long as you're not suspending... >> > > When external interrupt happened, CIRQ status register record the status > even CIRQ is not enabled. when execute the flush command, CIRQ will > resend the signals according to the status. So if don't clear the > status, CIRQ will resend the wrong signals. the ACK write operation will > clear the status. But at this time, we haven't suspended yet, and there is nothing to replay. Also, you only enable the edge capture when suspending. So what are you ACKing here? Can't you simply clear everything right when suspending and not do it at all on the fast path? Thanks, M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement 2016-11-04 22:21 ` Marc Zyngier @ 2016-11-08 2:57 ` Youlin Pei 2016-11-10 18:24 ` Marc Zyngier 0 siblings, 1 reply; 12+ messages in thread From: Youlin Pei @ 2016-11-08 2:57 UTC (permalink / raw) To: Marc Zyngier Cc: Rob Herring, Matthias Brugger, Thomas Gleixner, Jason Cooper, Mark Rutland, Russell King, linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, srv_heupstream, hongkun.cao, yong.wu, erin.lo, chieh-jay.liu On Fri, 2016-11-04 at 22:21 +0000, Marc Zyngier wrote: > On Fri, Nov 04 2016 at 04:42:57 AM, Youlin Pei <youlin.pei@mediatek.com> wrote: > > On Tue, 2016-11-01 at 20:49 +0000, Marc Zyngier wrote: > >> On Tue, Nov 01 2016 at 11:52:01 AM, Youlin Pei <youlin.pei@mediatek.com> wrote: > >> > In Mediatek SOCs, the CIRQ is a low power interrupt controller > >> > designed to works outside MCUSYS which comprises with Cortex-Ax > >> > cores,CCI and GIC. > >> > > >> > The CIRQ controller is integrated in between MCUSYS( include > >> > Cortex-Ax, CCI and GIC ) and interrupt sources as the second > >> > level interrupt controller. The external interrupts which outside > >> > MCUSYS will feed through CIRQ then bypass to GIC. CIRQ can monitors > >> > all edge trigger interupts. When an edge interrupt is triggered, > >> > CIRQ can record the status and generate a pulse signal to GIC when > >> > flush command executed. > >> > > >> > When system enters sleep mode, MCUSYS will be turned off to improve > >> > power consumption, also GIC is power down. The edge trigger interrupts > >> > will be lost in this scenario without CIRQ. > >> > > >> > This commit provides the CIRQ irqchip implement. > >> > > >> > Signed-off-by: Youlin Pei <youlin.pei@mediatek.com> > >> > --- > >> > drivers/irqchip/Makefile | 2 +- > >> > drivers/irqchip/irq-mtk-cirq.c | 262 ++++++++++++++++++++++++++++++++++++++++ > >> > 2 files changed, 263 insertions(+), 1 deletion(-) > >> > create mode 100644 drivers/irqchip/irq-mtk-cirq.c > >> > > >> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > >> > index e4dbfc8..8f33580 100644 > >> > --- a/drivers/irqchip/Makefile > >> > +++ b/drivers/irqchip/Makefile > >> > @@ -60,7 +60,7 @@ obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o > >> > obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o > >> > obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o > >> > obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o > >> > -obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o > >> > +obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o irq-mtk-cirq.o > >> > obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o > >> > obj-$(CONFIG_RENESAS_H8300H_INTC) += irq-renesas-h8300h.o > >> > obj-$(CONFIG_RENESAS_H8S_INTC) += irq-renesas-h8s.o > >> > diff --git a/drivers/irqchip/irq-mtk-cirq.c b/drivers/irqchip/irq-mtk-cirq.c > >> > new file mode 100644 > >> > index 0000000..fc43ef3 > >> > --- /dev/null > >> > +++ b/drivers/irqchip/irq-mtk-cirq.c > >> > @@ -0,0 +1,262 @@ > >> > +/* > >> > + * Copyright (c) 2016 MediaTek Inc. > >> > + * Author: Youlin.Pei <youlin.pei@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/irqchip.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/syscore_ops.h> > >> > + > >> > +#define CIRQ_ACK 0x40 > >> > +#define CIRQ_MASK_SET 0xc0 > >> > +#define CIRQ_MASK_CLR 0x100 > >> > +#define CIRQ_SENS_SET 0x180 > >> > +#define CIRQ_SENS_CLR 0x1c0 > >> > +#define CIRQ_POL_SET 0x240 > >> > +#define CIRQ_POL_CLR 0x280 > >> > +#define CIRQ_CONTROL 0x300 > >> > + > >> > +#define CIRQ_EN 0x1 > >> > +#define CIRQ_EDGE 0x2 > >> > +#define CIRQ_FLUSH 0x4 > >> > + > >> > +#define CIRQ_IRQ_NUM 0x200 > >> > + > >> > +struct mtk_cirq_chip_data { > >> > + void __iomem *base; > >> > + unsigned int ext_irq_start; > >> > +}; > >> > + > >> > +static struct mtk_cirq_chip_data *cirq_data; > >> > >> Are you guaranteed that you'll only ever have a single CIRQ in any > >> system? > > > > In Mediatek's SOC, only hace a single CIRQ. > > > >> > >> > + > >> > +static void mtk_cirq_write_mask(struct irq_data *data, unsigned int offset) > >> > +{ > >> > + struct mtk_cirq_chip_data *chip_data = data->chip_data; > >> > + unsigned int cirq_num = data->hwirq; > >> > + u32 mask = 1 << (cirq_num % 32); > >> > + > >> > + writel(mask, chip_data->base + offset + (cirq_num / 32) * 4); > >> > >> Why can't you use the relaxed accessors? > > > > It seems that i use wrong function, i will change the writel to > > writel_relaxed in next ve > > > >> > >> > +} > >> > + > >> > +static void mtk_cirq_mask(struct irq_data *data) > >> > +{ > >> > + mtk_cirq_write_mask(data, CIRQ_MASK_SET); > >> > + irq_chip_mask_parent(data); > >> > +} > >> > + > >> > +static void mtk_cirq_unmask(struct irq_data *data) > >> > +{ > >> > + mtk_cirq_write_mask(data, CIRQ_MASK_CLR); > >> > + irq_chip_unmask_parent(data); > >> > +} > >> > + > >> > +static void mtk_cirq_eoi(struct irq_data *data) > >> > +{ > >> > + mtk_cirq_write_mask(data, CIRQ_ACK); > >> > >> EOI and ACK have very different semantics. What is this write actually > >> doing? Also, you're now doing an additional MMIO write on each interrupt > >> EOI, doubling its cost. Do you really need to do actually signal the HW > >> that we've EOIed an interrupt? I would have hoped that you'd be able to > >> put it in "bypass" mode as long as you're not suspending... > >> > > > > When external interrupt happened, CIRQ status register record the status > > even CIRQ is not enabled. when execute the flush command, CIRQ will > > resend the signals according to the status. So if don't clear the > > status, CIRQ will resend the wrong signals. the ACK write operation will > > clear the status. > > But at this time, we haven't suspended yet, and there is nothing to > replay. Also, you only enable the edge capture when suspending. So what > are you ACKing here? Can't you simply clear everything right when > suspending and not do it at all on the fast path? I had planned to ACK the status in cirq suspend callback, but encountered a problem. following is a piece of code from http://lxr.free-electrons.com/source/kernel/power/suspend.c#L361 arch_suspend_disable_irqs(); ---step1 BUG_ON(!irqs_disabled()); error = syscore_suspend(); | ---cirq suspend(); ---step2 if ack the status in cirq suspend, the interrupts will be lost which happened during step1 to step2. So would you mind give me some suggestions about this? Thanks a lot! > > Thanks, > > M. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement 2016-11-08 2:57 ` Youlin Pei @ 2016-11-10 18:24 ` Marc Zyngier 2016-11-15 2:49 ` Youlin Pei 0 siblings, 1 reply; 12+ messages in thread From: Marc Zyngier @ 2016-11-10 18:24 UTC (permalink / raw) To: Youlin Pei Cc: Rob Herring, Matthias Brugger, Thomas Gleixner, Jason Cooper, Mark Rutland, Russell King, linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, srv_heupstream, hongkun.cao, yong.wu, erin.lo, chieh-jay.liu On 08/11/16 02:57, Youlin Pei wrote: > On Fri, 2016-11-04 at 22:21 +0000, Marc Zyngier wrote: >> On Fri, Nov 04 2016 at 04:42:57 AM, Youlin Pei <youlin.pei@mediatek.com> wrote: >>> On Tue, 2016-11-01 at 20:49 +0000, Marc Zyngier wrote: >>>> On Tue, Nov 01 2016 at 11:52:01 AM, Youlin Pei <youlin.pei@mediatek.com> wrote: >>>>> In Mediatek SOCs, the CIRQ is a low power interrupt controller >>>>> designed to works outside MCUSYS which comprises with Cortex-Ax >>>>> cores,CCI and GIC. >>>>> >>>>> The CIRQ controller is integrated in between MCUSYS( include >>>>> Cortex-Ax, CCI and GIC ) and interrupt sources as the second >>>>> level interrupt controller. The external interrupts which outside >>>>> MCUSYS will feed through CIRQ then bypass to GIC. CIRQ can monitors >>>>> all edge trigger interupts. When an edge interrupt is triggered, >>>>> CIRQ can record the status and generate a pulse signal to GIC when >>>>> flush command executed. >>>>> >>>>> When system enters sleep mode, MCUSYS will be turned off to improve >>>>> power consumption, also GIC is power down. The edge trigger interrupts >>>>> will be lost in this scenario without CIRQ. >>>>> >>>>> This commit provides the CIRQ irqchip implement. >>>>> >>>>> Signed-off-by: Youlin Pei <youlin.pei@mediatek.com> >>>>> --- >>>>> drivers/irqchip/Makefile | 2 +- >>>>> drivers/irqchip/irq-mtk-cirq.c | 262 ++++++++++++++++++++++++++++++++++++++++ >>>>> 2 files changed, 263 insertions(+), 1 deletion(-) >>>>> create mode 100644 drivers/irqchip/irq-mtk-cirq.c >>>>> >>>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile >>>>> index e4dbfc8..8f33580 100644 >>>>> --- a/drivers/irqchip/Makefile >>>>> +++ b/drivers/irqchip/Makefile >>>>> @@ -60,7 +60,7 @@ obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o >>>>> obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o >>>>> obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o >>>>> obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o >>>>> -obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o >>>>> +obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o irq-mtk-cirq.o >>>>> obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o >>>>> obj-$(CONFIG_RENESAS_H8300H_INTC) += irq-renesas-h8300h.o >>>>> obj-$(CONFIG_RENESAS_H8S_INTC) += irq-renesas-h8s.o >>>>> diff --git a/drivers/irqchip/irq-mtk-cirq.c b/drivers/irqchip/irq-mtk-cirq.c >>>>> new file mode 100644 >>>>> index 0000000..fc43ef3 >>>>> --- /dev/null >>>>> +++ b/drivers/irqchip/irq-mtk-cirq.c >>>>> @@ -0,0 +1,262 @@ >>>>> +/* >>>>> + * Copyright (c) 2016 MediaTek Inc. >>>>> + * Author: Youlin.Pei <youlin.pei@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/irqchip.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/syscore_ops.h> >>>>> + >>>>> +#define CIRQ_ACK 0x40 >>>>> +#define CIRQ_MASK_SET 0xc0 >>>>> +#define CIRQ_MASK_CLR 0x100 >>>>> +#define CIRQ_SENS_SET 0x180 >>>>> +#define CIRQ_SENS_CLR 0x1c0 >>>>> +#define CIRQ_POL_SET 0x240 >>>>> +#define CIRQ_POL_CLR 0x280 >>>>> +#define CIRQ_CONTROL 0x300 >>>>> + >>>>> +#define CIRQ_EN 0x1 >>>>> +#define CIRQ_EDGE 0x2 >>>>> +#define CIRQ_FLUSH 0x4 >>>>> + >>>>> +#define CIRQ_IRQ_NUM 0x200 >>>>> + >>>>> +struct mtk_cirq_chip_data { >>>>> + void __iomem *base; >>>>> + unsigned int ext_irq_start; >>>>> +}; >>>>> + >>>>> +static struct mtk_cirq_chip_data *cirq_data; >>>> >>>> Are you guaranteed that you'll only ever have a single CIRQ in any >>>> system? >>> >>> In Mediatek's SOC, only hace a single CIRQ. >>> >>>> >>>>> + >>>>> +static void mtk_cirq_write_mask(struct irq_data *data, unsigned int offset) >>>>> +{ >>>>> + struct mtk_cirq_chip_data *chip_data = data->chip_data; >>>>> + unsigned int cirq_num = data->hwirq; >>>>> + u32 mask = 1 << (cirq_num % 32); >>>>> + >>>>> + writel(mask, chip_data->base + offset + (cirq_num / 32) * 4); >>>> >>>> Why can't you use the relaxed accessors? >>> >>> It seems that i use wrong function, i will change the writel to >>> writel_relaxed in next ve >>> >>>> >>>>> +} >>>>> + >>>>> +static void mtk_cirq_mask(struct irq_data *data) >>>>> +{ >>>>> + mtk_cirq_write_mask(data, CIRQ_MASK_SET); >>>>> + irq_chip_mask_parent(data); >>>>> +} >>>>> + >>>>> +static void mtk_cirq_unmask(struct irq_data *data) >>>>> +{ >>>>> + mtk_cirq_write_mask(data, CIRQ_MASK_CLR); >>>>> + irq_chip_unmask_parent(data); >>>>> +} >>>>> + >>>>> +static void mtk_cirq_eoi(struct irq_data *data) >>>>> +{ >>>>> + mtk_cirq_write_mask(data, CIRQ_ACK); >>>> >>>> EOI and ACK have very different semantics. What is this write actually >>>> doing? Also, you're now doing an additional MMIO write on each interrupt >>>> EOI, doubling its cost. Do you really need to do actually signal the HW >>>> that we've EOIed an interrupt? I would have hoped that you'd be able to >>>> put it in "bypass" mode as long as you're not suspending... >>>> >>> >>> When external interrupt happened, CIRQ status register record the status >>> even CIRQ is not enabled. when execute the flush command, CIRQ will >>> resend the signals according to the status. So if don't clear the >>> status, CIRQ will resend the wrong signals. the ACK write operation will >>> clear the status. >> >> But at this time, we haven't suspended yet, and there is nothing to >> replay. Also, you only enable the edge capture when suspending. So what >> are you ACKing here? Can't you simply clear everything right when >> suspending and not do it at all on the fast path? > > I had planned to ACK the status in cirq suspend callback, but > encountered a problem. > following is a piece of code from > http://lxr.free-electrons.com/source/kernel/power/suspend.c#L361 > > arch_suspend_disable_irqs(); ---step1 > BUG_ON(!irqs_disabled()); > > error = syscore_suspend(); > | > ---cirq suspend(); ---step2 > > if ack the status in cirq suspend, the interrupts will be lost which > happened during step1 to step2. > > So would you mind give me some suggestions about this? > Thanks a lot! Right. So maybe there is a way: - On suspend you can iterate over all the cirq interrupts that have been recorded - For each of those, you inspect if it is pending at the GIC level (using the irq_get_irqchip_state helper) - if not pending, you Ack it, because it cannot be delivered anymore - If it is pending, then you know it happened between step 1 and step 2. - You then have the choice of either going into suspend and waking up immediately, or failing the suspend. Thoughts? M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement 2016-11-10 18:24 ` Marc Zyngier @ 2016-11-15 2:49 ` Youlin Pei 0 siblings, 0 replies; 12+ messages in thread From: Youlin Pei @ 2016-11-15 2:49 UTC (permalink / raw) To: Marc Zyngier Cc: Rob Herring, Matthias Brugger, Thomas Gleixner, Jason Cooper, Mark Rutland, Russell King, linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, srv_heupstream, hongkun.cao, yong.wu, erin.lo, chieh-jay.liu On Thu, 2016-11-10 at 18:24 +0000, Marc Zyngier wrote: > On 08/11/16 02:57, Youlin Pei wrote: > > On Fri, 2016-11-04 at 22:21 +0000, Marc Zyngier wrote: > >> On Fri, Nov 04 2016 at 04:42:57 AM, Youlin Pei <youlin.pei@mediatek.com> wrote: > >>> On Tue, 2016-11-01 at 20:49 +0000, Marc Zyngier wrote: > >>>> On Tue, Nov 01 2016 at 11:52:01 AM, Youlin Pei <youlin.pei@mediatek.com> wrote: > >>>>> In Mediatek SOCs, the CIRQ is a low power interrupt controller > >>>>> designed to works outside MCUSYS which comprises with Cortex-Ax > >>>>> cores,CCI and GIC. > >>>>> > >>>>> The CIRQ controller is integrated in between MCUSYS( include > >>>>> Cortex-Ax, CCI and GIC ) and interrupt sources as the second > >>>>> level interrupt controller. The external interrupts which outside > >>>>> MCUSYS will feed through CIRQ then bypass to GIC. CIRQ can monitors > >>>>> all edge trigger interupts. When an edge interrupt is triggered, > >>>>> CIRQ can record the status and generate a pulse signal to GIC when > >>>>> flush command executed. > >>>>> > >>>>> When system enters sleep mode, MCUSYS will be turned off to improve > >>>>> power consumption, also GIC is power down. The edge trigger interrupts > >>>>> will be lost in this scenario without CIRQ. > >>>>> > >>>>> This commit provides the CIRQ irqchip implement. > >>>>> > >>>>> Signed-off-by: Youlin Pei <youlin.pei@mediatek.com> > >>>>> --- > >>>>> drivers/irqchip/Makefile | 2 +- > >>>>> drivers/irqchip/irq-mtk-cirq.c | 262 ++++++++++++++++++++++++++++++++++++++++ > >>>>> 2 files changed, 263 insertions(+), 1 deletion(-) > >>>>> create mode 100644 drivers/irqchip/irq-mtk-cirq.c > >>>>> > >>>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > >>>>> index e4dbfc8..8f33580 100644 > >>>>> --- a/drivers/irqchip/Makefile > >>>>> +++ b/drivers/irqchip/Makefile > >>>>> @@ -60,7 +60,7 @@ obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o > >>>>> obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o > >>>>> obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o > >>>>> obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o > >>>>> -obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o > >>>>> +obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o irq-mtk-cirq.o > >>>>> obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o > >>>>> obj-$(CONFIG_RENESAS_H8300H_INTC) += irq-renesas-h8300h.o > >>>>> obj-$(CONFIG_RENESAS_H8S_INTC) += irq-renesas-h8s.o > >>>>> diff --git a/drivers/irqchip/irq-mtk-cirq.c b/drivers/irqchip/irq-mtk-cirq.c > >>>>> new file mode 100644 > >>>>> index 0000000..fc43ef3 > >>>>> --- /dev/null > >>>>> +++ b/drivers/irqchip/irq-mtk-cirq.c > >>>>> @@ -0,0 +1,262 @@ > >>>>> +/* > >>>>> + * Copyright (c) 2016 MediaTek Inc. > >>>>> + * Author: Youlin.Pei <youlin.pei@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/irqchip.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/syscore_ops.h> > >>>>> + > >>>>> +#define CIRQ_ACK 0x40 > >>>>> +#define CIRQ_MASK_SET 0xc0 > >>>>> +#define CIRQ_MASK_CLR 0x100 > >>>>> +#define CIRQ_SENS_SET 0x180 > >>>>> +#define CIRQ_SENS_CLR 0x1c0 > >>>>> +#define CIRQ_POL_SET 0x240 > >>>>> +#define CIRQ_POL_CLR 0x280 > >>>>> +#define CIRQ_CONTROL 0x300 > >>>>> + > >>>>> +#define CIRQ_EN 0x1 > >>>>> +#define CIRQ_EDGE 0x2 > >>>>> +#define CIRQ_FLUSH 0x4 > >>>>> + > >>>>> +#define CIRQ_IRQ_NUM 0x200 > >>>>> + > >>>>> +struct mtk_cirq_chip_data { > >>>>> + void __iomem *base; > >>>>> + unsigned int ext_irq_start; > >>>>> +}; > >>>>> + > >>>>> +static struct mtk_cirq_chip_data *cirq_data; > >>>> > >>>> Are you guaranteed that you'll only ever have a single CIRQ in any > >>>> system? > >>> > >>> In Mediatek's SOC, only hace a single CIRQ. > >>> > >>>> > >>>>> + > >>>>> +static void mtk_cirq_write_mask(struct irq_data *data, unsigned int offset) > >>>>> +{ > >>>>> + struct mtk_cirq_chip_data *chip_data = data->chip_data; > >>>>> + unsigned int cirq_num = data->hwirq; > >>>>> + u32 mask = 1 << (cirq_num % 32); > >>>>> + > >>>>> + writel(mask, chip_data->base + offset + (cirq_num / 32) * 4); > >>>> > >>>> Why can't you use the relaxed accessors? > >>> > >>> It seems that i use wrong function, i will change the writel to > >>> writel_relaxed in next ve > >>> > >>>> > >>>>> +} > >>>>> + > >>>>> +static void mtk_cirq_mask(struct irq_data *data) > >>>>> +{ > >>>>> + mtk_cirq_write_mask(data, CIRQ_MASK_SET); > >>>>> + irq_chip_mask_parent(data); > >>>>> +} > >>>>> + > >>>>> +static void mtk_cirq_unmask(struct irq_data *data) > >>>>> +{ > >>>>> + mtk_cirq_write_mask(data, CIRQ_MASK_CLR); > >>>>> + irq_chip_unmask_parent(data); > >>>>> +} > >>>>> + > >>>>> +static void mtk_cirq_eoi(struct irq_data *data) > >>>>> +{ > >>>>> + mtk_cirq_write_mask(data, CIRQ_ACK); > >>>> > >>>> EOI and ACK have very different semantics. What is this write actually > >>>> doing? Also, you're now doing an additional MMIO write on each interrupt > >>>> EOI, doubling its cost. Do you really need to do actually signal the HW > >>>> that we've EOIed an interrupt? I would have hoped that you'd be able to > >>>> put it in "bypass" mode as long as you're not suspending... > >>>> > >>> > >>> When external interrupt happened, CIRQ status register record the status > >>> even CIRQ is not enabled. when execute the flush command, CIRQ will > >>> resend the signals according to the status. So if don't clear the > >>> status, CIRQ will resend the wrong signals. the ACK write operation will > >>> clear the status. > >> > >> But at this time, we haven't suspended yet, and there is nothing to > >> replay. Also, you only enable the edge capture when suspending. So what > >> are you ACKing here? Can't you simply clear everything right when > >> suspending and not do it at all on the fast path? > > > > I had planned to ACK the status in cirq suspend callback, but > > encountered a problem. > > following is a piece of code from > > http://lxr.free-electrons.com/source/kernel/power/suspend.c#L361 > > > > arch_suspend_disable_irqs(); ---step1 > > BUG_ON(!irqs_disabled()); > > > > error = syscore_suspend(); > > | > > ---cirq suspend(); ---step2 > > > > if ack the status in cirq suspend, the interrupts will be lost which > > happened during step1 to step2. > > > > So would you mind give me some suggestions about this? > > Thanks a lot! > > Right. So maybe there is a way: > - On suspend you can iterate over all the cirq interrupts that have been > recorded > - For each of those, you inspect if it is pending at the GIC level > (using the irq_get_irqchip_state helper) > - if not pending, you Ack it, because it cannot be delivered anymore > - If it is pending, then you know it happened between step 1 and step 2. > - You then have the choice of either going into suspend and waking up > immediately, or failing the suspend. > > Thoughts? Will use this solution in next version. Thanks a lot! > > M. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] ARM: dts: mt2701: Add mtk-cirq node for mt2701 2016-11-01 11:51 [PATCH v2 0/3] Add Mediatek CIRQ interrupt controller Youlin Pei 2016-11-01 11:52 ` [PATCH v2 1/3] binding: irqchip: mtk-cirq: Add binding document Youlin Pei 2016-11-01 11:52 ` [PATCH v2 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement Youlin Pei @ 2016-11-01 11:52 ` Youlin Pei 2 siblings, 0 replies; 12+ messages in thread From: Youlin Pei @ 2016-11-01 11:52 UTC (permalink / raw) To: Marc Zyngier, Rob Herring, Matthias Brugger Cc: youlin.pei, Thomas Gleixner, Jason Cooper, Mark Rutland, Russell King, linux-kernel, devicetree, linux-arm-kernel, linux-mediatek, srv_heupstream, hongkun.cao, yong.wu, erin.lo, chieh-jay.liu This commit add mtk-cirq node to mt2701 dtsi. Signed-off-by: Youlin Pei <youlin.pei@mediatek.com> --- arch/arm/boot/dts/mt2701.dtsi | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi index 2800231..cd00bc4 100644 --- a/arch/arm/boot/dts/mt2701.dtsi +++ b/arch/arm/boot/dts/mt2701.dtsi @@ -22,7 +22,7 @@ / { compatible = "mediatek,mt2701"; - interrupt-parent = <&sysirq>; + interrupt-parent = <&cirq>; cpus { #address-cells = <1>; @@ -170,6 +170,15 @@ reg = <0 0x10200100 0 0x1c>; }; + cirq: interrupt-controller@10204000 { + compatible = "mediatek,mtk-cirq"; + interrupt-controller; + #interrupt-cells = <3>; + interrupt-parent = <&sysirq>; + reg = <0 0x10204000 0 0x4000>; + mediatek,ext-irq-start = <32>; + }; + apmixedsys: syscon@10209000 { compatible = "mediatek,mt2701-apmixedsys", "syscon"; reg = <0 0x10209000 0 0x1000>; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-11-15 3:09 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-01 11:51 [PATCH v2 0/3] Add Mediatek CIRQ interrupt controller Youlin Pei 2016-11-01 11:52 ` [PATCH v2 1/3] binding: irqchip: mtk-cirq: Add binding document Youlin Pei 2016-11-09 18:26 ` Rob Herring 2016-11-15 3:09 ` Youlin Pei 2016-11-01 11:52 ` [PATCH v2 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement Youlin Pei 2016-11-01 20:49 ` Marc Zyngier 2016-11-04 4:42 ` Youlin Pei 2016-11-04 22:21 ` Marc Zyngier 2016-11-08 2:57 ` Youlin Pei 2016-11-10 18:24 ` Marc Zyngier 2016-11-15 2:49 ` Youlin Pei 2016-11-01 11:52 ` [PATCH v2 3/3] ARM: dts: mt2701: Add mtk-cirq node for mt2701 Youlin Pei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).