On 2024/12/11 1:31, Bjorn Helgaas wrote:
On Mon, Dec 09, 2024 at 03:19:57PM +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/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
Add the newline.
ok
+++ b/drivers/pci/controller/cadence/pcie-sg2042.c
+#include "../../../irqchip/irq-msi-lib.h"
This is the only file outside drivers/irqchip/ that includes this.
What's special about this driver? Maybe this is a hint that something
here belongs in drivers/irqchip/?
This file is included due to MSI parent model is used in this driver and
I used helper functions such as msi_lib_init_dev_msi_info.
Actually I see Marc is working on MSI cleanup and will make
irq-msi-lib.h globally available, see
https://lore.kernel.org/linux-riscv/20241204124549.607054-2-maz@xxxxxxxxxx/
But before his PR is merged, we may have to include the file like this now.
+#ifdef CONFIG_SMP
No other drivers test CONFIG_SMP, why should this be different?
ok, seems it'ok to remove this test.
+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;
+}
+#endif /* CONFIG_SMP */
+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->msi_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));
Not sure this is needed. Does DT dma-ranges not cover this?
I prefer not introduce dma-ranges, we just need to allocate a small
continuous space here, and using dma_set_coherent_mask is also one of
the commonly used methods.
+static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie, struct device_node *msi_node)
Wrap to fit in 80 columns like the rest.
ok
+/*
+ * 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
s/PCIe controller/Root Port/
ok
+ * the PCIe peripheral registers, supports 1/2/4 byte aligned access, so
+ * directly use read should be fine.
s/use read/using read/
ok, thanks
+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 device_node *msi_node;
+ 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;
I don't think this is needed since CONFIG_PCIE_SG2042 selects
PCIE_CADENCE_HOST.
Yes, you are right.
Thanks,
Chen
Bjorn
