Attention is currently required from: Tarun Tuli, Subrata Banik, Caveh Jalali, Christian Walter, Nick Vaccaro, Daisuke Nojiri, Eric Lai, Boris Mittelberg.
Derek Huang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70773 )
Change subject: chromeos/cr50_enable_update.c: Clear EC AP_IDLE flag ......................................................................
Patch Set 16:
(8 comments)
File src/mainboard/google/brya/Kconfig:
https://review.coreboot.org/c/coreboot/+/70773/comment/0a020ed4_d5e7e1a1 PS12, Line 336: config CR50_RESET_CLEAR_EC_AP_IDLE_FLAG
this should move to common code. […]
Done
File src/mainboard/google/brya/mainboard.c:
https://review.coreboot.org/c/coreboot/+/70773/comment/043ea430_53db556a PS11, Line 16: #include <ec/google/chromeec/ec.h>
nit: consider alphabetic order please
Done
https://review.coreboot.org/c/coreboot/+/70773/comment/82f6ec9a_738ff940 PS11, Line 217: __weak
i don't think you need the `weak` implementation anymore now.
Done
File src/mainboard/google/brya/mainboard.c:
https://review.coreboot.org/c/coreboot/+/70773/comment/d7d6224c_6db87f20 PS12, Line 15: #include <vendorcode/google/chromeos/chromeos.h>
we don't need this header now.
Done
File src/mainboard/google/brya/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/70773/comment/169b7814_596db0cc PS11, Line 28: variant_prepare_cr50_reset
mainboard_prepare_cr50_reset
Done
File src/vendorcode/google/chromeos/cr50_enable_update.c:
https://review.coreboot.org/c/coreboot/+/70773/comment/8f76a028_438feac5 PS13, Line 159: if (CONFIG(CR50_RESET_CLEAR_EC_AP_IDLE_FLAG)) { : /* Send EC command to clear AP_IDLE flag */ : if (!google_chromeec_reboot(0, EC_REBOOT_NO_OP, : EC_REBOOT_FLAG_CLEAR_AP_IDLE | : EC_REBOOT_FLAG_ON_AP_SHUTDOWN)) : printk(BIOS_INFO, : "Successfully clear AP_IDLE flag"); : else : printk(BIOS_ERR, : "Failed to clear EC AP_IDLE flag"); : }
suggestion: please create a helper function
Done
File src/vendorcode/google/chromeos/cr50_enable_update.c:
https://review.coreboot.org/c/coreboot/+/70773/comment/8ba20e2f_f27560ee PS15, Line 79: printk(BIOS_INFO,
nit:no need warp line. […]
Done
https://review.coreboot.org/c/coreboot/+/70773/comment/06998666_88f9c720 PS15, Line 82: printk(BIOS_ERR,
nit:no need warp line. […]
Done