amdgpio: Suspend routine
Needs ReviewPublic
Actions

Authored by obiwac on Jul 28 2025, 10:36 AM.

Details

Reviewers

Summary

Mask all interrupts when suspending and warn when there are unserviced interrupts which might block entry to S0i3.

In the future we won't want to mask wake interrupts.

Once we can actually make use of GPIO interrupts on x86, we'll also want to unmask relevant pins when resuming.

Test Plan

Tested on Phoenix (AMD Framework 7040 series). Unserviced interrupts warning is printed when purposely not servicing interrupts.

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 66486
Build 63369: arc lint + arc unit

obiwac requested review of this revision.Jul 28 2025, 10:36 AM

sys/dev/amdgpio/amdgpio.c
546	Can it be a #define?
552	Should we use the pin info in sc?
589–591	Not sure if it is a proper way to do style related change in this patch.

Respond to review comments

thanks for the review!

sys/dev/amdgpio/amdgpio.c
552	I guess we could because `sc->sc_pin_info[i]` happens to be pin number i, but that's assuming `kernzp_pins` maps 1:1 its indices to pin numbers (which it does, but it could also not). without looking at `kernzp_pins` we can't say for sure if pin at index #i is pin #i.
589–591	can you elaborate on this? do you mean you'd rather I not update the style of the surrounding lines?

lwhsu added inline comments.

sys/dev/amdgpio/amdgpio.c
589–591	It's good to fix style issues, but in convention we like to separate the commits of the non-functional change and functional change. This has several benefits, like review can focus on the functional changes, and the non-functional changes can be ignored in git-blame and git-bisect, etc. We also have `.git-blame-ignore-revs` to help this.

Remove style changes

obiwac marked 2 inline comments as done.Aug 23 2025, 10:11 PM

obiwac added inline comments.

sys/dev/amdgpio/amdgpio.c
589–591	okay got it, I've reverted the style changes. Didn't know about `.git-blame-ignore-revs`, that's nice :)

sys/dev/amdgpio/amdgpio.c
555	Accourding to the PPR, UNSERVICED_INTERRUPT_MASK means the interrupt does not triggered? Do we need to print all of the untriggered pin?
565	In suspend, we only need to clear INTERRUPT_MASK_OFF bit? Since we need to interrupt to be enabled to wake it up? Also, according to the PPR. The way to check if a interrupt mask is just read the 11~13 bit. And this bit is set by the BIOS without doing anything from the ACPI dsdt. In theory, we can just read bit 11~13 to know that if it is a wakeup interrupt.

obiwac marked an inline comment as done.Aug 25 2025, 9:22 PM

obiwac added inline comments.

sys/dev/amdgpio/amdgpio.c
555	What do you mean by untriggered exactly? `UNSERVICED_INTERRUPT_MASK` here is just if one of interrupt status or wake status is set, in which case there is a pending ("unserviced") interrupt on that pin.
565	At the moment and for the purposes of this revision, I'm assuming nothing GPIO-related is a wake interrupt, and disabling all of them as wake interrupts. The next step would be to parse the `_AEI` object to know which pins are wake pins and not rely on the bits already set by the BIOS - I'm working on this. By the way curious as to where you're finding this info in the PPR? As far as I could tell GPIO information is only in the old BKDGs.