| Message ID | 20230321212822.5714-1-mario.limonciello@amd.com |
|---|---|
| Headers | show |
| Series | Use CCP driver to handle PSP I2C arbitration | expand |
> +static int psp_send_i2c_req_mendocino(enum psp_i2c_req_type i2c_req_type) > +{ > + int status, ret; > + > + ret = read_poll_timeout(psp_ring_platform_doorbell, status, > + (status != -EBUSY), > + PSP_I2C_REQ_RETRY_DELAY_US, > + PSP_I2C_REQ_RETRY_CNT * PSP_I2C_REQ_RETRY_DELAY_US, > + 0, i2c_req_type); > + if (ret) > + dev_err(psp_i2c_dev, "Timed out waiting for PSP to %s I2C bus\n", > + (i2c_req_type == PSP_I2C_REQ_ACQUIRE) ? > + "release" : "acquire"); > + > + return ret ? ret : status; > +} I think we need the value of the PSP_CMDRESP_STS field returned to the caller and its status checked like in psp_send_i2c_req_cezanne. Otherwise the function won't continue to poll when the PSP_I2C_REQ_STS_BUS_BUSY bit is set. FYI - there's a test on ChromeOS to stress test I2C bus arbitration: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/hwsec/tpm_contest.go I can try to run it assuming the ToT kernel runs on skyrim.
On 3/22/23 12:01, Mark Hasemeyer wrote: >> +static int psp_send_i2c_req_mendocino(enum psp_i2c_req_type i2c_req_type) >> +{ >> + int status, ret; >> + >> + ret = read_poll_timeout(psp_ring_platform_doorbell, status, >> + (status != -EBUSY), >> + PSP_I2C_REQ_RETRY_DELAY_US, >> + PSP_I2C_REQ_RETRY_CNT * PSP_I2C_REQ_RETRY_DELAY_US, >> + 0, i2c_req_type); >> + if (ret) >> + dev_err(psp_i2c_dev, "Timed out waiting for PSP to %s I2C bus\n", >> + (i2c_req_type == PSP_I2C_REQ_ACQUIRE) ? >> + "release" : "acquire"); >> + >> + return ret ? ret : status; >> +} > I think we need the value of the PSP_CMDRESP_STS field returned to the caller > and its status checked like in psp_send_i2c_req_cezanne. Otherwise the function > won't continue to poll when the PSP_I2C_REQ_STS_BUS_BUSY bit is set. In that case it looks like psp_send_check_i2c_req isn't handling this properly for Cezanne either in this series. I think psp_send_platform_access_msg returning -EIO is going to mean that check_i2c_req_sts never gets run. So either psp_send_platform_access_msg should return 0 for that case (expecting caller to investigate more closely) or psp_send_check_i2c_req needs to special case -EIO for more investigation. I lean upon the latter unless you have a strong opinion. > > FYI - there's a test on ChromeOS to stress test I2C bus arbitration: > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/hwsec/tpm_contest.go > I can try to run it assuming the ToT kernel runs on skyrim. I would expect ToT should run fine on Skyrim. I'll adjust as you suggested if you can please test it.
> So either psp_send_platform_access_msg should return 0 for that case > (expecting caller to investigate more closely) or psp_send_check_i2c_req > needs to special case -EIO for more investigation. > > I lean upon the latter unless you have a strong opinion. I'm ok with either, but if you go with the latter, the documentation will need to be updated in psp_send_check_i2c_req as it states that -EIO is used for basic mailbox comm errors. > I would expect ToT should run fine on Skyrim. I'll adjust as you suggested > if you can please test it. Sure! I can test the next version you send out.