Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42224 )
Change subject: soc/amd/common/blocks/gpio: Introduce gpio_get_bar ......................................................................
Patch Set 4: Code-Review-1
(2 comments)
https://review.coreboot.org/c/coreboot/+/42224/4/src/soc/amd/common/acpi/gpi... File src/soc/amd/common/acpi/gpio_bank_lib.asl:
https://review.coreboot.org/c/coreboot/+/42224/4/src/soc/amd/common/acpi/gpi... PS4, Line 9: Return (Add(Multiply(Arg0, 4), ACPIMMIO_GPIO_BASE)) The code here implies the banks are one after each other in ACPIMMIO space, splitting to banks appears completely unnecessary. And with this commit .asl and .c now have different implementation on how to arrive at the correct register.
So with gpio 64 and 128
gpio0_base + 64 * sizeof(uint32_t) == gpio1_base
gpio0_base + 128 * sizeof(uint32_t) == gpio2_base
https://review.coreboot.org/c/coreboot/+/42224/4/src/soc/amd/common/block/gp... File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42224/4/src/soc/amd/common/block/gp... PS4, Line 95: bar += gpio_get_offset(gpio_num); Here is the assembly injected for current master:
Disassembly of section .text.gpio_get_address:
00000000 <gpio_get_address>: 0: 8b 44 24 04 mov 0x4(%esp),%eax 4: 8d 04 85 00 15 d8 fe lea -0x127eb00(,%eax,4),%eax b: c3 ret
In other words:
return ACPIMMIO_GPIO0_BASE + gpio_num * sizeof(uint32_t);
The only thing that has to change for psp-verstage is replace every such #define with a symbol. That approach was offered in CB:37324 on June 5th already as it can reveal other problems too.