[1/2] power: regulator: add driver for ANATOP regulator

Message ID 20210307181849.83627-2-grandpaul@gmail.com
State New
Headers show
Series
  • ANATOP regulator driver
Related show

Commit Message

Ying-Chun Liu March 7, 2021, 6:18 p.m.
From: "Ying-Chun Liu (PaulLiu)" <paul.liu@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@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

-- 
2.30.1

Comments

Jaehoon Chung March 7, 2021, 11:04 p.m. | #1
Dear Ying-Chun

On 3/8/21 3:18 AM, Ying-Chun Liu wrote:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@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@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.

> +

> +#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.

> +		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.)

And How about passing the struct anatop_regulator instead of each values?
anatop_get/set_bits(struct anatop_regulator *regulator) {
..
}

> +{

> +	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;


> +	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.

> +	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"?


> +	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,

> +};

>
Ying-Chun Liu (PaulLiu) March 8, 2021, 1:04 p.m. | #2
Hi Jaehoon,

Thanks for the review.

On Mon, 8 Mar 2021 at 07:03, Jaehoon Chung <jh80.chung@samsung.com> wrote:

> Dear Ying-Chun

>

> On 3/8/21 3:18 AM, Ying-Chun Liu wrote:

> > From: "Ying-Chun Liu (PaulLiu)" <paul.liu@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@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.


>

> > +     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
Jaehoon Chung March 8, 2021, 9:47 p.m. | #3
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@samsung.com> wrote:

> 

>> Dear Ying-Chun

>>

>> On 3/8/21 3:18 AM, Ying-Chun Liu wrote:

>>> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@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@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

>

Patch

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>
+
+#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)
+		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)
+{
+	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;
+	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);
+	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);
+	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);
+	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,
+};