[PATCH 0/3] irqchip: Add Sophgo SG2042 MSI controller

[PATCH 0/3] irqchip: Add Sophgo SG2042 MSI controller

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

This controller is on the Sophgo SG2042 SoC to transform interrupts from
PCIe MSI to PLIC interrupts.

Chen Wang (3):
  dt-bindings: interrupt-controller: Add Sophgo SG2042 MSI
  irqchip: Add the Sophgo SG2042 MSI interrupt controller
  riscv: sophgo: dts: add msi controller for SG2042

 .../sophgo,sg2042-msi.yaml                    |  78 ++++++
 arch/riscv/boot/dts/sophgo/sg2042.dtsi        |  13 +
 drivers/irqchip/Kconfig                       |   8 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-sg2042-msi.c              | 255 ++++++++++++++++++
 5 files changed, 355 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
 create mode 100644 drivers/irqchip/irq-sg2042-msi.c


base-commit: 2d5404caa8c7bb5c4e0435f94b28834ae5456623
-- 
2.34.1

[PATCH 1/3] dt-bindings: interrupt-controller: Add Sophgo SG2042 MSI

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Add binding for Sophgo SG2042 MSI controller.

Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
---
 .../sophgo,sg2042-msi.yaml                    | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml b/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
new file mode 100644
index 000000000000..9fe99b74c211
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/sophgo,sg2042-msi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo SG2042 MSI Controller
+
+maintainers:
+  - Chen Wang <unicorn_wang@outlook.com>
+
+description:
+  This interrupt controller is in Sophgo SG2042 for transforming interrupts from
+  PCIe MSI to PLIC interrupts.
+
+allOf:
+  - $ref: /schemas/interrupts.yaml#
+  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
+
+properties:
+  compatible:
+    const: sophgo,sg2042-msi
+
+  reg:
+    items:
+      - description: clear register
+
+  reg-names:
+    items:
+      - const: clr
+
+  sophgo,msi-doorbell-addr:
+    description:
+      u64 value of the MSI doorbell address
+    $ref: /schemas/types.yaml#/definitions/uint64
+
+  sophgo,msi-base-vec:
+    description:
+      u32 value of the base of parent PLIC vector allocated
+      to MSI.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 64
+    maximum: 95
+
+  sophgo,msi-num-vecs:
+    description:
+      u32 value of the number of parent PLIC vectors allocated
+      to MSI.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 32
+
+  msi-controller: true
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - msi-controller
+  - sophgo,msi-doorbell-addr
+  - sophgo,msi-base-vec
+  - sophgo,msi-num-vecs
+
+additionalProperties: true
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    msi: msi-controller@30000000 {
+      compatible = "sophgo,sg2042-msi";
+      reg = <0x30000000 0x4>;
+      reg-names = "clr";
+      msi-controller;
+      sophgo,msi-doorbell-addr = <0x00000070 0x30010300>;
+      sophgo,msi-base-vec = <64>;
+      sophgo,msi-num-vecs = <32>;
+      interrupt-parent = <&plic>;
+    };
-- 
2.34.1

[PATCH 2/3] irqchip: Add the Sophgo SG2042 MSI interrupt controller

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Add driver for Sophgo SG2042 MSI interrupt controller.

Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
---
 drivers/irqchip/Kconfig          |   8 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-sg2042-msi.c | 255 +++++++++++++++++++++++++++++++
 3 files changed, 264 insertions(+)
 create mode 100644 drivers/irqchip/irq-sg2042-msi.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index d82bcab233a1..76a38a4d62eb 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -729,6 +729,14 @@ config MCHP_EIC
 	help
 	  Support for Microchip External Interrupt Controller.
 
+config SOPHGO_SG2042_MSI
+	bool "Sophgo SG2042 MSI controller"
+	depends on ARCH_SOPHGO || COMPILE_TEST
+	help
+	  Support for the Sophgo SG2042 MSI Controller.
+	  This on-chip interrupt controller enables MSI sources to be
+	  routed to the primary PLIC controller on SoC.
+
 config SUNPLUS_SP7021_INTC
 	bool "Sunplus SP7021 interrupt controller" if COMPILE_TEST
 	default SOC_SP7021
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e3679ec2b9f7..53617890268a 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -125,4 +125,5 @@ obj-$(CONFIG_WPCM450_AIC)		+= irq-wpcm450-aic.o
 obj-$(CONFIG_IRQ_IDT3243X)		+= irq-idt3243x.o
 obj-$(CONFIG_APPLE_AIC)			+= irq-apple-aic.o
 obj-$(CONFIG_MCHP_EIC)			+= irq-mchp-eic.o
