Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Tony Lindgren]

* Maciej Purski  [180312 12:24]:
> On 03/09/2018 04:58 PM, Tony Lindgren wrote:
> > * Mark Brown  [180309 12:43]:
> > > On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:
> > > 
> > > > I would like to kindly ask Fabio Estevam and Tony Lindgren to test the 
> > > > patch
> > > > series on their boards.
> > 
> > I gave it a quick try and this set still causes at least mmc0
> > to fail for me.
>
> Thanks. Here's a small patch, which adds some debugs. Maybe they will reveal,
> where the problem is.

Sorry for the delay, now back from ELC. I tried applying this on
top of Linux next + your six patches but it fails to apply. Do
I need something else too?

Regards,

Tony


> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index f1f11cf..0e80ba5 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2280,7 +2280,6 @@ int regulator_enable(struct regulator *regulator)
>  {
>   struct regulator_dev *rdev = regulator->rdev;
>   int ret = 0;
> - int ret2;
> 
>   if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
>   rdev_err(rdev, "not all coupled regulators registered\n");
> @@ -2298,15 +2297,9 @@ int regulator_enable(struct regulator *regulator)
> 
>   regulator_lock_dependent(rdev);
>   ret = _regulator_enable(rdev);
> - ret2 = regulator_balance_voltage(rdev, PM_SUSPEND_ON);
> + regulator_balance_voltage(rdev, PM_SUSPEND_ON);
>   regulator_unlock_dependent(rdev);
> 
> - if (ret2 != 0) {
> - rdev_err(rdev,
> - "balancing failed when trying to enable regulator: %d",
> - ret2);
> - }
> -
>   if (ret != 0 && rdev->supply)
>   regulator_disable(rdev->supply);
> 
> @@ -3149,7 +3142,7 @@ static int regulator_set_voltage_rdev(struct 
> regulator_dev *rdev, int min_uV,
>   ret = regulator_set_voltage_unlocked(rdev->supply,
>   best_supply_uV, INT_MAX, state);
>   if (ret)
> - dev_err(>dev, "Failed to decrease supply voltage: 
> %d\n",
> + dev_warn(>dev, "Failed to decrease supply 
> voltage: %d\n",
>   ret);
>   /* No need to fail here */
>   ret = 0;
> @@ -3332,11 +3325,8 @@ static int regulator_balance_voltage(struct 
> regulator_dev *rdev,
>   ret = regulator_set_voltage_rdev(best_rdev, best_uV,
>best_uV, state);
> 
> - if (ret < 0) {
> - rdev_err(rdev,
> - "Failed to set voltage with error: %d", ret);
> + if (ret < 0)
>   goto out;
> - }
>   }
> 
>  out:
> 
> 

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Tony Lindgren]

* Maciej Purski  [180312 12:24]:
> On 03/09/2018 04:58 PM, Tony Lindgren wrote:
> > * Mark Brown  [180309 12:43]:
> > > On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:
> > > 
> > > > I would like to kindly ask Fabio Estevam and Tony Lindgren to test the 
> > > > patch
> > > > series on their boards.
> > 
> > I gave it a quick try and this set still causes at least mmc0
> > to fail for me.
>
> Thanks. Here's a small patch, which adds some debugs. Maybe they will reveal,
> where the problem is.

Sorry for the delay, now back from ELC. I tried applying this on
top of Linux next + your six patches but it fails to apply. Do
I need something else too?

Regards,

Tony


> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index f1f11cf..0e80ba5 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2280,7 +2280,6 @@ int regulator_enable(struct regulator *regulator)
>  {
>   struct regulator_dev *rdev = regulator->rdev;
>   int ret = 0;
> - int ret2;
> 
>   if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
>   rdev_err(rdev, "not all coupled regulators registered\n");
> @@ -2298,15 +2297,9 @@ int regulator_enable(struct regulator *regulator)
> 
>   regulator_lock_dependent(rdev);
>   ret = _regulator_enable(rdev);
> - ret2 = regulator_balance_voltage(rdev, PM_SUSPEND_ON);
> + regulator_balance_voltage(rdev, PM_SUSPEND_ON);
>   regulator_unlock_dependent(rdev);
> 
> - if (ret2 != 0) {
> - rdev_err(rdev,
> - "balancing failed when trying to enable regulator: %d",
> - ret2);
> - }
> -
>   if (ret != 0 && rdev->supply)
>   regulator_disable(rdev->supply);
> 
> @@ -3149,7 +3142,7 @@ static int regulator_set_voltage_rdev(struct 
> regulator_dev *rdev, int min_uV,
>   ret = regulator_set_voltage_unlocked(rdev->supply,
>   best_supply_uV, INT_MAX, state);
>   if (ret)
> - dev_err(>dev, "Failed to decrease supply voltage: 
> %d\n",
> + dev_warn(>dev, "Failed to decrease supply 
> voltage: %d\n",
>   ret);
>   /* No need to fail here */
>   ret = 0;
> @@ -3332,11 +3325,8 @@ static int regulator_balance_voltage(struct 
> regulator_dev *rdev,
>   ret = regulator_set_voltage_rdev(best_rdev, best_uV,
>best_uV, state);
> 
> - if (ret < 0) {
> - rdev_err(rdev,
> - "Failed to set voltage with error: %d", ret);
> + if (ret < 0)
>   goto out;
> - }
>   }
> 
>  out:
> 
> 

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Maciej Purski]

On 03/09/2018 04:58 PM, Tony Lindgren wrote:

* Mark Brown  [180309 12:43]:

On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:


I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
series on their boards.


I gave it a quick try and this set still causes at least mmc0
to fail for me.

Regards,

Tony




Thanks. Here's a small patch, which adds some debugs. Maybe they will reveal,
where the problem is.

Best Regards,
Maciej Purski

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f1f11cf..0e80ba5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2280,7 +2280,6 @@ int regulator_enable(struct regulator *regulator)
 {
struct regulator_dev *rdev = regulator->rdev;
int ret = 0;
-   int ret2;

if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
rdev_err(rdev, "not all coupled regulators registered\n");
@@ -2298,15 +2297,9 @@ int regulator_enable(struct regulator *regulator)

regulator_lock_dependent(rdev);
ret = _regulator_enable(rdev);
-   ret2 = regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+   regulator_balance_voltage(rdev, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);

-   if (ret2 != 0) {
-   rdev_err(rdev,
-   "balancing failed when trying to enable regulator: %d",
-   ret2);
-   }
-
if (ret != 0 && rdev->supply)
regulator_disable(rdev->supply);

@@ -3149,7 +3142,7 @@ static int regulator_set_voltage_rdev(struct 
regulator_dev *rdev, int min_uV,
ret = regulator_set_voltage_unlocked(rdev->supply,
best_supply_uV, INT_MAX, state);
if (ret)
-   dev_err(>dev, "Failed to decrease supply voltage: 
%d\n",
+   dev_warn(>dev, "Failed to decrease supply voltage: 
%d\n",
ret);
/* No need to fail here */
ret = 0;
@@ -3332,11 +3325,8 @@ static int regulator_balance_voltage(struct 
regulator_dev *rdev,
ret = regulator_set_voltage_rdev(best_rdev, best_uV,
 best_uV, state);

-   if (ret < 0) {
-   rdev_err(rdev,
-   "Failed to set voltage with error: %d", ret);
+   if (ret < 0)
goto out;
-   }
}

 out:

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Maciej Purski]

On 03/09/2018 04:58 PM, Tony Lindgren wrote:

* Mark Brown  [180309 12:43]:

On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:


I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
series on their boards.


I gave it a quick try and this set still causes at least mmc0
to fail for me.

Regards,

Tony




Thanks. Here's a small patch, which adds some debugs. Maybe they will reveal,
where the problem is.

Best Regards,
Maciej Purski

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f1f11cf..0e80ba5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2280,7 +2280,6 @@ int regulator_enable(struct regulator *regulator)
 {
struct regulator_dev *rdev = regulator->rdev;
int ret = 0;
-   int ret2;

if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled) {
rdev_err(rdev, "not all coupled regulators registered\n");
@@ -2298,15 +2297,9 @@ int regulator_enable(struct regulator *regulator)

regulator_lock_dependent(rdev);
ret = _regulator_enable(rdev);
-   ret2 = regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+   regulator_balance_voltage(rdev, PM_SUSPEND_ON);
regulator_unlock_dependent(rdev);

-   if (ret2 != 0) {
-   rdev_err(rdev,
-   "balancing failed when trying to enable regulator: %d",
-   ret2);
-   }
-
if (ret != 0 && rdev->supply)
regulator_disable(rdev->supply);

@@ -3149,7 +3142,7 @@ static int regulator_set_voltage_rdev(struct 
regulator_dev *rdev, int min_uV,
ret = regulator_set_voltage_unlocked(rdev->supply,
best_supply_uV, INT_MAX, state);
if (ret)
-   dev_err(>dev, "Failed to decrease supply voltage: 
%d\n",
+   dev_warn(>dev, "Failed to decrease supply voltage: 
%d\n",
ret);
/* No need to fail here */
ret = 0;
@@ -3332,11 +3325,8 @@ static int regulator_balance_voltage(struct 
regulator_dev *rdev,
ret = regulator_set_voltage_rdev(best_rdev, best_uV,
 best_uV, state);

-   if (ret < 0) {
-   rdev_err(rdev,
-   "Failed to set voltage with error: %d", ret);
+   if (ret < 0)
goto out;
-   }
}

 out:

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Maciej Purski]

On 03/09/2018 01:56 PM, Mark Brown wrote:

On Fri, Mar 09, 2018 at 01:50:20PM +0100, Maciej Purski wrote:

On 03/09/2018 01:42 PM, Mark Brown wrote:



Is it possible to respin this in terms of the test/coupled branch in my
git tree?  There were a bunch of build fixes that got sent and I don't
want to loose any of those.



I have included those build fixes in my new patchset. I can extract the
newest changes and rebase them against your test/coupled branch, if that's
what you're asking for.


That was what I was asking for, yeah (though it can wait till people
manage to test).  It would also be helpful from a review point of view
to see what the fix is.



Hi,
this is the patch containing all the new changes applied to coupled regulators.


From 3b073fa86bd55feda7d3855c0c776aad6b659f67 Mon Sep 17 00:00:00 2001
From: Maciej Purski 
Date: Fri, 9 Mar 2018 14:02:03 +0100
Subject: [PATCH] regulator: core: Make locks re-entrant

Setting voltage, enabling/disabling regulators requires operations on
all regulators related with the regulator being changed. Therefore,
all of them should be locked for the whole operation. With the current
locking implementation, adding additional dependency (regulators
coupling) causes deadlocks in some cases.

Introduce a possibility to attempt to lock a mutex multiple times
by the same task without waiting on a mutex. This should handle all
reasonable coupling-supplying combinations, especially when two coupled
regulators share common supplies. The only situation that should be
forbidden is simultaneous coupling and supplying between a pair of
regulators.

The idea is based on clk core.

Signed-off-by: Maciej Purski 
---
 drivers/regulator/core.c | 214 ---
 include/linux/regulator/driver.h |   2 +
 2 files changed, 113 insertions(+), 103 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e685f8b..1999290 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -96,6 +96,7 @@ struct regulator_supply_alias {
const char *alias_supply;
 };

+
 static int _regulator_is_enabled(struct regulator_dev *rdev);
 static int _regulator_disable(struct regulator_dev *rdev);
 static int _regulator_get_voltage(struct regulator_dev *rdev);
@@ -151,65 +152,81 @@ static inline struct regulator_dev *rdev_get_supply(struct 
regulator_dev *rdev)

return NULL;
 }

-/**
- * regulator_lock_supply - lock a regulator and its supplies
- * @rdev: regulator source
- */
-static void regulator_lock_supply(struct regulator_dev *rdev)
+static void regulator_lock_nested(struct regulator_dev *rdev,
+ unsigned int subclass)
 {
-   int i;
+   if (!mutex_trylock(>mutex)) {
+   if (rdev->mutex_owner == current) {
+   rdev->ref_cnt++;
+   return;
+   }
+   mutex_lock_nested(>mutex, subclass);
+   }

-   for (i = 0; rdev; rdev = rdev_get_supply(rdev), i++)
-   mutex_lock_nested(>mutex, i);
+   rdev->ref_cnt = 1;
+   rdev->mutex_owner = current;
 }

-/**
- * regulator_unlock_supply - unlock a regulator and its supplies
- * @rdev: regulator source
- */
-static void regulator_unlock_supply(struct regulator_dev *rdev)
+static void regulator_unlock(struct regulator_dev *rdev)
 {
-   struct regulator *supply;
+   if (rdev->ref_cnt != 0) {
+   rdev->ref_cnt--;

-   while (1) {
-   mutex_unlock(>mutex);
-   supply = rdev->supply;
+   if (!rdev->ref_cnt) {
+   rdev->mutex_owner = NULL;
+   mutex_unlock(>mutex);
+   }
+   }
+}

-   if (!rdev->supply)
-   return;
+static int regulator_lock_recursive(struct regulator_dev *rdev,
+   unsigned int subclass)
+{
+   struct regulator_dev *c_rdev;
+   int i;
+
+   for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
+   c_rdev = rdev->coupling_desc.coupled_rdevs[i];
+
+   if (!c_rdev)
+   continue;
+
+   regulator_lock_nested(c_rdev, subclass++);

-   rdev = supply->rdev;
+   if (c_rdev->supply)
+   subclass = 
regulator_lock_recursive(c_rdev->supply->rdev,
+   subclass);
}
+
+   return subclass;
 }

-/**
- * regulator_lock_coupled - lock a group of coupled regulators
- * @rdev:  regulator source
- */
-static void regulator_lock_coupled(struct regulator_dev *rdev)
+static void regulator_unlock_dependent(struct regulator_dev *rdev)
 {
-   struct coupling_desc *c_desc = >coupling_desc;
-   int n_coupled = c_desc->n_coupled;
+   struct regulator_dev *c_rdev;
int i;

-   for (i = 0; i < n_coupled; i++)
-  

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Maciej Purski]

On 03/09/2018 01:56 PM, Mark Brown wrote:

On Fri, Mar 09, 2018 at 01:50:20PM +0100, Maciej Purski wrote:

On 03/09/2018 01:42 PM, Mark Brown wrote:



Is it possible to respin this in terms of the test/coupled branch in my
git tree?  There were a bunch of build fixes that got sent and I don't
want to loose any of those.



I have included those build fixes in my new patchset. I can extract the
newest changes and rebase them against your test/coupled branch, if that's
what you're asking for.


That was what I was asking for, yeah (though it can wait till people
manage to test).  It would also be helpful from a review point of view
to see what the fix is.



Hi,
this is the patch containing all the new changes applied to coupled regulators.


From 3b073fa86bd55feda7d3855c0c776aad6b659f67 Mon Sep 17 00:00:00 2001
From: Maciej Purski 
Date: Fri, 9 Mar 2018 14:02:03 +0100
Subject: [PATCH] regulator: core: Make locks re-entrant

Setting voltage, enabling/disabling regulators requires operations on
all regulators related with the regulator being changed. Therefore,
all of them should be locked for the whole operation. With the current
locking implementation, adding additional dependency (regulators
coupling) causes deadlocks in some cases.

Introduce a possibility to attempt to lock a mutex multiple times
by the same task without waiting on a mutex. This should handle all
reasonable coupling-supplying combinations, especially when two coupled
regulators share common supplies. The only situation that should be
forbidden is simultaneous coupling and supplying between a pair of
regulators.

The idea is based on clk core.

Signed-off-by: Maciej Purski 
---
 drivers/regulator/core.c | 214 ---
 include/linux/regulator/driver.h |   2 +
 2 files changed, 113 insertions(+), 103 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e685f8b..1999290 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -96,6 +96,7 @@ struct regulator_supply_alias {
const char *alias_supply;
 };

+
 static int _regulator_is_enabled(struct regulator_dev *rdev);
 static int _regulator_disable(struct regulator_dev *rdev);
 static int _regulator_get_voltage(struct regulator_dev *rdev);
@@ -151,65 +152,81 @@ static inline struct regulator_dev *rdev_get_supply(struct 
regulator_dev *rdev)

return NULL;
 }

-/**
- * regulator_lock_supply - lock a regulator and its supplies
- * @rdev: regulator source
- */
-static void regulator_lock_supply(struct regulator_dev *rdev)
+static void regulator_lock_nested(struct regulator_dev *rdev,
+ unsigned int subclass)
 {
-   int i;
+   if (!mutex_trylock(>mutex)) {
+   if (rdev->mutex_owner == current) {
+   rdev->ref_cnt++;
+   return;
+   }
+   mutex_lock_nested(>mutex, subclass);
+   }

-   for (i = 0; rdev; rdev = rdev_get_supply(rdev), i++)
-   mutex_lock_nested(>mutex, i);
+   rdev->ref_cnt = 1;
+   rdev->mutex_owner = current;
 }

-/**
- * regulator_unlock_supply - unlock a regulator and its supplies
- * @rdev: regulator source
- */
-static void regulator_unlock_supply(struct regulator_dev *rdev)
+static void regulator_unlock(struct regulator_dev *rdev)
 {
-   struct regulator *supply;
+   if (rdev->ref_cnt != 0) {
+   rdev->ref_cnt--;

-   while (1) {
-   mutex_unlock(>mutex);
-   supply = rdev->supply;
+   if (!rdev->ref_cnt) {
+   rdev->mutex_owner = NULL;
+   mutex_unlock(>mutex);
+   }
+   }
+}

-   if (!rdev->supply)
-   return;
+static int regulator_lock_recursive(struct regulator_dev *rdev,
+   unsigned int subclass)
+{
+   struct regulator_dev *c_rdev;
+   int i;
+
+   for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
+   c_rdev = rdev->coupling_desc.coupled_rdevs[i];
+
+   if (!c_rdev)
+   continue;
+
+   regulator_lock_nested(c_rdev, subclass++);

-   rdev = supply->rdev;
+   if (c_rdev->supply)
+   subclass = 
regulator_lock_recursive(c_rdev->supply->rdev,
+   subclass);
}
+
+   return subclass;
 }

-/**
- * regulator_lock_coupled - lock a group of coupled regulators
- * @rdev:  regulator source
- */
-static void regulator_lock_coupled(struct regulator_dev *rdev)
+static void regulator_unlock_dependent(struct regulator_dev *rdev)
 {
-   struct coupling_desc *c_desc = >coupling_desc;
-   int n_coupled = c_desc->n_coupled;
+   struct regulator_dev *c_rdev;
int i;

-   for (i = 0; i < n_coupled; i++)
-   if (c_desc->coupled_rdevs[i])

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Tony Lindgren]

* Mark Brown  [180309 12:43]:
> On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:
> 
> > I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
> > series on their boards.

I gave it a quick try and this set still causes at least mmc0
to fail for me.

Regards,

Tony

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Tony Lindgren]

* Mark Brown  [180309 12:43]:
> On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:
> 
> > I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
> > series on their boards.

I gave it a quick try and this set still causes at least mmc0
to fail for me.

Regards,

Tony

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Mark Brown]

On Fri, Mar 09, 2018 at 01:50:20PM +0100, Maciej Purski wrote:
> On 03/09/2018 01:42 PM, Mark Brown wrote:

> > Is it possible to respin this in terms of the test/coupled branch in my
> > git tree?  There were a bunch of build fixes that got sent and I don't
> > want to loose any of those.

> I have included those build fixes in my new patchset. I can extract the
> newest changes and rebase them against your test/coupled branch, if that's
> what you're asking for.

That was what I was asking for, yeah (though it can wait till people
manage to test).  It would also be helpful from a review point of view
to see what the fix is.


signature.asc
Description: PGP signature

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Mark Brown]

On Fri, Mar 09, 2018 at 01:50:20PM +0100, Maciej Purski wrote:
> On 03/09/2018 01:42 PM, Mark Brown wrote:

> > Is it possible to respin this in terms of the test/coupled branch in my
> > git tree?  There were a bunch of build fixes that got sent and I don't
> > want to loose any of those.

> I have included those build fixes in my new patchset. I can extract the
> newest changes and rebase them against your test/coupled branch, if that's
> what you're asking for.

That was what I was asking for, yeah (though it can wait till people
manage to test).  It would also be helpful from a review point of view
to see what the fix is.


signature.asc
Description: PGP signature

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Maciej Purski]

On 03/09/2018 01:42 PM, Mark Brown wrote:

On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:


I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
series on their boards.


That'd be great, yeah - I can run some tests with kernelci as well,
though the latency is relatively high there.

Is it possible to respin this in terms of the test/coupled branch in my
git tree?  There were a bunch of build fixes that got sent and I don't
want to loose any of those.



I have included those build fixes in my new patchset. I can extract the newest 
changes and rebase them against your test/coupled branch, if that's what you're 
asking for.

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Maciej Purski]

On 03/09/2018 01:42 PM, Mark Brown wrote:

On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:


I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
series on their boards.


That'd be great, yeah - I can run some tests with kernelci as well,
though the latency is relatively high there.

Is it possible to respin this in terms of the test/coupled branch in my
git tree?  There were a bunch of build fixes that got sent and I don't
want to loose any of those.



I have included those build fixes in my new patchset. I can extract the newest 
changes and rebase them against your test/coupled branch, if that's what you're 
asking for.

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Mark Brown]

On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:

> I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
> series on their boards.

That'd be great, yeah - I can run some tests with kernelci as well,
though the latency is relatively high there.

Is it possible to respin this in terms of the test/coupled branch in my
git tree?  There were a bunch of build fixes that got sent and I don't
want to loose any of those.


signature.asc
Description: PGP signature

Re: [PATCH v6 0/5] Add coupled regulators mechanism

[Mark Brown]

On Fri, Mar 09, 2018 at 01:22:02PM +0100, Maciej Purski wrote:

> I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
> series on their boards.

That'd be great, yeah - I can run some tests with kernelci as well,
though the latency is relatively high there.

Is it possible to respin this in terms of the test/coupled branch in my
git tree?  There were a bunch of build fixes that got sent and I don't
want to loose any of those.


signature.asc
Description: PGP signature

[PATCH v6 0/5] Add coupled regulators mechanism

[Maciej Purski]

Hi all,

this patchset adds a new mechanism to the framework - regulators' coupling.

On Odroid XU3/4 and other Exynos5422 based boards there is a case, that
different devices on the board are supplied by different regulators
with non-fixed voltages. If one of these devices temporarily requires 
higher voltage, there might occur a situation that the spread between 
devices' voltages is so high, that there is a risk of changing
'high' and 'low' states on the interconnection between devices powered
by those regulators. 

Algorithmicaly the problem was solved by: 
Inderpal Singh 
Doug Anderson 

The discussion on that subject can be found here:
https://lkml.org/lkml/2014/4/29/28

Therefore this patchset is an attempt to apply the idea to regulators core
as concluded in the discussion by Mark Brown and Doug Anderson.

This feature is required to enable support for generic CPUfreq 
and devfreq drivers for the mentioned boards. 

The current assumption is that an uncoupled regulator is a special case
of a coupled one, so they should share a common voltage setting path.

The previous version caused locking problems on some boards. The discussion
can be found here:
https://lkml.org/lkml/2018/3/5/965

Current idea, which should solve the locking issues is making it possible to try
to lock a mutex multiple times by a single task just like it is done in 
clk/clk.c.
This should handle all possible coupling-supply combinations except coupling 
with
a supply.

I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
series on their boards.

Best regards,

Maciej Purski

---
Changes in v6:
- change locking model
- fix build errors on non-of architectures
- update bindings

Changes in v5:
- rebase against current Mark Brown's regulators next

Changes in v4:
- make paths for coupled and uncoupled regulators common
- coupling descriptors are now always present in regulator_dev
- fail to probe if data inconsistency is detected
- retry to resolve coupling regultors in late init call
- rebase on top of linux-next 20180119
- fix commit messages
- split patches to make the patchset easier to review

