diff mbox series

[2/2] irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing

Message ID 20230905104721.52199-3-lpieralisi@kernel.org
State New
Headers show
Series irqchip/gic-v3: Enable non-coherent GIC designs probing | expand

Commit Message

Lorenzo Pieralisi Sept. 5, 2023, 10:47 a.m. UTC
The GIC architecture specification defines a set of registers
for redistributors and ITSes that control the sharebility and
cacheability attributes of redistributors/ITSes initiator ports
on the interconnect (GICR_[V]PROPBASER, GICR_[V]PENDBASER,
GITS_BASER<n>).

Architecturally the GIC provides a means to drive shareability
and cacheability attributes signals and related IWB/OWB/ISH barriers
but it is not mandatory for designs to wire up the corresponding
interconnect signals that control the cacheability/shareability
of transactions.

Redistributors and ITSes interconnect ports can be connected to
non-coherent interconnects that are not able to manage the
shareability/cacheability attributes; this implicitly makes
the redistributors and ITSes non-coherent observers.

So far, the GIC driver on probe executes a write to "probe" for
the redistributors and ITSes registers shareability bitfields
by writing a value (ie InnerShareable - the shareability domain the
CPUs are in) and check it back to detect whether the value sticks or
not; this hinges on a GIC programming model behaviour that predates the
current specifications, that just define shareability bits as writeable
but do not guarantee that writing certain shareability values
enable the expected behaviour for the redistributors/ITSes
memory interconnect ports.

To enable non-coherent GIC designs, introduce the "dma-noncoherent"
device tree property to allow firmware to describe redistributors and
ITSes as non-coherent observers on the memory interconnect and use the
property to force the shareability attributes to be programmed into the
redistributors and ITSes registers.

Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Comments

Lorenzo Pieralisi Oct. 3, 2023, 2:43 p.m. UTC | #1
On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote:

[...]

> >  	 * Make sure *all* the ITS are reset before we probe any, as
> >  	 * they may be sharing memory. If any of the ITS fails to
> > @@ -5396,7 +5405,8 @@ static int __init its_of_probe(struct device_node *node)
> >  			continue;
> >  		}
> >  
> > -		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
> > +		its_probe_one(&res, &np->fwnode, of_node_to_nid(np),
> > +			      of_property_read_bool(np, "dma-noncoherent"));
> >  	}
> >  	return 0;
> >  }
> > @@ -5533,7 +5543,8 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
> >  	}
> >  
> >  	err = its_probe_one(&res, dom_handle,
> > -			acpi_get_its_numa_node(its_entry->translation_id));
> > +			acpi_get_its_numa_node(its_entry->translation_id),
> > +			false);
> 
> I came up with the following alternative approach, which is as usual
> completely untested. It is entirely based on the quirk infrastructure,
> and doesn't touch the ACPI path at all.

Writing the ACPI bits. We can't use the quirks framework for ACPI (we
don't have "properties" and I don't think we want to attach any to the
fwnode_handle) that's why I generalized its_probe_one() above with an
extra param, that would have simplified ACPI parsing:

- we alloc struct its_node in its_probe_one() but at that stage
  ACPI parsing was already done. If we have to parse the MADT(ITS) again
  just to scan for non-coherent we then have to match the MADT entries
  to the *current* struct its_node* we are handling (MADT parsing
  callbacks don't even take a param - we have to resort to global
  variables - definitely doable but it is a bit ugly).

I will post the resulting code (or something else if I find anything
nicer) but I thought I would mention the reasoning behind the additional
bool flag above.

Lorenzo

> 
> Thanks,
> 
> 	M.
> 
> diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
> index 3db4592cda1c..00641e88aa38 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -29,4 +29,8 @@ void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>  void gic_enable_of_quirks(const struct device_node *np,
>  			  const struct gic_quirk *quirks, void *data);
>  
> +#define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
> +#define RDIST_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
> +#define RDIST_FLAGS_FORCE_NON_SHAREABLE		(1 << 2)
> +
>  #endif /* _IRQ_GIC_COMMON_H */
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index e0c2b10d154d..6edf59af757b 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -44,10 +44,6 @@
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_23144	(1ULL << 2)
>  #define ITS_FLAGS_FORCE_NON_SHAREABLE		(1ULL << 3)
>  
> -#define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
> -#define RDIST_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
> -#define RDIST_FLAGS_FORCE_NON_SHAREABLE		(1 << 2)
> -
>  #define RD_LOCAL_LPI_ENABLED                    BIT(0)
>  #define RD_LOCAL_PENDTABLE_PREALLOCATED         BIT(1)
>  #define RD_LOCAL_MEMRESERVE_DONE                BIT(2)
> @@ -4754,6 +4750,14 @@ static bool __maybe_unused its_enable_rk3588001(void *data)
>  	return true;
>  }
>  
> +static bool its_set_non_coherent(void *data)
> +{
> +	struct its_node *its = data;
> +
> +	its->flags |= ITS_FLAGS_FORCE_NON_SHAREABLE;
> +	return true;
> +}
> +
>  static const struct gic_quirk its_quirks[] = {
>  #ifdef CONFIG_CAVIUM_ERRATUM_22375
>  	{
> @@ -4808,6 +4812,11 @@ static const struct gic_quirk its_quirks[] = {
>  		.init   = its_enable_rk3588001,
>  	},
>  #endif
> +	{
> +		.desc	= "ITS: non-coherent attribute",
> +		.property = "dma-noncoherent",
> +		.init	= its_set_non_coherent,
> +	},
>  	{
>  	}
>  };
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index eedfa8e9f077..7f518c0ae723 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1857,6 +1857,14 @@ static bool gic_enable_quirk_arm64_2941627(void *data)
>  	return true;
>  }
>  
> +static bool rd_set_non_coherent(void *data)
> +{
> +	struct gic_chip_data *d = data;
> +
> +	d->rdists.flags |= RDIST_FLAGS_FORCE_NON_SHAREABLE;
> +	return true;
> +}
> +
>  static const struct gic_quirk gic_quirks[] = {
>  	{
>  		.desc	= "GICv3: Qualcomm MSM8996 broken firmware",
> @@ -1923,6 +1931,11 @@ static const struct gic_quirk gic_quirks[] = {
>  		.mask	= 0xff0f0fff,
>  		.init	= gic_enable_quirk_arm64_2941627,
>  	},
> +	{
> +		.desc	= "GICv3: non-coherent attribute",
> +		.property = "dma-noncoherent",
> +		.init	= rd_set_non_coherent,
> +	},
>  	{
>  	}
>  };
> 
> -- 
> Without deviation from the norm, progress is not possible.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Robin Murphy Oct. 3, 2023, 4:18 p.m. UTC | #2
On 03/10/2023 3:43 pm, Lorenzo Pieralisi wrote:
> On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote:
> 
> [...]
> 
>>>   	 * Make sure *all* the ITS are reset before we probe any, as
>>>   	 * they may be sharing memory. If any of the ITS fails to
>>> @@ -5396,7 +5405,8 @@ static int __init its_of_probe(struct device_node *node)
>>>   			continue;
>>>   		}
>>>   
>>> -		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
>>> +		its_probe_one(&res, &np->fwnode, of_node_to_nid(np),
>>> +			      of_property_read_bool(np, "dma-noncoherent"));
>>>   	}
>>>   	return 0;
>>>   }
>>> @@ -5533,7 +5543,8 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
>>>   	}
>>>   
>>>   	err = its_probe_one(&res, dom_handle,
>>> -			acpi_get_its_numa_node(its_entry->translation_id));
>>> +			acpi_get_its_numa_node(its_entry->translation_id),
>>> +			false);
>>
>> I came up with the following alternative approach, which is as usual
>> completely untested. It is entirely based on the quirk infrastructure,
>> and doesn't touch the ACPI path at all.
> 
> Writing the ACPI bits. We can't use the quirks framework for ACPI (we
> don't have "properties" and I don't think we want to attach any to the
> fwnode_handle) that's why I generalized its_probe_one() above with an
> extra param, that would have simplified ACPI parsing:
> 
> - we alloc struct its_node in its_probe_one() but at that stage
>    ACPI parsing was already done. If we have to parse the MADT(ITS) again
>    just to scan for non-coherent we then have to match the MADT entries
>    to the *current* struct its_node* we are handling (MADT parsing
>    callbacks don't even take a param - we have to resort to global
>    variables - definitely doable but it is a bit ugly).

