| Message ID | 20230310080518.78054-2-lucas.tanure@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add PCIe2 support for Rockchip Boards | expand |
On 2023-03-10 08:05, Lucas Tanure wrote: > The GIC600 integration in RK356x, used in rk3588, doesn't support > any of the shareability or cacheability attributes, and requires > both values to be set to 0b00 for all the ITS and Redistributor > tables. > > This is loosely based on prior work from XiaoDong Huang and > Peter Geis fixing this issue specifically for Rockchip 356x. No. If we are going to do *anything* about this thing, it is by describing the actual topology. And it has to work for both DT and ACPI. Alternatively, this is an erratum. M.
On 10-03-2023 08:56, Marc Zyngier wrote: > On 2023-03-10 08:05, Lucas Tanure wrote: >> The GIC600 integration in RK356x, used in rk3588, doesn't support >> any of the shareability or cacheability attributes, and requires >> both values to be set to 0b00 for all the ITS and Redistributor >> tables. >> >> This is loosely based on prior work from XiaoDong Huang and >> Peter Geis fixing this issue specifically for Rockchip 356x. > > No. > > If we are going to do *anything* about this thing, it is by > describing the actual topology. What do you mean by describe the topology? What kind of information are you expecting? And it has to work for both DT > and ACPI. > > Alternatively, this is an erratum. > > M.
On Fri, 10 Mar 2023 09:53:16 +0000, Lucas Tanure <lucas.tanure@collabora.com> wrote: > > On 10-03-2023 08:56, Marc Zyngier wrote: > > On 2023-03-10 08:05, Lucas Tanure wrote: > >> The GIC600 integration in RK356x, used in rk3588, doesn't support > >> any of the shareability or cacheability attributes, and requires > >> both values to be set to 0b00 for all the ITS and Redistributor > >> tables. > >> > >> This is loosely based on prior work from XiaoDong Huang and > >> Peter Geis fixing this issue specifically for Rockchip 356x. > > > > No. > > > > If we are going to do *anything* about this thing, it is by > > describing the actual topology. > What do you mean by describe the topology? Exactly what it means. Describe which shareability domains the GIC is in w.r.t the whole SoC. Do it consistently over the whole SoC. M.
On Fri, Mar 10, 2023 at 3:05 AM Lucas Tanure <lucas.tanure@collabora.com> wrote: > > The GIC600 integration in RK356x, used in rk3588, doesn't support > any of the shareability or cacheability attributes, and requires > both values to be set to 0b00 for all the ITS and Redistributor > tables. > > This is loosely based on prior work from XiaoDong Huang and > Peter Geis fixing this issue specifically for Rockchip 356x. Good Morning, Since the gic is using dma, would it be reasonable to have all memory allocations be requested with the GFP_DMA flag? Otherwise this doesn't fully solve the problem for rk356x, where only the lower 4GB range is DMA capable, but this tends to get allocated in the upper 4GB on 8GB boards. Very Respectfully, Peter Geis > > Suggested-by: Robin Murphy <robin.murphy@arm.com> > Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com> > --- > drivers/irqchip/irq-gic-v3-its.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > index 973ede0197e3..1c334dfeb647 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -42,6 +42,7 @@ > #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING (1ULL << 0) > #define ITS_FLAGS_WORKAROUND_CAVIUM_22375 (1ULL << 1) > #define ITS_FLAGS_WORKAROUND_CAVIUM_23144 (1ULL << 2) > +#define ITS_FLAGS_DMA_NON_COHERENT (1ULL << 3) > > #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0) > #define RDIST_FLAGS_RD_TABLES_PREALLOCATED (1 << 1) > @@ -2359,6 +2360,13 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, > its_write_baser(its, baser, val); > tmp = baser->val; > > + if (its->flags & ITS_FLAGS_DMA_NON_COHERENT) { > + if (tmp & GITS_BASER_SHAREABILITY_MASK) > + tmp &= ~GITS_BASER_SHAREABILITY_MASK; > + else > + gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order)); > + } > + > if ((val ^ tmp) & GITS_BASER_SHAREABILITY_MASK) { > /* > * Shareability didn't stick. Just use > @@ -3055,6 +3063,7 @@ static u64 its_clear_vpend_valid(void __iomem *vlpi_base, u64 clr, u64 set) > > static void its_cpu_init_lpis(void) > { > + struct its_node *its = list_first_entry(&its_nodes, struct its_node, entry); > void __iomem *rbase = gic_data_rdist_rd_base(); > struct page *pend_page; > phys_addr_t paddr; > @@ -3096,6 +3105,9 @@ static void its_cpu_init_lpis(void) > gicr_write_propbaser(val, rbase + GICR_PROPBASER); > tmp = gicr_read_propbaser(rbase + GICR_PROPBASER); > > + if (its->flags & ITS_FLAGS_DMA_NON_COHERENT) > + tmp &= ~GICR_PROPBASER_SHAREABILITY_MASK; > + > if ((tmp ^ val) & GICR_PROPBASER_SHAREABILITY_MASK) { > if (!(tmp & GICR_PROPBASER_SHAREABILITY_MASK)) { > /* > @@ -3120,6 +3132,9 @@ static void its_cpu_init_lpis(void) > gicr_write_pendbaser(val, rbase + GICR_PENDBASER); > tmp = gicr_read_pendbaser(rbase + GICR_PENDBASER); > > + if (its->flags & ITS_FLAGS_DMA_NON_COHERENT) > + tmp &= ~GICR_PENDBASER_SHAREABILITY_MASK; > + > if (!(tmp & GICR_PENDBASER_SHAREABILITY_MASK)) { > /* > * The HW reports non-shareable, we must remove the > @@ -5005,6 +5020,7 @@ 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 device_node *np = to_of_node(handle); > struct its_node *its; > void __iomem *its_base; > u64 baser, tmp, typer; > @@ -5076,6 +5092,9 @@ static int __init its_probe_one(struct resource *res, > its->get_msi_base = its_irq_get_msi_base; > its->msi_domain_flags = IRQ_DOMAIN_FLAG_MSI_REMAP; > > + if (np && !of_dma_is_coherent(np)) > + its->flags |= ITS_FLAGS_DMA_NON_COHERENT; > + > its_enable_quirks(its); > > err = its_alloc_tables(its); > @@ -5095,6 +5114,9 @@ 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_DMA_NON_COHERENT) > + tmp &= ~GITS_CBASER_SHAREABILITY_MASK; > + > if ((tmp ^ baser) & GITS_CBASER_SHAREABILITY_MASK) { > if (!(tmp & GITS_CBASER_SHAREABILITY_MASK)) { > /* > -- > 2.39.2 >
On Fri, 10 Mar 2023 11:41:46 +0000, Peter Geis <pgwipeout@gmail.com> wrote: > > On Fri, Mar 10, 2023 at 3:05 AM Lucas Tanure <lucas.tanure@collabora.com> wrote: > > > > The GIC600 integration in RK356x, used in rk3588, doesn't support > > any of the shareability or cacheability attributes, and requires > > both values to be set to 0b00 for all the ITS and Redistributor > > tables. > > > > This is loosely based on prior work from XiaoDong Huang and > > Peter Geis fixing this issue specifically for Rockchip 356x. > > Good Morning, > > Since the gic is using dma, would it be reasonable to have all memory > allocations be requested with the GFP_DMA flag? Otherwise this doesn't > fully solve the problem for rk356x, where only the lower 4GB range is > DMA capable, but this tends to get allocated in the upper 4GB on 8GB > boards. That's an erratum. Please treat as such. M.
On Fri, Mar 10, 2023 at 6:57 AM Marc Zyngier <maz@kernel.org> wrote: > > On Fri, 10 Mar 2023 11:41:46 +0000, > Peter Geis <pgwipeout@gmail.com> wrote: > > > > On Fri, Mar 10, 2023 at 3:05 AM Lucas Tanure <lucas.tanure@collabora.com> wrote: > > > > > > The GIC600 integration in RK356x, used in rk3588, doesn't support > > > any of the shareability or cacheability attributes, and requires > > > both values to be set to 0b00 for all the ITS and Redistributor > > > tables. > > > > > > This is loosely based on prior work from XiaoDong Huang and > > > Peter Geis fixing this issue specifically for Rockchip 356x. > > > > Good Morning, > > > > Since the gic is using dma, would it be reasonable to have all memory > > allocations be requested with the GFP_DMA flag? Otherwise this doesn't > > fully solve the problem for rk356x, where only the lower 4GB range is > > DMA capable, but this tends to get allocated in the upper 4GB on 8GB > > boards. > > That's an erratum. Please treat as such. Good Morning, Yes, believe me I'm fully aware of how broken rk356x is. I'm asking an educational question from a kernel standards point of view, absent the rk356x issues. Would it be reasonable that since the gic uses dma memory, allocations for the gic should be made with the GFP_DMA flag? Or is that a misuse of the flag? Very Respectfully, Peter Geis > > M. > > -- > Without deviation from the norm, progress is not possible.
On 2023-03-10 11:41, Peter Geis wrote: > On Fri, Mar 10, 2023 at 3:05 AM Lucas Tanure <lucas.tanure@collabora.com> wrote: >> >> The GIC600 integration in RK356x, used in rk3588, doesn't support >> any of the shareability or cacheability attributes, and requires >> both values to be set to 0b00 for all the ITS and Redistributor >> tables. >> >> This is loosely based on prior work from XiaoDong Huang and >> Peter Geis fixing this issue specifically for Rockchip 356x. > > Good Morning, > > Since the gic is using dma, would it be reasonable to have all memory > allocations be requested with the GFP_DMA flag? Otherwise this doesn't > fully solve the problem for rk356x, where only the lower 4GB range is > DMA capable, but this tends to get allocated in the upper 4GB on 8GB > boards. Not really, because there's no fixed definition of what GFP_DMA actually means, and it may mean nothing (same for GFP_DMA32, which may or may not be meaningful depending on kernel config and platform topology). Drivers should really use the DMA API allocation functions if they care about what they get, which comes back round to the notion from years ago of converting the ITS driver to a regular platform driver, so it can benefit from regular DT concepts like "dma-ranges" automatically. Thanks, Robin. > > Very Respectfully, > Peter Geis > >> >> Suggested-by: Robin Murphy <robin.murphy@arm.com> >> Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com> >> --- >> drivers/irqchip/irq-gic-v3-its.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c >> index 973ede0197e3..1c334dfeb647 100644 >> --- a/drivers/irqchip/irq-gic-v3-its.c >> +++ b/drivers/irqchip/irq-gic-v3-its.c >> @@ -42,6 +42,7 @@ >> #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING (1ULL << 0) >> #define ITS_FLAGS_WORKAROUND_CAVIUM_22375 (1ULL << 1) >> #define ITS_FLAGS_WORKAROUND_CAVIUM_23144 (1ULL << 2) >> +#define ITS_FLAGS_DMA_NON_COHERENT (1ULL << 3) >> >> #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0) >> #define RDIST_FLAGS_RD_TABLES_PREALLOCATED (1 << 1) >> @@ -2359,6 +2360,13 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, >> its_write_baser(its, baser, val); >> tmp = baser->val; >> >> + if (its->flags & ITS_FLAGS_DMA_NON_COHERENT) { >> + if (tmp & GITS_BASER_SHAREABILITY_MASK) >> + tmp &= ~GITS_BASER_SHAREABILITY_MASK; >> + else >> + gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order)); >> + } >> + >> if ((val ^ tmp) & GITS_BASER_SHAREABILITY_MASK) { >> /* >> * Shareability didn't stick. Just use >> @@ -3055,6 +3063,7 @@ static u64 its_clear_vpend_valid(void __iomem *vlpi_base, u64 clr, u64 set) >> >> static void its_cpu_init_lpis(void) >> { >> + struct its_node *its = list_first_entry(&its_nodes, struct its_node, entry); >> void __iomem *rbase = gic_data_rdist_rd_base(); >> struct page *pend_page; >> phys_addr_t paddr; >> @@ -3096,6 +3105,9 @@ static void its_cpu_init_lpis(void) >> gicr_write_propbaser(val, rbase + GICR_PROPBASER); >> tmp = gicr_read_propbaser(rbase + GICR_PROPBASER); >> >> + if (its->flags & ITS_FLAGS_DMA_NON_COHERENT) >> + tmp &= ~GICR_PROPBASER_SHAREABILITY_MASK; >> + >> if ((tmp ^ val) & GICR_PROPBASER_SHAREABILITY_MASK) { >> if (!(tmp & GICR_PROPBASER_SHAREABILITY_MASK)) { >> /* >> @@ -3120,6 +3132,9 @@ static void its_cpu_init_lpis(void) >> gicr_write_pendbaser(val, rbase + GICR_PENDBASER); >> tmp = gicr_read_pendbaser(rbase + GICR_PENDBASER); >> >> + if (its->flags & ITS_FLAGS_DMA_NON_COHERENT) >> + tmp &= ~GICR_PENDBASER_SHAREABILITY_MASK; >> + >> if (!(tmp & GICR_PENDBASER_SHAREABILITY_MASK)) { >> /* >> * The HW reports non-shareable, we must remove the >> @@ -5005,6 +5020,7 @@ 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 device_node *np = to_of_node(handle); >> struct its_node *its; >> void __iomem *its_base; >> u64 baser, tmp, typer; >> @@ -5076,6 +5092,9 @@ static int __init its_probe_one(struct resource *res, >> its->get_msi_base = its_irq_get_msi_base; >> its->msi_domain_flags = IRQ_DOMAIN_FLAG_MSI_REMAP; >> >> + if (np && !of_dma_is_coherent(np)) >> + its->flags |= ITS_FLAGS_DMA_NON_COHERENT; >> + >> its_enable_quirks(its); >> >> err = its_alloc_tables(its); >> @@ -5095,6 +5114,9 @@ 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_DMA_NON_COHERENT) >> + tmp &= ~GITS_CBASER_SHAREABILITY_MASK; >> + >> if ((tmp ^ baser) & GITS_CBASER_SHAREABILITY_MASK) { >> if (!(tmp & GITS_CBASER_SHAREABILITY_MASK)) { >> /* >> -- >> 2.39.2 >>
On Fri, 10 Mar 2023 12:04:16 +0000, Peter Geis <pgwipeout@gmail.com> wrote: > > On Fri, Mar 10, 2023 at 6:57 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Fri, 10 Mar 2023 11:41:46 +0000, > > Peter Geis <pgwipeout@gmail.com> wrote: > > > > > > On Fri, Mar 10, 2023 at 3:05 AM Lucas Tanure <lucas.tanure@collabora.com> wrote: > > > > > > > > The GIC600 integration in RK356x, used in rk3588, doesn't support > > > > any of the shareability or cacheability attributes, and requires > > > > both values to be set to 0b00 for all the ITS and Redistributor > > > > tables. > > > > > > > > This is loosely based on prior work from XiaoDong Huang and > > > > Peter Geis fixing this issue specifically for Rockchip 356x. > > > > > > Good Morning, > > > > > > Since the gic is using dma, would it be reasonable to have all memory > > > allocations be requested with the GFP_DMA flag? Otherwise this doesn't > > > fully solve the problem for rk356x, where only the lower 4GB range is > > > DMA capable, but this tends to get allocated in the upper 4GB on 8GB > > > boards. > > > > That's an erratum. Please treat as such. > > Good Morning, > > Yes, believe me I'm fully aware of how broken rk356x is. I'm asking an > educational question from a kernel standards point of view, absent the > rk356x issues. Would it be reasonable that since the gic uses dma > memory, allocations for the gic should be made with the GFP_DMA flag? > Or is that a misuse of the flag? As Robin points out in another part of the thread, the right fix would be to convert the ITS as a pure platform driver and handle the probing dependencies that this would generate. Then you can start making us of the DMA allocator and of the firmware description of what ranges are reachable from the GIC (although this isn't strictly the ITS, but also the redistributors). We're in a better place for this now than we were a few years ago, but this is still a sizeable amount of work. If someone is motivated enough, they can have a go at it. M.
On 10-03-2023 12:04, Robin Murphy wrote: > On 2023-03-10 11:41, Peter Geis wrote: >> On Fri, Mar 10, 2023 at 3:05 AM Lucas Tanure >> <lucas.tanure@collabora.com> wrote: >>> >>> The GIC600 integration in RK356x, used in rk3588, doesn't support >>> any of the shareability or cacheability attributes, and requires >>> both values to be set to 0b00 for all the ITS and Redistributor >>> tables. >>> >>> This is loosely based on prior work from XiaoDong Huang and >>> Peter Geis fixing this issue specifically for Rockchip 356x. >> >> Good Morning, >> >> Since the gic is using dma, would it be reasonable to have all memory >> allocations be requested with the GFP_DMA flag? Otherwise this doesn't >> fully solve the problem for rk356x, where only the lower 4GB range is >> DMA capable, but this tends to get allocated in the upper 4GB on 8GB >> boards. > > Not really, because there's no fixed definition of what GFP_DMA actually > means, and it may mean nothing (same for GFP_DMA32, which may or may not > be meaningful depending on kernel config and platform topology). Drivers > should really use the DMA API allocation functions if they care about > what they get, which comes back round to the notion from years ago of > converting the ITS driver to a regular platform driver, so it can > benefit from regular DT concepts like "dma-ranges" automatically. > > Thanks, > Robin. > I am looking how to do that conversion to platform driver. But about the communication between irq-gic-v3-its and irq-gic-v3. Should irq-gic-v3-its be a MFD child of irq-gic-v3? Or use the component bind/unbind framework? >> >> Very Respectfully, >> Peter Geis >> >>> >>> Suggested-by: Robin Murphy <robin.murphy@arm.com> >>> Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com> >>> --- >>> drivers/irqchip/irq-gic-v3-its.c | 22 ++++++++++++++++++++++ >>> 1 file changed, 22 insertions(+) >>> >>> diff --git a/drivers/irqchip/irq-gic-v3-its.c >>> b/drivers/irqchip/irq-gic-v3-its.c >>> index 973ede0197e3..1c334dfeb647 100644 >>> --- a/drivers/irqchip/irq-gic-v3-its.c >>> +++ b/drivers/irqchip/irq-gic-v3-its.c >>> @@ -42,6 +42,7 @@ >>> #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING (1ULL << 0) >>> #define ITS_FLAGS_WORKAROUND_CAVIUM_22375 (1ULL << 1) >>> #define ITS_FLAGS_WORKAROUND_CAVIUM_23144 (1ULL << 2) >>> +#define ITS_FLAGS_DMA_NON_COHERENT (1ULL << 3) >>> >>> #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0) >>> #define RDIST_FLAGS_RD_TABLES_PREALLOCATED (1 << 1) >>> @@ -2359,6 +2360,13 @@ static int its_setup_baser(struct its_node >>> *its, struct its_baser *baser, >>> its_write_baser(its, baser, val); >>> tmp = baser->val; >>> >>> + if (its->flags & ITS_FLAGS_DMA_NON_COHERENT) { >>> + if (tmp & GITS_BASER_SHAREABILITY_MASK) >>> + tmp &= ~GITS_BASER_SHAREABILITY_MASK; >>> + else >>> + gic_flush_dcache_to_poc(base, >>> PAGE_ORDER_TO_SIZE(order)); >>> + } >>> + >>> if ((val ^ tmp) & GITS_BASER_SHAREABILITY_MASK) { >>> /* >>> * Shareability didn't stick. Just use >>> @@ -3055,6 +3063,7 @@ static u64 its_clear_vpend_valid(void __iomem >>> *vlpi_base, u64 clr, u64 set) >>> >>> static void its_cpu_init_lpis(void) >>> { >>> + struct its_node *its = list_first_entry(&its_nodes, struct >>> its_node, entry); >>> void __iomem *rbase = gic_data_rdist_rd_base(); >>> struct page *pend_page; >>> phys_addr_t paddr; >>> @@ -3096,6 +3105,9 @@ static void its_cpu_init_lpis(void) >>> gicr_write_propbaser(val, rbase + GICR_PROPBASER); >>> tmp = gicr_read_propbaser(rbase + GICR_PROPBASER); >>> >>> + if (its->flags & ITS_FLAGS_DMA_NON_COHERENT) >>> + tmp &= ~GICR_PROPBASER_SHAREABILITY_MASK; >>> + >>> if ((tmp ^ val) & GICR_PROPBASER_SHAREABILITY_MASK) { >>> if (!(tmp & GICR_PROPBASER_SHAREABILITY_MASK)) { >>> /* >>> @@ -3120,6 +3132,9 @@ static void its_cpu_init_lpis(void) >>> gicr_write_pendbaser(val, rbase + GICR_PENDBASER); >>> tmp = gicr_read_pendbaser(rbase + GICR_PENDBASER); >>> >>> + if (its->flags & ITS_FLAGS_DMA_NON_COHERENT) >>> + tmp &= ~GICR_PENDBASER_SHAREABILITY_MASK; >>> + >>> if (!(tmp & GICR_PENDBASER_SHAREABILITY_MASK)) { >>> /* >>> * The HW reports non-shareable, we must remove the >>> @@ -5005,6 +5020,7 @@ 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 device_node *np = to_of_node(handle); >>> struct its_node *its; >>> void __iomem *its_base; >>> u64 baser, tmp, typer; >>> @@ -5076,6 +5092,9 @@ static int __init its_probe_one(struct resource >>> *res, >>> its->get_msi_base = its_irq_get_msi_base; >>> its->msi_domain_flags = IRQ_DOMAIN_FLAG_MSI_REMAP; >>> >>> + if (np && !of_dma_is_coherent(np)) >>> + its->flags |= ITS_FLAGS_DMA_NON_COHERENT; >>> + >>> its_enable_quirks(its); >>> >>> err = its_alloc_tables(its); >>> @@ -5095,6 +5114,9 @@ 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_DMA_NON_COHERENT) >>> + tmp &= ~GITS_CBASER_SHAREABILITY_MASK; >>> + >>> if ((tmp ^ baser) & GITS_CBASER_SHAREABILITY_MASK) { >>> if (!(tmp & GITS_CBASER_SHAREABILITY_MASK)) { >>> /* >>> -- >>> 2.39.2 >>>
On Tue, 14 Mar 2023 13:25:28 +0000, Lucas Tanure <lucas.tanure@collabora.com> wrote: > > On 10-03-2023 12:04, Robin Murphy wrote: > > On 2023-03-10 11:41, Peter Geis wrote: > >> On Fri, Mar 10, 2023 at 3:05 AM Lucas Tanure > >> <lucas.tanure@collabora.com> wrote: > >>> > >>> The GIC600 integration in RK356x, used in rk3588, doesn't support > >>> any of the shareability or cacheability attributes, and requires > >>> both values to be set to 0b00 for all the ITS and Redistributor > >>> tables. > >>> > >>> This is loosely based on prior work from XiaoDong Huang and > >>> Peter Geis fixing this issue specifically for Rockchip 356x. > >> > >> Good Morning, > >> > >> Since the gic is using dma, would it be reasonable to have all memory > >> allocations be requested with the GFP_DMA flag? Otherwise this doesn't > >> fully solve the problem for rk356x, where only the lower 4GB range is > >> DMA capable, but this tends to get allocated in the upper 4GB on 8GB > >> boards. > > > > Not really, because there's no fixed definition of what GFP_DMA > > actually means, and it may mean nothing (same for GFP_DMA32, which > > may or may not be meaningful depending on kernel config and platform > > topology). Drivers should really use the DMA API allocation > > functions if they care about what they get, which comes back round > > to the notion from years ago of converting the ITS driver to a > > regular platform driver, so it can benefit from regular DT concepts > > like "dma-ranges" automatically. > > > > Thanks, > > Robin. > > > I am looking how to do that conversion to platform driver. > But about the communication between irq-gic-v3-its and irq-gic-v3. > Should irq-gic-v3-its be a MFD child of irq-gic-v3? MFD? I'd rather suggest an VME bus driver. ;-) Seriously, this is an interrupt controller. Nothing else. It should probe the parent irqdomain, and stack onto that. No parent? Probe deferral. > Or use the component bind/unbind framework? I don't understand what you mean here. M.
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c index 973ede0197e3..1c334dfeb647 100644 --- a/drivers/irqchip/irq-gic-v3-its.c +++ b/drivers/irqchip/irq-gic-v3-its.c @@ -42,6 +42,7 @@ #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING (1ULL << 0) #define ITS_FLAGS_WORKAROUND_CAVIUM_22375 (1ULL << 1) #define ITS_FLAGS_WORKAROUND_CAVIUM_23144 (1ULL << 2) +#define ITS_FLAGS_DMA_NON_COHERENT (1ULL << 3) #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0) #define RDIST_FLAGS_RD_TABLES_PREALLOCATED (1 << 1) @@ -2359,6 +2360,13 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, its_write_baser(its, baser, val); tmp = baser->val; + if (its->flags & ITS_FLAGS_DMA_NON_COHERENT) { + if (tmp & GITS_BASER_SHAREABILITY_MASK) + tmp &= ~GITS_BASER_SHAREABILITY_MASK; + else + gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order)); + } + if ((val ^ tmp) & GITS_BASER_SHAREABILITY_MASK) { /* * Shareability didn't stick. Just use @@ -3055,6 +3063,7 @@ static u64 its_clear_vpend_valid(void __iomem *vlpi_base, u64 clr, u64 set) static void its_cpu_init_lpis(void) { + struct its_node *its = list_first_entry(&its_nodes, struct its_node, entry); void __iomem *rbase = gic_data_rdist_rd_base(); struct page *pend_page; phys_addr_t paddr; @@ -3096,6 +3105,9 @@ static void its_cpu_init_lpis(void) gicr_write_propbaser(val, rbase + GICR_PROPBASER); tmp = gicr_read_propbaser(rbase + GICR_PROPBASER); + if (its->flags & ITS_FLAGS_DMA_NON_COHERENT) + tmp &= ~GICR_PROPBASER_SHAREABILITY_MASK; + if ((tmp ^ val) & GICR_PROPBASER_SHAREABILITY_MASK) { if (!(tmp & GICR_PROPBASER_SHAREABILITY_MASK)) { /* @@ -3120,6 +3132,9 @@ static void its_cpu_init_lpis(void) gicr_write_pendbaser(val, rbase + GICR_PENDBASER); tmp = gicr_read_pendbaser(rbase + GICR_PENDBASER); + if (its->flags & ITS_FLAGS_DMA_NON_COHERENT) + tmp &= ~GICR_PENDBASER_SHAREABILITY_MASK; + if (!(tmp & GICR_PENDBASER_SHAREABILITY_MASK)) { /* * The HW reports non-shareable, we must remove the @@ -5005,6 +5020,7 @@ 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 device_node *np = to_of_node(handle); struct its_node *its; void __iomem *its_base; u64 baser, tmp, typer; @@ -5076,6 +5092,9 @@ static int __init its_probe_one(struct resource *res, its->get_msi_base = its_irq_get_msi_base; its->msi_domain_flags = IRQ_DOMAIN_FLAG_MSI_REMAP; + if (np && !of_dma_is_coherent(np)) + its->flags |= ITS_FLAGS_DMA_NON_COHERENT; + its_enable_quirks(its); err = its_alloc_tables(its); @@ -5095,6 +5114,9 @@ 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_DMA_NON_COHERENT) + tmp &= ~GITS_CBASER_SHAREABILITY_MASK; + if ((tmp ^ baser) & GITS_CBASER_SHAREABILITY_MASK) { if (!(tmp & GITS_CBASER_SHAREABILITY_MASK)) { /*
The GIC600 integration in RK356x, used in rk3588, doesn't support any of the shareability or cacheability attributes, and requires both values to be set to 0b00 for all the ITS and Redistributor tables. This is loosely based on prior work from XiaoDong Huang and Peter Geis fixing this issue specifically for Rockchip 356x. Suggested-by: Robin Murphy <robin.murphy@arm.com> Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com> --- drivers/irqchip/irq-gic-v3-its.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)