Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45341 )
Change subject: soc/intel/common/block/cse: Add helper function cse_send_global_reset ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45341/1/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/45341/1/src/soc/intel/common/block/... PS1, Line 666: cse_send_global_reset It is not very clear to me what the intention of adding this function is. Is it to provide a helper where the caller does not really have to provide GLOBAL_RESET as argument? The names of the two functions are so similar (request v/s send) that it does not really help understand the difference between the two. Maybe we should rename `cse_request_global_request()` to `cse_request_reset()` and this function as `cse_request_global_reset()`?
Also, I don't think we should have additional checks within this function. It should simply be a wrapper around `cse_request_global_request()`:
int cse_request_global_request(void) { return cse_request_reset(GLOBAL_RESET); }
The only additional check that you have in here is for `is_cse_enabled()`. That should be added to the above function instead of here.
https://review.coreboot.org/c/coreboot/+/45341/1/src/soc/intel/common/block/... PS1, Line 675: hfs1.data = me_read_config32(PCI_ME_HFSTS1); : if (hfs1.fields.operation_mode) : goto ret; This check is already performed by `cse_is_global_reset_allowed()` in `cse_request_global_reset()`.