How about a compromise of passing a whole MADT flags field into 
its_probe_one() (where its_of_probe() can just pass 0), to pass through 
to its_enable_quirks() to then match against an madt_flags field in the 
gic_quirk? gic_acpi_init() could then do something similar for the 
redistributor quirk, although I guess it would then need to distinguish 
GICC and GICR-based quirks cases since the respective flags are in 
different formats.

Thanks,
Robin.
Marc Zyngier Oct. 3, 2023, 4:44 p.m. UTC | #3
On Tue, 03 Oct 2023 15:43:40 +0100,
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote:
> 
> [...]
> 
> > >  	 * Make sure *all* the ITS are reset before we probe any, as
> > >  	 * they may be sharing memory. If any of the ITS fails to
> > > @@ -5396,7 +5405,8 @@ static int __init its_of_probe(struct device_node *node)
> > >  			continue;
> > >  		}
> > >  
> > > -		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
> > > +		its_probe_one(&res, &np->fwnode, of_node_to_nid(np),
> > > +			      of_property_read_bool(np, "dma-noncoherent"));
> > >  	}
> > >  	return 0;
> > >  }
> > > @@ -5533,7 +5543,8 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
> > >  	}
> > >  
> > >  	err = its_probe_one(&res, dom_handle,
> > > -			acpi_get_its_numa_node(its_entry->translation_id));
> > > +			acpi_get_its_numa_node(its_entry->translation_id),
> > > +			false);
> > 
> > I came up with the following alternative approach, which is as usual
> > completely untested. It is entirely based on the quirk infrastructure,
> > and doesn't touch the ACPI path at all.
> 
> Writing the ACPI bits. We can't use the quirks framework for ACPI (we
> don't have "properties" and I don't think we want to attach any to the
> fwnode_handle) that's why I generalized its_probe_one() above with an
> extra param, that would have simplified ACPI parsing:
> 
> - we alloc struct its_node in its_probe_one() but at that stage
>   ACPI parsing was already done. If we have to parse the MADT(ITS) again
>   just to scan for non-coherent we then have to match the MADT entries
>   to the *current* struct its_node* we are handling (MADT parsing
>   callbacks don't even take a param - we have to resort to global
>   variables - definitely doable but it is a bit ugly).

Well, a more acceptable approach would be for its_probe_one() to take
an allocated and possibly pre-populated its_node structure (crucially,
with the quirk flags set), which itself results in a bunch of low
hanging cleanups, see the patch below.

I have boot tested it in a DT guest, so it is obviously perfect.

	M.

From 978f654d4459adf0b8f3f8e896ca37035b3b114c Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Tue, 3 Oct 2023 17:35:27 +0100
Subject: [PATCH] irqchip/gic-v3-its: Split allocation from initialisation of
 its_node

In order to pave the way for more fancy quirk handling without making
more of a mess of this terrible driver, split the allocation of the
ITS descriptor (its_node) from the actual probing.

This will allow firmware-specific hooks to be added between these
two points.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 151 +++++++++++++++++++------------
 1 file changed, 91 insertions(+), 60 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index e0c2b10d154d..bf21383b714e 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4952,7 +4952,7 @@ static void __init __iomem *its_map_one(struct resource *res, int *err)
 	return NULL;
 }
 
-static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
+static int its_init_domain(struct its_node *its)
 {
 	struct irq_domain *inner_domain;
 	struct msi_domain_info *info;
@@ -4966,7 +4966,7 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
 
 	inner_domain = irq_domain_create_hierarchy(its_parent,
 						   its->msi_domain_flags, 0,
-						   handle, &its_domain_ops,
+						   its->fwnode_handle, &its_domain_ops,
 						   info);
 	if (!inner_domain) {
 		kfree(info);
@@ -5017,8 +5017,7 @@ static int its_init_vpe_domain(void)
 	return 0;
 }
 
-static int __init its_compute_its_list_map(struct resource *res,
-					   void __iomem *its_base)
+static int __init its_compute_its_list_map(struct its_node *its)
 {
 	int its_number;
 	u32 ctlr;
@@ -5032,15 +5031,15 @@ static int __init its_compute_its_list_map(struct resource *res,
 	its_number = find_first_zero_bit(&its_list_map, GICv4_ITS_LIST_MAX);
 	if (its_number >= GICv4_ITS_LIST_MAX) {
 		pr_err("ITS@%pa: No ITSList entry available!\n",
-		       &res->start);
+		       &its->phys_base);
 		return -EINVAL;
 	}
 
-	ctlr = readl_relaxed(its_base + GITS_CTLR);
+	ctlr = readl_relaxed(its->base + GITS_CTLR);
 	ctlr &= ~GITS_CTLR_ITS_NUMBER;
 	ctlr |= its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
-	writel_relaxed(ctlr, its_base + GITS_CTLR);
-	ctlr = readl_relaxed(its_base + GITS_CTLR);
+	writel_relaxed(ctlr, its->base + GITS_CTLR);
+	ctlr = readl_relaxed(its->base + GITS_CTLR);
 	if ((ctlr & GITS_CTLR_ITS_NUMBER) != (its_number << GITS_CTLR_ITS_NUMBER_SHIFT)) {
 		its_number = ctlr & GITS_CTLR_ITS_NUMBER;
 		its_number >>= GITS_CTLR_ITS_NUMBER_SHIFT;
@@ -5048,75 +5047,50 @@ static int __init its_compute_its_list_map(struct resource *res,
 
 	if (test_and_set_bit(its_number, &its_list_map)) {
 		pr_err("ITS@%pa: Duplicate ITSList entry %d\n",
-		       &res->start, its_number);
+		       &its->phys_base, its_number);
 		return -EINVAL;
 	}
 
 	return its_number;
 }
 
-static int __init its_probe_one(struct resource *res,
-				struct fwnode_handle *handle, int numa_node)
+static int __init its_probe_one(struct its_node *its)
 {
-	struct its_node *its;
-	void __iomem *its_base;
-	u64 baser, tmp, typer;
+	u64 baser, tmp;
 	struct page *page;
 	u32 ctlr;
 	int err;
 
-	its_base = its_map_one(res, &err);
-	if (!its_base)
-		return err;
-
-	pr_info("ITS %pR\n", res);
-
-	its = kzalloc(sizeof(*its), GFP_KERNEL);
-	if (!its) {
-		err = -ENOMEM;
-		goto out_unmap;
-	}
-
-	raw_spin_lock_init(&its->lock);
-	mutex_init(&its->dev_alloc_lock);
-	INIT_LIST_HEAD(&its->entry);
-	INIT_LIST_HEAD(&its->its_device_list);
-	typer = gic_read_typer(its_base + GITS_TYPER);
-	its->typer = typer;
-	its->base = its_base;
-	its->phys_base = res->start;
 	if (is_v4(its)) {
-		if (!(typer & GITS_TYPER_VMOVP)) {
-			err = its_compute_its_list_map(res, its_base);
+		if (!(its->typer & GITS_TYPER_VMOVP)) {
+			err = its_compute_its_list_map(its);
 			if (err < 0)
-				goto out_free_its;
+				goto out;
 
 			its->list_nr = err;
 
 			pr_info("ITS@%pa: Using ITS number %d\n",
-				&res->start, err);
+				&its->phys_base, err);
 		} else {
-			pr_info("ITS@%pa: Single VMOVP capable\n", &res->start);
+			pr_info("ITS@%pa: Single VMOVP capable\n", &its->phys_base);
 		}
 
 		if (is_v4_1(its)) {
-			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, typer);
+			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, its->typer);
 
-			its->sgir_base = ioremap(res->start + SZ_128K, SZ_64K);
+			its->sgir_base = ioremap(its->phys_base + SZ_128K, SZ_64K);
 			if (!its->sgir_base) {
 				err = -ENOMEM;
-				goto out_free_its;
+				goto out;
 			}
 
-			its->mpidr = readl_relaxed(its_base + GITS_MPIDR);
+			its->mpidr = readl_relaxed(its->base + GITS_MPIDR);
 
 			pr_info("ITS@%pa: Using GICv4.1 mode %08x %08x\n",
-				&res->start, its->mpidr, svpet);
+				&its->phys_base, its->mpidr, svpet);
 		}
 	}
 
