[PATCH v3 0/5] usb: typec: ucsi: ccg: add runtime pm support

[PATCH v3 0/5] usb: typec: ucsi: ccg: add runtime pm support

[Ajay Gupta]

Hi Heikki and Wolfram

These patches add support for runtime power management for UCSI CCGx driver.
I have tested them with NVIDIA GPU card which has i2c interface to talk to
CCG controller. I have added runtime pm support for the i2c bus driver as well.

Third version (v3) of patches fix comments from Heikki on adding common function
in ucsi.c which can be used by both ucsi_ccg and ucsi_acpi.

First patch refactors master_xfer() of i2c driver fixing comment from Wolfram.
Second and third patch add support for runtime pm in i2c bus driver and UCSI CCGx
driver.

Last two patches add workaround for an old version of ccg firmware
which has known issue of missing interrupt when a device is connected
to runtime resume the ccg controller. The workaround is needed because
if a GPU card doesn't get new firmware but gets new kernel then also it
should continue to work. The workaround is to request runtime resume of
i2c client which is UCSI Cypress CCGx driver. CCG driver will call the ISR
for any connector change event only if NVIDIA GPU has old CCG firmware with
the known issue.

All the five patches should go together since ucsi_ccg driver is dependent
on i2c driver for runtime pm wokaround. I would prefer it to go through
usb-tree since usb ucsi_ccg driver is the main driver getting runtime pm
functionality with the series.

Thanks
Ajay

Ajay Gupta (5):
  i2c: nvidia-gpu: refactor master_xfer
  i2c: nvidia-gpu: add runtime pm support
  usb: typec: ucsi: ccg: enable runtime pm support
  i2c: nvidia-gpu: resume ccgx i2c client
  usb: typec: ucsi: ccg: add runtime pm workaround

 drivers/i2c/busses/i2c-nvidia-gpu.c |  60 ++++++++++----
 drivers/usb/typec/ucsi/ucsi.c       |  10 +++
 drivers/usb/typec/ucsi/ucsi.h       |   1 +
 drivers/usb/typec/ucsi/ucsi_ccg.c   | 118 +++++++++++++++++++++++++++-
 4 files changed, 169 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH v3 1/5] i2c: nvidia-gpu: refactor master_xfer

[Ajay Gupta]

From: Ajay Gupta <ajayg@nvidia.com>

Added a local variable "send_stop" to simplify "goto" statements.

The "send_stop" handles below two case
1) When first i2c start fails and so i2c stop is not sent before
exiting

2) When i2c stop failed after all transfers and we do not need to
send another stop before exiting.

Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
---
Changes from v2->v3: None

 drivers/i2c/busses/i2c-nvidia-gpu.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
index 1c8f708f212b..2d9561ec2320 100644
--- a/drivers/i2c/busses/i2c-nvidia-gpu.c
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -169,6 +169,7 @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
 {
 	struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
 	int status, status2;
+	bool send_stop = true;
 	int i, j;
 
 	/*
@@ -182,37 +183,41 @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
 			/* gpu_i2c_read has implicit start */
 			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
 			if (status < 0)
-				goto stop;
+				goto exit;
 		} else {
 			u8 addr = i2c_8bit_addr_from_msg(msgs + i);
 
 			status = gpu_i2c_start(i2cd);
 			if (status < 0) {
 				if (i == 0)
-					return status;
-				goto stop;
+					send_stop = false;
+				goto exit;
 			}
 
 			status = gpu_i2c_write(i2cd, addr);
 			if (status < 0)
-				goto stop;
+				goto exit;
 
 			for (j = 0; j < msgs[i].len; j++) {
 				status = gpu_i2c_write(i2cd, msgs[i].buf[j]);
 				if (status < 0)
-					goto stop;
+					goto exit;
 			}
 		}
 	}
 	status = gpu_i2c_stop(i2cd);
-	if (status < 0)
-		return status;
+	if (status < 0) {
+		send_stop = false;
+		goto exit;
+	}
 
 	return i;
-stop:
-	status2 = gpu_i2c_stop(i2cd);
-	if (status2 < 0)
-		dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
+exit:
+	if (send_stop) {
+		status2 = gpu_i2c_stop(i2cd);
+		if (status2 < 0)
+			dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
+	}
 	return status;
 }
 
-- 
2.17.1

[PATCH v3 2/5] i2c: nvidia-gpu: add runtime pm support

[Ajay Gupta]

From: Ajay Gupta <ajayg@nvidia.com>

Enable runtime pm support with autosuspend delay of three second.
This is to make sure I2C client device Cypress CCGx has completed
all transaction.

Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
---
Changes from v1->v2:
	- Added __maybe_unused in gpu_i2c_suspend to avoid
	warning when CONFIG_PM is disabled. 

 drivers/i2c/busses/i2c-nvidia-gpu.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
index 2d9561ec2320..28fee85135ac 100644
--- a/drivers/i2c/busses/i2c-nvidia-gpu.c
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -176,6 +176,7 @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
 	 * The controller supports maximum 4 byte read due to known
 	 * limitation of sending STOP after every read.
 	 */
+	pm_runtime_get_sync(i2cd->dev);
 	for (i = 0; i < num; i++) {
 		if (msgs[i].flags & I2C_M_RD) {
 			/* program client address before starting read */
@@ -211,6 +212,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
 		goto exit;
 	}
 
+	pm_runtime_mark_last_busy(i2cd->dev);
+	pm_runtime_put_autosuspend(i2cd->dev);
 	return i;
 exit:
 	if (send_stop) {
@@ -218,6 +221,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
 		if (status2 < 0)
 			dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
 	}
+	pm_runtime_mark_last_busy(i2cd->dev);
+	pm_runtime_put_autosuspend(i2cd->dev);
 	return status;
 }
 
@@ -337,6 +342,11 @@ static int gpu_i2c_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto del_adapter;
 	}
 
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 3000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_put_autosuspend(&pdev->dev);
+	pm_runtime_allow(&pdev->dev);
+
 	return 0;
 
 del_adapter:
@@ -350,10 +360,16 @@ static void gpu_i2c_remove(struct pci_dev *pdev)
 {
 	struct gpu_i2c_dev *i2cd = dev_get_drvdata(&pdev->dev);
 
+	pm_runtime_get_noresume(i2cd->dev);
 	i2c_del_adapter(&i2cd->adapter);
 	pci_free_irq_vectors(pdev);
 }
 
+static __maybe_unused int gpu_i2c_suspend(struct device *dev)
+{
+	return 0;
+}
+
 static __maybe_unused int gpu_i2c_resume(struct device *dev)
 {
 	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
@@ -362,7 +378,8 @@ static __maybe_unused int gpu_i2c_resume(struct device *dev)
 	return 0;
 }
 
-static UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume, NULL);
+static UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, gpu_i2c_suspend, gpu_i2c_resume,
+			    NULL);
 
 static struct pci_driver gpu_i2c_driver = {
 	.name		= "nvidia-gpu",
-- 
2.17.1

[PATCH v3 3/5] usb: typec: ucsi: ccg: enable runtime pm support

[Ajay Gupta]

From: Ajay Gupta <ajayg@nvidia.com>

The change enables runtime pm support to UCSI CCG driver.
Added ucsi_resume() function to enable notification after
system reusme. Exported both ucsi_resume() and ucsi_send_command()
symbols in ucsi.c for modular build.

Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
---
Changes from v2->v3 : None
	- Fixed comments from Heikki by adding ucsi_resume()
	in ucsi.c and used it from ucsi_ccg.c
	- Also removed ucsi_ccg_runtime_idle() and
	ucsi_ccg_suspend() which were stubs and not needed.
	- We still need stub fucntion ucsi_ccg_runtime_suspend
	for runtime pm functionality

 drivers/usb/typec/ucsi/ucsi.c     | 10 ++++++++
 drivers/usb/typec/ucsi/ucsi.h     |  1 +
 drivers/usb/typec/ucsi/ucsi_ccg.c | 38 +++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 7850b851cecd..ba288b964dc8 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -206,7 +206,17 @@ int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(ucsi_send_command);
 
+int ucsi_resume(struct ucsi *ucsi)
+{
+	struct ucsi_control ctrl;
+
+	/* Restore UCSI notification enable mask after system resume */
+	UCSI_CMD_SET_NTFY_ENABLE(ctrl, UCSI_ENABLE_NTFY_ALL);
+	return ucsi_send_command(ucsi, &ctrl, NULL, 0);
+}
+EXPORT_SYMBOL_GPL(ucsi_resume);
 /* -------------------------------------------------------------------------- */
 
 void ucsi_altmode_update_active(struct ucsi_connector *con)
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 1e2981aef629..de87d0b8319d 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -430,6 +430,7 @@ int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
 		      void *retval, size_t size);
 
 void ucsi_altmode_update_active(struct ucsi_connector *con);
+int ucsi_resume(struct ucsi *ucsi);
 
 #if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
 struct typec_altmode *
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 9d46aa9e4e35..b15bc6c29c46 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -14,6 +14,8 @@
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
 
 #include <asm/unaligned.h>
 #include "ucsi.h"
@@ -210,6 +212,7 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	if (quirks && quirks->max_read_len)
 		max_read_len = quirks->max_read_len;
 
+	pm_runtime_get_sync(uc->dev);
 	while (rem_len > 0) {
 		msgs[1].buf = &data[len - rem_len];
 		rlen = min_t(u16, rem_len, max_read_len);
@@ -218,12 +221,14 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 		status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
 		if (status < 0) {
 			dev_err(uc->dev, "i2c_transfer failed %d\n", status);
+			pm_runtime_put_sync(uc->dev);
 			return status;
 		}
 		rab += rlen;
 		rem_len -= rlen;
 	}
 
+	pm_runtime_put_sync(uc->dev);
 	return 0;
 }
 
@@ -249,13 +254,16 @@ static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	msgs[0].len = len + sizeof(rab);
 	msgs[0].buf = buf;
 
+	pm_runtime_get_sync(uc->dev);
 	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
 	if (status < 0) {
 		dev_err(uc->dev, "i2c_transfer failed %d\n", status);
+		pm_runtime_put_sync(uc->dev);
 		kfree(buf);
 		return status;
 	}
 
+	pm_runtime_put_sync(uc->dev);
 	kfree(buf);
 	return 0;
 }
@@ -1134,6 +1142,10 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 	if (status)
 		dev_err(uc->dev, "cannot create sysfs group: %d\n", status);
 
+	pm_runtime_set_active(uc->dev);
+	pm_runtime_enable(uc->dev);
+	pm_runtime_idle(uc->dev);
+
 	return 0;
 }
 
@@ -1143,6 +1155,7 @@ static int ucsi_ccg_remove(struct i2c_client *client)
 
 	cancel_work_sync(&uc->work);
 	ucsi_unregister_ppm(uc->ucsi);
+	pm_runtime_disable(uc->dev);
 	free_irq(uc->irq, uc);
 	sysfs_remove_group(&uc->dev->kobj, &ucsi_ccg_attr_group);
 
@@ -1155,9 +1168,34 @@ static const struct i2c_device_id ucsi_ccg_device_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id);
 
+static int ucsi_ccg_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ucsi_ccg *uc = i2c_get_clientdata(client);
+
+	return ucsi_resume(uc->ucsi);
+}
+
+static int ucsi_ccg_runtime_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int ucsi_ccg_runtime_resume(struct device *dev)
+{
+	return 0;
+}
+
+static const struct dev_pm_ops ucsi_ccg_pm = {
+	.resume = ucsi_ccg_resume,
+	.runtime_suspend = ucsi_ccg_runtime_suspend,
+	.runtime_resume = ucsi_ccg_runtime_resume,
+};
+
 static struct i2c_driver ucsi_ccg_driver = {
 	.driver = {
 		.name = "ucsi_ccg",
+		.pm = &ucsi_ccg_pm,
 	},
 	.probe = ucsi_ccg_probe,
 	.remove = ucsi_ccg_remove,
-- 
2.17.1

[PATCH v3 4/5] i2c: nvidia-gpu: resume ccgx i2c client

[Ajay Gupta]

From: Ajay Gupta <ajayg@nvidia.com>

Cypress USB Type-C CCGx controller firmware version 3.1.10
(which is being used in many NVIDIA GPU cards) has known issue of
not triggering interrupt when a USB device is hot plugged to runtime
resume the controller. If any GPU card gets latest kernel with runtime
pm support but does not get latest fixed firmware then also it should
continue to work and therefore a workaround is required to check for
any connector change event

The workaround is to request runtime resume of i2c client
which is UCSI Cypress CCGx driver. CCG driver will call the ISR
for any connector change event only if NVIDIA GPU has old
CCG firmware with the known issue.

Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
---
Changes from v2->v3: None

 drivers/i2c/busses/i2c-nvidia-gpu.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
index 28fee85135ac..957b39c9760f 100644
--- a/drivers/i2c/busses/i2c-nvidia-gpu.c
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -51,6 +51,7 @@ struct gpu_i2c_dev {
 	void __iomem *regs;
 	struct i2c_adapter adapter;
 	struct i2c_board_info *gpu_ccgx_ucsi;
+	struct i2c_client *ccgx_client;
 };
 
 static void gpu_enable_i2c_bus(struct gpu_i2c_dev *i2cd)
@@ -271,8 +272,6 @@ static const struct property_entry ccgx_props[] = {
 
 static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq)
 {
-	struct i2c_client *ccgx_client;
-
 	i2cd->gpu_ccgx_ucsi = devm_kzalloc(i2cd->dev,
 					   sizeof(*i2cd->gpu_ccgx_ucsi),
 					   GFP_KERNEL);
@@ -284,8 +283,8 @@ static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq)
 	i2cd->gpu_ccgx_ucsi->addr = 0x8;
 	i2cd->gpu_ccgx_ucsi->irq = irq;
 	i2cd->gpu_ccgx_ucsi->properties = ccgx_props;
-	ccgx_client = i2c_new_device(&i2cd->adapter, i2cd->gpu_ccgx_ucsi);
-	if (!ccgx_client)
+	i2cd->ccgx_client = i2c_new_device(&i2cd->adapter, i2cd->gpu_ccgx_ucsi);
+	if (!i2cd->ccgx_client)
 		return -ENODEV;
 
 	return 0;
@@ -375,6 +374,13 @@ static __maybe_unused int gpu_i2c_resume(struct device *dev)
 	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
 
 	gpu_enable_i2c_bus(i2cd);
+	/*
+	 * Runtime resume ccgx client so that it can see for any
+	 * connector change event. Old ccg firmware has known
+	 * issue of not triggering interrupt when a device is
+	 * connected to runtime resume the controller.
+	 */
+	pm_request_resume(&i2cd->ccgx_client->dev);
 	return 0;
 }
 
