Craig Hesling has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37459 )
Change subject: hatch: Fix FPMCU pwr/rst gpio handling ......................................................................
Patch Set 3:
(8 comments)
https://review.coreboot.org/c/coreboot/+/37459/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37459/2//COMMIT_MSG@20 PS2, Line 20:
BUG=? […]
Oh, I thought we were avoiding ChromeOS specific references. Is this not the case?
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... PS2, Line 414: 1
Can you do a git blame on this to see why it was set to 1 in the first place? I am suspecting if the […]
It looks like it points back to b/128686027, but it is unclear if this particular change fixed the bug. With my changes, I do not see the symptoms of this bug(not being able to export gpio control signals in user space).
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... PS2, Line 424: *num = ARRAY_SIZE(default_sleep_gpio_table); : return default_sleep_gpio_table;
This can just be: […]
Gotcha.
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/helios/gpio.c:
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... PS2, Line 161: if (slp_typ == ACPI_S5) {
I believe Craig wants to disable the pads only when entering S5 and leave as is when going to S3.
Furquan is correct. This particular section simply ensures that power is cut immediately when user issues a shutdown. This sections does not break S3, setting the gpio base table breaks S3.
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... PS2, Line 166: *num = ARRAY_SIZE(sleep_gpio_table); : return sleep_gpio_table;
Same comment as baseboard.
Done
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/helios/ramstage.c:
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... PS2, Line 22: {
A comment here would be helpful
Done
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/gpio.c:
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... PS2, Line 154: *num = ARRAY_SIZE(sleep_gpio_table); : return sleep_gpio_table;
same comment as baseboard.
Done
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/ramstage.c:
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... PS2, Line 22: {
Same here
Done