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

[Tomasz Nowicki]

On 10.02.2016 11: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

[..]

> > -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.
>
> > +				struct fwnode_handle *handler)
> >   {
> > -	struct resource res;
> >   	struct its_node *its;
> >   	void __iomem *its_base;
> >   	struct irq_domain *inner_domain;
> > @@ -1435,33 +1436,26 @@ static int __init its_probe(struct device_node *node,
> >   	u64 baser, tmp;
> >   	int err;
> >
> > -	err = of_address_to_resource(node, 0, &res);
> > -	if (err) {
> > -		pr_warn("%s: no regs?\n", node->full_name);
> > -		return -ENXIO;
> > -	}
> > -
> > -	its_base = ioremap(res.start, resource_size(&res));
> > +	its_base = ioremap(phys_base, size);
> >   	if (!its_base) {
> > -		pr_warn("%s: unable to map registers\n", node->full_name);
> > +		pr_warn("Unable to map ITS registers\n");
>
> There is some value in at least displaying the base address - think of
> people writing their DT or ACPI tables. Please do not blindly remove
> debugging information which is often useful on a system with multiple ITS.

Yes, base address is helpful with multi ITS.

> >   		return -ENOMEM;
> >   	}
> >
> >   	val = readl_relaxed(its_base + GITS_PIDR2) & GIC_PIDR2_ARCH_MASK;
> >   	if (val != 0x30 && val != 0x40) {
> > -		pr_warn("%s: no ITS detected, giving up\n", node->full_name);
> > +		pr_warn("No ITS detected, giving up\n");
> >   		err = -ENODEV;
> >   		goto out_unmap;
> >   	}
> >
> >   	err = its_force_quiescent(its_base);
> >   	if (err) {
> > -		pr_warn("%s: failed to quiesce, giving up\n",
> > -			node->full_name);
> > +		pr_warn("Failed to quiesce, giving up\n");
> >   		goto out_unmap;
> >   	}
> >
> > -	pr_info("ITS: %s\n", node->full_name);
> > +	pr_info("ITS@0x%lx\n", (long)phys_base);
> >
> >   	its = kzalloc(sizeof(*its), GFP_KERNEL);
> >   	if (!its) {
> > @@ -1473,7 +1467,7 @@ static int __init its_probe(struct device_node *node,
> >   	INIT_LIST_HEAD(&its->entry);
> >   	INIT_LIST_HEAD(&its->its_device_list);
> >   	its->base = its_base;
> > -	its->phys_base = res.start;
> > +	its->phys_base = phys_base;
> >   	its->ite_size = ((readl_relaxed(its_base + GITS_TYPER) >> 4) & 0xf) + 1;
> >
> >   	its->cmd_base = kzalloc(ITS_CMD_QUEUE_SZ, GFP_KERNEL);
> > @@ -1485,7 +1479,7 @@ static int __init its_probe(struct device_node *node,
> >
> >   	its_enable_quirks(its);
> >
> > -	err = its_alloc_tables(node->full_name, its);
> > +	err = its_alloc_tables(its);
> >   	if (err)
> >   		goto out_free_cmd;
> >
> > @@ -1521,7 +1515,7 @@ static int __init its_probe(struct device_node *node,
> >   	writeq_relaxed(0, its->base + GITS_CWRITER);
> >   	writel_relaxed(GITS_CTLR_ENABLE, its->base + GITS_CTLR);
> >
> > -	if (of_property_read_bool(node, "msi-controller")) {
> > +	if (is_msi_controller) {
> >   		struct msi_domain_info *info;
> >
> >   		info = kzalloc(sizeof(*info), GFP_KERNEL);
> > @@ -1530,7 +1524,8 @@ static int __init its_probe(struct device_node *node,
> >   			goto out_free_tables;
> >   		}
> >
> > -		inner_domain = irq_domain_add_tree(node, &its_domain_ops, its);
> > +		inner_domain = irq_domain_create_tree(handler, &its_domain_ops,
> > +						      its);
> >   		if (!inner_domain) {
> >   			err = -ENOMEM;
> >   			kfree(info);
> > @@ -1558,10 +1553,28 @@ out_free_its:
> >   	kfree(its);
> >   out_unmap:
> >   	iounmap(its_base);
> > -	pr_err("ITS: failed probing %s (%d)\n", node->full_name, err);
> > +	pr_err("ITS@0x%lx: failed probing (%d)\n", (long)phys_base, err);
> >   	return err;
> >   }
> >
> > +static int __init
> > +its_of_probe(struct device_node *node, struct irq_domain *parent)
> > +{
> > +	struct resource res;
> > +	bool is_msi_controller = false;
> > +
> > +	if (of_address_to_resource(node, 0, &res)) {
> > +		pr_warn("%s: no regs?\n", node->full_name);
> > +		return -ENXIO;
> > +	}
> > +
> > +	if (of_property_read_bool(node, "msi-controller"))
> > +		is_msi_controller = true;
>
> This is where you should return early, with a message saying that this
> ITS is being ignored if it doesn't have the msi-controller property.

Agree, will do.

Thanks,
Tomasz