linux-kernel - Re: [PATCH v5 5/7] PCI: spacemit: Add SpacemiT PCIe host driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4a7d6e07-82f8-437f-bccb-cf294974a53e@riscstar.com>
Date: Mon, 10 Nov 2025 09:05:35 -0600
From: Alex Elder <elder@...cstar.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
 lpieralisi@...nel.org, kwilczynski@...nel.org, mani@...nel.org,
 robh@...nel.org, bhelgaas@...gle.com
Cc: dlan@...too.org, aurelien@...el32.net, johannes@...felt.com,
 p.zabel@...gutronix.de, christian.bruel@...s.st.com,
 thippeswamy.havalige@....com, krishna.chundru@....qualcomm.com,
 mayank.rana@....qualcomm.com, qiang.yu@....qualcomm.com,
 shradha.t@...sung.com, inochiama@...il.com, guodong@...cstar.com,
 linux-pci@...r.kernel.org, spacemit@...ts.linux.dev,
 linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 5/7] PCI: spacemit: Add SpacemiT PCIe host driver

On 11/8/25 7:00 AM, Christophe JAILLET wrote:
> Le 07/11/2025 à 20:15, Alex Elder a écrit :
>> Introduce a driver for the PCIe host controller found in the SpacemiT
>> K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
>> The driver supports three PCIe ports that operate at PCIe gen2 transfer
>> rates (5 GT/sec).  The first port uses a combo PHY, which may be
>> configured for use for USB 3 instead.
>>
>> Signed-off-by: Alex Elder <elder@...cstar.com>
>> ---
> 
> ...
> 
>> +static int k1_pcie_probe(struct platform_device *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct k1_pcie *k1;
>> +    int ret;
>> +
>> +    k1 = devm_kzalloc(dev, sizeof(*k1), GFP_KERNEL);
>> +    if (!k1)
>> +        return -ENOMEM;
>> +
>> +    k1->pmu = syscon_regmap_lookup_by_phandle_args(dev_of_node(dev),
>> +                               SYSCON_APMU, 1,
>> +                               &k1->pmu_off);
>> +    if (IS_ERR(k1->pmu))
>> +        return dev_err_probe(dev, PTR_ERR(k1->pmu),
>> +                     "failed to lookup PMU registers\n");
>> +
>> +    k1->link = devm_platform_ioremap_resource_byname(pdev, "link");
>> +    if (!k1->link)
> 
> if (IS_ERR(k1->link)) ?

Yes, you're right.  I'll fix this in the next version.


> 
>> +        return dev_err_probe(dev, -ENOMEM,
>> +                     "failed to map \"link\" registers\n");
> 
> Message with -ENOMEM are ignored, so a direct return -ENOMEM is less 
> verbose and will bhave the same. See [1].
> 
> But in this case, I think it should be PTR_ERR(k1->link).

Yes, that's what it will be.

> [1]: https://elixir.bootlin.com/linux/v6.18-rc2/source/drivers/base/ 
> core.c#L5015
> 
>> +
>> +    k1->pci.dev = dev;
>> +    k1->pci.ops = &k1_pcie_ops;
>> +    dw_pcie_cap_set(&k1->pci, REQ_RES);
>> +
>> +    k1->pci.pp.ops = &k1_pcie_host_ops;
>> +
>> +    /* Hold the PHY in reset until we start the link */
>> +    regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
>> +            APP_HOLD_PHY_RST);
>> +
>> +    ret = devm_regulator_get_enable(dev, "vpcie3v3");
>> +    if (ret)
>> +        return dev_err_probe(dev, ret,
>> +                     "failed to get \"vpcie3v3\" supply\n");
>> +
>> +    pm_runtime_set_active(dev);
>> +    pm_runtime_no_callbacks(dev);
>> +    devm_pm_runtime_enable(dev);
>> +
>> +    platform_set_drvdata(pdev, k1);
>> +
>> +    ret = k1_pcie_parse_port(k1);
>> +    if (ret)
>> +        return dev_err_probe(dev, ret, "failed to parse root port\n");
>> +
>> +    ret = dw_pcie_host_init(&k1->pci.pp);
>> +    if (ret)
>> +        return dev_err_probe(dev, ret, "failed to initialize host\n");
>> +
>> +    return 0;
>> +}
> 
> ...
> 
>> +static const struct of_device_id k1_pcie_of_match_table[] = {
>> +    { .compatible = "spacemit,k1-pcie", },
>> +    { },
> 
> Unneeded trainling comma after a terminator.

Unneeded, but not erroneous.  In any case, I'll drop it based
on your preference.

Thank you for your review.

					-Alex

> 
>> +};
>> +
>> +static struct platform_driver k1_pcie_driver = {
>> +    .probe    = k1_pcie_probe,
>> +    .remove    = k1_pcie_remove,
>> +    .driver = {
>> +        .name            = "spacemit-k1-pcie",
>> +        .of_match_table        = k1_pcie_of_match_table,
>> +        .probe_type        = PROBE_PREFER_ASYNCHRONOUS,
>> +    },
>> +};
>> +module_platform_driver(k1_pcie_driver);
>