Attention is currently required from: Maulik V Vaghela, Mario Scheithauer, Subrata Banik, Lean Sheng Tan, Patrick Rudolph. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55861 )
Change subject: soc/intel/elkhartlake: Expose FIVR config to mainboard ......................................................................
Patch Set 3: Code-Review+1
(3 comments)
File src/soc/intel/elkhartlake/chip.h:
https://review.coreboot.org/c/coreboot/+/55861/comment/6078cf5c_1ed76b58 PS3, Line 77: I would align FIVR_VOLTAGE_MIN_RETENTION with FIVR_VOLTAGE_NORMAL as it belongs to the same bitmask.
https://review.coreboot.org/c/coreboot/+/55861/comment/007c911b_78568caf PS3, Line 382: enum fivr_states v1p05_state_bitmap; : enum fivr_states vnn_state_bitmap; : enum fivr_states vnn_sx_state_bitmap; : enum fivr_supported_voltage v1p05_volt_bitmap; : enum fivr_supported_voltage vnn_volt_bitmap; Is there any benefit in calling all the members *_bitmap? The type is clear with the enum, so why not drop _bitmap and use it as state? And for the fivr_supported_voltage-members, I would rather call them v1p05_rail and vnn_rail respectively and drop the _bitmap, too.
https://review.coreboot.org/c/coreboot/+/55861/comment/5758b58c_419b3689 PS3, Line 393: unsigned int vcc_low_high_usec; : /* From retention mode voltage to high current mode voltage */ : unsigned int vcc_ret_high_usec; : /* From retention mode voltage to low current mode voltage */ : unsigned int vcc_ret_low_usec; : /* From off(0V) to high current mode voltage */ : unsigned int vcc_off_high_usec; It seems like "us" is more often used for µs along the tree than usec. If you would like to have shorter member names I would adapt.