Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752868AbdC2FaO (ORCPT ); Wed, 29 Mar 2017 01:30:14 -0400 Received: from mailgw02.mediatek.com ([210.61.82.184]:49173 "EHLO mailgw02.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1752482AbdC2FaM (ORCPT ); Wed, 29 Mar 2017 01:30:12 -0400 Message-ID: <1490765406.30825.9.camel@mtksdaap41> Subject: Re: [PATCH v3 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement From: Youlin Pei To: Marc Zyngier CC: Rob Herring , Matthias Brugger , Thomas Gleixner , "Jason Cooper" , Mark Rutland , Russell King , , , , , , , , Date: Wed, 29 Mar 2017 13:30:06 +0800 In-Reply-To: <499f9186-82c1-cb5a-91de-754c89bcdd3d@arm.com> References: <1487040961-18319-1-git-send-email-youlin.pei@mediatek.com> <1487040961-18319-3-git-send-email-youlin.pei@mediatek.com> <499f9186-82c1-cb5a-91de-754c89bcdd3d@arm.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11670 Lines: 367 On Fri, 2017-03-24 at 16:22 +0000, Marc Zyngier wrote: > On 14/02/17 02:56, Youlin Pei 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 > > --- > > drivers/irqchip/Makefile | 2 +- > > drivers/irqchip/irq-mtk-cirq.c | 288 ++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 289 insertions(+), 1 deletion(-) > > create mode 100644 drivers/irqchip/irq-mtk-cirq.c > > > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > > index 0e55d94..db9acd1 100644 > > --- a/drivers/irqchip/Makefile > > +++ b/drivers/irqchip/Makefile > > @@ -61,7 +61,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..b5cf506 > > --- /dev/null > > +++ b/drivers/irqchip/irq-mtk-cirq.c > > @@ -0,0 +1,288 @@ > > +/* > > + * Copyright (c) 2016 MediaTek Inc. > > + * Author: Youlin.Pei > > + * > > + * 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#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 > > + > > +struct mtk_cirq_chip_data { > > + void __iomem *base; > > + unsigned int ext_irq_start; > > + unsigned int ext_irq_end; > > + struct irq_domain *domain; > > +}; > > + > > +static struct mtk_cirq_chip_data *cirq_data; > > Question in relation to the sysirq patches that have been queued for > 4.12: Are we going to see something similar with this driver, where > you're going to have to support multiple non-contiguous register sets? > I'd rather have this kind of thing from day one, instead of adding that > at a later time... > > Specially given that the platform that is using this driver does also > have a sysirq controller. Hi Marc, Thanks for your review. All of the cirq's registers are in continuous space, So no need to support multiple non-continuous register sets. > > > + > > +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_relaxed(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 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 = irq_chip_eoi_parent, > > + .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 || > > + fwspec->param[1] > cirq_data->ext_irq_end) > > + 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 ret; > > + irq_hw_number_t hwirq; > > + unsigned int type; > > + struct irq_fwspec *fwspec = arg; > > + struct irq_fwspec parent_fwspec = *fwspec; > > + > > + ret = mtk_cirq_domain_translate(domain, fwspec, &hwirq, &type); > > + if (ret) > > + return ret; > > + > > + if (WARN_ON(nr_irqs != 1)) > > + return -EINVAL; > > + > > + irq_domain_set_hwirq_and_chip(domain, virq, hwirq, > > + &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, mask; > > + unsigned int irq, hwirq_num; > > + bool pending, masked; > > + int i, pendret, maskret; > > + > > + hwirq_num = cirq_data->ext_irq_end - cirq_data->ext_irq_start + 1; > > + for (i = 0; i < hwirq_num; i++) { > > + irq = irq_find_mapping(cirq_data->domain, i); > > + if (irq) { > > + pendret = irq_get_irqchip_state(irq, > > + IRQCHIP_STATE_PENDING, > > + &pending); > > + > > + maskret = irq_get_irqchip_state(irq, > > + IRQCHIP_STATE_MASKED, > > + &masked); > > + > > + if (pendret == 0 && maskret == 0 && > > + (pending && !masked)) > > + continue; > > + } > > + > > + mask = 1 << (i % 32); > > + writel_relaxed(mask, cirq_data->base + CIRQ_ACK + (i / 32) * 4); > > + } > > It would be worth documenting exactly what this is for. I had to go back > to our previous discussion in November to remember it. Okay, i will add some comments to explain what this is for in next version. Thanks! > > > + > > + /* set edge_only mode, record edge-triggerd interrupts */ > > + /* enable cirq */ > > + value = readl_relaxed(cirq_data->base + CIRQ_CONTROL); > > + value |= (CIRQ_EDGE | CIRQ_EN); > > + writel_relaxed(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_relaxed(value | CIRQ_FLUSH, cirq_data->base + CIRQ_CONTROL); > > + > > + /* disable cirq */ > > + value = readl_relaxed(cirq_data->base + CIRQ_CONTROL); > > + value &= ~(CIRQ_EDGE | CIRQ_EN); > > + writel_relaxed(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; > > + unsigned int irq_num; > > + 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_index(node, "mediatek,ext-irq-range", 0, > > + &cirq_data->ext_irq_start); > > + if (ret) > > + goto out_unmap; > > + > > + ret = of_property_read_u32_index(node, "mediatek,ext-irq-range", 1, > > + &cirq_data->ext_irq_end); > > + if (ret) > > + goto out_unmap; > > + > > + irq_num = cirq_data->ext_irq_end - cirq_data->ext_irq_start + 1; > > + domain = irq_domain_add_hierarchy(domain_parent, 0, > > + irq_num, node, > > + &cirq_domain_ops, cirq_data); > > + if (!domain) { > > + ret = -ENOMEM; > > + goto out_unmap; > > + } > > + cirq_data->domain = domain; > > + > > + 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); > > > > It otherwise looks much better than the previous iteration. > > Thanks, > > M.