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

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

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

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.

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.

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

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

+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

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

quoted

+	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