Re: [PATCH 5/6] PCI: spacemit: introduce SpacemiT PCIe host driver

[Alex Elder <elder@riscstar.com>]

On 8/13/25 4:27 PM, Alex Elder wrote:
> On 8/13/25 4:22 PM, Bjorn Helgaas wrote:
>> On Wed, Aug 13, 2025 at 01:46:59PM -0500, Alex Elder wrote:
>>> Introduce a driver for the PCIe root complex 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 v2 transfer
>>> rates (5 GT/sec).  The first port uses a combo PHY, which may be
>>> configured for use for USB 3 instead.

I'm following up on a few things I said last month.

>> I assume "PCIe v2" means what most people call "PCIe gen2", but the
>> spec encourages avoidance "genX" because it's ambiguous.
> 
> Yes, that's what I meant, but I did try to clarify with the
> transfer rate.
> 
>>> +config PCIE_K1
>>> +    bool "SpacemiT K1 host mode PCIe controller"
>>
>> Style of nearby entries is:
>>
>>    "SpacemiT K1 PCIe controller (host mode)"
> 
> OK I'll fix that.
> 
>> Please alphabetize by the company name ("SpacemiT") in the menu entry.
> 
> OK.

I will be renaming the Kconfig option to be PCIE_SPACEMIT_K1
(instead of just PCIE_K1).  I'm renaming the source file to
be "pcie-spacemit-k1.c" instead of "pcie-k1.c" as well.

>>> +#define K1_PCIE_VENDOR_ID    0x201f
>>> +#define K1_PCIE_DEVICE_ID    0x0001
>>
>> I assume this (0x201f) has been reserved by the PCI-SIG?  I don't see
>> it at:
>>
>>    https://pcisig.com/membership/member-companies?combine=0x201f
> 
> I hadn't even thought to check that.  I will follow up.  Thanks
> for pointing this out.

I inquired yesterday about this, and was told that this
will be finalized next week.  I told them that the driver
would not be accepted upstream unless the vendor ID had
been properly reserved by PCI-SIG.

>> Possibly rename this to PCI_VENDOR_ID_K1 (or maybe
>> PCI_VENDOR_ID_SPACEMIT?) to match the usual format in
>> include/linux/pci_ids.h, since it seems likely to end up there
>> eventually.
> 
> OK.

I will use PCI_VENDOR_ID_SPACEMIT and PCI_DEVICE_ID_SPACEMIT_K1.

>>> +#define PCIE_RC_PERST            BIT(12)    /* 0: PERST# high; 1: 
>>> low */
>>
>> Maybe avoid confusion by describing as "1: assert PERST#" or similar?
> 
> OK.  I struggled with how to express this to avoid confusion.
> But I do think "assert PERST#" is better.
> 
>>> +    /* Wait the PCIe-mandated 100 msec before deasserting PERST# */
>>> +    mdelay(100);
>>
>> I think this is PCIE_T_PVPERL_MS.  Comment is superfluous then.
> 
> Excellent, thank you, I'll use that.
> 
>>> +static int k1_pcie_probe(struct platform_device *pdev)
>>> +{
>>> +    struct device *dev = &pdev->dev;
>>> +    struct dw_pcie_rp *pp;
>>> +    struct dw_pcie *pci;
>>> +    struct k1_pcie *k1;
>>> +    int ret;
>>> +
>>> +    k1 = devm_kzalloc(dev, sizeof(*k1), GFP_KERNEL);
>>> +    if (!k1)
>>> +        return -ENOMEM;
>>> +    dev_set_drvdata(dev, k1);
>>
>> Most neighboring drivers use platform_set_drvdata().  Personally, I
>> would set drvdata after initializing k1 because I don't like to
>> advertise pointers to uninitialized things.
> 
> OK, I understand that and will do it the way you suggest.
> 
>>> +static void k1_pcie_remove(struct platform_device *pdev)
>>> +{
>>> +    struct k1_pcie *k1 = dev_get_drvdata(&pdev->dev);
>>
>> Neighbors use platform_get_drvdata().
> 
> Yes, that goes with platform_set_drvdata().
Actually, many of them use dev_get_drvdata().  And I think
that's why I used dev_set_drvdata() in the first place, to
match dev_get_drvdata().

But in any case, I'll switch to setting and getting platform
driver data.

					-Alex

> 
>>> +    struct dw_pcie_rp *pp = &k1->pci.pp;
>>> +
>>> +    dw_pcie_host_deinit(pp);
>>> +}
> 
> Thank you very much for your review.
> 
>                      -Alex


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy