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

[PATCH 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 per channel 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                 | 74 ++++++++++++++--------
 2 files changed, 53 insertions(+), 32 deletions(-)

-- 
2.7.4

[PATCH 1/4] dt-binding: irq: imx-irqsteer: use irq number per channel 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,irqs-per-chan 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>
---
 .../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..eaabcda 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,irqs-per-chan: Number of input interrupts per 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,irqs-per-chan= <64>;
 		interrupt-controller;
 		#interrupt-cells = <1>;
 	};
-- 
2.7.4

[PATCH 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>
---
 .../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 eaabcda..beeed4a 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. NOTE at least one output interrupt has to be specified.
 - clocks: Should contain one clock for entry in clock-names
   see Documentation/devicetree/bindings/clock/clock-bindings.txt
 - clock-names:
-- 
2.7.4

[PATCH 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>
---
 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..1bebf0a 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,chan-irq-number", &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 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>
---
 drivers/irqchip/irq-imx-irqsteer.c | 39 +++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
index 1bebf0a..54802fa 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;
@@ -117,7 +121,7 @@ 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;
+	int i, ret;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -130,12 +134,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);
@@ -177,8 +175,23 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler,
-					 data);
+	data->irq_count = of_irq_count(np);
+	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 +201,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 1/4] dt-binding: irq: imx-irqsteer: use irq number per channel instead of group number

[Lucas Stach]

Am Freitag, den 18.01.2019, 07:53 +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,irqs-per-chan to represents how many interrupts supported
> by this irqsteer channel.

Sorry, but total NACK. I've got to great lengths with dumping the
actually implemented register layout on i.MX8M and AFAICS the IRQs are
always managed in groups of 64 IRQs, even if less than that are
connected as input IRQs. This is what the actually present register set
on i.MX8M tells us.

Regards,
Lucas

> 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>
> ---
>  .../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..eaabcda 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,irqs-per-chan: Number of input interrupts per 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,irqs-per-chan= <64>;
> >  		interrupt-controller;
> >  		#interrupt-cells = <1>;
> >  	};

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

[Lucas Stach]

Am Freitag, den 18.01.2019, 07:53 +0000 schrieb Aisheng Dong:
> One irqsteer channel can support up to 8 output interrupts.

This has been discussed when upstreaming the driver. The controller may
support multiple output IRQs, but only one them is actually used
depending on the CHANCTRL config. There is no use in hooking up all the
output IRQs in DT, if only one of them is actually used. Some of the
outputs may not even be visible to the Linux system, but may belong to
a Cortex M4 subsystem. All of those configurations can be described in
DT by changing the upstream interrupt and "fsl,channel" in a coherent
way.

Please correct me if my understanding is totally wrong.

Regards,
Lucas

> 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>
> ---
>  drivers/irqchip/irq-imx-irqsteer.c | 39 +++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 1bebf0a..54802fa 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;
> @@ -117,7 +121,7 @@ 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;
> > +	int i, ret;
>  
> >  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> >  	if (!data)
> @@ -130,12 +134,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);
> @@ -177,8 +175,23 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  	}
>  
> > -	irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler,
> > -					 data);
> > +	data->irq_count = of_irq_count(np);
> > +	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 +201,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 1/4] dt-binding: irq: imx-irqsteer: use irq number per channel instead of group number

[Marc Zyngier]

On 18/01/2019 08:48, Lucas Stach wrote:
> Am Freitag, den 18.01.2019, 07:53 +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,irqs-per-chan to represents how many interrupts supported
>> by this irqsteer channel.
> 
> Sorry, but total NACK. I've got to great lengths with dumping the
> actually implemented register layout on i.MX8M and AFAICS the IRQs are
> always managed in groups of 64 IRQs, even if less than that are
> connected as input IRQs. This is what the actually present register set
> on i.MX8M tells us.

Also, I'd really like the DT bindings not to change at every release. So
whatever change (if any) has to be done for this driver to support
existing HW, please make sure that the DT bindings are kept as stable as
possible.

Thanks,

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

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

[Aisheng Dong]

> -----Original Message-----
> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Friday, January 18, 2019 4:48 PM
> 
> Am Freitag, den 18.01.2019, 07:53 +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,irqs-per-chan to represents how many interrupts
> > supported by this irqsteer channel.
> 
> Sorry, but total NACK. I've got to great lengths with dumping the actually
> implemented register layout on i.MX8M and AFAICS the IRQs are always
> managed in groups of 64 IRQs, even if less than that are connected as input
> IRQs. This is what the actually present register set on i.MX8M tells us.
> 

The register layout varies depends on the irq per channel supports.
It's true 64 IRQs on MX8M, but not true for QXP and QM.
That's why we shouldn't use fsl,irq-groups.

Regards
Dong Aisheng

> Regards,
> Lucas
> 
> > 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>
> > ---
> >  .../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..eaabcda 100644
> > ---
> > a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.
> > txt
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqst
> > +++ eer.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,irqs-per-chan: Number of input interrupts per 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,irqs-per-chan= <64>;
> > >  		interrupt-controller;
> > >  		#interrupt-cells = <1>;
> > >  	};

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

[Aisheng Dong]

> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Friday, January 18, 2019 5:39 PM
> On 18/01/2019 08:48, Lucas Stach wrote:
> > Am Freitag, den 18.01.2019, 07:53 +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,irqs-per-chan to represents how many interrupts
> >> supported by this irqsteer channel.
> >
> > Sorry, but total NACK. I've got to great lengths with dumping the
> > actually implemented register layout on i.MX8M and AFAICS the IRQs are
> > always managed in groups of 64 IRQs, even if less than that are
> > connected as input IRQs. This is what the actually present register
> > set on i.MX8M tells us.
> 
> Also, I'd really like the DT bindings not to change at every release. So whatever
> change (if any) has to be done for this driver to support existing HW, please
> make sure that the DT bindings are kept as stable as possible.
> 

Sorry I should clarify it a bit.
There's still no users in Devicetree.
So I guess we can update it, right? Or not?

Regards
Dong Aisheng

> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...

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

[Aisheng Dong]

> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Friday, January 18, 2019 4:53 PM
> Am Freitag, den 18.01.2019, 07:53 +0000 schrieb Aisheng Dong:
> > One irqsteer channel can support up to 8 output interrupts.
> 
> This has been discussed when upstreaming the driver. The controller may
> support multiple output IRQs, but only one them is actually used depending on
> the CHANCTRL config. There is no use in hooking up all the output IRQs in DT, if
> only one of them is actually used. Some of the outputs may not even be visible
> to the Linux system, but may belong to a Cortex M4 subsystem. All of those
> configurations can be described in DT by changing the upstream interrupt and
> "fsl,channel" in a coherent way.
> 
> Please correct me if my understanding is totally wrong.

I'm afraid your understanding of CHAN seems wrong.
(Binding doc of that property needs change as well).

On QXP DC SS, the IRQSTEER supports 512 interrupts with 8 interrupt output
Conntected to GIC.
The current driver does not support it as it assumes only one interrupt output used.

Regards
Dong Aisheng

> 
> Regards,
> Lucas
> 
> > 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>
> > ---
> >  drivers/irqchip/irq-imx-irqsteer.c | 39
> > +++++++++++++++++++++++++++-----------
> >  1 file changed, 28 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-imx-irqsteer.c
> > b/drivers/irqchip/irq-imx-irqsteer.c
> > index 1bebf0a..54802fa 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;
> > @@ -117,7 +121,7 @@ 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;
> > > +	int i, ret;
> >
> > >  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > >  	if (!data)
> > @@ -130,12 +134,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);
> > @@ -177,8 +175,23 @@ static int imx_irqsteer_probe(struct
> > platform_device *pdev)
> > >  		return -ENOMEM;
> > >  	}
> >
> > > -	irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler,
> > > -					 data);
> > > +	data->irq_count = of_irq_count(np);
> > > +	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 +201,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 1/4] dt-binding: irq: imx-irqsteer: use irq number per channel instead of group number

[Marc Zyngier]

On 18/01/2019 09:46, Aisheng Dong wrote:
>> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
>> Sent: Friday, January 18, 2019 5:39 PM
>> On 18/01/2019 08:48, Lucas Stach wrote:
>>> Am Freitag, den 18.01.2019, 07:53 +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,irqs-per-chan to represents how many interrupts
>>>> supported by this irqsteer channel.
>>>
>>> Sorry, but total NACK. I've got to great lengths with dumping the
>>> actually implemented register layout on i.MX8M and AFAICS the IRQs are
>>> always managed in groups of 64 IRQs, even if less than that are
>>> connected as input IRQs. This is what the actually present register
>>> set on i.MX8M tells us.
>>
>> Also, I'd really like the DT bindings not to change at every release. So whatever
>> change (if any) has to be done for this driver to support existing HW, please
>> make sure that the DT bindings are kept as stable as possible.
>>
> 
> Sorry I should clarify it a bit.
> There's still no users in Devicetree.
> So I guess we can update it, right? Or not?

