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

On 8/31/2025 12:45 PM, Manivannan Sadhasivam wrote:
> On Thu, Aug 28, 2025 at 10:17:40AM GMT, Chen Wang wrote:
>> From: Chen Wang <[email protected]>
>>
>> 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.
>>
> Supported data rate, lanes etc...
OK, I will add these descriptions in next revision.
>> Signed-off-by: Chen Wang <[email protected]>
>> ---
>>   drivers/pci/controller/cadence/Kconfig       |  12 ++
>>   drivers/pci/controller/cadence/Makefile      |   1 +
>>   drivers/pci/controller/cadence/pcie-sg2042.c | 134 +++++++++++++++++++
>>   3 files changed, 147 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 666e16b6367f..b1f1941d5208 100644
>> --- a/drivers/pci/controller/cadence/Kconfig
>> +++ b/drivers/pci/controller/cadence/Kconfig
>> @@ -42,6 +42,17 @@ config PCIE_CADENCE_PLAT_EP
>>   	  endpoint mode. This PCIe controller may be embedded into many
>>   	  different vendors SoCs.
>>   
>> +config PCIE_SG2042
> PCIE_SG2042_HOST
ok
>
>> +	bool "Sophgo SG2042 PCIe controller (host mode)"
> Since this driver doesn't implement irqchip, you should make it tristate and as
> a module.
Yes, I will implement it as a module.
>
>> +	depends on ARCH_SOPHGO || COMPILE_TEST
>> +	depends on OF
> 	depends on OF && (ARCH_SOPHGO || COMPILE_TEST)

OK

[......]

>> +
>> +	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
>> +	if (!bridge) {
>> +		dev_err(dev, "Failed to alloc host bridge!\n");
> Use dev_err_probe() here and below.
OK
>> +		return -ENOMEM;
>> +	}
>> +
>> +	bridge->ops = &sg2042_pcie_host_ops;
>> +
>> +	rc = pci_host_bridge_priv(bridge);
>> +	pcie = &rc->pcie;
> You are setting drvdata only below.

Sorry, I don't understand your question here. I guess you are confused 
about the statement `platform_set_drvdata(pdev, pcie);`. Let me explain 
why we need to do like this.

Originally, I defined a drvdata stucture:

struct sg2042_pcie {
     struct cdns_pcie    *cdns_pcie;

     // other properties
};

But after code cleanup, I find there is only "cdns_pcie" left, so I 
remove the `struct sg2042_pcie` and directy set `cdns_pcie` (in new 
version, it is renamed to pcie) as drvdata.

>> +	pcie->dev = dev;
>> +
>> +	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;
>> +	}
>> +
> Why do you need pm_runtime_get_sync()? DT binding doesn't provide a
> power-domain, so you just need:
>
> 	pm_runtime_set_active()
>          pm_runtime_no_callbacks()
>          devm_pm_runtime_enable()
OK, I will improve this.
>> +	ret = cdns_pcie_init_phy(dev, pcie);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to init phy!\n");
>> +		goto err_get_sync;
>> +	}
>> +
>> +	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(pcie);
>> +
>> +err_get_sync:
>> +	pm_runtime_put(dev);
>> +	pm_runtime_disable(dev);
>> +
>> +	return ret;
>> +}
>> +
>> +static void sg2042_pcie_shutdown(struct platform_device *pdev)
>> +{
>> +	struct cdns_pcie *pcie = platform_get_drvdata(pdev);
>> +	struct device *dev = &pdev->dev;
>> +
>> +	cdns_pcie_disable_phy(pcie);
>> +
>> +	pm_runtime_put(dev);
>> +	pm_runtime_disable(dev);
> You don't need these as per my above suggestion.
OK.
>> +}
>> +
>> +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,
> Why no remove()?

OK, I will replace shutdown with remove when I implement it as a module 
in next revision.

Thanks,

Chen

>
> - Mani
>