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.
Details
- Reviewers
aokblast avg gonzo mckusick - Commits
- rGa4d738d78305: amdgpio: Mask and service interrupts
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
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. |
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
clears the interrupt status, then sets FCH::GPIO::GPIO_WAKE_INTERRUPT_MASTER_SWITCH[eoi] = 1 to Wouldn't it means we need to clear the two bits? |
sys/dev/amdgpio/amdgpio.c | ||
---|---|---|
417 | Ignore this comment, I drunk too much:). |
sys/dev/amdgpio/amdgpio.c | ||
---|---|---|
417 | 乾杯!🍻 |
sys/dev/amdgpio/amdgpio.c | ||
---|---|---|
401–406 | Yeah, you can borrow such code from D26407. | |
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 |