On 8/13/25 4:27 PM, Alex Elder wrote:

On 8/13/25 4:22 PM, Bjorn Helgaas wrote:

quoted

On Wed, Aug 13, 2025 at 01:46:59PM -0500, Alex Elder wrote:

quoted

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.

quoted

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.

quoted

quoted

+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.

quoted

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.

quoted

quoted

+#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.

quoted

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.

quoted

quoted

+#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.

quoted

quoted

+    /* 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.

quoted

quoted

+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.

quoted

quoted

+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

quoted

quoted

+    struct dw_pcie_rp *pp = &k1->pci.pp;
+
+    dw_pcie_host_deinit(pp);
+}

Thank you very much for your review.

                     -Alex