Re: [PATCH v2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

On 12 October 2017 at 08:49, Marc Zyngier [off-list ref] wrote:

Hi Ard,

On 12/10/17 07:58, Ard Biesheuvel wrote:

quoted

The Socionext Synquacer SoC's implementation of GICv3 has a so-called
'pre-ITS', which maps 32-bit writes targeted at a separate window of
size '4 << device_id_bits' onto writes to GITS_TRANSLATER with device
ID taken from bits [device_id_bits + 1:2] of the window offset.
Writes that target GITS_TRANSLATER directly are reported as originating
from device ID #0.

So add a workaround for this. Given that this breaks isolation, clear
the IRQ_DOMAIN_FLAG_MSI_REMAP flag as well.

Signed-off-by: Ard Biesheuvel <redacted>
---
v2: - use a 32-bit host address/size rather than a PCI address, to factor
      out the involvement of an SMMU (which the platform does have, but it
      is unclear atm if it can be exposed to the OS)
    - add msi_domain_flags member to move the quirk flag checks out of the
      common code path

 Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
 arch/arm64/Kconfig                                                    |  8 +++
 drivers/irqchip/irq-gic-v3-its.c                                      | 53 +++++++++++++++++---
 3 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
index 4c29cdab0ea5..3d8b3f910aef 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt

@@ -75,6 +75,10 @@ These nodes must have the following properties:
 - reg: Specifies the base physical address and size of the ITS
   registers.

+Optional:
+- socionext,synquacer-pre-its: (u32, u32) tuple describing the host address
+  and size of the pre-ITS window.
+
 The main GIC node must contain the appropriate #address-cells,
 #size-cells and ranges properties for the reg property of all ITS
 nodes.

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 0df64a6a56d4..c4361dff2b74 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig

@@ -539,6 +539,14 @@ config QCOM_QDF2400_ERRATUM_0065

        If unsure, say Y.

+config SOCIONEXT_SYNQUACER_PREITS
+     bool "Socionext Synquacer: Workaround for GICv3 pre-ITS"
+     default y
+     help
+       Socionext Synquacer SoCs implement a separate h/w block to generate
+       MSI doorbell writes with non-zero values for the device ID.
+
+       If unsure, say Y.
 endmenu

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index e8d89343d613..7a4536ce8e72 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c

@@ -46,6 +46,7 @@
 #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING                (1ULL << 0)
 #define ITS_FLAGS_WORKAROUND_CAVIUM_22375    (1ULL << 1)
 #define ITS_FLAGS_WORKAROUND_CAVIUM_23144    (1ULL << 2)
+#define ITS_FLAGS_WORKAROUND_SOCIONEXT_PREITS        (1ULL << 3)

 #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING  (1 << 0)

@@ -102,6 +103,11 @@ struct its_node {
      u32                     ite_size;
      u32                     device_ids;
      int                     numa_node;
+     unsigned int            msi_domain_flags;
+#ifdef CONFIG_SOCIONEXT_SYNQUACER_PREITS
+     u32                     pre_its_base;
+     u32                     pre_its_size;
+#endif
      bool                    is_v4;
 };

@@ -1095,14 +1101,31 @@ static int its_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
      return IRQ_SET_MASK_OK_DONE;
 }

+static u64 its_irq_get_msi_base(struct its_device *its_dev)
+{
+     struct its_node *its = its_dev->its;
+
+#ifdef CONFIG_SOCIONEXT_SYNQUACER_PREITS
+     if (its->flags & ITS_FLAGS_WORKAROUND_SOCIONEXT_PREITS)
+
+             /*
+              * The Socionext Synquacer SoC has a so-called 'pre-ITS',
+              * which maps 32-bit writes targeted at a separate window of
+              * size '4 << device_id_bits' onto writes to GITS_TRANSLATER
+              * with device ID taken from bits [device_id_bits + 1:2] of
+              * the window offset.
+              */
+             return its->pre_its_base + (its_dev->device_id << 2);
+#endif
+     return its->phys_base + GITS_TRANSLATER;
+}
+
 static void its_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
 {
      struct its_device *its_dev = irq_data_get_irq_chip_data(d);
-     struct its_node *its;
      u64 addr;

-     its = its_dev->its;
-     addr = its->phys_base + GITS_TRANSLATER;
+     addr = its_irq_get_msi_base(its_dev);

      msg->address_lo         = lower_32_bits(addr);
      msg->address_hi         = upper_32_bits(addr);

@@ -1666,6 +1689,11 @@ static int its_alloc_tables(struct its_node *its)
              ids     = 0x14;                 /* 20 bits, 8MB */
      }

+#ifdef CONFIG_SOCIONEXT_SYNQUACER_PREITS
+     if (its->flags & ITS_FLAGS_WORKAROUND_SOCIONEXT_PREITS)
+             ids = ilog2(its->pre_its_size) - 2;

You probably want to check that this is smaller or equal to the ITS's
own view of the DevID space.

quoted

+#endif
+
      its->device_ids = ids;

      for (i = 0; i < GITS_BASER_NR_REGS; i++) {

@@ -2788,11 +2816,22 @@ static const struct gic_quirk its_quirks[] = {
      }
 };

-static void its_enable_quirks(struct its_node *its)
+static void its_enable_quirks(struct its_node *its,
+                           struct fwnode_handle *handle)
 {
      u32 iidr = readl_relaxed(its->base + GITS_IIDR);

      gic_enable_quirks(iidr, its_quirks, its);
+
+#ifdef CONFIG_SOCIONEXT_SYNQUACER_PREITS
+     if (!fwnode_property_read_u32_array(handle,
+                                         "socionext,synquacer-pre-its",
+                                         &its->pre_its_base, 2)) {

Erm. Populating these two fields with a single read is pretty horrible...

quoted

+             its->flags |= ITS_FLAGS_WORKAROUND_SOCIONEXT_PREITS;
+             its->msi_domain_flags &= ~IRQ_DOMAIN_FLAG_MSI_REMAP;
+             pr_info("ITS: enabling workaround for Socionext Synquacer pre-ITS\n");
+     }
+#endif

I must say I'm not fond of adding a quirk outside of the quirk
infrastructure. Could you try this slightly different approach?

- Add the handle to the struct its_node
- Add a quirk structure to the its_quirks array, with a match on
whatever version of the GIC this platform is using (I suppose some
revision of GIC-500)

After spinning v3 with this change, I realised that this will result in the

GIC: enabling workaround for xxx

message to be printed for all GIC-500s, given that this is triggered
by the IIDR match itself.

I will add a patch to the series to change the quirk->init() prototype
to return a bool indicating whether it matched or not.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html