Message-ID:
 <BM1PR01MB254540560C1281CE9898A5A0FEE12@BM1PR01MB2545.INDPRD01.PROD.OUTLOOK.COM>
Date: Wed, 22 Jan 2025 21:28:12 +0800
From: Chen Wang <unicorn_wang@...look.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
 Chen Wang <unicornxw@...il.com>
Cc: kw@...ux.com, u.kleine-koenig@...libre.com, aou@...s.berkeley.edu,
 arnd@...db.de, bhelgaas@...gle.com, conor+dt@...nel.org, guoren@...nel.org,
 inochiama@...look.com, krzk+dt@...nel.org, lee@...nel.org,
 lpieralisi@...nel.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, helgaas@...nel.org
Subject: Re: [PATCH v3 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe driver


On 2025/1/19 20:23, Manivannan Sadhasivam wrote:
> On Wed, Jan 15, 2025 at 03:06:57PM +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.
>>
> Please add more info about the IP. Like IP revision, PCIe Gen, lane count,
> etc...
ok.
>> Signed-off-by: Chen Wang <unicorn_wang@...look.com>
>> ---
>>   drivers/pci/controller/cadence/Kconfig       |  13 +
>>   drivers/pci/controller/cadence/Makefile      |   1 +
>>   drivers/pci/controller/cadence/pcie-sg2042.c | 528 +++++++++++++++++++
>>   3 files changed, 542 insertions(+)
>>   create mode 100644 drivers/pci/controller/cadence/pcie-sg2042.c
>>
>> diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
>> index 8a0044bb3989..292eb2b20e9c 100644
>> --- a/drivers/pci/controller/cadence/Kconfig
>> +++ b/drivers/pci/controller/cadence/Kconfig
>> @@ -42,6 +42,18 @@ config PCIE_CADENCE_PLAT_EP
>>   	  endpoint mode. This PCIe controller may be embedded into many
>>   	  different vendors SoCs.
>>   
>> +config PCIE_SG2042
>> +	bool "Sophgo SG2042 PCIe controller (host mode)"
>> +	depends on ARCH_SOPHGO || COMPILE_TEST
>> +	depends on OF
>> +	select IRQ_MSI_LIB
>> +	select PCI_MSI
>> +	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.
>> +
>>   config PCI_J721E
>>   	bool
>>   
>> @@ -67,4 +79,5 @@ 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.
>> +
> Spurious newline.

oops, I will fix this.

[......]

>> +/*
>> + * SG2042 PCIe controller supports two ways to report MSI:
>> + *
>> + * - Method A, the PCIe 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 PCIe 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, default is Method B.
> How to configure them? I can only see "sophgo,sg2042-msi" in the binding.


The value of the msi-parent attribute is used in dts to distinguish 
them, for example:

```dts

msi: msi-controller@...0010300 {
     ......
};

pcie_rc0: pcie@...0000000 {
     msi-parent = <&msi>;
};

pcie_rc1: pcie@...2000000 {
     ......
     msi-parent = <&msi_pcie>;
     msi_pcie: interrupt-controller {
         ......
     };
};

```

Which means:

pcie_rc0 uses Method B

pcie_rc1 uses Method A.

[......]

>> +struct sg2042_pcie {
>> +	struct cdns_pcie	*cdns_pcie;
>> +
>> +	struct regmap		*syscon;
>> +
>> +	u32			link_id;
>> +
>> +	struct irq_domain	*msi_domain;
>> +
>> +	int			msi_irq;
>> +
>> +	dma_addr_t		msi_phys;
>> +	void			*msi_virt;
>> +
>> +	u32			num_applied_vecs; /* used to speed up ISR */
>> +
> Get rid of the newline between struct members, please.
ok
>> +	raw_spinlock_t		msi_lock;
>> +	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
>> +};
>> +
> [...]
>
>> +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie,
>> +				 struct device_node *msi_node)
>> +{
>> +	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;
>> +
>> +	if (!of_property_read_bool(msi_node, "msi-controller"))
>> +		return -ENODEV;
>> +
>> +	ret = of_irq_get_byname(msi_node, "msi");
>> +	if (ret <= 0) {
>> +		dev_err(dev, "%pOF: failed to get MSI irq\n", msi_node);
>> +		return ret;
>> +	}
>> +	pcie->msi_irq = ret;
>> +
>> +	irq_set_chained_handler_and_data(pcie->msi_irq,
>> +					 sg2042_pcie_msi_chained_isr, pcie);
>> +
>> +	parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS,
>> +						 &sg2042_pcie_msi_domain_ops, pcie);
>> +	if (!parent_domain) {
>> +		dev_err(dev, "%pfw: Failed to create IRQ domain\n", fwnode);
>> +		return -ENOMEM;
>> +	}
>> +	irq_domain_update_bus_token(parent_domain, DOMAIN_BUS_NEXUS);
>> +
> The MSI controller is wired to PLIC isn't it? If so, why can't you use
> hierarchial MSI domain implementation as like other controller drivers?

The method used here is somewhat similar to dw_pcie_allocate_domains() 
in drivers/pci/controller/dwc/pcie-designware-host.c. This MSI 
controller is about Method A, the PCIe controller implements an MSI 
interrupt controller inside, and connect to PLIC upward through only ONE 
interrupt line. Because MSI to PLIC is multiple to one, I use linear 
mode here and use chained ISR to handle the interrupts.

[......]

>> +/* Dummy ops which will be assigned to cdns_pcie.ops, which must be !NULL. */
> Because cadence code driver doesn't check for !ops? Please fix it instead. And
> the fix is trivial.

ok, I will fix this for cadence code together in next version.

[......]

>> +	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
>> +	if (!bridge) {
>> +		dev_err(dev, "Failed to alloc host bridge!\n");
> Use dev_err_probe() throughout the probe function.
Got it.
>> +		return -ENOMEM;
>> +	}
>> +
>> +	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;
> -ENODEV
Got.
>> +	}
>> +	syscon = syscon_node_to_regmap(np_syscon);
> You should drop the np reference count once you are done with it.
ok, I will double check this through the file.
>> +	if (IS_ERR(syscon)) {
>> +		dev_err(dev, "Failed to get regmap for syscon\n");
>> +		return -ENOMEM;
> PTR_ERR(syscon)

yes.


>> +	}
>> +	pcie->syscon = syscon;
>> +
>> +	if (of_property_read_u32(np, "sophgo,link-id", &pcie->link_id)) {
>> +		dev_err(dev, "Unable to parse sophgo,link-id\n");
> "Unable to parse \"sophgo,link-id\"\n"
ok.
>> +		return -EINVAL;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, pcie);
>> +
>> +	pm_runtime_enable(dev);
>> +
>> +	ret = pm_runtime_get_sync(dev);
> Use pm_runtime_resume_and_get().
Got it.
>> +	if (ret < 0) {
>> +		dev_err(dev, "pm_runtime_get_sync failed\n");
>> +		goto err_get_sync;
>> +	}
>> +
>> +	msi_node = of_parse_phandle(dev->of_node, "msi-parent", 0);
>> +	if (!msi_node) {
>> +		dev_err(dev, "Failed to get msi-parent!\n");
>> +		return -1;
> -ENODATA
Thanks, this should be fixed.
>
>> +	}
>> +
>> +	if (of_device_is_compatible(msi_node, "sophgo,sg2042-pcie-msi")) {
>> +		ret = sg2042_pcie_setup_msi(pcie, msi_node);
> Same as above. np leak here.
ok, will check this through the file.
>
> - Mani

Thanks,

Chen

Powered by blists - more mailing lists