Page MenuHomeFreeBSD

amdgpio: Mask and service interrupts
ClosedPublic

Authored by obiwac on Jul 28 2025, 10:31 AM.
Tags
None
Referenced Files
F132587993: D51588.diff
Sat, Oct 18, 5:14 AM
Unknown Object (File)
Sun, Oct 12, 3:03 PM
Unknown Object (File)
Sat, Oct 11, 7:16 AM
Unknown Object (File)
Sat, Oct 11, 7:16 AM
Unknown Object (File)
Sat, Oct 11, 7:16 AM
Unknown Object (File)
Sat, Oct 11, 7:16 AM
Unknown Object (File)
Fri, Oct 10, 11:59 PM
Unknown Object (File)
Fri, Oct 3, 6:53 PM
Subscribers

Details

Summary

Mask all interrupts coming from the AMD GPIO controller and service any potential interrupts. Unserviced interrupts can block entry to S0i3 on certain AMD CPUs.

Test Plan

Tested on Phoenix (AMD Framework 7040 series). Can log touchpad interrupts (pin 8) if enabled. Validated masking is enabled by checking for unserviced interrupts (in subsequent patch).

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sys/dev/amdgpio/amdgpio.c
61–62

Style nit here.

396

Can this be a define?

462

IMHO, amdgpio should be INTR_TYPE_MPSAFE? ACPI interrupt in x86 is handled by IOAPIC. Therefore, MPSafe is fine here since they can run in parallel without giant lock.

489

Please delete the new line

obiwac marked 3 inline comments as done.

Respond to @aokblast 's review comments

Thanks for reviewing this!

sys/dev/amdgpio/amdgpio.c
61–62

When grepping for struct resource_spec, I think I'm doing the same thing (with tabs after the commas). What specifically is wrong with style here?

391

I wonder if we should even have a filter here, since we won't really be getting many strays. Idk if filtering before handling adds a lot of overhead

396

absolutely

462

yeah you're right. The handler will be MP-safe.

aokblast added inline comments.
sys/dev/amdgpio/amdgpio.c
61–62

Ah. ok. It looks a little bit wired on my emacs.

417

I see the description in PPR

Configure the GPIO controller interrupt. The GPIO controller interrupt is an active low level interrupt. The driver

clears the interrupt status, then sets FCH::GPIO::GPIO_WAKE_INTERRUPT_MASTER_SWITCH[eoi] = 1 to
deassert the interrupt request after acknowledging the interrupt.

Wouldn't it means we need to clear the two bits?

This revision is now accepted and ready to land.Aug 25 2025, 3:17 PM
sys/dev/amdgpio/amdgpio.c
417

Ignore this comment, I drunk too much:).

obiwac added inline comments.
sys/dev/amdgpio/amdgpio.c
417

乾杯!🍻

mckusick added a subscriber: mckusick.

Mentor approval.

This revision was automatically updated to reflect the committed changes.
obiwac marked 2 inline comments as done.
sys/dev/amdgpio/amdgpio.c
401–406

Yeah, you can borrow such code from D26407.
Reading AMD_GPIO_PINS_EXPOSED hardware registers in an interrupt handler can be quite expensive.

423

It seems like currently the function always returns FILTER_HANDLED, rv variable does not have much purpose and its initialization with FILTER_STRAY is useless.

What I think that this function should do is set FILTER_HANDLED only if at least one pin had an interrupt mask.

427–431

The corresponding parameter to bus_setup_intr could simply be NULL and this can be removed.

Thanks for taking a look @avg

sys/dev/amdgpio/amdgpio.c
401–406

Ah! I didn't realize that patch was already handling interrupts for amdgpio. I was subscribed to it but never read through it in depth.

I will copy that code next time I touch this.

423

The idea here was to have subsequent patches only set FILTER_HANDLED if we had a consumer of this interrupt.

If we were to mirror D26407, I guess gpiobus_handle_intr could return EINVAL on stray like intr_event_handle does.

427–431

I was wondering if we should be doing everything in the filter routine or if there was anything we'd have to put in an ithread routine. But I guess not so I'll delete this