Furquan Shaikh 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 13:
(9 comments)
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pmclib.h:
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... PS13, Line 257: uint32_t Shouldn't this be const struct pmc_ipc_cmd *cmd?
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... PS13, Line 257: struct const
https://review.coreboot.org/c/coreboot/+/37871/10/src/soc/intel/common/block... File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/37871/10/src/soc/intel/common/block... PS10, Line 645: PMC_IPC_CMD_SIZE
Done
Oops. I caused this confusion, but this is just a register living in the PMC memory mapped register space. Please disregard my comment.
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... PS13, Line 34: nit: Use tabs for consistency?
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... PS13, Line 45: nit: space instead of tab.
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... PS13, Line 47: #define PMC_IPC_USBC_CMD_ID 0xA7 nit: Move this above before PMC_IPC_STS_OFFSET since I believe CMD_ID is related to PMC_IPC_CMD_OFFSET
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... PS13, Line 618: (void *)pmcbase + PMC_IPC_STS_OFFSET Shouldn't this be (void *)(pmcbase + PMC_IPC_STS_OFFSET)?
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... PS13, Line 636: uintptr_t
looks like pmcbase is being cast to (void *) everywhere […]
I think the uses of (void *) are not correct. Those should be (void *)(pmcbase + xyz)
https://review.coreboot.org/c/coreboot/+/37871/13/src/soc/intel/common/block... PS13, Line 640: inlen > PMC_IPC_DATA_SIZE What is the significance of this check now? The caller can never pass in more data than what pmc_ipc_buffer allows.
-
Furquan Shaikh (Code Review)