[PATCH v2 0/3] HID: intel-ish-hid: support s2idle and s3 in ish_suspend()

[PATCH v2 0/3] HID: intel-ish-hid: support s2idle and s3 in ish_suspend()

[Zhang Lixu]

Currently, the NO_D3 flag is set in ish_probe(), the intel-ish-ipc driver
puts the ISH into D0i3 when system enter both suspend-to-idle(S0ix) and
suspend-to-mem(S3). These patches are to put the ISH into D3 when system
enter S3 and put the ISH into D0i3 when system enter S0ix.

I tested these patches on the following platforms:
ISH 5.2: ICL
ISH 5.0: CNL CFL WHL CML
ISH 4.0: GLK
ISH 3.0: KBL

Test steps:
* Run the IIO Sensor tool to read the accel_3d sensor data
* Run cmd "echo mem > /sys/power/state" to suspend-to-mem
* Press the keyboard to wake up OS.
* Check if the tool can get the sensor data after OS resume.
* Run cmd "echo freeze > /sys/power/state" to suspend-to-idle
* Press the keyboard to wake up OS.
* Check if the tool can get the sensor data after OS resume.

Test results:
The tool can get the accel_3d sensor data after resuming from both
suspend-to-mem and suspend-to-idle.

Changes from v1:
* Fix the indentation issue
* Elaborate the reason to remove the NO_D3 flag
* Split the PATCH v1 to three changes, and try to minimize the lines change

Zhang Lixu (3):
  HID: intel-ish-hid: ipc: set NO_D3 flag only when needed
  HID: intel-ish-hid: ipc: make ish suspend paths clear
  HID: intel-ish-hid: ipc: check the NO_D3 flag to distinguish resume
    paths

 drivers/hid/intel-ish-hid/ipc/hw-ish.h  |  1 +
 drivers/hid/intel-ish-hid/ipc/ipc.c     |  2 +-
 drivers/hid/intel-ish-hid/ipc/pci-ish.c | 95 +++++++++++++++----------
 3 files changed, 61 insertions(+), 37 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] HID: intel-ish-hid: ipc: set NO_D3 flag only when needed

[Zhang Lixu]

Currently, the NO_D3 flag is set in ish_probe(), and cleared in
ish_remove(). So even if the system goes into S3, ISH is still
in D0i3 state. It makes more sense that put ISH into D3 as system
goes into S3 and put ISH into D0i3 as system goes into suspend-to-idle.
I remove the NO_D3 setting in ish_probe(), so that ISH can enter
D3 state when system enters S3. Only set N0_D3 flag when system
enters the suspend-to-idle or platform specified, and clear it
when system resume.

When the ISH enters D3, the FW will check the DMA bit status.
If the DMA bit is set, the FW will reset automatically. So the
DMA bit need be clear before putting ISH into D3 state.

Signed-off-by: Zhang Lixu <lixu.zhang@intel.com>
---
 drivers/hid/intel-ish-hid/ipc/hw-ish.h  |  1 +
 drivers/hid/intel-ish-hid/ipc/ipc.c     |  2 +-
 drivers/hid/intel-ish-hid/ipc/pci-ish.c | 21 +++++++++++++++++++--
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/hw-ish.h b/drivers/hid/intel-ish-hid/ipc/hw-ish.h
index 1065692f90e2..ddd8e8e87cfa 100644
--- a/drivers/hid/intel-ish-hid/ipc/hw-ish.h
+++ b/drivers/hid/intel-ish-hid/ipc/hw-ish.h
@@ -77,5 +77,6 @@ irqreturn_t ish_irq_handler(int irq, void *dev_id);
 struct ishtp_device *ish_dev_init(struct pci_dev *pdev);
 int ish_hw_start(struct ishtp_device *dev);
 void ish_device_disable(struct ishtp_device *dev);
+int ish_disable_dma(struct ishtp_device *dev);
 
 #endif /* _ISHTP_HW_ISH_H_ */
diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index 18fe8af89aad..8f8dfdf64833 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -672,7 +672,7 @@ irqreturn_t ish_irq_handler(int irq, void *dev_id)
  *
  * Return: 0 for success else error code.
  */
-static int ish_disable_dma(struct ishtp_device *dev)
+int ish_disable_dma(struct ishtp_device *dev)
 {
 	unsigned int	dma_delay;
 
diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index aa80b4d3b740..89b14d2edd0b 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -14,6 +14,7 @@
 #include <linux/types.h>
 #include <linux/pci.h>
 #include <linux/sched.h>
+#include <linux/suspend.h>
 #include <linux/interrupt.h>
 #include <linux/workqueue.h>
 #define CREATE_TRACE_POINTS
@@ -97,6 +98,11 @@ static const struct pci_device_id ish_invalid_pci_ids[] = {
 	{}
 };
 
+static inline bool ish_should_enter_d0i3(struct pci_dev *pdev)
+{
+	return !pm_suspend_via_firmware() || pdev->device == CHV_DEVICE_ID;
+}
+
 /**
  * ish_probe() - PCI driver probe callback
  * @pdev:	pci device
@@ -147,7 +153,6 @@ static int ish_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* mapping IO device memory */
 	hw->mem_addr = pcim_iomap_table(pdev)[0];
 	ishtp->pdev = pdev;
-	pdev->dev_flags |= PCI_DEV_FLAGS_NO_D3;
 
 	/* request and enable interrupt */
 	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
@@ -184,7 +189,6 @@ static void ish_remove(struct pci_dev *pdev)
 	struct ishtp_device *ishtp_dev = pci_get_drvdata(pdev);
 
 	ishtp_bus_remove_all_clients(ishtp_dev, false);
-	pdev->dev_flags &= ~PCI_DEV_FLAGS_NO_D3;
 	ish_device_disable(ishtp_dev);
 }
 
@@ -209,6 +213,8 @@ static void __maybe_unused ish_resume_handler(struct work_struct *work)
 	uint32_t fwsts;
 	int ret;
 
+	pdev->dev_flags &= ~PCI_DEV_FLAGS_NO_D3;
+
 	/* Get ISH FW status */
 	fwsts = IPC_GET_ISH_FWSTS(dev->ops->get_fw_status(dev));
 
@@ -267,6 +273,17 @@ static int __maybe_unused ish_suspend(struct device *device)
 						 !dev->suspend_flag,
 						  msecs_to_jiffies(25));
 
+	if (ish_should_enter_d0i3(pdev)) {
+		/* Set the NO_D3 flag, the ISH would enter D0i3 */
+		pdev->dev_flags |= PCI_DEV_FLAGS_NO_D3;
+	} else {
+		/*
+		 * Clear the DMA bit before putting ISH into D3,
+		 * or ISH FW would reset automatically.
+		 */
+		ish_disable_dma(dev);
+	}
+
 	return 0;
 }
 
-- 
2.17.1

[PATCH v2 2/3] HID: intel-ish-hid: ipc: make ish suspend paths clear

[Zhang Lixu]

For suspend-to-idle, send suspend message and set N0_D3 flag to put
the ISH into D0i3 state.
For suspend-to-mem, disable the DMA bit before ISH entering D3, and
NO_D3 flag is cleared by default, then the ISH would enter D3.

Signed-off-by: Zhang Lixu <lixu.zhang@intel.com>
---
 drivers/hid/intel-ish-hid/ipc/pci-ish.c | 52 +++++++++++++++----------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index 89b14d2edd0b..35081f2cf781 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -223,6 +223,8 @@ static void __maybe_unused ish_resume_handler(struct work_struct *work)
 	 * it means ISH isn't powered off, in this case, send a resume message.
 	 */
 	if (fwsts >= FWSTS_SENSOR_APP_LOADED) {
+		disable_irq_wake(pdev->irq);
+
 		ishtp_send_resume(dev);
 
 		/* Waiting to get resume response */
@@ -255,27 +257,36 @@ static int __maybe_unused ish_suspend(struct device *device)
 	struct pci_dev *pdev = to_pci_dev(device);
 	struct ishtp_device *dev = pci_get_drvdata(pdev);
 
-	enable_irq_wake(pdev->irq);
-	/*
-	 * If previous suspend hasn't been asnwered then ISH is likely dead,
-	 * don't attempt nested notification
-	 */
-	if (dev->suspend_flag)
-		return	0;
-
-	dev->resume_flag = 0;
-	dev->suspend_flag = 1;
-	ishtp_send_suspend(dev);
-
-	/* 25 ms should be enough for live ISH to flush all IPC buf */
-	if (dev->suspend_flag)
-		wait_event_interruptible_timeout(dev->suspend_wait,
-						 !dev->suspend_flag,
-						  msecs_to_jiffies(25));
-
 	if (ish_should_enter_d0i3(pdev)) {
-		/* Set the NO_D3 flag, the ISH would enter D0i3 */
-		pdev->dev_flags |= PCI_DEV_FLAGS_NO_D3;
+		/*
+		 * If previous suspend hasn't been asnwered then ISH is likely
+		 * dead, don't attempt nested notification
+		 */
+		if (dev->suspend_flag)
+			return	0;
+
+		dev->resume_flag = 0;
+		dev->suspend_flag = 1;
+		ishtp_send_suspend(dev);
+
+		/* 25 ms should be enough for live ISH to flush all IPC buf */
+		if (dev->suspend_flag)
+			wait_event_interruptible_timeout(dev->suspend_wait,
+					!dev->suspend_flag,
+					msecs_to_jiffies(25));
+
+		if (dev->suspend_flag) {
+			/*
+			 * It looks like FW halt, clear the DMA bit, and put
+			 * ISH into D3, and FW would reset on resume.
+			 */
+			ish_disable_dma(dev);
+		} else {
+			/* Set the NO_D3 flag, the ISH would enter D0i3 */
+			pdev->dev_flags |= PCI_DEV_FLAGS_NO_D3;
+
+			enable_irq_wake(pdev->irq);
+		}
 	} else {
 		/*
 		 * Clear the DMA bit before putting ISH into D3,
@@ -304,7 +315,6 @@ static int __maybe_unused ish_resume(struct device *device)
 	ish_resume_device = device;
 	dev->resume_flag = 1;
 
-	disable_irq_wake(pdev->irq);
 	schedule_work(&resume_work);
 
 	return 0;
-- 
2.17.1

[PATCH v2 3/3] HID: intel-ish-hid: ipc: check the NO_D3 flag to distinguish resume paths

[Zhang Lixu]

The NO_D3 flag would be set if the ISH enter D0i3 in ish_suspend(),
The resume paths can be distinguished by checking the NO_D3 flag.
It's more reasonable than checking the FW status.

Signed-off-by: Zhang Lixu <lixu.zhang@intel.com>
---
 drivers/hid/intel-ish-hid/ipc/pci-ish.c | 34 +++++++++++--------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index 35081f2cf781..f269852304e5 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -210,19 +210,11 @@ static void __maybe_unused ish_resume_handler(struct work_struct *work)
 {
 	struct pci_dev *pdev = to_pci_dev(ish_resume_device);
 	struct ishtp_device *dev = pci_get_drvdata(pdev);
-	uint32_t fwsts;
 	int ret;
 
-	pdev->dev_flags &= ~PCI_DEV_FLAGS_NO_D3;
-
-	/* Get ISH FW status */
-	fwsts = IPC_GET_ISH_FWSTS(dev->ops->get_fw_status(dev));
-
-	/*
-	 * If currently, in ISH FW, sensor app is loaded or beyond that,
-	 * it means ISH isn't powered off, in this case, send a resume message.
-	 */
-	if (fwsts >= FWSTS_SENSOR_APP_LOADED) {
+	/* Check the NO_D3 flag to distinguish the resume paths */
+	if (pdev->dev_flags & PCI_DEV_FLAGS_NO_D3) {
+		pdev->dev_flags &= ~PCI_DEV_FLAGS_NO_D3;
 		disable_irq_wake(pdev->irq);
 
 		ishtp_send_resume(dev);
@@ -232,16 +224,20 @@ static void __maybe_unused ish_resume_handler(struct work_struct *work)
 			ret = wait_event_interruptible_timeout(dev->resume_wait,
 				!dev->resume_flag,
 				msecs_to_jiffies(WAIT_FOR_RESUME_ACK_MS));
-	}
 
-	/*
-	 * If in ISH FW, sensor app isn't loaded yet, or no resume response.
-	 * That means this platform is not S0ix compatible, or something is
-	 * wrong with ISH FW. So on resume, full reboot of ISH processor will
-	 * happen, so need to go through init sequence again.
-	 */
-	if (dev->resume_flag)
+		/*
+		 * If the flag is not cleared, something is wrong with ISH FW.
+		 * So on resume, need to go through init sequence again.
+		 */
+		if (dev->resume_flag)
+			ish_init(dev);
+	} else {
+		/*
+		 * Resume from the D3, full reboot of ISH processor will happen,
+		 * so need to go through init sequence again.
+		 */
 		ish_init(dev);
+	}
 }
 
 /**
-- 
2.17.1

Re: [PATCH v2 0/3] HID: intel-ish-hid: support s2idle and s3 in ish_suspend()

[Srinivas Pandruvada]

On Thu, 2019-08-08 at 18:21 +0800, Zhang Lixu wrote:
> Currently, the NO_D3 flag is set in ish_probe(), the intel-ish-ipc
> driver
> puts the ISH into D0i3 when system enter both suspend-to-idle(S0ix)
> and
> suspend-to-mem(S3). These patches are to put the ISH into D3 when
> system
> enter S3 and put the ISH into D0i3 when system enter S0ix.
> 
> I tested these patches on the following platforms:
> ISH 5.2: ICL
> ISH 5.0: CNL CFL WHL CML
> ISH 4.0: GLK
> ISH 3.0: KBL
> 
> Test steps:
> * Run the IIO Sensor tool to read the accel_3d sensor data
> * Run cmd "echo mem > /sys/power/state" to suspend-to-mem
> * Press the keyboard to wake up OS.
> * Check if the tool can get the sensor data after OS resume.
> * Run cmd "echo freeze > /sys/power/state" to suspend-to-idle
> * Press the keyboard to wake up OS.
> * Check if the tool can get the sensor data after OS resume.
> 
> Test results:
> The tool can get the accel_3d sensor data after resuming from both
> suspend-to-mem and suspend-to-idle.
> 
> Changes from v1:
> * Fix the indentation issue
> * Elaborate the reason to remove the NO_D3 flag
> * Split the PATCH v1 to three changes, and try to minimize the lines
> change
> 
> Zhang Lixu (3):
>   HID: intel-ish-hid: ipc: set NO_D3 flag only when needed
>   HID: intel-ish-hid: ipc: make ish suspend paths clear
>   HID: intel-ish-hid: ipc: check the NO_D3 flag to distinguish resume
>     paths
> 

For the series:

Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>


>  drivers/hid/intel-ish-hid/ipc/hw-ish.h  |  1 +
>  drivers/hid/intel-ish-hid/ipc/ipc.c     |  2 +-
>  drivers/hid/intel-ish-hid/ipc/pci-ish.c | 95 +++++++++++++++------
> ----
>  3 files changed, 61 insertions(+), 37 deletions(-)
> 

Re: [PATCH v2 0/3] HID: intel-ish-hid: support s2idle and s3 in ish_suspend()

[Jiri Kosina]

On Thu, 8 Aug 2019, Zhang Lixu wrote:

> Currently, the NO_D3 flag is set in ish_probe(), the intel-ish-ipc driver
> puts the ISH into D0i3 when system enter both suspend-to-idle(S0ix) and
> suspend-to-mem(S3). These patches are to put the ISH into D3 when system
> enter S3 and put the ISH into D0i3 when system enter S0ix.
> 
> I tested these patches on the following platforms:
> ISH 5.2: ICL
> ISH 5.0: CNL CFL WHL CML
> ISH 4.0: GLK
> ISH 3.0: KBL
> 
> Test steps:
> * Run the IIO Sensor tool to read the accel_3d sensor data
> * Run cmd "echo mem > /sys/power/state" to suspend-to-mem
> * Press the keyboard to wake up OS.
> * Check if the tool can get the sensor data after OS resume.
> * Run cmd "echo freeze > /sys/power/state" to suspend-to-idle
> * Press the keyboard to wake up OS.
> * Check if the tool can get the sensor data after OS resume.
> 
> Test results:
> The tool can get the accel_3d sensor data after resuming from both
> suspend-to-mem and suspend-to-idle.
> 
> Changes from v1:
> * Fix the indentation issue
> * Elaborate the reason to remove the NO_D3 flag
> * Split the PATCH v1 to three changes, and try to minimize the lines change

Applied to for-5.4/ish. Thanks,

-- 
Jiri Kosina
SUSE Labs