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

[Szuying Chen]

Hi Linus,

I sincerely apologize for the oversight. I accidentally missed this 
thread while compiling the v2 patch set.

I have now reviewed your comments and will address them in the 
upcoming v3. Please see my notes inline below.

>Hi Szuying,

>thanks for your patch!

>On Mon, Oct 5, 2026 at 5:54 AM Szuying Chen <Chloe_Chen@xxxxxxxxxxxxxx>> 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@xxxxxxxxxxxxxx>>
>> Co-authored-by: YD Tseng <Yd_Tseng@xxxxxxxxxxxxxx>>

>(...)
>> +#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.

We agree that ACPI vs. PCI probing alone does not justify separate drivers.
However, our rationale for a dedicated driver is based on the following 
significant hardware differences:

Register semantics conflict: Offset 0x0C controls the clock rate in 
gpio-amdpt, but enables interrupts in gpio-amdptpci. Sharing the 
implementation would require complex hardware-specific logic for registers 
with completely different behaviors.

Interrupt controller: The gpio-amdpt driver does not support interrupts at 
all, whereas gpio-amdptpci integrates a dedicated interrupt controller 
(INT_LVL, INT_STAT, INT_MASK) that requires a full irq_chip implementation.

Wakeup support: gpio-amdptpci supports wakeup from GPIO interrupts, 
a capability not present or supported in gpio-amdpt.
  
Therefore, we believe maintaining a separate driver would provide clearer 
hardware-specific implementation and avoid unnecessary complexity from 
combining different register behaviors and interrupt / wakeup architectures. 

Since the common part is limited to the basic data registers, which we plan 
to handle through gpio-generic, there is little code left to share. We will 
add this rationale to the commit message in v3.

>> +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.

Agreed. 
We will drop the pt27_rd/wr wrappers and use ioread/iowrite directly in v3.

>> +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.

Agreed. We will drop the pt27_runtime_put_autosuspend() wrapper and use
the generic runtime PM helper directly in v3.

>> +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?

We are evaluating regmap for the register cache and the suspend/resume 
restore path, and we expect to use it in v3. 

The remaining points to verify are which registers must be marked volatile
(e.g. INT_STAT) and how regmap interacts with our runtime PM handling.
If regmap turns out not to fit, we will explain the specific reason in v3.

>> +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.

Regarding gpio-generic: the reason we currently have our own accessors is 
that every register access must be guarded by runtime PM, 
since the device may be in D3 when GPIO is accessed. 

We will try to convert the DIR/IN/OUT handling to gpio-generic in v3. 
If runtime PM turns out to be required around the accessors, 
we will evaluate adding generic runtime PM support to gpio-mmio.c 
as a separate patch, as you suggested, and will explain our findings 
in the cover letter or commit message.

>> +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?

We will rework the IRQ runtime PM handling as you suggested: take a 
runtime PM reference in the irqchip .enable() callback and release it in 
.disable(), so that registers are only accessed while the device is resumed. 

This lets us drop pt27_irq_regs_accessible() together with the irq_bus_active 
and irq_pm_held flags, which were a workaround for not having this guarantee. 

We will also remove enabled_irq_mask unless we find it is required, 
in which case we will explain why in v3.

>I will probably find more on further reviews but these things are a good starting point for reworking the driver.

>Yours,
>Linus Walleij


Thanks for your suggestions!

Best regards,
Chloe Chen
==================================================================================================================
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.
==================================================================================================================