Message-ID: <20241112212055.GA1859446@bhelgaas>
Date: Tue, 12 Nov 2024 15:20:55 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Chen Wang <unicornxw@...il.com>
Cc: kw@...ux.com, u.kleine-koenig@...libre.com, aou@...s.berkeley.edu,
	arnd@...db.de, bhelgaas@...gle.com, unicorn_wang@...look.com,
	conor+dt@...nel.org, guoren@...nel.org, inochiama@...look.com,
	krzk+dt@...nel.org, lee@...nel.org, lpieralisi@...nel.org,
	manivannan.sadhasivam@...aro.org, palmer@...belt.com,
	paul.walmsley@...ive.com, pbrobinson@...il.com, robh@...nel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, linux-riscv@...ts.infradead.org,
	chao.wei@...hgo.com, xiaoguang.xing@...hgo.com,
	fengchun.li@...hgo.com
Subject: Re: [PATCH 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe driver

On Mon, Nov 11, 2024 at 01:59:56PM +0800, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@...look.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
> 

Powered by blists - more mailing lists