Not all 64 interrupts may be used in one group. e.g. most irqsteer in
imx8qxp and imx8qm subsystems supports only 32 interrupts.
And one irqsteer channel can support up to 8 output interrupts.
This patch series aims to support 32 interrupts chan and multi output
interrupts.
Tested on:
iMX8QXP MEK with MIPI CSI capture and DC Display
iMX8MQ EVK with MIPI DSI Display
Dong Aisheng (4):
dt-binding: irq: imx-irqsteer: use irq number instead of group number
dt-bindings: irq: imx-irqsteer: add multi output interrupts support
irq: imx-irqsteer: change to use reg_num instead of irq_group
irq: imx: irqsteer: add multi output interrupts support
.../bindings/interrupt-controller/fsl,irqsteer.txt | 11 ++-
drivers/irqchip/irq-imx-irqsteer.c | 103 +++++++++++++++------
2 files changed, 80 insertions(+), 34 deletions(-)
--
2.7.4
Not all 64 interrupts may be used in one group. e.g. most irqsteer in
imx8qxp and imx8qm subsystems supports only 32 interrupts.
As the IP integration parameters are Channel number and interrupts number,
let's use fsl,num-irqs to represents how many interrupts supported
by this irqsteer channel.
Cc: Marc Zyngier <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: [email protected]
Signed-off-by: Dong Aisheng <[email protected]>
---
ChangeLog:
v1->v2:
* change property name fsl,irqs-per-chan to fsl,num-irqs.
---
.../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
index 45790ce..6d0a41b 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
@@ -16,8 +16,8 @@ Required properties:
- #interrupt-cells: Specifies the number of cells needed to encode an
interrupt source. The value must be 1.
- fsl,channel: The output channel that all input IRQs should be steered into.
-- fsl,irq-groups: Number of IRQ groups managed by this controller instance.
- Each group manages 64 input interrupts.
+- fsl,num-irqs: Number of input interrupts of this channel.
+ Should be multiple of 32 input interrupts and up to 512 interrupts.
Example:
@@ -28,7 +28,7 @@ Example:
clocks = <&clk IMX8MQ_CLK_DISP_APB_ROOT>;
clock-names = "ipg";
fsl,channel = <0>;
- fsl,irq-groups = <1>;
+ fsl,num-irqs = <64>;
interrupt-controller;
#interrupt-cells = <1>;
};
--
2.7.4
One irqsteer channel can support up to 8 output interrupts.
Cc: Marc Zyngier <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: [email protected]
Signed-off-by: Dong Aisheng <[email protected]>
---
ChangeLog:
v1->v2:
* remove one unnecessary note.
---
.../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
index 6d0a41b..984ab92 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
@@ -6,8 +6,9 @@ Required properties:
- "fsl,imx8m-irqsteer"
- "fsl,imx-irqsteer"
- reg: Physical base address and size of registers.
-- interrupts: Should contain the parent interrupt line used to multiplex the
- input interrupts.
+- interrupts: Should contain the up to 8 parent interrupt lines used to
+ multiplex the input interrupts. They should be sepcified seqencially
+ from output 0 to 7.
- clocks: Should contain one clock for entry in clock-names
see Documentation/devicetree/bindings/clock/clock-bindings.txt
- clock-names:
--
2.7.4
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 <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Shawn Guo <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
v1->v2:
* The using of property name updated accordingly
---
drivers/irqchip/irq-imx-irqsteer.c | 35 +++++++++++++++++++----------------
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
index 5b3f1d7..67ed862 100644
--- a/drivers/irqchip/irq-imx-irqsteer.c
+++ b/drivers/irqchip/irq-imx-irqsteer.c
@@ -13,7 +13,7 @@
#include <linux/of_platform.h>
#include <linux/spinlock.h>
-#define CTRL_STRIDE_OFF(_t, _r) (_t * 8 * _r)
+#define CTRL_STRIDE_OFF(_t, _r) (_t * 4 * _r)
#define CHANCTRL 0x0
#define CHANMASK(n, t) (CTRL_STRIDE_OFF(t, 0) + 0x4 * (n) + 0x4)
#define CHANSET(n, t) (CTRL_STRIDE_OFF(t, 1) + 0x4 * (n) + 0x4)
@@ -26,7 +26,7 @@ struct irqsteer_data {
struct clk *ipg_clk;
int irq;
raw_spinlock_t lock;
- int irq_groups;
+ int reg_num;
int channel;
struct irq_domain *domain;
u32 *saved_reg;
@@ -35,7 +35,7 @@ struct irqsteer_data {
static int imx_irqsteer_get_reg_index(struct irqsteer_data *data,
unsigned long irqnum)
{
- return (data->irq_groups * 2 - irqnum / 32 - 1);
+ return (data->reg_num - irqnum / 32 - 1);
}
static void imx_irqsteer_irq_unmask(struct irq_data *d)
@@ -46,9 +46,9 @@ static void imx_irqsteer_irq_unmask(struct irq_data *d)
u32 val;
raw_spin_lock_irqsave(&data->lock, flags);
- val = readl_relaxed(data->regs + CHANMASK(idx, data->irq_groups));
+ val = readl_relaxed(data->regs + CHANMASK(idx, data->reg_num));
val |= BIT(d->hwirq % 32);
- writel_relaxed(val, data->regs + CHANMASK(idx, data->irq_groups));
+ writel_relaxed(val, data->regs + CHANMASK(idx, data->reg_num));
raw_spin_unlock_irqrestore(&data->lock, flags);
}
@@ -60,9 +60,9 @@ static void imx_irqsteer_irq_mask(struct irq_data *d)
u32 val;
raw_spin_lock_irqsave(&data->lock, flags);
- val = readl_relaxed(data->regs + CHANMASK(idx, data->irq_groups));
+ val = readl_relaxed(data->regs + CHANMASK(idx, data->reg_num));
val &= ~BIT(d->hwirq % 32);
- writel_relaxed(val, data->regs + CHANMASK(idx, data->irq_groups));
+ writel_relaxed(val, data->regs + CHANMASK(idx, data->reg_num));
raw_spin_unlock_irqrestore(&data->lock, flags);
}
@@ -94,13 +94,13 @@ static void imx_irqsteer_irq_handler(struct irq_desc *desc)
chained_irq_enter(irq_desc_get_chip(desc), desc);
- for (i = 0; i < data->irq_groups * 64; i += 32) {
+ for (i = 0; i < data->reg_num * 32; i += 32) {
int idx = imx_irqsteer_get_reg_index(data, i);
unsigned long irqmap;
int pos, virq;
irqmap = readl_relaxed(data->regs +
- CHANSTATUS(idx, data->irq_groups));
+ CHANSTATUS(idx, data->reg_num));
for_each_set_bit(pos, &irqmap, 32) {
virq = irq_find_mapping(data->domain, pos + i);
@@ -146,12 +146,15 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
raw_spin_lock_init(&data->lock);
- of_property_read_u32(np, "fsl,irq-groups", &data->irq_groups);
+ of_property_read_u32(np, "fsl,num-irqs", &data->reg_num);
of_property_read_u32(np, "fsl,channel", &data->channel);
+ /* one register bit map represents 32 input interrupts */
+ data->reg_num /= 32;
+
if (IS_ENABLED(CONFIG_PM_SLEEP)) {
data->saved_reg = devm_kzalloc(&pdev->dev,
- sizeof(u32) * data->irq_groups * 2,
+ sizeof(u32) * data->reg_num,
GFP_KERNEL);
if (!data->saved_reg)
return -ENOMEM;
@@ -166,7 +169,7 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
/* steer all IRQs into configured channel */
writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
- data->domain = irq_domain_add_linear(np, data->irq_groups * 64,
+ data->domain = irq_domain_add_linear(np, data->reg_num * 32,
&imx_irqsteer_domain_ops, data);
if (!data->domain) {
dev_err(&pdev->dev, "failed to create IRQ domain\n");
@@ -199,9 +202,9 @@ static void imx_irqsteer_save_regs(struct irqsteer_data *data)
{
int i;
- for (i = 0; i < data->irq_groups * 2; i++)
+ for (i = 0; i < data->reg_num; i++)
data->saved_reg[i] = readl_relaxed(data->regs +
- CHANMASK(i, data->irq_groups));
+ CHANMASK(i, data->reg_num));
}
static void imx_irqsteer_restore_regs(struct irqsteer_data *data)
@@ -209,9 +212,9 @@ static void imx_irqsteer_restore_regs(struct irqsteer_data *data)
int i;
writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
- for (i = 0; i < data->irq_groups * 2; i++)
+ for (i = 0; i < data->reg_num; i++)
writel_relaxed(data->saved_reg[i],
- data->regs + CHANMASK(i, data->irq_groups));
+ data->regs + CHANMASK(i, data->reg_num));
}
static int imx_irqsteer_suspend(struct device *dev)
--
2.7.4
One irqsteer channel can support up to 8 output interrupts.
Cc: Marc Zyngier <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Shawn Guo <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
ChangeLog:
v1->v2:
* calculate irq_count by fsl,num-irqs instead of parsing interrupts
property from devicetree to match the input interrupts and outputs
* improve output interrupt handler by searching only two registers
withint the same group
---
drivers/irqchip/irq-imx-irqsteer.c | 76 +++++++++++++++++++++++++++++---------
1 file changed, 59 insertions(+), 17 deletions(-)
diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
index 67ed862..cc40039 100644
--- a/drivers/irqchip/irq-imx-irqsteer.c
+++ b/drivers/irqchip/irq-imx-irqsteer.c
@@ -10,6 +10,7 @@
#include <linux/irqchip/chained_irq.h>
#include <linux/irqdomain.h>
#include <linux/kernel.h>
+#include <linux/of_irq.h>
#include <linux/of_platform.h>
#include <linux/spinlock.h>
@@ -21,10 +22,13 @@
#define CHAN_MINTDIS(t) (CTRL_STRIDE_OFF(t, 3) + 0x4)
#define CHAN_MASTRSTAT(t) (CTRL_STRIDE_OFF(t, 3) + 0x8)
+#define CHAN_MAX_OUTPUT_INT 0x8
+
struct irqsteer_data {
void __iomem *regs;
struct clk *ipg_clk;
- int irq;
+ int irq[CHAN_MAX_OUTPUT_INT];
+ int irq_count;
raw_spinlock_t lock;
int reg_num;
int channel;
@@ -87,26 +91,45 @@ static const struct irq_domain_ops imx_irqsteer_domain_ops = {
.xlate = irq_domain_xlate_onecell,
};
+static int imx_irqsteer_get_hwirq_base(struct irqsteer_data *data, u32 irq)
+{
+ int i;
+
+ for (i = 0; i < data->irq_count; i++) {
+ if (data->irq[i] == irq)
+ break;
+ }
+
+ return i * 64;
+}
+
static void imx_irqsteer_irq_handler(struct irq_desc *desc)
{
struct irqsteer_data *data = irq_desc_get_handler_data(desc);
+ int hwirq;
int i;
chained_irq_enter(irq_desc_get_chip(desc), desc);
- for (i = 0; i < data->reg_num * 32; i += 32) {
- int idx = imx_irqsteer_get_reg_index(data, i);
+ hwirq = imx_irqsteer_get_hwirq_base(data, irq_desc_get_irq(desc));
+
+ for (i = 0; i < 2; i++) {
+ int idx = imx_irqsteer_get_reg_index(data, hwirq);
unsigned long irqmap;
int pos, virq;
+ if (hwirq >= data->reg_num * 32)
+ break;
+
irqmap = readl_relaxed(data->regs +
CHANSTATUS(idx, data->reg_num));
for_each_set_bit(pos, &irqmap, 32) {
- virq = irq_find_mapping(data->domain, pos + i);
+ virq = irq_find_mapping(data->domain, pos + hwirq);
if (virq)
generic_handle_irq(virq);
}
+ hwirq += 32;
}
chained_irq_exit(irq_desc_get_chip(desc), desc);
@@ -117,7 +140,8 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
struct device_node *np = pdev->dev.of_node;
struct irqsteer_data *data;
struct resource *res;
- int ret;
+ u32 irqs_num;
+ int i, ret;
data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
if (!data)
@@ -130,12 +154,6 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
return PTR_ERR(data->regs);
}
- data->irq = platform_get_irq(pdev, 0);
- if (data->irq <= 0) {
- dev_err(&pdev->dev, "failed to get irq\n");
- return -ENODEV;
- }
-
data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
if (IS_ERR(data->ipg_clk)) {
ret = PTR_ERR(data->ipg_clk);
@@ -146,11 +164,17 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
raw_spin_lock_init(&data->lock);
- of_property_read_u32(np, "fsl,num-irqs", &data->reg_num);
+ of_property_read_u32(np, "fsl,num-irqs", &irqs_num);
of_property_read_u32(np, "fsl,channel", &data->channel);
- /* one register bit map represents 32 input interrupts */
- data->reg_num /= 32;
+ /*
+ * There is one output irqs for each group of 64 inputs.
+ * One register bit map can represent 32 input interrupts.
+ */
+ data->irq_count = irqs_num / 64;
+ if (irqs_num % 64)
+ data->irq_count += 1;
+ data->reg_num = irqs_num / 32;
if (IS_ENABLED(CONFIG_PM_SLEEP)) {
data->saved_reg = devm_kzalloc(&pdev->dev,
@@ -177,8 +201,22 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
return -ENOMEM;
}
- irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler,
- data);
+ if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT) {
+ clk_disable_unprepare(data->ipg_clk);
+ return -EINVAL;
+ }
+
+ for (i = 0; i < data->irq_count; i++) {
+ data->irq[i] = irq_of_parse_and_map(np, i);
+ if (!data->irq[i]) {
+ clk_disable_unprepare(data->ipg_clk);
+ return -EINVAL;
+ }
+
+ irq_set_chained_handler_and_data(data->irq[i],
+ imx_irqsteer_irq_handler,
+ data);
+ }
platform_set_drvdata(pdev, data);
@@ -188,8 +226,12 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
static int imx_irqsteer_remove(struct platform_device *pdev)
{
struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev);
+ int i;
+
+ for (i = 0; i < irqsteer_data->irq_count; i++)
+ irq_set_chained_handler_and_data(irqsteer_data->irq[i],
+ NULL, NULL);
- irq_set_chained_handler_and_data(irqsteer_data->irq, NULL, NULL);
irq_domain_remove(irqsteer_data->domain);
clk_disable_unprepare(irqsteer_data->ipg_clk);
--
2.7.4
Am Mittwoch, den 30.01.2019, 13:05 +0000 schrieb Aisheng Dong:
> Not all 64 interrupts may be used in one group. e.g. most irqsteer in
> imx8qxp and imx8qm subsystems supports only 32 interrupts.
>
> As the IP integration parameters are Channel number and interrupts number,
> let's use fsl,num-irqs to represents how many interrupts supported
> by this irqsteer channel.
>
> > Cc: Marc Zyngier <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Lucas Stach <[email protected]>
> > Cc: Shawn Guo <[email protected]>
> Cc: [email protected]
> Signed-off-by: Dong Aisheng <[email protected]>
As the original fsl,irq-groups was born out of a misunderstanding of
the HW config options and I'm not aware of any users of the current
binding, I'm fine with merging this. I do think this warrants a
backport to stable though, so we don't end up with different bindings
in stable and current kernels.
Reviewed-by: Lucas Stach <[email protected]>
> ---
> ChangeLog:
> v1->v2:
> * change property name fsl,irqs-per-chan to fsl,num-irqs.
> ---
> .../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> index 45790ce..6d0a41b 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> @@ -16,8 +16,8 @@ Required properties:
> - #interrupt-cells: Specifies the number of cells needed to encode an
> interrupt source. The value must be 1.
> - fsl,channel: The output channel that all input IRQs should be steered into.
> -- fsl,irq-groups: Number of IRQ groups managed by this controller instance.
> - Each group manages 64 input interrupts.
> +- fsl,num-irqs: Number of input interrupts of this channel.
> + Should be multiple of 32 input interrupts and up to 512 interrupts.
>
> Example:
>
> @@ -28,7 +28,7 @@ Example:
> > clocks = <&clk IMX8MQ_CLK_DISP_APB_ROOT>;
> > clock-names = "ipg";
> > fsl,channel = <0>;
> > - fsl,irq-groups = <1>;
> > + fsl,num-irqs = <64>;
> > interrupt-controller;
> > #interrupt-cells = <1>;
> > };
Am Mittwoch, den 30.01.2019, 13:05 +0000 schrieb Aisheng Dong:
> One irqsteer channel can support up to 8 output interrupts.
>
> > Cc: Marc Zyngier <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Lucas Stach <[email protected]>
> > Cc: Shawn Guo <[email protected]>
> Cc: [email protected]
> > Signed-off-by: Dong Aisheng <[email protected]>
> ---
> ChangeLog:
> v1->v2:
> * remove one unnecessary note.
> ---
> .../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> index 6d0a41b..984ab92 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> @@ -6,8 +6,9 @@ Required properties:
> > - "fsl,imx8m-irqsteer"
> > - "fsl,imx-irqsteer"
> - reg: Physical base address and size of registers.
> -- interrupts: Should contain the parent interrupt line used to multiplex the
> - input interrupts.
> +- interrupts: Should contain the up to 8 parent interrupt lines used to
> + multiplex the input interrupts. They should be sepcified seqencially
Typos here, should be: "specified sequentially"
> + from output 0 to 7.
> - clocks: Should contain one clock for entry in clock-names
> see Documentation/devicetree/bindings/clock/clock-bindings.txt
> - clock-names:
Am Mittwoch, den 30.01.2019, 13:06 +0000 schrieb Aisheng Dong:
> One group can manage 64 interrupts by using two registers (e.g. STATUS/SET).
> However, the integrated irqsteer may support only 32 interrupts which
> needs only one register in a group. But the current driver assume there's
> a mininum of two registers in a group which result in a wrong register map
> for 32 interrupts per channel irqsteer. Let's use the reg_num caculated by
> interrupts per channel instead of irq_group to cover this case.
>
> > Cc: Marc Zyngier <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Lucas Stach <[email protected]>
> > Cc: Shawn Guo <[email protected]>
> Signed-off-by: Dong Aisheng <[email protected]>
Minor nit below, otherwise:
Reviewed-by: Lucas Stach <[email protected]>
> ---
> v1->v2:
> * The using of property name updated accordingly
> ---
> drivers/irqchip/irq-imx-irqsteer.c | 35 +++++++++++++++++++----------------
> 1 file changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 5b3f1d7..67ed862 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -13,7 +13,7 @@
> #include <linux/of_platform.h>
> #include <linux/spinlock.h>
>
> > -#define CTRL_STRIDE_OFF(_t, _r) (_t * 8 * _r)
> > +#define CTRL_STRIDE_OFF(_t, _r) (_t * 4 * _r)
> > #define CHANCTRL 0x0
> > #define CHANMASK(n, t) (CTRL_STRIDE_OFF(t, 0) + 0x4 * (n) + 0x4)
> > #define CHANSET(n, t) (CTRL_STRIDE_OFF(t, 1) + 0x4 * (n) + 0x4)
> @@ -26,7 +26,7 @@ struct irqsteer_data {
> > > struct clk *ipg_clk;
> > > int irq;
> > > raw_spinlock_t lock;
> > > - int irq_groups;
> > > + int reg_num;
> > > int channel;
> > > struct irq_domain *domain;
> > > u32 *saved_reg;
> @@ -35,7 +35,7 @@ struct irqsteer_data {
> static int imx_irqsteer_get_reg_index(struct irqsteer_data *data,
> > unsigned long irqnum)
> {
> > - return (data->irq_groups * 2 - irqnum / 32 - 1);
> > + return (data->reg_num - irqnum / 32 - 1);
> }
>
> static void imx_irqsteer_irq_unmask(struct irq_data *d)
> @@ -46,9 +46,9 @@ static void imx_irqsteer_irq_unmask(struct irq_data *d)
> > u32 val;
>
> > raw_spin_lock_irqsave(&data->lock, flags);
> > - val = readl_relaxed(data->regs + CHANMASK(idx, data->irq_groups));
> > + val = readl_relaxed(data->regs + CHANMASK(idx, data->reg_num));
> > val |= BIT(d->hwirq % 32);
> > - writel_relaxed(val, data->regs + CHANMASK(idx, data->irq_groups));
> > + writel_relaxed(val, data->regs + CHANMASK(idx, data->reg_num));
> > raw_spin_unlock_irqrestore(&data->lock, flags);
> }
>
> @@ -60,9 +60,9 @@ static void imx_irqsteer_irq_mask(struct irq_data *d)
> > u32 val;
>
> > raw_spin_lock_irqsave(&data->lock, flags);
> > - val = readl_relaxed(data->regs + CHANMASK(idx, data->irq_groups));
> > + val = readl_relaxed(data->regs + CHANMASK(idx, data->reg_num));
> > val &= ~BIT(d->hwirq % 32);
> > - writel_relaxed(val, data->regs + CHANMASK(idx, data->irq_groups));
> > + writel_relaxed(val, data->regs + CHANMASK(idx, data->reg_num));
> > raw_spin_unlock_irqrestore(&data->lock, flags);
> }
>
> @@ -94,13 +94,13 @@ static void imx_irqsteer_irq_handler(struct irq_desc *desc)
>
> > chained_irq_enter(irq_desc_get_chip(desc), desc);
>
> > - for (i = 0; i < data->irq_groups * 64; i += 32) {
> > + for (i = 0; i < data->reg_num * 32; i += 32) {
> > int idx = imx_irqsteer_get_reg_index(data, i);
> > unsigned long irqmap;
> > int pos, virq;
>
> > irqmap = readl_relaxed(data->regs +
> > - CHANSTATUS(idx, data->irq_groups));
> > + CHANSTATUS(idx, data->reg_num));
>
> > for_each_set_bit(pos, &irqmap, 32) {
> > virq = irq_find_mapping(data->domain, pos + i);
> @@ -146,12 +146,15 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>
> > raw_spin_lock_init(&data->lock);
>
> > - of_property_read_u32(np, "fsl,irq-groups", &data->irq_groups);
> > + of_property_read_u32(np, "fsl,num-irqs", &data->reg_num);
> > of_property_read_u32(np, "fsl,channel", &data->channel);
>
> > + /* one register bit map represents 32 input interrupts */
> > + data->reg_num /= 32;
> +
> > if (IS_ENABLED(CONFIG_PM_SLEEP)) {
> > data->saved_reg = devm_kzalloc(&pdev->dev,
> > - sizeof(u32) * data->irq_groups * 2,
> > + sizeof(u32) * data->reg_num,
> GFP_KERNEL);
Does this last parameter now fit on the line above?
> if (!data->saved_reg)
> > return -ENOMEM;
> @@ -166,7 +169,7 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> > /* steer all IRQs into configured channel */
> > writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
>
> > - data->domain = irq_domain_add_linear(np, data->irq_groups * 64,
> > + data->domain = irq_domain_add_linear(np, data->reg_num * 32,
> > &imx_irqsteer_domain_ops, data);
> > if (!data->domain) {
> > dev_err(&pdev->dev, "failed to create IRQ domain\n");
> @@ -199,9 +202,9 @@ static void imx_irqsteer_save_regs(struct irqsteer_data *data)
> {
> > int i;
>
> > - for (i = 0; i < data->irq_groups * 2; i++)
> > + for (i = 0; i < data->reg_num; i++)
> > data->saved_reg[i] = readl_relaxed(data->regs +
> > - CHANMASK(i, data->irq_groups));
> > + CHANMASK(i, data->reg_num));
> }
>
> static void imx_irqsteer_restore_regs(struct irqsteer_data *data)
> @@ -209,9 +212,9 @@ static void imx_irqsteer_restore_regs(struct irqsteer_data *data)
> > int i;
>
> > writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
> > - for (i = 0; i < data->irq_groups * 2; i++)
> > + for (i = 0; i < data->reg_num; i++)
> > writel_relaxed(data->saved_reg[i],
> > - data->regs + CHANMASK(i, data->irq_groups));
> > + data->regs + CHANMASK(i, data->reg_num));
> }
>
> static int imx_irqsteer_suspend(struct device *dev)
Am Mittwoch, den 30.01.2019, 13:06 +0000 schrieb Aisheng Dong:
> One irqsteer channel can support up to 8 output interrupts.
>
> > Cc: Marc Zyngier <[email protected]>
> > Cc: Lucas Stach <[email protected]>
> > Cc: Shawn Guo <[email protected]>
> > Signed-off-by: Dong Aisheng <[email protected]>
> ---
> ChangeLog:
> v1->v2:
> * calculate irq_count by fsl,num-irqs instead of parsing interrupts
> property from devicetree to match the input interrupts and outputs
> * improve output interrupt handler by searching only two registers
> withint the same group
> ---
> drivers/irqchip/irq-imx-irqsteer.c | 76 +++++++++++++++++++++++++++++---------
> 1 file changed, 59 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 67ed862..cc40039 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -10,6 +10,7 @@
> #include <linux/irqchip/chained_irq.h>
> #include <linux/irqdomain.h>
> #include <linux/kernel.h>
> +#include <linux/of_irq.h>
> #include <linux/of_platform.h>
> #include <linux/spinlock.h>
>
> @@ -21,10 +22,13 @@
> > #define CHAN_MINTDIS(t) (CTRL_STRIDE_OFF(t, 3) + 0x4)
> > #define CHAN_MASTRSTAT(t) (CTRL_STRIDE_OFF(t, 3) + 0x8)
>
> > +#define CHAN_MAX_OUTPUT_INT 0x8
> +
> struct irqsteer_data {
> > > void __iomem *regs;
> > > struct clk *ipg_clk;
> > > - int irq;
> > > + int irq[CHAN_MAX_OUTPUT_INT];
> > > + int irq_count;
> > > raw_spinlock_t lock;
> > > int reg_num;
> > > int channel;
> @@ -87,26 +91,45 @@ static const struct irq_domain_ops imx_irqsteer_domain_ops = {
> > > .xlate = irq_domain_xlate_onecell,
> };
>
> +static int imx_irqsteer_get_hwirq_base(struct irqsteer_data *data, u32 irq)
> +{
> > + int i;
> +
> > + for (i = 0; i < data->irq_count; i++) {
> > + if (data->irq[i] == irq)
> + break;
return i * 64; here...
> + }
> +
> + return i * 64;
... and -EINVAL or something here, so we don't return a out of bounds
hwirq base if the loop ever doesn't match something?
> +}
> +
> static void imx_irqsteer_irq_handler(struct irq_desc *desc)
> {
> > struct irqsteer_data *data = irq_desc_get_handler_data(desc);
> > + int hwirq;
> > int i;
>
> > chained_irq_enter(irq_desc_get_chip(desc), desc);
>
> > - for (i = 0; i < data->reg_num * 32; i += 32) {
> > - int idx = imx_irqsteer_get_reg_index(data, i);
> > + hwirq = imx_irqsteer_get_hwirq_base(data, irq_desc_get_irq(desc));
> +
> > + for (i = 0; i < 2; i++) {
> > + int idx = imx_irqsteer_get_reg_index(data, hwirq);
> > unsigned long irqmap;
> > int pos, virq;
>
> > + if (hwirq >= data->reg_num * 32)
> > + break;
> +
> > irqmap = readl_relaxed(data->regs +
> > CHANSTATUS(idx, data->reg_num));
>
> > for_each_set_bit(pos, &irqmap, 32) {
> > - virq = irq_find_mapping(data->domain, pos + i);
> + virq = irq_find_mapping(data->domain, pos + hwirq);
The irq index calculation need to be "pos + i * 32 + hwirq", otherwise
this will map to the wrong virqs for the second register in each group.
> if (virq)
> > generic_handle_irq(virq);
> > }
> + hwirq += 32;
Could be folded into the loop head.
> }
>
> > chained_irq_exit(irq_desc_get_chip(desc), desc);
> @@ -117,7 +140,8 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> > struct device_node *np = pdev->dev.of_node;
> > struct irqsteer_data *data;
> > struct resource *res;
> > - int ret;
> > + u32 irqs_num;
> > + int i, ret;
>
> > data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > if (!data)
> @@ -130,12 +154,6 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> > return PTR_ERR(data->regs);
> > }
>
> > - data->irq = platform_get_irq(pdev, 0);
> > - if (data->irq <= 0) {
> > - dev_err(&pdev->dev, "failed to get irq\n");
> > - return -ENODEV;
> > - }
> -
> > data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> > if (IS_ERR(data->ipg_clk)) {
> > ret = PTR_ERR(data->ipg_clk);
> @@ -146,11 +164,17 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>
> > raw_spin_lock_init(&data->lock);
>
> > - of_property_read_u32(np, "fsl,num-irqs", &data->reg_num);
> > + of_property_read_u32(np, "fsl,num-irqs", &irqs_num);
> > of_property_read_u32(np, "fsl,channel", &data->channel);
>
> > - /* one register bit map represents 32 input interrupts */
> > - data->reg_num /= 32;
> > + /*
> + * There is one output irqs for each group of 64 inputs.
"irq", singular.
> + * One register bit map can represent 32 input interrupts.
> > + */
> > + data->irq_count = irqs_num / 64;
> > + if (irqs_num % 64)
> + data->irq_count += 1;
This is a weird way of writing DIV_ROUND_UP.
> + data->reg_num = irqs_num / 32;
>
> > if (IS_ENABLED(CONFIG_PM_SLEEP)) {
> > data->saved_reg = devm_kzalloc(&pdev->dev,
> @@ -177,8 +201,22 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> > return -ENOMEM;
> > }
>
> > - irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler,
> > - data);
> > + if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT) {
> > + clk_disable_unprepare(data->ipg_clk);
> > + return -EINVAL;
> > + }
> +
> > + for (i = 0; i < data->irq_count; i++) {
> > + data->irq[i] = irq_of_parse_and_map(np, i);
> > + if (!data->irq[i]) {
> > + clk_disable_unprepare(data->ipg_clk);
> + return -EINVAL;
With a lot of failure paths now replicating the clk_disable_unprepare,
return error, I think this warrants a common cleanup path that all
those paths could reach via simple goto.
> + }
> +
> > + irq_set_chained_handler_and_data(data->irq[i],
> > + imx_irqsteer_irq_handler,
> > + data);
> > + }
>
> > platform_set_drvdata(pdev, data);
>
> @@ -188,8 +226,12 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> static int imx_irqsteer_remove(struct platform_device *pdev)
> {
> > struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev);
> > + int i;
> +
> > + for (i = 0; i < irqsteer_data->irq_count; i++)
> > + irq_set_chained_handler_and_data(irqsteer_data->irq[i],
> > + NULL, NULL);
>
> > - irq_set_chained_handler_and_data(irqsteer_data->irq, NULL, NULL);
> > irq_domain_remove(irqsteer_data->domain);
>
> > clk_disable_unprepare(irqsteer_data->ipg_clk);
On Wed, Jan 30, 2019 at 9:21 PM Lucas Stach <[email protected]> wrote:
>
> Am Mittwoch, den 30.01.2019, 13:05 +0000 schrieb Aisheng Dong:
> > One irqsteer channel can support up to 8 output interrupts.
> >
> > > Cc: Marc Zyngier <[email protected]>
> > > Cc: Rob Herring <[email protected]>
> > > Cc: Lucas Stach <[email protected]>
> > > Cc: Shawn Guo <[email protected]>
> > Cc: [email protected]
> > > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
> > ChangeLog:
> > v1->v2:
> > * remove one unnecessary note.
> > ---
> > .../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > index 6d0a41b..984ab92 100644
> > --- a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > @@ -6,8 +6,9 @@ Required properties:
> > > - "fsl,imx8m-irqsteer"
> > > - "fsl,imx-irqsteer"
> > - reg: Physical base address and size of registers.
> > -- interrupts: Should contain the parent interrupt line used to multiplex the
> > - input interrupts.
> > +- interrupts: Should contain the up to 8 parent interrupt lines used to
> > + multiplex the input interrupts. They should be sepcified seqencially
>
> Typos here, should be: "specified sequentially"
>
Nice catch. Will update in new version.
Thanks
Regards
Dong Aisheng
> > + from output 0 to 7.
> > - clocks: Should contain one clock for entry in clock-names
> > see Documentation/devicetree/bindings/clock/clock-bindings.txt
> > - clock-names:
On Wed, Jan 30, 2019 at 9:33 PM Lucas Stach <[email protected]> wrote:
>
> Am Mittwoch, den 30.01.2019, 13:06 +0000 schrieb Aisheng Dong:
> > One irqsteer channel can support up to 8 output interrupts.
> >
> > > Cc: Marc Zyngier <[email protected]>
> > > Cc: Lucas Stach <[email protected]>
> > > Cc: Shawn Guo <[email protected]>
> > > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
> > ChangeLog:
> > v1->v2:
> > * calculate irq_count by fsl,num-irqs instead of parsing interrupts
> > property from devicetree to match the input interrupts and outputs
> > * improve output interrupt handler by searching only two registers
> > withint the same group
> > ---
> > drivers/irqchip/irq-imx-irqsteer.c | 76 +++++++++++++++++++++++++++++---------
> > 1 file changed, 59 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> > index 67ed862..cc40039 100644
> > --- a/drivers/irqchip/irq-imx-irqsteer.c
> > +++ b/drivers/irqchip/irq-imx-irqsteer.c
> > @@ -10,6 +10,7 @@
> > #include <linux/irqchip/chained_irq.h>
> > #include <linux/irqdomain.h>
> > #include <linux/kernel.h>
> > +#include <linux/of_irq.h>
> > #include <linux/of_platform.h>
> > #include <linux/spinlock.h>
> >
> > @@ -21,10 +22,13 @@
> > > #define CHAN_MINTDIS(t) (CTRL_STRIDE_OFF(t, 3) + 0x4)
> > > #define CHAN_MASTRSTAT(t) (CTRL_STRIDE_OFF(t, 3) + 0x8)
> >
> > > +#define CHAN_MAX_OUTPUT_INT 0x8
> > +
> > struct irqsteer_data {
> > > > void __iomem *regs;
> > > > struct clk *ipg_clk;
> > > > - int irq;
> > > > + int irq[CHAN_MAX_OUTPUT_INT];
> > > > + int irq_count;
> > > > raw_spinlock_t lock;
> > > > int reg_num;
> > > > int channel;
> > @@ -87,26 +91,45 @@ static const struct irq_domain_ops imx_irqsteer_domain_ops = {
> > > > .xlate = irq_domain_xlate_onecell,
> > };
> >
> > +static int imx_irqsteer_get_hwirq_base(struct irqsteer_data *data, u32 irq)
> > +{
> > > + int i;
> > +
> > > + for (i = 0; i < data->irq_count; i++) {
> > > + if (data->irq[i] == irq)
> > + break;
>
> return i * 64; here...
> > + }
> > +
> > + return i * 64;
>
> ... and -EINVAL or something here, so we don't return a out of bounds
> hwirq base if the loop ever doesn't match something?
>
Good suggestion, will add it.
> > +}
> > +
> > static void imx_irqsteer_irq_handler(struct irq_desc *desc)
> > {
> > > struct irqsteer_data *data = irq_desc_get_handler_data(desc);
> > > + int hwirq;
> > > int i;
> >
> > > chained_irq_enter(irq_desc_get_chip(desc), desc);
> >
> > > - for (i = 0; i < data->reg_num * 32; i += 32) {
> > > - int idx = imx_irqsteer_get_reg_index(data, i);
> > > + hwirq = imx_irqsteer_get_hwirq_base(data, irq_desc_get_irq(desc));
> > +
> > > + for (i = 0; i < 2; i++) {
> > > + int idx = imx_irqsteer_get_reg_index(data, hwirq);
> > > unsigned long irqmap;
> > > int pos, virq;
> >
> > > + if (hwirq >= data->reg_num * 32)
> > > + break;
> > +
> > > irqmap = readl_relaxed(data->regs +
> > > CHANSTATUS(idx, data->reg_num));
> >
> > > for_each_set_bit(pos, &irqmap, 32) {
> > > - virq = irq_find_mapping(data->domain, pos + i);
> > + virq = irq_find_mapping(data->domain, pos + hwirq);
>
> The irq index calculation need to be "pos + i * 32 + hwirq", otherwise
> this will map to the wrong virqs for the second register in each group.
>
For second register map, hwirq will plus 32 in next round.
So i can't see this will map a wrong virqs.
And it looks to me ""pos + i * 32 + hwirq" is equal to "hwirq + 32".
Am i missed something?
> > if (virq)
> > > generic_handle_irq(virq);
> > > }
> > + hwirq += 32;
>
> Could be folded into the loop head.
>
You mean “for (i = 0; i < 2; i++, hwirq +=32)” ?
I feel that's not quite necessary.
> > }
> >
> > > chained_irq_exit(irq_desc_get_chip(desc), desc);
> > @@ -117,7 +140,8 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> > > struct device_node *np = pdev->dev.of_node;
> > > struct irqsteer_data *data;
> > > struct resource *res;
> > > - int ret;
> > > + u32 irqs_num;
> > > + int i, ret;
> >
> > > data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > > if (!data)
> > @@ -130,12 +154,6 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> > > return PTR_ERR(data->regs);
> > > }
> >
> > > - data->irq = platform_get_irq(pdev, 0);
> > > - if (data->irq <= 0) {
> > > - dev_err(&pdev->dev, "failed to get irq\n");
> > > - return -ENODEV;
> > > - }
> > -
> > > data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> > > if (IS_ERR(data->ipg_clk)) {
> > > ret = PTR_ERR(data->ipg_clk);
> > @@ -146,11 +164,17 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> >
> > > raw_spin_lock_init(&data->lock);
> >
> > > - of_property_read_u32(np, "fsl,num-irqs", &data->reg_num);
> > > + of_property_read_u32(np, "fsl,num-irqs", &irqs_num);
> > > of_property_read_u32(np, "fsl,channel", &data->channel);
> >
> > > - /* one register bit map represents 32 input interrupts */
> > > - data->reg_num /= 32;
> > > + /*
> > + * There is one output irqs for each group of 64 inputs.
>
> "irq", singular.
>
Got it
> > + * One register bit map can represent 32 input interrupts.
> > > + */
> > > + data->irq_count = irqs_num / 64;
> > > + if (irqs_num % 64)
> > + data->irq_count += 1;
>
> This is a weird way of writing DIV_ROUND_UP.
>
Good suggestion
> > + data->reg_num = irqs_num / 32;
> >
> > > if (IS_ENABLED(CONFIG_PM_SLEEP)) {
> > > data->saved_reg = devm_kzalloc(&pdev->dev,
> > @@ -177,8 +201,22 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> > > return -ENOMEM;
> > > }
> >
> > > - irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler,
> > > - data);
> > > + if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT) {
> > > + clk_disable_unprepare(data->ipg_clk);
> > > + return -EINVAL;
> > > + }
> > +
> > > + for (i = 0; i < data->irq_count; i++) {
> > > + data->irq[i] = irq_of_parse_and_map(np, i);
> > > + if (!data->irq[i]) {
> > > + clk_disable_unprepare(data->ipg_clk);
> > + return -EINVAL;
>
> With a lot of failure paths now replicating the clk_disable_unprepare,
> return error, I think this warrants a common cleanup path that all
> those paths could reach via simple goto.
>
Sound goods to me
Regards
Dong Aisheng
> > + }
> > +
> > > + irq_set_chained_handler_and_data(data->irq[i],
> > > + imx_irqsteer_irq_handler,
> > > + data);
> > > + }
> >
> > > platform_set_drvdata(pdev, data);
> >
> > @@ -188,8 +226,12 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
> > static int imx_irqsteer_remove(struct platform_device *pdev)
> > {
> > > struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev);
> > > + int i;
> > +
> > > + for (i = 0; i < irqsteer_data->irq_count; i++)
> > > + irq_set_chained_handler_and_data(irqsteer_data->irq[i],
> > > + NULL, NULL);
> >
> > > - irq_set_chained_handler_and_data(irqsteer_data->irq, NULL, NULL);
> > > irq_domain_remove(irqsteer_data->domain);
> >
> > > clk_disable_unprepare(irqsteer_data->ipg_clk);
Am Mittwoch, den 30.01.2019, 22:03 +0800 schrieb Dong Aisheng:
> > On Wed, Jan 30, 2019 at 9:33 PM Lucas Stach <[email protected]> wrote:
> >
> > Am Mittwoch, den 30.01.2019, 13:06 +0000 schrieb Aisheng Dong:
> > > One irqsteer channel can support up to 8 output interrupts.
> > >
> > > > > > > > Cc: Marc Zyngier <[email protected]>
> > > > > > > > Cc: Lucas Stach <[email protected]>
> > > > > > > > Cc: Shawn Guo <[email protected]>
> > > > Signed-off-by: Dong Aisheng <[email protected]>
> > >
> > > ---
> > > ChangeLog:
> > > v1->v2:
> > > * calculate irq_count by fsl,num-irqs instead of parsing interrupts
> > > property from devicetree to match the input interrupts and outputs
> > > * improve output interrupt handler by searching only two registers
> > > withint the same group
> > > ---
> > > drivers/irqchip/irq-imx-irqsteer.c | 76 +++++++++++++++++++++++++++++---------
> > > 1 file changed, 59 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> > > index 67ed862..cc40039 100644
> > > --- a/drivers/irqchip/irq-imx-irqsteer.c
> > > +++ b/drivers/irqchip/irq-imx-irqsteer.c
> > > @@ -10,6 +10,7 @@
> > > #include <linux/irqchip/chained_irq.h>
> > > #include <linux/irqdomain.h>
> > > #include <linux/kernel.h>
> > > +#include <linux/of_irq.h>
> > > #include <linux/of_platform.h>
> > > #include <linux/spinlock.h>
> > >
> > > @@ -21,10 +22,13 @@
> > > > #define CHAN_MINTDIS(t) (CTRL_STRIDE_OFF(t, 3) + 0x4)
> > > > #define CHAN_MASTRSTAT(t) (CTRL_STRIDE_OFF(t, 3) + 0x8)
> > > > +#define CHAN_MAX_OUTPUT_INT 0x8
> > >
> > > +
> > > struct irqsteer_data {
> > > > > void __iomem *regs;
> > > > > struct clk *ipg_clk;
> > > > > - int irq;
> > > > > + int irq[CHAN_MAX_OUTPUT_INT];
> > > > > + int irq_count;
> > > > > raw_spinlock_t lock;
> > > > > int reg_num;
> > > > > int channel;
> > >
> > > @@ -87,26 +91,45 @@ static const struct irq_domain_ops imx_irqsteer_domain_ops = {
> > > > > .xlate = irq_domain_xlate_onecell,
> > >
> > > };
> > >
> > > +static int imx_irqsteer_get_hwirq_base(struct irqsteer_data *data, u32 irq)
> > > +{
> > > > + int i;
> > >
> > > +
> > > > + for (i = 0; i < data->irq_count; i++) {
> > > > + if (data->irq[i] == irq)
> > >
> > > + break;
> >
> > return i * 64; here...
> > > + }
> > > +
> > > + return i * 64;
> >
> > ... and -EINVAL or something here, so we don't return a out of bounds
> > hwirq base if the loop ever doesn't match something?
> >
>
> Good suggestion, will add it.
>
> > > +}
> > > +
> > > static void imx_irqsteer_irq_handler(struct irq_desc *desc)
> > > {
> > > > struct irqsteer_data *data = irq_desc_get_handler_data(desc);
> > > > + int hwirq;
> > > > int i;
> > > > chained_irq_enter(irq_desc_get_chip(desc), desc);
> > > > - for (i = 0; i < data->reg_num * 32; i += 32) {
> > > > - int idx = imx_irqsteer_get_reg_index(data, i);
> > > > + hwirq = imx_irqsteer_get_hwirq_base(data, irq_desc_get_irq(desc));
> > >
> > > +
> > > > + for (i = 0; i < 2; i++) {
> > > > + int idx = imx_irqsteer_get_reg_index(data, hwirq);
> > > > unsigned long irqmap;
> > > > int pos, virq;
> > > > + if (hwirq >= data->reg_num * 32)
> > > > + break;
> > >
> > > +
> > > > irqmap = readl_relaxed(data->regs +
> > > > CHANSTATUS(idx, data->reg_num));
> > > > for_each_set_bit(pos, &irqmap, 32) {
> > > > - virq = irq_find_mapping(data->domain, pos + i);
> > >
> > > + virq = irq_find_mapping(data->domain, pos + hwirq);
> >
> > The irq index calculation need to be "pos + i * 32 + hwirq", otherwise
> > this will map to the wrong virqs for the second register in each group.
> >
>
> For second register map, hwirq will plus 32 in next round.
> So i can't see this will map a wrong virqs.
> And it looks to me ""pos + i * 32 + hwirq" is equal to "hwirq + 32".
> Am i missed something?
You are right, I forgot about the hwirq being incremented in the loop
when writing this comment.
> > > if (virq)
> > > > generic_handle_irq(virq);
> > > > }
> > >
> > > + hwirq += 32;
> >
> > Could be folded into the loop head.
> >
>
> You mean “for (i = 0; i < 2; i++, hwirq +=32)” ?
> I feel that's not quite necessary.
I personally find that quite a bit clearer than incrementing the loop
variables at different spots. And I probably wouldn't have missed hwirq
being incremented in the loop if I had seen it in the head.
Regards,
Lucas
On Wed, Jan 30, 2019 at 9:23 PM Lucas Stach <[email protected]> wrote:
[...]
> > > + /* one register bit map represents 32 input interrupts */
> > > + data->reg_num /= 32;
> > +
> > > if (IS_ENABLED(CONFIG_PM_SLEEP)) {
> > > data->saved_reg = devm_kzalloc(&pdev->dev,
> > > - sizeof(u32) * data->irq_groups * 2,
> > > + sizeof(u32) * data->reg_num,
> > GFP_KERNEL);
>
> Does this last parameter now fit on the line above?
>
No, 81 now. :)
Regards
Dong Aisheng