Messages in this thread | | | From | Daniel Palmer <> | Date | Thu, 6 Aug 2020 19:03:10 +0900 | Subject | Re: [PATCH 2/3] irqchip: mstar: msc313-intc interrupt controller driver |
| |
Hi Marc,
On Thu, 6 Aug 2020 at 01:26, Marc Zyngier <maz@kernel.org> wrote: > > +struct msc313_intc { > > + struct irq_domain *domain; > > + void __iomem *base; > > + struct irq_chip irqchip; > > Why do you need to embed the irq_chip on a per-controller basis?
Current chips have 1 instance of each type of controller but some of the newer ones seem to have an extra copy of the non-FIQ version with different offset to the GIC.
> > +}; > > + > > +static void msc313_intc_maskunmask(struct msc313_intc *intc, int > > hwirq, bool mask) > > +{ > > + int regoff = REGOFF(hwirq); > > + void __iomem *addr = intc->base + REGOFF_MASK + regoff; > > + u16 bit = IRQBIT(hwirq); > > + u16 reg = readw_relaxed(addr); > > + > > + if (mask) > > + reg |= bit; > > + else > > + reg &= ~bit; > > + > > + writew_relaxed(reg, addr); > > RMW on a shared MMIO register. Not going to end well. This is valid > for all the callbacks, I believe.
Do you have any suggestions on how to resolve that? It seems usually an interrupt controller has set and clear registers to get around this. Would defining a spinlock at the top of the driver and using that around the read and modify sequences be good enough?
> > + > > + if (flow_type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_LEVEL_HIGH)) > > + reg &= ~bit; > > + else > > + reg |= bit; > > I don't follow grasp the logic here. What happens on EDGE_BOTH, for > example?
To be honest I don't quite remember. I'll check and rewrite this.
> This driver has a massive feeling of déja-vu. It is almost > a copy of the one posted at [1], which I reviewed early > this week. The issues are the exact same, and I'm 98% > sure this is the same IP block used by two SoC vendors.
This would make a lot of sense considering MediaTek bought MStar for their TV SoCs. The weirdness with only using 16 bits in a register suggests they've inherited the shared ARM/8051 bus that the MStar chips have. Thanks for the tip off.
Cheers,
Daniel
| |