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 assume "PCIe v2" means what most people call "PCIe gen2", but the
spec encourages avoidance "genX" because it's ambiguous.
> +config PCIE_K1
> + bool "SpacemiT K1 host mode PCIe controller"
Style of nearby entries is:
"SpacemiT K1 PCIe controller (host mode)"
Please alphabetize by the company name ("SpacemiT") in the menu entry.
> +#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
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.
> +#define PCIE_RC_PERST BIT(12) /* 0: PERST# high; 1: low */
Maybe avoid confusion by describing as "1: assert PERST#" or similar?
> + /* Wait the PCIe-mandated 100 msec before deasserting PERST# */
> + mdelay(100);
I think this is PCIE_T_PVPERL_MS. Comment is superfluous then.
> +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.
> +static void k1_pcie_remove(struct platform_device *pdev)
> +{
> + struct k1_pcie *k1 = dev_get_drvdata(&pdev->dev);
Neighbors use platform_get_drvdata().
> + struct dw_pcie_rp *pp = &k1->pci.pp;
> +
> + dw_pcie_host_deinit(pp);
> +}
