Linux-Samsung-soc Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] Fix regulators coupling for Exynos5800
       [not found] <CGME20200529124948eucas1p175379ead8afd1932f7b7ae61e35cf632@eucas1p1.samsung.com>
@ 2020-05-29 12:49 ` Marek Szyprowski
       [not found]   ` <CGME20200529124951eucas1p213c748fb481a37cf98918caae5e30fd2@eucas1p2.samsung.com>
                     ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Marek Szyprowski @ 2020-05-29 12:49 UTC (permalink / raw)
  To: linux-kernel, linux-pm, Mark Brown, Dmitry Osipenko
  Cc: Marek Szyprowski, Liam Girdwood, Lucas Stach,
	Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, Viresh Kumar,
	peron.clem, Nishanth Menon, Stephen Boyd, Vincent Guittot,
	Rafael Wysocki, linux-samsung-soc, Chanwoo Choi

Hi!

This patchset is another attempt to fix the regulator coupling on
Exynos5800/5422 SoCs. Here are links to the previous attempts:

https://lore.kernel.org/linux-samsung-soc/20191008101709.qVNy8eijBi0LynOteWFMnTg4GUwKG599n6OyYoX1Abs@z/
https://lore.kernel.org/lkml/20191017102758.8104-1-m.szyprowski@samsung.com/
https://lore.kernel.org/linux-pm/cover.1589528491.git.viresh.kumar@linaro.org/
https://lore.kernel.org/linux-pm/20200528131130.17984-1-m.szyprowski@samsung.com/

The problem is with "vdd_int" regulator coupled with "vdd_arm" on Odroid
XU3/XU4 boards family. "vdd_arm" is handled by CPUfreq. "vdd_int" is
handled by devfreq. CPUfreq initialized quite early during boot and it
starts changing OPPs and "vdd_arm" value. Sometimes CPU activity during
boot goes down and some low-frequency OPPs are selected, what in turn
causes lowering "vdd_arm". This happens before devfreq applies its
requirements on "vdd_int". Regulator balancing code reduces "vdd_arm"
voltage value, what in turn causes lowering "vdd_int" value to the lowest
possible value. This is much below the operation point of the wcore bus,
which still runs at the highest frequency.

The issue was hard to notice because in the most cases the board managed
to boot properly, even when the regulator was set to lowest value allowed
by the regulator constraints. However, it caused some random issues,
which can be observed as "Unhandled prefetch abort" or low USB stability.

Adding more and more special cases to the generic code has been rejected,
so the only way to ensure the desired behavior on Exynos5800-based SoCs 
is to make a custom regulator coupler driver.

Best regards,
Marek Szyprowski


Patch summary:

Marek Szyprowski (2):
  regulator: extract voltage balancing code to separate function
  soc: samsung: Add simple voltage coupler for Exynos5800

 arch/arm/mach-exynos/Kconfig                  |  1 +
 drivers/regulator/core.c                      | 49 ++++++++-------
 drivers/soc/samsung/Kconfig                   |  3 +
 drivers/soc/samsung/Makefile                  |  1 +
 .../soc/samsung/exynos-regulator-coupler.c    | 59 +++++++++++++++++++
 include/linux/regulator/coupler.h             |  8 +++
 6 files changed, 101 insertions(+), 20 deletions(-)
 create mode 100644 drivers/soc/samsung/exynos-regulator-coupler.c

-- 
2.17.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] regulator: extract voltage balancing code to the separate function
       [not found]   ` <CGME20200529124951eucas1p213c748fb481a37cf98918caae5e30fd2@eucas1p2.samsung.com>
@ 2020-05-29 12:49     ` Marek Szyprowski
  0 siblings, 0 replies; 11+ messages in thread
From: Marek Szyprowski @ 2020-05-29 12:49 UTC (permalink / raw)
  To: linux-kernel, linux-pm, Mark Brown, Dmitry Osipenko
  Cc: Marek Szyprowski, Liam Girdwood, Lucas Stach,
	Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, Viresh Kumar,
	peron.clem, Nishanth Menon, Stephen Boyd, Vincent Guittot,
	Rafael Wysocki, linux-samsung-soc, Chanwoo Choi

Move the coupled regulators voltage balancing code to the separate
function and allow to call it from the custom regulator couplers.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/regulator/core.c          | 49 ++++++++++++++++++-------------
 include/linux/regulator/coupler.h |  8 +++++
 2 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 941783a14b45..370c655ad8f6 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3642,36 +3642,19 @@ static int regulator_get_optimal_voltage(struct regulator_dev *rdev,
 	return done;
 }
 
