[PATCH 0/5] Add PCIe support to Sophgo SG2042 SoC

[PATCH 0/5] Add PCIe support to Sophgo SG2042 SoC

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Sophgo's SG2042 SoC uses Cadence PCIe core to implement RC mode.

SG2042 PCIe controller supports two ways to report MSI:

Method A, the PICe controller implements an MSI interrupt controller
inside, and connect to PLIC upward through one interrupt line. Provides
memory-mapped msi address, and by programming the upper 32 bits of the
address to zero, it can be compatible with old pcie devices that only
support 32-bit msi address.

Method B, the PICe controller connects to PLIC upward through an
independent MSI controller "sophgo,sg2042-msi" on the SOC. The MSI
controller provides multiple(up to 32) interrupt sources to PLIC.
Compared with the first method, the advantage is that the interrupt
source is expanded, but because for SG2042, the msi address provided
by the MSI controller is fixed and only supports 64-bit address(> 2^32),
it is not compatible with old pcie devices that only support 32-bit
msi address.

This patchset depends on another patchset for the SG2042 MSI controller[msi].
If you need to test the DTS part, you need to apply the corresponding
patchset.

Link: https://lore.kernel.org/linux-riscv/cover.1731296803.git.unicorn_wang@outlook.com/ [msi]

Thanks,
Chen

Chen Wang (5):
  dt-bindings: pci: Add Sophgo SG2042 PCIe host
  PCI: sg2042: Add Sophgo SG2042 PCIe driver
  dt-bindings: mfd: syscon: Add sg2042 pcie ctrl compatible
  riscv: sophgo: dts: add pcie controllers for SG2042
  riscv: sophgo: dts: enable pcie for PioneerBox

 .../devicetree/bindings/mfd/syscon.yaml       |   2 +
 .../bindings/pci/sophgo,sg2042-pcie-host.yaml |  88 +++
 .../boot/dts/sophgo/sg2042-milkv-pioneer.dts  |  12 +
 arch/riscv/boot/dts/sophgo/sg2042.dtsi        |  82 +++
 drivers/pci/controller/cadence/Kconfig        |  11 +
 drivers/pci/controller/cadence/Makefile       |   1 +
 drivers/pci/controller/cadence/pcie-sg2042.c  | 611 ++++++++++++++++++
 7 files changed, 807 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
 create mode 100644 drivers/pci/controller/cadence/pcie-sg2042.c


base-commit: 2d5404caa8c7bb5c4e0435f94b28834ae5456623
-- 
2.34.1

[PATCH 1/5] dt-bindings: pci: Add Sophgo SG2042 PCIe host

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Add binding for Sophgo SG2042 PCIe host controller.

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

diff --git a/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml b/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
new file mode 100644
index 000000000000..d4d2232f354f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
@@ -0,0 +1,88 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/sophgo,sg2042-pcie-host.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo SG2042 PCIe Host (Cadence PCIe Wrapper)
+
+description: |+
+  Sophgo SG2042 PCIe host controller is based on the Cadence PCIe core.
+  It shares common features with the PCIe core and inherits common properties
+  defined in Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml.
+
+maintainers:
+  - Chen Wang <unicorn_wang@outlook.com>
+
+properties:
+  compatible:
+    const: sophgo,sg2042-pcie-host
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: reg
+      - const: cfg
+
+  sophgo,syscon-pcie-ctrl:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Phandle to the SYSCON entry
+
+  sophgo,link-id:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Cadence IP link ID.
+
+  sophgo,internal-msi:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: Identifies whether the PCIE node uses internal MSI controller.
+
+  vendor-id:
+    const: 0x1f1c
+
+  device-id:
+    const: 0x2042
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-names:
+    const: msi
+
+allOf:
+  - $ref: cdns-pcie-host.yaml#
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - sophgo,syscon-pcie-ctrl
+  - sophgo,link-id
+  - vendor-id
+  - device-id
+  - ranges
+
+additionalProperties: true
+
+examples:
+  - |
+    pcie@62000000 {
+      compatible = "sophgo,sg2042-pcie-host";
+      device_type = "pci";
+      reg = <0x62000000  0x00800000>,
+            <0x48000000  0x00001000>;
+      reg-names = "reg", "cfg";
+      #address-cells = <3>;
+      #size-cells = <2>;
+      ranges = <0x81000000 0 0x00000000 0xde000000 0 0x00010000>,
+               <0x82000000 0 0xd0400000 0xd0400000 0 0x0d000000>;
+      bus-range = <0x80 0xbf>;
+      vendor-id = <0x1f1c>;
+      device-id = <0x2042>;
+      cdns,no-bar-match-nbits = <48>;
+      sophgo,link-id = <0>;
+      sophgo,syscon-pcie-ctrl = <&cdns_pcie1_ctrl>;
+      sophgo,internal-msi;
+      interrupt-parent = <&intc>;
+    };
-- 
2.34.1

[PATCH 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe driver

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Add support for PCIe controller in SG2042 SoC. The controller
uses the Cadence PCIe core programmed by pcie-cadence*.c. The
PCIe controller will work in host mode only.

Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
---
 drivers/pci/controller/cadence/Kconfig       |  11 +
 drivers/pci/controller/cadence/Makefile      |   1 +
 drivers/pci/controller/cadence/pcie-sg2042.c | 611 +++++++++++++++++++
 3 files changed, 623 insertions(+)
 create mode 100644 drivers/pci/controller/cadence/pcie-sg2042.c

diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
index 8a0044bb3989..45a16215ea94 100644
--- a/drivers/pci/controller/cadence/Kconfig
+++ b/drivers/pci/controller/cadence/Kconfig
@@ -67,4 +67,15 @@ config PCI_J721E_EP
 	  Say Y here if you want to support the TI J721E PCIe platform
 	  controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe
 	  core.
+
+config PCIE_SG2042
+	bool "Sophgo SG2042 PCIe controller (host mode)"
+	depends on ARCH_SOPHGO || COMPILE_TEST
+	depends on OF
+	select PCIE_CADENCE_HOST
+	help
+	  Say Y here if you want to support the Sophgo SG2042 PCIe platform
+	  controller in host mode. Sophgo SG2042 PCIe controller uses Cadence
+	  PCIe core.
+
 endmenu
diff --git a/drivers/pci/controller/cadence/Makefile b/drivers/pci/controller/cadence/Makefile
index 9bac5fb2f13d..89aa316f54ac 100644
--- a/drivers/pci/controller/cadence/Makefile
+++ b/drivers/pci/controller/cadence/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_PCIE_CADENCE_HOST) += pcie-cadence-host.o
 obj-$(CONFIG_PCIE_CADENCE_EP) += pcie-cadence-ep.o
 obj-$(CONFIG_PCIE_CADENCE_PLAT) += pcie-cadence-plat.o
 obj-$(CONFIG_PCI_J721E) += pci-j721e.o
+obj-$(CONFIG_PCIE_SG2042) += pcie-sg2042.o
\ No newline at end of file
diff --git a/drivers/pci/controller/cadence/pcie-sg2042.c b/drivers/pci/controller/cadence/pcie-sg2042.c
new file mode 100644
index 000000000000..809edb8e7259
--- /dev/null
+++ b/drivers/pci/controller/cadence/pcie-sg2042.c
@@ -0,0 +1,611 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * pcie-sg2042 - PCIe controller driver for Sophgo SG2042 SoC
+ *
+ * Copyright (C) 2024 Sophgo Technology Inc.
+ * Copyright (C) 2024 Chen Wang <unicorn_wang@outlook.com>
+ */
+
+#include <linux/bits.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/msi.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#include "pcie-cadence.h"
+
+/*
+ * SG2042 PCIe controller supports two ways to report MSI:
+ * - Method A, the PICe controller implements an MSI interrupt controller inside,
+ *   and connect to PLIC upward through one interrupt line. Provides
+ *   memory-mapped msi address, and by programming the upper 32 bits of the
+ *   address to zero, it can be compatible with old pcie devices that only
+ *   support 32-bit msi address.
+ * - Method B, the PICe controller connects to PLIC upward through an
+ *   independent MSI controller "sophgo,sg2042-msi" on the SOC. The MSI
+ *   controller provides multiple(up to 32) interrupt sources to PLIC.
+ *   Compared with the first method, the advantage is that the interrupt source
+ *   is expanded, but because for SG2042, the msi address provided by the MSI
+ *   controller is fixed and only supports 64-bit address(> 2^32), it is not
+ *   compatible with old pcie devices that only support 32-bit msi address.
+ * Method A & B can be configured in DTS with property "sophgo,internal-msi",
+ * default is Method B.
+ */
+
+#define MAX_MSI_IRQS		8
+#define MAX_MSI_IRQS_PER_CTRL	1
+#define MAX_MSI_CTRLS		(MAX_MSI_IRQS / MAX_MSI_IRQS_PER_CTRL)
+#define MSI_DEF_NUM_VECTORS	MAX_MSI_IRQS
+#define BYTE_NUM_PER_MSI_VEC	4
+
+#define REG_CLEAR		0x0804
+#define REG_STATUS		0x0810
+#define REG_LINK0_MSI_ADDR_SIZE	0x085C
+#define REG_LINK1_MSI_ADDR_SIZE	0x080C
+#define REG_LINK0_MSI_ADDR_LOW	0x0860
+#define REG_LINK0_MSI_ADDR_HIGH	0x0864
+#define REG_LINK1_MSI_ADDR_LOW	0x0868
+#define REG_LINK1_MSI_ADDR_HIGH	0x086C
+
+#define REG_CLEAR_LINK0_BIT	2
+#define REG_CLEAR_LINK1_BIT	3
+#define REG_STATUS_LINK0_BIT	2
+#define REG_STATUS_LINK1_BIT	3
+
+#define REG_LINK0_MSI_ADDR_SIZE_MASK	GENMASK(15, 0)
+#define REG_LINK1_MSI_ADDR_SIZE_MASK	GENMASK(31, 16)
+
+#define SG2042_CDNS_PLAT_CPU_TO_BUS_ADDR	0xCFFFFFFFFF
+
+struct sg2042_pcie {
+	struct cdns_pcie *cdns_pcie;
+
+	struct regmap *syscon;
+
+	u32 link_id;
+	u32 internal_msi; /* Flag if use internal MSI controller, default external */
+
+	struct irq_domain *msi_domain;
+
+	int msi_irq;
+
+	dma_addr_t msi_phys;
+	void *msi_virt;
+
+	u32 num_applied_vecs; /* number of applied vectors, used to speed up ISR */
+
+	raw_spinlock_t lock;
+	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
+};
+
+static void sg2042_pcie_msi_irq_mask_external(struct irq_data *d)
+{
+	pci_msi_mask_irq(d);
+	irq_chip_mask_parent(d);
+}
+
+static void sg2042_pcie_msi_irq_unmask_external(struct irq_data *d)
+{
+	pci_msi_unmask_irq(d);
+	irq_chip_unmask_parent(d);
+}
+
+static struct irq_chip sg2042_pcie_msi_chip_external = {
+	.name		= "SG2042 PCIe MSI External",
+	.irq_ack	= irq_chip_ack_parent,
+	.irq_mask	= sg2042_pcie_msi_irq_mask_external,
+	.irq_unmask	= sg2042_pcie_msi_irq_unmask_external,
+};
+
+static struct msi_domain_info sg2042_pcie_msi_domain_info_external = {
+	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
+	.chip	= &sg2042_pcie_msi_chip_external,
+};
+
+static struct irq_chip sg2042_pcie_msi_chip = {
+	.name = "SG2042 PCIe MSI",
+	.irq_ack = irq_chip_ack_parent,
+};
+
+static struct msi_domain_info sg2042_pcie_msi_domain_info = {
+	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
+	.chip	= &sg2042_pcie_msi_chip,
+};
+
+static void sg2042_pcie_msi_clear_status(struct sg2042_pcie *pcie)
+{
+	u32 status, clr_msi_in_bit;
+
+	if (pcie->link_id == 1)
+		clr_msi_in_bit = BIT(REG_CLEAR_LINK1_BIT);
+	else
+		clr_msi_in_bit = BIT(REG_CLEAR_LINK0_BIT);
+
+	regmap_read(pcie->syscon, REG_CLEAR, &status);
+	status |= clr_msi_in_bit;
+	regmap_write(pcie->syscon, REG_CLEAR, status);
+
+	/* need write 0 to reset, hardware can not reset automatically */
+	status &= ~clr_msi_in_bit;
+	regmap_write(pcie->syscon, REG_CLEAR, status);
+}
+
+static int sg2042_pcie_msi_irq_set_affinity(struct irq_data *d,
+					    const struct cpumask *mask,
+					    bool force)
+{
+	if (d->parent_data)
+		return irq_chip_set_affinity_parent(d, mask, force);
+
+	return -EINVAL;
+}
+
+static void sg2042_pcie_msi_irq_compose_msi_msg(struct irq_data *d,
+						struct msi_msg *msg)
+{
+	struct sg2042_pcie *pcie = irq_data_get_irq_chip_data(d);
+	struct device *dev = pcie->cdns_pcie->dev;
+
+	msg->address_lo = lower_32_bits(pcie->msi_phys) + BYTE_NUM_PER_MSI_VEC * d->hwirq;
+	msg->address_hi = upper_32_bits(pcie->msi_phys);
+	msg->data = 1;
+
+	pcie->num_applied_vecs = d->hwirq;
+
+	dev_info(dev, "compose msi msg hwirq[%d] address_hi[%#x] address_lo[%#x]\n",
+		 (int)d->hwirq, msg->address_hi, msg->address_lo);
+}
+
+static void sg2042_pcie_msi_irq_ack(struct irq_data *d)
+{
+	struct sg2042_pcie *pcie = irq_data_get_irq_chip_data(d);
+
+	sg2042_pcie_msi_clear_status(pcie);
+}
+
+static struct irq_chip sg2042_pcie_msi_bottom_chip = {
+	.name			= "SG2042 PCIe PLIC-MSI translator",
+	.irq_ack		= sg2042_pcie_msi_irq_ack,
+	.irq_compose_msi_msg	= sg2042_pcie_msi_irq_compose_msi_msg,
+#ifdef CONFIG_SMP
+	.irq_set_affinity	= sg2042_pcie_msi_irq_set_affinity,
+#endif
+};
+
+static int sg2042_pcie_irq_domain_alloc(struct irq_domain *domain,
+					unsigned int virq, unsigned int nr_irqs,
+					void *args)
+{
+	struct sg2042_pcie *pcie = domain->host_data;
+	unsigned long flags;
+	u32 i;
+	int bit;
+
+	raw_spin_lock_irqsave(&pcie->lock, flags);
+
+	bit = bitmap_find_free_region(pcie->msi_irq_in_use, MSI_DEF_NUM_VECTORS,
+				      order_base_2(nr_irqs));
+
+	raw_spin_unlock_irqrestore(&pcie->lock, flags);
+
+	if (bit < 0)
+		return -ENOSPC;
+
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_info(domain, virq + i, bit + i,
+				    &sg2042_pcie_msi_bottom_chip,
+				    pcie, handle_edge_irq,
+				    NULL, NULL);
+
+	return 0;
+}
+
+static void sg2042_pcie_irq_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_pcie *pcie = irq_data_get_irq_chip_data(d);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&pcie->lock, flags);
+
+	bitmap_release_region(pcie->msi_irq_in_use, d->hwirq,
+			      order_base_2(nr_irqs));
+
+	raw_spin_unlock_irqrestore(&pcie->lock, flags);
+}
+
+static const struct irq_domain_ops sg2042_pcie_msi_domain_ops = {
+	.alloc	= sg2042_pcie_irq_domain_alloc,
+	.free	= sg2042_pcie_irq_domain_free,
+};
+
+/*
+ * We use the usual two domain structure, the top one being a generic PCI/MSI
+ * domain, the bottom one being SG2042-specific and handling the actual HW
+ * interrupt allocation.
+ * At the same time, for internal MSI controller(Method A), bottom chip uses a
+ * chained handler to handle the controller's MSI IRQ edge triggered.
+ */
+static int sg2042_pcie_create_msi_domain(struct sg2042_pcie *pcie,
+					 struct irq_domain *parent)
+{
+	struct device *dev = pcie->cdns_pcie->dev;
+	struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node);
+
+	if (pcie->internal_msi)
+		pcie->msi_domain = pci_msi_create_irq_domain(fwnode,
+							     &sg2042_pcie_msi_domain_info,
+							     parent);
+
+	else
+		pcie->msi_domain = pci_msi_create_irq_domain(fwnode,
+							     &sg2042_pcie_msi_domain_info_external,
+							     parent);
+
+	if (!pcie->msi_domain) {
+		dev_err(dev, "Failed to create MSI domain\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int sg2042_pcie_setup_msi_external(struct sg2042_pcie *pcie)
+{
+	struct device *dev = pcie->cdns_pcie->dev;
+	struct device_node *np = dev->of_node;
+	struct irq_domain *parent_domain;
+	struct device_node *parent_np;
+
+	if (!of_find_property(np, "interrupt-parent", NULL)) {
+		dev_err(dev, "Can't find interrupt-parent!\n");
+		return -EINVAL;
+	}
+
+	parent_np = of_irq_find_parent(np);
+	if (!parent_np) {
+		dev_err(dev, "Can't find node of interrupt-parent!\n");
+		return -ENXIO;
+	}
+
+	parent_domain = irq_find_host(parent_np);
+	of_node_put(parent_np);
+	if (!parent_domain) {
+		dev_err(dev, "Can't find domain of interrupt-parent!\n");
+		return -ENXIO;
+	}
+
+	return sg2042_pcie_create_msi_domain(pcie, parent_domain);
+}
+
+static int sg2042_pcie_init_msi_data(struct sg2042_pcie *pcie)
+{
+	struct device *dev = pcie->cdns_pcie->dev;
+	u32 value;
+	int ret;
+
+	raw_spin_lock_init(&pcie->lock);
+
+	/*
+	 * Though the PCIe controller can address >32-bit address space, to
+	 * facilitate endpoints that support only 32-bit MSI target address,
+	 * the mask is set to 32-bit to make sure that MSI target address is
+	 * always a 32-bit address
+	 */
+	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
+	if (ret < 0)
+		return ret;
+
+	pcie->msi_virt = dma_alloc_coherent(dev, BYTE_NUM_PER_MSI_VEC * MAX_MSI_IRQS,
+					    &pcie->msi_phys, GFP_KERNEL);
+	if (!pcie->msi_virt)
+		return -ENOMEM;
+
+	/* Program the msi address and size */
+	if (pcie->link_id == 1) {
+		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_LOW,
+			     lower_32_bits(pcie->msi_phys));
+		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_HIGH,
+			     upper_32_bits(pcie->msi_phys));
+
+		regmap_read(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, &value);
+		value = (value & REG_LINK1_MSI_ADDR_SIZE_MASK) | MAX_MSI_IRQS;
+		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, value);
+	} else {
+		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_LOW,
+			     lower_32_bits(pcie->msi_phys));
+		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_HIGH,
+			     upper_32_bits(pcie->msi_phys));
+
+		regmap_read(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, &value);
+		value = (value & REG_LINK0_MSI_ADDR_SIZE_MASK) | (MAX_MSI_IRQS << 16);
+		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, value);
+	}
+
+	return 0;
+}
+
+static irqreturn_t sg2042_pcie_msi_handle_irq(struct sg2042_pcie *pcie)
+{
+	u32 i, pos;
+	unsigned long val;
+	u32 status, num_vectors;
+	irqreturn_t ret = IRQ_NONE;
+
+	num_vectors = pcie->num_applied_vecs;
+	for (i = 0; i <= num_vectors; i++) {
+		status = readl((void *)(pcie->msi_virt + i * BYTE_NUM_PER_MSI_VEC));
+		if (!status)
+			continue;
+
+		ret = IRQ_HANDLED;
+		val = status;
+		pos = 0;
+		while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL,
+					    pos)) != MAX_MSI_IRQS_PER_CTRL) {
+			generic_handle_domain_irq(pcie->msi_domain->parent,
+						  (i * MAX_MSI_IRQS_PER_CTRL) +
+						  pos);
+			pos++;
+		}
+		writel(0, ((void *)(pcie->msi_virt) + i * BYTE_NUM_PER_MSI_VEC));
+	}
+	return ret;
+}
+
+static void sg2042_pcie_msi_chained_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u32 status, st_msi_in_bit;
+	struct sg2042_pcie *pcie;
+
+	chained_irq_enter(chip, desc);
+
+	pcie = irq_desc_get_handler_data(desc);
+	if (pcie->link_id == 1)
+		st_msi_in_bit = REG_STATUS_LINK1_BIT;
+	else
+		st_msi_in_bit = REG_STATUS_LINK0_BIT;
+
+	regmap_read(pcie->syscon, REG_STATUS, &status);
+	if ((status >> st_msi_in_bit) & 0x1) {
+		sg2042_pcie_msi_clear_status(pcie);
+
+		sg2042_pcie_msi_handle_irq(pcie);
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie, struct platform_device *pdev)
+{
+	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;
+
+	parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS,
+						 &sg2042_pcie_msi_domain_ops, pcie);
+	if (!parent_domain) {
+		dev_err(dev, "Failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+	irq_domain_update_bus_token(parent_domain, DOMAIN_BUS_NEXUS);
+
+	ret = sg2042_pcie_create_msi_domain(pcie, parent_domain);
+	if (ret) {
+		irq_domain_remove(parent_domain);
+		return ret;
+	}
+
+	ret = sg2042_pcie_init_msi_data(pcie);
+	if (ret) {
+		dev_err(dev, "Failed to initialize msi data!\n");
+		return ret;
+	}
+
+	ret = platform_get_irq_byname(pdev, "msi");
+	if (ret <= 0) {
+		dev_err(dev, "failed to get MSI irq\n");
+		return ret;
+	}
+	pcie->msi_irq = ret;
+
+	irq_set_chained_handler_and_data(pcie->msi_irq,
+					 sg2042_pcie_msi_chained_isr, pcie);
+
+	return 0;
+}
+
+static void sg2042_pcie_free_msi(struct sg2042_pcie *pcie)
+{
+	struct device *dev = pcie->cdns_pcie->dev;
+
+	if (pcie->msi_irq)
+		irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL);
+
+	if (pcie->msi_virt)
+		dma_free_coherent(dev, BYTE_NUM_PER_MSI_VEC * MAX_MSI_IRQS,
+				  pcie->msi_virt, pcie->msi_phys);
+}
+
+static u64 sg2042_cdns_pcie_cpu_addr_fixup(struct cdns_pcie *pcie, u64 cpu_addr)
+{
+	return cpu_addr & SG2042_CDNS_PLAT_CPU_TO_BUS_ADDR;
+}
+
+static const struct cdns_pcie_ops sg2042_cdns_pcie_ops = {
+	.cpu_addr_fixup = sg2042_cdns_pcie_cpu_addr_fixup,
+};
+
+/*
+ * SG2042 only support 4-byte aligned access, so for the rootbus (i.e. to read
+ * the PCIe controller itself, read32 is required. For non-rootbus (i.e. to read
+ * the PCIe peripheral registers, supports 1/2/4 byte aligned access, so
+ * directly use read should be fine.
+ * The same is true for write.
+ */
+static int sg2042_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
+				   int where, int size, u32 *value)
+{
+	if (pci_is_root_bus(bus))
+		return pci_generic_config_read32(bus, devfn, where, size,
+						 value);
+
+	return pci_generic_config_read(bus, devfn, where, size, value);
+}
+
+static int sg2042_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
+				    int where, int size, u32 value)
+{
+	if (pci_is_root_bus(bus))
+		return pci_generic_config_write32(bus, devfn, where, size,
+						  value);
+
+	return pci_generic_config_write(bus, devfn, where, size, value);
+}
+
+static struct pci_ops sg2042_pcie_host_ops = {
+	.map_bus	= cdns_pci_map_bus,
+	.read		= sg2042_pcie_config_read,
+	.write		= sg2042_pcie_config_write,
+};
+
+static int sg2042_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct pci_host_bridge *bridge;
+	struct device_node *np_syscon;
+	struct cdns_pcie *cdns_pcie;
+	struct sg2042_pcie *pcie;
+	struct cdns_pcie_rc *rc;
+	struct regmap *syscon;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST))
+		return -ENODEV;
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
+	if (!bridge) {
+		dev_err(dev, "Failed to alloc host bridge!\n");
+		return -ENOMEM;
+	}
+	bridge->ops = &sg2042_pcie_host_ops;
+
+	rc = pci_host_bridge_priv(bridge);
+	cdns_pcie = &rc->pcie;
+	cdns_pcie->dev = dev;
+	cdns_pcie->ops = &sg2042_cdns_pcie_ops;
+	pcie->cdns_pcie = cdns_pcie;
+
+	np_syscon = of_parse_phandle(np, "sophgo,syscon-pcie-ctrl", 0);
+	if (!np_syscon) {
+		dev_err(dev, "Failed to get syscon node\n");
+		return -ENOMEM;
+	}
+	syscon = syscon_node_to_regmap(np_syscon);
+	if (IS_ERR(syscon)) {
+		dev_err(dev, "Failed to get regmap for syscon\n");
+		return -ENOMEM;
+	}
+	pcie->syscon = syscon;
+
+	if (of_property_read_u32(np, "sophgo,link-id", &pcie->link_id)) {
+		dev_err(dev, "Unable to parse link ID\n");
+		return -EINVAL;
+	}
+
+	pcie->internal_msi = 0;
+	if (of_property_read_bool(np, "sophgo,internal-msi"))
+		pcie->internal_msi = 1;
+
+	platform_set_drvdata(pdev, pcie);
+
+	pm_runtime_enable(dev);
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "pm_runtime_get_sync failed\n");
+		goto err_get_sync;
+	}
+
+	if (pcie->internal_msi) {
+		ret = sg2042_pcie_setup_msi(pcie, pdev);
+		if (ret < 0)
+			goto err_setup_msi;
+	} else {
+		ret = sg2042_pcie_setup_msi_external(pcie);
+		if (ret < 0)
+			goto err_setup_msi;
+	}
+
+	ret = cdns_pcie_init_phy(dev, cdns_pcie);
+	if (ret) {
+		dev_err(dev, "Failed to init phy!\n");
+		goto err_setup_msi;
+	}
+
+	ret = cdns_pcie_host_setup(rc);
+	if (ret < 0) {
+		dev_err(dev, "Failed to setup host!\n");
+		goto err_host_setup;
+	}
+
+	return 0;
+
+err_host_setup:
+	cdns_pcie_disable_phy(cdns_pcie);
+
+err_setup_msi:
+	sg2042_pcie_free_msi(pcie);
+
+err_get_sync:
+	pm_runtime_put(dev);
+	pm_runtime_disable(dev);
+
+	return ret;
+}
+
+static void sg2042_pcie_shutdown(struct platform_device *pdev)
+{
+	struct sg2042_pcie *pcie = platform_get_drvdata(pdev);
+	struct cdns_pcie *cdns_pcie = pcie->cdns_pcie;
+	struct device *dev = &pdev->dev;
+
+	sg2042_pcie_free_msi(pcie);
+
+	cdns_pcie_disable_phy(cdns_pcie);
+
+	pm_runtime_put(dev);
+	pm_runtime_disable(dev);
+}
+
+static const struct of_device_id sg2042_pcie_of_match[] = {
+	{ .compatible = "sophgo,sg2042-pcie-host" },
+	{},
+};
+
+static struct platform_driver sg2042_pcie_driver = {
+	.driver = {
+		.name		= "sg2042-pcie",
+		.of_match_table	= sg2042_pcie_of_match,
+		.pm		= &cdns_pcie_pm_ops,
+	},
+	.probe		= sg2042_pcie_probe,
+	.shutdown	= sg2042_pcie_shutdown,
+};
+builtin_platform_driver(sg2042_pcie_driver);
-- 
2.34.1

