[PATCH 0/6] Use correct LDO5 control registers for PCA9450

[PATCH 0/6] Use correct LDO5 control registers for PCA9450

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

This patchset fixes the control of the LDO5 regulator by providing an
option for letting the driver know which of the two possible control
registers is currently in use by the hardware.

It also fixes the enable register for LDO5 to use PCA9450_REG_LDO5CTRL_L
as specified by the datasheet.

The last patch makes use of the fix by adjusting the devicetree for
the Kontron i.MX8MM boards.

In Linux this currently doesn't fix any functional issues, but in
U-Boot similar changes are needed in order to fix SD card access.
See the following thread for more information:

https://lists.denx.de/pipermail/u-boot/2023-January/506103.html

Frieder Schrempf (6):
  dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios
  regulator: pca9450: Fix enable register for LDO5
  Revert "regulator: pca9450: Add SD_VSEL GPIO for LDO5"
  regulator: Add operation to let drivers select vsel register
  regulator: pca9450: Fix control register for LDO5
  arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

 .../regulator/nxp,pca9450-regulator.yaml      | 23 ++++++---
 .../dts/freescale/imx8mm-kontron-bl-osm-s.dts |  6 +--
 .../boot/dts/freescale/imx8mm-kontron-bl.dts  |  6 +--
 .../dts/freescale/imx8mm-kontron-osm-s.dtsi   |  1 +
 .../boot/dts/freescale/imx8mm-kontron-sl.dtsi |  1 +
 drivers/regulator/helpers.c                   | 16 ++++++-
 drivers/regulator/pca9450-regulator.c         | 47 ++++++++++++++-----
 include/linux/regulator/driver.h              |  5 ++
 8 files changed, 79 insertions(+), 26 deletions(-)

-- 
2.39.1

[PATCH 1/6] dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

The sd-vsel-gpios property is abandoned in its current meaning as an
output. We now use it to specify an optional signal that can be
evaluated by the driver in order to retrieve the current status
of the SD_VSEL signal that is used to select the control register
of LDO5.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 .../regulator/nxp,pca9450-regulator.yaml      | 23 ++++++++++++++-----
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
index 835b53302db8..c86534538a4e 100644
--- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
@@ -40,8 +40,24 @@ properties:
     description: |
       list of regulators provided by this controller
 
+    properties:
+      LDO5:
+        type: object
+        $ref: regulator.yaml#
+        description:
+          Properties for single LDO5 regulator.
+
+        properties:
+          sd-vsel-gpios:
+            description:
+              GPIO that can be used to read the current status of the SD_VSEL
+              signal in order for the driver to know if LDO5CTRL_L or LDO5CTRL_H
+              is used by the hardware.
+
+        unevaluatedProperties: false
+
     patternProperties:
-      "^LDO[1-5]$":
+      "^LDO[1-4]$":
         type: object
         $ref: regulator.yaml#
         description:
@@ -76,11 +92,6 @@ properties:
 
     additionalProperties: false
 
-  sd-vsel-gpios:
-    description: GPIO that is used to switch LDO5 between being configured by
-      LDO5CTRL_L or LDO5CTRL_H register. Use this if the SD_VSEL signal is
-      connected to a host GPIO.
-
   nxp,i2c-lt-enable:
     type: boolean
     description:
-- 
2.39.1

[PATCH 2/6] regulator: pca9450: Fix enable register for LDO5

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

The LDO5 regulator has two configuration registers, but only
LDO5CTRL_L contains the bits for enabling/disabling the regulator.

Fixes: 0935ff5f1f0a ("regulator: pca9450: add pca9450 pmic driver")
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/regulator/pca9450-regulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c
index c6351fac9f4d..a815666566b5 100644
--- a/drivers/regulator/pca9450-regulator.c
+++ b/drivers/regulator/pca9450-regulator.c
@@ -447,7 +447,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
 			.n_linear_ranges = ARRAY_SIZE(pca9450_ldo5_volts),
 			.vsel_reg = PCA9450_REG_LDO5CTRL_H,
 			.vsel_mask = LDO5HOUT_MASK,
-			.enable_reg = PCA9450_REG_LDO5CTRL_H,
+			.enable_reg = PCA9450_REG_LDO5CTRL_L,
 			.enable_mask = LDO5H_EN_MASK,
 			.owner = THIS_MODULE,
 		},
@@ -656,7 +656,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
 			.n_linear_ranges = ARRAY_SIZE(pca9450_ldo5_volts),
 			.vsel_reg = PCA9450_REG_LDO5CTRL_H,
 			.vsel_mask = LDO5HOUT_MASK,
-			.enable_reg = PCA9450_REG_LDO5CTRL_H,
+			.enable_reg = PCA9450_REG_LDO5CTRL_L,
 			.enable_mask = LDO5H_EN_MASK,
 			.owner = THIS_MODULE,
 		},
-- 
2.39.1

[PATCH 3/6] Revert "regulator: pca9450: Add SD_VSEL GPIO for LDO5"

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

This reverts commit 8c67a11bae889f51fe5054364c3c789dfae3ad73.

It turns out that all boards using the PCA9450 actually have the
SD_VSEL input conencted to the VSELECT signal of the SoCs SD/MMC
interface. Therefore we don't need manual control for this signal
via GPIO and threre aren't any users.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/regulator/pca9450-regulator.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c
index a815666566b5..804a22c0e376 100644
--- a/drivers/regulator/pca9450-regulator.c
+++ b/drivers/regulator/pca9450-regulator.c
@@ -5,7 +5,6 @@
  */
 
 #include <linux/err.h>
-#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -33,7 +32,6 @@ struct pca9450_regulator_desc {
 struct pca9450 {
 	struct device *dev;
 	struct regmap *regmap;
-	struct gpio_desc *sd_vsel_gpio;
 	enum pca9450_chip_type type;
 	unsigned int rcnt;
 	int irq;
@@ -834,18 +832,6 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
 		}
 	}
 
-	/*
-	 * The driver uses the LDO5CTRL_H register to control the LDO5 regulator.
-	 * This is only valid if the SD_VSEL input of the PMIC is high. Let's
-	 * check if the pin is available as GPIO and set it to high.
-	 */
-	pca9450->sd_vsel_gpio = gpiod_get_optional(pca9450->dev, "sd-vsel", GPIOD_OUT_HIGH);
-
-	if (IS_ERR(pca9450->sd_vsel_gpio)) {
-		dev_err(&i2c->dev, "Failed to get SD_VSEL GPIO\n");
-		return PTR_ERR(pca9450->sd_vsel_gpio);
-	}
-
 	dev_info(&i2c->dev, "%s probed.\n",
 		type == PCA9450_TYPE_PCA9450A ? "pca9450a" : "pca9450bc");
 
-- 
2.39.1

[PATCH 4/6] regulator: Add operation to let drivers select vsel register

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

There are regulators that use multiple registers for storing the
voltage. Add a get_reg_voltage_sel member to struct regulator_ops in
order to let drivers register a function that returns the currently
used register.