-static int regulator_balance_voltage(struct regulator_dev *rdev,
-				     suspend_state_t state)
+int regulator_do_balance_voltage(struct regulator_dev *rdev,
+				 suspend_state_t state, bool skip_coupled)
 {
 	struct regulator_dev **c_rdevs;
 	struct regulator_dev *best_rdev;
 	struct coupling_desc *c_desc = &rdev->coupling_desc;
-	struct regulator_coupler *coupler = c_desc->coupler;
 	int i, ret, n_coupled, best_min_uV, best_max_uV, best_c_rdev;
 	unsigned int delta, best_delta;
 	unsigned long c_rdev_done = 0;
 	bool best_c_rdev_done;
 
 	c_rdevs = c_desc->coupled_rdevs;
-	n_coupled = c_desc->n_coupled;
-
-	/*
-	 * If system is in a state other than PM_SUSPEND_ON, don't check
-	 * other coupled regulators.
-	 */
-	if (state != PM_SUSPEND_ON)
-		n_coupled = 1;
-
-	if (c_desc->n_resolved < n_coupled) {
-		rdev_err(rdev, "Not all coupled regulators registered\n");
-		return -EPERM;
-	}
-
-	/* Invoke custom balancer for customized couplers */
-	if (coupler && coupler->balance_voltage)
-		return coupler->balance_voltage(coupler, rdev, state);
+	n_coupled = skip_coupled ? 1 : c_desc->n_coupled;
 
 	/*
 	 * Find the best possible voltage change on each loop. Leave the loop
@@ -3742,6 +3725,32 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
 	return ret;
 }
 
+static int regulator_balance_voltage(struct regulator_dev *rdev,
+				     suspend_state_t state)
+{
+	struct coupling_desc *c_desc = &rdev->coupling_desc;
+	struct regulator_coupler *coupler = c_desc->coupler;
+	bool skip_coupled = false;
+
+	/*
+	 * If system is in a state other than PM_SUSPEND_ON, don't check
+	 * other coupled regulators.
+	 */
+	if (state != PM_SUSPEND_ON)
+		skip_coupled = true;
+
+	if (c_desc->n_resolved < c_desc->n_coupled) {
+		rdev_err(rdev, "Not all coupled regulators registered\n");
+		return -EPERM;
+	}
+
+	/* Invoke custom balancer for customized couplers */
+	if (coupler && coupler->balance_voltage)
+		return coupler->balance_voltage(coupler, rdev, state);
+
+	return regulator_do_balance_voltage(rdev, state, skip_coupled);
+}
+
 /**
  * regulator_set_voltage - set regulator output voltage
  * @regulator: regulator source
diff --git a/include/linux/regulator/coupler.h b/include/linux/regulator/coupler.h
index 0212d6255e4e..5f86824bd117 100644
--- a/include/linux/regulator/coupler.h
+++ b/include/linux/regulator/coupler.h
@@ -62,6 +62,8 @@ int regulator_get_voltage_rdev(struct regulator_dev *rdev);
 int regulator_set_voltage_rdev(struct regulator_dev *rdev,
 			       int min_uV, int max_uV,
 			       suspend_state_t state);
+int regulator_do_balance_voltage(struct regulator_dev *rdev,
+				 suspend_state_t state, bool skip_coupled);
 #else
 static inline int regulator_coupler_register(struct regulator_coupler *coupler)
 {
@@ -92,6 +94,12 @@ static inline int regulator_set_voltage_rdev(struct regulator_dev *rdev,
 {
 	return -EINVAL;
 }
+static inline int regulator_do_balance_voltage(struct regulator_dev *rdev,
+					       suspend_state_t state,
+					       bool skip_coupled)
+{
+	return -EINVAL;
+}
 #endif
 
 #endif
-- 
2.17.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/2] soc: samsung: Add simple voltage coupler for Exynos5800
       [not found]   ` <CGME20200529124952eucas1p2565c598c3c0164b5dec6ed83e19148b8@eucas1p2.samsung.com>
@ 2020-05-29 12:49     ` Marek Szyprowski
  2020-05-29 17:43       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Szyprowski @ 2020-05-29 12:49 UTC (permalink / raw)
  To: linux-kernel, linux-pm, Mark Brown, Dmitry Osipenko
  Cc: Marek Szyprowski, Liam Girdwood, Lucas Stach,
	Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, Viresh Kumar,
	peron.clem, Nishanth Menon, Stephen Boyd, Vincent Guittot,
	Rafael Wysocki, linux-samsung-soc, Chanwoo Choi

Add custom voltage regulator coupler for Exynos5800 SoCs, which require
coupling between "vdd_arm" and "vdd_int" regulators. This coupler ensures
that coupled regulators voltage balancing is done only when clients for
each regulator (cpufreq for "vdd_arm" and devfreq for "vdd_int") apply
their constraints, so the voltage values don't go beyond the
bootloader-selected operation point during the boot process. This also
ensures proper voltage balancing if any of those drivers is missing.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm/mach-exynos/Kconfig                  |  1 +
 drivers/soc/samsung/Kconfig                   |  3 +
 drivers/soc/samsung/Makefile                  |  1 +
 .../soc/samsung/exynos-regulator-coupler.c    | 59 +++++++++++++++++++
 4 files changed, 64 insertions(+)
 create mode 100644 drivers/soc/samsung/exynos-regulator-coupler.c

diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
index 76838255b5fa..f185cd3d4c62 100644
--- a/arch/arm/mach-exynos/Kconfig
+++ b/arch/arm/mach-exynos/Kconfig
@@ -118,6 +118,7 @@ config SOC_EXYNOS5800
 	bool "Samsung EXYNOS5800"
 	default y
 	depends on SOC_EXYNOS5420
+	select EXYNOS_REGULATOR_COUPLER
 
 config EXYNOS_MCPM
 	bool
diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
index c7a2003687c7..264185664594 100644
--- a/drivers/soc/samsung/Kconfig
+++ b/drivers/soc/samsung/Kconfig
@@ -37,4 +37,7 @@ config EXYNOS_PM_DOMAINS
 	bool "Exynos PM domains" if COMPILE_TEST
 	depends on PM_GENERIC_DOMAINS || COMPILE_TEST
 
+config EXYNOS_REGULATOR_COUPLER
+	bool "Exynos SoC Regulator Coupler" if COMPILE_TEST
+	depends on ARCH_EXYNOS || COMPILE_TEST
 endif
diff --git a/drivers/soc/samsung/Makefile b/drivers/soc/samsung/Makefile
index edd1d6ea064d..ecc3a32f6406 100644
--- a/drivers/soc/samsung/Makefile
+++ b/drivers/soc/samsung/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_EXYNOS_PMU)	+= exynos-pmu.o
 obj-$(CONFIG_EXYNOS_PMU_ARM_DRIVERS)	+= exynos3250-pmu.o exynos4-pmu.o \
 					exynos5250-pmu.o exynos5420-pmu.o
 obj-$(CONFIG_EXYNOS_PM_DOMAINS) += pm_domains.o
+obj-$(CONFIG_EXYNOS_REGULATOR_COUPLER) += exynos-regulator-coupler.o
diff --git a/drivers/soc/samsung/exynos-regulator-coupler.c b/drivers/soc/samsung/exynos-regulator-coupler.c
new file mode 100644
index 000000000000..54445918bd75
--- /dev/null
+++ b/drivers/soc/samsung/exynos-regulator-coupler.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 Samsung Electronics Co., Ltd.
+ *	      http://www.samsung.com/
+ * Author: Marek Szyprowski <m.szyprowski@samsung.com>
+ *
+ * Samsung Exynos SoC voltage coupler
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/regulator/coupler.h>
+#include <linux/regulator/driver.h>
+
+static int exynos_coupler_balance_voltage(struct regulator_coupler *coupler,
+					  struct regulator_dev *rdev,
+					  suspend_state_t state)
+{
+	struct coupling_desc *c_desc = &rdev->coupling_desc;
+	int ret, cons_uV = 0, cons_max_uV = INT_MAX;
+	bool skip_coupled = false;
+
+	/* get coupled regulator constraints */
+	ret = regulator_check_consumers(c_desc->coupled_rdevs[1], &cons_uV,
+					&cons_max_uV, state);
+	if (ret < 0)
+		return ret;
+
+	/* skip adjusting coupled regulator if it has no constraints set yet */
+	if (cons_uV == 0)
+		skip_coupled = true;
+
+	return regulator_do_balance_voltage(rdev, state, skip_coupled);
+}
+
+static int exynos_coupler_attach(struct regulator_coupler *coupler,
+				 struct regulator_dev *rdev)
+{
+	if (strcmp(rdev_get_name(rdev), "vdd_arm") == 0 ||
+	    strcmp(rdev_get_name(rdev), "vdd_int") == 0)
+		return 0;
+
+	return -EINVAL;
+}
+
+static struct regulator_coupler exynos_coupler = {
+	.attach_regulator = exynos_coupler_attach,
+	.balance_voltage  = exynos_coupler_balance_voltage,
+};
+
+static int __init exynos_coupler_init(void)
+{
+	if (!of_machine_is_compatible("samsung,exynos5800"))
+		return 0;
+
+	return regulator_coupler_register(&exynos_coupler);
+}
+arch_initcall(exynos_coupler_init);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] Fix regulators coupling for Exynos5800
  2020-05-29 12:49 ` [PATCH 0/2] Fix regulators coupling for Exynos5800 Marek Szyprowski
       [not found]   ` <CGME20200529124951eucas1p213c748fb481a37cf98918caae5e30fd2@eucas1p2.samsung.com>
       [not found]   ` <CGME20200529124952eucas1p2565c598c3c0164b5dec6ed83e19148b8@eucas1p2.samsung.com>
@ 2020-05-29 16:52   ` Mark Brown
  2020-05-29 16:58     ` Mark Brown
  2 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2020-05-29 16:52 UTC (permalink / raw)
  To: linux-pm, Dmitry Osipenko, Marek Szyprowski, linux-kernel
  Cc: Krzysztof Kozlowski, Stephen Boyd, Bartlomiej Zolnierkiewicz,
	Viresh Kumar, Nishanth Menon, Chanwoo Choi, linux-samsung-soc,
	Liam Girdwood, Lucas Stach, peron.clem, Rafael Wysocki,
	Vincent Guittot

On Fri, 29 May 2020 14:49:38 +0200, Marek Szyprowski wrote:
> This patchset is another attempt to fix the regulator coupling on
> Exynos5800/5422 SoCs. Here are links to the previous attempts:
> 
> https://lore.kernel.org/linux-samsung-soc/20191008101709.qVNy8eijBi0LynOteWFMnTg4GUwKG599n6OyYoX1Abs@z/
> https://lore.kernel.org/lkml/20191017102758.8104-1-m.szyprowski@samsung.com/
> https://lore.kernel.org/linux-pm/cover.1589528491.git.viresh.kumar@linaro.org/
> https://lore.kernel.org/linux-pm/20200528131130.17984-1-m.szyprowski@samsung.com/
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/1] regulator: extract voltage balancing code to the separate function
      commit: 752db83a5dfd4fd3a0624b9ab440ed947fa003ca

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] Fix regulators coupling for Exynos5800
  2020-05-29 16:52   ` [PATCH 0/2] Fix regulators coupling " Mark Brown
