Re: [PATCH 2/3] drivers/irqchip: add NXP INTMUX interrupt multiplexer support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

+#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...



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux