On Thu, Jan 23, 2025 at 12:12:03PM +0000, Marc Zyngier wrote:

On Wed, 22 Jan 2025 17:34:51 +0000,
Manivannan Sadhasivam [off-list ref] wrote:

quoted

+ Marc (for the IRQCHIP implementation review)

On Wed, Jan 22, 2025 at 09:28:12PM +0800, Chen Wang wrote:

quoted

quoted

quoted

+static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie,
+				 struct device_node *msi_node)
+{
+	struct device *dev = pcie->cdns_pcie->dev;
+	struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node);
+	struct irq_domain *parent_domain;
+	int ret = 0;
+
+	if (!of_property_read_bool(msi_node, "msi-controller"))
+		return -ENODEV;
+
+	ret = of_irq_get_byname(msi_node, "msi");
+	if (ret <= 0) {
+		dev_err(dev, "%pOF: failed to get MSI irq\n", msi_node);
+		return ret;
+	}
+	pcie->msi_irq = ret;
+
+	irq_set_chained_handler_and_data(pcie->msi_irq,
+					 sg2042_pcie_msi_chained_isr, pcie);
+
+	parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS,
+						 &sg2042_pcie_msi_domain_ops, pcie);
+	if (!parent_domain) {
+		dev_err(dev, "%pfw: Failed to create IRQ domain\n", fwnode);
+		return -ENOMEM;
+	}
+	irq_domain_update_bus_token(parent_domain, DOMAIN_BUS_NEXUS);
+

The MSI controller is wired to PLIC isn't it? If so, why can't you use
hierarchial MSI domain implementation as like other controller drivers?

The method used here is somewhat similar to dw_pcie_allocate_domains() in
drivers/pci/controller/dwc/pcie-designware-host.c. This MSI controller is
about Method A, the PCIe controller implements an MSI interrupt controller
inside, and connect to PLIC upward through only ONE interrupt line. Because
MSI to PLIC is multiple to one, I use linear mode here and use chained ISR
to handle the interrupts.

Hmm, ok. I'm not an IRQCHIP expert, but I'll defer to Marc to review the IRQCHIP
implementation part.

I don't offer this service anymore, I'm afraid.

As for the "I create my own non-hierarchical IRQ domain", this is
something that happens for all completely mis-designed interrupt
controllers, MSI or not, that multiplex interrupts.

These implementations are stuck in the previous century, and seeing
this on modern designs, for a "server SoC", is really pathetic.

maybe you now understand why I don't offer this sort of reviewing
service anymore.

Ok, I can understand your pain as a maintainer. I'll try my best to review these
implementations as I have no other choice :)

- Mani

-- 
மணிவண்ணன் சதாசிவம்