> -----Original Message-----
> From: Marc Zyngier <maz@xxxxxxxxxx>
> Sent: 2019年12月18日 17:38
> To: Joakim Zhang <qiangqing.zhang@xxxxxxx>
> Cc: tglx@xxxxxxxxxxxxx; jason@xxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; S.j.
> Wang <shengjiu.wang@xxxxxxx>; kernel@xxxxxxxxxxxxxx;
> festevam@xxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Andy Duan <fugang.duan@xxxxxxx>;
> Aisheng Dong <aisheng.dong@xxxxxxx>
> Subject: Re: [PATCH 2/3] drivers/irqchip: add NXP INTMUX interrupt
> multiplexer support
>
> On 2019-12-18 07:20, Joakim Zhang wrote:
> > From: Shengjiu Wang <shengjiu.wang@xxxxxxx>
> >
> > 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@xxxxxxx>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@xxxxxxx>
> > ---
> > 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.
Hi Marc,
Yes, we defined these two macros and have not used yet. I will remove it firstly in V2.
> > +#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;
> [...]
> };
Ok, I will do it in V2.
Thanks.
Best Regards,
Joakim Zhang
>
> > +};
> > +
> > +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...
