All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH v2 0/2] support for ROHM BD71837 and BD71847 PMICs
@ 2019-04-24 12:33 Matti Vaittinen
  2019-04-24 12:35 ` [U-Boot] [PATCH v2 1/2] regulator: bd71837: copy the bd71837 pmic driver from NXP imx u-boot Matti Vaittinen
  2019-04-24 12:37 ` [U-Boot] [PATCH v2 2/2] regulator: bd718x7: support ROHM BD71837 and BD71847 PMICs Matti Vaittinen
  0 siblings, 2 replies; 7+ messages in thread
From: Matti Vaittinen @ 2019-04-24 12:33 UTC (permalink / raw)
  To: u-boot

Patch series to support for ROHM BD71837 and BD71847 PMICs.

ROHM BD71837 and BD71847 is PMIC intended for powering single-core,
dual-core, and quad-core SoC’s such as NXP-i.MX 8M. BD71847 is used
for example on NXP imx8mm EVK.

Series adds PMIC driver with register read and write support, and
regulator driver with enable/disable and voltage changing support
with certain limitations:
- Enable/Disable state control is by default only allowed for BD71837
  bucks 3 and 4. This is due to a HW feature which leaves SW
  controlled power rails unpowered after reset which leads to SNVS
  state.
- Voltage control for enabled regulator is only allowed for "DVS"
  regulators (bucks 1-4 on BD71837, bucks 1 and 2 on BD71847). This
  is done because changing voltage on other regulators may cause
  under-/over shoot when regulator is enabled.

Drivers expect to see PMIC node with proper regulator sub-nodes given
from device tree but.

Driver is tested using BD71837 and BD71847 break-out board connected
to Beagle Bone Black and device-tree entry which was created as
desctibed in dt-binding document for BD718x7 shipped with Linux.

This patch series does not support DT properties which allow changing
the reset target between SNVS and READY or identifying boot-critical
regulators and disabling SW control based on reset target as the
Linux driver does.

Changelog v2: (fix issues pointed by Simon Glass)
- Fix typo in patch series name (BD71827 => BD71837)
- Drop drivers/power/pmic/pmic_bd71837.c (the old PMIC interface)
- Fix characters in hex numbers to lowercase
- use pmic_clrsetbits() instead of using separate reads and writes
- fix styling issues

Changelog v1:
- This version is created based on the RFC v1.
  https://lists.denx.de/pipermail/u-boot/2019-March/363076.html
  https://lists.denx.de/pipermail/u-boot/2019-March/363077.html
- Support BD71847.
- Unlock the PMIC protection register

Patch 1:
- Cherry picks the initial PMIC driver for BD71837 from NXP's i.MX
  repository at: https://source.codeaurora.org/external/imx/uboot-imx
  (commits e9a3bec2e95a and acdc5c297a96). Fixes checkpatch issues.

Patch 2:
- Support BD71847 PMIC.
- Add support for BD71837 and BD71847 regulators.

---

Matti Vaittinen (2):
  regulator: bd71837: copy the bd71837 pmic driver from NXP imx u-boot
  regulator: bd718x7: support ROHM BD71837 and BD71847 PMICs

 drivers/power/pmic/Kconfig        |   7 +
 drivers/power/pmic/Makefile       |   1 +
 drivers/power/pmic/bd71837.c      | 122 ++++++++
 drivers/power/regulator/Kconfig   |  15 +
 drivers/power/regulator/Makefile  |   1 +
 drivers/power/regulator/bd71837.c | 469 ++++++++++++++++++++++++++++++
 include/power/bd71837.h           | 103 +++++++
 7 files changed, 718 insertions(+)
 create mode 100644 drivers/power/pmic/bd71837.c
 create mode 100644 drivers/power/regulator/bd71837.c
 create mode 100644 include/power/bd71837.h

-- 
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	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH v2 1/2] regulator: bd71837: copy the bd71837 pmic driver from NXP imx u-boot
  2019-04-24 12:33 [U-Boot] [PATCH v2 0/2] support for ROHM BD71837 and BD71847 PMICs Matti Vaittinen
@ 2019-04-24 12:35 ` Matti Vaittinen
  2019-04-24 23:58   ` Simon Glass
  2019-04-24 12:37 ` [U-Boot] [PATCH v2 2/2] regulator: bd718x7: support ROHM BD71837 and BD71847 PMICs Matti Vaittinen
  1 sibling, 1 reply; 7+ messages in thread
From: Matti Vaittinen @ 2019-04-24 12:35 UTC (permalink / raw)
  To: u-boot

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

Changelog v1 => v2:
- Drop drivers/power/pmic/pmic_bd71837.c (the old PMIC interface)
- Fix characters in hex numbers to lowercase

 drivers/power/pmic/Kconfig   |  7 +++
 drivers/power/pmic/Makefile  |  1 +
 drivers/power/pmic/bd71837.c | 90 ++++++++++++++++++++++++++++++++++++
 include/power/bd71837.h      | 62 +++++++++++++++++++++++++
 4 files changed, 160 insertions(+)
 create mode 100644 drivers/power/pmic/bd71837.c
 create mode 100644 include/power/bd71837.h

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..e605a88196 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
diff --git a/drivers/power/pmic/bd71837.c b/drivers/power/pmic/bd71837.c
new file mode 100644
index 0000000000..b749f9430a
--- /dev/null
+++ b/drivers/power/pmic/bd71837.c
@@ -0,0 +1,90 @@
+// 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/include/power/bd71837.h b/include/power/bd71837.h
new file mode 100644
index 0000000000..38c69b2b90
--- /dev/null
+++ b/include/power/bd71837.h
@@ -0,0 +1,62 @@
+/* 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,
+};
+
+#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] 7+ messages in thread

* [U-Boot] [PATCH v2 2/2] regulator: bd718x7: support ROHM BD71837 and BD71847 PMICs
  2019-04-24 12:33 [U-Boot] [PATCH v2 0/2] support for ROHM BD71837 and BD71847 PMICs Matti Vaittinen
  2019-04-24 12:35 ` [U-Boot] [PATCH v2 1/2] regulator: bd71837: copy the bd71837 pmic driver from NXP imx u-boot Matti Vaittinen
@ 2019-04-24 12:37 ` Matti Vaittinen
  2019-04-24 23:58   ` Simon Glass
  1 sibling, 1 reply; 7+ messages in thread
From: Matti Vaittinen @ 2019-04-24 12:37 UTC (permalink / raw)
  To: u-boot

BD71837 and BD71847 is PMIC intended for powering single-core,
dual-core, and quad-core SoC’s such as NXP-i.MX 8M. BD71847
is used for example on NXP imx8mm EVK.

Add regulator driver for ROHM BD71837 and BD71847 PMICs.
BD71837 contains 8 bucks and 7 LDOS. BD71847 is reduced
version containing 6 bucks and 6 LDOs. Voltages for DVS
bucks (1-4 on BD71837, 1 and 2 on BD71847) 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.

BD718x7 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 BD71837  bucks
3 and 4 by default. The impact of this limitation must be
evaluated board-by board and restrictions may need to be
modified. (Linux driver get's these limitations from DT and we
may want to implement same on u-Boot driver).

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---

Changelog v1 => v2:
- document structs containing the platdata
- use pmic_clrsetbits() instead of using separate reads and writes
- fix styling issues

 drivers/power/pmic/bd71837.c      |  42 ++-
 drivers/power/regulator/Kconfig   |  15 +
 drivers/power/regulator/Makefile  |   1 +
 drivers/power/regulator/bd71837.c | 469 ++++++++++++++++++++++++++++++
 include/power/bd71837.h           | 147 ++++++----
 5 files changed, 616 insertions(+), 58 deletions(-)
 create mode 100644 drivers/power/regulator/bd71837.c

diff --git a/drivers/power/pmic/bd71837.c b/drivers/power/pmic/bd71837.c
index b749f9430a..12c2e15a19 100644
--- a/drivers/power/pmic/bd71837.c
+++ b/drivers/power/pmic/bd71837.c
@@ -3,6 +3,8 @@
  * Copyright 2018 NXP
  */
 
+#define DEBUG
+
 #include <common.h>
 #include <fdtdec.h>
 #include <errno.h>
@@ -16,15 +18,15 @@ DECLARE_GLOBAL_DATA_PTR;
 
 static const struct pmic_child_info pmic_children_info[] = {
 	/* buck */
-	{ .prefix = "b", .driver = BD71837_REGULATOR_DRIVER},
+	{ .prefix = "b", .driver = BD718XX_REGULATOR_DRIVER},
 	/* ldo */
-	{ .prefix = "l", .driver = BD71837_REGULATOR_DRIVER},
+	{ .prefix = "l", .driver = BD718XX_REGULATOR_DRIVER},
 	{ },
 };
 
 static int bd71837_reg_count(struct udevice *dev)
 {
-	return BD71837_REG_NUM;
+	return BD718XX_MAX_REGISTER - 1;
 }
 
 static int bd71837_write(struct udevice *dev, uint reg, const uint8_t *buff,
@@ -55,7 +57,7 @@ static int bd71837_bind(struct udevice *dev)
 
 	regulators_node = dev_read_subnode(dev, "regulators");
 	if (!ofnode_valid(regulators_node)) {
-		debug("%s: %s regulators subnode not found!", __func__,
+		debug("%s: %s regulators subnode not found!\n", __func__,
 		      dev->name);
 		return -ENXIO;
 	}
@@ -70,6 +72,34 @@ static int bd71837_bind(struct udevice *dev)
 	return 0;
 }
 
+static int bd718x7_probe(struct udevice *dev)
+{
+	int ret;
+	u8 unlock;
+
+	/* Unlock the PMIC regulator control before probing the children */
+	ret = pmic_reg_read(dev, BD718XX_REGLOCK);
+	if (ret < 0) {
+		debug("%s: %s Failed to read lock register, error %d\n",
+		      __func__, dev->name, ret);
+		return ret;
+	}
+
+	unlock = ret;
+	unlock &= ~(BD718XX_REGLOCK_PWRSEQ | BD718XX_REGLOCK_VREG);
+
+	ret = pmic_reg_write(dev, BD718XX_REGLOCK, unlock);
+	if (ret) {
+		debug("%s: %s Failed to unlock regulator control\n", __func__,
+		      dev->name);
+		return ret;
+	}
+	debug("%s: '%s' - BD718x7 PMIC register unlocked\n", __func__,
+	      dev->name);
+
+	return 0;
+}
+
 static struct dm_pmic_ops bd71837_ops = {
 	.reg_count = bd71837_reg_count,
 	.read = bd71837_read,
@@ -77,7 +107,8 @@ static struct dm_pmic_ops bd71837_ops = {
 };
 
 static const struct udevice_id bd71837_ids[] = {
-	{ .compatible = "rohm,bd71837", .data = 0x4b, },
+	{ .compatible = "rohm,bd71837", .data = ROHM_CHIP_TYPE_BD71837, },
+	{ .compatible = "rohm,bd71847", .data = ROHM_CHIP_TYPE_BD71847, },
 	{ }
 };
 
@@ -86,5 +117,6 @@ U_BOOT_DRIVER(pmic_bd71837) = {
 	.id = UCLASS_PMIC,
 	.of_match = bd71837_ids,
 	.bind = bd71837_bind,
+	.probe = bd718x7_probe,
 	.ops = &bd71837_ops,
 };
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..060e6efe74
--- /dev/null
+++ b/drivers/power/regulator/bd71837.c
@@ -0,0 +1,469 @@
+// 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
+#define DEBUG
+
+/**
+ * struct bd71837_vrange - describe linear range of voltages
+ *
+ * @min_volt:	smallest voltage in range
+ * @step:	how much voltage changes at each selector step
+ * @min_sel:	smallest selector in the range
+ * @max_sel:	maximum selector in the range
+ * @rangeval:	register value used to select this range if selectible
+ *		ranges are supported
+ */
+struct bd71837_vrange {
+	unsigned int	min_volt;
+	unsigned int	step;
+	u8		min_sel;
+	u8		max_sel;
+	u8		rangeval;
+};
+
+/**
+ * struct bd71837_platdata - describe regulator control registers
+ *
+ * @name:	name of the regulator. Used for matching the dt-entry
+ * @enable_reg:	register address used to enable/disable regulator
+ * @enablemask:	register mask used to enable/disable regulator
+ * @volt_reg:	register address used to configure regulator voltage
+ * @volt_mask:	register mask used to configure regulator voltage
+ * @ranges:	pointer to ranges of regulator voltages and matching register
+ *		values
+ * @numranges:	number of voltage ranges pointed by ranges
+ * @rangemask:	mask for selecting used ranges if multiple ranges are supported
+ * @sel_mask:	bit to toggle in order to transfer the register control to SW
+ * @dvs:	whether the voltage can be changed when regulator is enabled
+ */
+struct 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 dvs_buck_vranges[] = {
+	BD_RANGE(700000, 10000, 0, 0x3c, 0),
+	BD_RANGE(1300000, 0, 0x3d, 0x3f, 0),
+};
+
+static struct bd71837_vrange bd71847_buck3_vranges[] = {
+	BD_RANGE(700000, 100000, 0x00, 0x03, 0),
+	BD_RANGE(1050000, 50000, 0x04, 0x05, 0),
+	BD_RANGE(1200000, 150000, 0x06, 0x07, 0),
+	BD_RANGE(550000, 50000, 0x0, 0x7, 0x40),
+	BD_RANGE(675000, 100000, 0x0, 0x3, 0x80),
+	BD_RANGE(1025000, 50000, 0x4, 0x5, 0x80),
+	BD_RANGE(1175000, 150000, 0x6, 0x7, 0x80),
+};
+
+static struct bd71837_vrange bd71847_buck4_vranges[] = {
+	BD_RANGE(3000000, 100000, 0x00, 0x03, 0),
+	BD_RANGE(2600000, 100000, 0x00, 0x03, 40),
+};
+
+static struct bd71837_vrange bd71837_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 bd71837_buck6_vranges[] = {
+	BD_RANGE(3000000, 100000, 0x00, 0x03, 0),
+};
+
+static struct bd71837_vrange nodvs_buck3_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 nodvs_buck4_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 bd71837_ldo5_vranges[] = {
+	BD_RANGE(1800000, 100000, 0x00, 0x0f, 0),
+};
+
+static struct bd71837_vrange bd71847_ldo5_vranges[] = {
+	BD_RANGE(1800000, 100000, 0x00, 0x0f, 0),
+	BD_RANGE(800000, 100000, 0x00, 0x0f, 0x20),
+};
+
+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_platdata bd71837_reg_data[] = {
+/* Bucks 1-4 which support dynamic voltage scaling */
+	BD_DATA("BUCK1", BD718XX_BUCK1_CTRL, HW_STATE_CONTROL,
+		BD718XX_BUCK1_VOLT_RUN, DVS_BUCK_RUN_MASK, dvs_buck_vranges, 0,
+		true, BD718XX_BUCK_SEL),
+	BD_DATA("BUCK2", BD718XX_BUCK2_CTRL, HW_STATE_CONTROL,
+		BD718XX_BUCK2_VOLT_RUN, DVS_BUCK_RUN_MASK, dvs_buck_vranges, 0,
+		true, BD718XX_BUCK_SEL),
+	BD_DATA("BUCK3", BD71837_BUCK3_CTRL, BD718XX_BUCK_EN,
+		BD71837_BUCK3_VOLT_RUN, DVS_BUCK_RUN_MASK, dvs_buck_vranges, 0,
+		true, BD718XX_BUCK_SEL),
+	BD_DATA("BUCK4", BD71837_BUCK4_CTRL, BD718XX_BUCK_EN,
+		BD71837_BUCK4_VOLT_RUN, DVS_BUCK_RUN_MASK, dvs_buck_vranges, 0,
+		true, BD718XX_BUCK_SEL),
+/* Bucks 5-8 which do not support dynamic voltage scaling */
+	BD_DATA("BUCK5", BD718XX_1ST_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
+		BD718XX_1ST_NODVS_BUCK_VOLT, BD718XX_1ST_NODVS_BUCK_MASK,
+		bd71837_buck5_vranges, 0x80, false, BD718XX_BUCK_SEL),
+	BD_DATA("BUCK6", BD718XX_2ND_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
+		BD718XX_2ND_NODVS_BUCK_VOLT, BD71837_BUCK6_MASK,
+		bd71837_buck6_vranges, 0, false, BD718XX_BUCK_SEL),
+	BD_DATA("BUCK7", BD718XX_3RD_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
+		BD718XX_3RD_NODVS_BUCK_VOLT, BD718XX_3RD_NODVS_BUCK_MASK,
+		nodvs_buck3_vranges, 0, false, BD718XX_BUCK_SEL),
+	BD_DATA("BUCK8", BD718XX_4TH_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
+		BD718XX_4TH_NODVS_BUCK_VOLT, BD718XX_4TH_NODVS_BUCK_MASK,
+		nodvs_buck4_vranges, 0, false, BD718XX_BUCK_SEL),
+/* LDOs */
+	BD_DATA("LDO1", BD718XX_LDO1_VOLT, HW_STATE_CONTROL, BD718XX_LDO1_VOLT,
+		BD718XX_LDO1_MASK, ldo1_vranges, 0x20, false, BD718XX_LDO_SEL),
+	BD_DATA("LDO2", BD718XX_LDO2_VOLT, HW_STATE_CONTROL, BD718XX_LDO2_VOLT,
+		BD718XX_LDO2_MASK, ldo2_vranges, 0, false, BD718XX_LDO_SEL),
+	BD_DATA("LDO3", BD718XX_LDO3_VOLT, HW_STATE_CONTROL, BD718XX_LDO3_VOLT,
+		BD718XX_LDO3_MASK, ldo3_vranges, 0, false, BD718XX_LDO_SEL),
+	BD_DATA("LDO4", BD718XX_LDO4_VOLT, HW_STATE_CONTROL, BD718XX_LDO4_VOLT,
+		BD718XX_LDO4_MASK, ldo4_vranges, 0, false, BD718XX_LDO_SEL),
+	BD_DATA("LDO5", BD718XX_LDO5_VOLT, HW_STATE_CONTROL, BD718XX_LDO5_VOLT,
+		BD71837_LDO5_MASK, bd71837_ldo5_vranges, 0, false,
+		BD718XX_LDO_SEL),
+	BD_DATA("LDO6", BD718XX_LDO6_VOLT, HW_STATE_CONTROL, BD718XX_LDO6_VOLT,
+		BD718XX_LDO6_MASK, ldo6_vranges, 0, false, BD718XX_LDO_SEL),
+	BD_DATA("LDO7", BD71837_LDO7_VOLT, HW_STATE_CONTROL, BD71837_LDO7_VOLT,
+		BD71837_LDO7_MASK, ldo7_vranges, 0, false, BD718XX_LDO_SEL),
+};
+
+static struct bd71837_platdata bd71847_reg_data[] = {
+/* Bucks 1 and 2 which support dynamic voltage scaling */
+	BD_DATA("BUCK1", BD718XX_BUCK1_CTRL, HW_STATE_CONTROL,
+		BD718XX_BUCK1_VOLT_RUN, DVS_BUCK_RUN_MASK, dvs_buck_vranges, 0,
+		true, BD718XX_BUCK_SEL),
+	BD_DATA("BUCK2", BD718XX_BUCK2_CTRL, HW_STATE_CONTROL,
+		BD718XX_BUCK2_VOLT_RUN, DVS_BUCK_RUN_MASK, dvs_buck_vranges, 0,
+		true, BD718XX_BUCK_SEL),
+/* Bucks 3-6 which do not support dynamic voltage scaling */
+	BD_DATA("BUCK3", BD718XX_1ST_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
+		BD718XX_1ST_NODVS_BUCK_VOLT, BD718XX_1ST_NODVS_BUCK_MASK,
+		bd71847_buck3_vranges, 0xc0, false, BD718XX_BUCK_SEL),
+	BD_DATA("BUCK4", BD718XX_2ND_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
+		BD718XX_2ND_NODVS_BUCK_VOLT, BD71837_BUCK6_MASK,
+		bd71847_buck4_vranges, 0x40, false, BD718XX_BUCK_SEL),
+	BD_DATA("BUCK5", BD718XX_3RD_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
+		BD718XX_3RD_NODVS_BUCK_VOLT, BD718XX_3RD_NODVS_BUCK_MASK,
+		nodvs_buck3_vranges, 0, false, BD718XX_BUCK_SEL),
+	BD_DATA("BUCK6", BD718XX_4TH_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
+		BD718XX_4TH_NODVS_BUCK_VOLT, BD718XX_4TH_NODVS_BUCK_MASK,
+		nodvs_buck4_vranges, 0, false, BD718XX_BUCK_SEL),
+/* LDOs */
+	BD_DATA("LDO1", BD718XX_LDO1_VOLT, HW_STATE_CONTROL, BD718XX_LDO1_VOLT,
+		BD718XX_LDO1_MASK, ldo1_vranges, 0x20, false, BD718XX_LDO_SEL),
+	BD_DATA("LDO2", BD718XX_LDO2_VOLT, HW_STATE_CONTROL, BD718XX_LDO2_VOLT,
+		BD718XX_LDO2_MASK, ldo2_vranges, 0, false, BD718XX_LDO_SEL),
+	BD_DATA("LDO3", BD718XX_LDO3_VOLT, HW_STATE_CONTROL, BD718XX_LDO3_VOLT,
+		BD718XX_LDO3_MASK, ldo3_vranges, 0, false, BD718XX_LDO_SEL),
+	BD_DATA("LDO4", BD718XX_LDO4_VOLT, HW_STATE_CONTROL, BD718XX_LDO4_VOLT,
+		BD718XX_LDO4_MASK, ldo4_vranges, 0, false, BD718XX_LDO_SEL),
+	BD_DATA("LDO5", BD718XX_LDO5_VOLT, HW_STATE_CONTROL, BD718XX_LDO5_VOLT,
+		BD71847_LDO5_MASK, bd71847_ldo5_vranges, 0x20, false,
+		BD718XX_LDO_SEL),
+	BD_DATA("LDO6", BD718XX_LDO6_VOLT, HW_STATE_CONTROL, BD718XX_LDO6_VOLT,
+		BD718XX_LDO6_MASK, ldo6_vranges, 0, false, BD718XX_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_platdata *plat = 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 (plat->enablemask == HW_STATE_CONTROL)
+		return 1;
+
+	val = pmic_reg_read(dev->parent, plat->enable_reg);
+	if (val < 0)
+		return val;
+
+	return (val & plat->enablemask);
+}
+
+static int bd71837_set_enable(struct udevice *dev, bool enable)
+{
+	int val = 0;
+	struct bd71837_platdata *plat = 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 (plat->enablemask == HW_STATE_CONTROL)
+		return -EINVAL;
+
+	if (enable)
+		val = plat->enablemask;
+
+	return pmic_clrsetbits(dev->parent, plat->enable_reg, plat->enablemask,
+			       val);
+}
+
+static int bd71837_set_value(struct udevice *dev, int uvolt)
+{
+	u8 sel;
+	u8 range;
+	int i;
+	int not_found = 1;
+	struct bd71837_platdata *plat = 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 (!plat->dvs)
+		if (bd71837_get_enable(dev)) {
+			pr_err("Only DVS bucks can be changed when enabled\n");
+			return -EINVAL;
+		}
+
+	for (i = 0; i < plat->numranges; i++) {
+		struct bd71837_vrange *r = &plat->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(plat->volt_mask) - 1;
+
+	if (plat->rangemask)
+		sel |= range;
+
+	return pmic_clrsetbits(dev->parent, plat->volt_reg, plat->volt_mask |
+			       plat->rangemask, sel);
+}
+
+static int bd71837_get_value(struct udevice *dev)
+{
+	u8 reg, range;
+	unsigned int tmp;
+	struct bd71837_platdata *plat = dev_get_platdata(dev);
+	int i;
+
+	reg = pmic_reg_read(dev->parent, plat->volt_reg);
+	if (reg < 0)
+		return reg;
+
+	range = reg & plat->rangemask;
+
+	reg &= plat->volt_mask;
+	reg >>= ffs(plat->volt_mask) - 1;
+
+	for (i = 0; i < plat->numranges; i++) {
+		struct bd71837_vrange *r = &plat->ranges[i];
+
+		if (plat->rangemask && ((plat->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_platdata *plat = dev_get_platdata(dev);
+	int i, ret;
+	struct dm_regulator_uclass_platdata *uc_pdata;
+	int type;
+	struct bd71837_platdata *init_data;
+	int data_amnt;
+
+	type = dev_get_driver_data(dev_get_parent(dev));
+
+	switch (type) {
+	case ROHM_CHIP_TYPE_BD71837:
+		init_data = bd71837_reg_data;
+		data_amnt = ARRAY_SIZE(bd71837_reg_data);
+		break;
+	case ROHM_CHIP_TYPE_BD71847:
+		init_data = bd71847_reg_data;
+		data_amnt = ARRAY_SIZE(bd71847_reg_data);
+		break;
+	default:
+		debug("Unknown PMIC type\n");
+		init_data = NULL;
+		data_amnt = 0;
+		break;
+	}
+
+	for (i = 0; i < data_amnt; i++) {
+		if (!strcmp(dev->name, init_data[i].name)) {
+			*plat = init_data[i];
+			if (plat->enablemask != HW_STATE_CONTROL) {
+				/*
+				 * 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;
+
+				return pmic_clrsetbits(dev->parent,
+						      plat->enable_reg,
+						      plat->sel_mask,
+						      plat->sel_mask);
+			}
+			return 0;
+		}
+	}
+
+	pr_err("Unknown regulator '%s'\n", dev->name);
+
+	return -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 = BD718XX_REGULATOR_DRIVER,
+	.id = UCLASS_REGULATOR,
+	.ops = &bd71837_regulator_ops,
+	.probe = bd71837_regulator_probe,
+	.platdata_auto_alloc_size = sizeof(struct bd71837_platdata),
+};
diff --git a/include/power/bd71837.h b/include/power/bd71837.h
index 38c69b2b90..75e07e1de3 100644
--- a/include/power/bd71837.h
+++ b/include/power/bd71837.h
@@ -1,62 +1,103 @@
 /* SPDX-License-Identifier: GPL-2.0-or-later */
 /* Copyright (C) 2018 ROHM Semiconductors */
 
-#ifndef BD71837_H_
-#define BD71837_H_
+#ifndef BD718XX_H_
+#define BD718XX_H_
 
-#define BD71837_REGULATOR_DRIVER "bd71837_regulator"
+#define BD718XX_REGULATOR_DRIVER "bd718x7_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,
+	ROHM_CHIP_TYPE_BD71837 = 0,
+	ROHM_CHIP_TYPE_BD71847,
+	ROHM_CHIP_TYPE_BD70528,
+	ROHM_CHIP_TYPE_AMOUNT
 };
 
+enum {
+	BD718XX_REV			= 0x00,
+	BD718XX_SWRESET			= 0x01,
+	BD718XX_I2C_DEV			= 0x02,
+	BD718XX_PWRCTRL0		= 0x03,
+	BD718XX_PWRCTRL1		= 0x04,
+	BD718XX_BUCK1_CTRL		= 0x05,
+	BD718XX_BUCK2_CTRL		= 0x06,
+	BD71837_BUCK3_CTRL		= 0x07,
+	BD71837_BUCK4_CTRL		= 0x08,
+	BD718XX_1ST_NODVS_BUCK_CTRL	= 0x09,
+	BD718XX_2ND_NODVS_BUCK_CTRL	= 0x0a,
+	BD718XX_3RD_NODVS_BUCK_CTRL	= 0x0b,
+	BD718XX_4TH_NODVS_BUCK_CTRL	= 0x0c,
+	BD718XX_BUCK1_VOLT_RUN		= 0x0d,
+	BD718XX_BUCK1_VOLT_IDLE		= 0x0e,
+	BD718XX_BUCK1_VOLT_SUSP		= 0x0f,
+	BD718XX_BUCK2_VOLT_RUN		= 0x10,
+	BD718XX_BUCK2_VOLT_IDLE		= 0x11,
+	BD71837_BUCK3_VOLT_RUN		= 0x12,
+	BD71837_BUCK4_VOLT_RUN		= 0x13,
+	BD718XX_1ST_NODVS_BUCK_VOLT	= 0x14,
+	BD718XX_2ND_NODVS_BUCK_VOLT	= 0x15,
+	BD718XX_3RD_NODVS_BUCK_VOLT	= 0x16,
+	BD718XX_4TH_NODVS_BUCK_VOLT	= 0x17,
+	BD718XX_LDO1_VOLT		= 0x18,
+	BD718XX_LDO2_VOLT		= 0x19,
+	BD718XX_LDO3_VOLT		= 0x1a,
+	BD718XX_LDO4_VOLT		= 0x1b,
+	BD718XX_LDO5_VOLT		= 0x1c,
+	BD718XX_LDO6_VOLT		= 0x1d,
+	BD71837_LDO7_VOLT		= 0x1e,
+	BD718XX_TRANS_COND0		= 0x1f,
+	BD718XX_TRANS_COND1		= 0x20,
+	BD718XX_VRFAULTEN		= 0x21,
+	BD718XX_MVRFLTMASK0		= 0x22,
+	BD718XX_MVRFLTMASK1		= 0x23,
+	BD718XX_MVRFLTMASK2		= 0x24,
+	BD718XX_RCVCFG			= 0x25,
+	BD718XX_RCVNUM			= 0x26,
+	BD718XX_PWRONCONFIG0		= 0x27,
+	BD718XX_PWRONCONFIG1		= 0x28,
+	BD718XX_RESETSRC		= 0x29,
+	BD718XX_MIRQ			= 0x2a,
+	BD718XX_IRQ			= 0x2b,
+	BD718XX_IN_MON			= 0x2c,
+	BD718XX_POW_STATE		= 0x2d,
+	BD718XX_OUT32K			= 0x2e,
+	BD718XX_REGLOCK			= 0x2f,
+	BD718XX_MUXSW_EN		= 0x30,
+	BD718XX_REG_OTPVER		= 0xff,
+	BD718XX_MAX_REGISTER		= 0x100,
+};
+
+#define BD718XX_REGLOCK_PWRSEQ		0x1
+#define BD718XX_REGLOCK_VREG		0x10
+
+#define BD718XX_BUCK_EN			0x01
+#define BD718XX_LDO_EN			0x40
+#define BD718XX_BUCK_SEL		0x02
+#define BD718XX_LDO_SEL			0x80
+
+#define DVS_BUCK_RUN_MASK		0x3f
+#define BD718XX_1ST_NODVS_BUCK_MASK	0x07
+#define BD718XX_3RD_NODVS_BUCK_MASK	0x07
+#define BD718XX_4TH_NODVS_BUCK_MASK	0x3f
+
+#define BD71847_BUCK3_MASK		0x07
+#define BD71847_BUCK3_RANGE_MASK	0xc0
+#define BD71847_BUCK4_MASK		0x03
+#define BD71847_BUCK4_RANGE_MASK	0x40
+
+#define BD71837_BUCK5_RANGE_MASK	0x80
+#define BD71837_BUCK6_MASK		0x03
+
+#define BD718XX_LDO1_MASK		0x03
+#define BD718XX_LDO1_RANGE_MASK		0x20
+#define BD718XX_LDO2_MASK		0x20
+#define BD718XX_LDO3_MASK		0x0f
+#define BD718XX_LDO4_MASK		0x0f
+#define BD718XX_LDO6_MASK		0x0f
+
+#define BD71837_LDO5_MASK		0x0f
+#define BD71847_LDO5_MASK		0x0f
+#define BD71847_LDO5_RANGE_MASK		0x20
+#define BD71837_LDO7_MASK		0x0f
+
 #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] 7+ messages in thread

* [U-Boot] [PATCH v2 1/2] regulator: bd71837: copy the bd71837 pmic driver from NXP imx u-boot
  2019-04-24 12:35 ` [U-Boot] [PATCH v2 1/2] regulator: bd71837: copy the bd71837 pmic driver from NXP imx u-boot Matti Vaittinen
@ 2019-04-24 23:58   ` Simon Glass
  0 siblings, 0 replies; 7+ messages in thread
From: Simon Glass @ 2019-04-24 23:58 UTC (permalink / raw)
  To: u-boot

On Wed, 24 Apr 2019 at 06:35, Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:
>
> 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>
> ---
>
> Changelog v1 => v2:
> - Drop drivers/power/pmic/pmic_bd71837.c (the old PMIC interface)
> - Fix characters in hex numbers to lowercase
>
>  drivers/power/pmic/Kconfig   |  7 +++
>  drivers/power/pmic/Makefile  |  1 +
>  drivers/power/pmic/bd71837.c | 90 ++++++++++++++++++++++++++++++++++++
>  include/power/bd71837.h      | 62 +++++++++++++++++++++++++
>  4 files changed, 160 insertions(+)
>  create mode 100644 drivers/power/pmic/bd71837.c
>  create mode 100644 include/power/bd71837.h

Reviewed-by: Simon Glass <sjg@chromium.org>

Can you drop fdtdec.h?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH v2 2/2] regulator: bd718x7: support ROHM BD71837 and BD71847 PMICs
  2019-04-24 12:37 ` [U-Boot] [PATCH v2 2/2] regulator: bd718x7: support ROHM BD71837 and BD71847 PMICs Matti Vaittinen
@ 2019-04-24 23:58   ` Simon Glass
  2019-04-25  5:58     ` Vaittinen, Matti
  0 siblings, 1 reply; 7+ messages in thread
From: Simon Glass @ 2019-04-24 23:58 UTC (permalink / raw)
  To: u-boot

HI Matti,

On Wed, 24 Apr 2019 at 06:37, Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:
>
> BD71837 and BD71847 is PMIC intended for powering single-core,
> dual-core, and quad-core SoC’s such as NXP-i.MX 8M. BD71847
> is used for example on NXP imx8mm EVK.
>
> Add regulator driver for ROHM BD71837 and BD71847 PMICs.
> BD71837 contains 8 bucks and 7 LDOS. BD71847 is reduced
> version containing 6 bucks and 6 LDOs. Voltages for DVS

This is great info and I think it should be in your Kconfig help -
i.e.a bit more detail in your description of the chip.

> bucks (1-4 on BD71837, 1 and 2 on BD71847) 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.
>
> BD718x7 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 BD71837  bucks
> 3 and 4 by default. The impact of this limitation must be
> evaluated board-by board and restrictions may need to be
> modified. (Linux driver get's these limitations from DT and we
> may want to implement same on u-Boot driver).
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
>
> Changelog v1 => v2:
> - document structs containing the platdata
> - use pmic_clrsetbits() instead of using separate reads and writes
> - fix styling issues
>
>  drivers/power/pmic/bd71837.c      |  42 ++-
>  drivers/power/regulator/Kconfig   |  15 +
>  drivers/power/regulator/Makefile  |   1 +
>  drivers/power/regulator/bd71837.c | 469 ++++++++++++++++++++++++++++++
>  include/power/bd71837.h           | 147 ++++++----
>  5 files changed, 616 insertions(+), 58 deletions(-)
>  create mode 100644 drivers/power/regulator/bd71837.c
>
> diff --git a/drivers/power/pmic/bd71837.c b/drivers/power/pmic/bd71837.c
> index b749f9430a..12c2e15a19 100644
> --- a/drivers/power/pmic/bd71837.c
> +++ b/drivers/power/pmic/bd71837.c
> @@ -3,6 +3,8 @@
>   * Copyright 2018 NXP
>   */
>
> +#define DEBUG
> +
>  #include <common.h>
>  #include <fdtdec.h>
>  #include <errno.h>
> @@ -16,15 +18,15 @@ DECLARE_GLOBAL_DATA_PTR;
>
>  static const struct pmic_child_info pmic_children_info[] = {
>         /* buck */
> -       { .prefix = "b", .driver = BD71837_REGULATOR_DRIVER},
> +       { .prefix = "b", .driver = BD718XX_REGULATOR_DRIVER},
>         /* ldo */
> -       { .prefix = "l", .driver = BD71837_REGULATOR_DRIVER},
> +       { .prefix = "l", .driver = BD718XX_REGULATOR_DRIVER},
>         { },
>  };
>
>  static int bd71837_reg_count(struct udevice *dev)
>  {
> -       return BD71837_REG_NUM;
> +       return BD718XX_MAX_REGISTER - 1;
>  }
>
>  static int bd71837_write(struct udevice *dev, uint reg, const uint8_t *buff,
> @@ -55,7 +57,7 @@ static int bd71837_bind(struct udevice *dev)
>
>         regulators_node = dev_read_subnode(dev, "regulators");
>         if (!ofnode_valid(regulators_node)) {
> -               debug("%s: %s regulators subnode not found!", __func__,
> +               debug("%s: %s regulators subnode not found!\n", __func__,
>                       dev->name);
>                 return -ENXIO;
>         }
> @@ -70,6 +72,34 @@ static int bd71837_bind(struct udevice *dev)
>         return 0;
>  }
>
> +static int bd718x7_probe(struct udevice *dev)
> +{
> +       int ret;
> +       u8 unlock;
> +
> +       /* Unlock the PMIC regulator control before probing the children */
> +       ret = pmic_reg_read(dev, BD718XX_REGLOCK);
> +       if (ret < 0) {
> +               debug("%s: %s Failed to read lock register, error %d\n",
> +                     __func__, dev->name, ret);
> +               return ret;
> +       }
> +
> +       unlock = ret;
> +       unlock &= ~(BD718XX_REGLOCK_PWRSEQ | BD718XX_REGLOCK_VREG);
> +
> +       ret = pmic_reg_write(dev, BD718XX_REGLOCK, unlock);

Can you use pmic_clrsetbits() ?

> +       if (ret) {
> +               debug("%s: %s Failed to unlock regulator control\n", __func__,
> +                     dev->name);
> +               return ret;
> +       }
> +       debug("%s: '%s' - BD718x7 PMIC register unlocked\n", __func__,
> +             dev->name);
> +
> +       return 0;
> +}
> +
>  static struct dm_pmic_ops bd71837_ops = {
>         .reg_count = bd71837_reg_count,
>         .read = bd71837_read,
> @@ -77,7 +107,8 @@ static struct dm_pmic_ops bd71837_ops = {
>  };
>
>  static const struct udevice_id bd71837_ids[] = {
> -       { .compatible = "rohm,bd71837", .data = 0x4b, },
> +       { .compatible = "rohm,bd71837", .data = ROHM_CHIP_TYPE_BD71837, },
> +       { .compatible = "rohm,bd71847", .data = ROHM_CHIP_TYPE_BD71847, },
>         { }
>  };
>
> @@ -86,5 +117,6 @@ U_BOOT_DRIVER(pmic_bd71837) = {
>         .id = UCLASS_PMIC,
>         .of_match = bd71837_ids,
>         .bind = bd71837_bind,
> +       .probe = bd718x7_probe,
>         .ops = &bd71837_ops,
>  };
> 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..060e6efe74
> --- /dev/null
> +++ b/drivers/power/regulator/bd71837.c
> @@ -0,0 +1,469 @@
> +// 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>

Drop this?

> +#include <i2c.h>
> +#include <power/pmic.h>
> +#include <power/regulator.h>
> +#include <power/bd71837.h>

Put above power/pmic to keep ordering

> +
> +#define HW_STATE_CONTROL 0
> +#define DEBUG
> +
> +/**
> + * struct bd71837_vrange - describe linear range of voltages
> + *
> + * @min_volt:  smallest voltage in range
> + * @step:      how much voltage changes at each selector step
> + * @min_sel:   smallest selector in the range
> + * @max_sel:   maximum selector in the range
> + * @rangeval:  register value used to select this range if selectible
> + *             ranges are supported
> + */
> +struct bd71837_vrange {
> +       unsigned int    min_volt;
> +       unsigned int    step;
> +       u8              min_sel;
> +       u8              max_sel;
> +       u8              rangeval;
> +};
> +
> +/**
> + * struct bd71837_platdata - describe regulator control registers
> + *
> + * @name:      name of the regulator. Used for matching the dt-entry
> + * @enable_reg:        register address used to enable/disable regulator
> + * @enablemask:        register mask used to enable/disable regulator
> + * @volt_reg:  register address used to configure regulator voltage
> + * @volt_mask: register mask used to configure regulator voltage
> + * @ranges:    pointer to ranges of regulator voltages and matching register
> + *             values
> + * @numranges: number of voltage ranges pointed by ranges
> + * @rangemask: mask for selecting used ranges if multiple ranges are supported
> + * @sel_mask:  bit to toggle in order to transfer the register control to SW
> + * @dvs:       whether the voltage can be changed when regulator is enabled
> + */
> +struct 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 dvs_buck_vranges[] = {
> +       BD_RANGE(700000, 10000, 0, 0x3c, 0),
> +       BD_RANGE(1300000, 0, 0x3d, 0x3f, 0),
> +};
> +
> +static struct bd71837_vrange bd71847_buck3_vranges[] = {
> +       BD_RANGE(700000, 100000, 0x00, 0x03, 0),
> +       BD_RANGE(1050000, 50000, 0x04, 0x05, 0),
> +       BD_RANGE(1200000, 150000, 0x06, 0x07, 0),
> +       BD_RANGE(550000, 50000, 0x0, 0x7, 0x40),
> +       BD_RANGE(675000, 100000, 0x0, 0x3, 0x80),
> +       BD_RANGE(1025000, 50000, 0x4, 0x5, 0x80),
> +       BD_RANGE(1175000, 150000, 0x6, 0x7, 0x80),
> +};
> +
> +static struct bd71837_vrange bd71847_buck4_vranges[] = {
> +       BD_RANGE(3000000, 100000, 0x00, 0x03, 0),
> +       BD_RANGE(2600000, 100000, 0x00, 0x03, 40),
> +};
> +
> +static struct bd71837_vrange bd71837_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 bd71837_buck6_vranges[] = {
> +       BD_RANGE(3000000, 100000, 0x00, 0x03, 0),
> +};
> +
> +static struct bd71837_vrange nodvs_buck3_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 nodvs_buck4_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 bd71837_ldo5_vranges[] = {
> +       BD_RANGE(1800000, 100000, 0x00, 0x0f, 0),
> +};
> +
> +static struct bd71837_vrange bd71847_ldo5_vranges[] = {
> +       BD_RANGE(1800000, 100000, 0x00, 0x0f, 0),
> +       BD_RANGE(800000, 100000, 0x00, 0x0f, 0x20),
> +};
> +
> +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_platdata bd71837_reg_data[] = {
> +/* Bucks 1-4 which support dynamic voltage scaling */
> +       BD_DATA("BUCK1", BD718XX_BUCK1_CTRL, HW_STATE_CONTROL,
> +               BD718XX_BUCK1_VOLT_RUN, DVS_BUCK_RUN_MASK, dvs_buck_vranges, 0,
> +               true, BD718XX_BUCK_SEL),
> +       BD_DATA("BUCK2", BD718XX_BUCK2_CTRL, HW_STATE_CONTROL,
> +               BD718XX_BUCK2_VOLT_RUN, DVS_BUCK_RUN_MASK, dvs_buck_vranges, 0,
> +               true, BD718XX_BUCK_SEL),
> +       BD_DATA("BUCK3", BD71837_BUCK3_CTRL, BD718XX_BUCK_EN,
> +               BD71837_BUCK3_VOLT_RUN, DVS_BUCK_RUN_MASK, dvs_buck_vranges, 0,
> +               true, BD718XX_BUCK_SEL),
> +       BD_DATA("BUCK4", BD71837_BUCK4_CTRL, BD718XX_BUCK_EN,
> +               BD71837_BUCK4_VOLT_RUN, DVS_BUCK_RUN_MASK, dvs_buck_vranges, 0,
> +               true, BD718XX_BUCK_SEL),
> +/* Bucks 5-8 which do not support dynamic voltage scaling */
> +       BD_DATA("BUCK5", BD718XX_1ST_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
> +               BD718XX_1ST_NODVS_BUCK_VOLT, BD718XX_1ST_NODVS_BUCK_MASK,
> +               bd71837_buck5_vranges, 0x80, false, BD718XX_BUCK_SEL),
> +       BD_DATA("BUCK6", BD718XX_2ND_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
> +               BD718XX_2ND_NODVS_BUCK_VOLT, BD71837_BUCK6_MASK,
> +               bd71837_buck6_vranges, 0, false, BD718XX_BUCK_SEL),
> +       BD_DATA("BUCK7", BD718XX_3RD_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
> +               BD718XX_3RD_NODVS_BUCK_VOLT, BD718XX_3RD_NODVS_BUCK_MASK,
> +               nodvs_buck3_vranges, 0, false, BD718XX_BUCK_SEL),
> +       BD_DATA("BUCK8", BD718XX_4TH_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
> +               BD718XX_4TH_NODVS_BUCK_VOLT, BD718XX_4TH_NODVS_BUCK_MASK,
> +               nodvs_buck4_vranges, 0, false, BD718XX_BUCK_SEL),
> +/* LDOs */
> +       BD_DATA("LDO1", BD718XX_LDO1_VOLT, HW_STATE_CONTROL, BD718XX_LDO1_VOLT,
> +               BD718XX_LDO1_MASK, ldo1_vranges, 0x20, false, BD718XX_LDO_SEL),
> +       BD_DATA("LDO2", BD718XX_LDO2_VOLT, HW_STATE_CONTROL, BD718XX_LDO2_VOLT,
> +               BD718XX_LDO2_MASK, ldo2_vranges, 0, false, BD718XX_LDO_SEL),
> +       BD_DATA("LDO3", BD718XX_LDO3_VOLT, HW_STATE_CONTROL, BD718XX_LDO3_VOLT,
> +               BD718XX_LDO3_MASK, ldo3_vranges, 0, false, BD718XX_LDO_SEL),
> +       BD_DATA("LDO4", BD718XX_LDO4_VOLT, HW_STATE_CONTROL, BD718XX_LDO4_VOLT,
> +               BD718XX_LDO4_MASK, ldo4_vranges, 0, false, BD718XX_LDO_SEL),
> +       BD_DATA("LDO5", BD718XX_LDO5_VOLT, HW_STATE_CONTROL, BD718XX_LDO5_VOLT,
> +               BD71837_LDO5_MASK, bd71837_ldo5_vranges, 0, false,
> +               BD718XX_LDO_SEL),
> +       BD_DATA("LDO6", BD718XX_LDO6_VOLT, HW_STATE_CONTROL, BD718XX_LDO6_VOLT,
> +               BD718XX_LDO6_MASK, ldo6_vranges, 0, false, BD718XX_LDO_SEL),
> +       BD_DATA("LDO7", BD71837_LDO7_VOLT, HW_STATE_CONTROL, BD71837_LDO7_VOLT,
> +               BD71837_LDO7_MASK, ldo7_vranges, 0, false, BD718XX_LDO_SEL),
> +};
> +
> +static struct bd71837_platdata bd71847_reg_data[] = {
> +/* Bucks 1 and 2 which support dynamic voltage scaling */
> +       BD_DATA("BUCK1", BD718XX_BUCK1_CTRL, HW_STATE_CONTROL,
> +               BD718XX_BUCK1_VOLT_RUN, DVS_BUCK_RUN_MASK, dvs_buck_vranges, 0,
> +               true, BD718XX_BUCK_SEL),
> +       BD_DATA("BUCK2", BD718XX_BUCK2_CTRL, HW_STATE_CONTROL,
> +               BD718XX_BUCK2_VOLT_RUN, DVS_BUCK_RUN_MASK, dvs_buck_vranges, 0,
> +               true, BD718XX_BUCK_SEL),
> +/* Bucks 3-6 which do not support dynamic voltage scaling */
> +       BD_DATA("BUCK3", BD718XX_1ST_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
> +               BD718XX_1ST_NODVS_BUCK_VOLT, BD718XX_1ST_NODVS_BUCK_MASK,
> +               bd71847_buck3_vranges, 0xc0, false, BD718XX_BUCK_SEL),
> +       BD_DATA("BUCK4", BD718XX_2ND_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
> +               BD718XX_2ND_NODVS_BUCK_VOLT, BD71837_BUCK6_MASK,
> +               bd71847_buck4_vranges, 0x40, false, BD718XX_BUCK_SEL),
> +       BD_DATA("BUCK5", BD718XX_3RD_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
> +               BD718XX_3RD_NODVS_BUCK_VOLT, BD718XX_3RD_NODVS_BUCK_MASK,
> +               nodvs_buck3_vranges, 0, false, BD718XX_BUCK_SEL),
> +       BD_DATA("BUCK6", BD718XX_4TH_NODVS_BUCK_CTRL, HW_STATE_CONTROL,
> +               BD718XX_4TH_NODVS_BUCK_VOLT, BD718XX_4TH_NODVS_BUCK_MASK,
> +               nodvs_buck4_vranges, 0, false, BD718XX_BUCK_SEL),
> +/* LDOs */
> +       BD_DATA("LDO1", BD718XX_LDO1_VOLT, HW_STATE_CONTROL, BD718XX_LDO1_VOLT,
> +               BD718XX_LDO1_MASK, ldo1_vranges, 0x20, false, BD718XX_LDO_SEL),
> +       BD_DATA("LDO2", BD718XX_LDO2_VOLT, HW_STATE_CONTROL, BD718XX_LDO2_VOLT,
> +               BD718XX_LDO2_MASK, ldo2_vranges, 0, false, BD718XX_LDO_SEL),
> +       BD_DATA("LDO3", BD718XX_LDO3_VOLT, HW_STATE_CONTROL, BD718XX_LDO3_VOLT,
> +               BD718XX_LDO3_MASK, ldo3_vranges, 0, false, BD718XX_LDO_SEL),
> +       BD_DATA("LDO4", BD718XX_LDO4_VOLT, HW_STATE_CONTROL, BD718XX_LDO4_VOLT,
> +               BD718XX_LDO4_MASK, ldo4_vranges, 0, false, BD718XX_LDO_SEL),
> +       BD_DATA("LDO5", BD718XX_LDO5_VOLT, HW_STATE_CONTROL, BD718XX_LDO5_VOLT,
> +               BD71847_LDO5_MASK, bd71847_ldo5_vranges, 0x20, false,
> +               BD718XX_LDO_SEL),
> +       BD_DATA("LDO6", BD718XX_LDO6_VOLT, HW_STATE_CONTROL, BD718XX_LDO6_VOLT,
> +               BD718XX_LDO6_MASK, ldo6_vranges, 0, false, BD718XX_LDO_SEL),
> +};
> +
> +static int vrange_find_value(struct bd71837_vrange *r, u8 sel,

Can you use uint instea of u8?

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

Same here

> +{
> +       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_platdata *plat = 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 (plat->enablemask == HW_STATE_CONTROL)
> +               return 1;
> +
> +       val = pmic_reg_read(dev->parent, plat->enable_reg);
> +       if (val < 0)
> +               return val;
> +
> +       return (val & plat->enablemask);
> +}
> +
> +static int bd71837_set_enable(struct udevice *dev, bool enable)
> +{
> +       int val = 0;
> +       struct bd71837_platdata *plat = 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 (plat->enablemask == HW_STATE_CONTROL)
> +               return -EINVAL;
> +
> +       if (enable)
> +               val = plat->enablemask;
> +
> +       return pmic_clrsetbits(dev->parent, plat->enable_reg, plat->enablemask,
> +                              val);
> +}
> +
> +static int bd71837_set_value(struct udevice *dev, int uvolt)
> +{
> +       u8 sel;
> +       u8 range;

and here

> +       int i;
> +       int not_found = 1;

I think the logic would be easier if you used 'found'

> +       struct bd71837_platdata *plat = 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 (!plat->dvs)
> +               if (bd71837_get_enable(dev)) {
> +                       pr_err("Only DVS bucks can be changed when enabled\n");
> +                       return -EINVAL;
> +               }
> +
> +       for (i = 0; i < plat->numranges; i++) {
> +               struct bd71837_vrange *r = &plat->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(plat->volt_mask) - 1;
> +
> +       if (plat->rangemask)
> +               sel |= range;
> +
> +       return pmic_clrsetbits(dev->parent, plat->volt_reg, plat->volt_mask |
> +                              plat->rangemask, sel);
> +}
> +
> +static int bd71837_get_value(struct udevice *dev)
> +{
> +       u8 reg, range;

and here

> +       unsigned int tmp;
> +       struct bd71837_platdata *plat = dev_get_platdata(dev);
> +       int i;
> +

[..]

Regards,
Simon

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH v2 2/2] regulator: bd718x7: support ROHM BD71837 and BD71847 PMICs
  2019-04-24 23:58   ` Simon Glass
@ 2019-04-25  5:58     ` Vaittinen, Matti
  2019-05-06 19:50       ` Simon Glass
  0 siblings, 1 reply; 7+ messages in thread
From: Vaittinen, Matti @ 2019-04-25  5:58 UTC (permalink / raw)
  To: u-boot

Hello Simon and thanks again for taking the time to check this =)

On Wed, 2019-04-24 at 17:58 -0600, Simon Glass wrote:
> HI Matti,
> 
> On Wed, 24 Apr 2019 at 06:37, Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> > 
> > BD71837 and BD71847 is PMIC intended for powering single-core,
> > dual-core, and quad-core SoC’s such as NXP-i.MX 8M. BD71847
> > is used for example on NXP imx8mm EVK.
> > 
> > Add regulator driver for ROHM BD71837 and BD71847 PMICs.
> > BD71837 contains 8 bucks and 7 LDOS. BD71847 is reduced
> > version containing 6 bucks and 6 LDOs. Voltages for DVS
> 
> This is great info and I think it should be in your Kconfig help -
> i.e.a bit more detail in your description of the chip.

Good idea. I'll do so in the next version.

> > +static int bd718x7_probe(struct udevice *dev)
> > +{
> > +       int ret;
> > +       u8 unlock;
> > +
> > +       /* Unlock the PMIC regulator control before probing the
> > children */
> > +       ret = pmic_reg_read(dev, BD718XX_REGLOCK);
> > +       if (ret < 0) {
> > +               debug("%s: %s Failed to read lock register, error
> > %d\n",
> > +                     __func__, dev->name, ret);
> > +               return ret;
> > +       }
> > +
> > +       unlock = ret;
> > +       unlock &= ~(BD718XX_REGLOCK_PWRSEQ | BD718XX_REGLOCK_VREG);
> > +
> > +       ret = pmic_reg_write(dev, BD718XX_REGLOCK, unlock);
> 
> Can you use pmic_clrsetbits() ?

Sure. I'll fix this too. Makes this much nicer.

> > index 0000000000..060e6efe74
> > --- /dev/null
> > +++ b/drivers/power/regulator/bd71837.c
> > @@ -0,0 +1,469 @@
> > +// 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>
> 
> Drop this?

errno.h? I return -EINVAL from few of the functions. Or do you mean
i2c.h? I think that can be dropped, thanks.

> 
> > +#include <i2c.h>
> > +#include <power/pmic.h>
> > +#include <power/regulator.h>
> > +#include <power/bd71837.h>
> 
> Put above power/pmic to keep ordering

I'll do that.

> > 
> > +static int vrange_find_value(struct bd71837_vrange *r, u8 sel,
> 
> Can you use uint instea of u8?

I'll replace u8 with uint8_t for all occurrences in this file. I
personally prefer uint8_t. I've got this u8 infection from the linux
kernel code where u8 seems to be preferred =)

> > +
> > +static int bd71837_set_value(struct udevice *dev, int uvolt)
> > +{
> > +       u8 sel;
> > +       u8 range;
> 
> and here
> 
> > +       int i;
> > +       int not_found = 1;
> 
> I think the logic would be easier if you used 'found'

I see your point =) not_found became not_found just because return
value 0 from vrange_find_selector() (or pretty much any other function
I write) means success. So direct assignment to variable made it
'not_found' :] But "double negation" (!not_....) is indeed a bit
difficult. I'll change this too.

> 
> > +       struct bd71837_platdata *plat = 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 (!plat->dvs)
> > +               if (bd71837_get_enable(dev)) {
> > +                       pr_err("Only DVS bucks can be changed when
> > enabled\n");
> > +                       return -EINVAL;
> > +               }
> > +
> > +       for (i = 0; i < plat->numranges; i++) {
> > +               struct bd71837_vrange *r = &plat->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(plat->volt_mask) - 1;
> > +
> > +       if (plat->rangemask)
> > +               sel |= range;
> > +
> > +       return pmic_clrsetbits(dev->parent, plat->volt_reg, plat-
> > >volt_mask |
> > +                              plat->rangemask, sel);
> > +}

Best Regards
	Matti Vaittinen

--
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 ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [U-Boot] [PATCH v2 2/2] regulator: bd718x7: support ROHM BD71837 and BD71847 PMICs
  2019-04-25  5:58     ` Vaittinen, Matti
@ 2019-05-06 19:50       ` Simon Glass
  0 siblings, 0 replies; 7+ messages in thread
From: Simon Glass @ 2019-05-06 19:50 UTC (permalink / raw)
  To: u-boot

Hi Matti,

On Wed, 24 Apr 2019 at 23:58, Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
>
> Hello Simon and thanks again for taking the time to check this =)
>
> On Wed, 2019-04-24 at 17:58 -0600, Simon Glass wrote:
> > HI Matti,
> >
> > On Wed, 24 Apr 2019 at 06:37, Matti Vaittinen
> > <matti.vaittinen@fi.rohmeurope.com> wrote:
> > >
> > > BD71837 and BD71847 is PMIC intended for powering single-core,
> > > dual-core, and quad-core SoC’s such as NXP-i.MX 8M. BD71847
> > > is used for example on NXP imx8mm EVK.
> > >
> > > Add regulator driver for ROHM BD71837 and BD71847 PMICs.
> > > BD71837 contains 8 bucks and 7 LDOS. BD71847 is reduced
> > > version containing 6 bucks and 6 LDOs. Voltages for DVS
> >
> > This is great info and I think it should be in your Kconfig help -
> > i.e.a bit more detail in your description of the chip.
>
> Good idea. I'll do so in the next version.
>
> > > +static int bd718x7_probe(struct udevice *dev)
> > > +{
> > > +       int ret;
> > > +       u8 unlock;
> > > +
> > > +       /* Unlock the PMIC regulator control before probing the
> > > children */
> > > +       ret = pmic_reg_read(dev, BD718XX_REGLOCK);
> > > +       if (ret < 0) {
> > > +               debug("%s: %s Failed to read lock register, error
> > > %d\n",
> > > +                     __func__, dev->name, ret);
> > > +               return ret;
> > > +       }
> > > +
> > > +       unlock = ret;
> > > +       unlock &= ~(BD718XX_REGLOCK_PWRSEQ | BD718XX_REGLOCK_VREG);
> > > +
> > > +       ret = pmic_reg_write(dev, BD718XX_REGLOCK, unlock);
> >
> > Can you use pmic_clrsetbits() ?
>
> Sure. I'll fix this too. Makes this much nicer.
>
> > > index 0000000000..060e6efe74
> > > --- /dev/null
> > > +++ b/drivers/power/regulator/bd71837.c
> > > @@ -0,0 +1,469 @@
> > > +// 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>
> >
> > Drop this?
>
> errno.h? I return -EINVAL from few of the functions. Or do you mean
> i2c.h? I think that can be dropped, thanks.

I mean that errno.h should be included already?

>
> >
> > > +#include <i2c.h>
> > > +#include <power/pmic.h>
> > > +#include <power/regulator.h>
> > > +#include <power/bd71837.h>
> >
> > Put above power/pmic to keep ordering
>
> I'll do that.
>
> > >
> > > +static int vrange_find_value(struct bd71837_vrange *r, u8 sel,
> >
> > Can you use uint instea of u8?
>
> I'll replace u8 with uint8_t for all occurrences in this file. I
> personally prefer uint8_t. I've got this u8 infection from the linux
> kernel code where u8 seems to be preferred =)

No, u8 is preferred over uint8_t.

I mean that you shouldn't be using u8 for arguments. You should use
uint (unsigned int).

>
> > > +
> > > +static int bd71837_set_value(struct udevice *dev, int uvolt)
> > > +{
> > > +       u8 sel;
> > > +       u8 range;
> >
> > and here
> >
> > > +       int i;
> > > +       int not_found = 1;
> >
> > I think the logic would be easier if you used 'found'
>
> I see your point =) not_found became not_found just because return
> value 0 from vrange_find_selector() (or pretty much any other function
> I write) means success. So direct assignment to variable made it
> 'not_found' :] But "double negation" (!not_....) is indeed a bit
> difficult. I'll change this too.

[..]

> Simon says - in Latin please.
> "non cogito me" dixit Rene Descarte, deinde evanescavit
>
> (Thanks for the translation Simon)

I hope it is close :-)

- Simon

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-05-06 19:50 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-24 12:33 [U-Boot] [PATCH v2 0/2] support for ROHM BD71837 and BD71847 PMICs Matti Vaittinen
2019-04-24 12:35 ` [U-Boot] [PATCH v2 1/2] regulator: bd71837: copy the bd71837 pmic driver from NXP imx u-boot Matti Vaittinen
2019-04-24 23:58   ` Simon Glass
2019-04-24 12:37 ` [U-Boot] [PATCH v2 2/2] regulator: bd718x7: support ROHM BD71837 and BD71847 PMICs Matti Vaittinen
2019-04-24 23:58   ` Simon Glass
2019-04-25  5:58     ` Vaittinen, Matti
2019-05-06 19:50       ` Simon Glass

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.