[PATCH v3] gpio: amdptpci: add support for AMD PT PCI controller

[PATCH v3] gpio: amdptpci: add support for AMD PT PCI controller

[Szuying Chen]

The GPIO controller on AMD PT PCI platforms currently has no in-tree
driver, so its pins cannot be used from Linux.

The existing gpio-amdpt driver cannot be reused. Setting aside ACPI
vs. PCI probing, the hardware differs significantly:

 - Register semantics conflict: offset 0x0C controls the clock rate
   in gpio-amdpt but enables interrupts in this controller.
 - Interrupt controller: gpio-amdpt has no interrupt support, while
   this controller has its own INT_LVL, INT_STAT and INT_MASK
   registers that require a full irq_chip implementation.
 - Wakeup: this controller can wake the system from GPIO interrupts,
   which gpio-amdpt does not support.

Only the basic data registers are common, and they are handled
through gpio-generic, so little code is left to share.

Add the gpio-amdptpci driver to support the GPIO functionality on
AMD PT PCI platforms.

Co-developed-by: YD Tseng <Yd_Tseng@asmedia.com.tw>
Signed-off-by: YD Tseng <Yd_Tseng@asmedia.com.tw>
Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
---
V2->V3:
	- Rework the driver to use gpio-generic.
	- Move runtime PM handling to gpio_chip request() and
	free() callbacks.
	- Simplify locking with guard() and scoped_guard().
	- Use devm_add_action_or_reset() for resource cleanup.
	- Add rationale for a separate driver to the commit message.
	- Regmap was evaluated but not used: the registers are mix of
	8/16/32 bit and it would need more callbacks to define, which
	complicates the code.
v1->v2:
	- Add .irq_request/release_resources to set irq_pm_held flag.
	- switch to devres in probe/remove.
	- Add synchronize_irq().
---
 drivers/gpio/Kconfig         |   6 +
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-amdptpci.c | 610 +++++++++++++++++++++++++++++++++++
 3 files changed, 617 insertions(+)
 create mode 100644 drivers/gpio/gpio-amdptpci.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a48586bb8edb..1c2f6fc164a1 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1833,6 +1833,12 @@ endmenu
 menu "PCI GPIO expanders"
 	depends on PCI

+config GPIO_AMDPTPCI
+	tristate "AMD Promontory PCI GPIO support"
+	depends on X86
+	help
+	  Driver for Promontory IOHub PCI GPIO functionality.
+
 config GPIO_AMD8111
 	tristate "AMD 8111 GPIO driver"
 	depends on X86 || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index dc9e6d643b5b..ca26e98fc8e8 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_GPIO_ALTERA)  		+= gpio-altera.o
 obj-$(CONFIG_GPIO_AMD8111)		+= gpio-amd8111.o
 obj-$(CONFIG_GPIO_AMD_FCH)		+= gpio-amd-fch.o
 obj-$(CONFIG_GPIO_AMDPT)		+= gpio-amdpt.o
+obj-$(CONFIG_GPIO_AMDPTPCI)		+= gpio-amdptpci.o
 obj-$(CONFIG_GPIO_ARIZONA)		+= gpio-arizona.o
 obj-$(CONFIG_GPIO_ASPEED)		+= gpio-aspeed.o
 obj-$(CONFIG_GPIO_ASPEED_SGPIO)		+= gpio-aspeed-sgpio.o
diff --git a/drivers/gpio/gpio-amdptpci.c b/drivers/gpio/gpio-amdptpci.c
new file mode 100644
index 000000000000..171c6811a33e
--- /dev/null
+++ b/drivers/gpio/gpio-amdptpci.c
@@ -0,0 +1,610 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * gpio-amdptpci.c - AMD Promontory PCI GPIO Controller Driver
+ *
+ * Copyright (C) 2026 ASMedia Technology Inc.
+ * Author:	Chloe Chen <chloe_chen@asmedia.com.tw>
+ *		YD Tseng <yd_tseng@asmedia.com.tw>
+ */
+
+#include <linux/bits.h>
+#include <linux/compiler.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/generic.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/pm_wakeup.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+#define PT27_NUM_GPIOS          24
+#define PT27_WAKE_GPIOS         12
+#define PT27_GPIO_MASK          GENMASK(PT27_NUM_GPIOS - 1, 0)
+#define PT27_WAKE_MASK          GENMASK(PT27_WAKE_GPIOS - 1, 0)
+
+#define PT27_REG_DIR            0x0000
+#define PT27_REG_IN             0x0004
+#define PT27_REG_OUT            0x0008
+#define PT27_REG_INT_EN         0x000C
+#define PT27_REG_INT_LVL        0x0010
+#define PT27_REG_INT_MODE0      0x0014
+#define PT27_REG_INT_MODE1      0x0018
+#define PT27_REG_INT_STAT       0x001C
+#define PT27_REG_INT_MASK       0x0020
+#define PT27_REG_INT_CTL        0x0024
+
+#define PT27_REG_WAKE_EN        0x0034
+#define PT27_REG_WAKE_LVL       0x0036
+#define PT27_REG_WAKE_MODE      0x0038
+#define PT27_REG_WAKE_STAT      0x003A
+
+#define PT27_INT_MODE_MASK      0x3U
+#define PT27_MODE_LEVEL         0x0U
+#define PT27_MODE_RISING        0x1U
+#define PT27_MODE_FALLING       0x2U
+#define PT27_MODE_BOTH          0x3U
+
+#define PT27_INT_CTL_MODE       BIT(0)
+#define PT27_INT_CTL_PAD_EN     BIT(2)
+
+#define PT27_AUTOSUSPEND_DELAY_MS   2000
+
+struct pt27_saved_regs {
+	u32 dir;
+	u32 out;
+	u32 int_en;
+	u32 int_lvl;
+	u32 int_mode0;
+	u16 int_mode1;
+	u32 int_mask;
+	u8  int_ctl;
+	u16 wake_en;
+	u16 wake_lvl;
+	u16 wake_mode;
+};
+
+struct pt27_gpio {
+struct device                     *dev;
+struct gpio_generic_chipgen;
+	void __iomem              *base;
+	struct pt27_saved_regs	  saved;
+	raw_spinlock_t            lock;
+	u16                       wake_mask;
+	bool                      use_msi;
+	int                       irq;
+};
+
+static void pt27_save_regs(struct pt27_gpio *chip)
+{
+	chip->saved.dir       = ioread32(chip->base + PT27_REG_DIR);
+	chip->saved.out       = ioread32(chip->base + PT27_REG_OUT);
+
+	chip->saved.int_en    = ioread32(chip->base + PT27_REG_INT_EN);
+	chip->saved.int_lvl   = ioread32(chip->base + PT27_REG_INT_LVL);
+	chip->saved.int_mode0 = ioread32(chip->base + PT27_REG_INT_MODE0);
+	chip->saved.int_mode1 = ioread16(chip->base + PT27_REG_INT_MODE1);
+	chip->saved.int_mask  = ioread32(chip->base + PT27_REG_INT_MASK);
+	chip->saved.int_ctl   = ioread8(chip->base + PT27_REG_INT_CTL);
+
+	chip->saved.wake_en   = ioread16(chip->base + PT27_REG_WAKE_EN);
+	chip->saved.wake_lvl  = ioread16(chip->base + PT27_REG_WAKE_LVL);
+	chip->saved.wake_mode = ioread16(chip->base + PT27_REG_WAKE_MODE);
+}
+
+static void pt27_restore_regs(struct pt27_gpio *chip)
+{
+	iowrite32(chip->saved.dir,       chip->base + PT27_REG_DIR);
+	iowrite32(chip->saved.out,       chip->base + PT27_REG_OUT);
+
+	iowrite32(chip->saved.int_lvl,   chip->base + PT27_REG_INT_LVL);
+	iowrite32(chip->saved.int_mode0, chip->base + PT27_REG_INT_MODE0);
+	iowrite16(chip->saved.int_mode1, chip->base + PT27_REG_INT_MODE1);
+	iowrite8(chip->saved.int_ctl,    chip->base + PT27_REG_INT_CTL);
+
+	iowrite16(chip->saved.wake_lvl,  chip->base + PT27_REG_WAKE_LVL);
+	iowrite16(chip->saved.wake_mode, chip->base + PT27_REG_WAKE_MODE);
+	iowrite16(chip->saved.wake_en,   chip->base + PT27_REG_WAKE_EN);
+
+	iowrite32(chip->saved.int_en,   chip->base + PT27_REG_INT_EN);
+	iowrite32(chip->saved.int_mask, chip->base + PT27_REG_INT_MASK);
+}
+
+static int pt27_gpio_request(struct gpio_chip *gc, unsigned int offset)
+{
+	struct pt27_gpio *chip = gpiochip_get_data(gc);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(chip->dev);
+	if (ret < 0) {
+		dev_err_ratelimited(chip->dev, "%s: cannot resume for GPIO%u: %d\n",  __func__, offset, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void pt27_gpio_free(struct gpio_chip *gc, unsigned int offset)
+{
+	struct pt27_gpio *chip = gpiochip_get_data(gc);
+
+	pm_runtime_put_autosuspend(chip->dev);
+}
+
+static int pt27_irq_reqres(struct irq_data *d)
+{
+	struct gpio_chip *gc   = irq_data_get_irq_chip_data(d);
+	irq_hw_number_t   hwirq = irqd_to_hwirq(d);
+	int ret;
+
+	ret = gpiochip_reqres_irq(gc, hwirq);
+	return ret;
+}
+
+static void pt27_irq_relres(struct irq_data *d)
+{
+	struct gpio_chip *gc   = irq_data_get_irq_chip_data(d);
+	irq_hw_number_t   hwirq = irqd_to_hwirq(d);
+
+	gpiochip_relres_irq(gc, hwirq);
+}
+
+static void pt27_irq_mask(struct irq_data *d)
+{
+	struct gpio_chip *gc   = irq_data_get_irq_chip_data(d);
+	struct pt27_gpio *chip = gpiochip_get_data(gc);
+	irq_hw_number_t   hwirq = irqd_to_hwirq(d);
+	u32 val;
+
+	guard(raw_spinlock_irqsave)(&chip->lock);
+
+	val  = ioread32(chip->base + PT27_REG_INT_MASK);
+	val |= BIT(hwirq);
+	iowrite32(val, chip->base + PT27_REG_INT_MASK);
+
+	val  = ioread32(chip->base + PT27_REG_INT_EN);
+	val &= ~BIT(hwirq);
+	iowrite32(val, chip->base + PT27_REG_INT_EN);
+
+	gpiochip_disable_irq(gc, hwirq);
+}
+
+static void pt27_irq_unmask(struct irq_data *d)
+{
+	struct gpio_chip *gc   = irq_data_get_irq_chip_data(d);
+	struct pt27_gpio *chip = gpiochip_get_data(gc);
+	irq_hw_number_t   hwirq = irqd_to_hwirq(d);
+	u32 val;
+
+	gpiochip_enable_irq(gc, hwirq);
+
+	guard(raw_spinlock_irqsave)(&chip->lock);
+
+	val  = ioread32(chip->base + PT27_REG_INT_EN);
+	val |= BIT(hwirq);
+	iowrite32(val, chip->base + PT27_REG_INT_EN);
+
+	val  = ioread32(chip->base + PT27_REG_INT_MASK);
+	val &= ~BIT(hwirq);
+	iowrite32(val, chip->base + PT27_REG_INT_MASK);
+
+}
+
+static void pt27_irq_ack(struct irq_data *d)
+{
+	struct gpio_chip *gc   = irq_data_get_irq_chip_data(d);
+	struct pt27_gpio *chip = gpiochip_get_data(gc);
+
+	iowrite32(BIT(irqd_to_hwirq(d)), chip->base + PT27_REG_INT_STAT);
+}
+
+static int pt27_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct gpio_chip *gc   = irq_data_get_irq_chip_data(d);
+	struct pt27_gpio *chip = gpiochip_get_data(gc);
+	irq_hw_number_t   hwirq = irqd_to_hwirq(d);
+	u32 lvl_reg;
+	u8  lvl_bit, mode_val;
+	unsigned int shift;
+
+	switch (type) {
+	case IRQ_TYPE_LEVEL_HIGH:
+		lvl_bit = 1;
+		mode_val = PT27_MODE_LEVEL;
+		break;
+
+	case IRQ_TYPE_LEVEL_LOW:
+		lvl_bit = 0;
+		mode_val = PT27_MODE_LEVEL;
+		break;
+
+	case IRQ_TYPE_EDGE_RISING:
+		lvl_bit = 1;
+		mode_val = PT27_MODE_RISING;
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		lvl_bit = 0;
+		mode_val = PT27_MODE_FALLING;
+		break;
+
+	case IRQ_TYPE_EDGE_BOTH:
+		lvl_bit = 0;
+		mode_val = PT27_MODE_BOTH;
+		break;
+
+	default:
+		return -EINVAL;
+
+	}
+
+	scoped_guard(raw_spinlock_irqsave, &chip->lock) {
+		lvl_reg = ioread32(chip->base + PT27_REG_INT_LVL);
+		if (lvl_bit)
+			lvl_reg |= BIT(hwirq);
+		else
+			lvl_reg &= ~BIT(hwirq);
+
+		iowrite32(lvl_reg, chip->base + PT27_REG_INT_LVL);
+
+		if (hwirq < 16) {
+			u32 reg = ioread32(chip->base + PT27_REG_INT_MODE0);
+
+			shift  = hwirq * 2;
+			reg   &= ~(PT27_INT_MODE_MASK << shift);
+			reg   |= ((u32)mode_val << shift);
+			iowrite32(reg, chip->base + PT27_REG_INT_MODE0);
+		} else {
+			u16 reg = ioread16(chip->base + PT27_REG_INT_MODE1);
+
+			shift  = (hwirq - 16) * 2;
+			reg   &= ~((u16)(PT27_INT_MODE_MASK << shift));
+			reg   |= (u16)((u16)mode_val << shift);
+			iowrite16(reg, chip->base + PT27_REG_INT_MODE1);
+		}
+
+		iowrite32(BIT(hwirq), chip->base + PT27_REG_INT_STAT);
+	}
+
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		irq_set_handler_locked(d, handle_level_irq);
+	else
+		irq_set_handler_locked(d, handle_edge_irq);
+
+	return 0;
+}
+
+static int pt27_irq_set_wake(struct irq_data *d, unsigned int on)
+{
+	struct gpio_chip  *gc   = irq_data_get_irq_chip_data(d);
+	struct pt27_gpio  *chip = gpiochip_get_data(gc);
+	irq_hw_number_t   hwirq = irqd_to_hwirq(d);
+	unsigned int      trig  = irqd_get_trigger_type(d);
+	u16               wake_en, wake_lvl, wake_mode;
+
+	if (hwirq >= PT27_WAKE_GPIOS) {
+		dev_dbg(chip->dev, "%s: GPIO%lu does not support wake (max GPIO%u)\n", __func__, hwirq, PT27_WAKE_GPIOS - 1);
+		return -EINVAL;
+	}
+
+	if (on && trig == IRQ_TYPE_NONE)
+		return -EINVAL;
+
+	guard(raw_spinlock_irqsave)(&chip->lock);
+
+	wake_en   = ioread16(chip->base + PT27_REG_WAKE_EN);
+	wake_lvl  = ioread16(chip->base + PT27_REG_WAKE_LVL);
+	wake_mode = ioread16(chip->base + PT27_REG_WAKE_MODE);
+
+	if (on) {
+		wake_en |= BIT(hwirq);
+		chip->wake_mask |= (u16)BIT(hwirq);
+
+		if (trig & IRQ_TYPE_LEVEL_MASK) {
+			wake_mode |= BIT(hwirq);
+			if (trig == IRQ_TYPE_LEVEL_HIGH)
+				wake_lvl |= BIT(hwirq);
+			else
+				wake_lvl &= ~BIT(hwirq);
+		} else {
+			wake_mode &= ~BIT(hwirq);
+			if (trig == IRQ_TYPE_EDGE_RISING ||
+			    trig == IRQ_TYPE_EDGE_BOTH)
+				wake_lvl |= BIT(hwirq);
+			else
+				wake_lvl &= ~BIT(hwirq);
+		}
+	} else {
+		wake_en &= ~BIT(hwirq);
+		chip->wake_mask &= ~(u16)BIT(hwirq);
+	}
+
+	iowrite16(wake_lvl,  chip->base + PT27_REG_WAKE_LVL);
+	iowrite16(wake_mode, chip->base + PT27_REG_WAKE_MODE);
+	iowrite16(wake_en,   chip->base + PT27_REG_WAKE_EN);
+
+	return 0;
+}
+
+static const struct irq_chip pt27_gpio_irq_chip = {
+	.name		= "pt27_gpio",
+	.irq_ack	= pt27_irq_ack,
+	.irq_mask	= pt27_irq_mask,
+	.irq_unmask	= pt27_irq_unmask,
+	.irq_set_type	= pt27_irq_set_type,
+	.irq_set_wake	= pt27_irq_set_wake,
+	.irq_request_resources	= pt27_irq_reqres,
+	.irq_release_resources	= pt27_irq_relres,
+	.flags	= IRQCHIP_IMMUTABLE,
+};
+
+static irqreturn_t pt27_gpio_isr(int irq, void *data)
+{
+	struct pt27_gpio *chip = data;
+	unsigned long pending;
+	u32 stat, en, mask;
+
+	scoped_guard(raw_spinlock_irqsave, &chip->lock) {
+
+	  stat = ioread32(chip->base + PT27_REG_INT_STAT);
+	  en   = ioread32(chip->base + PT27_REG_INT_EN);
+	  mask = ioread32(chip->base + PT27_REG_INT_MASK);
+
+	  pending =  stat & en & ~mask & PT27_GPIO_MASK;
+	}
+
+	if (!pending)
+		return IRQ_NONE;
+
+	while (pending) {
+		unsigned int hwirq = __ffs(pending);
+
+		pending &= ~BIT(hwirq);
+		if (generic_handle_domain_irq(chip->gen.gc.irq.domain, hwirq)) {
+
+			dev_warn_ratelimited(chip->dev, "unhandled GPIO%u IRQ, masking\n", hwirq);
+
+			guard(raw_spinlock_irqsave)(&chip->lock);
+			iowrite32(ioread32(chip->base + PT27_REG_INT_MASK) | BIT(hwirq), chip->base + PT27_REG_INT_MASK);
+			iowrite32(ioread32(chip->base + PT27_REG_INT_EN) & ~BIT(hwirq), chip->base + PT27_REG_INT_EN);
+			iowrite32(BIT(hwirq), chip->base + PT27_REG_INT_STAT);
+		}
+	}
+	return IRQ_HANDLED;
+}
+
+static void pt27_hw_clear(void *data)
+{
+	struct pt27_gpio *chip = data;
+
+	iowrite32(0, chip->base + PT27_REG_INT_EN);
+	iowrite32(PT27_GPIO_MASK, chip->base + PT27_REG_INT_MASK);
+	iowrite32(PT27_GPIO_MASK, chip->base + PT27_REG_INT_STAT);
+	iowrite16(0, chip->base + PT27_REG_WAKE_EN);
+	iowrite16(PT27_WAKE_MASK, chip->base + PT27_REG_WAKE_STAT);
+}
+
+static void pt27_hw_init(struct pt27_gpio *chip)
+{
+	u8 int_ctl;
+
+	int_ctl  = ioread8(chip->base + PT27_REG_INT_CTL);
+	int_ctl |=  PT27_INT_CTL_MODE;
+	int_ctl &= ~PT27_INT_CTL_PAD_EN;
+	iowrite8(int_ctl, chip->base + PT27_REG_INT_CTL);
+
+	pt27_hw_clear(chip);
+}
+
+static int pt27_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_generic_chip_config config = { };
+	struct pt27_gpio *chip;
+	struct gpio_chip *gc;
+	struct gpio_irq_chip *girq;
+	unsigned long irq_flags;
+	int ret;
+
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->dev = dev;
+	raw_spin_lock_init(&chip->lock);
+	pci_set_drvdata(pdev, chip);
+
+	ret = pcim_enable_device(pdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable PCI device\n");
+
+	chip->base = pcim_iomap_region(pdev, 0, "pt27_gpio");
+	if (IS_ERR(chip->base))
+		return dev_err_probe(dev, PTR_ERR(chip->base), "failed to claim and map BAR0\n");
+
+	pci_set_master(pdev);
+
+	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to allocate IRQ vector\n");
+
+	chip->use_msi = pdev->msi_enabled || pdev->msix_enabled;
+	irq_flags     = chip->use_msi ? 0 : IRQF_SHARED;
+	chip->irq     = pci_irq_vector(pdev, 0);
+
+	pt27_hw_init(chip);
+
+	ret = devm_add_action_or_reset(dev, pt27_hw_clear, chip);
+	if (ret)
+		return ret;
+
+	config.dev    = dev;
+	config.sz     = 4;
+	config.dat    = chip->base + PT27_REG_IN;
+	config.set    = chip->base + PT27_REG_OUT;
+	config.dirout = chip->base + PT27_REG_DIR;
+	config.flags  = GPIO_GENERIC_READ_OUTPUT_REG_SET;
+
+	ret = gpio_generic_chip_init(&chip->gen, &config);
+	if (ret)
+		return dev_err_probe(dev, ret, "generic chip init failed\n");
+
+	gc = &chip->gen.gc;
+	gc->label     = dev_name(dev);
+	gc->parent    = dev;
+	gc->owner     = THIS_MODULE;
+	gc->base      = -1;
+	gc->ngpio     = PT27_NUM_GPIOS;
+	gc->request   = pt27_gpio_request;
+	gc->free      = pt27_gpio_free;
+
+	girq = &gc->irq;
+	gpio_irq_chip_set_chip(girq, &pt27_gpio_irq_chip);
+	girq->parent_handler = NULL;
+	girq->num_parents    = 0;
+	girq->parents        = NULL;
+	girq->default_type   = IRQ_TYPE_NONE;
+	girq->handler        = handle_bad_irq;
+
+	ret = devm_device_init_wakeup(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable wake capability\n");
+
+	dev_pm_set_driver_flags(dev, DPM_FLAG_SMART_SUSPEND);
+
+	pm_runtime_set_autosuspend_delay(dev, PT27_AUTOSUSPEND_DELAY_MS);
+	pm_runtime_use_autosuspend(dev);
+
+	ret = devm_gpiochip_add_data(dev, gc, chip);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register GPIO chip\n");
+
+	ret = devm_request_irq(dev, chip->irq, pt27_gpio_isr,
+			       irq_flags | IRQF_NO_THREAD, dev_name(dev), chip);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to request IRQ %d\n", chip->irq);
+
+	dev_info(dev,
+		 "PT27 GPIO controller: %u GPIOs (%u wake-capable), IRQ=%d (%s)\n",
+		 PT27_NUM_GPIOS, PT27_WAKE_GPIOS, chip->irq,
+		 chip->use_msi ? (pdev->msix_enabled ? "MSI-X" : "MSI") : "INTx");
+
+	pm_runtime_put_noidle(dev);
+	pm_runtime_allow(dev);
+
+	return 0;
+}
+
+static void pt27_gpio_remove(struct pci_dev *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	pm_runtime_forbid(dev);
+	pm_runtime_get_noresume(dev);
+	pm_runtime_dont_use_autosuspend(dev);
+}
+
+static int __maybe_unused pt27_gpio_suspend(struct device *dev)
+{
+	struct pt27_gpio *chip = dev_get_drvdata(dev);
+
+	if (pm_runtime_status_suspended(dev)) {
+		dev_dbg(dev, "%s: already runtime-suspended, skipping save\n", __func__);
+		return 0;
+	}
+
+	scoped_guard(raw_spinlock_irqsave, &chip->lock) {
+	  pt27_save_regs(chip);
+	  iowrite32(0, chip->base + PT27_REG_INT_EN);
+	  iowrite32(PT27_GPIO_MASK, chip->base + PT27_REG_INT_MASK);
+	  iowrite32(PT27_GPIO_MASK, chip->base + PT27_REG_INT_STAT);
+	  iowrite16(PT27_WAKE_MASK, chip->base + PT27_REG_WAKE_STAT);
+	}
+
+	synchronize_irq(chip->irq);
+	return 0;
+}
+
+static int __maybe_unused pt27_gpio_resume(struct device *dev)
+{
+	struct pt27_gpio *chip = dev_get_drvdata(dev);
+	u16 wake_status;
+
+	if (pm_runtime_status_suspended(dev))
+		return 0;
+
+	scoped_guard(raw_spinlock_irqsave, &chip->lock) {
+	  wake_status = ioread16(chip->base + PT27_REG_WAKE_STAT) & PT27_WAKE_MASK;
+	  iowrite16(wake_status, chip->base + PT27_REG_WAKE_STAT);
+	  pt27_restore_regs(chip);
+	}
+
+	if (wake_status)
+		pm_wakeup_event(dev, 0);
+
+	return 0;
+}
+
+static int __maybe_unused pt27_gpio_runtime_suspend(struct device *dev)
+{
+	struct pt27_gpio *chip = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (chip->wake_mask && !device_may_wakeup(dev)) {
+		ret = -EBUSY;
+		break;
+	}
+
+	scoped_guard(raw_spinlock_irqsave, &chip->lock) {
+
+	  pt27_save_regs(chip);
+	  iowrite32(0, chip->base + PT27_REG_INT_EN);
+	  iowrite32(PT27_GPIO_MASK, chip->base + PT27_REG_INT_MASK);
+	  iowrite32(PT27_GPIO_MASK, chip->base + PT27_REG_INT_STAT);
+	  iowrite16(PT27_WAKE_MASK, chip->base + PT27_REG_WAKE_STAT);
+	}
+
+	if (!ret)
+		synchronize_irq(chip->irq);
+
+	return ret;
+}
+
+static int __maybe_unused pt27_gpio_runtime_resume(struct device *dev)
+{
+	return pt27_gpio_resume(dev);
+}
+
+static const struct dev_pm_ops pt27_gpio_pm_ops __maybe_unused = {
+	SYSTEM_SLEEP_PM_OPS(pt27_gpio_suspend, pt27_gpio_resume)
+	RUNTIME_PM_OPS(pt27_gpio_runtime_suspend, pt27_gpio_runtime_resume, NULL)
+};
+
+#define PCI_DEVICE_ID_PT27	0x444b
+
+static const struct pci_device_id pt27_gpio_pci_ids[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_PT27) },
+	{ }
+};
+MODULE_DEVICE_TABLE(pci, pt27_gpio_pci_ids);
+
+static struct pci_driver pt27_gpio_pci_driver = {
+	.name     = "ptpci-gpio",
+	.id_table = pt27_gpio_pci_ids,
+	.probe    = pt27_gpio_probe,
+	.remove   = pt27_gpio_remove,
+	.driver   = {
+		.pm = pm_ptr(&pt27_gpio_pm_ops),
+	},
+};
+
+module_pci_driver(pt27_gpio_pci_driver);
+
+MODULE_DESCRIPTION("AMD Promontory PCI GPIO Driver");
+MODULE_LICENSE("GPL");
--
2.48.1

==================================================================================================================
This email and any attachments to it contain confidential information and are intended solely for the use of the individual to whom it 
is addressed.If you are not the intended recipient or receive it accidentally, please immediately notify the sender by e-mail and delete 
the message and any attachments from your computer system, and destroy all hard copies. If any, please be advised that any unauthorized 
disclosure, copying, distribution or any action taken or omitted in reliance on this, is illegal and prohibited. Furthermore, any views 
or opinions expressed are solely those of the author and do not represent those of ASMedia Technology Inc. Thank you for your cooperation.
==================================================================================================================

Re: [PATCH v3] gpio: amdptpci: add support for AMD PT PCI controller

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] gpio: amdptpci: missing runtime PM reference in pt27_irq_reqres()
- [High] gpio: amdptpci: MMIO access while runtime-suspended in pt27_gpio_isr()
- [High] gpio: amdptpci: pt27_irq_set_wake() accesses MMIO while runtime-suspended
- [Medium] gpio: amdptpci: invalid break and unlocked wake_mask in runtime suspend
- [Medium] gpio: amdptpci: fix devres ordering and PM cleanup in pt27_gpio_probe()
- [Medium] gpio: amdptpci: race on DIR and OUT registers due to wrong lock in PM callbacks
- [Medium] gpio: amdptpci: missing wake IRQ dispatch in pt27_gpio_resume() and wake_en gate
- [Low] gpio: amdptpci: missing space in struct pt27_gpio gen member declaration
- [Low] gpio: amdptpci: missing select GPIO_GENERIC and GPIOLIB_IRQCHIP in Kconfig
--

commit aad2d02577183448f2ddceeabe365624f8d6d152
Author: Szuying Chen <Chloe_Chen@asmedia.com.tw>

gpio: amdptpci: add support for AMD PT PCI controller

This commit adds the gpio-amdptpci driver to support the GPIO controller,
interrupt handling, and wakeup capabilities on AMD Promontory PCI
platforms.

> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 55e4280029d86..bcfb653a09804 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1890,6 +1890,12 @@ config GPIO_TC9563
>  menu "PCI GPIO expanders"
>  	depends on PCI
>  
> +config GPIO_AMDPTPCI
> +	tristate "AMD Promontory PCI GPIO support"
> +	depends on X86
> +	help
> +	  Driver for Promontory IOHub PCI GPIO functionality.

[Severity: Low]
Should config GPIO_AMDPTPCI select GPIO_GENERIC and GPIOLIB_IRQCHIP?

In drivers/gpio/gpio-amdptpci.c, pt27_gpio_probe() unconditionally calls
gpio_generic_chip_init() (which requires CONFIG_GPIO_GENERIC) and sets up
gc->irq via gpio_irq_chip_set_chip() while the IRQ callbacks use
gpiochip_reqres_irq(), gpiochip_relres_irq(), gpiochip_enable_irq(), and
gpiochip_disable_irq() (which require CONFIG_GPIOLIB_IRQCHIP). Without
selecting those symbols, builds where no other enabled driver selects them
will fail at compile or link time.

