Messages in this thread | | | From | Joakim Zhang <> | Subject | RE: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt multiplexer support | Date | Fri, 20 Dec 2019 11:59:55 +0000 |
| |
> -----Original Message----- > From: Marc Zyngier <maz@kernel.org> > Sent: 2019年12月20日 18:49 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: tglx@linutronix.de; jason@lakedaemon.net; robh+dt@kernel.org; > mark.rutland@arm.com; shawnguo@kernel.org; s.hauer@pengutronix.de; > kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>; > linux-kernel@vger.kernel.org; Andy Duan <fugang.duan@nxp.com>; > linux-arm-kernel@lists.infradead.org; S.j. Wang <shengjiu.wang@nxp.com> > Subject: Re: [PATCH V3 2/2] drivers/irqchip: add NXP INTMUX interrupt > multiplexer support > > 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 <shengjiu.wang@nxp.com> > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > --- > > 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 <linux/clk.h> > > +#include <linux/interrupt.h> > > +#include <linux/irq.h> > > +#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h> > > +#include <linux/kernel.h> #include <linux/of_irq.h> #include > > +<linux/of_platform.h> #include <linux/spinlock.h> > > + > > +#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. Yes.
> > + 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 > */ Go it.
> > + 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. It is all level, I will correct it.
> > + > > + 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]; Got it.
> > +} > > + > > +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. Got it.
> > + 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. Make sense. Thanks.
> > + } > > + > > + 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. Yes.
Thanks for your kindly review, Marc :-)
Best Regards, Joakim Zhang > > + > > + 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...
| |