[PATCH 3/5] dt-bindings: mfd: syscon: Add sg2042 pcie ctrl compatible

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Document SOPHGO SG2042 compatible for PCIe control registers.
These registers are shared by pcie controller nodes.

Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
---
 Documentation/devicetree/bindings/mfd/syscon.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
index cc9b17ad69f2..55f919690001 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.yaml
+++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
@@ -106,6 +106,7 @@ select:
           - rockchip,rk3576-qos
           - rockchip,rk3588-qos
           - rockchip,rv1126-qos
+          - sophgo,sg2042-pcie-ctrl
           - st,spear1340-misc
           - stericsson,nomadik-pmu
           - starfive,jh7100-sysmain
@@ -203,6 +204,7 @@ properties:
           - rockchip,rk3576-qos
           - rockchip,rk3588-qos
           - rockchip,rv1126-qos
+          - sophgo,sg2042-pcie-ctrl
           - st,spear1340-misc
           - stericsson,nomadik-pmu
           - starfive,jh7100-sysmain
-- 
2.34.1

[PATCH 4/5] riscv: sophgo: dts: add pcie controllers for SG2042

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Add PCIe controller nodes in DTS for Sophgo SG2042.
Default they are disabled.

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

diff --git a/arch/riscv/boot/dts/sophgo/sg2042.dtsi b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
index e62ac51ac55a..dca51fa9381b 100644
--- a/arch/riscv/boot/dts/sophgo/sg2042.dtsi
+++ b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
@@ -195,6 +195,88 @@ clkgen: clock-controller@7030012000 {
 			#clock-cells = <1>;
 		};
 
