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

Hi Marc,

On Thu, 6 Aug 2020 at 01:26, Marc Zyngier [off-list ref] wrote:

quoted

+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.

quoted

+};
+
+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?

quoted

+
+     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