Messages in this thread | | | Date | Thu, 17 Sep 2015 23:30:48 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH 2/2] ARM: irqchip: mxs: add Alpascale ASM9260 support |
| |
On Thu, 17 Sep 2015, Oleksij Rempel wrote: > static struct icoll_priv icoll_priv; > static struct irq_domain *icoll_domain; > +static DEFINE_RAW_SPINLOCK(icoll_lock); > + > +/* calculate bit offset depending on number of intterupt per register */ > +static u32 icoll_intr_bitshift(struct irq_data *d, u32 bit) > +{ > + /* > + * We expect intr_per_reg to be 4 or 1, it means > + * "n" will be 3 or 0. > + */ > + int n = icoll_priv.intr_per_reg - 1; > + > + /* > + * If n = 0, "bit" is never shifted. > + * If n = 3, mask lower part of hwirq to convert it > + * in 0, 1, 2 or 3 and then multiply it by 8 (or shift by 3) > + */ > + return bit << ((d->hwirq & n) << n); > +} > + > +/* calculate mem offset depending on number of intterupt per register */ > +static void __iomem *icoll_intr_reg(struct irq_data *d) > +{ > + int n = icoll_priv.intr_per_reg >> 1; > + > + /* offset = hwirq / intr_per_reg * 0x10 */ > + return icoll_priv.intr + ((d->hwirq >> n) * 0x10); > +} > > static void icoll_ack_irq(struct irq_data *d) > { > @@ -76,14 +107,21 @@ static void icoll_ack_irq(struct irq_data *d) > > static void icoll_mask_irq(struct irq_data *d) > { > - __raw_writel(BM_ICOLL_INTR_ENABLE, > - icoll_priv.intr + CLR_REG + HW_ICOLL_INTERRUPTn(d->hwirq)); > + __raw_writel(icoll_intr_bitshift(d, BM_ICOLL_INTR_ENABLE), > + icoll_intr_reg(d) + CLR_REG); > } > > static void icoll_unmask_irq(struct irq_data *d) > { > - __raw_writel(BM_ICOLL_INTR_ENABLE, > - icoll_priv.intr + SET_REG + HW_ICOLL_INTERRUPTn(d->hwirq)); > + raw_spin_lock(&icoll_lock); > + if (icoll_priv.clear) > + __raw_writel(ASM9260_BM_CLEAR_BIT(d->hwirq), > + icoll_priv.clear + > + ASM9260_HW_ICOLL_CLEARn(d->hwirq)); > + > + __raw_writel(icoll_intr_bitshift(d, BM_ICOLL_INTR_ENABLE), > + icoll_intr_reg(d) + SET_REG); > + raw_spin_unlock(&icoll_lock); > }
I don't think this is a brilliant idea. You burden the existing users with that bit shift mask machinery, an extra conditional and the spinlock in the unmask path.
You can simply implement a seperate pair of callbacks and assign them at setup time.
Btw, why is that spinlock only in unmask()?
> static struct irq_chip mxs_icoll_chip = { > @@ -115,12 +153,34 @@ static const struct irq_domain_ops icoll_irq_domain_ops = { > .xlate = irq_domain_xlate_onecell, > }; > > +static void __init icoll_add_domain(struct device_node *np, > + int num) > +{ > + icoll_domain = irq_domain_add_linear(np, num, > + &icoll_irq_domain_ops, NULL); > + > + if (!icoll_domain) > + panic("%s: unable add irq domain", np->full_name); > + irq_set_default_host(icoll_domain); > + set_handle_irq(icoll_handle_irq); > +} > + > +static void __iomem * __init icoll_init_iobase(struct device_node *np) > +{ > + void __iomem *icoll_base; > + > + icoll_base = of_io_request_and_map(np, 0, np->name); > + if (!icoll_base) > + panic("%s: unable to map resource", np->full_name); > + return icoll_base; > +} > + > static int __init icoll_of_init(struct device_node *np, > struct device_node *interrupt_parent) > { > - void __iomem *icoll_base = of_iomap(np, 0); > - WARN_ON(!icoll_base); > + void __iomem *icoll_base; > > + icoll_base = icoll_init_iobase(np); > icoll_priv.vector = icoll_base + HW_ICOLL_VECTOR; > icoll_priv.levelack = icoll_base + HW_ICOLL_LEVELACK; > icoll_priv.ctrl = icoll_base + HW_ICOLL_CTRL; > @@ -135,8 +195,38 @@ static int __init icoll_of_init(struct device_node *np, > */ > stmp_reset_block(icoll_priv.ctrl); > > - icoll_domain = irq_domain_add_linear(np, ICOLL_NUM_IRQS, > - &icoll_irq_domain_ops, NULL); > + icoll_add_domain(np, ICOLL_NUM_IRQS); > + > return icoll_domain ? 0 : -ENODEV; > }
That part of the patch should move into the preparatory patch. It has nothing to do with asm9260.
Thanks,
tglx
| |