Hi Marc,
On Thu, 6 Aug 2020 at 01:26, Marc Zyngier <maz@xxxxxxxxxx> 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
