[U-Boot] [RFT 0/8] exynos: Fix reboot on Odroid HC1

[U-Boot] [RFT 0/8] exynos: Fix reboot on Odroid HC1

[Krzysztof Kozlowski]

Hi,

Odroid HC1 does not reboot properly (at least from SD card but
I do not expect difference on eMMC), if LDO4/VDD_ADC was turned
off by Linux kernel.  This condition is so far always, because
Linux kernel did not enable ADC on ODroid HC1, therefore the
VDD_ADC regulator was turned off as unused.

The issue is in detection of revision which later is used to load
proper DTB.

The revision is obtained by ADC read of a voltage depending on VDD_ADC.
Therefore:
1. VDD_ADC has to be turned on (but board detection happens before
   power is initialized),
2. Turning VDD_ADC should wait with ramp delay,
3. Reading the value from ADC should wait for it to stabilize.

I must admit I did not test it on other boards because latest U-Boot
does not boot from SD card.

Commends and testing are welcomed.

Best regards,
Krzysztof

Krzysztof Kozlowski (8):
  exynos: Redo detection of revision when all resources are ready
  exynos: Wait till ADC stabilizes before checking Odroid HC1 revision
  adc: exynos-adc: Fix wrong bit operation used to stop the ADC
  regulator: Add support for ramp delay
  power: regulator: s2mps11: Fix step for LDO27 and LDO35
  power: regulator: s2mps11: Add enable delay
  arm: dts: exynos: Add supply for ADC block to Odroid XU3 family
  arm: dts: exynos: Add ramp delay property to LDO regulators to Odroid
    XU3 family

 arch/arm/dts/exynos5422-odroidxu3.dts       | 20 +++++++++
 board/samsung/common/board.c                | 19 ++++++++-
 board/samsung/common/exynos5-dt-types.c     | 34 +++++++++++++++-
 drivers/adc/exynos-adc.c                    |  2 +-
 drivers/power/regulator/regulator-uclass.c  | 45 ++++++++++++++++++++-
 drivers/power/regulator/s2mps11_regulator.c | 13 +++++-
 include/power/regulator.h                   |  2 +
 7 files changed, 129 insertions(+), 6 deletions(-)

-- 
2.17.1

[U-Boot] [RFT 1/8] exynos: Redo detection of revision when all resources are ready

[Krzysztof Kozlowski]

Detection of board type is done early - before power setup.  In case of
Odroid XU3/XU4/HC1 family, the detection is done using ADC which
is supplied by LDO4/VDD_ADC regulator.  This regulator could be turned
off (e.g. by kernel before reboot);  If ADC is used early, the
regulators are not yet available and the detection won't work.

Try to detect the revision again, once power is brought up.

This is necessary to fix the detection of Odroid HC1 after reboot, if
kernel turned off the LDO4 regulator.  Otherwise the board is not
detected....

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 board/samsung/common/board.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/board/samsung/common/board.c b/board/samsung/common/board.c
index 6fd26a3a9198..1e2dabe68d11 100644
--- a/board/samsung/common/board.c
+++ b/board/samsung/common/board.c
@@ -147,6 +147,11 @@ int board_early_init_f(void)
 {
 	int err;
 #ifdef CONFIG_BOARD_TYPES
+	/*
+	 * It is done early so power might not be set up yet.  In such case
+	 * specific revision detection with ADC might not work and need to me
+	 * redone later.
+	 */
 	set_board_type();
 #endif
 	err = board_uart_init();
@@ -166,9 +171,21 @@ int board_early_init_f(void)
 #if defined(CONFIG_POWER) || defined(CONFIG_DM_PMIC)
 int power_init_board(void)
 {
+	int ret;
+
 	set_ps_hold_ctrl();
 
-	return exynos_power_init();
+	ret = exynos_power_init();
+
+#ifdef CONFIG_BOARD_TYPES
+	/*
+	 * Since power is on, redo the board detection (external peripherals
+	 * are on).
+	 */
+	set_board_type();
+#endif
+
+	return ret;
 }
 #endif
 
-- 
2.17.1

[U-Boot] [RFT 2/8] exynos: Wait till ADC stabilizes before checking Odroid HC1 revision

[Krzysztof Kozlowski]

Fix detection of Odroid HC1 (Exynos5422) after reboot if kernel disabled
the LDO4/VDD_ADC regulator.

The LDO4 supplies both ADC block and the ADC input AIN9.  Voltage on
AIN9 will rise slowly, so be patient and wait for it to stabilize.

First reads on Odroid HC1 return 305, 1207, 1297 and finally 1308
(reference value is 1309).

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 board/samsung/common/exynos5-dt-types.c | 34 ++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/board/samsung/common/exynos5-dt-types.c b/board/samsung/common/exynos5-dt-types.c
index 7a86e9187768..ba8584f1a5d8 100644
--- a/board/samsung/common/exynos5-dt-types.c
+++ b/board/samsung/common/exynos5-dt-types.c
@@ -57,12 +57,44 @@ static unsigned int odroid_get_rev(void)
 	return 0;
 }
 
+/*
+ * Read ADC at least twice and check the resuls.  If regulator providing voltage
+ * on to measured point was just turned on, first reads might require time
+ * to stabilize.
+ */
+static int odroid_get_adc_val(unsigned int *adcval)
+{
+	unsigned int adcval_prev = 0;
+	int ret, retries = 20;
+
+	ret = adc_channel_single_shot("adc", CONFIG_ODROID_REV_AIN,
+				      &adcval_prev);
+	if (ret)
+		return ret;
+
+	while (retries--) {
+		mdelay(5);
+
+		ret = adc_channel_single_shot("adc", CONFIG_ODROID_REV_AIN,
+					      adcval);
+		if (ret)
+			return ret;
+
+		if ((100 * abs(*adcval - adcval_prev) / adcval_prev) < 3)
+			return ret;
+
+		adcval_prev = *adcval;
+	}
+
+	return ret;
+}
+
 static int odroid_get_board_type(void)
 {
 	unsigned int adcval;
 	int ret, i;
 
-	ret = adc_channel_single_shot("adc", CONFIG_ODROID_REV_AIN, &adcval);
+	ret = odroid_get_adc_val(&adcval);
 	if (ret)
 		goto rev_default;
 
-- 
2.17.1

[U-Boot] [RFT 3/8] adc: exynos-adc: Fix wrong bit operation used to stop the ADC

[Krzysztof Kozlowski]

When stopping the ADC_V2_CON1_STC_EN should be cleared.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/adc/exynos-adc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/adc/exynos-adc.c b/drivers/adc/exynos-adc.c
index d33e3d632afc..12c49fc8cefb 100644
--- a/drivers/adc/exynos-adc.c
+++ b/drivers/adc/exynos-adc.c
@@ -62,7 +62,7 @@ int exynos_adc_stop(struct udevice *dev)
 
 	/* Stop conversion */
 	cfg = readl(&regs->con1);
-	cfg |= ~ADC_V2_CON1_STC_EN;
+	cfg &= ~ADC_V2_CON1_STC_EN;
 
 	writel(cfg, &regs->con1);
 
-- 
2.17.1

[U-Boot] [RFT 4/8] regulator: Add support for ramp delay

[Krzysztof Kozlowski]

Changing voltage and enabling regulator might require delays so the
regulator stabilizes at expected level.

Add support for "regulator-ramp-delay" binding which can introduce
required time to both enabling the regulator and to changing the
voltage.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/power/regulator/regulator-uclass.c | 45 +++++++++++++++++++++-
 include/power/regulator.h                  |  2 +
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
index 39e46279d533..4119f244c74b 100644
--- a/drivers/power/regulator/regulator-uclass.c
+++ b/drivers/power/regulator/regulator-uclass.c
@@ -35,10 +35,22 @@ int regulator_get_value(struct udevice *dev)
 	return ops->get_value(dev);
 }
 
+static void regulator_set_value_delay(struct udevice *dev, int old_uV,
+				      int new_uV, unsigned int ramp_delay)
+{
+	int delay = DIV_ROUND_UP(abs(new_uV - old_uV), ramp_delay);
+
+	debug("regulator %s: delay %u us (%d uV -> %d uV)\n", dev->name,
+	      delay, old_uV, new_uV);
+
+	udelay(delay);
+}
+
 int regulator_set_value(struct udevice *dev, int uV)
 {
 	const struct dm_regulator_ops *ops = dev_get_driver_ops(dev);
 	struct dm_regulator_uclass_platdata *uc_pdata;
+	int ret, old_uV = uV;
 
 	uc_pdata = dev_get_uclass_platdata(dev);
 	if (uc_pdata->min_uV != -ENODATA && uV < uc_pdata->min_uV)
@@ -49,7 +61,18 @@ int regulator_set_value(struct udevice *dev, int uV)
 	if (!ops || !ops->set_value)
 		return -ENOSYS;
 
-	return ops->set_value(dev, uV);
+	if (uc_pdata->ramp_delay)
+		old_uV = regulator_get_value(dev);
+
+	ret = ops->set_value(dev, uV);
+
+	if (!ret) {
+		if (uc_pdata->ramp_delay && old_uV > 0)
+			regulator_set_value_delay(dev, old_uV, uV,
+						  uc_pdata->ramp_delay);
+	}
+
+	return ret;
 }
 
 /*
@@ -107,6 +130,7 @@ int regulator_set_enable(struct udevice *dev, bool enable)
 {
 	const struct dm_regulator_ops *ops = dev_get_driver_ops(dev);
 	struct dm_regulator_uclass_platdata *uc_pdata;
+	int ret, old_enable = 0;
 
 	if (!ops || !ops->set_enable)
 		return -ENOSYS;
@@ -115,7 +139,22 @@ int regulator_set_enable(struct udevice *dev, bool enable)
 	if (!enable && uc_pdata->always_on)
 		return 0;
 
-	return ops->set_enable(dev, enable);
+	if (uc_pdata->ramp_delay)
+		old_enable = regulator_get_enable(dev);
+
+	ret = ops->set_enable(dev, enable);
+	if (!ret) {
+		if (uc_pdata->ramp_delay && !old_enable) {
+			int uV = regulator_get_value(dev);
+
+			if (uV > 0) {
+				regulator_set_value_delay(dev, 0, uV,
+							  uc_pdata->ramp_delay);
+			}
+		}
+	}
+
+	return ret;
 }
 
 int regulator_get_mode(struct udevice *dev)
@@ -324,6 +363,8 @@ static int regulator_pre_probe(struct udevice *dev)
 						-ENODATA);
 	uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
 	uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
+	uc_pdata->ramp_delay = dev_read_u32_default(dev, "regulator-ramp-delay",
+						    0);
 
 	/* Those values are optional (-ENODATA if unset) */
 	if ((uc_pdata->min_uV != -ENODATA) &&
diff --git a/include/power/regulator.h b/include/power/regulator.h
index 5318ab3acece..c13fa1f336e5 100644
--- a/include/power/regulator.h
+++ b/include/power/regulator.h
@@ -149,6 +149,7 @@ enum regulator_flag {
  * @max_uA*    - maximum amperage (micro Amps)
  * @always_on* - bool type, true or false
  * @boot_on*   - bool type, true or false
+ * @ramp_delay - Time to settle down after voltage change (unit: uV/us)
  * TODO(sjg at chromium.org): Consider putting the above two into @flags
  * @flags:     - flags value (see REGULATOR_FLAG_...)
  * @name**     - fdt regulator name - should be taken from the device tree
@@ -169,6 +170,7 @@ struct dm_regulator_uclass_platdata {
 	int max_uV;
 	int min_uA;
 	int max_uA;
+	unsigned int ramp_delay;
 	bool always_on;
 	bool boot_on;
 	const char *name;
-- 
2.17.1

[U-Boot] [RFT 5/8] power: regulator: s2mps11: Fix step for LDO27 and LDO35

[Krzysztof Kozlowski]

LDO27 and LDO35 have 25 mV step, not 50 mV.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/power/regulator/s2mps11_regulator.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/power/regulator/s2mps11_regulator.c b/drivers/power/regulator/s2mps11_regulator.c
index ced504eb1476..723d27f67c9a 100644
--- a/drivers/power/regulator/s2mps11_regulator.c
+++ b/drivers/power/regulator/s2mps11_regulator.c
@@ -346,6 +346,8 @@ static int s2mps11_ldo_hex2volt(int ldo, int hex)
 	case 11:
 	case 22:
 	case 23:
+	case 27:
+	case 35:
 		uV = hex * S2MPS11_LDO_STEP + S2MPS11_LDO_UV_MIN;
 		break;
 	default:
@@ -366,6 +368,8 @@ static int s2mps11_ldo_volt2hex(int ldo, int uV)
 	case 11:
 	case 22:
 	case 23:
+	case 27:
+	case 35:
 		hex = (uV - S2MPS11_LDO_UV_MIN) / S2MPS11_LDO_STEP;
 		break;
 	default:
-- 
2.17.1

[U-Boot] [RFT 6/8] power: regulator: s2mps11: Add enable delay

[Krzysztof Kozlowski]

According to datasheet, the output on LDO regulators will start
appearing after 10-15 us.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/power/regulator/s2mps11_regulator.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/power/regulator/s2mps11_regulator.c b/drivers/power/regulator/s2mps11_regulator.c
index 723d27f67c9a..1f1581852ee2 100644
--- a/drivers/power/regulator/s2mps11_regulator.c
+++ b/drivers/power/regulator/s2mps11_regulator.c
@@ -551,7 +551,14 @@ static int ldo_get_enable(struct udevice *dev)
 
 static int ldo_set_enable(struct udevice *dev, bool enable)
 {
-	return s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable);
+	int ret;
+
+	ret = s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable);
+
+	/* Wait the "enable delay" for voltage to start to rise */
+	udelay(15);
+
+	return ret;
 }
 
 static int ldo_get_mode(struct udevice *dev)
-- 
2.17.1

[U-Boot] [RFT 7/8] arm: dts: exynos: Add supply for ADC block to Odroid XU3 family

[Krzysztof Kozlowski]

The ADC block requires VDD supply to be on so provide one.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 arch/arm/dts/exynos5422-odroidxu3.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/exynos5422-odroidxu3.dts b/arch/arm/dts/exynos5422-odroidxu3.dts
index e859dd1b981a..9dfae90667cf 100644
--- a/arch/arm/dts/exynos5422-odroidxu3.dts
+++ b/arch/arm/dts/exynos5422-odroidxu3.dts
@@ -32,6 +32,7 @@
 
 	adc at 12D10000 {
 		u-boot,dm-pre-reloc;
+		vdd-supply = <&ldo4_reg>;
 		status = "okay";
 	};
 
-- 
2.17.1

[U-Boot] [RFT 8/8] arm: dts: exynos: Add ramp delay property to LDO regulators to Odroid XU3 family

[Krzysztof Kozlowski]

Add startup time to LDO regulators of S2MPS11 PMIC on Odroid XU3/XU4/HC1
family of boards to be sure the voltage is proper before relying on the
regulator.

The datasheet for all the S2MPS1x family is inconsistent here and does
not specify unambiguously the value of ramp delay for LDO.  It mentions
30 mV/us in one timing diagram but then omits it completely in LDO
regulator characteristics table (it is specified for bucks).

However the vendor kernels for Galaxy S5 and Odroid XU3 use values of 12
mV/us or 24 mV/us.

Without the ramp delay value the consumers do not wait for voltage
settle after changing it.  Although the proper value of ramp delay for
LDOs is unknown, it seems safer to use at least some value from
reference kernel than to leave it unset.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 arch/arm/dts/exynos5422-odroidxu3.dts | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm/dts/exynos5422-odroidxu3.dts b/arch/arm/dts/exynos5422-odroidxu3.dts
index 9dfae90667cf..04ecc404f907 100644
--- a/arch/arm/dts/exynos5422-odroidxu3.dts
+++ b/arch/arm/dts/exynos5422-odroidxu3.dts
@@ -45,6 +45,7 @@
 					regulator-name = "vdd_ldo1";
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1000000>;
+					regulator-ramp-delay = <12000>;
 					regulator-always-on;
 				};
 
@@ -52,18 +53,21 @@
 					regulator-name = "vddq_mmc0";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
+					regulator-ramp-delay = <12000>;
 				};
 
 				ldo4_reg: LDO4 {
 					regulator-name = "vdd_adc";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
+					regulator-ramp-delay = <12000>;
 				};
 
 				ldo5_reg: LDO5 {
 					regulator-name = "vdd_ldo5";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
+					regulator-ramp-delay = <12000>;
 					regulator-always-on;
 				};
 
@@ -71,6 +75,7 @@
 					regulator-name = "vdd_ldo6";
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1000000>;
+					regulator-ramp-delay = <12000>;
 					regulator-always-on;
 				};
 
@@ -78,6 +83,7 @@
 					regulator-name = "vdd_ldo7";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
+					regulator-ramp-delay = <12000>;
 					regulator-always-on;
 				};
 
@@ -85,6 +91,7 @@
 					regulator-name = "vdd_ldo8";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
+					regulator-ramp-delay = <12000>;
 					regulator-always-on;
 				};
 
@@ -92,6 +99,7 @@
 					regulator-name = "vdd_ldo9";
 					regulator-min-microvolt = <3000000>;
 					regulator-max-microvolt = <3000000>;
+					regulator-ramp-delay = <12000>;
 					regulator-always-on;
 				};
 
@@ -99,6 +107,7 @@
 					regulator-name = "vdd_ldo10";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
+					regulator-ramp-delay = <12000>;
 					regulator-always-on;
 				};
 
@@ -106,6 +115,7 @@
 					regulator-name = "vdd_ldo11";
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1000000>;
+					regulator-ramp-delay = <12000>;
 					regulator-always-on;
 				};
 
@@ -113,6 +123,7 @@
 					regulator-name = "vdd_ldo12";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
+					regulator-ramp-delay = <12000>;
 					regulator-always-on;
 				};
 
@@ -120,12 +131,14 @@
 					regulator-name = "vddq_mmc2";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
+					regulator-ramp-delay = <12000>;
 				};
 
 				ldo15_reg: LDO15 {
 					regulator-name = "vdd_ldo15";
 					regulator-min-microvolt = <3300000>;
 					regulator-max-microvolt = <3300000>;
+					regulator-ramp-delay = <12000>;
 					regulator-always-on;
 				};
 
@@ -133,6 +146,7 @@
 					regulator-name = "vdd_ldo16";
 					regulator-min-microvolt = <2200000>;
 					regulator-max-microvolt = <2200000>;
+					regulator-ramp-delay = <12000>;
 					regulator-always-on;
 				};
 
@@ -140,6 +154,7 @@
 					regulator-name = "vdd_ldo17";
 					regulator-min-microvolt = <3300000>;
 					regulator-max-microvolt = <3300000>;
+					regulator-ramp-delay = <12000>;
 					regulator-always-on;
 				};
 
@@ -147,18 +162,21 @@
 					regulator-name = "vdd_emmc_1V8";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
+					regulator-ramp-delay = <12000>;
 				};
 
 				ldo19_reg: LDO19 {
 					regulator-name = "vdd_sd";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
+					regulator-ramp-delay = <12000>;
 				};
 
 				ldo24_reg: LDO24 {
 					regulator-name = "tsp_io";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
+					regulator-ramp-delay = <12000>;
 					regulator-always-on;
 				};
 
@@ -166,6 +184,7 @@
 					regulator-name = "vdd_ldo26";
 					regulator-min-microvolt = <3000000>;
 					regulator-max-microvolt = <3000000>;
+					regulator-ramp-delay = <12000>;
 					regulator-always-on;
 				};
 
-- 
2.17.1

[U-Boot] [RFT 4/8] regulator: Add support for ramp delay

[Simon Glass]

Hi Krzysztof,

On Sat, 9 Feb 2019 at 16:54, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> Changing voltage and enabling regulator might require delays so the
> regulator stabilizes at expected level.
>
> Add support for "regulator-ramp-delay" binding which can introduce
> required time to both enabling the regulator and to changing the
> voltage.

Is this binding used in Linux? Can you please add binding
documentation for this?

>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  drivers/power/regulator/regulator-uclass.c | 45 +++++++++++++++++++++-
>  include/power/regulator.h                  |  2 +
>  2 files changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
> index 39e46279d533..4119f244c74b 100644
> --- a/drivers/power/regulator/regulator-uclass.c
> +++ b/drivers/power/regulator/regulator-uclass.c
> @@ -35,10 +35,22 @@ int regulator_get_value(struct udevice *dev)
>         return ops->get_value(dev);
>  }
>
> +static void regulator_set_value_delay(struct udevice *dev, int old_uV,
> +                                     int new_uV, unsigned int ramp_delay)
> +{
> +       int delay = DIV_ROUND_UP(abs(new_uV - old_uV), ramp_delay);

So the ramp delay is microseconds per (microvolt delta)?

> +
> +       debug("regulator %s: delay %u us (%d uV -> %d uV)\n", dev->name,
> +             delay, old_uV, new_uV);
> +
> +       udelay(delay);
> +}
> +
>  int regulator_set_value(struct udevice *dev, int uV)
>  {
>         const struct dm_regulator_ops *ops = dev_get_driver_ops(dev);
>         struct dm_regulator_uclass_platdata *uc_pdata;
> +       int ret, old_uV = uV;
>
>         uc_pdata = dev_get_uclass_platdata(dev);
>         if (uc_pdata->min_uV != -ENODATA && uV < uc_pdata->min_uV)
> @@ -49,7 +61,18 @@ int regulator_set_value(struct udevice *dev, int uV)
>         if (!ops || !ops->set_value)
>                 return -ENOSYS;
>
> -       return ops->set_value(dev, uV);
> +       if (uc_pdata->ramp_delay)
> +               old_uV = regulator_get_value(dev);
> +
> +       ret = ops->set_value(dev, uV);
> +
> +       if (!ret) {
> +               if (uc_pdata->ramp_delay && old_uV > 0)
> +                       regulator_set_value_delay(dev, old_uV, uV,
> +                                                 uc_pdata->ramp_delay);
> +       }
> +
> +       return ret;
>  }
>
>  /*
> @@ -107,6 +130,7 @@ int regulator_set_enable(struct udevice *dev, bool enable)
>  {
>         const struct dm_regulator_ops *ops = dev_get_driver_ops(dev);
>         struct dm_regulator_uclass_platdata *uc_pdata;
> +       int ret, old_enable = 0;
>
>         if (!ops || !ops->set_enable)
>                 return -ENOSYS;
> @@ -115,7 +139,22 @@ int regulator_set_enable(struct udevice *dev, bool enable)
>         if (!enable && uc_pdata->always_on)
>                 return 0;
>
> -       return ops->set_enable(dev, enable);
> +       if (uc_pdata->ramp_delay)
> +               old_enable = regulator_get_enable(dev);
> +
> +       ret = ops->set_enable(dev, enable);
> +       if (!ret) {
> +               if (uc_pdata->ramp_delay && !old_enable) {
> +                       int uV = regulator_get_value(dev);
> +
> +                       if (uV > 0) {
> +                               regulator_set_value_delay(dev, 0, uV,
> +                                                         uc_pdata->ramp_delay);
> +                       }

How come there is a delay when enabling it as well as when setting the voltage?

> +               }
> +       }
> +
> +       return ret;
>  }
>
>  int regulator_get_mode(struct udevice *dev)
> @@ -324,6 +363,8 @@ static int regulator_pre_probe(struct udevice *dev)
>                                                 -ENODATA);
>         uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
>         uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
> +       uc_pdata->ramp_delay = dev_read_u32_default(dev, "regulator-ramp-delay",
> +                                                   0);
>
>         /* Those values are optional (-ENODATA if unset) */
>         if ((uc_pdata->min_uV != -ENODATA) &&
> diff --git a/include/power/regulator.h b/include/power/regulator.h
> index 5318ab3acece..c13fa1f336e5 100644
> --- a/include/power/regulator.h
> +++ b/include/power/regulator.h
> @@ -149,6 +149,7 @@ enum regulator_flag {
>   * @max_uA*    - maximum amperage (micro Amps)
>   * @always_on* - bool type, true or false
>   * @boot_on*   - bool type, true or false
> + * @ramp_delay - Time to settle down after voltage change (unit: uV/us)

us/uV isn't it?

>   * TODO(sjg at chromium.org): Consider putting the above two into @flags

This comment needs updating or moving up.

>   * @flags:     - flags value (see REGULATOR_FLAG_...)
>   * @name**     - fdt regulator name - should be taken from the device tree
> @@ -169,6 +170,7 @@ struct dm_regulator_uclass_platdata {
>         int max_uV;
>         int min_uA;
>         int max_uA;
> +       unsigned int ramp_delay;
>         bool always_on;
>         bool boot_on;
>         const char *name;
> --
> 2.17.1
>

Regards,
Simon

[U-Boot] [RFT 6/8] power: regulator: s2mps11: Add enable delay

[Lukasz Majewski]

Hi Krzysztof,

> According to datasheet, the output on LDO regulators will start
> appearing after 10-15 us.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  drivers/power/regulator/s2mps11_regulator.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/regulator/s2mps11_regulator.c
> b/drivers/power/regulator/s2mps11_regulator.c index
> 723d27f67c9a..1f1581852ee2 100644 ---
> a/drivers/power/regulator/s2mps11_regulator.c +++
> b/drivers/power/regulator/s2mps11_regulator.c @@ -551,7 +551,14 @@
> static int ldo_get_enable(struct udevice *dev) 
>  static int ldo_set_enable(struct udevice *dev, bool enable)
>  {
> -	return s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable);
> +	int ret;
> +
> +	ret = s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable);
> +
> +	/* Wait the "enable delay" for voltage to start to rise */
> +	udelay(15);

I assume, that this value is the same as in the Linux driver?

> +
> +	return ret;
>  }
>  
>  static int ldo_get_mode(struct udevice *dev)




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190211/6dfd218b/attachment.sig>

[U-Boot] [RFT 8/8] arm: dts: exynos: Add ramp delay property to LDO regulators to Odroid XU3 family

[Lukasz Majewski]

Hi Krzysztof,

> Add startup time to LDO regulators of S2MPS11 PMIC on Odroid
> XU3/XU4/HC1 family of boards to be sure the voltage is proper before
> relying on the regulator.
> 
> The datasheet for all the S2MPS1x family is inconsistent here and does
> not specify unambiguously the value of ramp delay for LDO.  It
> mentions 30 mV/us in one timing diagram but then omits it completely
> in LDO regulator characteristics table (it is specified for bucks).
> 
> However the vendor kernels for Galaxy S5 and Odroid XU3 use values of
> 12 mV/us or 24 mV/us.
> 
> Without the ramp delay value the consumers do not wait for voltage
> settle after changing it.  Although the proper value of ramp delay for
> LDOs is unknown, it seems safer to use at least some value from
> reference kernel than to leave it unset.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Reviewed-by: Lukasz Majewski <lukma@denx.de>

> ---
>  arch/arm/dts/exynos5422-odroidxu3.dts | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/arch/arm/dts/exynos5422-odroidxu3.dts
> b/arch/arm/dts/exynos5422-odroidxu3.dts index
> 9dfae90667cf..04ecc404f907 100644 ---
> a/arch/arm/dts/exynos5422-odroidxu3.dts +++
> b/arch/arm/dts/exynos5422-odroidxu3.dts @@ -45,6 +45,7 @@
>  					regulator-name = "vdd_ldo1";
>  					regulator-min-microvolt =
> <1000000>; regulator-max-microvolt = <1000000>;
> +					regulator-ramp-delay =
> <12000>; regulator-always-on;
>  				};
>  
> @@ -52,18 +53,21 @@
>  					regulator-name = "vddq_mmc0";
>  					regulator-min-microvolt =
> <1800000>; regulator-max-microvolt = <1800000>;
> +					regulator-ramp-delay =
> <12000>; };
>  
>  				ldo4_reg: LDO4 {
>  					regulator-name = "vdd_adc";
>  					regulator-min-microvolt =
> <1800000>; regulator-max-microvolt = <1800000>;
> +					regulator-ramp-delay =
> <12000>; };
>  
>  				ldo5_reg: LDO5 {
>  					regulator-name = "vdd_ldo5";
>  					regulator-min-microvolt =
> <1800000>; regulator-max-microvolt = <1800000>;
> +					regulator-ramp-delay =
> <12000>; regulator-always-on;
>  				};
>  
> @@ -71,6 +75,7 @@
>  					regulator-name = "vdd_ldo6";
>  					regulator-min-microvolt =
> <1000000>; regulator-max-microvolt = <1000000>;
> +					regulator-ramp-delay =
> <12000>; regulator-always-on;
>  				};
>  
> @@ -78,6 +83,7 @@
>  					regulator-name = "vdd_ldo7";
>  					regulator-min-microvolt =
> <1800000>; regulator-max-microvolt = <1800000>;
> +					regulator-ramp-delay =
> <12000>; regulator-always-on;
>  				};
>  
> @@ -85,6 +91,7 @@
>  					regulator-name = "vdd_ldo8";
>  					regulator-min-microvolt =
> <1800000>; regulator-max-microvolt = <1800000>;
> +					regulator-ramp-delay =
> <12000>; regulator-always-on;
>  				};
>  
> @@ -92,6 +99,7 @@
>  					regulator-name = "vdd_ldo9";
>  					regulator-min-microvolt =
> <3000000>; regulator-max-microvolt = <3000000>;
> +					regulator-ramp-delay =
> <12000>; regulator-always-on;
>  				};
>  
> @@ -99,6 +107,7 @@
>  					regulator-name = "vdd_ldo10";
>  					regulator-min-microvolt =
> <1800000>; regulator-max-microvolt = <1800000>;
> +					regulator-ramp-delay =
> <12000>; regulator-always-on;
>  				};
>  
> @@ -106,6 +115,7 @@
>  					regulator-name = "vdd_ldo11";
>  					regulator-min-microvolt =
> <1000000>; regulator-max-microvolt = <1000000>;
> +					regulator-ramp-delay =
> <12000>; regulator-always-on;
>  				};
>  
> @@ -113,6 +123,7 @@
>  					regulator-name = "vdd_ldo12";
>  					regulator-min-microvolt =
> <1800000>; regulator-max-microvolt = <1800000>;
> +					regulator-ramp-delay =
> <12000>; regulator-always-on;
>  				};
>  
> @@ -120,12 +131,14 @@
>  					regulator-name = "vddq_mmc2";
>  					regulator-min-microvolt =
> <2800000>; regulator-max-microvolt = <2800000>;
> +					regulator-ramp-delay =
> <12000>; };
>  
>  				ldo15_reg: LDO15 {
>  					regulator-name = "vdd_ldo15";
>  					regulator-min-microvolt =
> <3300000>; regulator-max-microvolt = <3300000>;
> +					regulator-ramp-delay =
> <12000>; regulator-always-on;
>  				};
>  
> @@ -133,6 +146,7 @@
>  					regulator-name = "vdd_ldo16";
>  					regulator-min-microvolt =
> <2200000>; regulator-max-microvolt = <2200000>;
> +					regulator-ramp-delay =
> <12000>; regulator-always-on;
>  				};
>  
> @@ -140,6 +154,7 @@
>  					regulator-name = "vdd_ldo17";
>  					regulator-min-microvolt =
> <3300000>; regulator-max-microvolt = <3300000>;
> +					regulator-ramp-delay =
> <12000>; regulator-always-on;
>  				};
>  
> @@ -147,18 +162,21 @@
>  					regulator-name =
> "vdd_emmc_1V8"; regulator-min-microvolt = <1800000>;
>  					regulator-max-microvolt =
> <1800000>;
> +					regulator-ramp-delay =
> <12000>; };
>  
>  				ldo19_reg: LDO19 {
>  					regulator-name = "vdd_sd";
>  					regulator-min-microvolt =
> <2800000>; regulator-max-microvolt = <2800000>;
> +					regulator-ramp-delay =
> <12000>; };
>  
>  				ldo24_reg: LDO24 {
>  					regulator-name = "tsp_io";
>  					regulator-min-microvolt =
> <2800000>; regulator-max-microvolt = <2800000>;
> +					regulator-ramp-delay =
> <12000>; regulator-always-on;
>  				};
>  
> @@ -166,6 +184,7 @@
>  					regulator-name = "vdd_ldo26";
>  					regulator-min-microvolt =
> <3000000>; regulator-max-microvolt = <3000000>;
> +					regulator-ramp-delay =
> <12000>; regulator-always-on;
>  				};
>  




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190211/f9bfe399/attachment.sig>

[U-Boot] [RFT 1/8] exynos: Redo detection of revision when all resources are ready

[Lukasz Majewski]

Hi Krzysztof,

> Detection of board type is done early - before power setup.  In case
> of Odroid XU3/XU4/HC1 family, the detection is done using ADC which
> is supplied by LDO4/VDD_ADC regulator.  This regulator could be turned
> off (e.g. by kernel before reboot);  If ADC is used early, the
> regulators are not yet available and the detection won't work.
> 
> Try to detect the revision again, once power is brought up.
> 
> This is necessary to fix the detection of Odroid HC1 after reboot, if
> kernel turned off the LDO4 regulator.  Otherwise the board is not
> detected....

But such approach seems not to be the optimal one (as we perform
detection twice - with default LDO4 enabled after power on and after
soft reset).

I would expect to enable the LDO4 regulator in the early code (I2C
would be probably necessary) and then read ADC value properly once.

(I also guess that the "work-by-chance" approach is caused by default
settings of PMIC after power on).

As fair as I remember, TI is able to read the EEPROM via I2C in the
very early u-boot (MLO to be precise) code and then make the decision
regarding the platform.

Maybe it would be possible to do the same with Samsung?

And another thought - if the set_board_type() can be called latter and
it works - why cannot we move it to this latter point and execute
exactly once?

> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  board/samsung/common/board.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/board/samsung/common/board.c
> b/board/samsung/common/board.c index 6fd26a3a9198..1e2dabe68d11 100644
> --- a/board/samsung/common/board.c
> +++ b/board/samsung/common/board.c
> @@ -147,6 +147,11 @@ int board_early_init_f(void)
>  {
>  	int err;
>  #ifdef CONFIG_BOARD_TYPES
> +	/*
> +	 * It is done early so power might not be set up yet.  In
> such case
> +	 * specific revision detection with ADC might not work and
> need to me
> +	 * redone later.
> +	 */
>  	set_board_type();
>  #endif
>  	err = board_uart_init();
> @@ -166,9 +171,21 @@ int board_early_init_f(void)
>  #if defined(CONFIG_POWER) || defined(CONFIG_DM_PMIC)
>  int power_init_board(void)
>  {
> +	int ret;
> +
>  	set_ps_hold_ctrl();
>  
> -	return exynos_power_init();
> +	ret = exynos_power_init();
> +
> +#ifdef CONFIG_BOARD_TYPES
> +	/*
> +	 * Since power is on, redo the board detection (external
> peripherals
> +	 * are on).
> +	 */
> +	set_board_type();
> +#endif
> +
> +	return ret;
>  }
>  #endif
>  




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190211/c5071abd/attachment.sig>

[U-Boot] [RFT 1/8] exynos: Redo detection of revision when all resources are ready

[Krzysztof Kozlowski]

On Mon, 11 Feb 2019 at 08:20, Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Krzysztof,
>
> > Detection of board type is done early - before power setup.  In case
> > of Odroid XU3/XU4/HC1 family, the detection is done using ADC which
> > is supplied by LDO4/VDD_ADC regulator.  This regulator could be turned
> > off (e.g. by kernel before reboot);  If ADC is used early, the
> > regulators are not yet available and the detection won't work.
> >
> > Try to detect the revision again, once power is brought up.
> >
> > This is necessary to fix the detection of Odroid HC1 after reboot, if
> > kernel turned off the LDO4 regulator.  Otherwise the board is not
> > detected....
>
> But such approach seems not to be the optimal one (as we perform
> detection twice - with default LDO4 enabled after power on and after
> soft reset).
>
> I would expect to enable the LDO4 regulator in the early code (I2C
> would be probably necessary) and then read ADC value properly once.
>
> (I also guess that the "work-by-chance" approach is caused by default
> settings of PMIC after power on).

So basically you want to move the board detection after the
exynos_power_init()... maybe it is possible. The other way is to split
set_board_type() into OF part (for compatible and main board
difference) and revision detection (requiring ADC). Maybe it is
possible, but isn't it used before?

> As fair as I remember, TI is able to read the EEPROM via I2C in the
> very early u-boot (MLO to be precise) code and then make the decision
> regarding the platform.
>
> Maybe it would be possible to do the same with Samsung?
>
> And another thought - if the set_board_type() can be called latter and
> it works - why cannot we move it to this latter point and execute
> exactly once?

It is the same as previous idea... or I do not see the difference...

Best regards,
Krzysztof

[U-Boot] [RFT 1/8] exynos: Redo detection of revision when all resources are ready

[Lukasz Majewski]

Hi Krzysztof,

> On Mon, 11 Feb 2019 at 08:20, Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Hi Krzysztof,
> >  
> > > Detection of board type is done early - before power setup.  In
> > > case of Odroid XU3/XU4/HC1 family, the detection is done using
> > > ADC which is supplied by LDO4/VDD_ADC regulator.  This regulator
> > > could be turned off (e.g. by kernel before reboot);  If ADC is
> > > used early, the regulators are not yet available and the
> > > detection won't work.
> > >
> > > Try to detect the revision again, once power is brought up.
> > >
> > > This is necessary to fix the detection of Odroid HC1 after
> > > reboot, if kernel turned off the LDO4 regulator.  Otherwise the
> > > board is not detected....  
> >
> > But such approach seems not to be the optimal one (as we perform
> > detection twice - with default LDO4 enabled after power on and after
> > soft reset).
> >
> > I would expect to enable the LDO4 regulator in the early code (I2C
> > would be probably necessary) and then read ADC value properly once.
> >
> > (I also guess that the "work-by-chance" approach is caused by
> > default settings of PMIC after power on).  
> 
> So basically you want to move the board detection after the
> exynos_power_init()... maybe it is possible. The other way is to split
> set_board_type() into OF part (for compatible and main board
> difference) and revision detection (requiring ADC). Maybe it is
> possible, but isn't it used before?

I do want to avoid making the detection twice;

First time when we have the LDO4 enabled by default and the second time
when it may happen that we do a soft reset from Linux (which disabled
LDO4).

> 
> > As fair as I remember, TI is able to read the EEPROM via I2C in the
> > very early u-boot (MLO to be precise) code and then make the
> > decision regarding the platform.
> >
> > Maybe it would be possible to do the same with Samsung?
> >
> > And another thought - if the set_board_type() can be called latter
> > and it works - why cannot we move it to this latter point and
> > execute exactly once?  
> 
> It is the same as previous idea... or I do not see the difference...
> 
> Best regards,
> Krzysztof




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190211/5ec9aac1/attachment.sig>

[U-Boot] [RFT 4/8] regulator: Add support for ramp delay

[Krzysztof Kozlowski]

On Sun, 10 Feb 2019 at 10:49, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Krzysztof,
>
> On Sat, 9 Feb 2019 at 16:54, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > Changing voltage and enabling regulator might require delays so the
> > regulator stabilizes at expected level.
> >
> > Add support for "regulator-ramp-delay" binding which can introduce
> > required time to both enabling the regulator and to changing the
> > voltage.
>
> Is this binding used in Linux? Can you please add binding
> documentation for this?

Yes, these are bindings from the kernel. I will add them.

>
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
> >  drivers/power/regulator/regulator-uclass.c | 45 +++++++++++++++++++++-
> >  include/power/regulator.h                  |  2 +
> >  2 files changed, 45 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c
> > index 39e46279d533..4119f244c74b 100644
> > --- a/drivers/power/regulator/regulator-uclass.c
> > +++ b/drivers/power/regulator/regulator-uclass.c
> > @@ -35,10 +35,22 @@ int regulator_get_value(struct udevice *dev)
> >         return ops->get_value(dev);
> >  }
> >
> > +static void regulator_set_value_delay(struct udevice *dev, int old_uV,
> > +                                     int new_uV, unsigned int ramp_delay)
> > +{
> > +       int delay = DIV_ROUND_UP(abs(new_uV - old_uV), ramp_delay);
>
> So the ramp delay is microseconds per (microvolt delta)?

The ramp delay is microvolt per microseconds.
int delay = uv / (uV/uS) = uS

>
> > +
> > +       debug("regulator %s: delay %u us (%d uV -> %d uV)\n", dev->name,
> > +             delay, old_uV, new_uV);
> > +
> > +       udelay(delay);
> > +}
> > +
> >  int regulator_set_value(struct udevice *dev, int uV)
> >  {
> >         const struct dm_regulator_ops *ops = dev_get_driver_ops(dev);
> >         struct dm_regulator_uclass_platdata *uc_pdata;
> > +       int ret, old_uV = uV;
> >
> >         uc_pdata = dev_get_uclass_platdata(dev);
> >         if (uc_pdata->min_uV != -ENODATA && uV < uc_pdata->min_uV)
> > @@ -49,7 +61,18 @@ int regulator_set_value(struct udevice *dev, int uV)
> >         if (!ops || !ops->set_value)
> >                 return -ENOSYS;
> >
> > -       return ops->set_value(dev, uV);
> > +       if (uc_pdata->ramp_delay)
> > +               old_uV = regulator_get_value(dev);
> > +
> > +       ret = ops->set_value(dev, uV);
> > +
> > +       if (!ret) {
> > +               if (uc_pdata->ramp_delay && old_uV > 0)
> > +                       regulator_set_value_delay(dev, old_uV, uV,
> > +                                                 uc_pdata->ramp_delay);
> > +       }
> > +
> > +       return ret;
> >  }
> >
> >  /*
> > @@ -107,6 +130,7 @@ int regulator_set_enable(struct udevice *dev, bool enable)
> >  {
> >         const struct dm_regulator_ops *ops = dev_get_driver_ops(dev);
> >         struct dm_regulator_uclass_platdata *uc_pdata;
> > +       int ret, old_enable = 0;
> >
> >         if (!ops || !ops->set_enable)
> >                 return -ENOSYS;
> > @@ -115,7 +139,22 @@ int regulator_set_enable(struct udevice *dev, bool enable)
> >         if (!enable && uc_pdata->always_on)
> >                 return 0;
> >
> > -       return ops->set_enable(dev, enable);
> > +       if (uc_pdata->ramp_delay)
> > +               old_enable = regulator_get_enable(dev);
> > +
> > +       ret = ops->set_enable(dev, enable);
> > +       if (!ret) {
> > +               if (uc_pdata->ramp_delay && !old_enable) {
> > +                       int uV = regulator_get_value(dev);
> > +
> > +                       if (uV > 0) {
> > +                               regulator_set_value_delay(dev, 0, uV,
> > +                                                         uc_pdata->ramp_delay);
> > +                       }
>
> How come there is a delay when enabling it as well as when setting the voltage?

Enabling a regulator also requires the time, which might be sum of:
1. Initial enable delay (not included here),
2. Rising of voltage delay (ramp delay) from 0 to expected.

In Linux kernel this is separate property regulator-enable-ramp-delay.
Here I reused the ramp delay to make it simpler.

However indeed there is no point to wait with ramp delay if the
regulator is disabled.

>
> > +               }
> > +       }
> > +
> > +       return ret;
> >  }
> >
> >  int regulator_get_mode(struct udevice *dev)
> > @@ -324,6 +363,8 @@ static int regulator_pre_probe(struct udevice *dev)
> >                                                 -ENODATA);
> >         uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
> >         uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
> > +       uc_pdata->ramp_delay = dev_read_u32_default(dev, "regulator-ramp-delay",
> > +                                                   0);
> >
> >         /* Those values are optional (-ENODATA if unset) */
> >         if ((uc_pdata->min_uV != -ENODATA) &&
> > diff --git a/include/power/regulator.h b/include/power/regulator.h
> > index 5318ab3acece..c13fa1f336e5 100644
> > --- a/include/power/regulator.h
> > +++ b/include/power/regulator.h
> > @@ -149,6 +149,7 @@ enum regulator_flag {
> >   * @max_uA*    - maximum amperage (micro Amps)
> >   * @always_on* - bool type, true or false
> >   * @boot_on*   - bool type, true or false
> > + * @ramp_delay - Time to settle down after voltage change (unit: uV/us)
>
> us/uV isn't it?

No, opposite.

>
> >   * TODO(sjg at chromium.org): Consider putting the above two into @flags
>
> This comment needs updating or moving up.

Yes, thanks!

Best regards,
Krzysztof

[U-Boot] [RFT 1/8] exynos: Redo detection of revision when all resources are ready

[Krzysztof Kozlowski]

On Mon, 11 Feb 2019 at 09:14, Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Krzysztof,
>
> > On Mon, 11 Feb 2019 at 08:20, Lukasz Majewski <lukma@denx.de> wrote:
> > >
> > > Hi Krzysztof,
> > >
> > > > Detection of board type is done early - before power setup.  In
> > > > case of Odroid XU3/XU4/HC1 family, the detection is done using
> > > > ADC which is supplied by LDO4/VDD_ADC regulator.  This regulator
> > > > could be turned off (e.g. by kernel before reboot);  If ADC is
> > > > used early, the regulators are not yet available and the
> > > > detection won't work.
> > > >
> > > > Try to detect the revision again, once power is brought up.
> > > >
> > > > This is necessary to fix the detection of Odroid HC1 after
> > > > reboot, if kernel turned off the LDO4 regulator.  Otherwise the
> > > > board is not detected....
> > >
> > > But such approach seems not to be the optimal one (as we perform
> > > detection twice - with default LDO4 enabled after power on and after
> > > soft reset).
> > >
> > > I would expect to enable the LDO4 regulator in the early code (I2C
> > > would be probably necessary) and then read ADC value properly once.
> > >
> > > (I also guess that the "work-by-chance" approach is caused by
> > > default settings of PMIC after power on).
> >
> > So basically you want to move the board detection after the
> > exynos_power_init()... maybe it is possible. The other way is to split
> > set_board_type() into OF part (for compatible and main board
> > difference) and revision detection (requiring ADC). Maybe it is
> > possible, but isn't it used before?
>
> I do want to avoid making the detection twice;
>
> First time when we have the LDO4 enabled by default and the second time
> when it may happen that we do a soft reset from Linux (which disabled
> LDO4).

This I understand but isn't the board type used BEFORE the power init?
Power init cannot be moved early as it depends on having proper
resources (as I wrote in commit msg)... so only board detection can be
moved later. But if setting up resources (e.g. regulators) requires
board type then it is circular dependency... so I asked - isn't board
type used before power init?

Best regards,
Krzysztof

[U-Boot] [RFT 6/8] power: regulator: s2mps11: Add enable delay

[Krzysztof Kozlowski]

On Mon, 11 Feb 2019 at 08:11, Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Krzysztof,
>
> > According to datasheet, the output on LDO regulators will start
> > appearing after 10-15 us.
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
> >  drivers/power/regulator/s2mps11_regulator.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/power/regulator/s2mps11_regulator.c
> > b/drivers/power/regulator/s2mps11_regulator.c index
> > 723d27f67c9a..1f1581852ee2 100644 ---
> > a/drivers/power/regulator/s2mps11_regulator.c +++
> > b/drivers/power/regulator/s2mps11_regulator.c @@ -551,7 +551,14 @@
> > static int ldo_get_enable(struct udevice *dev)
> >  static int ldo_set_enable(struct udevice *dev, bool enable)
> >  {
> > -     return s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable);
> > +     int ret;
> > +
> > +     ret = s2mps11_ldo_enable(dev, PMIC_OP_SET, &enable);
> > +
> > +     /* Wait the "enable delay" for voltage to start to rise */
> > +     udelay(15);
>
> I assume, that this value is the same as in the Linux driver?

No, Linux drivers does not do it. It should... but we never
implemented it there.

Best regards,
Krzysztof

[U-Boot] [RFT 1/8] exynos: Redo detection of revision when all resources are ready

[Minkyu Kang]

On 11/02/2019 17:17, Krzysztof Kozlowski wrote:
> On Mon, 11 Feb 2019 at 09:14, Lukasz Majewski <lukma@denx.de> wrote:
>>
>> Hi Krzysztof,
>>
>>> On Mon, 11 Feb 2019 at 08:20, Lukasz Majewski <lukma@denx.de> wrote:
>>>>
>>>> Hi Krzysztof,
>>>>
>>>>> Detection of board type is done early - before power setup.  In
>>>>> case of Odroid XU3/XU4/HC1 family, the detection is done using
>>>>> ADC which is supplied by LDO4/VDD_ADC regulator.  This regulator
>>>>> could be turned off (e.g. by kernel before reboot);  If ADC is
>>>>> used early, the regulators are not yet available and the
>>>>> detection won't work.
>>>>>
>>>>> Try to detect the revision again, once power is brought up.
>>>>>
>>>>> This is necessary to fix the detection of Odroid HC1 after
>>>>> reboot, if kernel turned off the LDO4 regulator.  Otherwise the
>>>>> board is not detected....
>>>>
>>>> But such approach seems not to be the optimal one (as we perform
>>>> detection twice - with default LDO4 enabled after power on and after
>>>> soft reset).
>>>>
>>>> I would expect to enable the LDO4 regulator in the early code (I2C
>>>> would be probably necessary) and then read ADC value properly once.
>>>>
>>>> (I also guess that the "work-by-chance" approach is caused by
>>>> default settings of PMIC after power on).
>>>
>>> So basically you want to move the board detection after the
>>> exynos_power_init()... maybe it is possible. The other way is to split
>>> set_board_type() into OF part (for compatible and main board
>>> difference) and revision detection (requiring ADC). Maybe it is
>>> possible, but isn't it used before?
>>
>> I do want to avoid making the detection twice;

I agreed.
And if you want to move set_board_type, then please use proper function such as a board_late_init or a misc_init. set_board_type doesn't have any relation with power_init

>>
>> First time when we have the LDO4 enabled by default and the second time
>> when it may happen that we do a soft reset from Linux (which disabled
>> LDO4).
> 
> This I understand but isn't the board type used BEFORE the power init?
> Power init cannot be moved early as it depends on having proper
> resources (as I wrote in commit msg)... so only board detection can be
> moved later. But if setting up resources (e.g. regulators) requires
> board type then it is circular dependency... so I asked - isn't board
> type used before power init?
> 

I'm not sure but, it looks that can be moved to after power_init.
But, please verify carefully before you re-post.

Thanks,
Minkyu Kang.