Re: [PATCH 1/7] irqchip/gic-v3: Add a DMA Non-Coherent flag

[Lucas Tanure <lucas.tanure@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx> 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@xxxxxxx>
> > > Signed-off-by: Lucas Tanure <lucas.tanure@xxxxxxxxxxxxx>
> > > ---
> > >   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