[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<MAUPR01MB110724916F72E34B35E9E6361FE07A@MAUPR01MB11072.INDPRD01.PROD.OUTLOOK.COM>
Date: Mon, 1 Sep 2025 14:00:28 +0800
From: Chen Wang <unicorn_wang@...look.com>
To: Manivannan Sadhasivam <mani@...nel.org>, Chen Wang <unicornxw@...il.com>
Cc: kwilczynski@...nel.org, u.kleine-koenig@...libre.com,
aou@...s.berkeley.edu, alex@...ti.fr, arnd@...db.de, bwawrzyn@...co.com,
bhelgaas@...gle.com, conor+dt@...nel.org, 18255117159@....com,
inochiama@...il.com, kishon@...nel.org, krzk+dt@...nel.org,
lpieralisi@...nel.org, palmer@...belt.com, paul.walmsley@...ive.com,
robh@...nel.org, s-vadapalli@...com, tglx@...utronix.de,
thomas.richard@...tlin.com, sycamoremoon376@...il.com,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pci@...r.kernel.org, linux-riscv@...ts.infradead.org,
sophgo@...ts.linux.dev, rabenda.cn@...il.com, chao.wei@...hgo.com,
xiaoguang.xing@...hgo.com, fengchun.li@...hgo.com
Subject: 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 <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.
>>
> Supported data rate, lanes etc...
OK, I will add these descriptions in next revision.
>> Signed-off-by: Chen Wang <unicorn_wang@...look.com>
>> ---
>> 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
>
Powered by blists - more mailing lists