Changes in v3:
- move dts parsing code to of_regulator.c, in order to the so,
  add a new commit in which of_regulator_find_by_node() is moved
  to of_regulator.c as well
- improve error messages
- move max_spread variable to constraints
- perform resolving of coupled regulators under a list mutex
- remove useless locking functions
- some minor refactorization
- improve commit messages

Changes in RFC v2:
- allow coupling n regulators (in fact up to constant value, now
  set to 10)
- change algorithm to be more readable
- introduce better locking
- add more comments
- split first patch into two
- update commit messages
- change sequence of the patches

Maciej Purski (6):
  regulator: core: Make locks re-entrant
  regulator: bindings: Add properties for coupled regulators
  regulator: core: Parse coupled regulators properties
  regulator: core: Resolve coupled regulators
  regulator: core: Add voltage balancing mechanism
  regulator: core: Change voltage setting path

 .../devicetree/bindings/regulator/regulator.txt|   5 +
 drivers/regulator/core.c   | 549 ++---
 drivers/regulator/internal.h   |  28 +-
 drivers/regulator/of_regulator.c   | 151 ++
 include/linux/regulator/driver.h   |  20 +
 include/linux/regulator/machine.h  |   4 +
 6 files changed, 677 insertions(+), 80 deletions(-)

-- 
2.7.4

