Re: [PATCH 5/6] irqchip: Add Loongson PCH MSI controller

[Jiaxun Yang <jiaxun.yang@flygoat.com>]

On Thu, 23 Apr 2020 15:41:35 +0100
Marc Zyngier <maz@kernel.org> wrote:

> On Wed, 22 Apr 2020 22:24:25 +0800
> Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> 
> > This controller appears on Loongson-7A family of PCH to transform
> > interrupts from PCI MSI into HyperTransport vectorized interrrupts
> > and send them to procrssor's HT vector controller.
> > 
> > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> > ---
[...]
> > +	ret = irq_domain_alloc_irqs_parent(domain, virq, 1,
> > &fwspec);
> > +	if (ret)
> > +		return ret;
> > +
> > +	irq_domain_set_info(domain, virq, hwirq,
> > +			    &middle_irq_chip, NULL,
> > +			    handle_simple_irq, NULL, NULL);  
> 
> No, this should at least be handle_edge_irq. More importantly, you
> should use the flow set by the underlying irqchip, and not provide
> your own.

Hi Marc,
Thanks for your review.

The underlying irqchip (HTVEC) follows a simple_irq flow as it only
has mask/unmask callback, and it doesn't have tyoe configuration. so I
followed simple_irq flow.

How can I use the flow set by the underlying irqchip? Just use
irq_domain_set_hwirq_and_chip here and set_handler in HTVEC driver?


> 
> > +	irq_set_probe(virq);  
> 
> Probe? what does it mean for MSIs? I also don't see how you tell the
> underlying irqchip that the MSI is edge triggered.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int pch_msi_middle_domain_alloc(struct irq_domain *domain,
> > +					   unsigned int virq,
> > +					   unsigned int nr_irqs,
> > void *args) +{
> > +	struct pch_msi_data *priv = domain->host_data;
> > +	int hwirq, err, i;
> > +
> > +	hwirq = pch_msi_allocate_hwirq(priv, nr_irqs);
> > +	if (hwirq < 0)
> > +		return hwirq;
> > +
> > +	for (i = 0; i < nr_irqs; i++) {
> > +		err = pch_msi_parent_domain_alloc(domain, virq +
> > i, hwirq + i);
> > +		if (err)
> > +			goto err_hwirq;
> > +
> > +		irq_domain_set_hwirq_and_chip(domain, virq + i,
> > hwirq + i,
> > +					      &middle_irq_chip,
> > priv);
> > +	}
> > +
> > +	return 0;
> > +err_hwirq:
> > +	while (--i >= 0)
> > +		irq_domain_free_irqs_parent(domain, virq, i);
> > +
> > +	pch_msi_free_hwirq(priv, hwirq, nr_irqs);
> > +	return err;
> > +}
> > +
> > +static void pch_msi_middle_domain_free(struct irq_domain *domain,
> > +					   unsigned int virq,
> > +					   unsigned int nr_irqs)
> > +{
> > +	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> > +	struct pch_msi_data *priv = irq_data_get_irq_chip_data(d);
> > +
> > +	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
> > +	pch_msi_free_hwirq(priv, d->hwirq, nr_irqs);
> > +}
> > +
> > +static const struct irq_domain_ops pch_msi_middle_domain_ops = {
> > +	.alloc	= pch_msi_middle_domain_alloc,
> > +	.free	= pch_msi_middle_domain_free,
> > +};
> > +
> > +static int pch_msi_init_domains(struct pch_msi_data *priv,
> > +				struct device_node *node,
> > +				struct device_node *parent)
> > +{
> > +	struct irq_domain *middle_domain, *msi_domain,
> > *parent_domain; +
> > +	parent_domain = irq_find_host(parent);
> > +	if (!parent_domain) {
> > +		pr_err("Failed to find the parent domain\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	middle_domain = irq_domain_add_tree(NULL,
> > +
> > &pch_msi_middle_domain_ops,
> > +					    priv);  
> 
> You don't really need a tree, unless your interrupt space is huge and
> very sparse. Given that the DT example says 64, I would go with a
> linear domain if that number is realistic.
> 
It can up to 192 in latest generation of chip, is it still suitable?

In the latest generation, we have a enhanced version of HTVEC which has
another delivery system that will be able to configure affinity. That's
why I placed set_affinity call back here and in PCH PIC driver.

Thanks. 

[...]
> > +}
> > +
> > +IRQCHIP_DECLARE(pch_msi, "loongson,pch-msi-1.0", pch_msi_init);  
> 
> Thanks,
> 
> 	M.

--
Jiaxun Yang