On 3/26/21 5:32 AM, Sean Anderson wrote: > > > On 3/25/21 2:44 PM, Ying-Chun Liu wrote: >> From: "Ying-Chun Liu (PaulLiu)" <paul....@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....@linaro.org> >> Cc: Fabio Estevam <fabio.este...@nxp.com> >> Cc: Jaehoon Chung <jh80.ch...@samsung.com> >> Cc: Peng Fan <peng....@nxp.com> >> Cc: Sean Anderson <sean.ander...@seco.com> >> --- >> v2: add functions for set selector and delay. Define ANATOP_REGULATOR_STEP >> v3: add vin-supply. move regmap retrival to probe >> --- >> drivers/power/regulator/Kconfig | 10 + >> drivers/power/regulator/Makefile | 1 + >> drivers/power/regulator/anatop_regulator.c | 287 +++++++++++++++++++++ >> 3 files changed, 298 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 > > nit: LDO > >> + 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..af5f9d2a2b >> --- /dev/null >> +++ b/drivers/power/regulator/anatop_regulator.c >> @@ -0,0 +1,287 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved. >> + * Copyright (C) 2021 Linaro >> + */ >> + >> +#include <common.h> >> +#include <dm.h> >> +#include <errno.h> >> +#include <log.h> >> +#include <regmap.h> >> +#include <syscon.h> >> +#include <dm/device-internal.h> >> +#include <dm/device_compat.h> >> +#include <linux/bitops.h> >> +#include <linux/delay.h> >> +#include <linux/err.h> >> +#include <linux/ioport.h> >> +#include <power/pmic.h> >> +#include <power/regulator.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 >> + >> +#define BIT_WIDTH_MAX 32 >> + >> +#define ANATOP_REGULATOR_STEP 25000 >> +#define MIN_DROPOUT_UV 125000 >> + >> +struct anatop_regulator { >> + const char *name; >> + struct regmap *regmap; >> + 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) >> +{ >> + const struct anatop_regulator *anatop_reg = dev_get_plat(dev); >> + struct regmap *regmap; >> + int err; >> + u32 val, mask; >> + >> + regmap = anatop_reg->regmap; >> + >> + if (bit_width == BIT_WIDTH_MAX) >> + mask = ~0; >> + else >> + mask = (1 << bit_width) - 1; >> + >> + err = regmap_read(regmap, addr, &val); > > Just use anatop_reg->regmap here. > >> + 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) >> +{ >> + const struct anatop_regulator *anatop_reg = dev_get_plat(dev); >> + struct regmap *regmap; >> + int err; >> + u32 val, mask; >> + >> + regmap = anatop_reg->regmap; >> + >> + if (bit_width == 32) >> + mask = ~0; >> + else >> + mask = (1 << bit_width) - 1; >> + >> + err = regmap_read(regmap, addr, &val); >> + if (err) { >> + dev_dbg(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_dbg(dev, >> + "%s: cannot wrie reg (%d)\n", __func__, > > nit: write > > You do not need to add the function name, since you can enable > CONFIG_LOGF_FUNC instead. This goes for the rest of your debug messages > as well. > >> + 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 ret; >> + >> + 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; > > Use -ENOSYS to match what regulator_set_value returns.
If prevent to access to wrong register, also need to check other location where it's used. Best Regards, Jaehoon Chung > >> + >> + 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 + MIN_DROPOUT_UV); >> + if (ret) >> + return ret; >> + } >> + >> + ret = anatop_set_bits(dev, >> + anatop_reg->control_reg, >> + anatop_reg->vol_bit_shift, >> + anatop_reg->vol_bit_width, >> + val); >> + >> + 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 *anatop_reg; >> + struct dm_regulator_uclass_plat *uc_pdata; >> + struct udevice *syscon; >> + int ret = 0; >> + u32 val; >> + >> + anatop_reg = dev_get_plat(dev); >> + uc_pdata = dev_get_uclass_plat(dev); >> + >> + anatop_reg->name = ofnode_read_string(dev_ofnode(dev), >> + "regulator-name"); >> + if (!anatop_reg->name) >> + return log_msg_ret("regulator-name", -EINVAL); >> + >> + ret = device_get_supply_regulator(dev, "vin-supply", >> + &anatop_reg->supply); >> + if (!ret) { > > perhaps > > if (ret != -ENODEV) { > if (ret) > return log_msg_ret("get vin-supply", ret); > ret = regulator_set_enable(...); > /* etc */ > } > > It would be nice to find out if the supply exists but could not be > probed (since that is likely a programmer error) > >> + ret = regulator_set_enable(anatop_reg->supply, true); >> + if (ret) >> + return ret; >> + } >> + >> + ret = dev_read_u32(dev, >> + "anatop-reg-offset", >> + &anatop_reg->control_reg); >> + if (ret) >> + return log_msg_ret("anatop-reg-offset", ret); >> + >> + ret = dev_read_u32(dev, >> + "anatop-vol-bit-width", >> + &anatop_reg->vol_bit_width); >> + if (ret) >> + return log_msg_ret("anatop-vol-bit-width", ret); >> + >> + ret = dev_read_u32(dev, >> + "anatop-vol-bit-shift", >> + &anatop_reg->vol_bit_shift); >> + if (ret) >> + return log_msg_ret("anatop-vol-bit-shift", ret); >> + >> + ret = dev_read_u32(dev, >> + "anatop-min-bit-val", >> + &anatop_reg->min_bit_val); >> + if (ret) >> + return log_msg_ret("anatop-min-bit-val", ret); >> + >> + ret = dev_read_u32(dev, >> + "anatop-min-voltage", >> + &anatop_reg->min_voltage); >> + if (ret) >> + return log_msg_ret("anatop-min-voltage", ret); >> + >> + ret = dev_read_u32(dev, >> + "anatop-max-voltage", >> + &anatop_reg->max_voltage); >> + if (ret) >> + return log_msg_ret("anatop-max-voltage", ret); >> + >> + /* read LDO ramp up setting, only for core reg */ >> + dev_read_u32(dev, "anatop-delay-reg-offset", >> + &anatop_reg->delay_reg); >> + dev_read_u32(dev, "anatop-delay-bit-width", >> + &anatop_reg->delay_bit_width); >> + dev_read_u32(dev, "anatop-delay-bit-shift", >> + &anatop_reg->delay_bit_shift); >> + >> + syscon = dev_get_parent(dev); >> + if (!syscon) { >> + dev_dbg(dev, "%s: unable to find syscon device\n", __func__); >> + return -ENOENT; >> + } >> + >> + anatop_reg->regmap = syscon_get_regmap(syscon); >> + if (IS_ERR(anatop_reg->regmap)) { >> + dev_dbg(dev, "%s: unable to find regmap (%ld)\n", __func__, >> + PTR_ERR(anatop_reg->regmap)); >> + return -ENOENT; >> + } >> + >> + /* 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) >> + */ >> + val = anatop_get_bits(dev, >> + anatop_reg->delay_reg, >> + anatop_reg->delay_bit_shift, >> + anatop_reg->delay_bit_width); >> + uc_pdata->ramp_delay = (LDO_RAMP_UP_UNIT_IN_CYCLES << val) >> + / LDO_RAMP_UP_FREQ_IN_MHZ + 1; >> + } >> + >> + 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, >> +}; >> >