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

[Alex Elder <elder@riscstar.com>]

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@riscstar.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);
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv