[U-Boot,4/4] rockchip: puma-rk3399: Enable vdd-log during bootup.

Message ID 20181206112542.20143-5-christoph.muellner@theobroma-systems.com
State Superseded
Delegated to: Philipp Tomsich
Headers show
Series
  • rk3399: puma: Enable PWM regulator for Puma.
Related show

Commit Message

Christoph Muellner Dec. 6, 2018, 11:25 a.m.
On the RK3399-Q7 "Puma" module VDD_LOG is generated by an external
regulator, which sets the voltage level to 900 mV, which is within
the allowed range and which works quite fine.

However, in specific use-cases we need to adjust VDD_LOG to
maintain stable operation or to reduce power consumption.
This adjustment can be done via pwm2.

This patch allows to tune the VDD_LOG voltage level
via DTS. To do so the following things are done:

 * Setup pin muxing for PWM2 (based on DTS settings)
 * Turn on the vdd_log regulator (based on DTS settings)

Reported-by: Assaf Agmon <assaf@r-go.io>
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
---
 board/theobroma-systems/puma_rk3399/puma-rk3399.c | 43 +++++++++++++++++++++++
 1 file changed, 43 insertions(+)

Comments

Philipp Tomsich Dec. 6, 2018, 1:40 p.m. | #1
> On 06.12.2018, at 12:25, Christoph Muellner <christoph.muellner@theobroma-systems.com> wrote:
> 
> On the RK3399-Q7 "Puma" module VDD_LOG is generated by an external
> regulator, which sets the voltage level to 900 mV, which is within
> the allowed range and which works quite fine.
> 
> However, in specific use-cases we need to adjust VDD_LOG to
> maintain stable operation or to reduce power consumption.
> This adjustment can be done via pwm2.
> 
> This patch allows to tune the VDD_LOG voltage level
> via DTS. To do so the following things are done:
> 
> * Setup pin muxing for PWM2 (based on DTS settings)
> * Turn on the vdd_log regulator (based on DTS settings)
> 
> Reported-by: Assaf Agmon <assaf@r-go.io>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>

Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

See below for requested changes.

> ---
> board/theobroma-systems/puma_rk3399/puma-rk3399.c | 43 +++++++++++++++++++++++
> 1 file changed, 43 insertions(+)
> 
> diff --git a/board/theobroma-systems/puma_rk3399/puma-rk3399.c b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
> index 573e691457f..e2915fcca50 100644
> --- a/board/theobroma-systems/puma_rk3399/puma-rk3399.c
> +++ b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
> @@ -23,10 +23,34 @@
> #include <power/regulator.h>
> #include <u-boot/sha256.h>
> 
> +static int setup_pinctrl(void)

I’d prefer a forward-declaration and this further down, as I want to keep board_init()
up front.  Just a personal preference, but still ...