@ 2020-05-29 16:58     ` Mark Brown
  2020-05-29 17:33       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2020-05-29 16:58 UTC (permalink / raw)
  To: linux-pm, Dmitry Osipenko, Marek Szyprowski, linux-kernel
  Cc: Krzysztof Kozlowski, Stephen Boyd, Bartlomiej Zolnierkiewicz,
	Viresh Kumar, Nishanth Menon, Chanwoo Choi, linux-samsung-soc,
	Liam Girdwood, Lucas Stach, peron.clem, Rafael Wysocki,
	Vincent Guittot


[-- Attachment #1: Type: text/plain, Size: 347 bytes --]

On Fri, May 29, 2020 at 05:52:15PM +0100, Mark Brown wrote:

> [1/1] regulator: extract voltage balancing code to the separate function
>       commit: 752db83a5dfd4fd3a0624b9ab440ed947fa003ca

Let me know if you need a pull request for this - I figured it was too
late to apply the second patch before the merge window with the cross
tree stuff.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] Fix regulators coupling for Exynos5800
  2020-05-29 16:58     ` Mark Brown
@ 2020-05-29 17:33       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2020-05-29 17:33 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-pm, Dmitry Osipenko, Marek Szyprowski, linux-kernel,
	Stephen Boyd, Bartlomiej Zolnierkiewicz, Viresh Kumar,
	Nishanth Menon, Chanwoo Choi, linux-samsung-soc, Liam Girdwood,
	Lucas Stach, peron.clem, Rafael Wysocki, Vincent Guittot

On Fri, May 29, 2020 at 05:58:27PM +0100, Mark Brown wrote:
> On Fri, May 29, 2020 at 05:52:15PM +0100, Mark Brown wrote:
> 
> > [1/1] regulator: extract voltage balancing code to the separate function
> >       commit: 752db83a5dfd4fd3a0624b9ab440ed947fa003ca
> 
> Let me know if you need a pull request for this - I figured it was too
> late to apply the second patch before the merge window with the cross
> tree stuff.

