[v4,8/8] i2c: designware: Add doorbell support for Skyrim

Message ID 20230310211954.2490-9-mario.limonciello@amd.com
State New
Headers
Series Export platform features from ccp driver |

Commit Message

Mario Limonciello March 10, 2023, 9:19 p.m. UTC
  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

Jarkko Nikula March 13, 2023, 10:29 a.m. UTC | #1
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?
  
Andy Shevchenko March 13, 2023, 11:34 a.m. UTC | #2
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).
  
Mark Hasemeyer March 20, 2023, 7:04 p.m. UTC | #3
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.
  
Mark Hasemeyer March 20, 2023, 7:31 p.m. UTC | #4
> +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.
  

Patch

diff --git a/drivers/i2c/busses/i2c-designware-amdpsp.c b/drivers/i2c/busses/i2c-designware-amdpsp.c
index 105584abcf8f..e5614d69c743 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,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;