[PATCH v3 0/2] implement workaround for Socionext Synquacer pre-ITS

[PATCH v3 0/2] implement workaround for Socionext Synquacer pre-ITS

[Ard Biesheuvel]

>From patch 2/2:

  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.

v3: - add patch to pull device ID space discovery forward, so we can quirk
      it as well (as we already do for Cavium)
    - use existing quirks framework as much as possible
    - get rid of ITS_WORKAROUND_xxx flag: it is no longer needed after the
      refactoring

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

Ard Biesheuvel (2):
  drivers/irqchip: gicv3: probe device ID space before quirks handling
  drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

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

-- 
2.11.0

[PATCH v3 1/2] drivers/irqchip: gicv3: probe device ID space before quirks handling

[Ard Biesheuvel]

Before adding another SoC whose device ID space deviates from the
value presented in the GIC ID registers, let's slightly refactor
the code so that the ID registers are probed before that quirks
handling executes. This allows us to move the device ID override
into the quirk handler itself.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index e8d89343d613..891de07fd4cc 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1650,23 +1650,14 @@ static void its_free_tables(struct its_node *its)
 
 static int its_alloc_tables(struct its_node *its)
 {
-	u64 typer = gic_read_typer(its->base + GITS_TYPER);
-	u32 ids = GITS_TYPER_DEVBITS(typer);
 	u64 shr = GITS_BASER_InnerShareable;
 	u64 cache = GITS_BASER_RaWaWb;
 	u32 psz = SZ_64K;
 	int err, i;
 
-	if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_22375) {
-		/*
-		* erratum 22375: only alloc 8MB table size
-		* erratum 24313: ignore memory access type
-		*/
-		cache   = GITS_BASER_nCnB;
-		ids     = 0x14;                 /* 20 bits, 8MB */
-	}
-
-	its->device_ids = ids;
+	if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_22375)
+		/* erratum 24313: ignore memory access type */
+		cache = GITS_BASER_nCnB;
 
 	for (i = 0; i < GITS_BASER_NR_REGS; i++) {
 		struct its_baser *baser = its->tables + i;
@@ -2741,6 +2732,8 @@ static void __maybe_unused its_enable_quirk_cavium_22375(void *data)
 {
 	struct its_node *its = data;
 
+	/* erratum 22375: only alloc 8MB table size */
+	its->device_ids = 0x14;		/* 20 bits, 8MB */
 	its->flags |= ITS_FLAGS_WORKAROUND_CAVIUM_22375;
 }
 
@@ -2942,6 +2935,7 @@ static int __init its_probe_one(struct resource *res,
 	its->base = its_base;
 	its->phys_base = res->start;
 	its->ite_size = GITS_TYPER_ITT_ENTRY_SIZE(typer);
+	its->device_ids = GITS_TYPER_DEVBITS(typer);
 	its->is_v4 = !!(typer & GITS_TYPER_VLPIS);
 	if (its->is_v4) {
 		if (!(typer & GITS_TYPER_VMOVP)) {
-- 
2.11.0

[PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

[Ard Biesheuvel]

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 <ard.biesheuvel@linaro.org>
---
 Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
 arch/arm64/Kconfig                                                    |  8 +++
 drivers/irqchip/irq-gic-v3-its.c                                      | 63 +++++++++++++++++++-
 3 files changed, 72 insertions(+), 3 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..0798a61bbf99 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, as implemented on the Socionext Synquacer SoC
+
 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 891de07fd4cc..2fc4fba0cc4c 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -97,12 +97,18 @@ struct its_node {
 	struct its_cmd_block	*cmd_write;
 	struct its_baser	tables[GITS_BASER_NR_REGS];
 	struct its_collection	*collections;
+	struct fwnode_handle	*fwnode_handle;
 	struct list_head	its_device_list;
 	u64			flags;
 	u32			ite_size;
 	u32			device_ids;
 	int			numa_node;
+	unsigned int		msi_domain_flag_mask;
 	bool			is_v4;
+
+	/* for Socionext Synquacer pre-ITS */
+	u32			pre_its_base;
+	u32			pre_its_size;
 };
 
 #define ITS_ITT_ALIGN		SZ_256
@@ -1095,14 +1101,29 @@ 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;
+
+	if (unlikely(its->pre_its_size > 0))
+		/*
+		 * 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);
+
+	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);
@@ -2752,6 +2773,27 @@ static void __maybe_unused its_enable_quirk_qdf2400_e0065(void *data)
 	its->ite_size = 16;
 }
 
+static void __maybe_unused its_enable_quirk_socionext_synquacer(void *data)
+{
+	struct its_node *its = data;
+	u32 pre_its_window[2];
+	u32 ids;
+
+	if (!fwnode_property_read_u32_array(its->fwnode_handle,
+					    "socionext,synquacer-pre-its",
+					    pre_its_window,
+					    ARRAY_SIZE(pre_its_window))) {
+		its->pre_its_base = pre_its_window[0];
+		its->pre_its_size = pre_its_window[1];
+
+		ids = ilog2(its->pre_its_size) - 2;
+		if (its->device_ids > ids)
+			its->device_ids = ids;
+
+		its->msi_domain_flag_mask = ~IRQ_DOMAIN_FLAG_MSI_REMAP;
+	}
+}
+
 static const struct gic_quirk its_quirks[] = {
 #ifdef CONFIG_CAVIUM_ERRATUM_22375
 	{
@@ -2777,6 +2819,19 @@ static const struct gic_quirk its_quirks[] = {
 		.init	= its_enable_quirk_qdf2400_e0065,
 	},
 #endif
+#ifdef CONFIG_SOCIONEXT_SYNQUACER_PREITS
+	{
+		/*
+		 * The Socionext Synquacer SoC incorporates ARM's own GIC-500
+		 * implementation, but with a 'pre-ITS' added that requires
+		 * special handling in software.
+		 */
+		.desc	= "ITS: Socionext Synquacer pre-ITS",
+		.iidr	= 0x0001143b,
+		.mask	= 0xffffffff,
+		.init	= its_enable_quirk_socionext_synquacer,
+	},
+#endif
 	{
 	}
 };
@@ -2806,6 +2861,7 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
 	inner_domain->parent = its_parent;
 	irq_domain_update_bus_token(inner_domain, DOMAIN_BUS_NEXUS);
 	inner_domain->flags |= IRQ_DOMAIN_FLAG_MSI_REMAP;
+	inner_domain->flags &= its->msi_domain_flag_mask;
 	info->ops = &its_msi_domain_ops;
 	info->data = its;
 	inner_domain->host_data = info;
@@ -2959,6 +3015,7 @@ static int __init its_probe_one(struct resource *res,
 		goto out_free_its;
 	}
 	its->cmd_write = its->cmd_base;
+	its->fwnode_handle = handle;
 
 	its_enable_quirks(its);
 
-- 
2.11.0

[PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

[Ard Biesheuvel]

On 12 October 2017 at 19:32, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 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 <ard.biesheuvel@linaro.org>
> ---
>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
>  arch/arm64/Kconfig                                                    |  8 +++
>  drivers/irqchip/irq-gic-v3-its.c                                      | 63 +++++++++++++++++++-
>  3 files changed, 72 insertions(+), 3 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..0798a61bbf99 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, as implemented on the Socionext Synquacer SoC
> +
>  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 891de07fd4cc..2fc4fba0cc4c 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -97,12 +97,18 @@ struct its_node {
>         struct its_cmd_block    *cmd_write;
>         struct its_baser        tables[GITS_BASER_NR_REGS];
>         struct its_collection   *collections;
> +       struct fwnode_handle    *fwnode_handle;
>         struct list_head        its_device_list;
>         u64                     flags;
>         u32                     ite_size;
>         u32                     device_ids;
>         int                     numa_node;
> +       unsigned int            msi_domain_flag_mask;
>         bool                    is_v4;
> +
> +       /* for Socionext Synquacer pre-ITS */
> +       u32                     pre_its_base;
> +       u32                     pre_its_size;
>  };
>
>  #define ITS_ITT_ALIGN          SZ_256
> @@ -1095,14 +1101,29 @@ 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;
> +
> +       if (unlikely(its->pre_its_size > 0))
> +               /*
> +                * 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);
> +
> +       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);
> @@ -2752,6 +2773,27 @@ static void __maybe_unused its_enable_quirk_qdf2400_e0065(void *data)
>         its->ite_size = 16;
>  }
>
> +static void __maybe_unused its_enable_quirk_socionext_synquacer(void *data)
> +{
> +       struct its_node *its = data;
> +       u32 pre_its_window[2];
> +       u32 ids;
> +
> +       if (!fwnode_property_read_u32_array(its->fwnode_handle,
> +                                           "socionext,synquacer-pre-its",
> +                                           pre_its_window,
> +                                           ARRAY_SIZE(pre_its_window))) {
> +               its->pre_its_base = pre_its_window[0];
> +               its->pre_its_size = pre_its_window[1];
> +
> +               ids = ilog2(its->pre_its_size) - 2;
> +               if (its->device_ids > ids)
> +                       its->device_ids = ids;
> +
> +               its->msi_domain_flag_mask = ~IRQ_DOMAIN_FLAG_MSI_REMAP;
> +       }
> +}
> +
>  static const struct gic_quirk its_quirks[] = {
>  #ifdef CONFIG_CAVIUM_ERRATUM_22375
>         {
> @@ -2777,6 +2819,19 @@ static const struct gic_quirk its_quirks[] = {
>                 .init   = its_enable_quirk_qdf2400_e0065,
>         },
>  #endif
> +#ifdef CONFIG_SOCIONEXT_SYNQUACER_PREITS
> +       {
> +               /*
> +                * The Socionext Synquacer SoC incorporates ARM's own GIC-500
> +                * implementation, but with a 'pre-ITS' added that requires
> +                * special handling in software.
> +                */
> +               .desc   = "ITS: Socionext Synquacer pre-ITS",
> +               .iidr   = 0x0001143b,
> +               .mask   = 0xffffffff,
> +               .init   = its_enable_quirk_socionext_synquacer,
> +       },
> +#endif
>         {
>         }
>  };
> @@ -2806,6 +2861,7 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
>         inner_domain->parent = its_parent;
>         irq_domain_update_bus_token(inner_domain, DOMAIN_BUS_NEXUS);
>         inner_domain->flags |= IRQ_DOMAIN_FLAG_MSI_REMAP;
> +       inner_domain->flags &= its->msi_domain_flag_mask;

Oops. its->msi_domain_flag_mask defaults to 0, so this breaks other platforms.

Unless there are better suggestions, I will move the ~ from the
assignment to here.



>         info->ops = &its_msi_domain_ops;
>         info->data = its;
>         inner_domain->host_data = info;
> @@ -2959,6 +3015,7 @@ static int __init its_probe_one(struct resource *res,
>                 goto out_free_its;
>         }
>         its->cmd_write = its->cmd_base;
> +       its->fwnode_handle = handle;
>
>         its_enable_quirks(its);
>
> --
> 2.11.0
>

[PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

[Marc Zyngier]

On 12/10/17 20:11, Ard Biesheuvel wrote:
> On 12 October 2017 at 19:32, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> 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 <ard.biesheuvel@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
>>  arch/arm64/Kconfig                                                    |  8 +++
>>  drivers/irqchip/irq-gic-v3-its.c                                      | 63 +++++++++++++++++++-
>>  3 files changed, 72 insertions(+), 3 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..0798a61bbf99 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, as implemented on the Socionext Synquacer SoC
>> +
>>  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 891de07fd4cc..2fc4fba0cc4c 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -97,12 +97,18 @@ struct its_node {
>>         struct its_cmd_block    *cmd_write;
>>         struct its_baser        tables[GITS_BASER_NR_REGS];
>>         struct its_collection   *collections;
>> +       struct fwnode_handle    *fwnode_handle;
>>         struct list_head        its_device_list;
>>         u64                     flags;
>>         u32                     ite_size;
>>         u32                     device_ids;
>>         int                     numa_node;
>> +       unsigned int            msi_domain_flag_mask;
>>         bool                    is_v4;
>> +
>> +       /* for Socionext Synquacer pre-ITS */
>> +       u32                     pre_its_base;
>> +       u32                     pre_its_size;
>>  };
>>
>>  #define ITS_ITT_ALIGN          SZ_256
>> @@ -1095,14 +1101,29 @@ 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;
>> +
>> +       if (unlikely(its->pre_its_size > 0))
>> +               /*
>> +                * 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);
>> +
>> +       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);
>> @@ -2752,6 +2773,27 @@ static void __maybe_unused its_enable_quirk_qdf2400_e0065(void *data)
>>         its->ite_size = 16;
>>  }
>>
>> +static void __maybe_unused its_enable_quirk_socionext_synquacer(void *data)
>> +{
>> +       struct its_node *its = data;
>> +       u32 pre_its_window[2];
>> +       u32 ids;
>> +
>> +       if (!fwnode_property_read_u32_array(its->fwnode_handle,
>> +                                           "socionext,synquacer-pre-its",
>> +                                           pre_its_window,
>> +                                           ARRAY_SIZE(pre_its_window))) {
>> +               its->pre_its_base = pre_its_window[0];
>> +               its->pre_its_size = pre_its_window[1];
>> +
>> +               ids = ilog2(its->pre_its_size) - 2;
>> +               if (its->device_ids > ids)
>> +                       its->device_ids = ids;
>> +
>> +               its->msi_domain_flag_mask = ~IRQ_DOMAIN_FLAG_MSI_REMAP;
>> +       }
>> +}
>> +
>>  static const struct gic_quirk its_quirks[] = {
>>  #ifdef CONFIG_CAVIUM_ERRATUM_22375
>>         {
>> @@ -2777,6 +2819,19 @@ static const struct gic_quirk its_quirks[] = {
>>                 .init   = its_enable_quirk_qdf2400_e0065,
>>         },
>>  #endif
>> +#ifdef CONFIG_SOCIONEXT_SYNQUACER_PREITS
>> +       {
>> +               /*
>> +                * The Socionext Synquacer SoC incorporates ARM's own GIC-500
>> +                * implementation, but with a 'pre-ITS' added that requires
>> +                * special handling in software.
>> +                */
>> +               .desc   = "ITS: Socionext Synquacer pre-ITS",
>> +               .iidr   = 0x0001143b,
>> +               .mask   = 0xffffffff,
>> +               .init   = its_enable_quirk_socionext_synquacer,
>> +       },
>> +#endif
>>         {
>>         }
>>  };
>> @@ -2806,6 +2861,7 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
>>         inner_domain->parent = its_parent;
>>         irq_domain_update_bus_token(inner_domain, DOMAIN_BUS_NEXUS);
>>         inner_domain->flags |= IRQ_DOMAIN_FLAG_MSI_REMAP;
>> +       inner_domain->flags &= its->msi_domain_flag_mask;
> 
> Oops. its->msi_domain_flag_mask defaults to 0, so this breaks other platforms.
> 
> Unless there are better suggestions, I will move the ~ from the
> assignment to here.
> 
> 
> 
>>         info->ops = &its_msi_domain_ops;
>>         info->data = its;
>>         inner_domain->host_data = info;
>> @@ -2959,6 +3015,7 @@ static int __init its_probe_one(struct resource *res,
>>                 goto out_free_its;
>>         }
>>         its->cmd_write = its->cmd_base;
>> +       its->fwnode_handle = handle;

Or initialize msi_domain_flags with its default value here, and clear it
in the quirk handler.

Other than that (and the spurious message you reported in the v2
thread), this looks pretty good to me. Let's try to converge on the DT
side so that I can queue that for 4.15.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

[Rob Herring]

On Thu, Oct 12, 2017 at 1:32 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> 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 <ard.biesheuvel@linaro.org>
> ---
>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
>  arch/arm64/Kconfig                                                    |  8 +++
>  drivers/irqchip/irq-gic-v3-its.c                                      | 63 +++++++++++++++++++-
>  3 files changed, 72 insertions(+), 3 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..0798a61bbf99 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, as implemented on the Socionext Synquacer SoC

Sounds like a separate h/w block to me and addresses should be in
"reg". I would suggest you define a separate node for the pre-its
block and then use of_find_compatible_node() from the GIC driver to
retrieve the node and whatever you need from it. Or do an SoC specific
compatible string for the GIC and let that imply whatever information
you need. Then the next quirk doesn't need a DT update.

Rob

[PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

[Ard Biesheuvel]

On 12 October 2017 at 22:34, Rob Herring <robh+dt@kernel.org> wrote:
> On Thu, Oct 12, 2017 at 1:32 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> 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 <ard.biesheuvel@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
>>  arch/arm64/Kconfig                                                    |  8 +++
>>  drivers/irqchip/irq-gic-v3-its.c                                      | 63 +++++++++++++++++++-
>>  3 files changed, 72 insertions(+), 3 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..0798a61bbf99 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, as implemented on the Socionext Synquacer SoC
>
> Sounds like a separate h/w block to me and addresses should be in
> "reg". I would suggest you define a separate node for the pre-its
> block and then use of_find_compatible_node() from the GIC driver to
> retrieve the node and whatever you need from it. Or do an SoC specific
> compatible string for the GIC and let that imply whatever information
> you need. Then the next quirk doesn't need a DT update.
>

For my understanding, you mean either

    gic: interrupt-controller at 30000000 {
        compatible = "arm,gic-v3";
        ...

        its: gic-its at 30020000 {
            compatible = "arm,gic-v3-its";
            reg = <0x0 0x30020000 0x0 0x20000>;
            #msi-cells = <1>;
            msi-controller;
        };
    };

    preits at 58000000 {
        compatible = "socionext,synquacer-pre-its";
        reg = <0x0 0x58000000 0x0 0x200000>;
        msi-slave = <&its>;
    };

or

    gic: interrupt-controller at 30000000 {
        compatible = "arm,gic-v3";
        ...

        its: gic-its at 30020000 {
            compatible = "socionext,synquacer-pre-its", "arm,gic-v3-its";
            reg = <0x0 0x30020000 0x0 0x20000>,
                  <0x0 0x58000000 0x0 0x200000>;
            #msi-cells = <1>;
            msi-controller;
        };
    };

right?

Marc, what do you think? IIRC we did discuss option #2 at some point,
but I don't remember if/why we rejected it.

[PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

[Marc Zyngier]

[+Mark]

On 12/10/17 23:24, Ard Biesheuvel wrote:
> On 12 October 2017 at 22:34, Rob Herring <robh+dt@kernel.org> wrote:
>> On Thu, Oct 12, 2017 at 1:32 PM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> 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 <ard.biesheuvel@linaro.org>
>>> ---
>>>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
>>>  arch/arm64/Kconfig                                                    |  8 +++
>>>  drivers/irqchip/irq-gic-v3-its.c                                      | 63 +++++++++++++++++++-
>>>  3 files changed, 72 insertions(+), 3 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..0798a61bbf99 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, as implemented on the Socionext Synquacer SoC
>>
>> Sounds like a separate h/w block to me and addresses should be in
>> "reg". I would suggest you define a separate node for the pre-its
>> block and then use of_find_compatible_node() from the GIC driver to
>> retrieve the node and whatever you need from it. Or do an SoC specific
>> compatible string for the GIC and let that imply whatever information
>> you need. Then the next quirk doesn't need a DT update.
>>
> 
> For my understanding, you mean either
> 
>     gic: interrupt-controller at 30000000 {
>         compatible = "arm,gic-v3";
>         ...
> 
>         its: gic-its at 30020000 {
>             compatible = "arm,gic-v3-its";
>             reg = <0x0 0x30020000 0x0 0x20000>;
>             #msi-cells = <1>;
>             msi-controller;
>         };
>     };
> 
>     preits at 58000000 {
>         compatible = "socionext,synquacer-pre-its";
>         reg = <0x0 0x58000000 0x0 0x200000>;
>         msi-slave = <&its>;
>     };
> 
> or
> 
>     gic: interrupt-controller at 30000000 {
>         compatible = "arm,gic-v3";
>         ...
> 
>         its: gic-its at 30020000 {
>             compatible = "socionext,synquacer-pre-its", "arm,gic-v3-its";
>             reg = <0x0 0x30020000 0x0 0x20000>,
>                   <0x0 0x58000000 0x0 0x200000>;
>             #msi-cells = <1>;
>             msi-controller;
>         };
>     };
> 
> right?
> 
> Marc, what do you think? IIRC we did discuss option #2 at some point,
> but I don't remember if/why we rejected it.

I dislike #2 because these registers are not part of the regular ITS,
and would get in the way of potential extensions of the ITS (I don't
know of any, but just in case...).

I also dislike #1 as the "msi-slave" part is both ugly and confusing
(are we writing to the ITS? to the pre-ITS?).

How about putting the pre-its node *inside* the its node itself? It
makes it obvious which pre-ITS corresponds to which ITS, and it leaves
the ITS regs untouched.

Thoughts?

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

[Ard Biesheuvel]

On 13 October 2017 at 10:47, Marc Zyngier <marc.zyngier@arm.com> wrote:
> [+Mark]
>
> On 12/10/17 23:24, Ard Biesheuvel wrote:
>> On 12 October 2017 at 22:34, Rob Herring <robh+dt@kernel.org> wrote:
>>> On Thu, Oct 12, 2017 at 1:32 PM, Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org> wrote:
>>>> 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 <ard.biesheuvel@linaro.org>
>>>> ---
>>>>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
>>>>  arch/arm64/Kconfig                                                    |  8 +++
>>>>  drivers/irqchip/irq-gic-v3-its.c                                      | 63 +++++++++++++++++++-
>>>>  3 files changed, 72 insertions(+), 3 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..0798a61bbf99 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, as implemented on the Socionext Synquacer SoC
>>>
>>> Sounds like a separate h/w block to me and addresses should be in
>>> "reg". I would suggest you define a separate node for the pre-its
>>> block and then use of_find_compatible_node() from the GIC driver to
>>> retrieve the node and whatever you need from it. Or do an SoC specific
>>> compatible string for the GIC and let that imply whatever information
>>> you need. Then the next quirk doesn't need a DT update.
>>>
>>
>> For my understanding, you mean either
>>
>>     gic: interrupt-controller at 30000000 {
>>         compatible = "arm,gic-v3";
>>         ...
>>
>>         its: gic-its at 30020000 {
>>             compatible = "arm,gic-v3-its";
>>             reg = <0x0 0x30020000 0x0 0x20000>;
>>             #msi-cells = <1>;
>>             msi-controller;
>>         };
>>     };
>>
>>     preits at 58000000 {
>>         compatible = "socionext,synquacer-pre-its";
>>         reg = <0x0 0x58000000 0x0 0x200000>;
>>         msi-slave = <&its>;
>>     };
>>
>> or
>>
>>     gic: interrupt-controller at 30000000 {
>>         compatible = "arm,gic-v3";
>>         ...
>>
>>         its: gic-its at 30020000 {
>>             compatible = "socionext,synquacer-pre-its", "arm,gic-v3-its";
>>             reg = <0x0 0x30020000 0x0 0x20000>,
>>                   <0x0 0x58000000 0x0 0x200000>;
>>             #msi-cells = <1>;
>>             msi-controller;
>>         };
>>     };
>>
>> right?
>>
>> Marc, what do you think? IIRC we did discuss option #2 at some point,
>> but I don't remember if/why we rejected it.
>
> I dislike #2 because these registers are not part of the regular ITS,
> and would get in the way of potential extensions of the ITS (I don't
> know of any, but just in case...).
>

I see.

> I also dislike #1 as the "msi-slave" part is both ugly and confusing
> (are we writing to the ITS? to the pre-ITS?).
>
> How about putting the pre-its node *inside* the its node itself? It
> makes it obvious which pre-ITS corresponds to which ITS, and it leaves
> the ITS regs untouched.
>

Something like this?

 gic: interrupt-controller at 30000000 {
     compatible = "arm,gic-v3";
     ...

     its: gic-its at 30020000 {
         compatible = "arm,gic-v3-its";
         reg = <0x0 0x30020000 0x0 0x20000>;
         #msi-cells = <1>;
         msi-controller;

+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges;
+
+        preits at 58000000 {
+            compatible = "socionext,synquacer-pre-its";
+            reg = <0x58000000 0x200000>;
+        };
     };
 };

[PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

[Marc Zyngier]

On 13/10/17 10:55, Ard Biesheuvel wrote:
> On 13 October 2017 at 10:47, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> [+Mark]
>>
>> On 12/10/17 23:24, Ard Biesheuvel wrote:
>>> On 12 October 2017 at 22:34, Rob Herring <robh+dt@kernel.org> wrote:
>>>> On Thu, Oct 12, 2017 at 1:32 PM, Ard Biesheuvel
>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>> 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 <ard.biesheuvel@linaro.org>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
>>>>>  arch/arm64/Kconfig                                                    |  8 +++
>>>>>  drivers/irqchip/irq-gic-v3-its.c                                      | 63 +++++++++++++++++++-
>>>>>  3 files changed, 72 insertions(+), 3 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..0798a61bbf99 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, as implemented on the Socionext Synquacer SoC
>>>>
>>>> Sounds like a separate h/w block to me and addresses should be in
>>>> "reg". I would suggest you define a separate node for the pre-its
>>>> block and then use of_find_compatible_node() from the GIC driver to
>>>> retrieve the node and whatever you need from it. Or do an SoC specific
>>>> compatible string for the GIC and let that imply whatever information
>>>> you need. Then the next quirk doesn't need a DT update.
>>>>
>>>
>>> For my understanding, you mean either
>>>
>>>     gic: interrupt-controller at 30000000 {
>>>         compatible = "arm,gic-v3";
>>>         ...
>>>
>>>         its: gic-its at 30020000 {
>>>             compatible = "arm,gic-v3-its";
>>>             reg = <0x0 0x30020000 0x0 0x20000>;
>>>             #msi-cells = <1>;
>>>             msi-controller;
>>>         };
>>>     };
>>>
>>>     preits at 58000000 {
>>>         compatible = "socionext,synquacer-pre-its";
>>>         reg = <0x0 0x58000000 0x0 0x200000>;
>>>         msi-slave = <&its>;
>>>     };
>>>
>>> or
>>>
>>>     gic: interrupt-controller at 30000000 {
>>>         compatible = "arm,gic-v3";
>>>         ...
>>>
>>>         its: gic-its at 30020000 {
>>>             compatible = "socionext,synquacer-pre-its", "arm,gic-v3-its";
>>>             reg = <0x0 0x30020000 0x0 0x20000>,
>>>                   <0x0 0x58000000 0x0 0x200000>;
>>>             #msi-cells = <1>;
>>>             msi-controller;
>>>         };
>>>     };
>>>
>>> right?
>>>
>>> Marc, what do you think? IIRC we did discuss option #2 at some point,
>>> but I don't remember if/why we rejected it.
>>
>> I dislike #2 because these registers are not part of the regular ITS,
>> and would get in the way of potential extensions of the ITS (I don't
>> know of any, but just in case...).
>>
> 
> I see.
> 
>> I also dislike #1 as the "msi-slave" part is both ugly and confusing
>> (are we writing to the ITS? to the pre-ITS?).
>>
>> How about putting the pre-its node *inside* the its node itself? It
>> makes it obvious which pre-ITS corresponds to which ITS, and it leaves
>> the ITS regs untouched.
>>
> 
> Something like this?
> 
>  gic: interrupt-controller at 30000000 {
>      compatible = "arm,gic-v3";
>      ...
> 
>      its: gic-its at 30020000 {
>          compatible = "arm,gic-v3-its";
>          reg = <0x0 0x30020000 0x0 0x20000>;
>          #msi-cells = <1>;
>          msi-controller;
> 
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges;
> +
> +        preits at 58000000 {
> +            compatible = "socionext,synquacer-pre-its";
> +            reg = <0x58000000 0x200000>;
> +        };
>      };
>  };
> 

I'd be happy enough with that.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

[Rob Herring]

On Fri, Oct 13, 2017 at 4:47 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> [+Mark]
>
> On 12/10/17 23:24, Ard Biesheuvel wrote:
>> On 12 October 2017 at 22:34, Rob Herring <robh+dt@kernel.org> wrote:
>>> On Thu, Oct 12, 2017 at 1:32 PM, Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org> wrote:
>>>> 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 <ard.biesheuvel@linaro.org>
>>>> ---
>>>>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
>>>>  arch/arm64/Kconfig                                                    |  8 +++
>>>>  drivers/irqchip/irq-gic-v3-its.c                                      | 63 +++++++++++++++++++-
>>>>  3 files changed, 72 insertions(+), 3 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..0798a61bbf99 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, as implemented on the Socionext Synquacer SoC
>>>
>>> Sounds like a separate h/w block to me and addresses should be in
>>> "reg". I would suggest you define a separate node for the pre-its
>>> block and then use of_find_compatible_node() from the GIC driver to
>>> retrieve the node and whatever you need from it. Or do an SoC specific
>>> compatible string for the GIC and let that imply whatever information
>>> you need. Then the next quirk doesn't need a DT update.
>>>
>>
>> For my understanding, you mean either
>>
>>     gic: interrupt-controller at 30000000 {
>>         compatible = "arm,gic-v3";
>>         ...
>>
>>         its: gic-its at 30020000 {
>>             compatible = "arm,gic-v3-its";
>>             reg = <0x0 0x30020000 0x0 0x20000>;
>>             #msi-cells = <1>;
>>             msi-controller;
>>         };
>>     };
>>
>>     preits at 58000000 {
>>         compatible = "socionext,synquacer-pre-its";
>>         reg = <0x0 0x58000000 0x0 0x200000>;
>>         msi-slave = <&its>;
>>     };
>>
>> or
>>
>>     gic: interrupt-controller at 30000000 {
>>         compatible = "arm,gic-v3";
>>         ...
>>
>>         its: gic-its at 30020000 {
>>             compatible = "socionext,synquacer-pre-its", "arm,gic-v3-its";
>>             reg = <0x0 0x30020000 0x0 0x20000>,
>>                   <0x0 0x58000000 0x0 0x200000>;
>>             #msi-cells = <1>;
>>             msi-controller;
>>         };
>>     };
>>
>> right?
>>
>> Marc, what do you think? IIRC we did discuss option #2 at some point,
>> but I don't remember if/why we rejected it.
>
> I dislike #2 because these registers are not part of the regular ITS,
> and would get in the way of potential extensions of the ITS (I don't
> know of any, but just in case...).
>
> I also dislike #1 as the "msi-slave" part is both ugly and confusing
> (are we writing to the ITS? to the pre-ITS?).

Why do you need this? Are you ever going to have multiple pre-its's?
That's why I suggested just using of_find_compatible_node().

> How about putting the pre-its node *inside* the its node itself? It
> makes it obvious which pre-ITS corresponds to which ITS, and it leaves
> the ITS regs untouched.

Not wild about this, but it's fine if you do need to handle multiple instances.

Rob

[PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

[Ard Biesheuvel]

On 13 October 2017 at 19:50, Rob Herring <robh+dt@kernel.org> wrote:
> On Fri, Oct 13, 2017 at 4:47 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> [+Mark]
>>
>> On 12/10/17 23:24, Ard Biesheuvel wrote:
>>> On 12 October 2017 at 22:34, Rob Herring <robh+dt@kernel.org> wrote:
>>>> On Thu, Oct 12, 2017 at 1:32 PM, Ard Biesheuvel
>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>> 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 <ard.biesheuvel@linaro.org>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
>>>>>  arch/arm64/Kconfig                                                    |  8 +++
>>>>>  drivers/irqchip/irq-gic-v3-its.c                                      | 63 +++++++++++++++++++-
>>>>>  3 files changed, 72 insertions(+), 3 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..0798a61bbf99 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, as implemented on the Socionext Synquacer SoC
>>>>
>>>> Sounds like a separate h/w block to me and addresses should be in
>>>> "reg". I would suggest you define a separate node for the pre-its
>>>> block and then use of_find_compatible_node() from the GIC driver to
>>>> retrieve the node and whatever you need from it. Or do an SoC specific
>>>> compatible string for the GIC and let that imply whatever information
>>>> you need. Then the next quirk doesn't need a DT update.
>>>>
>>>
>>> For my understanding, you mean either
>>>
>>>     gic: interrupt-controller at 30000000 {
>>>         compatible = "arm,gic-v3";
>>>         ...
>>>
>>>         its: gic-its at 30020000 {
>>>             compatible = "arm,gic-v3-its";
>>>             reg = <0x0 0x30020000 0x0 0x20000>;
>>>             #msi-cells = <1>;
>>>             msi-controller;
>>>         };
>>>     };
>>>
>>>     preits at 58000000 {
>>>         compatible = "socionext,synquacer-pre-its";
>>>         reg = <0x0 0x58000000 0x0 0x200000>;
>>>         msi-slave = <&its>;
>>>     };
>>>
>>> or
>>>
>>>     gic: interrupt-controller at 30000000 {
>>>         compatible = "arm,gic-v3";
>>>         ...
>>>
>>>         its: gic-its at 30020000 {
>>>             compatible = "socionext,synquacer-pre-its", "arm,gic-v3-its";
>>>             reg = <0x0 0x30020000 0x0 0x20000>,
>>>                   <0x0 0x58000000 0x0 0x200000>;
>>>             #msi-cells = <1>;
>>>             msi-controller;
>>>         };
>>>     };
>>>
>>> right?
>>>
>>> Marc, what do you think? IIRC we did discuss option #2 at some point,
>>> but I don't remember if/why we rejected it.
>>
>> I dislike #2 because these registers are not part of the regular ITS,
>> and would get in the way of potential extensions of the ITS (I don't
>> know of any, but just in case...).
>>
>> I also dislike #1 as the "msi-slave" part is both ugly and confusing
>> (are we writing to the ITS? to the pre-ITS?).
>
> Why do you need this? Are you ever going to have multiple pre-its's?

The question is really whether we are ever going to have multiple
ITSes, in which case we will need to identify which ITS this single
pre-ITS is associated with, given that it can only target one. And
while I would prefer zero pre-ITSes in all cases [as would Marc, I'm
sure], having a pre-ITS for each ITS frame is not unimaginable either.
So yes, we need to record this information in one way or the other.

> That's why I suggested just using of_find_compatible_node().
>
>> How about putting the pre-its node *inside* the its node itself? It
>> makes it obvious which pre-ITS corresponds to which ITS, and it leaves
>> the ITS regs untouched.
>
> Not wild about this, but it's fine if you do need to handle multiple instances.
>
> Rob

[PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

[Marc Zyngier]

On Fri, Oct 13 2017 at  9:38:02 pm BST, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 13 October 2017 at 19:50, Rob Herring <robh+dt@kernel.org> wrote:
>> On Fri, Oct 13, 2017 at 4:47 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> [+Mark]
>>>
>>> On 12/10/17 23:24, Ard Biesheuvel wrote:
>>>> On 12 October 2017 at 22:34, Rob Herring <robh+dt@kernel.org> wrote:
>>>>> On Thu, Oct 12, 2017 at 1:32 PM, Ard Biesheuvel
>>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>> 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 <ard.biesheuvel@linaro.org>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
>>>>>>  arch/arm64/Kconfig                                                    |  8 +++
>>>>>>  drivers/irqchip/irq-gic-v3-its.c                                      | 63 +++++++++++++++++++-
>>>>>>  3 files changed, 72 insertions(+), 3 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..0798a61bbf99 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, as implemented on the Socionext Synquacer SoC
>>>>>
>>>>> Sounds like a separate h/w block to me and addresses should be in
>>>>> "reg". I would suggest you define a separate node for the pre-its
>>>>> block and then use of_find_compatible_node() from the GIC driver to
>>>>> retrieve the node and whatever you need from it. Or do an SoC specific
>>>>> compatible string for the GIC and let that imply whatever information
>>>>> you need. Then the next quirk doesn't need a DT update.
>>>>>
>>>>
>>>> For my understanding, you mean either
>>>>
>>>>     gic: interrupt-controller at 30000000 {
>>>>         compatible = "arm,gic-v3";
>>>>         ...
>>>>
>>>>         its: gic-its at 30020000 {
>>>>             compatible = "arm,gic-v3-its";
>>>>             reg = <0x0 0x30020000 0x0 0x20000>;
>>>>             #msi-cells = <1>;
>>>>             msi-controller;
>>>>         };
>>>>     };
>>>>
>>>>     preits at 58000000 {
>>>>         compatible = "socionext,synquacer-pre-its";
>>>>         reg = <0x0 0x58000000 0x0 0x200000>;
>>>>         msi-slave = <&its>;
>>>>     };
>>>>
>>>> or
>>>>
>>>>     gic: interrupt-controller at 30000000 {
>>>>         compatible = "arm,gic-v3";
>>>>         ...
>>>>
>>>>         its: gic-its at 30020000 {
>>>>             compatible = "socionext,synquacer-pre-its", "arm,gic-v3-its";
>>>>             reg = <0x0 0x30020000 0x0 0x20000>,
>>>>                   <0x0 0x58000000 0x0 0x200000>;
>>>>             #msi-cells = <1>;
>>>>             msi-controller;
>>>>         };
>>>>     };
>>>>
>>>> right?
>>>>
>>>> Marc, what do you think? IIRC we did discuss option #2 at some point,
>>>> but I don't remember if/why we rejected it.
>>>
>>> I dislike #2 because these registers are not part of the regular ITS,
>>> and would get in the way of potential extensions of the ITS (I don't
>>> know of any, but just in case...).
>>>
>>> I also dislike #1 as the "msi-slave" part is both ugly and confusing
>>> (are we writing to the ITS? to the pre-ITS?).
>>
>> Why do you need this? Are you ever going to have multiple pre-its's?
>
> The question is really whether we are ever going to have multiple
> ITSes, in which case we will need to identify which ITS this single
> pre-ITS is associated with, given that it can only target one. And
> while I would prefer zero pre-ITSes in all cases [as would Marc, I'm
> sure], having a pre-ITS for each ITS frame is not unimaginable either.
> So yes, we need to record this information in one way or the other.

Multi-ITS systems are out there. The box I'm using for a large part of
my GICv4 testing has 8 of them. It is quite common to place ITSs close
to the PCIe RC, and multi-socket systems usually have at least one per
socket.

Hopefully the pre-ITS concept is a one-off nightmare, something we'll
laugh about in two years from now. Maybe.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

[PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

[Marc Zyngier]

On Fri, Oct 13 2017 at  1:50:06 pm BST, Rob Herring <robh+dt@kernel.org> wrote:
> On Fri, Oct 13, 2017 at 4:47 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> [+Mark]
>>
>> On 12/10/17 23:24, Ard Biesheuvel wrote:
>>> On 12 October 2017 at 22:34, Rob Herring <robh+dt@kernel.org> wrote:
>>>> On Thu, Oct 12, 2017 at 1:32 PM, Ard Biesheuvel
>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>> 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 <ard.biesheuvel@linaro.org>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
>>>>>  arch/arm64/Kconfig                                                    |  8 +++
>>>>>  drivers/irqchip/irq-gic-v3-its.c                                      | 63 +++++++++++++++++++-
>>>>>  3 files changed, 72 insertions(+), 3 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..0798a61bbf99 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, as implemented on the Socionext Synquacer SoC
>>>>
>>>> Sounds like a separate h/w block to me and addresses should be in
>>>> "reg". I would suggest you define a separate node for the pre-its
>>>> block and then use of_find_compatible_node() from the GIC driver to
>>>> retrieve the node and whatever you need from it. Or do an SoC specific
>>>> compatible string for the GIC and let that imply whatever information
>>>> you need. Then the next quirk doesn't need a DT update.
>>>>
>>>
>>> For my understanding, you mean either
>>>
>>>     gic: interrupt-controller at 30000000 {
>>>         compatible = "arm,gic-v3";
>>>         ...
>>>
>>>         its: gic-its at 30020000 {
>>>             compatible = "arm,gic-v3-its";
>>>             reg = <0x0 0x30020000 0x0 0x20000>;
>>>             #msi-cells = <1>;
>>>             msi-controller;
>>>         };
>>>     };
>>>
>>>     preits at 58000000 {
>>>         compatible = "socionext,synquacer-pre-its";
>>>         reg = <0x0 0x58000000 0x0 0x200000>;
>>>         msi-slave = <&its>;
>>>     };
>>>
>>> or
>>>
>>>     gic: interrupt-controller at 30000000 {
>>>         compatible = "arm,gic-v3";
>>>         ...
>>>
>>>         its: gic-its at 30020000 {
>>>             compatible = "socionext,synquacer-pre-its", "arm,gic-v3-its";
>>>             reg = <0x0 0x30020000 0x0 0x20000>,
>>>                   <0x0 0x58000000 0x0 0x200000>;
>>>             #msi-cells = <1>;
>>>             msi-controller;
>>>         };
>>>     };
>>>
>>> right?
>>>
>>> Marc, what do you think? IIRC we did discuss option #2 at some point,
>>> but I don't remember if/why we rejected it.
>>
>> I dislike #2 because these registers are not part of the regular ITS,
>> and would get in the way of potential extensions of the ITS (I don't
>> know of any, but just in case...).
>>
>> I also dislike #1 as the "msi-slave" part is both ugly and confusing
>> (are we writing to the ITS? to the pre-ITS?).
>
> Why do you need this? Are you ever going to have multiple pre-its's?
> That's why I suggested just using of_find_compatible_node().

Who knows? This thing should have never existed the first place, but
someone felt the need to add this horror. I wouldn't be surprised if the
same vendor came up with the same broken design in a "bigger and better"
version of this SoC.

>> How about putting the pre-its node *inside* the its node itself? It
>> makes it obvious which pre-ITS corresponds to which ITS, and it leaves
>> the ITS regs untouched.
>
> Not wild about this, but it's fine if you do need to handle multiple
> instances.

Multiple ITS instances are pretty common. And broken designs seem to be
the norm rather than the exception, unfortunately.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

[PATCH v3 2/2] drivers/irqchip: gicv3: add workaround for Synquacer pre-ITS

[Rob Herring]

On Thu, Oct 12, 2017 at 4:34 PM, Rob Herring <robh+dt@kernel.org> wrote:
> On Thu, Oct 12, 2017 at 1:32 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> 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 <ard.biesheuvel@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt |  4 ++
>>  arch/arm64/Kconfig                                                    |  8 +++
>>  drivers/irqchip/irq-gic-v3-its.c                                      | 63 +++++++++++++++++++-
>>  3 files changed, 72 insertions(+), 3 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..0798a61bbf99 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, as implemented on the Socionext Synquacer SoC
>
> Sounds like a separate h/w block to me and addresses should be in
> "reg". I would suggest you define a separate node for the pre-its
> block and then use of_find_compatible_node() from the GIC driver to
> retrieve the node and whatever you need from it. Or do an SoC specific
> compatible string for the GIC and let that imply whatever information
> you need. Then the next quirk doesn't need a DT update.

Based on further irc discussion about what this pre-its thing is, I
think the property here is fine after all.

Acked-by: Rob Herring <robh@kernel.org>

Rob