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

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,

+};