[PATCH v6 0/5] Add coupled regulators mechanism

[Maciej Purski]

Hi all,

this patchset adds a new mechanism to the framework - regulators' coupling.

On Odroid XU3/4 and other Exynos5422 based boards there is a case, that
different devices on the board are supplied by different regulators
with non-fixed voltages. If one of these devices temporarily requires 
higher voltage, there might occur a situation that the spread between 
devices' voltages is so high, that there is a risk of changing
'high' and 'low' states on the interconnection between devices powered
by those regulators. 

Algorithmicaly the problem was solved by: 
Inderpal Singh 
Doug Anderson 

The discussion on that subject can be found here:
https://lkml.org/lkml/2014/4/29/28

Therefore this patchset is an attempt to apply the idea to regulators core
as concluded in the discussion by Mark Brown and Doug Anderson.

This feature is required to enable support for generic CPUfreq 
and devfreq drivers for the mentioned boards. 

The current assumption is that an uncoupled regulator is a special case
of a coupled one, so they should share a common voltage setting path.

The previous version caused locking problems on some boards. The discussion
can be found here:
https://lkml.org/lkml/2018/3/5/965

Current idea, which should solve the locking issues is making it possible to try
to lock a mutex multiple times by a single task just like it is done in 
clk/clk.c.
This should handle all possible coupling-supply combinations except coupling 
with
a supply.

