[PATCH net-next 0/7] net: ipa: use runtime PM reference counting

[PATCH net-next 0/7] net: ipa: use runtime PM reference counting

[Alex Elder]

This series does further rework of the IPA clock code so that we
rely on some of the core runtime power management code (including
its referencing counting) instead.

The first patch makes ipa_clock_get() act like pm_runtime_get_sync().

The second patch makes system suspend occur regardless of the
current reference count value, which is again more like how the
runtime PM core code behaves.

The third patch creates functions to encapsulate all hardware
suspend and resume activity.  The fourth uses those functions as
the ->runtime_suspend and ->runtime_resume power callbacks.  With
that in place, ipa_clock_get() and ipa_clock_put() are changed to
use runtime PM get and put functions when needed.

The fifth patch eliminates an extra clock reference previously used
to control system suspend.  The sixth eliminates the "IPA clock"
reference count and mutex.

The final patch replaces the one call to ipa_clock_get_additional()
with a call to pm_runtime_get_if_active(), making the former
unnecessary.

					-Alec

Alex Elder (7):
  net: ipa: have ipa_clock_get() return a value
  net: ipa: disable clock in suspend
  net: ipa: resume in ipa_clock_get()
  net: ipa: use runtime PM core
  net: ipa: get rid of extra clock reference
  net: ipa: kill IPA clock reference count
  net: ipa: kill ipa_clock_get_additional()

 drivers/net/ipa/ipa_clock.c     | 165 +++++++++++---------------------
 drivers/net/ipa/ipa_clock.h     |  18 ++--
 drivers/net/ipa/ipa_interrupt.c |   9 +-
 drivers/net/ipa/ipa_main.c      |  35 +++----
 drivers/net/ipa/ipa_modem.c     |  15 ++-
 drivers/net/ipa/ipa_smp2p.c     |  33 ++++---
 drivers/net/ipa/ipa_uc.c        |  12 ++-
 7 files changed, 121 insertions(+), 166 deletions(-)

-- 
2.27.0

[PATCH net-next 1/7] net: ipa: have ipa_clock_get() return a value

[Alex Elder]

We currently assume no errors occur when enabling or disabling the
IPA core clock and interconnects.  And although this commit exposes
errors that could occur, we generally assume this won't happen in
practice.

This commit changes ipa_clock_get() and ipa_clock_put() so each
returns a value.  The values returned are meant to mimic what the
runtime power management functions return, so we can set up error
handling here before we make the switch.  Have ipa_clock_get()
increment the reference count even if it returns an error, to match
the behavior of pm_runtime_get().

More details follow.

When taking a reference in ipa_clock_get(), return 0 for the first
reference, 1 for subsequent references, or a negative error code if
an error occurs.  Note that if ipa_clock_get() returns an error, we
must not touch hardware; in some cases such errors now cause entire
blocks of code to be skipped.

When dropping a reference in ipa_clock_put(), we return 0 or an
error code.  The error would come from ipa_clock_disable(), which
now returns what ipa_interconnect_disable() returns (either 0 or a
negative error code).  For now, callers ignore the return value;
if an error occurs, a message will have already been logged, and
little more can actually be done to improve the situation.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_clock.c     | 44 +++++++++++++++++++++------------
 drivers/net/ipa/ipa_clock.h     | 14 ++++++++---
 drivers/net/ipa/ipa_interrupt.c |  9 ++++---
 drivers/net/ipa/ipa_main.c      | 36 ++++++++++++++++-----------
 drivers/net/ipa/ipa_modem.c     | 15 +++++++----
 drivers/net/ipa/ipa_smp2p.c     | 28 +++++++++++----------
 drivers/net/ipa/ipa_uc.c        | 12 ++++++---
 7 files changed, 99 insertions(+), 59 deletions(-)

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index a67b6136e3c01..d5a8b45ee59d1 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -223,10 +223,11 @@ static int ipa_clock_enable(struct ipa *ipa)
 }
 
 /* Inverse of ipa_clock_enable() */
-static void ipa_clock_disable(struct ipa *ipa)
+static int ipa_clock_disable(struct ipa *ipa)
 {
 	clk_disable_unprepare(ipa->clock->core);
-	(void)ipa_interconnect_disable(ipa);
+
+	return ipa_interconnect_disable(ipa);
 }
 
 /* Get an IPA clock reference, but only if the reference count is
@@ -246,43 +247,51 @@ bool ipa_clock_get_additional(struct ipa *ipa)
  * Incrementing the reference count is intentionally deferred until
  * after the clock is running and endpoints are resumed.
  */
-void ipa_clock_get(struct ipa *ipa)
+int ipa_clock_get(struct ipa *ipa)
 {
 	struct ipa_clock *clock = ipa->clock;
 	int ret;
 
 	/* If the clock is running, just bump the reference count */
 	if (ipa_clock_get_additional(ipa))
-		return;
+		return 1;
 
 	/* Otherwise get the mutex and check again */
 	mutex_lock(&clock->mutex);
 
 	/* A reference might have been added before we got the mutex. */
-	if (ipa_clock_get_additional(ipa))
+	if (ipa_clock_get_additional(ipa)) {
+		ret = 1;
 		goto out_mutex_unlock;
+	}
 
 	ret = ipa_clock_enable(ipa);
-	if (!ret)
-		refcount_set(&clock->count, 1);
+
+	refcount_set(&clock->count, 1);
+
 out_mutex_unlock:
 	mutex_unlock(&clock->mutex);
+
+	return ret;
 }
 
 /* Attempt to remove an IPA clock reference.  If this represents the
  * last reference, disable the IPA clock under protection of the mutex.
  */
-void ipa_clock_put(struct ipa *ipa)
+int ipa_clock_put(struct ipa *ipa)
 {
 	struct ipa_clock *clock = ipa->clock;
+	int ret;
 
 	/* If this is not the last reference there's nothing more to do */
 	if (!refcount_dec_and_mutex_lock(&clock->count, &clock->mutex))
-		return;
+		return 0;
 
-	ipa_clock_disable(ipa);
+	ret = ipa_clock_disable(ipa);
 
 	mutex_unlock(&clock->mutex);
+
+	return ret;
 }
 
 /* Return the current IPA core clock rate */
@@ -388,7 +397,7 @@ void ipa_clock_exit(struct ipa_clock *clock)
  * ipa_suspend() - Power management system suspend callback
  * @dev:	IPA device structure
  *
- * Return:	Always returns zero
+ * Return:	0 on success, or a negative error code
  *
  * Called by the PM framework when a system suspend operation is invoked.
  * Suspends endpoints and releases the clock reference held to keep
@@ -405,16 +414,14 @@ static int ipa_suspend(struct device *dev)
 		gsi_suspend(&ipa->gsi);
 	}
 
-	ipa_clock_put(ipa);
-
-	return 0;
+	return ipa_clock_put(ipa);
 }
 
 /**
  * ipa_resume() - Power management system resume callback
  * @dev:	IPA device structure
  *
- * Return:	Always returns 0
+ * Return:	0 on success, or a negative error code
  *
  * Called by the PM framework when a system resume operation is invoked.
  * Takes an IPA clock reference to keep the clock running until suspend,
@@ -423,11 +430,16 @@ static int ipa_suspend(struct device *dev)
 static int ipa_resume(struct device *dev)
 {
 	struct ipa *ipa = dev_get_drvdata(dev);
+	int ret;
 
 	/* This clock reference will keep the IPA out of suspend
 	 * until we get a power management suspend request.
 	 */
-	ipa_clock_get(ipa);
+	ret = ipa_clock_get(ipa);
+	if (WARN_ON(ret < 0)) {
+		(void)ipa_clock_put(ipa);
+		return ret;
+	}
 
 	/* Endpoints aren't usable until setup is complete */
 	if (ipa->setup_complete) {
diff --git a/drivers/net/ipa/ipa_clock.h b/drivers/net/ipa/ipa_clock.h
index 2a0f7ff3c9e64..8692c0d98bd1c 100644
--- a/drivers/net/ipa/ipa_clock.h
+++ b/drivers/net/ipa/ipa_clock.h
@@ -54,14 +54,20 @@ void ipa_clock_exit(struct ipa_clock *clock);
  * ipa_clock_get() - Get an IPA clock reference
  * @ipa:	IPA pointer
  *
- * This call blocks if this is the first reference.
+ * Return:	0 if clock started, 1 if clock already running, or a negative
+ *		error code
+ *
+ * This call blocks if this is the first reference.  A reference is
+ * taken even if an error occurs starting the IPA clock.
  */
-void ipa_clock_get(struct ipa *ipa);
+int ipa_clock_get(struct ipa *ipa);
 
 /**
  * ipa_clock_get_additional() - Get an IPA clock reference if not first
  * @ipa:	IPA pointer
  *
+ * Return:	true if reference taken, false otherwise
+ *
  * This returns immediately, and only takes a reference if not the first
  */
 bool ipa_clock_get_additional(struct ipa *ipa);
@@ -70,10 +76,12 @@ bool ipa_clock_get_additional(struct ipa *ipa);
  * ipa_clock_put() - Drop an IPA clock reference
  * @ipa:	IPA pointer
  *
+ * Return:	0 if successful, or a negative error code
+ *
  * This drops a clock reference.  If the last reference is being dropped,
  * the clock is stopped and RX endpoints are suspended.  This call will
  * not block unless the last reference is dropped.
  */
-void ipa_clock_put(struct ipa *ipa);
+int ipa_clock_put(struct ipa *ipa);
 
 #endif /* _IPA_CLOCK_H_ */
diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
index aa37f03f4557f..934c14e066a0a 100644
--- a/drivers/net/ipa/ipa_interrupt.c
+++ b/drivers/net/ipa/ipa_interrupt.c
@@ -83,8 +83,11 @@ static irqreturn_t ipa_isr_thread(int irq, void *dev_id)
 	u32 pending;
 	u32 offset;
 	u32 mask;
+	int ret;
 
-	ipa_clock_get(ipa);
+	ret = ipa_clock_get(ipa);
+	if (WARN_ON(ret < 0))
+		goto out_clock_put;
 
 	/* The status register indicates which conditions are present,
 	 * including conditions whose interrupt is not enabled.  Handle
@@ -112,8 +115,8 @@ static irqreturn_t ipa_isr_thread(int irq, void *dev_id)
 		offset = ipa_reg_irq_clr_offset(ipa->version);
 		iowrite32(pending, ipa->reg_virt + offset);
 	}
-
-	ipa_clock_put(ipa);
+out_clock_put:
+	(void)ipa_clock_put(ipa);
 
 	return IRQ_HANDLED;
 }
diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index 25bbb456e0078..64112a6767743 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -431,7 +431,9 @@ static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
 	 * is held after initialization completes, and won't get dropped
 	 * unless/until a system suspend request arrives.
 	 */
-	ipa_clock_get(ipa);
+	ret = ipa_clock_get(ipa);
+	if (WARN_ON(ret < 0))
+		goto err_clock_put;
 
 	ipa_hardware_config(ipa, data);
 
@@ -475,7 +477,8 @@ static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
 	ipa_mem_deconfig(ipa);
 err_hardware_deconfig:
 	ipa_hardware_deconfig(ipa);
-	ipa_clock_put(ipa);
+err_clock_put:
+	(void)ipa_clock_put(ipa);
 
 	return ret;
 }
