LinuxLists.cc - [PATCH 0/6] Use correct LDO5 control registers for PCA9450

[permalink] [raw]

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

From: Frieder Schrempf <[email protected]>

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 <[email protected]>
---
.../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

2023-02-13 16:00:02

[permalink] [raw]

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

From: Frieder Schrempf <[email protected]>

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 <[email protected]>
---
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

2023-02-13 16:00:05

[permalink] [raw]

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

From: Frieder Schrempf <[email protected]>

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 <[email protected]>
---
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

2023-02-13 16:00:12

[permalink] [raw]

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

From: Frieder Schrempf <[email protected]>

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 <[email protected]>
---
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

2023-02-13 16:00:25

[permalink] [raw]

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

From: Frieder Schrempf <[email protected]>

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 <[email protected]>
---
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

2023-02-13 16:01:08

[permalink] [raw]

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

From: Frieder Schrempf <[email protected]>

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 <[email protected]>
---
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

2023-02-13 16:09:01

[permalink] [raw]

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

On 2/13/23 16:58, Frieder Schrempf wrote:
> From: Frieder Schrempf <[email protected]>
>
> 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 <[email protected]>

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.

2023-02-13 16:12:27

[permalink] [raw]

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

On 2/13/23 16:58, Frieder Schrempf wrote:
> From: Frieder Schrempf <[email protected]>
>
> 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 <[email protected]>

Reviewed-by: Marek Vasut <[email protected]>

2023-02-13 16:15:58

[permalink] [raw]

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

On 13.02.23 17:08, Marek Vasut wrote:
> On 2/13/23 16:58, Frieder Schrempf wrote:
>> From: Frieder Schrempf <[email protected]>
>>
>> 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 <[email protected]>
>
> 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.

2023-02-13 16:16:05

[permalink] [raw]

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

Hi Frieder,

thanks for the patch.

On 23-02-13, Frieder Schrempf wrote:
> From: Frieder Schrempf <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>
>
>

2023-02-13 16:17:06

[permalink] [raw]

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

On 2/13/23 16:58, Frieder Schrempf wrote:
> From: Frieder Schrempf <[email protected]>
>
> 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 ?

2023-02-13 16:19:29

[permalink] [raw]

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

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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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.

2023-02-13 16:25:23

[permalink] [raw]

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

On 13.02.23 17:16, Marek Vasut wrote:
> On 2/13/23 16:58, Frieder Schrempf wrote:
>> From: Frieder Schrempf <[email protected]>
>>
>> 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".

2023-02-13 18:10:33

[permalink] [raw]

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

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 <[email protected]>
>>>
>>> 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.

2023-02-13 18:12:58

[permalink] [raw]

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

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.

2023-02-13 19:56:30

[permalink] [raw]

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

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

2023-02-13 20:59:35

[permalink] [raw]

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

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.

2023-02-13 21:02:20

by Fabio Estevam

[permalink] [raw]

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

Hi Frieder,

On Mon, Feb 13, 2023 at 1:19 PM Frieder Schrempf
<[email protected]> 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.

2023-02-14 01:40:57

by kernel test robot

[permalink] [raw]

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

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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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

2023-02-14 08:11:18

[permalink] [raw]

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

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

2023-02-14 08:26:20

[permalink] [raw]

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

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/[email protected]/
https://lore.kernel.org/lkml/[email protected]/

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

2023-02-14 09:20:24

by kernel test robot

[permalink] [raw]

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

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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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

2023-02-14 11:47:01

[permalink] [raw]

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

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/[email protected]/
> https://lore.kernel.org/lkml/[email protected]/
>
> 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

2023-02-14 21:06:47

by Mark Brown

[permalink] [raw]

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

On Mon, Feb 13, 2023 at 04:58:22PM +0100, Frieder Schrempf wrote:
> From: Frieder Schrempf <[email protected]>
>
> 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.

Attachments:

(No filename) (927.00 B)
signature.asc (488.00 B)
Download all attachments

2023-02-15 20:02:20

by Rob Herring

[permalink] [raw]

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

On Mon, Feb 13, 2023 at 04:58:19PM +0100, Frieder Schrempf wrote:
> From: Frieder Schrempf <[email protected]>
>
> 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 <[email protected]>
> ---
> .../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

2023-02-16 01:27:40

[permalink] [raw]

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

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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> .../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.

2023-02-16 02:31:05

by Rob Herring

[permalink] [raw]

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

On Wed, Feb 15, 2023 at 7:27 PM Marek Vasut <[email protected]> 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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >> ---
> >> .../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

2023-02-16 09:06:00

[permalink] [raw]

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

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 <[email protected]>
>>
>> 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

2023-02-16 10:16:12