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

[Bjorn Helgaas <helgaas@kernel.org>]

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