+		pcie_rc0: pcie@7060000000 {
+			compatible = "sophgo,sg2042-pcie-host";
+			device_type = "pci";
+			reg = <0x70 0x60000000  0x0 0x02000000>,
+			      <0x40 0x00000000  0x0 0x00001000>;
+			reg-names = "reg", "cfg";
+			#address-cells = <3>;
+			#size-cells = <2>;
+			ranges = <0x01000000 0x0  0xc0000000  0x40 0xc0000000  0x0 0x00400000>,
+				 <0x42000000 0x0  0xd0000000  0x40 0xd0000000  0x0 0x10000000>,
+				 <0x02000000 0x0  0xe0000000  0x40 0xe0000000  0x0 0x20000000>,
+				 <0x43000000 0x42 0x00000000  0x42 0x00000000  0x2 0x00000000>,
+				 <0x03000000 0x41 0x00000000  0x41 0x00000000  0x1 0x00000000>;
+			bus-range = <0x0 0x3f>;
+			vendor-id = <0x1f1c>;
+			device-id = <0x2042>;
+			cdns,no-bar-match-nbits = <48>;
+			sophgo,link-id = <0>;
+			sophgo,syscon-pcie-ctrl = <&cdns_pcie0_ctrl>;
+			interrupt-parent = <&msi>;
+			status = "disabled";
+		};
+
+		cdns_pcie0_ctrl: syscon@7061800000 {
+			compatible = "sophgo,sg2042-pcie-ctrl", "syscon";
+			reg = <0x70 0x61800000 0x0 0x800000>;
+		};
+
+		pcie_rc1: pcie@7062000000 {
+			compatible = "sophgo,sg2042-pcie-host";
+			device_type = "pci";
+			reg = <0x70 0x62000000  0x0 0x00800000>,
+			      <0x48 0x00000000  0x0 0x00001000>;
+			reg-names = "reg", "cfg";
+			#address-cells = <3>;
+			#size-cells = <2>;
+			ranges = <0x01000000 0x0  0xc0800000  0x48 0xc0800000  0x0 0x00400000>,
+				 <0x42000000 0x0  0xd0000000  0x48 0xd0000000  0x0 0x10000000>,
+				 <0x02000000 0x0  0xe0000000  0x48 0xe0000000  0x0 0x20000000>,
+				 <0x03000000 0x49 0x00000000  0x49 0x00000000  0x1 0x00000000>,
+				 <0x43000000 0x4a 0x00000000  0x4a 0x00000000  0x2 0x00000000>;
+			bus-range = <0x80 0xbf>;
+			vendor-id = <0x1f1c>;
+			device-id = <0x2042>;
+			cdns,no-bar-match-nbits = <48>;
+			sophgo,link-id = <0>;
+			sophgo,syscon-pcie-ctrl = <&cdns_pcie1_ctrl>;
+			sophgo,internal-msi;
+			interrupt-parent = <&intc>;
+			interrupts = <123 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi";
+			status = "disabled";
+		};
+
+		pcie_rc2: pcie@7062800000 {
+			compatible = "sophgo,sg2042-pcie-host";
+			device_type = "pci";
+			reg = <0x70 0x62800000  0x0 0x00800000>,
+			      <0x4c 0x00000000  0x0 0x00001000>;
+			reg-names = "reg", "cfg";
+			#address-cells = <3>;
+			#size-cells = <2>;
+			ranges = <0x01000000 0x0  0xc0c00000  0x4c 0xc0c00000  0x0 0x00400000>,
+				 <0x42000000 0x0  0xf8000000  0x4c 0xf8000000  0x0 0x04000000>,
+				 <0x02000000 0x0  0xfc000000  0x4c 0xfc000000  0x0 0x04000000>,
+				 <0x43000000 0x4e 0x00000000  0x4e 0x00000000  0x2 0x00000000>,
+				 <0x03000000 0x4d 0x00000000  0x4d 0x00000000  0x1 0x00000000>;
+			bus-range = <0xc0 0xff>;
+			vendor-id = <0x1f1c>;
+			device-id = <0x2042>;
+			cdns,no-bar-match-nbits = <48>;
+			sophgo,link-id = <1>;
+			sophgo,syscon-pcie-ctrl = <&cdns_pcie1_ctrl>;
+			interrupt-parent = <&msi>;
+			status = "disabled";
+		};
+
+		cdns_pcie1_ctrl: syscon@7063800000 {
+			compatible = "sophgo,sg2042-pcie-ctrl", "syscon";
+			reg = <0x70 0x63800000 0x0 0x800000>;
+		};
+
 		clint_mswi: interrupt-controller@7094000000 {
 			compatible = "sophgo,sg2042-aclint-mswi", "thead,c900-aclint-mswi";
 			reg = <0x00000070 0x94000000 0x00000000 0x00004000>;
-- 
2.34.1

[PATCH 5/5] riscv: sophgo: dts: enable pcie for PioneerBox

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Enable pcie controllers for PioneerBox, which uses SG2042 SoC.

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

diff --git a/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts b/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
index a3f9d6f22566..746b9c3d254c 100644
--- a/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
+++ b/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
@@ -45,6 +45,18 @@ mcu: syscon@17 {
 	};
 };
 
