[PATCH v2 0/2] imx-irqsteer for real

[PATCH v2 0/2] imx-irqsteer for real

[Lucas Stach]

Hi all,

this is a completely reworked submission of the imx-irqsteer driver. The
first version was bugged due to wrong/misleading documentation and buggy
implementation of the downstream driver that only worked correctly with
a very specific DT configuration.

The key information here is that a given SoC can have multiple instances
of the irqsteer controller, which can be connected to mutiple output IRQ
lines, but all the input IRQs to one instance are exclusively steered to
one of the output IRQs (which is called a channel). So it's a flexible
hardware design, were each irqsteer instance needs to be told which
upstream IRQ it should use, instead of this being fixed at SoC design
time.

Also the controller is configurable in the number of input IRQs supported,
but only in groups of 64 IRQs, which has an influence on the register
layout, as status, mask and force registers are packed together. Currently
existing implementations of the irqsteer block have between 1 and 8 IRQ
groups, which the documentation confusingly also calls a channel.

Also there is no per-groups status indication, but only a global status
bit, so the IRQ handling can not be optimized, but all status registers
must be checked.

After figuring out all those bits, the driver turned out to be pretty
simple.

Regards,
Lucas

Lucas Stach (2):
  dt-bindings: irq: add binding for Freescale IRQSTEER multiplexer
  irqchip: add driver for imx-irqsteer controller

 .../interrupt-controller/fsl,irqsteer.txt     |  34 +++
 drivers/irqchip/Kconfig                       |   8 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-imx-irqsteer.c            | 269 ++++++++++++++++++
 4 files changed, 312 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
 create mode 100644 drivers/irqchip/irq-imx-irqsteer.c

-- 
2.19.1

[PATCH v2 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     | 34 +++++++++++++++++++
 1 file changed, 34 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..fd4aaa379993
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
@@ -0,0 +1,34 @@
+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.
+- fsl,channel: The output channel that all input IRQs should be steered into.
+- fsl,irq-groups: Number of IRQ groups managed by this controller instance.
+  Each group manages 64 input interrupts.
+
+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 = <1>;
+		fsl,irq-groups = <1>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	};
-- 
2.19.1

