Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity

[Magnus Damm <magnus.damm@gmail.com>]

Hi Marc,

On Sun, Sep 12, 2021 at 4:32 AM Marc Zyngier <maz@kernel.org> wrote:
> On Sat, 11 Sep 2021 03:49:20 +0100,
> Magnus Damm <magnus.damm@gmail.com> wrote:
> > On Fri, Sep 10, 2021 at 10:19 PM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Fri, Sep 10, 2021 at 12:23 PM Marc Zyngier <maz@kernel.org> wrote:
> > > > On Thu, 09 Sep 2021 16:22:01 +0100,
> > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > >     GIC: enabling workaround for broken byte access
> >
> > Indeed, byte access is unsupported according to the EMEV2 documentation.
> >
> > The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on
> > page 97 says:
> > "Interrupt registers can be accessed via the APB bus, in 32-bit units"
> > "For details about register functions, see ARM Generic Interrupt
> > Controller Architecture Specification Architecture version 1.0"
> > The file  "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version
> > published in 2010 and is not marked as confidential.
>
> This is as bad as it gets. Do you know if any other Renesas platform
> is affected by the same issue?

Next time we have a beer together I would be happy to show you some
legacy interrupt controller code. =)

EMEV2 and the Emma Mobile product line came from the NEC Electronics
side that got merged into Renesas Electronics in 2010. Historically
NEC Electronics mainly used MIPS I've been told, and the Emma Mobile
SoCs were one of the earlier Cortex-A9 adopters. That might have
something to do with the rather loose interpretation of the spec.

Renesas SoCs from a similar era:
AP4 (sh7372) AP4EVB (Cortex-A8 + INTCA/INTCS)
R-Mobile A1 (r8a7740) Armadillo-800-EVA (Cortex-A9 + INTCA/INTCS)
R-Car M1A (r8a7778) Bock-W (Cortex-A9 + GIC)
R-Car H1 (r8a7779) Marzen (4 x Cortex-A9 + GIC)
Emma Mobile EMEV2 KZM9D (2 x Cortex-A9 + GIC)
SH-Mobile AG5 (sh73a0) KZM9G (2 x Cortex-A9 + GIC)

The INTCA/INTCS interrupt controllers came from the SH architecture
but were phased out once SMP became the norm. I've got the majority of
the boards above hooked up for remote access if anyone wants to test
something.

> > From my basic research, "ARM Generic Interrupt Controller Architecture
> > Specification Architecture version 1.0" is documented in ARM IHI 0048A
> > from 2008 (Non-Confidential) which contains:
> > "All GIC registers are 32-bit wide." and "All registers support 32-bit
> > word access..."
> > "In addition, the following registers support byte accesses:"
> > "ICDIPR"
>
> Renamed to GICD_IPRIORITYRn in IHI0048B.
>
> > "ICDIPTR"
>
> Renamed to GICD_ITARGETRn in IHI0048B.
>
> See IHI0048B_b ("B.1 Alternative register names" and specifically
> table B-1) for the translation table between GICv1 and GICv2 names.

Thanks.

> > So the GICv1 documentation says byte access is partially supported
> > however EMEV2 documentation says 32-bit access is required.
>
> Which is definitely an integration bug. Both set of registers *must*
> support byte accesses. This isn't optional and left to the
> appreciation of the integrator. This breaks the programming model
> badly, and prevents standard software from running unmodified.

This reminds me that on SH we used to fix up I/O access alignment for
certain on-chip devices by trapping. The fast path worked well and the
special case worked but was slow.

> One of the few things the GIC architecture got right is the absence of
> locking requirements, as all the registers can be accessed
> concurrently by multiple CPUs as long as they operate on distinct
> interrupts. This is why the enable and pending registers have both set
> and clear accessors, that the priority and target registers are byte
> accessible, and that everything else happens in CPU-private registers
> (the CPU interface).

Yeah the GIC is quite nice IMO. The legacy INTC hardware often had
separate registers for setting and clearing, however priority probably
required read-modify-write. SMP wasn't an issue. =)

> This requirement has been there from day-1. Even the good old DIC (the
> GIC's ancestor) that was included with the 11MP-Core says: "All
> Interrupt Distributor Registers are byte accessible.", which is more
> than actually necessary for the GIC. See DDI 0360F for details. And
> yes, SW written for the GIC does work on the DIC.

Interesting. For some not so big reason this makes me think of Monty Python. =)

> >
> > > > +               .compatible     = "arm,pl390",
> > > > +               .init           = gic_enable_rmw_access,
> > > > +       },
> >
> > May I ask about a clarification about the EMEV2 DTS and DT binding
> > documentation in:
> > arch/arm/boot/dts/emev2.dts
> > Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml
> >
> > On EMEV2 the DT compatible string currently seems to be the rather
> > generic "arm,pl390". In the DT binding documentation GICv1 is listed
> > in an example as "arm,cortex-a9-gic". Is there any reason for not
> > using the GICv1 compatible string (and 32-bit access) for EMEV2? Just
> > curious.
>
> GICv1 is an architecture specification. PL390 is an implementation of
> GICv1. The so called "Cortex-A9 GIC" doesn't really exist. It is
> simply the amalgamation of the CPU interface implemented by the A9
> (with the prototype of the GICv2 virtualisation extensions) with a
> distributor (usually a PL390, but not necessarily). All of them
> require that the priority and target registers are byte accessible.

Makes sense.

> As for changing the compatibility string, I don't see the point. This
> will break existing setups, and doesn't change the core of the
> issue. As far as I can see, the EMEV2 DT is correct in the sense that
> it describes the actual implementation of the GIC used.

I'm all for not breaking existing setups, but EMEV2 is a pretty rare
case. So my line of thinking was that instead of punishing all GIC
platforms with this EMEV2-specific workaround it is completely fine
from my side to to special case it in DT if it makes the rest of the
code any cleaner. But it is really up to you guys.

Thanks,

/ magnus