Attention is currently required from: Vijay P Hiremath, Tarun Tuli, Subrata Banik, Wonkyu Kim, Tanu Malhotra, Caveh Jalali, Li1 Feng, Kapil Porwal, Jay Patel, Boris Mittelberg, Gwendal Grignou.
Bernardo Perez Priego has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75039 )
Change subject: mb/google/rex: Add ISH support ......................................................................
Patch Set 5:
(11 comments)
File src/mainboard/google/rex/Kconfig:
https://review.coreboot.org/c/coreboot/+/75039/comment/a5ebdf4e_bcf80921 PS4, Line 9: DRIVERS_INTEL_ISH
this is not good, ISH can't be default enabled for Rex common code
Ack
File src/mainboard/google/rex/chromeos.c:
https://review.coreboot.org/c/coreboot/+/75039/comment/5022aaf2_58682647 PS4, Line 33: is_sensors_on_ap
which sensors ?
Done
https://review.coreboot.org/c/coreboot/+/75039/comment/9f13b5cf_7e7f65f4 PS4, Line 33: int
nit: bool
Done
https://review.coreboot.org/c/coreboot/+/75039/comment/24a03ad9_a9bc7c6e PS4, Line 43: CROS_EC_FW_CONFIG_SENSORS_ENABLED
do we have those CBI's being created ?
CROS_EC_FW_CONFIG_SENSORS_ENABLED is already there. We may need to add CROS_EC_FW_CONFIG_AP_SENSORS_ENABLED.
File src/mainboard/google/rex/chromeos.fmd:
https://review.coreboot.org/c/coreboot/+/75039/comment/1b1cf5fe_739fd1e5 PS5, Line 11: 4500K In case we need to increase ME_RW because of ISH.
File src/mainboard/google/rex/romstage.c:
https://review.coreboot.org/c/coreboot/+/75039/comment/794642a1_c679b91b 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. […]
Done
File src/mainboard/google/rex/variants/rex0/fw_config.c:
https://review.coreboot.org/c/coreboot/+/75039/comment/9f913975_a639cb64 PS4, Line 77: is_sensors_on_ap
should use the FW CONFIG here to override the GPIO
Ack
File src/mainboard/google/rex/variants/rex0/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/75039/comment/cbf7d40d_c06e39b6 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. […]
Ack
File src/soc/intel/meteorlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/75039/comment/406eb204_37f535d3 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.... […]
Done
File src/soc/intel/meteorlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/75039/comment/cb37e316_9c35983d PS4, Line 236: mainboard_ish_init_params(m_cfg);
why not like this […]
Done
https://review.coreboot.org/c/coreboot/+/75039/comment/19598152_b542b5cc 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
Done