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

[Marc Zyngier]

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