[PATCH 1/2] dt-bindings: irq: add binding for Freescale IRQSTEER multiplexer

[PATCH 1/2] dt-bindings: irq: add binding for Freescale IRQSTEER multiplexer

[Lucas Stach]

This adds the DT binding for the Freescale IRQSTEER interrupt
multiplexer found in the i.MX8 familiy SoCs.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 .../interrupt-controller/fsl,irqsteer.txt     | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
new file mode 100644
index 000000000000..ed2b18165591
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
@@ -0,0 +1,39 @@
+Freescale IRQSTEER Interrupt multiplexer
+
+Required properties:
+
+- compatible: should be:
+	- "fsl,imx8m-irqsteer"
+	- "fsl,imx-irqsteer"
+- reg: Physical base address and size of registers.
+- interrupts: Should contain the parent interrupt line used to multiplex the
+  input interrupts.
+- clocks: Should contain one clock for entry in clock-names
+  see Documentation/devicetree/bindings/clock/clock-bindings.txt
+- clock-names:
+   - "ipg": main logic clock
+- interrupt-controller: Identifies the node as an interrupt controller.
+- #interrupt-cells: Specifies the number of cells needed to encode an
+  interrupt source. The value must be 2.
+
+Optional properties:
+- fsl,channel: Number of channels managed by this controller. Each channel
+  contains up to 32 interrupt sources. If absent defaults to 1.
+- fsl,endian:
+  0: controller registers are little endian
+  1: controller registers are big endian
+  If absent defaults to 0.
+
+Example:
+
+	interrupt-controller@32e2d000 {
+		compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";
+		reg = <0x32e2d000 0x1000>;
+		interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&clk IMX8MQ_CLK_DISP_APB_ROOT>;
+		clock-names = "ipg";
+		fsl,channel = <2>;
+		fsl,endian = <1>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	};
-- 
2.19.0

[PATCH 2/2] irqchip: add driver for imx-irqsteer controller

[Lucas Stach]

The irqsteer block is a interrupt multiplexer/remapper found on the
i.MX8 line of SoCs.

Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
This submission is a heavily cleaned up version of the downstream driver.
---
 drivers/irqchip/Kconfig            |   8 +
 drivers/irqchip/Makefile           |   1 +
 drivers/irqchip/irq-imx-irqsteer.c | 281 +++++++++++++++++++++++++++++
 3 files changed, 290 insertions(+)
 create mode 100644 drivers/irqchip/irq-imx-irqsteer.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 383e7b70221d..07290b30abd2 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -371,6 +371,14 @@ config QCOM_PDC
 	  Power Domain Controller driver to manage and configure wakeup
 	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
 
+config IMX_IRQSTEER
+	bool "i.MX IRQSTEER support"
+	depends on ARCH_MXC || COMPILE_TEST
+	default ARCH_MXC
+	select IRQ_DOMAIN
+	help
+	  Support for the i.MX IRQSTEER interrupt multiplexer/remapper.
+
 endmenu
 
 config SIFIVE_PLIC
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index fbd1ec8070ef..2c77ba4cc3cc 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -88,3 +88,4 @@ obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
 obj-$(CONFIG_NDS32)			+= irq-ativic32.o
 obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
+obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
new file mode 100644
index 000000000000..3ff0698ea7bd
--- /dev/null
+++ b/drivers/irqchip/irq-imx-irqsteer.c
@@ -0,0 +1,281 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2017 NXP
+ * Copyright (C) 2018 Pengutronix, Lucas Stach <kernel@pengutronix.de>
+ */
+
+#include <linux/kernel.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_platform.h>
+#include <linux/spinlock.h>
+
+#define CHANREG_OFF(_t)	(_t * 4)
+#define CHANCTRL		0x0
+#define CHANMASK(n, t)		(0x4 + (0x4 * (n)) + (CHANREG_OFF(t) * 0))
+#define CHANSET(n, t)		(0x4 + (0x4 * (n)) + (CHANREG_OFF(t) * 1))
+#define CHANSTATUS(n, t)	(0x4 + (0x4 * (n)) + (CHANREG_OFF(t) * 2))
+#define CHAN_MINTDIS(t)		(0x4 + (CHANREG_OFF(t) * 3))
+#define CHAN_MASTRSTAT(t)	(0x8 + (CHANREG_OFF(t) * 3))
+
+struct irqsteer_irqchip_data {
+	spinlock_t lock;
+	struct platform_device *pdev;
+	void __iomem *regs;
+	struct clk *ipg_clk;
+	int irq;
+	int channum;
+	int big_endian;
+	struct irq_domain *domain;
+	int *saved_reg;
+};
+
+static int imx_irqsteer_get_reg_index(struct irqsteer_irqchip_data *data,
+				      unsigned long irqnum)
+{
+	if (data->big_endian)
+		return (data->channum - irqnum / 32 - 1);
+	else
+		return irqnum / 32;
+}
+
+static void imx_irqsteer_irq_unmask(struct irq_data *d)
+{
+	struct irqsteer_irqchip_data *data = d->chip_data;
+	int idx = imx_irqsteer_get_reg_index(data, d->hwirq);
+	u32 val;
+
+	spin_lock(&data->lock);
+	val = readl_relaxed(data->regs + CHANMASK(idx, data->channum));
+	val |= 1 << (d->hwirq % 32);
+	writel_relaxed(val, data->regs + CHANMASK(idx, data->channum));
+	spin_unlock(&data->lock);
+}
+
+static void imx_irqsteer_irq_mask(struct irq_data *d)
+{
+	struct irqsteer_irqchip_data *data = d->chip_data;
+	int idx = imx_irqsteer_get_reg_index(data, d->hwirq);
+	u32 val;
+
+	spin_lock(&data->lock);
+	val = readl_relaxed(data->regs + CHANMASK(idx, data->channum));
+	val &= ~(1 << (d->hwirq % 32));
+	writel_relaxed(val, data->regs + CHANMASK(idx, data->channum));
+	spin_unlock(&data->lock);
+}
+
+static int imx_irqsteer_set_type(struct irq_data *data, unsigned int flow_type)
+{
+	if ((flow_type & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_LEVEL_HIGH)
+		return 0;
+
+	return -EINVAL;
+}
+
+static struct irq_chip imx_irqsteer_irq_chip = {
+	.name		= "irqsteer",
+	.irq_eoi	= irq_chip_eoi_parent,
+	.irq_mask	= imx_irqsteer_irq_mask,
+	.irq_unmask	= imx_irqsteer_irq_unmask,
+	.irq_set_type	= imx_irqsteer_set_type,
+};
+
+static int imx_irqsteer_irq_map(struct irq_domain *h, unsigned int irq,
+				irq_hw_number_t hwirq)
+{
+	irq_set_chip_data(irq, h->host_data);
+	irq_set_chip_and_handler(irq, &imx_irqsteer_irq_chip, handle_level_irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops imx_irqsteer_domain_ops = {
+	.map		= imx_irqsteer_irq_map,
+	.xlate		= irq_domain_xlate_twocell,
+};
+
+static void imx_irqsteer_irq_handler(struct irq_desc *desc)
+{
+	struct irqsteer_irqchip_data *data = irq_desc_get_handler_data(desc);
+	u32 val;
+	int i;
+
+	chained_irq_enter(irq_desc_get_chip(desc), desc);
+
+	val = readl_relaxed(data->regs + CHAN_MASTRSTAT(data->channum));
+	if (!val)
+		goto out;
+
+	for (i = 0; i < data->channum; i++) {
+		int idx = data->big_endian ? (data->channum - i - 1) : i;
+		unsigned long irqmap;
+		int pos, virq;
+
+		irqmap = readl_relaxed(data->regs +
+				       CHANSTATUS(idx, data->channum));
+
+		for_each_set_bit(pos, &irqmap, 32) {
+			virq = irq_find_mapping(data->domain, pos);
+			if (virq)
+				generic_handle_irq(virq);
+		}
+	}
+
+out:
+	chained_irq_exit(irq_desc_get_chip(desc), desc);
+}
+
+static int imx_irqsteer_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct irqsteer_irqchip_data *data;
+	struct resource *res;
+	int ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data->regs)) {
+		dev_err(&pdev->dev, "failed to initialize reg\n");
+		return PTR_ERR(data->regs);
+	}
+
+	data->irq = platform_get_irq(pdev, 0);
+	if (data->irq <= 0) {
+		dev_err(&pdev->dev, "failed to get irq\n");
+		return -ENODEV;
+	}
+
+	data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
+	if (IS_ERR(data->ipg_clk)) {
+		ret = PTR_ERR(data->ipg_clk);
+		dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
+		return ret;
+	}
+
+	/* default to 1 channel and little endian */
+	data->channum = 1;
+	data->big_endian  = 0;
+	data->pdev = pdev;
+	spin_lock_init(&data->lock);
+
+	of_property_read_u32(np, "fsl,channel", &data->channum);
+	of_property_read_u32(np, "fsl,endian", &data->big_endian);
+
+	if (IS_ENABLED(CONFIG_PM_SLEEP)) {
+		data->saved_reg = devm_kzalloc(&pdev->dev,
+					       sizeof(u32) * data->channum + 1,
+					       GFP_KERNEL);
+		if (!data->saved_reg)
+			return -ENOMEM;
+	}
+
+	ret = clk_prepare_enable(data->ipg_clk);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
+		return ret;
+	}
+
+	/* enable channel 1 by default */
+	writel_relaxed(1, data->regs + CHANCTRL);
+
+	data->domain = irq_domain_add_linear(np, data->channum * 32,
+					     &imx_irqsteer_domain_ops, data);
+	if (!data->domain) {
+		dev_err(&data->pdev->dev, "failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler,
+					 data);
+
+	platform_set_drvdata(pdev, data);
+
+	return 0;
+}
+
+static int imx_irqsteer_remove(struct platform_device *pdev)
+{
+	struct irqsteer_irqchip_data *irqsteer_data = platform_get_drvdata(pdev);
+
+	irq_set_chained_handler_and_data(irqsteer_data->irq, NULL, NULL);
+	irq_domain_remove(irqsteer_data->domain);
+
+	clk_disable_unprepare(irqsteer_data->ipg_clk);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static void imx_irqsteer_save_regs(struct irqsteer_irqchip_data *data)
+{
+	int num;
+
+	data->saved_reg[0] = readl_relaxed(data->regs + CHANCTRL);
+	for (num = 0; num < data->channum; num++)
+		data->saved_reg[num + 1] = readl_relaxed(data->regs +
+						CHANMASK(num, data->channum));
+}
+
+static void imx_irqsteer_restore_regs(struct irqsteer_irqchip_data *data)
+{
+	int num;
+
+	writel_relaxed(data->saved_reg[0], data->regs + CHANCTRL);
+	for (num = 0; num < data->channum; num++)
+		writel_relaxed(data->saved_reg[num + 1],
+			       data->regs + CHANMASK(num, data->channum));
+}
+
+static int imx_irqsteer_suspend(struct device *dev)
+{
+	struct irqsteer_irqchip_data *irqsteer_data = dev_get_drvdata(dev);
+
+	imx_irqsteer_save_regs(irqsteer_data);
+	clk_disable_unprepare(irqsteer_data->ipg_clk);
+
+	return 0;
+}
+
+static int imx_irqsteer_resume(struct device *dev)
+{
+	struct irqsteer_irqchip_data *irqsteer_data = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(irqsteer_data->ipg_clk);
+	if (ret) {
+		dev_err(dev, "failed to enable ipg clk: %d\n", ret);
+		return ret;
+	}
+	imx_irqsteer_restore_regs(irqsteer_data);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops imx_irqsteer_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_irqsteer_suspend, imx_irqsteer_resume)
+};
+
+static const struct of_device_id imx_irqsteer_dt_ids[] = {
+	{ .compatible = "fsl,imx-irqsteer", },
+	{},
+};
+
+static struct platform_driver imx_irqsteer_driver = {
+	.driver = {
+		.name = "imx-irqsteer",
+		.of_match_table = imx_irqsteer_dt_ids,
+		.pm = &imx_irqsteer_pm_ops,
+	},
+	.probe = imx_irqsteer_probe,
+	.remove = imx_irqsteer_remove,
+};
+builtin_platform_driver(imx_irqsteer_driver);
-- 
2.19.0

