Hello Edward O'Callaghan,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/37591
to review the following change.
Change subject: mb/google/hatch/variant/hatch: Config MEM_STRAP GPIOs ......................................................................
mb/google/hatch/variant/hatch: Config MEM_STRAP GPIOs
Hatch always used the default MEM_STRAPs in baseboard. Adding explicit configuration for the Hatch variant in the event that MEM_STRAP is set differently in the baseboard gpio file.
BUG=b:144895517 BRANCH=hatch TEST=./util/abuild/abuild -p none -t google/hatch -x -a
Change-Id: I6f6cb0af2d159ba9532f413939b546526b64a935 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/variants/hatch/gpio.c 1 file changed, 10 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/37591/1
diff --git a/src/mainboard/google/hatch/variants/hatch/gpio.c b/src/mainboard/google/hatch/variants/hatch/gpio.c index 56f587b..9aac509 100644 --- a/src/mainboard/google/hatch/variants/hatch/gpio.c +++ b/src/mainboard/google/hatch/variants/hatch/gpio.c @@ -20,7 +20,16 @@
static const struct pad_config gpio_table[] = { /* C13 : EC_PCH_INT_L */ - PAD_CFG_GPI_APIC(GPP_C13, UP_20K, PLTRST, LEVEL, INVERT)}; + PAD_CFG_GPI_APIC(GPP_C13, UP_20K, PLTRST, LEVEL, INVERT), + /* F11 : PCH_MEM_STRAP2 */ + PAD_CFG_GPI(GPP_F11, NONE, PLTRST), + /* F20 : PCH_MEM_STRAP0 */ + PAD_CFG_GPI(GPP_F20, NONE, PLTRST), + /* F21 : PCH_MEM_STRAP1 */ + PAD_CFG_GPI(GPP_F21, NONE, PLTRST), + /* F22 : PCH_MEM_STRAP3 */ + PAD_CFG_GPI(GPP_F22, NONE, PLTRST), +};
const struct pad_config *override_gpio_table(size_t *num) {
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37591 )
Change subject: mb/google/hatch/variant/hatch: Config MEM_STRAP GPIOs ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37591/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37591/1//COMMIT_MSG@9 PS1, Line 9: Hatch always used the default MEM_STRAPs in baseboard. That has been intentional.
https://review.coreboot.org/c/coreboot/+/37591/1//COMMIT_MSG@10 PS1, Line 10: in the event that MEM_STRAP is set : differently in the baseboard gpio file That would be a problem in general with any GPIO. I don't understand why MEM_STRAPs would be special. In general, if any GPIO is being touched in baseboard gpio table, its impact will have to be evaluated on all variants.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37591 )
Change subject: mb/google/hatch/variant/hatch: Config MEM_STRAP GPIOs ......................................................................
Patch Set 1:
(2 comments)
Thanks for the review, PTAL.
https://review.coreboot.org/c/coreboot/+/37591/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37591/1//COMMIT_MSG@9 PS1, Line 9: Hatch always used the default MEM_STRAPs in baseboard.
That has been intentional.
I know however the assumptions already encoded in baseboard for all variants to be laptop/tablets has been superseeded. This is addressed in the follow up:
https://review.coreboot.org/c/coreboot/+/36988
https://review.coreboot.org/c/coreboot/+/37591/1//COMMIT_MSG@10 PS1, Line 10: in the event that MEM_STRAP is set : differently in the baseboard gpio file
That would be a problem in general with any GPIO. […]
This is not unlike commit 5a0edcbde153 (https://review.coreboot.org/c/coreboot/+/37106) and fixed in https://review.coreboot.org/c/coreboot/+/37589 ..
The only reason why this series focuses on memstrap in particular is because I wanted to deal with one functional element of gpio mux changes across variants at a time not change them all at once. The memstrap muxing is a primary case of assumptions made in baseboard that no longer generalise across all variants, in fact not all variants even have the same muxing.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37591 )
Change subject: mb/google/hatch/variant/hatch: Config MEM_STRAP GPIOs ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37591/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37591/1//COMMIT_MSG@9 PS1, Line 9: Hatch always used the default MEM_STRAPs in baseboard.
I know however the assumptions already encoded in baseboard for all variants to be laptop/tablets has been superseeded.
Independent of the form factor, the baseboard GPIOs are just one way of configuring the GPIOs which applies to most variants. And only the variants that do not follow this model then can provide their own overrides. I understand that we are supporting more and more variants and so the baseboard GPIOs might not be right for them. In that case it is better to think whether the baseboard GPIOs even make sense for the variant at all and if not, then just switch to providing a complete base GPIO table in that variant.
https://review.coreboot.org/c/coreboot/+/37591/1//COMMIT_MSG@10 PS1, Line 10: in the event that MEM_STRAP is set : differently in the baseboard gpio file
This is not unlike commit 5a0edcbde153 (https://review.coreboot.org/c/coreboot/+/37106) and fixed in https://review.coreboot.org/c/coreboot/+/37589
I don't think there was anything broken with 5a0edcbde153. I had added a comment indicating that the early_gpio_table configuration was intentionally skipped. Doing the configuration in early_gpio_table isn't wrong.
The memstrap muxing is a primary case of assumptions made in baseboard that no longer generalise across all variants, in fact not all variants even have the same muxing.
Agreed. And the variants that do not use the same muxing override it in the variant specific GPIO tables. I know variant "hatch" has been neglected for a while, so there can be things broken there.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37591?usp=email )
Change subject: mb/google/hatch/variant/hatch: Config MEM_STRAP GPIOs ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Edward O'Callaghan (Code Review) -
Furquan Shaikh (Code Review)
-
Martin L Roth (Code Review)