What do you mean by no users? This driver is in 5.0, and I assume people
are using it one way or another. Not having a platform in the kernel
tree is pretty much irrelevant, as the kernel tree is not a canonical
repository of existing platforms.

Thanks,

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

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

[Lucas Stach]

Am Freitag, den 18.01.2019, 09:54 +0000 schrieb Aisheng Dong:
> > > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > Sent: Friday, January 18, 2019 4:53 PM
> > Am Freitag, den 18.01.2019, 07:53 +0000 schrieb Aisheng Dong:
> > > One irqsteer channel can support up to 8 output interrupts.
> > 
> > This has been discussed when upstreaming the driver. The controller may
> > support multiple output IRQs, but only one them is actually used depending on
> > the CHANCTRL config. There is no use in hooking up all the output IRQs in DT, if
> > only one of them is actually used. Some of the outputs may not even be visible
> > to the Linux system, but may belong to a Cortex M4 subsystem. All of those
> > configurations can be described in DT by changing the upstream interrupt and
> > "fsl,channel" in a coherent way.
> > 
> > Please correct me if my understanding is totally wrong.
> 
> I'm afraid your understanding of CHAN seems wrong.
> (Binding doc of that property needs change as well).
> 
> On QXP DC SS, the IRQSTEER supports 512 interrupts with 8 interrupt output
> Conntected to GIC.
> The current driver does not support it as it assumes only one interrupt output used.

Okay, so let's take a step back. The description in the QXP RM is
actually better than what I've seen until now. Still it's totally
confusing that the "channel" terminology used with different meanings
in docs. Let's try to avoid this as much as possible.

So to get things straight: Each irqsteer controller has a number of IRQ
groups. All the input IRQs of one group are ORed together to form on
output IRQ. Depending on the SoC integration, a group can contain 32 or
64 IRQs, where DCSS irqsteer on MX8M and the big 512 input controllers
on QXP and QM both use 64 IRQs per group. You are claiming that the
smaller controllers on both QXP am QM have only 32 IRQs per group,
right?

So the only change that is needed is that the driver needs to know the
number of input IRQs per group, with a default of 64 to not break DT
compatibility.

Also if the connection between IRQ group and output IRQ is fixed, the
driver should be more clever about handling the chained IRQ. If you
know which of the upstream IRQs fired you only need to look at the 32
or 64 IRQ status registers of that specific group, not all of them.

Can you please clarify what the CHANCTRL setting changes in this setup?

Regards,
Lucas

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

[Aisheng Dong]

> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Friday, January 18, 2019 6:23 PM
[...]
> > > This has been discussed when upstreaming the driver. The controller
> > > may support multiple output IRQs, but only one them is actually used
> > > depending on the CHANCTRL config. There is no use in hooking up all
> > > the output IRQs in DT, if only one of them is actually used. Some of
> > > the outputs may not even be visible to the Linux system, but may
> > > belong to a Cortex M4 subsystem. All of those configurations can be
> > > described in DT by changing the upstream interrupt and "fsl,channel" in a
> coherent way.
> > >
> > > Please correct me if my understanding is totally wrong.
> >
> > I'm afraid your understanding of CHAN seems wrong.
> > (Binding doc of that property needs change as well).
> >
> > On QXP DC SS, the IRQSTEER supports 512 interrupts with 8 interrupt
> > output Conntected to GIC.
> > The current driver does not support it as it assumes only one interrupt
> output used.
> 
> Okay, so let's take a step back. The description in the QXP RM is actually better
> than what I've seen until now. Still it's totally confusing that the "channel"
> terminology used with different meanings in docs. Let's try to avoid this as
> much as possible.
> 
> So to get things straight: Each irqsteer controller has a number of IRQ groups.
> All the input IRQs of one group are ORed together to form on output IRQ.
> Depending on the SoC integration, a group can contain 32 or
> 64 IRQs, where DCSS irqsteer on MX8M and the big 512 input controllers on
> QXP and QM both use 64 IRQs per group. You are claiming that the smaller
> controllers on both QXP am QM have only 32 IRQs per group, right?
> 
> So the only change that is needed is that the driver needs to know the number
> of input IRQs per group, with a default of 64 to not break DT compatibility.
> 

Not exactly.
from HW point of view , there're two parameters during IRQSTEER integration.
For example,
DC in QXP:
parameter  IRQCHAN		=  1; 	//Number of IRQ Channels/Slots
parameter  NINT32		=  8;	//Number of interrupts in multiple of 32

MIPI CSI in MQ:
Parameter  IRQCHAN		= 1
Parameter  NINT32		= 1

You will see no group concept used here. Only channel number and interrupts number.
The group is an IP internal concept that ORed a group of 64 interrupts into an output
interrupt. But it may also only use 32 interrupts in the same group.

> Also if the connection between IRQ group and output IRQ is fixed, the driver
> should be more clever about handling the chained IRQ. If you know which of
> the upstream IRQs fired you only need to look at the 32 or 64 IRQ status
> registers of that specific group, not all of them.

Yes, that's right.
I planned to do that later with a separate patch before.

> 
> Can you please clarify what the CHANCTRL setting changes in this setup?
> 

IRQsteer supports up to 5 separate CAHNNELS which each of them supports up
to 512 interrupts. CHANCTL is used to enable those respective CHAN output interrupts.
e.g.
1~8 output interrupts of CHAN0.

One notable thing is the each channel has a separate address space.
That means the chan1 reg address is not the one we specified in default reg property.
So the correct dts may be like for multi channels cases.
interrupt-controller@32e2d000 {
        compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";
        reg = <0x32e2d000 0x1000>,
              <0x32e2e000 0x1000>,
              <0x32e2f000 0x1000>;
              ...
        reg-names = "ch0", "ch1", "ch2", ...;
        interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
        fsl,irqs-per-chan= <64>;
        interrupt-controller;
        #interrupt-cells = <2>; //cell 0: chan index cell 2: interrupt number
};
This makes the things quite complicated.

In reality, we still don't have such using cases so far as as multi channels usually
are used to deliver the interrupts to different cores,
e.g. M4, SCU, or DSP, A core don't handle it.
So I did not change it currently as it's another story.
This patch series mainly aims to add support for 32 or 512 interrupts channel and multiple
Outputs for a single CHANNEL case.

Regards
Dong Aisheng

> Regards,
> Lucas

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

[Aisheng Dong]

> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Friday, January 18, 2019 6:12 PM
[...]
> >> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> >> Sent: Friday, January 18, 2019 5:39 PM On 18/01/2019 08:48, Lucas
> >> Stach wrote:
> >>> Am Freitag, den 18.01.2019, 07:53 +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,irqs-per-chan to represents how many
> >>>> interrupts supported by this irqsteer channel.
> >>>
> >>> Sorry, but total NACK. I've got to great lengths with dumping the
> >>> actually implemented register layout on i.MX8M and AFAICS the IRQs
> >>> are always managed in groups of 64 IRQs, even if less than that are
> >>> connected as input IRQs. This is what the actually present register
> >>> set on i.MX8M tells us.
> >>
> >> Also, I'd really like the DT bindings not to change at every release.
> >> So whatever change (if any) has to be done for this driver to support
> >> existing HW, please make sure that the DT bindings are kept as stable as
> possible.
> >>
> >
> > Sorry I should clarify it a bit.
> > There's still no users in Devicetree.
> > So I guess we can update it, right? Or not?
> 
> What do you mean by no users? This driver is in 5.0, and I assume people are
> using it one way or another. Not having a platform in the kernel tree is pretty
> much irrelevant, as the kernel tree is not a canonical repository of existing
> platforms.
> 

I understand the concern.
Theoretically yes, but it's very unlikely that there's already an out of tree users
wants to use it for a long term as we're still at the very initial stage.

And the most important reason is that current using actually is wrong.
We can also choose to mark it as 'depreciated' and keep the backward compatibility in driver,
but I'm not sure whether it's worthy to do it as we may add a lot ugly code in driver
benefits no users.

Ideas?

Regards
Dong Aisheng

> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...

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

[Lucas Stach]

Am Dienstag, den 22.01.2019, 10:39 +0000 schrieb Aisheng Dong:
> > > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > Sent: Friday, January 18, 2019 6:23 PM
> 
> [...]
> > > > This has been discussed when upstreaming the driver. The controller
> > > > may support multiple output IRQs, but only one them is actually used
> > > > depending on the CHANCTRL config. There is no use in hooking up all
> > > > the output IRQs in DT, if only one of them is actually used. Some of
> > > > the outputs may not even be visible to the Linux system, but may
> > > > belong to a Cortex M4 subsystem. All of those configurations can be
> > > > described in DT by changing the upstream interrupt and "fsl,channel" in a
> > 
> > coherent way.
> > > > 
> > > > Please correct me if my understanding is totally wrong.
> > > 
> > > I'm afraid your understanding of CHAN seems wrong.
> > > (Binding doc of that property needs change as well).
> > > 
> > > On QXP DC SS, the IRQSTEER supports 512 interrupts with 8 interrupt
> > > output Conntected to GIC.
> > > The current driver does not support it as it assumes only one interrupt
> > 
> > output used.
> > 
> > Okay, so let's take a step back. The description in the QXP RM is actually better
> > than what I've seen until now. Still it's totally confusing that the "channel"
> > terminology used with different meanings in docs. Let's try to avoid this as
> > much as possible.
> > 
> > So to get things straight: Each irqsteer controller has a number of IRQ groups.
> > All the input IRQs of one group are ORed together to form on output IRQ.
> > Depending on the SoC integration, a group can contain 32 or
> > 64 IRQs, where DCSS irqsteer on MX8M and the big 512 input controllers on
> > QXP and QM both use 64 IRQs per group. You are claiming that the smaller
> > controllers on both QXP am QM have only 32 IRQs per group, right?
> > 
> > So the only change that is needed is that the driver needs to know the number
> > of input IRQs per group, with a default of 64 to not break DT compatibility.
> > 
> 
> Not exactly.
> from HW point of view , there're two parameters during IRQSTEER integration.
> For example,
> DC in QXP:
> > > parameter  IRQCHAN		=  1; 	//Number of IRQ Channels/Slots
> > > parameter  NINT32		=  8;	//Number of interrupts in multiple of 32

If this is always in multiples of 32, the only change we need to make
to the driver is to fix DT binding and interpretation of the
"fsl,irq-groups" property to be in multiples of 32.

This means i.MX8MQ DCSS irqsteer would need to change to 2 irq-groups,
but as this isn't used upstream yet we can still do this change without
breaking too much stuff and I would rather correct this now than
keeping a DT binding around that doesn't match the HW.

> MIPI CSI in MQ:
> > Parameter  IRQCHAN		= 1
> > Parameter  NINT32		= 1
> 
> You will see no group concept used here. Only channel number and interrupts number.
> The group is an IP internal concept that ORed a group of 64 interrupts into an output
> interrupt. But it may also only use 32 interrupts in the same group.

I suppose that the OR group size at that point is always 64 input IRQs
per output IRQ, right? So with NINT32 == 1 you end up with 1 output
IRQ, but for NINT32 == 3 you get 2 output IRQs, correct?

> > Also if the connection between IRQ group and output IRQ is fixed, the driver
> > should be more clever about handling the chained IRQ. If you know which of
> > the upstream IRQs fired you only need to look at the 32 or 64 IRQ status
> > registers of that specific group, not all of them.
> 
> Yes, that's right.
> I planned to do that later with a separate patch before.

Let's do it right with the first patch. This doesn't seem like a big
change.

> 
> > 
> > Can you please clarify what the CHANCTRL setting changes in this setup?
> > 
> 
> IRQsteer supports up to 5 separate CAHNNELS which each of them supports up
> to 512 interrupts. CHANCTL is used to enable those respective CHAN output interrupts.
> e.g.
> 1~8 output interrupts of CHAN0.
> 
> One notable thing is the each channel has a separate address space.
> That means the chan1 reg address is not the one we specified in default reg property.
> So the correct dts may be like for multi channels cases.
> interrupt-controller@32e2d000 {
>         compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";
>         reg = <0x32e2d000 0x1000>,
>               <0x32e2e000 0x1000>,
>               <0x32e2f000 0x1000>;
>               ...
>         reg-names = "ch0", "ch1", "ch2", ...;
>         interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
>         fsl,irqs-per-chan= <64>;
>         interrupt-controller;
>         #interrupt-cells = <2>; //cell 0: chan index cell 2: interrupt number
> };
> This makes the things quite complicated.

With the current binding, what keeps us from describing such a multi-
channel irqsteer with multiple DT nodes and have multiple driver
instances? I don't see why we would need to mix this all into one
driver instance. So for your above example, something like:

interrupt-controller@32e2d000 {
	compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";>
	reg = <0x32e2d000 0x1000>;
	interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
	fsl,channel = <0>;
};

interrupt-controller@32e2e000 {
	compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";>
	reg = <0x32e2e000 0x1000>;
	interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
	fsl,channel = <1>;
};

> In reality, we still don't have such using cases so far as as multi channels usually
> are used to deliver the interrupts to different cores,
> e.g. M4, SCU, or DSP, A core don't handle it.
> So I did not change it currently as it's another story.
> This patch series mainly aims to add support for 32 or 512 interrupts channel and multiple
> Outputs for a single CHANNEL case.

The thing is, if we want to even try to keep DT stability we need to
understand how this HW block can be used and how we can describe this
in the DT.

Regards,
Lucas

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

[Lucas Stach]

Am Dienstag, den 22.01.2019, 10:56 +0000 schrieb Aisheng Dong:
> > > > From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> > Sent: Friday, January 18, 2019 6:12 PM
> 
> [...]
> > > > > > > > From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> > > > Sent: Friday, January 18, 2019 5:39 PM On 18/01/2019 08:48, Lucas
> > > > Stach wrote:
> > > > > Am Freitag, den 18.01.2019, 07:53 +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,irqs-per-chan to represents how many
> > > > > > interrupts supported by this irqsteer channel.
> > > > > 
> > > > > Sorry, but total NACK. I've got to great lengths with dumping the
> > > > > actually implemented register layout on i.MX8M and AFAICS the IRQs
> > > > > are always managed in groups of 64 IRQs, even if less than that are
> > > > > connected as input IRQs. This is what the actually present register
> > > > > set on i.MX8M tells us.
> > > > 
> > > > Also, I'd really like the DT bindings not to change at every release.
> > > > So whatever change (if any) has to be done for this driver to support
> > > > existing HW, please make sure that the DT bindings are kept as stable as
> > 
> > possible.
> > > > 
> > > 
> > > Sorry I should clarify it a bit.
> > > There's still no users in Devicetree.
> > > So I guess we can update it, right? Or not?
> > 
> > What do you mean by no users? This driver is in 5.0, and I assume people are
> > using it one way or another. Not having a platform in the kernel tree is pretty
> > much irrelevant, as the kernel tree is not a canonical repository of existing
> > platforms.
> > 
> 
> I understand the concern.
> Theoretically yes, but it's very unlikely that there's already an out of tree users
> wants to use it for a long term as we're still at the very initial stage.
> 
> And the most important reason is that current using actually is wrong.
> We can also choose to mark it as 'depreciated' and keep the backward compatibility in driver,
> but I'm not sure whether it's worthy to do it as we may add a lot ugly code in driver
> benefits no users.
> 
> Ideas?

I'm all for doing a breaking DT change now. The binding is
significantly different from the downstream one anyways and I'm not
aware of any upstream users that wouldn't be able to cope with a change
at this point.

I want to reach a conclusion on the discussion about how the HW
actually works and is configured in reply to Patch 4/4 first.

Regards,
Lucas

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

[Marc Zyngier]

