[PATCH v3 4/4] regulator: core: Add voltage support for sync_state() callbacks

From: Saravana Kannan <saravanak@google.com>
To: Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Saravana Kannan <saravanak@google.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	John Stultz <john.stultz@linaro.org>,
	linux-kernel@vger.kernel.org, kernel-team@android.com
Subject: [PATCH v3 4/4] regulator: core: Add voltage support for sync_state() callbacks
Date: Wed, 15 Jul 2020 21:20:53 -0700	[thread overview]
Message-ID: <20200716042053.1927676-5-saravanak@google.com> (raw)
In-Reply-To: <20200716042053.1927676-1-saravanak@google.com>

Consider the following example:
- regulator-X is provided by device-X.
- regulator-X is a supplier to device-A, device-B and device-C.
- device-A is off/inactive from boot.
- device-B and device-C are left on/active by the bootloader
- regulator-X is left on boot by the bootloader at 2000 mV to supply
  device-B and device-C.

Example boot sequence:
1. device-X is probed successfully.
2. device-B is probed by driver-B
   a. driver-B gets regulator-X
   b. driver-B votes on regulator-X
   c. driver-B lowers device-B performance point.
   d. driver-B lowers voltage vote to 1000 mV.
   e. regulator-X voltage is lowered to 1000 mV.
3. System crashes or device-C becomes unreliable because regulator-X
   voltage was lowered to 1000 mV when device-C still needed it at 2000 mV

The issue reported by Marek Szyprowski [1] between vdd_int and vdd_arm
is very similar to example 2, except driver-B lowers the voltage of
device-C due to a regulator coupling instead of a direct request from
driver-B.

This patch addresses the problem in the example by:
1. When a boot-on regulator is registered, a minimum voltage limit that
   matches the boot time voltage is placed on the regulator.

2. When the regulator_sync_state() callback comes, the minimum voltage
   limit is removed along with the rest of the boot limits.

So with this patch and regulator_cleanup_timeout=0, the example will
work as follows:

1. device-X is probed successfully.
   a. regulator-X is registered.
   b. Since regulator-X is on, a minimum voltage of 2000 mV is made by
      the BOOT-LIMITS consumer.
   c. Since regulator-X is on, an enable vote is made by the BOOT-LIMITS
      consumer.
2. device-B is probed by driver-B
   a. driver-B gets regulator-X
   b. driver-B votes on regulator-X
   c. driver-B lowers device-B performance point.
   d. driver-B lowers voltage vote to 1000 mV.
   e. regulator-X voltage is NOT lowered to 1000 mV.
3. device-C is probed by driver-C
   a. Does stuff similar to device-B.
4. Since all consumers of device-X have probed, device-X gets
   sync_state() callback which is a call to regulator_sync_state():
   a. BOOT-LIMITS removes enable vote for regulator-X
   b. regulator-X remains on becaue device-B and device-C have their
      enable votes in.
   c. BOOT-LIMITS consumer is freed.
   d. regulator-X voltage drops is lowered to 1000 mV.
5. The system is stable because regulator-X voltage is NOT lowered to
   1000 mV when device-C still needed it at 2000 mV.

[1] - https://lore.kernel.org/lkml/20200605063724.9030-1-m.szyprowski@samsung.com/#t
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/regulator/core.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f10ef6ec1af1..9b70295820f3 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1783,6 +1783,8 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
  */
 static void regulator_add_boot_limits(struct regulator_dev *rdev)
 {
+	int boot_uV;
+
 	/* We already set up boot limits. */
 	if (rdev->boot_limits)
 		return;
@@ -1815,6 +1817,13 @@ static void regulator_add_boot_limits(struct regulator_dev *rdev)
 	}
 	rdev->open_count++;
 
+	if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) {
+		boot_uV = regulator_get_voltage_rdev(rdev);
+		if (boot_uV > 0)
+			regulator_set_voltage(rdev->boot_limits, boot_uV,
+								 INT_MAX);
+	}
+
 	if (regulator_enable(rdev->boot_limits)) {
 		dev_err(&rdev->dev, "Unable to set boot limits\n");
 		destroy_regulator(rdev->boot_limits);
@@ -1847,10 +1856,12 @@ static int regulator_del_boot_limits(struct regulator_dev *rdev, bool handoff)
 		return 0;
 
 	rdev_info(rdev, "removing boot limits\n");
-	if (!handoff)
+	if (!handoff) {
 		regulator_disable(rdev->boot_limits);
-	else
+		regulator_set_voltage(rdev->boot_limits, 0, INT_MAX);
+	} else {
 		rdev->use_count--;
+	}
 	destroy_regulator(rdev->boot_limits);
 	/*
 	 * Set it to an error value so that boot limits can't be set again once
-- 
2.28.0.rc0.105.gf9edc3c819-goog