+&pcie_rc0 {
+	status = "okay";
+};
+
+&pcie_rc1 {
+	status = "okay";
+};
+
+&pcie_rc2 {
+	status = "okay";
+};
+
 &sd {
 	bus-width = <4>;
 	no-sdio;
-- 
2.34.1

Re: [PATCH 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe driver

[kernel test robot]

Hi Chen,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 2d5404caa8c7bb5c4e0435f94b28834ae5456623]

url:    https://github.com/intel-lab-lkp/linux/commits/Chen-Wang/dt-bindings-pci-Add-Sophgo-SG2042-PCIe-host/20241111-140237
base:   2d5404caa8c7bb5c4e0435f94b28834ae5456623
patch link:    https://lore.kernel.org/r/5051f2375ff6218e7d44ce0c298efd5f9ee56964.1731303328.git.unicorn_wang%40outlook.com
patch subject: [PATCH 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe driver
config: um-allyesconfig (https://download.01.org/0day-ci/archive/20241111/202411111935.kqORCWuE-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241111/202411111935.kqORCWuE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411111935.kqORCWuE-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/pci/controller/cadence/pcie-sg2042.c:141:12: warning: 'sg2042_pcie_msi_irq_set_affinity' defined but not used [-Wunused-function]
     141 | static int sg2042_pcie_msi_irq_set_affinity(struct irq_data *d,
         |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/sg2042_pcie_msi_irq_set_affinity +141 drivers/pci/controller/cadence/pcie-sg2042.c

   140	
 > 141	static int sg2042_pcie_msi_irq_set_affinity(struct irq_data *d,
   142						    const struct cpumask *mask,
   143						    bool force)
   144	{
   145		if (d->parent_data)
   146			return irq_chip_set_affinity_parent(d, mask, force);
   147	
   148		return -EINVAL;
   149	}
   150	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/5] dt-bindings: pci: Add Sophgo SG2042 PCIe host

[Rob Herring]

On Mon, Nov 11, 2024 at 01:59:37PM +0800, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> Add binding for Sophgo SG2042 PCIe host controller.
> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> ---
>  .../bindings/pci/sophgo,sg2042-pcie-host.yaml | 88 +++++++++++++++++++
>  1 file changed, 88 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml b/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
> new file mode 100644
> index 000000000000..d4d2232f354f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
> @@ -0,0 +1,88 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/sophgo,sg2042-pcie-host.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo SG2042 PCIe Host (Cadence PCIe Wrapper)
> +
> +description: |+

Don't need '|+'

> +  Sophgo SG2042 PCIe host controller is based on the Cadence PCIe core.

> +  It shares common features with the PCIe core and inherits common properties
> +  defined in Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml.

That's clear from the $ref. No need to say that in prose.

> +
> +maintainers:
> +  - Chen Wang <unicorn_wang@outlook.com>
> +
> +properties:
> +  compatible:
> +    const: sophgo,sg2042-pcie-host
> +
> +  reg:
> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: reg
> +      - const: cfg
> +
> +  sophgo,syscon-pcie-ctrl:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Phandle to the SYSCON entry

Please describe what you need to access.

> +
> +  sophgo,link-id:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Cadence IP link ID.

Is this an index or related to the syscon? Nak for the former, use 
linux,pci-domain. For the latter, add an arg to sophgo,syscon-pcie-ctrl.

> +
> +  sophgo,internal-msi:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: Identifies whether the PCIE node uses internal MSI controller.

Wouldn't 'msi-parent' work for this purpose?

> +
> +  vendor-id:
> +    const: 0x1f1c
> +
> +  device-id:
> +    const: 0x2042
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-names:
> +    const: msi
> +
> +allOf:
> +  - $ref: cdns-pcie-host.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - sophgo,syscon-pcie-ctrl
> +  - sophgo,link-id
> +  - vendor-id
> +  - device-id
> +  - ranges

ranges is already required in the common schemas.

> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    pcie@62000000 {
> +      compatible = "sophgo,sg2042-pcie-host";
> +      device_type = "pci";
> +      reg = <0x62000000  0x00800000>,
> +            <0x48000000  0x00001000>;
> +      reg-names = "reg", "cfg";
> +      #address-cells = <3>;
> +      #size-cells = <2>;
> +      ranges = <0x81000000 0 0x00000000 0xde000000 0 0x00010000>,
> +               <0x82000000 0 0xd0400000 0xd0400000 0 0x0d000000>;
> +      bus-range = <0x80 0xbf>;
> +      vendor-id = <0x1f1c>;
> +      device-id = <0x2042>;
> +      cdns,no-bar-match-nbits = <48>;
> +      sophgo,link-id = <0>;
> +      sophgo,syscon-pcie-ctrl = <&cdns_pcie1_ctrl>;
> +      sophgo,internal-msi;
> +      interrupt-parent = <&intc>;
> +    };
> -- 
> 2.34.1
> 

Re: [PATCH 3/5] dt-bindings: mfd: syscon: Add sg2042 pcie ctrl compatible

[Rob Herring (Arm)]

On Mon, 11 Nov 2024 14:00:15 +0800, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> Document SOPHGO SG2042 compatible for PCIe control registers.
> These registers are shared by pcie controller nodes.
> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> ---
>  Documentation/devicetree/bindings/mfd/syscon.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe driver

[Bjorn Helgaas]

On Mon, Nov 11, 2024 at 01:59:56PM +0800, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> Add support for PCIe controller in SG2042 SoC. The controller
> uses the Cadence PCIe core programmed by pcie-cadence*.c. The
> PCIe controller will work in host mode only.

> +++ b/drivers/pci/controller/cadence/Kconfig
> @@ -67,4 +67,15 @@ config PCI_J721E_EP
>  	  Say Y here if you want to support the TI J721E PCIe platform
>  	  controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe
>  	  core.
> +
> +config PCIE_SG2042
> +	bool "Sophgo SG2042 PCIe controller (host mode)"
> +	depends on ARCH_SOPHGO || COMPILE_TEST
> +	depends on OF
> +	select PCIE_CADENCE_HOST
> +	help
> +	  Say Y here if you want to support the Sophgo SG2042 PCIe platform
> +	  controller in host mode. Sophgo SG2042 PCIe controller uses Cadence
> +	  PCIe core.

Reorder to keep these menu items in alphabetical order by vendor.

> +++ b/drivers/pci/controller/cadence/pcie-sg2042.c

> + * SG2042 PCIe controller supports two ways to report MSI:
> + * - Method A, the PICe controller implements an MSI interrupt controller inside,
> + *   and connect to PLIC upward through one interrupt line. Provides
> + *   memory-mapped msi address, and by programming the upper 32 bits of the
> + *   address to zero, it can be compatible with old pcie devices that only
> + *   support 32-bit msi address.
> + * - Method B, the PICe controller connects to PLIC upward through an
> + *   independent MSI controller "sophgo,sg2042-msi" on the SOC. The MSI
> + *   controller provides multiple(up to 32) interrupt sources to PLIC.
> + *   Compared with the first method, the advantage is that the interrupt source
> + *   is expanded, but because for SG2042, the msi address provided by the MSI
> + *   controller is fixed and only supports 64-bit address(> 2^32), it is not
> + *   compatible with old pcie devices that only support 32-bit msi address.
> + * Method A & B can be configured in DTS with property "sophgo,internal-msi",
> + * default is Method B.

s/PICe/PCIe/ (multiple)
s/msi/MSI/ (multiple)
s/pcie/PCIe/ (multiple)

Wrap comment (and code below) to fit in 80 columns.  Add blank lines
between paragraphs.

> +#define SG2042_CDNS_PLAT_CPU_TO_BUS_ADDR	0xCFFFFFFFFF

Remove (see below).

> +static void sg2042_pcie_msi_irq_compose_msi_msg(struct irq_data *d,
> +						struct msi_msg *msg)
> +{
> +	struct sg2042_pcie *pcie = irq_data_get_irq_chip_data(d);
> +	struct device *dev = pcie->cdns_pcie->dev;
> +
> +	msg->address_lo = lower_32_bits(pcie->msi_phys) + BYTE_NUM_PER_MSI_VEC * d->hwirq;
> +	msg->address_hi = upper_32_bits(pcie->msi_phys);
> +	msg->data = 1;
> +
> +	pcie->num_applied_vecs = d->hwirq;

This looks questionable.  How do you know d->hwirq increases every
time this is called?

> +	dev_info(dev, "compose msi msg hwirq[%d] address_hi[%#x] address_lo[%#x]\n",
> +		 (int)d->hwirq, msg->address_hi, msg->address_lo);

This seems too verbose to be a dev_info().  Maybe a dev_dbg() or
remove it altogether.

> + * We use the usual two domain structure, the top one being a generic PCI/MSI
> + * domain, the bottom one being SG2042-specific and handling the actual HW
> + * interrupt allocation.
> + * At the same time, for internal MSI controller(Method A), bottom chip uses a
> + * chained handler to handle the controller's MSI IRQ edge triggered.

Add blank line between paragraphs.

> +static int sg2042_pcie_setup_msi_external(struct sg2042_pcie *pcie)
> +{
> +	struct device *dev = pcie->cdns_pcie->dev;
> +	struct device_node *np = dev->of_node;
> +	struct irq_domain *parent_domain;
> +	struct device_node *parent_np;
> +
> +	if (!of_find_property(np, "interrupt-parent", NULL)) {
> +		dev_err(dev, "Can't find interrupt-parent!\n");
> +		return -EINVAL;
> +	}
> +
> +	parent_np = of_irq_find_parent(np);
> +	if (!parent_np) {
> +		dev_err(dev, "Can't find node of interrupt-parent!\n");

Can you use some kind of %pOF format to include more information here?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/core-api/printk-formats.rst?id=v6.11#n463

> +		return -ENXIO;
> +	}
> +
> +	parent_domain = irq_find_host(parent_np);
> +	of_node_put(parent_np);
> +	if (!parent_domain) {
> +		dev_err(dev, "Can't find domain of interrupt-parent!\n");

And here?

> +		return -ENXIO;
> +	}
> +
> +	return sg2042_pcie_create_msi_domain(pcie, parent_domain);
> +}

> +static int sg2042_pcie_init_msi_data(struct sg2042_pcie *pcie)
> +{
> +	struct device *dev = pcie->cdns_pcie->dev;
> +	u32 value;
> +	int ret;
> +
> +	raw_spin_lock_init(&pcie->lock);
> +
> +	/*
> +	 * Though the PCIe controller can address >32-bit address space, to
> +	 * facilitate endpoints that support only 32-bit MSI target address,
> +	 * the mask is set to 32-bit to make sure that MSI target address is
> +	 * always a 32-bit address
> +	 */
> +	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
> +	if (ret < 0)
> +		return ret;
> +
> +	pcie->msi_virt = dma_alloc_coherent(dev, BYTE_NUM_PER_MSI_VEC * MAX_MSI_IRQS,
> +					    &pcie->msi_phys, GFP_KERNEL);
> +	if (!pcie->msi_virt)
> +		return -ENOMEM;
> +
> +	/* Program the msi address and size */

s/msi/MSI/

> +	if (pcie->link_id == 1) {
> +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_LOW,
> +			     lower_32_bits(pcie->msi_phys));
> +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_HIGH,
> +			     upper_32_bits(pcie->msi_phys));
> +
> +		regmap_read(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, &value);
> +		value = (value & REG_LINK1_MSI_ADDR_SIZE_MASK) | MAX_MSI_IRQS;
> +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, value);
> +	} else {
> +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_LOW,
> +			     lower_32_bits(pcie->msi_phys));
> +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_HIGH,
> +			     upper_32_bits(pcie->msi_phys));
> +
> +		regmap_read(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, &value);
> +		value = (value & REG_LINK0_MSI_ADDR_SIZE_MASK) | (MAX_MSI_IRQS << 16);
> +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, value);
> +	}

Lot of pcie->link_id checking going on here.  Consider saving these
offsets in the struct sg2042_pcie so you don't need to test
everywhere.

> +	return 0;
> +}

> +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie, struct platform_device *pdev)
> +{
> +	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;
> +
> +	parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS,
> +						 &sg2042_pcie_msi_domain_ops, pcie);
> +	if (!parent_domain) {
> +		dev_err(dev, "Failed to create IRQ domain\n");
> +		return -ENOMEM;
> +	}
> +	irq_domain_update_bus_token(parent_domain, DOMAIN_BUS_NEXUS);
> +
> +	ret = sg2042_pcie_create_msi_domain(pcie, parent_domain);
> +	if (ret) {
> +		irq_domain_remove(parent_domain);
> +		return ret;
> +	}
> +
> +	ret = sg2042_pcie_init_msi_data(pcie);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize msi data!\n");

s/msi/MSI/

> +		return ret;
> +	}
> +
> +	ret = platform_get_irq_byname(pdev, "msi");
> +	if (ret <= 0) {
> +		dev_err(dev, "failed to get MSI irq\n");
> +		return ret;
> +	}
> +	pcie->msi_irq = ret;
> +
> +	irq_set_chained_handler_and_data(pcie->msi_irq,
> +					 sg2042_pcie_msi_chained_isr, pcie);
> +
> +	return 0;
> +}
> +
> +static void sg2042_pcie_free_msi(struct sg2042_pcie *pcie)
> +{
> +	struct device *dev = pcie->cdns_pcie->dev;
> +
> +	if (pcie->msi_irq)
> +		irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL);
> +
> +	if (pcie->msi_virt)
> +		dma_free_coherent(dev, BYTE_NUM_PER_MSI_VEC * MAX_MSI_IRQS,
> +				  pcie->msi_virt, pcie->msi_phys);
> +}
> +
> +static u64 sg2042_cdns_pcie_cpu_addr_fixup(struct cdns_pcie *pcie, u64 cpu_addr)
> +{
> +	return cpu_addr & SG2042_CDNS_PLAT_CPU_TO_BUS_ADDR;
> +}

Remove.  This translation between CPU and PCI bus addresses should be
described in the DT "ranges" property of the PCI host bridge.

See
https://lore.kernel.org/linux-pci/20241029-pci_fixup_addr-v7-3-8310dc24fb7c@nxp.com/
for a similar fix.

> +static const struct cdns_pcie_ops sg2042_cdns_pcie_ops = {
> +	.cpu_addr_fixup = sg2042_cdns_pcie_cpu_addr_fixup,
> +};
> +
> +/*
> + * SG2042 only support 4-byte aligned access, so for the rootbus (i.e. to read
> + * the PCIe controller itself, read32 is required. For non-rootbus (i.e. to read
> + * the PCIe peripheral registers, supports 1/2/4 byte aligned access, so
> + * directly use read should be fine.
> + * The same is true for write.

Add blank line between paragraphs or rewrap into a single paragraph.

> +static int sg2042_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct pci_host_bridge *bridge;
> +	struct device_node *np_syscon;
> +	struct cdns_pcie *cdns_pcie;
> +	struct sg2042_pcie *pcie;
> +	struct cdns_pcie_rc *rc;
> +	struct regmap *syscon;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST))
> +		return -ENODEV;
> +
> +	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> +	if (!pcie)
> +		return -ENOMEM;
> +
> +	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
> +	if (!bridge) {
> +		dev_err(dev, "Failed to alloc host bridge!\n");
> +		return -ENOMEM;
> +	}

Add blank line.

> +	bridge->ops = &sg2042_pcie_host_ops;
> +
> +	rc = pci_host_bridge_priv(bridge);
> +	cdns_pcie = &rc->pcie;
> +	cdns_pcie->dev = dev;
> +	cdns_pcie->ops = &sg2042_cdns_pcie_ops;
> +	pcie->cdns_pcie = cdns_pcie;
> +
> +	np_syscon = of_parse_phandle(np, "sophgo,syscon-pcie-ctrl", 0);
> +	if (!np_syscon) {
> +		dev_err(dev, "Failed to get syscon node\n");
> +		return -ENOMEM;
> +	}
> +	syscon = syscon_node_to_regmap(np_syscon);
> +	if (IS_ERR(syscon)) {
> +		dev_err(dev, "Failed to get regmap for syscon\n");
> +		return -ENOMEM;
> +	}
> +	pcie->syscon = syscon;
> +
> +	if (of_property_read_u32(np, "sophgo,link-id", &pcie->link_id)) {
> +		dev_err(dev, "Unable to parse link ID\n");
> +		return -EINVAL;
> +	}
> +
> +	pcie->internal_msi = 0;
> +	if (of_property_read_bool(np, "sophgo,internal-msi"))
> +		pcie->internal_msi = 1;
> +
> +	platform_set_drvdata(pdev, pcie);
> +
> +	pm_runtime_enable(dev);
> +
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_get_sync failed\n");
> +		goto err_get_sync;
> +	}
> +
> +	if (pcie->internal_msi) {
> +		ret = sg2042_pcie_setup_msi(pcie, pdev);
> +		if (ret < 0)
> +			goto err_setup_msi;
> +	} else {
> +		ret = sg2042_pcie_setup_msi_external(pcie);
> +		if (ret < 0)
> +			goto err_setup_msi;
> +	}
> +
> +	ret = cdns_pcie_init_phy(dev, cdns_pcie);
> +	if (ret) {
> +		dev_err(dev, "Failed to init phy!\n");
> +		goto err_setup_msi;
> +	}
> +
> +	ret = cdns_pcie_host_setup(rc);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to setup host!\n");
> +		goto err_host_setup;
> +	}
> +
> +	return 0;
> +
> +err_host_setup:
> +	cdns_pcie_disable_phy(cdns_pcie);
> +
> +err_setup_msi:
> +	sg2042_pcie_free_msi(pcie);
> +
> +err_get_sync:
> +	pm_runtime_put(dev);
> +	pm_runtime_disable(dev);
> +
> +	return ret;
> +}
> +
> +static void sg2042_pcie_shutdown(struct platform_device *pdev)
> +{
> +	struct sg2042_pcie *pcie = platform_get_drvdata(pdev);
> +	struct cdns_pcie *cdns_pcie = pcie->cdns_pcie;
> +	struct device *dev = &pdev->dev;
> +
> +	sg2042_pcie_free_msi(pcie);
> +
> +	cdns_pcie_disable_phy(cdns_pcie);
> +
> +	pm_runtime_put(dev);
> +	pm_runtime_disable(dev);
> +}
> +
> +static const struct of_device_id sg2042_pcie_of_match[] = {
> +	{ .compatible = "sophgo,sg2042-pcie-host" },
> +	{},
> +};
> +
> +static struct platform_driver sg2042_pcie_driver = {
> +	.driver = {
> +		.name		= "sg2042-pcie",
> +		.of_match_table	= sg2042_pcie_of_match,
> +		.pm		= &cdns_pcie_pm_ops,
> +	},
> +	.probe		= sg2042_pcie_probe,
> +	.shutdown	= sg2042_pcie_shutdown,
> +};
> +builtin_platform_driver(sg2042_pcie_driver);
> -- 
> 2.34.1
> 

Re: [PATCH 1/5] dt-bindings: pci: Add Sophgo SG2042 PCIe host

[Chen Wang]

On 2024/11/12 23:59, Rob Herring wrote:
> On Mon, Nov 11, 2024 at 01:59:37PM +0800, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@outlook.com>
>>
>> Add binding for Sophgo SG2042 PCIe host controller.
>>
>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>> ---
>>   .../bindings/pci/sophgo,sg2042-pcie-host.yaml | 88 +++++++++++++++++++
>>   1 file changed, 88 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml b/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
>> new file mode 100644
>> index 000000000000..d4d2232f354f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
>> @@ -0,0 +1,88 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pci/sophgo,sg2042-pcie-host.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Sophgo SG2042 PCIe Host (Cadence PCIe Wrapper)
>> +
>> +description: |+
> Don't need '|+'
Got, thanks.
>
>> +  Sophgo SG2042 PCIe host controller is based on the Cadence PCIe core.
>> +  It shares common features with the PCIe core and inherits common properties
>> +  defined in Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml.
> That's clear from the $ref. No need to say that in prose.
Got, thanks.
>
>> +
>> +maintainers:
>> +  - Chen Wang <unicorn_wang@outlook.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: sophgo,sg2042-pcie-host
>> +
>> +  reg:
>> +    maxItems: 2
>> +
>> +  reg-names:
>> +    items:
>> +      - const: reg
>> +      - const: cfg
>> +
>> +  sophgo,syscon-pcie-ctrl:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: Phandle to the SYSCON entry
> Please describe what you need to access.
>
>> +
>> +  sophgo,link-id:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: Cadence IP link ID.
> Is this an index or related to the syscon? Nak for the former, use
> linux,pci-domain. For the latter, add an arg to sophgo,syscon-pcie-ctrl.
Let me give you some background info.

SG2042 uses Cadence IP, every IP is composed of 2 cores(called link0 & 
link1 as Cadence's term). The Cadence IP has two modes of operation, 
selected by a strap pin.

In the single-link mode, the Cadence PCIe core instance associated with 
Link0 is connected to all the lanes and the Cadence PCIe core instance 
associated with Link1 is inactive.

In the dual-link mode, the Cadence PCIe core instance associated with 
Link0 is connected to the lower half of the lanes and the Cadence PCIe 
core instance associated with Link1 is connected to the upper half of 
the lanes.

SG2042 contains 2 Cadence IPs and configures the Cores as below:

```
                +-- Core(Link0) <---> pcie_rc0   +-----------------+
Cadence IP 1 --+                                | cdns_pcie0_ctrl |
                +-- Core(Link1) <---> disabled   +-----------------+
                +-- Core(Link0) <---> pcie_rc1   +-----------------+
Cadence IP 2 --+                                | cdns_pcie1_ctrl |
                +-- Core(Link1) <---> pcie_rc2   +-----------------+
```


pcie_rcX is pcie node ("sophgo,sg2042-pcie-host") defined in DTS.
cdns_pcie0_ctrl is syscon node ("sophgo,sg2042-pcie-ctrl") defined in DTS

cdns_pcieX_ctrl contains some registers shared by pcie_rcX, even two 
RC(Link)s may share different bits of the same register. For 
example，cdns_pcie1_ctrl contains registers shared by link0 & link1 for 
Cadence IP 2.

So we defined "sophgo,link-id" to flag which core(link) the rc maps to, 
with this we can know what registers(bits) we should use.

That's why I don't use "linux,pci-domain" and also it's not proper to 
define it as arg to "sophgo,syscon-pcie-ctrl".


>> +
>> +  sophgo,internal-msi:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description: Identifies whether the PCIE node uses internal MSI controller.
> Wouldn't 'msi-parent' work for this purpose?
I will check it out, thanks.
>
>> +
>> +  vendor-id:
>> +    const: 0x1f1c
>> +
>> +  device-id:
>> +    const: 0x2042
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  interrupt-names:
>> +    const: msi
>> +
>> +allOf:
>> +  - $ref: cdns-pcie-host.yaml#
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - sophgo,syscon-pcie-ctrl
>> +  - sophgo,link-id
>> +  - vendor-id
>> +  - device-id
>> +  - ranges
> ranges is already required in the common schemas.
Got.
>> +
>> +additionalProperties: true
>> +
>> +examples:
>> +  - |
>> +    pcie@62000000 {
>> +      compatible = "sophgo,sg2042-pcie-host";
>> +      device_type = "pci";
>> +      reg = <0x62000000  0x00800000>,
>> +            <0x48000000  0x00001000>;
>> +      reg-names = "reg", "cfg";
>> +      #address-cells = <3>;
>> +      #size-cells = <2>;
>> +      ranges = <0x81000000 0 0x00000000 0xde000000 0 0x00010000>,
>> +               <0x82000000 0 0xd0400000 0xd0400000 0 0x0d000000>;
>> +      bus-range = <0x80 0xbf>;
>> +      vendor-id = <0x1f1c>;
>> +      device-id = <0x2042>;
>> +      cdns,no-bar-match-nbits = <48>;
>> +      sophgo,link-id = <0>;
>> +      sophgo,syscon-pcie-ctrl = <&cdns_pcie1_ctrl>;
>> +      sophgo,internal-msi;
>> +      interrupt-parent = <&intc>;
>> +    };
>> -- 
>> 2.34.1
>>

Re: [PATCH 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe driver

[Chen Wang]

Hello~

On 2024/11/13 5:20, Bjorn Helgaas wrote:
> On Mon, Nov 11, 2024 at 01:59:56PM +0800, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@outlook.com>
>>
>> Add support for PCIe controller in SG2042 SoC. The controller
>> uses the Cadence PCIe core programmed by pcie-cadence*.c. The
>> PCIe controller will work in host mode only.
>> +++ b/drivers/pci/controller/cadence/Kconfig
>> @@ -67,4 +67,15 @@ config PCI_J721E_EP
>>   	  Say Y here if you want to support the TI J721E PCIe platform
>>   	  controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe
>>   	  core.
>> +
>> +config PCIE_SG2042
>> +	bool "Sophgo SG2042 PCIe controller (host mode)"
>> +	depends on ARCH_SOPHGO || COMPILE_TEST
>> +	depends on OF
>> +	select PCIE_CADENCE_HOST
>> +	help
>> +	  Say Y here if you want to support the Sophgo SG2042 PCIe platform
>> +	  controller in host mode. Sophgo SG2042 PCIe controller uses Cadence
>> +	  PCIe core.
> Reorder to keep these menu items in alphabetical order by vendor.

Sorry, I don't understand your question. I think the menu items in this 
Kconfig file are already sorted alphabetically.

>> +++ b/drivers/pci/controller/cadence/pcie-sg2042.c
>> + * SG2042 PCIe controller supports two ways to report MSI:
>> + * - Method A, the PICe controller implements an MSI interrupt controller inside,
>> + *   and connect to PLIC upward through one interrupt line. Provides
>> + *   memory-mapped msi address, and by programming the upper 32 bits of the
>> + *   address to zero, it can be compatible with old pcie devices that only
>> + *   support 32-bit msi address.
>> + * - Method B, the PICe controller connects to PLIC upward through an
>> + *   independent MSI controller "sophgo,sg2042-msi" on the SOC. The MSI
>> + *   controller provides multiple(up to 32) interrupt sources to PLIC.
>> + *   Compared with the first method, the advantage is that the interrupt source
>> + *   is expanded, but because for SG2042, the msi address provided by the MSI
>> + *   controller is fixed and only supports 64-bit address(> 2^32), it is not
>> + *   compatible with old pcie devices that only support 32-bit msi address.
>> + * Method A & B can be configured in DTS with property "sophgo,internal-msi",
>> + * default is Method B.
> s/PICe/PCIe/ (multiple)
> s/msi/MSI/ (multiple)
> s/pcie/PCIe/ (multiple)
>
> Wrap comment (and code below) to fit in 80 columns.  Add blank lines
> between paragraphs.
Got, will change.
>
>> +#define SG2042_CDNS_PLAT_CPU_TO_BUS_ADDR	0xCFFFFFFFFF
> Remove (see below).
Yes, after some testing, seems this address fixup is not need, will 
remove it.
>> +static void sg2042_pcie_msi_irq_compose_msi_msg(struct irq_data *d,
>> +						struct msi_msg *msg)
>> +{
>> +	struct sg2042_pcie *pcie = irq_data_get_irq_chip_data(d);
>> +	struct device *dev = pcie->cdns_pcie->dev;
>> +
>> +	msg->address_lo = lower_32_bits(pcie->msi_phys) + BYTE_NUM_PER_MSI_VEC * d->hwirq;
>> +	msg->address_hi = upper_32_bits(pcie->msi_phys);
>> +	msg->data = 1;
>> +
>> +	pcie->num_applied_vecs = d->hwirq;
> This looks questionable.  How do you know d->hwirq increases every
> time this is called?
Thanks, it's a bug, I will fix it.
>> +	dev_info(dev, "compose msi msg hwirq[%d] address_hi[%#x] address_lo[%#x]\n",
>> +		 (int)d->hwirq, msg->address_hi, msg->address_lo);
> This seems too verbose to be a dev_info().  Maybe a dev_dbg() or
> remove it altogether.
Will change it to dev_dbg.
>> + * We use the usual two domain structure, the top one being a generic PCI/MSI
>> + * domain, the bottom one being SG2042-specific and handling the actual HW
>> + * interrupt allocation.
>> + * At the same time, for internal MSI controller(Method A), bottom chip uses a
>> + * chained handler to handle the controller's MSI IRQ edge triggered.
> Add blank line between paragraphs.
OK.
>
>> +static int sg2042_pcie_setup_msi_external(struct sg2042_pcie *pcie)
>> +{
>> +	struct device *dev = pcie->cdns_pcie->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct irq_domain *parent_domain;
>> +	struct device_node *parent_np;
>> +
>> +	if (!of_find_property(np, "interrupt-parent", NULL)) {
>> +		dev_err(dev, "Can't find interrupt-parent!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	parent_np = of_irq_find_parent(np);
>> +	if (!parent_np) {
>> +		dev_err(dev, "Can't find node of interrupt-parent!\n");
> Can you use some kind of %pOF format to include more information here?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/core-api/printk-formats.rst?id=v6.11#n463

Thanks, will double check this.

[......]

>> +	if (pcie->link_id == 1) {
>> +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_LOW,
>> +			     lower_32_bits(pcie->msi_phys));
>> +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_HIGH,
>> +			     upper_32_bits(pcie->msi_phys));
>> +
>> +		regmap_read(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, &value);
>> +		value = (value & REG_LINK1_MSI_ADDR_SIZE_MASK) | MAX_MSI_IRQS;
>> +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, value);
>> +	} else {
>> +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_LOW,
>> +			     lower_32_bits(pcie->msi_phys));
>> +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_HIGH,
>> +			     upper_32_bits(pcie->msi_phys));
>> +
>> +		regmap_read(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, &value);
>> +		value = (value & REG_LINK0_MSI_ADDR_SIZE_MASK) | (MAX_MSI_IRQS << 16);
>> +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, value);
>> +	}
> Lot of pcie->link_id checking going on here.  Consider saving these
> offsets in the struct sg2042_pcie so you don't need to test
> everywhere.

Actually, there are not many places in the code to check link_id. If to 
add storage information in struct sg2042_pcie, at least four  u32 are 
needed. And this logic will only be called one time in the probe. So I 
think it is better to keep the current method. What do you think?

[......]

Re: [PATCH 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe driver

[Bjorn Helgaas]

On Fri, Nov 29, 2024 at 05:51:39PM +0800, Chen Wang wrote:
> On 2024/11/13 5:20, Bjorn Helgaas wrote:
> > On Mon, Nov 11, 2024 at 01:59:56PM +0800, Chen Wang wrote:
> > > From: Chen Wang <unicorn_wang@outlook.com>
> > > 
> > > Add support for PCIe controller in SG2042 SoC. The controller
> > > uses the Cadence PCIe core programmed by pcie-cadence*.c. The
> > > PCIe controller will work in host mode only.
> > > +++ b/drivers/pci/controller/cadence/Kconfig
> > > @@ -67,4 +67,15 @@ config PCI_J721E_EP
> > >   	  Say Y here if you want to support the TI J721E PCIe platform
> > >   	  controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe
> > >   	  core.
> > > +
> > > +config PCIE_SG2042
> > > +	bool "Sophgo SG2042 PCIe controller (host mode)"
> > > +	depends on ARCH_SOPHGO || COMPILE_TEST
> > > +	depends on OF
> > > +	select PCIE_CADENCE_HOST
> > > +	help
> > > +	  Say Y here if you want to support the Sophgo SG2042 PCIe platform
> > > +	  controller in host mode. Sophgo SG2042 PCIe controller uses Cadence
> > > +	  PCIe core.
> >
> > Reorder to keep these menu items in alphabetical order by vendor.
> 
> Sorry, I don't understand your question. I think the menu items in this
> Kconfig file are already sorted alphabetically.

Here's what menuconfig looks like after this patch:

  [*] Cadence platform PCIe controller (host mode)
  [*] Cadence platform PCIe controller (endpoint mode)
  [*] TI J721E PCIe controller (host mode)
  [*] TI J721E PCIe controller (endpoint mode)
  [ ] Sophgo SG2042 PCIe controller (host mode) (NEW)

"Sophgo" sorts *before* "TI".

> > > +	if (pcie->link_id == 1) {
> > > +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_LOW,
> > > +			     lower_32_bits(pcie->msi_phys));
> > > +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_HIGH,
> > > +			     upper_32_bits(pcie->msi_phys));
> > > +
> > > +		regmap_read(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, &value);
> > > +		value = (value & REG_LINK1_MSI_ADDR_SIZE_MASK) | MAX_MSI_IRQS;
> > > +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, value);
> > > +	} else {
> > > +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_LOW,
> > > +			     lower_32_bits(pcie->msi_phys));
> > > +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_HIGH,
> > > +			     upper_32_bits(pcie->msi_phys));
> > > +
> > > +		regmap_read(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, &value);
> > > +		value = (value & REG_LINK0_MSI_ADDR_SIZE_MASK) | (MAX_MSI_IRQS << 16);
> > > +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, value);
> > > +	}
> >
> > Lot of pcie->link_id checking going on here.  Consider saving these
> > offsets in the struct sg2042_pcie so you don't need to test
> > everywhere.
> 
> Actually, there are not many places in the code to check link_id. If to add
> storage information in struct sg2042_pcie, at least four  u32 are needed.
> And this logic will only be called one time in the probe. So I think it is
> better to keep the current method. What do you think?

1) I don't think "link_id" is a very good name since it appears to
refer to one of two PCIe Root Ports.  mvebu uses "marvell,pcie-port"
which looks like a similar usage, although unnecessarily
Marvell-specific.  And arch/arm/boot/dts/marvell/armada-370-db.dts has
separate stanzas for two Root Ports, without needing a property to
distinguish them, which would be even better.  I'm not sure why
arch/arm/boot/dts/marvell/armada-370.dtsi needs "marvell,pcie-port"
even though it also has separate stanzas.

2) I think checking pcie->link_id is likely to be harder to maintain
in the future, e.g., if a new chip adds more Root Ports, you'll have
to touch each use.

Bjorn

Re: [PATCH 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe driver

[Chen Wang]

On 2024/11/30 3:50, Bjorn Helgaas wrote:
> On Fri, Nov 29, 2024 at 05:51:39PM +0800, Chen Wang wrote:
>> On 2024/11/13 5:20, Bjorn Helgaas wrote:
>>> On Mon, Nov 11, 2024 at 01:59:56PM +0800, Chen Wang wrote:
>>>> From: Chen Wang <unicorn_wang@outlook.com>
>>>>
>>>> Add support for PCIe controller in SG2042 SoC. The controller
>>>> uses the Cadence PCIe core programmed by pcie-cadence*.c. The
>>>> PCIe controller will work in host mode only.
>>>> +++ b/drivers/pci/controller/cadence/Kconfig
>>>> @@ -67,4 +67,15 @@ config PCI_J721E_EP
>>>>    	  Say Y here if you want to support the TI J721E PCIe platform
>>>>    	  controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe
>>>>    	  core.
>>>> +
>>>> +config PCIE_SG2042
>>>> +	bool "Sophgo SG2042 PCIe controller (host mode)"
>>>> +	depends on ARCH_SOPHGO || COMPILE_TEST
>>>> +	depends on OF
>>>> +	select PCIE_CADENCE_HOST
>>>> +	help
>>>> +	  Say Y here if you want to support the Sophgo SG2042 PCIe platform
>>>> +	  controller in host mode. Sophgo SG2042 PCIe controller uses Cadence
>>>> +	  PCIe core.
>>> Reorder to keep these menu items in alphabetical order by vendor.
>> Sorry, I don't understand your question. I think the menu items in this
>> Kconfig file are already sorted alphabetically.
> Here's what menuconfig looks like after this patch:
>
>    [*] Cadence platform PCIe controller (host mode)
>    [*] Cadence platform PCIe controller (endpoint mode)
>    [*] TI J721E PCIe controller (host mode)
>    [*] TI J721E PCIe controller (endpoint mode)
>    [ ] Sophgo SG2042 PCIe controller (host mode) (NEW)
>
> "Sophgo" sorts *before* "TI".

I see what you mean. I thought we just had to make sure the item IDs in 
the Kconfig file were in alphabetical order.

I will make changes in the next version based on your suggestions.

>>>> +	if (pcie->link_id == 1) {
>>>> +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_LOW,
>>>> +			     lower_32_bits(pcie->msi_phys));
>>>> +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_HIGH,
>>>> +			     upper_32_bits(pcie->msi_phys));
>>>> +
>>>> +		regmap_read(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, &value);
>>>> +		value = (value & REG_LINK1_MSI_ADDR_SIZE_MASK) | MAX_MSI_IRQS;
>>>> +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, value);
>>>> +	} else {
>>>> +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_LOW,
>>>> +			     lower_32_bits(pcie->msi_phys));
>>>> +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_HIGH,
>>>> +			     upper_32_bits(pcie->msi_phys));
>>>> +
>>>> +		regmap_read(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, &value);
>>>> +		value = (value & REG_LINK0_MSI_ADDR_SIZE_MASK) | (MAX_MSI_IRQS << 16);
>>>> +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, value);
>>>> +	}
>>> Lot of pcie->link_id checking going on here.  Consider saving these
>>> offsets in the struct sg2042_pcie so you don't need to test
>>> everywhere.
>> Actually, there are not many places in the code to check link_id. If to add
>> storage information in struct sg2042_pcie, at least four  u32 are needed.
>> And this logic will only be called one time in the probe. So I think it is
>> better to keep the current method. What do you think?
> 1) I don't think "link_id" is a very good name since it appears to
> refer to one of two PCIe Root Ports.  mvebu uses "marvell,pcie-port"
> which looks like a similar usage, although unnecessarily
> Marvell-specific.  And arch/arm/boot/dts/marvell/armada-370-db.dts has
> separate stanzas for two Root Ports, without needing a property to
> distinguish them, which would be even better.  I'm not sure why
> arch/arm/boot/dts/marvell/armada-370.dtsi needs "marvell,pcie-port"
> even though it also has separate stanzas.
>
> 2) I think checking pcie->link_id is likely to be harder to maintain
> in the future, e.g., if a new chip adds more Root Ports, you'll have
> to touch each use.
>
> Bjorn

Thanks for your input. I looked at the Marvell solution you recommended. 
It seems that it is a relatively complex IP design to support mvebu, 
which can flexibly support the export of 1 to 4 variable root ports.
The Cadence IP used by SG2042 is much simpler, and it is fixed to export 
up to two root ports. So I plan to implement it in a simple way. 
Changing "link_id" to "pcie-port" will look better. The writing of link 
actually refers to the terminology in the Cadence manual, which is 
actually the concept of port.

By the way, there is no demand for scalability at present, because it is 
said that Sophgo currently only uses Cadence IP for the SG2042 SoC, and 
subsequent products should switch to solutions from other suppliers.

Any question please let me know.

Regards,

Chen

[PATCH 1/5] dt-bindings: pci: Add Sophgo SG2042 PCIe host

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Add binding for Sophgo SG2042 PCIe host controller.

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

diff --git a/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml b/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
new file mode 100644
index 000000000000..2cca3d113d11
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/sophgo,sg2042-pcie-host.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo SG2042 PCIe Host (Cadence PCIe Wrapper)
+
+description:
+  Sophgo SG2042 PCIe host controller is based on the Cadence PCIe core.
+
+maintainers:
+  - Chen Wang <unicorn_wang@outlook.com>
+
+properties:
+  compatible:
+    const: sophgo,sg2042-pcie-host
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: reg
+      - const: cfg
+
+  vendor-id:
+    const: 0x1f1c
+
+  device-id:
+    const: 0x2042
+
+  msi-parent: true
+
+allOf:
+  - $ref: cdns-pcie-host.yaml#
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - vendor-id
+  - device-id
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    pcie@62000000 {
+      compatible = "sophgo,sg2042-pcie-host";
+      device_type = "pci";
+      reg = <0x62000000  0x00800000>,
+            <0x48000000  0x00001000>;
+      reg-names = "reg", "cfg";
+      #address-cells = <3>;
+      #size-cells = <2>;
+      ranges = <0x81000000 0 0x00000000 0xde000000 0 0x00010000>,
+               <0x82000000 0 0xd0400000 0xd0400000 0 0x0d000000>;
+      bus-range = <0x00 0xff>;
+      vendor-id = <0x1f1c>;
+      device-id = <0x2042>;
+      cdns,no-bar-match-nbits = <48>;
+      msi-parent = <&msi>;
+    };
-- 
2.34.1

Re: [PATCH 1/5] dt-bindings: pci: Add Sophgo SG2042 PCIe host

[Rob Herring (Arm)]

On Thu, 28 Aug 2025 10:16:54 +0800, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> Add binding for Sophgo SG2042 PCIe host controller.
> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> ---
>  .../bindings/pci/sophgo,sg2042-pcie-host.yaml | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH 1/5] dt-bindings: pci: Add Sophgo SG2042 PCIe host

[Manivannan Sadhasivam]

On Thu, Aug 28, 2025 at 10:16:54AM GMT, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> Add binding for Sophgo SG2042 PCIe host controller.
> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> ---
>  .../bindings/pci/sophgo,sg2042-pcie-host.yaml | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml b/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
> new file mode 100644
> index 000000000000..2cca3d113d11
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/sophgo,sg2042-pcie-host.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo SG2042 PCIe Host (Cadence PCIe Wrapper)
> +
> +description:
> +  Sophgo SG2042 PCIe host controller is based on the Cadence PCIe core.
> +
> +maintainers:
> +  - Chen Wang <unicorn_wang@outlook.com>
> +
> +properties:
> +  compatible:
> +    const: sophgo,sg2042-pcie-host
> +
> +  reg:
> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: reg
> +      - const: cfg
> +
> +  vendor-id:
> +    const: 0x1f1c
> +
> +  device-id:
> +    const: 0x2042
> +
> +  msi-parent: true
> +
> +allOf:
> +  - $ref: cdns-pcie-host.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - vendor-id
> +  - device-id

Why are these IDs 'required'? The default IDs are invalid?

- Mani

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

Re: [PATCH 1/5] dt-bindings: pci: Add Sophgo SG2042 PCIe host

[Chen Wang]

On 8/31/2025 12:47 PM, Manivannan Sadhasivam wrote:
> On Thu, Aug 28, 2025 at 10:16:54AM GMT, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@outlook.com>
>>
>> Add binding for Sophgo SG2042 PCIe host controller.
>>
>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>> ---
>>   .../bindings/pci/sophgo,sg2042-pcie-host.yaml | 66 +++++++++++++++++++
>>   1 file changed, 66 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml b/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
>> new file mode 100644
>> index 000000000000..2cca3d113d11
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml
>> @@ -0,0 +1,66 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pci/sophgo,sg2042-pcie-host.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Sophgo SG2042 PCIe Host (Cadence PCIe Wrapper)
>> +
>> +description:
>> +  Sophgo SG2042 PCIe host controller is based on the Cadence PCIe core.
>> +
>> +maintainers:
>> +  - Chen Wang <unicorn_wang@outlook.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: sophgo,sg2042-pcie-host
>> +
>> +  reg:
>> +    maxItems: 2
>> +
>> +  reg-names:
>> +    items:
>> +      - const: reg
>> +      - const: cfg
>> +
>> +  vendor-id:
>> +    const: 0x1f1c
>> +
>> +  device-id:
>> +    const: 0x2042
>> +
>> +  msi-parent: true
>> +
>> +allOf:
>> +  - $ref: cdns-pcie-host.yaml#
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - vendor-id
>> +  - device-id
> Why are these IDs 'required'? The default IDs are invalid?

I find the default IDs I read from the SoC is still that for Cadence, it 
would confused when I run lspci, so I replace the IDs for Sophgo.

Anyway, it's ok for me to remove IDs as "required" in bindings but still 
set it in DTS.

Thanks,

Chen

>
> - Mani
>