On Tue, 22 Jan 2019 10:56:42 +0000,
Aisheng Dong <aisheng.dong@nxp.com> wrote:
> 
> > From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> > Sent: Friday, January 18, 2019 6:12 PM
> [...]
> > >> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> > >> Sent: Friday, January 18, 2019 5:39 PM On 18/01/2019 08:48, Lucas
> > >> Stach wrote:
> > >>> Am Freitag, den 18.01.2019, 07:53 +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,irqs-per-chan to represents how many
> > >>>> interrupts supported by this irqsteer channel.
> > >>>
> > >>> Sorry, but total NACK. I've got to great lengths with dumping the
> > >>> actually implemented register layout on i.MX8M and AFAICS the IRQs
> > >>> are always managed in groups of 64 IRQs, even if less than that are
> > >>> connected as input IRQs. This is what the actually present register
> > >>> set on i.MX8M tells us.
> > >>
> > >> Also, I'd really like the DT bindings not to change at every release.
> > >> So whatever change (if any) has to be done for this driver to support
> > >> existing HW, please make sure that the DT bindings are kept as stable as
> > possible.
> > >>
> > >
> > > Sorry I should clarify it a bit.
> > > There's still no users in Devicetree.
> > > So I guess we can update it, right? Or not?
> > 
> > What do you mean by no users? This driver is in 5.0, and I assume people are
> > using it one way or another. Not having a platform in the kernel tree is pretty
> > much irrelevant, as the kernel tree is not a canonical repository of existing
> > platforms.
> > 
> 
> I understand the concern.
> Theoretically yes, but it's very unlikely that there's already an out of tree users
> wants to use it for a long term as we're still at the very initial stage.
> 
> And the most important reason is that current using actually is wrong.
> We can also choose to mark it as 'depreciated' and keep the backward compatibility in driver,
> but I'm not sure whether it's worthy to do it as we may add a lot ugly code in driver
> benefits no users.
> 
> Ideas?

At this stage, and given that there is no consensus on how this driver
should work, I'm tempted to just rip it out entirely by reverting
0136afa08967 and be done with it until people work out a way forward.

	M.

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

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

[Aisheng Dong]

> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Tuesday, January 22, 2019 6:59 PM
> 
> Am Dienstag, den 22.01.2019, 10:39 +0000 schrieb Aisheng Dong:
> > > > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > > Sent: Friday, January 18, 2019 6:23 PM
> >
> > [...]
> > > > > This has been discussed when upstreaming the driver. The
> > > > > controller may support multiple output IRQs, but only one them
> > > > > is actually used depending on the CHANCTRL config. There is no
> > > > > use in hooking up all the output IRQs in DT, if only one of them
> > > > > is actually used. Some of the outputs may not even be visible to
> > > > > the Linux system, but may belong to a Cortex M4 subsystem. All
> > > > > of those configurations can be described in DT by changing the
> > > > > upstream interrupt and "fsl,channel" in a
> > >
> > > coherent way.
> > > > >
> > > > > Please correct me if my understanding is totally wrong.
> > > >
> > > > I'm afraid your understanding of CHAN seems wrong.
> > > > (Binding doc of that property needs change as well).
> > > >
> > > > On QXP DC SS, the IRQSTEER supports 512 interrupts with 8
> > > > interrupt output Conntected to GIC.
> > > > The current driver does not support it as it assumes only one
> > > > interrupt
> > >
> > > output used.
> > >
> > > Okay, so let's take a step back. The description in the QXP RM is
> > > actually better than what I've seen until now. Still it's totally confusing that
> the "channel"
> > > terminology used with different meanings in docs. Let's try to avoid
> > > this as much as possible.
> > >
> > > So to get things straight: Each irqsteer controller has a number of IRQ
> groups.
> > > All the input IRQs of one group are ORed together to form on output IRQ.
> > > Depending on the SoC integration, a group can contain 32 or
> > > 64 IRQs, where DCSS irqsteer on MX8M and the big 512 input
> > > controllers on QXP and QM both use 64 IRQs per group. You are
> > > claiming that the smaller controllers on both QXP am QM have only 32
> IRQs per group, right?
> > >
> > > So the only change that is needed is that the driver needs to know
> > > the number of input IRQs per group, with a default of 64 to not break DT
> compatibility.
> > >
> >
> > Not exactly.
> > from HW point of view , there're two parameters during IRQSTEER
> integration.
> > For example,
> > DC in QXP:
> > > > parameter  IRQCHAN		=  1; 	//Number of IRQ Channels/Slots
> > > > parameter  NINT32		=  8;	//Number of interrupts in multiple
> of 32
> 
> If this is always in multiples of 32, the only change we need to make to the
> driver is to fix DT binding and interpretation of the "fsl,irq-groups" property to
> be in multiples of 32.
> 
> This means i.MX8MQ DCSS irqsteer would need to change to 2 irq-groups, but
> as this isn't used upstream yet we can still do this change without breaking too
> much stuff and I would rather correct this now than keeping a DT binding
> around that doesn't match the HW.
> 

We want to avoid using of irq-groups as it's wrong.
Stick to HW parameters, only channel number and interrupts number should be used.

> > MIPI CSI in MQ:
> > > Parameter  IRQCHAN		= 1
> > > Parameter  NINT32		= 1
> >
> > You will see no group concept used here. Only channel number and
> interrupts number.
> > The group is an IP internal concept that ORed a group of 64 interrupts
> > into an output interrupt. But it may also only use 32 interrupts in the same
> group.
> 
> I suppose that the OR group size at that point is always 64 input IRQs per
> output IRQ, right? So with NINT32 == 1 you end up with 1 output IRQ, but for
> NINT32 == 3 you get 2 output IRQs, correct?

Yes, that's right.

> 
> > > Also if the connection between IRQ group and output IRQ is fixed,
> > > the driver should be more clever about handling the chained IRQ. If
> > > you know which of the upstream IRQs fired you only need to look at
> > > the 32 or 64 IRQ status registers of that specific group, not all of them.
> >
> > Yes, that's right.
> > I planned to do that later with a separate patch before.
> 
> Let's do it right with the first patch. This doesn't seem like a big change.
> 

We can do it.

> >
> > >
> > > Can you please clarify what the CHANCTRL setting changes in this setup?
> > >
> >
> > IRQsteer supports up to 5 separate CAHNNELS which each of them
> > supports up to 512 interrupts. CHANCTL is used to enable those respective
> CHAN output interrupts.
> > e.g.
> > 1~8 output interrupts of CHAN0.
> >
> > One notable thing is the each channel has a separate address space.
> > That means the chan1 reg address is not the one we specified in default reg
> property.
> > So the correct dts may be like for multi channels cases.
> > interrupt-controller@32e2d000 {
> >         compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";
> >         reg = <0x32e2d000 0x1000>,
> >               <0x32e2e000 0x1000>,
> >               <0x32e2f000 0x1000>;
> >               ...
> >         reg-names = "ch0", "ch1", "ch2", ...;
> >         interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> >         fsl,irqs-per-chan= <64>;
> >         interrupt-controller;
> >         #interrupt-cells = <2>; //cell 0: chan index cell 2: interrupt
> > number }; This makes the things quite complicated.
> 
> With the current binding, what keeps us from describing such a multi- channel
> irqsteer with multiple DT nodes and have multiple driver instances? I don't see
> why we would need to mix this all into one driver instance. 
> So for your above
> example, something like:
> 
> interrupt-controller@32e2d000 {
> 	compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";>
> 	reg = <0x32e2d000 0x1000>;
> 	interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> 	fsl,channel = <0>;
> };
> 
> interrupt-controller@32e2e000 {
> 	compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";>
> 	reg = <0x32e2e000 0x1000>;
> 	interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> 	fsl,channel = <1>;
> };
> 

Because from HW point of view, it IS actually one IRQSTEER module with multi
channels supported. So I feel describe each channel into several nodes
seems violate the HW a bit. That why I made the former dts binding as an example.

Another point is that there's only one physical CHANCTL register shared with multi
channels. However, each channel seems use a mirror CAHNCTRL register in its separate
register space to enable the channel. But needs care about overwrite others.
(Got this information after discussing with IC guys, still not verified)

> > In reality, we still don't have such using cases so far as as multi
> > channels usually are used to deliver the interrupts to different
> > cores, e.g. M4, SCU, or DSP, A core don't handle it.
> > So I did not change it currently as it's another story.
> > This patch series mainly aims to add support for 32 or 512 interrupts
> > channel and multiple Outputs for a single CHANNEL case.
> 
> The thing is, if we want to even try to keep DT stability we need to understand
> how this HW block can be used and how we can describe this in the DT.
> 

Yes, that's right.
But the binding is already there, so we can fix them one by one without breaking
the stability.

Regards
Dong Aisheng

> Regards,
> Lucas

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

[Aisheng Dong]

> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Tuesday, January 22, 2019 6:59 PM
> To: Aisheng Dong <aisheng.dong@nxp.com>; linux-kernel@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; shawnguo@kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; robh+dt@kernel.org; devicetree@vger.kernel.org;
> tglx@linutronix.de; Marc Zyngier <marc.zyngier@arm.com>
> Subject: Re: [PATCH 4/4] irq: imx: irqsteer: add multi output interrupts support
> 
> Am Dienstag, den 22.01.2019, 10:39 +0000 schrieb Aisheng Dong:
> > > > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > > Sent: Friday, January 18, 2019 6:23 PM
> >
> > [...]
> > > > > This has been discussed when upstreaming the driver. The
> > > > > controller may support multiple output IRQs, but only one them
> > > > > is actually used depending on the CHANCTRL config. There is no
> > > > > use in hooking up all the output IRQs in DT, if only one of them
> > > > > is actually used. Some of the outputs may not even be visible to
> > > > > the Linux system, but may belong to a Cortex M4 subsystem. All
> > > > > of those configurations can be described in DT by changing the
> > > > > upstream interrupt and "fsl,channel" in a
> > >
> > > coherent way.
> > > > >
> > > > > Please correct me if my understanding is totally wrong.
> > > >
> > > > I'm afraid your understanding of CHAN seems wrong.
> > > > (Binding doc of that property needs change as well).
> > > >
> > > > On QXP DC SS, the IRQSTEER supports 512 interrupts with 8
> > > > interrupt output Conntected to GIC.
> > > > The current driver does not support it as it assumes only one
> > > > interrupt
> > >
> > > output used.
> > >
> > > Okay, so let's take a step back. The description in the QXP RM is
> > > actually better than what I've seen until now. Still it's totally confusing that
> the "channel"
> > > terminology used with different meanings in docs. Let's try to avoid
> > > this as much as possible.
> > >
> > > So to get things straight: Each irqsteer controller has a number of IRQ
> groups.
> > > All the input IRQs of one group are ORed together to form on output IRQ.
> > > Depending on the SoC integration, a group can contain 32 or
> > > 64 IRQs, where DCSS irqsteer on MX8M and the big 512 input
> > > controllers on QXP and QM both use 64 IRQs per group. You are
> > > claiming that the smaller controllers on both QXP am QM have only 32
> IRQs per group, right?
> > >
> > > So the only change that is needed is that the driver needs to know
> > > the number of input IRQs per group, with a default of 64 to not break DT
> compatibility.
> > >
> >
> > Not exactly.
> > from HW point of view , there're two parameters during IRQSTEER
> integration.
> > For example,
> > DC in QXP:
> > > > parameter  IRQCHAN		=  1; 	//Number of IRQ Channels/Slots
> > > > parameter  NINT32		=  8;	//Number of interrupts in multiple
> of 32
> 
> If this is always in multiples of 32, the only change we need to make to the
> driver is to fix DT binding and interpretation of the "fsl,irq-groups" property to
> be in multiples of 32.
> 
> This means i.MX8MQ DCSS irqsteer would need to change to 2 irq-groups, but
> as this isn't used upstream yet we can still do this change without breaking too
> much stuff and I would rather correct this now than keeping a DT binding
> around that doesn't match the HW.
> 
> > MIPI CSI in MQ:
> > > Parameter  IRQCHAN		= 1
> > > Parameter  NINT32		= 1
> >
> > You will see no group concept used here. Only channel number and
> interrupts number.
> > The group is an IP internal concept that ORed a group of 64 interrupts
> > into an output interrupt. But it may also only use 32 interrupts in the same
> group.
> 
> I suppose that the OR group size at that point is always 64 input IRQs per
> output IRQ, right? So with NINT32 == 1 you end up with 1 output IRQ, but for
> NINT32 == 3 you get 2 output IRQs, correct?
> 
> > > Also if the connection between IRQ group and output IRQ is fixed,
> > > the driver should be more clever about handling the chained IRQ. If
> > > you know which of the upstream IRQs fired you only need to look at
> > > the 32 or 64 IRQ status registers of that specific group, not all of them.
> >
> > Yes, that's right.
> > I planned to do that later with a separate patch before.
> 
> Let's do it right with the first patch. This doesn't seem like a big change.
> 
> >
> > >
> > > Can you please clarify what the CHANCTRL setting changes in this setup?
> > >
> >
> > IRQsteer supports up to 5 separate CAHNNELS which each of them
> > supports up to 512 interrupts. CHANCTL is used to enable those respective
> CHAN output interrupts.
> > e.g.
> > 1~8 output interrupts of CHAN0.
> >
> > One notable thing is the each channel has a separate address space.
> > That means the chan1 reg address is not the one we specified in default reg
> property.
> > So the correct dts may be like for multi channels cases.
> > interrupt-controller@32e2d000 {
> >         compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";
> >         reg = <0x32e2d000 0x1000>,
> >               <0x32e2e000 0x1000>,
> >               <0x32e2f000 0x1000>;
> >               ...
> >         reg-names = "ch0", "ch1", "ch2", ...;
> >         interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> >         fsl,irqs-per-chan= <64>;
> >         interrupt-controller;
> >         #interrupt-cells = <2>; //cell 0: chan index cell 2: interrupt
> > number }; This makes the things quite complicated.
> 
> With the current binding, what keeps us from describing such a multi- channel
> irqsteer with multiple DT nodes and have multiple driver instances? I don't see
> why we would need to mix this all into one driver instance. So for your above
> example, something like:
> 
> interrupt-controller@32e2d000 {
> 	compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";>
> 	reg = <0x32e2d000 0x1000>;
> 	interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> 	fsl,channel = <0>;
> };
> 
> interrupt-controller@32e2e000 {
> 	compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";>
> 	reg = <0x32e2e000 0x1000>;
> 	interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> 	fsl,channel = <1>;
> };
> 

After a bit more thinking, I guess this might work as we have separate register space
We also have miniored CHANCTL for each channel, all things make them look like
separate IRQSTEERs.
e.g.
AIPS – slot 9	64KB	IRQSTR.SCU2
AIPS – slot 8	64KB	IRQSTR.DSP
AIPS – slot 7	64KB	IRQSTR.CM4_0
AIPS – slot 6	64KB	IRQSTR.SCU

I need talk to the IP module designer to make sure the mirror chanctl can work well
And get back to you later once have an conclusion.

If that's ok, then we may only need update fsl,irq-groups to fsl,irqs-per-chan.

(BTW, those irqsteers actually are not used by A core, they're usd by SCU, M4 and DSP.
So this actually does not affect A core side work. E.g. Linux)

Regards
Dong Aisheng

> > In reality, we still don't have such using cases so far as as multi
> > channels usually are used to deliver the interrupts to different
> > cores, e.g. M4, SCU, or DSP, A core don't handle it.
> > So I did not change it currently as it's another story.
> > This patch series mainly aims to add support for 32 or 512 interrupts
> > channel and multiple Outputs for a single CHANNEL case.
> 
> The thing is, if we want to even try to keep DT stability we need to understand
> how this HW block can be used and how we can describe this in the DT.
> 
> Regards,
> Lucas

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

[Lucas Stach]

Am Dienstag, den 22.01.2019, 12:03 +0000 schrieb Aisheng Dong:
> > > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > Sent: Tuesday, January 22, 2019 6:59 PM
> > 
> > Am Dienstag, den 22.01.2019, 10:39 +0000 schrieb Aisheng Dong:
> > > > > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > > > 
> > > > Sent: Friday, January 18, 2019 6:23 PM
> > > 
> > > [...]
> > > > > > This has been discussed when upstreaming the driver. The
> > > > > > controller may support multiple output IRQs, but only one them
> > > > > > is actually used depending on the CHANCTRL config. There is no
> > > > > > use in hooking up all the output IRQs in DT, if only one of them
> > > > > > is actually used. Some of the outputs may not even be visible to
> > > > > > the Linux system, but may belong to a Cortex M4 subsystem. All
> > > > > > of those configurations can be described in DT by changing the
> > > > > > upstream interrupt and "fsl,channel" in a
> > > > 
> > > > coherent way.
> > > > > > 
> > > > > > Please correct me if my understanding is totally wrong.
> > > > > 
> > > > > I'm afraid your understanding of CHAN seems wrong.
> > > > > (Binding doc of that property needs change as well).
> > > > > 
> > > > > On QXP DC SS, the IRQSTEER supports 512 interrupts with 8
> > > > > interrupt output Conntected to GIC.
> > > > > The current driver does not support it as it assumes only one
> > > > > interrupt
> > > > 
> > > > output used.
> > > > 
> > > > Okay, so let's take a step back. The description in the QXP RM is
> > > > actually better than what I've seen until now. Still it's totally confusing that
> > 
> > the "channel"
> > > > terminology used with different meanings in docs. Let's try to avoid
> > > > this as much as possible.
> > > > 
> > > > So to get things straight: Each irqsteer controller has a number of IRQ
> > 
> > groups.
> > > > All the input IRQs of one group are ORed together to form on output IRQ.
> > > > Depending on the SoC integration, a group can contain 32 or
> > > > 64 IRQs, where DCSS irqsteer on MX8M and the big 512 input
> > > > controllers on QXP and QM both use 64 IRQs per group. You are
> > > > claiming that the smaller controllers on both QXP am QM have only 32
> > 
> > IRQs per group, right?
> > > > 
> > > > So the only change that is needed is that the driver needs to know
> > > > the number of input IRQs per group, with a default of 64 to not break DT
> > 
> > compatibility.
> > > > 
> > > 
> > > Not exactly.
> > > from HW point of view , there're two parameters during IRQSTEER
> > 
> > integration.
> > > For example,
> > > DC in QXP:
> > > > > > > > > > > > > > > parameter  IRQCHAN		=  1; 	//Number of IRQ Channels/Slots
> > > > > parameter  NINT32		=  8;	//Number of interrupts in multiple
> > 
> > of 32
> > 
> > If this is always in multiples of 32, the only change we need to make to the
> > driver is to fix DT binding and interpretation of the "fsl,irq-groups" property to
> > be in multiples of 32.
> > 
> > This means i.MX8MQ DCSS irqsteer would need to change to 2 irq-groups, but
> > as this isn't used upstream yet we can still do this change without breaking too
> > much stuff and I would rather correct this now than keeping a DT binding
> > around that doesn't match the HW.
> > 
> 
> We want to avoid using of irq-groups as it's wrong.
> Stick to HW parameters, only channel number and interrupts number should be used.

