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@xxxxxxxxxxx>
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?
[......]
