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

Bjorn Helgaas
2024-12-10T17:31:25Z

On Mon, Dec 09, 2024 at 03:19:57PM +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/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.

> +++ 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/?

> +#ifdef CONFIG_SMP

No other drivers test CONFIG_SMP, why should this be different?

> +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?

> +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie, struct device_node *msi_node)

Wrap to fit in 80 columns like the rest.

> +/*
> + * 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/

> + * the PCIe peripheral registers, supports 1/2/4 byte aligned access, so
> + * directly use read should be fine.

s/use read/using read/

> +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.

Bjorn