-	its->numa_node = numa_node;
-
 	page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO,
 				get_order(ITS_CMD_QUEUE_SZ));
 	if (!page) {
@@ -5125,12 +5099,9 @@ static int __init its_probe_one(struct resource *res,
 	}
 	its->cmd_base = (void *)page_address(page);
 	its->cmd_write = its->cmd_base;
-	its->fwnode_handle = handle;
 	its->get_msi_base = its_irq_get_msi_base;
 	its->msi_domain_flags = IRQ_DOMAIN_FLAG_ISOLATED_MSI;
 
-	its_enable_quirks(its);
-
 	err = its_alloc_tables(its);
 	if (err)
 		goto out_free_cmd;
@@ -5174,7 +5145,7 @@ static int __init its_probe_one(struct resource *res,
 		ctlr |= GITS_CTLR_ImDe;
 	writel_relaxed(ctlr, its->base + GITS_CTLR);
 
-	err = its_init_domain(handle, its);
+	err = its_init_domain(its);
 	if (err)
 		goto out_free_tables;
 
@@ -5191,11 +5162,8 @@ static int __init its_probe_one(struct resource *res,
 out_unmap_sgir:
 	if (its->sgir_base)
 		iounmap(its->sgir_base);
-out_free_its:
-	kfree(its);
-out_unmap:
-	iounmap(its_base);
-	pr_err("ITS@%pa: failed probing (%d)\n", &res->start, err);
+out:
+	pr_err("ITS@%pa: failed probing (%d)\n", &its->phys_base, err);
 	return err;
 }
 
@@ -5356,10 +5324,53 @@ static const struct of_device_id its_device_id[] = {
 	{},
 };
 
+static struct its_node __init *its_node_init(struct resource *res,
+					     struct fwnode_handle *handle, int numa_node)
+{
+	void __iomem *its_base;
+	struct its_node *its;
+	int err;
+
+	its_base = its_map_one(res, &err);
+	if (!its_base)
+		return NULL;
+
+	pr_info("ITS %pR\n", res);
+
+	its = kzalloc(sizeof(*its), GFP_KERNEL);
+	if (!its)
+		goto out_unmap;
+
+	raw_spin_lock_init(&its->lock);
+	mutex_init(&its->dev_alloc_lock);
+	INIT_LIST_HEAD(&its->entry);
+	INIT_LIST_HEAD(&its->its_device_list);
+
+	its->typer = gic_read_typer(its_base + GITS_TYPER);
+	its->base = its_base;
+	its->phys_base = res->start;
+
+	its->numa_node = numa_node;
+	its->fwnode_handle = handle;
+
+	return its;
+
+out_unmap:
+	iounmap(its_base);
+	return NULL;
+}
+
+static void its_node_destroy(struct its_node *its)
+{
+	iounmap(its->base);
+	kfree(its);
+}
+
 static int __init its_of_probe(struct device_node *node)
 {
 	struct device_node *np;
 	struct resource res;
+	int err;
 
 	/*
 	 * Make sure *all* the ITS are reset before we probe any, as
@@ -5369,8 +5380,6 @@ static int __init its_of_probe(struct device_node *node)
 	 */
 	for (np = of_find_matching_node(node, its_device_id); np;
 	     np = of_find_matching_node(np, its_device_id)) {
-		int err;
-
 		if (!of_device_is_available(np) ||
 		    !of_property_read_bool(np, "msi-controller") ||
 		    of_address_to_resource(np, 0, &res))
@@ -5383,6 +5392,8 @@ static int __init its_of_probe(struct device_node *node)
 
 	for (np = of_find_matching_node(node, its_device_id); np;
 	     np = of_find_matching_node(np, its_device_id)) {
+		struct its_node *its;
+
 		if (!of_device_is_available(np))
 			continue;
 		if (!of_property_read_bool(np, "msi-controller")) {
@@ -5396,7 +5407,17 @@ static int __init its_of_probe(struct device_node *node)
 			continue;
 		}
 
-		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
+
+		its = its_node_init(&res, &np->fwnode, of_node_to_nid(np));
+		if (!its)
+			return -ENOMEM;
+
+		its_enable_quirks(its);
+		err = its_probe_one(its);
+		if (err)  {
+			its_node_destroy(its);
+			return err;
+		}
 	}
 	return 0;
 }
@@ -5508,6 +5529,7 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
 {
 	struct acpi_madt_generic_translator *its_entry;
 	struct fwnode_handle *dom_handle;
+	struct its_node *its;
 	struct resource res;
 	int err;
 
@@ -5532,11 +5554,20 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
 		goto dom_err;
 	}
 
-	err = its_probe_one(&res, dom_handle,
-			acpi_get_its_numa_node(its_entry->translation_id));
+	its = its_node_init(&res, dom_handle,
+			    acpi_get_its_numa_node(its_entry->translation_id));
+	if (!its) {
+		err = -ENOMEM;
+		goto node_err;
+	}
+
+	/* Stick ACPI quirk handling here */
+
+	err = its_probe_one(its);
 	if (!err)
 		return 0;
 
+node_err:
 	iort_deregister_domain_token(its_entry->translation_id);
 dom_err:
 	irq_domain_free_fwnode(dom_handle);
Lorenzo Pieralisi Oct. 4, 2023, 7:13 a.m. UTC | #4
On Tue, Oct 03, 2023 at 05:44:27PM +0100, Marc Zyngier wrote:
> On Tue, 03 Oct 2023 15:43:40 +0100,
> Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> > 
> > On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote:
> > 
> > [...]
> > 
> > > >  	 * Make sure *all* the ITS are reset before we probe any, as
> > > >  	 * they may be sharing memory. If any of the ITS fails to
> > > > @@ -5396,7 +5405,8 @@ static int __init its_of_probe(struct device_node *node)
> > > >  			continue;
> > > >  		}
> > > >  
> > > > -		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
> > > > +		its_probe_one(&res, &np->fwnode, of_node_to_nid(np),
> > > > +			      of_property_read_bool(np, "dma-noncoherent"));
> > > >  	}
> > > >  	return 0;
> > > >  }
> > > > @@ -5533,7 +5543,8 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
> > > >  	}
> > > >  
> > > >  	err = its_probe_one(&res, dom_handle,
> > > > -			acpi_get_its_numa_node(its_entry->translation_id));
> > > > +			acpi_get_its_numa_node(its_entry->translation_id),
> > > > +			false);
> > > 
> > > I came up with the following alternative approach, which is as usual
> > > completely untested. It is entirely based on the quirk infrastructure,
> > > and doesn't touch the ACPI path at all.
> > 
> > Writing the ACPI bits. We can't use the quirks framework for ACPI (we
> > don't have "properties" and I don't think we want to attach any to the
> > fwnode_handle) that's why I generalized its_probe_one() above with an
> > extra param, that would have simplified ACPI parsing:
> > 
> > - we alloc struct its_node in its_probe_one() but at that stage
> >   ACPI parsing was already done. If we have to parse the MADT(ITS) again
> >   just to scan for non-coherent we then have to match the MADT entries
> >   to the *current* struct its_node* we are handling (MADT parsing
> >   callbacks don't even take a param - we have to resort to global
> >   variables - definitely doable but it is a bit ugly).
> 
> Well, a more acceptable approach would be for its_probe_one() to take
> an allocated and possibly pre-populated its_node structure (crucially,
> with the quirk flags set), which itself results in a bunch of low
> hanging cleanups, see the patch below.
> 
> I have boot tested it in a DT guest, so it is obviously perfect.

Thanks Marc I will test it and add the ACPI parsing bits.

Lorenzo

