| Message ID | 20230905104721.52199-3-lpieralisi@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irqchip/gic-v3: Enable non-coherent GIC designs probing | expand |
On Tue, 05 Sep 2023 11:47:21 +0100, Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > 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(-) > > 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; Please use the non_coherent attribute to set the flag, instead of using it as some sideband signalling. Not having this information stored in the its_node structure makes it harder to debug. We have an over-engineered quirk framework, and it should be put to a good use. > > 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); It really feels that the flag should instead be communicated by the base GIC driver, as it readily communicates the whole rdists structure already. > + > /* > * 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. 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, + }, { } };
On 05/09/2023 12:34 pm, Marc Zyngier wrote: > On Tue, 05 Sep 2023 11:47:21 +0100, > Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: >> >> 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(-) >> >> 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; > > Please use the non_coherent attribute to set the flag, instead of > using it as some sideband signalling. Not having this information > stored in the its_node structure makes it harder to debug. > > We have an over-engineered quirk framework, and it should be put to a > good use. > >> >> 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); > > It really feels that the flag should instead be communicated by the > base GIC driver, as it readily communicates the whole rdists structure > already. > >> + >> /* >> * 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. FWIW I think I agree that looks a bit easier to follow overall, and especially since we already have the SoC-based Rockchip variant of this using a quirk, having two different ways of carrying the same underlying information through certain paths does seem a bit icky. Cheers, Robin. > > 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, > + }, > { > } > }; >
On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote: > On Tue, 05 Sep 2023 11:47:21 +0100, > Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > > > 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(-) > > > > 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; > > Please use the non_coherent attribute to set the flag, instead of > using it as some sideband signalling. Not having this information > stored in the its_node structure makes it harder to debug. > > We have an over-engineered quirk framework, and it should be put to a > good use. That makes sense, I will do - I was wondering though whether this is a quirk or a binding that describes the architecture because the architecture can't provide that information. That's the reason why I have not implemented it as a quirk but obviously that's exactly the feedback I need. > > > > > 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); > > It really feels that the flag should instead be communicated by the > base GIC driver, as it readily communicates the whole rdists structure > already. Yes I had the same feeling while writing the code but the patch below (if we treat this as quirk) is already much cleaner. > > + > > /* > > * 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. 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. Thanks, 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 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 12:34:58PM +0100, Marc Zyngier wrote: > On Tue, 05 Sep 2023 11:47:21 +0100, > Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > > > 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(-) > > > > 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; > > Please use the non_coherent attribute to set the flag, instead of > using it as some sideband signalling. Not having this information > stored in the its_node structure makes it harder to debug. > > We have an over-engineered quirk framework, and it should be put to a > good use. > > > > > 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); > > It really feels that the flag should instead be communicated by the > base GIC driver, as it readily communicates the whole rdists structure > already. > > > + > > /* > > * 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. > > 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, > + }, > { > } > }; > > -- > 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 Tue, 05 Sep 2023 15:24:51 +0100, Lorenzo Pieralisi <lpieralisi@kernel.org> wrote: > > On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote: > > @@ -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); > } > Ah, well spotted. Yes, please fold this in. Thanks, 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 Wed, 06 Sep 2023 12:01:06 +0100, Fang Xiang <fangxiang3@xiaomi.com> wrote: > > I have tested this patch above on my deivce and it works > well. Looking forward the official release. If that's a 'Tested-by:' tag, please respond to v2 with such a tag. Thanks, M.
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, 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);
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.
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;
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(-)