Received: by 2002:a25:8b91:0:0:0:0:0 with SMTP id j17csp3032260ybl; Fri, 20 Dec 2019 02:50:35 -0800 (PST) X-Google-Smtp-Source: APXvYqwMfpcXokCqjQMwDAZYOocWCLrELNkK04AxJ+1uvw78IADLwOkeOIl7re+tpXBFd1QYoLEo X-Received: by 2002:a9d:2264:: with SMTP id o91mr14545522ota.328.1576839035293; Fri, 20 Dec 2019 02:50:35 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1576839035; cv=none; d=google.com; s=arc-20160816; b=S2RB0ZU17KVzuDeo3Oa9eF+rDc5koDf0cO/TnxtLjxTpmyR1JhRGtGK/gRS7GyVFiZ tHttk/GxvrCYlg/5O7pt2u+cc9ujDYYfo7HQkONuOTQRZkPTDlaOTh6rZsxnDLwBGidL v1dPSQ7ISIpjx7nXnp8ZxoU9Dzl4loLn2WHP4uoO1wiCyIuVs9bpT5anUj9wjgRYNeGD CjyzwZ/C0TvnF3ynZEr2v+4uRgh3diLnyTy6GHnGFywfO+z+lTegkEKwlHDM13X0r5J/ inOfsXiY5pQ3zpk4Dr1MAr+8RAr7u928PJFL1EFMncHRDybMz4aupAGUbsrn8UFoHYKN Z1ew== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:message-id:references :in-reply-to:cc:from:date:content-transfer-encoding:mime-version :subject:to; bh=Kr4PDC8p2Z9lKMwLAFQxZPZO2+ELsOFAZqE+aRS2RPk=; b=AqB8dKoJ1OJa443gC9dz2oXRQBMfq5jgv149TS0mA/plxwCoV5gH96Tf23bQsz9JVC A0zTYNt0MJhNOObZGXybd8mW8j09K/S2lPOEC9s0lnpd9lOk/+cPXxzunxy3RgZdPrqI +3AskshSIQlRHS0rC6lYDDo01GtRF9ijJTouWgLuuxEGgH76veZM/d6OW7QCgDaSmHgj 497xDYFMThgEYCpSJLbdQNaWNATCJXALqkjMw2O27z8EHJsuNCwXHBZSBarrvaFxRa8Y 3VTqQsOavm4WAKCvAzY3V6vKITo7oV5aoZXP8CQEN5BVrxjWesWOQn/4YNr2ts1JJkLz 1HSA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id c12si2395720otp.106.2019.12.20.02.50.22; Fri, 20 Dec 2019 02:50:35 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727241AbfLTKtJ (ORCPT + 99 others); Fri, 20 Dec 2019 05:49:09 -0500 Received: from inca-roads.misterjones.org ([213.251.177.50]:37220 "EHLO inca-roads.misterjones.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726210AbfLTKtJ (ORCPT ); Fri, 20 Dec 2019 05:49:09 -0500 Received: from www-data by cheepnis.misterjones.org with local (Exim 4.80) (envelope-from ) id 1iiFq8-0003mg-V6; Fri, 20 Dec 2019 11:49:01 +0100 To: Joakim Zhang Subject: Re: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt multiplexer support X-PHP-Originating-Script: 0:main.inc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Date: Fri, 20 Dec 2019 10:49:00 +0000 From: Marc Zyngier Cc: , , , , , , , , , , , Shengjiu Wang In-Reply-To: <1576827431-31942-3-git-send-email-qiangqing.zhang@nxp.com> References: <1576827431-31942-1-git-send-email-qiangqing.zhang@nxp.com> <1576827431-31942-3-git-send-email-qiangqing.zhang@nxp.com> Message-ID: <2a2b06ab6d5b3fab51e46cd7dec3f12d@www.loen.fr> X-Sender: maz@kernel.org User-Agent: Roundcube Webmail/0.7.2 X-SA-Exim-Connect-IP: X-SA-Exim-Rcpt-To: qiangqing.zhang@nxp.com, tglx@linutronix.de, jason@lakedaemon.net, robh+dt@kernel.org, mark.rutland@arm.com, shawnguo@kernel.org, s.hauer@pengutronix.de, kernel@pengutronix.de, linux-imx@nxp.com, linux-kernel@vger.kernel.org, fugang.duan@nxp.com, linux-arm-kernel@lists.infradead.org, shengjiu.wang@nxp.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on cheepnis.misterjones.org); SAEximRunCond expanded to false Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2019-12-20 07:37, Joakim Zhang wrote: > The Interrupt Multiplexer (INTMUX) expands the number of peripherals > that can interrupt the core: > * The INTMUX has 8 channels that are assigned to 8 NVIC interrupt > slots. > * Each INTMUX channel can receive up to 32 interrupt sources and has > 1 > interrupt output. > * The INTMUX routes the interrupt sources to the interrupt outputs. > > Signed-off-by: Shengjiu Wang > Signed-off-by: Joakim Zhang > --- > drivers/irqchip/Kconfig | 6 + > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-imx-intmux.c | 311 > +++++++++++++++++++++++++++++++ > 3 files changed, 318 insertions(+) > create mode 100644 drivers/irqchip/irq-imx-intmux.c > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index ba152954324b..7e2b1e9d0b45 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -457,6 +457,12 @@ config IMX_IRQSTEER > help > Support for the i.MX IRQSTEER interrupt multiplexer/remapper. > > +config IMX_INTMUX > + def_bool y if ARCH_MXC > + select IRQ_DOMAIN > + help > + Support for the i.MX INTMUX interrupt multiplexer. > + > config LS1X_IRQ > bool "Loongson-1 Interrupt Controller" > depends on MACH_LOONGSON32 > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index e806dda690ea..af976a79d1fb 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -100,6 +100,7 @@ obj-$(CONFIG_CSKY_MPINTC) += irq-csky-mpintc.o > obj-$(CONFIG_CSKY_APB_INTC) += irq-csky-apb-intc.o > obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o > obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o > +obj-$(CONFIG_IMX_INTMUX) += irq-imx-intmux.o > obj-$(CONFIG_MADERA_IRQ) += irq-madera.o > obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o > obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o > diff --git a/drivers/irqchip/irq-imx-intmux.c > b/drivers/irqchip/irq-imx-intmux.c > new file mode 100644 > index 000000000000..94c67fdd7163 > --- /dev/null > +++ b/drivers/irqchip/irq-imx-intmux.c > @@ -0,0 +1,311 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright 2017 NXP > + > +/* INTMUX Block Diagram > + * > + * ________________ > + * interrupt source # 0 +---->| | > + * | | | > + * interrupt source # 1 +++-->| | > + * ... | | | channel # 0 > |--------->interrupt out # 0 > + * ... | | | | > + * ... | | | | > + * interrupt source # X-1 +++-->|________________| > + * | | | > + * | | | > + * | | | ________________ > + * +---->| | > + * | | | | | > + * | +-->| | > + * | | | | channel # 1 > |--------->interrupt out # 1 > + * | | +>| | > + * | | | | | > + * | | | |________________| > + * | | | > + * | | | > + * | | | ... > + * | | | ... > + * | | | > + * | | | ________________ > + * +---->| | > + * | | | | > + * +-->| | > + * | | channel # N > |--------->interrupt out # N > + * +>| | > + * | | > + * |________________| > + * > + * > + * N: Interrupt Channel Instance Number (N=7) > + * X: Interrupt Source Number for each channel (X=32) > + * > + * The INTMUX interrupt multiplexer has 8 channels, each channel > receives 32 > + * interrupt sources and generates 1 interrupt output. > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define CHANIER(n) (0x10 + (0x40 * n)) > +#define CHANIPR(n) (0x20 + (0x40 * n)) > + > +#define CHAN_MAX_NUM 0x8 > + > +struct intmux_irqchip_data { > + int chanidx; > + int irq; > + struct irq_domain *domain; > +}; > + > +struct intmux_data { > + raw_spinlock_t lock; > + void __iomem *regs; > + struct clk *ipg_clk; > + int channum; > + struct intmux_irqchip_data irqchip_data[]; > +}; > + > +static void imx_intmux_irq_mask(struct irq_data *d) > +{ > + struct intmux_irqchip_data *irqchip_data = d->chip_data; > + int idx = irqchip_data->chanidx; > + struct intmux_data *data = container_of(irqchip_data, struct > intmux_data, > + irqchip_data[idx]); > + unsigned long flags; > + void __iomem *reg; > + u32 val; > + > + raw_spin_lock_irqsave(&data->lock, flags); > + reg = data->regs + CHANIER(idx); > + val = readl_relaxed(reg); > + /* disable the interrupt source of this channel */ > + val &= ~BIT(d->hwirq); > + writel_relaxed(val, reg); > + raw_spin_unlock_irqrestore(&data->lock, flags); > +} > + > +static void imx_intmux_irq_unmask(struct irq_data *d) > +{ > + struct intmux_irqchip_data *irqchip_data = d->chip_data; > + int idx = irqchip_data->chanidx; > + struct intmux_data *data = container_of(irqchip_data, struct > intmux_data, > + irqchip_data[idx]); > + unsigned long flags; > + void __iomem *reg; > + u32 val; > + > + raw_spin_lock_irqsave(&data->lock, flags); > + reg = data->regs + CHANIER(idx); > + val = readl_relaxed(reg); > + /* enable the interrupt source of this channel */ > + val |= BIT(d->hwirq); > + writel_relaxed(val, reg); > + raw_spin_unlock_irqrestore(&data->lock, flags); > +} > + > +static struct irq_chip imx_intmux_irq_chip = { > + .name = "intmux", > + .irq_mask = imx_intmux_irq_mask, > + .irq_unmask = imx_intmux_irq_unmask, > +}; > + > +static int imx_intmux_irq_map(struct irq_domain *h, unsigned int > irq, > + irq_hw_number_t hwirq) > +{ > + irq_set_status_flags(irq, IRQ_LEVEL); You shouldn't need to do this if you had it right in the xlate callback, see below. > + irq_set_chip_data(irq, h->host_data); > + irq_set_chip_and_handler(irq, &imx_intmux_irq_chip, > handle_level_irq); > + > + return 0; > +} > + > +static int imx_intmux_irq_xlate(struct irq_domain *d, struct > device_node *node, > + const u32 *intspec, unsigned int intsize, > + unsigned long *out_hwirq, unsigned int *out_type) > +{ > + struct intmux_irqchip_data *irqchip_data = d->host_data; > + int idx = irqchip_data->chanidx; > + struct intmux_data *data = container_of(irqchip_data, struct > intmux_data, > + irqchip_data[idx]); > + > + /* two cells needed in interrupt specifier: > + * the 1st cell: hw interrupt number > + * the 2nd cell: channel index > + */ The comment format is: /* * comments */ > + if (WARN_ON(intsize != 2)) > + return -EINVAL; > + > + if (WARN_ON(intspec[1] >= data->channum)) > + return -EINVAL; > + > + *out_hwirq = intspec[0]; > + *out_type = IRQ_TYPE_NONE; Why NONE? Your interrupts are all level, if I trust the map function. > + > + return 0; > +} > + > +static int imx_intmux_irq_select(struct irq_domain *d, struct > irq_fwspec *fwspec, > + enum irq_domain_bus_token bus_token) > +{ > + struct intmux_irqchip_data *irqchip_data = d->host_data; > + > + /* Not for us */ > + if (fwspec->fwnode != d->fwnode) > + return false; > + > + if (irqchip_data->chanidx == fwspec->param[1]) > + return true; > + else > + return false; This is trivially simplified as return irqchip_data->chanidx == fwspec->param[1]; > +} > + > +static const struct irq_domain_ops imx_intmux_domain_ops = { > + .map = imx_intmux_irq_map, > + .xlate = imx_intmux_irq_xlate, > + .select = imx_intmux_irq_select, > +}; > + > +static void imx_intmux_irq_handler(struct irq_desc *desc) > +{ > + struct intmux_irqchip_data *irqchip_data = > irq_desc_get_handler_data(desc); > + int idx = irqchip_data->chanidx; > + struct intmux_data *data = container_of(irqchip_data, struct > intmux_data, > + irqchip_data[idx]); > + unsigned long irqstat; > + int pos, virq; > + > + chained_irq_enter(irq_desc_get_chip(desc), desc); > + > + /* read the interrupt source pending status of this channel */ > + irqstat = readl_relaxed(data->regs + CHANIPR(idx)); > + > + for_each_set_bit(pos, &irqstat, 32) { > + virq = irq_find_mapping(irqchip_data->domain, pos); > + if (virq) > + generic_handle_irq(virq); > + } > + > + chained_irq_exit(irq_desc_get_chip(desc), desc); > +} > + > +static int imx_intmux_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct intmux_data *data; > + int channum; > + int i, ret; > + > + ret = of_property_read_u32(np, "fsl,intmux_chans", &channum); > + if (ret) { > + channum = 1; > + } else if (channum > CHAN_MAX_NUM) { > + dev_err(&pdev->dev, "supports up to %d multiplex channels\n", > + CHAN_MAX_NUM); > + return -EINVAL; > + } > + > + data = devm_kzalloc(&pdev->dev, sizeof(*data) + > + channum * sizeof(data->irqchip_data[0]), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(data->regs)) { > + dev_err(&pdev->dev, "failed to initialize reg\n"); > + return PTR_ERR(data->regs); > + } > + > + data->ipg_clk = devm_clk_get(&pdev->dev, "ipg"); > + if (IS_ERR(data->ipg_clk)) { > + ret = PTR_ERR(data->ipg_clk); > + if (ret != -EPROBE_DEFER) > + dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret); > + return ret; > + } > + > + data->channum = channum; > + raw_spin_lock_init(&data->lock); > + > + ret = clk_prepare_enable(data->ipg_clk); > + if (ret) { > + dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret); > + return ret; > + } > + > + for (i = 0; i < channum; i++) { > + data->irqchip_data[i].chanidx = i; > + > + data->irqchip_data[i].irq = irq_of_parse_and_map(np, i); > + if (data->irqchip_data[i].irq <= 0) { > + ret = -EINVAL; > + dev_err(&pdev->dev, "failed to get irq\n"); > + goto out; > + } > + > + data->irqchip_data[i].domain = > + irq_domain_add_linear(np, 32, &imx_intmux_domain_ops, > + &data->irqchip_data[i]); Same problem as before. If the line is too long, use an intermediate variable. Or leave the assignment as a single long line. Don't split assignment like this. > + if (!data->irqchip_data[i].domain) { > + ret = -ENOMEM; > + dev_err(&pdev->dev, "failed to create IRQ domain\n"); > + goto out; > + } > + > + irq_set_chained_handler_and_data(data->irqchip_data[i].irq, > + imx_intmux_irq_handler, > + &data->irqchip_data[i]); > + > + /* disable interrupt sources of this channel firstly */ > + writel_relaxed(0, data->regs + CHANIER(i)); So you disable interrupts *after* enabling the mux interrupt. If you have screaming interrupts, this will lockup. You really need to do it *before* enabling the chained interrupt. > + } > + > + platform_set_drvdata(pdev, data); > + > + return 0; > +out: > + clk_disable_unprepare(data->ipg_clk); > + return ret; > +} > + > +static int imx_intmux_remove(struct platform_device *pdev) > +{ > + struct intmux_data *data = platform_get_drvdata(pdev); > + int i; > + > + for (i = 0; i < data->channum; i++) { > + /* clear interrupt sources pending status of this channel */ > + writel_relaxed(0, data->regs + CHANIPR(i)); If these are level interrupts, this won't do a thing. You need to actively *disable* them. > + > + irq_set_chained_handler_and_data(data->irqchip_data[i].irq, > + NULL, NULL); > + > + irq_domain_remove(data->irqchip_data[i].domain); > + } > + > + clk_disable_unprepare(data->ipg_clk); > + > + return 0; > +} > + > +static const struct of_device_id imx_intmux_id[] = { > + { .compatible = "fsl,imx-intmux", }, > + { /* sentinel */ }, > +}; > + > +static struct platform_driver imx_intmux_driver = { > + .driver = { > + .name = "imx-intmux", > + .of_match_table = imx_intmux_id, > + }, > + .probe = imx_intmux_probe, > + .remove = imx_intmux_remove, > +}; > +builtin_platform_driver(imx_intmux_driver); M. -- Jazz is not dead. It just smells funny...