linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH v1 0/3] bd718x7: Support SNVS low power state
@ 2019-02-12 14:16 Matti Vaittinen
  2019-02-12 14:17 ` [RFC PATCH v1 1/3] devicetree: bindings: bd718x7: document HW state related ROHM specific properties Matti Vaittinen
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Matti Vaittinen @ 2019-02-12 14:16 UTC (permalink / raw)
  To: mazziesaccount, matti.vaittinen
  Cc: Lee Jones, Rob Herring, Mark Rutland, Liam Girdwood, Mark Brown,
	devicetree, linux-kernel, heikki.haikola, mikko.mutanen,
	Robin Gong ‎, Elven Wang ‎, Anson Huang ‎

Patch series adding SNVS power state support to ROHM bd718x7 driver.

The SNVS is a low power state used by i.MX family of SoCs. In SNVS
state processor and most of the peripherials are shut off in order
to minimize power consumption.

BD71837 and BD71847 can be configured to use the SNVS state as a
reset/shutdown target state. There is some HW limitations though.
Main limitation is that regulators which have been under SW control
(regarding enable/disable state) will stay shut down after such reset.
Thus if SNVS is used as reset target the control of boot crucial
regulators must be left to HW state machine.

This patch seris allows one to describe the boot crucial regulators
and reset/shutdown target state as well as the default HW state
voltages from device-tree. I am unsure if the device-tree is the
correct way to bring this information and hence I send this initial
patch set as an RFC. Please comment and I will generate properly tested
patch series on top of this idea if this is seen as a reasonable
approach.

---

Matti Vaittinen (3):
  devicetree: bindings: bd718x7: document HW state related ROHM specific
    properties
  regulator: add regulator_desc_list_voltage_linear_range
  regulator: bd718x7: Support SNVS low power state

 .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt  |  17 +++
 .../bindings/regulator/rohm,bd71837-regulator.txt  |  36 ++++++
 drivers/regulator/bd718x7-regulator.c              | 135 +++++++++++++++++++++
 drivers/regulator/helpers.c                        |  39 ++++--
 include/linux/regulator/driver.h                   |   6 +
 5 files changed, 223 insertions(+), 10 deletions(-)

-- 
2.14.3


-- 
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 ~~~

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [RFC PATCH v1 1/3] devicetree: bindings: bd718x7: document HW state related ROHM specific properties
  2019-02-12 14:16 [RFC PATCH v1 0/3] bd718x7: Support SNVS low power state Matti Vaittinen
@ 2019-02-12 14:17 ` Matti Vaittinen
  2019-02-12 14:18 ` [RFC PATCH v1 2/3] regulator: add regulator_desc_list_voltage_linear_range Matti Vaittinen
  2019-02-12 14:19 ` [RFC PATCH v1 3/3] regulator: bd718x7: Support SNVS low power state Matti Vaittinen
  2 siblings, 0 replies; 8+ messages in thread
From: Matti Vaittinen @ 2019-02-12 14:17 UTC (permalink / raw)
  To: mazziesaccount, matti.vaittinen
  Cc: Lee Jones, Rob Herring, Mark Rutland, Liam Girdwood, Mark Brown,
	devicetree, linux-kernel, heikki.haikola, mikko.mutanen,
	Robin Gong ‎, Elven Wang ‎, Anson Huang ‎

Add ROHM BD71837 / BD71847 specific device tree bindings for
controlling the PMIC shutdown/reset states and voltages for
different HW states. The PMIC was designed to be used with NXP
i.MX8 SoC and it supports SNVS low power state which seems to
be typical for NXP i.MX SoCs. However, when SNVS is used we must
not allow SW to control enabling/disabling those regulators which
are crucial for system to boot as there is a HW limitation which
causes SW controlled regulators to be kept shut down after SNVS
reset.

Allow setting the SNVS to be used as reset target state and allow
marking those regulators which are critical for boot.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt  | 17 ++++++++++
 .../bindings/regulator/rohm,bd71837-regulator.txt  | 36 ++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
index a4b056761eaa..2aff8287fdd8 100644
--- a/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
+++ b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
@@ -23,6 +23,23 @@ Required properties:
 
 Optional properties:
 - clock-output-names	: Should contain name for output clock.
+- rohm,reset-snvs-powered : Transfer BD718x7 to SNVS state at reset.
+
+The BD718x7 supports two different HW states as reset target states. States
+are called as SNVS and READY. At the SNVS state all other logic and external
+devices apart from the SNVS power domain are shut off. Please refer to NXP
+i.MX8 documentation for further information regarding SNVS state. When a
+reset is done via SNVS state the PMIC OTP data is not reload.
+
+At READY state all the PMIC power outputs go down and OTP is reload. There
+is a HW quirk in BD718x7 which causes power outputs which have been under
+SW control to stay down when reset has switched power state to SNVS. If
+reset is done via READY state the power outputs will be set to HW control
+by OTP loading. Thus the reset target state is set to READY by default. If
+SNVS state is used the boot crucial regulators should have
+"rohm,regulator-crucial-for-boot" property set in regulator node. Please
+see the ./regulator/rohm,bd71837-regulator.txt for details of regulator
+bindings.
 
 Example:
 
diff --git a/Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt b/Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt
index 4b98ca26e61a..8ff692891dea 100644
--- a/Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt
@@ -27,8 +27,44 @@ BUCK1, BUCK2, BUCK3, BUCK4, BUCK5, BUCK6
 LDO1, LDO2, LDO3, LDO4, LDO5, LDO6
 
 Optional properties:
+- rohm,regulator-crucial-for-boot : Any regulators which are required to stay
+				    enabled for system to boot must be excluded
+				    from SW control by this property if SNVS
+				    state is used as reset target. Please see
+				    the ../mfd/rohm,bd71837-pmic.txt for more
+				    details.
+- rohm,dvs-run-voltage		: PMIC default "RUN" state voltage in uV.
+				  See below table for bucks which support this.
+- rohm,dvs-idle-voltage		: PMIC default "IDLE" state voltage in uV.
+				  See below table for bucks which support this.
+- rohm,dvs-suspend-voltage	: PMIC default "SUSPEND" state voltage in uV.
+				  See below table for bucks which support this.
 - Any optional property defined in bindings/regulator/regulator.txt
 
