[PATCH 0/2] regulator: Fix enable delay handling

[PATCH 0/2] regulator: Fix enable delay handling

[Sascha Hauer]

The fixed regulator has a "off-on-delay-us" property which we wrongly
interpret as delay needed until a regulator has been fully ramped up
and a "startup-delay-us" which exists for that purpose, but we didn't
handle at all. This series fixes this.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
Sascha Hauer (2):
      regulator: fix handling of off_on_delay
      regulator: fixed: handle startup-delay-us property

 drivers/regulator/core.c  | 14 +++++++++++---
 drivers/regulator/fixed.c |  3 +++
 include/regulator.h       |  3 +++
 3 files changed, 17 insertions(+), 3 deletions(-)
---
base-commit: 0024921364eb4c8bc8089fdc198440b0d67a239f
change-id: 20260129-regulator-ramp-40e98c515312

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>

[PATCH 1/2] regulator: fix handling of off_on_delay

[Sascha Hauer]

In Linux the off_on_delay field in a regulator is described as:

> The guard time (in uS), before re-enabling a regulator

The primary user is the fixed regulator which puts the value of
the "off-on-delay-us" property into it.

In barebox we put the off_on_delay into the enable_time_us field
which is the delay we introduce when turning on a regulator. This
is wrong and fixed in this commit.

The off_on_delay is defined as the time we should take before
re-enabling a regulator. Linux does it the complicated way of
remembering the time when it was last disabled and delays the
re-enabling if necessary. We use the simple approach here of
just waiting the off_on_delay time in the regulator disable path.

Fixes: 26a4c78917 ("regulator: add support for struct regulator_desc::off_on_delay")
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c | 5 ++++-
 include/regulator.h      | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index cd07955894..8ca937e8bd 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -96,6 +96,9 @@ static int regulator_disable_rdev(struct regulator_dev *rdev)
 
 	rdev->enable_count--;
 
+	if (rdev->off_on_delay)
+		udelay(rdev->off_on_delay);
+
 	return regulator_disable(rdev->supply);
 }
 
@@ -309,7 +312,7 @@ int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
 	node->dev = rdev->dev;
 
 	if (rdev->desc->off_on_delay)
-		rdev->enable_time_us = rdev->desc->off_on_delay;
+		rdev->off_on_delay = rdev->desc->off_on_delay;
 
 	if (rdev->desc->fixed_uV && rdev->desc->n_voltages == 1)
 		rdev->min_uv = rdev->max_uv = rdev->desc->fixed_uV;