The fsl,irq-groups property is exactly your NINT32 parameter above. I
just wrongly assumed that it's always in multiples of 64, as that's
what the i.MX8MQ DCSS irqsteer module looks like. We should fix this
and be done with it.

> > > MIPI CSI in MQ:
> > > > > > > > Parameter  IRQCHAN		= 1
> > > > Parameter  NINT32		= 1
> > > 
> > > You will see no group concept used here. Only channel number and
> > 
> > interrupts number.
> > > The group is an IP internal concept that ORed a group of 64 interrupts
> > > into an output interrupt. But it may also only use 32 interrupts in the same
> > 
> > group.
> > 
> > I suppose that the OR group size at that point is always 64 input IRQs per
> > output IRQ, right? So with NINT32 == 1 you end up with 1 output IRQ, but for
> > NINT32 == 3 you get 2 output IRQs, correct?
> 
> Yes, that's right.
> 
> > 
> > > > Also if the connection between IRQ group and output IRQ is fixed,
> > > > the driver should be more clever about handling the chained IRQ. If
> > > > you know which of the upstream IRQs fired you only need to look at
> > > > the 32 or 64 IRQ status registers of that specific group, not all of them.
> > > 
> > > Yes, that's right.
> > > I planned to do that later with a separate patch before.
> > 
> > Let's do it right with the first patch. This doesn't seem like a big change.
> > 
> 
> We can do it.
> 
> > > 
> > > > 
> > > > Can you please clarify what the CHANCTRL setting changes in this setup?
> > > > 
> > > 
> > > IRQsteer supports up to 5 separate CAHNNELS which each of them
> > > supports up to 512 interrupts. CHANCTL is used to enable those respective
> > 
> > CHAN output interrupts.
> > > e.g.
> > > 1~8 output interrupts of CHAN0.
> > > 
> > > One notable thing is the each channel has a separate address space.
> > > That means the chan1 reg address is not the one we specified in default reg
> > 
> > property.
> > > So the correct dts may be like for multi channels cases.
> > > interrupt-controller@32e2d000 {
> > >         compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";
> > >         reg = <0x32e2d000 0x1000>,
> > >               <0x32e2e000 0x1000>,
> > >               <0x32e2f000 0x1000>;
> > >               ...
> > >         reg-names = "ch0", "ch1", "ch2", ...;
> > >         interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> > >         fsl,irqs-per-chan= <64>;
> > >         interrupt-controller;
> > >         #interrupt-cells = <2>; //cell 0: chan index cell 2: interrupt
> > > number }; This makes the things quite complicated.
> > 
> > With the current binding, what keeps us from describing such a multi- channel
> > irqsteer with multiple DT nodes and have multiple driver instances? I don't see
> > why we would need to mix this all into one driver instance. 
> > So for your above
> > example, something like:
> > 
> > interrupt-controller@32e2d000 {
> > 	compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";>
> > 	reg = <0x32e2d000 0x1000>;
> > 	interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> > 	fsl,channel = <0>;
> > };
> > 
> > interrupt-controller@32e2e000 {
> > 	compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";>
> > 	reg = <0x32e2e000 0x1000>;
> > 	interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> > 	fsl,channel = <1>;
> > };
> > 
> 
> Because from HW point of view, it IS actually one IRQSTEER module with multi
> channels supported. So I feel describe each channel into several nodes
> seems violate the HW a bit. That why I made the former dts binding as an example.

Yes, DT describes HW but that doesn't mean we slavishly need to stick
to the HW module boundaries. DT is always also an abstraction over the
hardware, so if we can both describe the HW more easily and keep the
driver simpler by treating the HW block as multiple instances of the
same thing, I think we should do this.

> Another point is that there's only one physical CHANCTL register shared with multi
> channels. However, each channel seems use a mirror CAHNCTRL register in its separate
> register space to enable the channel. But needs care about overwrite others.
> (Got this information after discussing with IC guys, still not verified)

So that's something I don't understand yet. The docs state that only
one of the CHANCTL CH bit can be active at any time. If it's only one
physical register this can't be true.

If the CHANCTL in each channel register space is just a mirror of a
single physical register then sure, we could get into issues when
multiple driver instances try to change their "private" CHANCTL mirror
via a RMW cycle. So it's quite crucial to find out how it's wired up
internally.

Regards,
Lucas

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

[Aisheng Dong]

> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Tuesday, January 22, 2019 8:52 PM
> To: Aisheng Dong <aisheng.dong@nxp.com>; linux-kernel@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; shawnguo@kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; robh+dt@kernel.org; devicetree@vger.kernel.org;
> tglx@linutronix.de; Marc Zyngier <marc.zyngier@arm.com>
> Subject: Re: [PATCH 4/4] irq: imx: irqsteer: add multi output interrupts support
> 
> Am Dienstag, den 22.01.2019, 12:03 +0000 schrieb Aisheng Dong:
> > > > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > > Sent: Tuesday, January 22, 2019 6:59 PM
> > >
> > > Am Dienstag, den 22.01.2019, 10:39 +0000 schrieb Aisheng Dong:
> > > > > > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > > > >
> > > > > Sent: Friday, January 18, 2019 6:23 PM
> > > >
> > > > [...]
> > > > > > > This has been discussed when upstreaming the driver. The
> > > > > > > controller may support multiple output IRQs, but only one
> > > > > > > them is actually used depending on the CHANCTRL config.
> > > > > > > There is no use in hooking up all the output IRQs in DT, if
> > > > > > > only one of them is actually used. Some of the outputs may
> > > > > > > not even be visible to the Linux system, but may belong to a
> > > > > > > Cortex M4 subsystem. All of those configurations can be
> > > > > > > described in DT by changing the upstream interrupt and
> > > > > > > "fsl,channel" in a
> > > > >
> > > > > coherent way.
> > > > > > >
> > > > > > > Please correct me if my understanding is totally wrong.
> > > > > >
> > > > > > I'm afraid your understanding of CHAN seems wrong.
> > > > > > (Binding doc of that property needs change as well).
> > > > > >
> > > > > > On QXP DC SS, the IRQSTEER supports 512 interrupts with 8
> > > > > > interrupt output Conntected to GIC.
> > > > > > The current driver does not support it as it assumes only one
> > > > > > interrupt
> > > > >
> > > > > output used.
> > > > >
> > > > > Okay, so let's take a step back. The description in the QXP RM
> > > > > is actually better than what I've seen until now. Still it's
> > > > > totally confusing that
> > >
> > > the "channel"
> > > > > terminology used with different meanings in docs. Let's try to
> > > > > avoid this as much as possible.
> > > > >
> > > > > So to get things straight: Each irqsteer controller has a number
> > > > > of IRQ
> > >
> > > groups.
> > > > > All the input IRQs of one group are ORed together to form on output
> IRQ.
> > > > > Depending on the SoC integration, a group can contain 32 or
> > > > > 64 IRQs, where DCSS irqsteer on MX8M and the big 512 input
> > > > > controllers on QXP and QM both use 64 IRQs per group. You are
> > > > > claiming that the smaller controllers on both QXP am QM have
> > > > > only 32
> > >
> > > IRQs per group, right?
> > > > >
> > > > > So the only change that is needed is that the driver needs to
> > > > > know the number of input IRQs per group, with a default of 64 to
> > > > > not break DT
> > >
> > > compatibility.
> > > > >
> > > >
> > > > Not exactly.
> > > > from HW point of view , there're two parameters during IRQSTEER
> > >
> > > integration.
> > > > For example,
> > > > DC in QXP:
> > > > > > > > > > > > > > > > parameter  IRQCHAN		=  1;
> 	//Number of IRQ Channels/Slots
> > > > > > parameter  NINT32		=  8;	//Number of interrupts in
> multiple
> > >
> > > of 32
> > >
> > > If this is always in multiples of 32, the only change we need to
> > > make to the driver is to fix DT binding and interpretation of the
> > > "fsl,irq-groups" property to be in multiples of 32.
> > >
> > > This means i.MX8MQ DCSS irqsteer would need to change to 2
> > > irq-groups, but as this isn't used upstream yet we can still do this
> > > change without breaking too much stuff and I would rather correct
> > > this now than keeping a DT binding around that doesn't match the HW.
> > >
> >
> > We want to avoid using of irq-groups as it's wrong.
> > Stick to HW parameters, only channel number and interrupts number should
> be used.
> 
> The fsl,irq-groups property is exactly your NINT32 parameter above. I just
> wrongly assumed that it's always in multiples of 64, as that's what the
> i.MX8MQ DCSS irqsteer module looks like. We should fix this and be done with
> it.
> 

