Brandon Breitenstein 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 8:
(12 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 4: 2017
2017-2020
Done
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 […]
I based the coreboot implementation on the Kernel implementation and they used Bytes for inlen and 32bit word for outlen for some reason (not specified why they did that) The only reason I went the same route was to keep it in sync with the kernel implementation.
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 25: #include <soc/pm.h> : #include <stdint.h> : #include <string.h> : #include <timer.h> : #include <types.h> : #include <security/vboot/vboot_common.h>
header files inclusion should be sorted.
Will sort this was put in unsorted before the addition of the new includes (vboot_common was at bottom of list already after timer)
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 37: (1 << 0)
BIT(0) ...
Done
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.
Done
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?
this is just reading the status register nothing is actually getting sent to PMC here. Kernel has a 1 us delay between checks I can add that in but its not necessary
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 614: (
extra () not needed.
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 627: 4
should this be 3?
the maximum size for any PMC command is 4 hence using it here in case this needs to be expanded for other pmc requests
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 631: inlen
add bounds check or assert... or something.
Done
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 636:
no need to split line?
Done
https://review.coreboot.org/c/coreboot/+/37871/7/src/soc/intel/common/block/... PS7, Line 637: "Wrote %x into pmcbase WBUF0"
add newline?
Done
-
Brandon Breitenstein (Code Review)