> +{
> +	int ret;
> +	struct udevice *pinctrl;
> +
> +	ret = uclass_get_device(UCLASS_PINCTRL, 0, &pinctrl);
> +	if (ret) {
> +		debug("%s: Cannot find pinctrl device\n", __func__);

Please use pr_debug.

> +		goto out;
> +	}
> +
> +	/* Enable pwm2 for vdd_log regulator. */
> +	ret = pinctrl_request_noflags(pinctrl, PERIPH_ID_PWM2);
> +	if (ret) {
> +		printf("%s PWM2 pinctrl init fail!\n", __func__);

Don’t use printf here — either this is a pr_err or a pr_debug.

> +		goto out;
> +	}
> +
> +out:
> +	return 0;
> +}
> +
> int board_init(void)
> {
> 	int ret;
> 
> +	setup_pinctrl();
> +
> 	/*
> 	 * We need to call into regulators_enable_boot_on() again, as the call
> 	 * during SPL may have not included all regulators.
> @@ -35,6 +59,25 @@ int board_init(void)
> 	if (ret)
> 		debug("%s: Cannot enable boot on regulator\n", __func__);
> 
> +	/*
> +	 * vdd_log is ignored by regulators_enable_boot_on(), because
> +	 * regulator-min-microvolt != regulator-max-microvolt.
> +	 * Therefore we explicitly enable it here.
> +	 */
> +	struct udevice *regulator;
> +	ret = regulator_get_by_platname("vdd_log", &regulator);
> +	if (ret) {
> +		debug("%s Looking up regulator vdd-log failed!\n", __func__);
> +		goto out;
> +	}
> +
> +	ret = regulator_set_enable(regulator, true);
>  +	if (ret) {
> +		debug("%s Enabling vdd-log failed!\n", __func__);
> +		goto out;
> +	}

Generally speaking: I would prefer a
	if (ret == 0)
		ret = regulator_set_enable(regulator, true);
	if (ret)
		pr_debug(…)	

However, this should be irrelevant anyway: why doesn’t 
	ret = regulators_enable_boot_on(false); 
in board_init() suffice?

> +
> +out:
> 	return 0;
> }
> 
> -- 
> 2.11.0
>
Christoph Muellner Dec. 6, 2018, 4:50 p.m. | #2
> On 06.12.2018, at 14:40, Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote:
> 
> 
> 
>> On 06.12.2018, at 12:25, Christoph Muellner <christoph.muellner@theobroma-systems.com> wrote:
>> 
>> On the RK3399-Q7 "Puma" module VDD_LOG is generated by an external
>> regulator, which sets the voltage level to 900 mV, which is within
>> the allowed range and which works quite fine.
>> 
>> However, in specific use-cases we need to adjust VDD_LOG to
>> maintain stable operation or to reduce power consumption.
>> This adjustment can be done via pwm2.
>> 
>> This patch allows to tune the VDD_LOG voltage level
>> via DTS. To do so the following things are done:
>> 
>> * Setup pin muxing for PWM2 (based on DTS settings)
>> * Turn on the vdd_log regulator (based on DTS settings)
>> 
>> Reported-by: Assaf Agmon <assaf@r-go.io>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> 
> Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> 
> See below for requested changes.
> 
>> ---
>> board/theobroma-systems/puma_rk3399/puma-rk3399.c | 43 +++++++++++++++++++++++
>> 1 file changed, 43 insertions(+)
>> 
>> diff --git a/board/theobroma-systems/puma_rk3399/puma-rk3399.c b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
>> index 573e691457f..e2915fcca50 100644
>> --- a/board/theobroma-systems/puma_rk3399/puma-rk3399.c
>> +++ b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
>> @@ -23,10 +23,34 @@
>> #include <power/regulator.h>
>> #include <u-boot/sha256.h>
>> 
>> +static int setup_pinctrl(void)
> 
> I’d prefer a forward-declaration and this further down, as I want to keep board_init()
> up front.  Just a personal preference, but still ...
> 
>> +{
>> +	int ret;
>> +	struct udevice *pinctrl;
>> +
>> +	ret = uclass_get_device(UCLASS_PINCTRL, 0, &pinctrl);
>> +	if (ret) {
>> +		debug("%s: Cannot find pinctrl device\n", __func__);
> 
> Please use pr_debug.
> 
>> +		goto out;
>> +	}
>> +
>> +	/* Enable pwm2 for vdd_log regulator. */
>> +	ret = pinctrl_request_noflags(pinctrl, PERIPH_ID_PWM2);
>> +	if (ret) {
>> +		printf("%s PWM2 pinctrl init fail!\n", __func__);
> 
> Don’t use printf here — either this is a pr_err or a pr_debug.
> 
>> +		goto out;
>> +	}
>> +
>> +out:
>> +	return 0;
>> +}
>> +
>> int board_init(void)
>> {
>> 	int ret;
>> 
>> +	setup_pinctrl();
>> +
>> 	/*
>> 	 * We need to call into regulators_enable_boot_on() again, as the call
>> 	 * during SPL may have not included all regulators.
>> @@ -35,6 +59,25 @@ int board_init(void)
>> 	if (ret)
>> 		debug("%s: Cannot enable boot on regulator\n", __func__);
>> 
>> +	/*
>> +	 * vdd_log is ignored by regulators_enable_boot_on(), because
>> +	 * regulator-min-microvolt != regulator-max-microvolt.
>> +	 * Therefore we explicitly enable it here.
>> +	 */
>> +	struct udevice *regulator;
>> +	ret = regulator_get_by_platname("vdd_log", &regulator);
>> +	if (ret) {
>> +		debug("%s Looking up regulator vdd-log failed!\n", __func__);
>> +		goto out;
>> +	}
>> +
>> +	ret = regulator_set_enable(regulator, true);
>> +	if (ret) {
>> +		debug("%s Enabling vdd-log failed!\n", __func__);
>> +		goto out;
>> +	}
> 
> Generally speaking: I would prefer a
> 	if (ret == 0)
> 		ret = regulator_set_enable(regulator, true);
> 	if (ret)
> 		pr_debug(…)	
> 
> However, this should be irrelevant anyway: why doesn’t 
> 	ret = regulators_enable_boot_on(false); 
> in board_init() suffice?

That's noted in the comment above.
In regulator_pre_probe() is a test if uc_pdata->min_uV == uc_pdata->max_uV.
Only if this is true, the regulator will be considered to get REGULATOR_FLAG_AUTOSET.
If that flag is not given, then the regulator will not be turned on by
regulators_enable_boot_on().

In other words: the regulator subsystem does not allow to enable
regulators by default, when regulator-min-microvolt and
regulator-max-microvolt is different in the DTS.
However these values are required for PWM regulators to specify
the possible range, they are covering (from 0 to 100 % duty-cycle).

Also note, that U-Boot's pwm_regulator driver currently evaluates
the non-documented regulator-init-microvolt entry in the DTS node.
It uses this value, if given, to setup the voltage during probe().
However, the pwm_regulator is not probed, because of the reasons above.


> 
>> +
>> +out:
>> 	return 0;
>> }
>> 
>> -- 
>> 2.11.0

Patch

diff --git a/board/theobroma-systems/puma_rk3399/puma-rk3399.c b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
index 573e691457f..e2915fcca50 100644
--- a/board/theobroma-systems/puma_rk3399/puma-rk3399.c
+++ b/board/theobroma-systems/puma_rk3399/puma-rk3399.c
@@ -23,10 +23,34 @@ 
 #include <power/regulator.h>
 #include <u-boot/sha256.h>
 
+static int setup_pinctrl(void)
+{
+	int ret;
+	struct udevice *pinctrl;
+
+	ret = uclass_get_device(UCLASS_PINCTRL, 0, &pinctrl);
+	if (ret) {
+		debug("%s: Cannot find pinctrl device\n", __func__);
+		goto out;
+	}
+
+	/* Enable pwm2 for vdd_log regulator. */
+	ret = pinctrl_request_noflags(pinctrl, PERIPH_ID_PWM2);
+	if (ret) {
+		printf("%s PWM2 pinctrl init fail!\n", __func__);
+		goto out;
+	}
+
+out:
+	return 0;
+}
+
 int board_init(void)
 {
 	int ret;
 
+	setup_pinctrl();
+
 	/*
 	 * We need to call into regulators_enable_boot_on() again, as the call
 	 * during SPL may have not included all regulators.
@@ -35,6 +59,25 @@  int board_init(void)
 	if (ret)
 		debug("%s: Cannot enable boot on regulator\n", __func__);
 
+	/*
+	 * vdd_log is ignored by regulators_enable_boot_on(), because
+	 * regulator-min-microvolt != regulator-max-microvolt.
+	 * Therefore we explicitly enable it here.
+	 */
+	struct udevice *regulator;
+	ret = regulator_get_by_platname("vdd_log", &regulator);
+	if (ret) {
+		debug("%s Looking up regulator vdd-log failed!\n", __func__);
+		goto out;
+	}
+
+	ret = regulator_set_enable(regulator, true);
+	if (ret) {
+		debug("%s Enabling vdd-log failed!\n", __func__);
+		goto out;
+	}
+
+out:
 	return 0;
 }