Message ID | 1547570199-25944-1-git-send-email-guoren@kernel.org |
---|---|
State | Superseded, archived |
Headers | show |
Series | irqchip/irq-csky-mpintc: Add triger type and priority setting | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | warning | "total: 0 errors, 1 warnings, 146 lines checked" |
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.
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
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.
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
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