Thanks, I think it will not be needed. I'll apply the second patch after
the merge window.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] soc: samsung: Add simple voltage coupler for Exynos5800
  2020-05-29 12:49     ` [PATCH 2/2] soc: samsung: Add simple voltage coupler for Exynos5800 Marek Szyprowski
@ 2020-05-29 17:43       ` Krzysztof Kozlowski
  2020-06-01  6:28         ` Marek Szyprowski
  0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2020-05-29 17:43 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-kernel, linux-pm, Mark Brown, Dmitry Osipenko,
	Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz,
	Viresh Kumar, peron.clem, Nishanth Menon, Stephen Boyd,
	Vincent Guittot, Rafael Wysocki, linux-samsung-soc, Chanwoo Choi

On Fri, May 29, 2020 at 02:49:40PM +0200, Marek Szyprowski wrote:
> Add custom voltage regulator coupler for Exynos5800 SoCs, which require
> coupling between "vdd_arm" and "vdd_int" regulators. This coupler ensures
> that coupled regulators voltage balancing is done only when clients for
> each regulator (cpufreq for "vdd_arm" and devfreq for "vdd_int") apply
> their constraints, so the voltage values don't go beyond the
> bootloader-selected operation point during the boot process. This also
> ensures proper voltage balancing if any of those drivers is missing.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  arch/arm/mach-exynos/Kconfig                  |  1 +
>  drivers/soc/samsung/Kconfig                   |  3 +
>  drivers/soc/samsung/Makefile                  |  1 +
>  .../soc/samsung/exynos-regulator-coupler.c    | 59 +++++++++++++++++++
>  4 files changed, 64 insertions(+)
>  create mode 100644 drivers/soc/samsung/exynos-regulator-coupler.c
> 
> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
> index 76838255b5fa..f185cd3d4c62 100644
> --- a/arch/arm/mach-exynos/Kconfig
> +++ b/arch/arm/mach-exynos/Kconfig
> @@ -118,6 +118,7 @@ config SOC_EXYNOS5800
>  	bool "Samsung EXYNOS5800"
>  	default y
>  	depends on SOC_EXYNOS5420
> +	select EXYNOS_REGULATOR_COUPLER
>  
>  config EXYNOS_MCPM
>  	bool
> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
> index c7a2003687c7..264185664594 100644
> --- a/drivers/soc/samsung/Kconfig
> +++ b/drivers/soc/samsung/Kconfig
> @@ -37,4 +37,7 @@ config EXYNOS_PM_DOMAINS
>  	bool "Exynos PM domains" if COMPILE_TEST
>  	depends on PM_GENERIC_DOMAINS || COMPILE_TEST
>  
> +config EXYNOS_REGULATOR_COUPLER
> +	bool "Exynos SoC Regulator Coupler" if COMPILE_TEST
> +	depends on ARCH_EXYNOS || COMPILE_TEST
>  endif
> diff --git a/drivers/soc/samsung/Makefile b/drivers/soc/samsung/Makefile
> index edd1d6ea064d..ecc3a32f6406 100644
> --- a/drivers/soc/samsung/Makefile
> +++ b/drivers/soc/samsung/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_EXYNOS_PMU)	+= exynos-pmu.o
>  obj-$(CONFIG_EXYNOS_PMU_ARM_DRIVERS)	+= exynos3250-pmu.o exynos4-pmu.o \
>  					exynos5250-pmu.o exynos5420-pmu.o
>  obj-$(CONFIG_EXYNOS_PM_DOMAINS) += pm_domains.o
> +obj-$(CONFIG_EXYNOS_REGULATOR_COUPLER) += exynos-regulator-coupler.o
> diff --git a/drivers/soc/samsung/exynos-regulator-coupler.c b/drivers/soc/samsung/exynos-regulator-coupler.c
> new file mode 100644
> index 000000000000..54445918bd75
> --- /dev/null
> +++ b/drivers/soc/samsung/exynos-regulator-coupler.c
> @@ -0,0 +1,59 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
> + *	      http://www.samsung.com/
> + * Author: Marek Szyprowski <m.szyprowski@samsung.com>
> + *
> + * Samsung Exynos SoC voltage coupler
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/regulator/coupler.h>
> +#include <linux/regulator/driver.h>
> +
> +static int exynos_coupler_balance_voltage(struct regulator_coupler *coupler,
> +					  struct regulator_dev *rdev,
> +					  suspend_state_t state)
> +{
> +	struct coupling_desc *c_desc = &rdev->coupling_desc;
> +	int ret, cons_uV = 0, cons_max_uV = INT_MAX;
> +	bool skip_coupled = false;
> +
> +	/* get coupled regulator constraints */
> +	ret = regulator_check_consumers(c_desc->coupled_rdevs[1], &cons_uV,
> +					&cons_max_uV, state);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* skip adjusting coupled regulator if it has no constraints set yet */
> +	if (cons_uV == 0)
> +		skip_coupled = true;
> +
> +	return regulator_do_balance_voltage(rdev, state, skip_coupled);
> +}
> +
> +static int exynos_coupler_attach(struct regulator_coupler *coupler,
> +				 struct regulator_dev *rdev)
> +{
> +	if (strcmp(rdev_get_name(rdev), "vdd_arm") == 0 ||
> +	    strcmp(rdev_get_name(rdev), "vdd_int") == 0)

Thanks for the patch!

You hard-coded specific names of regulators existing
only in DTS. They do not come from any driver nor bindings, therefore
they could change making driver unusable. Someone could add a new DTS
with different names and expect this to work as well.

You need bindings for this. Something like:
Documentation/devicetree/bindings/regulator/nvidia,tegra-regulators-coupling.txt
But better in YAML, if possible.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] soc: samsung: Add simple voltage coupler for Exynos5800
  2020-05-29 17:43       ` Krzysztof Kozlowski
@ 2020-06-01  6:28         ` Marek Szyprowski
       [not found]           ` <CGME20200602130931eucas1p1cd784c8f692fa91dc566504543a927de@eucas1p1.samsung.com>
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Szyprowski @ 2020-06-01  6:28 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: linux-kernel, linux-pm, Mark Brown, Dmitry Osipenko,
	Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz,
	Viresh Kumar, peron.clem, Nishanth Menon, Stephen Boyd,
	Vincent Guittot, Rafael Wysocki, linux-samsung-soc, Chanwoo Choi

Hi Krzysztof,

On 29.05.2020 19:43, Krzysztof Kozlowski wrote:
> On Fri, May 29, 2020 at 02:49:40PM +0200, Marek Szyprowski wrote:
>> Add custom voltage regulator coupler for Exynos5800 SoCs, which require
>> coupling between "vdd_arm" and "vdd_int" regulators. This coupler ensures
>> that coupled regulators voltage balancing is done only when clients for
>> each regulator (cpufreq for "vdd_arm" and devfreq for "vdd_int") apply
>> their constraints, so the voltage values don't go beyond the
>> bootloader-selected operation point during the boot process. This also
>> ensures proper voltage balancing if any of those drivers is missing.

I've intentionally didn't add any new properties nor bindings, because I 
assume that the generic regulator coupling bindings fully describe what 
is needed for Exynos5800 SoCs. This driver 'fixes' only the issue (or 
lets it call undefined behavior) in the regulator core, which is fatal 
for Exynos5800.

Please note that this "attach" callback is called only for the 
regulators, which are marked as coupled, so for all existing board it 
might be even enough to simply return 0 without any check. IMHO for all 
existing Exynos5422/5800 the name-based check is enough. I can change it 
to a check for particular DT path peculiar for Exynos5422/5800 (the 
check if the given regulator mathes the phandle specificied in the 
exynos-bus node for "wcore" or A15's CPU).

>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   arch/arm/mach-exynos/Kconfig                  |  1 +
>>   drivers/soc/samsung/Kconfig                   |  3 +
>>   drivers/soc/samsung/Makefile                  |  1 +
>>   .../soc/samsung/exynos-regulator-coupler.c    | 59 +++++++++++++++++++
>>   4 files changed, 64 insertions(+)
>>   create mode 100644 drivers/soc/samsung/exynos-regulator-coupler.c
>>
>> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
>> index 76838255b5fa..f185cd3d4c62 100644
>> --- a/arch/arm/mach-exynos/Kconfig
>> +++ b/arch/arm/mach-exynos/Kconfig
>> @@ -118,6 +118,7 @@ config SOC_EXYNOS5800
>>   	bool "Samsung EXYNOS5800"
>>   	default y
>>   	depends on SOC_EXYNOS5420
>> +	select EXYNOS_REGULATOR_COUPLER
>>   
>>   config EXYNOS_MCPM
>>   	bool
>> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
>> index c7a2003687c7..264185664594 100644
>> --- a/drivers/soc/samsung/Kconfig
>> +++ b/drivers/soc/samsung/Kconfig
>> @@ -37,4 +37,7 @@ config EXYNOS_PM_DOMAINS
>>   	bool "Exynos PM domains" if COMPILE_TEST
>>   	depends on PM_GENERIC_DOMAINS || COMPILE_TEST
>>   
>> +config EXYNOS_REGULATOR_COUPLER
>> +	bool "Exynos SoC Regulator Coupler" if COMPILE_TEST
>> +	depends on ARCH_EXYNOS || COMPILE_TEST
>>   endif
>> diff --git a/drivers/soc/samsung/Makefile b/drivers/soc/samsung/Makefile
>> index edd1d6ea064d..ecc3a32f6406 100644
>> --- a/drivers/soc/samsung/Makefile
>> +++ b/drivers/soc/samsung/Makefile
>> @@ -9,3 +9,4 @@ obj-$(CONFIG_EXYNOS_PMU)	+= exynos-pmu.o
>>   obj-$(CONFIG_EXYNOS_PMU_ARM_DRIVERS)	+= exynos3250-pmu.o exynos4-pmu.o \
>>   					exynos5250-pmu.o exynos5420-pmu.o
>>   obj-$(CONFIG_EXYNOS_PM_DOMAINS) += pm_domains.o
>> +obj-$(CONFIG_EXYNOS_REGULATOR_COUPLER) += exynos-regulator-coupler.o
>> diff --git a/drivers/soc/samsung/exynos-regulator-coupler.c b/drivers/soc/samsung/exynos-regulator-coupler.c
>> new file mode 100644
>> index 000000000000..54445918bd75
>> --- /dev/null
>> +++ b/drivers/soc/samsung/exynos-regulator-coupler.c
>> @@ -0,0 +1,59 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
>> + *	      http://www.samsung.com/
>> + * Author: Marek Szyprowski <m.szyprowski@samsung.com>
>> + *
>> + * Samsung Exynos SoC voltage coupler
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/of.h>
>> +#include <linux/regulator/coupler.h>
>> +#include <linux/regulator/driver.h>
>> +
>> +static int exynos_coupler_balance_voltage(struct regulator_coupler *coupler,
>> +					  struct regulator_dev *rdev,
>> +					  suspend_state_t state)
>> +{
>> +	struct coupling_desc *c_desc = &rdev->coupling_desc;
>> +	int ret, cons_uV = 0, cons_max_uV = INT_MAX;
>> +	bool skip_coupled = false;
>> +
>> +	/* get coupled regulator constraints */
>> +	ret = regulator_check_consumers(c_desc->coupled_rdevs[1], &cons_uV,
>> +					&cons_max_uV, state);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* skip adjusting coupled regulator if it has no constraints set yet */
>> +	if (cons_uV == 0)
>> +		skip_coupled = true;
>> +
>> +	return regulator_do_balance_voltage(rdev, state, skip_coupled);
>> +}
>> +
>> +static int exynos_coupler_attach(struct regulator_coupler *coupler,
>> +				 struct regulator_dev *rdev)
>> +{
>> +	if (strcmp(rdev_get_name(rdev), "vdd_arm") == 0 ||
>> +	    strcmp(rdev_get_name(rdev), "vdd_int") == 0)
> Thanks for the patch!
>
> You hard-coded specific names of regulators existing
> only in DTS. They do not come from any driver nor bindings, therefore
> they could change making driver unusable. Someone could add a new DTS
> with different names and expect this to work as well.
>
> You need bindings for this. Something like:
> Documentation/devicetree/bindings/regulator/nvidia,tegra-regulators-coupling.txt
> But better in YAML, if possible.
>
> Best regards,
> Krzysztof
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] soc: samsung: Add simple voltage coupler for Exynos5800
       [not found]           ` <CGME20200602130931eucas1p1cd784c8f692fa91dc566504543a927de@eucas1p1.samsung.com>
@ 2020-06-02 13:02             ` Marek Szyprowski
  2020-06-02 15:15               ` Dmitry Osipenko
  0 siblings, 1 reply; 11+ messages in thread
From: Marek Szyprowski @ 2020-06-02 13:02 UTC (permalink / raw)
  To: linux-kernel, linux-pm, Mark Brown, Dmitry Osipenko
  Cc: Marek Szyprowski, Liam Girdwood, Lucas Stach,
	Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, Viresh Kumar,
	peron.clem, Nishanth Menon, Stephen Boyd, Vincent Guittot,
	Rafael Wysocki, linux-samsung-soc, Chanwoo Choi

Add a simple custom voltage regulator coupler for Exynos5800 SoCs, which
require coupling between "vdd_arm" and "vdd_int" regulators. This coupler
ensures that the voltage balancing for the coupled regulators is done
only when clients for the each regulator apply their constraints, so the
voltage values don't go beyond the bootloader-selected operation point
during the boot process. This also ensures proper voltage balancing if
any of the client driver is missing.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
v2:
- removed dependency on the regulator names as pointed by krzk, now it
  works with all coupled regulators. So far the coupling between the
  regulators on Exynos5800 based boards is defined only between
  "vdd_arm" and "vdd_int" supplies.
---
 arch/arm/mach-exynos/Kconfig                  |  1 +
 drivers/soc/samsung/Kconfig                   |  3 +
 drivers/soc/samsung/Makefile                  |  1 +
 .../soc/samsung/exynos-regulator-coupler.c    | 56 +++++++++++++++++++
 4 files changed, 61 insertions(+)
 create mode 100644 drivers/soc/samsung/exynos-regulator-coupler.c

diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
index 76838255b5fa..f185cd3d4c62 100644
--- a/arch/arm/mach-exynos/Kconfig
+++ b/arch/arm/mach-exynos/Kconfig
@@ -118,6 +118,7 @@ config SOC_EXYNOS5800
 	bool "Samsung EXYNOS5800"
 	default y
 	depends on SOC_EXYNOS5420
+	select EXYNOS_REGULATOR_COUPLER
 
 config EXYNOS_MCPM
 	bool
diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
index c7a2003687c7..264185664594 100644
--- a/drivers/soc/samsung/Kconfig
+++ b/drivers/soc/samsung/Kconfig
@@ -37,4 +37,7 @@ config EXYNOS_PM_DOMAINS
 	bool "Exynos PM domains" if COMPILE_TEST
 	depends on PM_GENERIC_DOMAINS || COMPILE_TEST
 
+config EXYNOS_REGULATOR_COUPLER
+	bool "Exynos SoC Regulator Coupler" if COMPILE_TEST
+	depends on ARCH_EXYNOS || COMPILE_TEST
 endif
diff --git a/drivers/soc/samsung/Makefile b/drivers/soc/samsung/Makefile
index edd1d6ea064d..ecc3a32f6406 100644
--- a/drivers/soc/samsung/Makefile
+++ b/drivers/soc/samsung/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_EXYNOS_PMU)	+= exynos-pmu.o
 obj-$(CONFIG_EXYNOS_PMU_ARM_DRIVERS)	+= exynos3250-pmu.o exynos4-pmu.o \
 					exynos5250-pmu.o exynos5420-pmu.o
 obj-$(CONFIG_EXYNOS_PM_DOMAINS) += pm_domains.o
+obj-$(CONFIG_EXYNOS_REGULATOR_COUPLER) += exynos-regulator-coupler.o
diff --git a/drivers/soc/samsung/exynos-regulator-coupler.c b/drivers/soc/samsung/exynos-regulator-coupler.c
new file mode 100644
index 000000000000..370a0ce4de3a
--- /dev/null
+++ b/drivers/soc/samsung/exynos-regulator-coupler.c
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 Samsung Electronics Co., Ltd.
+ *	      http://www.samsung.com/
+ * Author: Marek Szyprowski <m.szyprowski@samsung.com>
+ *
+ * Simple Samsung Exynos SoC voltage coupler. Ensures that all
+ * clients set their constraints before balancing the voltages.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/regulator/coupler.h>
+#include <linux/regulator/driver.h>
+
+static int exynos_coupler_balance_voltage(struct regulator_coupler *coupler,
+					  struct regulator_dev *rdev,
+					  suspend_state_t state)
+{
+	struct coupling_desc *c_desc = &rdev->coupling_desc;
+	int ret, cons_uV = 0, cons_max_uV = INT_MAX;
+	bool skip_coupled = false;
+
+	/* get coupled regulator constraints */
+	ret = regulator_check_consumers(c_desc->coupled_rdevs[1], &cons_uV,
+					&cons_max_uV, state);
+	if (ret < 0)
+		return ret;
+
+	/* skip adjusting coupled regulator if it has no constraints set yet */
+	if (cons_uV == 0)
+		skip_coupled = true;
+
+	return regulator_do_balance_voltage(rdev, state, skip_coupled);
+}
+
+static int exynos_coupler_attach(struct regulator_coupler *coupler,
+				 struct regulator_dev *rdev)
+{
+	return 0;
+}
+
+static struct regulator_coupler exynos_coupler = {
+	.attach_regulator = exynos_coupler_attach,
+	.balance_voltage  = exynos_coupler_balance_voltage,
+};
+
+static int __init exynos_coupler_init(void)
+{
+	if (!of_machine_is_compatible("samsung,exynos5800"))
+		return 0;
+
+	return regulator_coupler_register(&exynos_coupler);
+}
+arch_initcall(exynos_coupler_init);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] soc: samsung: Add simple voltage coupler for Exynos5800
  2020-06-02 13:02             ` [PATCH v2] " Marek Szyprowski
@ 2020-06-02 15:15               ` Dmitry Osipenko
  2020-06-04 13:28                 ` Marek Szyprowski
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Osipenko @ 2020-06-02 15:15 UTC (permalink / raw)
  To: Marek Szyprowski, linux-kernel, linux-pm, Mark Brown
  Cc: Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz,
	Krzysztof Kozlowski, Viresh Kumar, peron.clem, Nishanth Menon,
	Stephen Boyd, Vincent Guittot, Rafael Wysocki, linux-samsung-soc,
	Chanwoo Choi

02.06.2020 16:02, Marek Szyprowski пишет:
> Add a simple custom voltage regulator coupler for Exynos5800 SoCs, which
> require coupling between "vdd_arm" and "vdd_int" regulators. This coupler
> ensures that the voltage balancing for the coupled regulators is done
> only when clients for the each regulator apply their constraints, so the
> voltage values don't go beyond the bootloader-selected operation point
> during the boot process. This also ensures proper voltage balancing if
> any of the client driver is missing.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> v2:
> - removed dependency on the regulator names as pointed by krzk, now it
>   works with all coupled regulators. So far the coupling between the
>   regulators on Exynos5800 based boards is defined only between
>   "vdd_arm" and "vdd_int" supplies.
> ---
>  arch/arm/mach-exynos/Kconfig                  |  1 +
>  drivers/soc/samsung/Kconfig                   |  3 +
>  drivers/soc/samsung/Makefile                  |  1 +
>  .../soc/samsung/exynos-regulator-coupler.c    | 56 +++++++++++++++++++
>  4 files changed, 61 insertions(+)
>  create mode 100644 drivers/soc/samsung/exynos-regulator-coupler.c
> 
> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
> index 76838255b5fa..f185cd3d4c62 100644
> --- a/arch/arm/mach-exynos/Kconfig
> +++ b/arch/arm/mach-exynos/Kconfig
> @@ -118,6 +118,7 @@ config SOC_EXYNOS5800
>  	bool "Samsung EXYNOS5800"
>  	default y
>  	depends on SOC_EXYNOS5420
> +	select EXYNOS_REGULATOR_COUPLER
>  
>  config EXYNOS_MCPM
>  	bool
> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
> index c7a2003687c7..264185664594 100644
> --- a/drivers/soc/samsung/Kconfig
> +++ b/drivers/soc/samsung/Kconfig
> @@ -37,4 +37,7 @@ config EXYNOS_PM_DOMAINS
>  	bool "Exynos PM domains" if COMPILE_TEST
>  	depends on PM_GENERIC_DOMAINS || COMPILE_TEST
>  
> +config EXYNOS_REGULATOR_COUPLER
> +	bool "Exynos SoC Regulator Coupler" if COMPILE_TEST
> +	depends on ARCH_EXYNOS || COMPILE_TEST
>  endif
> diff --git a/drivers/soc/samsung/Makefile b/drivers/soc/samsung/Makefile
> index edd1d6ea064d..ecc3a32f6406 100644
> --- a/drivers/soc/samsung/Makefile
> +++ b/drivers/soc/samsung/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_EXYNOS_PMU)	+= exynos-pmu.o
>  obj-$(CONFIG_EXYNOS_PMU_ARM_DRIVERS)	+= exynos3250-pmu.o exynos4-pmu.o \
>  					exynos5250-pmu.o exynos5420-pmu.o
>  obj-$(CONFIG_EXYNOS_PM_DOMAINS) += pm_domains.o
> +obj-$(CONFIG_EXYNOS_REGULATOR_COUPLER) += exynos-regulator-coupler.o
> diff --git a/drivers/soc/samsung/exynos-regulator-coupler.c b/drivers/soc/samsung/exynos-regulator-coupler.c
> new file mode 100644
> index 000000000000..370a0ce4de3a
> --- /dev/null
> +++ b/drivers/soc/samsung/exynos-regulator-coupler.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
> + *	      http://www.samsung.com/
> + * Author: Marek Szyprowski <m.szyprowski@samsung.com>
> + *
> + * Simple Samsung Exynos SoC voltage coupler. Ensures that all
> + * clients set their constraints before balancing the voltages.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/regulator/coupler.h>
> +#include <linux/regulator/driver.h>
> +
> +static int exynos_coupler_balance_voltage(struct regulator_coupler *coupler,
> +					  struct regulator_dev *rdev,
> +					  suspend_state_t state)
> +{
> +	struct coupling_desc *c_desc = &rdev->coupling_desc;
> +	int ret, cons_uV = 0, cons_max_uV = INT_MAX;
> +	bool skip_coupled = false;
> +
> +	/* get coupled regulator constraints */
> +	ret = regulator_check_consumers(c_desc->coupled_rdevs[1], &cons_uV,
> +					&cons_max_uV, state);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* skip adjusting coupled regulator if it has no constraints set yet */
> +	if (cons_uV == 0)
> +		skip_coupled = true;

Hello Marek,

Does this mean that you're going to allow to violate the coupling
constraints while coupled regulator has no consumers?

I don't think that you may want to skip the coupled balancing ever.
Instead you may want to assume that the min-voltage constraint equals to
the current regulator's voltage while the coupled regulator has no
consumers.

Yours variant of the balancer doesn't prevent the voltage dropping on
regulator's enabling while coupled regulator doesn't have active
consumers. This is the problem which we previously had once OPP code was
changed to enable regulator.

Secondly, yours variant of the balancer also doesn't handle the case
where set_voltage() is invoked while one of the couples doesn't have
active consumers because voltage of this couple may drop more than
allowed on the voltage re-balancing.

I'd suggest to simply change the regulator_get_optimal_voltage() to
limit the desired_min_uV to the current voltage if coupled regulator has
no consumers.

I don't think that any of the today's upstream kernel coupled-regulator
users really need to support the case where a regulator couple is
allowed *not* to have active consumers, so for now it should be fine to
change the core code to accommodate the needs of the Exynos regulators
(IMO). We may get back to this later on once there will be a real need
support that case.

Please also note that I'm assuming that each of the coupled regulators
doesn't have more than one consumer at a time in yours case (correct?),
because yours solution won't work well in a case of multiple consumers.
There is no universal solution for this bootstrapping problem yet.

> +	return regulator_do_balance_voltage(rdev, state, skip_coupled);
> +}
> +
> +static int exynos_coupler_attach(struct regulator_coupler *coupler,
> +				 struct regulator_dev *rdev)
> +{
> +	return 0;
> +}
> +
> +static struct regulator_coupler exynos_coupler = {
> +	.attach_regulator = exynos_coupler_attach,
> +	.balance_voltage  = exynos_coupler_balance_voltage,
> +};
> +
> +static int __init exynos_coupler_init(void)
> +{
> +	if (!of_machine_is_compatible("samsung,exynos5800"))
> +		return 0;
> +
> +	return regulator_coupler_register(&exynos_coupler);
> +}
> +arch_initcall(exynos_coupler_init);
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] soc: samsung: Add simple voltage coupler for Exynos5800
  2020-06-02 15:15               ` Dmitry Osipenko
@ 2020-06-04 13:28                 ` Marek Szyprowski
  0 siblings, 0 replies; 11+ messages in thread
From: Marek Szyprowski @ 2020-06-04 13:28 UTC (permalink / raw)
  To: Dmitry Osipenko, linux-kernel, linux-pm, Mark Brown
  Cc: Liam Girdwood, Lucas Stach, Bartlomiej Zolnierkiewicz,
	Krzysztof Kozlowski, Viresh Kumar, peron.clem, Nishanth Menon,
	Stephen Boyd, Vincent Guittot, Rafael Wysocki, linux-samsung-soc,
	Chanwoo Choi

Hi Dmitry,

On 02.06.2020 17:15, Dmitry Osipenko wrote:
> 02.06.2020 16:02, Marek Szyprowski пишет:
>> Add a simple custom voltage regulator coupler for Exynos5800 SoCs, which
>> require coupling between "vdd_arm" and "vdd_int" regulators. This coupler
>> ensures that the voltage balancing for the coupled regulators is done
>> only when clients for the each regulator apply their constraints, so the
>> voltage values don't go beyond the bootloader-selected operation point
>> during the boot process. This also ensures proper voltage balancing if
>> any of the client driver is missing.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   (...)
> Hello Marek,
>
> Does this mean that you're going to allow to violate the coupling
> constraints while coupled regulator has no consumers?
>
> I don't think that you may want to skip the coupled balancing ever.
> Instead you may want to assume that the min-voltage constraint equals to
> the current regulator's voltage while the coupled regulator has no
> consumers.
>
> Yours variant of the balancer doesn't prevent the voltage dropping on
> regulator's enabling while coupled regulator doesn't have active
> consumers. This is the problem which we previously had once OPP code was
> changed to enable regulator.
>
> Secondly, yours variant of the balancer also doesn't handle the case
> where set_voltage() is invoked while one of the couples doesn't have
> active consumers because voltage of this couple may drop more than
> allowed on the voltage re-balancing.
Indeed. I've focused on disabling balancing when there are no consumers 
and I didn't notice that the max_spread might be violated in such case.
> I'd suggest to simply change the regulator_get_optimal_voltage() to
> limit the desired_min_uV to the current voltage if coupled regulator has
> no consumers.

