* [PATCH] gpio: amdptpci: add support for AMD PT PCI controller
@ 2026-10-05 3:54 Szuying Chen
2026-10-05 4:06 ` sashiko-bot
2026-10-07 11:15 ` Linus Walleij
0 siblings, 2 replies; 3+ messages in thread
From: Szuying Chen @ 2026-10-05 3:54 UTC (permalink / raw)
To: linusw, brgl, linux-kernel, linux-gpio
Cc: Yd_Tseng, Jesse1_Chang, Richard_Hsu, Chloe_Chen
This commit implements the new gpio-amdptpci driver to support the
GPIO functionality on AMD PT PCI platforms.
Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
Co-authored-by: YD Tseng <Yd_Tseng@asmedia.com.tw>
---
drivers/gpio/Kconfig | 6 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-amdptpci.c | 891 +++++++++++++++++++++++++++++++++++
3 files changed, 898 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..ac2a7b46c3cd
--- /dev/null
+++ b/drivers/gpio/gpio-amdptpci.c
@@ -0,0 +1,891 @@
+// 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/interrupt.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.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_INT_CTL_MODE BIT(0)
+#define PT27_INT_CTL_LVL BIT(1)
+#define PT27_INT_CTL_PAD_EN BIT(2)
+
+#define PT27_REG_VENDOR0 0x0028
+#define PT27_REG_VENDOR1 0x002C
+
+#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_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;
+ u32 vendor0;
+ u32 vendor1;
+ u16 wake_en;
+ u16 wake_lvl;
+ u16 wake_mode;
+};
+
+struct pt27_gpio {
+ struct device *dev;
+ struct gpio_chip gc;
+ void __iomem *base;
+ struct pt27_saved_regs saved;
+ bool saved_valid;
+ raw_spinlock_t lock;
+ struct mutex irq_bus_lock;
+ bool irq_bus_active;
+ u32 enabled_irq_mask;
+ bool irq_pm_held;
+ u16 wake_mask;
+ bool use_msi;
+ int irq;
+};
+
+static inline u32 pt27_rd32(struct pt27_gpio *c, u32 off)
+{
+ return ioread32(c->base + off);
+}
+
+static inline void pt27_wr32(struct pt27_gpio *c, u32 off, u32 v)
+{
+ iowrite32(v, c->base + off);
+}
+
+static inline u16 pt27_rd16(struct pt27_gpio *c, u32 off)
+{
+ return ioread16(c->base + off);
+}
+
+static inline void pt27_wr16(struct pt27_gpio *c, u32 off, u16 v)
+{
+ iowrite16(v, c->base + off);
+}
+
+static inline u8 pt27_rd8(struct pt27_gpio *c, u32 off)
+{
+ return ioread8(c->base + off);
+}
+
+static inline void pt27_wr8(struct pt27_gpio *c, u32 off, u8 v)
+{
+ iowrite8(v, c->base + off);
+}
+
+static void pt27_runtime_put_autosuspend(struct pt27_gpio *chip)
+{
+ pm_runtime_mark_last_busy(chip->dev);
+ pm_runtime_put_autosuspend(chip->dev);
+
+}
+
+static void pt27_save_regs(struct pt27_gpio *chip)
+{
+ chip->saved.dir = pt27_rd32(chip, PT27_REG_DIR);
+ chip->saved.out = pt27_rd32(chip, PT27_REG_OUT);
+ chip->saved.int_en = pt27_rd32(chip, PT27_REG_INT_EN);
+ chip->saved.int_lvl = pt27_rd32(chip, PT27_REG_INT_LVL);
+ chip->saved.int_mode0 = pt27_rd32(chip, PT27_REG_INT_MODE0);
+ chip->saved.int_mode1 = pt27_rd16(chip, PT27_REG_INT_MODE1);
+ chip->saved.int_mask = pt27_rd32(chip, PT27_REG_INT_MASK);
+ chip->saved.int_ctl = pt27_rd8(chip, PT27_REG_INT_CTL);
+ chip->saved.vendor0 = pt27_rd32(chip, PT27_REG_VENDOR0);
+ chip->saved.vendor1 = pt27_rd32(chip, PT27_REG_VENDOR1);
+ chip->saved.wake_en = pt27_rd16(chip, PT27_REG_WAKE_EN);
+ chip->saved.wake_lvl = pt27_rd16(chip, PT27_REG_WAKE_LVL);
+ chip->saved.wake_mode = pt27_rd16(chip, PT27_REG_WAKE_MODE);
+ chip->saved_valid = true;
+
+}
+
+static void pt27_restore_regs(struct pt27_gpio *chip)
+{
+ pt27_wr32(chip, PT27_REG_INT_EN, 0);
+ pt27_wr32(chip, PT27_REG_INT_MASK, PT27_GPIO_MASK);
+
+ pt27_wr32(chip, PT27_REG_DIR, chip->saved.dir);
+ pt27_wr32(chip, PT27_REG_OUT, chip->saved.out);
+ pt27_wr32(chip, PT27_REG_INT_LVL, chip->saved.int_lvl);
+ pt27_wr32(chip, PT27_REG_INT_MODE0, chip->saved.int_mode0);
+ pt27_wr16(chip, PT27_REG_INT_MODE1, chip->saved.int_mode1);
+ pt27_wr8(chip, PT27_REG_INT_CTL, chip->saved.int_ctl);
+ pt27_wr32(chip, PT27_REG_VENDOR0, chip->saved.vendor0);
+ pt27_wr32(chip, PT27_REG_VENDOR1, chip->saved.vendor1);
+
+ pt27_wr16(chip, PT27_REG_WAKE_LVL, chip->saved.wake_lvl);
+ pt27_wr16(chip, PT27_REG_WAKE_MODE, chip->saved.wake_mode);
+ pt27_wr16(chip, PT27_REG_WAKE_EN, chip->saved.wake_en);
+
+ pt27_wr32(chip, PT27_REG_INT_STAT, PT27_GPIO_MASK);
+ pt27_wr16(chip, PT27_REG_WAKE_STAT, PT27_WAKE_MASK);
+
+ pt27_wr32(chip, PT27_REG_INT_EN, chip->saved.int_en);
+ pt27_wr32(chip, PT27_REG_INT_MASK, chip->saved.int_mask);
+
+}
+
+static int pt27_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+ struct pt27_gpio *chip = gpiochip_get_data(gc);
+ unsigned long flags;
+ u32 val;
+ int ret;
+
+ ret = pm_runtime_resume_and_get(chip->dev);
+ if (ret < 0)
+ return ret;
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+ val = pt27_rd32(chip, PT27_REG_DIR);
+ val &= ~BIT(offset); /* 0 = input */
+ pt27_wr32(chip, PT27_REG_DIR, val);
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+
+ pt27_runtime_put_autosuspend(chip);
+ return 0;
+}
+
+static int pt27_gpio_direction_output(struct gpio_chip *gc, unsigned int offset,
+ int value)
+{
+ struct pt27_gpio *chip = gpiochip_get_data(gc);
+ unsigned long flags;
+ u32 val;
+ int ret;
+
+ ret = pm_runtime_resume_and_get(chip->dev);
+ if (ret < 0)
+ return ret;
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+
+ val = pt27_rd32(chip, PT27_REG_OUT);
+ if (value)
+ val |= BIT(offset);
+ else
+ val &= ~BIT(offset);
+ pt27_wr32(chip, PT27_REG_OUT, val);
+
+ val = pt27_rd32(chip, PT27_REG_DIR);
+ val |= BIT(offset);
+ pt27_wr32(chip, PT27_REG_DIR, val);
+
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+
+ pt27_runtime_put_autosuspend(chip);
+ return 0;
+}
+
+static int pt27_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+ struct pt27_gpio *chip = gpiochip_get_data(gc);
+ unsigned long flags;
+ u32 val;
+ int ret;
+
+ ret = pm_runtime_resume_and_get(chip->dev);
+ if (ret < 0)
+ return ret;
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+ val = pt27_rd32(chip, PT27_REG_DIR);
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+
+ pt27_runtime_put_autosuspend(chip);
+
+ return (val & BIT(offset)) ? GPIO_LINE_DIRECTION_OUT
+ : GPIO_LINE_DIRECTION_IN;
+}
+
+static int pt27_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+ struct pt27_gpio *chip = gpiochip_get_data(gc);
+ unsigned long flags;
+ u32 dir, val;
+ int ret;
+
+ ret = pm_runtime_resume_and_get(chip->dev);
+ if (ret < 0)
+ return ret;
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+ dir = pt27_rd32(chip, PT27_REG_DIR);
+ val = pt27_rd32(chip, (dir & BIT(offset)) ? PT27_REG_OUT : PT27_REG_IN);
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+
+ pt27_runtime_put_autosuspend(chip);
+
+ return !!(val & BIT(offset));
+}
+
+static int pt27_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+ struct pt27_gpio *chip = gpiochip_get_data(gc);
+ unsigned long flags;
+ u32 val;
+ int ret;
+
+ ret = pm_runtime_resume_and_get(chip->dev);
+ if (ret < 0) {
+ dev_err_ratelimited(chip->dev,
+ "%s: cannot resume to set GPIO%u: %d\n",
+ __func__, offset, ret);
+ return ret;
+ }
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+ val = pt27_rd32(chip, PT27_REG_OUT);
+ if (value)
+ val |= BIT(offset);
+ else
+ val &= ~BIT(offset);
+ pt27_wr32(chip, PT27_REG_OUT, val);
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+
+ pt27_runtime_put_autosuspend(chip);
+ return 0;
+}
+
+static u32 pt27_pending_irqs_locked(struct pt27_gpio *chip)
+{
+ u32 stat = pt27_rd32(chip, PT27_REG_INT_STAT);
+ u32 en = pt27_rd32(chip, PT27_REG_INT_EN);
+ u32 mask = pt27_rd32(chip, PT27_REG_INT_MASK);
+
+ return stat & en & ~mask & PT27_GPIO_MASK;
+}
+
+static bool pt27_irq_regs_accessible(struct pt27_gpio *chip)
+{
+
+ return READ_ONCE(chip->irq_bus_active) ||
+ READ_ONCE(chip->irq_pm_held);
+}
+
+static void pt27_irq_take_pm_hold(struct pt27_gpio *chip)
+{
+ unsigned long flags;
+ bool take = false;
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+ if (!chip->irq_pm_held) {
+ chip->irq_pm_held = true;
+ take = true;
+ }
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+
+ if (take)
+ pm_runtime_get_noresume(chip->dev);
+}
+
+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);
+ unsigned long flags;
+ u32 val;
+ bool drop_pm = false;
+
+ gpiochip_disable_irq(gc, hwirq);
+
+ if (!pt27_irq_regs_accessible(chip)) {
+ dev_err_ratelimited(chip->dev,
+ "%s: GPIO%lu remains masked; PT27 is not in D0\n",
+ __func__, hwirq);
+ return;
+ }
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+
+ val = pt27_rd32(chip, PT27_REG_INT_MASK);
+ val |= BIT(hwirq);
+ pt27_wr32(chip, PT27_REG_INT_MASK, val);
+
+ val = pt27_rd32(chip, PT27_REG_INT_EN);
+ val &= ~BIT(hwirq);
+ pt27_wr32(chip, PT27_REG_INT_EN, val);
+
+ chip->enabled_irq_mask &= ~BIT(hwirq);
+ if (!chip->enabled_irq_mask && chip->irq_pm_held) {
+ chip->irq_pm_held = false;
+ drop_pm = true;
+ }
+
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+
+ if (drop_pm)
+ pt27_runtime_put_autosuspend(chip);
+
+}
+
+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);
+ unsigned long flags;
+ u32 val;
+
+ if (!pt27_irq_regs_accessible(chip)) {
+ dev_err_ratelimited(chip->dev,
+ "%s: cannot unmask GPIO%lu; PT27 is not in D0\n",
+ __func__, hwirq);
+ return;
+ }
+
+ pt27_irq_take_pm_hold(chip);
+
+ gpiochip_enable_irq(gc, hwirq);
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+
+ pt27_wr32(chip, PT27_REG_INT_STAT, BIT(hwirq));
+
+ val = pt27_rd32(chip, PT27_REG_INT_EN);
+ val |= BIT(hwirq);
+ pt27_wr32(chip, PT27_REG_INT_EN, val);
+
+ val = pt27_rd32(chip, PT27_REG_INT_MASK);
+ val &= ~BIT(hwirq);
+ pt27_wr32(chip, PT27_REG_INT_MASK, val);
+
+ chip->enabled_irq_mask |= BIT(hwirq);
+
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+
+}
+
+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);
+ irq_hw_number_t hwirq = irqd_to_hwirq(d);
+
+ pt27_wr32(chip, PT27_REG_INT_STAT, BIT(hwirq));
+}
+
+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);
+ unsigned long flags;
+ u32 lvl_reg;
+ u8 lvl_bit, mode_val;
+ unsigned int shift;
+
+ if (!pt27_irq_regs_accessible(chip))
+ return -EIO;
+
+ 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;
+ }
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+
+ lvl_reg = pt27_rd32(chip, PT27_REG_INT_LVL);
+ if (lvl_bit)
+ lvl_reg |= BIT(hwirq);
+ else
+ lvl_reg &= ~BIT(hwirq);
+ pt27_wr32(chip, PT27_REG_INT_LVL, lvl_reg);
+
+ if (hwirq < 16) {
+ u32 reg = pt27_rd32(chip, PT27_REG_INT_MODE0);
+
+ shift = hwirq * 2;
+ reg &= ~(PT27_INT_MODE_MASK << shift);
+ reg |= ((u32)mode_val << shift);
+ pt27_wr32(chip, PT27_REG_INT_MODE0, reg);
+ } else {
+ u16 reg = pt27_rd16(chip, PT27_REG_INT_MODE1);
+
+ shift = (hwirq - 16) * 2;
+ reg &= ~((u16)(PT27_INT_MODE_MASK << shift));
+ reg |= (u16)(mode_val << shift);
+ pt27_wr16(chip, PT27_REG_INT_MODE1, reg);
+ }
+
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+
+ 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);
+ unsigned long flags;
+ u16 wake_en, wake_lvl, wake_mode;
+
+ if (hwirq >= PT27_WAKE_GPIOS) {
+ dev_warn(chip->dev,
+ "%s: GPIO%lu does not support wake (max GPIO%u)\n",
+ __func__, hwirq, PT27_WAKE_GPIOS - 1);
+ return -EINVAL;
+ }
+ if (!pt27_irq_regs_accessible(chip))
+ return -EIO;
+ if (on && trig == IRQ_TYPE_NONE)
+ return -EINVAL;
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+
+ wake_en = pt27_rd16(chip, PT27_REG_WAKE_EN);
+ wake_lvl = pt27_rd16(chip, PT27_REG_WAKE_LVL);
+ wake_mode = pt27_rd16(chip, 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);
+ }
+
+ pt27_wr16(chip, PT27_REG_WAKE_EN, wake_en);
+ pt27_wr16(chip, PT27_REG_WAKE_LVL, wake_lvl);
+ pt27_wr16(chip, PT27_REG_WAKE_MODE, wake_mode);
+
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+
+ return 0;
+}
+
+static void pt27_irq_bus_lock(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct pt27_gpio *chip = gpiochip_get_data(gc);
+ int ret;
+
+ mutex_lock(&chip->irq_bus_lock);
+
+ ret = pm_runtime_resume_and_get(chip->dev);
+ if (ret < 0) {
+ WRITE_ONCE(chip->irq_bus_active, false);
+ dev_err_ratelimited(chip->dev,
+ "%s: cannot resume for IRQ configuration: %d\n",
+ __func__, ret);
+ return;
+ }
+
+ WRITE_ONCE(chip->irq_bus_active, true);
+}
+
+static void pt27_irq_bus_sync_unlock(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct pt27_gpio *chip = gpiochip_get_data(gc);
+
+ if (READ_ONCE(chip->irq_bus_active)) {
+ WRITE_ONCE(chip->irq_bus_active, false);
+ pt27_runtime_put_autosuspend(chip);
+ }
+
+ mutex_unlock(&chip->irq_bus_lock);
+}
+
+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_bus_lock = pt27_irq_bus_lock,
+ .irq_bus_sync_unlock = pt27_irq_bus_sync_unlock,
+ .flags = IRQCHIP_IMMUTABLE,
+ GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static irqreturn_t pt27_gpio_isr(int irq, void *data)
+{
+ struct pt27_gpio *chip = data;
+ unsigned long pending;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+ pending = pt27_pending_irqs_locked(chip);
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+
+ if (!pending)
+ return IRQ_NONE;
+
+ while (pending) {
+ unsigned int hwirq = __ffs(pending);
+
+ pending &= ~BIT(hwirq);
+ generic_handle_domain_irq(chip->gc.irq.domain, hwirq);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static void pt27_hw_init(struct pt27_gpio *chip)
+{
+ u8 int_ctl;
+
+ int_ctl = pt27_rd8(chip, PT27_REG_INT_CTL);
+ int_ctl |= PT27_INT_CTL_MODE;
+ int_ctl &= ~PT27_INT_CTL_PAD_EN;
+ pt27_wr8(chip, PT27_REG_INT_CTL, int_ctl);
+
+ pt27_wr32(chip, PT27_REG_INT_EN, 0);
+ pt27_wr32(chip, PT27_REG_INT_MASK, PT27_GPIO_MASK);
+ pt27_wr32(chip, PT27_REG_INT_STAT, PT27_GPIO_MASK);
+ pt27_wr16(chip, PT27_REG_WAKE_EN, 0);
+ pt27_wr16(chip, PT27_REG_WAKE_STAT, PT27_WAKE_MASK);
+}
+
+static int pt27_gpio_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+ struct device *dev = &pdev->dev;
+ struct pt27_gpio *chip;
+ 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);
+ mutex_init(&chip->irq_bus_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_MSIX | PCI_IRQ_MSI);
+ if (ret < 0) {
+ ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX);
+ if (ret < 0) {
+ ret = dev_err_probe(dev, ret,
+ "failed to allocate IRQ vector\n");
+ goto err_clear_master;
+ }
+ chip->use_msi = false;
+ irq_flags = IRQF_SHARED;
+ dev_dbg(dev, "Using shared INTx interrupt\n");
+ } else {
+ chip->use_msi = true;
+ irq_flags = 0;
+ dev_dbg(dev, "Using %s interrupt vector\n",
+ pdev->msix_enabled ? "MSI-X" : "MSI");
+ }
+ chip->irq = pci_irq_vector(pdev, 0);
+
+ pt27_hw_init(chip);
+
+ pm_runtime_set_autosuspend_delay(dev, PT27_AUTOSUSPEND_DELAY_MS);
+ pm_runtime_use_autosuspend(dev);
+
+ chip->gc.label = dev_name(dev);
+ chip->gc.parent = dev;
+ chip->gc.owner = THIS_MODULE;
+ chip->gc.base = -1; /* Dynamic base assignment */
+ chip->gc.ngpio = PT27_NUM_GPIOS;
+ chip->gc.direction_input = pt27_gpio_direction_input;
+ chip->gc.direction_output = pt27_gpio_direction_output;
+ chip->gc.get_direction = pt27_gpio_get_direction;
+ chip->gc.get = pt27_gpio_get;
+ chip->gc.set = pt27_gpio_set;
+ chip->gc.can_sleep = true;
+
+ girq = &chip->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 = device_init_wakeup(dev, true);
+ if (ret) {
+ ret = dev_err_probe(dev, ret, "failed to enable wake capability\n");
+ goto err_free_vectors;
+ }
+
+ dev_pm_set_driver_flags(dev, DPM_FLAG_SMART_SUSPEND);
+
+ ret = devm_request_irq(dev, chip->irq, pt27_gpio_isr,
+ irq_flags | IRQF_NO_THREAD, dev_name(dev), chip);
+ if (ret) {
+ ret = dev_err_probe(dev, ret,
+ "failed to request IRQ %d\n", chip->irq);
+ goto err_wakeup;
+ }
+
+ ret = devm_gpiochip_add_data(dev, &chip->gc, chip);
+ if (ret) {
+ ret = dev_err_probe(dev, ret, "failed to register GPIO chip\n");
+ goto err_free_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_autosuspend(dev);
+ pm_runtime_allow(dev);
+
+ return 0;
+
+err_free_irq:
+ devm_free_irq(dev, chip->irq, chip);
+err_wakeup:
+ device_init_wakeup(dev, false);
+err_free_vectors:
+ pci_free_irq_vectors(pdev);
+err_clear_master:
+ pci_clear_master(pdev);
+ return ret;
+}
+
+static void pt27_gpio_remove(struct pci_dev *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct pt27_gpio *chip = pci_get_drvdata(pdev);
+ unsigned long flags;
+ bool drop_pm;
+
+ /* Restore the canonical "unbound PCI device" Runtime-PM state. */
+ pm_runtime_forbid(dev);
+ pm_runtime_get_noresume(dev);
+ pm_runtime_dont_use_autosuspend(dev);
+
+ mutex_lock(&chip->irq_bus_lock);
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+ pt27_wr32(chip, PT27_REG_INT_EN, 0);
+ pt27_wr32(chip, PT27_REG_INT_MASK, PT27_GPIO_MASK);
+ pt27_wr32(chip, PT27_REG_INT_STAT, PT27_GPIO_MASK);
+ pt27_wr16(chip, PT27_REG_WAKE_EN, 0);
+ pt27_wr16(chip, PT27_REG_WAKE_STAT, PT27_WAKE_MASK);
+
+ chip->enabled_irq_mask = 0;
+ chip->wake_mask = 0;
+ drop_pm = chip->irq_pm_held;
+ chip->irq_pm_held = false;
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+
+ if (drop_pm)
+ pm_runtime_put_noidle(dev);
+
+ device_init_wakeup(dev, false);
+
+ devm_free_irq(dev, chip->irq, chip);
+ pci_free_irq_vectors(pdev);
+ pci_clear_master(pdev);
+
+ mutex_unlock(&chip->irq_bus_lock);
+}
+
+static int __maybe_unused pt27_gpio_suspend(struct device *dev)
+{
+ struct pt27_gpio *chip = dev_get_drvdata(dev);
+ unsigned long flags;
+ u32 int_mask;
+
+ if (pm_runtime_status_suspended(dev)) {
+ dev_dbg(dev, "%s: already runtime-suspended, skipping save\n",
+ __func__);
+ return 0;
+ }
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+ pt27_save_regs(chip);
+ pt27_wr32(chip, PT27_REG_INT_EN, 0);
+ int_mask = PT27_REG_INT_MASK & ~chip->wake_mask;
+ pt27_wr32(chip, PT27_REG_INT_MASK, int_mask);
+ pt27_wr32(chip, PT27_REG_INT_STAT, PT27_GPIO_MASK);
+ pt27_wr16(chip, PT27_REG_WAKE_STAT, PT27_WAKE_MASK);
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+
+ return 0;
+}
+
+static int __maybe_unused pt27_gpio_resume(struct device *dev)
+{
+ struct pt27_gpio *chip = dev_get_drvdata(dev);
+ unsigned long flags;
+ u16 wake_status;
+
+ if (!chip->saved_valid)
+ return 0;
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+ wake_status = pt27_rd16(chip, PT27_REG_WAKE_STAT) & PT27_WAKE_MASK;
+ pt27_restore_regs(chip);
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+
+ 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);
+ unsigned long flags;
+ int ret = 0;
+
+ raw_spin_lock_irqsave(&chip->lock, flags);
+
+ if (chip->irq_pm_held || chip->enabled_irq_mask) {
+ ret = -EBUSY;
+ goto out_unlock;
+ }
+
+ if (chip->wake_mask && !device_may_wakeup(dev)) {
+ ret = -EBUSY;
+ goto out_unlock;
+ }
+
+ if (pt27_pending_irqs_locked(chip)) {
+ ret = -EBUSY;
+ goto out_unlock;
+ }
+
+ pt27_save_regs(chip);
+
+ pt27_wr32(chip, PT27_REG_INT_EN, 0);
+ pt27_wr32(chip, PT27_REG_INT_MASK, PT27_GPIO_MASK);
+ pt27_wr32(chip, PT27_REG_INT_STAT, PT27_GPIO_MASK);
+ pt27_wr16(chip, PT27_REG_WAKE_STAT, PT27_WAKE_MASK);
+
+out_unlock:
+ raw_spin_unlock_irqrestore(&chip->lock, flags);
+ 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 PT PCI GPIO Controller");
+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.
==================================================================================================================
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] gpio: amdptpci: add support for AMD PT PCI controller
2026-10-05 3:54 [PATCH] gpio: amdptpci: add support for AMD PT PCI controller Szuying Chen
@ 2026-10-05 4:06 ` sashiko-bot
2026-10-07 11:15 ` Linus Walleij
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-10-05 4:06 UTC (permalink / raw)
To: Szuying Chen; +Cc: linux-gpio
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] pm: shared IRQ handler accesses hardware registers while suspended
- [High] pm: missing synchronize_irq() in runtime_suspend for shared IRQ
- [High] gpio: incorrect macro usage unmasks all interrupts during system suspend
- [High] devres: gpiochip remains active after IRQ vectors are freed
- [High] locking: pm_runtime_put_autosuspend called under raw_spinlock
- [High] pm: concurrent mask and unmask race leads to hardware access in D3
- [High] irq: masking drops PM hold causing unmask to fail for level IRQs
--
commit 91c25cdd13553e16d22acf9d04f5ee20824cbf4b
Author: Szuying Chen <Chloe_Chen@asmedia.com.tw>
gpio: amdptpci: add support for AMD PT PCI controller
This commit implements the new gpio-amdptpci driver to support the GPIO
functionality on AMD PT PCI platforms.
> diff --git a/drivers/gpio/gpio-amdptpci.c b/drivers/gpio/gpio-amdptpci.c
> new file mode 100644
> index 0000000000000..ac2a7b46c3cde
> --- /dev/null
> +++ b/drivers/gpio/gpio-amdptpci.c
> @@ -274,40 +274,38 @@ static bool pt27_irq_regs_accessible(struct pt27_gpio *chip)
[ ... ]
> +static void pt27_irq_take_pm_hold(struct pt27_gpio *chip)
> +{
> + unsigned long flags;
> + bool take = false;
> +
> + raw_spin_lock_irqsave(&chip->lock, flags);
> + if (!chip->irq_pm_held) {
> + chip->irq_pm_held = true;
> + take = true;
> + }
> + raw_spin_unlock_irqrestore(&chip->lock, flags);
> +
> + if (take)
> + pm_runtime_get_noresume(chip->dev);
^^^^
[Severity: High]
Does this non-resuming PM call race with the put on another CPU, allowing
execution to proceed while the device is in D3?
If CPU 1 executes pt27_irq_mask() and clears chip->irq_pm_held, then is
preempted before calling pm_runtime_put_autosuspend():
CPU 0 could execute pt27_irq_unmask(), set chip->irq_pm_held to true, and
call pm_runtime_get_noresume().
When CPU 1 resumes, it suspends the device. Then CPU 0 proceeds to perform
MMIO writes in pt27_irq_unmask() while the device is in D3.
[ ... ]
> +static void pt27_irq_mask(struct irq_data *d)
> +{
[ ... ]
> + chip->enabled_irq_mask &= ~BIT(hwirq);
> + if (!chip->enabled_irq_mask && chip->irq_pm_held) {
> + chip->irq_pm_held = false;
> + drop_pm = true;
> + }
^^^^
[Severity: High]
Would dropping the PM hold here cause the unmask operation to fail for
level-triggered or threaded IRQs?
For a level-triggered IRQ, the core genirq subsystem calls irq_mask() before
handling and irq_unmask() after.
In pt27_irq_mask(), if it is the only enabled IRQ, enabled_irq_mask becomes
0, so the driver sets irq_pm_held = false and drops its PM reference.
> +
> + raw_spin_unlock_irqrestore(&chip->lock, flags);
> +
> + if (drop_pm)
> + pt27_runtime_put_autosuspend(chip);
^^^^
[Severity: High]
Can calling this here cause a lockdep splat and system crash on PREEMPT_RT
kernels?
The genirq core invokes pt27_irq_mask() with the per-IRQ descriptor lock
(desc->lock) held, which is a raw_spinlock_t.
Calling pt27_runtime_put_autosuspend() acquires dev->power.lock, which is a
sleepable spinlock_t. Nesting a sleepable spinlock inside a raw spinlock
violates RT lock nesting rules.
> +}
> +
> +static void pt27_irq_unmask(struct irq_data *d)
> +{
[ ... ]
> + if (!pt27_irq_regs_accessible(chip)) {
> + dev_err_ratelimited(chip->dev,
> + "%s: cannot unmask GPIO%lu; PT27 is not in D0\n",
> + __func__, hwirq);
> + return;
> + }
^^^^
[Severity: High]
Following the irq_mask() check above, later, pt27_irq_unmask() checks
pt27_irq_regs_accessible(), which evaluates to false because irq_pm_held
is now false and irq_bus_active is not held by genirq during standard
handling.
Will this unmask abort and leave the IRQ permanently masked?
[ ... ]
> +static u32 pt27_pending_irqs_locked(struct pt27_gpio *chip)
> +{
> + u32 stat = pt27_rd32(chip, PT27_REG_INT_STAT);
^^^^
[Severity: High]
Can this register read fail if the device is runtime-suspended?
> + u32 en = pt27_rd32(chip, PT27_REG_INT_EN);
> + u32 mask = pt27_rd32(chip, PT27_REG_INT_MASK);
> +
> + return stat & en & ~mask & PT27_GPIO_MASK;
> +}
[ ... ]
> +static irqreturn_t pt27_gpio_isr(int irq, void *data)
> +{
> + struct pt27_gpio *chip = data;
> + unsigned long pending;
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&chip->lock, flags);
> + pending = pt27_pending_irqs_locked(chip);
^^^^
[Severity: High]
Does this code access hardware registers unconditionally without ensuring the
device is powered on?
Because the driver can fall back to a shared INTx interrupt in probe():
chip->use_msi = false;
irq_flags = IRQF_SHARED;
another device sharing the INTx line could trigger this handler while this
device is in a runtime suspended state (D3hot). Accessing registers of a
suspended PCI device causes invalid reads or PCIe bus errors.
[ ... ]
> +static void pt27_gpio_remove(struct pci_dev *pdev)
> +{
[ ... ]
> + device_init_wakeup(dev, false);
> +
> + devm_free_irq(dev, chip->irq, chip);
> + pci_free_irq_vectors(pdev);
> + pci_clear_master(pdev);
> +
> + mutex_unlock(&chip->irq_bus_lock);
^^^^
[Severity: High]
Can userspace operations concurrent with device removal access freed IRQ
vectors here?
probe() uses devm_gpiochip_add_data() to register the GPIO chip, which
defers unregistration until after pt27_gpio_remove() returns.
However, this manually frees the backend IRQ vectors via
pci_free_irq_vectors().
This creates a window where the GPIO chip is fully visible and usable by
userspace or gpiolib, but its backing IRQ resources have been destroyed,
potentially leading to use-after-free crashes or invalid hardware accesses.
[ ... ]
> +static int __maybe_unused pt27_gpio_suspend(struct device *dev)
> +{
[ ... ]
> + raw_spin_lock_irqsave(&chip->lock, flags);
> + pt27_save_regs(chip);
> + pt27_wr32(chip, PT27_REG_INT_EN, 0);
> + int_mask = PT27_REG_INT_MASK & ~chip->wake_mask;
> + pt27_wr32(chip, PT27_REG_INT_MASK, int_mask);
^^^^
[Severity: High]
Is an incorrect bitmask written to the hardware here, breaking the system
wake functionality?
PT27_REG_INT_MASK is a register offset (0x0020), not a bitmask of all GPIOs.
Since hardware interprets bit 0 as unmasked (based on unmask function logic),
writing 0x0020 & ~... clears bits 0-4 and 6-23, unmasking almost all GPIO
interrupts when the system sleeps, causing a severe interrupt storm.
[ ... ]
> +static int __maybe_unused pt27_gpio_runtime_suspend(struct device *dev)
> +{
[ ... ]
> + pt27_save_regs(chip);
> +
> + pt27_wr32(chip, PT27_REG_INT_EN, 0);
> + pt27_wr32(chip, PT27_REG_INT_MASK, PT27_GPIO_MASK);
> + pt27_wr32(chip, PT27_REG_INT_STAT, PT27_GPIO_MASK);
> + pt27_wr16(chip, PT27_REG_WAKE_STAT, PT27_WAKE_MASK);
> +
> +out_unlock:
> + raw_spin_unlock_irqrestore(&chip->lock, flags);
> + return ret;
^^^^
[Severity: High]
Should synchronize_irq(chip->irq) be called outside the spinlock here?
During runtime suspend, the hardware is configured for suspend here, but it
returns and allows the PM core to power off the device (e.g., to D3) without
waiting for concurrent irq handlers to finish.
Because it lacks synchronize_irq(), a concurrently executing pt27_gpio_isr
on another CPU will continue to access the device's MMIO registers after
power is removed.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261005035410.18899-1-Chloe_Chen@asmedia.com.tw?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] gpio: amdptpci: add support for AMD PT PCI controller
2026-10-05 3:54 [PATCH] gpio: amdptpci: add support for AMD PT PCI controller Szuying Chen
2026-10-05 4:06 ` sashiko-bot
@ 2026-10-07 11:15 ` Linus Walleij
1 sibling, 0 replies; 3+ messages in thread
From: Linus Walleij @ 2026-10-07 11:15 UTC (permalink / raw)
To: Szuying Chen
Cc: brgl, linux-kernel, linux-gpio, Yd_Tseng, Jesse1_Chang,
Richard_Hsu
Hi Szuying,
thanks for your patch!
On Mon, Oct 5, 2026 at 5:54 AM Szuying Chen <Chloe_Chen@asmedia.com.tw> wrote:
> This commit implements the new gpio-amdptpci driver to support the
> GPIO functionality on AMD PT PCI platforms.
>
> Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> Co-authored-by: YD Tseng <Yd_Tseng@asmedia.com.tw>
(...)
> +#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
Compare this to drivers/gpio/gpio-amdpt.c:
/* PCI-E MMIO register offsets */
#define PT_DIRECTION_REG 0x00
#define PT_INPUTDATA_REG 0x04
#define PT_OUTPUTDATA_REG 0x08
#define PT_CLOCKRATE_REG 0x0C
#define PT_SYNC_REG 0x28
So this hardware appears to be related?
I can see that there is a lot of modifications to it though, and that it
may warrant a new driver. But this needs to be motivated in the
commit message.
The fact that one is an ACPI-probed MMIO driver and the other is
a PCI driver doesn't really matter for code sharing though, I need
a better motivation if the drivers should not share code.
> +static inline u32 pt27_rd32(struct pt27_gpio *c, u32 off)
> +{
> + return ioread32(c->base + off);
> +}
> +
> +static inline void pt27_wr32(struct pt27_gpio *c, u32 off, u32 v)
> +{
> + iowrite32(v, c->base + off);
> +}
> +
> +static inline u16 pt27_rd16(struct pt27_gpio *c, u32 off)
> +{
> + return ioread16(c->base + off);
> +}
> +
> +static inline void pt27_wr16(struct pt27_gpio *c, u32 off, u16 v)
> +{
> + iowrite16(v, c->base + off);
> +}
> +
> +static inline u8 pt27_rd8(struct pt27_gpio *c, u32 off)
> +{
> + return ioread8(c->base + off);
> +}
> +
> +static inline void pt27_wr8(struct pt27_gpio *c, u32 off, u8 v)
> +{
> + iowrite8(v, c->base + off);
> +}
What is the point of all this indirection? Just use the ioread/write
accessors directly in the code.
> +static void pt27_runtime_put_autosuspend(struct pt27_gpio *chip)
> +{
> + pm_runtime_mark_last_busy(chip->dev);
> + pm_runtime_put_autosuspend(chip->dev);
> +
> +}
Runtime PM already has a helper function like this, use it.
> +static void pt27_save_regs(struct pt27_gpio *chip)
> +{
> + chip->saved.dir = pt27_rd32(chip, PT27_REG_DIR);
> + chip->saved.out = pt27_rd32(chip, PT27_REG_OUT);
> + chip->saved.int_en = pt27_rd32(chip, PT27_REG_INT_EN);
> + chip->saved.int_lvl = pt27_rd32(chip, PT27_REG_INT_LVL);
> + chip->saved.int_mode0 = pt27_rd32(chip, PT27_REG_INT_MODE0);
> + chip->saved.int_mode1 = pt27_rd16(chip, PT27_REG_INT_MODE1);
> + chip->saved.int_mask = pt27_rd32(chip, PT27_REG_INT_MASK);
> + chip->saved.int_ctl = pt27_rd8(chip, PT27_REG_INT_CTL);
> + chip->saved.vendor0 = pt27_rd32(chip, PT27_REG_VENDOR0);
> + chip->saved.vendor1 = pt27_rd32(chip, PT27_REG_VENDOR1);
> + chip->saved.wake_en = pt27_rd16(chip, PT27_REG_WAKE_EN);
> + chip->saved.wake_lvl = pt27_rd16(chip, PT27_REG_WAKE_LVL);
> + chip->saved.wake_mode = pt27_rd16(chip, PT27_REG_WAKE_MODE);
> + chip->saved_valid = true;
> +
> +}
> +
> +static void pt27_restore_regs(struct pt27_gpio *chip)
> +{
> + pt27_wr32(chip, PT27_REG_INT_EN, 0);
> + pt27_wr32(chip, PT27_REG_INT_MASK, PT27_GPIO_MASK);
> +
> + pt27_wr32(chip, PT27_REG_DIR, chip->saved.dir);
> + pt27_wr32(chip, PT27_REG_OUT, chip->saved.out);
> + pt27_wr32(chip, PT27_REG_INT_LVL, chip->saved.int_lvl);
> + pt27_wr32(chip, PT27_REG_INT_MODE0, chip->saved.int_mode0);
> + pt27_wr16(chip, PT27_REG_INT_MODE1, chip->saved.int_mode1);
> + pt27_wr8(chip, PT27_REG_INT_CTL, chip->saved.int_ctl);
> + pt27_wr32(chip, PT27_REG_VENDOR0, chip->saved.vendor0);
> + pt27_wr32(chip, PT27_REG_VENDOR1, chip->saved.vendor1);
> +
> + pt27_wr16(chip, PT27_REG_WAKE_LVL, chip->saved.wake_lvl);
> + pt27_wr16(chip, PT27_REG_WAKE_MODE, chip->saved.wake_mode);
> + pt27_wr16(chip, PT27_REG_WAKE_EN, chip->saved.wake_en);
> +
> + pt27_wr32(chip, PT27_REG_INT_STAT, PT27_GPIO_MASK);
> + pt27_wr16(chip, PT27_REG_WAKE_STAT, PT27_WAKE_MASK);
> +
> + pt27_wr32(chip, PT27_REG_INT_EN, chip->saved.int_en);
> + pt27_wr32(chip, PT27_REG_INT_MASK, chip->saved.int_mask);
> +
> +}
If you're gonna do this what about using regmap which already has
an internal register cache, knows how to restore them and can deal
with the different word sizes and all?
> +static int pt27_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
> +{
(...)
> + val &= ~BIT(offset); /* 0 = input */
(...)
> +static int pt27_gpio_direction_output(struct gpio_chip *gc, unsigned int offset,
> + int value)
> +{
> + if (value)
> + val |= BIT(offset);
> + else
> + val &= ~BIT(offset);
(...)
> +static int pt27_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
(...)
> + return (val & BIT(offset)) ? GPIO_LINE_DIRECTION_OUT
> + : GPIO_LINE_DIRECTION_IN;
(...)
> +static int pt27_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> + dir = pt27_rd32(chip, PT27_REG_DIR);
> + val = pt27_rd32(chip, (dir & BIT(offset)) ? PT27_REG_OUT : PT27_REG_IN);
(...)
> +static int pt27_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
> + val = pt27_rd32(chip, PT27_REG_OUT);
> + if (value)
> + val |= BIT(offset);
> + else
> + val &= ~BIT(offset);
(...)
Just use select GPIO_GENERIC and follow the pattern of the
other drivers using this. Something like:
struct gpio_generic_chip_config config;
config = (struct gpio_generic_chip_config) {
.dev = dev,
.sz = 4,
.dat = g->base + PT27_REG_IN,
.set = g->base + PT27_REG_OUT,
.dirout = g->base + PT27_REG_DIR,
.flags = GPIO_GENERIC_READ_OUTPUT_REG_SET,
};
If you *absolutely* need this runtime PM around all accessors, then
implement generic runtime PM handling to the gpio-mmio.c library and
add a new GPIO_GENERIC_RUNTIME_PM flag to
<linux/gpio/generic.h> as a separate patch.
> +static bool pt27_irq_regs_accessible(struct pt27_gpio *chip)
> +{
> +
> + return READ_ONCE(chip->irq_bus_active) ||
> + READ_ONCE(chip->irq_pm_held);
> +}
Looks like second-guessing the state of runtime PM?
This can't be right. Make sure the code only access
registers when runtime PM is resumed.
For example whenever the irqchip .enable() callback
is called, get runtime PM, put it on .disable().
> +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);
> + unsigned long flags;
> + u32 val;
> +
> + if (!pt27_irq_regs_accessible(chip)) {
> + dev_err_ratelimited(chip->dev,
> + "%s: cannot unmask GPIO%lu; PT27 is not in D0\n",
> + __func__, hwirq);
> + return;
> + }
This looks wrong as per above reasoning.
> + chip->enabled_irq_mask |= BIT(hwirq);
I don't see why you would track this.
> +static void pt27_irq_bus_lock(struct irq_data *d)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct pt27_gpio *chip = gpiochip_get_data(gc);
> + int ret;
> +
> + mutex_lock(&chip->irq_bus_lock);
> +
> + ret = pm_runtime_resume_and_get(chip->dev);
> + if (ret < 0) {
> + WRITE_ONCE(chip->irq_bus_active, false);
> + dev_err_ratelimited(chip->dev,
> + "%s: cannot resume for IRQ configuration: %d\n",
> + __func__, ret);
> + return;
> + }
> +
> + WRITE_ONCE(chip->irq_bus_active, true);
Why is this tracked locally in the driver? Surely the
frameworks (runtime PM or irqchip) track this.
> +}
Isn't this solving the issue I pointed out above? If yes,
why is the workaround still there?
I will probably find more on further reviews but these things
are a good starting point for reworking the driver.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-07 11:15 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-05 3:54 [PATCH] gpio: amdptpci: add support for AMD PT PCI controller Szuying Chen
2026-10-05 4:06 ` sashiko-bot
2026-10-07 11:15 ` Linus Walleij
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox