Mendocino and later platform don't use the platform feature mailbox for
communication for I2C arbitration, they rely upon ringing a doorbell.
Detect the platform by the device ID of the root port and choose the
appropriate method.
Link: https://lore.kernel.org/linux-i2c/[email protected]/
Signed-off-by: Mario Limonciello <[email protected]>
---
v4->v5:
* Poll for busy
* Rename to mendocino
* Add explicit dependency on PCI
v3->v4:
* Adjust to use PCI device ID and function pointers instead
v2->v3:
* Use CPU ID rather than ACPI ID, this will be pushed to a later patch
v1->v2:
* New patch
---
drivers/i2c/busses/Kconfig | 1 +
drivers/i2c/busses/i2c-designware-amdpsp.c | 52 +++++++++++++++++-----
2 files changed, 43 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index d53bf716f97d..2aba5ffa8b03 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -568,6 +568,7 @@ config I2C_DESIGNWARE_AMDPSP
bool "AMD PSP I2C semaphore support"
depends on ACPI
depends on I2C_DESIGNWARE_PLATFORM
+ depends on PCI
imply CRYPTO_DEV_SP_PSP
help
This driver enables managed host access to the selected I2C bus shared
diff --git a/drivers/i2c/busses/i2c-designware-amdpsp.c b/drivers/i2c/busses/i2c-designware-amdpsp.c
index 105584abcf8f..87b6cc345d1d 100644
--- a/drivers/i2c/busses/i2c-designware-amdpsp.c
+++ b/drivers/i2c/busses/i2c-designware-amdpsp.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
#include <linux/i2c.h>
+#include <linux/pci.h>
#include <linux/psp-platform-access.h>
#include <linux/psp.h>
#include <linux/workqueue.h>
@@ -36,6 +37,8 @@ static u32 psp_i2c_access_count;
static bool psp_i2c_mbox_fail;
static struct device *psp_i2c_dev;
+static int (*_psp_send_i2c_req)(enum psp_i2c_req_type);
+
/* Helper to verify status returned by PSP */
static int check_i2c_req_sts(struct psp_i2c_req *req)
{
@@ -73,34 +76,55 @@ static int psp_send_check_i2c_req(struct psp_i2c_req *req)
return check_i2c_req_sts(req);
}
-static int psp_send_i2c_req(enum psp_i2c_req_type i2c_req_type)
+static int psp_send_i2c_req_cezanne(enum psp_i2c_req_type i2c_req_type)
{
struct psp_i2c_req *req;
- unsigned long start;
int status, ret;
/* Allocate command-response buffer */
req = kzalloc(sizeof(*req), GFP_KERNEL);
if (!req)
return -ENOMEM;
-
req->hdr.payload_size = sizeof(*req);
req->type = i2c_req_type;
-
- start = jiffies;
ret = read_poll_timeout(psp_send_check_i2c_req, status,
(status != -EBUSY),
PSP_I2C_REQ_RETRY_DELAY_US,
PSP_I2C_REQ_RETRY_CNT * PSP_I2C_REQ_RETRY_DELAY_US,
0, req);
- if (ret) {
+ kfree(req);
+
+ 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");
- goto cleanup;
- }
- ret = status;
+ return ret ? ret : status;
+}
+
+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;
+}
+
+static int psp_send_i2c_req(enum psp_i2c_req_type i2c_req_type)
+{
+ unsigned long start = jiffies;
+ int ret;
+
+ ret = _psp_send_i2c_req(i2c_req_type);
if (ret) {
dev_err(psp_i2c_dev, "PSP communication error\n");
goto cleanup;
@@ -115,7 +139,6 @@ static int psp_send_i2c_req(enum psp_i2c_req_type i2c_req_type)
psp_i2c_mbox_fail = true;
}
- kfree(req);
return ret;
}
@@ -263,6 +286,8 @@ static const struct i2c_lock_operations i2c_dw_psp_lock_ops = {
int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev)
{
+ struct pci_dev *rdev;
+
if (!IS_REACHABLE(CRYPTO_DEV_CCP_DD))
return -ENODEV;
@@ -276,6 +301,13 @@ int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev)
if (psp_i2c_dev)
return -EEXIST;
+ /* Cezanne uses platform mailbox, Mendocino and later use doorbell */
+ rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0));
+ if (rdev->device == 0x1630)
+ _psp_send_i2c_req = psp_send_i2c_req_cezanne;
+ else
+ _psp_send_i2c_req = psp_send_i2c_req_mendocino;
+
if (psp_check_platform_access_status())
return -EPROBE_DEFER;
--
2.34.1
> +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.