@@ -493,7 +496,7 @@ static void ipa_deconfig(struct ipa *ipa)
 	ipa->interrupt = NULL;
 	ipa_mem_deconfig(ipa);
 	ipa_hardware_deconfig(ipa);
-	ipa_clock_put(ipa);
+	(void)ipa_clock_put(ipa);
 }
 
 static int ipa_firmware_load(struct device *dev)
@@ -750,20 +753,22 @@ static int ipa_probe(struct platform_device *pdev)
 		goto err_table_exit;
 
 	/* The clock needs to be active for config and setup */
-	ipa_clock_get(ipa);
+	ret = ipa_clock_get(ipa);
+	if (WARN_ON(ret < 0))
+		goto err_clock_put;
 
 	ret = ipa_config(ipa, data);
 	if (ret)
-		goto err_clock_put;	/* Error */
+		goto err_clock_put;
 
 	dev_info(dev, "IPA driver initialized");
 
 	/* If the modem is doing early initialization, it will trigger a
-	 * call to ipa_setup() call when it has finished.  In that case
-	 * we're done here.
+	 * call to ipa_setup() when it has finished.  In that case we're
+	 * done here.
 	 */
 	if (modem_init)
-		goto out_clock_put;	/* Done; no error */
+		goto done;
 
 	/* Otherwise we need to load the firmware and have Trust Zone validate
 	 * and install it.  If that succeeds we can proceed with setup.
@@ -775,16 +780,15 @@ static int ipa_probe(struct platform_device *pdev)
 	ret = ipa_setup(ipa);
 	if (ret)
 		goto err_deconfig;
-
-out_clock_put:
-	ipa_clock_put(ipa);
+done:
+	(void)ipa_clock_put(ipa);
 
 	return 0;
 
 err_deconfig:
 	ipa_deconfig(ipa);
 err_clock_put:
-	ipa_clock_put(ipa);
+	(void)ipa_clock_put(ipa);
 	ipa_modem_exit(ipa);
 err_table_exit:
 	ipa_table_exit(ipa);
@@ -810,7 +814,9 @@ static int ipa_remove(struct platform_device *pdev)
 	struct ipa_clock *clock = ipa->clock;
 	int ret;
 
-	ipa_clock_get(ipa);
+	ret = ipa_clock_get(ipa);
+	if (WARN_ON(ret < 0))
+		goto out_clock_put;
 
 	if (ipa->setup_complete) {
 		ret = ipa_modem_stop(ipa);
@@ -826,8 +832,8 @@ static int ipa_remove(struct platform_device *pdev)
 	}
 
 	ipa_deconfig(ipa);
-
-	ipa_clock_put(ipa);
+out_clock_put:
+	(void)ipa_clock_put(ipa);
 
 	ipa_modem_exit(ipa);
 	ipa_table_exit(ipa);
diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c
index ad4019e8016ec..06e44afd2cf66 100644
--- a/drivers/net/ipa/ipa_modem.c
+++ b/drivers/net/ipa/ipa_modem.c
@@ -45,7 +45,9 @@ static int ipa_open(struct net_device *netdev)
 	struct ipa *ipa = priv->ipa;
 	int ret;
 
-	ipa_clock_get(ipa);
+	ret = ipa_clock_get(ipa);
+	if (WARN_ON(ret < 0))
+		goto err_clock_put;
 
 	ret = ipa_endpoint_enable_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]);
 	if (ret)
@@ -62,7 +64,7 @@ static int ipa_open(struct net_device *netdev)
 err_disable_tx:
 	ipa_endpoint_disable_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]);
 err_clock_put:
-	ipa_clock_put(ipa);
+	(void)ipa_clock_put(ipa);
 
 	return ret;
 }
@@ -78,7 +80,7 @@ static int ipa_stop(struct net_device *netdev)
 	ipa_endpoint_disable_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]);
 	ipa_endpoint_disable_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]);
 
-	ipa_clock_put(ipa);
+	(void)ipa_clock_put(ipa);
 
 	return 0;
 }
@@ -297,7 +299,9 @@ static void ipa_modem_crashed(struct ipa *ipa)
 	struct device *dev = &ipa->pdev->dev;
 	int ret;
 
-	ipa_clock_get(ipa);
+	ret = ipa_clock_get(ipa);
+	if (WARN_ON(ret < 0))
+		goto out_clock_put;
 
 	ipa_endpoint_modem_pause_all(ipa, true);
 
@@ -324,7 +328,8 @@ static void ipa_modem_crashed(struct ipa *ipa)
 	if (ret)
 		dev_err(dev, "error %d zeroing modem memory regions\n", ret);
 
-	ipa_clock_put(ipa);
+out_clock_put:
+	(void)ipa_clock_put(ipa);
 }
 
 static int ipa_modem_notify(struct notifier_block *nb, unsigned long action,
diff --git a/drivers/net/ipa/ipa_smp2p.c b/drivers/net/ipa/ipa_smp2p.c
index 0d15438a79e2d..f84d6523636e3 100644
--- a/drivers/net/ipa/ipa_smp2p.c
+++ b/drivers/net/ipa/ipa_smp2p.c
@@ -150,24 +150,26 @@ static void ipa_smp2p_panic_notifier_unregister(struct ipa_smp2p *smp2p)
 static irqreturn_t ipa_smp2p_modem_setup_ready_isr(int irq, void *dev_id)
 {
 	struct ipa_smp2p *smp2p = dev_id;
+	int ret;
 
 	mutex_lock(&smp2p->mutex);
 
-	if (!smp2p->disabled) {
-		int ret;
+	if (smp2p->disabled)
+		goto out_mutex_unlock;
+	smp2p->disabled = true;		/* If any others arrive, ignore them */
 
