[PATCH V2 0/4] irq: imx-irqsteer: add 32 interrupts chan and multi outputs support

[PATCH V2 0/4] irq: imx-irqsteer: add 32 interrupts chan and multi outputs support

[Aisheng Dong]

Not all 64 interrupts may be used in one group. e.g. most irqsteer in
imx8qxp and imx8qm subsystems supports only 32 interrupts.
And one irqsteer channel can support up to 8 output interrupts.

This patch series aims to support 32 interrupts chan and multi output
interrupts.

Tested on:
iMX8QXP MEK with MIPI CSI capture and DC Display
iMX8MQ EVK with MIPI DSI Display

Dong Aisheng (4):
  dt-binding: irq: imx-irqsteer: use irq number instead of group number
  dt-bindings: irq: imx-irqsteer: add multi output interrupts support
  irq: imx-irqsteer: change to use reg_num instead of irq_group
  irq: imx: irqsteer: add multi output interrupts support

 .../bindings/interrupt-controller/fsl,irqsteer.txt |  11 ++-
 drivers/irqchip/irq-imx-irqsteer.c                 | 103 +++++++++++++++------
 2 files changed, 80 insertions(+), 34 deletions(-)

-- 
2.7.4

[PATCH V2 1/4] dt-binding: irq: imx-irqsteer: use irq number instead of group number

[Aisheng Dong]

Not all 64 interrupts may be used in one group. e.g. most irqsteer in
imx8qxp and imx8qm subsystems supports only 32 interrupts.

As the IP integration parameters are Channel number and interrupts number,
let's use fsl,num-irqs to represents how many interrupts supported
by this irqsteer channel.

Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
 v1->v2:
  * change property name fsl,irqs-per-chan to fsl,num-irqs.
---
 .../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt       | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
index 45790ce..6d0a41b 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
@@ -16,8 +16,8 @@ Required properties:
 - #interrupt-cells: Specifies the number of cells needed to encode an
   interrupt source. The value must be 1.
 - 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.
+- fsl,num-irqs: Number of input interrupts of this channel.
+  Should be multiple of 32 input interrupts and up to 512 interrupts.
 
 Example:
 
@@ -28,7 +28,7 @@ Example:
 		clocks = <&clk IMX8MQ_CLK_DISP_APB_ROOT>;
 		clock-names = "ipg";
 		fsl,channel = <0>;
-		fsl,irq-groups = <1>;
+		fsl,num-irqs = <64>;
 		interrupt-controller;
 		#interrupt-cells = <1>;
 	};
-- 
2.7.4

[PATCH V2 2/4] dt-bindings: irq: imx-irqsteer: add multi output interrupts support

[Aisheng Dong]

One irqsteer channel can support up to 8 output interrupts.

Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
v1->v2:
 * remove one unnecessary note.
---
 .../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt        | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
index 6d0a41b..984ab92 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
@@ -6,8 +6,9 @@ Required properties:
 	- "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.
+- interrupts: Should contain the up to 8 parent interrupt lines used to
+  multiplex the input interrupts. They should be sepcified seqencially
+  from output 0 to 7.
 - clocks: Should contain one clock for entry in clock-names
   see Documentation/devicetree/bindings/clock/clock-bindings.txt
 - clock-names:
-- 
2.7.4

[PATCH V2 3/4] irq: imx-irqsteer: change to use reg_num instead of irq_group

[Aisheng Dong]

One group can manage 64 interrupts by using two registers (e.g. STATUS/SET).
However, the integrated irqsteer may support only 32 interrupts which
needs only one register in a group. But the current driver assume there's
a mininum of two registers in a group which result in a wrong register map
for 32 interrupts per channel irqsteer. Let's use the reg_num caculated by
interrupts per channel instead of irq_group to cover this case.

Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
v1->v2:
 * The using of property name updated accordingly
---
 drivers/irqchip/irq-imx-irqsteer.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
index 5b3f1d7..67ed862 100644
--- a/drivers/irqchip/irq-imx-irqsteer.c
+++ b/drivers/irqchip/irq-imx-irqsteer.c
@@ -13,7 +13,7 @@
 #include <linux/of_platform.h>
 #include <linux/spinlock.h>
 
-#define CTRL_STRIDE_OFF(_t, _r)	(_t * 8 * _r)
+#define CTRL_STRIDE_OFF(_t, _r)	(_t * 4 * _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)
@@ -26,7 +26,7 @@ struct irqsteer_data {
 	struct clk		*ipg_clk;
 	int			irq;
 	raw_spinlock_t		lock;
-	int			irq_groups;
+	int			reg_num;
 	int			channel;
 	struct irq_domain	*domain;
 	u32			*saved_reg;
@@ -35,7 +35,7 @@ struct irqsteer_data {
 static int imx_irqsteer_get_reg_index(struct irqsteer_data *data,
 				      unsigned long irqnum)
 {
-	return (data->irq_groups * 2 - irqnum / 32 - 1);
+	return (data->reg_num - irqnum / 32 - 1);
 }
 
 static void imx_irqsteer_irq_unmask(struct irq_data *d)
@@ -46,9 +46,9 @@ static void imx_irqsteer_irq_unmask(struct irq_data *d)
 	u32 val;
 
 	raw_spin_lock_irqsave(&data->lock, flags);
-	val = readl_relaxed(data->regs + CHANMASK(idx, data->irq_groups));
+	val = readl_relaxed(data->regs + CHANMASK(idx, data->reg_num));
 	val |= BIT(d->hwirq % 32);
-	writel_relaxed(val, data->regs + CHANMASK(idx, data->irq_groups));
+	writel_relaxed(val, data->regs + CHANMASK(idx, data->reg_num));
 	raw_spin_unlock_irqrestore(&data->lock, flags);
 }
 
@@ -60,9 +60,9 @@ static void imx_irqsteer_irq_mask(struct irq_data *d)
 	u32 val;
 
 	raw_spin_lock_irqsave(&data->lock, flags);
-	val = readl_relaxed(data->regs + CHANMASK(idx, data->irq_groups));
+	val = readl_relaxed(data->regs + CHANMASK(idx, data->reg_num));
 	val &= ~BIT(d->hwirq % 32);
-	writel_relaxed(val, data->regs + CHANMASK(idx, data->irq_groups));
+	writel_relaxed(val, data->regs + CHANMASK(idx, data->reg_num));
 	raw_spin_unlock_irqrestore(&data->lock, flags);
 }
 
