* [PATCH] irqchip/irq-csky-mpintc: Add triger type and priority setting
@ 2019-01-15 16:36 guoren
2019-01-17 17:17 ` Marc Zyngier
0 siblings, 1 reply; 5+ messages in thread
From: guoren @ 2019-01-15 16:36 UTC (permalink / raw)
To: tglx, jason, marc.zyngier, robh+dt, mark.rutland
Cc: linux-kernel, devicetree, guoren, Guo Ren
From: Guo Ren <ren_guo@c-sky.com>
Support 4 triger types:
- IRQ_TYPE_LEVEL_HIGH
- IRQ_TYPE_LEVEL_LOW
- IRQ_TYPE_EDGE_RISING
- IRQ_TYPE_EDGE_FALLING
Support 0-255 priority setting for each irq.
Signed-off-by: Guo Ren <ren_guo@c-sky.com>
---
.../bindings/interrupt-controller/csky,mpintc.txt | 24 ++++++-
drivers/irqchip/irq-csky-mpintc.c | 78 +++++++++++++++++++++-
2 files changed, 99 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt b/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
index ab921f1..364b789 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
@@ -11,6 +11,14 @@ Interrupt number definition:
16-31 : private irq, and we use 16 as the co-processor timer.
31-1024: common irq for soc ip.
+Interrupt triger mode:
+ IRQ_TYPE_LEVEL_HIGH (default)
+ IRQ_TYPE_LEVEL_LOW
+ IRQ_TYPE_EDGE_RISING
+ IRQ_TYPE_EDGE_FALLING
+
+Interrupt priority range: 0-255
+
=============================
intc node bindings definition
=============================
@@ -26,7 +34,7 @@ intc node bindings definition
- #interrupt-cells
Usage: required
Value type: <u32>
- Definition: must be <1>
+ Definition: could be <1> or <2>
- interrupt-controller:
Usage: required
@@ -35,6 +43,18 @@ Examples:
intc: interrupt-controller {
compatible = "csky,mpintc";
- #interrupt-cells = <1>;
+ #interrupt-cells = <2>;
interrupt-controller;
};
+
+ 0: device-example {
+ ...
+ interrupts = <33 IRQ_TYPE_EDGE_RISING>;
+ interrupt-parent = <&intc>;
+ };
+
+ 1: device-example {
+ ...
+ interrupts = <34 ((priority << 4) | IRQ_TYPE_EDGE_RISING)>;
+ interrupt-parent = <&intc>;
+ };
diff --git a/drivers/irqchip/irq-csky-mpintc.c b/drivers/irqchip/irq-csky-mpintc.c
index c67c961..9edc6d3 100644
--- a/drivers/irqchip/irq-csky-mpintc.c
+++ b/drivers/irqchip/irq-csky-mpintc.c
@@ -29,9 +29,12 @@ static void __iomem *INTCL_base;
#define INTCG_ICTLR 0x0
#define INTCG_CICFGR 0x100
+#define INTCG_CIPRTR 0x200
#define INTCG_CIDSTR 0x1000
#define INTCL_PICTLR 0x0
+#define INTCL_CFGR 0x14
+#define INTCL_PRTR 0x20
#define INTCL_SIGR 0x60
#define INTCL_HPPIR 0x68
#define INTCL_RDYIR 0x6c
@@ -73,6 +76,78 @@ static void csky_mpintc_eoi(struct irq_data *d)
writel_relaxed(d->hwirq, reg_base + INTCL_CACR);
}
+static int csky_mpintc_set_type(struct irq_data *d, unsigned int type)
+{
+ unsigned int priority, triger;
+ unsigned int offset, bit_offset;
+ void __iomem *reg_base;
+
+ /*
+ * type Bit field: | 32 - 12 | 11 - 4 | 3 - 0 |
+ * reserved priority triger type
+ */
+ triger = type & IRQ_TYPE_SENSE_MASK;
+ priority = (type >> 4) & 0xff;
+
+ switch (triger) {
+ case IRQ_TYPE_LEVEL_HIGH:
+ triger = 0;
+ break;
+ case IRQ_TYPE_LEVEL_LOW:
+ triger = 1;
+ break;
+ case IRQ_TYPE_EDGE_RISING:
+ triger = 2;
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ triger = 3;
+ break;
+ default:
+ triger = 0;
+ break;
+ }
+
+ if (d->hwirq < COMM_IRQ_BASE) {
+ reg_base = this_cpu_read(intcl_reg);
+
+ if (triger) {
+ offset = ((d->hwirq * 2) / 32) * 4;
+ bit_offset = (d->hwirq * 2) % 32;
+
+ writel_relaxed(triger << bit_offset,
+ reg_base + INTCL_CFGR + offset);
+ }
+
+ if (priority) {
+ offset = ((d->hwirq * 8) / 32) * 4;
+ bit_offset = (d->hwirq * 8) % 32;
+
+ writel_relaxed(priority << bit_offset,
+ reg_base + INTCL_PRTR + offset);
+ }
+ } else {
+ reg_base = INTCG_base;
+
+ if (triger) {
+ offset = (((d->hwirq - COMM_IRQ_BASE) * 2) / 32) * 4;
+ bit_offset = ((d->hwirq - COMM_IRQ_BASE) * 2) % 32;
+
+ writel_relaxed(triger << bit_offset,
+ reg_base + INTCG_CICFGR + offset);
+ }
+
+ if (priority) {
+ offset = (((d->hwirq - COMM_IRQ_BASE) * 8) / 32) * 4;
+ bit_offset = ((d->hwirq - COMM_IRQ_BASE) * 8) % 32;
+
+ writel_relaxed(priority << bit_offset,
+ reg_base + INTCG_CIPRTR + offset);
+ }
+ }
+
+ return 0;
+}
+
#ifdef CONFIG_SMP
static int csky_irq_set_affinity(struct irq_data *d,
const struct cpumask *mask_val,
@@ -105,6 +180,7 @@ static struct irq_chip csky_irq_chip = {
.irq_eoi = csky_mpintc_eoi,
.irq_enable = csky_mpintc_enable,
.irq_disable = csky_mpintc_disable,
+ .irq_set_type = csky_mpintc_set_type,
#ifdef CONFIG_SMP
.irq_set_affinity = csky_irq_set_affinity,
#endif
@@ -127,7 +203,7 @@ static int csky_irqdomain_map(struct irq_domain *d, unsigned int irq,
static const struct irq_domain_ops csky_irqdomain_ops = {
.map = csky_irqdomain_map,
- .xlate = irq_domain_xlate_onecell,
+ .xlate = irq_domain_xlate_onetwocell,
};
#ifdef CONFIG_SMP
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] irqchip/irq-csky-mpintc: Add triger type and priority setting
2019-01-15 16:36 [PATCH] irqchip/irq-csky-mpintc: Add triger type and priority setting guoren
@ 2019-01-17 17:17 ` Marc Zyngier
2019-01-18 6:28 ` Guo Ren
0 siblings, 1 reply; 5+ messages in thread
From: Marc Zyngier @ 2019-01-17 17:17 UTC (permalink / raw)
To: guoren, tglx, jason, robh+dt, mark.rutland
Cc: linux-kernel, devicetree, Guo Ren
Hi Guo,
On 15/01/2019 16:36, guoren@kernel.org wrote:
> From: Guo Ren <ren_guo@c-sky.com>
>
> Support 4 triger types:
> - IRQ_TYPE_LEVEL_HIGH
> - IRQ_TYPE_LEVEL_LOW
> - IRQ_TYPE_EDGE_RISING
> - IRQ_TYPE_EDGE_FALLING
>
> Support 0-255 priority setting for each irq.
>
> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> ---
> .../bindings/interrupt-controller/csky,mpintc.txt | 24 ++++++-
> drivers/irqchip/irq-csky-mpintc.c | 78 +++++++++++++++++++++-
> 2 files changed, 99 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt b/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
> index ab921f1..364b789 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
> @@ -11,6 +11,14 @@ Interrupt number definition:
> 16-31 : private irq, and we use 16 as the co-processor timer.
> 31-1024: common irq for soc ip.
>
> +Interrupt triger mode:
> + IRQ_TYPE_LEVEL_HIGH (default)
> + IRQ_TYPE_LEVEL_LOW
> + IRQ_TYPE_EDGE_RISING
> + IRQ_TYPE_EDGE_FALLING
> +
> +Interrupt priority range: 0-255
> +
> =============================
> intc node bindings definition
> =============================
> @@ -26,7 +34,7 @@ intc node bindings definition
> - #interrupt-cells
> Usage: required
> Value type: <u32>
> - Definition: must be <1>
> + Definition: could be <1> or <2>
> - interrupt-controller:
> Usage: required
>
> @@ -35,6 +43,18 @@ Examples:
>
> intc: interrupt-controller {
> compatible = "csky,mpintc";
> - #interrupt-cells = <1>;
> + #interrupt-cells = <2>;
> interrupt-controller;
> };
> +
> + 0: device-example {
> + ...
> + interrupts = <33 IRQ_TYPE_EDGE_RISING>;
> + interrupt-parent = <&intc>;
> + };
> +
> + 1: device-example {
> + ...
> + interrupts = <34 ((priority << 4) | IRQ_TYPE_EDGE_RISING)>;
> + interrupt-parent = <&intc>;
> + };
> diff --git a/drivers/irqchip/irq-csky-mpintc.c b/drivers/irqchip/irq-csky-mpintc.c
> index c67c961..9edc6d3 100644
> --- a/drivers/irqchip/irq-csky-mpintc.c
> +++ b/drivers/irqchip/irq-csky-mpintc.c
> @@ -29,9 +29,12 @@ static void __iomem *INTCL_base;
>
> #define INTCG_ICTLR 0x0
> #define INTCG_CICFGR 0x100
> +#define INTCG_CIPRTR 0x200
> #define INTCG_CIDSTR 0x1000
>
> #define INTCL_PICTLR 0x0
> +#define INTCL_CFGR 0x14
> +#define INTCL_PRTR 0x20
> #define INTCL_SIGR 0x60
> #define INTCL_HPPIR 0x68
> #define INTCL_RDYIR 0x6c
> @@ -73,6 +76,78 @@ static void csky_mpintc_eoi(struct irq_data *d)
> writel_relaxed(d->hwirq, reg_base + INTCL_CACR);
> }
>
> +static int csky_mpintc_set_type(struct irq_data *d, unsigned int type)
> +{
> + unsigned int priority, triger;
nit: s/triger/trigger/ everywhere.
> + unsigned int offset, bit_offset;
> + void __iomem *reg_base;
> +
> + /*
> + * type Bit field: | 32 - 12 | 11 - 4 | 3 - 0 |
> + * reserved priority triger type
> + */
> + triger = type & IRQ_TYPE_SENSE_MASK;
> + priority = (type >> 4) & 0xff;
Definitely not. The Linux API to set the trigger does not carry any
priority information, nor should it. Priorities should be set
statically, and no driver should ever be able to change it.
> +
> + switch (triger) {
> + case IRQ_TYPE_LEVEL_HIGH:
> + triger = 0;
> + break;
> + case IRQ_TYPE_LEVEL_LOW:
> + triger = 1;
> + break;
> + case IRQ_TYPE_EDGE_RISING:
> + triger = 2;
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + triger = 3;
Can you define some macros that represent these magic values?
> + break;
> + default:
> + triger = 0;
> + break;
If you get an invalid combination, you shouldn't blindly accept it, but
instead return an error.
> + }
> +
> + if (d->hwirq < COMM_IRQ_BASE) {
> + reg_base = this_cpu_read(intcl_reg);
Are you guaranteed to be in a non-preemptible section here? I can see
things going wrong if not.
> +
> + if (triger) {
> + offset = ((d->hwirq * 2) / 32) * 4;
> + bit_offset = (d->hwirq * 2) % 32;
This needs to be turned into a set of macros so that the non-percpu code
can reuse it.
> +
> + writel_relaxed(triger << bit_offset,
> + reg_base + INTCL_CFGR + offset);
> + }
> +
> + if (priority) {
> + offset = ((d->hwirq * 8) / 32) * 4;
> + bit_offset = (d->hwirq * 8) % 32;
> +
> + writel_relaxed(priority << bit_offset,
> + reg_base + INTCL_PRTR + offset);
> + }
And this should only be set at boot time.
> + } else {
> + reg_base = INTCG_base;
> +
> + if (triger) {
> + offset = (((d->hwirq - COMM_IRQ_BASE) * 2) / 32) * 4;
> + bit_offset = ((d->hwirq - COMM_IRQ_BASE) * 2) % 32;
> +
> + writel_relaxed(triger << bit_offset,
> + reg_base + INTCG_CICFGR + offset);
> + }
> +
> + if (priority) {
> + offset = (((d->hwirq - COMM_IRQ_BASE) * 8) / 32) * 4;
> + bit_offset = ((d->hwirq - COMM_IRQ_BASE) * 8) % 32;
> +
> + writel_relaxed(priority << bit_offset,
> + reg_base + INTCG_CIPRTR + offset);
> + }
> + }
Same as above.
> +
> + return 0;
> +}
> +
> #ifdef CONFIG_SMP
> static int csky_irq_set_affinity(struct irq_data *d,
> const struct cpumask *mask_val,
> @@ -105,6 +180,7 @@ static struct irq_chip csky_irq_chip = {
> .irq_eoi = csky_mpintc_eoi,
> .irq_enable = csky_mpintc_enable,
> .irq_disable = csky_mpintc_disable,
> + .irq_set_type = csky_mpintc_set_type,
> #ifdef CONFIG_SMP
> .irq_set_affinity = csky_irq_set_affinity,
> #endif
> @@ -127,7 +203,7 @@ static int csky_irqdomain_map(struct irq_domain *d, unsigned int irq,
>
> static const struct irq_domain_ops csky_irqdomain_ops = {
> .map = csky_irqdomain_map,
> - .xlate = irq_domain_xlate_onecell,
> + .xlate = irq_domain_xlate_onetwocell,
> };
>
> #ifdef CONFIG_SMP
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] irqchip/irq-csky-mpintc: Add triger type and priority setting
2019-01-17 17:17 ` Marc Zyngier
@ 2019-01-18 6:28 ` Guo Ren
2019-01-18 9:32 ` Marc Zyngier
0 siblings, 1 reply; 5+ messages in thread
From: Guo Ren @ 2019-01-18 6:28 UTC (permalink / raw)
To: Marc Zyngier
Cc: tglx, jason, robh+dt, mark.rutland, linux-kernel, devicetree, Guo Ren
Thx Marc,
On Thu, Jan 17, 2019 at 05:17:45PM +0000, Marc Zyngier wrote:
> Hi Guo,
>
> On 15/01/2019 16:36, guoren@kernel.org wrote:
> > From: Guo Ren <ren_guo@c-sky.com>
> >
> > Support 4 triger types:
> > - IRQ_TYPE_LEVEL_HIGH
> > - IRQ_TYPE_LEVEL_LOW
> > - IRQ_TYPE_EDGE_RISING
> > - IRQ_TYPE_EDGE_FALLING
> >
> > Support 0-255 priority setting for each irq.
> >
> > Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> > ---
> > .../bindings/interrupt-controller/csky,mpintc.txt | 24 ++++++-
> > drivers/irqchip/irq-csky-mpintc.c | 78 +++++++++++++++++++++-
> > 2 files changed, 99 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt b/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
> > index ab921f1..364b789 100644
> > --- a/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
> > @@ -11,6 +11,14 @@ Interrupt number definition:
> > 16-31 : private irq, and we use 16 as the co-processor timer.
> > 31-1024: common irq for soc ip.
> >
> > +Interrupt triger mode:
> > + IRQ_TYPE_LEVEL_HIGH (default)
> > + IRQ_TYPE_LEVEL_LOW
> > + IRQ_TYPE_EDGE_RISING
> > + IRQ_TYPE_EDGE_FALLING
> > +
> > +Interrupt priority range: 0-255
> > +
> > =============================
> > intc node bindings definition
> > =============================
> > @@ -26,7 +34,7 @@ intc node bindings definition
> > - #interrupt-cells
> > Usage: required
> > Value type: <u32>
> > - Definition: must be <1>
> > + Definition: could be <1> or <2>
> > - interrupt-controller:
> > Usage: required
> >
> > @@ -35,6 +43,18 @@ Examples:
> >
> > intc: interrupt-controller {
> > compatible = "csky,mpintc";
> > - #interrupt-cells = <1>;
> > + #interrupt-cells = <2>;
> > interrupt-controller;
> > };
> > +
> > + 0: device-example {
> > + ...
> > + interrupts = <33 IRQ_TYPE_EDGE_RISING>;
> > + interrupt-parent = <&intc>;
> > + };
> > +
> > + 1: device-example {
> > + ...
> > + interrupts = <34 ((priority << 4) | IRQ_TYPE_EDGE_RISING)>;
> > + interrupt-parent = <&intc>;
> > + };
> > diff --git a/drivers/irqchip/irq-csky-mpintc.c b/drivers/irqchip/irq-csky-mpintc.c
> > index c67c961..9edc6d3 100644
> > --- a/drivers/irqchip/irq-csky-mpintc.c
> > +++ b/drivers/irqchip/irq-csky-mpintc.c
> > @@ -29,9 +29,12 @@ static void __iomem *INTCL_base;
> >
> > #define INTCG_ICTLR 0x0
> > #define INTCG_CICFGR 0x100
> > +#define INTCG_CIPRTR 0x200
> > #define INTCG_CIDSTR 0x1000
> >
> > #define INTCL_PICTLR 0x0
> > +#define INTCL_CFGR 0x14
> > +#define INTCL_PRTR 0x20
> > #define INTCL_SIGR 0x60
> > #define INTCL_HPPIR 0x68
> > #define INTCL_RDYIR 0x6c
> > @@ -73,6 +76,78 @@ static void csky_mpintc_eoi(struct irq_data *d)
> > writel_relaxed(d->hwirq, reg_base + INTCL_CACR);
> > }
> >
> > +static int csky_mpintc_set_type(struct irq_data *d, unsigned int type)
> > +{
> > + unsigned int priority, triger;
>
> nit: s/triger/trigger/ everywhere.
Ok
>
> > + unsigned int offset, bit_offset;
> > + void __iomem *reg_base;
> > +
> > + /*
> > + * type Bit field: | 32 - 12 | 11 - 4 | 3 - 0 |
> > + * reserved priority triger type
> > + */
> > + triger = type & IRQ_TYPE_SENSE_MASK;
> > + priority = (type >> 4) & 0xff;
>
> Definitely not. The Linux API to set the trigger does not carry any
> priority information, nor should it. Priorities should be set
> statically, and no driver should ever be able to change it.
Currently priority in dts is:
interrupts = <34 ((priority << 4) | IRQ_TYPE_EDGE_RISING)>;
change it to:
interrupts = <34 IRQ_TYPE_EDGE_RISING priority>;
Implement csky own csky_irq_domain_xlate_cells() ...
int csky_irq_domain_xlate_cells(struct irq_domain *d, struct device_node *ctrlr,
const u32 *intspec, unsigned int intsize,
irq_hw_number_t *out_hwirq, unsigned int *out_type)
{
if (WARN_ON(intsize < 1))
return -EINVAL;
*out_hwirq = intspec[0];
if (intsize > 1)
*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
else
*out_type = IRQ_TYPE_NONE;
if (intsize > 2)
setup_priority(d->hwirq, intspec[2]);
return 0;
}
Hmm?
>
> > +
> > + switch (triger) {
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + triger = 0;
> > + break;
> > + case IRQ_TYPE_LEVEL_LOW:
> > + triger = 1;
> > + break;
> > + case IRQ_TYPE_EDGE_RISING:
> > + triger = 2;
> > + break;
> > + case IRQ_TYPE_EDGE_FALLING:
> > + triger = 3;
>
> Can you define some macros that represent these magic values?
OK.
>
> > + break;
> > + default:
> > + triger = 0;
> > + break;
>
> If you get an invalid combination, you shouldn't blindly accept it, but
> instead return an error.
OK.
>
> > + }
> > +
> > + if (d->hwirq < COMM_IRQ_BASE) {
> > + reg_base = this_cpu_read(intcl_reg);
>
> Are you guaranteed to be in a non-preemptible section here? I can see
> things going wrong if not.
???
In percpu-def.h, I see this_cpu_read is safe() for preemption or
interrupt.
/*
* Operations with implied preemption/interrupt protection. These
* operations can be used without worrying about preemption or interrupt.
*/
#define this_cpu_read(pcp) __pcpu_size_call_return(this_cpu_read_, pcp)
And:
#define __pcpu_size_call_return(stem, variable) \
({ \
typeof(variable) pscr_ret__; \
__verify_pcpu_ptr(&(variable)); \
switch(sizeof(variable)) { \
case 1: pscr_ret__ = stem##1(variable); break; \
case 2: pscr_ret__ = stem##2(variable); break; \
case 4: pscr_ret__ = stem##4(variable); break; \
case 8: pscr_ret__ = stem##8(variable); break; \
default: \
__bad_size_call_parameter(); break; \
} \
pscr_ret__; \
})
What's the wrong with preemption?
> > +
> > + if (triger) {
> > + offset = ((d->hwirq * 2) / 32) * 4;
> > + bit_offset = (d->hwirq * 2) % 32;
>
> This needs to be turned into a set of macros so that the non-percpu code
> can reuse it.
#define IRQ_OFFSET(irq) \
((irq < COMM_IRQ_BASE) ? irq : irq - COMM_IRQ_BASE)
#define TRIG_VAL(trigger, irq) \
(trigger << ((IRQ_OFFSET(irq) * 2) % 32))
#define TRIG_VAL_MSK(irq) \
(3 << ((IRQ_OFFSET(irq) * 2) % 32))
#define TRIG_BASE(irq) \
((((IRQ_OFFSET(irq) * 2) / 32) * 4) + \
((irq < COMM_IRQ_BASE) ? this_cpu_read(intcl_reg) : INTCG_base))
tmp = readl_relaxed(TRIG_BASE(d->hwirq)) & (~TRIG_VAL_MSK(d->hwirq));
writel_relaxed(tmp | TRIG_VAL(triger, d->hwirq), TRIG_BASE(d->hwirq));
Hmm?
>
> > +
> > + writel_relaxed(triger << bit_offset,
> > + reg_base + INTCL_CFGR + offset);
> > + }
> > +
> > + if (priority) {
> > + offset = ((d->hwirq * 8) / 32) * 4;
> > + bit_offset = (d->hwirq * 8) % 32;
> > +
> > + writel_relaxed(priority << bit_offset,
> > + reg_base + INTCL_PRTR + offset);
> > + }
>
> And this should only be set at boot time.
The same with above reply.
Best Regards
Guo Ren
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] irqchip/irq-csky-mpintc: Add triger type and priority setting
2019-01-18 6:28 ` Guo Ren
@ 2019-01-18 9:32 ` Marc Zyngier
2019-01-18 17:02 ` Guo Ren
0 siblings, 1 reply; 5+ messages in thread
From: Marc Zyngier @ 2019-01-18 9:32 UTC (permalink / raw)
To: Guo Ren
Cc: tglx, jason, robh+dt, mark.rutland, linux-kernel, devicetree, Guo Ren
On 18/01/2019 06:28, Guo Ren wrote:
> Thx Marc,
>
> On Thu, Jan 17, 2019 at 05:17:45PM +0000, Marc Zyngier wrote:
>> Hi Guo,
>>
>> On 15/01/2019 16:36, guoren@kernel.org wrote:
>>> From: Guo Ren <ren_guo@c-sky.com>
>>>
>>> Support 4 triger types:
>>> - IRQ_TYPE_LEVEL_HIGH
>>> - IRQ_TYPE_LEVEL_LOW
>>> - IRQ_TYPE_EDGE_RISING
>>> - IRQ_TYPE_EDGE_FALLING
>>>
>>> Support 0-255 priority setting for each irq.
>>>
>>> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
>>> ---
>>> .../bindings/interrupt-controller/csky,mpintc.txt | 24 ++++++-
>>> drivers/irqchip/irq-csky-mpintc.c | 78 +++++++++++++++++++++-
>>> 2 files changed, 99 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt b/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
>>> index ab921f1..364b789 100644
>>> --- a/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
>>> @@ -11,6 +11,14 @@ Interrupt number definition:
>>> 16-31 : private irq, and we use 16 as the co-processor timer.
>>> 31-1024: common irq for soc ip.
>>>
>>> +Interrupt triger mode:
>>> + IRQ_TYPE_LEVEL_HIGH (default)
>>> + IRQ_TYPE_LEVEL_LOW
>>> + IRQ_TYPE_EDGE_RISING
>>> + IRQ_TYPE_EDGE_FALLING
>>> +
>>> +Interrupt priority range: 0-255
>>> +
>>> =============================
>>> intc node bindings definition
>>> =============================
>>> @@ -26,7 +34,7 @@ intc node bindings definition
>>> - #interrupt-cells
>>> Usage: required
>>> Value type: <u32>
>>> - Definition: must be <1>
>>> + Definition: could be <1> or <2>
>>> - interrupt-controller:
>>> Usage: required
>>>
>>> @@ -35,6 +43,18 @@ Examples:
>>>
>>> intc: interrupt-controller {
>>> compatible = "csky,mpintc";
>>> - #interrupt-cells = <1>;
>>> + #interrupt-cells = <2>;
>>> interrupt-controller;
>>> };
>>> +
>>> + 0: device-example {
>>> + ...
>>> + interrupts = <33 IRQ_TYPE_EDGE_RISING>;
>>> + interrupt-parent = <&intc>;
>>> + };
>>> +
>>> + 1: device-example {
>>> + ...
>>> + interrupts = <34 ((priority << 4) | IRQ_TYPE_EDGE_RISING)>;
>>> + interrupt-parent = <&intc>;
>>> + };
>>> diff --git a/drivers/irqchip/irq-csky-mpintc.c b/drivers/irqchip/irq-csky-mpintc.c
>>> index c67c961..9edc6d3 100644
>>> --- a/drivers/irqchip/irq-csky-mpintc.c
>>> +++ b/drivers/irqchip/irq-csky-mpintc.c
>>> @@ -29,9 +29,12 @@ static void __iomem *INTCL_base;
>>>
>>> #define INTCG_ICTLR 0x0
>>> #define INTCG_CICFGR 0x100
>>> +#define INTCG_CIPRTR 0x200
>>> #define INTCG_CIDSTR 0x1000
>>>
>>> #define INTCL_PICTLR 0x0
>>> +#define INTCL_CFGR 0x14
>>> +#define INTCL_PRTR 0x20
>>> #define INTCL_SIGR 0x60
>>> #define INTCL_HPPIR 0x68
>>> #define INTCL_RDYIR 0x6c
>>> @@ -73,6 +76,78 @@ static void csky_mpintc_eoi(struct irq_data *d)
>>> writel_relaxed(d->hwirq, reg_base + INTCL_CACR);
>>> }
>>>
>>> +static int csky_mpintc_set_type(struct irq_data *d, unsigned int type)
>>> +{
>>> + unsigned int priority, triger;
>>
>> nit: s/triger/trigger/ everywhere.
> Ok
>
>>
>>> + unsigned int offset, bit_offset;
>>> + void __iomem *reg_base;
>>> +
>>> + /*
>>> + * type Bit field: | 32 - 12 | 11 - 4 | 3 - 0 |
>>> + * reserved priority triger type
>>> + */
>>> + triger = type & IRQ_TYPE_SENSE_MASK;
>>> + priority = (type >> 4) & 0xff;
>>
>> Definitely not. The Linux API to set the trigger does not carry any
>> priority information, nor should it. Priorities should be set
>> statically, and no driver should ever be able to change it.
> Currently priority in dts is:
>
> interrupts = <34 ((priority << 4) | IRQ_TYPE_EDGE_RISING)>;
>
> change it to:
>
> interrupts = <34 IRQ_TYPE_EDGE_RISING priority>;
>
I don't think you need to change the DT format, as this is quite painful
for users.
> Implement csky own csky_irq_domain_xlate_cells() ...
>
> int csky_irq_domain_xlate_cells(struct irq_domain *d, struct device_node *ctrlr,
> const u32 *intspec, unsigned int intsize,
> irq_hw_number_t *out_hwirq, unsigned int *out_type)
> {
> if (WARN_ON(intsize < 1))
> return -EINVAL;
> *out_hwirq = intspec[0];
> if (intsize > 1)
> *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
> else
> *out_type = IRQ_TYPE_NONE;
>
> if (intsize > 2)
> setup_priority(d->hwirq, intspec[2]);
That's still a problem. Linux doesn't expect interrupts to have
different priorities. All interrupts are equal in that respect, and
interrupt nesting is not something we expect.
I'd be more confident if you programmed a default priority at boot time,
and completely ignored the DT information.
>
> return 0;
> }
> Hmm?
>
>>
>>> +
>>> + switch (triger) {
>>> + case IRQ_TYPE_LEVEL_HIGH:
>>> + triger = 0;
>>> + break;
>>> + case IRQ_TYPE_LEVEL_LOW:
>>> + triger = 1;
>>> + break;
>>> + case IRQ_TYPE_EDGE_RISING:
>>> + triger = 2;
>>> + break;
>>> + case IRQ_TYPE_EDGE_FALLING:
>>> + triger = 3;
>>
>> Can you define some macros that represent these magic values?
> OK.
>>
>>> + break;
>>> + default:
>>> + triger = 0;
>>> + break;
>>
>> If you get an invalid combination, you shouldn't blindly accept it, but
>> instead return an error.
> OK.
>
>>
>>> + }
>>> +
>>> + if (d->hwirq < COMM_IRQ_BASE) {
>>> + reg_base = this_cpu_read(intcl_reg);
>>
>> Are you guaranteed to be in a non-preemptible section here? I can see
>> things going wrong if not.
>
> ???
> In percpu-def.h, I see this_cpu_read is safe() for preemption or
> interrupt.
Sorry, I wasn't clear, see below.
> What's the wrong with preemption?
The problem is that if the driver calls irq_set_type() on a per-CPU
interrupt without preemption being disabled, it can be preempted at any
point and migrated anywhere before the call to this_cpu_read() takes
place. This means you can never know which CPU you've programmed.
One possible approach is to mandate these interrupts to be only changed
in non-preemptible context, which is what the various ARM GICs do for
their per-CPU interrupts.
>
>>> +
>>> + if (triger) {
>>> + offset = ((d->hwirq * 2) / 32) * 4;
>>> + bit_offset = (d->hwirq * 2) % 32;
>>
>> This needs to be turned into a set of macros so that the non-percpu code
>> can reuse it.
>
>
> #define IRQ_OFFSET(irq) \
> ((irq < COMM_IRQ_BASE) ? irq : irq - COMM_IRQ_BASE)
>
> #define TRIG_VAL(trigger, irq) \
> (trigger << ((IRQ_OFFSET(irq) * 2) % 32))
>
> #define TRIG_VAL_MSK(irq) \
> (3 << ((IRQ_OFFSET(irq) * 2) % 32))
>
> #define TRIG_BASE(irq) \
> ((((IRQ_OFFSET(irq) * 2) / 32) * 4) + \
> ((irq < COMM_IRQ_BASE) ? this_cpu_read(intcl_reg) : INTCG_base))
>
> tmp = readl_relaxed(TRIG_BASE(d->hwirq)) & (~TRIG_VAL_MSK(d->hwirq));
> writel_relaxed(tmp | TRIG_VAL(triger, d->hwirq), TRIG_BASE(d->hwirq));
>
> Hmm?
I was only looking for something that abstract the offsets, such as:
#define BYTE_OFFSET(i) (((i) * 2) / 32) * 4)
#define BIT_OFFSET(i) ((i) * 2) % 32)
and keep the rest of the structure as is.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] irqchip/irq-csky-mpintc: Add triger type and priority setting
2019-01-18 9:32 ` Marc Zyngier
@ 2019-01-18 17:02 ` Guo Ren
0 siblings, 0 replies; 5+ messages in thread
From: Guo Ren @ 2019-01-18 17:02 UTC (permalink / raw)
To: Marc Zyngier
Cc: tglx, jason, robh+dt, mark.rutland, linux-kernel, devicetree, Guo Ren
On Fri, Jan 18, 2019 at 09:32:03AM +0000, Marc Zyngier wrote:
> On 18/01/2019 06:28, Guo Ren wrote:
> > Thx Marc,
> >
> > On Thu, Jan 17, 2019 at 05:17:45PM +0000, Marc Zyngier wrote:
> >> Hi Guo,
> >>
> >> On 15/01/2019 16:36, guoren@kernel.org wrote:
> >>> From: Guo Ren <ren_guo@c-sky.com>
> >>>
> >>> Support 4 triger types:
> >>> - IRQ_TYPE_LEVEL_HIGH
> >>> - IRQ_TYPE_LEVEL_LOW
> >>> - IRQ_TYPE_EDGE_RISING
> >>> - IRQ_TYPE_EDGE_FALLING
> >>>
> >>> Support 0-255 priority setting for each irq.
> >>>
> >>> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> >>> ---
> >>> .../bindings/interrupt-controller/csky,mpintc.txt | 24 ++++++-
> >>> drivers/irqchip/irq-csky-mpintc.c | 78 +++++++++++++++++++++-
> >>> 2 files changed, 99 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt b/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
> >>> index ab921f1..364b789 100644
> >>> --- a/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
> >>> +++ b/Documentation/devicetree/bindings/interrupt-controller/csky,mpintc.txt
> >>> @@ -11,6 +11,14 @@ Interrupt number definition:
> >>> 16-31 : private irq, and we use 16 as the co-processor timer.
> >>> 31-1024: common irq for soc ip.
> >>>
> >>> +Interrupt triger mode:
> >>> + IRQ_TYPE_LEVEL_HIGH (default)
> >>> + IRQ_TYPE_LEVEL_LOW
> >>> + IRQ_TYPE_EDGE_RISING
> >>> + IRQ_TYPE_EDGE_FALLING
> >>> +
> >>> +Interrupt priority range: 0-255
> >>> +
> >>> =============================
> >>> intc node bindings definition
> >>> =============================
> >>> @@ -26,7 +34,7 @@ intc node bindings definition
> >>> - #interrupt-cells
> >>> Usage: required
> >>> Value type: <u32>
> >>> - Definition: must be <1>
> >>> + Definition: could be <1> or <2>
> >>> - interrupt-controller:
> >>> Usage: required
> >>>
> >>> @@ -35,6 +43,18 @@ Examples:
> >>>
> >>> intc: interrupt-controller {
> >>> compatible = "csky,mpintc";
> >>> - #interrupt-cells = <1>;
> >>> + #interrupt-cells = <2>;
> >>> interrupt-controller;
> >>> };
> >>> +
> >>> + 0: device-example {
> >>> + ...
> >>> + interrupts = <33 IRQ_TYPE_EDGE_RISING>;
> >>> + interrupt-parent = <&intc>;
> >>> + };
> >>> +
> >>> + 1: device-example {
> >>> + ...
> >>> + interrupts = <34 ((priority << 4) | IRQ_TYPE_EDGE_RISING)>;
> >>> + interrupt-parent = <&intc>;
> >>> + };
> >>> diff --git a/drivers/irqchip/irq-csky-mpintc.c b/drivers/irqchip/irq-csky-mpintc.c
> >>> index c67c961..9edc6d3 100644
> >>> --- a/drivers/irqchip/irq-csky-mpintc.c
> >>> +++ b/drivers/irqchip/irq-csky-mpintc.c
> >>> @@ -29,9 +29,12 @@ static void __iomem *INTCL_base;
> >>>
> >>> #define INTCG_ICTLR 0x0
> >>> #define INTCG_CICFGR 0x100
> >>> +#define INTCG_CIPRTR 0x200
> >>> #define INTCG_CIDSTR 0x1000
> >>>
> >>> #define INTCL_PICTLR 0x0
> >>> +#define INTCL_CFGR 0x14
> >>> +#define INTCL_PRTR 0x20
> >>> #define INTCL_SIGR 0x60
> >>> #define INTCL_HPPIR 0x68
> >>> #define INTCL_RDYIR 0x6c
> >>> @@ -73,6 +76,78 @@ static void csky_mpintc_eoi(struct irq_data *d)
> >>> writel_relaxed(d->hwirq, reg_base + INTCL_CACR);
> >>> }
> >>>
> >>> +static int csky_mpintc_set_type(struct irq_data *d, unsigned int type)
> >>> +{
> >>> + unsigned int priority, triger;
> >>
> >> nit: s/triger/trigger/ everywhere.
> > Ok
> >
> >>
> >>> + unsigned int offset, bit_offset;
> >>> + void __iomem *reg_base;
> >>> +
> >>> + /*
> >>> + * type Bit field: | 32 - 12 | 11 - 4 | 3 - 0 |
> >>> + * reserved priority triger type
> >>> + */
> >>> + triger = type & IRQ_TYPE_SENSE_MASK;
> >>> + priority = (type >> 4) & 0xff;
> >>
> >> Definitely not. The Linux API to set the trigger does not carry any
> >> priority information, nor should it. Priorities should be set
> >> statically, and no driver should ever be able to change it.
> > Currently priority in dts is:
> >
> > interrupts = <34 ((priority << 4) | IRQ_TYPE_EDGE_RISING)>;
> >
> > change it to:
> >
> > interrupts = <34 IRQ_TYPE_EDGE_RISING priority>;
> >
>
> I don't think you need to change the DT format, as this is quite painful
> for users.
I'll keep this style and I think it's good place to set priority.
>
> > Implement csky own csky_irq_domain_xlate_cells() ...
> >
> > int csky_irq_domain_xlate_cells(struct irq_domain *d, struct device_node *ctrlr,
> > const u32 *intspec, unsigned int intsize,
> > irq_hw_number_t *out_hwirq, unsigned int *out_type)
> > {
> > if (WARN_ON(intsize < 1))
> > return -EINVAL;
> > *out_hwirq = intspec[0];
> > if (intsize > 1)
> > *out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
> > else
> > *out_type = IRQ_TYPE_NONE;
> >
> > if (intsize > 2)
> > setup_priority(d->hwirq, intspec[2]);
>
> That's still a problem. Linux doesn't expect interrupts to have
> different priorities. All interrupts are equal in that respect, and
> interrupt nesting is not something we expect.
>
> I'd be more confident if you programmed a default priority at boot time,
> and completely ignored the DT information.
The priority for us is to setup a watchdog or NMI style interrupt.
Mostly users don't need care about priority setting and just keep as
default 0. See my "PATCH V2", we can continue talk there.
>
> >
> > return 0;
> > }
> > Hmm?
> >
> >>
> >>> +
> >>> + switch (triger) {
> >>> + case IRQ_TYPE_LEVEL_HIGH:
> >>> + triger = 0;
> >>> + break;
> >>> + case IRQ_TYPE_LEVEL_LOW:
> >>> + triger = 1;
> >>> + break;
> >>> + case IRQ_TYPE_EDGE_RISING:
> >>> + triger = 2;
> >>> + break;
> >>> + case IRQ_TYPE_EDGE_FALLING:
> >>> + triger = 3;
> >>
> >> Can you define some macros that represent these magic values?
> > OK.
> >>
> >>> + break;
> >>> + default:
> >>> + triger = 0;
> >>> + break;
> >>
> >> If you get an invalid combination, you shouldn't blindly accept it, but
> >> instead return an error.
> > OK.
> >
> >>
> >>> + }
> >>> +
> >>> + if (d->hwirq < COMM_IRQ_BASE) {
> >>> + reg_base = this_cpu_read(intcl_reg);
> >>
> >> Are you guaranteed to be in a non-preemptible section here? I can see
> >> things going wrong if not.
> >
> > ???
> > In percpu-def.h, I see this_cpu_read is safe() for preemption or
> > interrupt.
>
> Sorry, I wasn't clear, see below.
>
> > What's the wrong with preemption?
>
> The problem is that if the driver calls irq_set_type() on a per-CPU
> interrupt without preemption being disabled, it can be preempted at any
> point and migrated anywhere before the call to this_cpu_read() takes
> place. This means you can never know which CPU you've programmed.
>
> One possible approach is to mandate these interrupts to be only changed
> in non-preemptible context, which is what the various ARM GICs do for
> their per-CPU interrupts.
Wow... Thank you for reminding! In "PATCH V2", I set them in irq_enable.
Please have a look.
>
> >
> >>> +
> >>> + if (triger) {
> >>> + offset = ((d->hwirq * 2) / 32) * 4;
> >>> + bit_offset = (d->hwirq * 2) % 32;
> >>
> >> This needs to be turned into a set of macros so that the non-percpu code
> >> can reuse it.
> >
> >
> > #define IRQ_OFFSET(irq) \
> > ((irq < COMM_IRQ_BASE) ? irq : irq - COMM_IRQ_BASE)
> >
> > #define TRIG_VAL(trigger, irq) \
> > (trigger << ((IRQ_OFFSET(irq) * 2) % 32))
> >
> > #define TRIG_VAL_MSK(irq) \
> > (3 << ((IRQ_OFFSET(irq) * 2) % 32))
> >
> > #define TRIG_BASE(irq) \
> > ((((IRQ_OFFSET(irq) * 2) / 32) * 4) + \
> > ((irq < COMM_IRQ_BASE) ? this_cpu_read(intcl_reg) : INTCG_base))
> >
> > tmp = readl_relaxed(TRIG_BASE(d->hwirq)) & (~TRIG_VAL_MSK(d->hwirq));
> > writel_relaxed(tmp | TRIG_VAL(triger, d->hwirq), TRIG_BASE(d->hwirq));
> >
> > Hmm?
>
> I was only looking for something that abstract the offsets, such as:
>
> #define BYTE_OFFSET(i) (((i) * 2) / 32) * 4)
> #define BIT_OFFSET(i) ((i) * 2) % 32)
>
> and keep the rest of the structure as is.
Ok.
Best Regards
Guo Ren
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-01-18 17:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-15 16:36 [PATCH] irqchip/irq-csky-mpintc: Add triger type and priority setting guoren
2019-01-17 17:17 ` Marc Zyngier
2019-01-18 6:28 ` Guo Ren
2019-01-18 9:32 ` Marc Zyngier
2019-01-18 17:02 ` Guo Ren
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).