Hi Magnus,
On Sat, 11 Sep 2021 03:49:20 +0100, Magnus Damm magnus.damm@gmail.com wrote:
Hi Geert, Mark, RMK, everyone,
Thanks for your efforts. Let me just chime in with a few details and a question.
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 accessIndeed, 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?
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.
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.
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).
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.
.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.
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.
Thanks,
M.