| Message ID | 20230905104721.52199-1-lpieralisi@kernel.org |
|---|---|
| Headers | show |
| Series | irqchip/gic-v3: Enable non-coherent GIC designs probing | expand |
On Tue, 05 Sep 2023 13:30:47 +0100, Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote: > > 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. > > The patch below makes sense and it is much cleaner - provided we > consider this a quirk, which I am not sure it is, that's the only > question I have. Come on, the whole architecture is a bag of sick hacks. And given that it makes the change slightly less ugly, I'd rather have that. M.
On Tue, Sep 05, 2023 at 04:24:51PM +0200, Lorenzo Pieralisi wrote: > On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote: > > > > 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. > > > > 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, > > + }, > > For the records, that requires adding a gic_enable_of_quirks() call for the > ITS DT node that at the moment is not needed, something like this: > > -- >8 -- > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > index 25a12b46ce35..3ae3cb9aadd9 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -4826,6 +4826,10 @@ static void its_enable_quirks(struct its_node *its) > u32 iidr = readl_relaxed(its->base + GITS_IIDR); > > gic_enable_quirks(iidr, its_quirks, its); > + > + if (is_of_node(its->fwnode_handle)) > + gic_enable_of_quirks(to_of_node(its->fwnode_handle), > + its_quirks, its); > } > > static int its_save_disable(void) > > > { > > } > > }; > > 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, > > + }, > > { > > } > > }; I have tested this patch above on my deivce and it works well. Looking forward the official release.
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
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.
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.
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.
The GICv3 architecture specifications provide a means for the system programmer to set the shareability and cacheability attributes the GIC components (redistributors and ITSes) use to drive memory transactions. Albeit the architecture give control over shareability/cacheability memory transactions attributes (and barriers), it is allowed to connect the GIC interconnect ports to non-coherent memory ports on the interconnect, basically tying off shareability/cacheability "wires" and de-facto making the redistributors and ITSes non-coherent memory observers. This series aims at starting a discussion over a possible solution to this problem, by adding to the GIC device tree bindings the standard dma-noncoherent property. The GIC driver uses the property to force the redistributors and ITSes shareability attributes to non-shareable, which consequently forces the driver to use CMOs on GIC memory tables. On ARM DT DMA is default non-coherent, so the GIC driver can't rely on the generic DT dma-coherent/non-coherent property management layer (of_dma_is_coherent()) which would default all GIC designs in the field as non-coherent; it has to rely on ad-hoc dma-noncoherent property handling. When a consistent approach is agreed upon for DT an equivalent binding will be put forward for ACPI based systems. Lorenzo Pieralisi (2): dt-bindings: interrupt-controller: arm,gic-v3: Add dma-noncoherent property irqchip/gic-v3: Enable non-coherent redistributors/ITSes probing .../interrupt-controller/arm,gic-v3.yaml | 8 ++++++++ drivers/irqchip/irq-gic-v3-its.c | 19 +++++++++++++++---- 2 files changed, 23 insertions(+), 4 deletions(-)