Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38620 )
Change subject: mb/google/volteer: add volteer mainboard initial support ......................................................................
Patch Set 16:
(16 comments)
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... File src/mainboard/google/volteer/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 148:
nit: tab is missing.
Done
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... File src/mainboard/google/volteer/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 27: 1
Note for future change: EN_PP3300_TOUCHSCREEN should be set to 0 here and the power to touchscreen s […]
Ack
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 96: DN_20K
This is not required.
Done
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 106: DN_20K
Why is there a PD? Is that the default state of the pad? In general, I think for all PAD_NC we can k […]
Done
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 160: UP_20K
Why is an internal pull-up being configured?
Line 81 of the volteer GPIO spec sheet says it's needed.
https://docs.google.com/spreadsheets/d/16vwVzoDrLwf4v-Wb_PLXj8oCmEnBOafwwhUA...
I spoke to Catherine about this, here's what she said. I will remove these and the development effort for the sensors can request they be set as needed.
Catherine response: "so GPP-D1/D2/D3 are all ISH sensor signals. we don't use them, Intel have a few guy working on ISH use those when they do use those for ISH development: depends on sensor selection, it may need internal PU GPP_R4=HDA_RST_L, againg, only Intel guys doing HDA use that."
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 162: UP_20K
same here?
Line 82 of the volteer GPIO spec sheet says it's needed.
https://docs.google.com/spreadsheets/d/16vwVzoDrLwf4v-Wb_PLXj8oCmEnBOafwwhUA...
Will remove pull based on Catherine's response.
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 164: UP_20K
same here?
Line 83 of the volteer GPIO spec sheet says it's needed.
https://docs.google.com/spreadsheets/d/16vwVzoDrLwf4v-Wb_PLXj8oCmEnBOafwwhUA...
Will remove pull based on Catherine's response.
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 178: DN_20K
This is not required.
Done
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 233: DN_20K
Why? I think this can simply be PAD_CFG_GPO.
Done
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 237: DN_20K
Why?
Done
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 265: /* F9 : Reserved ==> NC */
Why is there no GPP_F9 configuration here?
Volteer declares F9 as reserved. I'll set it as an NC.
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 278: /* PAD_CFG_GPI_SCI_LOW(GPP_F15, NONE, PLTRST, EDGE_SINGLE), */
Not needed?
Done
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 337: PLTRST
Why PLTRST and not DEEP?
Done
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 354: DN_20K
Not required?
Line 180 of the volteer GPIO spreadsheet shows a PD needed.
Will remove pull based on Catherine's response.
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 425: DN_20K
Not required.
Done
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 440: RSMRST
Why is this RSMRST?
Done