Attention is currently required from: Furquan Shaikh, Maulik V Vaghela, Tim Wawrzynczak, Subrata Banik, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57804 )
Change subject: soc/intel/common/../cse: Create APIs for CSE device state transition ......................................................................
Patch Set 3:
(12 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/57804/comment/56d4fd09_4bb2646a PS3, Line 10: publically nit: move to next line
File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/57804/comment/eaf114bb_aeca659d PS3, Line 22: /* Wait up to 15 sec for HECI to get ready */ : #define HECI_DELAY_READY (15 * 1000) : /* Wait up to 100 usec between circular buffer polls */ : #define HECI_DELAY 100 : /* Wait up to 5 sec for CSE to chew something we sent */ : #define HECI_SEND_TIMEOUT (5 * 1000) : /* Wait up to 5 sec for CSE to blurp a reply */ : #define HECI_READ_TIMEOUT (5 * 1000) : /* Wait up to 1 ms for CSE CIP */ : #define HECI_CIP_TIMEOUT 1000 Idea for another patch: Append the time units to the macro names, e.g.:
/* Wait up to 15 sec for HECI to get ready */ #define HECI_DELAY_READY_MS (15 * 1000) /* Wait up to 100 usec between circular buffer polls */ #define HECI_DELAY_US 100 /* Wait up to 5 sec for CSE to chew something we sent */ #define HECI_SEND_TIMEOUT_MS (5 * 1000) /* Wait up to 5 sec for CSE to blurp a reply */ #define HECI_READ_TIMEOUT_MS (5 * 1000) /* Wait up to 1 ms for CSE CIP */ #define HECI_CIP_TIMEOUT_US 1000
https://review.coreboot.org/c/coreboot/+/57804/comment/e2bbdccf_3ae74381 PS3, Line 903: void This function should return success/failure. See comment on line 939.
https://review.coreboot.org/c/coreboot/+/57804/comment/fcc3942c_980a3664 PS3, Line 910: while ((dev_idle_ctrl & CSE_DEV_CIP) == CSE_DEV_CIP) { : udelay(HECI_CIP_TIMEOUT); : dev_idle_ctrl = read_bar(MMIO_CSE_DEVIDLE); : } This results in an infinite loop if CSE fails to set the `CSE_DEV_CIP` bit, which is annoying to debug.
Also, this loop doesn't use the `HECI_CIP_TIMEOUT` value as a timeout (exit the polling loop after `HECI_CIP_TIMEOUT` time has passed), but to reduce the polling rate (reads to `MMIO_CSE_DEVIDLE` only happen once every `HECI_CIP_TIMEOUT` time).
Instead, I'd use the functionality provided by `<timer.h>` to implement the polling loop:
#include <timer.h>
static bool disable_cse_idle(void) { struct stopwatch sw; uint32_t dev_idle_ctrl = read_bar(MMIO_CSE_DEVIDLE); dev_idle_ctrl &= ~CSE_DEV_IDLE; write_bar(MMIO_CSE_DEVIDLE, dev_idle_ctrl);
stopwatch_init_usecs_expire(&sw, HECI_CIP_TIMEOUT); do { dev_idle_ctrl = read_bar(MMIO_CSE_DEVIDLE); if ((dev_idle_ctrl & CSE_DEV_CIP) == CSE_DEV_CIP) return true; udelay(HECI_DELAY); } while (!stopwatch_expired(&sw));
return false; }
The idea of `udelay(HECI_DELAY);` in the loop is to reduce the polling rate, in case reads to `MMIO_CSE_DEVIDLE` trigger CSE interrupts. Without this delay, the interrupts caused by the polling loop can slow down normal CSE operation to the point where the timeout expires before the CSE sets CSE_DEV_CIP.
With this approach, the polling time is slightly lower than `HECI_CIP_TIMEOUT`. If this is a concern, replace `return false;` with this:
dev_idle_ctrl = read_bar(MMIO_CSE_DEVIDLE); return (dev_idle_ctrl & CSE_DEV_CIP) == CSE_DEV_CIP;
https://review.coreboot.org/c/coreboot/+/57804/comment/c5af1bef_b2702d6a PS3, Line 926: if ((dev_idle_ctrl & CSE_DEV_IDLE) == CSE_DEV_IDLE) : return true; : : return false; return (dev_idle_ctrl & CSE_DEV_IDLE) == CSE_DEV_IDLE;
https://review.coreboot.org/c/coreboot/+/57804/comment/34d7f23d_11916642 PS3, Line 936: if (is_cse_idle()) { If `is_cse_idle()` returns false, the `ensure_cse_active()` function returns `DEV_IDLE`. Is this intentional?
https://review.coreboot.org/c/coreboot/+/57804/comment/dff0d90b_29c2bb58 PS3, Line 937: disable_cse_idle(); With the proposed changes to this function, its return value should be handled accordingly.
https://review.coreboot.org/c/coreboot/+/57804/comment/995fa9de_b7f92b59 PS3, Line 939: PCI_COMMAND_MASTER Is enabling Bus Master strictly required? If it is not, please don't enable it.
https://review.coreboot.org/c/coreboot/+/57804/comment/e8168c4f_d78e868f PS3, Line 946: enum cse_device_state current_state Why take the current state as a parameter? I'd just call `is_cse_idle()` instead.
https://review.coreboot.org/c/coreboot/+/57804/comment/7af88f75_f1909d6b PS3, Line 946: set_cse_to_prior_state What's this `prior state`? Why not name this function `ensure_cse_idle`?
File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/57804/comment/7a512c4d_0f194708 PS3, Line 284: enum cse_device_state { : DEV_IDLE, : DEV_ACTIVE, : }; If there aren't any other states, I wouldn't use an enum. But I think the current approach is reasonable.
https://review.coreboot.org/c/coreboot/+/57804/comment/edce4b34_1629f56d PS3, Line 289: being brings?