On 2020-08-06 11:03, Daniel Palmer wrote:
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.
It is much better to have an irqchip structure per irqchip type,
rather than one per instance, as you can then make the irqchip const.
It also saves memory when you have more than a single instance.
The only case where it doesn't work is when PM gets involved, as the
parent_device pointer is stupidly stored in the irqchip device.
> +};
> +
> +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?
Yes, a spinlock is the conventional way to solve it. Make sure
you use the irqsave/irqrestore versions, as mask/unmask can
also occur whilst in interrupt context.
> +
> + 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.
It is indeed the 16bit accesses that reminded me of the MTK
code, as that's very unusual.
Hopefully you can work with the MTK guys to resolve this quickly.
M.
--
Jazz is not dead. It just smells funny...