-- 
2.17.1

[PATCH v3 5/5] usb: typec: ucsi: ccg: add runtime pm workaround

[Ajay Gupta]

From: Ajay Gupta <ajayg@nvidia.com>

Cypress USB Type-C CCGx controller firmware version 3.1.10
(which is being used in many NVIDIA GPU cards) has known issue of
not triggering interrupt when a USB device is hot plugged to runtime
resume the controller. If any GPU card gets latest kernel with runtime
pm support but does not get latest fixed firmware then also it should
continue to work and therefore a workaround is required to check for
any connector change event.

The workaround is that i2c bus driver will call pm_request_resume()
to runtime resume ucsi_ccg driver. CCG driver will call the ISR
for any connector change event for NVIDIA GPU card and only if it has
old CCG firmware with the known issue.

Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
---
Changes from v2->v3: None

 drivers/usb/typec/ucsi/ucsi_ccg.c | 80 +++++++++++++++++++++++++++++--
 1 file changed, 76 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index b15bc6c29c46..a5b81c011148 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -109,12 +109,21 @@ struct version_format {
 	__le16 build;
 	u8 patch;
 	u8 ver;
+#define CCG_VERSION_PATCH(x) ((x) << 16)
+#define CCG_VERSION(x)	((x) << 24)
 #define CCG_VERSION_MIN_SHIFT (0)
 #define CCG_VERSION_MIN_MASK (0xf << CCG_VERSION_MIN_SHIFT)
 #define CCG_VERSION_MAJ_SHIFT (4)
 #define CCG_VERSION_MAJ_MASK (0xf << CCG_VERSION_MAJ_SHIFT)
 } __packed;
 
+/*
+ * Firmware version 3.1.10 or earlier, built for NVIDIA has known issue
+ * of missing interrupt when a device is connected for runtime resume
+ */
+#define CCG_FW_BUILD_NVIDIA	(('n' << 8) | 'v')
+#define CCG_OLD_FW_VERSION	(CCG_VERSION(0x31) | CCG_VERSION_PATCH(10))
+
 struct version_info {
 	struct version_format base;
 	struct version_format app;
@@ -172,6 +181,7 @@ struct ucsi_ccg {
 	struct ccg_dev_info info;
 	/* version info for boot, primary and secondary */
 	struct version_info version[FW2 + 1];
+	u32 fw_version;
 	/* CCG HPI communication flags */
 	unsigned long flags;
 #define RESET_PENDING	0
@@ -185,6 +195,8 @@ struct ucsi_ccg {
 
 	/* fw build with vendor information */
 	u16 fw_build;
+	bool run_isr; /* flag to call ISR routine during resume */
+	struct work_struct pm_work;
 };
 
 static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
@@ -212,6 +224,18 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	if (quirks && quirks->max_read_len)
 		max_read_len = quirks->max_read_len;
 
+	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
+	    uc->fw_version <= CCG_OLD_FW_VERSION) {
+		mutex_lock(&uc->lock);
+		/*
+		 * Do not schedule pm_work to run ISR in
+		 * ucsi_ccg_runtime_resume() after pm_runtime_get_sync()
+		 * since we are already in ISR path.
+		 */
+		uc->run_isr = false;
+		mutex_unlock(&uc->lock);
+	}
+
 	pm_runtime_get_sync(uc->dev);
 	while (rem_len > 0) {
 		msgs[1].buf = &data[len - rem_len];
@@ -254,6 +278,18 @@ static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	msgs[0].len = len + sizeof(rab);
 	msgs[0].buf = buf;
 
+	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
+	    uc->fw_version <= CCG_OLD_FW_VERSION) {
+		mutex_lock(&uc->lock);
+		/*
+		 * Do not schedule pm_work to run ISR in
+		 * ucsi_ccg_runtime_resume() after pm_runtime_get_sync()
+		 * since we are already in ISR path.
+		 */
+		uc->run_isr = false;
+		mutex_unlock(&uc->lock);
+	}
+
 	pm_runtime_get_sync(uc->dev);
 	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
 	if (status < 0) {
@@ -383,6 +419,13 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static void ccg_pm_workaround_work(struct work_struct *pm_work)
+{
+	struct ucsi_ccg *uc = container_of(pm_work, struct ucsi_ccg, pm_work);
+
+	ucsi_notify(uc->ucsi);
+}
+
 static int get_fw_info(struct ucsi_ccg *uc)
 {
 	int err;
@@ -392,6 +435,9 @@ static int get_fw_info(struct ucsi_ccg *uc)
 	if (err < 0)
 		return err;
 
+	uc->fw_version = CCG_VERSION(uc->version[FW2].app.ver) |
+			CCG_VERSION_PATCH(uc->version[FW2].app.patch);
+
 	err = ccg_read(uc, CCGX_RAB_DEVICE_MODE, (u8 *)(&uc->info),
 		       sizeof(uc->info));
 	if (err < 0)
@@ -740,11 +786,12 @@ static bool ccg_check_fw_version(struct ucsi_ccg *uc, const char *fw_name,
 	}
 
 	/* compare input version with FWCT version */
-	cur_version = le16_to_cpu(app->build) | app->patch << 16 |
-			app->ver << 24;
+	cur_version = le16_to_cpu(app->build) | CCG_VERSION_PATCH(app->patch) |
+			CCG_VERSION(app->ver);
 
-	new_version = le16_to_cpu(fw_cfg.app.build) | fw_cfg.app.patch << 16 |
-			fw_cfg.app.ver << 24;
+	new_version = le16_to_cpu(fw_cfg.app.build) |
+			CCG_VERSION_PATCH(fw_cfg.app.patch) |
+			CCG_VERSION(fw_cfg.app.ver);
 
 	if (!ccg_check_vendor_version(uc, app, &fw_cfg))
 		goto out_release_firmware;
@@ -1084,8 +1131,10 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 	uc->ppm.sync = ucsi_ccg_sync;
 	uc->dev = dev;
 	uc->client = client;
+	uc->run_isr = true;
 	mutex_init(&uc->lock);
 	INIT_WORK(&uc->work, ccg_update_firmware);
+	INIT_WORK(&uc->pm_work, ccg_pm_workaround_work);
 
 	/* Only fail FW flashing when FW build information is not provided */
 	status = device_property_read_u16(dev, "ccgx,firmware-build",
@@ -1153,6 +1202,7 @@ static int ucsi_ccg_remove(struct i2c_client *client)
 {
 	struct ucsi_ccg *uc = i2c_get_clientdata(client);
 
+	cancel_work_sync(&uc->pm_work);
 	cancel_work_sync(&uc->work);
 	ucsi_unregister_ppm(uc->ucsi);
 	pm_runtime_disable(uc->dev);
@@ -1183,6 +1233,28 @@ static int ucsi_ccg_runtime_suspend(struct device *dev)
 
 static int ucsi_ccg_runtime_resume(struct device *dev)
 {
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ucsi_ccg *uc = i2c_get_clientdata(client);
+	bool schedule = true;
+
+	/*
+	 * Firmware version 3.1.10 or earlier, built for NVIDIA has known issue
+	 * of missing interrupt when a device is connected for runtime resume.
+	 * Schedule a work to call ISR as a workaround.
+	 */
+	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
+	    uc->fw_version <= CCG_OLD_FW_VERSION) {
+		mutex_lock(&uc->lock);
+		if (!uc->run_isr) {
+			uc->run_isr = true;
+			schedule = false;
+		}
+		mutex_unlock(&uc->lock);
+
+		if (schedule)
+			schedule_work(&uc->pm_work);
+	}
+
 	return 0;
 }
 
-- 
2.17.1

Re: [PATCH v3 1/5] i2c: nvidia-gpu: refactor master_xfer

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

On Wed, May 22, 2019 at 11:31:38AM -0700, Ajay Gupta wrote:
> From: Ajay Gupta <ajayg@nvidia.com>
> 
> Added a local variable "send_stop" to simplify "goto" statements.
> 
> The "send_stop" handles below two case
> 1) When first i2c start fails and so i2c stop is not sent before
> exiting
> 
> 2) When i2c stop failed after all transfers and we do not need to
> send another stop before exiting.
> 
> Signed-off-by: Ajay Gupta <ajayg@nvidia.com>

Nice! It was only a brainstorming suggestion to use a dedicated flag.
But I must say I like the outcome; much better readable code IMO.

Suggested-by: Wolfram Sang <wsa@the-dreams.de>
Reviewed-by: Wolfram Sang <wsa@the-dreams.de>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/5] i2c: nvidia-gpu: add runtime pm support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 815 bytes --]


> @@ -211,6 +212,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
>  		goto exit;
>  	}
>  
> +	pm_runtime_mark_last_busy(i2cd->dev);
> +	pm_runtime_put_autosuspend(i2cd->dev);
>  	return i;
>  exit:
>  	if (send_stop) {
> @@ -218,6 +221,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
>  		if (status2 < 0)
>  			dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
>  	}
> +	pm_runtime_mark_last_busy(i2cd->dev);
> +	pm_runtime_put_autosuspend(i2cd->dev);

Maybe another worthwhile refactorization possible here? The exit code
path and 'all good' code path look very similar. This can be done
incrementally, though. I think for now it is okay.

> +static __maybe_unused int gpu_i2c_suspend(struct device *dev)
> +{
> +	return 0;
> +}

Why do we need this?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 4/5] i2c: nvidia-gpu: resume ccgx i2c client

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

On Wed, May 22, 2019 at 11:31:41AM -0700, Ajay Gupta wrote:
> From: Ajay Gupta <ajayg@nvidia.com>
> 
> Cypress USB Type-C CCGx controller firmware version 3.1.10
> (which is being used in many NVIDIA GPU cards) has known issue of
> not triggering interrupt when a USB device is hot plugged to runtime
> resume the controller. If any GPU card gets latest kernel with runtime
> pm support but does not get latest fixed firmware then also it should
> continue to work and therefore a workaround is required to check for
> any connector change event
> 
> The workaround is to request runtime resume of i2c client
> which is UCSI Cypress CCGx driver. CCG driver will call the ISR
> for any connector change event only if NVIDIA GPU has old
> CCG firmware with the known issue.
> 
> Signed-off-by: Ajay Gupta <ajayg@nvidia.com>

Can't say if this is a valid workaround but here is my ack for modifying
the I2C realm:

Acked-by: Wolfram Sang <wsa@the-dreams.de>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v3 2/5] i2c: nvidia-gpu: add runtime pm support

[Ajay Gupta]

Hi Wolfram,

> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org <linux-usb-owner@vger.kernel.org>
> On Behalf Of Wolfram Sang
> Sent: Saturday, May 25, 2019 12:57 PM
> To: Ajay Gupta <ajaykuee@gmail.com>
> Cc: heikki.krogerus@linux.intel.com; linux-usb@vger.kernel.org; linux-
> i2c@vger.kernel.org; Ajay Gupta <ajayg@nvidia.com>
> Subject: Re: [PATCH v3 2/5] i2c: nvidia-gpu: add runtime pm support
> 
> 
> > @@ -211,6 +212,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter
> *adap,
> >  		goto exit;
> >  	}
> >
> > +	pm_runtime_mark_last_busy(i2cd->dev);
> > +	pm_runtime_put_autosuspend(i2cd->dev);
> >  	return i;
> >  exit:
> >  	if (send_stop) {
> > @@ -218,6 +221,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter
> *adap,
> >  		if (status2 < 0)
> >  			dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
> >  	}
> > +	pm_runtime_mark_last_busy(i2cd->dev);
> > +	pm_runtime_put_autosuspend(i2cd->dev);
> 
> Maybe another worthwhile refactorization possible here? The exit code path
> and 'all good' code path look very similar. 
> This can be done incrementally, though. I think for now it is okay.
Thanks for suggestion. Yes , it is possible to refactor a bit further here.
 
> > +static __maybe_unused int gpu_i2c_suspend(struct device *dev) {
> > +	return 0;
> > +}
> 
> Why do we need this?
I2C function of PCI bus remains in D0 (lspci output) without this function. 

Following document also seems to insist on this.
" For this to work, the device's driver has to provide a 
pm->runtime_suspend() callback "

Documentation/power/pci.txt 
"First, a PCI device is put into a low-power state, or suspended, with the help
of pm_schedule_suspend() or pm_runtime_suspend() which for PCI devices call
pci_pm_runtime_suspend() to do the actual job.  For this to work, the device's
driver has to provide a pm->runtime_suspend() callback (see below), which is
run by pci_pm_runtime_suspend() as the first action. If the driver's callback
returns successfully, the device's standard configuration registers are saved,
the device is prepared to generate wakeup signals and, finally, it is put into
the target low-power state."

Thanks
Ajay
> nvpublic

RE: [PATCH v3 2/5] i2c: nvidia-gpu: add runtime pm support

[Ajay Gupta]

Hi Wolfram,

> > -----Original Message-----
> > From: linux-usb-owner@vger.kernel.org <linux-usb-
> owner@vger.kernel.org>
> > On Behalf Of Wolfram Sang
> > Sent: Saturday, May 25, 2019 12:57 PM
> > To: Ajay Gupta <ajaykuee@gmail.com>
> > Cc: heikki.krogerus@linux.intel.com; linux-usb@vger.kernel.org; linux-
> > i2c@vger.kernel.org; Ajay Gupta <ajayg@nvidia.com>
> > Subject: Re: [PATCH v3 2/5] i2c: nvidia-gpu: add runtime pm support
> >
> >
> > > @@ -211,6 +212,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter
> > *adap,
> > >  		goto exit;
> > >  	}
> > >
> > > +	pm_runtime_mark_last_busy(i2cd->dev);
> > > +	pm_runtime_put_autosuspend(i2cd->dev);
> > >  	return i;
> > >  exit:
> > >  	if (send_stop) {
> > > @@ -218,6 +221,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter
> > *adap,
> > >  		if (status2 < 0)
> > >  			dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
> > >  	}
> > > +	pm_runtime_mark_last_busy(i2cd->dev);
> > > +	pm_runtime_put_autosuspend(i2cd->dev);
> >
> > Maybe another worthwhile refactorization possible here? The exit code
> path
> > and 'all good' code path look very similar.
> > This can be done incrementally, though. I think for now it is okay.
> Thanks for suggestion. Yes , it is possible to refactor a bit further here.
> 
> > > +static __maybe_unused int gpu_i2c_suspend(struct device *dev) {
> > > +	return 0;
> > > +}
> >
> > Why do we need this?
> I2C function of PCI bus remains in D0 (lspci output) without this function.

Do you still see any issue with gpu_i2c_suspend()?

Thanks
Ajay
> nvpublic
> 
> Following document also seems to insist on this.
> " For this to work, the device's driver has to provide a
> pm->runtime_suspend() callback "
> 
> Documentation/power/pci.txt
> "First, a PCI device is put into a low-power state, or suspended, with the help
> of pm_schedule_suspend() or pm_runtime_suspend() which for PCI devices
> call
> pci_pm_runtime_suspend() to do the actual job.  For this to work, the
> device's
> driver has to provide a pm->runtime_suspend() callback (see below), which
> is
> run by pci_pm_runtime_suspend() as the first action. If the driver's callback
> returns successfully, the device's standard configuration registers are saved,
> the device is prepared to generate wakeup signals and, finally, it is put into
> the target low-power state."
> 
> Thanks
> Ajay
> > nvpublic