[PATCH v2 1/2] power: regulator: add driver for ANATOP regulator
Sean Anderson
sean.anderson at seco.com
Tue Mar 23 16:06:29 CET 2021
On 3/23/21 9:48 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.
+CC Peng Fan, Fabio Estevam
I don't see support for vin-supply. I have made comments below showing
the (relatively-minimal IMO) changes necessary to support it, along
with some additional comments.
>
> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu at linaro.org>
> ---
> v2: add functions for set selector and delay. Define ANATOP_REGULATOR_STEP
> ---
> drivers/power/regulator/Kconfig | 10 +
> drivers/power/regulator/Makefile | 1 +
> drivers/power/regulator/anatop_regulator.c | 299 +++++++++++++++++++++
> 3 files changed, 310 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..d824f4acc4
> --- /dev/null
> +++ b/drivers/power/regulator/anatop_regulator.c
> @@ -0,0 +1,299 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> +// Copyright (C) 2021 Linaro
Use C-style comments (/* */) for everything other than the initial SPDX
line.
> +
> +#include <common.h>
> +#include <errno.h>
> +#include <dm.h>
> +#include <log.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
These should be below with the other linux headers.
> +#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>
These should be above the linux headers.
> +
> +#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
> +
> +#define BIT_WIDTH_MAX 32
> +
> +#define ANATOP_REGULATOR_STEP 25000
> +
> +struct anatop_regulator {
> + const char *name;
struct udevice *supply;
> + 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__);
Use dev_dbg, per [1]. This applies to most other logs in this file as
well.
> + return -ENODEV;
Use ENOENT, per [1].
[1] https://lists.denx.de/pipermail/u-boot/2021-March/445207.html
> + }
> +
> + 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);
> + }
This should be done in probe().
> +
> + if (bit_width == BIT_WIDTH_MAX)
> + 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;
> +}
> +
> +static int 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 -ENODEV;
> + }
> +
> + 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);
> + }
Again, should be done in probe.
> +
> + 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 err;
> + }
> + 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 err;
> + }
> +
> + return 0;
> +}
> +
> +static u32 anatop_get_selector(struct udevice *dev,
> + const struct anatop_regulator *anatop_reg)
> +{
> + u32 val = anatop_get_bits(dev,
> + anatop_reg->control_reg,
> + anatop_reg->vol_bit_shift,
> + anatop_reg->vol_bit_width);
> +
> + val = val - anatop_reg->min_bit_val;
> +
> + return val;
> +}
> +
> +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;
This does not need to be set to 0.
> + 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,
> + ANATOP_REGULATOR_STEP);
> + if (sel * ANATOP_REGULATOR_STEP + 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);
if (anatop_reg->supply) {
ret = regulator_set_value(anatop_reg->supply, uV + 150000);
if (ret)
return ret;
}
> +
> + /* check whether need to care about LDO ramp up speed */
Can this be handled by setting dm_regulator_uclass_platdata.ramp_delay?
> + 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_selector(dev, anatop_reg);
> +
> + 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 sel;
> +
> + sel = anatop_get_selector(dev, anatop_reg);
> +
> + return sel * ANATOP_REGULATOR_STEP + 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;
Please use consistent names for this structure.
> + int ret = 0;
> +
> + sreg = dev_get_plat(dev);
> + if (!sreg) {
> + dev_err(dev, "failed to get plat data\n");
The driver model checks for this. Do not check it again.
> + 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 = device_get_supply_regulator(dev, "vin-supply", &sreg->supply)
if (ret)
return ret;
ret = regulator_set_enable(sreg->supply, true);
if (ret)
return ret;
> +
> + ret = ofnode_read_u32(dev_ofnode(dev),
use dev_read_u32.
> + "anatop-reg-offset",
> + &sreg->control_reg);
> + if (ret) {
> + dev_err(dev, "no anatop-reg-offset property set\n");
> + return ret;
> + }
Perhaps consider something like
ret = dev_read_u32(dev, "some-prop", &sreg->some_prop);
if (ret)
return log_msg_ret("some-prop", 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,
> +};
>
More information about the U-Boot
mailing list