amdgpio: Suspend routine

[obiwac]

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.

[aokblast]

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.

[obiwac]

Respond to review comments

[obiwac]

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]

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.

[obiwac]

Remove style changes

[obiwac]

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 :)

[aokblast]

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]

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.

[emaste]

Is it reasonable to commit this (with the understanding that there will still be issues post-resume)?

[avg]

In D51589#1311788, @emaste wrote:

Is it reasonable to commit this (with the understanding that there will still be issues post-resume)?

IMO, yes.