[PATCH v1 0/4] pmbus: read back the VOUT_COMMAND setpoint, fix the nbxv3 VID handoff

[PATCH v1 0/4] pmbus: read back the VOUT_COMMAND setpoint, fix the nbxv3 VID handoff

[Vincent Jardin]

On nbxv3 the VID handoff compared READ_VOUT with the fused core
voltage and raised VOUT_COMMAND by the difference. READ_VOUT sits
below the setpoint by the load line (droop) the MPQ8646 applies from
its NVM, so a module fused 0x14 (850 mV) got 875.8 mV: the core ran
above its fused voltage.

The handoff has to compare and verify the setpoint instead. PMBus
exposes it as VOUT_COMMAND, which regulator_get_value() does not
return:

- patch 1 makes the exported PMBus regulator helpers check that the
  device is a PMBus regulator
- patch 2 adds pmbus_regulator_read_vout_command()
- patch 3 tests it on sandbox, the patch 1 check included
- patch 4 makes the nbxv3 handoff write the fused table value as is
  and verify it through VOUT_COMMAND.


Vincent Jardin (4):
  power: regulator: pmbus: check the device in the exported helpers
  power: regulator: pmbus: read back the VOUT_COMMAND setpoint
  test: dm: pmbus: test pmbus_regulator_read_vout_command()
  board: nbxv3: VID handoff writes table value, no droop compensation

 board/nxp/lx2160a/nbxv3/vid_handoff.c  | 77 ++++++++++----------------
 drivers/power/regulator/pmbus_helper.c | 64 +++++++++++++++++----
 include/pmbus.h                        | 14 +++++
 test/dm/pmbus.c                        | 41 ++++++++++++++
 4 files changed, 137 insertions(+), 59 deletions(-)

---
base-commit: 2b8902913c19eb581fb0d567201c09b8fa46ac74
branch: for-upstream/pmbus-vout-command-v1

-- 
2.43.0

[PATCH v1 1/4] power: regulator: pmbus: check the device in the exported helpers

[Vincent Jardin]

pmbus_regulator_read_temp() takes any struct udevice and reads its
private data as a struct pmbus_regulator_priv.

Add a pmbus_regulator_dev_priv() in order to return the private data of
a device for which driver uses pmbus_regulator_ops, the ops every PMBus
chip driver installs, and use it in pmbus_regulator_read_temp().

Signed-off-by: Vincent Jardin <vjardin@free.fr>

---

 drivers/power/regulator/pmbus_helper.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/power/regulator/pmbus_helper.c b/drivers/power/regulator/pmbus_helper.c
index 4763470442d..1809c6cdfb5 100644
--- a/drivers/power/regulator/pmbus_helper.c
+++ b/drivers/power/regulator/pmbus_helper.c
@@ -144,6 +144,15 @@ const struct dm_regulator_ops pmbus_regulator_ops = {
 	.set_enable = pmbus_regulator_set_enable,
 };
 
+static struct pmbus_regulator_priv *
+pmbus_regulator_dev_priv(struct udevice *dev)
+{
+	if (!dev || !dev->driver || dev->driver->ops != &pmbus_regulator_ops)
+		return NULL;
+
+	return dev_get_priv(dev);
+}
+
 int pmbus_regulator_read_temp(struct udevice *reg_dev, int *temp_mc)
 {
 	struct pmbus_regulator_priv *priv;
@@ -153,7 +162,7 @@ int pmbus_regulator_read_temp(struct udevice *reg_dev, int *temp_mc)
 
 	if (!reg_dev || !temp_mc)
 		return -EINVAL;
-	priv = dev_get_priv(reg_dev);
+	priv = pmbus_regulator_dev_priv(reg_dev);
 	if (!priv || !priv->i2c_dev)
 		return -ENODEV;
 
-- 
2.43.0

[PATCH v1 2/4] power: regulator: pmbus: read back the VOUT_COMMAND setpoint

[Vincent Jardin]

regulator_get_value() returns READ_VOUT, the output as measured.

On a regulator with a load line (droop) configured, that sits below the
setpoint under load, so a caller cannot tell from it what
regulator_set_value() last wrote, nor whether the setpoint already
has the value it wants.

Add pmbus_regulator_read_vout_command(): let's read VOUT_COMMAND (21h) and
decode it exactly like READ_VOUT, through the chip's pmbus_driver_info and
VOUT_MODE, without exposing pmbus_regulator_priv.

Signed-off-by: Vincent Jardin <vjardin@free.fr>
---

 drivers/power/regulator/pmbus_helper.c | 53 ++++++++++++++++++++------
 include/pmbus.h                        | 14 +++++++
 2 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/drivers/power/regulator/pmbus_helper.c b/drivers/power/regulator/pmbus_helper.c
index 1809c6cdfb5..6c76ee91c9c 100644
--- a/drivers/power/regulator/pmbus_helper.c
+++ b/drivers/power/regulator/pmbus_helper.c
@@ -34,12 +34,28 @@ static int pmbus_regulator_select_page(struct pmbus_regulator_priv *priv)
 	return dm_i2c_write(priv->i2c_dev, PMBUS_PAGE, &p, 1);
 }
 
