Re: [PATCH V3 06/10] irqchip, GICv3, ITS: Refator ITS dt init code to prepare for ACPI.

[Hanjun Guo]

On 2016/2/10 18:47, Marc Zyngier wrote:
> On 19/01/16 13:11, Tomasz Nowicki wrote:
> > Similarly to GICv3 core, we need to extract common code before adding
> > ACPI support. No functional changes.
> >
> > Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
> > Signed-off-by: Tomasz Nowicki <tn@xxxxxxxxxxxx>
> > ---
> >   drivers/irqchip/irq-gic-v3-its.c   | 82 +++++++++++++++++++++++---------------
> >   drivers/irqchip/irq-gic-v3.c       |  6 +--
> >   include/linux/irqchip/arm-gic-v3.h |  2 +-
> >   3 files changed, 52 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> > index 2bbed18..fecb7a6 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -813,7 +813,7 @@ static void its_free_tables(struct its_node *its)
> >   	}
> >   }
> >
> > -static int its_alloc_tables(const char *node_name, struct its_node *its)
> > +static int its_alloc_tables(struct its_node *its)
> >   {
> >   	int err;
> >   	int i;
> > @@ -868,8 +868,8 @@ static int its_alloc_tables(const char *node_name, struct its_node *its)
> >   				    order);
> >   			if (order >= MAX_ORDER) {
> >   				order = MAX_ORDER - 1;
> > -				pr_warn("%s: Device Table too large, reduce its page order to %u\n",
> > -					node_name, order);
> > +				pr_warn("ITS@0x%lx: Device Table too large, reduce its page order to %u\n",
> > +					its->phys_base, order);
> >   			}
> >   		}
> >
> > @@ -878,8 +878,8 @@ static int its_alloc_tables(const char *node_name, struct its_node *its)
> >   		if (alloc_pages > GITS_BASER_PAGES_MAX) {
> >   			alloc_pages = GITS_BASER_PAGES_MAX;
> >   			order = get_order(GITS_BASER_PAGES_MAX * psz);
> > -			pr_warn("%s: Device Table too large, reduce its page order to %u (%u pages)\n",
> > -				node_name, order, alloc_pages);
> > +			pr_warn("ITS@0x%lx: Device Table too large, reduce its page order to %u (%u pages)\n",
> > +				its->phys_base, order, alloc_pages);
> >   		}
> >
> >   		base = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
> > @@ -948,8 +948,8 @@ retry_baser:
> >   		}
> >
> >   		if (val != tmp) {
> > -			pr_err("ITS: %s: GITS_BASER%d doesn't stick: %lx %lx\n",
> > -			       node_name, i,
> > +			pr_err("ITS@0x%lx: GITS_BASER%d doesn't stick: %lx %lx\n",
> > +			       its->phys_base, i,
> >   			       (unsigned long) val, (unsigned long) tmp);
> >   			err = -ENXIO;
> >   			goto out_free;
> > @@ -1424,10 +1424,11 @@ static void its_enable_quirks(struct its_node *its)
> >   	gic_enable_quirks(iidr, its_quirks, its);
> >   }
> >
> > -static int __init its_probe(struct device_node *node,
> > -			    struct irq_domain *parent)
> > +static int __init its_probe_one(phys_addr_t phys_base, unsigned long size,
> > +				struct irq_domain *parent,
> > +				bool is_msi_controller,
>
> I really question the fact that you are keeping this msi_controller
> thing. Let's face it: if this is not an MSI controller, then the whole
> thing is absolutely pointless.
>
> So I'd rather you simplify the whole in a separate patch, and just don't
> bother initializing the ITS if it cannot be used for MSIs.

Agree, that will simplify the code a lot.

Thanks
Hanjun