No, not exactly the same thing. Using group will confuse people that the group is 32.
However, internally Group is fixed 64 interrupts although it may not use all the
64 interrupts. E.g. 32 interrupts.
See CHn_MINTDIS register which is also defined fixed to 64.

The two HW parameter for integration is already very clear. We should use interrupts
Number for the channel. Not group. 

> > > > MIPI CSI in MQ:
> > > > > > > > > Parameter  IRQCHAN		= 1
> > > > > Parameter  NINT32		= 1
> > > >
> > > > You will see no group concept used here. Only channel number and
> > >
> > > interrupts number.
> > > > The group is an IP internal concept that ORed a group of 64
> > > > interrupts into an output interrupt. But it may also only use 32
> > > > interrupts in the same
> > >
> > > group.
> > >
> > > I suppose that the OR group size at that point is always 64 input
> > > IRQs per output IRQ, right? So with NINT32 == 1 you end up with 1
> > > output IRQ, but for
> > > NINT32 == 3 you get 2 output IRQs, correct?
> >
> > Yes, that's right.
> >
> > >
> > > > > Also if the connection between IRQ group and output IRQ is
> > > > > fixed, the driver should be more clever about handling the
> > > > > chained IRQ. If you know which of the upstream IRQs fired you
> > > > > only need to look at the 32 or 64 IRQ status registers of that specific
> group, not all of them.
> > > >
> > > > Yes, that's right.
> > > > I planned to do that later with a separate patch before.
> > >
> > > Let's do it right with the first patch. This doesn't seem like a big change.
> > >
> >
> > We can do it.
> >
> > > >
> > > > >
> > > > > Can you please clarify what the CHANCTRL setting changes in this
> setup?
> > > > >
> > > >
> > > > IRQsteer supports up to 5 separate CAHNNELS which each of them
> > > > supports up to 512 interrupts. CHANCTL is used to enable those
> > > > respective
> > >
> > > CHAN output interrupts.
> > > > e.g.
> > > > 1~8 output interrupts of CHAN0.
> > > >
> > > > One notable thing is the each channel has a separate address space.
> > > > That means the chan1 reg address is not the one we specified in
> > > > default reg
> > >
> > > property.
> > > > So the correct dts may be like for multi channels cases.
> > > > interrupt-controller@32e2d000 {
> > > >         compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";
> > > >         reg = <0x32e2d000 0x1000>,
> > > >               <0x32e2e000 0x1000>,
> > > >               <0x32e2f000 0x1000>;
> > > >               ...
> > > >         reg-names = "ch0", "ch1", "ch2", ...;
> > > >         interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> > > >         fsl,irqs-per-chan= <64>;
> > > >         interrupt-controller;
> > > >         #interrupt-cells = <2>; //cell 0: chan index cell 2:
> > > > interrupt number }; This makes the things quite complicated.
> > >
> > > With the current binding, what keeps us from describing such a
> > > multi- channel irqsteer with multiple DT nodes and have multiple
> > > driver instances? I don't see why we would need to mix this all into one
> driver instance.
> > > So for your above
> > > example, something like:
> > >
> > > interrupt-controller@32e2d000 {
> > > 	compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";>
> > > 	reg = <0x32e2d000 0x1000>;
> > > 	interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> > > 	fsl,channel = <0>;
> > > };
> > >
> > > interrupt-controller@32e2e000 {
> > > 	compatible = "fsl,imx8m-irqsteer", "fsl,imx-irqsteer";>
> > > 	reg = <0x32e2e000 0x1000>;
> > > 	interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> > > 	fsl,channel = <1>;
> > > };
> > >
> >
> > Because from HW point of view, it IS actually one IRQSTEER module with
> > multi channels supported. So I feel describe each channel into several
> > nodes seems violate the HW a bit. That why I made the former dts binding as
> an example.
> 
> Yes, DT describes HW but that doesn't mean we slavishly need to stick to the
> HW module boundaries. DT is always also an abstraction over the hardware, so
> if we can both describe the HW more easily and keep the driver simpler by
> treating the HW block as multiple instances of the same thing, I think we
> should do this.
> 

Yes, please check my another mail that I'm intend to agree with you.

> > Another point is that there's only one physical CHANCTL register
> > shared with multi channels. However, each channel seems use a mirror
> > CAHNCTRL register in its separate register space to enable the channel. But
> needs care about overwrite others.
> > (Got this information after discussing with IC guys, still not
> > verified)
> 
> So that's something I don't understand yet. The docs state that only one of the
> CHANCTL CH bit can be active at any time. If it's only one physical register this
> can't be true.

Doc is a bit wrong here. It can enable all channels at the same time.

> 
> If the CHANCTL in each channel register space is just a mirror of a single
> physical register then sure, we could get into issues when multiple driver
> instances try to change their "private" CHANCTL mirror via a RMW cycle. So it's
> quite crucial to find out how it's wired up internally.
> 

I need double check with IP owner.

Regards
Dong Aisheng

> Regards,
> Lucas

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

[Lucas Stach]

Hi,

