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 10:
(13 comments)
https://review.coreboot.org/c/coreboot/+/37871/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37871/10//COMMIT_MSG@10 PS10, Line 10: PMC IPC driver is needed to communicate with EC to get the PD : status nit: PMC IPC driver should only be a provider of API such that any component that wants to talk to the PMC using IPC can make use of the API. It is not really the responsibility of PMC IPC driver to talk to the EC. That will be done at a different layer.
https://review.coreboot.org/c/coreboot/+/37871/10//COMMIT_MSG@12 PS10, Line 12: Can you please add appropriate partner BUG=?
https://review.coreboot.org/c/coreboot/+/37871/10/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/pmclib.h:
https://review.coreboot.org/c/coreboot/+/37871/10/src/soc/intel/common/block... PS10, Line 237: * Send PMC IPC command Can you please add comments providing more details about the params and the return value?
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 34: Please be consistent about use of tabs and spaces.
https://review.coreboot.org/c/coreboot/+/37871/10/src/soc/intel/common/block... PS10, Line 602: int Since you are using coreboot error codes, why not use cb_err_t?
https://review.coreboot.org/c/coreboot/+/37871/10/src/soc/intel/common/block... PS10, Line 607: PMC_IPC_XFER_TIMEOUT_MS How was this timeout decided? Is this documented anywhere?
https://review.coreboot.org/c/coreboot/+/37871/10/src/soc/intel/common/block... PS10, Line 610: if (ipcsts & PMC_IPC_STS_BUSY) : return CB_SUCCESS; I am confused. If the status indicates that PMC is busy, this function returns success? Shouldn't it wait until the busy bit is cleared?
https://review.coreboot.org/c/coreboot/+/37871/10/src/soc/intel/common/block... PS10, Line 614: Does there need to be a delay instead of a tight loop for checking the status register?
https://review.coreboot.org/c/coreboot/+/37871/10/src/soc/intel/common/block... PS10, Line 618: PMC_IPC_XFER_TIMEOUT_MS nit: This can go on the previous line.
https://review.coreboot.org/c/coreboot/+/37871/10/src/soc/intel/common/block... PS10, Line 620: I2C I2C? Why?
https://review.coreboot.org/c/coreboot/+/37871/10/src/soc/intel/common/block... PS10, Line 623: uint8_t I know you mentioned this on a previous patchset that inlen is in bytes and outlen in words(4-bytes) to make it consistent with the kernel. But, it looks really confusing to me. In general, if there is no other reason to do that, I would say let's just keep it bytes for both.
In fact, after reading the entire function, I think we should do this differently than what kernel is doing.
Have a structure that defines the ipc buffer (this can represent both read and write buffers):
struct pmc_ipc_buffer { uint32_t buf0; uint32_t buf1; uint32_t buf2; uint32_t buf3; }
In case of write, buf0 represents data to be written to WBUF0, buf1 to WBUF1 and so on. In case of read, buf0 represents data to be read from RBUF0, buf1 from RBUF1 and so on.
Now, different drivers can use this buffer to create their own command and response structures. In the case of TCSS connection request, it will be:
union tcss_connection_req { struct pmc_ipc_buffer wb; struct { uint8_t usage:4; uint8_t usb3_port_number:4; uint8_t usb2_port_number:4; uint8_t ufp:1; uint8_t ori_hsl:1; uint8_t ori_sbu:1; uint8_t dbg_acc:1; } }
and the response can then be: union tcss_connection_resp { struct pmc_ipc_buffer rb; struct { uint8_t usage:4; uint8_t usb3_port_number:4; uint8_t usb2_port_number:4; uint8_t rsvd:4; uint8_t status:2; } }
And the signature of this function can be: int pmc_send_ipc_cmd(uint32_t cmd, uint32_t sub, const struct pmc_ipc_buffer *wb, struct pmc_ipc_buffer *rb)
That will also let you avoid all the math that is being done for inlen and also the extra memcpy.
write32((void *)(pmcbase + PMC_IPC_WBUF0), wb->buf0); write32((void *)(pmcbase + PMC_IPC_WBUF1), wb->buf1); write32((void *)(pmcbase + PMC_IPC_WBUF2), wb->buf2); write32((void *)(pmcbase + PMC_IPC_WBUF3), wb->buf3);
BTW, depending upon whether the request/response messages are SoC specific or not, I think you can add tcss_connection_req/tcss_connection_resp as well. But, I am not sure about the status.
https://review.coreboot.org/c/coreboot/+/37871/10/src/soc/intel/common/block... PS10, Line 645: PMC_IPC_CMD Where is this register documented? Can you please email me the doc#?
I think having a structure for this would again be useful.
struct pmc_ipc_cmd { uint32_t cmd:12; uint32_t subcmd:4; uint32_t len:16; }
I just made up the bit field sizes based on what I understand of the code here. Please feel free to correct it in case there are mistakes.
https://review.coreboot.org/c/coreboot/+/37871/10/src/soc/intel/common/block... PS10, Line 645: PMC_IPC_CMD_SIZE If these are offsets, those should be named accordingly.
-
Furquan Shaikh (Code Review)