@@ -94,13 +94,13 @@ static void imx_irqsteer_irq_handler(struct irq_desc *desc)
 
 	chained_irq_enter(irq_desc_get_chip(desc), desc);
 
-	for (i = 0; i < data->irq_groups * 64; i += 32) {
+	for (i = 0; i < data->reg_num * 32; 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));
+				       CHANSTATUS(idx, data->reg_num));
 
 		for_each_set_bit(pos, &irqmap, 32) {
 			virq = irq_find_mapping(data->domain, pos + i);
@@ -146,12 +146,15 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
 
 	raw_spin_lock_init(&data->lock);
 
-	of_property_read_u32(np, "fsl,irq-groups", &data->irq_groups);
+	of_property_read_u32(np, "fsl,num-irqs", &data->reg_num);
 	of_property_read_u32(np, "fsl,channel", &data->channel);
 
+	/* one register bit map represents 32 input interrupts */
+	data->reg_num /= 32;
+
 	if (IS_ENABLED(CONFIG_PM_SLEEP)) {
 		data->saved_reg = devm_kzalloc(&pdev->dev,
-					sizeof(u32) * data->irq_groups * 2,
+					sizeof(u32) * data->reg_num,
 					GFP_KERNEL);
 		if (!data->saved_reg)
 			return -ENOMEM;
@@ -166,7 +169,7 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
 	/* 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,
+	data->domain = irq_domain_add_linear(np, data->reg_num * 32,
 					     &imx_irqsteer_domain_ops, data);
 	if (!data->domain) {
 		dev_err(&pdev->dev, "failed to create IRQ domain\n");
@@ -199,9 +202,9 @@ static void imx_irqsteer_save_regs(struct irqsteer_data *data)
 {
 	int i;
 
-	for (i = 0; i < data->irq_groups * 2; i++)
+	for (i = 0; i < data->reg_num; i++)
 		data->saved_reg[i] = readl_relaxed(data->regs +
-						CHANMASK(i, data->irq_groups));
+						CHANMASK(i, data->reg_num));
 }
 
 static void imx_irqsteer_restore_regs(struct irqsteer_data *data)
@@ -209,9 +212,9 @@ static void imx_irqsteer_restore_regs(struct irqsteer_data *data)
 	int i;
 
 	writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
-	for (i = 0; i < data->irq_groups * 2; i++)
+	for (i = 0; i < data->reg_num; i++)
 		writel_relaxed(data->saved_reg[i],
-			       data->regs + CHANMASK(i, data->irq_groups));
+			       data->regs + CHANMASK(i, data->reg_num));
 }
 
 static int imx_irqsteer_suspend(struct device *dev)
-- 
2.7.4

[PATCH V2 4/4] irq: imx: irqsteer: add multi output interrupts support

[Aisheng Dong]

One irqsteer channel can support up to 8 output interrupts.

Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
ChangeLog:
v1->v2:
 * calculate irq_count by fsl,num-irqs instead of parsing interrupts
   property from devicetree to match the input interrupts and outputs
 * improve output interrupt handler by searching only two registers
   withint the same group
---
 drivers/irqchip/irq-imx-irqsteer.c | 76 +++++++++++++++++++++++++++++---------
 1 file changed, 59 insertions(+), 17 deletions(-)

diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
index 67ed862..cc40039 100644
--- a/drivers/irqchip/irq-imx-irqsteer.c
+++ b/drivers/irqchip/irq-imx-irqsteer.c
@@ -10,6 +10,7 @@
 #include <linux/irqchip/chained_irq.h>
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
+#include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/spinlock.h>
 
@@ -21,10 +22,13 @@
 #define CHAN_MINTDIS(t)		(CTRL_STRIDE_OFF(t, 3) + 0x4)
 #define CHAN_MASTRSTAT(t)	(CTRL_STRIDE_OFF(t, 3) + 0x8)
 
+#define CHAN_MAX_OUTPUT_INT	0x8
+
 struct irqsteer_data {
 	void __iomem		*regs;
 	struct clk		*ipg_clk;
-	int			irq;
+	int			irq[CHAN_MAX_OUTPUT_INT];
+	int			irq_count;
 	raw_spinlock_t		lock;
 	int			reg_num;
 	int			channel;
@@ -87,26 +91,45 @@ static const struct irq_domain_ops imx_irqsteer_domain_ops = {
 	.xlate		= irq_domain_xlate_onecell,
 };
 
+static int imx_irqsteer_get_hwirq_base(struct irqsteer_data *data, u32 irq)
+{
+	int i;
+
+	for (i = 0; i < data->irq_count; i++) {
+		if (data->irq[i] == irq)
+			break;
+	}
+
+	return i * 64;
+}
+
 static void imx_irqsteer_irq_handler(struct irq_desc *desc)
 {
 	struct irqsteer_data *data = irq_desc_get_handler_data(desc);
+	int hwirq;
 	int i;
 
 	chained_irq_enter(irq_desc_get_chip(desc), desc);
 
-	for (i = 0; i < data->reg_num * 32; i += 32) {
-		int idx = imx_irqsteer_get_reg_index(data, i);
+	hwirq = imx_irqsteer_get_hwirq_base(data, irq_desc_get_irq(desc));
+
+	for (i = 0; i < 2; i++) {
+		int idx = imx_irqsteer_get_reg_index(data, hwirq);
 		unsigned long irqmap;
 		int pos, virq;
 
+		if (hwirq >= data->reg_num * 32)
+			break;
+
 		irqmap = readl_relaxed(data->regs +
 				       CHANSTATUS(idx, data->reg_num));
 
 		for_each_set_bit(pos, &irqmap, 32) {
-			virq = irq_find_mapping(data->domain, pos + i);
+			virq = irq_find_mapping(data->domain, pos + hwirq);
 			if (virq)
 				generic_handle_irq(virq);
 		}
+		hwirq += 32;
 	}
 
 	chained_irq_exit(irq_desc_get_chip(desc), desc);
@@ -117,7 +140,8 @@ 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;
+	u32 irqs_num;
+	int i, ret;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -130,12 +154,6 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
 		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);
@@ -146,11 +164,17 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
 
 	raw_spin_lock_init(&data->lock);
 
-	of_property_read_u32(np, "fsl,num-irqs", &data->reg_num);
+	of_property_read_u32(np, "fsl,num-irqs", &irqs_num);
 	of_property_read_u32(np, "fsl,channel", &data->channel);
 
-	/* one register bit map represents 32 input interrupts */
-	data->reg_num /= 32;
+	/*
+	 * There is one output irqs for each group of 64 inputs.
+	 * One register bit map can represent 32 input interrupts.
+	 */
+	data->irq_count = irqs_num / 64;
+	if (irqs_num % 64)
+		data->irq_count += 1;
+	data->reg_num = irqs_num / 32;
 
 	if (IS_ENABLED(CONFIG_PM_SLEEP)) {
 		data->saved_reg = devm_kzalloc(&pdev->dev,
@@ -177,8 +201,22 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler,
-					 data);
+	if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT) {
+		clk_disable_unprepare(data->ipg_clk);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < data->irq_count; i++) {
+		data->irq[i] = irq_of_parse_and_map(np, i);
+		if (!data->irq[i]) {
+			clk_disable_unprepare(data->ipg_clk);
+			return -EINVAL;
+		}
+
+		irq_set_chained_handler_and_data(data->irq[i],
+						 imx_irqsteer_irq_handler,
+						 data);
+	}
 
 	platform_set_drvdata(pdev, data);
 
@@ -188,8 +226,12 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
 static int imx_irqsteer_remove(struct platform_device *pdev)
 {
 	struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < irqsteer_data->irq_count; i++)
+		irq_set_chained_handler_and_data(irqsteer_data->irq[i],
+						 NULL, NULL);
 
-	irq_set_chained_handler_and_data(irqsteer_data->irq, NULL, NULL);
 	irq_domain_remove(irqsteer_data->domain);
 
 	clk_disable_unprepare(irqsteer_data->ipg_clk);
-- 
2.7.4

Re: [PATCH V2 1/4] dt-binding: irq: imx-irqsteer: use irq number instead of group number

[Lucas Stach]

Am Mittwoch, den 30.01.2019, 13:05 +0000 schrieb Aisheng Dong:
> Not all 64 interrupts may be used in one group. e.g. most irqsteer in
> imx8qxp and imx8qm subsystems supports only 32 interrupts.
> 
> As the IP integration parameters are Channel number and interrupts number,
> let's use fsl,num-irqs to represents how many interrupts supported
> by this irqsteer channel.
> 
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

As the original fsl,irq-groups was born out of a misunderstanding of
the HW config options and I'm not aware of any users of the current
binding, I'm fine with merging this. I do think this warrants a
backport to stable though, so we don't end up with different bindings
in stable and current kernels.

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>


> ---
> ChangeLog:
>  v1->v2:
>   * change property name fsl,irqs-per-chan to fsl,num-irqs.
> ---
>  .../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt       | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> index 45790ce..6d0a41b 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> @@ -16,8 +16,8 @@ Required properties:
>  - #interrupt-cells: Specifies the number of cells needed to encode an
>    interrupt source. The value must be 1.
>  - 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.
> +- fsl,num-irqs: Number of input interrupts of this channel.
> +  Should be multiple of 32 input interrupts and up to 512 interrupts.
>  
>  Example:
>  
> @@ -28,7 +28,7 @@ Example:
> >  		clocks = <&clk IMX8MQ_CLK_DISP_APB_ROOT>;
> >  		clock-names = "ipg";
> >  		fsl,channel = <0>;
> > -		fsl,irq-groups = <1>;
> > +		fsl,num-irqs = <64>;
> >  		interrupt-controller;
> >  		#interrupt-cells = <1>;
> >  	};

Re: [PATCH V2 2/4] dt-bindings: irq: imx-irqsteer: add multi output interrupts support

[Lucas Stach]

Am Mittwoch, den 30.01.2019, 13:05 +0000 schrieb Aisheng Dong:
> One irqsteer channel can support up to 8 output interrupts.
> 
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: devicetree@vger.kernel.org
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> ChangeLog:
> v1->v2:
>  * remove one unnecessary note.
> ---
>  .../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt        | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> index 6d0a41b..984ab92 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> @@ -6,8 +6,9 @@ Required properties:
> >  	- "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.
> +- interrupts: Should contain the up to 8 parent interrupt lines used to
> +  multiplex the input interrupts. They should be sepcified seqencially

Typos here, should be: "specified sequentially"

> +  from output 0 to 7.
>  - clocks: Should contain one clock for entry in clock-names
>    see Documentation/devicetree/bindings/clock/clock-bindings.txt
>  - clock-names:

Re: [PATCH V2 3/4] irq: imx-irqsteer: change to use reg_num instead of irq_group

[Lucas Stach]

Am Mittwoch, den 30.01.2019, 13:06 +0000 schrieb Aisheng Dong:
> One group can manage 64 interrupts by using two registers (e.g. STATUS/SET).
> However, the integrated irqsteer may support only 32 interrupts which
> needs only one register in a group. But the current driver assume there's
> a mininum of two registers in a group which result in a wrong register map
> for 32 interrupts per channel irqsteer. Let's use the reg_num caculated by
> interrupts per channel instead of irq_group to cover this case.
> 
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

Minor nit below, otherwise:

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> v1->v2:
>  * The using of property name updated accordingly
> ---
>  drivers/irqchip/irq-imx-irqsteer.c | 35 +++++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 5b3f1d7..67ed862 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -13,7 +13,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/spinlock.h>
>  
> > -#define CTRL_STRIDE_OFF(_t, _r)	(_t * 8 * _r)
> > +#define CTRL_STRIDE_OFF(_t, _r)	(_t * 4 * _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)
> @@ -26,7 +26,7 @@ struct irqsteer_data {
> > >  	struct clk		*ipg_clk;
> > >  	int			irq;
> > >  	raw_spinlock_t		lock;
> > > -	int			irq_groups;
> > > +	int			reg_num;
> > >  	int			channel;
> > >  	struct irq_domain	*domain;
> > >  	u32			*saved_reg;
> @@ -35,7 +35,7 @@ struct irqsteer_data {
>  static int imx_irqsteer_get_reg_index(struct irqsteer_data *data,
> >  				      unsigned long irqnum)
>  {
> > -	return (data->irq_groups * 2 - irqnum / 32 - 1);
> > +	return (data->reg_num - irqnum / 32 - 1);
>  }
>  
>  static void imx_irqsteer_irq_unmask(struct irq_data *d)
> @@ -46,9 +46,9 @@ static void imx_irqsteer_irq_unmask(struct irq_data *d)
> >  	u32 val;
>  
> >  	raw_spin_lock_irqsave(&data->lock, flags);
> > -	val = readl_relaxed(data->regs + CHANMASK(idx, data->irq_groups));
> > +	val = readl_relaxed(data->regs + CHANMASK(idx, data->reg_num));
> >  	val |= BIT(d->hwirq % 32);
> > -	writel_relaxed(val, data->regs + CHANMASK(idx, data->irq_groups));
> > +	writel_relaxed(val, data->regs + CHANMASK(idx, data->reg_num));
> >  	raw_spin_unlock_irqrestore(&data->lock, flags);
>  }
>  
> @@ -60,9 +60,9 @@ static void imx_irqsteer_irq_mask(struct irq_data *d)
> >  	u32 val;
>  
> >  	raw_spin_lock_irqsave(&data->lock, flags);
> > -	val = readl_relaxed(data->regs + CHANMASK(idx, data->irq_groups));
> > +	val = readl_relaxed(data->regs + CHANMASK(idx, data->reg_num));
> >  	val &= ~BIT(d->hwirq % 32);
> > -	writel_relaxed(val, data->regs + CHANMASK(idx, data->irq_groups));
> > +	writel_relaxed(val, data->regs + CHANMASK(idx, data->reg_num));
> >  	raw_spin_unlock_irqrestore(&data->lock, flags);
>  }
>  
> @@ -94,13 +94,13 @@ static void imx_irqsteer_irq_handler(struct irq_desc *desc)
>  
> >  	chained_irq_enter(irq_desc_get_chip(desc), desc);
>  
> > -	for (i = 0; i < data->irq_groups * 64; i += 32) {
> > +	for (i = 0; i < data->reg_num * 32; 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));
> > +				       CHANSTATUS(idx, data->reg_num));
>  
> >  		for_each_set_bit(pos, &irqmap, 32) {
> >  			virq = irq_find_mapping(data->domain, pos + i);
> @@ -146,12 +146,15 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  
> >  	raw_spin_lock_init(&data->lock);
>  
> > -	of_property_read_u32(np, "fsl,irq-groups", &data->irq_groups);
> > +	of_property_read_u32(np, "fsl,num-irqs", &data->reg_num);
> >  	of_property_read_u32(np, "fsl,channel", &data->channel);
>  
> > +	/* one register bit map represents 32 input interrupts */
> > +	data->reg_num /= 32;
> +
> >  	if (IS_ENABLED(CONFIG_PM_SLEEP)) {
> >  		data->saved_reg = devm_kzalloc(&pdev->dev,
> > -					sizeof(u32) * data->irq_groups * 2,
> > +					sizeof(u32) * data->reg_num,
>  					GFP_KERNEL);

Does this last parameter now fit on the line above?

>  		if (!data->saved_reg)
> >  			return -ENOMEM;
> @@ -166,7 +169,7 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> >  	/* 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,
> > +	data->domain = irq_domain_add_linear(np, data->reg_num * 32,
> >  					     &imx_irqsteer_domain_ops, data);
> >  	if (!data->domain) {
> >  		dev_err(&pdev->dev, "failed to create IRQ domain\n");
> @@ -199,9 +202,9 @@ static void imx_irqsteer_save_regs(struct irqsteer_data *data)
>  {
> >  	int i;
>  
> > -	for (i = 0; i < data->irq_groups * 2; i++)
> > +	for (i = 0; i < data->reg_num; i++)
> >  		data->saved_reg[i] = readl_relaxed(data->regs +
> > -						CHANMASK(i, data->irq_groups));
> > +						CHANMASK(i, data->reg_num));
>  }
>  
>  static void imx_irqsteer_restore_regs(struct irqsteer_data *data)
> @@ -209,9 +212,9 @@ static void imx_irqsteer_restore_regs(struct irqsteer_data *data)
> >  	int i;
>  
> >  	writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
> > -	for (i = 0; i < data->irq_groups * 2; i++)
> > +	for (i = 0; i < data->reg_num; i++)
> >  		writel_relaxed(data->saved_reg[i],
> > -			       data->regs + CHANMASK(i, data->irq_groups));
> > +			       data->regs + CHANMASK(i, data->reg_num));
>  }
>  
>  static int imx_irqsteer_suspend(struct device *dev)

Re: [PATCH V2 4/4] irq: imx: irqsteer: add multi output interrupts support

[Lucas Stach]

Am Mittwoch, den 30.01.2019, 13:06 +0000 schrieb Aisheng Dong:
> One irqsteer channel can support up to 8 output interrupts.
> 
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> ChangeLog:
> v1->v2:
>  * calculate irq_count by fsl,num-irqs instead of parsing interrupts
>    property from devicetree to match the input interrupts and outputs
>  * improve output interrupt handler by searching only two registers
>    withint the same group
> ---
>  drivers/irqchip/irq-imx-irqsteer.c | 76 +++++++++++++++++++++++++++++---------
>  1 file changed, 59 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 67ed862..cc40039 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -10,6 +10,7 @@
>  #include <linux/irqchip/chained_irq.h>
>  #include <linux/irqdomain.h>
>  #include <linux/kernel.h>
> +#include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/spinlock.h>
>  
> @@ -21,10 +22,13 @@
> >  #define CHAN_MINTDIS(t)		(CTRL_STRIDE_OFF(t, 3) + 0x4)
> >  #define CHAN_MASTRSTAT(t)	(CTRL_STRIDE_OFF(t, 3) + 0x8)
>  
> > +#define CHAN_MAX_OUTPUT_INT	0x8
> +
>  struct irqsteer_data {
> > >  	void __iomem		*regs;
> > >  	struct clk		*ipg_clk;
> > > -	int			irq;
> > > +	int			irq[CHAN_MAX_OUTPUT_INT];
> > > +	int			irq_count;
> > >  	raw_spinlock_t		lock;
> > >  	int			reg_num;
> > >  	int			channel;
> @@ -87,26 +91,45 @@ static const struct irq_domain_ops imx_irqsteer_domain_ops = {
> > >  	.xlate		= irq_domain_xlate_onecell,
>  };
>  
> +static int imx_irqsteer_get_hwirq_base(struct irqsteer_data *data, u32 irq)
> +{
> > +	int i;
> +
> > +	for (i = 0; i < data->irq_count; i++) {
> > +		if (data->irq[i] == irq)
> +			break;

return i * 64; here...
> +	}
> +
> +	return i * 64;

... and -EINVAL or something here, so we don't return a out of bounds
hwirq base if the loop ever doesn't match something?
 
> +}
> +
>  static void imx_irqsteer_irq_handler(struct irq_desc *desc)
>  {
> >  	struct irqsteer_data *data = irq_desc_get_handler_data(desc);
> > +	int hwirq;
> >  	int i;
>  
> >  	chained_irq_enter(irq_desc_get_chip(desc), desc);
>  
> > -	for (i = 0; i < data->reg_num * 32; i += 32) {
> > -		int idx = imx_irqsteer_get_reg_index(data, i);
> > +	hwirq = imx_irqsteer_get_hwirq_base(data, irq_desc_get_irq(desc));
> +
> > +	for (i = 0; i < 2; i++) {
> > +		int idx = imx_irqsteer_get_reg_index(data, hwirq);
> >  		unsigned long irqmap;
> >  		int pos, virq;
>  
> > +		if (hwirq >= data->reg_num * 32)
> > +			break;
> +
> >  		irqmap = readl_relaxed(data->regs +
> >  				       CHANSTATUS(idx, data->reg_num));
>  
> >  		for_each_set_bit(pos, &irqmap, 32) {
> > -			virq = irq_find_mapping(data->domain, pos + i);
> +			virq = irq_find_mapping(data->domain, pos + hwirq);

The irq index calculation need to be "pos + i * 32 + hwirq", otherwise
this will map to the wrong virqs for the second register in each group.

>  			if (virq)
> >  				generic_handle_irq(virq);
> >  		}
> +		hwirq += 32;

Could be folded into the loop head.

>  	}
>  
> >  	chained_irq_exit(irq_desc_get_chip(desc), desc);
> @@ -117,7 +140,8 @@ 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;
> > +	u32 irqs_num;
> > +	int i, ret;
>  
> >  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> >  	if (!data)
> @@ -130,12 +154,6 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> >  		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);
> @@ -146,11 +164,17 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  
> >  	raw_spin_lock_init(&data->lock);
>  
> > -	of_property_read_u32(np, "fsl,num-irqs", &data->reg_num);
> > +	of_property_read_u32(np, "fsl,num-irqs", &irqs_num);
> >  	of_property_read_u32(np, "fsl,channel", &data->channel);
>  
> > -	/* one register bit map represents 32 input interrupts */
> > -	data->reg_num /= 32;
> > +	/*
> +	 * There is one output irqs for each group of 64 inputs.

"irq", singular.

> +	 * One register bit map can represent 32 input interrupts.
> > +	 */
> > +	data->irq_count = irqs_num / 64;
> > +	if (irqs_num % 64)
> +		data->irq_count += 1;

This is a weird way of writing DIV_ROUND_UP.

> +	data->reg_num = irqs_num / 32;
>  
> >  	if (IS_ENABLED(CONFIG_PM_SLEEP)) {
> >  		data->saved_reg = devm_kzalloc(&pdev->dev,
> @@ -177,8 +201,22 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  	}
>  
> > -	irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler,
> > -					 data);
> > +	if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT) {
> > +		clk_disable_unprepare(data->ipg_clk);
> > +		return -EINVAL;
> > +	}
> +
> > +	for (i = 0; i < data->irq_count; i++) {
> > +		data->irq[i] = irq_of_parse_and_map(np, i);
> > +		if (!data->irq[i]) {
> > +			clk_disable_unprepare(data->ipg_clk);
> +			return -EINVAL;

With a lot of failure paths now replicating the clk_disable_unprepare,
return error, I think this warrants a common cleanup path that all
those paths could reach via simple goto.

> +		}
> +
> > +		irq_set_chained_handler_and_data(data->irq[i],
> > +						 imx_irqsteer_irq_handler,
> > +						 data);
> > +	}
>  
> >  	platform_set_drvdata(pdev, data);
>  
> @@ -188,8 +226,12 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  static int imx_irqsteer_remove(struct platform_device *pdev)
>  {
> >  	struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev);
> > +	int i;
> +
> > +	for (i = 0; i < irqsteer_data->irq_count; i++)
> > +		irq_set_chained_handler_and_data(irqsteer_data->irq[i],
> > +						 NULL, NULL);
>  
> > -	irq_set_chained_handler_and_data(irqsteer_data->irq, NULL, NULL);
> >  	irq_domain_remove(irqsteer_data->domain);
>  
> >  	clk_disable_unprepare(irqsteer_data->ipg_clk);

Re: [PATCH V2 2/4] dt-bindings: irq: imx-irqsteer: add multi output interrupts support

[Dong Aisheng]

On Wed, Jan 30, 2019 at 9:21 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Mittwoch, den 30.01.2019, 13:05 +0000 schrieb Aisheng Dong:
> > One irqsteer channel can support up to 8 output interrupts.
> >
> > > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Lucas Stach <l.stach@pengutronix.de>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> > ChangeLog:
> > v1->v2:
> >  * remove one unnecessary note.
> > ---
> >  .../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt        | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > index 6d0a41b..984ab92 100644
> > --- a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > @@ -6,8 +6,9 @@ Required properties:
> > >     - "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.
> > +- interrupts: Should contain the up to 8 parent interrupt lines used to
> > +  multiplex the input interrupts. They should be sepcified seqencially
>
> Typos here, should be: "specified sequentially"
>

Nice catch. Will update in new version.
Thanks

Regards
Dong Aisheng

> > +  from output 0 to 7.
> >  - clocks: Should contain one clock for entry in clock-names
> >    see Documentation/devicetree/bindings/clock/clock-bindings.txt
> >  - clock-names:

Re: [PATCH V2 4/4] irq: imx: irqsteer: add multi output interrupts support

[Dong Aisheng]

On Wed, Jan 30, 2019 at 9:33 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Mittwoch, den 30.01.2019, 13:06 +0000 schrieb Aisheng Dong:
> > One irqsteer channel can support up to 8 output interrupts.
> >
> > > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > > Cc: Lucas Stach <l.stach@pengutronix.de>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> > ChangeLog:
> > v1->v2:
> >  * calculate irq_count by fsl,num-irqs instead of parsing interrupts
> >    property from devicetree to match the input interrupts and outputs
> >  * improve output interrupt handler by searching only two registers
> >    withint the same group
> > ---
> >  drivers/irqchip/irq-imx-irqsteer.c | 76 +++++++++++++++++++++++++++++---------
> >  1 file changed, 59 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> > index 67ed862..cc40039 100644
> > --- a/drivers/irqchip/irq-imx-irqsteer.c
> > +++ b/drivers/irqchip/irq-imx-irqsteer.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/irqchip/chained_irq.h>
> >  #include <linux/irqdomain.h>
> >  #include <linux/kernel.h>
> > +#include <linux/of_irq.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/spinlock.h>
> >
> > @@ -21,10 +22,13 @@
> > >  #define CHAN_MINTDIS(t)            (CTRL_STRIDE_OFF(t, 3) + 0x4)
> > >  #define CHAN_MASTRSTAT(t)  (CTRL_STRIDE_OFF(t, 3) + 0x8)
> >
> > > +#define CHAN_MAX_OUTPUT_INT        0x8
> > +
> >  struct irqsteer_data {
> > > >   void __iomem            *regs;
> > > >   struct clk              *ipg_clk;
> > > > - int                     irq;
> > > > + int                     irq[CHAN_MAX_OUTPUT_INT];
> > > > + int                     irq_count;
> > > >   raw_spinlock_t          lock;
> > > >   int                     reg_num;
> > > >   int                     channel;
> > @@ -87,26 +91,45 @@ static const struct irq_domain_ops imx_irqsteer_domain_ops = {
> > > >   .xlate          = irq_domain_xlate_onecell,
> >  };
> >
> > +static int imx_irqsteer_get_hwirq_base(struct irqsteer_data *data, u32 irq)
> > +{
> > > +   int i;
> > +
> > > +   for (i = 0; i < data->irq_count; i++) {
> > > +           if (data->irq[i] == irq)
> > +                     break;
>
> return i * 64; here...
> > +     }
> > +
> > +     return i * 64;
>
> ... and -EINVAL or something here, so we don't return a out of bounds
> hwirq base if the loop ever doesn't match something?
>

Good suggestion, will add it.

> > +}
> > +
> >  static void imx_irqsteer_irq_handler(struct irq_desc *desc)
> >  {
> > >     struct irqsteer_data *data = irq_desc_get_handler_data(desc);
> > > +   int hwirq;
> > >     int i;
> >
> > >     chained_irq_enter(irq_desc_get_chip(desc), desc);
> >
> > > -   for (i = 0; i < data->reg_num * 32; i += 32) {
> > > -           int idx = imx_irqsteer_get_reg_index(data, i);
> > > +   hwirq = imx_irqsteer_get_hwirq_base(data, irq_desc_get_irq(desc));
> > +
> > > +   for (i = 0; i < 2; i++) {
> > > +           int idx = imx_irqsteer_get_reg_index(data, hwirq);
> > >             unsigned long irqmap;
> > >             int pos, virq;
> >
> > > +           if (hwirq >= data->reg_num * 32)
> > > +                   break;
> > +
> > >             irqmap = readl_relaxed(data->regs +
> > >                                    CHANSTATUS(idx, data->reg_num));
> >
> > >             for_each_set_bit(pos, &irqmap, 32) {
> > > -                   virq = irq_find_mapping(data->domain, pos + i);
> > +                     virq = irq_find_mapping(data->domain, pos + hwirq);
>
> The irq index calculation need to be "pos + i * 32 + hwirq", otherwise
> this will map to the wrong virqs for the second register in each group.
>

For second register map, hwirq will plus 32 in next round.
So i can't see this will map a wrong virqs.
And it looks to me ""pos + i * 32 + hwirq" is equal to "hwirq + 32".
Am i missed something?

> >                       if (virq)
> > >                             generic_handle_irq(virq);
> > >             }
> > +             hwirq += 32;
>
> Could be folded into the loop head.
>

You mean “for (i = 0; i < 2; i++, hwirq +=32)” ?
I feel that's not quite necessary.

> >       }
> >
> > >     chained_irq_exit(irq_desc_get_chip(desc), desc);
> > @@ -117,7 +140,8 @@ 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;
> > > +   u32 irqs_num;
> > > +   int i, ret;
> >
> > >     data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > >     if (!data)
> > @@ -130,12 +154,6 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> > >             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);
> > @@ -146,11 +164,17 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> >
> > >     raw_spin_lock_init(&data->lock);
> >
> > > -   of_property_read_u32(np, "fsl,num-irqs", &data->reg_num);
> > > +   of_property_read_u32(np, "fsl,num-irqs", &irqs_num);
> > >     of_property_read_u32(np, "fsl,channel", &data->channel);
> >
> > > -   /* one register bit map represents 32 input interrupts */
> > > -   data->reg_num /= 32;
> > > +   /*
> > +      * There is one output irqs for each group of 64 inputs.
>
> "irq", singular.
>

Got it

> > +      * One register bit map can represent 32 input interrupts.
> > > +    */
> > > +   data->irq_count = irqs_num / 64;
> > > +   if (irqs_num % 64)
> > +             data->irq_count += 1;
>
> This is a weird way of writing DIV_ROUND_UP.
>

Good suggestion

> > +     data->reg_num = irqs_num / 32;
> >
> > >     if (IS_ENABLED(CONFIG_PM_SLEEP)) {
> > >             data->saved_reg = devm_kzalloc(&pdev->dev,
> > @@ -177,8 +201,22 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> > >             return -ENOMEM;
> > >     }
> >
> > > -   irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler,
> > > -                                    data);
> > > +   if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT) {
> > > +           clk_disable_unprepare(data->ipg_clk);
> > > +           return -EINVAL;
> > > +   }
> > +
> > > +   for (i = 0; i < data->irq_count; i++) {
> > > +           data->irq[i] = irq_of_parse_and_map(np, i);
> > > +           if (!data->irq[i]) {
> > > +                   clk_disable_unprepare(data->ipg_clk);
> > +                     return -EINVAL;
>
> With a lot of failure paths now replicating the clk_disable_unprepare,
> return error, I think this warrants a common cleanup path that all
> those paths could reach via simple goto.
>

Sound goods to me

Regards
Dong Aisheng

> > +             }
> > +
> > > +           irq_set_chained_handler_and_data(data->irq[i],
> > > +                                            imx_irqsteer_irq_handler,
> > > +                                            data);
> > > +   }
> >
> > >     platform_set_drvdata(pdev, data);
> >
> > @@ -188,8 +226,12 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> >  static int imx_irqsteer_remove(struct platform_device *pdev)
> >  {
> > >     struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev);
> > > +   int i;
> > +
> > > +   for (i = 0; i < irqsteer_data->irq_count; i++)
> > > +           irq_set_chained_handler_and_data(irqsteer_data->irq[i],
> > > +                                            NULL, NULL);
> >
> > > -   irq_set_chained_handler_and_data(irqsteer_data->irq, NULL, NULL);
> > >     irq_domain_remove(irqsteer_data->domain);
> >
> > >     clk_disable_unprepare(irqsteer_data->ipg_clk);

Re: [PATCH V2 4/4] irq: imx: irqsteer: add multi output interrupts support

[Lucas Stach]

Am Mittwoch, den 30.01.2019, 22:03 +0800 schrieb Dong Aisheng:
> > On Wed, Jan 30, 2019 at 9:33 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > Am Mittwoch, den 30.01.2019, 13:06 +0000 schrieb Aisheng Dong:
> > > One irqsteer channel can support up to 8 output interrupts.
> > > 
> > > > > > > > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > > > > > > > Cc: Lucas Stach <l.stach@pengutronix.de>
> > > > > > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > 
> > > ---
> > > ChangeLog:
> > > v1->v2:
> > >  * calculate irq_count by fsl,num-irqs instead of parsing interrupts
> > >    property from devicetree to match the input interrupts and outputs
> > >  * improve output interrupt handler by searching only two registers
> > >    withint the same group
> > > ---
> > >  drivers/irqchip/irq-imx-irqsteer.c | 76 +++++++++++++++++++++++++++++---------
> > >  1 file changed, 59 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> > > index 67ed862..cc40039 100644
> > > --- a/drivers/irqchip/irq-imx-irqsteer.c
> > > +++ b/drivers/irqchip/irq-imx-irqsteer.c
> > > @@ -10,6 +10,7 @@
> > >  #include <linux/irqchip/chained_irq.h>
> > >  #include <linux/irqdomain.h>
> > >  #include <linux/kernel.h>
> > > +#include <linux/of_irq.h>
> > >  #include <linux/of_platform.h>
> > >  #include <linux/spinlock.h>
> > > 
> > > @@ -21,10 +22,13 @@
> > > >  #define CHAN_MINTDIS(t)            (CTRL_STRIDE_OFF(t, 3) + 0x4)
> > > >  #define CHAN_MASTRSTAT(t)  (CTRL_STRIDE_OFF(t, 3) + 0x8)
> > > > +#define CHAN_MAX_OUTPUT_INT        0x8
> > > 
> > > +
> > >  struct irqsteer_data {
> > > > >   void __iomem            *regs;
> > > > >   struct clk              *ipg_clk;
> > > > > - int                     irq;
> > > > > + int                     irq[CHAN_MAX_OUTPUT_INT];
> > > > > + int                     irq_count;
> > > > >   raw_spinlock_t          lock;
> > > > >   int                     reg_num;
> > > > >   int                     channel;
> > > 
> > > @@ -87,26 +91,45 @@ static const struct irq_domain_ops imx_irqsteer_domain_ops = {
> > > > >   .xlate          = irq_domain_xlate_onecell,
> > > 
> > >  };
> > > 
> > > +static int imx_irqsteer_get_hwirq_base(struct irqsteer_data *data, u32 irq)
> > > +{
> > > > +   int i;
> > > 
> > > +
> > > > +   for (i = 0; i < data->irq_count; i++) {
> > > > +           if (data->irq[i] == irq)
> > > 
> > > +                     break;
> > 
> > return i * 64; here...
> > > +     }
> > > +
> > > +     return i * 64;
> > 
> > ... and -EINVAL or something here, so we don't return a out of bounds
> > hwirq base if the loop ever doesn't match something?
> > 
> 
> Good suggestion, will add it.
> 
> > > +}
> > > +
> > >  static void imx_irqsteer_irq_handler(struct irq_desc *desc)
> > >  {
> > > >     struct irqsteer_data *data = irq_desc_get_handler_data(desc);
> > > > +   int hwirq;
> > > >     int i;
> > > >     chained_irq_enter(irq_desc_get_chip(desc), desc);
> > > > -   for (i = 0; i < data->reg_num * 32; i += 32) {
> > > > -           int idx = imx_irqsteer_get_reg_index(data, i);
> > > > +   hwirq = imx_irqsteer_get_hwirq_base(data, irq_desc_get_irq(desc));
> > > 
> > > +
> > > > +   for (i = 0; i < 2; i++) {
> > > > +           int idx = imx_irqsteer_get_reg_index(data, hwirq);
> > > >             unsigned long irqmap;
> > > >             int pos, virq;
> > > > +           if (hwirq >= data->reg_num * 32)
> > > > +                   break;
> > > 
> > > +
> > > >             irqmap = readl_relaxed(data->regs +
> > > >                                    CHANSTATUS(idx, data->reg_num));
> > > >             for_each_set_bit(pos, &irqmap, 32) {
> > > > -                   virq = irq_find_mapping(data->domain, pos + i);
> > > 
> > > +                     virq = irq_find_mapping(data->domain, pos + hwirq);
> > 
> > The irq index calculation need to be "pos + i * 32 + hwirq", otherwise
> > this will map to the wrong virqs for the second register in each group.
> > 
> 
> For second register map, hwirq will plus 32 in next round.
> So i can't see this will map a wrong virqs.
> And it looks to me ""pos + i * 32 + hwirq" is equal to "hwirq + 32".
> Am i missed something?

You are right, I forgot about the hwirq being incremented in the loop
when writing this comment.

> > >                       if (virq)
> > > >                             generic_handle_irq(virq);
> > > >             }
> > > 
> > > +             hwirq += 32;
> > 
> > Could be folded into the loop head.
> > 
> 
> You mean “for (i = 0; i < 2; i++, hwirq +=32)” ?
> I feel that's not quite necessary.

I personally find that quite a bit clearer than incrementing the loop
variables at different spots. And I probably wouldn't have missed hwirq
being incremented in the loop if I had seen it in the head.

Regards,
Lucas

Re: [PATCH V2 3/4] irq: imx-irqsteer: change to use reg_num instead of irq_group

[Dong Aisheng]

On Wed, Jan 30, 2019 at 9:23 PM Lucas Stach <l.stach@pengutronix.de> wrote:
[...]
> > > +   /* one register bit map represents 32 input interrupts */
> > > +   data->reg_num /= 32;
> > +
> > >     if (IS_ENABLED(CONFIG_PM_SLEEP)) {
> > >             data->saved_reg = devm_kzalloc(&pdev->dev,
> > > -                                   sizeof(u32) * data->irq_groups * 2,
> > > +                                   sizeof(u32) * data->reg_num,
> >                                       GFP_KERNEL);
>
> Does this last parameter now fit on the line above?
>

No, 81 now. :)

Regards
Dong Aisheng