Attention is currently required from: Angel Pons, Nick Vaccaro. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62384 )
Change subject: mb/google/brya/var/agah: Add GPU power sequencing ......................................................................
Patch Set 3:
(12 comments)
Patchset:
PS3: Thanks Angel! đ
File src/mainboard/google/brya/variants/agah/variant.c:
https://review.coreboot.org/c/coreboot/+/62384/comment/ac846f8d_384fdba2 PS3, Line 34: gpio_t pg_gpio;
AIUI, `pwr_en_gpio` is an output (wired to the VR's "enable" pin) and `pg_gpio` is an input (wired f [âŚ]
You got it! I can add comments.
https://review.coreboot.org/c/coreboot/+/62384/comment/67aa2dce_48b21d5f PS3, Line 56: timeout_us
Shouldn't this be `seq->timeout_us`? The `timeout_us` parameter is always `DEFAULT_PG_TIMEOUT_US`, a [âŚ]
Oops, yes.
https://review.coreboot.org/c/coreboot/+/62384/comment/d156ac3a_4a88e22d PS3, Line 65: gpio_output(GPU_ALLRAILS_PG, 0);
Unlike the other power-good signals, this one is an output. [âŚ]
Yes
https://review.coreboot.org/c/coreboot/+/62384/comment/a06bba9a_544689bc PS3, Line 77: a
nit: capitalize `A`
Done
https://review.coreboot.org/c/coreboot/+/62384/comment/1a062cd6_13973141 PS3, Line 85: the series
What does this mean?
Good question đ fixed
https://review.coreboot.org/c/coreboot/+/62384/comment/fcdbfba8_398d43c2 PS3, Line 126: void variant_fill_ssdt(const struct device *dev)
Does this need to be skipped if `gpu_powered_on` is false?
Good call, it would not have enumerated in that case! đ
https://review.coreboot.org/c/coreboot/+/62384/comment/e3d0c751_18a1d239 PS3, Line 126: dev
I'm pretty sure this `dev` is unused, as it's the mainboard device. [âŚ]
đ good idea
https://review.coreboot.org/c/coreboot/+/62384/comment/10e53d6a_3d412de9 PS3, Line 145: 4
nit: sizeof(uint32_t)
Done
https://review.coreboot.org/c/coreboot/+/62384/comment/23da1cdc_ff02602c PS3, Line 149: res = probe_resource(dgpu, idx);
Does this work for 64-bit resources?
In a way, yes... in PCI land there are BAR0-BAR5, each 32 bits wide, but a 64-bit BAR is made from combining two 32-bit BARs together (e.g. BAR0-BAR1) IIRC, and this (should) correctly copy those 6 BAR registers into place.
https://review.coreboot.org/c/coreboot/+/62384/comment/1167edb2_991d4a01 PS3, Line 154: PCI_BASE_ADDRESS_0 - : VGAB_BYTE_OFFSET + sizeof(uint32_t) * i
Isn't this equivalent to `idx - VGAB_BYTE_OFFSET`?
Good eye! That reads much cleaner
https://review.coreboot.org/c/coreboot/+/62384/comment/9fe048ac_42a9843c PS3, Line 160: */
nit: space before comment end
Done
-
Tim Wawrzynczak (Code Review)