-		/* The clock needs to be active for setup */
-		ipa_clock_get(smp2p->ipa);
+	/* The clock needs to be active for setup */
+	ret = ipa_clock_get(smp2p->ipa);
+	if (WARN_ON(ret < 0))
+		goto out_clock_put;
 
-		ret = ipa_setup(smp2p->ipa);
-		if (ret)
-			dev_err(&smp2p->ipa->pdev->dev,
-				"error %d from ipa_setup()\n", ret);
-		smp2p->disabled = true;
-
-		ipa_clock_put(smp2p->ipa);
-	}
+	/* An error here won't cause driver shutdown, so warn if one occurs */
+	ret = ipa_setup(smp2p->ipa);
+	WARN(ret != 0, "error %d from ipa_setup()\n", ret);
 
+out_clock_put:
+	(void)ipa_clock_put(smp2p->ipa);
+out_mutex_unlock:
 	mutex_unlock(&smp2p->mutex);
 
 	return IRQ_HANDLED;
@@ -206,7 +208,7 @@ static void ipa_smp2p_clock_release(struct ipa *ipa)
 	if (!ipa->smp2p->clock_on)
 		return;
 
-	ipa_clock_put(ipa);
+	(void)ipa_clock_put(ipa);
 	ipa->smp2p->clock_on = false;
 }
 
diff --git a/drivers/net/ipa/ipa_uc.c b/drivers/net/ipa/ipa_uc.c
index f88ee02457d49..9c8818c390731 100644
--- a/drivers/net/ipa/ipa_uc.c
+++ b/drivers/net/ipa/ipa_uc.c
@@ -154,7 +154,7 @@ static void ipa_uc_response_hdlr(struct ipa *ipa, enum ipa_irq_id irq_id)
 	case IPA_UC_RESPONSE_INIT_COMPLETED:
 		if (ipa->uc_clocked) {
 			ipa->uc_loaded = true;
-			ipa_clock_put(ipa);
+			(void)ipa_clock_put(ipa);
 			ipa->uc_clocked = false;
 		} else {
 			dev_warn(dev, "unexpected init_completed response\n");
@@ -182,21 +182,25 @@ void ipa_uc_deconfig(struct ipa *ipa)
 	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_1);
 	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_0);
 	if (ipa->uc_clocked)
-		ipa_clock_put(ipa);
+		(void)ipa_clock_put(ipa);
 }
 
 /* Take a proxy clock reference for the microcontroller */
 void ipa_uc_clock(struct ipa *ipa)
 {
 	static bool already;
+	int ret;
 
 	if (already)
 		return;
 	already = true;		/* Only do this on first boot */
 
 	/* This clock reference dropped in ipa_uc_response_hdlr() above */
-	ipa_clock_get(ipa);
-	ipa->uc_clocked = true;
+	ret = ipa_clock_get(ipa);
+	if (WARN(ret < 0, "error %d getting proxy clock\n", ret))
+		(void)ipa_clock_put(ipa);
+
+	ipa->uc_clocked = ret >= 0;
 }
 
 /* Send a command to the microcontroller */
-- 
2.27.0

[PATCH net-next 2/7] net: ipa: disable clock in suspend

[Alex Elder]

Disable the IPA clock rather than dropping a reference to it in the
system suspend callback.  This forces the suspend to occur without
affecting existing references.

Similarly, enable the clock rather than taking a reference in
ipa_resume(), forcing a resume without changing the reference count.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_clock.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index d5a8b45ee59d1..864991f7ba4b5 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -414,7 +414,7 @@ static int ipa_suspend(struct device *dev)
 		gsi_suspend(&ipa->gsi);
 	}
 
