[PATCH 0/3] regulator: misc fixes

[PATCH 0/3] regulator: misc fixes

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1984 bytes --]

This series adds couple of a bit unrelated fixes/improvements which I
implemented while working to support yet another ROHM PMIC.

Patch1 addresses a potential problem where the voltage selector for
pickable-ranges is used in same fashion as a 'apply-bit'. Eg, when a
regulator voltage is changed so that also the range changes, the effect
of the range and voltage selector change is not taken into account until
the voltage selector is written to hardware. In such case caching the
voltage-selector value will cause the voltage range change to not change
the actual output voltage.

Patch2 is a fix to ROHM bd71818 and bd71879 low-power state specific
voltage settings where some of the voltages were common to many states.
The old behaviour was that changing voltage for one state caused it to
change for all states. This had a potential impact of accidentally
changing the runtime voltage when trying to configure voltage for a
low-power state, potentially hurting the boot-up process.

Patch3 is just a minor print improvement to help pinpointing unsupported
voltage in device-tree.

Series is based on v6.9

---

Matti Vaittinen (3):
  regulator: don't cache vsel to ensure voltage setting
  regulator: bd71828: Don't overwrite runtime voltages
  regulator: rohm-regulator: warn if unsupported voltage is set

 drivers/regulator/bd71828-regulator.c | 58 +--------------------------
 drivers/regulator/helpers.c           | 10 ++++-
 drivers/regulator/rohm-regulator.c    |  4 ++
 3 files changed, 14 insertions(+), 58 deletions(-)


base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
-- 
2.44.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 1/3] regulator: don't cache vsel to ensure voltage setting

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2035 bytes --]

Some PMICs treat the vsel_reg same as apply-bit. Eg, when voltage range
is changed, the new voltage setting is not taking effect until the vsel
register is written.

Force the vsel value to be written to hardware even if the old selector
was same as the new (but range changed) by using regmap_write_bits() and
not the regmap_update_bits().

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
I have a feeling this same thing should be done with the apply-bit
writes. I just wonder why no-one has reported problems with it. Maybe
all the hardware with the apply bit has 'self clearing' apply register
marked as volatile? Anyways, I'll leave it out of this patch because I
have no hardware to test the apply-bit stuff, and as I haven't seen any
problem reports about it.
---
 drivers/regulator/helpers.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/helpers.c b/drivers/regulator/helpers.c
index d49268336553..28c2f40b19cc 100644
--- a/drivers/regulator/helpers.c
+++ b/drivers/regulator/helpers.c
@@ -211,8 +211,14 @@ int regulator_set_voltage_sel_pickable_regmap(struct regulator_dev *rdev,
 		if (ret)
 			return ret;
 
-		ret = regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
-				  rdev->desc->vsel_mask, sel);
+		/*
+		 * Some PMICs treat the vsel_reg same as apply-bit. Force it
+		 * to be written even if the old selector were same as the new
+		 * (but range changed) by using regmap_write_bits() and not the
+		 * regmap_update_bits().
+		 */
+		ret = regmap_write_bits(rdev->regmap, rdev->desc->vsel_reg,
+					rdev->desc->vsel_mask, sel);
 	}
 
 	if (ret)
-- 
2.44.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] regulator: don't cache vsel to ensure voltage setting

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

On Thu, May 16, 2024 at 11:53:46AM +0300, Matti Vaittinen wrote:

> +		/*
> +		 * Some PMICs treat the vsel_reg same as apply-bit. Force it
> +		 * to be written even if the old selector were same as the new
> +		 * (but range changed) by using regmap_write_bits() and not the
> +		 * regmap_update_bits().
> +		 */
> +		ret = regmap_write_bits(rdev->regmap, rdev->desc->vsel_reg,
> +					rdev->desc->vsel_mask, sel);

This feels like a special case for those devices, for devices that don't
have this behaviour it'd be a regression in that it'd increase I/O
traffic.  At least this should be specialised to only devices that have
ranges I think?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] regulator: don't cache vsel to ensure voltage setting

[Matti Vaittinen]

Hi Mark,

Added some driver folks to CC. It'd be great to lean how other devices 
avoid having intermediate voltages if range and vsel write is not atomic.

On 5/16/24 14:33, Mark Brown wrote:
> On Thu, May 16, 2024 at 11:53:46AM +0300, Matti Vaittinen wrote:
> 
>> +		/*
>> +		 * Some PMICs treat the vsel_reg same as apply-bit. Force it
>> +		 * to be written even if the old selector were same as the new
>> +		 * (but range changed) by using regmap_write_bits() and not the
>> +		 * regmap_update_bits().
>> +		 */
>> +		ret = regmap_write_bits(rdev->regmap, rdev->desc->vsel_reg,
>> +					rdev->desc->vsel_mask, sel);
> 
> This feels like a special case for those devices, for devices that don't
> have this behaviour it'd be a regression in that it'd increase I/O
> traffic.

I thought most of devices which have pickable voltage ranges and a 
separate register for range and voltage selector would need some 
mechanism to prevent 'intermediate voltages' as writing the vsel and 
range won't be atomic.

Also, the "pickable ranges" is not that widely used, and many of the 
users have the range and vsel selectors in same register (and if they 
don't, they're likely to see unwanted side effects due to non atomic write).

I did actually go through the drivers. These seem to be clear:

atc260x-regulator.c: vsel and range in same register
max77650-regulator.c: vsel and range in same register
tps6287x-regulator.c: According to a data-sheet, this requires a vsel 
write for range change to take effect (just like the device I am working 
with), so the current implementation may be suffering from the bug I am 
trying to solve.

Anyways, following might be unnecessarily impacted:

max77541-regulator.c - but, I briefly looked at the data-sheet, and I am 
not sure the driver operates according to the spec. The spec I found 
stated the range bits must not be changed when the output is enabled.

mt6358-regulator.c: I didn't find proper spec details.

>  At least this should be specialised to only devices that have
> ranges I think?

This operation is the regulator_set_voltage_sel_pickable_regmap() - 
which should be only used for devices which use pickable ranges. 
Furthermore, it's inside the else branch for if (rdev->desc->vsel_reg == 
rdev->desc->vsel_range_reg) meaning the range selection bit is in 
separate register from the voltage selection.

Anyways, thanks for the heads up. I really didn't know we had other 
users which would be affected. I'll consider adding a flag to the 
regulator desc for deciding if the vsel should always be written when 
range changes.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

[PATCH 2/3] regulator: bd71828: Don't overwrite runtime voltages

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 7987 bytes --]

Some of the regulators on the BD71828 have common voltage setting for
RUN/SUSPEND/IDLE/LPSR states. The enable control can be set for each
state though.

The driver allows setting the voltage values for these states via
device-tree. As a side effect, setting the voltages for
SUSPEND/IDLE/LPSR will also change the RUN level voltage which is not
desired and can break the system.

The comment in code reflects this behaviour, but it is likely to not
make people any happier. The right thing to do is to allow setting the
enable/disable state at SUSPEND/IDLE/LPSR via device-tree, but to
disallow setting state specific voltages for those regulators.

BUCK1 is a bit different. It only shares the SUSPEND and LPSR state
voltages. The former behaviour of allowing to silently overwrite the
SUSPEND state voltage by LPSR state voltage is also changed here so that
the SUSPEND voltage is prioritized over LPSR voltage.

Prevent setting PMIC state specific voltages for regulators which do not
support it.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Fixes: 522498f8cb8c ("regulator: bd71828: Basic support for ROHM bd71828 PMIC regulators")
---
 drivers/regulator/bd71828-regulator.c | 58 +--------------------------
 1 file changed, 2 insertions(+), 56 deletions(-)

diff --git a/drivers/regulator/bd71828-regulator.c b/drivers/regulator/bd71828-regulator.c
index 08d4ee369287..dd871ffe979c 100644
--- a/drivers/regulator/bd71828-regulator.c
+++ b/drivers/regulator/bd71828-regulator.c
@@ -206,14 +206,11 @@ static const struct bd71828_regulator_data bd71828_rdata[] = {
 			.suspend_reg = BD71828_REG_BUCK1_SUSP_VOLT,
 			.suspend_mask = BD71828_MASK_BUCK1267_VOLT,
 			.suspend_on_mask = BD71828_MASK_SUSP_EN,
-			.lpsr_on_mask = BD71828_MASK_LPSR_EN,
 			/*
 			 * LPSR voltage is same as SUSPEND voltage. Allow
-			 * setting it so that regulator can be set enabled at
-			 * LPSR state
+			 * only enabling/disabling regulator for LPSR state
 			 */
-			.lpsr_reg = BD71828_REG_BUCK1_SUSP_VOLT,
-			.lpsr_mask = BD71828_MASK_BUCK1267_VOLT,
+			.lpsr_on_mask = BD71828_MASK_LPSR_EN,
 		},
 		.reg_inits = buck1_inits,
 		.reg_init_amnt = ARRAY_SIZE(buck1_inits),
