Attention is currently required from: Tarun Tuli, Tanu Malhotra, Caveh Jalali, Li1 Feng, Kapil Porwal, Jay Patel, Bernardo Perez Priego, Boris Mittelberg, Gwendal Grignou.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75039 )
Change subject: mb/google/rex: Add ISH support ......................................................................
Patch Set 4:
(9 comments)
File src/mainboard/google/rex/Kconfig:
https://review.coreboot.org/c/coreboot/+/75039/comment/898d28c1_fdb0f59c PS4, Line 9: DRIVERS_INTEL_ISH this is not good, ISH can't be default enabled for Rex common code
File src/mainboard/google/rex/chromeos.c:
https://review.coreboot.org/c/coreboot/+/75039/comment/9e073444_f7c3ca36 PS4, Line 33: is_sensors_on_ap which sensors ?
https://review.coreboot.org/c/coreboot/+/75039/comment/fbf8ee6b_beb25bf4 PS4, Line 43: CROS_EC_FW_CONFIG_SENSORS_ENABLED do we have those CBI's being created ?
File src/mainboard/google/rex/romstage.c:
https://review.coreboot.org/c/coreboot/+/75039/comment/fc745de9_e119cf28 PS4, Line 9: void mainboard_ish_init_params(FSP_M_CONFIG *m_cfg) : { : /* Enable ISH if sensors are managed by AP. */ : m_cfg->PchIshEnable = is_devfn_enabled(PCI_DEVFN_ISH) && : is_sensors_on_ap(); : } this code doesn't belong here. check fsp_params.c
File src/mainboard/google/rex/variants/rex0/fw_config.c:
https://review.coreboot.org/c/coreboot/+/75039/comment/3080c732_04c03845 PS4, Line 77: is_sensors_on_ap should use the FW CONFIG here to override the GPIO
File src/mainboard/google/rex/variants/rex0/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/75039/comment/143f2c92_e7bcaace PS4, Line 272: device ref ish on : chip drivers/intel/ish : register "firmware_name" = ""rex_ish.bin"" : device generic 0 on end : end : end u just can't enable ISH by default. we are not packing ISH inside CSE blobs. plus i don't think u need fw name
File src/soc/intel/meteorlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/75039/comment/0166aabd_331dfb4d PS4, Line 12: void mainboard_ish_init_params(FSP_M_CONFIG *m_cfg); https://github.com/coreboot/coreboot/blob/master/src/soc/intel/meteorlake/ro...
File src/soc/intel/meteorlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/75039/comment/2b3ec0b0_c9862def PS4, Line 236: mainboard_ish_init_params(m_cfg); why not like this
``` /* Enable ISH if sensors are managed by AP. */ m_cfg->PchIshEnable = is_devfn_enabled(PCI_DEVFN_ISH) && is_sensors_on_ap(); ```
https://review.coreboot.org/c/coreboot/+/75039/comment/a823839f_1511d162 PS4, Line 352: __weak void mainboard_ish_init_params(FSP_M_CONFIG *m_cfg) : { : printk(BIOS_DEBUG, "WEAK: %s/%s called\n", __FILE__, __func__); : } don't need weak here