Attention is currently required from: Tim Wawrzynczak, Nick Vaccaro. Angel Pons 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: Code-Review+1
(12 comments)
File src/mainboard/google/brya/variants/agah/variant.c:
https://review.coreboot.org/c/coreboot/+/62384/comment/81e5899d_4cc6bfb0 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 from the VR's "power good" pin), right?
https://review.coreboot.org/c/coreboot/+/62384/comment/f41e2c35_b42e3102 PS3, Line 56: timeout_us Shouldn't this be `seq->timeout_us`? The `timeout_us` parameter is always `DEFAULT_PG_TIMEOUT_US`, and could be dropped.
https://review.coreboot.org/c/coreboot/+/62384/comment/5310caef_5f90d4e4 PS3, Line 65: gpio_output(GPU_ALLRAILS_PG, 0); Unlike the other power-good signals, this one is an output. I imagine it's wired to the dGPU's "power good" input pin, right?
https://review.coreboot.org/c/coreboot/+/62384/comment/6369112e_0c666020 PS3, Line 77: a nit: capitalize `A`
https://review.coreboot.org/c/coreboot/+/62384/comment/4ac137f7_b74a0ed6 PS3, Line 85: the series What does this mean?
https://review.coreboot.org/c/coreboot/+/62384/comment/cb38a6db_6010b72a PS3, Line 126: void variant_fill_ssdt(const struct device *dev) Does this need to be skipped if `gpu_powered_on` is false?
https://review.coreboot.org/c/coreboot/+/62384/comment/d5e5d61d_8896027f PS3, Line 126: dev I'm pretty sure this `dev` is unused, as it's the mainboard device. So, I'd strongly recommend renaming it to something like `unused` to avoid accidentally using it (e.g. writing `dev` because of treacherous muscle memory).
https://review.coreboot.org/c/coreboot/+/62384/comment/84e17094_6f042538 PS3, Line 145: 4 nit: sizeof(uint32_t)
https://review.coreboot.org/c/coreboot/+/62384/comment/a7bc7b93_699ec541 PS3, Line 149: res = probe_resource(dgpu, idx); Does this work for 64-bit resources?
https://review.coreboot.org/c/coreboot/+/62384/comment/7183338a_ae874a9f PS3, Line 154: PCI_BASE_ADDRESS_0 - : VGAB_BYTE_OFFSET + sizeof(uint32_t) * i Isn't this equivalent to `idx - VGAB_BYTE_OFFSET`?
https://review.coreboot.org/c/coreboot/+/62384/comment/fc83e063_fd9b6cea PS3, Line 160: */ nit: space before comment end
https://review.coreboot.org/c/coreboot/+/62384/comment/763bf670_2bc01f53 PS3, Line 164: 0x100 What is this magic number? I presume it's the size of the PCI config space?
-
Angel Pons (Code Review)