diff --git a/include/regulator.h b/include/regulator.h
index bca2d37d1a..09b4969c74 100644
--- a/include/regulator.h
+++ b/include/regulator.h
@@ -123,6 +123,7 @@ struct regulator_dev {
 	struct device_node *node;
 	int enable_count;
 	int enable_time_us;
+	unsigned int off_on_delay;
 	int min_uv;
 	int max_uv;
 	struct list_head consumer_list;

-- 
2.47.3

[PATCH 2/2] regulator: fixed: handle startup-delay-us property

[Sascha Hauer]

The "startup-delay-us" property of the fixed regulator has never been
handled by barebox. Handle it now.

The "startup-delay-us" property is specified as:

| startup time in microseconds

Then there's a regulator generic property "regulator-enable-ramp-delay"
which barebox handles, specified as:

| The time taken, in microseconds, for the supply rail to
| reach the target voltage, plus/minus whatever tolerance the board
| design requires. This property describes the total system ramp time
| required due to the combination of internal ramping of the regulator
| itself, and board design issues such as trace capacitance and load
| on the supply.

We just use the bigger of the two times as the time a regulator needs
to become stable.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/regulator/core.c  | 9 +++++++--
 drivers/regulator/fixed.c | 3 +++
 include/regulator.h       | 2 ++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 8ca937e8bd..1f8d77dc96 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -297,6 +297,7 @@ int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
 {
 	const char *name;
 	int ret;
+	u32 val = 0;
 
 	if (!rdev || !node)
 		return -EINVAL;
@@ -313,12 +314,16 @@ int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
 
 	if (rdev->desc->off_on_delay)
 		rdev->off_on_delay = rdev->desc->off_on_delay;
+	if (rdev->desc->enable_time_us)
+		rdev->enable_time_us = rdev->desc->enable_time_us;
 
 	if (rdev->desc->fixed_uV && rdev->desc->n_voltages == 1)
 		rdev->min_uv = rdev->max_uv = rdev->desc->fixed_uV;
 
-	of_property_read_u32(node, "regulator-enable-ramp-delay",
-			&rdev->enable_time_us);
+	of_property_read_u32(node, "regulator-enable-ramp-delay", &val);
+	if (val > rdev->enable_time_us)
+		rdev->enable_time_us = val;
+
 	of_property_read_u32(node, "regulator-min-microvolt",
 			&rdev->min_uv);
 	of_property_read_u32(node, "regulator-max-microvolt",
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 0edb5ceb10..e00519c2fe 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -61,6 +61,9 @@ static int regulator_fixed_probe(struct device *dev)
 	if (!of_property_read_u32(np, "off-on-delay-us", &delay))
 		fix->rdesc.off_on_delay = delay;
 
+	if (!of_property_read_u32(np, "startup-delay-us", &delay))
+		fix->rdesc.enable_time_us = delay;
+
 	if (of_find_property(np, "vin-supply", NULL))
 		fix->rdesc.supply_name = "vin";
 
diff --git a/include/regulator.h b/include/regulator.h
index 09b4969c74..7ae69a7f54 100644
--- a/include/regulator.h
+++ b/include/regulator.h
@@ -63,6 +63,7 @@ struct regulator_bulk_data {
  * @volt_table: Voltage mapping table (if table based mapping)
  * @fixed_uV: Fixed voltage of rails.
  * @off_on_delay: guard time (in uS), before re-enabling a regulator
+ * @enable_time_us: Time taken for initial enable of regulator (in uS).
  */
 
 struct regulator_desc {
@@ -93,6 +94,7 @@ struct regulator_desc {
 	const unsigned int *volt_table;
 	int fixed_uV;
 	unsigned int off_on_delay;
+	unsigned int enable_time_us;
 };
 
 /**

-- 
2.47.3

Re: [PATCH 1/2] regulator: fix handling of off_on_delay

[Marco Felsch]

On 26-01-29, Sascha Hauer wrote:
> In Linux the off_on_delay field in a regulator is described as:
> 
> > The guard time (in uS), before re-enabling a regulator
> 
> The primary user is the fixed regulator which puts the value of
> the "off-on-delay-us" property into it.
> 
> In barebox we put the off_on_delay into the enable_time_us field
> which is the delay we introduce when turning on a regulator. This
> is wrong and fixed in this commit.
> 
> The off_on_delay is defined as the time we should take before
> re-enabling a regulator. Linux does it the complicated way of
> remembering the time when it was last disabled and delays the
> re-enabling if necessary. We use the simple approach here of
> just waiting the off_on_delay time in the regulator disable path.
> 
> Fixes: 26a4c78917 ("regulator: add support for struct regulator_desc::off_on_delay")
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/regulator/core.c | 5 ++++-
>  include/regulator.h      | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index cd07955894..8ca937e8bd 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -96,6 +96,9 @@ static int regulator_disable_rdev(struct regulator_dev *rdev)
>  
>  	rdev->enable_count--;
>  
> +	if (rdev->off_on_delay)
> +		udelay(rdev->off_on_delay);
> +
>  	return regulator_disable(rdev->supply);
>  }
>  
> @@ -309,7 +312,7 @@ int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
>  	node->dev = rdev->dev;
>  
>  	if (rdev->desc->off_on_delay)
> -		rdev->enable_time_us = rdev->desc->off_on_delay;

Nit: is is worth to remove the enable_time_us completely and
re-introduce it within the next commit?

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

> +		rdev->off_on_delay = rdev->desc->off_on_delay;
>  
>  	if (rdev->desc->fixed_uV && rdev->desc->n_voltages == 1)
>  		rdev->min_uv = rdev->max_uv = rdev->desc->fixed_uV;
> diff --git a/include/regulator.h b/include/regulator.h
> index bca2d37d1a..09b4969c74 100644
> --- a/include/regulator.h
> +++ b/include/regulator.h
> @@ -123,6 +123,7 @@ struct regulator_dev {
>  	struct device_node *node;
>  	int enable_count;
>  	int enable_time_us;
> +	unsigned int off_on_delay;
>  	int min_uv;
>  	int max_uv;
>  	struct list_head consumer_list;
> 
> -- 
> 2.47.3
> 
> 
> 

-- 
#gernperDu 
#CallMeByMyFirstName

Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | https://www.pengutronix.de/ |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |

Re: [PATCH 2/2] regulator: fixed: handle startup-delay-us property

[Marco Felsch]

On 26-01-29, Sascha Hauer wrote:
> The "startup-delay-us" property of the fixed regulator has never been
> handled by barebox. Handle it now.
> 
> The "startup-delay-us" property is specified as:
> 
> | startup time in microseconds
> 
> Then there's a regulator generic property "regulator-enable-ramp-delay"
> which barebox handles, specified as:
> 
> | The time taken, in microseconds, for the supply rail to
> | reach the target voltage, plus/minus whatever tolerance the board
> | design requires. This property describes the total system ramp time
> | required due to the combination of internal ramping of the regulator
> | itself, and board design issues such as trace capacitance and load
> | on the supply.
> 
> We just use the bigger of the two times as the time a regulator needs
> to become stable.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/regulator/core.c  | 9 +++++++--
>  drivers/regulator/fixed.c | 3 +++
>  include/regulator.h       | 2 ++
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 8ca937e8bd..1f8d77dc96 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -297,6 +297,7 @@ int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
>  {
>  	const char *name;
>  	int ret;
> +	u32 val = 0;
>  
>  	if (!rdev || !node)
>  		return -EINVAL;
> @@ -313,12 +314,16 @@ int of_regulator_register(struct regulator_dev *rdev, struct device_node *node)
>  
>  	if (rdev->desc->off_on_delay)
>  		rdev->off_on_delay = rdev->desc->off_on_delay;
> +	if (rdev->desc->enable_time_us)
> +		rdev->enable_time_us = rdev->desc->enable_time_us;
>  
>  	if (rdev->desc->fixed_uV && rdev->desc->n_voltages == 1)
>  		rdev->min_uv = rdev->max_uv = rdev->desc->fixed_uV;
>  
> -	of_property_read_u32(node, "regulator-enable-ramp-delay",
> -			&rdev->enable_time_us);
> +	of_property_read_u32(node, "regulator-enable-ramp-delay", &val);
> +	if (val > rdev->enable_time_us)

Nit: Worth a comment to make it more obvious?

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>


> +		rdev->enable_time_us = val;
> +
>  	of_property_read_u32(node, "regulator-min-microvolt",
>  			&rdev->min_uv);
>  	of_property_read_u32(node, "regulator-max-microvolt",
> diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
> index 0edb5ceb10..e00519c2fe 100644
> --- a/drivers/regulator/fixed.c
> +++ b/drivers/regulator/fixed.c
> @@ -61,6 +61,9 @@ static int regulator_fixed_probe(struct device *dev)
>  	if (!of_property_read_u32(np, "off-on-delay-us", &delay))
>  		fix->rdesc.off_on_delay = delay;
>  
> +	if (!of_property_read_u32(np, "startup-delay-us", &delay))
> +		fix->rdesc.enable_time_us = delay;
> +
>  	if (of_find_property(np, "vin-supply", NULL))
>  		fix->rdesc.supply_name = "vin";
>  
> diff --git a/include/regulator.h b/include/regulator.h
> index 09b4969c74..7ae69a7f54 100644
> --- a/include/regulator.h
> +++ b/include/regulator.h
> @@ -63,6 +63,7 @@ struct regulator_bulk_data {
>   * @volt_table: Voltage mapping table (if table based mapping)
>   * @fixed_uV: Fixed voltage of rails.
>   * @off_on_delay: guard time (in uS), before re-enabling a regulator
> + * @enable_time_us: Time taken for initial enable of regulator (in uS).
>   */
>  
>  struct regulator_desc {
> @@ -93,6 +94,7 @@ struct regulator_desc {
>  	const unsigned int *volt_table;
>  	int fixed_uV;
>  	unsigned int off_on_delay;
> +	unsigned int enable_time_us;
>  };
>  
>  /**
> 
> -- 
> 2.47.3
> 
> 
> 

-- 
#gernperDu 
#CallMeByMyFirstName

Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | https://www.pengutronix.de/ |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |