Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46312 )
Change subject: soc/intel/common: Add support for CSE Lite driver to handle CSE RW and metadata ......................................................................
Patch Set 10:
(18 comments)
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... File src/soc/intel/common/block/cse/Kconfig:
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 33: FW_MAIN_A CSE_RW_A
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 40: FW_MAIN_B CSE_RW_B
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 47: CSE_LITE_SKU_NO_ERROR = 0, nit: blank line after this to match the other entries in the enum.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 49: = 1, General comment: Why do we need to manually assign numbers to each of the enums? It should be fine to drop the "= <number>" since these are just assigned one after the other.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 159: struct cse_cbfs_rw_info { This structure definition is not required. This is exactly the same as `struct cse_rw_metadata`. We don't need to define a separate structure in this file.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 436: cse_get_cbfs_rdev This function can now be completely dropped. See my comments below.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 523: if the CSE CBFS RW placed outside : * of FW_MAIN_A and FW_MAIN_B. This is currently always true. I don't think this comment is required.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 535: return NULL; deadcode.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 590: source_info cse_rw_metadata
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 601: : if (source_info->fw_ver.major != cse_rw_ver->major) : return source_info->fw_ver.major - cse_rw_ver->major; : else if (source_info->fw_ver.minor != cse_rw_ver->minor) : return source_info->fw_ver.minor - cse_rw_ver->minor; : else if (source_info->fw_ver.hotfix != cse_rw_ver->hotfix) : return source_info->fw_ver.hotfix - cse_rw_ver->hotfix; : else : return source_info->fw_ver.build - cse_rw_ver->build; This should be changed to: return memcmp(cse_rw_ver, cse_rw_metadata->fw_version, sizeof(*cse_rw_ver));
There is no benefit of comparing each field individually here. memcmp already provides you the same functionality.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 764: *source_info cse_rw_metadata
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 768: cse_get_cbfs_rw Probably name cse_get_source_rdev()?
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 771: enum csme_failure_reason rv; Place all the declarations at the start of the function.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 772: rdev_mmap(&source_rdev, 0, region_device_sz(&source_rdev)); rdev_mmap_full(&source_rdev);
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 794: const struct region_device *source_info_rdev This parameter is not required at all with the Kconfig check in caller.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 803: struct cse_cbfs_rw_info source_info; : : /* Read CSE CBFS RW metadata */ : if (!cse_get_cbfs_rw_metadata(source_info_rdev, &source_info)) { : printk(BIOS_ERR, "cse_lite: Failed to get CSE RW blob metadata\n"); : return CSE_LITE_SKU_UNSPECIFIED; : } This can be simplified as mentioned below:
``` struct cse_rw_metadata cse_rw_metadata; if (cbfs_boot_load_file(CONFIG_SOC_INTEL_CSE_RW_METADATA_CBFS_NAME, &cse_rw_metadata, sizeof(cse_rw_metadata), CBFS_TYPE_STRUCT) != sizeof(cse_rw_metadata) { printk(...); return CSE_LITE_SKU_RW_BLOB_NOT_FOUND; } ```
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 811: &source_info This will then change to `&cse_rw_metadata`.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 851: struct region_device source_info_rdev; : if (cse_get_cbfs_rdev(&source_info_rdev)) Things actually become much simpler now that the CBFS file is added as a struct. All you need to do is: ``` struct cse_rw_metadata cse_rw_metadata; if (cbfs_boot_load_file(CONFIG_SOC_INTEL_CSE_RW_METADATA_CBFS_NAME, &cse_rw_metadata, sizeof(cse_rw_metadata), CBFS_TYPE_STRUCT) != sizeof(cse_rw_metadata) { printk(...); } ```
Also combining this with earlier suggestion of using Kconfig, I think this should now be something like:
``` if (CONFIG(SOC_INTEL_CSE_RW_UPDATE)) { rv = cse_fw_update(&cse_bp_info.bp_info); if (rv) cse_trigger_recovery(rv); }
... ```