Re: [PATCH v3] irqchip: Add support for ARMv7-M's NVIC

[Uwe Kleine-König]

On Thu, Apr 18, 2013 at 11:35:22AM +0200, Thomas Gleixner wrote:
> On Wed, 17 Apr 2013, Uwe Kleine-König wrote:
> > +struct nvic_bank_data {
> > +	/*
> > +	 * For irq i base holds nvic_base + 4 * i / 32. So you can access the
> > +	 * right ISER register (i.e ISER[i / 32]) by just taking base + ISER.
> > +	 * Ditto for ICER.
> > +	 */
> > +	void __iomem *base;
> > +};
> 
> What's the point of a struct with a single member? Why not having an
> array of base pointers ?
It gives a name to that single member and maybe makes future changes
easier. Obviously you could argue ...

When switching to generic irq chip this struct probably goes away, so I
suggest to postpone this discussion.

> > +static struct nvic_chip_data {
> > +	struct irq_domain *domain;
> > +	struct nvic_bank_data bdata[NVIC_MAX_BANKS];
> > +} nvic_chip_data;
> > +
> > +asmlinkage void __exception_irq_entry
> > +nvic_do_IRQ(irq_hw_number_t hwirq, struct pt_regs *regs)
> > +{
> > +	unsigned int irq = irq_linear_revmap(nvic_chip_data.domain, hwirq);
> > +
> > +	handle_IRQ(irq, regs);
> > +}
> > +
> > +static inline void __iomem *nvic_bank_base(struct irq_data *d)
> > +{
> > +	struct nvic_bank_data *bank_data = irq_data_get_irq_chip_data(d);
> > +	return bank_data->base;
> > +}
> > +
> > +static void nvic_mask_irq(struct irq_data *d)
> > +{
> > +	u32 mask = 1 << (d->hwirq % 32);
> > +
> > +	writel_relaxed(mask, nvic_bank_base(d) + NVIC_ICER);
> > +}
> > +
> > +static void nvic_unmask_irq(struct irq_data *d)
> > +{
> > +	u32 mask = 1 << (d->hwirq % 32);
> > +
> > +	writel_relaxed(mask, nvic_bank_base(d) + NVIC_ISER);
> > +}
> 
> How is that different from what the generic irq chip implementation
> does? The only difference is that mask is generated by d->hwirq and
> not by d->irq. And due to the fact, that you use a full linear mapping
> between hwirq and virq the generic code simply works.
I'm not sure what you mean when you say "full linear mapping". AFAICT
using irq_domain_add_linear doesn't imply that two consecutive hardware
irq numbers get consecutive Linux irq numbers, so using d->irq won't work.
 
> Even if it would not work, it would be trivial to extend the generic
> chip with that functionality instead of hacking another slightly
> different copy of the same thing.
I will try that and report back.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/