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

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

[guoren]

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

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

[Marc Zyngier]

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...

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

[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

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

[Marc Zyngier]

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...

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

[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