[PATCH v2 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>
---
v2:
 - use raw_spinlock
 - add proper clk error handling
 - align register offset calculation with reality
 - use correct SPDX identifier
 - simplify DT configuration
---
 drivers/irqchip/Kconfig            |   8 +
 drivers/irqchip/Makefile           |   1 +
 drivers/irqchip/irq-imx-irqsteer.c | 269 +++++++++++++++++++++++++++++
 3 files changed, 278 insertions(+)
 create mode 100644 drivers/irqchip/irq-imx-irqsteer.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 51a5ef0e96ed..25607a002bf1 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -391,6 +391,14 @@ config CSKY_APB_INTC
 	  by C-SKY single core SOC system. It use mmio map apb-bus to visit
 	  the controller's register.
 
+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 794c13d3ac3d..8ef99b3f7e4e 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -91,3 +91,4 @@ obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
 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
diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
new file mode 100644
index 000000000000..099148045657
--- /dev/null
+++ b/drivers/irqchip/irq-imx-irqsteer.c
@@ -0,0 +1,269 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2017 NXP
+ * Copyright (C) 2018 Pengutronix, Lucas Stach <kernel@pengutronix.de>
+ */
+
+#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_platform.h>
+#include <linux/spinlock.h>
+
+#define CTRL_STRIDE_OFF(_t, _r)	(_t * 8 * _r)
+#define CHANCTRL		0x0
+#define CHANMASK(n, t)		(CTRL_STRIDE_OFF(t, 0) + 0x4 * (n) + 0x4)
+#define CHANSET(n, t)		(CTRL_STRIDE_OFF(t, 1) + 0x4 * (n) + 0x4)
+#define CHANSTATUS(n, t)	(CTRL_STRIDE_OFF(t, 2) + 0x4 * (n) + 0x4)
+#define CHAN_MINTDIS(t)		(CTRL_STRIDE_OFF(t, 3) + 0x4)
+#define CHAN_MASTRSTAT(t)	(CTRL_STRIDE_OFF(t, 3) + 0x8)
+
+struct irqsteer_data {
+	void __iomem		*regs;
+	struct clk		*ipg_clk;
+	int			irq;
+	raw_spinlock_t		lock;
+	int			irq_groups;
+	int			channel;
+	struct irq_domain	*domain;
+	u32			*saved_reg;
+};
+
+static int imx_irqsteer_get_reg_index(struct irqsteer_data *data,
+				      unsigned long irqnum)
+{
+	return (data->irq_groups * 2 - irqnum / 32 - 1);
+}
+
+static void imx_irqsteer_irq_unmask(struct irq_data *d)
+{
+	struct irqsteer_data *data = d->chip_data;
+	int idx = imx_irqsteer_get_reg_index(data, d->hwirq);
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&data->lock, flags);
+	val = readl_relaxed(data->regs + CHANMASK(idx, data->irq_groups));
+	val |= BIT(d->hwirq % 32);
+	writel_relaxed(val, data->regs + CHANMASK(idx, data->irq_groups));
+	raw_spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static void imx_irqsteer_irq_mask(struct irq_data *d)
+{
+	struct irqsteer_data *data = d->chip_data;
+	int idx = imx_irqsteer_get_reg_index(data, d->hwirq);
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&data->lock, flags);
+	val = readl_relaxed(data->regs + CHANMASK(idx, data->irq_groups));
+	val &= ~BIT(d->hwirq % 32);
+	writel_relaxed(val, data->regs + CHANMASK(idx, data->irq_groups));
+	raw_spin_unlock_irqrestore(&data->lock, flags);
+}
+
+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_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_data *data = irq_desc_get_handler_data(desc);
+	int i;
+
+	chained_irq_enter(irq_desc_get_chip(desc), desc);
+
+	for (i = 0; i < data->irq_groups * 64; i += 32) {
+		int idx = imx_irqsteer_get_reg_index(data, i);
+		unsigned long irqmap;
+		int pos, virq;
+
+		irqmap = readl_relaxed(data->regs +
+				       CHANSTATUS(idx, data->irq_groups));
+
+		for_each_set_bit(pos, &irqmap, 32) {
+			virq = irq_find_mapping(data->domain, pos + i);
+			if (virq)
+				generic_handle_irq(virq);
+		}
+	}
+
+	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_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);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
+		return ret;
+	}
+
+	raw_spin_lock_init(&data->lock);
+
+	of_property_read_u32(np, "fsl,irq-groups", &data->irq_groups);
+	of_property_read_u32(np, "fsl,channel", &data->channel);
+
+	if (IS_ENABLED(CONFIG_PM_SLEEP)) {
+		data->saved_reg = devm_kzalloc(&pdev->dev,
+					sizeof(u32) * data->irq_groups * 2,
+					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;
+	}
+
+	/* steer all IRQs into configured channel */
+	writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
+
+	data->domain = irq_domain_add_linear(np, data->irq_groups * 64,
+					     &imx_irqsteer_domain_ops, data);
+	if (!data->domain) {
+		dev_err(&pdev->dev, "failed to create IRQ domain\n");
+		clk_disable_unprepare(data->ipg_clk);
+		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_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_data *data)
+{
+	int num;
+
+	for (num = 0; num < data->irq_groups; num++)
+		data->saved_reg[num + 1] = readl_relaxed(data->regs +
+						CHANMASK(num, data->irq_groups));
+}
+
+static void imx_irqsteer_restore_regs(struct irqsteer_data *data)
+{
+	int num;
+
+	writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
+	for (num = 0; num < data->irq_groups; num++)
+		writel_relaxed(data->saved_reg[num + 1],
+			       data->regs + CHANMASK(num, data->irq_groups));
+}
+
+static int imx_irqsteer_suspend(struct device *dev)
+{
+	struct irqsteer_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_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.1

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

[Marc Zyngier]

Hi Lucas,

On Fri, 14 Dec 2018 10:22:44 +0000,
Lucas Stach <l.stach@pengutronix.de> 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>
> ---
> v2:
>  - use raw_spinlock
>  - add proper clk error handling
>  - align register offset calculation with reality
>  - use correct SPDX identifier
>  - simplify DT configuration
> ---
>  drivers/irqchip/Kconfig            |   8 +
>  drivers/irqchip/Makefile           |   1 +
>  drivers/irqchip/irq-imx-irqsteer.c | 269 +++++++++++++++++++++++++++++
>  3 files changed, 278 insertions(+)
>  create mode 100644 drivers/irqchip/irq-imx-irqsteer.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 51a5ef0e96ed..25607a002bf1 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -391,6 +391,14 @@ config CSKY_APB_INTC
>  	  by C-SKY single core SOC system. It use mmio map apb-bus to visit
>  	  the controller's register.
>  
> +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 794c13d3ac3d..8ef99b3f7e4e 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -91,3 +91,4 @@ obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
>  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
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> new file mode 100644
> index 000000000000..099148045657
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2017 NXP
> + * Copyright (C) 2018 Pengutronix, Lucas Stach <kernel@pengutronix.de>
> + */
> +
> +#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_platform.h>
> +#include <linux/spinlock.h>
> +
> +#define CTRL_STRIDE_OFF(_t, _r)	(_t * 8 * _r)
> +#define CHANCTRL		0x0
> +#define CHANMASK(n, t)		(CTRL_STRIDE_OFF(t, 0) + 0x4 * (n) + 0x4)
> +#define CHANSET(n, t)		(CTRL_STRIDE_OFF(t, 1) + 0x4 * (n) + 0x4)
> +#define CHANSTATUS(n, t)	(CTRL_STRIDE_OFF(t, 2) + 0x4 * (n) + 0x4)
> +#define CHAN_MINTDIS(t)		(CTRL_STRIDE_OFF(t, 3) + 0x4)
> +#define CHAN_MASTRSTAT(t)	(CTRL_STRIDE_OFF(t, 3) + 0x8)
> +
> +struct irqsteer_data {
> +	void __iomem		*regs;
> +	struct clk		*ipg_clk;
> +	int			irq;
> +	raw_spinlock_t		lock;
> +	int			irq_groups;
> +	int			channel;
> +	struct irq_domain	*domain;
> +	u32			*saved_reg;
> +};
> +
> +static int imx_irqsteer_get_reg_index(struct irqsteer_data *data,
> +				      unsigned long irqnum)
> +{
> +	return (data->irq_groups * 2 - irqnum / 32 - 1);
> +}
> +
> +static void imx_irqsteer_irq_unmask(struct irq_data *d)
> +{
> +	struct irqsteer_data *data = d->chip_data;
> +	int idx = imx_irqsteer_get_reg_index(data, d->hwirq);
> +	unsigned long flags;
> +	u32 val;
> +
> +	raw_spin_lock_irqsave(&data->lock, flags);
> +	val = readl_relaxed(data->regs + CHANMASK(idx, data->irq_groups));
> +	val |= BIT(d->hwirq % 32);
> +	writel_relaxed(val, data->regs + CHANMASK(idx, data->irq_groups));
> +	raw_spin_unlock_irqrestore(&data->lock, flags);
> +}
> +
> +static void imx_irqsteer_irq_mask(struct irq_data *d)
> +{
> +	struct irqsteer_data *data = d->chip_data;
> +	int idx = imx_irqsteer_get_reg_index(data, d->hwirq);
> +	unsigned long flags;
> +	u32 val;
> +
> +	raw_spin_lock_irqsave(&data->lock, flags);
> +	val = readl_relaxed(data->regs + CHANMASK(idx, data->irq_groups));
> +	val &= ~BIT(d->hwirq % 32);
> +	writel_relaxed(val, data->regs + CHANMASK(idx, data->irq_groups));
> +	raw_spin_unlock_irqrestore(&data->lock, flags);
> +}
> +
> +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_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,

I've tried to understand why you are using two cells, but couldn't
find an explanation from the DT binding. As far as I can see, you only
support LEVEL_HIGH signalling, meaning that you could perfectly
encode this in a single cell. What am I missing?

> +};
> +
> +static void imx_irqsteer_irq_handler(struct irq_desc *desc)
> +{
> +	struct irqsteer_data *data = irq_desc_get_handler_data(desc);
> +	int i;
> +
> +	chained_irq_enter(irq_desc_get_chip(desc), desc);
> +
> +	for (i = 0; i < data->irq_groups * 64; i += 32) {
> +		int idx = imx_irqsteer_get_reg_index(data, i);
> +		unsigned long irqmap;
> +		int pos, virq;
> +
> +		irqmap = readl_relaxed(data->regs +
> +				       CHANSTATUS(idx, data->irq_groups));
> +
> +		for_each_set_bit(pos, &irqmap, 32) {
> +			virq = irq_find_mapping(data->domain, pos + i);
> +			if (virq)
> +				generic_handle_irq(virq);
> +		}
> +	}
> +
> +	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_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);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	raw_spin_lock_init(&data->lock);
> +
> +	of_property_read_u32(np, "fsl,irq-groups", &data->irq_groups);
> +	of_property_read_u32(np, "fsl,channel", &data->channel);
> +
> +	if (IS_ENABLED(CONFIG_PM_SLEEP)) {
> +		data->saved_reg = devm_kzalloc(&pdev->dev,
> +					sizeof(u32) * data->irq_groups * 2,
> +					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;
> +	}
> +
> +	/* steer all IRQs into configured channel */
> +	writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);

Could you explain what this channel is exactly?

> +
> +	data->domain = irq_domain_add_linear(np, data->irq_groups * 64,
> +					     &imx_irqsteer_domain_ops, data);
> +	if (!data->domain) {
> +		dev_err(&pdev->dev, "failed to create IRQ domain\n");
> +		clk_disable_unprepare(data->ipg_clk);
> +		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_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_data *data)
> +{
> +	int num;
> +
> +	for (num = 0; num < data->irq_groups; num++)
> +		data->saved_reg[num + 1] = readl_relaxed(data->regs +
> +						CHANMASK(num, data->irq_groups));

You seem to only use one u32 per IRQ group. Yet, you've allocated
twice as much memory to that effect. Why? Also, you're avoiding to use
index 0, which I find rather odd.

Given that the DT binding states that each group manages 64
interrupts, I have the feeling that you're missing some interrupt
enable state.

> +}
> +
> +static void imx_irqsteer_restore_regs(struct irqsteer_data *data)
> +{
> +	int num;
> +
> +	writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
> +	for (num = 0; num < data->irq_groups; num++)
> +		writel_relaxed(data->saved_reg[num + 1],
> +			       data->regs + CHANMASK(num, data->irq_groups));
> +}
> +
> +static int imx_irqsteer_suspend(struct device *dev)
> +{
> +	struct irqsteer_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_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.1
> 

This looks much better than the original submission. If you quickly
clarify the few questions I have, we can probably move this quickly
enough.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

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

[Lucas Stach]

Hi Marc,

Am Samstag, den 15.12.2018, 09:45 +0000 schrieb Marc Zyngier:
> Hi Lucas,
> 
> On Fri, 14 Dec 2018 10:22:44 +0000,
> > Lucas Stach <l.stach@pengutronix.de> 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>
> > ---
> > v2:
> >  - use raw_spinlock
> >  - add proper clk error handling
> >  - align register offset calculation with reality
> >  - use correct SPDX identifier
> >  - simplify DT configuration
> > ---

[...]

> > +static const struct irq_domain_ops imx_irqsteer_domain_ops = {
> > > > > > +	.map		= imx_irqsteer_irq_map,
> > +	.xlate		= irq_domain_xlate_twocell,
> 
> I've tried to understand why you are using two cells, but couldn't
> find an explanation from the DT binding. As far as I can see, you only
> support LEVEL_HIGH signalling, meaning that you could perfectly
> encode this in a single cell. What am I missing?

Nothing. At this point I'm sure that the controller only support level-
high IRQs. Will switch to a single cell in the next spin.

[...]

> > +static int imx_irqsteer_probe(struct platform_device *pdev)
> > +{
> > > > +	struct device_node *np = pdev->dev.of_node;
> > > > +	struct irqsteer_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);
> > > > +		if (ret != -EPROBE_DEFER)
> > > > +			dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > +
> > > > +	raw_spin_lock_init(&data->lock);
> > +
> > > > +	of_property_read_u32(np, "fsl,irq-groups", &data->irq_groups);
> > > > +	of_property_read_u32(np, "fsl,channel", &data->channel);
> > +
> > > > +	if (IS_ENABLED(CONFIG_PM_SLEEP)) {
> > > > +		data->saved_reg = devm_kzalloc(&pdev->dev,
> > > > +					sizeof(u32) * data->irq_groups * 2,
> > > > +					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;
> > > > +	}
> > +
> > > > +	/* steer all IRQs into configured channel */
> > +	writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
> 
> Could you explain what this channel is exactly?

I've tired in the cover letter, but seems I still failed, so let me try
again. ;)

Each irqsteer instance can be connected to multiple upstream IRQ lines,
but only one of them can be used at runtime. This register controls
which output IRQ line will be used by this controller instance. With
multiple controller instances in the system it's a way to offload the
decision of the IRQ routing from the hardware to the software guys.

Let's try to add an example: Suppose there are 2 instances of the
irqsteer controller. Both are connected to upstream GIC IRQs 20 and 21.
The channel controls which of those IRQs are used by each instance, so
there are 2 valid DT configurations in this scenario:

This is valid:

irqsteer@0 {
	interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>
	fsl,channel = <0>;
};

irqsteer@1 {
	interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>
	fsl,channel = <1>;
};

As well as this:

irqsteer@0 {
	interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>
	fsl,channel = <1>;
};

irqsteer@1 {
	interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>
	fsl,channel = <0>;
};
	
[...]
> > +#ifdef CONFIG_PM_SLEEP
> > +static void imx_irqsteer_save_regs(struct irqsteer_data *data)
> > +{
> > > > +	int num;
> > +
> > > > +	for (num = 0; num < data->irq_groups; num++)
> > > > +		data->saved_reg[num + 1] = readl_relaxed(data->regs +
> > +						CHANMASK(num, data->irq_groups));
> 
> You seem to only use one u32 per IRQ group. Yet, you've allocated
> twice as much memory to that effect. Why? Also, you're avoiding to use
> index 0, which I find rather odd.
> 
> Given that the DT binding states that each group manages 64
> interrupts, I have the feeling that you're missing some interrupt
> enable state.

And you are right with this observation. I failed to properly rework
this part of the code when switching to the irq-group indexing.

Regards,
Lucas

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

[Marc Zyngier]

Hi Lucas,

On 17/12/2018 10:09, Lucas Stach wrote:
> Hi Marc,
> 
> Am Samstag, den 15.12.2018, 09:45 +0000 schrieb Marc Zyngier:
>> Hi Lucas,
>>
>> On Fri, 14 Dec 2018 10:22:44 +0000,
>>> Lucas Stach <l.stach@pengutronix.de> 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>
>>> ---
>>> v2:
>>>  - use raw_spinlock
>>>  - add proper clk error handling
>>>  - align register offset calculation with reality
>>>  - use correct SPDX identifier
>>>  - simplify DT configuration
>>> ---
> 
> [...]
> 
>>> +static const struct irq_domain_ops imx_irqsteer_domain_ops = {
>>>>>>> +	.map		= imx_irqsteer_irq_map,
>>> +	.xlate		= irq_domain_xlate_twocell,
>>
>> I've tried to understand why you are using two cells, but couldn't
>> find an explanation from the DT binding. As far as I can see, you only
>> support LEVEL_HIGH signalling, meaning that you could perfectly
>> encode this in a single cell. What am I missing?
> 
> Nothing. At this point I'm sure that the controller only support level-
> high IRQs. Will switch to a single cell in the next spin.

OK, good.

> 
> [...]
> 
>>> +static int imx_irqsteer_probe(struct platform_device *pdev)
>>> +{
>>>>> +	struct device_node *np = pdev->dev.of_node;
>>>>> +	struct irqsteer_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);
>>>>> +		if (ret != -EPROBE_DEFER)
>>>>> +			dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
>>>>> +		return ret;
>>>>> +	}
>>> +
>>>>> +	raw_spin_lock_init(&data->lock);
>>> +
>>>>> +	of_property_read_u32(np, "fsl,irq-groups", &data->irq_groups);
>>>>> +	of_property_read_u32(np, "fsl,channel", &data->channel);
>>> +
>>>>> +	if (IS_ENABLED(CONFIG_PM_SLEEP)) {
>>>>> +		data->saved_reg = devm_kzalloc(&pdev->dev,
>>>>> +					sizeof(u32) * data->irq_groups * 2,
>>>>> +					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;
>>>>> +	}
>>> +
>>>>> +	/* steer all IRQs into configured channel */
>>> +	writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
>>
>> Could you explain what this channel is exactly?
> 
> I've tired in the cover letter, but seems I still failed, so let me try
> again. ;)
> 
> Each irqsteer instance can be connected to multiple upstream IRQ lines,
> but only one of them can be used at runtime. This register controls
> which output IRQ line will be used by this controller instance. With
> multiple controller instances in the system it's a way to offload the
> decision of the IRQ routing from the hardware to the software guys.
> 
> Let's try to add an example: Suppose there are 2 instances of the
> irqsteer controller. Both are connected to upstream GIC IRQs 20 and 21.
> The channel controls which of those IRQs are used by each instance, so
> there are 2 valid DT configurations in this scenario:
> 
> This is valid:
> 
> irqsteer@0 {
> 	interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>
> 	fsl,channel = <0>;
> };
> 
> irqsteer@1 {
> 	interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>
> 	fsl,channel = <1>;
> };
> 
> As well as this:
> 
> irqsteer@0 {
> 	interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>
> 	fsl,channel = <1>;
> };
> 
> irqsteer@1 {
> 	interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>
> 	fsl,channel = <0>;
> };

OK, this is now making sense, thanks for that. I'm wondering if it'd
make sense to expose both IRQs in the DT for each irqsteer, and use
fsl,channel as the selector? It doesn't change much in the driver, but
seems to describe the HW in a more complete way.

I don't care much either way, and I'll leave it for you and the DT folks
to decide.

> 	
> [...]
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static void imx_irqsteer_save_regs(struct irqsteer_data *data)
>>> +{
>>>>> +	int num;
>>> +
>>>>> +	for (num = 0; num < data->irq_groups; num++)
>>>>> +		data->saved_reg[num + 1] = readl_relaxed(data->regs +
>>> +						CHANMASK(num, data->irq_groups));
>>
>> You seem to only use one u32 per IRQ group. Yet, you've allocated
>> twice as much memory to that effect. Why? Also, you're avoiding to use
>> index 0, which I find rather odd.
>>
>> Given that the DT binding states that each group manages 64
>> interrupts, I have the feeling that you're missing some interrupt
>> enable state.
> 
> And you are right with this observation. I failed to properly rework
> this part of the code when switching to the irq-group indexing.

Thanks,

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

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

[Lucas Stach]

Am Montag, den 17.12.2018, 10:32 +0000 schrieb Marc Zyngier:
[...]
> > > > > > +	/* steer all IRQs into configured channel */
> > > > +	writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
> > > 
> > > Could you explain what this channel is exactly?
> > 
> > I've tired in the cover letter, but seems I still failed, so let me try
> > again. ;)
> > 
> > Each irqsteer instance can be connected to multiple upstream IRQ lines,
> > but only one of them can be used at runtime. This register controls
> > which output IRQ line will be used by this controller instance. With
> > multiple controller instances in the system it's a way to offload the
> > decision of the IRQ routing from the hardware to the software guys.
> > 
> > Let's try to add an example: Suppose there are 2 instances of the
> > irqsteer controller. Both are connected to upstream GIC IRQs 20 and 21.
> > The channel controls which of those IRQs are used by each instance, so
> > there are 2 valid DT configurations in this scenario:
> > 
> > This is valid:
> > 
> > irqsteer@0 {
> > 	interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>
> > 	fsl,channel = <0>;
> > };
> > 
> > irqsteer@1 {
> > 	interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>
> > 	fsl,channel = <1>;
> > };
> > 
> > As well as this:
> > 
> > irqsteer@0 {
> > 	interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>
> > 	fsl,channel = <1>;
> > };
> > 
> > irqsteer@1 {
> > 	interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>
> > 	fsl,channel = <0>;
> > };
> 
> OK, this is now making sense, thanks for that. I'm wondering if it'd
> make sense to expose both IRQs in the DT for each irqsteer, and use
> fsl,channel as the selector? It doesn't change much in the driver, but
> seems to describe the HW in a more complete way.
> 
> I don't care much either way, and I'll leave it for you and the DT folks
> to decide.

At least according to the preliminary documentation available about the
i.MX8QM not all of the channels are routed to an upstream IRQ which is
visible to the Linux system. Some of them may also go to the Cortex-M
subsystem, so for your suggestion to work I would need a scheme to
describe the output interrupts with holes in between them.

I guess that complicates things a bit too much for little gain, as I
don't see us switching the controller between different channels at
runtime (which is the only thing I could imagine which would benefit
from this more complete HW description). The current binding can deal
with having some channels which route to something invisible to the
Linux system just fine, so I'm leaning toward keeping things as they
are now.

Regards,
Lucas

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

[Marc Zyngier]

On 17/12/2018 13:52, Lucas Stach wrote:
> Am Montag, den 17.12.2018, 10:32 +0000 schrieb Marc Zyngier:
> [...]
>> OK, this is now making sense, thanks for that. I'm wondering if it'd
>> make sense to expose both IRQs in the DT for each irqsteer, and use
>> fsl,channel as the selector? It doesn't change much in the driver, but
>> seems to describe the HW in a more complete way.
>>
>> I don't care much either way, and I'll leave it for you and the DT folks
>> to decide.
> 
> At least according to the preliminary documentation available about the
> i.MX8QM not all of the channels are routed to an upstream IRQ which is
> visible to the Linux system. Some of them may also go to the Cortex-M
> subsystem, so for your suggestion to work I would need a scheme to
> describe the output interrupts with holes in between them.
> 
> I guess that complicates things a bit too much for little gain, as I
> don't see us switching the controller between different channels at
> runtime (which is the only thing I could imagine which would benefit
> from this more complete HW description). The current binding can deal
> with having some channels which route to something invisible to the
> Linux system just fine, so I'm leaning toward keeping things as they
> are now.

Fair enough. I'll freeze the tree tomorrow, so if you want this into
4.21, now is the time.

Thanks,

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