Re: [PATCH 2/2] irqchip: add driver for imx-irqsteer controller

[Fabio Estevam]

Hi Lucas,

On Tue, Oct 16, 2018 at 1:43 PM Lucas Stach <l.stach@pengutronix.de> wrote:

> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -0,0 +1,281 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later

According to Documentation/process/license-rules.rst this should be:
// SPDX-License-Identifier: GPL-2.0+

> +       ret = clk_prepare_enable(data->ipg_clk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
> +               return ret;
> +       }
> +
> +       /* enable channel 1 by default */
> +       writel_relaxed(1, data->regs + CHANCTRL);
> +
> +       data->domain = irq_domain_add_linear(np, data->channum * 32,
> +                                            &imx_irqsteer_domain_ops, data);
> +       if (!data->domain) {
> +               dev_err(&data->pdev->dev, "failed to create IRQ domain\n");

You should call clk_disable_unprepare(irqsteer_data->ipg_clk); prior
to returning.

> +               return -ENOMEM;

Re: [PATCH 2/2] irqchip: add driver for imx-irqsteer controller

[Marc Zyngier]

Hi Lucas,

On 16/10/18 17:42, Lucas Stach wrote:
> The irqsteer block is a interrupt multiplexer/remapper found on the
> i.MX8 line of SoCs.
> 
> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> This submission is a heavily cleaned up version of the downstream driver.
> ---
>  drivers/irqchip/Kconfig            |   8 +
>  drivers/irqchip/Makefile           |   1 +
>  drivers/irqchip/irq-imx-irqsteer.c | 281 +++++++++++++++++++++++++++++
>  3 files changed, 290 insertions(+)
>  create mode 100644 drivers/irqchip/irq-imx-irqsteer.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 383e7b70221d..07290b30abd2 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -371,6 +371,14 @@ config QCOM_PDC
>  	  Power Domain Controller driver to manage and configure wakeup
>  	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>  
> +config IMX_IRQSTEER
> +	bool "i.MX IRQSTEER support"
> +	depends on ARCH_MXC || COMPILE_TEST
> +	default ARCH_MXC
> +	select IRQ_DOMAIN
> +	help
> +	  Support for the i.MX IRQSTEER interrupt multiplexer/remapper.
> +
>  endmenu
>  
>  config SIFIVE_PLIC
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index fbd1ec8070ef..2c77ba4cc3cc 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -88,3 +88,4 @@ obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
>  obj-$(CONFIG_NDS32)			+= irq-ativic32.o
>  obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
>  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
> +obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> new file mode 100644
> index 000000000000..3ff0698ea7bd
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -0,0 +1,281 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2017 NXP
> + * Copyright (C) 2018 Pengutronix, Lucas Stach <kernel@pengutronix.de>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_platform.h>
> +#include <linux/spinlock.h>
> +
> +#define CHANREG_OFF(_t)	(_t * 4)
> +#define CHANCTRL		0x0
> +#define CHANMASK(n, t)		(0x4 + (0x4 * (n)) + (CHANREG_OFF(t) * 0))
> +#define CHANSET(n, t)		(0x4 + (0x4 * (n)) + (CHANREG_OFF(t) * 1))
> +#define CHANSTATUS(n, t)	(0x4 + (0x4 * (n)) + (CHANREG_OFF(t) * 2))
> +#define CHAN_MINTDIS(t)		(0x4 + (CHANREG_OFF(t) * 3))
> +#define CHAN_MASTRSTAT(t)	(0x8 + (CHANREG_OFF(t) * 3))
> +
> +struct irqsteer_irqchip_data {
> +	spinlock_t lock;

raw_spinlock_t, please.

> +	struct platform_device *pdev;
> +	void __iomem *regs;
> +	struct clk *ipg_clk;
> +	int irq;
> +	int channum;
> +	int big_endian;

bool?

> +	struct irq_domain *domain;
> +	int *saved_reg;
> +};
> +
> +static int imx_irqsteer_get_reg_index(struct irqsteer_irqchip_data *data,
> +				      unsigned long irqnum)
> +{
> +	if (data->big_endian)
> +		return (data->channum - irqnum / 32 - 1);
> +	else
> +		return irqnum / 32;
> +}
> +
> +static void imx_irqsteer_irq_unmask(struct irq_data *d)
> +{
> +	struct irqsteer_irqchip_data *data = d->chip_data;
> +	int idx = imx_irqsteer_get_reg_index(data, d->hwirq);
> +	u32 val;
> +
> +	spin_lock(&data->lock);
> +	val = readl_relaxed(data->regs + CHANMASK(idx, data->channum));
> +	val |= 1 << (d->hwirq % 32);
> +	writel_relaxed(val, data->regs + CHANMASK(idx, data->channum));
> +	spin_unlock(&data->lock);

If you take an interrupt within this sequence (because a driver is
calling enable_irq/disable_irq, for example), and that you try to mask
or unmask (which your flow handler will do), you're in for a nice
deadlock. Consider using the irqsave/irqrestore variants.

> +}
> +
> +static void imx_irqsteer_irq_mask(struct irq_data *d)
> +{
> +	struct irqsteer_irqchip_data *data = d->chip_data;
> +	int idx = imx_irqsteer_get_reg_index(data, d->hwirq);
> +	u32 val;
> +
> +	spin_lock(&data->lock);
> +	val = readl_relaxed(data->regs + CHANMASK(idx, data->channum));
> +	val &= ~(1 << (d->hwirq % 32));
> +	writel_relaxed(val, data->regs + CHANMASK(idx, data->channum));
> +	spin_unlock(&data->lock);
> +}
> +
> +static int imx_irqsteer_set_type(struct irq_data *data, unsigned int flow_type)
> +{
> +	if ((flow_type & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_LEVEL_HIGH)
> +		return 0;
> +
> +	return -EINVAL;
> +}
> +
> +static struct irq_chip imx_irqsteer_irq_chip = {
> +	.name		= "irqsteer",
> +	.irq_eoi	= irq_chip_eoi_parent,

What does it mean to call into the parent while this is a chained
interrupt controller which, by definition, doesn't have any parent?

> +	.irq_mask	= imx_irqsteer_irq_mask,
> +	.irq_unmask	= imx_irqsteer_irq_unmask,
> +	.irq_set_type	= imx_irqsteer_set_type,
> +};
> +
> +static int imx_irqsteer_irq_map(struct irq_domain *h, unsigned int irq,
> +				irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_data(irq, h->host_data);
> +	irq_set_chip_and_handler(irq, &imx_irqsteer_irq_chip, handle_level_irq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops imx_irqsteer_domain_ops = {
> +	.map		= imx_irqsteer_irq_map,
> +	.xlate		= irq_domain_xlate_twocell,
> +};
> +
> +static void imx_irqsteer_irq_handler(struct irq_desc *desc)
> +{
> +	struct irqsteer_irqchip_data *data = irq_desc_get_handler_data(desc);
> +	u32 val;
> +	int i;
> +
> +	chained_irq_enter(irq_desc_get_chip(desc), desc);
> +
> +	val = readl_relaxed(data->regs + CHAN_MASTRSTAT(data->channum));
> +	if (!val)
> +		goto out;
> +
> +	for (i = 0; i < data->channum; i++) {
> +		int idx = data->big_endian ? (data->channum - i - 1) : i;

How about reusing your imx_irqsteer_get_reg_index helper here?

Also what is the use of val? Shouldn't you only read the status register
for channels that have interrupts pending (assuming I understand how
this thing works)?

> +		unsigned long irqmap;
> +		int pos, virq;
> +
> +		irqmap = readl_relaxed(data->regs +
> +				       CHANSTATUS(idx, data->channum));
> +
> +		for_each_set_bit(pos, &irqmap, 32) {
> +			virq = irq_find_mapping(data->domain, pos);
> +			if (virq)
> +				generic_handle_irq(virq);
> +		}
> +	}
> +
> +out:
> +	chained_irq_exit(irq_desc_get_chip(desc), desc);

I guess you can have a local variable caching this irq_desc_get_chip().

> +}
> +
> +static int imx_irqsteer_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct irqsteer_irqchip_data *data;
> +	struct resource *res;
> +	int ret;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(data->regs)) {
> +		dev_err(&pdev->dev, "failed to initialize reg\n");
> +		return PTR_ERR(data->regs);
> +	}
> +
> +	data->irq = platform_get_irq(pdev, 0);
> +	if (data->irq <= 0) {
> +		dev_err(&pdev->dev, "failed to get irq\n");
> +		return -ENODEV;
> +	}
> +
> +	data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> +	if (IS_ERR(data->ipg_clk)) {
> +		ret = PTR_ERR(data->ipg_clk);
> +		dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* default to 1 channel and little endian */
> +	data->channum = 1;
> +	data->big_endian  = 0;

s/0/false/

> +	data->pdev = pdev;
> +	spin_lock_init(&data->lock);
> +
> +	of_property_read_u32(np, "fsl,channel", &data->channum);
> +	of_property_read_u32(np, "fsl,endian", &data->big_endian);

If you're reading a u32, please use the same type in the data structure.

> +
> +	if (IS_ENABLED(CONFIG_PM_SLEEP)) {
> +		data->saved_reg = devm_kzalloc(&pdev->dev,
> +					       sizeof(u32) * data->channum + 1,
> +					       GFP_KERNEL);

Use the appropriate type for saved_reg.

> +		if (!data->saved_reg)
> +			return -ENOMEM;
> +	}
> +
> +	ret = clk_prepare_enable(data->ipg_clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* enable channel 1 by default */
> +	writel_relaxed(1, data->regs + CHANCTRL);

Is that channel 1? or channel 0? Also, who enables the other channels? I
have the feeling that this driver is unable to service more than the
first 32 interrupts... But again, I'm guessing how the HW works here.

> +
> +	data->domain = irq_domain_add_linear(np, data->channum * 32,
> +					     &imx_irqsteer_domain_ops, data);
> +	if (!data->domain) {
> +		dev_err(&data->pdev->dev, "failed to create IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler,
> +					 data);
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	return 0;
> +}
> +
> +static int imx_irqsteer_remove(struct platform_device *pdev)
> +{
> +	struct irqsteer_irqchip_data *irqsteer_data = platform_get_drvdata(pdev);
> +
> +	irq_set_chained_handler_and_data(irqsteer_data->irq, NULL, NULL);
> +	irq_domain_remove(irqsteer_data->domain);
> +
> +	clk_disable_unprepare(irqsteer_data->ipg_clk);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static void imx_irqsteer_save_regs(struct irqsteer_irqchip_data *data)
> +{
> +	int num;
> +
> +	data->saved_reg[0] = readl_relaxed(data->regs + CHANCTRL);
> +	for (num = 0; num < data->channum; num++)
> +		data->saved_reg[num + 1] = readl_relaxed(data->regs +
> +						CHANMASK(num, data->channum));
> +}
> +
> +static void imx_irqsteer_restore_regs(struct irqsteer_irqchip_data *data)
> +{
> +	int num;
> +
> +	writel_relaxed(data->saved_reg[0], data->regs + CHANCTRL);
> +	for (num = 0; num < data->channum; num++)
> +		writel_relaxed(data->saved_reg[num + 1],
> +			       data->regs + CHANMASK(num, data->channum));
> +}
> +
> +static int imx_irqsteer_suspend(struct device *dev)
> +{
> +	struct irqsteer_irqchip_data *irqsteer_data = dev_get_drvdata(dev);
> +
> +	imx_irqsteer_save_regs(irqsteer_data);
> +	clk_disable_unprepare(irqsteer_data->ipg_clk);
> +
> +	return 0;
> +}
> +
> +static int imx_irqsteer_resume(struct device *dev)
> +{
> +	struct irqsteer_irqchip_data *irqsteer_data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(irqsteer_data->ipg_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable ipg clk: %d\n", ret);
> +		return ret;
> +	}
> +	imx_irqsteer_restore_regs(irqsteer_data);
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops imx_irqsteer_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_irqsteer_suspend, imx_irqsteer_resume)
> +};
> +
> +static const struct of_device_id imx_irqsteer_dt_ids[] = {
> +	{ .compatible = "fsl,imx-irqsteer", },
> +	{},
> +};
> +
> +static struct platform_driver imx_irqsteer_driver = {
> +	.driver = {
> +		.name = "imx-irqsteer",
> +		.of_match_table = imx_irqsteer_dt_ids,
> +		.pm = &imx_irqsteer_pm_ops,
> +	},
> +	.probe = imx_irqsteer_probe,
> +	.remove = imx_irqsteer_remove,
> +};
> +builtin_platform_driver(imx_irqsteer_driver);
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/2] dt-bindings: irq: add binding for Freescale IRQSTEER multiplexer

[Rob Herring]

On Tue, Oct 16, 2018 at 06:42:17PM +0200, Lucas Stach wrote:
> This adds the DT binding for the Freescale IRQSTEER interrupt
> multiplexer found in the i.MX8 familiy SoCs.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  .../interrupt-controller/fsl,irqsteer.txt     | 39 +++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> new file mode 100644
> index 000000000000..ed2b18165591
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> @@ -0,0 +1,39 @@
> +Freescale IRQSTEER Interrupt multiplexer
> +
> +Required properties:
> +
> +- compatible: should be:
> +	- "fsl,imx8m-irqsteer"
> +	- "fsl,imx-irqsteer"
> +- reg: Physical base address and size of registers.
> +- interrupts: Should contain the parent interrupt line used to multiplex the
> +  input interrupts.
> +- clocks: Should contain one clock for entry in clock-names
> +  see Documentation/devicetree/bindings/clock/clock-bindings.txt
> +- clock-names:
> +   - "ipg": main logic clock
> +- interrupt-controller: Identifies the node as an interrupt controller.
> +- #interrupt-cells: Specifies the number of cells needed to encode an
> +  interrupt source. The value must be 2.
> +
> +Optional properties:
> +- fsl,channel: Number of channels managed by this controller. Each channel
> +  contains up to 32 interrupt sources. If absent defaults to 1.

What's a channel? Why isn't this implied by the compatible?

> +- fsl,endian:

The standard property for endianness doesn't work for you?

> +  0: controller registers are little endian
> +  1: controller registers are big endian
> +  If absent defaults to 0.
> +
> +Example:
> +
> +	interrupt-controller@32e2d000 {
> +		compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";
> +		reg = <0x32e2d000 0x1000>;
> +		interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&clk IMX8MQ_CLK_DISP_APB_ROOT>;
> +		clock-names = "ipg";
> +		fsl,channel = <2>;
> +		fsl,endian = <1>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +	};
> -- 
> 2.19.0
> 

Re: [PATCH 1/2] dt-bindings: irq: add binding for Freescale IRQSTEER multiplexer

[Lucas Stach]

Hi Rob,

Am Donnerstag, den 25.10.2018, 09:35 -0500 schrieb Rob Herring:
> On Tue, Oct 16, 2018 at 06:42:17PM +0200, Lucas Stach wrote:
> > This adds the DT binding for the Freescale IRQSTEER interrupt
> > multiplexer found in the i.MX8 familiy SoCs.
> > 
> > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  .../interrupt-controller/fsl,irqsteer.txt     | 39 +++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > new file mode 100644
> > index 000000000000..ed2b18165591
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > @@ -0,0 +1,39 @@
> > +Freescale IRQSTEER Interrupt multiplexer
> > +
> > +Required properties:
> > +
> > +- compatible: should be:
> > > > +	- "fsl,imx8m-irqsteer"
> > > > +	- "fsl,imx-irqsteer"
> > +- reg: Physical base address and size of registers.
> > +- interrupts: Should contain the parent interrupt line used to multiplex the
> > +  input interrupts.
> > +- clocks: Should contain one clock for entry in clock-names
> > +  see Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +- clock-names:
> > +   - "ipg": main logic clock
> > +- interrupt-controller: Identifies the node as an interrupt controller.
> > +- #interrupt-cells: Specifies the number of cells needed to encode an
> > +  interrupt source. The value must be 2.
> > +
> > +Optional properties:
> > +- fsl,channel: Number of channels managed by this controller. Each channel
> > +  contains up to 32 interrupt sources. If absent defaults to 1.
> 
> What's a channel? Why isn't this implied by the compatible?

The documentation calls the mux outputs a channel. Basically the
irqsteer IP block can mux up to 512 input interrupts onto up to 8
output interrupt lines (just noticed that the 32 in the description is
wrong and should be 64).

So the channel count defines the number of input IRQs in steps of 64
and the output IRQs in steps of 1. The register layout/offsets don't
change with different number of channels, as those are always laid out
as if the controller supported the maximum count of 8 channels. It's
just the actual implemented number of registers that change.

Do you prefer to encode this in the compatible, or is it okay to keep
this a a parameter?

> > +- fsl,endian:
> 
> The standard property for endianness doesn't work for you?

It does, I'll switch to that.

Regards,
Lucas

Re: [PATCH 1/2] dt-bindings: irq: add binding for Freescale IRQSTEER multiplexer

[Rob Herring]

On Fri, Oct 26, 2018 at 11:11 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi Rob,
>
> Am Donnerstag, den 25.10.2018, 09:35 -0500 schrieb Rob Herring:
> > On Tue, Oct 16, 2018 at 06:42:17PM +0200, Lucas Stach wrote:
> > > This adds the DT binding for the Freescale IRQSTEER interrupt
> > > multiplexer found in the i.MX8 familiy SoCs.
> > >
> > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > ---
> > >  .../interrupt-controller/fsl,irqsteer.txt     | 39 +++++++++++++++++++
> > >  1 file changed, 39 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > > new file mode 100644
> > > index 000000000000..ed2b18165591
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > > @@ -0,0 +1,39 @@
> > > +Freescale IRQSTEER Interrupt multiplexer
> > > +
> > > +Required properties:
> > > +
> > > +- compatible: should be:
> > > > > +       - "fsl,imx8m-irqsteer"
> > > > > +       - "fsl,imx-irqsteer"
> > > +- reg: Physical base address and size of registers.
> > > +- interrupts: Should contain the parent interrupt line used to multiplex the
> > > +  input interrupts.
> > > +- clocks: Should contain one clock for entry in clock-names
> > > +  see Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > +- clock-names:
> > > +   - "ipg": main logic clock
> > > +- interrupt-controller: Identifies the node as an interrupt controller.
> > > +- #interrupt-cells: Specifies the number of cells needed to encode an
> > > +  interrupt source. The value must be 2.
> > > +
> > > +Optional properties:
> > > +- fsl,channel: Number of channels managed by this controller. Each channel
> > > +  contains up to 32 interrupt sources. If absent defaults to 1.
> >
> > What's a channel? Why isn't this implied by the compatible?
>
> The documentation calls the mux outputs a channel. Basically the
> irqsteer IP block can mux up to 512 input interrupts onto up to 8
> output interrupt lines (just noticed that the 32 in the description is
> wrong and should be 64).

Then shouldn't you have up to 8 int specifiers in 'interrupts' property?

> So the channel count defines the number of input IRQs in steps of 64
> and the output IRQs in steps of 1. The register layout/offsets don't
> change with different number of channels, as those are always laid out
> as if the controller supported the maximum count of 8 channels. It's
> just the actual implemented number of registers that change.

So do you actually need to know how many channels? We don't typically
put into DT what interrupt lines are actually connected. We assume the
interrupt connections in DT are correct.

Also, if 'interrupts' listed all the muxed output connections, then
you could get the same information from there.

Rob