Attention is currently required from: Cliff Huang, Maulik V Vaghela, Tim Wawrzynczak, Patrick Rudolph. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59855 )
Change subject: soc/intel/common/block/pcie/rtd3: Add ModPHY power gate support for RTD3 ......................................................................
Patch Set 3:
(4 comments)
File src/soc/intel/common/block/pcie/rtd3/rtd3.c:
https://review.coreboot.org/c/coreboot/+/59855/comment/0ed5906c_a85bd27a PS2, Line 238: static bool mutex_created;
Done
Why we need to initialize to `false` ? Default value itself is false. Isn't it?
#include <stdio.h> #include <stdbool.h> int main() { static bool a; bool b; printf("Default value of static variable : %s\n", a == true ? "true" : "false"); printf("Default value of non-static variable : %s\n", b == true ? "true" : "false"); return 0; }
$gcc -o main *.c -lm $main Default value of static variable : false Default value of non-static variable : false
File src/soc/intel/common/block/pcie/rtd3/rtd3.c:
https://review.coreboot.org/c/coreboot/+/59855/comment/e896e1f0_e0af4d3e PS3, Line 83: bool enabl Just thoughts:
Do you really need line #90 if/else clause ?
can't we have like this?
enum modphy_pg_state { PG_DISABLE = 0, PG_ENABLE = 1, };
static void pcie_rtd3_enable_modphy_pg(unsigned int pcie_rp, enum modphy_pg_state state) { /* Enter the critical section */ .... acpigen_write_store_int_to_namestr(state, "EMPG"); acpigen_write_delay_until_namestr_int(100, "AMPG", state);
/* Exit the critical section */ .... }
https://review.coreboot.org/c/coreboot/+/59855/comment/cc697db2_a9f21973 PS3, Line 112: pcie_rtd3_enable_modphy_pg(pcie_rp, false); pcie_rtd3_enable_modphy_pg(pcie_rp, PG_DISABLE);
https://review.coreboot.org/c/coreboot/+/59855/comment/bc3138ec_221bcec2 PS3, Line 159: pcie_rtd3_enable_modphy_pg(pcie_rp, true); pcie_rtd3_enable_modphy_pg(pcie_rp, PG_ENABLE);