Re: [PATCH] irqchip/irq-csky-mpintc: Add triger type and priority setting

From: Guo Ren
Date: Fri Jan 18 2019 - 12:02:15 EST

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@xxxxxxxxxx wrote:
> >>> From: Guo Ren <ren_guo@xxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxx>
> >>> ---
> >>> .../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