| 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. | expand |
> 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
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; }