[PATCH 1/2] power: regulator: add driver for ANATOP regulator
Jaehoon Chung
jh80.chung at samsung.com
Mon Mar 8 22:47:44 CET 2021
On 3/8/21 10:04 PM, Paul Liu wrote:
> Hi Jaehoon,
>
> Thanks for the review.
>
> On Mon, 8 Mar 2021 at 07:03, Jaehoon Chung <jh80.chung at samsung.com> wrote:
>
>> Dear Ying-Chun
>>
>> On 3/8/21 3:18 AM, Ying-Chun Liu wrote:
>>> From: "Ying-Chun Liu (PaulLiu)" <paul.liu at linaro.org>
>>>
>>> Anatop is an integrated regulator inside i.MX6 SoC.
>>> There are 3 digital regulators which controls PU, CORE (ARM), and SOC.
>>> And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB).
>>> This patch adds the Anatop regulator driver.
>>>
>>> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu at linaro.org>
>>> ---
>>> drivers/power/regulator/Kconfig | 10 +
>>> drivers/power/regulator/Makefile | 1 +
>>> drivers/power/regulator/anatop_regulator.c | 289 +++++++++++++++++++++
>>> 3 files changed, 300 insertions(+)
>>> create mode 100644 drivers/power/regulator/anatop_regulator.c
>>>
>>> diff --git a/drivers/power/regulator/Kconfig
>> b/drivers/power/regulator/Kconfig
>>> index fbbea18c7d..1cd1f3f5ed 100644
>>> --- a/drivers/power/regulator/Kconfig
>>> +++ b/drivers/power/regulator/Kconfig
>>> @@ -312,6 +312,16 @@ config DM_REGULATOR_STPMIC1
>>> by the PMIC device. This driver is controlled by a device tree node
>>> which includes voltage limits.
>>>
>>> +config DM_REGULATOR_ANATOP
>>> + bool "Enable driver for ANATOP regulators"
>>> + depends on DM_REGULATOR
>>> + select REGMAP
>>> + select SYSCON
>>> + help
>>> + Enable support for the Freescale i.MX on-chip ANATOP LDOs
>>> + regulators. It is recommended that this option be enabled on
>>> + i.MX6 platform.
>>> +
>>> config SPL_DM_REGULATOR_STPMIC1
>>> bool "Enable driver for STPMIC1 regulators in SPL"
>>> depends on SPL_DM_REGULATOR && PMIC_STPMIC1
>>> diff --git a/drivers/power/regulator/Makefile
>> b/drivers/power/regulator/Makefile
>>> index 9d58112dcb..e7198da911 100644
>>> --- a/drivers/power/regulator/Makefile
>>> +++ b/drivers/power/regulator/Makefile
>>> @@ -30,3 +30,4 @@ obj-$(CONFIG_DM_REGULATOR_TPS65910) +=
>> tps65910_regulator.o
>>> obj-$(CONFIG_DM_REGULATOR_TPS62360) += tps62360_regulator.o
>>> obj-$(CONFIG_$(SPL_)DM_REGULATOR_STPMIC1) += stpmic1.o
>>> obj-$(CONFIG_DM_REGULATOR_TPS65941) += tps65941_regulator.o
>>> +obj-$(CONFIG_$(SPL_)DM_REGULATOR_ANATOP) += anatop_regulator.o
>>> diff --git a/drivers/power/regulator/anatop_regulator.c
>> b/drivers/power/regulator/anatop_regulator.c
>>> new file mode 100644
>>> index 0000000000..2bb5cdbac5
>>> --- /dev/null
>>> +++ b/drivers/power/regulator/anatop_regulator.c
>>> @@ -0,0 +1,289 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +//
>>> +// Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
>>> +// Copyright (C) 2021 Linaro
>>> +
>>> +#include <common.h>
>>> +#include <errno.h>
>>> +#include <dm.h>
>>> +#include <log.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/err.h>
>>> +#include <power/pmic.h>
>>> +#include <power/regulator.h>
>>> +#include <regmap.h>
>>> +#include <syscon.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/ioport.h>
>>> +#include <dm/device-internal.h>
>>> +#include <dm/device_compat.h>
>>> +#include <dm/read.h>
>>
>> Well, i think that it can be removed more than now.
>>
>>
> Will fix it in v2.
>
>
>>> +
>>> +#define LDO_RAMP_UP_UNIT_IN_CYCLES 64 /* 64 cycles per step */
>>> +#define LDO_RAMP_UP_FREQ_IN_MHZ 24 /* cycle based on 24M OSC */
>>> +
>>> +#define LDO_POWER_GATE 0x00
>>> +#define LDO_FET_FULL_ON 0x1f
>>> +
>>> +struct anatop_regulator {
>>> + const char *name;
>>> + u32 control_reg;
>>> + u32 vol_bit_shift;
>>> + u32 vol_bit_width;
>>> + u32 min_bit_val;
>>> + u32 min_voltage;
>>> + u32 max_voltage;
>>> + u32 delay_reg;
>>> + u32 delay_bit_shift;
>>> + u32 delay_bit_width;
>>> +};
>>> +
>>> +static u32 anatop_get_bits(struct udevice *dev, u32 addr, int bit_shift,
>>> + int bit_width)
>>> +{
>>> + struct udevice *syscon;
>>> + struct regmap *regmap;
>>> + int err;
>>> + u32 val, mask;
>>> +
>>> + syscon = dev_get_parent(dev);
>>> + if (!syscon) {
>>> + dev_err(dev, "%s: unable to find syscon device\n",
>> __func__);
>>> + return err;
>>> + }
>>> +
>>> + regmap = syscon_get_regmap(syscon);
>>> + if (IS_ERR(regmap)) {
>>> + dev_err(dev, "%s: unable to find regmap (%ld)\n", __func__,
>>> + PTR_ERR(regmap));
>>> + return PTR_ERR(regmap);
>>> + }
>>> +
>>> + if (bit_width == 32)
>>
>> Use macro instead of 32, plz.
>>
>>
> Will fix it in v2.
>
>
>>> + mask = ~0;
>>> + else
>>> + mask = (1 << bit_width) - 1;
>>> +
>>> + err = regmap_read(regmap, addr, &val);
>>> + if (err)
>>> + return err;
>>> +
>>> + val = (val >> bit_shift) & mask;
>>> +
>>> + return val;
>>> +}
>>> +
>>> +void anatop_set_bits(struct udevice *dev, u32 addr, int bit_shift,
>>> + int bit_width, u32 data)
>>
>> static?
>> doesn't it need to return int type?
>> If there is a somehting failed, it seems that it needs to pass the error
>> number.
>> (get_bits is returend to error..but set_bits doesn't return. It's strange.)
>>
>>
> Will fix it in v2.
>
>
>> And How about passing the struct anatop_regulator instead of each values?
>> anatop_get/set_bits(struct anatop_regulator *regulator) {
>> ..
>> }
>>
>>
> Will fix it in v2.
>
>
>>> +{
>>> + struct udevice *syscon;
>>> + struct regmap *regmap;
>>> + int err;
>>> + u32 val, mask;
>>> +
>>> + syscon = dev_get_parent(dev);
>>> + if (!syscon) {
>>> + dev_err(dev, "%s: unable to find syscon device\n",
>> __func__);
>>> + return;
>>> + }
>>> +
>>> + regmap = syscon_get_regmap(syscon);
>>> + if (IS_ERR(regmap)) {
>>> + dev_err(dev,
>>> + "%s: unable to find regmap (%ld)\n", __func__,
>>> + PTR_ERR(regmap));
>>> + return;
>>> + }
>>> +
>>> + if (bit_width == 32)
>>> + mask = ~0;
>>> + else
>>> + mask = (1 << bit_width) - 1;
>>> +
>>> + err = regmap_read(regmap, addr, &val);
>>> + if (err) {
>>> + dev_err(dev,
>>> + "%s: cannot read reg (%d)\n", __func__,
>>> + err);
>>> + return;
>>> + }
>>> + val = val & ~(mask << bit_shift);
>>> + err = regmap_write(regmap, addr, (data << bit_shift) | val);
>>> + if (err) {
>>> + dev_err(dev,
>>> + "%s: cannot wrie reg (%d)\n", __func__,
>>> + err);
>>> + return;
>>> + }
>>> +}
>>> +
>>> +static int anatop_set_voltage(struct udevice *dev, int uV)
>>> +{
>>> + const struct anatop_regulator *anatop_reg = dev_get_plat(dev);
>>> + u32 val;
>>> + u32 sel;
>>> + int delayus = 0;
>>> + int ret = 0;
>>> + int uv;
>>> +
>>> + uv = uV;
>>
>> Not need to assign at here. Assign to above.
>> int uv = uV;
>>
>> Will fix it in v2.
>
>>
>>> + dev_dbg(dev, "%s: uv %d, min %d, max %d\n", __func__,
>>> + uv, anatop_reg->min_voltage,
>>> + anatop_reg->max_voltage);
>>> +
>>> + if (uv < anatop_reg->min_voltage)
>>> + return -EINVAL;
>>> +
>>> + if (!anatop_reg->control_reg)
>>> + return -ENOTSUPP;
>>> +
>>> + sel = DIV_ROUND_UP(uv - anatop_reg->min_voltage, 25000);
>>
>> What is 25000? If it's min value, use MACRO, plz.
>>
>>
> It is "STEP" or "RESOLUTION". Sorry for my bad English. 25000 uV is the
> anatop regulator's "STEP" or "RESOLUTION". So the uV is register value *
> 25000 uV + min uV.
> I'll change it to use MACRO with a proper name to avoid confusion.
>
>
>
>>> + if (sel * 25000 + anatop_reg->min_voltage >
>> anatop_reg->max_voltage)
>>> + return -EINVAL;
>>> + val = anatop_reg->min_bit_val + sel;
>>> + dev_dbg(dev, "%s: calculated val %d\n", __func__, val);
>>> +
>>> + /* check whether need to care about LDO ramp up speed */
>>> + if (anatop_reg->delay_bit_width) {
>>> + /*
>>> + * the delay for LDO ramp up time is
>>> + * based on the register setting, we need
>>> + * to calculate how many steps LDO need to
>>> + * ramp up, and how much delay needed. (us)
>>> + */
>>> + u32 old_sel;
>>> + u32 new_sel = sel;
>>> +
>>> + old_sel = anatop_get_bits(dev,
>>> + anatop_reg->control_reg,
>>> + anatop_reg->vol_bit_shift,
>>> + anatop_reg->vol_bit_width);
>>> + old_sel = old_sel - anatop_reg->min_bit_val;
>>> +
>>> + if (new_sel > old_sel) {
>>> + val = anatop_get_bits(dev,
>>> + anatop_reg->delay_reg,
>>> + anatop_reg->delay_bit_shift,
>>> + anatop_reg->delay_bit_width);
>>> + delayus = (new_sel - old_sel) *
>>> + (LDO_RAMP_UP_UNIT_IN_CYCLES << val) /
>>> + LDO_RAMP_UP_FREQ_IN_MHZ + 1;
>>> + }
>>> + }
>>> +
>>> + anatop_set_bits(dev,
>>> + anatop_reg->control_reg,
>>> + anatop_reg->vol_bit_shift,
>>> + anatop_reg->vol_bit_width,
>>> + val);
>>> +
>>> + if (delayus)
>>> + udelay(delayus);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int anatop_get_voltage(struct udevice *dev)
>>> +{
>>> + const struct anatop_regulator *anatop_reg = dev_get_plat(dev);
>>> + u32 val;
>>> + u32 sel;
>>> +
>>> + val = anatop_get_bits(dev,
>>> + anatop_reg->control_reg,
>>> + anatop_reg->vol_bit_shift,
>>> + anatop_reg->vol_bit_width);
>>> + sel = val - anatop_reg->min_bit_val;
>>> +
>>> + return sel * 25000 + anatop_reg->min_voltage;
>>> +}
>>> +
>>> +static const struct dm_regulator_ops anatop_regulator_ops = {
>>> + .set_value = anatop_set_voltage,
>>> + .get_value = anatop_get_voltage,
>>> +};
>>> +
>>> +static int anatop_regulator_probe(struct udevice *dev)
>>> +{
>>> + struct anatop_regulator *sreg;
>>> + int ret = 0;
>>> +
>>> + sreg = dev_get_plat(dev);
>>> + if (!sreg) {
>>> + dev_err(dev, "failed to get plat data\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + sreg->name = ofnode_read_string(dev_ofnode(dev), "regulator-name");
>>> + if (!sreg->name) {
>>> + dev_err(dev, "failed to get a regulator-name\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + ret = ofnode_read_u32(dev_ofnode(dev),
>>> + "anatop-reg-offset",
>>> + &sreg->control_reg);
>>> + if (ret) {
>>> + dev_err(dev, "no anatop-reg-offset property set\n");
>>> + return ret;
>>> + }
>>> + ret = ofnode_read_u32(dev_ofnode(dev),
>>> + "anatop-vol-bit-width",
>>> + &sreg->vol_bit_width);
>>> + if (ret) {
>>> + dev_err(dev, "no anatop-vol-bit-width property set\n");
>>> + return ret;
>>> + }
>>> + ret = ofnode_read_u32(dev_ofnode(dev),
>>> + "anatop-vol-bit-shift",
>>> + &sreg->vol_bit_shift);
>>> + if (ret) {
>>> + dev_err(dev, "no anatop-vol-bit-shift property set\n");
>>> + return ret;
>>> + }
>>> + ret = ofnode_read_u32(dev_ofnode(dev),
>>> + "anatop-min-bit-val",
>>> + &sreg->min_bit_val);
>>> + if (ret) {
>>> + dev_err(dev, "no anatop-min-bit-val property set\n");
>>> + return ret;
>>> + }
>>> + ret = ofnode_read_u32(dev_ofnode(dev),
>>> + "anatop-min-voltage",
>>> + &sreg->min_voltage);
>>
>> I don't know why anatop-min-voltage is need?
>> Doesn't it use "regulator-min-microvolt"?
>>
>>
> It is needed. It is because the anatop-min-voltage is used to calculate the
> register value of anatop.
> But regulator-min-microvolt might not be equal to anatop-min-voltage.
> Because regulator-min-microvolt describes the device that consumes the
> regulator.
> So the device might need higher min-microvolt based on the hardware design
> and will be described in device tree settings.
> But the value to be written in the anatop register must be based
> on anatop-min-voltage.
Thanks for explanation.
I guessed that it can be used regulator-min-microvolt after checked your [PATCH 2/2].
Because there are same value with regulator-microvolt and antop-min-voltage.
Best Regards,
Jaehoon Chung
>
>
>>
>>> + if (ret) {
>>> + dev_err(dev, "no anatop-min-voltage property set\n");
>>> + return ret;
>>> + }
>>> + ret = ofnode_read_u32(dev_ofnode(dev),
>>> + "anatop-max-voltage",
>>> + &sreg->max_voltage);
>>
>> Ditto.
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>> + if (ret) {
>>> + dev_err(dev, "no anatop-max-voltage property set\n");
>>> + return ret;
>>> + }
>>> +
>>> + /* read LDO ramp up setting, only for core reg */
>>> + ofnode_read_u32(dev_ofnode(dev), "anatop-delay-reg-offset",
>>> + &sreg->delay_reg);
>>> + ofnode_read_u32(dev_ofnode(dev), "anatop-delay-bit-width",
>>> + &sreg->delay_bit_width);
>>> + ofnode_read_u32(dev_ofnode(dev), "anatop-delay-bit-shift",
>>> + &sreg->delay_bit_shift);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct udevice_id of_anatop_regulator_match_tbl[] = {
>>> + { .compatible = "fsl,anatop-regulator", },
>>> + { /* end */ }
>>> +};
>>> +
>>> +U_BOOT_DRIVER(anatop_regulator) = {
>>> + .name = "anatop_regulator",
>>> + .id = UCLASS_REGULATOR,
>>> + .ops = &anatop_regulator_ops,
>>> + .of_match = of_anatop_regulator_match_tbl,
>>> + .plat_auto = sizeof(struct anatop_regulator),
>>> + .probe = anatop_regulator_probe,
>>> +};
>>>
>>
>> Yours,
> Paul
>
More information about the U-Boot
mailing list