+obj-$(CONFIG_SOPHGO_SG2042_MSI)		+= irq-sg2042-msi.o
 obj-$(CONFIG_SUNPLUS_SP7021_INTC)	+= irq-sp7021-intc.o
diff --git a/drivers/irqchip/irq-sg2042-msi.c b/drivers/irqchip/irq-sg2042-msi.c
new file mode 100644
index 000000000000..79449f974ed5
--- /dev/null
+++ b/drivers/irqchip/irq-sg2042-msi.c
@@ -0,0 +1,255 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SG2042 MSI Controller
+ *
+ * Copyright (C) 2024 Sophgo Technology Inc.
+ * Copyright (C) 2024 Chen Wang <unicorn_wang@outlook.com>
+ */
+
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct sg2042_msi_data {
+	void __iomem	*reg_clr; /* clear reg, see TRM, 10.1.33, GP_INTR0_CLR */
+
+	u64		doorbell_addr; /* see TRM, 10.1.32, GP_INTR0_SET */
+
+	u32		irq_first; /* The vector number that MSIs starts */
+	u32		num_irqs;  /* The number of vectors for MSIs */
+
+	unsigned long	*msi_map;
+	struct mutex	msi_map_lock; /* lock for msi_map */
+};
+
+static int sg2042_msi_allocate_hwirq(struct sg2042_msi_data *priv, int num_req)
+{
+	int first;
+
+	mutex_lock(&priv->msi_map_lock);
+
+	first = bitmap_find_free_region(priv->msi_map, priv->num_irqs,
+					get_count_order(num_req));
+	if (first < 0) {
+		mutex_unlock(&priv->msi_map_lock);
+		return -ENOSPC;
+	}
+
+	mutex_unlock(&priv->msi_map_lock);
+
+	return priv->irq_first + first;
+}
+
+static void sg2042_msi_free_hwirq(struct sg2042_msi_data *priv,
+				  int hwirq, int num_req)
+{
+	int first = hwirq - priv->irq_first;
+
+	mutex_lock(&priv->msi_map_lock);
+	bitmap_release_region(priv->msi_map, first, get_count_order(num_req));
+	mutex_unlock(&priv->msi_map_lock);
+}
+
+static void sg2042_msi_irq_ack(struct irq_data *d)
+{
+	struct sg2042_msi_data *data  = irq_data_get_irq_chip_data(d);
+	int bit_off = d->hwirq - data->irq_first;
+
+	writel(1 << bit_off, (unsigned int *)data->reg_clr);
+
+	irq_chip_ack_parent(d);
+}
+
+static void sg2042_msi_irq_compose_msi_msg(struct irq_data *data,
+					   struct msi_msg *msg)
+{
+	struct sg2042_msi_data *priv = irq_data_get_irq_chip_data(data);
+
+	msg->address_hi = upper_32_bits(priv->doorbell_addr);
+	msg->address_lo = lower_32_bits(priv->doorbell_addr);
+	msg->data = 1 << (data->hwirq - priv->irq_first);
+
+	pr_debug("%s hwirq[%d]: address_hi[%#x], address_lo[%#x], data[%#x]\n",
+		 __func__,
+		 (int)data->hwirq, msg->address_hi, msg->address_lo, msg->data);
+}
+
+static struct irq_chip sg2042_msi_middle_irq_chip = {
+	.name			= "SG2042 MSI",
+	.irq_ack		= sg2042_msi_irq_ack,
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+#ifdef CONFIG_SMP
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+#endif
+	.irq_compose_msi_msg	= sg2042_msi_irq_compose_msi_msg,
+};
+
+static int sg2042_msi_parent_domain_alloc(struct irq_domain *domain,
+					  unsigned int virq, int hwirq)
+{
+	struct irq_fwspec fwspec;
+	struct irq_data *d;
+	int ret;
+
+	fwspec.fwnode = domain->parent->fwnode;
+	fwspec.param_count = 2;
+	fwspec.param[0] = hwirq;
+	fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
+
+	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
+	if (ret)
+		return ret;
+
+	d = irq_domain_get_irq_data(domain->parent, virq);
+	return d->chip->irq_set_type(d, IRQ_TYPE_EDGE_RISING);
+}
+
+static int sg2042_msi_middle_domain_alloc(struct irq_domain *domain,
+					  unsigned int virq,
+					  unsigned int nr_irqs, void *args)
+{
+	struct sg2042_msi_data *priv = domain->host_data;
+	int hwirq, err, i;
+
+	hwirq = sg2042_msi_allocate_hwirq(priv, nr_irqs);
+	if (hwirq < 0)
+		return hwirq;
+
+	for (i = 0; i < nr_irqs; i++) {
+		err = sg2042_msi_parent_domain_alloc(domain, virq + i, hwirq + i);
+		if (err)
+			goto err_hwirq;
+
+		pr_debug("%s: virq[%d], hwirq[%d]\n",
+			 __func__, virq + i, (int)hwirq + i);
+
+		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+					      &sg2042_msi_middle_irq_chip, priv);
+	}
+
+	return 0;
+
+err_hwirq:
+	sg2042_msi_free_hwirq(priv, hwirq, nr_irqs);
+	irq_domain_free_irqs_parent(domain, virq, i);
+
+	return err;
+}
+
+static void sg2042_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 sg2042_msi_data *priv = irq_data_get_irq_chip_data(d);
+
+	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
+	sg2042_msi_free_hwirq(priv, d->hwirq, nr_irqs);
+}
+
+static const struct irq_domain_ops pch_msi_middle_domain_ops = {
+	.alloc	= sg2042_msi_middle_domain_alloc,
+	.free	= sg2042_msi_middle_domain_free,
+};
+
+static int sg2042_msi_init_domains(struct sg2042_msi_data *priv,
+				   struct device_node *node)
+{
+	struct irq_domain *plic_domain, *middle_domain;
+	struct device_node *plic_node;
+	struct fwnode_handle *fwnode = of_node_to_fwnode(node);
+
+	if (!of_find_property(node, "interrupt-parent", NULL)) {
+		pr_err("Can't find interrupt-parent!\n");
+		return -EINVAL;
+	}
+
+	plic_node = of_irq_find_parent(node);
+	if (!plic_node) {
+		pr_err("Failed to find the PLIC node!\n");
+		return -ENXIO;
+	}
+
+	plic_domain = irq_find_host(plic_node);
+	of_node_put(plic_node);
+	if (!plic_domain) {
+		pr_err("Failed to find the PLIC domain\n");
+		return -ENXIO;
+	}
+
+	middle_domain = irq_domain_create_hierarchy(plic_domain, 0, priv->num_irqs,
+						    fwnode,
+						    &pch_msi_middle_domain_ops,
+						    priv);
+	if (!middle_domain) {
+		pr_err("Failed to create the MSI middle domain\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int sg2042_msi_probe(struct platform_device *pdev)
+{
+	struct sg2042_msi_data *data;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(struct sg2042_msi_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->reg_clr = devm_platform_ioremap_resource_byname(pdev, "clr");
+	if (IS_ERR(data->reg_clr)) {
+		dev_err(&pdev->dev, "Failed to map clear register\n");
+		return PTR_ERR(data->reg_clr);
+	}
+
+	if (of_property_read_u64(pdev->dev.of_node, "sophgo,msi-doorbell-addr",
+				 &data->doorbell_addr)) {
+		dev_err(&pdev->dev, "Unable to parse MSI doorbell addr\n");
+		return -EINVAL;
+	}
+
+	if (of_property_read_u32(pdev->dev.of_node, "sophgo,msi-base-vec",
+				 &data->irq_first)) {
+		dev_err(&pdev->dev, "Unable to parse MSI vec base\n");
+		return -EINVAL;
+	}
+
+	if (of_property_read_u32(pdev->dev.of_node, "sophgo,msi-num-vecs",
+				 &data->num_irqs)) {
+		dev_err(&pdev->dev, "Unable to parse MSI vec number\n");
+		return -EINVAL;
+	}
+
+	mutex_init(&data->msi_map_lock);
+
+	data->msi_map = bitmap_zalloc(data->num_irqs, GFP_KERNEL);
+	if (!data->msi_map)
+		return -ENOMEM;
+
+	return sg2042_msi_init_domains(data, pdev->dev.of_node);
+}
+
+static const struct of_device_id sg2042_msi_of_match[] = {
+	{ .compatible = "sophgo,sg2042-msi" },
+	{}
+};
+
+static struct platform_driver sg2042_msi_driver = {
+	.driver = {
+		.name = "sg2042-msi",
+		.of_match_table = of_match_ptr(sg2042_msi_of_match),
+	},
+	.probe = sg2042_msi_probe,
+};
+builtin_platform_driver(sg2042_msi_driver);
-- 
2.34.1

[PATCH 3/3] riscv: sophgo: dts: add msi controller for SG2042

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Add msi-controller node to dts for SG2042.

Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
---
 arch/riscv/boot/dts/sophgo/sg2042.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/riscv/boot/dts/sophgo/sg2042.dtsi b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
index e62ac51ac55a..35651cbac764 100644
--- a/arch/riscv/boot/dts/sophgo/sg2042.dtsi
+++ b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
@@ -173,6 +173,19 @@ pllclk: clock-controller@70300100c0 {
 			#clock-cells = <1>;
 		};
 
+		msi: msi-controller@7030010304 {
+			compatible = "sophgo,sg2042-msi";
+			reg = <0x70 0x30010304 0x0 0x4>;
+			reg-names = "clr";
+			interrupt-controller;
+			#interrupt-cells = <1>;
+			msi-controller;
+			sophgo,msi-doorbell-addr = <0x00000070 0x30010300>;
+			sophgo,msi-base-vec = <64>;
+			sophgo,msi-num-vecs = <32>;
+			interrupt-parent = <&intc>;
+		};
+
 		rpgate: clock-controller@7030010368 {
 			compatible = "sophgo,sg2042-rpgate";
 			reg = <0x70 0x30010368 0x0 0x98>;
-- 
2.34.1

Re: [PATCH 1/3] dt-bindings: interrupt-controller: Add Sophgo SG2042 MSI

[Rob Herring]

On Mon, Nov 11, 2024 at 12:01:36PM +0800, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> Add binding for Sophgo SG2042 MSI controller.
> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> ---
>  .../sophgo,sg2042-msi.yaml                    | 78 +++++++++++++++++++
>  1 file changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml b/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
> new file mode 100644
> index 000000000000..9fe99b74c211
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
> @@ -0,0 +1,78 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/sophgo,sg2042-msi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo SG2042 MSI Controller
> +
> +maintainers:
> +  - Chen Wang <unicorn_wang@outlook.com>
> +
> +description:
> +  This interrupt controller is in Sophgo SG2042 for transforming interrupts from
> +  PCIe MSI to PLIC interrupts.
> +
> +allOf:
> +  - $ref: /schemas/interrupts.yaml#
> +  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
> +
> +properties:
> +  compatible:
> +    const: sophgo,sg2042-msi
> +
> +  reg:
> +    items:
> +      - description: clear register
> +
> +  reg-names:
> +    items:
> +      - const: clr
> +
> +  sophgo,msi-doorbell-addr:
> +    description:
> +      u64 value of the MSI doorbell address
> +    $ref: /schemas/types.yaml#/definitions/uint64

Why not use a 'reg' entry?

In any case, this should be a translatable address (i.e. honor 
#address-cells/#size-cells and ranges).

> +
> +  sophgo,msi-base-vec:
> +    description:
> +      u32 value of the base of parent PLIC vector allocated
> +      to MSI.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 64
> +    maximum: 95
> +
> +  sophgo,msi-num-vecs:
> +    description:
> +      u32 value of the number of parent PLIC vectors allocated
> +      to MSI.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 32

Use 'msi-ranges' for these.

> +
> +  msi-controller: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - msi-controller
> +  - sophgo,msi-doorbell-addr
> +  - sophgo,msi-base-vec
> +  - sophgo,msi-num-vecs
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    msi: msi-controller@30000000 {
> +      compatible = "sophgo,sg2042-msi";
> +      reg = <0x30000000 0x4>;
> +      reg-names = "clr";
> +      msi-controller;
> +      sophgo,msi-doorbell-addr = <0x00000070 0x30010300>;
> +      sophgo,msi-base-vec = <64>;
> +      sophgo,msi-num-vecs = <32>;
> +      interrupt-parent = <&plic>;
> +    };
> -- 
> 2.34.1
> 

Re: [PATCH 2/3] irqchip: Add the Sophgo SG2042 MSI interrupt controller

[Thomas Gleixner]

On Mon, Nov 11 2024 at 12:01, Chen Wang wrote:
> +struct sg2042_msi_data {
> +	void __iomem	*reg_clr; /* clear reg, see TRM, 10.1.33, GP_INTR0_CLR */

Please make these tail comments tabular aligned so they actually stand
out.

  https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#comment-style

> +
> +	u64		doorbell_addr; /* see TRM, 10.1.32, GP_INTR0_SET */
> +
> +	u32		irq_first; /* The vector number that MSIs starts */
> +	u32		num_irqs;  /* The number of vectors for MSIs */
> +
> +	unsigned long	*msi_map;
> +	struct mutex	msi_map_lock; /* lock for msi_map */
> +};
> +
> +static int sg2042_msi_allocate_hwirq(struct sg2042_msi_data *priv, int num_req)
> +{
> +	int first;
> +
> +	mutex_lock(&priv->msi_map_lock);

Please use

       guard(mutex)(&priv->msi_map_lock);

which removes all the mutex_unlock() hackery and boils this down

> +
> +	first = bitmap_find_free_region(priv->msi_map, priv->num_irqs,
> +					get_count_order(num_req));
> +	if (first < 0) {
> +		mutex_unlock(&priv->msi_map_lock);
> +		return -ENOSPC;
> +	}
> +
> +	mutex_unlock(&priv->msi_map_lock);
> +
> +	return priv->irq_first + first;

to

	guard(mutex)(&priv->msi_map_lock);
	first = bitmap_find_free_region(priv->msi_map, priv->num_irqs,
					get_count_order(num_req));
	return first >= 0 ? priv->irq_first + first : -ENOSPC;

See?

> +}
> +
> +static void sg2042_msi_free_hwirq(struct sg2042_msi_data *priv,
> +				  int hwirq, int num_req)
> +{
> +	int first = hwirq - priv->irq_first;
> +
> +	mutex_lock(&priv->msi_map_lock);

Ditto.

> +	bitmap_release_region(priv->msi_map, first, get_count_order(num_req));
> +	mutex_unlock(&priv->msi_map_lock);
> +}

> +static void sg2042_msi_irq_compose_msi_msg(struct irq_data *data,
> +					   struct msi_msg *msg)
> +{
> +	struct sg2042_msi_data *priv = irq_data_get_irq_chip_data(data);
> +
> +	msg->address_hi = upper_32_bits(priv->doorbell_addr);
> +	msg->address_lo = lower_32_bits(priv->doorbell_addr);
> +	msg->data = 1 << (data->hwirq - priv->irq_first);
> +
> +	pr_debug("%s hwirq[%d]: address_hi[%#x], address_lo[%#x], data[%#x]\n",
> +		 __func__,

No point in having this line break. You have 100 characters. Please fix
this all over the place.

> +		 (int)data->hwirq, msg->address_hi, msg->address_lo, msg->data);

(int) ? Why can't you use the proper conversion specifier instead of %d?

> +static int sg2042_msi_middle_domain_alloc(struct irq_domain *domain,
> +					  unsigned int virq,
> +					  unsigned int nr_irqs, void *args)
> +{
> +	struct sg2042_msi_data *priv = domain->host_data;
> +	int hwirq, err, i;
> +
> +	hwirq = sg2042_msi_allocate_hwirq(priv, nr_irqs);
> +	if (hwirq < 0)
> +		return hwirq;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		err = sg2042_msi_parent_domain_alloc(domain, virq + i, hwirq + i);
> +		if (err)
> +			goto err_hwirq;
> +
> +		pr_debug("%s: virq[%d], hwirq[%d]\n",
> +			 __func__, virq + i, (int)hwirq + i);

No line break required.

> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +					      &sg2042_msi_middle_irq_chip, priv);
> +	}

> +static int sg2042_msi_init_domains(struct sg2042_msi_data *priv,
> +				   struct device_node *node)
> +{
> +	struct irq_domain *plic_domain, *middle_domain;
> +	struct device_node *plic_node;
> +	struct fwnode_handle *fwnode = of_node_to_fwnode(node);

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations

> +	if (!of_find_property(node, "interrupt-parent", NULL)) {
> +		pr_err("Can't find interrupt-parent!\n");
> +		return -EINVAL;
> +	}
> +
> +	plic_node = of_irq_find_parent(node);
> +	if (!plic_node) {
> +		pr_err("Failed to find the PLIC node!\n");
> +		return -ENXIO;
> +	}
> +
> +	plic_domain = irq_find_host(plic_node);
> +	of_node_put(plic_node);
> +	if (!plic_domain) {
> +		pr_err("Failed to find the PLIC domain\n");
> +		return -ENXIO;
> +	}
> +
> +	middle_domain = irq_domain_create_hierarchy(plic_domain, 0, priv->num_irqs,
> +						    fwnode,
> +						    &pch_msi_middle_domain_ops,
> +						    priv);

So now you have created a domain. How is that supposed to be used by the
PCI layer?

> +	if (!middle_domain) {
> +		pr_err("Failed to create the MSI middle domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}

> +static int sg2042_msi_probe(struct platform_device *pdev)
> +{

....

> +	data->msi_map = bitmap_zalloc(data->num_irqs, GFP_KERNEL);
> +	if (!data->msi_map)
> +		return -ENOMEM;
> +
> +	return sg2042_msi_init_domains(data, pdev->dev.of_node);

In case of error this leaks data->msi_map, no?

> +static struct platform_driver sg2042_msi_driver = {
> +	.driver = {
> +		.name = "sg2042-msi",
> +		.of_match_table = of_match_ptr(sg2042_msi_of_match),
> +	},
> +	.probe = sg2042_msi_probe,
> +};

Please see the documentation I pointed you to above and search for
struct initializers.

Thanks,

        tglx

Re: [PATCH 2/3] irqchip: Add the Sophgo SG2042 MSI interrupt controller

[Chen Wang]

On 2024/11/13 14:14, Thomas Gleixner wrote:
> On Mon, Nov 11 2024 at 12:01, Chen Wang wrote:
>> +struct sg2042_msi_data {
>> +	void __iomem	*reg_clr; /* clear reg, see TRM, 10.1.33, GP_INTR0_CLR */
> Please make these tail comments tabular aligned so they actually stand
> out.
>
>    https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#comment-style
Got, will fix this.
>> +
>> +	u64		doorbell_addr; /* see TRM, 10.1.32, GP_INTR0_SET */
>> +
>> +	u32		irq_first; /* The vector number that MSIs starts */
>> +	u32		num_irqs;  /* The number of vectors for MSIs */
>> +
>> +	unsigned long	*msi_map;
>> +	struct mutex	msi_map_lock; /* lock for msi_map */
>> +};
>> +
>> +static int sg2042_msi_allocate_hwirq(struct sg2042_msi_data *priv, int num_req)
>> +{
>> +	int first;
>> +
>> +	mutex_lock(&priv->msi_map_lock);
> Please use
>
>         guard(mutex)(&priv->msi_map_lock);
>
> which removes all the mutex_unlock() hackery and boils this down
Thanks, will double check.
>
>> +
>> +	first = bitmap_find_free_region(priv->msi_map, priv->num_irqs,
>> +					get_count_order(num_req));
>> +	if (first < 0) {
>> +		mutex_unlock(&priv->msi_map_lock);
>> +		return -ENOSPC;
>> +	}
>> +
>> +	mutex_unlock(&priv->msi_map_lock);
>> +
>> +	return priv->irq_first + first;
> to
>
> 	guard(mutex)(&priv->msi_map_lock);
> 	first = bitmap_find_free_region(priv->msi_map, priv->num_irqs,
> 					get_count_order(num_req));
> 	return first >= 0 ? priv->irq_first + first : -ENOSPC;
>
> See?
>
>> +}
>> +
>> +static void sg2042_msi_free_hwirq(struct sg2042_msi_data *priv,
>> +				  int hwirq, int num_req)
>> +{
>> +	int first = hwirq - priv->irq_first;
>> +
>> +	mutex_lock(&priv->msi_map_lock);
> Ditto.
>
>> +	bitmap_release_region(priv->msi_map, first, get_count_order(num_req));
>> +	mutex_unlock(&priv->msi_map_lock);
>> +}
>> +static void sg2042_msi_irq_compose_msi_msg(struct irq_data *data,
>> +					   struct msi_msg *msg)
>> +{
>> +	struct sg2042_msi_data *priv = irq_data_get_irq_chip_data(data);
>> +
>> +	msg->address_hi = upper_32_bits(priv->doorbell_addr);
>> +	msg->address_lo = lower_32_bits(priv->doorbell_addr);
>> +	msg->data = 1 << (data->hwirq - priv->irq_first);
>> +
>> +	pr_debug("%s hwirq[%d]: address_hi[%#x], address_lo[%#x], data[%#x]\n",
>> +		 __func__,
> No point in having this line break. You have 100 characters. Please fix
> this all over the place.
Got.
>
>> +		 (int)data->hwirq, msg->address_hi, msg->address_lo, msg->data);
> (int) ? Why can't you use the proper conversion specifier instead of %d?
Will double-check.
>
>> +static int sg2042_msi_middle_domain_alloc(struct irq_domain *domain,
>> +					  unsigned int virq,
>> +					  unsigned int nr_irqs, void *args)
>> +{
>> +	struct sg2042_msi_data *priv = domain->host_data;
>> +	int hwirq, err, i;
>> +
>> +	hwirq = sg2042_msi_allocate_hwirq(priv, nr_irqs);
>> +	if (hwirq < 0)
>> +		return hwirq;
>> +
>> +	for (i = 0; i < nr_irqs; i++) {
>> +		err = sg2042_msi_parent_domain_alloc(domain, virq + i, hwirq + i);
>> +		if (err)
>> +			goto err_hwirq;
>> +
>> +		pr_debug("%s: virq[%d], hwirq[%d]\n",
>> +			 __func__, virq + i, (int)hwirq + i);
> No line break required.
>
>> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>> +					      &sg2042_msi_middle_irq_chip, priv);
>> +	}
>> +static int sg2042_msi_init_domains(struct sg2042_msi_data *priv,
>> +				   struct device_node *node)
>> +{
>> +	struct irq_domain *plic_domain, *middle_domain;
>> +	struct device_node *plic_node;
>> +	struct fwnode_handle *fwnode = of_node_to_fwnode(node);
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations
Thanks, will double-check.
>> +	if (!of_find_property(node, "interrupt-parent", NULL)) {
>> +		pr_err("Can't find interrupt-parent!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	plic_node = of_irq_find_parent(node);
>> +	if (!plic_node) {
>> +		pr_err("Failed to find the PLIC node!\n");
>> +		return -ENXIO;
>> +	}
>> +
>> +	plic_domain = irq_find_host(plic_node);
>> +	of_node_put(plic_node);
>> +	if (!plic_domain) {
>> +		pr_err("Failed to find the PLIC domain\n");
>> +		return -ENXIO;
>> +	}
>> +
>> +	middle_domain = irq_domain_create_hierarchy(plic_domain, 0, priv->num_irqs,
>> +						    fwnode,
>> +						    &pch_msi_middle_domain_ops,
>> +						    priv);
> So now you have created a domain. How is that supposed to be used by the
> PCI layer?

Here I create the domain and attached it to the fwnode. In PCI driver, 
it can set this msi controller as its ""interrupt-parent" and find the 
domain attached as below:

static int pcie_probe(struct platform_device *pdev)
{
     struct device *dev = &pdev->dev;
     parent_node = of_irq_find_parent(dev->of_node);
     parent_domain = irq_find_host(parent_node);
     ...
}

>> +	if (!middle_domain) {
>> +		pr_err("Failed to create the MSI middle domain\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	return 0;
>> +}
>> +static int sg2042_msi_probe(struct platform_device *pdev)
>> +{
> ....
>
>> +	data->msi_map = bitmap_zalloc(data->num_irqs, GFP_KERNEL);
>> +	if (!data->msi_map)
>> +		return -ENOMEM;
>> +
>> +	return sg2042_msi_init_domains(data, pdev->dev.of_node);
> In case of error this leaks data->msi_map, no?
Thanks, I will correct this.
>> +static struct platform_driver sg2042_msi_driver = {
>> +	.driver = {
>> +		.name = "sg2042-msi",
>> +		.of_match_table = of_match_ptr(sg2042_msi_of_match),
>> +	},
>> +	.probe = sg2042_msi_probe,
>> +};
> Please see the documentation I pointed you to above and search for
> struct initializers.
>
> Thanks,
>
>          tglx

Re: [PATCH 1/3] dt-bindings: interrupt-controller: Add Sophgo SG2042 MSI

[Chen Wang]

On 2024/11/12 23:52, Rob Herring wrote:
> On Mon, Nov 11, 2024 at 12:01:36PM +0800, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@outlook.com>
>>
>> Add binding for Sophgo SG2042 MSI controller.
>>
>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>> ---
>>   .../sophgo,sg2042-msi.yaml                    | 78 +++++++++++++++++++
>>   1 file changed, 78 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml b/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
>> new file mode 100644
>> index 000000000000..9fe99b74c211
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
>> @@ -0,0 +1,78 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/interrupt-controller/sophgo,sg2042-msi.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Sophgo SG2042 MSI Controller
>> +
>> +maintainers:
>> +  - Chen Wang <unicorn_wang@outlook.com>
>> +
>> +description:
>> +  This interrupt controller is in Sophgo SG2042 for transforming interrupts from
>> +  PCIe MSI to PLIC interrupts.
>> +
>> +allOf:
>> +  - $ref: /schemas/interrupts.yaml#
>> +  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: sophgo,sg2042-msi
>> +
>> +  reg:
>> +    items:
>> +      - description: clear register
>> +
>> +  reg-names:
>> +    items:
>> +      - const: clr
>> +
>> +  sophgo,msi-doorbell-addr:
>> +    description:
>> +      u64 value of the MSI doorbell address
>> +    $ref: /schemas/types.yaml#/definitions/uint64
> Why not use a 'reg' entry?
>
> In any case, this should be a translatable address (i.e. honor
> #address-cells/#size-cells and ranges).
This is just a address to write MSI data, but not a register for driver 
access. So I think it seems better to configure it separately, rather 
than as other registers. This address does not need ioremap.

What do you think? I would like to hear your opinion.

Regards

Chen

>> +
>> +  sophgo,msi-base-vec:
>> +    description:
>> +      u32 value of the base of parent PLIC vector allocated
>> +      to MSI.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 64
>> +    maximum: 95
>> +
>> +  sophgo,msi-num-vecs:
>> +    description:
>> +      u32 value of the number of parent PLIC vectors allocated
>> +      to MSI.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 1
>> +    maximum: 32
> Use 'msi-ranges' for these.
Got, will fix this and thanks.
>> +
>> +  msi-controller: true
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - msi-controller
>> +  - sophgo,msi-doorbell-addr
>> +  - sophgo,msi-base-vec
>> +  - sophgo,msi-num-vecs
>> +
>> +additionalProperties: true
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    msi: msi-controller@30000000 {
>> +      compatible = "sophgo,sg2042-msi";
>> +      reg = <0x30000000 0x4>;
>> +      reg-names = "clr";
>> +      msi-controller;
>> +      sophgo,msi-doorbell-addr = <0x00000070 0x30010300>;
>> +      sophgo,msi-base-vec = <64>;
>> +      sophgo,msi-num-vecs = <32>;
>> +      interrupt-parent = <&plic>;
>> +    };
>> -- 
>> 2.34.1
>>

Re: [PATCH 2/3] irqchip: Add the Sophgo SG2042 MSI interrupt controller

[Thomas Gleixner]

On Wed, Nov 13 2024 at 14:43, Chen Wang wrote:
> On 2024/11/13 14:14, Thomas Gleixner wrote:
>>> +
>>> +	middle_domain = irq_domain_create_hierarchy(plic_domain, 0, priv->num_irqs,
>>> +						    fwnode,
>>> +						    &pch_msi_middle_domain_ops,
>>> +						    priv);
>> So now you have created a domain. How is that supposed to be used by the
>> PCI layer?
>
> Here I create the domain and attached it to the fwnode. In PCI driver, 
> it can set this msi controller as its ""interrupt-parent" and find the 
> domain attached as below:
>
> static int pcie_probe(struct platform_device *pdev)
> {
>      struct device *dev = &pdev->dev;
>      parent_node = of_irq_find_parent(dev->of_node);
>      parent_domain = irq_find_host(parent_node);
>      ...
> }

I assume you then want to create a global PCI/MSI domain via
pci_msi_create_irq_domain(), right?

That's not the preferred way to do that. Any new implementation should
use the MSI parent model, where each PCI device creates it's own per
device MSI domain with the MSI interrupt controller as parent
domain.

There is a library with helper functions, irq-msi-lib.[ch]. See
gicv2m_allocate_domains() or pch_msi_init_domains() for reference.

Thanks

        tglx

Re: [PATCH 2/3] irqchip: Add the Sophgo SG2042 MSI interrupt controller

[Chen Wang]

On 2024/11/13 23:31, Thomas Gleixner wrote:
> On Wed, Nov 13 2024 at 14:43, Chen Wang wrote:
>> On 2024/11/13 14:14, Thomas Gleixner wrote:
>>>> +
>>>> +	middle_domain = irq_domain_create_hierarchy(plic_domain, 0, priv->num_irqs,
>>>> +						    fwnode,
>>>> +						    &pch_msi_middle_domain_ops,
>>>> +						    priv);
>>> So now you have created a domain. How is that supposed to be used by the
>>> PCI layer?
>> Here I create the domain and attached it to the fwnode. In PCI driver,
>> it can set this msi controller as its ""interrupt-parent" and find the
>> domain attached as below:
>>
>> static int pcie_probe(struct platform_device *pdev)
>> {
>>       struct device *dev = &pdev->dev;
>>       parent_node = of_irq_find_parent(dev->of_node);
>>       parent_domain = irq_find_host(parent_node);
>>       ...
>> }
> I assume you then want to create a global PCI/MSI domain via
> pci_msi_create_irq_domain(), right?
Yes, I am writing another pcie driver, which will call 
pci_msi_create_irq_domain() to create a child domain of this middle_domain.
>
> That's not the preferred way to do that. Any new implementation should
> use the MSI parent model, where each PCI device creates it's own per
> device MSI domain with the MSI interrupt controller as parent
> domain.
>
> There is a library with helper functions, irq-msi-lib.[ch]. See
> gicv2m_allocate_domains() or pch_msi_init_domains() for reference.

Thanks, I will check this out.

Regards,

Chen

>
> Thanks
>
>          tglx