> 
> 	M.
> 
> From 978f654d4459adf0b8f3f8e896ca37035b3b114c Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Tue, 3 Oct 2023 17:35:27 +0100
> Subject: [PATCH] irqchip/gic-v3-its: Split allocation from initialisation of
>  its_node
> 
> In order to pave the way for more fancy quirk handling without making
> more of a mess of this terrible driver, split the allocation of the
> ITS descriptor (its_node) from the actual probing.
> 
> This will allow firmware-specific hooks to be added between these
> two points.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 151 +++++++++++++++++++------------
>  1 file changed, 91 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index e0c2b10d154d..bf21383b714e 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -4952,7 +4952,7 @@ static void __init __iomem *its_map_one(struct resource *res, int *err)
>  	return NULL;
>  }
>  
> -static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
> +static int its_init_domain(struct its_node *its)
>  {
>  	struct irq_domain *inner_domain;
>  	struct msi_domain_info *info;
> @@ -4966,7 +4966,7 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
>  
>  	inner_domain = irq_domain_create_hierarchy(its_parent,
>  						   its->msi_domain_flags, 0,
> -						   handle, &its_domain_ops,
> +						   its->fwnode_handle, &its_domain_ops,
>  						   info);
>  	if (!inner_domain) {
>  		kfree(info);
> @@ -5017,8 +5017,7 @@ static int its_init_vpe_domain(void)
>  	return 0;
>  }
>  
> -static int __init its_compute_its_list_map(struct resource *res,
> -					   void __iomem *its_base)
> +static int __init its_compute_its_list_map(struct its_node *its)
>  {
>  	int its_number;
>  	u32 ctlr;
> @@ -5032,15 +5031,15 @@ static int __init its_compute_its_list_map(struct resource *res,
>  	its_number = find_first_zero_bit(&its_list_map, GICv4_ITS_LIST_MAX);
>  	if (its_number >= GICv4_ITS_LIST_MAX) {
>  		pr_err("ITS@%pa: No ITSList entry available!\n",
> -		       &res->start);
> +		       &its->phys_base);
>  		return -EINVAL;
>  	}
>  
> -	ctlr = readl_relaxed(its_base + GITS_CTLR);
> +	ctlr = readl_relaxed(its->base + GITS_CTLR);
>  	ctlr &= ~GITS_CTLR_ITS_NUMBER;
>  	ctlr |= its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
> -	writel_relaxed(ctlr, its_base + GITS_CTLR);
> -	ctlr = readl_relaxed(its_base + GITS_CTLR);
> +	writel_relaxed(ctlr, its->base + GITS_CTLR);
> +	ctlr = readl_relaxed(its->base + GITS_CTLR);
>  	if ((ctlr & GITS_CTLR_ITS_NUMBER) != (its_number << GITS_CTLR_ITS_NUMBER_SHIFT)) {
>  		its_number = ctlr & GITS_CTLR_ITS_NUMBER;
>  		its_number >>= GITS_CTLR_ITS_NUMBER_SHIFT;
> @@ -5048,75 +5047,50 @@ static int __init its_compute_its_list_map(struct resource *res,
>  
>  	if (test_and_set_bit(its_number, &its_list_map)) {
>  		pr_err("ITS@%pa: Duplicate ITSList entry %d\n",
> -		       &res->start, its_number);
> +		       &its->phys_base, its_number);
>  		return -EINVAL;
>  	}
>  
>  	return its_number;
>  }
>  
> -static int __init its_probe_one(struct resource *res,
> -				struct fwnode_handle *handle, int numa_node)
> +static int __init its_probe_one(struct its_node *its)
>  {
> -	struct its_node *its;
> -	void __iomem *its_base;
> -	u64 baser, tmp, typer;
> +	u64 baser, tmp;
>  	struct page *page;
>  	u32 ctlr;
>  	int err;
>  
> -	its_base = its_map_one(res, &err);
> -	if (!its_base)
> -		return err;
> -
> -	pr_info("ITS %pR\n", res);
> -
> -	its = kzalloc(sizeof(*its), GFP_KERNEL);
> -	if (!its) {
> -		err = -ENOMEM;
> -		goto out_unmap;
> -	}
> -
> -	raw_spin_lock_init(&its->lock);
> -	mutex_init(&its->dev_alloc_lock);
> -	INIT_LIST_HEAD(&its->entry);
> -	INIT_LIST_HEAD(&its->its_device_list);
> -	typer = gic_read_typer(its_base + GITS_TYPER);
> -	its->typer = typer;
> -	its->base = its_base;
> -	its->phys_base = res->start;
>  	if (is_v4(its)) {
> -		if (!(typer & GITS_TYPER_VMOVP)) {
> -			err = its_compute_its_list_map(res, its_base);
> +		if (!(its->typer & GITS_TYPER_VMOVP)) {
> +			err = its_compute_its_list_map(its);
>  			if (err < 0)
> -				goto out_free_its;
> +				goto out;
>  
>  			its->list_nr = err;
>  
>  			pr_info("ITS@%pa: Using ITS number %d\n",
> -				&res->start, err);
> +				&its->phys_base, err);
>  		} else {
> -			pr_info("ITS@%pa: Single VMOVP capable\n", &res->start);
> +			pr_info("ITS@%pa: Single VMOVP capable\n", &its->phys_base);
>  		}
>  
>  		if (is_v4_1(its)) {
> -			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, typer);
> +			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, its->typer);
>  
> -			its->sgir_base = ioremap(res->start + SZ_128K, SZ_64K);
> +			its->sgir_base = ioremap(its->phys_base + SZ_128K, SZ_64K);
>  			if (!its->sgir_base) {
>  				err = -ENOMEM;
> -				goto out_free_its;
> +				goto out;
>  			}
>  
> -			its->mpidr = readl_relaxed(its_base + GITS_MPIDR);
> +			its->mpidr = readl_relaxed(its->base + GITS_MPIDR);
>  
>  			pr_info("ITS@%pa: Using GICv4.1 mode %08x %08x\n",
> -				&res->start, its->mpidr, svpet);
> +				&its->phys_base, its->mpidr, svpet);
>  		}
>  	}
>  
> -	its->numa_node = numa_node;
> -
>  	page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO,
>  				get_order(ITS_CMD_QUEUE_SZ));
>  	if (!page) {
> @@ -5125,12 +5099,9 @@ static int __init its_probe_one(struct resource *res,
>  	}
>  	its->cmd_base = (void *)page_address(page);
>  	its->cmd_write = its->cmd_base;
> -	its->fwnode_handle = handle;
>  	its->get_msi_base = its_irq_get_msi_base;
>  	its->msi_domain_flags = IRQ_DOMAIN_FLAG_ISOLATED_MSI;
>  
> -	its_enable_quirks(its);
> -
>  	err = its_alloc_tables(its);
>  	if (err)
>  		goto out_free_cmd;
> @@ -5174,7 +5145,7 @@ static int __init its_probe_one(struct resource *res,
>  		ctlr |= GITS_CTLR_ImDe;
>  	writel_relaxed(ctlr, its->base + GITS_CTLR);
>  
> -	err = its_init_domain(handle, its);
> +	err = its_init_domain(its);
>  	if (err)
>  		goto out_free_tables;
>  
> @@ -5191,11 +5162,8 @@ static int __init its_probe_one(struct resource *res,
>  out_unmap_sgir:
>  	if (its->sgir_base)
>  		iounmap(its->sgir_base);
> -out_free_its:
> -	kfree(its);
> -out_unmap:
> -	iounmap(its_base);
> -	pr_err("ITS@%pa: failed probing (%d)\n", &res->start, err);
> +out:
> +	pr_err("ITS@%pa: failed probing (%d)\n", &its->phys_base, err);
>  	return err;
>  }
>  
> @@ -5356,10 +5324,53 @@ static const struct of_device_id its_device_id[] = {
>  	{},
>  };
>  
> +static struct its_node __init *its_node_init(struct resource *res,
> +					     struct fwnode_handle *handle, int numa_node)
> +{
> +	void __iomem *its_base;
> +	struct its_node *its;
> +	int err;
> +
> +	its_base = its_map_one(res, &err);
> +	if (!its_base)
> +		return NULL;
> +
> +	pr_info("ITS %pR\n", res);
> +
> +	its = kzalloc(sizeof(*its), GFP_KERNEL);
> +	if (!its)
> +		goto out_unmap;
> +
> +	raw_spin_lock_init(&its->lock);
> +	mutex_init(&its->dev_alloc_lock);
> +	INIT_LIST_HEAD(&its->entry);
> +	INIT_LIST_HEAD(&its->its_device_list);
> +
> +	its->typer = gic_read_typer(its_base + GITS_TYPER);
> +	its->base = its_base;
> +	its->phys_base = res->start;
> +
> +	its->numa_node = numa_node;
> +	its->fwnode_handle = handle;
> +
> +	return its;
> +
> +out_unmap:
> +	iounmap(its_base);
> +	return NULL;
> +}
> +
> +static void its_node_destroy(struct its_node *its)
> +{
> +	iounmap(its->base);
> +	kfree(its);
> +}
> +
>  static int __init its_of_probe(struct device_node *node)
>  {
>  	struct device_node *np;
>  	struct resource res;
> +	int err;
>  
>  	/*
>  	 * Make sure *all* the ITS are reset before we probe any, as
> @@ -5369,8 +5380,6 @@ static int __init its_of_probe(struct device_node *node)
>  	 */
>  	for (np = of_find_matching_node(node, its_device_id); np;
>  	     np = of_find_matching_node(np, its_device_id)) {
> -		int err;
> -
>  		if (!of_device_is_available(np) ||
>  		    !of_property_read_bool(np, "msi-controller") ||
>  		    of_address_to_resource(np, 0, &res))
> @@ -5383,6 +5392,8 @@ static int __init its_of_probe(struct device_node *node)
>  
>  	for (np = of_find_matching_node(node, its_device_id); np;
>  	     np = of_find_matching_node(np, its_device_id)) {
> +		struct its_node *its;
> +
>  		if (!of_device_is_available(np))
>  			continue;
>  		if (!of_property_read_bool(np, "msi-controller")) {
> @@ -5396,7 +5407,17 @@ static int __init its_of_probe(struct device_node *node)
>  			continue;
>  		}
>  
> -		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
> +
> +		its = its_node_init(&res, &np->fwnode, of_node_to_nid(np));
> +		if (!its)
> +			return -ENOMEM;
> +
> +		its_enable_quirks(its);
> +		err = its_probe_one(its);
> +		if (err)  {
> +			its_node_destroy(its);
> +			return err;
> +		}
>  	}
>  	return 0;
>  }
> @@ -5508,6 +5529,7 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
>  {
>  	struct acpi_madt_generic_translator *its_entry;
>  	struct fwnode_handle *dom_handle;
> +	struct its_node *its;
>  	struct resource res;
>  	int err;
>  
> @@ -5532,11 +5554,20 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
>  		goto dom_err;
>  	}
>  
> -	err = its_probe_one(&res, dom_handle,
> -			acpi_get_its_numa_node(its_entry->translation_id));
> +	its = its_node_init(&res, dom_handle,
> +			    acpi_get_its_numa_node(its_entry->translation_id));
> +	if (!its) {
> +		err = -ENOMEM;
> +		goto node_err;
> +	}
> +
> +	/* Stick ACPI quirk handling here */
> +
> +	err = its_probe_one(its);
>  	if (!err)
>  		return 0;
>  
> +node_err:
>  	iort_deregister_domain_token(its_entry->translation_id);
>  dom_err:
>  	irq_domain_free_fwnode(dom_handle);
> -- 
> 2.34.1
> 
> -- 
> Without deviation from the norm, progress is not possible.
Lorenzo Pieralisi Oct. 5, 2023, 1:59 p.m. UTC | #5
On Tue, Oct 03, 2023 at 05:44:27PM +0100, Marc Zyngier wrote:
> On Tue, 03 Oct 2023 15:43:40 +0100,
> Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> > 
> > On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote:
> > 
> > [...]
> > 
> > > >  	 * Make sure *all* the ITS are reset before we probe any, as
> > > >  	 * they may be sharing memory. If any of the ITS fails to
> > > > @@ -5396,7 +5405,8 @@ static int __init its_of_probe(struct device_node *node)
> > > >  			continue;
> > > >  		}
> > > >  
> > > > -		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
> > > > +		its_probe_one(&res, &np->fwnode, of_node_to_nid(np),
> > > > +			      of_property_read_bool(np, "dma-noncoherent"));
> > > >  	}
> > > >  	return 0;
> > > >  }
> > > > @@ -5533,7 +5543,8 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
> > > >  	}
> > > >  
> > > >  	err = its_probe_one(&res, dom_handle,
> > > > -			acpi_get_its_numa_node(its_entry->translation_id));
> > > > +			acpi_get_its_numa_node(its_entry->translation_id),
> > > > +			false);
> > > 
> > > I came up with the following alternative approach, which is as usual
> > > completely untested. It is entirely based on the quirk infrastructure,
> > > and doesn't touch the ACPI path at all.
> > 
> > Writing the ACPI bits. We can't use the quirks framework for ACPI (we
> > don't have "properties" and I don't think we want to attach any to the
> > fwnode_handle) that's why I generalized its_probe_one() above with an
> > extra param, that would have simplified ACPI parsing:
> > 
> > - we alloc struct its_node in its_probe_one() but at that stage
> >   ACPI parsing was already done. If we have to parse the MADT(ITS) again
> >   just to scan for non-coherent we then have to match the MADT entries
> >   to the *current* struct its_node* we are handling (MADT parsing
> >   callbacks don't even take a param - we have to resort to global
> >   variables - definitely doable but it is a bit ugly).
> 
> Well, a more acceptable approach would be for its_probe_one() to take
> an allocated and possibly pre-populated its_node structure (crucially,
> with the quirk flags set), which itself results in a bunch of low
> hanging cleanups, see the patch below.
> 
> I have boot tested it in a DT guest, so it is obviously perfect.