-	return ipa_clock_put(ipa);
+	return ipa_clock_disable(ipa);
 }
 
 /**
@@ -432,14 +432,9 @@ static int ipa_resume(struct device *dev)
 	struct ipa *ipa = dev_get_drvdata(dev);
 	int ret;
 
-	/* This clock reference will keep the IPA out of suspend
-	 * until we get a power management suspend request.
-	 */
-	ret = ipa_clock_get(ipa);
-	if (WARN_ON(ret < 0)) {
-		(void)ipa_clock_put(ipa);
+	ret = ipa_clock_enable(ipa);
+	if (WARN_ON(ret < 0))
 		return ret;
-	}
 
 	/* Endpoints aren't usable until setup is complete */
 	if (ipa->setup_complete) {
-- 
2.27.0

[PATCH net-next 3/7] net: ipa: resume in ipa_clock_get()

[Alex Elder]

Introduce ipa_runtime_suspend() and ipa_runtime_resume(), which
encapsulate the activities necessary for suspending and resuming
the IPA hardware.  Call these functions from ipa_clock_get() and
ipa_clock_put() when the first reference is taken or last one is
dropped.

When the very first clock reference is taken (for ipa_config()),
setup isn't complete yet, so (as before) only the core clock gets
enabled.

When the last clock reference is dropped (after ipa_deconfig()),
ipa_teardown() will have made the setup_complete flag false, so
there too, the core clock will be stopped without affecting GSI
or the endpoints.

Otherwise these new functions will perform the desired suspend and
resume actions once setup is complete.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_clock.c | 63 ++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index 864991f7ba4b5..c0a8fdf0777f4 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -10,6 +10,7 @@
 #include <linux/device.h>
 #include <linux/interconnect.h>
 #include <linux/pm.h>
+#include <linux/pm_runtime.h>
 #include <linux/bitops.h>
 
 #include "ipa.h"
@@ -230,6 +231,38 @@ static int ipa_clock_disable(struct ipa *ipa)
 	return ipa_interconnect_disable(ipa);
 }
 
+static int ipa_runtime_suspend(struct device *dev)
+{
+	struct ipa *ipa = dev_get_drvdata(dev);
+
+	/* Endpoints aren't usable until setup is complete */
+	if (ipa->setup_complete) {
+		__clear_bit(IPA_POWER_FLAG_RESUMED, ipa->clock->flags);
+		ipa_endpoint_suspend(ipa);
+		gsi_suspend(&ipa->gsi);
+	}
+
+	return ipa_clock_disable(ipa);
+}
+
+static int ipa_runtime_resume(struct device *dev)
+{
+	struct ipa *ipa = dev_get_drvdata(dev);
+	int ret;
+
+	ret = ipa_clock_enable(ipa);
+	if (WARN_ON(ret < 0))
+		return ret;
+
+	/* Endpoints aren't usable until setup is complete */
+	if (ipa->setup_complete) {
+		gsi_resume(&ipa->gsi);
+		ipa_endpoint_resume(ipa);
+	}
+
+	return 0;
+}
+
 /* Get an IPA clock reference, but only if the reference count is
  * already non-zero.  Returns true if the additional reference was
  * added successfully, or false otherwise.
@@ -265,7 +298,7 @@ int ipa_clock_get(struct ipa *ipa)
 		goto out_mutex_unlock;
 	}
 
-	ret = ipa_clock_enable(ipa);
+	ret = ipa_runtime_resume(&ipa->pdev->dev);
 
 	refcount_set(&clock->count, 1);
 
@@ -287,7 +320,7 @@ int ipa_clock_put(struct ipa *ipa)
 	if (!refcount_dec_and_mutex_lock(&clock->count, &clock->mutex))
 		return 0;
 
-	ret = ipa_clock_disable(ipa);
+	ret = ipa_runtime_suspend(&ipa->pdev->dev);
 
 	mutex_unlock(&clock->mutex);
 
@@ -405,16 +438,7 @@ void ipa_clock_exit(struct ipa_clock *clock)
  */
 static int ipa_suspend(struct device *dev)
 {
-	struct ipa *ipa = dev_get_drvdata(dev);
-
-	/* Endpoints aren't usable until setup is complete */
-	if (ipa->setup_complete) {
-		__clear_bit(IPA_POWER_FLAG_RESUMED, ipa->clock->flags);
-		ipa_endpoint_suspend(ipa);
-		gsi_suspend(&ipa->gsi);
-	}
-
-	return ipa_clock_disable(ipa);
+	return ipa_runtime_suspend(dev);
 }
 
 /**
@@ -429,20 +453,7 @@ static int ipa_suspend(struct device *dev)
  */
 static int ipa_resume(struct device *dev)
 {
-	struct ipa *ipa = dev_get_drvdata(dev);
-	int ret;
-
-	ret = ipa_clock_enable(ipa);
-	if (WARN_ON(ret < 0))
-		return ret;
-
-	/* Endpoints aren't usable until setup is complete */
-	if (ipa->setup_complete) {
-		gsi_resume(&ipa->gsi);
-		ipa_endpoint_resume(ipa);
-	}
-
-	return 0;
+	return ipa_runtime_resume(dev);
 }
 
 const struct dev_pm_ops ipa_pm_ops = {
-- 
2.27.0

[PATCH net-next 4/7] net: ipa: use runtime PM core

[Alex Elder]

Use the runtime power management core to cause hardware suspend and
resume to occur.  Enable it in ipa_clock_init() (without autosuspend),
and disable it in ipa_clock_exit().

Use ipa_runtime_suspend() as the ->runtime_suspend power operation,
and arrange for it to be called by having ipa_clock_get() call
pm_runtime_get_sync() when the first clock reference is taken.
Similarly, use ipa_runtime_resume() as the ->runtime_resume power
operation, and pm_runtime_put() when the last IPA clock reference
is dropped.

Introduce ipa_runtime_idle() as the ->runtime_idle power operation,
and have it return a non-zero value; this way suspend will never
occur except when forced.

Use pm_runtime_force_suspend() and pm_runtime_force_resume() as the
system suspend and resume callbacks, and remove ipa_suspend() and
ipa_resume().

Store a pointer to the device structure passed to ipa_clock_init(),
so it can be used by ipa_clock_exit() to disable runtime power
management.

For now we preserve IPA clock reference counting.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_clock.c | 75 +++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index c0a8fdf0777f4..f1ee0b46da005 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -60,6 +60,7 @@ enum ipa_power_flag {
  * struct ipa_clock - IPA clocking information
  * @count:		Clocking reference count
  * @mutex:		Protects clock enable/disable
+ * @dev:		IPA device pointer
  * @core:		IPA core clock
  * @flags:		Boolean state flags
  * @interconnect_count:	Number of elements in interconnect[]
@@ -68,6 +69,7 @@ enum ipa_power_flag {
 struct ipa_clock {
 	refcount_t count;
 	struct mutex mutex; /* protects clock enable/disable */
+	struct device *dev;
 	struct clk *core;
 	DECLARE_BITMAP(flags, IPA_POWER_FLAG_COUNT);
 	u32 interconnect_count;
@@ -263,13 +265,29 @@ static int ipa_runtime_resume(struct device *dev)
 	return 0;
 }
 
+static int ipa_runtime_idle(struct device *dev)
+{
+	return -EAGAIN;
+}
+
 /* Get an IPA clock reference, but only if the reference count is
  * already non-zero.  Returns true if the additional reference was
  * added successfully, or false otherwise.
  */
 bool ipa_clock_get_additional(struct ipa *ipa)
 {
-	return refcount_inc_not_zero(&ipa->clock->count);
+	struct device *dev;
+	int ret;
+
+	if (!refcount_inc_not_zero(&ipa->clock->count))
+		return false;
+
+	dev = &ipa->pdev->dev;
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		dev_err(dev, "error %d enabling power\n", ret);
+
+	return true;
 }
 
 /* Get an IPA clock reference.  If the reference count is non-zero, it is
@@ -283,6 +301,7 @@ bool ipa_clock_get_additional(struct ipa *ipa)
 int ipa_clock_get(struct ipa *ipa)
 {
 	struct ipa_clock *clock = ipa->clock;
+	struct device *dev;
 	int ret;
 
 	/* If the clock is running, just bump the reference count */
@@ -298,7 +317,8 @@ int ipa_clock_get(struct ipa *ipa)
 		goto out_mutex_unlock;
 	}
 
-	ret = ipa_runtime_resume(&ipa->pdev->dev);
+	dev = &ipa->pdev->dev;
+	ret = pm_runtime_get_sync(dev);
 
 	refcount_set(&clock->count, 1);
 
@@ -313,14 +333,17 @@ int ipa_clock_get(struct ipa *ipa)
  */
 int ipa_clock_put(struct ipa *ipa)
 {
+	struct device *dev = &ipa->pdev->dev;
 	struct ipa_clock *clock = ipa->clock;
+	int last;
 	int ret;
 
 	/* If this is not the last reference there's nothing more to do */
-	if (!refcount_dec_and_mutex_lock(&clock->count, &clock->mutex))
-		return 0;
+	last = refcount_dec_and_mutex_lock(&clock->count, &clock->mutex);
 
-	ret = ipa_runtime_suspend(&ipa->pdev->dev);
+	ret = pm_runtime_put(dev);
+	if (!last)
+		return ret;
 
 	mutex_unlock(&clock->mutex);
 
@@ -394,6 +417,7 @@ ipa_clock_init(struct device *dev, const struct ipa_clock_data *data)
 		ret = -ENOMEM;
 		goto err_clk_put;
 	}
+	clock->dev = dev;
 	clock->core = clk;
 	clock->interconnect_count = data->interconnect_count;
 
@@ -404,6 +428,9 @@ ipa_clock_init(struct device *dev, const struct ipa_clock_data *data)
 	mutex_init(&clock->mutex);
 	refcount_set(&clock->count, 0);
 
+	pm_runtime_dont_use_autosuspend(dev);
+	pm_runtime_enable(dev);
+
 	return clock;
 
 err_kfree:
@@ -420,43 +447,17 @@ void ipa_clock_exit(struct ipa_clock *clock)
 	struct clk *clk = clock->core;
 
 	WARN_ON(refcount_read(&clock->count) != 0);
+	pm_runtime_disable(clock->dev);
 	mutex_destroy(&clock->mutex);
 	ipa_interconnect_exit(clock);
 	kfree(clock);
 	clk_put(clk);
 }
 
-/**
- * ipa_suspend() - Power management system suspend callback
- * @dev:	IPA device structure
- *
- * Return:	0 on success, or a negative error code
- *
- * Called by the PM framework when a system suspend operation is invoked.
- * Suspends endpoints and releases the clock reference held to keep
- * the IPA clock running until this point.
- */
-static int ipa_suspend(struct device *dev)
-{
-	return ipa_runtime_suspend(dev);
-}
-
-/**
- * ipa_resume() - Power management system resume callback
- * @dev:	IPA device structure
- *
- * Return:	0 on success, or a negative error code
- *
- * Called by the PM framework when a system resume operation is invoked.
- * Takes an IPA clock reference to keep the clock running until suspend,
- * and resumes endpoints.
- */
-static int ipa_resume(struct device *dev)
-{
-	return ipa_runtime_resume(dev);
-}
-
 const struct dev_pm_ops ipa_pm_ops = {
-	.suspend	= ipa_suspend,
-	.resume		= ipa_resume,
+	.suspend		= pm_runtime_force_suspend,
+	.resume			= pm_runtime_force_resume,
+	.runtime_suspend	= ipa_runtime_suspend,
+	.runtime_resume		= ipa_runtime_resume,
+	.runtime_idle		= ipa_runtime_idle,
 };
-- 
2.27.0

[PATCH net-next 5/7] net: ipa: get rid of extra clock reference

[Alex Elder]

Suspending the IPA hardware is now managed by the runtime PM core
code.  The ->runtime_idle callback returns a non-zero value, so it
will never suspend except when forced.  As a result, there's no need
to take an extra "do not suspend" clock reference.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_main.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index 64112a6767743..f332210ce5354 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -427,14 +427,6 @@ static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
 {
 	int ret;
 
-	/* Get a clock reference to allow initialization.  This reference
-	 * is held after initialization completes, and won't get dropped
-	 * unless/until a system suspend request arrives.
-	 */
-	ret = ipa_clock_get(ipa);
-	if (WARN_ON(ret < 0))
-		goto err_clock_put;
-
 	ipa_hardware_config(ipa, data);
 
 	ret = ipa_mem_config(ipa);
@@ -477,8 +469,6 @@ static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
 	ipa_mem_deconfig(ipa);
 err_hardware_deconfig:
 	ipa_hardware_deconfig(ipa);
-err_clock_put:
-	(void)ipa_clock_put(ipa);
 
 	return ret;
 }
@@ -496,7 +486,6 @@ static void ipa_deconfig(struct ipa *ipa)
 	ipa->interrupt = NULL;
 	ipa_mem_deconfig(ipa);
 	ipa_hardware_deconfig(ipa);
-	(void)ipa_clock_put(ipa);
 }
 
 static int ipa_firmware_load(struct device *dev)
