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

From: Bjorn Helgaas <helgaas@kernel.org>
To: Alex Elder <elder@riscstar.com>
Cc: lpieralisi@kernel.org, kwilczynski@kernel.org, mani@kernel.org,
	robh@kernel.org, bhelgaas@google.com, krzk+dt@kernel.org,
	conor+dt@kernel.org, vkoul@kernel.org, kishon@kernel.org,
	dlan@gentoo.org, paul.walmsley@sifive.com, palmer@dabbelt.com,
	aou@eecs.berkeley.edu, alex@ghiti.fr, p.zabel@pengutronix.de,
	tglx@linutronix.de, johan+linaro@kernel.org,
	thippeswamy.havalige@amd.com, namcao@linutronix.de,
	mayank.rana@oss.qualcomm.com, shradha.t@samsung.com,
	inochiama@gmail.com, quic_schintav@quicinc.com,
	fan.ni@samsung.com, devicetree@vger.kernel.org,
	linux-phy@lists.infradead.org, linux-pci@vger.kernel.org,
	spacemit@lists.linux.dev, linux-riscv@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/6] PCI: spacemit: introduce SpacemiT PCIe host driver
Date: Wed, 13 Aug 2025 16:22:19 -0500	[thread overview]
Message-ID: <20250813212219.GA294849@bhelgaas> (raw)
In-Reply-To: <20250813184701.2444372-6-elder@riscstar.com>

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);
> +}

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