+static int pmbus_regulator_vout_to_uv(struct pmbus_regulator_priv *priv,
+				      u16 raw, u8 vout_mode)
+{
+	s64 uv;
+
+	if (priv->info)
+		uv = pmbus_reg2data(priv->info, PSC_VOLTAGE_OUT, raw, vout_mode);
+	else
+		uv = pmbus_reg2data_linear16(raw, vout_mode);
+
+	if (uv > INT_MAX)
+		uv = INT_MAX;
+	if (uv < INT_MIN)
+		uv = INT_MIN;
+	return (int)uv;
+}
+
 static int pmbus_regulator_get_value(struct udevice *dev)
 {
 	struct pmbus_regulator_priv *priv = dev_get_priv(dev);
 	u8 vout_mode = 0;
 	u16 raw = 0;
-	s64 uv;
 	int ret;
 
 	ret = pmbus_regulator_select_page(priv);
@@ -49,16 +65,7 @@ static int pmbus_regulator_get_value(struct udevice *dev)
 	if (pmbus_read_word(priv->i2c_dev, PMBUS_READ_VOUT, &raw))
 		return -EIO;
 
-	if (priv->info)
-		uv = pmbus_reg2data(priv->info, PSC_VOLTAGE_OUT, raw, vout_mode);
-	else
-		uv = pmbus_reg2data_linear16(raw, vout_mode);
-
-	if (uv > INT_MAX)
-		uv = INT_MAX;
-	if (uv < INT_MIN)
-		uv = INT_MIN;
-	return (int)uv;
+	return pmbus_regulator_vout_to_uv(priv, raw, vout_mode);
 }
 
 static int pmbus_regulator_set_value(struct udevice *dev, int uV)
@@ -187,6 +194,30 @@ int pmbus_regulator_read_temp(struct udevice *reg_dev, int *temp_mc)
 	return 0;
 }
 
+int pmbus_regulator_read_vout_command(struct udevice *reg_dev, int *uV)
+{
+	struct pmbus_regulator_priv *priv;
+	u8 vout_mode = 0;
+	u16 raw = 0;
+	int ret;
+
+	if (!reg_dev || !uV)
+		return -EINVAL;
+	priv = pmbus_regulator_dev_priv(reg_dev);
+	if (!priv || !priv->i2c_dev)
+		return -ENODEV;
+
+	ret = pmbus_regulator_select_page(priv);
+	if (ret)
+		return ret;
+	if (pmbus_read_byte(priv->i2c_dev, PMBUS_VOUT_MODE, &vout_mode) ||
+	    pmbus_read_word(priv->i2c_dev, PMBUS_VOUT_COMMAND, &raw))
+		return -EIO;
+
+	*uV = pmbus_regulator_vout_to_uv(priv, raw, vout_mode);
+	return 0;
+}
+
 enum pmbus_data_format
 pmbus_regulator_identify_vout(struct udevice *i2c_dev,
 			      struct pmbus_driver_info *info)
diff --git a/include/pmbus.h b/include/pmbus.h
index bb85a61e62a..e91d97a0b10 100644
--- a/include/pmbus.h
+++ b/include/pmbus.h
@@ -466,6 +466,20 @@ void pmbus_print_status_word(struct udevice *chip);
  */
 int pmbus_regulator_read_temp(struct udevice *reg_dev, int *temp_mc);
 
+/*
+ * pmbus_regulator_read_vout_command() - the output voltage setpoint
+ *
+ * Read VOUT_COMMAND (21h), decoded like READ_VOUT through the chip's
+ * pmbus_driver_info and VOUT_MODE. regulator_get_value() returns
+ * READ_VOUT, the output as measured, which on a regulator with a load
+ * line (droop) sits below the setpoint under load; this returns the
+ * setpoint itself, which is what regulator_set_value() writes.
+ *
+ * Returns 0 on success, or a negative errno (-ENODEV if reg is not a
+ * probed pmbus regulator, -EIO on bus error).
+ */
+int pmbus_regulator_read_vout_command(struct udevice *reg_dev, int *uV);
+
 /*
  * Look up the pmbus_driver_info of a probed UCLASS_REGULATOR device at
  * (bus_seq, addr) that is driven by a pmbus_helper based chip driver
-- 
2.43.0

[PATCH v1 3/4] test: dm: pmbus: test pmbus_regulator_read_vout_command()

[Vincent Jardin]

Read the setpoint of the emulated chip (VOUT_MODE 0x18, VOUT_COMMAND
0x0200: 2 V), write a new one with regulator_set_value() and read it
back, and check the -EINVAL and -ENODEV cases.

Signed-off-by: Vincent Jardin <vjardin@free.fr>
---

 test/dm/pmbus.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/test/dm/pmbus.c b/test/dm/pmbus.c
index 0184b201829..a2ef6b7b7d5 100644
--- a/test/dm/pmbus.c
+++ b/test/dm/pmbus.c
@@ -10,6 +10,8 @@
 #include <i2c.h>
 #include <pmbus.h>
 #include <dm/test.h>
+#include <power/regulator.h>
+#include <power/sandbox_pmic.h>
 #include <test/test.h>
 #include <test/ut.h>
 
@@ -190,6 +192,45 @@ static int dm_test_pmbus_vout(struct unit_test_state *uts)
 
 DM_TEST(dm_test_pmbus_vout, UTF_SCAN_FDT | UTF_CONSOLE);
 
+/* pmbus_regulator_read_vout_command() returns the setpoint, VOUT_COMMAND */
+static int dm_test_pmbus_vout_command(struct unit_test_state *uts)
+{
+	struct udevice *dev, *other;
+	int uv;
+
+	ut_assertok(uclass_get_device_by_name(UCLASS_REGULATOR, "pmbus@70",
+					      &dev));
+
+	/* VOUT_MODE 0x18 (LINEAR16, 2^-8) and VOUT_COMMAND 0x0200: 2 V */
+	ut_assertok(pmbus_regulator_read_vout_command(dev, &uv));
+	ut_asserteq(2000000, uv);
+
+	/* regulator_set_value() writes VOUT_COMMAND, read it back */
+	ut_assertok(regulator_set_value(dev, 1500000));
+	ut_assertok(pmbus_regulator_read_vout_command(dev, &uv));
+	ut_asserteq(1500000, uv);
+
+	ut_asserteq(-EINVAL, pmbus_regulator_read_vout_command(dev, NULL));
+
+	/*
+	 * A device that is not a PMBus regulator is refused: another
+	 * regulator, and the chip emulator, whose private data (the
+	 * register file) must not be read as a pmbus_regulator_priv.
+	 */
+	ut_assertok(uclass_get_device_by_name(UCLASS_REGULATOR,
+					      SANDBOX_BUCK1_DEVNAME, &other));
+	ut_asserteq(-ENODEV, pmbus_regulator_read_vout_command(other, &uv));
+	ut_asserteq(-ENODEV, pmbus_regulator_read_temp(other, &uv));
+	ut_assertok(uclass_get_device_by_name(UCLASS_I2C_EMUL, "emul-pmbus",
+					      &other));
+	ut_asserteq(-ENODEV, pmbus_regulator_read_vout_command(other, &uv));
+	ut_asserteq(-ENODEV, pmbus_regulator_read_temp(other, &uv));
+
+	return 0;
+}
+
+DM_TEST(dm_test_pmbus_vout_command, UTF_SCAN_FDT);
+
 /* pmbus dump walks every standard register; spot-check one line */
 static int dm_test_pmbus_dump(struct unit_test_state *uts)
 {
-- 
2.43.0

[PATCH v1 4/4] board: nbxv3: VID handoff writes table value, no droop compensation

[Vincent Jardin]

The handoff compared READ_VOUT with the fused voltage, and when the
rail read low it wrote the table value, then a second VOUT_COMMAND
raised by the measured shortfall:
  cmd = target + (target - rail).

On a module fused 0x14 (850 mV) the loaded rail reads 824.2 mV, so it wrote
875.8 mV:

  VID handoff fuse=0x14 rail 824218 uV, target 850 mV -> write 850000 uV
  VID handoff verified 849609 uV (target 850000 uV, cmd 875782 uV after 1 correction)

The 25 mV is the load line the MPQ8646 applies from its NVM
(MFR_DROOP_CFG). It is the VR's design, not an error to correct, and
compensating it in software runs the core above its fused voltage.

Do what the VR expects: map the fuse through the LX2160ARM table and
write that value, unaltered, to VOUT_COMMAND. Compare the setpoint
(VOUT_COMMAND), never the loaded rail, to decide whether to write, and
read the setpoint back to verify.

READ_VOUT is printed for information only.

A module whose VR boots with a setpoint above its fuse, e.g. 850 mV on
a 0x12 (825 mV) part, now gets 825 mV written. The previous code left
it untouched, because the loaded rail happened to read 824.2 mV.

Reported-by: Nicolas Perrin <nperrin@freebox.fr>
Signed-off-by: Vincent Jardin <vjardin@free.fr>
---

 board/nxp/lx2160a/nbxv3/vid_handoff.c | 77 +++++++++++----------------
 1 file changed, 30 insertions(+), 47 deletions(-)

diff --git a/board/nxp/lx2160a/nbxv3/vid_handoff.c b/board/nxp/lx2160a/nbxv3/vid_handoff.c
index d3edaa61080..65f2814b30d 100644
--- a/board/nxp/lx2160a/nbxv3/vid_handoff.c
+++ b/board/nxp/lx2160a/nbxv3/vid_handoff.c
@@ -11,6 +11,7 @@
 #include <event.h>
 #include <linux/delay.h>
 #include <log.h>
+#include <pmbus.h>
 #include <vsprintf.h>
 #include <asm/io.h>
 #include <asm/arch/immap_lsch3.h>
@@ -40,27 +41,13 @@ static const u16 lx2160a_vid_table[32] = {
 	[0x14] = 8500,
 };
 
-/*
- * Write VOUT_COMMAND, let the slew finish, read the rail back.
- * Returns the rail in uV, or a negative errno from the write.
- */
-static int nbxv3_vid_write_readback(struct udevice *reg, int cmd_uV)
-{
-	int ret = regulator_set_value(reg, cmd_uV);
-
-	if (ret)
-		return ret;
-	udelay(NBXV3_VID_SLEW_SETTLE_US);
-	return regulator_get_value(reg);
-}
-
 static int nbxv3_vid_handoff(void)
 {
 	struct ccsr_gur *gur = (void *)(CFG_SYS_FSL_GUTS_ADDR);
 	struct udevice *reg;
 	u32 fusesr;
 	u8 vid;
-	int target_mv, target_uV, actual_uV, cmd_uV;
+	int target_mv, target_uV, rail_uV, set_uV;
 	char *override_str;
 	unsigned long override_mv;
 	int ret;
@@ -98,46 +85,42 @@ static int nbxv3_vid_handoff(void)
 	}
 
 	target_uV = target_mv * 1000;
-	actual_uV = regulator_get_value(reg);
-
-	if (abs(actual_uV - target_uV) <= NBXV3_VID_LSB_uV) {
-		printf("MPQ8785 (%s): VID handoff fuse=0x%02x target=%d mV (rail already at %d uV, untouched)\n",
-		       NBXV3_VDD_REGULATOR_NAME, vid, target_mv, actual_uV);
+	rail_uV = regulator_get_value(reg);
+
+	/*
+	 * Compare the setpoint, VOUT_COMMAND, with the table value, never
+	 * the rail. READ_VOUT sits below the setpoint under load, by the
+	 * load line the VR applies from its NVM (MFR_DROOP_CFG): that is
+	 * the VR's design, not an error to correct.
+	 * Writing the setpoint higher to bring READ_VOUT onto the fuse
+	 * drives the core above its fused voltage. Don't do it !
+	 */
+	if (!pmbus_regulator_read_vout_command(reg, &set_uV) &&
+	    abs(set_uV - target_uV) <= NBXV3_VID_LSB_uV) {
+		printf("MPQ8785 (%s): VID handoff fuse=0x%02x target=%d mV, VOUT_COMMAND already %d uV (rail %d uV), untouched\n",
+		       NBXV3_VDD_REGULATOR_NAME, vid, target_mv, set_uV,
+		       rail_uV);
 		return 0;
 	}
 
-	printf("MPQ8785 (%s): VID handoff fuse=0x%02x rail %d uV, target %d mV -> write %d uV\n",
-	       NBXV3_VDD_REGULATOR_NAME, vid, actual_uV, target_mv, target_uV);
-
-	ret = nbxv3_vid_write_readback(reg, target_uV);
-	if (ret < 0) {
+	/* The table value, as is: the VR corrects internally */
+	ret = regulator_set_value(reg, target_uV);
+	if (ret) {
 		printf("WARNING: MPQ8785 (%s): VID handoff set_value(%d uV) failed (%d), rail left at %d uV\n",
-		       NBXV3_VDD_REGULATOR_NAME, target_uV, ret, actual_uV);
-		return 0;
-	}
-	actual_uV = ret;
-	if (abs(actual_uV - target_uV) <= NBXV3_VID_LSB_uV) {
-		printf("MPQ8785 (%s): VID handoff verified %d uV (target %d uV, cmd %d uV)\n",
-		       NBXV3_VDD_REGULATOR_NAME, actual_uV, target_uV, target_uV);
+		       NBXV3_VDD_REGULATOR_NAME, target_uV, ret, rail_uV);
 		return 0;
 	}
+	udelay(NBXV3_VID_SLEW_SETTLE_US);
+	rail_uV = regulator_get_value(reg);
 
-	cmd_uV = target_uV + (target_uV - actual_uV);
-	ret = nbxv3_vid_write_readback(reg, cmd_uV);
-	if (ret < 0) {
-		printf("WARNING: MPQ8785 (%s): VID handoff correction set_value(%d uV) failed (%d), rail left at %d uV\n",
-		       NBXV3_VDD_REGULATOR_NAME, cmd_uV, ret, actual_uV);
-		return 0;
-	}
-	actual_uV = ret;
-	if (abs(actual_uV - target_uV) <= NBXV3_VID_LSB_uV)
-		printf("MPQ8785 (%s): VID handoff verified %d uV (target %d uV, cmd %d uV after 1 correction)\n",
-		       NBXV3_VDD_REGULATOR_NAME, actual_uV, target_uV, cmd_uV);
+	ret = pmbus_regulator_read_vout_command(reg, &set_uV);
+	if (ret || abs(set_uV - target_uV) > NBXV3_VID_LSB_uV)
+		printf("WARNING: MPQ8785 (%s): VID handoff wrote %d uV, VOUT_COMMAND reads %d uV (%d)\n",
+		       NBXV3_VDD_REGULATOR_NAME, target_uV, ret ? 0 : set_uV,
+		       ret);
 	else
-		printf("WARNING: MPQ8785 (%s): VID handoff rail %d uV %s target %d uV after correction (cmd %d uV) -- VOUT_MAX / non-linear load line, fix on the VR side\n",
-		       NBXV3_VDD_REGULATOR_NAME, actual_uV,
-		       actual_uV < target_uV ? "below" : "above", target_uV,
-		       cmd_uV);
+		printf("MPQ8785 (%s): VID handoff fuse=0x%02x VOUT_COMMAND set to %d uV (rail %d uV under load)\n",
+		       NBXV3_VDD_REGULATOR_NAME, vid, set_uV, rail_uV);
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH v1 0/4] pmbus: read back the VOUT_COMMAND setpoint, fix the nbxv3 VID handoff

[Vincent Jardin]

Hi Peng,

Regarding this serie too:
  https://patchwork.ozlabs.org/project/uboot/list/?series=525625
it is a gentle ping.

Without patch 4, the VID handoff could run the core above its fused
voltage.

Please, could you have a review when you have time ?

Thank you,
  Vincent