Right, this sounds like a best solution. I have an idea to try to add it 
again to the core as a simple check: if regultor is boot_on, use current 
voltage as min_uV until a consumer is registered. I've checked and this 
approach fixes the issue. I will submit a patch in a few minutes.

> I don't think that any of the today's upstream kernel coupled-regulator
> users really need to support the case where a regulator couple is
> allowed *not* to have active consumers, so for now it should be fine to
> change the core code to accommodate the needs of the Exynos regulators
> (IMO). We may get back to this later on once there will be a real need
> support that case.
>
> Please also note that I'm assuming that each of the coupled regulators
> doesn't have more than one consumer at a time in yours case (correct?),
> because yours solution won't work well in a case of multiple consumers.
> There is no universal solution for this bootstrapping problem yet.

There are only a single consumers for each coupled regulator (cpufreq 
for vdd_arm and devfreq for vdd_int).

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20200529124948eucas1p175379ead8afd1932f7b7ae61e35cf632@eucas1p1.samsung.com>
2020-05-29 12:49 ` [PATCH 0/2] Fix regulators coupling for Exynos5800 Marek Szyprowski
     [not found]   ` <CGME20200529124951eucas1p213c748fb481a37cf98918caae5e30fd2@eucas1p2.samsung.com>
2020-05-29 12:49     ` [PATCH 1/2] regulator: extract voltage balancing code to the separate function Marek Szyprowski
     [not found]   ` <CGME20200529124952eucas1p2565c598c3c0164b5dec6ed83e19148b8@eucas1p2.samsung.com>
2020-05-29 12:49     ` [PATCH 2/2] soc: samsung: Add simple voltage coupler for Exynos5800 Marek Szyprowski
2020-05-29 17:43       ` Krzysztof Kozlowski
2020-06-01  6:28         ` Marek Szyprowski
     [not found]           ` <CGME20200602130931eucas1p1cd784c8f692fa91dc566504543a927de@eucas1p1.samsung.com>
2020-06-02 13:02             ` [PATCH v2] " Marek Szyprowski
2020-06-02 15:15               ` Dmitry Osipenko
2020-06-04 13:28                 ` Marek Szyprowski
2020-05-29 16:52   ` [PATCH 0/2] Fix regulators coupling " Mark Brown
2020-05-29 16:58     ` Mark Brown
2020-05-29 17:33       ` Krzysztof Kozlowski

Linux-Samsung-soc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-samsung-soc/0 linux-samsung-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-samsung-soc linux-samsung-soc/ https://lore.kernel.org/linux-samsung-soc \
		linux-samsung-soc@vger.kernel.org
	public-inbox-index linux-samsung-soc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-samsung-soc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git