amdgpio: Mask and service interrupts

[obiwac]

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

[aokblast]

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]

Respond to @aokblast 's review comments

[obiwac]

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]

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?

[aokblast]

sys/dev/amdgpio/amdgpio.c

417

Ignore this comment, I drunk too much:).

[obiwac]

sys/dev/amdgpio/amdgpio.c

417

乾杯！🍻

[mckusick]

Mentor approval.

[avg]

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.

[obiwac]

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