[PATCH v2 0/2] power: pmic: add TPS65913 support
Existing PALMAS PMIC driver is fully compatible with TI TPS65913 PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS TF701T. Add TPS65913 dts compatible with TPS659038 data.
Issue with regulators is more general then I though initially. It touches all pmic regulators.
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
Second patch changes this behavior. Post-probe function is introduced which performs probing of each pmics child and if it is a regulator, regulator_autoset function is called, which handles always-on and boot-on regulators, but if none of those props are set, the regulator is disabled.
Later disabled regulators can be re-enabled by devices which use them without issues.
I have tested this change on multiple devices with different PMICs (LG P895, max77663; TF300T with TPS65911; HTC One X with TPS80031; Tegratab with TPS65913). Without this patch they behave as described in the third paragraph. With patch behavior is correct.
--- Changes from v1: - implemented general fix of regulator behavior, which include: - introduction of post-probe into pmic-uclass driver - probing all pmic childs in post-probe - check if device is regulator > autoset if regulator is boot-on or always-on else disable regulator ---
Svyatoslav Ryhel (2): power: pmic: support TI TPS65913 PMIC power: pmic: fix regulators behaviour
drivers/power/pmic/palmas.c | 1 + drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+)
Existing PALMAS PMIC driver is fully compatible with TI TPS65913 PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS TF701T. TPS65913 shares same structure of regulators like TPS659038 so data can be reused.
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # NVIDIA Tegratab Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com --- drivers/power/pmic/palmas.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/power/pmic/palmas.c b/drivers/power/pmic/palmas.c index 6080cbff0b..d6eb9cb433 100644 --- a/drivers/power/pmic/palmas.c +++ b/drivers/power/pmic/palmas.c @@ -87,6 +87,7 @@ static struct dm_pmic_ops palmas_ops = {
static const struct udevice_id palmas_ids[] = { { .compatible = "ti,tps659038", .data = TPS659038 }, + { .compatible = "ti,tps65913" , .data = TPS659038 }, { .compatible = "ti,tps65917" , .data = TPS65917 }, { } };
On 7/16/23 03:34, Svyatoslav Ryhel wrote:
Existing PALMAS PMIC driver is fully compatible with TI TPS65913 PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS TF701T. TPS65913 shares same structure of regulators like TPS659038 so data can be reused.
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # NVIDIA Tegratab Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
Reviewed-by: Jaehoon Chung jh80.chung@samsung.com
Best Regards, Jaehoon Chung
drivers/power/pmic/palmas.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/power/pmic/palmas.c b/drivers/power/pmic/palmas.c index 6080cbff0b..d6eb9cb433 100644 --- a/drivers/power/pmic/palmas.c +++ b/drivers/power/pmic/palmas.c @@ -87,6 +87,7 @@ static struct dm_pmic_ops palmas_ops = {
static const struct udevice_id palmas_ids[] = { { .compatible = "ti,tps659038", .data = TPS659038 },
- { .compatible = "ti,tps65913" , .data = TPS659038 }, { .compatible = "ti,tps65917" , .data = TPS65917 }, { }
};
31 жовтня 2023 р. 07:02:54 GMT+02:00, Jaehoon Chung jh80.chung@samsung.com написав(-ла):
On 7/16/23 03:34, Svyatoslav Ryhel wrote:
Existing PALMAS PMIC driver is fully compatible with TI TPS65913 PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS TF701T. TPS65913 shares same structure of regulators like TPS659038 so data can be reused.
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # NVIDIA Tegratab Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
Reviewed-by: Jaehoon Chung jh80.chung@samsung.com
Best Regards, Jaehoon Chung
power: pmic: add TPS65913 support patchset is outdated already and should not be applied!
drivers/power/pmic/palmas.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/power/pmic/palmas.c b/drivers/power/pmic/palmas.c index 6080cbff0b..d6eb9cb433 100644 --- a/drivers/power/pmic/palmas.c +++ b/drivers/power/pmic/palmas.c @@ -87,6 +87,7 @@ static struct dm_pmic_ops palmas_ops = {
static const struct udevice_id palmas_ids[] = { { .compatible = "ti,tps659038", .data = TPS659038 },
- { .compatible = "ti,tps65913" , .data = TPS659038 }, { .compatible = "ti,tps65917" , .data = TPS65917 }, { }
};
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This patch changes this behavior. Post-probe function is introduced which performs probing of each pmics child and if it is a regulator, regulator_autoset function is called, which handles always-on and boot-on regulators, but if none of those props are set, the regulator is disabled.
Later disabled regulators can be re-enabled by devices which use them without issues.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com --- drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c index 0e2f5e1f41..8a26b519c9 100644 --- a/drivers/power/pmic/pmic-uclass.c +++ b/drivers/power/pmic/pmic-uclass.c @@ -16,6 +16,7 @@ #include <dm/device-internal.h> #include <dm/uclass-internal.h> #include <power/pmic.h> +#include <power/regulator.h> #include <linux/ctype.h>
#if CONFIG_IS_ENABLED(PMIC_CHILDREN) @@ -198,9 +199,26 @@ static int pmic_pre_probe(struct udevice *dev) return 0; }
+static int pmic_post_probe(struct udevice *dev) +{ + struct udevice *child; + int ret; + + device_foreach_child_probe(child, dev) { + if (device_get_uclass_id(child) == UCLASS_REGULATOR) { + ret = regulator_autoset(child); + if (ret == -EMEDIUMTYPE) + regulator_set_enable(child, false); + }; + }; + + return 0; +} + UCLASS_DRIVER(pmic) = { .id = UCLASS_PMIC, .name = "pmic", .pre_probe = pmic_pre_probe, + .post_probe = pmic_post_probe, .per_device_auto = sizeof(struct uc_pmic_priv), };
Hi Svyatoslav,
On Sat, 15 Jul 2023 at 12:34, Svyatoslav Ryhel clamor95@gmail.com wrote:
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This patch changes this behavior. Post-probe function is introduced which performs probing of each pmics child and if it is a regulator, regulator_autoset function is called, which handles always-on and boot-on regulators, but if none of those props are set, the regulator is disabled.
Later disabled regulators can be re-enabled by devices which use them without issues.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
Can you add a test for this to test/dm/pmic.c ?
diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c index 0e2f5e1f41..8a26b519c9 100644 --- a/drivers/power/pmic/pmic-uclass.c +++ b/drivers/power/pmic/pmic-uclass.c @@ -16,6 +16,7 @@ #include <dm/device-internal.h> #include <dm/uclass-internal.h> #include <power/pmic.h> +#include <power/regulator.h> #include <linux/ctype.h>
#if CONFIG_IS_ENABLED(PMIC_CHILDREN) @@ -198,9 +199,26 @@ static int pmic_pre_probe(struct udevice *dev) return 0; }
+static int pmic_post_probe(struct udevice *dev) +{
struct udevice *child;int ret;device_foreach_child_probe(child, dev) {if (device_get_uclass_id(child) == UCLASS_REGULATOR) {ret = regulator_autoset(child);if (ret == -EMEDIUMTYPE)regulator_set_enable(child, false);};};return 0;+}
UCLASS_DRIVER(pmic) = { .id = UCLASS_PMIC, .name = "pmic", .pre_probe = pmic_pre_probe,
.post_probe = pmic_post_probe, .per_device_auto = sizeof(struct uc_pmic_priv),};
2.39.2
Regards, Simon
16 липня 2023 р. 02:40:37 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sat, 15 Jul 2023 at 12:34, Svyatoslav Ryhel clamor95@gmail.com wrote:
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This patch changes this behavior. Post-probe function is introduced which performs probing of each pmics child and if it is a regulator, regulator_autoset function is called, which handles always-on and boot-on regulators, but if none of those props are set, the regulator is disabled.
Later disabled regulators can be re-enabled by devices which use them without issues.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
Can you add a test for this to test/dm/pmic.c ?
Sure, may you specify what I should add to tests/dm/pmic.c? Which behavior is needed?
diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c index 0e2f5e1f41..8a26b519c9 100644 --- a/drivers/power/pmic/pmic-uclass.c +++ b/drivers/power/pmic/pmic-uclass.c @@ -16,6 +16,7 @@ #include <dm/device-internal.h> #include <dm/uclass-internal.h> #include <power/pmic.h> +#include <power/regulator.h> #include <linux/ctype.h>
#if CONFIG_IS_ENABLED(PMIC_CHILDREN) @@ -198,9 +199,26 @@ static int pmic_pre_probe(struct udevice *dev) return 0; }
+static int pmic_post_probe(struct udevice *dev) +{
struct udevice *child;int ret;device_foreach_child_probe(child, dev) {if (device_get_uclass_id(child) == UCLASS_REGULATOR) {ret = regulator_autoset(child);if (ret == -EMEDIUMTYPE)regulator_set_enable(child, false);};};return 0;+}
UCLASS_DRIVER(pmic) = { .id = UCLASS_PMIC, .name = "pmic", .pre_probe = pmic_pre_probe,
.post_probe = pmic_post_probe, .per_device_auto = sizeof(struct uc_pmic_priv),};
2.39.2
Regards, Simon
Hi Svyatoslav,
On Sat, 15 Jul 2023 at 22:08, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 02:40:37 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sat, 15 Jul 2023 at 12:34, Svyatoslav Ryhel clamor95@gmail.com wrote:
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This patch changes this behavior. Post-probe function is introduced which performs probing of each pmics child and if it is a regulator, regulator_autoset function is called, which handles always-on and boot-on regulators, but if none of those props are set, the regulator is disabled.
Later disabled regulators can be re-enabled by devices which use them without issues.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
Can you add a test for this to test/dm/pmic.c ?
Sure, may you specify what I should add to tests/dm/pmic.c? Which behavior is needed?
Just for the change you are making...you should be able to check that it sets the regulators if you make sure there are some autoset ones in sandbox.
diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c index 0e2f5e1f41..8a26b519c9 100644 --- a/drivers/power/pmic/pmic-uclass.c +++ b/drivers/power/pmic/pmic-uclass.c @@ -16,6 +16,7 @@ #include <dm/device-internal.h> #include <dm/uclass-internal.h> #include <power/pmic.h> +#include <power/regulator.h> #include <linux/ctype.h>
#if CONFIG_IS_ENABLED(PMIC_CHILDREN) @@ -198,9 +199,26 @@ static int pmic_pre_probe(struct udevice *dev) return 0; }
+static int pmic_post_probe(struct udevice *dev) +{
struct udevice *child;int ret;device_foreach_child_probe(child, dev) {if (device_get_uclass_id(child) == UCLASS_REGULATOR) {ret = regulator_autoset(child);if (ret == -EMEDIUMTYPE)regulator_set_enable(child, false);};};return 0;+}
UCLASS_DRIVER(pmic) = { .id = UCLASS_PMIC, .name = "pmic", .pre_probe = pmic_pre_probe,
.post_probe = pmic_post_probe, .per_device_auto = sizeof(struct uc_pmic_priv),};
2.39.2
Regards, Simon
Regards, Simon
16 липня 2023 р. 16:08:10 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sat, 15 Jul 2023 at 22:08, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 02:40:37 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sat, 15 Jul 2023 at 12:34, Svyatoslav Ryhel clamor95@gmail.com wrote:
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This patch changes this behavior. Post-probe function is introduced which performs probing of each pmics child and if it is a regulator, regulator_autoset function is called, which handles always-on and boot-on regulators, but if none of those props are set, the regulator is disabled.
Later disabled regulators can be re-enabled by devices which use them without issues.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
Can you add a test for this to test/dm/pmic.c ?
Sure, may you specify what I should add to tests/dm/pmic.c? Which behavior is needed?
Just for the change you are making...you should be able to check that it sets the regulators if you make sure there are some autoset ones in sandbox.
Easier to tell then to do. I am basically interfering into device establishment process. If smth goes wrong, any regulator related test should fail. At least my devices refused to boot/gave unexpected behavior while developing. I will look if I can find anything meaningful.
diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c index 0e2f5e1f41..8a26b519c9 100644 --- a/drivers/power/pmic/pmic-uclass.c +++ b/drivers/power/pmic/pmic-uclass.c @@ -16,6 +16,7 @@ #include <dm/device-internal.h> #include <dm/uclass-internal.h> #include <power/pmic.h> +#include <power/regulator.h> #include <linux/ctype.h>
#if CONFIG_IS_ENABLED(PMIC_CHILDREN) @@ -198,9 +199,26 @@ static int pmic_pre_probe(struct udevice *dev) return 0; }
+static int pmic_post_probe(struct udevice *dev) +{
struct udevice *child;int ret;device_foreach_child_probe(child, dev) {if (device_get_uclass_id(child) == UCLASS_REGULATOR) {ret = regulator_autoset(child);if (ret == -EMEDIUMTYPE)regulator_set_enable(child, false);};};return 0;+}
UCLASS_DRIVER(pmic) = { .id = UCLASS_PMIC, .name = "pmic", .pre_probe = pmic_pre_probe,
.post_probe = pmic_post_probe, .per_device_auto = sizeof(struct uc_pmic_priv),};
2.39.2
Regards, Simon
Regards, Simon
Hi Svyatoslav,
On Sun, 16 Jul 2023 at 07:20, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 16:08:10 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sat, 15 Jul 2023 at 22:08, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 02:40:37 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sat, 15 Jul 2023 at 12:34, Svyatoslav Ryhel clamor95@gmail.com wrote:
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This patch changes this behavior. Post-probe function is introduced which performs probing of each pmics child and if it is a regulator, regulator_autoset function is called, which handles always-on and boot-on regulators, but if none of those props are set, the regulator is disabled.
Later disabled regulators can be re-enabled by devices which use them without issues.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
Can you add a test for this to test/dm/pmic.c ?
Sure, may you specify what I should add to tests/dm/pmic.c? Which behavior is needed?
Just for the change you are making...you should be able to check that it sets the regulators if you make sure there are some autoset ones in sandbox.
Easier to tell then to do. I am basically interfering into device establishment process. If smth goes wrong, any regulator related test should fail. At least my devices refused to boot/gave unexpected behavior while developing. I will look if I can find anything meaningful.
Something like this in test/dm/regulator.c:
struct udevice *pmic, *reg;
// This should automatically probe the ut_asserok(pmic_get("pmic@41"), &pmic);
// Check that the regulators are probed and on ut_assertok(regulator_get_by_devname("ldo2", ®)); ut_assert(regulator_get_enable(reg));
See test.dtsi and sandbox_pmic.dtsi for the DT nodes. You'll need to update ldo2 t too add the properties to cause an autoset.
Regards, Simon
Hi,
If will resend this patch according Simon's comment, I will check again.
Best Regards, Jaehoon Chung
-----Original Message----- From: Simon Glass sjg@chromium.org Sent: Wednesday, July 19, 2023 10:08 AM To: Svyatoslav Ryhel clamor95@gmail.com Cc: Jaehoon Chung jh80.chung@samsung.com; Patrick Delaunay patrick.delaunay@foss.st.com; u- boot@lists.denx.de Subject: Re: [PATCH v2 2/2] power: pmic: fix regulators behaviour
Hi Svyatoslav,
On Sun, 16 Jul 2023 at 07:20, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 16:08:10 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sat, 15 Jul 2023 at 22:08, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 02:40:37 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sat, 15 Jul 2023 at 12:34, Svyatoslav Ryhel clamor95@gmail.com wrote:
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This patch changes this behavior. Post-probe function is introduced which performs probing of each pmics child and if it is a regulator, regulator_autoset function is called, which handles always-on and boot-on regulators, but if none of those props are set, the regulator is disabled.
Later disabled regulators can be re-enabled by devices which use them without issues.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
Can you add a test for this to test/dm/pmic.c ?
Sure, may you specify what I should add to tests/dm/pmic.c? Which behavior is needed?
Just for the change you are making...you should be able to check that it sets the regulators if you make sure there are some autoset ones in sandbox.
Easier to tell then to do. I am basically interfering into device establishment process. If smth
goes wrong, any regulator related test should fail. At least my devices refused to boot/gave unexpected behavior while developing. I will look if I can find anything meaningful.
Something like this in test/dm/regulator.c:
struct udevice *pmic, *reg;
// This should automatically probe the ut_asserok(pmic_get("pmic@41"), &pmic);
// Check that the regulators are probed and on ut_assertok(regulator_get_by_devname("ldo2", ®)); ut_assert(regulator_get_enable(reg));
See test.dtsi and sandbox_pmic.dtsi for the DT nodes. You'll need to update ldo2 t too add the properties to cause an autoset.
Regards, Simon
31 жовтня 2023 р. 07:11:02 GMT+02:00, Jaehoon Chung jh80.chung@samsung.com написав(-ла):
Hi,
If will resend this patch according Simon's comment, I will check again.
Best Regards, Jaehoon Chung
It will not be updated since it was split into 2 different bigger patchsets one of which was dropped as well. You have missed a 3 or 4 month of discussion.
-----Original Message----- From: Simon Glass sjg@chromium.org Sent: Wednesday, July 19, 2023 10:08 AM To: Svyatoslav Ryhel clamor95@gmail.com Cc: Jaehoon Chung jh80.chung@samsung.com; Patrick Delaunay patrick.delaunay@foss.st.com; u- boot@lists.denx.de Subject: Re: [PATCH v2 2/2] power: pmic: fix regulators behaviour
Hi Svyatoslav,
On Sun, 16 Jul 2023 at 07:20, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 16:08:10 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sat, 15 Jul 2023 at 22:08, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 02:40:37 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sat, 15 Jul 2023 at 12:34, Svyatoslav Ryhel clamor95@gmail.com wrote: > > Currently device tree entries of regulators are completely > ignored and regulators are probed only if they are called > by the device which uses it. This results into two issues: > regulators which must run under boot-on or always-on mode > are ignored and not enabled; dts props like voltage are > not applied to the regulator so the regulator may be enabled > with random actual voltage, which may have unexpected > consequences. > > This patch changes this behavior. Post-probe function is > introduced which performs probing of each pmics child and if > it is a regulator, regulator_autoset function is called, which > handles always-on and boot-on regulators, but if none of those > props are set, the regulator is disabled. > > Later disabled regulators can be re-enabled by devices which > use them without issues. > > Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com > --- > drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+)
Can you add a test for this to test/dm/pmic.c ?
Sure, may you specify what I should add to tests/dm/pmic.c? Which behavior is needed?
Just for the change you are making...you should be able to check that it sets the regulators if you make sure there are some autoset ones in sandbox.
Easier to tell then to do. I am basically interfering into device establishment process. If smth
goes wrong, any regulator related test should fail. At least my devices refused to boot/gave unexpected behavior while developing. I will look if I can find anything meaningful.
Something like this in test/dm/regulator.c:
struct udevice *pmic, *reg;
// This should automatically probe the ut_asserok(pmic_get("pmic@41"), &pmic);
// Check that the regulators are probed and on ut_assertok(regulator_get_by_devname("ldo2", ®)); ut_assert(regulator_get_enable(reg));
See test.dtsi and sandbox_pmic.dtsi for the DT nodes. You'll need to update ldo2 t too add the properties to cause an autoset.
Regards, Simon
On 7/16/23 03:34, Svyatoslav Ryhel wrote:
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This patch changes this behavior. Post-probe function is introduced which performs probing of each pmics child and if it is a regulator, regulator_autoset function is called, which handles always-on and boot-on regulators, but if none of those props are set, the regulator is disabled.
Later disabled regulators can be re-enabled by devices which use them without issues.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
Reviewed-by: Jaehoon Chung jh80.chung@samsung.com
Best Regards, Jaehoon Chung
drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c index 0e2f5e1f41..8a26b519c9 100644 --- a/drivers/power/pmic/pmic-uclass.c +++ b/drivers/power/pmic/pmic-uclass.c @@ -16,6 +16,7 @@ #include <dm/device-internal.h> #include <dm/uclass-internal.h> #include <power/pmic.h> +#include <power/regulator.h> #include <linux/ctype.h>
#if CONFIG_IS_ENABLED(PMIC_CHILDREN) @@ -198,9 +199,26 @@ static int pmic_pre_probe(struct udevice *dev) return 0; }
+static int pmic_post_probe(struct udevice *dev) +{
- struct udevice *child;
- int ret;
- device_foreach_child_probe(child, dev) {
if (device_get_uclass_id(child) == UCLASS_REGULATOR) {ret = regulator_autoset(child);if (ret == -EMEDIUMTYPE)regulator_set_enable(child, false);};- };
- return 0;
+}
UCLASS_DRIVER(pmic) = { .id = UCLASS_PMIC, .name = "pmic", .pre_probe = pmic_pre_probe,
- .post_probe = pmic_post_probe, .per_device_auto = sizeof(struct uc_pmic_priv),
};
31 жовтня 2023 р. 07:03:17 GMT+02:00, Jaehoon Chung jh80.chung@samsung.com написав(-ла):
On 7/16/23 03:34, Svyatoslav Ryhel wrote:
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This patch changes this behavior. Post-probe function is introduced which performs probing of each pmics child and if it is a regulator, regulator_autoset function is called, which handles always-on and boot-on regulators, but if none of those props are set, the regulator is disabled.
Later disabled regulators can be re-enabled by devices which use them without issues.
Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
Reviewed-by: Jaehoon Chung jh80.chung@samsung.com
Best Regards, Jaehoon Chung
power: pmic: add TPS65913 support patchset is outdated already and should not be applied!
drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c index 0e2f5e1f41..8a26b519c9 100644 --- a/drivers/power/pmic/pmic-uclass.c +++ b/drivers/power/pmic/pmic-uclass.c @@ -16,6 +16,7 @@ #include <dm/device-internal.h> #include <dm/uclass-internal.h> #include <power/pmic.h> +#include <power/regulator.h> #include <linux/ctype.h>
#if CONFIG_IS_ENABLED(PMIC_CHILDREN) @@ -198,9 +199,26 @@ static int pmic_pre_probe(struct udevice *dev) return 0; }
+static int pmic_post_probe(struct udevice *dev) +{
- struct udevice *child;
- int ret;
- device_foreach_child_probe(child, dev) {
if (device_get_uclass_id(child) == UCLASS_REGULATOR) {ret = regulator_autoset(child);if (ret == -EMEDIUMTYPE)regulator_set_enable(child, false);};- };
- return 0;
+}
UCLASS_DRIVER(pmic) = { .id = UCLASS_PMIC, .name = "pmic", .pre_probe = pmic_pre_probe,
- .post_probe = pmic_post_probe, .per_device_auto = sizeof(struct uc_pmic_priv),
};
On 2023-07-15 20:34, Svyatoslav Ryhel wrote:
Existing PALMAS PMIC driver is fully compatible with TI TPS65913 PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS TF701T. Add TPS65913 dts compatible with TPS659038 data.
Issue with regulators is more general then I though initially. It touches all pmic regulators.
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This sounds like something a call to regulators_enable_boot_on like is done on other platforms/boards could solve?
Regards, Jonas
Second patch changes this behavior. Post-probe function is introduced which performs probing of each pmics child and if it is a regulator, regulator_autoset function is called, which handles always-on and boot-on regulators, but if none of those props are set, the regulator is disabled.
Later disabled regulators can be re-enabled by devices which use them without issues.
I have tested this change on multiple devices with different PMICs (LG P895, max77663; TF300T with TPS65911; HTC One X with TPS80031; Tegratab with TPS65913). Without this patch they behave as described in the third paragraph. With patch behavior is correct.
Changes from v1:
- implemented general fix of regulator behavior, which include:
- introduction of post-probe into pmic-uclass driver
- probing all pmic childs in post-probe
- check if device is regulator > autoset if regulator is boot-on or always-on else disable regulator
Svyatoslav Ryhel (2): power: pmic: support TI TPS65913 PMIC power: pmic: fix regulators behaviour
drivers/power/pmic/palmas.c | 1 + drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+)
16 липня 2023 р. 02:37:39 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-15 20:34, Svyatoslav Ryhel wrote:
Existing PALMAS PMIC driver is fully compatible with TI TPS65913 PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS TF701T. Add TPS65913 dts compatible with TPS659038 data.
Issue with regulators is more general then I though initially. It touches all pmic regulators.
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This sounds like something a call to regulators_enable_boot_on like is done on other platforms/boards could solve?
Yes, regulators_enable_boot_on can solve this if called from the board, but why should every device call this from the board if this should be done automatically? What should do devices without board? Isn't u-boot moving towards all in device tree setup?
Best regards, Svyatoslav R.
Regards, Jonas
Second patch changes this behavior. Post-probe function is introduced which performs probing of each pmics child and if it is a regulator, regulator_autoset function is called, which handles always-on and boot-on regulators, but if none of those props are set, the regulator is disabled.
Later disabled regulators can be re-enabled by devices which use them without issues.
I have tested this change on multiple devices with different PMICs (LG P895, max77663; TF300T with TPS65911; HTC One X with TPS80031; Tegratab with TPS65913). Without this patch they behave as described in the third paragraph. With patch behavior is correct.
Changes from v1:
- implemented general fix of regulator behavior, which include:
- introduction of post-probe into pmic-uclass driver
- probing all pmic childs in post-probe
- check if device is regulator > autoset if regulator is boot-on or always-on else disable regulator
Svyatoslav Ryhel (2): power: pmic: support TI TPS65913 PMIC power: pmic: fix regulators behaviour
drivers/power/pmic/palmas.c | 1 + drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+)
On 2023-07-16 05:57, Svyatoslav Ryhel wrote:
16 липня 2023 р. 02:37:39 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-15 20:34, Svyatoslav Ryhel wrote:
Existing PALMAS PMIC driver is fully compatible with TI TPS65913 PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS TF701T. Add TPS65913 dts compatible with TPS659038 data.
Issue with regulators is more general then I though initially. It touches all pmic regulators.
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This sounds like something a call to regulators_enable_boot_on like is done on other platforms/boards could solve?
Yes, regulators_enable_boot_on can solve this if called from the board, but why should every device call this from the board if this should be done automatically? What should do devices without board? Isn't u-boot moving towards all in device tree setup?
Main diff is that regulators_enable_boot_on covers all regulators, including fixed/gpio/pwm regulators, not just regulators that are children of a pmic. Meaning platforms/boards would still need to somehow autoset non-pmic regulators.
Maybe regulators_enable_boot_on could be called from the default power_init_board or similar, when a Kconfig is enabled, to cover more platforms/boards.
Regards, Jonas
Best regards, Svyatoslav R.
Regards, Jonas
Second patch changes this behavior. Post-probe function is introduced which performs probing of each pmics child and if it is a regulator, regulator_autoset function is called, which handles always-on and boot-on regulators, but if none of those props are set, the regulator is disabled.
Later disabled regulators can be re-enabled by devices which use them without issues.
I have tested this change on multiple devices with different PMICs (LG P895, max77663; TF300T with TPS65911; HTC One X with TPS80031; Tegratab with TPS65913). Without this patch they behave as described in the third paragraph. With patch behavior is correct.
Changes from v1:
- implemented general fix of regulator behavior, which include:
- introduction of post-probe into pmic-uclass driver
- probing all pmic childs in post-probe
- check if device is regulator > autoset if regulator is boot-on or always-on else disable regulator
Svyatoslav Ryhel (2): power: pmic: support TI TPS65913 PMIC power: pmic: fix regulators behaviour
drivers/power/pmic/palmas.c | 1 + drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+)
16 липня 2023 р. 11:07:15 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-16 05:57, Svyatoslav Ryhel wrote:
16 липня 2023 р. 02:37:39 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-15 20:34, Svyatoslav Ryhel wrote:
Existing PALMAS PMIC driver is fully compatible with TI TPS65913 PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS TF701T. Add TPS65913 dts compatible with TPS659038 data.
Issue with regulators is more general then I though initially. It touches all pmic regulators.
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This sounds like something a call to regulators_enable_boot_on like is done on other platforms/boards could solve?
Yes, regulators_enable_boot_on can solve this if called from the board, but why should every device call this from the board if this should be done automatically? What should do devices without board? Isn't u-boot moving towards all in device tree setup?
Main diff is that regulators_enable_boot_on covers all regulators, including fixed/gpio/pwm regulators, not just regulators that are children of a pmic. Meaning platforms/boards would still need to somehow autoset non-pmic regulators.
This is a good point. Non pmic regulators are not probed/set.
Maybe regulators_enable_boot_on could be called from the default power_init_board or similar, when a Kconfig is enabled, to cover more platforms/boards.
Since u-boot is moving towards device trees this should be undesireable approach. What is possible is to set all non pmic regulators to probe on post bind and on post probe call regulator_autoset.
Regards, Jonas
Best regards, Svyatoslav R.
Regards, Jonas
Second patch changes this behavior. Post-probe function is introduced which performs probing of each pmics child and if it is a regulator, regulator_autoset function is called, which handles always-on and boot-on regulators, but if none of those props are set, the regulator is disabled.
Later disabled regulators can be re-enabled by devices which use them without issues.
I have tested this change on multiple devices with different PMICs (LG P895, max77663; TF300T with TPS65911; HTC One X with TPS80031; Tegratab with TPS65913). Without this patch they behave as described in the third paragraph. With patch behavior is correct.
Changes from v1:
- implemented general fix of regulator behavior, which include:
- introduction of post-probe into pmic-uclass driver
- probing all pmic childs in post-probe
- check if device is regulator > autoset if regulator is boot-on or always-on else disable regulator
Svyatoslav Ryhel (2): power: pmic: support TI TPS65913 PMIC power: pmic: fix regulators behaviour
drivers/power/pmic/palmas.c | 1 + drivers/power/pmic/pmic-uclass.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+)
Hi Svyatoslav,
On Sun, 16 Jul 2023 at 07:04, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 11:07:15 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-16 05:57, Svyatoslav Ryhel wrote:
16 липня 2023 р. 02:37:39 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-15 20:34, Svyatoslav Ryhel wrote:
Existing PALMAS PMIC driver is fully compatible with TI TPS65913 PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS TF701T. Add TPS65913 dts compatible with TPS659038 data.
Issue with regulators is more general then I though initially. It touches all pmic regulators.
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This sounds like something a call to regulators_enable_boot_on like is done on other platforms/boards could solve?
Yes, regulators_enable_boot_on can solve this if called from the board, but why should every device call this from the board if this should be done automatically? What should do devices without board? Isn't u-boot moving towards all in device tree setup?
Main diff is that regulators_enable_boot_on covers all regulators, including fixed/gpio/pwm regulators, not just regulators that are children of a pmic. Meaning platforms/boards would still need to somehow autoset non-pmic regulators.
This is a good point. Non pmic regulators are not probed/set.
Maybe regulators_enable_boot_on could be called from the default power_init_board or similar, when a Kconfig is enabled, to cover more platforms/boards.
Since u-boot is moving towards device trees this should be undesireable approach. What is possible is to set all non pmic regulators to probe on post bind and on post probe call regulator_autoset.
Yes we should handle this automatically without board-specific C code, where possible.
Regards, Simon
16 липня 2023 р. 16:07:55 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sun, 16 Jul 2023 at 07:04, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 11:07:15 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-16 05:57, Svyatoslav Ryhel wrote:
16 липня 2023 р. 02:37:39 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-15 20:34, Svyatoslav Ryhel wrote:
Existing PALMAS PMIC driver is fully compatible with TI TPS65913 PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS TF701T. Add TPS65913 dts compatible with TPS659038 data.
Issue with regulators is more general then I though initially. It touches all pmic regulators.
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This sounds like something a call to regulators_enable_boot_on like is done on other platforms/boards could solve?
Yes, regulators_enable_boot_on can solve this if called from the board, but why should every device call this from the board if this should be done automatically? What should do devices without board? Isn't u-boot moving towards all in device tree setup?
Main diff is that regulators_enable_boot_on covers all regulators, including fixed/gpio/pwm regulators, not just regulators that are children of a pmic. Meaning platforms/boards would still need to somehow autoset non-pmic regulators.
This is a good point. Non pmic regulators are not probed/set.
Maybe regulators_enable_boot_on could be called from the default power_init_board or similar, when a Kconfig is enabled, to cover more platforms/boards.
Since u-boot is moving towards device trees this should be undesireable approach. What is possible is to set all non pmic regulators to probe on post bind and on post probe call regulator_autoset.
Yes we should handle this automatically without board-specific C code, where possible.
I will look into this and reload patches once done. Only downfall I can see right now is that strongly bound regulators (like pmic-regulator complex) will require more attention. From what I can remember such bounds may be battery-regulator and charger-regulator. I will exclude those from probing as well. But if regulator is felf sustained device. I see no downsides.
Regards, Simon
On 2023-07-16 15:17, Svyatoslav Ryhel wrote:
16 липня 2023 р. 16:07:55 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sun, 16 Jul 2023 at 07:04, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 11:07:15 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-16 05:57, Svyatoslav Ryhel wrote:
16 липня 2023 р. 02:37:39 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-15 20:34, Svyatoslav Ryhel wrote: > Existing PALMAS PMIC driver is fully compatible with TI TPS65913 > PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS > TF701T. Add TPS65913 dts compatible with TPS659038 data. > > Issue with regulators is more general then I though initially. > It touches all pmic regulators. > > Currently device tree entries of regulators are completely > ignored and regulators are probed only if they are called > by the device which uses it. This results into two issues: > regulators which must run under boot-on or always-on mode > are ignored and not enabled; dts props like voltage are > not applied to the regulator so the regulator may be enabled > with random actual voltage, which may have unexpected > consequences.
This sounds like something a call to regulators_enable_boot_on like is done on other platforms/boards could solve?
Yes, regulators_enable_boot_on can solve this if called from the board, but why should every device call this from the board if this should be done automatically? What should do devices without board? Isn't u-boot moving towards all in device tree setup?
Main diff is that regulators_enable_boot_on covers all regulators, including fixed/gpio/pwm regulators, not just regulators that are children of a pmic. Meaning platforms/boards would still need to somehow autoset non-pmic regulators.
This is a good point. Non pmic regulators are not probed/set.
Maybe regulators_enable_boot_on could be called from the default power_init_board or similar, when a Kconfig is enabled, to cover more platforms/boards.
Since u-boot is moving towards device trees this should be undesireable approach. What is possible is to set all non pmic regulators to probe on post bind and on post probe call regulator_autoset.
Yes we should handle this automatically without board-specific C code, where possible.
I will look into this and reload patches once done. Only downfall I can see right now is that strongly bound regulators (like pmic-regulator complex) will require more attention. From what I can remember such bounds may be battery-regulator and charger-regulator. I will exclude those from probing as well. But if regulator is felf sustained device. I see no downsides.
Great, keep in mind that after commit 4fcba5d556b4 ("regulator: implement basic reference counter"), calling autoset multiple times on fixed/gpio regulator would increase their reference counter.
Meaning regulators_enable_boot_on should probably be if calling autoset is moved to regulator uclass post probe or similar. Else we may end up with boot-on regulators that start with a reference counter of >1 instead of =1.
Regards, Jonas
Regards, Simon
16 липня 2023 р. 17:27:39 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-16 15:17, Svyatoslav Ryhel wrote:
16 липня 2023 р. 16:07:55 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sun, 16 Jul 2023 at 07:04, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 11:07:15 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-16 05:57, Svyatoslav Ryhel wrote:
16 липня 2023 р. 02:37:39 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла): > On 2023-07-15 20:34, Svyatoslav Ryhel wrote: >> Existing PALMAS PMIC driver is fully compatible with TI TPS65913 >> PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS >> TF701T. Add TPS65913 dts compatible with TPS659038 data. >> >> Issue with regulators is more general then I though initially. >> It touches all pmic regulators. >> >> Currently device tree entries of regulators are completely >> ignored and regulators are probed only if they are called >> by the device which uses it. This results into two issues: >> regulators which must run under boot-on or always-on mode >> are ignored and not enabled; dts props like voltage are >> not applied to the regulator so the regulator may be enabled >> with random actual voltage, which may have unexpected >> consequences. > > This sounds like something a call to regulators_enable_boot_on like is > done on other platforms/boards could solve?
Yes, regulators_enable_boot_on can solve this if called from the board, but why should every device call this from the board if this should be done automatically? What should do devices without board? Isn't u-boot moving towards all in device tree setup?
Main diff is that regulators_enable_boot_on covers all regulators, including fixed/gpio/pwm regulators, not just regulators that are children of a pmic. Meaning platforms/boards would still need to somehow autoset non-pmic regulators.
This is a good point. Non pmic regulators are not probed/set.
Maybe regulators_enable_boot_on could be called from the default power_init_board or similar, when a Kconfig is enabled, to cover more platforms/boards.
Since u-boot is moving towards device trees this should be undesireable approach. What is possible is to set all non pmic regulators to probe on post bind and on post probe call regulator_autoset.
Yes we should handle this automatically without board-specific C code, where possible.
I will look into this and reload patches once done. Only downfall I can see right now is that strongly bound regulators (like pmic-regulator complex) will require more attention. From what I can remember such bounds may be battery-regulator and charger-regulator. I will exclude those from probing as well. But if regulator is felf sustained device. I see no downsides.
Great, keep in mind that after commit 4fcba5d556b4 ("regulator: implement basic reference counter"), calling autoset multiple times on fixed/gpio regulator would increase their reference counter.
Meaning regulators_enable_boot_on should probably be if calling autoset is moved to regulator uclass post probe or similar. Else we may end up with boot-on regulators that start with a reference counter of >1 instead of =1.
Thanks for a warning.
Ideally regulators_enable_boot_on/regulators_enable_boot_off should be removed at all. Then all regulators stuff will be handled within regulators uclass.
Regards, Jonas
Regards, Simon
16 липня 2023 р. 16:07:55 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sun, 16 Jul 2023 at 07:04, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 11:07:15 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-16 05:57, Svyatoslav Ryhel wrote:
16 липня 2023 р. 02:37:39 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-15 20:34, Svyatoslav Ryhel wrote:
Existing PALMAS PMIC driver is fully compatible with TI TPS65913 PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS TF701T. Add TPS65913 dts compatible with TPS659038 data.
Issue with regulators is more general then I though initially. It touches all pmic regulators.
Currently device tree entries of regulators are completely ignored and regulators are probed only if they are called by the device which uses it. This results into two issues: regulators which must run under boot-on or always-on mode are ignored and not enabled; dts props like voltage are not applied to the regulator so the regulator may be enabled with random actual voltage, which may have unexpected consequences.
This sounds like something a call to regulators_enable_boot_on like is done on other platforms/boards could solve?
Yes, regulators_enable_boot_on can solve this if called from the board, but why should every device call this from the board if this should be done automatically? What should do devices without board? Isn't u-boot moving towards all in device tree setup?
Main diff is that regulators_enable_boot_on covers all regulators, including fixed/gpio/pwm regulators, not just regulators that are children of a pmic. Meaning platforms/boards would still need to somehow autoset non-pmic regulators.
This is a good point. Non pmic regulators are not probed/set.
Maybe regulators_enable_boot_on could be called from the default power_init_board or similar, when a Kconfig is enabled, to cover more platforms/boards.
Since u-boot is moving towards device trees this should be undesireable approach. What is possible is to set all non pmic regulators to probe on post bind and on post probe call regulator_autoset.
Yes we should handle this automatically without board-specific C code, where possible.
Commit, Jonas Karlman refers to 4fcba5d556b4 ("regulator: implement basic reference counter") introduces too much conflicting.
1. It should not have been merged in the current state and instead expanded for all uclass. Counting regulator users is not fixed/gpio regulator specific and can be beneficial for all devices.
2. Usage of board and device tree call to set up regulators will not work well with counting enables/disables. The correct solution should be to remove board calls for regulator setup but it will involve rework of many boards.
Regards, Simon
On 2023-07-16 20:57, Svyatoslav Ryhel wrote:
16 липня 2023 р. 16:07:55 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sun, 16 Jul 2023 at 07:04, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 11:07:15 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-16 05:57, Svyatoslav Ryhel wrote:
16 липня 2023 р. 02:37:39 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-15 20:34, Svyatoslav Ryhel wrote: > Existing PALMAS PMIC driver is fully compatible with TI TPS65913 > PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS > TF701T. Add TPS65913 dts compatible with TPS659038 data. > > Issue with regulators is more general then I though initially. > It touches all pmic regulators. > > Currently device tree entries of regulators are completely > ignored and regulators are probed only if they are called > by the device which uses it. This results into two issues: > regulators which must run under boot-on or always-on mode > are ignored and not enabled; dts props like voltage are > not applied to the regulator so the regulator may be enabled > with random actual voltage, which may have unexpected > consequences.
This sounds like something a call to regulators_enable_boot_on like is done on other platforms/boards could solve?
Yes, regulators_enable_boot_on can solve this if called from the board, but why should every device call this from the board if this should be done automatically? What should do devices without board? Isn't u-boot moving towards all in device tree setup?
Main diff is that regulators_enable_boot_on covers all regulators, including fixed/gpio/pwm regulators, not just regulators that are children of a pmic. Meaning platforms/boards would still need to somehow autoset non-pmic regulators.
This is a good point. Non pmic regulators are not probed/set.
Maybe regulators_enable_boot_on could be called from the default power_init_board or similar, when a Kconfig is enabled, to cover more platforms/boards.
Since u-boot is moving towards device trees this should be undesireable approach. What is possible is to set all non pmic regulators to probe on post bind and on post probe call regulator_autoset.
Yes we should handle this automatically without board-specific C code, where possible.
Commit, Jonas Karlman refers to 4fcba5d556b4 ("regulator: implement basic reference counter") introduces too much conflicting.
- It should not have been merged in the current state and instead expanded for all uclass. Counting regulator users is not fixed/gpio regulator specific and can be beneficial for all devices.
I think it was limited to fixed/gpio regulators just to not affect too many arch/boards. And can always be extended to core regulator uclass in the future.
There is also drivers that do not currently keep enable/disable balanced, will shortly send a series with fixes for a few I have discovered.
- Usage of board and device tree call to set up regulators will not work well with counting enables/disables. The correct solution should be to remove board calls for regulator setup but it will involve rework of many boards.
This was main reason why I mentioned this commit, it currently works, but if you introduce new code that call regulator_autoset from post probe, existing code must be adjusted or removed.
E.g. regulators_enable_boot_on called in a rockchip arch shared board_init could be removed. Or logic in regulator_autoset can be changed to only run code once for each regulator. Or add a Kconfig to enable post probe autoset and disable autoset in regulators_enable_boot_on.
Regards, Jonas
Regards, Simon
17 липня 2023 р. 21:52:58 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-16 20:57, Svyatoslav Ryhel wrote:
16 липня 2023 р. 16:07:55 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sun, 16 Jul 2023 at 07:04, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 11:07:15 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-16 05:57, Svyatoslav Ryhel wrote:
16 липня 2023 р. 02:37:39 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла): > On 2023-07-15 20:34, Svyatoslav Ryhel wrote: >> Existing PALMAS PMIC driver is fully compatible with TI TPS65913 >> PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS >> TF701T. Add TPS65913 dts compatible with TPS659038 data. >> >> Issue with regulators is more general then I though initially. >> It touches all pmic regulators. >> >> Currently device tree entries of regulators are completely >> ignored and regulators are probed only if they are called >> by the device which uses it. This results into two issues: >> regulators which must run under boot-on or always-on mode >> are ignored and not enabled; dts props like voltage are >> not applied to the regulator so the regulator may be enabled >> with random actual voltage, which may have unexpected >> consequences. > > This sounds like something a call to regulators_enable_boot_on like is > done on other platforms/boards could solve?
Yes, regulators_enable_boot_on can solve this if called from the board, but why should every device call this from the board if this should be done automatically? What should do devices without board? Isn't u-boot moving towards all in device tree setup?
Main diff is that regulators_enable_boot_on covers all regulators, including fixed/gpio/pwm regulators, not just regulators that are children of a pmic. Meaning platforms/boards would still need to somehow autoset non-pmic regulators.
This is a good point. Non pmic regulators are not probed/set.
Maybe regulators_enable_boot_on could be called from the default power_init_board or similar, when a Kconfig is enabled, to cover more platforms/boards.
Since u-boot is moving towards device trees this should be undesireable approach. What is possible is to set all non pmic regulators to probe on post bind and on post probe call regulator_autoset.
Yes we should handle this automatically without board-specific C code, where possible.
Commit, Jonas Karlman refers to 4fcba5d556b4 ("regulator: implement basic reference counter") introduces too much conflicting.
- It should not have been merged in the current state and instead expanded for all uclass. Counting regulator users is not fixed/gpio regulator specific and can be beneficial for all devices.
I think it was limited to fixed/gpio regulators just to not affect too many arch/boards. And can always be extended to core regulator uclass in the future.
I have spent the entire day on this stuff. I will load my propositions tomorrow.
There is also drivers that do not currently keep enable/disable balanced, will shortly send a series with fixes for a few I have discovered.
Well, iirc all devices without remove have imbalanced calls. It should not be as critical as it is for linux.
- Usage of board and device tree call to set up regulators will not work well with counting enables/disables. The correct solution should be to remove board calls for regulator setup but it will involve rework of many boards.
This was main reason why I mentioned this commit, it currently works, but if you introduce new code that call regulator_autoset from post probe, existing code must be adjusted or removed.
Removed.
E.g. regulators_enable_boot_on called in a rockchip arch shared board_init could be removed. Or logic in regulator_autoset can be changed to only run code once for each regulator. Or add a Kconfig to enable post probe autoset and disable autoset in regulators_enable_boot_on.
I will load tomorrow what I have found. But it is not complete and I would need all possible help and assist I can get from community.
Jonas, Simon, thank you for your responces. Those are highly appresiated.
Best regards, Svyatoslav R.
Regards, Jonas
Regards, Simon
Problems which were faced with this patchset exceed just TPS65913 support. TPS65913 support commits will be moved into different patchset dedicated exclusively to PMICs bringup. Power uclasses commits will be sent as separate patchset with proper title.
Best regards, Svyatoslav R.
пн, 17 лип. 2023 р. о 21:52 Jonas Karlman jonas@kwiboo.se пише:
On 2023-07-16 20:57, Svyatoslav Ryhel wrote:
16 липня 2023 р. 16:07:55 GMT+03:00, Simon Glass sjg@chromium.org написав(-ла):
Hi Svyatoslav,
On Sun, 16 Jul 2023 at 07:04, Svyatoslav Ryhel clamor95@gmail.com wrote:
16 липня 2023 р. 11:07:15 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла):
On 2023-07-16 05:57, Svyatoslav Ryhel wrote:
16 липня 2023 р. 02:37:39 GMT+03:00, Jonas Karlman jonas@kwiboo.se написав(-ла): > On 2023-07-15 20:34, Svyatoslav Ryhel wrote: >> Existing PALMAS PMIC driver is fully compatible with TI TPS65913 >> PMIC found in many Tegra 4 devices, like Tegra Note 7 and ASUS >> TF701T. Add TPS65913 dts compatible with TPS659038 data. >> >> Issue with regulators is more general then I though initially. >> It touches all pmic regulators. >> >> Currently device tree entries of regulators are completely >> ignored and regulators are probed only if they are called >> by the device which uses it. This results into two issues: >> regulators which must run under boot-on or always-on mode >> are ignored and not enabled; dts props like voltage are >> not applied to the regulator so the regulator may be enabled >> with random actual voltage, which may have unexpected >> consequences. > > This sounds like something a call to regulators_enable_boot_on like is > done on other platforms/boards could solve?
Yes, regulators_enable_boot_on can solve this if called from the board, but why should every device call this from the board if this should be done automatically? What should do devices without board? Isn't u-boot moving towards all in device tree setup?
Main diff is that regulators_enable_boot_on covers all regulators, including fixed/gpio/pwm regulators, not just regulators that are children of a pmic. Meaning platforms/boards would still need to somehow autoset non-pmic regulators.
This is a good point. Non pmic regulators are not probed/set.
Maybe regulators_enable_boot_on could be called from the default power_init_board or similar, when a Kconfig is enabled, to cover more platforms/boards.
Since u-boot is moving towards device trees this should be undesireable approach. What is possible is to set all non pmic regulators to probe on post bind and on post probe call regulator_autoset.
Yes we should handle this automatically without board-specific C code, where possible.
Commit, Jonas Karlman refers to 4fcba5d556b4 ("regulator: implement basic reference counter") introduces too much conflicting.
- It should not have been merged in the current state and instead expanded for all uclass. Counting regulator users is not fixed/gpio regulator specific and can be beneficial for all devices.
I think it was limited to fixed/gpio regulators just to not affect too many arch/boards. And can always be extended to core regulator uclass in the future.
There is also drivers that do not currently keep enable/disable balanced, will shortly send a series with fixes for a few I have discovered.
- Usage of board and device tree call to set up regulators will not work well with counting enables/disables. The correct solution should be to remove board calls for regulator setup but it will involve rework of many boards.
This was main reason why I mentioned this commit, it currently works, but if you introduce new code that call regulator_autoset from post probe, existing code must be adjusted or removed.
E.g. regulators_enable_boot_on called in a rockchip arch shared board_init could be removed. Or logic in regulator_autoset can be changed to only run code once for each regulator. Or add a Kconfig to enable post probe autoset and disable autoset in regulators_enable_boot_on.
Regards, Jonas
Regards, Simon
participants (4)
-
Jaehoon Chung -
Jonas Karlman -
Simon Glass -
Svyatoslav Ryhel