@@ -288,13 +285,7 @@ static const struct bd71828_regulator_data bd71828_rdata[] = {
 				     ROHM_DVS_LEVEL_SUSPEND |
 				     ROHM_DVS_LEVEL_LPSR,
 			.run_reg = BD71828_REG_BUCK3_VOLT,
-			.idle_reg = BD71828_REG_BUCK3_VOLT,
-			.suspend_reg = BD71828_REG_BUCK3_VOLT,
-			.lpsr_reg = BD71828_REG_BUCK3_VOLT,
 			.run_mask = BD71828_MASK_BUCK3_VOLT,
-			.idle_mask = BD71828_MASK_BUCK3_VOLT,
-			.suspend_mask = BD71828_MASK_BUCK3_VOLT,
-			.lpsr_mask = BD71828_MASK_BUCK3_VOLT,
 			.idle_on_mask = BD71828_MASK_IDLE_EN,
 			.suspend_on_mask = BD71828_MASK_SUSP_EN,
 			.lpsr_on_mask = BD71828_MASK_LPSR_EN,
@@ -329,13 +320,7 @@ static const struct bd71828_regulator_data bd71828_rdata[] = {
 				     ROHM_DVS_LEVEL_SUSPEND |
 				     ROHM_DVS_LEVEL_LPSR,
 			.run_reg = BD71828_REG_BUCK4_VOLT,
-			.idle_reg = BD71828_REG_BUCK4_VOLT,
-			.suspend_reg = BD71828_REG_BUCK4_VOLT,
-			.lpsr_reg = BD71828_REG_BUCK4_VOLT,
 			.run_mask = BD71828_MASK_BUCK4_VOLT,
-			.idle_mask = BD71828_MASK_BUCK4_VOLT,
-			.suspend_mask = BD71828_MASK_BUCK4_VOLT,
-			.lpsr_mask = BD71828_MASK_BUCK4_VOLT,
 			.idle_on_mask = BD71828_MASK_IDLE_EN,
 			.suspend_on_mask = BD71828_MASK_SUSP_EN,
 			.lpsr_on_mask = BD71828_MASK_LPSR_EN,
@@ -370,13 +355,7 @@ static const struct bd71828_regulator_data bd71828_rdata[] = {
 				     ROHM_DVS_LEVEL_SUSPEND |
 				     ROHM_DVS_LEVEL_LPSR,
 			.run_reg = BD71828_REG_BUCK5_VOLT,
-			.idle_reg = BD71828_REG_BUCK5_VOLT,
-			.suspend_reg = BD71828_REG_BUCK5_VOLT,
-			.lpsr_reg = BD71828_REG_BUCK5_VOLT,
 			.run_mask = BD71828_MASK_BUCK5_VOLT,
-			.idle_mask = BD71828_MASK_BUCK5_VOLT,
-			.suspend_mask = BD71828_MASK_BUCK5_VOLT,
-			.lpsr_mask = BD71828_MASK_BUCK5_VOLT,
 			.idle_on_mask = BD71828_MASK_IDLE_EN,
 			.suspend_on_mask = BD71828_MASK_SUSP_EN,
 			.lpsr_on_mask = BD71828_MASK_LPSR_EN,
@@ -493,13 +472,7 @@ static const struct bd71828_regulator_data bd71828_rdata[] = {
 				     ROHM_DVS_LEVEL_SUSPEND |
 				     ROHM_DVS_LEVEL_LPSR,
 			.run_reg = BD71828_REG_LDO1_VOLT,
-			.idle_reg = BD71828_REG_LDO1_VOLT,
-			.suspend_reg = BD71828_REG_LDO1_VOLT,
-			.lpsr_reg = BD71828_REG_LDO1_VOLT,
 			.run_mask = BD71828_MASK_LDO_VOLT,
-			.idle_mask = BD71828_MASK_LDO_VOLT,
-			.suspend_mask = BD71828_MASK_LDO_VOLT,
-			.lpsr_mask = BD71828_MASK_LDO_VOLT,
 			.idle_on_mask = BD71828_MASK_IDLE_EN,
 			.suspend_on_mask = BD71828_MASK_SUSP_EN,
 			.lpsr_on_mask = BD71828_MASK_LPSR_EN,
@@ -533,13 +506,7 @@ static const struct bd71828_regulator_data bd71828_rdata[] = {
 				     ROHM_DVS_LEVEL_SUSPEND |
 				     ROHM_DVS_LEVEL_LPSR,
 			.run_reg = BD71828_REG_LDO2_VOLT,
-			.idle_reg = BD71828_REG_LDO2_VOLT,
-			.suspend_reg = BD71828_REG_LDO2_VOLT,
-			.lpsr_reg = BD71828_REG_LDO2_VOLT,
 			.run_mask = BD71828_MASK_LDO_VOLT,
-			.idle_mask = BD71828_MASK_LDO_VOLT,
-			.suspend_mask = BD71828_MASK_LDO_VOLT,
-			.lpsr_mask = BD71828_MASK_LDO_VOLT,
 			.idle_on_mask = BD71828_MASK_IDLE_EN,
 			.suspend_on_mask = BD71828_MASK_SUSP_EN,
 			.lpsr_on_mask = BD71828_MASK_LPSR_EN,
@@ -573,13 +540,7 @@ static const struct bd71828_regulator_data bd71828_rdata[] = {
 				     ROHM_DVS_LEVEL_SUSPEND |
 				     ROHM_DVS_LEVEL_LPSR,
 			.run_reg = BD71828_REG_LDO3_VOLT,
-			.idle_reg = BD71828_REG_LDO3_VOLT,
-			.suspend_reg = BD71828_REG_LDO3_VOLT,
-			.lpsr_reg = BD71828_REG_LDO3_VOLT,
 			.run_mask = BD71828_MASK_LDO_VOLT,
-			.idle_mask = BD71828_MASK_LDO_VOLT,
-			.suspend_mask = BD71828_MASK_LDO_VOLT,
-			.lpsr_mask = BD71828_MASK_LDO_VOLT,
 			.idle_on_mask = BD71828_MASK_IDLE_EN,
 			.suspend_on_mask = BD71828_MASK_SUSP_EN,
 			.lpsr_on_mask = BD71828_MASK_LPSR_EN,
@@ -614,13 +575,7 @@ static const struct bd71828_regulator_data bd71828_rdata[] = {
 				     ROHM_DVS_LEVEL_SUSPEND |
 				     ROHM_DVS_LEVEL_LPSR,
 			.run_reg = BD71828_REG_LDO4_VOLT,
-			.idle_reg = BD71828_REG_LDO4_VOLT,
-			.suspend_reg = BD71828_REG_LDO4_VOLT,
-			.lpsr_reg = BD71828_REG_LDO4_VOLT,
 			.run_mask = BD71828_MASK_LDO_VOLT,
-			.idle_mask = BD71828_MASK_LDO_VOLT,
-			.suspend_mask = BD71828_MASK_LDO_VOLT,
-			.lpsr_mask = BD71828_MASK_LDO_VOLT,
 			.idle_on_mask = BD71828_MASK_IDLE_EN,
 			.suspend_on_mask = BD71828_MASK_SUSP_EN,
 			.lpsr_on_mask = BD71828_MASK_LPSR_EN,
@@ -655,13 +610,7 @@ static const struct bd71828_regulator_data bd71828_rdata[] = {
 				     ROHM_DVS_LEVEL_SUSPEND |
 				     ROHM_DVS_LEVEL_LPSR,
 			.run_reg = BD71828_REG_LDO5_VOLT,
-			.idle_reg = BD71828_REG_LDO5_VOLT,
-			.suspend_reg = BD71828_REG_LDO5_VOLT,
-			.lpsr_reg = BD71828_REG_LDO5_VOLT,
 			.run_mask = BD71828_MASK_LDO_VOLT,
-			.idle_mask = BD71828_MASK_LDO_VOLT,
-			.suspend_mask = BD71828_MASK_LDO_VOLT,
-			.lpsr_mask = BD71828_MASK_LDO_VOLT,
 			.idle_on_mask = BD71828_MASK_IDLE_EN,
 			.suspend_on_mask = BD71828_MASK_SUSP_EN,
 			.lpsr_on_mask = BD71828_MASK_LPSR_EN,
@@ -720,9 +669,6 @@ static const struct bd71828_regulator_data bd71828_rdata[] = {
 			.suspend_reg = BD71828_REG_LDO7_VOLT,
 			.lpsr_reg = BD71828_REG_LDO7_VOLT,
 			.run_mask = BD71828_MASK_LDO_VOLT,
-			.idle_mask = BD71828_MASK_LDO_VOLT,
-			.suspend_mask = BD71828_MASK_LDO_VOLT,
-			.lpsr_mask = BD71828_MASK_LDO_VOLT,
 			.idle_on_mask = BD71828_MASK_IDLE_EN,
 			.suspend_on_mask = BD71828_MASK_SUSP_EN,
 			.lpsr_on_mask = BD71828_MASK_LPSR_EN,
-- 
2.44.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/3] regulator: rohm-regulator: warn if unsupported voltage is set

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1831 bytes --]

A few of the ROHM PMICs allow setting low-power state specific voltages
for regulators. These voltages are then taken in use (by the hardware)
when the PMIC state is changed.

The voltages for these states can be given via device-tree. If
unsupported voltage has been given, the users have only seen print:
"driver callback failed to parse DT for regulator <name>". This does
help to pinpoint the problem to some extent, but there may be several
properties in device-tree so it should help if we can be a bit more
specific.

Print the voltage value and the run-state property if voltage can not be
supported.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/regulator/rohm-regulator.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/regulator/rohm-regulator.c b/drivers/regulator/rohm-regulator.c
index 0e2418ed957c..4b95ca01959c 100644
--- a/drivers/regulator/rohm-regulator.c
+++ b/drivers/regulator/rohm-regulator.c
@@ -46,6 +46,7 @@ static int set_dvs_level(const struct regulator_desc *desc,
 			continue;
 		if (ret == uv) {
 			i <<= ffs(desc->vsel_mask) - 1;
+
 			ret = regmap_update_bits(regmap, reg, mask, i);
 			if (omask && !ret)
 				ret = regmap_update_bits(regmap, oreg, omask,
@@ -53,6 +54,9 @@ static int set_dvs_level(const struct regulator_desc *desc,
 			break;
 		}
 	}
+	if (i == desc->n_voltages)
+		pr_warn("Unsupported %s voltage %u\n", prop, uv);
+
 	return ret;
 }
 
-- 
2.44.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] regulator: misc fixes

[Mark Brown]

On Thu, 16 May 2024 11:52:44 +0300, Matti Vaittinen wrote:
> This series adds couple of a bit unrelated fixes/improvements which I
> implemented while working to support yet another ROHM PMIC.
> 
> Patch1 addresses a potential problem where the voltage selector for
> pickable-ranges is used in same fashion as a 'apply-bit'. Eg, when a
> regulator voltage is changed so that also the range changes, the effect
> of the range and voltage selector change is not taken into account until
> the voltage selector is written to hardware. In such case caching the
> voltage-selector value will cause the voltage range change to not change
> the actual output voltage.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/3] regulator: don't cache vsel to ensure voltage setting
      (no commit info)
[2/3] regulator: bd71828: Don't overwrite runtime voltages
      commit: 0f9f7c63c415e287cd57b5c98be61eb320dedcfc
[3/3] regulator: rohm-regulator: warn if unsupported voltage is set
      commit: cb3a0f84ae0caa5eabf40457485473edc1e2d3f0

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark