caveh jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37871 )
Change subject: soc/intel/common/block: Enable PMC IPC driver ......................................................................
Patch Set 7:
(11 comments)
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/pmclib.h:
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 239: int pmc_send_ipc_cmd(uint32_t cmd, uint32_t sub, uint8_t *in, uint32_t inlen, : uint32_t *out, uint32_t outlen) looking at the implementation, it looks like inlen is in bytes while outlen is in 32bit words. that's pretty confusing. i'd prefer "len" to always be in units of bytes, but if there's a special need to count words here, please make it clear.
maybe "in" should be in words also to force the caller to round up and do any necessary padding. right now the the padding is happening silently.
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 41: PMC_IPC_XFER_TIMEOUT please suffix with _MS for clarity.
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 608: read32 is it OK to hammer the PMC in a tight loop?
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 614: ( extra () not needed.
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 617: stopwatch_duration_msecs(&sw) this is relative to current time. PMC_IPC_XFER_TIMEOUT seems to make more sense here.
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 622: uint8_t const uint8_t *?
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 627: 4 should this be 3?
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 631: inlen add bounds check or assert... or something.
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 634: ((inlen + 3) / 4) is there a powers-of-2 roundup macro in coreboot?
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 636: no need to split line?
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 637: "Wrote %x into pmcbase WBUF0" add newline?
-
caveh jalali (Code Review)