Thx Marc,
Sorry for late reply:
On Mon, Feb 18, 2019 at 02:38:23PM +0000, Marc Zyngier wrote:
> On Mon, 18 Feb 2019 10:04:40 +0800
> guo...@kernel.org wrote:
>
> > From: Guo Ren <ren_...@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.
> >
> > Changelog:
> > - Fixup this_cpu_read() preempted problem.
> > - Optimize the coding style.
> >
> > Signed-off-by: Guo Ren <ren_...@c-sky.com>
> > Cc: Marc Zyngier <marc.zyng...@arm.com>
> > ---
> > drivers/irqchip/irq-csky-mpintc.c | 105
> > +++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 104 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/irqchip/irq-csky-mpintc.c
> > b/drivers/irqchip/irq-csky-mpintc.c
> > index 99d3f3f..07a3752 100644
> > --- a/drivers/irqchip/irq-csky-mpintc.c
> > +++ b/drivers/irqchip/irq-csky-mpintc.c
> > @@ -17,6 +17,7 @@
> > #include <asm/reg_ops.h>
> >
> > static struct irq_domain *root_domain;
> > +
> > static void __iomem *INTCG_base;
> > static void __iomem *INTCL_base;
> >
> > @@ -29,9 +30,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_RDYIR 0x6c
> > #define INTCL_SENR 0xa0
> > @@ -40,6 +44,51 @@ static void __iomem *INTCL_base;
> >
> > static DEFINE_PER_CPU(void __iomem *, intcl_reg);
> >
> > +static unsigned long *__trigger;
> > +static unsigned long *__priority;
> > +
> > +#define IRQ_OFFSET(irq) ((irq < COMM_IRQ_BASE) ? irq : (irq -
> > COMM_IRQ_BASE))
> > +
> > +#define TRIG_BYTE_OFFSET(i) ((((i) * 2) / 32) * 4)
> > +#define TRIG_BIT_OFFSET(i) (((i) * 2) % 32)
> > +
> > +#define PRI_BYTE_OFFSET(i) ((((i) * 8) / 32) * 4)
> > +#define PRI_BIT_OFFSET(i) (((i) * 8) % 32)
> > +
> > +#define TRIG_VAL(trigger, irq) (trigger <<
> > TRIG_BIT_OFFSET(IRQ_OFFSET(irq)))
> > +#define TRIG_VAL_MSK(irq) (~(3 << TRIG_BIT_OFFSET(IRQ_OFFSET(irq))))
> > +#define PRI_VAL(priority, irq) (priority <<
> > PRI_BIT_OFFSET(IRQ_OFFSET(irq)))
> > +#define PRI_VAL_MSK(irq) (~(0xff << PRI_BIT_OFFSET(IRQ_OFFSET(irq))))
> > +
> > +#define TRIG_BASE(irq) \
> > + (TRIG_BYTE_OFFSET(IRQ_OFFSET(irq)) + ((irq < COMM_IRQ_BASE) ? \
> > + (this_cpu_read(intcl_reg) + INTCL_CFGR) : (INTCG_base + INTCG_CICFGR)))
> > +
> > +#define PRI_BASE(irq) \
> > + (PRI_BYTE_OFFSET(IRQ_OFFSET(irq)) + ((irq < COMM_IRQ_BASE) ? \
> > + (this_cpu_read(intcl_reg) + INTCL_PRTR) : (INTCG_base + INTCG_CIPRTR)))
> > +
> > +static DEFINE_SPINLOCK(setup_lock);
> > +static void setup_trigger_priority(unsigned long irq, unsigned long
> > trigger,
> > + unsigned long priority)
> > +{
> > + unsigned int tmp;
> > +
> > + spin_lock(&setup_lock);
> > +
> > + /* setup trigger */
> > + tmp = readl_relaxed(TRIG_BASE(irq)) & TRIG_VAL_MSK(irq);
> > +
> > + writel_relaxed(tmp | TRIG_VAL(trigger, irq), TRIG_BASE(irq));
> > +
> > + /* setup priority */
> > + tmp = readl_relaxed(PRI_BASE(irq)) & PRI_VAL_MSK(irq);
> > +
> > + writel_relaxed(tmp | PRI_VAL(priority, irq), PRI_BASE(irq));
> > +
> > + spin_unlock(&setup_lock);
> > +}
> > +
> > static void csky_mpintc_handler(struct pt_regs *regs)
> > {
> > void __iomem *reg_base = this_cpu_read(intcl_reg);
> > @@ -52,6 +101,9 @@ static void csky_mpintc_enable(struct irq_data *d)
> > {
> > void __iomem *reg_base = this_cpu_read(intcl_reg);
> >
> > + setup_trigger_priority(d->hwirq, __trigger[d->hwirq],
> > + __priority[d->hwirq]);
> > +
> > writel_relaxed(d->hwirq, reg_base + INTCL_SENR);
> > }
> >
> > @@ -69,6 +121,28 @@ 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)
> > +{
> > + switch (type & IRQ_TYPE_SENSE_MASK) {
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + __trigger[d->hwirq] = 0;
> > + break;
> > + case IRQ_TYPE_LEVEL_LOW:
> > + __trigger[d->hwirq] = 1;
> > + break;
> > + case IRQ_TYPE_EDGE_RISING:
> > + __trigger[d->hwirq] = 2;
> > + break;
> > + case IRQ_TYPE_EDGE_FALLING:
> > + __trigger[d->hwirq] = 3;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > #ifdef CONFIG_SMP
> > static int csky_irq_set_affinity(struct irq_data *d,
> > const struct cpumask *mask_val,
> > @@ -101,6 +175,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
> > @@ -121,9 +196,29 @@ static int csky_irqdomain_map(struct irq_domain *d,
> > unsigned int irq,
> > return 0;
> > }
> >
> > +static int csky_irq_domain_xlate_cells(struct irq_domain *d,
> > + struct device_node *ctrlr, const u32 *intspec,
> > + unsigned int intsize, unsigned long *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;
>
> What does IRQ_TYPE_NONE mean in this context? Shouldn't it actually be
> whatever the HW defaults to? Or even better, whatever was expected in
> the previous definition of the DT binding?
Yes, it shouldn't use IRQ_TYPE_NONE and I'll use IRQ_TYPE_LEVEL_HIGH.
>
> > +
> > + if (intsize > 2)
> > + __priority[*out_hwirq] = intspec[2];
>
> And what is the used priority in this case?
C-SKY MPINTC could support interrupt's priority and this will be set in
INTCG_CIPRTR register. It is set in csky_mpintc_enable function.
>
> > +
> > + return 0;
> > +}
> > +
> > static const struct irq_domain_ops csky_irqdomain_ops = {
> > .map = csky_irqdomain_map,
> > - .xlate = irq_domain_xlate_onecell,
> > + .xlate = csky_irq_domain_xlate_cells,
> > };
> >
> > #ifdef CONFIG_SMP
> > @@ -157,6 +252,14 @@ csky_mpintc_init(struct device_node *node, struct
> > device_node *parent)
> > if (ret < 0)
> > nr_irq = INTC_IRQS;
> >
> > + __priority = kcalloc(nr_irq, sizeof(unsigned long), GFP_KERNEL);
> > + if (__priority == NULL)
> > + return -ENXIO;
> > +
> > + __trigger = kcalloc(nr_irq, sizeof(unsigned long), GFP_KERNEL);
> > + if (__trigger == NULL)
> > + return -ENXIO;
>
> Maybe you should consider initializing these arrays to something that
> makes sense for the case where the DT doesn't carry this information
> (which is 100% of the DTs up to this point).
Yes, and zero is enough.
/**
* kcalloc - allocate memory for an array. The memory is set to zero.
* @n: number of elements.
* @size: element size.
* @flags: the type of memory to allocate (see kmalloc).
*/
static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
{
return kmalloc_array(n, size, flags | __GFP_ZERO);
}
>
> > +
> > if (INTCG_base == NULL) {
> > INTCG_base = ioremap(mfcr("cr<31, 14>"),
> > INTCL_SIZE*nr_cpu_ids + INTCG_SIZE);
>
>
> Not directly related to this patch: Please add a cover letter to you
> patch series, and describe the goal of the whole series as well as the
> changes you make along the way. It will definitely help the reviewers.
I'll seperate them first.
Best Regards
Guo Ren
