[PATCH 1/3] regulator: rk8xx: Return correct voltage for buck converters
Jonas Karlman
jonas at kwiboo.se
Mon Aug 7 04:33:04 CEST 2023
Hi Simon,
On 2023-08-07 03:33, Simon Glass wrote:
> Hi Jonas,
>
> On Sun, 6 Aug 2023 at 06:04, Jonas Karlman <jonas at kwiboo.se> wrote:
>>
>> From: Joseph Chen <chenjh at rock-chips.com>
>>
>> Information from the first range group is always used to calculate the
>> voltage returned for buck converters. This may result in wrong voltage
>> reported back to the regulator_get_value caller.
>>
>> Traverse all the possible BUCK ranges to fix this issue.
>>
>> Fixes: addd062beacc ("power: pmic: rk816: support rk816 pmic")
>> Fixes: b62280745e55 ("power: pmic: rk805: support rk805 pmic")
>> Fixes: b4a35574b38d ("power: pmic: rk817: support rk817 pmic")
>> Fixes: ee30068fa574 ("power: pmic: rk809: support rk809 pmic")
>> Signed-off-by: Joseph Chen <chenjh at rock-chips.com>
>> [jonas at kwiboo.se: fix checkpatch error, update commit message]
>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>> ---
>> drivers/power/regulator/rk8xx.c | 98 ++++++++++++++++++++-------------
>> 1 file changed, 60 insertions(+), 38 deletions(-)
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
>>
>> diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c
>> index e95640a39b0a..21949d678326 100644
>> --- a/drivers/power/regulator/rk8xx.c
>> +++ b/drivers/power/regulator/rk8xx.c
>> @@ -88,62 +88,65 @@ struct rk8xx_reg_info {
>> u8 config_reg;
>> u8 vsel_mask;
>> u8 min_sel;
>> + /* only for buck now */
>> + u8 max_sel;
>> + u8 range_num;
>> };
>
> Please comment the items. What is range_num? The number of ranges or
> the number of ranges - 1?
Will take a closer look and add comments in v2.
>
>>
>> static const struct rk8xx_reg_info rk808_buck[] = {
>> - { 712500, 12500, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, REG_BUCK1_CONFIG, RK808_BUCK_VSEL_MASK, },
>> - { 712500, 12500, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, REG_BUCK2_CONFIG, RK808_BUCK_VSEL_MASK, },
>> - { 712500, 12500, NA, NA, REG_BUCK3_CONFIG, RK808_BUCK_VSEL_MASK, },
>> - { 1800000, 100000, REG_BUCK4_ON_VSEL, REG_BUCK4_SLP_VSEL, REG_BUCK4_CONFIG, RK808_BUCK4_VSEL_MASK, },
>> + { 712500, 12500, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, REG_BUCK1_CONFIG, RK808_BUCK_VSEL_MASK, 0x00, 0x3f, 1},
>> + { 712500, 12500, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, REG_BUCK2_CONFIG, RK808_BUCK_VSEL_MASK, 0x00, 0x3f, 1},
>> + { NA, NA, NA, NA, REG_BUCK3_CONFIG, NA, NA, NA, 1},
>> + { 1800000, 100000, REG_BUCK4_ON_VSEL, REG_BUCK4_SLP_VSEL, REG_BUCK4_CONFIG, RK808_BUCK4_VSEL_MASK, 0x00, 0x0f, 1},
>> };
>>
>> static const struct rk8xx_reg_info rk816_buck[] = {
>> /* buck 1 */
>> - { 712500, 12500, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, 0x00, },
>> - { 1800000, 200000, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, 0x3c, },
>> - { 2300000, 0, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, 0x3f, },
>> + { 712500, 12500, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, 0x00, 0x3b, 3},
>> + { 1800000, 200000, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, 0x3c, 0x3e, 3},
>> + { 2300000, 0, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, 0x3f, 0x3f, 3},
>> /* buck 2 */
>> - { 712500, 12500, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, 0x00, },
>> - { 1800000, 200000, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, 0x3c, },
>> - { 2300000, 0, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, 0x3f, },
>> + { 712500, 12500, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, 0x00, 0x3b, 3},
>> + { 1800000, 200000, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, 0x3c, 0x3e, 3},
>> + { 2300000, 0, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, 0x3f, 0x3f, 3},
>> /* buck 3 */
>> - { 712500, 12500, NA, NA, REG_BUCK3_CONFIG, RK818_BUCK_VSEL_MASK, },
>> + { NA, NA, NA, NA, REG_BUCK3_CONFIG, NA, NA, NA, 1},
>> /* buck 4 */
>> - { 800000, 100000, REG_BUCK4_ON_VSEL, REG_BUCK4_SLP_VSEL, REG_BUCK4_CONFIG, RK818_BUCK4_VSEL_MASK, },
>> + { 800000, 100000, REG_BUCK4_ON_VSEL, REG_BUCK4_SLP_VSEL, REG_BUCK4_CONFIG, RK818_BUCK4_VSEL_MASK, 0x00, 0x1b, 1},
>> };
>>
>> static const struct rk8xx_reg_info rk809_buck5[] = {
>> /* buck 5 */
>> - { 1500000, 0, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, RK809_BUCK5_VSEL_MASK, 0x00, },
>> - { 1800000, 200000, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, RK809_BUCK5_VSEL_MASK, 0x01, },
>> - { 2800000, 200000, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, RK809_BUCK5_VSEL_MASK, 0x04, },
>> - { 3300000, 300000, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, RK809_BUCK5_VSEL_MASK, 0x06, },
>> + { 1500000, 0, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, RK809_BUCK5_VSEL_MASK, 0x00, 0x00, 4},
>> + { 1800000, 200000, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, RK809_BUCK5_VSEL_MASK, 0x01, 0x03, 4},
>> + { 2800000, 200000, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, RK809_BUCK5_VSEL_MASK, 0x04, 0x05, 4},
>> + { 3300000, 300000, RK809_BUCK5_CONFIG(0), RK809_BUCK5_CONFIG(1), NA, RK809_BUCK5_VSEL_MASK, 0x06, 0x07, 4},
>> };
>>
>> static const struct rk8xx_reg_info rk817_buck[] = {
>> /* buck 1 */
>> - { 500000, 12500, RK817_BUCK_ON_VSEL(1), RK817_BUCK_SLP_VSEL(1), RK817_BUCK_CONFIG(1), RK817_BUCK_VSEL_MASK, 0x00, },
>> - { 1500000, 100000, RK817_BUCK_ON_VSEL(1), RK817_BUCK_SLP_VSEL(1), RK817_BUCK_CONFIG(1), RK817_BUCK_VSEL_MASK, 0x50, },
>> - { 2400000, 0, RK817_BUCK_ON_VSEL(1), RK817_BUCK_SLP_VSEL(1), RK817_BUCK_CONFIG(1), RK817_BUCK_VSEL_MASK, 0x59, },
>> + { 500000, 12500, RK817_BUCK_ON_VSEL(1), RK817_BUCK_SLP_VSEL(1), RK817_BUCK_CONFIG(1), RK817_BUCK_VSEL_MASK, 0x00, 0x4f, 3},
>> + { 1500000, 100000, RK817_BUCK_ON_VSEL(1), RK817_BUCK_SLP_VSEL(1), RK817_BUCK_CONFIG(1), RK817_BUCK_VSEL_MASK, 0x50, 0x58, 3},
>> + { 2400000, 0, RK817_BUCK_ON_VSEL(1), RK817_BUCK_SLP_VSEL(1), RK817_BUCK_CONFIG(1), RK817_BUCK_VSEL_MASK, 0x59, 0x59, 3},
>> /* buck 2 */
>> - { 500000, 12500, RK817_BUCK_ON_VSEL(2), RK817_BUCK_SLP_VSEL(2), RK817_BUCK_CONFIG(2), RK817_BUCK_VSEL_MASK, 0x00, },
>> - { 1500000, 100000, RK817_BUCK_ON_VSEL(2), RK817_BUCK_SLP_VSEL(2), RK817_BUCK_CONFIG(2), RK817_BUCK_VSEL_MASK, 0x50, },
>> - { 2400000, 0, RK817_BUCK_ON_VSEL(2), RK817_BUCK_SLP_VSEL(2), RK817_BUCK_CONFIG(2), RK817_BUCK_VSEL_MASK, 0x59, },
>> + { 500000, 12500, RK817_BUCK_ON_VSEL(2), RK817_BUCK_SLP_VSEL(2), RK817_BUCK_CONFIG(2), RK817_BUCK_VSEL_MASK, 0x00, 0x4f, 3},
>> + { 1500000, 100000, RK817_BUCK_ON_VSEL(2), RK817_BUCK_SLP_VSEL(2), RK817_BUCK_CONFIG(2), RK817_BUCK_VSEL_MASK, 0x50, 0x58, 3},
>> + { 2400000, 0, RK817_BUCK_ON_VSEL(2), RK817_BUCK_SLP_VSEL(2), RK817_BUCK_CONFIG(2), RK817_BUCK_VSEL_MASK, 0x59, 0x59, 3},
>> /* buck 3 */
>> - { 500000, 12500, RK817_BUCK_ON_VSEL(3), RK817_BUCK_SLP_VSEL(3), RK817_BUCK_CONFIG(3), RK817_BUCK_VSEL_MASK, 0x00, },
>> - { 1500000, 100000, RK817_BUCK_ON_VSEL(3), RK817_BUCK_SLP_VSEL(3), RK817_BUCK_CONFIG(3), RK817_BUCK_VSEL_MASK, 0x50, },
>> - { 2400000, 0, RK817_BUCK_ON_VSEL(3), RK817_BUCK_SLP_VSEL(3), RK817_BUCK_CONFIG(3), RK817_BUCK_VSEL_MASK, 0x59, },
>> + { 500000, 12500, RK817_BUCK_ON_VSEL(3), RK817_BUCK_SLP_VSEL(3), RK817_BUCK_CONFIG(3), RK817_BUCK_VSEL_MASK, 0x00, 0x4f, 3},
>> + { 1500000, 100000, RK817_BUCK_ON_VSEL(3), RK817_BUCK_SLP_VSEL(3), RK817_BUCK_CONFIG(3), RK817_BUCK_VSEL_MASK, 0x50, 0x58, 3},
>> + { 2400000, 0, RK817_BUCK_ON_VSEL(3), RK817_BUCK_SLP_VSEL(3), RK817_BUCK_CONFIG(3), RK817_BUCK_VSEL_MASK, 0x59, 0x59, 3},
>> /* buck 4 */
>> - { 500000, 12500, RK817_BUCK_ON_VSEL(4), RK817_BUCK_SLP_VSEL(4), RK817_BUCK_CONFIG(4), RK817_BUCK_VSEL_MASK, 0x00, },
>> - { 1500000, 100000, RK817_BUCK_ON_VSEL(4), RK817_BUCK_SLP_VSEL(4), RK817_BUCK_CONFIG(4), RK817_BUCK_VSEL_MASK, 0x50, },
>> - { 3400000, 0, RK817_BUCK_ON_VSEL(4), RK817_BUCK_SLP_VSEL(4), RK817_BUCK_CONFIG(4), RK817_BUCK_VSEL_MASK, 0x63, },
>> + { 500000, 12500, RK817_BUCK_ON_VSEL(4), RK817_BUCK_SLP_VSEL(4), RK817_BUCK_CONFIG(4), RK817_BUCK_VSEL_MASK, 0x00, 0x4f, 3},
>> + { 1500000, 100000, RK817_BUCK_ON_VSEL(4), RK817_BUCK_SLP_VSEL(4), RK817_BUCK_CONFIG(4), RK817_BUCK_VSEL_MASK, 0x50, 0x62, 3},
>> + { 3400000, 0, RK817_BUCK_ON_VSEL(4), RK817_BUCK_SLP_VSEL(4), RK817_BUCK_CONFIG(4), RK817_BUCK_VSEL_MASK, 0x63, 0x63, 3},
>> };
>>
>> static const struct rk8xx_reg_info rk818_buck[] = {
>> - { 712500, 12500, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, },
>> - { 712500, 12500, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, },
>> - { 712500, 12500, NA, NA, REG_BUCK3_CONFIG, RK818_BUCK_VSEL_MASK, },
>> - { 1800000, 100000, REG_BUCK4_ON_VSEL, REG_BUCK4_SLP_VSEL, REG_BUCK4_CONFIG, RK818_BUCK4_VSEL_MASK, },
>> + { 712500, 12500, REG_BUCK1_ON_VSEL, REG_BUCK1_SLP_VSEL, REG_BUCK1_CONFIG, RK818_BUCK_VSEL_MASK, 0x00, 0x3f, 1},
>> + { 712500, 12500, REG_BUCK2_ON_VSEL, REG_BUCK2_SLP_VSEL, REG_BUCK2_CONFIG, RK818_BUCK_VSEL_MASK, 0x00, 0x3f, 1},
>> + { NA, NA, NA, NA, REG_BUCK3_CONFIG, NA, NA, NA, 1},
>> + { 1800000, 100000, REG_BUCK4_ON_VSEL, REG_BUCK4_SLP_VSEL, REG_BUCK4_CONFIG, RK818_BUCK4_VSEL_MASK, 0x00, 0x10, 1},
>> };
>>
>> #ifdef ENABLE_DRIVER
>> @@ -706,10 +709,9 @@ static int _ldo_get_suspend_enable(struct udevice *pmic, int ldo)
>> static int buck_get_value(struct udevice *dev)
>> {
>> int buck = dev->driver_data - 1;
>> - /* We assume level-1 voltage is enough for usage in U-Boot */
>> const struct rk8xx_reg_info *info = get_buck_reg(dev->parent, buck, 0);
>> int mask = info->vsel_mask;
>> - int ret, val;
>> + int i, ret, val;
>>
>> if (info->vsel_reg == NA)
>> return -ENOSYS;
>> @@ -717,9 +719,20 @@ static int buck_get_value(struct udevice *dev)
>> ret = pmic_reg_read(dev->parent, info->vsel_reg);
>> if (ret < 0)
>> return ret;
>> +
>> val = ret & mask;
>> + if (val >= info->min_sel && val <= info->max_sel)
>> + goto finish;
>>
>> - return info->min_uv + val * info->step_uv;
>> + /* unlucky to try */
>> + for (i = 1; i < info->range_num; i++) {
>> + info++;
>> + if (val <= info->max_sel && val >= info->min_sel)
>> + break;
>> + }
>
> Can you add a comment here as to what happens when you fall through?
Sure, will try to get a better understanding and add comments in v2.
>
>> +
>> +finish:
>> + return info->min_uv + (val - info->min_sel) * info->step_uv;
>> }
>>
>> static int buck_set_value(struct udevice *dev, int uvolt)
>> @@ -732,10 +745,9 @@ static int buck_set_value(struct udevice *dev, int uvolt)
>> static int buck_get_suspend_value(struct udevice *dev)
>> {
>> int buck = dev->driver_data - 1;
>> - /* We assume level-1 voltage is enough for usage in U-Boot */
>> const struct rk8xx_reg_info *info = get_buck_reg(dev->parent, buck, 0);
>> int mask = info->vsel_mask;
>> - int ret, val;
>> + int i, ret, val;
>>
>> if (info->vsel_sleep_reg == NA)
>> return -ENOSYS;
>> @@ -745,8 +757,18 @@ static int buck_get_suspend_value(struct udevice *dev)
>> return ret;
>>
>> val = ret & mask;
>> + if (val <= info->max_sel && val >= info->min_sel)
>> + goto finish;
>>
>> - return info->min_uv + val * info->step_uv;
>> + /* unlucky to try */
>> + for (i = 1; i < info->range_num; i++) {
>> + info++;
>
> It seems you could drop the if() part above and just use this loop, if
> i started at 0 and you incremented info after the next line?
You are probably right, I only imported this commit as-is from vendor
U-Boot and only fixed a missing space that was reported by checkpatch.
Without this patch some regulators would report completely wrong voltage
e.g. for 1.8V it reported 1.5V and for 3.3V it reported ~1.8V. Something
that completely broke the logic of the IO-domain driver.
Will try to refactor this code to reduce complexity in v2.
Regards,
Jonas
>
>> + if (val <= info->max_sel && val >= info->min_sel)
>> + break;
>> + }
>> +
>> +finish:
>> + return info->min_uv + (val - info->min_sel) * info->step_uv;
>> }
>>
>> static int buck_set_suspend_value(struct udevice *dev, int uvolt)
>> --
>> 2.41.0
>>
>
> Regards,
> Simon
More information about the U-Boot
mailing list