Am Dienstag, den 22.01.2019, 13:17 +0000 schrieb Aisheng Dong:
> > > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > Sent: Tuesday, January 22, 2019 8:52 PM
> > > > To: Aisheng Dong <aisheng.dong@nxp.com>; linux-kernel@vger.kernel.org
> > > > > > Cc: linux-arm-kernel@lists.infradead.org; shawnguo@kernel.org; dl-linux-imx
> > > > > > > > <linux-imx@nxp.com>; robh+dt@kernel.org; devicetree@vger.kernel.org;
> > > > tglx@linutronix.de; Marc Zyngier <marc.zyngier@arm.com>
> > Subject: Re: [PATCH 4/4] irq: imx: irqsteer: add multi output interrupts support
> > 
> > Am Dienstag, den 22.01.2019, 12:03 +0000 schrieb Aisheng Dong:
> > > > > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > > > 
> > > > Sent: Tuesday, January 22, 2019 6:59 PM
> > > > 
> > > > Am Dienstag, den 22.01.2019, 10:39 +0000 schrieb Aisheng Dong:
> > > > > > > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > > > > > 
> > > > > > Sent: Friday, January 18, 2019 6:23 PM
> > > > > 
> > > > > [...]
> > > > > > > > This has been discussed when upstreaming the driver. The
> > > > > > > > controller may support multiple output IRQs, but only one
> > > > > > > > them is actually used depending on the CHANCTRL config.
> > > > > > > > There is no use in hooking up all the output IRQs in DT, if
> > > > > > > > only one of them is actually used. Some of the outputs may
> > > > > > > > not even be visible to the Linux system, but may belong to a
> > > > > > > > Cortex M4 subsystem. All of those configurations can be
> > > > > > > > described in DT by changing the upstream interrupt and
> > > > > > > > "fsl,channel" in a
> > > > > > 
> > > > > > coherent way.
> > > > > > > > 
> > > > > > > > Please correct me if my understanding is totally wrong.
> > > > > > > 
> > > > > > > I'm afraid your understanding of CHAN seems wrong.
> > > > > > > (Binding doc of that property needs change as well).
> > > > > > > 
> > > > > > > On QXP DC SS, the IRQSTEER supports 512 interrupts with 8
> > > > > > > interrupt output Conntected to GIC.
> > > > > > > The current driver does not support it as it assumes only one
> > > > > > > interrupt
> > > > > > 
> > > > > > output used.
> > > > > > 
> > > > > > Okay, so let's take a step back. The description in the QXP RM
> > > > > > is actually better than what I've seen until now. Still it's
> > > > > > totally confusing that
> > > > 
> > > > the "channel"
> > > > > > terminology used with different meanings in docs. Let's try to
> > > > > > avoid this as much as possible.
> > > > > > 
> > > > > > So to get things straight: Each irqsteer controller has a number
> > > > > > of IRQ
> > > > 
> > > > groups.
> > > > > > All the input IRQs of one group are ORed together to form on output
> > 
> > IRQ.
> > > > > > Depending on the SoC integration, a group can contain 32 or
> > > > > > 64 IRQs, where DCSS irqsteer on MX8M and the big 512 input
> > > > > > controllers on QXP and QM both use 64 IRQs per group. You are
> > > > > > claiming that the smaller controllers on both QXP am QM have
> > > > > > only 32
> > > > 
> > > > IRQs per group, right?
> > > > > > 
> > > > > > So the only change that is needed is that the driver needs to
> > > > > > know the number of input IRQs per group, with a default of 64 to
> > > > > > not break DT
> > > > 
> > > > compatibility.
> > > > > > 
> > > > > 
> > > > > Not exactly.
> > > > > from HW point of view , there're two parameters during IRQSTEER
> > > > 
> > > > integration.
> > > > > For example,
> > > > > DC in QXP:
> > > > > > > > > > > > > > > > > parameter  IRQCHAN		=  1;
> > 
> > 	//Number of IRQ Channels/Slots
> > > > > > > parameter  NINT32		=  8;	//Number of interrupts in
> > 
> > multiple
> > > > 
> > > > of 32
> > > > 
> > > > If this is always in multiples of 32, the only change we need to
> > > > make to the driver is to fix DT binding and interpretation of the
> > > > "fsl,irq-groups" property to be in multiples of 32.
> > > > 
> > > > This means i.MX8MQ DCSS irqsteer would need to change to 2
> > > > irq-groups, but as this isn't used upstream yet we can still do this
> > > > change without breaking too much stuff and I would rather correct
> > > > this now than keeping a DT binding around that doesn't match the HW.
> > > > 
> > > 
> > > We want to avoid using of irq-groups as it's wrong.
> > > Stick to HW parameters, only channel number and interrupts number should
> > 
> > be used.
> > 
> > The fsl,irq-groups property is exactly your NINT32 parameter above. I just
> > wrongly assumed that it's always in multiples of 64, as that's what the
> > i.MX8MQ DCSS irqsteer module looks like. We should fix this and be done with
> > it.
> > 
> 
> No, not exactly the same thing. Using group will confuse people that the group is 32.
> However, internally Group is fixed 64 interrupts although it may not use all the
> 64 interrupts. E.g. 32 interrupts.
> See CHn_MINTDIS register which is also defined fixed to 64.
> 
> The two HW parameter for integration is already very clear. We should use interrupts
> Number for the channel. Not group. 

Okay, I see that the name irq-groups is confusing for you. But then I
find the -per-chan naming confusing.

So given that we seem to agree to split each channel into it's own DT
node, there is no need to name the property "something-per-chan", as
it's implied by the split DT nodes that all properties in one node are
referencing a channel.

May I suggest to name the property "fsl,num-irqs"?

Regards,
Lucas

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

[Lucas Stach]

Am Freitag, den 18.01.2019, 07:53 +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>
> ---
>  .../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 eaabcda..beeed4a 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. NOTE at least one output interrupt has to be specified.

I don't think the note clarifies anything. Given that the thing is a
irq multiplexer I don't think anyone expects that you could get away
with no output irq.

If anything this should specify that it's always groups of (up to) 64
input irqs being muxed onto a single output irq.

Regards,
Lucas

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

[Lucas Stach]

Am Freitag, den 18.01.2019, 07:53 +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>
> ---
>  drivers/irqchip/irq-imx-irqsteer.c | 39 +++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 1bebf0a..54802fa 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;
> @@ -117,7 +121,7 @@ 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;
> > +	int i, ret;
>  
> >  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> >  	if (!data)
> @@ -130,12 +134,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);
> @@ -177,8 +175,23 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  	}
>  
> > -	irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler,
> > -					 data);
> +	data->irq_count = of_irq_count(np);

We normally don't validate stuff that comes from the DT, but I guess it
might be helpful to validate that the number of output irqs specified
matches what the number of input irqs, i.e. there is one output irqs
specified for each group of 64 inputs.

> +	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);

We really want some data about the output irq being passed here, so we
can cut down the number of register reads in the irq handler to the
maximum of 2 status registers per group.

Regards,
Lucas

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

[Dong Aisheng]

Hi Lucas,

[...]

> > > The fsl,irq-groups property is exactly your NINT32 parameter above. I just
> > > wrongly assumed that it's always in multiples of 64, as that's what the
> > > i.MX8MQ DCSS irqsteer module looks like. We should fix this and be done with
> > > it.
> > >
> >
> > No, not exactly the same thing. Using group will confuse people that the group is 32.
> > However, internally Group is fixed 64 interrupts although it may not use all the
> > 64 interrupts. E.g. 32 interrupts.
> > See CHn_MINTDIS register which is also defined fixed to 64.
> >
> > The two HW parameter for integration is already very clear. We should use interrupts
> > Number for the channel. Not group.
>
> Okay, I see that the name irq-groups is confusing for you. But then I
> find the -per-chan naming confusing.
>
> So given that we seem to agree to split each channel into it's own DT
> node, there is no need to name the property "something-per-chan", as
> it's implied by the split DT nodes that all properties in one node are
> referencing a channel.
>

Double checked with IP module owner by looking into the RTL code,
writes to CHANnCTL registers have no effect.
This register is reserved and the doc will be updated.
Verified on both MX8QXP and MX8MQ.

All channels are isolated and have independent programming model.
So it will be okay to define them separately in device tree.

> May I suggest to name the property "fsl,num-irqs"?

Sounds good to me.

Regards
Dong Aisheng

>
> Regards,
> Lucas
>

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

[Dong Aisheng]

On Fri, Jan 25, 2019 at 6:55 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Freitag, den 18.01.2019, 07:53 +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>
> > ---
> >  drivers/irqchip/irq-imx-irqsteer.c | 39 +++++++++++++++++++++++++++-----------
> >  1 file changed, 28 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> > index 1bebf0a..54802fa 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;
> > @@ -117,7 +121,7 @@ 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;
> > > +   int i, ret;
> >
> > >     data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > >     if (!data)
> > @@ -130,12 +134,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);
> > @@ -177,8 +175,23 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> > >             return -ENOMEM;
> > >     }
> >
> > > -   irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler,
> > > -                                    data);
> > +     data->irq_count = of_irq_count(np);
>
> We normally don't validate stuff that comes from the DT, but I guess it
> might be helpful to validate that the number of output irqs specified
> matches what the number of input irqs, i.e. there is one output irqs
> specified for each group of 64 inputs.
>

Sound like a good idea.
I think we can calculate the irq_count from irqs number instead of of_irq_count.
Then the following irq_of_parse_and_map() will validate them automatically.

However, i think we'd better to keep the irq_count range check as well.

> > +     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);
>
> We really want some data about the output irq being passed here, so we
> can cut down the number of register reads in the irq handler to the
> maximum of 2 status registers per group.
>

Will implement it and resend.

Regards
Dong Aisheng

> Regards,
> Lucas

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

[Dong Aisheng]

On Fri, Jan 25, 2019 at 6:47 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Freitag, den 18.01.2019, 07:53 +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>
> > ---
> >  .../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 eaabcda..beeed4a 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. NOTE at least one output interrupt has to be specified.
>
> I don't think the note clarifies anything. Given that the thing is a
> irq multiplexer I don't think anyone expects that you could get away
> with no output irq.
>

Sound reasonable to me.
I will remove the note and keep other as it is.

Regards
Dong Aisheng