-- 
2.27.0

[PATCH net-next 6/7] net: ipa: kill IPA clock reference count

[Alex Elder]

The runtime power management core code maintains a usage count.  This
count mirrors the IPA clock reference count, and there's no need to
maintain both.  So get rid of the IPA clock reference count and just
rely on the runtime PM usage count to determine when the hardware
should be suspended or resumed.

Use pm_runtime_get_if_active() in ipa_clock_get_additional().  We
care whether power is active, regardless of whether it's in use, so
pass true for its ign_usage_count argument.

The IPA clock mutex is just used to make enabling/disabling the
clock and updating the reference count occur atomically.  Without
the reference count, there's no need for the mutex, so get rid of
that too.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_clock.c | 77 +++----------------------------------
 1 file changed, 6 insertions(+), 71 deletions(-)

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index f1ee0b46da005..ab6626c617b91 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -4,8 +4,6 @@
  * Copyright (C) 2018-2021 Linaro Ltd.
  */
 
-#include <linux/refcount.h>
-#include <linux/mutex.h>
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/interconnect.h>
@@ -58,8 +56,6 @@ enum ipa_power_flag {
 
 /**
  * struct ipa_clock - IPA clocking information
- * @count:		Clocking reference count
- * @mutex:		Protects clock enable/disable
  * @dev:		IPA device pointer
  * @core:		IPA core clock
  * @flags:		Boolean state flags
@@ -67,8 +63,6 @@ enum ipa_power_flag {
  * @interconnect:	Interconnect array
  */
 struct ipa_clock {
-	refcount_t count;
-	struct mutex mutex; /* protects clock enable/disable */
 	struct device *dev;
 	struct clk *core;
 	DECLARE_BITMAP(flags, IPA_POWER_FLAG_COUNT);
@@ -276,78 +270,24 @@ static int ipa_runtime_idle(struct device *dev)
  */
 bool ipa_clock_get_additional(struct ipa *ipa)
 {
-	struct device *dev;
-	int ret;
-
-	if (!refcount_inc_not_zero(&ipa->clock->count))
-		return false;
-
-	dev = &ipa->pdev->dev;
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0)
-		dev_err(dev, "error %d enabling power\n", ret);
-
-	return true;
+	return pm_runtime_get_if_active(&ipa->pdev->dev, true) > 0;
 }
 
 /* Get an IPA clock reference.  If the reference count is non-zero, it is
- * incremented and return is immediate.  Otherwise it is checked again
- * under protection of the mutex, and if appropriate the IPA clock
- * is enabled.
- *
- * Incrementing the reference count is intentionally deferred until
- * after the clock is running and endpoints are resumed.
+ * incremented and return is immediate.  Otherwise the IPA clock is
+ * enabled.
  */
 int ipa_clock_get(struct ipa *ipa)
 {
-	struct ipa_clock *clock = ipa->clock;
-	struct device *dev;
-	int ret;
-
-	/* If the clock is running, just bump the reference count */
-	if (ipa_clock_get_additional(ipa))
-		return 1;
-
-	/* Otherwise get the mutex and check again */
-	mutex_lock(&clock->mutex);
-
-	/* A reference might have been added before we got the mutex. */
-	if (ipa_clock_get_additional(ipa)) {
-		ret = 1;
-		goto out_mutex_unlock;
-	}
-
-	dev = &ipa->pdev->dev;
-	ret = pm_runtime_get_sync(dev);
-
-	refcount_set(&clock->count, 1);
-
-out_mutex_unlock:
-	mutex_unlock(&clock->mutex);
-
-	return ret;
+	return pm_runtime_get_sync(&ipa->pdev->dev);
 }
 
 /* Attempt to remove an IPA clock reference.  If this represents the
- * last reference, disable the IPA clock under protection of the mutex.
+ * last reference, disable the IPA clock.
  */
 int ipa_clock_put(struct ipa *ipa)
 {
-	struct device *dev = &ipa->pdev->dev;
-	struct ipa_clock *clock = ipa->clock;
-	int last;
-	int ret;
-
-	/* If this is not the last reference there's nothing more to do */
-	last = refcount_dec_and_mutex_lock(&clock->count, &clock->mutex);
-
-	ret = pm_runtime_put(dev);
-	if (!last)
-		return ret;
-
-	mutex_unlock(&clock->mutex);
-
-	return ret;
+	return pm_runtime_put(&ipa->pdev->dev);
 }
 
 /* Return the current IPA core clock rate */
@@ -425,9 +365,6 @@ ipa_clock_init(struct device *dev, const struct ipa_clock_data *data)
 	if (ret)
 		goto err_kfree;
 
-	mutex_init(&clock->mutex);
-	refcount_set(&clock->count, 0);
-
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_enable(dev);
 
@@ -446,9 +383,7 @@ void ipa_clock_exit(struct ipa_clock *clock)
 {
 	struct clk *clk = clock->core;
 
-	WARN_ON(refcount_read(&clock->count) != 0);
 	pm_runtime_disable(clock->dev);
-	mutex_destroy(&clock->mutex);
 	ipa_interconnect_exit(clock);
 	kfree(clock);
 	clk_put(clk);
-- 
2.27.0

[PATCH net-next 7/7] net: ipa: kill ipa_clock_get_additional()

[Alex Elder]

Now that ipa_clock_get_additional() is a trivial wrapper around
pm_runtime_get_if_active(), just open-code it in its only caller
and delete the function.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_clock.c |  9 ---------
 drivers/net/ipa/ipa_clock.h | 10 ----------
 drivers/net/ipa/ipa_smp2p.c |  5 ++++-
 3 files changed, 4 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index ab6626c617b91..6df66c574d594 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -264,15 +264,6 @@ static int ipa_runtime_idle(struct device *dev)
 	return -EAGAIN;
 }
 
-/* Get an IPA clock reference, but only if the reference count is
- * already non-zero.  Returns true if the additional reference was
- * added successfully, or false otherwise.
- */
-bool ipa_clock_get_additional(struct ipa *ipa)
-{
-	return pm_runtime_get_if_active(&ipa->pdev->dev, true) > 0;
-}
-
 /* Get an IPA clock reference.  If the reference count is non-zero, it is
  * incremented and return is immediate.  Otherwise the IPA clock is
  * enabled.
diff --git a/drivers/net/ipa/ipa_clock.h b/drivers/net/ipa/ipa_clock.h
index 8692c0d98bd1c..5c118f2c42e7a 100644
--- a/drivers/net/ipa/ipa_clock.h
+++ b/drivers/net/ipa/ipa_clock.h
@@ -62,16 +62,6 @@ void ipa_clock_exit(struct ipa_clock *clock);
  */
 int ipa_clock_get(struct ipa *ipa);
 
-/**
- * ipa_clock_get_additional() - Get an IPA clock reference if not first
- * @ipa:	IPA pointer
- *
- * Return:	true if reference taken, false otherwise
- *
- * This returns immediately, and only takes a reference if not the first
- */
-bool ipa_clock_get_additional(struct ipa *ipa);
-
 /**
  * ipa_clock_put() - Drop an IPA clock reference
  * @ipa:	IPA pointer
diff --git a/drivers/net/ipa/ipa_smp2p.c b/drivers/net/ipa/ipa_smp2p.c
index f84d6523636e3..04b977cf91593 100644
--- a/drivers/net/ipa/ipa_smp2p.c
+++ b/drivers/net/ipa/ipa_smp2p.c
@@ -9,6 +9,7 @@
 #include <linux/interrupt.h>
 #include <linux/notifier.h>
 #include <linux/panic_notifier.h>
+#include <linux/pm_runtime.h>
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/smem_state.h>
 
@@ -84,13 +85,15 @@ struct ipa_smp2p {
  */
 static void ipa_smp2p_notify(struct ipa_smp2p *smp2p)
 {
+	struct device *dev;
 	u32 value;
 	u32 mask;
 
 	if (smp2p->notified)
 		return;
 
-	smp2p->clock_on = ipa_clock_get_additional(smp2p->ipa);
+	dev = &smp2p->ipa->pdev->dev;
+	smp2p->clock_on = pm_runtime_get_if_active(dev, true) > 0;
 
 	/* Signal whether the clock is enabled */
 	mask = BIT(smp2p->enabled_bit);
-- 
2.27.0