Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44586 )
Change subject: mb/google/dedede: Add support to override gpio config in bootblock ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44586/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44586/5//COMMIT_MSG@7 PS5, Line 7: Add support to override gpio config in bootblock
Should we consider making a modification to the override mechanism to add entries from the override […]
I agree with Furquan's analysis. I have been burnt once for the same situation Furquan mentioned. I forgot to configure the GPIO in the baseboard early gpio table and the override was not applying.
For the use-case here, we don't need an early override table. GPIOs used for LTE power sequencing is dedicated for WWAN purposes. Those GPIOs can be configured in the baseboard early gpio table and overridden only in the ramstage.
But if the use-case for these GPIOs change in the future and if we have to configure them in bootblock, then we will be in the same situation as we are today.
Should we consider making a modification to the override mechanism to add entries from the override table that aren't in the base table?
We can do it, but does that bode well with the definition of "override". My understanding is to override something we have to have a base definition. Without the base definition, do we want to allow overriding entries.
An alternate option is to override the entire early gpio table. variant_early_gpio_table() is overrideable. So define a complete override table in the variant and return that. const struct pad_config *__weak variant_early_gpio_table(size_t *num) { *num = ARRAY_SIZE(early_gpio_table); return early_gpio_table; }