Re: [PATCH] PCI: octeon: Add link down handler support for PCIe MAC controller

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Jan 20, 2026 at 09:14:29PM -0800, Pragnesh Patel wrote:
> From: Suneel Garapati <sgarapati@marvell.com>
> 
> This driver adds support for link-down interrupt in PCIe MAC (PEM)
> controller in Root complex mode to support hot-plug removal of
> endpoint devices.

PEM?

> An interrupt handler is registered for RST_INT msix vector
> which is triggered with link going down. This handler
> performs cleanup of root bridge and its children and
> re-initializes root bridge to kernel for next link-up event.

Wrap to fill 75 columns.

> +config PCI_OCTEON_PEM
> +	bool "Marvell Octeon PEM (PCIe MAC) controller"
> +	depends on ARM64 || COMPILE_TEST
> +	depends on PCI_MSI
> +	depends on HOTPLUG_PCI_PCIE
> +	help
> +	  Say Y here if you want PEM controller support for Marvell ARM64 Octeon SoCs
> +	  in root complex mode.

Wrap to fit in 80 columns like the rest of the file.

> +++ b/drivers/pci/controller/pci-octeon-pem.c

This looks like a PCIe controller, so name it "pcie-octeon-pem.c".
There are some older drivers for PCIe controllers that use "pci-", but
that was a mistake.

> +#include "../hotplug/pciehp.h"

This looks like a red flag, see below.

> +#define PCI_DEVID_OCTEON_PEM	0xA06C

Typical names are PCI_DEVICE_ID_<vendor>_<device> (see
include/linux/pci_ids.h).

In this case, I guess it would be "PCI_DEVICE_ID_CAVIUM_OCTEON" or
similar.

> +#define ID_SHIFT		36

Use GENMASK() instead of shift/mask, so you don't need #defines like
this.

> +#define DOMAIN_OFFSET		0x3
> +#define ON_OFFSET		0xE0
> +#define RST_SOFT_PERST_OFFSET	0x298
> +#define RST_INT_OFFSET		0x300
> +#define RST_INT_ENA_W1C_OFFSET	0x310
> +#define RST_INT_ENA_W1S_OFFSET	0x318
> +#define RST_INT_LINKDOWN	BIT(1)

The "_OFFSET" suffixes look unnecessarily wordy.

> +static void pem_recover_rc_link(struct work_struct *ws)
> +{

> +	/* Check if HP interrupt thread is in progress
> +	 * and wait for it to complete
> +	 */

Multi-line comment style in drivers/pci/ is:

  /*
   * Check ...
   */

Wrap to fill 78 columns or so.

> +	dev_dbg(&pem_dev->dev, "PEM%d HP ist done\n", pem->index);

s/ist/is/

> +	/* Clean-up device and RC bridge */
> +	pci_stop_and_remove_bus_device(root_port);

I'm doubtful about removing a Root Port.  I don't know of any standard
PCIe mechanism for doing that.

> +	pci_unlock_rescan_remove();
> +
> +	usleep_range(100, 200);

Where does this delay come from?  If it's something in the PCIe spec,
we should have a #define in drivers/pci/pci.h for it.  If it's in the
Octeon spec, a reference to that would be helpful.

> +	writeq(0x0, pem->base + RST_SOFT_PERST_OFFSET);
> +
> +	while (timeout--) {
> +		/* Check for PEM_OOR to be set */
> +		pem_reg = readq(pem->base + ON_OFFSET);
> +		if (pem_reg & BIT(1))

Add a #define for this BIT(1).

> +			break;
> +		usleep_range(1000, 2000);

Same delay question as above.

> +	}
> +	if (!timeout) {
> +		dev_warn(&pem_dev->dev,
> +			 "PEM failed to get out of reset\n");
> +		return;
> +	}
> +
> +	pci_lock_rescan_remove();

I haven't reviewed this in detail, but it looks like unusual knowledge
of pciehp internals that might not be appropriate for a controller
driver.

> +static int pem_register_interrupts(struct pci_dev *pdev)
> +{
> +	struct pem_ctlr *pem = pci_get_drvdata(pdev);
> +	int nvec, err;
> +
> +	nvec = pci_msix_vec_count(pdev);

Add blank line before comment, use multi-line comment style.

> +	/* Some earlier silicon versions do not support RST vector
> +	 * so check on table size before registering otherwise
> +	 * return with info message.
> +	 */

> +/* Supported devices */
> +static const struct pci_device_id pem_id_table[] = {
> +	{PCI_VDEVICE(CAVIUM, PCI_DEVID_OCTEON_PEM)},

Same question as Mani, I guess.  PCI controllers are not themselves
PCI devices; they're platform_devices normally discovered via ACPI or
DT.

Root Ports *are* PCIe devices, and normally don't need device specific
code since they are PCI-to-PCI bridges.  The device-specific code is
usually in a PCIe controller (Root Complex) driver that claims the
relevant platform_device.

It's problematic to claim Root Ports, because portdrv normally claims
them so it can support AER, pciehp, etc.