[U-Boot] [PATCH 0/4] rk3399: puma: Enable PWM regulator for Puma.
This patch series allows to tune VDD_LOG on RK3399-Q7 Puma boards using a PWM adjustable regulator.
The effect of the series is, that VDD_LOG will be set to about 950 mV on Puma. This is required to address stability issues on Puma.
Christoph Muellner (4): rockchip: rk3399-puma: Cleanup vdd_log node in puma DTSI. power: regulator: Allow PWM regulator to be omitted from SPL. rockchip: defconfig: puma-rk3399: enable PWM regulator rockchip: puma-rk3399: Enable vdd-log during bootup.
arch/arm/dts/rk3399-puma.dtsi | 5 +-- board/theobroma-systems/puma_rk3399/puma-rk3399.c | 43 +++++++++++++++++++++++ configs/puma-rk3399_defconfig | 1 + drivers/power/regulator/Kconfig | 7 ++++ drivers/power/regulator/Makefile | 2 +- 5 files changed, 53 insertions(+), 5 deletions(-)
* Eliminate non-standard entries (rockchip,pwm_id and rockchip,pwm_voltage). * Define target voltage level for vdd-log to 950 mV.
Signed-off-by: Christoph Muellner christoph.muellner@theobroma-systems.com --- arch/arm/dts/rk3399-puma.dtsi | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/arch/arm/dts/rk3399-puma.dtsi b/arch/arm/dts/rk3399-puma.dtsi index 11ffcb71778..2cac16e2ce7 100644 --- a/arch/arm/dts/rk3399-puma.dtsi +++ b/arch/arm/dts/rk3399-puma.dtsi @@ -180,10 +180,7 @@ regulator-max-microvolt = <1400000>; regulator-always-on; regulator-boot-on; - - /* for rockchip boot on */ - rockchip,pwm_id= <2>; - rockchip,pwm_voltage = <1000000>; + regulator-init-microvolt = <950000>; }; };
Christoph,
On 06.12.2018, at 12:25, Christoph Muellner christoph.muellner@theobroma-systems.com wrote:
- Eliminate non-standard entries (rockchip,pwm_id and rockchip,pwm_voltage).
- Define target voltage level for vdd-log to 950 mV.
Could you provide the necessary background in the commit message to assert that the non-standard entries are indeed not needed and why you want to set the voltage to 950mV?
In other words: a bit of motivation for this change would be appreciated.
Signed-off-by: Christoph Muellner christoph.muellner@theobroma-systems.com
arch/arm/dts/rk3399-puma.dtsi | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/arch/arm/dts/rk3399-puma.dtsi b/arch/arm/dts/rk3399-puma.dtsi index 11ffcb71778..2cac16e2ce7 100644 --- a/arch/arm/dts/rk3399-puma.dtsi +++ b/arch/arm/dts/rk3399-puma.dtsi @@ -180,10 +180,7 @@ regulator-max-microvolt = <1400000>; regulator-always-on; regulator-boot-on;
/* for rockchip boot on */rockchip,pwm_id= <2>;rockchip,pwm_voltage = <1000000>;
regulator-init-microvolt = <950000>;};
-- 2.11.0
This patch allows to enable the PWM regulator driver independent for U-Boot and SPL.
Signed-off-by: Christoph Muellner christoph.muellner@theobroma-systems.com --- drivers/power/regulator/Kconfig | 7 +++++++ drivers/power/regulator/Makefile | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig index 2561a8a8561..5bf1f135157 100644 --- a/drivers/power/regulator/Kconfig +++ b/drivers/power/regulator/Kconfig @@ -61,6 +61,13 @@ config REGULATOR_PWM This driver is controlled by a device tree node which includes voltage limits.
+config SPL_REGULATOR_PWM + bool "Enable Driver for PWM regulators in SPL" + depends on REGULATOR_PWM + ---help--- + This config enables implementation of driver-model regulator uclass + features for PWM regulators in SPL. + config DM_REGULATOR_MAX77686 bool "Enable Driver Model for REGULATOR MAX77686" depends on DM_REGULATOR && DM_PMIC_MAX77686 diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile index a5f5683d6e9..0bb802b03d2 100644 --- a/drivers/power/regulator/Makefile +++ b/drivers/power/regulator/Makefile @@ -9,7 +9,7 @@ obj-$(CONFIG_REGULATOR_ACT8846) += act8846.o obj-$(CONFIG_REGULATOR_AS3722) += as3722_regulator.o obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o obj-$(CONFIG_$(SPL_)DM_PMIC_PFUZE100) += pfuze100.o -obj-$(CONFIG_REGULATOR_PWM) += pwm_regulator.o +obj-$(CONFIG_$(SPL_)REGULATOR_PWM) += pwm_regulator.o obj-$(CONFIG_$(SPL_)DM_REGULATOR_FIXED) += fixed.o obj-$(CONFIG_$(SPL_)DM_REGULATOR_GPIO) += gpio-regulator.o obj-$(CONFIG_REGULATOR_RK8XX) += rk8xx.o
On 06.12.2018, at 12:25, Christoph Muellner christoph.muellner@theobroma-systems.com wrote:
This patch allows to enable the PWM regulator driver independent for U-Boot and SPL.
Signed-off-by: Christoph Muellner christoph.muellner@theobroma-systems.com
Reviewed-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
Signed-off-by: Christoph Muellner christoph.muellner@theobroma-systems.com --- configs/puma-rk3399_defconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/configs/puma-rk3399_defconfig b/configs/puma-rk3399_defconfig index 8e33e09cee8..d23cf4bc523 100644 --- a/configs/puma-rk3399_defconfig +++ b/configs/puma-rk3399_defconfig @@ -78,6 +78,7 @@ CONFIG_PINCTRL_ROCKCHIP_RK3399=y CONFIG_DM_PMIC=y CONFIG_PMIC_RK8XX=y CONFIG_SPL_DM_REGULATOR=y +CONFIG_REGULATOR_PWM=y CONFIG_DM_REGULATOR_FIXED=y CONFIG_SPL_DM_REGULATOR_FIXED=y CONFIG_DM_REGULATOR_GPIO=y
On 06.12.2018, at 12:25, Christoph Muellner christoph.muellner@theobroma-systems.com wrote:
Signed-off-by: Christoph Muellner christoph.muellner@theobroma-systems.com
You are missing a commit message. Please revise.
configs/puma-rk3399_defconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/configs/puma-rk3399_defconfig b/configs/puma-rk3399_defconfig index 8e33e09cee8..d23cf4bc523 100644 --- a/configs/puma-rk3399_defconfig +++ b/configs/puma-rk3399_defconfig @@ -78,6 +78,7 @@ CONFIG_PINCTRL_ROCKCHIP_RK3399=y CONFIG_DM_PMIC=y CONFIG_PMIC_RK8XX=y CONFIG_SPL_DM_REGULATOR=y +CONFIG_REGULATOR_PWM=y CONFIG_DM_REGULATOR_FIXED=y CONFIG_SPL_DM_REGULATOR_FIXED=y CONFIG_DM_REGULATOR_GPIO=y -- 2.11.0
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(+)
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", ®ulator); + 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; }
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", ®ulator);
- 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
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", ®ulator);
- 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
participants (3)
-
Christoph Muellner -
Christoph Müllner
-
Philipp Tomsich