Re: [PATCH v4 03/10] irqchip: Add Broadcom bcm2712 MSI-X interrupt controller

Hi Thomas,

Thank you for the review!

On 10/28/24 22:06, Thomas Gleixner wrote:

On Fri, Oct 25 2024 at 15:45, Stanimir Varbanov wrote:

quoted

Add an interrupt controller driver for MSI-X Interrupt Peripheral (MIP)
hardware block found in bcm2712. The interrupt controller is used to
handle MSI-X interrupts from peripherials behind PCIe endpoints like
RP1 south bridge found in RPi5.

There are two MIPs on bcm2712, the first has 64 consecutive SPIs
assigned to 64 output vectors, and the second has 17 SPIs, but only
8 of them are consecutive starting at the 8th output vector.

This starts to converge nicely. Just a few remaining nitpicks.

quoted

+static int mip_alloc_hwirq(struct mip_priv *mip, unsigned int nr_irqs,
+			   unsigned int *hwirq)
+{
+	int bit;
+
+	spin_lock(&mip->lock);
+	bit = bitmap_find_free_region(mip->bitmap, mip->num_msis,
+				      ilog2(nr_irqs));
+	spin_unlock(&mip->lock);

This should be

        scoped_guard(spinlock, &mip->lock)
		bit = bitmap_find_free_region(mip->bitmap, mip->num_msis, ilog2(nr_irqs));

quoted

+	if (bit < 0)
+		return bit;
+
+	if (hwirq)
+		*hwirq = bit;

But what's the point of this conditional? The only call site hands in a
valid pointer, no?

quoted

+	return 0;

And therefore the whole thing can be simplified to:

static int mip_alloc_hwirq(struct mip_priv *mip, unsigned int nr_irqs)
{
        guard(spinlock)(&mip_lock);
        return bitmap_find_free_region(mip->bitmap, mip->num_msis, ilog2(nr_irqs));
}

and the callsite becomes:

        irq = mip_alloc_hwirq(mip, nr_irqs);
        if (irq < 0)
        	return irq;
Hmm?

quoted

+}
+
+static void mip_free_hwirq(struct mip_priv *mip, unsigned int hwirq,
+			   unsigned int nr_irqs)
+{
+	spin_lock(&mip->lock);

	guard(spinlock)(&mip->lock);

Will address the above comments in next version.

quoted

+	bitmap_release_region(mip->bitmap, hwirq, ilog2(nr_irqs));
+	spin_unlock(&mip->lock);
+}

quoted

+	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &fwspec);
+	if (ret) {
+		mip_free_hwirq(mip, irq, nr_irqs);
+		return ret;

                goto err_free_hwirq; ?

quoted

+	}
+
+	for (i = 0; i < nr_irqs; i++) {
+		irqd = irq_domain_get_irq_data(domain->parent, virq + i);
+		irqd->chip->irq_set_type(irqd, IRQ_TYPE_EDGE_RISING);
+
+		ret = irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+						    &mip_middle_irq_chip, mip);
+		if (ret)
+			goto err_free;
+
+		irqd = irq_get_irq_data(virq + i);
+		irqd_set_single_target(irqd);
+		irqd_set_affinity_on_activate(irqd);
+	}
+
+	return 0;
+
+err_free:
+	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
+	mip_free_hwirq(mip, irq, nr_irqs);
+	return ret;
+}
+
+static int __init mip_of_msi_init(struct device_node *node,
+				  struct device_node *parent)

No line break required here.

OK.

quoted

+{
+	struct platform_device *pdev;
+	struct mip_priv *mip;
+	int ret;
+
+	pdev = of_find_device_by_node(node);
+	of_node_put(node);
+	if (!pdev)
+		return -EPROBE_DEFER;
+
+	mip = kzalloc(sizeof(*mip), GFP_KERNEL);
+	if (!mip)
+		return -ENOMEM;
+
+	spin_lock_init(&mip->lock);
+	mip->dev = &pdev->dev;
+
+	ret = mip_parse_dt(mip, node);
+	if (ret)
+		goto err_priv;
+
+	mip->base = of_iomap(node, 0);
+	if (!mip->base) {
+		ret = -ENXIO;
+		goto err_priv;
+	}
+
+	mip->bitmap = bitmap_zalloc(mip->num_msis, GFP_KERNEL);
+	if (!mip->bitmap) {
+		ret = -ENOMEM;
+		goto err_base;
+	}
+
+	/*
+	 * All MSI-X masked in for the host, masked out for the
+	 * VPU, and edge-triggered.
+	 */
+	writel(0, mip->base + MIP_INT_MASKL_HOST);
+	writel(0, mip->base + MIP_INT_MASKH_HOST);
+	writel(~0, mip->base + MIP_INT_MASKL_VPU);
+	writel(~0, mip->base + MIP_INT_MASKH_VPU);
+	writel(~0, mip->base + MIP_INT_CFGL_HOST);
+	writel(~0, mip->base + MIP_INT_CFGH_HOST);

What undoes that in case mpi_init_domains() fails? Or is it harmless? I
really have no idea what masked in and masked out means here.

It should be harmless, but I could move registers initialization in
mip_init_domains() and fix the comments.

quoted

+	dev_dbg(&pdev->dev,
+		"MIP: MSI-X count: %u, base: %u, offset: %u, msg_addr: %llx\n",

Please move the string up. You have 100 characters width available.

OK.

quoted

+		mip->num_msis, mip->msi_base, mip->msi_offset, mip->msg_addr);

Thanks,

        tglx

regards,
~Stan