The pca9450 driver will be a user of this as the LDO5 regulator of
that chip uses two different control registers depending on the
state of an external signal.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/regulator/helpers.c      | 16 ++++++++++++++--
 include/linux/regulator/driver.h |  5 +++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/helpers.c b/drivers/regulator/helpers.c
index ad2237a95572..e629b0bea3d0 100644
--- a/drivers/regulator/helpers.c
+++ b/drivers/regulator/helpers.c
@@ -223,6 +223,16 @@ int regulator_set_voltage_sel_pickable_regmap(struct regulator_dev *rdev,
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage_sel_pickable_regmap);
 
+unsigned int regulator_get_hwreg_voltage_sel_regmap(struct regulator_dev *rdev)
+{
+	const struct regulator_ops *ops = rdev->desc->ops;
+
+	if (ops->get_reg_voltage_sel)
+		return ops->get_reg_voltage_sel(rdev);
+
+	return rdev->desc->vsel_reg;
+}
+
 /**
  * regulator_get_voltage_sel_regmap - standard get_voltage_sel for regmap users
  *
@@ -234,10 +244,11 @@ EXPORT_SYMBOL_GPL(regulator_set_voltage_sel_pickable_regmap);
  */
 int regulator_get_voltage_sel_regmap(struct regulator_dev *rdev)
 {
+	unsigned int vsel_reg = regulator_get_hwreg_voltage_sel_regmap(rdev);
 	unsigned int val;
 	int ret;
 
-	ret = regmap_read(rdev->regmap, rdev->desc->vsel_reg, &val);
+	ret = regmap_read(rdev->regmap, vsel_reg, &val);
 	if (ret != 0)
 		return ret;
 
@@ -260,11 +271,12 @@ EXPORT_SYMBOL_GPL(regulator_get_voltage_sel_regmap);
  */
 int regulator_set_voltage_sel_regmap(struct regulator_dev *rdev, unsigned sel)
 {
+	unsigned int vsel_reg = regulator_get_hwreg_voltage_sel_regmap(rdev);
 	int ret;
 
 	sel <<= ffs(rdev->desc->vsel_mask) - 1;
 
-	ret = regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
+	ret = regmap_update_bits(rdev->regmap, vsel_reg,
 				  rdev->desc->vsel_mask, sel);
 	if (ret)
 		return ret;
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index d3b4a3d4514a..c9953b2f63d5 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -77,6 +77,10 @@ enum regulator_detection_severity {
  * @get_voltage_sel: Return the currently configured voltage selector for the
  *                   regulator; return -ENOTRECOVERABLE if regulator can't
  *                   be read at bootup and hasn't been set yet.
+ * @get_reg_voltage_sel: Return the register used for getting/setting the
+ *                       voltage of the regulator. This is useful if the
+ *                       regulator uses multiple registers internally, switched
+ *                       by some condition like the state of an external signal.
  * @list_voltage: Return one of the supported voltages, in microvolts; zero
  *	if the selector indicates a voltage that is unusable on this system;
  *	or negative errno.  Selectors range from zero to one less than
@@ -168,6 +172,7 @@ struct regulator_ops {
 	int (*set_voltage_sel) (struct regulator_dev *, unsigned selector);
 	int (*get_voltage) (struct regulator_dev *);
 	int (*get_voltage_sel) (struct regulator_dev *);
+	unsigned int (*get_reg_voltage_sel) (struct regulator_dev *);
 
 	/* get/set regulator current  */
 	int (*set_current_limit) (struct regulator_dev *,
-- 
2.39.1

[PATCH 5/6] regulator: pca9450: Fix control register for LDO5

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

For LDO5 we need to be able to check the status of the SD_VSEL input in
order to know which control register is used. Read the status of the
SD_VSEL signal via GPIO and add a get_reg_voltage_sel operation for
LDO5 that returns the register that is currently in use to the core.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/regulator/pca9450-regulator.c | 45 ++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c
index 804a22c0e376..a0802c6cb259 100644
--- a/drivers/regulator/pca9450-regulator.c
+++ b/drivers/regulator/pca9450-regulator.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -32,6 +33,7 @@ struct pca9450_regulator_desc {
 struct pca9450 {
 	struct device *dev;
 	struct regmap *regmap;
+	struct gpio_desc *sd_vsel_gpio;
 	enum pca9450_chip_type type;
 	unsigned int rcnt;
 	int irq;
@@ -55,6 +57,16 @@ static const struct regmap_config pca9450_regmap_config = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
+static unsigned int pca9450_ldo5_get_reg_voltage_sel(struct regulator_dev *rdev)
+{
+	struct pca9450 *pca9450 = rdev_get_drvdata(rdev);
+
+	if (pca9450->sd_vsel_gpio && !gpiod_get_value(pca9450->sd_vsel_gpio))
+		return PCA9450_REG_LDO5CTRL_L;
+
+	return PCA9450_REG_LDO5CTRL_H;
+}
+
 /*
  * BUCK1/2/3
  * BUCK1RAM[1:0] BUCK1 DVS ramp rate setting
@@ -97,6 +109,16 @@ static const struct regulator_ops pca9450_ldo_regulator_ops = {
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
 };
 
+static const struct regulator_ops pca9450_ldo5_regulator_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+	.list_voltage = regulator_list_voltage_linear_range,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.get_reg_voltage_sel = pca9450_ldo5_get_reg_voltage_sel,
+};
+
 /*
  * BUCK1/2/3
  * 0.60 to 2.1875V (12.5mV step)
@@ -438,12 +460,11 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
 			.of_match = of_match_ptr("LDO5"),
 			.regulators_node = of_match_ptr("regulators"),
 			.id = PCA9450_LDO5,
-			.ops = &pca9450_ldo_regulator_ops,
+			.ops = &pca9450_ldo5_regulator_ops,
 			.type = REGULATOR_VOLTAGE,
 			.n_voltages = PCA9450_LDO5_VOLTAGE_NUM,
 			.linear_ranges = pca9450_ldo5_volts,
 			.n_linear_ranges = ARRAY_SIZE(pca9450_ldo5_volts),
-			.vsel_reg = PCA9450_REG_LDO5CTRL_H,
 			.vsel_mask = LDO5HOUT_MASK,
 			.enable_reg = PCA9450_REG_LDO5CTRL_L,
 			.enable_mask = LDO5H_EN_MASK,
@@ -647,12 +668,11 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
 			.of_match = of_match_ptr("LDO5"),
 			.regulators_node = of_match_ptr("regulators"),
 			.id = PCA9450_LDO5,
-			.ops = &pca9450_ldo_regulator_ops,
+			.ops = &pca9450_ldo5_regulator_ops,
 			.type = REGULATOR_VOLTAGE,
 			.n_voltages = PCA9450_LDO5_VOLTAGE_NUM,
 			.linear_ranges = pca9450_ldo5_volts,
 			.n_linear_ranges = ARRAY_SIZE(pca9450_ldo5_volts),
-			.vsel_reg = PCA9450_REG_LDO5CTRL_H,
 			.vsel_mask = LDO5HOUT_MASK,
 			.enable_reg = PCA9450_REG_LDO5CTRL_L,
 			.enable_mask = LDO5H_EN_MASK,
@@ -705,6 +725,7 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
 				      of_device_get_match_data(&i2c->dev);
 	const struct pca9450_regulator_desc	*regulator_desc;
 	struct regulator_config config = { };
+	struct regulator_dev *ldo5;
 	struct pca9450 *pca9450;
 	unsigned int device_id, i;
 	unsigned int reset_ctrl;
@@ -770,6 +791,7 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
 
 		config.regmap = pca9450->regmap;
 		config.dev = pca9450->dev;
+		config.driver_data = pca9450;
 
 		rdev = devm_regulator_register(pca9450->dev, desc, &config);
 		if (IS_ERR(rdev)) {
@@ -779,6 +801,9 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
 				desc->name, ret);
 			return ret;
 		}
+
+		if (!strcmp(desc->name, "ldo5"))
+			ldo5 = rdev;
 	}
 
 	ret = devm_request_threaded_irq(pca9450->dev, pca9450->irq, NULL,
@@ -832,6 +857,18 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
 		}
 	}
 
+	/*
+	 * For LDO5 we need to be able to check the status of the SD_VSEL input in
+	 * order to know which control register is used. Most boards connect SD_VSEL
+	 * to the VSELECT signal, so we can use the GPIO that is internally routed
+	 * to this signal (if SION bit is set in IOMUX).
+	 */
+	pca9450->sd_vsel_gpio = gpiod_get_optional(&ldo5->dev, "sd-vsel", GPIOD_IN);
+	if (IS_ERR(pca9450->sd_vsel_gpio)) {
+		dev_err(&i2c->dev, "Failed to get SD_VSEL GPIO\n");
+		return ret;
+	}
+
 	dev_info(&i2c->dev, "%s probed.\n",
 		type == PCA9450_TYPE_PCA9450A ? "pca9450a" : "pca9450bc");
 
-- 
2.39.1

[PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

This fixes the LDO5 regulator handling of the pca9450 driver by
taking the status of the SD_VSEL into account to determine which
configuration register is used for the voltage setting.

Even without this change there is no functional issue, as the code
for switching the voltage in sdhci.c currently switches both, the
VSELECT/SD_VSEL signal and the regulator voltage at the same time
and doesn't run into an invalid corner case.

We should still make sure, that we always use the correct register
when controlling the regulator. At least in U-Boot this fixes an
actual bug where the wrong IO voltage is used.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts | 6 +++---
 arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts       | 6 +++---
 arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi   | 1 +
 arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi      | 1 +
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts
index 8b16bd68576c..bdcd9cd843c7 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts
@@ -344,7 +344,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d0
 			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d0
 			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d0
 			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
-			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
+			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
 		>;
 	};
 
@@ -357,7 +357,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d4
 			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d4
 			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d4
 			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
-			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
+			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
 		>;
 	};
 
@@ -370,7 +370,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
 			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
 			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
 			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
-			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
+			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
 		>;
 	};
 };
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
index a079322a3793..ba2a4491cf31 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
@@ -321,7 +321,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d0
 			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d0
 			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d0
 			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
-			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
+			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
 		>;
 	};
 
@@ -334,7 +334,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d4
 			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d4
 			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d4
 			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
-			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
+			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
 		>;
 	};
 
@@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
 			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
 			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
 			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
-			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
+			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
 		>;
 	};
 };
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
index 5172883717d1..90daaf54e704 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
@@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 {
 				regulator-name = "NVCC_SD (LDO5)";
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <3300000>;
+				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
 			};
 		};
 	};
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi
index 1f8326613ee9..7468a8aa771d 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi
@@ -195,6 +195,7 @@ reg_nvcc_sd: LDO5 {
 				regulator-name = "NVCC_SD (LDO5)";
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <3300000>;
+				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
 			};
 		};
 	};
-- 
2.39.1

Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

[Marek Vasut]

On 2/13/23 16:58, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> This fixes the LDO5 regulator handling of the pca9450 driver by
> taking the status of the SD_VSEL into account to determine which
> configuration register is used for the voltage setting.
> 
> Even without this change there is no functional issue, as the code
> for switching the voltage in sdhci.c currently switches both, the
> VSELECT/SD_VSEL signal and the regulator voltage at the same time
> and doesn't run into an invalid corner case.
> 
> We should still make sure, that we always use the correct register
> when controlling the regulator. At least in U-Boot this fixes an
> actual bug where the wrong IO voltage is used.
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>

I may have a kind-of obvious request, since we removed those SD_VSEL 
very recently from other boards, could you just revert all those patches 
and only fill in the SION bit in V2 on all those boards too ? That way, 
we fix the LDO5 regulator for everyone who used it before.

Re: [PATCH 2/6] regulator: pca9450: Fix enable register for LDO5

[Marek Vasut]

On 2/13/23 16:58, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> The LDO5 regulator has two configuration registers, but only
> LDO5CTRL_L contains the bits for enabling/disabling the regulator.
> 
> Fixes: 0935ff5f1f0a ("regulator: pca9450: add pca9450 pmic driver")
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

[Frieder Schrempf]

On 13.02.23 17:08, Marek Vasut wrote:
> On 2/13/23 16:58, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> This fixes the LDO5 regulator handling of the pca9450 driver by
>> taking the status of the SD_VSEL into account to determine which
>> configuration register is used for the voltage setting.
>>
>> Even without this change there is no functional issue, as the code
>> for switching the voltage in sdhci.c currently switches both, the
>> VSELECT/SD_VSEL signal and the regulator voltage at the same time
>> and doesn't run into an invalid corner case.
>>
>> We should still make sure, that we always use the correct register
>> when controlling the regulator. At least in U-Boot this fixes an
>> actual bug where the wrong IO voltage is used.
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> I may have a kind-of obvious request, since we removed those SD_VSEL
> very recently from other boards, could you just revert all those patches
> and only fill in the SION bit in V2 on all those boards too ? That way,
> we fix the LDO5 regulator for everyone who used it before.

I thought about that, but as the SD_VSEL signal is controlling the LDO5,
I found it more useful/correct to have the sd-vsel-gpios property inside
the LDO5 regulator node. Therefore the old usage where sd-vsel-gpios was
inside the PMIC node isn't compatible anymore.

Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

[Marco Felsch]

Hi Frieder,

thanks for the patch.

On 23-02-13, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> This fixes the LDO5 regulator handling of the pca9450 driver by
> taking the status of the SD_VSEL into account to determine which
> configuration register is used for the voltage setting.
> 
> Even without this change there is no functional issue, as the code
> for switching the voltage in sdhci.c currently switches both, the
> VSELECT/SD_VSEL signal and the regulator voltage at the same time
> and doesn't run into an invalid corner case.
> 
> We should still make sure, that we always use the correct register
> when controlling the regulator. At least in U-Boot this fixes an
> actual bug where the wrong IO voltage is used.
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts | 6 +++---
>  arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts       | 6 +++---
>  arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi   | 1 +
>  arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi      | 1 +
>  4 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts
> index 8b16bd68576c..bdcd9cd843c7 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts
> @@ -344,7 +344,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d0
>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d0
>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d0
>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>  		>;
>  	};
>  
> @@ -357,7 +357,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d4
>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d4
>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d4
>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>  		>;
>  	};
>  
> @@ -370,7 +370,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>  		>;
>  	};
>  };
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
> index a079322a3793..ba2a4491cf31 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
> @@ -321,7 +321,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d0
>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d0
>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d0
>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>  		>;
>  	};
>  
> @@ -334,7 +334,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d4
>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d4
>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d4
>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>  		>;
>  	};
>  
> @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0

The VSELECT pin should be driven by the (u)sdhc core...

>  		>;
>  	};
>  };
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
> index 5172883717d1..90daaf54e704 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
> @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 {
>  				regulator-name = "NVCC_SD (LDO5)";
>  				regulator-min-microvolt = <1800000>;
>  				regulator-max-microvolt = <3300000>;
> +				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;

and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be
muxed as GPIO, which is not the case. So I think that u-boot have a bug
within the (u)sdhc core.

Regards,
  Marco

>  			};
>  		};
>  	};
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi
> index 1f8326613ee9..7468a8aa771d 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi
> @@ -195,6 +195,7 @@ reg_nvcc_sd: LDO5 {
>  				regulator-name = "NVCC_SD (LDO5)";
>  				regulator-min-microvolt = <1800000>;
>  				regulator-max-microvolt = <3300000>;
> +				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
>  			};
>  		};
>  	};
> -- 
> 2.39.1
> 
> 
> 

Re: [PATCH 3/6] Revert "regulator: pca9450: Add SD_VSEL GPIO for LDO5"

[Marek Vasut]

On 2/13/23 16:58, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> This reverts commit 8c67a11bae889f51fe5054364c3c789dfae3ad73.
> 
> It turns out that all boards using the PCA9450 actually have the
> SD_VSEL input conencted to the VSELECT signal of the SoCs SD/MMC

'connected' , typo .

btw would it make sense to squash 3..5 patches into a single patch ?

Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

[Frieder Schrempf]

On 13.02.23 17:15, Marco Felsch wrote:
> Hi Frieder,
> 
> thanks for the patch.
> 
> On 23-02-13, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> This fixes the LDO5 regulator handling of the pca9450 driver by
>> taking the status of the SD_VSEL into account to determine which
>> configuration register is used for the voltage setting.
>>
>> Even without this change there is no functional issue, as the code
>> for switching the voltage in sdhci.c currently switches both, the
>> VSELECT/SD_VSEL signal and the regulator voltage at the same time
>> and doesn't run into an invalid corner case.
>>
>> We should still make sure, that we always use the correct register
>> when controlling the regulator. At least in U-Boot this fixes an
>> actual bug where the wrong IO voltage is used.
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> ---
>>  arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts | 6 +++---
>>  arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts       | 6 +++---
>>  arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi   | 1 +
>>  arch/arm64/boot/dts/freescale/imx8mm-kontron-sl.dtsi      | 1 +
>>  4 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts
>> index 8b16bd68576c..bdcd9cd843c7 100644
>> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts
>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl-osm-s.dts
>> @@ -344,7 +344,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d0
>>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d0
>>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d0
>>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>>  		>;
>>  	};
>>  
>> @@ -357,7 +357,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d4
>>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d4
>>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d4
>>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>>  		>;
>>  	};
>>  
>> @@ -370,7 +370,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
>>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
>>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
>>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>>  		>;
>>  	};
>>  };
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
>> index a079322a3793..ba2a4491cf31 100644
>> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-bl.dts
>> @@ -321,7 +321,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d0
>>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d0
>>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d0
>>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>>  		>;
>>  	};
>>  
>> @@ -334,7 +334,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d4
>>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d4
>>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d4
>>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>>  		>;
>>  	};
>>  
>> @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
>>  			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
>>  			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
>>  			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
> 
> The VSELECT pin should be driven by the (u)sdhc core...
> 
>>  		>;
>>  	};
>>  };
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>> index 5172883717d1..90daaf54e704 100644
>> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>> @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 {
>>  				regulator-name = "NVCC_SD (LDO5)";
>>  				regulator-min-microvolt = <1800000>;
>>  				regulator-max-microvolt = <3300000>;
>> +				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
> 
> and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be
> muxed as GPIO, which is not the case. So I think that u-boot have a bug
> within the (u)sdhc core.

No, we don't mux the signal as GPIO. We still use the VSLECT mux option,
but enable the SION bit, so we can read back the current state of the
VSELECT signal via the GPIO.

Re: [PATCH 3/6] Revert "regulator: pca9450: Add SD_VSEL GPIO for LDO5"

[Frieder Schrempf]

On 13.02.23 17:16, Marek Vasut wrote:
> On 2/13/23 16:58, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> This reverts commit 8c67a11bae889f51fe5054364c3c789dfae3ad73.
>>
>> It turns out that all boards using the PCA9450 actually have the
>> SD_VSEL input conencted to the VSELECT signal of the SoCs SD/MMC
> 
> 'connected' , typo .

Thanks!

> btw would it make sense to squash 3..5 patches into a single patch ?

Hm, don't know. I think the changes are easier to understand with the
current separation between "revert", "core changes" and "pca9450 changes".

Re: [PATCH 3/6] Revert "regulator: pca9450: Add SD_VSEL GPIO for LDO5"

[Marek Vasut]

On 2/13/23 17:23, Frieder Schrempf wrote:
> On 13.02.23 17:16, Marek Vasut wrote:
>> On 2/13/23 16:58, Frieder Schrempf wrote:
>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>
>>> This reverts commit 8c67a11bae889f51fe5054364c3c789dfae3ad73.
>>>
>>> It turns out that all boards using the PCA9450 actually have the
>>> SD_VSEL input conencted to the VSELECT signal of the SoCs SD/MMC
>>
>> 'connected' , typo .
> 
> Thanks!
> 
>> btw would it make sense to squash 3..5 patches into a single patch ?
> 
> Hm, don't know. I think the changes are easier to understand with the
> current separation between "revert", "core changes" and "pca9450 changes".

OK, I don't mind either way.

Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

[Marek Vasut]

On 2/13/23 17:15, Marco Felsch wrote:

[...]

>> @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
>>   			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
>>   			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
>>   			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
> 
> The VSELECT pin should be driven by the (u)sdhc core...
> 
>>   		>;
>>   	};
>>   };
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>> index 5172883717d1..90daaf54e704 100644
>> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>> @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 {
>>   				regulator-name = "NVCC_SD (LDO5)";
>>   				regulator-min-microvolt = <1800000>;
>>   				regulator-max-microvolt = <3300000>;
>> +				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
> 
> and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be
> muxed as GPIO, which is not the case. So I think that u-boot have a bug
> within the (u)sdhc core.

The trick here is that the VSELECT is operated by the usdhc block as a 
function pin, but the PMIC driver can read the current state of the 
VSELECT pin by reading out the GPIO block SR register. Since the IOMUX 
SION bit is set on the VSELECT pin, the state of the pin is reflected in 
the GPIO block SR register even if the pin is muxed as function pin.

Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

[Marco Felsch]

Hi Marek, Frieder,

On 23-02-13, Marek Vasut wrote:
> On 2/13/23 17:15, Marco Felsch wrote:
> 
> [...]
> 
> > > @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
> > >   			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
> > >   			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
> > >   			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
> > > -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
> > > +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
> > 
> > The VSELECT pin should be driven by the (u)sdhc core...
> > 
> > >   		>;
> > >   	};
> > >   };
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
> > > index 5172883717d1..90daaf54e704 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
> > > @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 {
> > >   				regulator-name = "NVCC_SD (LDO5)";
> > >   				regulator-min-microvolt = <1800000>;
> > >   				regulator-max-microvolt = <3300000>;
> > > +				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
> > 
> > and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be
> > muxed as GPIO, which is not the case. So I think that u-boot have a bug
> > within the (u)sdhc core.
> 
> The trick here is that the VSELECT is operated by the usdhc block as a
> function pin, but the PMIC driver can read the current state of the VSELECT
> pin by reading out the GPIO block SR register. Since the IOMUX SION bit is
> set on the VSELECT pin, the state of the pin is reflected in the GPIO block
> SR register even if the pin is muxed as function pin.
> 

Thanks for this explanation :) Why does the regulator driver need to
know the current state of this pin? Since the voltage switching requires
some cmd's before the actual voltage level switch. So this must be
handled within the core.

Also after checking the driver, adding the sd-vsel-gpios will request
the specified gpio as output-high. Out of curiosity, what's the bug you
triggering within U-Boot?

Regards,
  Marco

Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

[Marek Vasut]

On 2/13/23 20:56, Marco Felsch wrote:
> Hi Marek, Frieder,

Hi,

> On 23-02-13, Marek Vasut wrote:
>> On 2/13/23 17:15, Marco Felsch wrote:
>>
>> [...]
>>
>>>> @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
>>>>    			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
>>>>    			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
>>>>    			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>>>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>>>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>>>
>>> The VSELECT pin should be driven by the (u)sdhc core...
>>>
>>>>    		>;
>>>>    	};
>>>>    };
>>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>>>> index 5172883717d1..90daaf54e704 100644
>>>> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>>>> @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 {
>>>>    				regulator-name = "NVCC_SD (LDO5)";
>>>>    				regulator-min-microvolt = <1800000>;
>>>>    				regulator-max-microvolt = <3300000>;
>>>> +				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
>>>
>>> and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be
>>> muxed as GPIO, which is not the case. So I think that u-boot have a bug
>>> within the (u)sdhc core.
>>
>> The trick here is that the VSELECT is operated by the usdhc block as a
>> function pin, but the PMIC driver can read the current state of the VSELECT
>> pin by reading out the GPIO block SR register. Since the IOMUX SION bit is
>> set on the VSELECT pin, the state of the pin is reflected in the GPIO block
>> SR register even if the pin is muxed as function pin.
>>
> 
> Thanks for this explanation :) Why does the regulator driver need to
> know the current state of this pin?

Because that regulator has an input pin which selects between two states 
of that regulator, L and H, and whatever L or H is depends on what is 
configured into the regulator via I2C. To correctly report the state of 
the regulator, you have to know the state of that input (selector) pin.

> Since the voltage switching requires
> some cmd's before the actual voltage level switch. So this must be
> handled within the core.
> 
> Also after checking the driver, adding the sd-vsel-gpios will request
> the specified gpio as output-high.

The GPIO would have to be requested as input, obviously.

> Out of curiosity, what's the bug you
> triggering within U-Boot?

AFAICT the readback of the initial state of the regulator (see paragraph 
above), which affects Linux all the same.

Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

[Fabio Estevam]

Hi Frieder,

On Mon, Feb 13, 2023 at 1:19 PM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:

> No, we don't mux the signal as GPIO. We still use the VSLECT mux option,
> but enable the SION bit, so we can read back the current state of the
> VSELECT signal via the GPIO.

Please include this explanation in the commit log.

Re: [PATCH 4/6] regulator: Add operation to let drivers select vsel register

[kernel test robot]

Hi Frieder,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on broonie-regulator/for-next]
[also build test WARNING on shawnguo/for-next arm/for-next arm/fixes arm64/for-next/core kvmarm/next rockchip/for-next soc/for-next xilinx-xlnx/master linus/master v6.2-rc8 next-20230213]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Frieder-Schrempf/dt-bindings-regulator-pca9450-Document-new-usage-of-sd-vsel-gpios/20230214-013045
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
patch link:    https://lore.kernel.org/r/20230213155833.1644366-5-frieder%40fris.de
patch subject: [PATCH 4/6] regulator: Add operation to let drivers select vsel register
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230214/202302140905.36f6bYXV-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/b76ab45ee4b60334c27d870b6d744a937ff94636
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Frieder-Schrempf/dt-bindings-regulator-pca9450-Document-new-usage-of-sd-vsel-gpios/20230214-013045
        git checkout b76ab45ee4b60334c27d870b6d744a937ff94636
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/regulator/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302140905.36f6bYXV-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/regulator/helpers.c:226:14: warning: no previous prototype for 'regulator_get_hwreg_voltage_sel_regmap' [-Wmissing-prototypes]
     226 | unsigned int regulator_get_hwreg_voltage_sel_regmap(struct regulator_dev *rdev)
         |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/regulator_get_hwreg_voltage_sel_regmap +226 drivers/regulator/helpers.c

   225	
 > 226	unsigned int regulator_get_hwreg_voltage_sel_regmap(struct regulator_dev *rdev)
   227	{
   228		const struct regulator_ops *ops = rdev->desc->ops;
   229	
   230		if (ops->get_reg_voltage_sel)
   231			return ops->get_reg_voltage_sel(rdev);
   232	
   233		return rdev->desc->vsel_reg;
   234	}
   235	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

[Marco Felsch]

On 23-02-13, Marek Vasut wrote:
> On 2/13/23 20:56, Marco Felsch wrote:
> > Hi Marek, Frieder,
> 
> Hi,
> 
> > On 23-02-13, Marek Vasut wrote:
> > > On 2/13/23 17:15, Marco Felsch wrote:
> > > 
> > > [...]
> > > 
> > > > > @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
> > > > >    			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
> > > > >    			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
> > > > >    			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
> > > > > -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
> > > > > +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
> > > > 
> > > > The VSELECT pin should be driven by the (u)sdhc core...
> > > > 
> > > > >    		>;
> > > > >    	};
> > > > >    };
> > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
> > > > > index 5172883717d1..90daaf54e704 100644
> > > > > --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
> > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
> > > > > @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 {
> > > > >    				regulator-name = "NVCC_SD (LDO5)";
> > > > >    				regulator-min-microvolt = <1800000>;
> > > > >    				regulator-max-microvolt = <3300000>;
> > > > > +				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
> > > > 
> > > > and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be
> > > > muxed as GPIO, which is not the case. So I think that u-boot have a bug
> > > > within the (u)sdhc core.
> > > 
> > > The trick here is that the VSELECT is operated by the usdhc block as a
> > > function pin, but the PMIC driver can read the current state of the VSELECT
> > > pin by reading out the GPIO block SR register. Since the IOMUX SION bit is
> > > set on the VSELECT pin, the state of the pin is reflected in the GPIO block
> > > SR register even if the pin is muxed as function pin.
> > > 
> > 
> > Thanks for this explanation :) Why does the regulator driver need to
> > know the current state of this pin?
> 
> Because that regulator has an input pin which selects between two states of
> that regulator, L and H, and whatever L or H is depends on what is
> configured into the regulator via I2C. To correctly report the state of the
> regulator, you have to know the state of that input (selector) pin.
> 
> > Since the voltage switching requires
> > some cmd's before the actual voltage level switch. So this must be
> > handled within the core.
> > 
> > Also after checking the driver, adding the sd-vsel-gpios will request
> > the specified gpio as output-high.
> 
> The GPIO would have to be requested as input, obviously.

But that isn't the case. According the driver comment they just want to
make sure that this GPIO is high to ensure that the correct regulator
config registers are used.

> > Out of curiosity, what's the bug you
> > triggering within U-Boot?
> 
> AFAICT the readback of the initial state of the regulator (see paragraph
> above), which affects Linux all the same.

According the binding the driver should check this and apply the value
to the corresponding L/H register but unfortunately this isn't the case
yet. Does U-Boot handle this correctly?

Regards,
  Marco

Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

[Frieder Schrempf]

Hi Marco,

On 14.02.23 09:10, Marco Felsch wrote:
> On 23-02-13, Marek Vasut wrote:
>> On 2/13/23 20:56, Marco Felsch wrote:
>>> Hi Marek, Frieder,
>>
>> Hi,
>>
>>> On 23-02-13, Marek Vasut wrote:
>>>> On 2/13/23 17:15, Marco Felsch wrote:
>>>>
>>>> [...]
>>>>
>>>>>> @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
>>>>>>    			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
>>>>>>    			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
>>>>>>    			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
>>>>>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
>>>>>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
>>>>>
>>>>> The VSELECT pin should be driven by the (u)sdhc core...
>>>>>
>>>>>>    		>;
>>>>>>    	};
>>>>>>    };
>>>>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>>>>>> index 5172883717d1..90daaf54e704 100644
>>>>>> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
>>>>>> @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 {
>>>>>>    				regulator-name = "NVCC_SD (LDO5)";
>>>>>>    				regulator-min-microvolt = <1800000>;
>>>>>>    				regulator-max-microvolt = <3300000>;
>>>>>> +				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
>>>>>
>>>>> and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be
>>>>> muxed as GPIO, which is not the case. So I think that u-boot have a bug
>>>>> within the (u)sdhc core.
>>>>
>>>> The trick here is that the VSELECT is operated by the usdhc block as a
>>>> function pin, but the PMIC driver can read the current state of the VSELECT
>>>> pin by reading out the GPIO block SR register. Since the IOMUX SION bit is
>>>> set on the VSELECT pin, the state of the pin is reflected in the GPIO block
>>>> SR register even if the pin is muxed as function pin.
>>>>
>>>
>>> Thanks for this explanation :) Why does the regulator driver need to
>>> know the current state of this pin?
>>
>> Because that regulator has an input pin which selects between two states of
>> that regulator, L and H, and whatever L or H is depends on what is
>> configured into the regulator via I2C. To correctly report the state of the
>> regulator, you have to know the state of that input (selector) pin.
>>
>>> Since the voltage switching requires
>>> some cmd's before the actual voltage level switch. So this must be
>>> handled within the core.
>>>
>>> Also after checking the driver, adding the sd-vsel-gpios will request
>>> the specified gpio as output-high.
>>
>> The GPIO would have to be requested as input, obviously.
> 
> But that isn't the case. According the driver comment they just want to
> make sure that this GPIO is high to ensure that the correct regulator
> config registers are used.

It seems like you look at the wrong code. We previously had the
sd-vsel-gpios used as you describe, as an output set to a fixed high
level to make sure that the SD_VSEL state matches the driver using the H
register. This code is reverted in patch 3 and patch 5 implements the
sd-vsel-gpios as input as described by Marek. See:

https://lore.kernel.org/lkml/20230213155833.1644366-4-frieder@fris.de/
https://lore.kernel.org/lkml/20230213155833.1644366-6-frieder@fris.de/

Sorry, my scripts didn't cc everyone for all patches, which is probably
why you missed these changes.


> 
>>> Out of curiosity, what's the bug you
>>> triggering within U-Boot?
>>
>> AFAICT the readback of the initial state of the regulator (see paragraph
>> above), which affects Linux all the same.
> 
> According the binding the driver should check this and apply the value
> to the corresponding L/H register but unfortunately this isn't the case
> yet. Does U-Boot handle this correctly?

See above.

Thanks
Frieder

Re: [PATCH 4/6] regulator: Add operation to let drivers select vsel register

[kernel test robot]

Hi Frieder,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on broonie-regulator/for-next]
[also build test WARNING on shawnguo/for-next arm/for-next arm/fixes arm64/for-next/core kvmarm/next rockchip/for-next soc/for-next xilinx-xlnx/master linus/master v6.2-rc8 next-20230214]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Frieder-Schrempf/dt-bindings-regulator-pca9450-Document-new-usage-of-sd-vsel-gpios/20230214-013045
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
patch link:    https://lore.kernel.org/r/20230213155833.1644366-5-frieder%40fris.de
patch subject: [PATCH 4/6] regulator: Add operation to let drivers select vsel register
config: i386-randconfig-a003-20230213 (https://download.01.org/0day-ci/archive/20230214/202302141754.N3CvO9lF-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/b76ab45ee4b60334c27d870b6d744a937ff94636
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Frieder-Schrempf/dt-bindings-regulator-pca9450-Document-new-usage-of-sd-vsel-gpios/20230214-013045
        git checkout b76ab45ee4b60334c27d870b6d744a937ff94636
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/regulator/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302141754.N3CvO9lF-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/regulator/helpers.c:226:14: warning: no previous prototype for function 'regulator_get_hwreg_voltage_sel_regmap' [-Wmissing-prototypes]
   unsigned int regulator_get_hwreg_voltage_sel_regmap(struct regulator_dev *rdev)
                ^
   drivers/regulator/helpers.c:226:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   unsigned int regulator_get_hwreg_voltage_sel_regmap(struct regulator_dev *rdev)
   ^
   static 
   1 warning generated.


vim +/regulator_get_hwreg_voltage_sel_regmap +226 drivers/regulator/helpers.c

   225	
 > 226	unsigned int regulator_get_hwreg_voltage_sel_regmap(struct regulator_dev *rdev)
   227	{
   228		const struct regulator_ops *ops = rdev->desc->ops;
   229	
   230		if (ops->get_reg_voltage_sel)
   231			return ops->get_reg_voltage_sel(rdev);
   232	
   233		return rdev->desc->vsel_reg;
   234	}
   235	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 6/6] arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal

[Marco Felsch]

On 23-02-14, Frieder Schrempf wrote:
> Hi Marco,
> 
> On 14.02.23 09:10, Marco Felsch wrote:
> > On 23-02-13, Marek Vasut wrote:
> >> On 2/13/23 20:56, Marco Felsch wrote:
> >>> Hi Marek, Frieder,
> >>
> >> Hi,
> >>
> >>> On 23-02-13, Marek Vasut wrote:
> >>>> On 2/13/23 17:15, Marco Felsch wrote:
> >>>>
> >>>> [...]
> >>>>
> >>>>>> @@ -347,7 +347,7 @@ MX8MM_IOMUXC_SD2_DATA1_USDHC2_DATA1		0x1d6
> >>>>>>    			MX8MM_IOMUXC_SD2_DATA2_USDHC2_DATA2		0x1d6
> >>>>>>    			MX8MM_IOMUXC_SD2_DATA3_USDHC2_DATA3		0x1d6
> >>>>>>    			MX8MM_IOMUXC_SD2_CD_B_GPIO2_IO12		0x019
> >>>>>> -			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x1d0
> >>>>>> +			MX8MM_IOMUXC_GPIO1_IO04_USDHC2_VSELECT		0x400001d0
> >>>>>
> >>>>> The VSELECT pin should be driven by the (u)sdhc core...
> >>>>>
> >>>>>>    		>;
> >>>>>>    	};
> >>>>>>    };
> >>>>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
> >>>>>> index 5172883717d1..90daaf54e704 100644
> >>>>>> --- a/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
> >>>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-kontron-osm-s.dtsi
> >>>>>> @@ -196,6 +196,7 @@ reg_nvcc_sd: LDO5 {
> >>>>>>    				regulator-name = "NVCC_SD (LDO5)";
> >>>>>>    				regulator-min-microvolt = <1800000>;
> >>>>>>    				regulator-max-microvolt = <3300000>;
> >>>>>> +				sd-vsel-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
> >>>>>
> >>>>> and by using the sd-vsel-gpios property the IOMUXC_GPIO1_IO04 have to be
> >>>>> muxed as GPIO, which is not the case. So I think that u-boot have a bug
> >>>>> within the (u)sdhc core.
> >>>>
> >>>> The trick here is that the VSELECT is operated by the usdhc block as a
> >>>> function pin, but the PMIC driver can read the current state of the VSELECT
> >>>> pin by reading out the GPIO block SR register. Since the IOMUX SION bit is
> >>>> set on the VSELECT pin, the state of the pin is reflected in the GPIO block
> >>>> SR register even if the pin is muxed as function pin.
> >>>>
> >>>
> >>> Thanks for this explanation :) Why does the regulator driver need to
> >>> know the current state of this pin?
> >>
> >> Because that regulator has an input pin which selects between two states of
> >> that regulator, L and H, and whatever L or H is depends on what is
> >> configured into the regulator via I2C. To correctly report the state of the
> >> regulator, you have to know the state of that input (selector) pin.
> >>
> >>> Since the voltage switching requires
> >>> some cmd's before the actual voltage level switch. So this must be
> >>> handled within the core.
> >>>
> >>> Also after checking the driver, adding the sd-vsel-gpios will request
> >>> the specified gpio as output-high.
> >>
> >> The GPIO would have to be requested as input, obviously.
> > 
> > But that isn't the case. According the driver comment they just want to
> > make sure that this GPIO is high to ensure that the correct regulator
> > config registers are used.
> 
> It seems like you look at the wrong code. We previously had the
> sd-vsel-gpios used as you describe, as an output set to a fixed high
> level to make sure that the SD_VSEL state matches the driver using the H
> register. This code is reverted in patch 3 and patch 5 implements the
> sd-vsel-gpios as input as described by Marek. See:
> 
> https://lore.kernel.org/lkml/20230213155833.1644366-4-frieder@fris.de/
> https://lore.kernel.org/lkml/20230213155833.1644366-6-frieder@fris.de/
> 
> Sorry, my scripts didn't cc everyone for all patches, which is probably
> why you missed these changes.

Ah okay this brings light into the dark :) Thanks for the pointers, just
received the DT changes.

Regards,
  Marco

Re: [PATCH 4/6] regulator: Add operation to let drivers select vsel register

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

On Mon, Feb 13, 2023 at 04:58:22PM +0100, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> There are regulators that use multiple registers for storing the
> voltage. Add a get_reg_voltage_sel member to struct regulator_ops in
> order to let drivers register a function that returns the currently
> used register.
> 
> The pca9450 driver will be a user of this as the LDO5 regulator of
> that chip uses two different control registers depending on the
> state of an external signal.

Aside from the build warnings the bots reported it's not clear to
me that it's better to do this than it is to just have these
drivers implement appropriate ops directly - there's probably
going to be cases when it's a different bitfield in the same
register, and by the time you've implemented the op so things
aren't completely data driven I'm not sure how much you win by
reusing the register read/write.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/6] dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios

[Rob Herring]

On Mon, Feb 13, 2023 at 04:58:19PM +0100, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> The sd-vsel-gpios property is abandoned in its current meaning as an
> output. We now use it to specify an optional signal that can be
> evaluated by the driver in order to retrieve the current status
> of the SD_VSEL signal that is used to select the control register
> of LDO5.
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  .../regulator/nxp,pca9450-regulator.yaml      | 23 ++++++++++++++-----
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> index 835b53302db8..c86534538a4e 100644
> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> @@ -40,8 +40,24 @@ properties:
>      description: |
>        list of regulators provided by this controller
>  
> +    properties:
> +      LDO5:
> +        type: object
> +        $ref: regulator.yaml#
> +        description:
> +          Properties for single LDO5 regulator.
> +
> +        properties:
> +          sd-vsel-gpios:

It is a pin on the device, right? Then it belongs in the device node as 
it was.

Can't the direction of the signal tell you how it is used? Assuming the 
pin is bidirectional?

The binding should support any possible way the device is wired, not 
just what's been seen so far on some boards.

Rob

Re: [PATCH 1/6] dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios

[Marek Vasut]

On 2/15/23 21:02, Rob Herring wrote:
> On Mon, Feb 13, 2023 at 04:58:19PM +0100, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> The sd-vsel-gpios property is abandoned in its current meaning as an
>> output. We now use it to specify an optional signal that can be
>> evaluated by the driver in order to retrieve the current status
>> of the SD_VSEL signal that is used to select the control register
>> of LDO5.
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> ---
>>   .../regulator/nxp,pca9450-regulator.yaml      | 23 ++++++++++++++-----
>>   1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>> index 835b53302db8..c86534538a4e 100644
>> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>> @@ -40,8 +40,24 @@ properties:
>>       description: |
>>         list of regulators provided by this controller
>>   
>> +    properties:
>> +      LDO5:
>> +        type: object
>> +        $ref: regulator.yaml#
>> +        description:
>> +          Properties for single LDO5 regulator.
>> +
>> +        properties:
>> +          sd-vsel-gpios:
> 
> It is a pin on the device, right? Then it belongs in the device node as
> it was.
> 
> Can't the direction of the signal tell you how it is used? Assuming the
> pin is bidirectional?