> diff --git a/drivers/gpio/gpio-amdptpci.c b/drivers/gpio/gpio-amdptpci.c
> new file mode 100644
> index 0000000000000..171c6811a33e1
> --- /dev/null
> +++ b/drivers/gpio/gpio-amdptpci.c

[ ... ]

> +struct pt27_gpio {
> +struct device                     *dev;
> +struct gpio_generic_chipgen;
> +	void __iomem              *base;
> +	struct pt27_saved_regs	  saved;
> +	raw_spinlock_t            lock;
> +	u16                       wake_mask;
> +	bool                      use_msi;
> +	int                       irq;
> +};

[Severity: Low]
There is a missing space between struct gpio_generic_chip and gen in
struct pt27_gpio, which causes compilation to fail when pt27_gpio_isr()
and pt27_gpio_probe() reference chip->gen.

> +
> +static void pt27_save_regs(struct pt27_gpio *chip)
> +{
> +	chip->saved.dir       = ioread32(chip->base + PT27_REG_DIR);
> +	chip->saved.out       = ioread32(chip->base + PT27_REG_OUT);

[ ... ]

> +static void pt27_restore_regs(struct pt27_gpio *chip)
> +{
> +	iowrite32(chip->saved.dir,       chip->base + PT27_REG_DIR);
> +	iowrite32(chip->saved.out,       chip->base + PT27_REG_OUT);

[Severity: Medium]
Can saving and restoring PT27_REG_DIR and PT27_REG_OUT under chip->lock
race with gpio-mmio operations and desynchronize the shadow registers?

In pt27_gpio_probe(), gpio_generic_chip_init() configures gpio-mmio to
manage PT27_REG_OUT and PT27_REG_DIR using chip->gen.lock as well as the
shadow state in chip->gen.sdata and chip->gen.sdir. However,
pt27_gpio_suspend(), pt27_gpio_runtime_suspend(), and pt27_gpio_resume()
call pt27_save_regs() and pt27_restore_regs() while holding only
chip->lock instead of chip->gen.lock (via gpio_generic_lock_irqsave()).

Because gc->can_sleep is false and the system sleep callbacks run during
the normal suspend/resume phase, concurrent atomic GPIO consumers calling
gpiod_set_value() or gpiod_direction_output() are not serialized against
pt27_save_regs() and pt27_restore_regs(), which can overwrite register
updates on resume and leave the hardware out of sync with chip->gen.sdata
and chip->gen.sdir.

[ ... ]

> +static int pt27_irq_reqres(struct irq_data *d)
> +{
> +	struct gpio_chip *gc   = irq_data_get_irq_chip_data(d);
> +	irq_hw_number_t   hwirq = irqd_to_hwirq(d);
> +	int ret;
> +
> +	ret = gpiochip_reqres_irq(gc, hwirq);
> +	return ret;
> +}
> +
> +static void pt27_irq_relres(struct irq_data *d)
> +{
> +	struct gpio_chip *gc   = irq_data_get_irq_chip_data(d);
> +	irq_hw_number_t   hwirq = irqd_to_hwirq(d);
> +
> +	gpiochip_relres_irq(gc, hwirq);
> +}

[Severity: High]
Should pt27_irq_reqres() and pt27_irq_relres() also manage runtime PM
references for the device?

Currently, pm_runtime_resume_and_get() and pm_runtime_put_autosuspend()
are only called in pt27_gpio_request() and pt27_gpio_free(). When a
consumer requests a GPIO interrupt directly through the irq_domain (for
example via request_irq() or ACPI GpioInt) without calling gpiod_request(),
gpiochip_reqres_irq() does not invoke gc->request().

If the controller has already runtime-suspended into PCI D3hot after the
2000 ms autosuspend delay, gpiochip_reqres_irq() -> gpiochip_lock_as_irq()
reads PT27_REG_DIR while in D3, gets 0xffffffff, treats the pin as an
output, and fails with -EIO. Furthermore, even if the device is in D0 when
the interrupt is requested, nothing holds a runtime PM reference, so the
device will autosuspend after 2000 ms and pt27_gpio_runtime_suspend() will
clear PT27_REG_INT_EN and mask all interrupts in PT27_REG_INT_MASK while
the IRQ is active.

[ ... ]

> +static int pt27_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> +	struct gpio_chip  *gc   = irq_data_get_irq_chip_data(d);
> +	struct pt27_gpio  *chip = gpiochip_get_data(gc);
> +	irq_hw_number_t   hwirq = irqd_to_hwirq(d);
> +	unsigned int      trig  = irqd_get_trigger_type(d);
> +	u16               wake_en, wake_lvl, wake_mode;
> +
> +	if (hwirq >= PT27_WAKE_GPIOS) {
> +		dev_dbg(chip->dev, "%s: GPIO%lu does not support wake (max GPIO%u)\n", __func__, hwirq, PT27_WAKE_GPIOS - 1);
> +		return -EINVAL;
> +	}
> +
> +	if (on && trig == IRQ_TYPE_NONE)
> +		return -EINVAL;
> +
> +	guard(raw_spinlock_irqsave)(&chip->lock);
> +
> +	wake_en   = ioread16(chip->base + PT27_REG_WAKE_EN);
> +	wake_lvl  = ioread16(chip->base + PT27_REG_WAKE_LVL);
> +	wake_mode = ioread16(chip->base + PT27_REG_WAKE_MODE);

[Severity: High]
What happens when pt27_irq_set_wake() is called while the controller is
runtime-suspended, or when the device later resumes?

When a consumer calls enable_irq_wake() during system suspend (or
disable_irq_wake() during system resume) while the GPIO controller is
runtime-suspended in PCI D3, pt27_irq_set_wake() performs ioread16() and
iowrite16() directly on PT27_REG_WAKE_EN, PT27_REG_WAKE_LVL, and
PT27_REG_WAKE_MODE without ensuring the PCI device is in D0 and without
updating chip->saved.wake_en, chip->saved.wake_lvl, and
chip->saved.wake_mode.

In addition to the MMIO accesses failing in D3, when pt27_gpio_resume() or
pt27_gpio_runtime_resume() subsequently calls pt27_restore_regs(), it
writes the stale chip->saved.wake_* values back to the hardware registers,
overwriting any wake configuration changes made after pt27_save_regs().

[ ... ]

> +static irqreturn_t pt27_gpio_isr(int irq, void *data)
> +{
> +	struct pt27_gpio *chip = data;
> +	unsigned long pending;
> +	u32 stat, en, mask;
> +
> +	scoped_guard(raw_spinlock_irqsave, &chip->lock) {
> +
> +	  stat = ioread32(chip->base + PT27_REG_INT_STAT);
> +	  en   = ioread32(chip->base + PT27_REG_INT_EN);
> +	  mask = ioread32(chip->base + PT27_REG_INT_MASK);
> +
> +	  pending =  stat & en & ~mask & PT27_GPIO_MASK;
> +	}

[Severity: High]
When MSI/MSI-X is not used, pt27_gpio_probe() requests the interrupt with
IRQF_SHARED. If the PT27 controller is runtime-suspended in PCI D3 and
another device on the shared INTx line asserts an interrupt, won't
pt27_gpio_isr() read BAR0 MMIO registers while the device is powered down?

Should pt27_gpio_isr() check whether the device is runtime-active (for
example with pm_runtime_get_if_active()) before accessing hardware
registers?

[ ... ]

> +static int pt27_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{

[ ... ]

> +	pt27_hw_init(chip);
> +
> +	ret = devm_add_action_or_reset(dev, pt27_hw_clear, chip);
> +	if (ret)
> +		return ret;

[ ... ]

> +	pm_runtime_set_autosuspend_delay(dev, PT27_AUTOSUSPEND_DELAY_MS);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	ret = devm_gpiochip_add_data(dev, gc, chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register GPIO chip\n");
> +
> +	ret = devm_request_irq(dev, chip->irq, pt27_gpio_isr,
> +			       irq_flags | IRQF_NO_THREAD, dev_name(dev), chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to request IRQ %d\n", chip->irq);

[Severity: Medium]
Is the devres registration order and runtime PM error cleanup in
pt27_gpio_probe() safe during probe and unbind?

First, devm_gpiochip_add_data() registers the GPIO chip and IRQ domain
before devm_request_irq() installs pt27_gpio_isr(). Consumers (including
acpi_gpiochip_request_interrupts() inside gpiochip_add_irqchip()) can
request and unmask GPIO interrupts before the parent IRQ handler is
registered. On driver unbind, reverse devres ordering frees chip->irq
before removing the gpiochip and before running pt27_hw_clear(), leaving a
window where hardware interrupts remain unmasked after the IRQ handler has
been removed.

Second, if devm_gpiochip_add_data() or devm_request_irq() fails after
pm_runtime_use_autosuspend(dev), pt27_gpio_probe() returns without calling
pm_runtime_dont_use_autosuspend(dev), leaving use_autosuspend enabled on
the PCI device.

[ ... ]

> +static int __maybe_unused pt27_gpio_suspend(struct device *dev)
> +{
> +	struct pt27_gpio *chip = dev_get_drvdata(dev);
> +
> +	if (pm_runtime_status_suspended(dev)) {
> +		dev_dbg(dev, "%s: already runtime-suspended, skipping save\n", __func__);
> +		return 0;
> +	}
> +
> +	scoped_guard(raw_spinlock_irqsave, &chip->lock) {
> +	  pt27_save_regs(chip);
> +	  iowrite32(0, chip->base + PT27_REG_INT_EN);
> +	  iowrite32(PT27_GPIO_MASK, chip->base + PT27_REG_INT_MASK);
> +	  iowrite32(PT27_GPIO_MASK, chip->base + PT27_REG_INT_STAT);
> +	  iowrite16(PT27_WAKE_MASK, chip->base + PT27_REG_WAKE_STAT);
> +	}
> +
> +	synchronize_irq(chip->irq);
> +	return 0;
> +}
> +
> +static int __maybe_unused pt27_gpio_resume(struct device *dev)
> +{
> +	struct pt27_gpio *chip = dev_get_drvdata(dev);
> +	u16 wake_status;
> +
> +	if (pm_runtime_status_suspended(dev))
> +		return 0;
> +
> +	scoped_guard(raw_spinlock_irqsave, &chip->lock) {
> +	  wake_status = ioread16(chip->base + PT27_REG_WAKE_STAT) & PT27_WAKE_MASK;
> +	  iowrite16(wake_status, chip->base + PT27_REG_WAKE_STAT);
> +	  pt27_restore_regs(chip);
> +	}
> +
> +	if (wake_status)
> +		pm_wakeup_event(dev, 0);
> +
> +	return 0;
> +}

[Severity: Medium]
When a wake-enabled GPIO interrupt fires while the controller is suspended,
how does the consumer's interrupt handler get invoked on resume?

In pt27_gpio_suspend() and pt27_gpio_runtime_suspend(), PT27_REG_INT_EN is
cleared to 0 and PT27_REG_INT_MASK is set to PT27_GPIO_MASK, so wake events
occurring during suspend are only latched in PT27_REG_WAKE_STAT. In
pt27_gpio_resume(), wake_status is read and cleared from
PT27_REG_WAKE_STAT, and pm_wakeup_event(dev, 0) is called, but
generic_handle_domain_irq() is never called for the set bits in
wake_status, so edge-triggered wakeup interrupts are dropped.

Also, in pt27_gpio_suspend(), if !device_may_wakeup(dev) (for example when
wakeup is disabled via sysfs), PT27_REG_WAKE_EN is left enabled in hardware
rather than being cleared before entering system sleep.

> +
> +static int __maybe_unused pt27_gpio_runtime_suspend(struct device *dev)
> +{
> +	struct pt27_gpio *chip = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	if (chip->wake_mask && !device_may_wakeup(dev)) {
> +		ret = -EBUSY;
> +		break;
> +	}
> +
> +	scoped_guard(raw_spinlock_irqsave, &chip->lock) {
> +
> +	  pt27_save_regs(chip);
> +	  iowrite32(0, chip->base + PT27_REG_INT_EN);
> +	  iowrite32(PT27_GPIO_MASK, chip->base + PT27_REG_INT_MASK);
> +	  iowrite32(PT27_GPIO_MASK, chip->base + PT27_REG_INT_STAT);
> +	  iowrite16(PT27_WAKE_MASK, chip->base + PT27_REG_WAKE_STAT);
> +	}

[Severity: Medium]
In pt27_gpio_runtime_suspend(), the break statement is not inside a loop or
switch statement, which causes a build error when CONFIG_PM is enabled:

  error: break statement not within loop or switch

Should this check be performed inside scoped_guard(raw_spinlock_irqsave,
&chip->lock) (since pt27_irq_set_wake() updates chip->wake_mask under
chip->lock) and return -EBUSY directly so it does not fall through and
disable PT27_REG_INT_EN when aborting runtime suspend?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028084531.6237-1-Chloe_Chen@asmedia.com.tw?part=1