Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32233 )
Change subject: chromeos: clean up "recovery" and "write protect" GPIOs ......................................................................
Patch Set 1:
(3 comments)
Feel free to keep the discussion going to decide what to do with the lb_gpio table in the long run. If everyone is okay with my comments, can we commit this?
https://review.coreboot.org/#/c/32233/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32233/1//COMMIT_MSG@15 PS1, Line 15: The cached value of the "recovery" GPIO is read only on certain : boards which have a physical recovery switch.
Right, but those can't be handled in this table anyway. […]
Re. "recovery" isn't read at all: Unless I'm mistaken, it is read on a few boards, by calling flag_replace with the same "recovery" GPIO and resample_at_runtime=1. For example:
flag_replace(FLAG_RECSW, sysinfo_lookup_gpio("recovery", 1, new_rk_gpio_input_from_coreboot));
https://review.coreboot.org/#/c/32233/1//COMMIT_MSG@18 PS1, Line 18: Most of these inaccuracies are from : non-inverted values on ACTIVE_LOW GPIOs.
+1 to what you said about letting depthcharge read the GPIO state for write-protect.
Re. removing "recovery": see my comment response above.
Re. using ACTIVE_HIGH for inverting initial values: This almost underlines the fact that we are doing something really strange here -- using a GPIO table to transfer a value from one firmware application to another. I would prefer to keep it as the "true polarity" right now, and decide whether we would like to go the direction that Julius mentioned (all GPIOs are sampled in depthcharge), or choose some other vehicle for transferring these values (no one says they need to be transferred in lb_gpio table in the first place).
Re. read write-protect GPIO in depthcharge: Yeah, this might be a good idea. However I think that would be best to put in a subsequent CL. I'd like to first correct all of the discrepancies between "initial value" and get_write_protect_state/get_recovery_mode_switch.
As I mentioned in the offline email, it's also very hard to understand which GPIO needs an initial value and which doesn't; which GPIO is read in depthcharge onward and which isn't. We may even think about a bigger refactor here to smooth out these bumps.
https://review.coreboot.org/#/c/32233/1/src/mainboard/google/gru/chromeos.c File src/mainboard/google/gru/chromeos.c:
https://review.coreboot.org/#/c/32233/1/src/mainboard/google/gru/chromeos.c@... PS1, Line 36: get_write_protect_state() ^ !wp_polarity, "write protect"},
I think in this case, leaving it as gpio_get() would be cleaner?
I agree it's a bit confusing, but I think it's really important to get all of the WP initial value in a consistent state, reading from get_write_protect_state, before we make our next move here.