+Supported default DVS states:
+
+BD71837:
+buck	| dvs-run-voltage	| dvs-idle-voltage	| dvs-suspend-voltage
+-----------------------------------------------------------------------------
+1	| supported		| supported		| supported
+----------------------------------------------------------------------------
+2	| supported		| supported		| not supported
+----------------------------------------------------------------------------
+3	| supported		| not supported		| not supported
+----------------------------------------------------------------------------
+4	| supported		| not supported		| not supported
+----------------------------------------------------------------------------
+rest	| not supported		| not supported		| not supported
+
+BD71847:
+buck	| dvs-run-voltage	| dvs-idle-voltage	| dvs-suspend-voltage
+-----------------------------------------------------------------------------
+1	| supported		| supported		| supported
+----------------------------------------------------------------------------
+2	| supported		| supported		| not supported
+----------------------------------------------------------------------------
+rest	| not supported		| not supported		| not supported
+
 Example:
 regulators {
 	buck1: BUCK1 {
-- 
2.14.3


-- 
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 ~~~

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [RFC PATCH v1 2/3] regulator: add regulator_desc_list_voltage_linear_range
  2019-02-12 14:16 [RFC PATCH v1 0/3] bd718x7: Support SNVS low power state Matti Vaittinen
  2019-02-12 14:17 ` [RFC PATCH v1 1/3] devicetree: bindings: bd718x7: document HW state related ROHM specific properties Matti Vaittinen
@ 2019-02-12 14:18 ` Matti Vaittinen
  2019-02-12 15:34   ` Mark Brown
  2019-02-12 14:19 ` [RFC PATCH v1 3/3] regulator: bd718x7: Support SNVS low power state Matti Vaittinen
  2 siblings, 1 reply; 8+ messages in thread
From: Matti Vaittinen @ 2019-02-12 14:18 UTC (permalink / raw)
  To: mazziesaccount, matti.vaittinen
  Cc: Lee Jones, Rob Herring, Mark Rutland, Liam Girdwood, Mark Brown,
	devicetree, linux-kernel, heikki.haikola, mikko.mutanen,
	Robin Gong ‎, Elven Wang ‎, Anson Huang ‎

Add regulator_desc_list_voltage_linear_range which can be used
by drivers for getting the voltages before regulator is registered.
This may be useful for drivers which need to fetch the voltage
selectors at device-tree parsing callback.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---

Please not apply as such. This is compile-tested only and sent
for collecting feedback. If idea is Ok I will do proper testing
and send a non RFC patch.

 drivers/regulator/helpers.c      | 39 +++++++++++++++++++++++++++++----------
 include/linux/regulator/driver.h |  6 ++++++
 2 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/drivers/regulator/helpers.c b/drivers/regulator/helpers.c
index 5686a1335bd3..68ac6017ef28 100644
--- a/drivers/regulator/helpers.c
+++ b/drivers/regulator/helpers.c
@@ -594,28 +594,30 @@ int regulator_list_voltage_pickable_linear_range(struct regulator_dev *rdev,
 EXPORT_SYMBOL_GPL(regulator_list_voltage_pickable_linear_range);
 
 /**
- * regulator_list_voltage_linear_range - List voltages for linear ranges
+ * regulator_desc_list_voltage_linear_range - List voltages for linear ranges
  *
- * @rdev: Regulator device
+ * @desc: Regulator desc for regulator which volatges are to be listed
  * @selector: Selector to convert into a voltage
  *
  * Regulators with a series of simple linear mappings between voltages
- * and selectors can set linear_ranges in the regulator descriptor and
- * then use this function as their list_voltage() operation,
+ * and selectors who have set linear_ranges in the regulator descriptor
+ * can use this function prior regulator registration to list voltages.
+ * This is useful when voltages need to be listed during device-tree
+ * parsing.
  */
-int regulator_list_voltage_linear_range(struct regulator_dev *rdev,
-					unsigned int selector)
+int regulator_desc_list_voltage_linear_range(const struct regulator_desc *desc,
+					     unsigned int selector)
 {
 	const struct regulator_linear_range *range;
 	int i;
 
-	if (!rdev->desc->n_linear_ranges) {
-		BUG_ON(!rdev->desc->n_linear_ranges);
+	if (!desc->n_linear_ranges) {
+		BUG_ON(!desc->n_linear_ranges);
 		return -EINVAL;
 	}
 
-	for (i = 0; i < rdev->desc->n_linear_ranges; i++) {
-		range = &rdev->desc->linear_ranges[i];
+	for (i = 0; i < desc->n_linear_ranges; i++) {
+		range = &desc->linear_ranges[i];
 
 		if (!(selector >= range->min_sel &&
 		      selector <= range->max_sel))
@@ -628,6 +630,23 @@ int regulator_list_voltage_linear_range(struct regulator_dev *rdev,
 
 	return -EINVAL;
 }
+EXPORT_SYMBOL_GPL(regulator_desc_list_voltage_linear_range);
+
+/**
+ * regulator_list_voltage_linear_range - List voltages for linear ranges
+ *
+ * @rdev: Regulator device
+ * @selector: Selector to convert into a voltage
+ *
+ * Regulators with a series of simple linear mappings between voltages
+ * and selectors can set linear_ranges in the regulator descriptor and
+ * then use this function as their list_voltage() operation,
+ */
+int regulator_list_voltage_linear_range(struct regulator_dev *rdev,
+					unsigned int selector)
+{
+	return regulator_desc_list_voltage_linear_range(rdev->desc, selector);
+}
 EXPORT_SYMBOL_GPL(regulator_list_voltage_linear_range);
 
 /**
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 7f8345bff4e1..05efe2b057c1 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -539,4 +539,10 @@ void *regulator_get_init_drvdata(struct regulator_init_data *reg_init_data);
 void regulator_lock(struct regulator_dev *rdev);
 void regulator_unlock(struct regulator_dev *rdev);
 
+/*
+ * Helper functions intended to be used by regulator drivers prior registering
+ * their regulators.
+ */
+int regulator_desc_list_voltage_linear_range(const struct regulator_desc *desc,
+					     unsigned int selector);
 #endif
-- 
2.14.3


-- 
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 ~~~

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [RFC PATCH v1 3/3] regulator: bd718x7: Support SNVS low power state
  2019-02-12 14:16 [RFC PATCH v1 0/3] bd718x7: Support SNVS low power state Matti Vaittinen
  2019-02-12 14:17 ` [RFC PATCH v1 1/3] devicetree: bindings: bd718x7: document HW state related ROHM specific properties Matti Vaittinen
  2019-02-12 14:18 ` [RFC PATCH v1 2/3] regulator: add regulator_desc_list_voltage_linear_range Matti Vaittinen
@ 2019-02-12 14:19 ` Matti Vaittinen
  2019-02-12 16:08   ` Mark Brown
  2 siblings, 1 reply; 8+ messages in thread
From: Matti Vaittinen @ 2019-02-12 14:19 UTC (permalink / raw)
  To: mazziesaccount, matti.vaittinen
  Cc: Lee Jones, Rob Herring, Mark Rutland, Liam Girdwood, Mark Brown,
	devicetree, linux-kernel, heikki.haikola, mikko.mutanen,
	Robin Gong ‎, Elven Wang ‎, Anson Huang ‎

read ROHM BD71837 / BD71847 specific device tree bindings for
controlling the PMIC shutdown/reset states and voltages for
different HW states. The PMIC was designed to be used with NXP
i.MX8 SoC and it supports SNVS low power state which seems to
be typical for NXP i.MX SoCs. However, when SNVS is used we must
not allow SW to control enabling/disabling those regulators which
are crucial for system to boot as there is a HW limitation which
causes SW controlled regulators to be kept shut down after SNVS
reset.

Allow setting the SNVS to be used as reset target state and allow
marking those regulators which are critical for boot.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---

Please not apply as such. This is compile-tested only and sent
for collecting feedback. If idea is Ok I will do proper testing
and send a non RFC patch.

 drivers/regulator/bd718x7-regulator.c | 135 ++++++++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)

diff --git a/drivers/regulator/bd718x7-regulator.c b/drivers/regulator/bd718x7-regulator.c
index ccea133778c8..327ef9223cb0 100644
--- a/drivers/regulator/bd718x7-regulator.c
+++ b/drivers/regulator/bd718x7-regulator.c
@@ -350,6 +350,135 @@ static const struct reg_init bd71837_ldo6_inits[] = {
 	},
 };
 
+#define NUM_DVS_BUCKS 4
+
+struct of_dvs_setting {
+	const char *prop;
+	unsigned int reg;
+};
+
+static int set_dvs_levels(const struct of_dvs_setting *dvs,
+			  struct device_node *np,
+			  const struct regulator_desc *desc,
+			  struct regmap *regmap)
+{
+	int ret, i;
+	unsigned int uv;
+
+	ret = of_property_read_u32(np, dvs->prop, &uv);
+	if (ret) {
+		if (ret != -EINVAL)
+			return ret;
+		return 0;
+	}
+
+	for (i = 0; i < desc->n_voltages; i++) {
+		ret = regulator_desc_list_voltage_linear_range(desc, i);
+		if (ret < 0)
+			continue;
+		if (ret == uv) {
+			i <<= ffs(desc->vsel_mask) - 1;
+			ret = regmap_update_bits(regmap, dvs->reg,
+						 DVS_BUCK_RUN_MASK, i);
+			break;
+		}
+	}
+	return ret;
+}
+
+static int buck4_set_hw_dvs_levels(struct device_node *np,
+			    const struct regulator_desc *desc,
+			    struct regulator_config *cfg)
+{
+	int ret, i;
+	const struct of_dvs_setting dvs[] = {
+		{
+			.prop = "rohm,dvs-run-voltage",
+			.reg = BD71837_REG_BUCK4_VOLT_RUN,
+		},
+	};
+
+	for (i = 0; i < ARRAY_SIZE(dvs); i++) {
+		ret = set_dvs_levels(&dvs[i], np, desc, cfg->regmap);
+		if (ret)
+			break;
+	}
+	return ret;
+}
+static int buck3_set_hw_dvs_levels(struct device_node *np,
+			    const struct regulator_desc *desc,
+			    struct regulator_config *cfg)
+{
+	int ret, i;
+	const struct of_dvs_setting dvs[] = {
+		{
+			.prop = "rohm,dvs-run-voltage",
+			.reg = BD71837_REG_BUCK3_VOLT_RUN,
+		},
+	};
+
+	for (i = 0; i < ARRAY_SIZE(dvs); i++) {
+		ret = set_dvs_levels(&dvs[i], np, desc, cfg->regmap);
+		if (ret)
+			break;
+	}
+	return ret;
+}
+
+static int buck2_set_hw_dvs_levels(struct device_node *np,
+			    const struct regulator_desc *desc,
+			    struct regulator_config *cfg)
+{
+	int ret, i;
+	const struct of_dvs_setting dvs[] = {
+		{
+			.prop = "rohm,dvs-run-voltage",
+			.reg = BD718XX_REG_BUCK2_VOLT_RUN,
+		},
+		{
+			.prop = "rohm,dvs-idle-voltage",
+			.reg = BD718XX_REG_BUCK2_VOLT_IDLE,
+		},
+	};
+
+
+
+	for (i = 0; i < ARRAY_SIZE(dvs); i++) {
+		ret = set_dvs_levels(&dvs[i], np, desc, cfg->regmap);
+		if (ret)
+			break;
+	}
+	return ret;
+}
+
+static int buck1_set_hw_dvs_levels(struct device_node *np,
+			    const struct regulator_desc *desc,
+			    struct regulator_config *cfg)
+{
+	int ret, i;
+	const struct of_dvs_setting dvs[] = {
+		{
+			.prop = "rohm,dvs-run-voltage",
+			.reg = BD718XX_REG_BUCK1_VOLT_RUN,
+		},
+		{
+			.prop = "rohm,dvs-idle-voltage",
+			.reg = BD718XX_REG_BUCK1_VOLT_IDLE,
+		},
+		{
+			.prop = "rohm,dvs-suspend-voltage",
+			.reg = BD718XX_REG_BUCK1_VOLT_SUSP,
+		},
+	};
+
+	for (i = 0; i < ARRAY_SIZE(dvs); i++) {
+		ret = set_dvs_levels(&dvs[i], np, desc, cfg->regmap);
+		if (ret)
+			break;
+	}
+	return ret;
+}
+
 static const struct bd718xx_regulator_data bd71847_regulators[] = {
 	{
 		.desc = {
@@ -368,6 +497,7 @@ static const struct bd718xx_regulator_data bd71847_regulators[] = {
 			.enable_reg = BD718XX_REG_BUCK1_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
 			.owner = THIS_MODULE,
+			.of_parse_cb = buck1_set_hw_dvs_levels,
 		},
 		.init = {
 			.reg = BD718XX_REG_BUCK1_CTRL,
@@ -391,6 +521,7 @@ static const struct bd718xx_regulator_data bd71847_regulators[] = {
 			.enable_reg = BD718XX_REG_BUCK2_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
 			.owner = THIS_MODULE,
+			.of_parse_cb = buck2_set_hw_dvs_levels,
 		},
 		.init = {
 			.reg = BD718XX_REG_BUCK2_CTRL,
@@ -662,6 +793,7 @@ static const struct bd718xx_regulator_data bd71837_regulators[] = {
 			.enable_reg = BD718XX_REG_BUCK1_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
 			.owner = THIS_MODULE,
+			.of_parse_cb = buck1_set_hw_dvs_levels,
 		},
 		.init = {
 			.reg = BD718XX_REG_BUCK1_CTRL,
@@ -685,6 +817,7 @@ static const struct bd718xx_regulator_data bd71837_regulators[] = {
 			.enable_reg = BD718XX_REG_BUCK2_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
 			.owner = THIS_MODULE,
+			.of_parse_cb = buck1_set_hw_dvs_levels,
 		},
 		.init = {
 			.reg = BD718XX_REG_BUCK2_CTRL,
@@ -708,6 +841,7 @@ static const struct bd718xx_regulator_data bd71837_regulators[] = {
 			.enable_reg = BD71837_REG_BUCK3_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
 			.owner = THIS_MODULE,
+			.of_parse_cb = buck3_set_hw_dvs_levels,
 		},
 		.init = {
 			.reg = BD71837_REG_BUCK3_CTRL,
@@ -731,6 +865,7 @@ static const struct bd718xx_regulator_data bd71837_regulators[] = {
 			.enable_reg = BD71837_REG_BUCK4_CTRL,
 			.enable_mask = BD718XX_BUCK_EN,
 			.owner = THIS_MODULE,
+			.of_parse_cb = buck4_set_hw_dvs_levels,
 		},
 		.init = {
 			.reg = BD71837_REG_BUCK4_CTRL,
-- 
2.14.3


-- 
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 ~~~

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH v1 2/3] regulator: add regulator_desc_list_voltage_linear_range
  2019-02-12 14:18 ` [RFC PATCH v1 2/3] regulator: add regulator_desc_list_voltage_linear_range Matti Vaittinen
@ 2019-02-12 15:34   ` Mark Brown
  0 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2019-02-12 15:34 UTC (permalink / raw)
  To: Matti Vaittinen
  Cc: mazziesaccount, Lee Jones, Rob Herring, Mark Rutland,
	Liam Girdwood, devicetree, linux-kernel, heikki.haikola,
	mikko.mutanen, Robin Gong, Elven Wang, Anson Huang

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

On Tue, Feb 12, 2019 at 04:18:46PM +0200, Matti Vaittinen wrote:

> Add regulator_desc_list_voltage_linear_range which can be used
> by drivers for getting the voltages before regulator is registered.
> This may be useful for drivers which need to fetch the voltage
> selectors at device-tree parsing callback.

This seems fine.

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH v1 3/3] regulator: bd718x7: Support SNVS low power state
  2019-02-12 14:19 ` [RFC PATCH v1 3/3] regulator: bd718x7: Support SNVS low power state Matti Vaittinen
@ 2019-02-12 16:08   ` Mark Brown
  2019-02-13  7:38     ` Matti Vaittinen
  0 siblings, 1 reply; 8+ messages in thread
From: Mark Brown @ 2019-02-12 16:08 UTC (permalink / raw)
  To: Matti Vaittinen
  Cc: mazziesaccount, Lee Jones, Rob Herring, Mark Rutland,
	Liam Girdwood, devicetree, linux-kernel, heikki.haikola,
	mikko.mutanen, Robin Gong, Elven Wang, Anson Huang

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

On Tue, Feb 12, 2019 at 04:19:38PM +0200, Matti Vaittinen wrote:
> read ROHM BD71837 / BD71847 specific device tree bindings for
> controlling the PMIC shutdown/reset states and voltages for
> different HW states. The PMIC was designed to be used with NXP
> i.MX8 SoC and it supports SNVS low power state which seems to
> be typical for NXP i.MX SoCs. However, when SNVS is used we must
> not allow SW to control enabling/disabling those regulators which
> are crucial for system to boot as there is a HW limitation which
> causes SW controlled regulators to be kept shut down after SNVS
> reset.
> 
> Allow setting the SNVS to be used as reset target state and allow
> marking those regulators which are critical for boot.

The general idea seems fine but I'm wondering if we should use the
existing bindings and just prevent any change with fixed configurations
- that *should* just be a case of picking appropriate constraints I
think.  Why does this need completely new properties other than
preventing the user from shooting themselves in the foot?

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH v1 3/3] regulator: bd718x7: Support SNVS low power state
  2019-02-12 16:08   ` Mark Brown
@ 2019-02-13  7:38     ` Matti Vaittinen
  2019-02-13  8:33       ` Matti Vaittinen
  0 siblings, 1 reply; 8+ messages in thread
From: Matti Vaittinen @ 2019-02-13  7:38 UTC (permalink / raw)
  To: Mark Brown
  Cc: mazziesaccount, Lee Jones, Rob Herring, Mark Rutland,
	Liam Girdwood, devicetree, linux-kernel, heikki.haikola,
	mikko.mutanen, Robin Gong, Elven Wang, Anson Huang

Hello Mark,

On Tue, Feb 12, 2019 at 04:08:31PM +0000, Mark Brown wrote:
> On Tue, Feb 12, 2019 at 04:19:38PM +0200, Matti Vaittinen wrote:
> > read ROHM BD71837 / BD71847 specific device tree bindings for
> > controlling the PMIC shutdown/reset states and voltages for
> > different HW states. The PMIC was designed to be used with NXP
> > i.MX8 SoC and it supports SNVS low power state which seems to
> > be typical for NXP i.MX SoCs. However, when SNVS is used we must
> > not allow SW to control enabling/disabling those regulators which
> > are crucial for system to boot as there is a HW limitation which
> > causes SW controlled regulators to be kept shut down after SNVS
> > reset.
> > 
> > Allow setting the SNVS to be used as reset target state and allow
> > marking those regulators which are critical for boot.
> 
> The general idea seems fine but I'm wondering if we should use the
> existing bindings and just prevent any change with fixed configurations
> - that *should* just be a case of picking appropriate constraints I
> think.  Why does this need completely new properties other than
> preventing the user from shooting themselves in the foot?

The BD71847 and BD71847 have 'HW state machine' in PMIC. It has READY,
RUN, IDLE, SUSPEND, SNVS,.. states.

At RUN state, the enabling and disabling status can be either a
predefined configuration - or controlled by SW. There is one bit for
each regulator which can be used to switch the control to the SW.

By default the BD718x7 driver turns this control bit so that all of the
regulators are controlled by SW. This is done at driver probe, right
after the regulators have been registered.

Now when a reset occurs (SW reset or reset via power button press,
external WDG, ...) the PMIC disables all power outputs no matter if they
are controlled by SW or HW.

Unfortunately there is this 'feature' in PMIC so that when PMIC starts
up after reset, those regulators which were controlled by SW won't be
powered again - no matter if they were enabled before reset. This
happens only whn reset target state was SNVS state.

So with current driver design, even if constrains prevented the
regulator core from touching the regulators, the driver still changes
the control to SW. So we need to parse some attribute in the BD718x7
driver side. Besides, I did not spot a 'do not touch me' property from
the bindings :)

But after writing all this - I think you are correct. We do not need the
rohm,regulator-crucial-for-boot. I guess we can check for the
combination of:

regulator-always-on and regulator-boot-on

and interpret this as "rohm,regulator-crucial-for-boot".

I just think I need to document that those flags are required
for critical regulators if SNVS is used as reset target even if there
should be no one touching those regulators.

Thanks once again for the feedback Mark!

Br,
	Matti Vaittinen

-- 
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 ~~~

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH v1 3/3] regulator: bd718x7: Support SNVS low power state
  2019-02-13  7:38     ` Matti Vaittinen
@ 2019-02-13  8:33       ` Matti Vaittinen
  0 siblings, 0 replies; 8+ messages in thread
From: Matti Vaittinen @ 2019-02-13  8:33 UTC (permalink / raw)
  To: Mark Brown
  Cc: mazziesaccount, Lee Jones, Rob Herring, Mark Rutland,
	Liam Girdwood, devicetree, linux-kernel, heikki.haikola,
	mikko.mutanen, Robin Gong, Elven Wang, Anson Huang

Hello peeps,

And my apologies! I was in a hurry yesterday. I had to pick up kid kid
from kindegarden and I wanted to send the RFC before that - bad mistake.
Should've learnd by now.

Today I decided to continue work and use combination of
regulator-always-on and regulator-boot-on instead of
rohm,regulator-crucial-for-boot. I was ashtonished by the fact that I
didn't find my latest work from my git. After a while I found unsaved
buffer from another editor instance with changes concerning handling of
"rohm,regulator-crucial-for-boot"...

On Wed, Feb 13, 2019 at 09:38:27AM +0200, Matti Vaittinen wrote:
> Hello Mark,
> 
> On Tue, Feb 12, 2019 at 04:08:31PM +0000, Mark Brown wrote:
> 
> So with current driver design, even if constrains prevented the
> regulator core from touching the regulators, the driver still changes
> the control to SW. So we need to parse some attribute in the BD718x7
> driver side. Besides, I did not spot a 'do not touch me' property from
> the bindings :)

So no wonder it was not clear that the core constrains were not enough.
Previous patch did not contain the part of change that skipped turning
the regulator control to SW if regulator was marked crucial.

> But after writing all this - I think you are correct. We do not need the
> rohm,regulator-crucial-for-boot. I guess we can check for the
> combination of:
> 
> regulator-always-on and regulator-boot-on
> 
> and interpret this as "rohm,regulator-crucial-for-boot".
> 
> I just think I need to document that those flags are required
> for critical regulators if SNVS is used as reset target even if there
> should be no one touching those regulators.

Anyways, this is still valid - I'll prepare next version with all
intended changes included... :/

Br,
	Matti Vaittinen

-- 
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 ~~~

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-02-13  8:33 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-12 14:16 [RFC PATCH v1 0/3] bd718x7: Support SNVS low power state Matti Vaittinen
2019-02-12 14:17 ` [RFC PATCH v1 1/3] devicetree: bindings: bd718x7: document HW state related ROHM specific properties Matti Vaittinen
2019-02-12 14:18 ` [RFC PATCH v1 2/3] regulator: add regulator_desc_list_voltage_linear_range Matti Vaittinen
2019-02-12 15:34   ` Mark Brown
2019-02-12 14:19 ` [RFC PATCH v1 3/3] regulator: bd718x7: Support SNVS low power state Matti Vaittinen
2019-02-12 16:08   ` Mark Brown
2019-02-13  7:38     ` Matti Vaittinen
2019-02-13  8:33       ` Matti Vaittinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).