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