If you don't mind I will post it together with the resulting series.

I have just removed:

/* Stick ACPI quirk handling here */

point taken :)

Thanks,
Lorenzo

> 	M.
> 
> From 978f654d4459adf0b8f3f8e896ca37035b3b114c Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Tue, 3 Oct 2023 17:35:27 +0100
> Subject: [PATCH] irqchip/gic-v3-its: Split allocation from initialisation of
>  its_node
> 
> In order to pave the way for more fancy quirk handling without making
> more of a mess of this terrible driver, split the allocation of the
> ITS descriptor (its_node) from the actual probing.
> 
> This will allow firmware-specific hooks to be added between these
> two points.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 151 +++++++++++++++++++------------
>  1 file changed, 91 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index e0c2b10d154d..bf21383b714e 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -4952,7 +4952,7 @@ static void __init __iomem *its_map_one(struct resource *res, int *err)
>  	return NULL;
>  }
>  
> -static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
> +static int its_init_domain(struct its_node *its)
>  {
>  	struct irq_domain *inner_domain;
>  	struct msi_domain_info *info;
> @@ -4966,7 +4966,7 @@ static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
>  
>  	inner_domain = irq_domain_create_hierarchy(its_parent,
>  						   its->msi_domain_flags, 0,
> -						   handle, &its_domain_ops,
> +						   its->fwnode_handle, &its_domain_ops,
>  						   info);
>  	if (!inner_domain) {
>  		kfree(info);
> @@ -5017,8 +5017,7 @@ static int its_init_vpe_domain(void)
>  	return 0;
>  }
>  
> -static int __init its_compute_its_list_map(struct resource *res,
> -					   void __iomem *its_base)
> +static int __init its_compute_its_list_map(struct its_node *its)
>  {
>  	int its_number;
>  	u32 ctlr;
> @@ -5032,15 +5031,15 @@ static int __init its_compute_its_list_map(struct resource *res,
>  	its_number = find_first_zero_bit(&its_list_map, GICv4_ITS_LIST_MAX);
>  	if (its_number >= GICv4_ITS_LIST_MAX) {
>  		pr_err("ITS@%pa: No ITSList entry available!\n",
> -		       &res->start);
> +		       &its->phys_base);
>  		return -EINVAL;
>  	}
>  
> -	ctlr = readl_relaxed(its_base + GITS_CTLR);
> +	ctlr = readl_relaxed(its->base + GITS_CTLR);
>  	ctlr &= ~GITS_CTLR_ITS_NUMBER;
>  	ctlr |= its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
> -	writel_relaxed(ctlr, its_base + GITS_CTLR);
> -	ctlr = readl_relaxed(its_base + GITS_CTLR);
> +	writel_relaxed(ctlr, its->base + GITS_CTLR);
> +	ctlr = readl_relaxed(its->base + GITS_CTLR);
>  	if ((ctlr & GITS_CTLR_ITS_NUMBER) != (its_number << GITS_CTLR_ITS_NUMBER_SHIFT)) {
>  		its_number = ctlr & GITS_CTLR_ITS_NUMBER;
>  		its_number >>= GITS_CTLR_ITS_NUMBER_SHIFT;
> @@ -5048,75 +5047,50 @@ static int __init its_compute_its_list_map(struct resource *res,
>  
>  	if (test_and_set_bit(its_number, &its_list_map)) {
>  		pr_err("ITS@%pa: Duplicate ITSList entry %d\n",
> -		       &res->start, its_number);
> +		       &its->phys_base, its_number);
>  		return -EINVAL;
>  	}
>  
>  	return its_number;
>  }
>  
> -static int __init its_probe_one(struct resource *res,
> -				struct fwnode_handle *handle, int numa_node)
> +static int __init its_probe_one(struct its_node *its)
>  {
> -	struct its_node *its;
> -	void __iomem *its_base;
> -	u64 baser, tmp, typer;
> +	u64 baser, tmp;
>  	struct page *page;
>  	u32 ctlr;
>  	int err;
>  
> -	its_base = its_map_one(res, &err);
> -	if (!its_base)
> -		return err;
> -
> -	pr_info("ITS %pR\n", res);
> -
> -	its = kzalloc(sizeof(*its), GFP_KERNEL);
> -	if (!its) {
> -		err = -ENOMEM;
> -		goto out_unmap;
> -	}
> -
> -	raw_spin_lock_init(&its->lock);
> -	mutex_init(&its->dev_alloc_lock);
> -	INIT_LIST_HEAD(&its->entry);
> -	INIT_LIST_HEAD(&its->its_device_list);
> -	typer = gic_read_typer(its_base + GITS_TYPER);
> -	its->typer = typer;
> -	its->base = its_base;
> -	its->phys_base = res->start;
>  	if (is_v4(its)) {
> -		if (!(typer & GITS_TYPER_VMOVP)) {
> -			err = its_compute_its_list_map(res, its_base);
> +		if (!(its->typer & GITS_TYPER_VMOVP)) {
> +			err = its_compute_its_list_map(its);
>  			if (err < 0)
> -				goto out_free_its;
> +				goto out;
>  
>  			its->list_nr = err;
>  
>  			pr_info("ITS@%pa: Using ITS number %d\n",
> -				&res->start, err);
> +				&its->phys_base, err);
>  		} else {
> -			pr_info("ITS@%pa: Single VMOVP capable\n", &res->start);
> +			pr_info("ITS@%pa: Single VMOVP capable\n", &its->phys_base);
>  		}
>  
>  		if (is_v4_1(its)) {
> -			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, typer);
> +			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, its->typer);
>  
> -			its->sgir_base = ioremap(res->start + SZ_128K, SZ_64K);
> +			its->sgir_base = ioremap(its->phys_base + SZ_128K, SZ_64K);
>  			if (!its->sgir_base) {
>  				err = -ENOMEM;
> -				goto out_free_its;
> +				goto out;
>  			}
>  
> -			its->mpidr = readl_relaxed(its_base + GITS_MPIDR);
> +			its->mpidr = readl_relaxed(its->base + GITS_MPIDR);
>  
>  			pr_info("ITS@%pa: Using GICv4.1 mode %08x %08x\n",
> -				&res->start, its->mpidr, svpet);
> +				&its->phys_base, its->mpidr, svpet);
>  		}
>  	}
>  
> -	its->numa_node = numa_node;
> -
>  	page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO,
>  				get_order(ITS_CMD_QUEUE_SZ));
>  	if (!page) {
> @@ -5125,12 +5099,9 @@ static int __init its_probe_one(struct resource *res,
>  	}
>  	its->cmd_base = (void *)page_address(page);
>  	its->cmd_write = its->cmd_base;
> -	its->fwnode_handle = handle;
>  	its->get_msi_base = its_irq_get_msi_base;
>  	its->msi_domain_flags = IRQ_DOMAIN_FLAG_ISOLATED_MSI;
>  
> -	its_enable_quirks(its);
> -
>  	err = its_alloc_tables(its);
>  	if (err)
>  		goto out_free_cmd;
> @@ -5174,7 +5145,7 @@ static int __init its_probe_one(struct resource *res,
>  		ctlr |= GITS_CTLR_ImDe;
>  	writel_relaxed(ctlr, its->base + GITS_CTLR);
>  
> -	err = its_init_domain(handle, its);
> +	err = its_init_domain(its);
>  	if (err)
>  		goto out_free_tables;
>  
> @@ -5191,11 +5162,8 @@ static int __init its_probe_one(struct resource *res,
>  out_unmap_sgir:
>  	if (its->sgir_base)
>  		iounmap(its->sgir_base);
> -out_free_its:
> -	kfree(its);
> -out_unmap:
> -	iounmap(its_base);
> -	pr_err("ITS@%pa: failed probing (%d)\n", &res->start, err);
> +out:
> +	pr_err("ITS@%pa: failed probing (%d)\n", &its->phys_base, err);
>  	return err;
>  }
>  
> @@ -5356,10 +5324,53 @@ static const struct of_device_id its_device_id[] = {
>  	{},
>  };
>  
> +static struct its_node __init *its_node_init(struct resource *res,
> +					     struct fwnode_handle *handle, int numa_node)
> +{
> +	void __iomem *its_base;
> +	struct its_node *its;
> +	int err;
> +
> +	its_base = its_map_one(res, &err);
> +	if (!its_base)
> +		return NULL;
> +
> +	pr_info("ITS %pR\n", res);
> +
> +	its = kzalloc(sizeof(*its), GFP_KERNEL);
> +	if (!its)
> +		goto out_unmap;
> +
> +	raw_spin_lock_init(&its->lock);
> +	mutex_init(&its->dev_alloc_lock);
> +	INIT_LIST_HEAD(&its->entry);
> +	INIT_LIST_HEAD(&its->its_device_list);
> +
> +	its->typer = gic_read_typer(its_base + GITS_TYPER);
> +	its->base = its_base;
> +	its->phys_base = res->start;
> +
> +	its->numa_node = numa_node;
> +	its->fwnode_handle = handle;
> +
> +	return its;
> +
> +out_unmap:
> +	iounmap(its_base);
> +	return NULL;
> +}
> +
> +static void its_node_destroy(struct its_node *its)
> +{
> +	iounmap(its->base);
> +	kfree(its);
> +}
> +
>  static int __init its_of_probe(struct device_node *node)
>  {
>  	struct device_node *np;
>  	struct resource res;
> +	int err;
>  
>  	/*
>  	 * Make sure *all* the ITS are reset before we probe any, as
> @@ -5369,8 +5380,6 @@ static int __init its_of_probe(struct device_node *node)
>  	 */
>  	for (np = of_find_matching_node(node, its_device_id); np;
>  	     np = of_find_matching_node(np, its_device_id)) {
> -		int err;
> -
>  		if (!of_device_is_available(np) ||
>  		    !of_property_read_bool(np, "msi-controller") ||
>  		    of_address_to_resource(np, 0, &res))
> @@ -5383,6 +5392,8 @@ static int __init its_of_probe(struct device_node *node)
>  
>  	for (np = of_find_matching_node(node, its_device_id); np;
>  	     np = of_find_matching_node(np, its_device_id)) {
> +		struct its_node *its;
> +
>  		if (!of_device_is_available(np))
>  			continue;
>  		if (!of_property_read_bool(np, "msi-controller")) {
> @@ -5396,7 +5407,17 @@ static int __init its_of_probe(struct device_node *node)
>  			continue;
>  		}
>  
> -		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
> +
> +		its = its_node_init(&res, &np->fwnode, of_node_to_nid(np));
> +		if (!its)
> +			return -ENOMEM;
> +
> +		its_enable_quirks(its);
> +		err = its_probe_one(its);
> +		if (err)  {
> +			its_node_destroy(its);
> +			return err;
> +		}
>  	}
>  	return 0;
>  }
> @@ -5508,6 +5529,7 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
>  {
>  	struct acpi_madt_generic_translator *its_entry;
>  	struct fwnode_handle *dom_handle;
> +	struct its_node *its;
>  	struct resource res;
>  	int err;
>  
> @@ -5532,11 +5554,20 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
>  		goto dom_err;
>  	}
>  
> -	err = its_probe_one(&res, dom_handle,
> -			acpi_get_its_numa_node(its_entry->translation_id));
> +	its = its_node_init(&res, dom_handle,
> +			    acpi_get_its_numa_node(its_entry->translation_id));
> +	if (!its) {
> +		err = -ENOMEM;
> +		goto node_err;
> +	}
> +
> +	/* Stick ACPI quirk handling here */
> +
> +	err = its_probe_one(its);
>  	if (!err)
>  		return 0;
>  
> +node_err:
>  	iort_deregister_domain_token(its_entry->translation_id);
>  dom_err:
>  	irq_domain_free_fwnode(dom_handle);
> -- 
> 2.34.1
> 
> -- 
> Without deviation from the norm, progress is not possible.
diff mbox series

Patch

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index e0c2b10d154d..758ea3092305 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -5056,7 +5056,8 @@  static int __init its_compute_its_list_map(struct resource *res,
 }
 
 static int __init its_probe_one(struct resource *res,
-				struct fwnode_handle *handle, int numa_node)
+				struct fwnode_handle *handle, int numa_node,
+				bool non_coherent)
 {
 	struct its_node *its;
 	void __iomem *its_base;
@@ -5148,7 +5149,7 @@  static int __init its_probe_one(struct resource *res,
 	gits_write_cbaser(baser, its->base + GITS_CBASER);
 	tmp = gits_read_cbaser(its->base + GITS_CBASER);
 
-	if (its->flags & ITS_FLAGS_FORCE_NON_SHAREABLE)
+	if (its->flags & ITS_FLAGS_FORCE_NON_SHAREABLE || non_coherent)
 		tmp &= ~GITS_CBASER_SHAREABILITY_MASK;
 
 	if ((tmp ^ baser) & GITS_CBASER_SHAREABILITY_MASK) {
@@ -5356,11 +5357,19 @@  static const struct of_device_id its_device_id[] = {
 	{},
 };
 
+static void of_check_rdists_coherent(struct device_node *node)
+{
+	if (of_property_read_bool(node, "dma-noncoherent"))
+		gic_rdists->flags |= RDIST_FLAGS_FORCE_NON_SHAREABLE;
+}
+
 static int __init its_of_probe(struct device_node *node)
 {
 	struct device_node *np;
 	struct resource res;
 
+	of_check_rdists_coherent(node);
+
 	/*
 	 * Make sure *all* the ITS are reset before we probe any, as
 	 * they may be sharing memory. If any of the ITS fails to
@@ -5396,7 +5405,8 @@  static int __init its_of_probe(struct device_node *node)
 			continue;
 		}
 
-		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
+		its_probe_one(&res, &np->fwnode, of_node_to_nid(np),
+			      of_property_read_bool(np, "dma-noncoherent"));
 	}
 	return 0;
 }
@@ -5533,7 +5543,8 @@  static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
 	}
 
 	err = its_probe_one(&res, dom_handle,
-			acpi_get_its_numa_node(its_entry->translation_id));
+			acpi_get_its_numa_node(its_entry->translation_id),
+			false);
 	if (!err)
 		return 0;