Attention is currently required from: Shelley Chen, Douglas Anderson. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56901 )
Change subject: trogdor: Fix "TPM interrupt" lb_gpio to be ACTIVE_HIGH ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Pardon my ignorance, but what exactly makes this a "latched" GPIO? Is it because it's a GPIO that' […]
"latched" means the line is edge triggered and the hardware will set an interrupt flag as soon as it sees the appropriate edge which is cleared the next time software checks for it. In coreboot it's basically a totally different API -- gpio_input_irq() vs. gpio_input() and gpio_irq_status() vs. gpio_get(). But we're using the same table to pass both kinds of GPIOs to depthcharge, and in depthcharge they're wrapped in the same GpioOps API despite having different underpinnings (new_gpio_latched_from_coreboot() vs. new_gpio_input_from_coreboot()). The problem here is that the parsing of this table is handled by the same code for both kinds of GPIOs (sysinfo_lookup_gpio()), and as soon as that sees an ACTIVE_LOW flag it will automatically wrap the whole thing in a new_gpio_not() in depthcharge. So ACTIVE_LOW means "negate this in depthcharge", which makes sense for level-trigged GPIOs (because in our depthcharge code we always assume asserted = high for everything and use this negation mechanism for active low lines), but it doesn't make sense for latched GPIOs because the decision which edge to trigger on was already made by the initial coreboot code that programmed the controller to do this latching, and from then on out it can only return 1 for "yes I saw the edge" or 0 for "didn't see a new edge yet since the last time I was called". Putting a negation wrapper around that is always wrong.
(GPIO_AP_EC_INT is fine because it's not using the latched mechanism, it's a normal level-triggered GPIO and the EC only takes it high again after the triggering condition has been removed. If you ask me they should have programmed H1 to just do the same thing because it's a ton easier to deal with, but nobody asked me about that...)