Messages in this thread | | | Subject | Re: [PATCH 2/3] drivers/irqchip: add NXP INTMUX interrupt multiplexer support | Date | Wed, 18 Dec 2019 09:37:47 +0000 | From | Marc Zyngier <> |
| |
On 2019-12-18 07:20, Joakim Zhang wrote: > From: Shengjiu Wang <shengjiu.wang@nxp.com> > > The intmux module is used to output internal interrupt in subsystem > to system with 32-to-8 configuration. It has several multiplex > channels depends on system. > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > --- > drivers/irqchip/irq-imx-intmux.c | 220 > +++++++++++++++++++++++++++++++ > 1 file changed, 220 insertions(+) > create mode 100644 drivers/irqchip/irq-imx-intmux.c > > diff --git a/drivers/irqchip/irq-imx-intmux.c > b/drivers/irqchip/irq-imx-intmux.c > new file mode 100644 > index 000000000000..fa24b968f30b > --- /dev/null > +++ b/drivers/irqchip/irq-imx-intmux.c > @@ -0,0 +1,220 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright 2017 NXP > + > +#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/module.h> > +#include <linux/of_platform.h> > +#include <linux/spinlock.h> > + > +#define CHANCSR(n) (0x0 + 0x40 * n) > +#define CHANVEC(n) (0x4 + 0x40 * n)
These two macros are unused.
> +#define CHANIER(n) (0x10 + (0x40 * n)) > +#define CHANIPR(n) (0x20 + (0x40 * n)) > + > +struct intmux_irqchip_data { > + int chanidx; > + int irq; > + struct irq_domain *domain; > + unsigned int irqstat;
It would make things a bit readable if you aligned the various fields:
struct intmux_irqchip_data { int chanidx; int irq; struct irq_domain *domain; [...] };
> +}; > + > +struct intmux_data { > + spinlock_t lock; > + struct platform_device *pdev; > + 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; > + u32 idx = irqchip_data->chanidx; > + struct intmux_data *intmux_data = container_of(irqchip_data, > + struct intmux_data, irqchip_data[idx]); > + void __iomem *reg; > + u32 val; > + > + spin_lock(&intmux_data->lock);
This is racy. you could take an interrupt while executing disable_irq(), which calls this. In turn, the interrupt handler will try to acquire this lock -> deadlock.
Please turn this into its _irqsave version.
> + reg = intmux_data->regs + CHANIER(idx); > + val = readl_relaxed(reg); > + /* disable the interrupt source of this channel */ > + val &= ~(1 << d->hwirq);
val &= ~BIT(d->hwirq);
> + writel_relaxed(val, reg); > + spin_unlock(&intmux_data->lock); > +} > + > +static void imx_intmux_irq_unmask(struct irq_data *d) > +{ > + struct intmux_irqchip_data *irqchip_data = d->chip_data; > + u32 idx = irqchip_data->chanidx; > + struct intmux_data *intmux_data = container_of(irqchip_data, > + struct intmux_data, irqchip_data[idx]); > + void __iomem *reg; > + u32 val; > + > + spin_lock(&intmux_data->lock); > + reg = intmux_data->regs + CHANIER(idx); > + val = readl_relaxed(reg); > + /* enable the interrupt source of this channel */ > + val |= 1 << d->hwirq; > + writel_relaxed(val, reg); > + spin_unlock(&intmux_data->lock); > +} > + > +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); > + irq_set_chip_data(irq, h->host_data); > + irq_set_chip_and_handler(irq, &imx_intmux_irq_chip, > handle_level_irq); > + > + return 0; > +} > + > +static const struct irq_domain_ops imx_intmux_domain_ops = { > + .map = imx_intmux_irq_map, > + .xlate = irq_domain_xlate_onecell, > +}; > + > +static void imx_intmux_update_irqstat(struct intmux_irqchip_data > *irqchip_data) > +{ > + int i = irqchip_data->chanidx; > + struct intmux_data *intmux_data = container_of(irqchip_data, > + struct intmux_data, irqchip_data[i]); > + > + /* read the interrupt source pending status of this channel */ > + irqchip_data->irqstat = readl_relaxed(intmux_data->regs + > CHANIPR(i));
Why does it need to be stored into the data structure, instead of sinply being returned by the function?
> +} > + > +static void imx_intmux_irq_handler(struct irq_desc *desc) > +{ > + struct intmux_irqchip_data *irqchip_data = > irq_desc_get_handler_data(desc); > + int pos, virq; > + > + chained_irq_enter(irq_desc_get_chip(desc), desc); > + > + imx_intmux_update_irqstat(irqchip_data); > + > + for_each_set_bit(pos, (unsigned long *)&irqchip_data->irqstat, 32) > {
This is broken on big-endian. Never cast a smaller type into unsigned long if you're going to use any of the bit iterators.
> + 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 *intmux_data; > + int channum; > + int i, ret; > + > + ret = of_property_read_u32(np, "fsl,intmux_chans", &channum); > + if (ret) > + channum = 1; > + > + intmux_data = devm_kzalloc(&pdev->dev, sizeof(*intmux_data) + > + channum * sizeof(intmux_data->irqchip_data[0]), > + GFP_KERNEL); > + if (!intmux_data) > + return -ENOMEM; > + > + intmux_data->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(intmux_data->regs)) { > + dev_err(&pdev->dev, "failed to initialize reg\n"); > + return PTR_ERR(intmux_data->regs); > + } > + > + intmux_data->ipg_clk = devm_clk_get(&pdev->dev, "ipg"); > + if (IS_ERR(intmux_data->ipg_clk)) { > + ret = PTR_ERR(intmux_data->ipg_clk); > + dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret); > + return ret; > + } > + > + intmux_data->channum = channum; > + intmux_data->pdev = pdev;
What is the point of keeping track of this? The only instance where you go from MUX to device is just below, and you already have the device at hand.
> + spin_lock_init(&intmux_data->lock); > + > + ret = clk_prepare_enable(intmux_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++) { > + intmux_data->irqchip_data[i].chanidx = i; > + intmux_data->irqchip_data[i].irq = platform_get_irq(pdev, i); > + if (intmux_data->irqchip_data[i].irq <= 0) { > + dev_err(&pdev->dev, "failed to get irq\n"); > + return -ENODEV; > + } > + > + intmux_data->irqchip_data[i].domain = irq_domain_add_linear( > + np, > + 32, > + &imx_intmux_domain_ops, > + &intmux_data->irqchip_data[i]);
Please indent this in a readable manner. If you need an intermediate variable, so be it. Or have a long line if you want, but don't write things like this.
> + if (!intmux_data->irqchip_data[i].domain) { > + dev_err(&intmux_data->pdev->dev, > + "failed to create IRQ domain\n"); > + return -ENOMEM; > + } > + > + irq_set_chained_handler_and_data(intmux_data->irqchip_data[i].irq, > + imx_intmux_irq_handler, > + &intmux_data->irqchip_data[i]);
Shouldn't you initialize the HW to some sane state here? Like having having all interrupts masked?
> + } > + > + platform_set_drvdata(pdev, intmux_data); > + > + return 0; > +} > + > +static int imx_intmux_remove(struct platform_device *pdev) > +{ > + struct intmux_data *intmux_data = platform_get_drvdata(pdev); > + int i; > + > + for (i = 0; i < intmux_data->channum; i++) { > + irq_set_chained_handler_and_data(intmux_data->irqchip_data[i].irq, > + NULL, NULL);
Same thing here. Shouldn't you make sure that no interrupt can fire anymore?
> + > + irq_domain_remove(intmux_data->irqchip_data[i].domain); > + } > + > + platform_set_drvdata(pdev, NULL); > + clk_disable_unprepare(intmux_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);
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |