diff mbox series

[v11,4/5] mei: gsc: add runtime pm handlers

Message ID 20220315131157.3972238-5-alexander.usyskin@intel.com (mailing list archive)
State New, archived
Headers show
Series Add driver for GSC controller | expand

Commit Message

Alexander Usyskin March 15, 2022, 1:11 p.m. UTC 
From: Tomas Winkler <tomas.winkler@intel.com>

Implement runtime handlers for mei-gsc, to track
idle state of the device properly.

CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
V4: drop debug prints
V5: Rebase
V6: Rebase
V7: add Greg KH Reviewed-by
V8: Rebase
V9: Rebase
V11: Rebase
---
 drivers/misc/mei/gsc-me.c | 67 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

Comments

Rodrigo Vivi March 22, 2022, 4:56 p.m. UTC | #1 
On Tue, Mar 15, 2022 at 03:11:56PM +0200, Alexander Usyskin wrote:
> From: Tomas Winkler <tomas.winkler@intel.com>
> 
> Implement runtime handlers for mei-gsc, to track
> idle state of the device properly.
> 
> CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> ---
> V4: drop debug prints
> V5: Rebase
> V6: Rebase
> V7: add Greg KH Reviewed-by
> V8: Rebase
> V9: Rebase
> V11: Rebase
> ---
>  drivers/misc/mei/gsc-me.c | 67 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mei/gsc-me.c b/drivers/misc/mei/gsc-me.c
> index 58e39c00f150..32ea75f5e7aa 100644
> --- a/drivers/misc/mei/gsc-me.c
> +++ b/drivers/misc/mei/gsc-me.c
> @@ -159,7 +159,72 @@ static int __maybe_unused mei_gsc_pm_resume(struct device *device)
>  	return 0;
>  }
>  
> -static SIMPLE_DEV_PM_OPS(mei_gsc_pm_ops, mei_gsc_pm_suspend, mei_gsc_pm_resume);
> +static int __maybe_unused mei_gsc_pm_runtime_idle(struct device *device)
> +{
> +	struct mei_device *dev = dev_get_drvdata(device);
> +
> +	if (!dev)
> +		return -ENODEV;
> +	if (mei_write_is_idle(dev))
> +		pm_runtime_autosuspend(device);
> +
> +	return -EBUSY;

I see now... this function takes to the autosuspend but returns the EBUSY so
the pci subsystem won't take it. Many other drivers taking this approach.

> +}
> +
> +static int  __maybe_unused mei_gsc_pm_runtime_suspend(struct device *device)
> +{
> +	struct mei_device *dev = dev_get_drvdata(device);
> +	struct mei_me_hw *hw;
> +	int ret;
> +
> +	if (!dev)
> +		return -ENODEV;
> +
> +	mutex_lock(&dev->device_lock);
> +
> +	if (mei_write_is_idle(dev)) {
> +		hw = to_me_hw(dev);
> +		hw->pg_state = MEI_PG_ON;
> +		ret = 0;
> +	} else {
> +		ret = -EAGAIN;
> +	}
> +
> +	mutex_unlock(&dev->device_lock);
> +
> +	return ret;
> +}
> +
> +static int __maybe_unused mei_gsc_pm_runtime_resume(struct device *device)
> +{
> +	struct mei_device *dev = dev_get_drvdata(device);
> +	struct mei_me_hw *hw;
> +	irqreturn_t irq_ret;
> +
> +	if (!dev)
> +		return -ENODEV;
> +
> +	mutex_lock(&dev->device_lock);
> +
> +	hw = to_me_hw(dev);
> +	hw->pg_state = MEI_PG_OFF;
> +
> +	mutex_unlock(&dev->device_lock);
> +
> +	irq_ret = mei_me_irq_thread_handler(1, dev);
> +	if (irq_ret != IRQ_HANDLED)
> +		dev_err(dev->dev, "thread handler fail %d\n", irq_ret);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops mei_gsc_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(mei_gsc_pm_suspend,
> +				mei_gsc_pm_resume)
> +	SET_RUNTIME_PM_OPS(mei_gsc_pm_runtime_suspend,
> +			   mei_gsc_pm_runtime_resume,
> +			   mei_gsc_pm_runtime_idle)
> +};

Thank you for all the explanation.

I would prefer if you could move the autosuspend enable and autosuspend delay
to this patch instead of having it on the patch 2. Concerning some strange
behaviour in some bisect...

But that's minor and up to you:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>



>  
>  static const struct auxiliary_device_id mei_gsc_id_table[] = {
>  	{
> -- 
> 2.32.0
>
diff mbox series

Patch

diff --git a/drivers/misc/mei/gsc-me.c b/drivers/misc/mei/gsc-me.c
index 58e39c00f150..32ea75f5e7aa 100644
--- a/drivers/misc/mei/gsc-me.c
+++ b/drivers/misc/mei/gsc-me.c
@@ -159,7 +159,72 @@  static int __maybe_unused mei_gsc_pm_resume(struct device *device)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(mei_gsc_pm_ops, mei_gsc_pm_suspend, mei_gsc_pm_resume);
+static int __maybe_unused mei_gsc_pm_runtime_idle(struct device *device)
+{
+	struct mei_device *dev = dev_get_drvdata(device);
+
+	if (!dev)
+		return -ENODEV;
+	if (mei_write_is_idle(dev))
+		pm_runtime_autosuspend(device);
+
+	return -EBUSY;
+}
+
+static int  __maybe_unused mei_gsc_pm_runtime_suspend(struct device *device)
+{
+	struct mei_device *dev = dev_get_drvdata(device);
+	struct mei_me_hw *hw;
+	int ret;
+
+	if (!dev)
+		return -ENODEV;
+
+	mutex_lock(&dev->device_lock);
+
+	if (mei_write_is_idle(dev)) {
+		hw = to_me_hw(dev);
+		hw->pg_state = MEI_PG_ON;
+		ret = 0;
+	} else {
+		ret = -EAGAIN;
+	}
+
+	mutex_unlock(&dev->device_lock);
+
+	return ret;
+}
+
+static int __maybe_unused mei_gsc_pm_runtime_resume(struct device *device)
+{
+	struct mei_device *dev = dev_get_drvdata(device);
+	struct mei_me_hw *hw;
+	irqreturn_t irq_ret;
+
+	if (!dev)
+		return -ENODEV;
+
+	mutex_lock(&dev->device_lock);
+
+	hw = to_me_hw(dev);
+	hw->pg_state = MEI_PG_OFF;
+
+	mutex_unlock(&dev->device_lock);
+
+	irq_ret = mei_me_irq_thread_handler(1, dev);
+	if (irq_ret != IRQ_HANDLED)
+		dev_err(dev->dev, "thread handler fail %d\n", irq_ret);
+
+	return 0;
+}
+
+static const struct dev_pm_ops mei_gsc_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(mei_gsc_pm_suspend,
+				mei_gsc_pm_resume)
+	SET_RUNTIME_PM_OPS(mei_gsc_pm_runtime_suspend,
+			   mei_gsc_pm_runtime_resume,
+			   mei_gsc_pm_runtime_idle)
+};
 
 static const struct auxiliary_device_id mei_gsc_id_table[] = {
 	{