[U-Boot] [PATCH 4/4] rockchip: puma-rk3399: Enable vdd-log during bootup.
Christoph Müllner
christoph.muellner at theobroma-systems.com
Thu Dec 6 16:50:07 UTC 2018
> On 06.12.2018, at 14:40, Philipp Tomsich <philipp.tomsich at theobroma-systems.com> wrote:
>
>
>
>> On 06.12.2018, at 12:25, Christoph Muellner <christoph.muellner at 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 at r-go.io>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
>> Signed-off-by: Christoph Muellner <christoph.muellner at theobroma-systems.com>
>
> Reviewed-by: Philipp Tomsich <philipp.tomsich at 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
More information about the U-Boot
mailing list