Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44359 )
Change subject: mainboard/google/volteer: Enable long cr50 ready pulses ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44359/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/chromeos.c:
https://review.coreboot.org/c/coreboot/+/44359/4/src/mainboard/google/voltee... PS4, Line 43: tlcl_lib_init();
I think we had a discussion elsewhere. […]
Ack
https://review.coreboot.org/c/coreboot/+/44359/8/src/mainboard/google/voltee... File src/mainboard/google/volteer/mainboard.c:
https://review.coreboot.org/c/coreboot/+/44359/8/src/mainboard/google/voltee... PS8, Line 40: tlcl_lib_init(); : if (get_cr50_long_interrupt_pulses()) { : printk(BIOS_INFO, "Enabling S0i3.4\n"); : } else { : /* Disable S03.4, preventing the GPIO block from switching to : * slow clock. */ : printk(BIOS_INFO, "Not enabling S0i3.4\n"); : cfg->LpmStateDisableMask = LPM_S0i3_4; : }
nit: Put this in a helper function just to logically keep all changes together. […]
Done.
In time, I think that this mainboard_update_s0ix_disable_mask() should be moved into some common place, from which other board using Tiger Lake, or future Intel designs, can call it. (Until such time when our cr50 chips ship with new enough firmware that we can count on the long pulse support to always be there, at which point we no longer need to runtime branching here.)
https://review.coreboot.org/c/coreboot/+/44359/8/src/soc/intel/tigerlake/Kco... File src/soc/intel/tigerlake/Kconfig:
PS8:
Can you please push this in a separate patch? We typically separate out SoC changes from mainboard c […]
I have created: https://review.coreboot.org/c/coreboot/+/44626
https://review.coreboot.org/c/coreboot/+/44359/8/src/soc/intel/tigerlake/Kco... PS8, Line 173: config
A comment here would be very helpful as to why CR50_USE_LONG_INTERRUPT_PULSES is being selected.
Done (in the separate changelist, linked above).