[M] Change in coreboot[main]: soc/intel/cse: Back up PSR data during CSE FW downgrade

[Krishna P Bhat D (Code Review)]

Attention is currently required from: Anil Kumar K, Jérémy Compostella, Paul Menzel, Rizwan Qureshi, Subrata Banik, sridhar siricilla.

Krishna P Bhat D has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74577?usp=email )

Change subject: soc/intel/cse: Back up PSR data during CSE FW downgrade
......................................................................

Patch Set 52:

(13 comments)

File src/soc/intel/common/block/cse/cse_lite.c:

https://review.coreboot.org/c/coreboot/+/74577/comment/fedb2e11_a397c148 :
PS43, Line 138: 
              : struct psr_heci_fw_downgrade_backup_res {
              : 	struct psr_heci_header header;
              : 	uint32_t status;
              : } __packed;
              :
> > Can't we push this struct into command handler itself? […]

Done

https://review.coreboot.org/c/coreboot/+/74577/comment/1eb09778_fbc1b479 :
PS43, Line 754: PSR_BACKUP_STATUS_CMOS_OFFSET
> We cannot use cbmem-id since the status should be tracked across resets

Done

https://review.coreboot.org/c/coreboot/+/74577/comment/a844ded0_905e0c48 :
PS43, Line 760: if (cse_boot_to_rw(cse_bp_info) != CB_SUCCESS)
              : 		return;
> […]

Done

https://review.coreboot.org/c/coreboot/+/74577/comment/eaa86f80_7a67fe21 :
PS43, Line 792: 		printk(BIOS_DEBUG, "cse_lite: PSR_HECI_FW_DOWNGRADE_BACKUP command sent\n");
              : 		if (backup_psr_resp.status != PSR_STATUS_SUCCESS)
              : 			printk(BIOS_DEBUG, "cse_lite: PSR_HECI_FW_DOWNGRADE_BACKUP command"
              : 			       "returned %d\n", (int)backup_psr_resp.status)
> Need it to separate for event log in the next patch.

Done

https://review.coreboot.org/c/coreboot/+/74577/comment/ab440dd8_da6c63be :
PS43, Line 797:
> > what is the source of the backed up date ? CSE NVM? […]

Done

https://review.coreboot.org/c/coreboot/+/74577/comment/c4159a94_12d11985 :
PS43, Line 799: PSR_BACKUP_DONE
> The idea is to make sure that the final state of CMOS variable is set to BACKUP_PENDING. […]

Done

https://review.coreboot.org/c/coreboot/+/74577/comment/1428ea57_4d478203 :
PS43, Line 823: if (cse_data_clear_request(cse_bp_info) != CB_SUCCESS) {
> No, the data is held in separate region in the CSE data partition which will not be cleared. […]

Done

https://review.coreboot.org/c/coreboot/+/74577/comment/f977ea7e_f193293b :
PS43, Line 825: CB_SUCCESS
> IIRC, If the data clear failed the next boot to RW will fail and CSE will boot from RO with error. […]

Done

File src/soc/intel/common/block/cse/cse_lite.c:

https://review.coreboot.org/c/coreboot/+/74577/comment/97bf8bf2_5de722d5 :
PS50, Line 1072:
>

Acknowledged

https://review.coreboot.org/c/coreboot/+/74577/comment/058c3e57_1451ee06 :
PS50, Line 1082: goto log_and_exit
>

Acknowledged

File src/soc/intel/common/block/cse/cse_lite.c:

https://review.coreboot.org/c/coreboot/+/74577/comment/b4fce977_f0af6ac7 :
PS51, Line 1103: {
> same as below

Done

https://review.coreboot.org/c/coreboot/+/74577/comment/8079d910_5513cc14 :
PS51, Line 1105: {
> do we need a brace here ? i thought every statement is single statement ?

Done

File src/soc/intel/common/block/include/intelblocks/cse.h:

https://review.coreboot.org/c/coreboot/+/74577/comment/bfa0d82f_acf84abd :
PS17, Line 84: #define PSR_BACKUP_STATUS_CMOS_OFFSET 161
> > CBMEM cannot be used since we need to use it across reset. […]

Done