* [U-Boot] [RFC PATCH v1 1/2] copy the bd71837 pmic driver from NXP imx u-boot
@ 2019-03-27 12:39 Matti Vaittinen
2019-03-27 12:40 ` [U-Boot] [RFC PATCH v1 2/2] power: regulator: support ROHM BD71837 PMIC Matti Vaittinen
0 siblings, 1 reply; 6+ messages in thread
From: Matti Vaittinen @ 2019-03-27 12:39 UTC (permalink / raw)
To: u-boot
Add initial support for ROHM BD71837 PMIC
https://source.codeaurora.org/external/imx/uboot-imx
cherry picked, styled and merged commits:
- MLK-18387 pmic: Add pmic driver for BD71837: e9a3bec2e95a
- MLK-18590 pmic: bd71837: Change to use new fdt API: acdc5c297a96
Signed-off-by: Ye Li <ye.li@nxp.com>
Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
drivers/power/pmic/Kconfig | 7 +++
drivers/power/pmic/Makefile | 2 +
drivers/power/pmic/bd71837.c | 89 +++++++++++++++++++++++++++++++
drivers/power/pmic/pmic_bd71837.c | 31 +++++++++++
include/power/bd71837.h | 64 ++++++++++++++++++++++
5 files changed, 193 insertions(+)
diff --git a/drivers/power/pmic/Kconfig b/drivers/power/pmic/Kconfig
index 8cf60ebcf3..e154d0a57b 100644
--- a/drivers/power/pmic/Kconfig
+++ b/drivers/power/pmic/Kconfig
@@ -48,6 +48,13 @@ config PMIC_AS3722
interface and is designs to cover most of the power managementment
required for a tablets or laptop.
+config DM_PMIC_BD71837
+ bool "Enable Driver Model for PMIC BD71837"
+ depends on DM_PMIC
+ help
+ This config enables implementation of driver-model pmic uclass features
+ for PMIC BD71837. The driver implements read/write operations.
+
config DM_PMIC_FAN53555
bool "Enable support for OnSemi FAN53555"
depends on DM_PMIC && DM_REGULATOR && DM_I2C
diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile
index 637352ab2b..e74c6190a8 100644
--- a/drivers/power/pmic/Makefile
+++ b/drivers/power/pmic/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_DM_PMIC_FAN53555) += fan53555.o
obj-$(CONFIG_DM_PMIC_MAX77686) += max77686.o
obj-$(CONFIG_DM_PMIC_MAX8998) += max8998.o
obj-$(CONFIG_DM_PMIC_MC34708) += mc34708.o
+obj-$(CONFIG_$(SPL_)DM_PMIC_BD71837) += bd71837.o
obj-$(CONFIG_$(SPL_)DM_PMIC_PFUZE100) += pfuze100.o
obj-$(CONFIG_PMIC_S2MPS11) += s2mps11.o
obj-$(CONFIG_DM_PMIC_SANDBOX) += sandbox.o i2c_pmic_emul.o
@@ -30,6 +31,7 @@ obj-$(CONFIG_POWER_MAX77696) += pmic_max77696.o
obj-$(CONFIG_POWER_MAX8998) += pmic_max8998.o
obj-$(CONFIG_POWER_MAX8997) += pmic_max8997.o
obj-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o
+obj-$(CONFIG_POWER_BD71837) += pmic_bd71837.o
obj-$(CONFIG_POWER_PFUZE100) += pmic_pfuze100.o
obj-$(CONFIG_POWER_PFUZE3000) += pmic_pfuze3000.o
obj-$(CONFIG_POWER_TPS65217) += pmic_tps65217.o
diff --git a/drivers/power/pmic/bd71837.c b/drivers/power/pmic/bd71837.c
new file mode 100644
index 0000000000..eadf373a18
--- /dev/null
+++ b/drivers/power/pmic/bd71837.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Copyright 2018 NXP *
+
+#include <common.h>
+#include <fdtdec.h>
+#include <errno.h>
+#include <dm.h>
+#include <i2c.h>
+#include <power/pmic.h>
+#include <power/regulator.h>
+#include <power/bd71837.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static const struct pmic_child_info pmic_children_info[] = {
+ /* buck */
+ { .prefix = "b", .driver = BD71837_REGULATOR_DRIVER},
+ /* ldo */
+ { .prefix = "l", .driver = BD71837_REGULATOR_DRIVER},
+ { },
+};
+
+static int bd71837_reg_count(struct udevice *dev)
+{
+ return BD71837_REG_NUM;
+}
+
+static int bd71837_write(struct udevice *dev, uint reg, const uint8_t *buff,
+ int len)
+{
+ if (dm_i2c_write(dev, reg, buff, len)) {
+ pr_err("write error to device: %p register: %#x!", dev, reg);
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int bd71837_read(struct udevice *dev, uint reg, uint8_t *buff, int len)
+{
+ if (dm_i2c_read(dev, reg, buff, len)) {
+ pr_err("read error from device: %p register: %#x!", dev, reg);
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int bd71837_bind(struct udevice *dev)
+{
+ int children;
+ ofnode regulators_node;
+
+ regulators_node = dev_read_subnode(dev, "regulators");
+ if (!ofnode_valid(regulators_node)) {
+ debug("%s: %s regulators subnode not found!", __func__,
+ dev->name);
+ return -ENXIO;
+ }
+
+ debug("%s: '%s' - found regulators subnode\n", __func__, dev->name);
+
+ children = pmic_bind_children(dev, regulators_node, pmic_children_info);
+ if (!children)
+ debug("%s: %s - no child found\n", __func__, dev->name);
+
+ /* Always return success for this device */
+ return 0;
+}
+
+static struct dm_pmic_ops bd71837_ops = {
+ .reg_count = bd71837_reg_count,
+ .read = bd71837_read,
+ .write = bd71837_write,
+};
+
+static const struct udevice_id bd71837_ids[] = {
+ { .compatible = "rohm,bd71837", .data = 0x4b, },
+ { }
+};
+
+U_BOOT_DRIVER(pmic_bd71837) = {
+ .name = "bd71837 pmic",
+ .id = UCLASS_PMIC,
+ .of_match = bd71837_ids,
+ .bind = bd71837_bind,
+ .ops = &bd71837_ops,
+};
diff --git a/drivers/power/pmic/pmic_bd71837.c b/drivers/power/pmic/pmic_bd71837.c
new file mode 100644
index 0000000000..3bb8db4081
--- /dev/null
+++ b/drivers/power/pmic/pmic_bd71837.c
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Copyright (C) 2014 Gateworks Corporation
+//
+// Tim Harvey <tharvey@gateworks.com>
+
+#include <common.h>
+#include <errno.h>
+#include <i2c.h>
+#include <power/pmic.h>
+#include <power/bd71837.h>
+
+static const char bd71837_name[] = "BD71837";
+int power_bd71837_init(unsigned char bus)
+{
+ struct pmic *p = pmic_alloc();
+
+ if (!p) {
+ printf("%s: POWER allocation error!\n", __func__);
+ return -ENOMEM;
+ }
+
+ p->name = bd71837_name;
+ p->interface = PMIC_I2C;
+ p->number_of_regs = BD71837_REG_NUM;
+ p->hw.i2c.addr = 0x4b;
+ p->hw.i2c.tx_num = 1;
+ p->bus = bus;
+
+ return 0;
+}
diff --git a/include/power/bd71837.h b/include/power/bd71837.h
new file mode 100644
index 0000000000..9c74f6fc61
--- /dev/null
+++ b/include/power/bd71837.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright (C) 2018 ROHM Semiconductors */
+
+#ifndef BD71837_H_
+#define BD71837_H_
+
+#define BD71837_REGULATOR_DRIVER "bd71837_regulator"
+
+enum {
+ BD71837_REV = 0x00,
+ BD71837_SWRESET = 0x01,
+ BD71837_I2C_DEV = 0x02,
+ BD71837_PWRCTRL0 = 0x03,
+ BD71837_PWRCTRL1 = 0x04,
+ BD71837_BUCK1_CTRL = 0x05,
+ BD71837_BUCK2_CTRL = 0x06,
+ BD71837_BUCK3_CTRL = 0x07,
+ BD71837_BUCK4_CTRL = 0x08,
+ BD71837_BUCK5_CTRL = 0x09,
+ BD71837_BUCK6_CTRL = 0x0A,
+ BD71837_BUCK7_CTRL = 0x0B,
+ BD71837_BUCK8_CTRL = 0x0C,
+ BD71837_BUCK1_VOLT_RUN = 0x0D,
+ BD71837_BUCK1_VOLT_IDLE = 0x0E,
+ BD71837_BUCK1_VOLT_SUSP = 0x0F,
+ BD71837_BUCK2_VOLT_RUN = 0x10,
+ BD71837_BUCK2_VOLT_IDLE = 0x11,
+ BD71837_BUCK3_VOLT_RUN = 0x12,
+ BD71837_BUCK4_VOLT_RUN = 0x13,
+ BD71837_BUCK5_VOLT = 0x14,
+ BD71837_BUCK6_VOLT = 0x15,
+ BD71837_BUCK7_VOLT = 0x16,
+ BD71837_BUCK8_VOLT = 0x17,
+ BD71837_LDO1_VOLT = 0x18,
+ BD71837_LDO2_VOLT = 0x19,
+ BD71837_LDO3_VOLT = 0x1A,
+ BD71837_LDO4_VOLT = 0x1B,
+ BD71837_LDO5_VOLT = 0x1C,
+ BD71837_LDO6_VOLT = 0x1D,
+ BD71837_LDO7_VOLT = 0x1E,
+ BD71837_TRANS_COND0 = 0x1F,
+ BD71837_TRANS_COND1 = 0x20,
+ BD71837_VRFAULTEN = 0x21,
+ BD71837_MVRFLTMASK0 = 0x22,
+ BD71837_MVRFLTMASK1 = 0x23,
+ BD71837_MVRFLTMASK2 = 0x24,
+ BD71837_RCVCFG = 0x25,
+ BD71837_RCVNUM = 0x26,
+ BD71837_PWRONCONFIG0 = 0x27,
+ BD71837_PWRONCONFIG1 = 0x28,
+ BD71837_RESETSRC = 0x29,
+ BD71837_MIRQ = 0x2A,
+ BD71837_IRQ = 0x2B,
+ BD71837_IN_MON = 0x2C,
+ BD71837_POW_STATE = 0x2D,
+ BD71837_OUT32K = 0x2E,
+ BD71837_REGLOCK = 0x2F,
+ BD71837_MUXSW_EN = 0x30,
+ BD71837_REG_NUM,
+};
+
+int power_bd71837_init(unsigned char bus);
+
+#endif
--
2.17.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [RFC PATCH v1 2/2] power: regulator: support ROHM BD71837 PMIC
2019-03-27 12:39 [U-Boot] [RFC PATCH v1 1/2] copy the bd71837 pmic driver from NXP imx u-boot Matti Vaittinen
@ 2019-03-27 12:40 ` Matti Vaittinen
2019-04-04 7:03 ` Matti Vaittinen
2019-04-24 3:54 ` Simon Glass
0 siblings, 2 replies; 6+ messages in thread
From: Matti Vaittinen @ 2019-03-27 12:40 UTC (permalink / raw)
To: u-boot
Add regulator driver for ROHM BD71837 PMIC. BD71837 contains
8 bucks and 7 LDOS. Voltages for bucks 1-4 can be adjusted
when regulators are enabled. For other bucks and LDOs we may
have over- or undershooting if voltage is adjusted when
regulator is enabled. Thus this is prevented by default.
BD71837 has a quirk which may leave power output disabled
after reset if enable/disable state was controlled by SW.
Thus the SW control is only allowed for bucks3 and 4 by
default.
Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
drivers/power/regulator/Kconfig | 15 ++
drivers/power/regulator/Makefile | 1 +
drivers/power/regulator/bd71837.c | 373 ++++++++++++++++++++++++++++++
include/power/bd71837.h | 20 ++
4 files changed, 409 insertions(+)
diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
index 3ed0dd2264..323516587c 100644
--- a/drivers/power/regulator/Kconfig
+++ b/drivers/power/regulator/Kconfig
@@ -43,6 +43,21 @@ config REGULATOR_AS3722
but does not yet support change voltages. Currently this must be
done using direct register writes to the PMIC.
+config DM_REGULATOR_BD71837
+ bool "Enable Driver Model for REGULATOR BD71837"
+ depends on DM_REGULATOR && DM_PMIC_BD71837
+ help
+ This config enables implementation of driver-model regulator uclass
+ features for REGULATOR BD71837. The driver implements get/set api for:
+ value and enable.
+
+config SPL_DM_REGULATOR_BD71837
+ bool "Enable Driver Model for REGULATOR BD71837 in SPL"
+ depends on DM_REGULATOR_BD71837
+ help
+ This config enables implementation of driver-model regulator uclass
+ features for REGULATOR BD71837 in SPL.
+
config DM_REGULATOR_PFUZE100
bool "Enable Driver Model for REGULATOR PFUZE100"
depends on DM_REGULATOR && DM_PMIC_PFUZE100
diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
index f617ce723a..898ed5f084 100644
--- a/drivers/power/regulator/Makefile
+++ b/drivers/power/regulator/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_REGULATOR_ACT8846) += act8846.o
obj-$(CONFIG_REGULATOR_AS3722) += as3722_regulator.o
obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o
obj-$(CONFIG_$(SPL_)DM_PMIC_PFUZE100) += pfuze100.o
+obj-$(CONFIG_$(SPL_)DM_REGULATOR_BD71837) += bd71837.o
obj-$(CONFIG_$(SPL_)REGULATOR_PWM) += pwm_regulator.o
obj-$(CONFIG_$(SPL_)DM_REGULATOR_FAN53555) += fan53555.o
obj-$(CONFIG_$(SPL_)DM_REGULATOR_FIXED) += fixed.o
diff --git a/drivers/power/regulator/bd71837.c b/drivers/power/regulator/bd71837.c
new file mode 100644
index 0000000000..5b32425ba9
--- /dev/null
+++ b/drivers/power/regulator/bd71837.c
@@ -0,0 +1,373 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+//
+// Copyright (C) 2019 ROHM Semiconductors
+//
+// ROHM BD71837 regulator driver
+
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <i2c.h>
+#include <power/pmic.h>
+#include <power/regulator.h>
+#include <power/bd71837.h>
+
+#define HW_STATE_CONTROL 0
+
+struct bd71837_vrange {
+ unsigned int min_volt;
+ unsigned int step;
+ u8 min_sel;
+ u8 max_sel;
+ u8 rangeval;
+};
+
+struct bd71837_data {
+ const char *name;
+ u8 enable_reg;
+ u8 enablemask;
+ u8 volt_reg;
+ u8 volt_mask;
+ struct bd71837_vrange *ranges;
+ unsigned int numranges;
+ u8 rangemask;
+ u8 sel_mask;
+ bool dvs;
+};
+
+#define BD_RANGE(_min, _vstep, _sel_low, _sel_hi, _range_sel) \
+{ \
+ .min_volt = (_min), .step = (_vstep), .min_sel = (_sel_low), \
+ .max_sel = (_sel_hi), .rangeval = (_range_sel) \
+}
+
+#define BD_DATA(_name, enreg, enmask, vreg, vmask, _range, rmask, _dvs, sel) \
+{ \
+ .name = (_name), .enable_reg = (enreg), .enablemask = (enmask), \
+ .volt_reg = (vreg), .volt_mask = (vmask), .ranges = (_range), \
+ .numranges = ARRAY_SIZE(_range), .rangemask = (rmask), .dvs = (_dvs), \
+ .sel_mask = (sel) \
+}
+
+static struct bd71837_vrange buck1to4_vranges[] = {
+ BD_RANGE(700000, 10000, 0, 0x3C, 0),
+ BD_RANGE(1300000, 0, 0x3D, 0x3F, 0),
+};
+
+static struct bd71837_vrange buck5_vranges[] = {
+ BD_RANGE(700000, 100000, 0, 0x3, 0),
+ BD_RANGE(1050000, 50000, 0x04, 0x05, 0),
+ BD_RANGE(1200000, 150000, 0x06, 0x07, 0),
+ BD_RANGE(675000, 100000, 0x0, 0x3, 0x80),
+ BD_RANGE(1025000, 50000, 0x04, 0x05, 0x80),
+ BD_RANGE(1175000, 150000, 0x06, 0x07, 0x80),
+};
+
+static struct bd71837_vrange buck6_vranges[] = {
+ BD_RANGE(3000000, 100000, 0x00, 0x03, 0),
+};
+
+static struct bd71837_vrange buck7_vranges[] = {
+ BD_RANGE(1605000, 90000, 0, 1, 0),
+ BD_RANGE(1755000, 45000, 2, 4, 0),
+ BD_RANGE(1905000, 45000, 5, 7, 0),
+};
+
+static struct bd71837_vrange buck8_vranges[] = {
+ BD_RANGE(800000, 10000, 0x00, 0x3C, 0),
+};
+
+static struct bd71837_vrange ldo1_vranges[] = {
+ BD_RANGE(3000000, 100000, 0x00, 0x03, 0),
+ BD_RANGE(1600000, 100000, 0x00, 0x03, 0x20),
+};
+
+static struct bd71837_vrange ldo2_vranges[] = {
+ BD_RANGE(900000, 0, 0, 0, 0),
+ BD_RANGE(800000, 0, 1, 1, 0),
+};
+
+static struct bd71837_vrange ldo3_vranges[] = {
+ BD_RANGE(1800000, 100000, 0x00, 0x0F, 0),
+};
+
+static struct bd71837_vrange ldo4_vranges[] = {
+ BD_RANGE(900000, 100000, 0x00, 0x09, 0),
+};
+
+static struct bd71837_vrange ldo5_vranges[] = {
+ BD_RANGE(1800000, 100000, 0x00, 0x0F, 0),
+};
+
+static struct bd71837_vrange ldo6_vranges[] = {
+ BD_RANGE(900000, 100000, 0x00, 0x09, 0),
+};
+
+static struct bd71837_vrange ldo7_vranges[] = {
+ BD_RANGE(1800000, 100000, 0x00, 0x0F, 0),
+};
+
+/* We use enable mask 'HW_STATE_CONTROL' to indicate that this regulator
+ * must not be enabled or disabled by SW. The typical use-case for BD71837
+ * is powering NXP i.MX8. In this use-case we (for now) only allow control
+ * for BUCK3 and BUCK4 which are not boot critical.
+ */
+static struct bd71837_data bd71837_reg_data[] = {
+ BD_DATA("BUCK1", BD71837_BUCK1_CTRL, HW_STATE_CONTROL,
+ BD71837_BUCK1_VOLT_RUN, DVS_BUCK_RUN_MASK, buck1to4_vranges, 0,
+ true, BD71837_BUCK_SEL),
+ BD_DATA("BUCK2", BD71837_BUCK2_CTRL, HW_STATE_CONTROL,
+ BD71837_BUCK2_VOLT_RUN, DVS_BUCK_RUN_MASK, buck1to4_vranges, 0,
+ true, BD71837_BUCK_SEL),
+ BD_DATA("BUCK3", BD71837_BUCK3_CTRL, BD71837_BUCK_EN,
+ BD71837_BUCK3_VOLT_RUN, DVS_BUCK_RUN_MASK, buck1to4_vranges, 0,
+ true, BD71837_BUCK_SEL),
+ BD_DATA("BUCK4", BD71837_BUCK4_CTRL, BD71837_BUCK_EN,
+ BD71837_BUCK4_VOLT_RUN, DVS_BUCK_RUN_MASK, buck1to4_vranges, 0,
+ true, BD71837_BUCK_SEL),
+ BD_DATA("BUCK5", BD71837_BUCK5_CTRL, HW_STATE_CONTROL,
+ BD71837_BUCK5_VOLT, BD71837_BUCK5_MASK, buck5_vranges, 0x80,
+ false, BD71837_BUCK_SEL),
+ BD_DATA("BUCK6", BD71837_BUCK6_CTRL, HW_STATE_CONTROL,
+ BD71837_BUCK6_VOLT, BD71837_BUCK6_MASK, buck6_vranges, 0,
+ false, BD71837_BUCK_SEL),
+ BD_DATA("BUCK7", BD71837_BUCK7_CTRL, HW_STATE_CONTROL,
+ BD71837_BUCK7_VOLT, BD71837_BUCK7_MASK, buck7_vranges, 0,
+ false, BD71837_BUCK_SEL),
+ BD_DATA("BUCK8", BD71837_BUCK8_CTRL, HW_STATE_CONTROL,
+ BD71837_BUCK8_VOLT, BD71837_BUCK8_MASK, buck8_vranges, 0,
+ false, BD71837_BUCK_SEL),
+ BD_DATA("LDO1", BD71837_LDO1_VOLT, HW_STATE_CONTROL, BD71837_LDO1_VOLT,
+ BD71837_LDO1_MASK, ldo1_vranges, 0x20, false, BD71837_LDO_SEL),
+ BD_DATA("LDO2", BD71837_LDO2_VOLT, HW_STATE_CONTROL, BD71837_LDO2_VOLT,
+ BD71837_LDO2_MASK, ldo2_vranges, 0, false, BD71837_LDO_SEL),
+ BD_DATA("LDO3", BD71837_LDO3_VOLT, HW_STATE_CONTROL, BD71837_LDO3_VOLT,
+ BD71837_LDO3_MASK, ldo3_vranges, 0, false, BD71837_LDO_SEL),
+ BD_DATA("LDO4", BD71837_LDO4_VOLT, HW_STATE_CONTROL, BD71837_LDO4_VOLT,
+ BD71837_LDO4_MASK, ldo4_vranges, 0, false, BD71837_LDO_SEL),
+ BD_DATA("LDO5", BD71837_LDO5_VOLT, HW_STATE_CONTROL, BD71837_LDO5_VOLT,
+ BD71837_LDO5_MASK, ldo5_vranges, 0, false, BD71837_LDO_SEL),
+ BD_DATA("LDO6", BD71837_LDO6_VOLT, HW_STATE_CONTROL, BD71837_LDO6_VOLT,
+ BD71837_LDO6_MASK, ldo6_vranges, 0, false, BD71837_LDO_SEL),
+ BD_DATA("LDO7", BD71837_LDO7_VOLT, HW_STATE_CONTROL, BD71837_LDO7_VOLT,
+ BD71837_LDO7_MASK, ldo7_vranges, 0, false, BD71837_LDO_SEL),
+};
+
+static int vrange_find_value(struct bd71837_vrange *r, u8 sel,
+ unsigned int *val)
+{
+ if (!val || sel < r->min_sel || sel > r->max_sel)
+ return -EINVAL;
+
+ *val = r->min_volt + r->step * (sel - r->min_sel);
+ return 0;
+}
+
+static int vrange_find_selector(struct bd71837_vrange *r, int val, u8 *sel)
+{
+ int ret = -EINVAL;
+ int num_vals = r->max_sel - r->min_sel + 1;
+
+ if (val >= r->min_volt &&
+ val <= r->min_volt + r->step * (num_vals - 1)) {
+ if (r->step) {
+ *sel = r->min_sel + ((val - r->min_volt) / r->step);
+ ret = 0;
+ } else {
+ *sel = r->min_sel;
+ ret = 0;
+ }
+ }
+ return ret;
+}
+
+static int bd71837_get_enable(struct udevice *dev)
+{
+ int val;
+ struct bd71837_data *d = dev_get_platdata(dev);
+
+ /* boot critical regulators on bd71837 must not be controlled by sw
+ * due to the 'feature' which leaves power rails down if bd71837 is
+ * reseted to snvs state. hence we can't get the state here.
+ *
+ * if we are alive it means we probably are on run state and
+ * if the regulator can't be controlled we can assume it is
+ * enabled.
+ */
+ if (d->enablemask == HW_STATE_CONTROL)
+ return 1;
+
+ val = pmic_reg_read(dev->parent, d->enable_reg);
+
+ if (val < 0)
+ return val;
+
+ return (val & d->enablemask);
+}
+
+static int bd71837_set_enable(struct udevice *dev, bool enable)
+{
+ int val;
+ struct bd71837_data *d = dev_get_platdata(dev);
+
+ /* boot critical regulators on bd71837 must not be controlled by sw
+ * due to the 'feature' which leaves power rails down if bd71837 is
+ * reseted to snvs state. Hence we can't set the state here.
+ */
+ if (d->enablemask == HW_STATE_CONTROL)
+ return -EINVAL;
+
+ val = pmic_reg_read(dev->parent, d->enable_reg);
+
+ if (val < 0)
+ return val;
+
+ if (enable)
+ val |= d->enablemask;
+ else
+ val &= ~d->enablemask;
+
+ return pmic_reg_write(dev->parent, d->enable_reg, val);
+}
+
+static int bd71837_set_value(struct udevice *dev, int uvolt)
+{
+ u8 sel, reg;
+ u8 range;
+ int i;
+ int not_found = 1;
+ struct bd71837_data *d = dev_get_platdata(dev);
+
+ /* An under/overshooting may occur if voltage is changed for other
+ * regulators but buck 1,2,3 or 4 when regulator is enabled. Prevent
+ * change to protect the HW
+ */
+ if (!d->dvs)
+ if (bd71837_get_enable(dev)) {
+ pr_err("Only buck1-4 can be changed when enabled\n");
+ return -EINVAL;
+ }
+
+ for (i = 0; i < d->numranges; i++) {
+ struct bd71837_vrange *r = &d->ranges[i];
+
+ not_found = vrange_find_selector(r, uvolt, &sel);
+ if (!not_found) {
+ unsigned int tmp;
+
+ /* We require exactly the requested value to be
+ * supported - this can be changed later if needed
+ */
+ range = r->rangeval;
+ not_found = vrange_find_value(r, sel, &tmp);
+ if (!not_found && tmp == uvolt)
+ break;
+ not_found = 1;
+ }
+ }
+
+ if (not_found)
+ return -EINVAL;
+
+ sel <<= ffs(d->volt_mask) - 1;
+
+ reg = pmic_reg_read(dev->parent, d->volt_reg);
+ if (reg < 0)
+ return reg;
+
+ reg &= ~d->volt_mask;
+ reg |= sel;
+ if (d->rangemask) {
+ reg &= ~d->rangemask;
+ reg |= range;
+ }
+
+ return pmic_reg_write(dev->parent, d->volt_reg, reg);
+}
+
+static int bd71837_get_value(struct udevice *dev)
+{
+ u8 reg, range;
+ unsigned int tmp;
+ struct bd71837_data *d = dev_get_platdata(dev);
+ int i;
+
+ reg = pmic_reg_read(dev->parent, d->volt_reg);
+ if (reg < 0)
+ return reg;
+
+ range = reg & d->rangemask;
+
+ reg &= d->volt_mask;
+ reg >>= ffs(d->volt_mask) - 1;
+
+ for (i = 0; i < d->numranges; i++) {
+ struct bd71837_vrange *r = &d->ranges[i];
+
+ if (d->rangemask && ((d->rangemask & range) != r->rangeval))
+ continue;
+
+ if (!vrange_find_value(r, reg, &tmp))
+ return tmp;
+ }
+
+ pr_err("Unknown voltage value read from pmic\n");
+
+ return -EINVAL;
+}
+
+static int bd71837_regulator_probe(struct udevice *dev)
+{
+ struct bd71837_data *d = dev_get_platdata(dev);
+ int i, ret;
+ struct dm_regulator_uclass_platdata *uc_pdata;
+
+ for (i = 0; i < ARRAY_SIZE(bd71837_reg_data); i++) {
+ if (!strcmp(dev->name, bd71837_reg_data[i].name)) {
+ *d = bd71837_reg_data[i];
+ if (d->enablemask != HW_STATE_CONTROL) {
+ u8 ctrl;
+
+ /* Take the regulator under SW control. Ensure
+ * the initial state matches dt flags and then
+ * write the SEL bit
+ */
+ uc_pdata = dev_get_uclass_platdata(dev);
+ ret = bd71837_set_enable(dev,
+ !!(uc_pdata->boot_on ||
+ uc_pdata->always_on));
+ if (ret)
+ return ret;
+
+ ctrl = pmic_reg_read(dev->parent,
+ d->enable_reg);
+ if (ctrl < 0)
+ return ctrl;
+
+ ctrl |= d->sel_mask;
+ return pmic_reg_write(dev->parent,
+ d->enable_reg, ctrl);
+ }
+ return 0;
+ }
+ }
+
+ pr_err("Unknown regulator '%s'\n", dev->name);
+
+ return -EINVAL;
+}
+
+static const struct dm_regulator_ops bd71837_regulator_ops = {
+ .get_value = bd71837_get_value,
+ .set_value = bd71837_set_value,
+ .get_enable = bd71837_get_enable,
+ .set_enable = bd71837_set_enable,
+};
+
+U_BOOT_DRIVER(bd71837_regulator) = {
+ .name = "bd71837_regulator",
+ .id = UCLASS_REGULATOR,
+ .ops = &bd71837_regulator_ops,
+ .probe = bd71837_regulator_probe,
+ .platdata_auto_alloc_size = sizeof(struct bd71837_data),
+};
diff --git a/include/power/bd71837.h b/include/power/bd71837.h
index 9c74f6fc61..da14dc9bb1 100644
--- a/include/power/bd71837.h
+++ b/include/power/bd71837.h
@@ -59,6 +59,26 @@ enum {
BD71837_REG_NUM,
};
+#define BD71837_BUCK_EN 0x01
+#define BD71837_LDO_EN 0x40
+#define BD71837_BUCK_SEL 0x02
+#define BD71837_LDO_SEL 0x80
+
+#define DVS_BUCK_RUN_MASK 0x3F
+#define BD71837_BUCK5_MASK 0x07
+#define BD71837_BUCK6_MASK 0x03
+#define BD71837_BUCK7_MASK 0x07
+#define BD71837_BUCK8_MASK 0x3F
+
+#define BD71837_LDO1_MASK 0x03
+#define BD71837_LDO1_RANGE_MASK 0x20
+#define BD71837_LDO2_MASK 0x20
+#define BD71837_LDO3_MASK 0x0F
+#define BD71837_LDO4_MASK 0x0F
+#define BD71837_LDO5_MASK 0x0F
+#define BD71837_LDO6_MASK 0x0F
+#define BD71837_LDO7_MASK 0x0F
+
int power_bd71837_init(unsigned char bus);
#endif
--
2.17.2
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [RFC PATCH v1 2/2] power: regulator: support ROHM BD71837 PMIC
2019-03-27 12:40 ` [U-Boot] [RFC PATCH v1 2/2] power: regulator: support ROHM BD71837 PMIC Matti Vaittinen
@ 2019-04-04 7:03 ` Matti Vaittinen
2019-04-24 3:54 ` Simon Glass
1 sibling, 0 replies; 6+ messages in thread
From: Matti Vaittinen @ 2019-04-04 7:03 UTC (permalink / raw)
To: u-boot
Hi de Ho Peeps,
On Wed, Mar 27, 2019 at 02:40:47PM +0200, Matti Vaittinen wrote:
> Add regulator driver for ROHM BD71837 PMIC. BD71837 contains
> 8 bucks and 7 LDOS. Voltages for bucks 1-4 can be adjusted
> when regulators are enabled. For other bucks and LDOs we may
> have over- or undershooting if voltage is adjusted when
> regulator is enabled. Thus this is prevented by default.
>
> BD71837 has a quirk which may leave power output disabled
> after reset if enable/disable state was controlled by SW.
> Thus the SW control is only allowed for bucks3 and 4 by
> default.
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> drivers/power/regulator/Kconfig | 15 ++
> drivers/power/regulator/Makefile | 1 +
> drivers/power/regulator/bd71837.c | 373 ++++++++++++++++++++++++++++++
> include/power/bd71837.h | 20 ++
> 4 files changed, 409 insertions(+)
>
This Patch:
This was my first patch to U-boot so I wonder if there is something to
improve. Is this Ok like this or should I have formatted it somehow
differently?
Also, is this type of submissions welcome? Any place to check if
submissions are rejected, being reviewed or forgotten?
Next Steps:
Finally, the BD71837 is mainly targeted for powering the i.MX8M. There's
another ROHM PMIC BD71847 - which is mainly used for powering the
i.MX8MM. The Linux driver I wrote does support both of these PMICs and I
was thinking that maybe I should add support for BD71847 in this u-Boot
driver too. But before investing on that work I would like to get some
feedback regarding the BD71837 u-boot driver. At least a sign that this
kind of submissions are welcome or information if I am doing something
completely wrong =)
About RFC tag:
I used RFC tag here mainly because I am unsure if the driver design fits
what the u-boot is heading on or if this is kind of driver u-Boot should
include. Is this correct use of RFC, and what are the consequences of
using RFC-tag? My assumption was that the patch with RFC is reviewed as
ither patches, but it is also a sign that the patch might have something
that makes it unsuitable for applying to u-Boot. Is this correct?
Finally, I guess I need to (re)submit the driver(s) without the RFC tag
at some point, any suggestions when?
Best Regards
Matti Vaittinen
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [RFC PATCH v1 2/2] power: regulator: support ROHM BD71837 PMIC
2019-03-27 12:40 ` [U-Boot] [RFC PATCH v1 2/2] power: regulator: support ROHM BD71837 PMIC Matti Vaittinen
2019-04-04 7:03 ` Matti Vaittinen
@ 2019-04-24 3:54 ` Simon Glass
2019-04-24 5:59 ` Vaittinen, Matti
1 sibling, 1 reply; 6+ messages in thread
From: Simon Glass @ 2019-04-24 3:54 UTC (permalink / raw)
To: u-boot
Hi Matti,
On Wed, 27 Mar 2019 at 06:40, Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:
>
> Add regulator driver for ROHM BD71837 PMIC. BD71837 contains
> 8 bucks and 7 LDOS. Voltages for bucks 1-4 can be adjusted
> when regulators are enabled. For other bucks and LDOs we may
> have over- or undershooting if voltage is adjusted when
> regulator is enabled. Thus this is prevented by default.
>
> BD71837 has a quirk which may leave power output disabled
> after reset if enable/disable state was controlled by SW.
> Thus the SW control is only allowed for bucks3 and 4 by
> default.
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> drivers/power/regulator/Kconfig | 15 ++
> drivers/power/regulator/Makefile | 1 +
> drivers/power/regulator/bd71837.c | 373 ++++++++++++++++++++++++++++++
> include/power/bd71837.h | 20 ++
> 4 files changed, 409 insertions(+)
>
Reviewed-by: Simon Glass <sjg@chromium.org>
But please see nits below.
> diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
> index 3ed0dd2264..323516587c 100644
> --- a/drivers/power/regulator/Kconfig
> +++ b/drivers/power/regulator/Kconfig
> @@ -43,6 +43,21 @@ config REGULATOR_AS3722
> but does not yet support change voltages. Currently this must be
> done using direct register writes to the PMIC.
>
> +config DM_REGULATOR_BD71837
> + bool "Enable Driver Model for REGULATOR BD71837"
> + depends on DM_REGULATOR && DM_PMIC_BD71837
> + help
> + This config enables implementation of driver-model regulator uclass
> + features for REGULATOR BD71837. The driver implements get/set api for:
> + value and enable.
> +
> +config SPL_DM_REGULATOR_BD71837
> + bool "Enable Driver Model for REGULATOR BD71837 in SPL"
> + depends on DM_REGULATOR_BD71837
> + help
> + This config enables implementation of driver-model regulator uclass
> + features for REGULATOR BD71837 in SPL.
> +
> config DM_REGULATOR_PFUZE100
> bool "Enable Driver Model for REGULATOR PFUZE100"
> depends on DM_REGULATOR && DM_PMIC_PFUZE100
> diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
> index f617ce723a..898ed5f084 100644
> --- a/drivers/power/regulator/Makefile
> +++ b/drivers/power/regulator/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_REGULATOR_ACT8846) += act8846.o
> obj-$(CONFIG_REGULATOR_AS3722) += as3722_regulator.o
> obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o
> obj-$(CONFIG_$(SPL_)DM_PMIC_PFUZE100) += pfuze100.o
> +obj-$(CONFIG_$(SPL_)DM_REGULATOR_BD71837) += bd71837.o
> obj-$(CONFIG_$(SPL_)REGULATOR_PWM) += pwm_regulator.o
> obj-$(CONFIG_$(SPL_)DM_REGULATOR_FAN53555) += fan53555.o
> obj-$(CONFIG_$(SPL_)DM_REGULATOR_FIXED) += fixed.o
> diff --git a/drivers/power/regulator/bd71837.c b/drivers/power/regulator/bd71837.c
> new file mode 100644
> index 0000000000..5b32425ba9
> --- /dev/null
> +++ b/drivers/power/regulator/bd71837.c
> @@ -0,0 +1,373 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +//
> +// Copyright (C) 2019 ROHM Semiconductors
> +//
> +// ROHM BD71837 regulator driver
The SPDX line has a // comment but everything else should use C comments:
/*
* Copyright ...
*
* ROHM ...
*/
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <i2c.h>
> +#include <power/pmic.h>
> +#include <power/regulator.h>
> +#include <power/bd71837.h>
> +
> +#define HW_STATE_CONTROL 0
> +
Need struct comment.
/**
* struct bd71837_vrange - description here
*
* @min_volt: Description here
* ...
*/
> +struct bd71837_vrange {
> + unsigned int min_volt;
> + unsigned int step;
> + u8 min_sel;
> + u8 max_sel;
> + u8 rangeval;
> +};
> +
Need struct comment
> +struct bd71837_data {
How about bd71837_platdata?
> + const char *name;
> + u8 enable_reg;
> + u8 enablemask;
> + u8 volt_reg;
> + u8 volt_mask;
> + struct bd71837_vrange *ranges;
> + unsigned int numranges;
> + u8 rangemask;
> + u8 sel_mask;
> + bool dvs;
> +};
> +
> +#define BD_RANGE(_min, _vstep, _sel_low, _sel_hi, _range_sel) \
> +{ \
> + .min_volt = (_min), .step = (_vstep), .min_sel = (_sel_low), \
> + .max_sel = (_sel_hi), .rangeval = (_range_sel) \
> +}
> +
> +#define BD_DATA(_name, enreg, enmask, vreg, vmask, _range, rmask, _dvs, sel) \
> +{ \
> + .name = (_name), .enable_reg = (enreg), .enablemask = (enmask), \
> + .volt_reg = (vreg), .volt_mask = (vmask), .ranges = (_range), \
> + .numranges = ARRAY_SIZE(_range), .rangemask = (rmask), .dvs = (_dvs), \
> + .sel_mask = (sel) \
> +}
> +
> +static struct bd71837_vrange buck1to4_vranges[] = {
> + BD_RANGE(700000, 10000, 0, 0x3C, 0),
Please use lower-case hex throughout.
> + BD_RANGE(1300000, 0, 0x3D, 0x3F, 0),
> +};
> +
> +static struct bd71837_vrange buck5_vranges[] = {
> + BD_RANGE(700000, 100000, 0, 0x3, 0),
> + BD_RANGE(1050000, 50000, 0x04, 0x05, 0),
> + BD_RANGE(1200000, 150000, 0x06, 0x07, 0),
> + BD_RANGE(675000, 100000, 0x0, 0x3, 0x80),
> + BD_RANGE(1025000, 50000, 0x04, 0x05, 0x80),
> + BD_RANGE(1175000, 150000, 0x06, 0x07, 0x80),
> +};
> +
> +static struct bd71837_vrange buck6_vranges[] = {
> + BD_RANGE(3000000, 100000, 0x00, 0x03, 0),
> +};
> +
> +static struct bd71837_vrange buck7_vranges[] = {
> + BD_RANGE(1605000, 90000, 0, 1, 0),
> + BD_RANGE(1755000, 45000, 2, 4, 0),
> + BD_RANGE(1905000, 45000, 5, 7, 0),
> +};
> +
> +static struct bd71837_vrange buck8_vranges[] = {
> + BD_RANGE(800000, 10000, 0x00, 0x3C, 0),
> +};
> +
> +static struct bd71837_vrange ldo1_vranges[] = {
> + BD_RANGE(3000000, 100000, 0x00, 0x03, 0),
> + BD_RANGE(1600000, 100000, 0x00, 0x03, 0x20),
> +};
> +
> +static struct bd71837_vrange ldo2_vranges[] = {
> + BD_RANGE(900000, 0, 0, 0, 0),
> + BD_RANGE(800000, 0, 1, 1, 0),
> +};
> +
> +static struct bd71837_vrange ldo3_vranges[] = {
> + BD_RANGE(1800000, 100000, 0x00, 0x0F, 0),
> +};
> +
> +static struct bd71837_vrange ldo4_vranges[] = {
> + BD_RANGE(900000, 100000, 0x00, 0x09, 0),
> +};
> +
> +static struct bd71837_vrange ldo5_vranges[] = {
> + BD_RANGE(1800000, 100000, 0x00, 0x0F, 0),
> +};
> +
> +static struct bd71837_vrange ldo6_vranges[] = {
> + BD_RANGE(900000, 100000, 0x00, 0x09, 0),
> +};
> +
> +static struct bd71837_vrange ldo7_vranges[] = {
> + BD_RANGE(1800000, 100000, 0x00, 0x0F, 0),
> +};
> +
> +/* We use enable mask 'HW_STATE_CONTROL' to indicate that this regulator
The first line of multi-line comments should be /* by itself. So:
/*
* We use ...
* ...
* last line.
*/
Please fix throughout.
> + * must not be enabled or disabled by SW. The typical use-case for BD71837
> + * is powering NXP i.MX8. In this use-case we (for now) only allow control
> + * for BUCK3 and BUCK4 which are not boot critical.
> + */
> +static struct bd71837_data bd71837_reg_data[] = {
> + BD_DATA("BUCK1", BD71837_BUCK1_CTRL, HW_STATE_CONTROL,
> + BD71837_BUCK1_VOLT_RUN, DVS_BUCK_RUN_MASK, buck1to4_vranges, 0,
> + true, BD71837_BUCK_SEL),
> + BD_DATA("BUCK2", BD71837_BUCK2_CTRL, HW_STATE_CONTROL,
> + BD71837_BUCK2_VOLT_RUN, DVS_BUCK_RUN_MASK, buck1to4_vranges, 0,
> + true, BD71837_BUCK_SEL),
> + BD_DATA("BUCK3", BD71837_BUCK3_CTRL, BD71837_BUCK_EN,
> + BD71837_BUCK3_VOLT_RUN, DVS_BUCK_RUN_MASK, buck1to4_vranges, 0,
> + true, BD71837_BUCK_SEL),
> + BD_DATA("BUCK4", BD71837_BUCK4_CTRL, BD71837_BUCK_EN,
> + BD71837_BUCK4_VOLT_RUN, DVS_BUCK_RUN_MASK, buck1to4_vranges, 0,
> + true, BD71837_BUCK_SEL),
> + BD_DATA("BUCK5", BD71837_BUCK5_CTRL, HW_STATE_CONTROL,
> + BD71837_BUCK5_VOLT, BD71837_BUCK5_MASK, buck5_vranges, 0x80,
> + false, BD71837_BUCK_SEL),
> + BD_DATA("BUCK6", BD71837_BUCK6_CTRL, HW_STATE_CONTROL,
> + BD71837_BUCK6_VOLT, BD71837_BUCK6_MASK, buck6_vranges, 0,
> + false, BD71837_BUCK_SEL),
> + BD_DATA("BUCK7", BD71837_BUCK7_CTRL, HW_STATE_CONTROL,
> + BD71837_BUCK7_VOLT, BD71837_BUCK7_MASK, buck7_vranges, 0,
> + false, BD71837_BUCK_SEL),
> + BD_DATA("BUCK8", BD71837_BUCK8_CTRL, HW_STATE_CONTROL,
> + BD71837_BUCK8_VOLT, BD71837_BUCK8_MASK, buck8_vranges, 0,
> + false, BD71837_BUCK_SEL),
> + BD_DATA("LDO1", BD71837_LDO1_VOLT, HW_STATE_CONTROL, BD71837_LDO1_VOLT,
> + BD71837_LDO1_MASK, ldo1_vranges, 0x20, false, BD71837_LDO_SEL),
> + BD_DATA("LDO2", BD71837_LDO2_VOLT, HW_STATE_CONTROL, BD71837_LDO2_VOLT,
> + BD71837_LDO2_MASK, ldo2_vranges, 0, false, BD71837_LDO_SEL),
> + BD_DATA("LDO3", BD71837_LDO3_VOLT, HW_STATE_CONTROL, BD71837_LDO3_VOLT,
> + BD71837_LDO3_MASK, ldo3_vranges, 0, false, BD71837_LDO_SEL),
> + BD_DATA("LDO4", BD71837_LDO4_VOLT, HW_STATE_CONTROL, BD71837_LDO4_VOLT,
> + BD71837_LDO4_MASK, ldo4_vranges, 0, false, BD71837_LDO_SEL),
> + BD_DATA("LDO5", BD71837_LDO5_VOLT, HW_STATE_CONTROL, BD71837_LDO5_VOLT,
> + BD71837_LDO5_MASK, ldo5_vranges, 0, false, BD71837_LDO_SEL),
> + BD_DATA("LDO6", BD71837_LDO6_VOLT, HW_STATE_CONTROL, BD71837_LDO6_VOLT,
> + BD71837_LDO6_MASK, ldo6_vranges, 0, false, BD71837_LDO_SEL),
> + BD_DATA("LDO7", BD71837_LDO7_VOLT, HW_STATE_CONTROL, BD71837_LDO7_VOLT,
> + BD71837_LDO7_MASK, ldo7_vranges, 0, false, BD71837_LDO_SEL),
> +};
> +
> +static int vrange_find_value(struct bd71837_vrange *r, u8 sel,
> + unsigned int *val)
> +{
> + if (!val || sel < r->min_sel || sel > r->max_sel)
> + return -EINVAL;
> +
> + *val = r->min_volt + r->step * (sel - r->min_sel);
> + return 0;
> +}
> +
> +static int vrange_find_selector(struct bd71837_vrange *r, int val, u8 *sel)
> +{
> + int ret = -EINVAL;
> + int num_vals = r->max_sel - r->min_sel + 1;
> +
> + if (val >= r->min_volt &&
> + val <= r->min_volt + r->step * (num_vals - 1)) {
> + if (r->step) {
> + *sel = r->min_sel + ((val - r->min_volt) / r->step);
> + ret = 0;
> + } else {
> + *sel = r->min_sel;
> + ret = 0;
> + }
> + }
> + return ret;
> +}
> +
> +static int bd71837_get_enable(struct udevice *dev)
> +{
> + int val;
> + struct bd71837_data *d = dev_get_platdata(dev);
Can you use plat instead of d? d is too short!
> +
> + /* boot critical regulators on bd71837 must not be controlled by sw
> + * due to the 'feature' which leaves power rails down if bd71837 is
> + * reseted to snvs state. hence we can't get the state here.
> + *
> + * if we are alive it means we probably are on run state and
> + * if the regulator can't be controlled we can assume it is
> + * enabled.
> + */
> + if (d->enablemask == HW_STATE_CONTROL)
> + return 1;
> +
> + val = pmic_reg_read(dev->parent, d->enable_reg);
> +
Drop blank line, since the next line relates to it. Same below.
> + if (val < 0)
> + return val;
> +
> + return (val & d->enablemask);
> +}
> +
> +static int bd71837_set_enable(struct udevice *dev, bool enable)
> +{
> + int val;
> + struct bd71837_data *d = dev_get_platdata(dev);
> +
> + /* boot critical regulators on bd71837 must not be controlled by sw
> + * due to the 'feature' which leaves power rails down if bd71837 is
> + * reseted to snvs state. Hence we can't set the state here.
> + */
> + if (d->enablemask == HW_STATE_CONTROL)
> + return -EINVAL;
> +
> + val = pmic_reg_read(dev->parent, d->enable_reg);
> +
> + if (val < 0)
> + return val;
> +
> + if (enable)
> + val |= d->enablemask;
> + else
> + val &= ~d->enablemask;
> +
> + return pmic_reg_write(dev->parent, d->enable_reg, val);
> +}
> +
> +static int bd71837_set_value(struct udevice *dev, int uvolt)
> +{
> + u8 sel, reg;
> + u8 range;
> + int i;
> + int not_found = 1;
> + struct bd71837_data *d = dev_get_platdata(dev);
> +
> + /* An under/overshooting may occur if voltage is changed for other
> + * regulators but buck 1,2,3 or 4 when regulator is enabled. Prevent
> + * change to protect the HW
> + */
> + if (!d->dvs)
> + if (bd71837_get_enable(dev)) {
> + pr_err("Only buck1-4 can be changed when enabled\n");
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < d->numranges; i++) {
> + struct bd71837_vrange *r = &d->ranges[i];
> +
> + not_found = vrange_find_selector(r, uvolt, &sel);
> + if (!not_found) {
> + unsigned int tmp;
> +
> + /* We require exactly the requested value to be
> + * supported - this can be changed later if needed
> + */
> + range = r->rangeval;
> + not_found = vrange_find_value(r, sel, &tmp);
> + if (!not_found && tmp == uvolt)
> + break;
> + not_found = 1;
> + }
> + }
> +
> + if (not_found)
> + return -EINVAL;
> +
> + sel <<= ffs(d->volt_mask) - 1;
> +
> + reg = pmic_reg_read(dev->parent, d->volt_reg);
> + if (reg < 0)
> + return reg;
> +
> + reg &= ~d->volt_mask;
> + reg |= sel;
> + if (d->rangemask) {
> + reg &= ~d->rangemask;
> + reg |= range;
> + }
> +
> + return pmic_reg_write(dev->parent, d->volt_reg, reg);
Can you use pmic_clrsetbits() ?
> +}
> +
> +static int bd71837_get_value(struct udevice *dev)
> +{
> + u8 reg, range;
> + unsigned int tmp;
> + struct bd71837_data *d = dev_get_platdata(dev);
> + int i;
> +
> + reg = pmic_reg_read(dev->parent, d->volt_reg);
> + if (reg < 0)
> + return reg;
> +
> + range = reg & d->rangemask;
> +
> + reg &= d->volt_mask;
> + reg >>= ffs(d->volt_mask) - 1;
> +
> + for (i = 0; i < d->numranges; i++) {
> + struct bd71837_vrange *r = &d->ranges[i];
> +
> + if (d->rangemask && ((d->rangemask & range) != r->rangeval))
> + continue;
> +
> + if (!vrange_find_value(r, reg, &tmp))
> + return tmp;
> + }
> +
> + pr_err("Unknown voltage value read from pmic\n");
> +
> + return -EINVAL;
> +}
> +
> +static int bd71837_regulator_probe(struct udevice *dev)
> +{
> + struct bd71837_data *d = dev_get_platdata(dev);
> + int i, ret;
> + struct dm_regulator_uclass_platdata *uc_pdata;
> +
> + for (i = 0; i < ARRAY_SIZE(bd71837_reg_data); i++) {
> + if (!strcmp(dev->name, bd71837_reg_data[i].name)) {
> + *d = bd71837_reg_data[i];
> + if (d->enablemask != HW_STATE_CONTROL) {
> + u8 ctrl;
> +
> + /* Take the regulator under SW control. Ensure
> + * the initial state matches dt flags and then
> + * write the SEL bit
> + */
> + uc_pdata = dev_get_uclass_platdata(dev);
> + ret = bd71837_set_enable(dev,
> + !!(uc_pdata->boot_on ||
> + uc_pdata->always_on));
> + if (ret)
> + return ret;
> +
> + ctrl = pmic_reg_read(dev->parent,
> + d->enable_reg);
> + if (ctrl < 0)
> + return ctrl;
> +
> + ctrl |= d->sel_mask;
> + return pmic_reg_write(dev->parent,
> + d->enable_reg, ctrl);
> + }
> + return 0;
> + }
> + }
> +
> + pr_err("Unknown regulator '%s'\n", dev->name);
> +
> + return -EINVAL;
-ENOENT ?
> +}
> +
> +static const struct dm_regulator_ops bd71837_regulator_ops = {
> + .get_value = bd71837_get_value,
> + .set_value = bd71837_set_value,
> + .get_enable = bd71837_get_enable,
> + .set_enable = bd71837_set_enable,
> +};
> +
> +U_BOOT_DRIVER(bd71837_regulator) = {
> + .name = "bd71837_regulator",
> + .id = UCLASS_REGULATOR,
> + .ops = &bd71837_regulator_ops,
> + .probe = bd71837_regulator_probe,
> + .platdata_auto_alloc_size = sizeof(struct bd71837_data),
> +};
> diff --git a/include/power/bd71837.h b/include/power/bd71837.h
> index 9c74f6fc61..da14dc9bb1 100644
> --- a/include/power/bd71837.h
> +++ b/include/power/bd71837.h
> @@ -59,6 +59,26 @@ enum {
> BD71837_REG_NUM,
> };
>
> +#define BD71837_BUCK_EN 0x01
> +#define BD71837_LDO_EN 0x40
> +#define BD71837_BUCK_SEL 0x02
> +#define BD71837_LDO_SEL 0x80
> +
> +#define DVS_BUCK_RUN_MASK 0x3F
> +#define BD71837_BUCK5_MASK 0x07
> +#define BD71837_BUCK6_MASK 0x03
> +#define BD71837_BUCK7_MASK 0x07
> +#define BD71837_BUCK8_MASK 0x3F
> +
> +#define BD71837_LDO1_MASK 0x03
> +#define BD71837_LDO1_RANGE_MASK 0x20
> +#define BD71837_LDO2_MASK 0x20
> +#define BD71837_LDO3_MASK 0x0F
> +#define BD71837_LDO4_MASK 0x0F
> +#define BD71837_LDO5_MASK 0x0F
> +#define BD71837_LDO6_MASK 0x0F
> +#define BD71837_LDO7_MASK 0x0F
> +
> int power_bd71837_init(unsigned char bus);
>
> #endif
> --
> 2.17.2
>
>
> --
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
>
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
This would be better in Latin.
Regards,
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [RFC PATCH v1 2/2] power: regulator: support ROHM BD71837 PMIC
2019-04-24 3:54 ` Simon Glass
@ 2019-04-24 5:59 ` Vaittinen, Matti
2019-04-24 16:54 ` Simon Glass
0 siblings, 1 reply; 6+ messages in thread
From: Vaittinen, Matti @ 2019-04-24 5:59 UTC (permalink / raw)
To: u-boot
Hello Simon,
Thanks a bunch for taking the time and reviewing this! I do appreciate!
On Tue, 2019-04-23 at 21:54 -0600, Simon Glass wrote:
> Hi Matti,
>
> On Wed, 27 Mar 2019 at 06:40, Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> >
> > Add regulator driver for ROHM BD71837 PMIC. BD71837 contains
> > 8 bucks and 7 LDOS. Voltages for bucks 1-4 can be adjusted
> > when regulators are enabled. For other bucks and LDOs we may
> > have over- or undershooting if voltage is adjusted when
> > regulator is enabled. Thus this is prevented by default.
> >
> > BD71837 has a quirk which may leave power output disabled
> > after reset if enable/disable state was controlled by SW.
> > Thus the SW control is only allowed for bucks3 and 4 by
> > default.
> >
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> > drivers/power/regulator/Kconfig | 15 ++
> > drivers/power/regulator/Makefile | 1 +
> > drivers/power/regulator/bd71837.c | 373
> > ++++++++++++++++++++++++++++++
> > include/power/bd71837.h | 20 ++
> > 4 files changed, 409 insertions(+)
> >
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> But please see nits below.
I see you reviewed the RFC version. I would like to ask you to see also
the non RFC version https://patchwork.ozlabs.org/patch/1080860/ which
supports also BD71847. I see that most (maybe all) of these comments
apply to that patch too - but you might have some additional ideas
too.
I will create v2 of this non RFC patch based on these comments (and fix
your comments to patch 1/2 too) but I won't add your reviewed-by just
yet - I hope you can also check the pieces adding BD71847 support and
give me a nudge if you see there something to improve. =)
> > --- /dev/null
> > +++ b/drivers/power/regulator/bd71837.c
> > @@ -0,0 +1,373 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +//
> > +// Copyright (C) 2019 ROHM Semiconductors
> > +//
> > +// ROHM BD71837 regulator driver
>
> The SPDX line has a // comment but everything else should use C
> comments:
>
> /*
> * Copyright ...
> *
> * ROHM ...
> */
This is fine for me. But just to note that this differs from Linux
notation which accepts also the copyright block being // - comments. I
am not sure how closely u-Boot follows (or wants to follow) kernel
coding guidelines. But as I said, I don't mind changing this.
> > +
> > +static int bd71837_regulator_probe(struct udevice *dev)
> > +{
> > + struct bd71837_data *d = dev_get_platdata(dev);
> > + int i, ret;
> > + struct dm_regulator_uclass_platdata *uc_pdata;
> > +
> > + for (i = 0; i < ARRAY_SIZE(bd71837_reg_data); i++) {
> > + if (!strcmp(dev->name, bd71837_reg_data[i].name)) {
> > + *d = bd71837_reg_data[i];
> > + if (d->enablemask != HW_STATE_CONTROL) {
> > + u8 ctrl;
> > +
> > + /* Take the regulator under SW
> > control. Ensure
> > + * the initial state matches dt
> > flags and then
> > + * write the SEL bit
> > + */
> > + uc_pdata =
> > dev_get_uclass_platdata(dev);
> > + ret = bd71837_set_enable(dev,
> > + !!(uc_pdat
> > a->boot_on ||
> > + uc_pdata-
> > >always_on));
> > + if (ret)
> > + return ret;
> > +
> > + ctrl = pmic_reg_read(dev->parent,
> > + d-
> > >enable_reg);
> > + if (ctrl < 0)
> > + return ctrl;
> > +
> > + ctrl |= d->sel_mask;
> > + return pmic_reg_write(dev->parent,
> > + d-
> > >enable_reg, ctrl);
> > + }
> > + return 0;
> > + }
> > + }
> > +
> > + pr_err("Unknown regulator '%s'\n", dev->name);
> > +
> > + return -EINVAL;
>
> -ENOENT ?
At first the -ENOENT sounded to me like a regulator/device is missing.
I thought that here we have extra (invalid/unknown) regulator in the
device-tree. Thus I used the -EINVAL. But I think you are right, we can
think that DT is correct and the driver lacks of correct regulator
entity. So -ENOENT can be appropriate. => I'll change this too.
> > --
> > Matti Vaittinen, Linux device drivers
> > ROHM Semiconductors, Finland SWDC
> > Kiviharjunlenkki 1E
> > 90220 OULU
> > FINLAND
> >
> > ~~~ "I don't think so," said Rene Descartes. Just then he vanished
> > ~~~
>
> This would be better in Latin.
Unfortunately that's far beyond my skills =) I can do Finnish though ;)
Rest of the comments seemed all like trivial fixes and I will apply
them =)
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [RFC PATCH v1 2/2] power: regulator: support ROHM BD71837 PMIC
2019-04-24 5:59 ` Vaittinen, Matti
@ 2019-04-24 16:54 ` Simon Glass
0 siblings, 0 replies; 6+ messages in thread
From: Simon Glass @ 2019-04-24 16:54 UTC (permalink / raw)
To: u-boot
Hi Matti,
On Tue, 23 Apr 2019 at 23:59, Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
>
> Hello Simon,
>
> Thanks a bunch for taking the time and reviewing this! I do appreciate!
>
> On Tue, 2019-04-23 at 21:54 -0600, Simon Glass wrote:
> > Hi Matti,
> >
> > On Wed, 27 Mar 2019 at 06:40, Matti Vaittinen
> > <matti.vaittinen@fi.rohmeurope.com> wrote:
> > >
> > > Add regulator driver for ROHM BD71837 PMIC. BD71837 contains
> > > 8 bucks and 7 LDOS. Voltages for bucks 1-4 can be adjusted
> > > when regulators are enabled. For other bucks and LDOs we may
> > > have over- or undershooting if voltage is adjusted when
> > > regulator is enabled. Thus this is prevented by default.
> > >
> > > BD71837 has a quirk which may leave power output disabled
> > > after reset if enable/disable state was controlled by SW.
> > > Thus the SW control is only allowed for bucks3 and 4 by
> > > default.
> > >
> > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > > ---
> > > drivers/power/regulator/Kconfig | 15 ++
> > > drivers/power/regulator/Makefile | 1 +
> > > drivers/power/regulator/bd71837.c | 373
> > > ++++++++++++++++++++++++++++++
> > > include/power/bd71837.h | 20 ++
> > > 4 files changed, 409 insertions(+)
> > >
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > But please see nits below.
>
> I see you reviewed the RFC version. I would like to ask you to see also
> the non RFC version https://patchwork.ozlabs.org/patch/1080860/ which
> supports also BD71847. I see that most (maybe all) of these comments
> apply to that patch too - but you might have some additional ideas
> too.
>
> I will create v2 of this non RFC patch based on these comments (and fix
> your comments to patch 1/2 too) but I won't add your reviewed-by just
> yet - I hope you can also check the pieces adding BD71847 support and
> give me a nudge if you see there something to improve. =)
>
> > > --- /dev/null
> > > +++ b/drivers/power/regulator/bd71837.c
> > > @@ -0,0 +1,373 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +//
> > > +// Copyright (C) 2019 ROHM Semiconductors
> > > +//
> > > +// ROHM BD71837 regulator driver
> >
> > The SPDX line has a // comment but everything else should use C
> > comments:
> >
> > /*
> > * Copyright ...
> > *
> > * ROHM ...
> > */
>
> This is fine for me. But just to note that this differs from Linux
> notation which accepts also the copyright block being // - comments. I
> am not sure how closely u-Boot follows (or wants to follow) kernel
> coding guidelines. But as I said, I don't mind changing this.
U-Boot mostly follows Linux, but you can see conventions by looking at
the code, generally.
>
> > > +
> > > +static int bd71837_regulator_probe(struct udevice *dev)
> > > +{
> > > + struct bd71837_data *d = dev_get_platdata(dev);
> > > + int i, ret;
> > > + struct dm_regulator_uclass_platdata *uc_pdata;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(bd71837_reg_data); i++) {
> > > + if (!strcmp(dev->name, bd71837_reg_data[i].name)) {
> > > + *d = bd71837_reg_data[i];
> > > + if (d->enablemask != HW_STATE_CONTROL) {
> > > + u8 ctrl;
> > > +
> > > + /* Take the regulator under SW
> > > control. Ensure
> > > + * the initial state matches dt
> > > flags and then
> > > + * write the SEL bit
> > > + */
> > > + uc_pdata =
> > > dev_get_uclass_platdata(dev);
> > > + ret = bd71837_set_enable(dev,
> > > + !!(uc_pdat
> > > a->boot_on ||
> > > + uc_pdata-
> > > >always_on));
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ctrl = pmic_reg_read(dev->parent,
> > > + d-
> > > >enable_reg);
> > > + if (ctrl < 0)
> > > + return ctrl;
> > > +
> > > + ctrl |= d->sel_mask;
> > > + return pmic_reg_write(dev->parent,
> > > + d-
> > > >enable_reg, ctrl);
> > > + }
> > > + return 0;
> > > + }
> > > + }
> > > +
> > > + pr_err("Unknown regulator '%s'\n", dev->name);
> > > +
> > > + return -EINVAL;
> >
> > -ENOENT ?
>
> At first the -ENOENT sounded to me like a regulator/device is missing.
> I thought that here we have extra (invalid/unknown) regulator in the
> device-tree. Thus I used the -EINVAL. But I think you are right, we can
> think that DT is correct and the driver lacks of correct regulator
> entity. So -ENOENT can be appropriate. => I'll change this too.
>
> > > --
> > > Matti Vaittinen, Linux device drivers
> > > ROHM Semiconductors, Finland SWDC
> > > Kiviharjunlenkki 1E
> > > 90220 OULU
> > > FINLAND
> > >
> > > ~~~ "I don't think so," said Rene Descartes. Just then he vanished
> > > ~~~
> >
> > This would be better in Latin.
>
> Unfortunately that's far beyond my skills =) I can do Finnish though ;)
Me also, but maybe something like this is close?
"non cogito me" dixit Rene Descarte, deinde evanescavit
Regards,
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-24 16:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-27 12:39 [U-Boot] [RFC PATCH v1 1/2] copy the bd71837 pmic driver from NXP imx u-boot Matti Vaittinen
2019-03-27 12:40 ` [U-Boot] [RFC PATCH v1 2/2] power: regulator: support ROHM BD71837 PMIC Matti Vaittinen
2019-04-04 7:03 ` Matti Vaittinen
2019-04-24 3:54 ` Simon Glass
2019-04-24 5:59 ` Vaittinen, Matti
2019-04-24 16:54 ` Simon Glass
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox