[PATCH 0/4] intel-ish-hid: Fixes for loading failures

[PATCH 0/4] intel-ish-hid: Fixes for loading failures

[Srinivas Pandruvada]

Two issues are reported related to loading of intel-ishtp-hid:

1. Loading failure after rmmod and then modprobe
(Patch 1/4 to Patch 3/4) address this. Here first two patches are just
clean up patches which gets used in Patch 3/4 to fix the issue.

2. Loading failure due to request_irq failure on some platforms
(Patch 4/4)

Even Xu (3):
  hid: intel-ish-hid: ipc: consolidate ish wake up operation
  hid: intel-ish-hid: ipc: Move DMA disable code to new function
  hid: intel-ish-hid: ipc: Fix driver reinit failure

Srinivas Pandruvada (1):
  hid: intel-ish-hid: request_irq failure

 drivers/hid/intel-ish-hid/ipc/ipc.c     | 107 +++++++++++++++++++++++---------
 drivers/hid/intel-ish-hid/ipc/pci-ish.c |   2 +-
 2 files changed, 79 insertions(+), 30 deletions(-)

-- 
2.7.4

[PATCH 1/4] hid: intel-ish-hid: ipc: consolidate ish wake up operation

[Srinivas Pandruvada]

From: Even Xu <even.xu@intel.com>

Same operations are done in ish_hw_start() and _ish_hw_reset() to
wakeup ISH device. Consolidate them by introducing a new function
ish_wakeup() and move the code there.

Signed-off-by: Even Xu <even.xu@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ipc/ipc.c | 45 +++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index e2517c1..d4c5721 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -638,6 +638,28 @@ irqreturn_t ish_irq_handler(int irq, void *dev_id)
 }
 
 /**
+ * ish_wakeup() - wakeup ishfw from waiting-for-host state
+ * @dev: ishtp device pointer
+ *
+ * Set the dma enable bit and send a void message to FW,
+ * it wil wakeup FW from waiting-for-host state.
+ */
+static void ish_wakeup(struct ishtp_device *dev)
+{
+	/* Set dma enable bit */
+	ish_reg_write(dev, IPC_REG_ISH_RMP2, IPC_RMP2_DMA_ENABLED);
+
+	/*
+	 * Send 0 IPC message so that ISH FW wakes up if it was already
+	 * asleep.
+	 */
+	ish_reg_write(dev, IPC_REG_HOST2ISH_DRBL, IPC_DRBL_BUSY_BIT);
+
+	/* Flush writes to doorbell and REMAP2 */
+	ish_reg_read(dev, IPC_REG_ISH_HOST_FWSTS);
+}
+
+/**
  * _ish_hw_reset() - HW reset
  * @dev: ishtp device pointer
  *
@@ -690,16 +712,8 @@ static int _ish_hw_reset(struct ishtp_device *dev)
 	csr |= PCI_D0;
 	pci_write_config_word(pdev, pdev->pm_cap + PCI_PM_CTRL, csr);
 
-	ish_reg_write(dev, IPC_REG_ISH_RMP2, IPC_RMP2_DMA_ENABLED);
-
-	/*
-	 * Send 0 IPC message so that ISH FW wakes up if it was already
-	 * asleep
-	 */
-	ish_reg_write(dev, IPC_REG_HOST2ISH_DRBL, IPC_DRBL_BUSY_BIT);
-
-	/* Flush writes to doorbell and REMAP2 */
-	ish_reg_read(dev, IPC_REG_ISH_HOST_FWSTS);
+	/* Now we can enable ISH DMA operation and wakeup ISHFW */
+	ish_wakeup(dev);
 
 	return	0;
 }
@@ -758,16 +772,9 @@ static int _ish_ipc_reset(struct ishtp_device *dev)
 int ish_hw_start(struct ishtp_device *dev)
 {
 	ish_set_host_rdy(dev);
-	/* After that we can enable ISH DMA operation */
-	ish_reg_write(dev, IPC_REG_ISH_RMP2, IPC_RMP2_DMA_ENABLED);
 
-	/*
-	 * Send 0 IPC message so that ISH FW wakes up if it was already
-	 * asleep
-	 */
-	ish_reg_write(dev, IPC_REG_HOST2ISH_DRBL, IPC_DRBL_BUSY_BIT);
-	/* Flush write to doorbell */
-	ish_reg_read(dev, IPC_REG_ISH_HOST_FWSTS);
+	/* After that we can enable ISH DMA operation and wakeup ISHFW */
+	ish_wakeup(dev);
 
 	set_host_ready(dev);
 
-- 
2.7.4

[PATCH 2/4] hid: intel-ish-hid: ipc: Move DMA disable code to new function

[Srinivas Pandruvada]

From: Even Xu <even.xu@intel.com>

Add a new function ish_disable_dma() and move DMA disable operations
here, so that this functionality can be reused.

Signed-off-by: Even Xu <even.xu@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ipc/ipc.c | 42 ++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index d4c5721..0e0dfa6 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -638,6 +638,36 @@ irqreturn_t ish_irq_handler(int irq, void *dev_id)
 }
 
 /**
+ * ish_disable_dma() - disable dma communication between host and ISHFW
+ * @dev: ishtp device pointer
+ *
+ * Clear the dma enable bit and wait for dma inactive.
+ *
+ * Return: 0 for success else error code.
+ */
+static int ish_disable_dma(struct ishtp_device *dev)
+{
+	unsigned int	dma_delay;
+
+	/* Clear the dma enable bit */
+	ish_reg_write(dev, IPC_REG_ISH_RMP2, 0);
+
+	/* wait for dma inactive */
+	for (dma_delay = 0; dma_delay < MAX_DMA_DELAY &&
+		_ish_read_fw_sts_reg(dev) & (IPC_ISH_IN_DMA);
+		dma_delay += 5)
+		mdelay(5);
+
+	if (dma_delay >= MAX_DMA_DELAY) {
+		dev_err(dev->devc,
+			"Wait for DMA inactive timeout\n");
+		return	-EBUSY;
+	}
+
+	return 0;
+}
+
+/**
  * ish_wakeup() - wakeup ishfw from waiting-for-host state
  * @dev: ishtp device pointer
  *
@@ -671,7 +701,6 @@ static int _ish_hw_reset(struct ishtp_device *dev)
 {
 	struct pci_dev *pdev = dev->pdev;
 	int	rv;
-	unsigned int	dma_delay;
 	uint16_t csr;
 
 	if (!pdev)
@@ -686,15 +715,8 @@ static int _ish_hw_reset(struct ishtp_device *dev)
 		return	-EINVAL;
 	}
 
-	/* Now trigger reset to FW */
-	ish_reg_write(dev, IPC_REG_ISH_RMP2, 0);
-
-	for (dma_delay = 0; dma_delay < MAX_DMA_DELAY &&
-		_ish_read_fw_sts_reg(dev) & (IPC_ISH_IN_DMA);
-		dma_delay += 5)
-		mdelay(5);
-
-	if (dma_delay >= MAX_DMA_DELAY) {
+	/* Disable dma communication between FW and host */
+	if (ish_disable_dma(dev)) {
 		dev_err(&pdev->dev,
 			"Can't reset - stuck with DMA in-progress\n");
 		return	-EBUSY;
-- 
2.7.4

[PATCH 3/4] hid: intel-ish-hid: ipc: Fix driver reinit failure

[Srinivas Pandruvada]

From: Even Xu <even.xu@intel.com>

When built as a module, modprobe followed by rmmod can fail because
DMA was still active. So to fix this, DMA needs to be disabled during
module exit.

This change disables DMA during modules exit and change the ISH PCI
device status to D3.

Signed-off-by: Even Xu <even.xu@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ipc/ipc.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index 0e0dfa6..66c6755 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -905,6 +905,26 @@ struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
  */
 void	ish_device_disable(struct ishtp_device *dev)
 {
+	struct pci_dev *pdev = dev->pdev;
+	uint16_t csr;
+
+	if (!pdev)
+		return;
+
+	/* Disable dma communication between FW and host */
+	if (ish_disable_dma(dev)) {
+		dev_err(&pdev->dev,
+			"Can't reset - stuck with DMA in-progress\n");
+		return;
+	}
+
+	/* Put ISH to D3 state for power saving */
+	pci_read_config_word(pdev, pdev->pm_cap + PCI_PM_CTRL, &csr);
+
+	csr &= ~PCI_PM_CTRL_STATE_MASK;
+	csr |= PCI_D3hot;
+	pci_write_config_word(pdev, pdev->pm_cap + PCI_PM_CTRL, csr);
+
 	dev->dev_state = ISHTP_DEV_DISABLED;
 	ish_clr_host_rdy(dev);
 }
-- 
2.7.4

RE: [PATCH 3/4] hid: intel-ish-hid: ipc: Fix driver reinit failure

[Ooi, Joyce]

> -----Original Message-----
> From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.com]
> Sent: Sunday, October 16, 2016 12:23 AM
> To: jikos@kernel.org
> Cc: linux-input@vger.kernel.org; Xu, Even <even.xu@intel.com>
> Subject: [PATCH 3/4] hid: intel-ish-hid: ipc: Fix driver reinit failure
> 
> From: Even Xu <even.xu@intel.com>
> 
> When built as a module, modprobe followed by rmmod can fail because DMA
> was still active. So to fix this, DMA needs to be disabled during module exit.
> 
> This change disables DMA during modules exit and change the ISH PCI device
> status to D3.
> 
> Signed-off-by: Even Xu <even.xu@intel.com>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/hid/intel-ish-hid/ipc/ipc.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
> index 0e0dfa6..66c6755 100644
> --- a/drivers/hid/intel-ish-hid/ipc/ipc.c
> +++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
> @@ -905,6 +905,26 @@ struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
>   */
>  void	ish_device_disable(struct ishtp_device *dev)
>  {
> +	struct pci_dev *pdev = dev->pdev;
> +	uint16_t csr;
> +
> +	if (!pdev)
> +		return;
> +
> +	/* Disable dma communication between FW and host */
> +	if (ish_disable_dma(dev)) {
> +		dev_err(&pdev->dev,
> +			"Can't reset - stuck with DMA in-progress\n");
> +		return;
> +	}
> +

> +	/* Put ISH to D3 state for power saving */
> +	pci_read_config_word(pdev, pdev->pm_cap + PCI_PM_CTRL, &csr);
> +
> +	csr &= ~PCI_PM_CTRL_STATE_MASK;
> +	csr |= PCI_D3hot;
> +	pci_write_config_word(pdev, pdev->pm_cap + PCI_PM_CTRL, csr);
> +

Do you think it would be cleaner if we use pci_set_power_state() ?
http://lxr.free-electrons.com/source/drivers/pci/pci.c#L877

>  	dev->dev_state = ISHTP_DEV_DISABLED;
>  	ish_clr_host_rdy(dev);
>  }
> --
> 2.7.4

Re: [PATCH 3/4] hid: intel-ish-hid: ipc: Fix driver reinit failure

[Pandruvada, Srinivas]

On Wed, 2016-10-19 at 09:58 +0000, Ooi, Joyce wrote:

[...]

> > +	/* Put ISH to D3 state for power saving */
> > +	pci_read_config_word(pdev, pdev->pm_cap + PCI_PM_CTRL,
> > &csr);
> > +
> > +	csr &= ~PCI_PM_CTRL_STATE_MASK;
> > +	csr |= PCI_D3hot;
> > +	pci_write_config_word(pdev, pdev->pm_cap + PCI_PM_CTRL,
> > csr);
> > +
> 
> Do you think it would be cleaner if we use pci_set_power_state() ?
> http://lxr.free-electrons.com/source/drivers/pci/pci.c#L877

Makes sense.
I will test and update this.

Thanks,
Srinivas

[PATCH 4/4] hid: intel-ish-hid: request_irq failure

[Srinivas Pandruvada]

On some platforms ISH interrupt is shared, which causes request_irq to
fail. This requires IRQF_SHARED irq flag.

But IRQF_NO_SUSPEND and IRQF_SHARED should not be used together, so
removed IRQF_NO_SUSPEND flag. Anyway this driver doesn't require
IRQF_NO_SUSPEND, as this interrupt is not required during "noirq" phases
of suspending and resuming devices as well as during the time when
nonboot CPUs are taken offline and brought back online.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ipc/pci-ish.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index 42f0bee..6fda0f9 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -146,7 +146,7 @@ static int ish_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	pdev->dev_flags |= PCI_DEV_FLAGS_NO_D3;
 
 	/* request and enable interrupt */
-	ret = request_irq(pdev->irq, ish_irq_handler, IRQF_NO_SUSPEND,
+	ret = request_irq(pdev->irq, ish_irq_handler, IRQF_SHARED,
 			  KBUILD_MODNAME, dev);
 	if (ret) {
 		dev_err(&pdev->dev, "ISH: request IRQ failure (%d)\n",
-- 
2.7.4