The pin is input to the PMIC, it is unidirection, i.e.

SoC(output)---->(input)PMIC

> The binding should support any possible way the device is wired, not
> just what's been seen so far on some boards.

The usage is always the above as far as I can tell.

Re: [PATCH 1/6] dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios

[Rob Herring]

On Wed, Feb 15, 2023 at 7:27 PM Marek Vasut <marex@denx.de> wrote:
>
> On 2/15/23 21:02, Rob Herring wrote:
> > On Mon, Feb 13, 2023 at 04:58:19PM +0100, Frieder Schrempf wrote:
> >> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>
> >> The sd-vsel-gpios property is abandoned in its current meaning as an
> >> output. We now use it to specify an optional signal that can be
> >> evaluated by the driver in order to retrieve the current status
> >> of the SD_VSEL signal that is used to select the control register
> >> of LDO5.
> >>
> >> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> >> ---
> >>   .../regulator/nxp,pca9450-regulator.yaml      | 23 ++++++++++++++-----
> >>   1 file changed, 17 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> >> index 835b53302db8..c86534538a4e 100644
> >> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> >> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> >> @@ -40,8 +40,24 @@ properties:
> >>       description: |
> >>         list of regulators provided by this controller
> >>
> >> +    properties:
> >> +      LDO5:
> >> +        type: object
> >> +        $ref: regulator.yaml#
> >> +        description:
> >> +          Properties for single LDO5 regulator.
> >> +
> >> +        properties:
> >> +          sd-vsel-gpios:
> >
> > It is a pin on the device, right? Then it belongs in the device node as
> > it was.
> >
> > Can't the direction of the signal tell you how it is used? Assuming the
> > pin is bidirectional?
>
> The pin is input to the PMIC, it is unidirection, i.e.
>
> SoC(output)---->(input)PMIC
>
> > The binding should support any possible way the device is wired, not
> > just what's been seen so far on some boards.
>
> The usage is always the above as far as I can tell.

This patch is saying the opposite though. Something else drives the
signal, but the signal is also routed to the SoC to sample the state.

Rob

Re: [PATCH 4/6] regulator: Add operation to let drivers select vsel register

[Frieder Schrempf]

Hi Mark,

On 14.02.23 22:06, Mark Brown wrote:
> On Mon, Feb 13, 2023 at 04:58:22PM +0100, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> There are regulators that use multiple registers for storing the
>> voltage. Add a get_reg_voltage_sel member to struct regulator_ops in
>> order to let drivers register a function that returns the currently
>> used register.
>>
>> The pca9450 driver will be a user of this as the LDO5 regulator of
>> that chip uses two different control registers depending on the
>> state of an external signal.
> 
> Aside from the build warnings the bots reported it's not clear to
> me that it's better to do this than it is to just have these
> drivers implement appropriate ops directly - there's probably
> going to be cases when it's a different bitfield in the same
> register, and by the time you've implemented the op so things
> aren't completely data driven I'm not sure how much you win by
> reusing the register read/write.

Thanks for the feedback. Makes sense to me, I can do that.

Just to be sure: you are suggesting to to leave the core untouched, use
the existing [get/set]_voltage() ops in the driver and reimplement the
logic of regulator_[get/set]_voltage_sel_regmap() there, right?

Thanks
Frieder

Re: [PATCH 1/6] dt-bindings: regulator: pca9450: Document new usage of sd-vsel-gpios

[Frieder Schrempf]

On 16.02.23 03:30, Rob Herring wrote:
> On Wed, Feb 15, 2023 at 7:27 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/15/23 21:02, Rob Herring wrote:
>>> On Mon, Feb 13, 2023 at 04:58:19PM +0100, Frieder Schrempf wrote:
>>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>
>>>> The sd-vsel-gpios property is abandoned in its current meaning as an
>>>> output. We now use it to specify an optional signal that can be
>>>> evaluated by the driver in order to retrieve the current status
>>>> of the SD_VSEL signal that is used to select the control register
>>>> of LDO5.
>>>>
>>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>> ---
>>>>   .../regulator/nxp,pca9450-regulator.yaml      | 23 ++++++++++++++-----
>>>>   1 file changed, 17 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>>>> index 835b53302db8..c86534538a4e 100644
>>>> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>>>> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>>>> @@ -40,8 +40,24 @@ properties:
>>>>       description: |
>>>>         list of regulators provided by this controller
>>>>
>>>> +    properties:
>>>> +      LDO5:
>>>> +        type: object
>>>> +        $ref: regulator.yaml#
>>>> +        description:
>>>> +          Properties for single LDO5 regulator.
>>>> +
>>>> +        properties:
>>>> +          sd-vsel-gpios:
>>>
>>> It is a pin on the device, right? Then it belongs in the device node as
>>> it was.

Physically it's a pin on the PCA9450 chip. If you look at the block
diagram in the datasheet [1] (page 3) you can see though, that the
SD_VSEL signal is routed to the LD05 regulator block inside the chip.
This makes me think that the signal is best described inside the LDO5 node.

>>>
>>> Can't the direction of the signal tell you how it is used? Assuming the
>>> pin is bidirectional?
>>
>> The pin is input to the PMIC, it is unidirection, i.e.
>>
>> SoC(output)---->(input)PMIC
>>
>>> The binding should support any possible way the device is wired, not
>>> just what's been seen so far on some boards.
>>
>> The usage is always the above as far as I can tell.

There is only one usage that is likely to occur and that is the one we
describe here.

There are other ways to wire up the signal of course and in some
unlikely event a hardware engineer might have the idea to hard-wire the
SD_VSEL to a fixed level or wire it up to a SoC pin that doesn't have
the VSELECT mux option.

But I don't really see a good reason for covering these cases in the
binding/driver if there are good chances we won't ever need them.

> This patch is saying the opposite though. Something else drives the
> signal, but the signal is also routed to the SoC to sample the state.

SoC                                  PMIC
+-----------------------+           +-------------------+
|                       |           |                   |
|                       |           |                   |
|  GPIO <----------+    |           |                   |
|                  |    |    SD_VSEL|   +-------+       |
|  USHC_VSELECT -->+------------------->| LDO5  |       |
|                       |           |   +-------+       |
|                       |           |                   |
+-----------------------+           +-------------------+

This is how the setup looks like. The SD_VSEL on the PMIC is always an
input. It's driven by the SoC's VSELECT signal (controlled by the USDHC
controller) and we use the SION bit in the IOMUX to internally loop back
the signal in order to sample it using the GPIO.

[1] https://www.nxp.com/docs/en/data-sheet/PCA9450DS.pdf

Re: [PATCH 4/6] regulator: Add operation to let drivers select vsel register

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

On Thu, Feb 16, 2023 at 10:05:49AM +0100, Frieder Schrempf wrote:

> Just to be sure: you are suggesting to to leave the core untouched, use
> the existing [get/set]_voltage() ops in the driver and reimplement the
> logic of regulator_[get/set]_voltage_sel_regmap() there, right?

Yes, exactly.  If we get a lot of drivers doing this we can
always factor out later but it feels premature right now.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]