I would like to kindly ask Fabio Estevam and Tony Lindgren to test the patch
series on their boards.

Best regards,

Maciej Purski

---
Changes in v6:
- change locking model
- fix build errors on non-of architectures
- update bindings

Changes in v5:
- rebase against current Mark Brown's regulators next

Changes in v4:
- make paths for coupled and uncoupled regulators common
- coupling descriptors are now always present in regulator_dev
- fail to probe if data inconsistency is detected
- retry to resolve coupling regultors in late init call
- rebase on top of linux-next 20180119
- fix commit messages
- split patches to make the patchset easier to review

Changes in v3:
- move dts parsing code to of_regulator.c, in order to the so,
  add a new commit in which of_regulator_find_by_node() is moved
  to of_regulator.c as well
- improve error messages
- move max_spread variable to constraints
- perform resolving of coupled regulators under a list mutex
- remove useless locking functions
- some minor refactorization
- improve commit messages

Changes in RFC v2:
- allow coupling n regulators (in fact up to constant value, now
  set to 10)
- change algorithm to be more readable
- introduce better locking
- add more comments
- split first patch into two
- update commit messages
- change sequence of the patches

Maciej Purski (6):
  regulator: core: Make locks re-entrant
  regulator: bindings: Add properties for coupled regulators
  regulator: core: Parse coupled regulators properties
  regulator: core: Resolve coupled regulators
  regulator: core: Add voltage balancing mechanism
  regulator: core: Change voltage setting path

 .../devicetree/bindings/regulator/regulator.txt|   5 +
 drivers/regulator/core.c   | 549 ++---
 drivers/regulator/internal.h   |  28 +-
 drivers/regulator/of_regulator.c   | 151 ++
 include/linux/regulator/driver.h   |  20 +
 include/linux/regulator/machine.h  |   4 +
 6 files changed, 677 insertions(+), 80 deletions(-)

-- 
2.7.4