| Message ID | 20200602130211.2727-1-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2] soc: samsung: Add simple voltage coupler for Exynos5800 | expand |
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); >
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
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);
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