Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200805110052.2655487-1-daniel@0x0f.com> <20200805110052.2655487-3-daniel@0x0f.com>
 <a2ac8875d67ce7afe1b28f01683e0c9d@kernel.org>
In-Reply-To: <a2ac8875d67ce7afe1b28f01683e0c9d@kernel.org>
From:   Daniel Palmer <daniel@0x0f.com>
Date:   Thu, 6 Aug 2020 19:03:10 +0900
Message-ID: <CAFr9PXmzZmHWv+DWppaXOih9p5pJK=3JYCCC+-XrnQ+S7sV+fw@mail.gmail.com>
Subject: Re: [PATCH 2/3] irqchip: mstar: msc313-intc interrupt controller driver
To:     Marc Zyngier <maz@kernel.org>
Cc:     linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        linux-kernel@vger.kernel.org, DTML <devicetree@vger.kernel.org>,
        tglx@linutronix.de, jason@lakedaemon.net,
        Rob Herring <robh+dt@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>, Willy Tarreau <w@1wt.eu>,
        mark-pk.tsai@mediatek.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Marc,

On Thu, 6 Aug 2020 at 01:26, Marc Zyngier <maz@kernel.org> 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 =3D REGOFF(hwirq);
> > +     void __iomem *addr =3D intc->base + REGOFF_MASK + regoff;
> > +     u16 bit =3D IRQBIT(hwirq);
> > +     u16 reg =3D readw_relaxed(addr);
> > +
> > +     if (mask)
> > +             reg |=3D bit;
> > +     else
> > +             reg &=3D ~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 &=3D ~bit;
> > +     else
> > +             reg |=3D 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=C3=A9ja-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