Re: [PATCH 2/3] irqchip: mstar: msc313-intc interrupt controller driver

[Daniel Palmer]

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