[v4,8/8] i2c: designware: Add doorbell support for Skyrim
Commit Message
Skyrim 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/20220916131854.687371-3-jsd@semihalf.com/
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
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/i2c-designware-amdpsp.c | 40 ++++++++++++++++------
1 file changed, 30 insertions(+), 10 deletions(-)
Comments
On 3/10/23 23:19, Mario Limonciello wrote:
> Skyrim 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/20220916131854.687371-3-jsd@semihalf.com/
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> 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/i2c-designware-amdpsp.c | 40 ++++++++++++++++------
> 1 file changed, 30 insertions(+), 10 deletions(-)
>
...
> @@ -276,6 +289,13 @@ int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev)
> if (psp_i2c_dev)
> return -EEXIST;
>
> + /* Cezanne uses platform mailbox, Skyrim 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_skyrim;
> +
Is there any possibility pci_get_domain_bus_and_slot() returns NULL? Or
is 00:00.0 always present?
On Mon, Mar 13, 2023 at 12:29:50PM +0200, Jarkko Nikula wrote:
> On 3/10/23 23:19, Mario Limonciello wrote:
...
> > + if (rdev->device == 0x1630)
> > + _psp_send_i2c_req = psp_send_i2c_req_cezanne;
> > + else
> > + _psp_send_i2c_req = psp_send_i2c_req_skyrim;
>
> Is there any possibility pci_get_domain_bus_and_slot() returns NULL? Or is
> 00:00.0 always present?
Theoretically it's possible in two cases (or more?):
1) no PCI support compiled;
2) no device found (PCI-less system).
Should this function retry acquiring the I2C bus while it is busy? Similar to
how the cezanne variant works. Also wondering if the function should be named
psp_send_i2c_req_mendocino for consistency.
> +static int psp_send_i2c_req_skyrim(enum psp_i2c_req_type i2c_req_type)
> +{
> + return psp_ring_platform_doorbell(i2c_req_type);
> +}
Should this function retry acquiring the I2C bus while it is busy? Similar to
how the cezanne variant works. Also wondering if the function should be named
psp_send_i2c_req_mendocino for consistency.
Sorry about the repost, the quoted code wasn't included originally.
@@ -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,43 @@ 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_skyrim(enum psp_i2c_req_type i2c_req_type)
+{
+ return psp_ring_platform_doorbell(i2c_req_type);
+}
+
+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 +127,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 +274,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 +289,13 @@ int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev)
if (psp_i2c_dev)
return -EEXIST;
+ /* Cezanne uses platform mailbox, Skyrim 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_skyrim;
+
if (psp_check_platform_access_status())
return -EPROBE_DEFER;