diff mbox series

[2/2] soc: samsung: Add simple voltage coupler for Exynos5800

Message ID 20200529124940.10675-3-m.szyprowski@samsung.com (mailing list archive)
State Not Applicable, archived
Headers show
Series Fix regulators coupling for Exynos5800 | expand

Commit Message

Marek Szyprowski May 29, 2020, 12:49 p.m. UTC
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

Comments

Krzysztof Kozlowski May 29, 2020, 5:43 p.m. UTC | #1
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
Marek Szyprowski June 1, 2020, 6:28 a.m. UTC | #2
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
diff mbox series

Patch

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);