diff mbox series

[RFC,1/4] regulator: core: Ensure the cached state matches the hardware state in regulator_set_voltage_unlocked()

Message ID 20240605074936.578687-2-prabhakar.mahadev-lad.rj@bp.renesas.com
State New
Headers show
Series Add SD/MMC support for Renesas RZ/V2H(P) SoC | expand

Commit Message

Lad, Prabhakar June 5, 2024, 7:49 a.m. UTC 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Ensure that the cached state matches the hardware setting before
considering it a no-op in regulator_set_voltage_unlocked().

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
Driver code flow:
1> set regulator to 1.8V (BIT0 = 1)
2> Regulator cached state now will be 1.8V
3> Now for some reason driver issues a reset to the IP block
   which resets the registers to default value. In this process
   the regulator is set to 3.3V (BIT0 = 0)
4> Now the driver requests the regulator core to set 1.8V
5> Due to below check of cached state we return back with success
   resulting undesired behaviour.
   
Hence an additional check is introduced to make sure the cache state
is matching with the HW.
---
 drivers/regulator/core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Mark Brown June 6, 2024, 12:05 p.m. UTC | #1 
On Wed, Jun 05, 2024 at 08:49:33AM +0100, Prabhakar wrote:

> Driver code flow:
> 1> set regulator to 1.8V (BIT0 = 1)
> 2> Regulator cached state now will be 1.8V
> 3> Now for some reason driver issues a reset to the IP block
>    which resets the registers to default value. In this process
>    the regulator is set to 3.3V (BIT0 = 0)
> 4> Now the driver requests the regulator core to set 1.8V

If something is resetting the regulator like this that's a problem in
general, we need to either have the driver notify the core when that
happens so it can reconfigure the regulator or have it reapply
configuration directly.  Obviously it's not great to have that happen at
all...
Lad, Prabhakar June 6, 2024, 2:12 p.m. UTC | #2 
Hi Mark,

On Thu, Jun 6, 2024 at 1:05 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Jun 05, 2024 at 08:49:33AM +0100, Prabhakar wrote:
>
> > Driver code flow:
> > 1> set regulator to 1.8V (BIT0 = 1)
> > 2> Regulator cached state now will be 1.8V
> > 3> Now for some reason driver issues a reset to the IP block
> >    which resets the registers to default value. In this process
> >    the regulator is set to 3.3V (BIT0 = 0)
> > 4> Now the driver requests the regulator core to set 1.8V
>
> If something is resetting the regulator like this that's a problem in
> general, we need to either have the driver notify the core when that
> happens so it can reconfigure the regulator or have it reapply
> configuration directly.  Obviously it's not great to have that happen at
> all...
>
Currently I am seeing this problem with SDHI driver. For the voltage
switch operation the MMC core requests the driver to do the change and
similarly the MMC core requests the reset operation.

> Having the core driver notify the core when that happens so it can reconfigure the regulator or have it reapply configuration directly.
Again doing this would be a problem as MMC core also maintains the IOS
states, reconfiguring the regulator would cause conflicts between the
states.

Ulf/Wolfram - please correct me if I'm wrong here.

Cheers,
Prabhakar
Mark Brown June 6, 2024, 2:38 p.m. UTC | #3 
On Thu, Jun 06, 2024 at 03:12:42PM +0100, Lad, Prabhakar wrote:
> On Thu, Jun 6, 2024 at 1:05 PM Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Jun 05, 2024 at 08:49:33AM +0100, Prabhakar wrote:

> > > Driver code flow:
> > > 1> set regulator to 1.8V (BIT0 = 1)
> > > 2> Regulator cached state now will be 1.8V
> > > 3> Now for some reason driver issues a reset to the IP block
> > >    which resets the registers to default value. In this process
> > >    the regulator is set to 3.3V (BIT0 = 0)
> > > 4> Now the driver requests the regulator core to set 1.8V

> > If something is resetting the regulator like this that's a problem in
> > general, we need to either have the driver notify the core when that
> > happens so it can reconfigure the regulator or have it reapply
> > configuration directly.  Obviously it's not great to have that happen at
> > all...

> Currently I am seeing this problem with SDHI driver. For the voltage
> switch operation the MMC core requests the driver to do the change and
> similarly the MMC core requests the reset operation.

> > Having the core driver notify the core when that happens so it can reconfigure the regulator or have it reapply configuration directly.

> Again doing this would be a problem as MMC core also maintains the IOS
> states, reconfiguring the regulator would cause conflicts between the
> states.

If the device can't cope with the requested configuration being applied
why is this going through the regulator API at all?  This just seems
quite confused, putting a bodge in the core like this clearly isn't the
right solution.
diff mbox series

Patch

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 5794f4e9dd52..65ee54b13428 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3765,10 +3765,13 @@  static int regulator_set_voltage_unlocked(struct regulator *regulator,
 
 	/* If we're setting the same range as last time the change
 	 * should be a noop (some cpufreq implementations use the same
-	 * voltage for multiple frequencies, for example).
+	 * voltage for multiple frequencies, for example). Also make sure
+	 * state is the same in HW.
 	 */
-	if (voltage->min_uV == min_uV && voltage->max_uV == max_uV)
-		goto out;
+	if (voltage->min_uV == min_uV && voltage->max_uV == max_uV) {
+		if (regulator_get_voltage_rdev(rdev) == min_uV)
+			goto out;
+	}
 
 	/* If we're trying to set a range that overlaps the current voltage,
 	 * return successfully even though the regulator does not support