2023-02-18 08:33:01

by Saravana Kannan

Subject: [RFC v1 0/4] Simplify regulator supply resolution code by offloading to driver core

Hi Mark/Liam,

This series is just an RFC to see if you agree with where this is going.
Please point out bugs, but don't bother with a proper code review.

The high level idea is to not reimplement what driver core can already
handle for us and use it to do some of the work. Instead of trying to
resolve supplies from all different code paths and bits and pieces of
the tree, we just build it from the root to the leaves by using deferred
probing to sequence things in the right order.

The last patch is the main one. Rest of them are just setting up for it.

I believe there's room for further simplification but this is what I
could whip up as a quick first draft that shows the high level idea.
I'll probably need some help with getting a better understanding of why
things are done in a specific order in regulator_register() before I
could attempt simplifying things further.

Ideally, regulator_register() would just have DT parsing, init data
struct sanity checks and adding the regulator device and then we move
everything else to into the probe function that's guaranteed to run only
after the supply has been resolved/ready to resolve.

fw_devlink/device links should further optimize the flow and also allow
us to simplify some of the guarantees and address some of the existing
FIXMEs. But this patch series is NOT dependent on fw_devlink or device
links.

Any thoughts on where this is going?

I've tested this on one hardware I have and it works and nothing is
broken. But the regulator tree in my hardware isn't that complicated or
deep. The regulators are also added mostly in the right order (due to
existing fw_devlink). So if you agree with the idea, the next step is to
ask people to give it a test.

Also, it's based on driver-core-next since that's what I had synced up
and had a working baseline. I'll rebase it on the regulator tree when I
go from RFC -> PATCH.

Thanks,
Saravana

Cc: Greg Kroah-Hartman <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Marek Szyprowski <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Sudeep Holla <[email protected]>
Cc: Tony Lindgren <[email protected]>
Cc: Doug Anderson <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Luca Weiss <[email protected]>

Saravana Kannan (4):
regulator: core: Add regulator devices to bus instead of class
regulator: core: Add sysfs class backward compatibility
regulator: core: Probe regulator devices
regulator: core: Move regulator supply resolving to the probe function

drivers/regulator/core.c | 102 +++++++++++++++++++------------
drivers/regulator/internal.h | 2 +-
drivers/regulator/of_regulator.c | 2 +-
3 files changed, 64 insertions(+), 42 deletions(-)

--
2.39.2.637.g21b0678d19-goog

2023-02-18 08:33:13

by Saravana Kannan

Subject: [RFC v1 1/4] regulator: core: Add regulator devices to bus instead of class

Add regulator devices to a bus instead of a class. This allows us to
probe these devices in later patches.

Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/regulator/core.c | 45 ++++++++++++++++----------------
drivers/regulator/internal.h | 2 +-
drivers/regulator/of_regulator.c | 2 +-
3 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index ae69e493913d..1a212edcf216 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1918,7 +1918,7 @@ static struct regulator_dev *regulator_lookup_by_name(const char *name)
{
struct device *dev;

- dev = class_find_device(&regulator_class, NULL, name, regulator_match);
+ dev = bus_find_device(&regulator_bus, NULL, name, regulator_match);

return dev ? dev_to_rdev(dev) : NULL;
}
@@ -5539,7 +5539,7 @@ regulator_register(struct device *dev,
rdev->supply_name = regulator_desc->supply_name;

/* register with sysfs */
- rdev->dev.class = &regulator_class;
+ rdev->dev.bus = &regulator_bus;
rdev->dev.parent = config->dev;
dev_set_name(&rdev->dev, "regulator.%lu",
(unsigned long) atomic_inc_return(&regulator_no));
@@ -5644,8 +5644,8 @@ regulator_register(struct device *dev,
mutex_unlock(&regulator_list_mutex);

/* try to resolve regulators supply since a new one was registered */
- class_for_each_device(&regulator_class, NULL, NULL,
- regulator_register_resolve_supply);
+ bus_for_each_dev(&regulator_bus, NULL, NULL,
+ regulator_register_resolve_supply);
kfree(config);
return rdev;

@@ -5772,14 +5772,15 @@ static const struct dev_pm_ops __maybe_unused regulator_pm_ops = {
};
#endif

-struct class regulator_class = {
+struct bus_type regulator_bus = {
.name = "regulator",
- .dev_release = regulator_dev_release,
+ .remove = regulator_dev_release,
.dev_groups = regulator_dev_groups,
#ifdef CONFIG_PM
.pm = &regulator_pm_ops,
#endif
};
+
/**
* regulator_has_full_constraints - the system has fully specified constraints
*
@@ -5939,7 +5940,7 @@ static void regulator_summary_show_subtree(struct seq_file *s,
seq_puts(s, "\n");

list_for_each_entry(consumer, &rdev->consumer_list, list) {
- if (consumer->dev && consumer->dev->class == &regulator_class)
+ if (consumer->dev && consumer->dev->bus == &regulator_bus)
continue;

seq_printf(s, "%*s%-*s ",
@@ -5969,8 +5970,8 @@ static void regulator_summary_show_subtree(struct seq_file *s,
summary_data.level = level;
summary_data.parent = rdev;

- class_for_each_device(&regulator_class, NULL, &summary_data,
- regulator_summary_show_children);
+ bus_for_each_dev(&regulator_bus, NULL, &summary_data,
+ regulator_summary_show_children);
}

struct summary_lock_data {
@@ -6025,11 +6026,11 @@ static int regulator_summary_lock_all(struct ww_acquire_ctx *ww_ctx,
lock_data.new_contended_rdev = new_contended_rdev;
lock_data.old_contended_rdev = old_contended_rdev;

- ret = class_for_each_device(&regulator_class, NULL, &lock_data,
- regulator_summary_lock_one);
+ ret = bus_for_each_dev(&regulator_bus, NULL, &lock_data,
+ regulator_summary_lock_one);
if (ret)
- class_for_each_device(&regulator_class, NULL, &lock_data,
- regulator_summary_unlock_one);
+ bus_for_each_dev(&regulator_bus, NULL, &lock_data,
+ regulator_summary_unlock_one);

return ret;
}
@@ -6065,8 +6066,8 @@ static void regulator_summary_lock(struct ww_acquire_ctx *ww_ctx)

static void regulator_summary_unlock(struct ww_acquire_ctx *ww_ctx)
{
- class_for_each_device(&regulator_class, NULL, NULL,
- regulator_summary_unlock_one);
+ bus_for_each_dev(&regulator_bus, NULL, NULL,
+ regulator_summary_unlock_one);
ww_acquire_fini(ww_ctx);

mutex_unlock(&regulator_list_mutex);
@@ -6092,8 +6093,8 @@ static int regulator_summary_show(struct seq_file *s, void *data)

regulator_summary_lock(&ww_ctx);

- class_for_each_device(&regulator_class, NULL, s,
- regulator_summary_show_roots);
+ bus_for_each_dev(&regulator_bus, NULL, s,
+ regulator_summary_show_roots);

regulator_summary_unlock(&ww_ctx);

@@ -6106,7 +6107,7 @@ static int __init regulator_init(void)
{
int ret;

- ret = class_register(&regulator_class);
+ ret = bus_register(&regulator_bus);

debugfs_root = debugfs_create_dir("regulator", NULL);
if (!debugfs_root)
@@ -6182,16 +6183,16 @@ static void regulator_init_complete_work_function(struct work_struct *work)
* bound yet. So attempt to resolve the input supplies for
* pending regulators before trying to disable unused ones.
*/
- class_for_each_device(&regulator_class, NULL, NULL,
- regulator_register_resolve_supply);
+ bus_for_each_dev(&regulator_bus, NULL, NULL,
+ regulator_register_resolve_supply);

/* If we have a full configuration then disable any regulators
* we have permission to change the status for and which are
* not in use or always_on. This is effectively the default
* for DT and ACPI as they have full constraints.
*/
- class_for_each_device(&regulator_class, NULL, NULL,
- regulator_late_cleanup);
+ bus_for_each_dev(&regulator_bus, NULL, NULL,
+ regulator_late_cleanup);
}

static DECLARE_DELAYED_WORK(regulator_init_complete_work,
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index fb4433068d29..6e489b3cffad 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -58,7 +58,7 @@ struct regulator {
struct dentry *debugfs;
};

-extern struct class regulator_class;
+extern struct bus_type regulator_bus;

static inline struct regulator_dev *dev_to_rdev(struct device *dev)
{
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 1b65e5e4e40f..f0590e68f31d 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -545,7 +545,7 @@ struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
{
struct device *dev;

- dev = class_find_device_by_of_node(&regulator_class, np);
+ dev = bus_find_device_by_of_node(&regulator_bus, np);

return dev ? dev_to_rdev(dev) : NULL;
}
--
2.39.2.637.g21b0678d19-goog

2023-02-18 08:33:13

by Saravana Kannan

Subject: [RFC v1 2/4] regulator: core: Add sysfs class backward compatibility

A regulator device's sysfs directory used to be created under
/sys/class/regulator when it is added to a class. Since the device is
now moved to be under a bus, add symlinks from /sys/class/regulator to
the real device sysfs directory.

Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/regulator/core.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 1a212edcf216..b6700d50d230 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -43,6 +43,7 @@ static LIST_HEAD(regulator_coupler_list);
static bool has_full_constraints;

static struct dentry *debugfs_root;
+static struct class_compat *regulator_compat_class;

/*
* struct regulator_map
@@ -5636,6 +5637,11 @@ regulator_register(struct device *dev,
if (ret != 0)
goto unset_supplies;

+ if (class_compat_create_link(regulator_compat_class, &rdev->dev,
+ rdev->dev.parent))
+ dev_warn(&rdev->dev,
+ "Failed to create compatibility class link\n");
+
rdev_init_debugfs(rdev);

/* try to resolve regulators coupling since a new one was registered */
@@ -5702,6 +5708,8 @@ void regulator_unregister(struct regulator_dev *rdev)
unset_regulator_supplies(rdev);
list_del(&rdev->list);
regulator_ena_gpio_free(rdev);
+ class_compat_remove_link(regulator_compat_class, &rdev->dev,
+ rdev->dev.parent);
device_unregister(&rdev->dev);

mutex_unlock(&regulator_list_mutex);
@@ -6107,7 +6115,13 @@ static int __init regulator_init(void)
{
int ret;

+ regulator_compat_class = class_compat_register("regulator");
+ if (!regulator_compat_class)
+ return -ENOMEM;
+
ret = bus_register(&regulator_bus);
+ if (ret)
+ goto unreg_compat;

debugfs_root = debugfs_create_dir("regulator", NULL);
if (!debugfs_root)
@@ -6120,11 +6134,16 @@ static int __init regulator_init(void)
debugfs_create_file("regulator_summary", 0444, debugfs_root,
NULL, &regulator_summary_fops);
#endif
+
regulator_dummy_init();

regulator_coupler_register(&generic_regulator_coupler);

return ret;
+
+unreg_compat:
+ class_compat_unregister(regulator_compat_class);
+ return ret;
}

/* init early to allow our consumers to complete system booting */
--
2.39.2.637.g21b0678d19-goog

2023-02-18 08:33:13

by Saravana Kannan

Subject: [RFC v1 3/4] regulator: core: Probe regulator devices

Since devices added to a bus can be probed, add a stub probe function
for regulator devices.

Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/regulator/core.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b6700d50d230..d5f9fdd79c14 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5780,6 +5780,17 @@ static const struct dev_pm_ops __maybe_unused regulator_pm_ops = {
};
#endif

+static int regulator_drv_probe(struct device *dev)
+{
+ return 0;
+}
+
+static struct device_driver regulator_drv = {
+ .name = "regulator_drv",
+ .bus = &regulator_bus,
+ .probe = regulator_drv_probe,
+};
+
struct bus_type regulator_bus = {
.name = "regulator",
.remove = regulator_dev_release,
@@ -6123,6 +6134,10 @@ static int __init regulator_init(void)
if (ret)
goto unreg_compat;

+ ret = driver_register(&regulator_drv);
+ if (ret)
+ goto unreg_bus;
+
debugfs_root = debugfs_create_dir("regulator", NULL);
if (!debugfs_root)
pr_warn("regulator: Failed to create debugfs directory\n");
@@ -6141,6 +6156,8 @@ static int __init regulator_init(void)

return ret;

+unreg_bus:
+ bus_unregister(&regulator_bus);
unreg_compat:
class_compat_unregister(regulator_compat_class);
return ret;
--
2.39.2.637.g21b0678d19-goog

2023-02-18 08:33:22

by Saravana Kannan

Subject: [RFC v1 4/4] regulator: core: Move regulator supply resolving to the probe function

We can simplify the regulator's supply resolving code if we resolve the
supply in the regulator's probe function. This allows us to:

- Consolidate the supply resolution code to one place.
- Avoid the need for recursion by allow driver core to take care of
handling dependencies.
- Avoid races and simplify locking by reusing the guarantees provided by
driver core.
- Avoid last minute/lazy resolving during regulator_get().
- Simplify error handling because we can assume the supply has been
resolved once a regulator is probed.
- Allow driver core to use device links/fw_devlink, where available, to
resolve the regulator supplies in the optimal order.

Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/regulator/core.c | 27 ++++++---------------------
1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d5f9fdd79c14..f3bf74d1a81d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1952,7 +1952,7 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
if (node) {
r = of_find_regulator_by_node(node);
of_node_put(node);
- if (r)
+ if (r && r->dev.links.status == DL_DEV_DRIVER_BOUND)
return r;

/*
@@ -1982,11 +1982,11 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
}
mutex_unlock(&regulator_list_mutex);

- if (r)
+ if (r && r->dev.links.status == DL_DEV_DRIVER_BOUND)
return r;

r = regulator_lookup_by_name(supply);
- if (r)
+ if (r && r->dev.links.status == DL_DEV_DRIVER_BOUND)
return r;

return ERR_PTR(-ENODEV);
@@ -2050,13 +2050,6 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
}
}

- /* Recursively resolve the supply of the supply */
- ret = regulator_resolve_supply(r);
- if (ret < 0) {
- put_device(&r->dev);
- goto out;
- }
-
/*
* Recheck rdev->supply with rdev->mutex lock held to avoid a race
* between rdev->supply null check and setting rdev->supply in
@@ -2178,13 +2171,6 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
return regulator;
}

- ret = regulator_resolve_supply(rdev);
- if (ret < 0) {
- regulator = ERR_PTR(ret);
- put_device(&rdev->dev);
- return regulator;
- }
-
if (!try_module_get(rdev->owner)) {
regulator = ERR_PTR(-EPROBE_DEFER);
put_device(&rdev->dev);
@@ -5649,9 +5635,6 @@ regulator_register(struct device *dev,
regulator_resolve_coupling(rdev);
mutex_unlock(&regulator_list_mutex);

- /* try to resolve regulators supply since a new one was registered */
- bus_for_each_dev(&regulator_bus, NULL, NULL,
- regulator_register_resolve_supply);
kfree(config);
return rdev;

@@ -5782,7 +5765,9 @@ static const struct dev_pm_ops __maybe_unused regulator_pm_ops = {

static int regulator_drv_probe(struct device *dev)
{
- return 0;
+ struct regulator_dev *rdev = dev_to_rdev(dev);
+
+ return regulator_resolve_supply(rdev);
}

static struct device_driver regulator_drv = {
--
2.39.2.637.g21b0678d19-goog

2023-02-18 08:37:13

by Saravana Kannan

Subject: Re: [RFC v1 0/4] Simplify regulator supply resolution code by offloading to driver core

On Sat, Feb 18, 2023 at 12:32 AM Saravana Kannan <[email protected]> wrote:
>
> Hi Mark/Liam,
>
> This series is just an RFC to see if you agree with where this is going.
> Please point out bugs, but don't bother with a proper code review.
>
> The high level idea is to not reimplement what driver core can already
> handle for us and use it to do some of the work. Instead of trying to
> resolve supplies from all different code paths and bits and pieces of
> the tree, we just build it from the root to the leaves by using deferred
> probing to sequence things in the right order.
>
> The last patch is the main one. Rest of them are just setting up for it.
>
> I believe there's room for further simplification but this is what I
> could whip up as a quick first draft that shows the high level idea.
> I'll probably need some help with getting a better understanding of why
> things are done in a specific order in regulator_register() before I
> could attempt simplifying things further.
>
> Ideally, regulator_register() would just have DT parsing, init data
> struct sanity checks and adding the regulator device and then we move
> everything else to into the probe function that's guaranteed to run only
> after the supply has been resolved/ready to resolve.
>
> fw_devlink/device links should further optimize the flow and also allow
> us to simplify some of the guarantees and address some of the existing
> FIXMEs. But this patch series is NOT dependent on fw_devlink or device
> links.
>
> Any thoughts on where this is going?
>
> I've tested this on one hardware I have and it works and nothing is
> broken. But the regulator tree in my hardware isn't that complicated or
> deep. The regulators are also added mostly in the right order (due to
> existing fw_devlink). So if you agree with the idea, the next step is to
> ask people to give it a test.
>
> Also, it's based on driver-core-next since that's what I had synced up
> and had a working baseline. I'll rebase it on the regulator tree when I
> go from RFC -> PATCH.
>
> Thanks,
> Saravana
>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Marek Szyprowski <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: Tony Lindgren <[email protected]>
> Cc: Doug Anderson <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Luca Weiss <[email protected]>

Christian, I was meaning to add you but I forgot. I'll add you to
future versions.

-Saravana

>
> Saravana Kannan (4):
> regulator: core: Add regulator devices to bus instead of class
> regulator: core: Add sysfs class backward compatibility
> regulator: core: Probe regulator devices
> regulator: core: Move regulator supply resolving to the probe function
>
> drivers/regulator/core.c | 102 +++++++++++++++++++------------
> drivers/regulator/internal.h | 2 +-
> drivers/regulator/of_regulator.c | 2 +-
> 3 files changed, 64 insertions(+), 42 deletions(-)
>
> --
> 2.39.2.637.g21b0678d19-goog
>

2023-02-18 08:54:21

by Greg Kroah-Hartman

Subject: Re: [RFC v1 0/4] Simplify regulator supply resolution code by offloading to driver core

On Sat, Feb 18, 2023 at 12:32:47AM -0800, Saravana Kannan wrote:
> Hi Mark/Liam,
>
> This series is just an RFC to see if you agree with where this is going.
> Please point out bugs, but don't bother with a proper code review.
>
> The high level idea is to not reimplement what driver core can already
> handle for us and use it to do some of the work. Instead of trying to
> resolve supplies from all different code paths and bits and pieces of
> the tree, we just build it from the root to the leaves by using deferred
> probing to sequence things in the right order.
>
> The last patch is the main one. Rest of them are just setting up for it.
>
> I believe there's room for further simplification but this is what I
> could whip up as a quick first draft that shows the high level idea.
> I'll probably need some help with getting a better understanding of why
> things are done in a specific order in regulator_register() before I
> could attempt simplifying things further.
>
> Ideally, regulator_register() would just have DT parsing, init data
> struct sanity checks and adding the regulator device and then we move
> everything else to into the probe function that's guaranteed to run only
> after the supply has been resolved/ready to resolve.
>
> fw_devlink/device links should further optimize the flow and also allow
> us to simplify some of the guarantees and address some of the existing
> FIXMEs. But this patch series is NOT dependent on fw_devlink or device
> links.
>
> Any thoughts on where this is going?
>
> I've tested this on one hardware I have and it works and nothing is
> broken. But the regulator tree in my hardware isn't that complicated or
> deep. The regulators are also added mostly in the right order (due to
> existing fw_devlink). So if you agree with the idea, the next step is to
> ask people to give it a test.
>
> Also, it's based on driver-core-next since that's what I had synced up
> and had a working baseline. I'll rebase it on the regulator tree when I
> go from RFC -> PATCH.

At first glance, this looks sane to me, thanks for doing this work!

greg k-h

2023-02-20 09:02:27

by Marek Szyprowski

Subject: Re: [RFC v1 0/4] Simplify regulator supply resolution code by offloading to driver core

Hi Saravana,

On 18.02.2023 09:32, Saravana Kannan wrote:
> Hi Mark/Liam,
>
> This series is just an RFC to see if you agree with where this is going.
> Please point out bugs, but don't bother with a proper code review.
>
> The high level idea is to not reimplement what driver core can already
> handle for us and use it to do some of the work. Instead of trying to
> resolve supplies from all different code paths and bits and pieces of
> the tree, we just build it from the root to the leaves by using deferred
> probing to sequence things in the right order.
>
> The last patch is the main one. Rest of them are just setting up for it.
>
> I believe there's room for further simplification but this is what I
> could whip up as a quick first draft that shows the high level idea.
> I'll probably need some help with getting a better understanding of why
> things are done in a specific order in regulator_register() before I
> could attempt simplifying things further.
>
> Ideally, regulator_register() would just have DT parsing, init data
> struct sanity checks and adding the regulator device and then we move
> everything else to into the probe function that's guaranteed to run only
> after the supply has been resolved/ready to resolve.
>
> fw_devlink/device links should further optimize the flow and also allow
> us to simplify some of the guarantees and address some of the existing
> FIXMEs. But this patch series is NOT dependent on fw_devlink or device
> links.
>
> Any thoughts on where this is going?
>
> I've tested this on one hardware I have and it works and nothing is
> broken. But the regulator tree in my hardware isn't that complicated or
> deep. The regulators are also added mostly in the right order (due to
> existing fw_devlink). So if you agree with the idea, the next step is to
> ask people to give it a test.
>
> Also, it's based on driver-core-next since that's what I had synced up
> and had a working baseline. I'll rebase it on the regulator tree when I
> go from RFC -> PATCH.

I've applied this patchset on top of linux next-20230220 and gave it a
try on my test farm, as it revealed a few issues related to regulator
initialization in the past. It looks that handling of some corner cases
is missing, because this patchset introduced a regression on Samsung
Snow/Peach-Pit/Peach-Pi Chromebooks, as well as Hardkernel's Odroid-M1
board. It looks that the issue is common - PHY devices don't probe
properly. This is an output from Odroid-M1 board
(arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts):

# cat /sys/kernel/debug/devices_deferred 2>/dev/null
fd8c0000.usb    platform: wait for supplier host-port
fe830000.phy
fe8a0000.usb2phy        rockchip-usb2phy: failed to create phy
fe8b0000.usb2phy        rockchip-usb2phy: failed to create phy
3c0800000.pcie  rockchip-dw-pcie: failed to get vpcie3v3 regulator
fcc00000.usb    platform: wait for supplier otg-port
fd000000.usb    platform: wait for supplier host-port
fd800000.usb    platform: wait for supplier otg-port
fd840000.usb    platform: wait for supplier otg-port
fd880000.usb    platform: wait for supplier host-port
fe820000.phy

If you need any additional tests on the mentioned boards, let me know.


> Thanks,
> Saravana
>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Marek Szyprowski <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: Tony Lindgren <[email protected]>
> Cc: Doug Anderson <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Luca Weiss <[email protected]>
>
> Saravana Kannan (4):
> regulator: core: Add regulator devices to bus instead of class
> regulator: core: Add sysfs class backward compatibility
> regulator: core: Probe regulator devices
> regulator: core: Move regulator supply resolving to the probe function
>
> drivers/regulator/core.c | 102 +++++++++++++++++++------------
> drivers/regulator/internal.h | 2 +-
> drivers/regulator/of_regulator.c | 2 +-
> 3 files changed, 64 insertions(+), 42 deletions(-)
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

2023-02-21 22:38:15

by Saravana Kannan

Subject: Re: [RFC v1 0/4] Simplify regulator supply resolution code by offloading to driver core

On Mon, Feb 20, 2023 at 1:02 AM Marek Szyprowski
<[email protected]> wrote:
>
> Hi Saravana,
>
> On 18.02.2023 09:32, Saravana Kannan wrote:
> > Hi Mark/Liam,
> >
> > This series is just an RFC to see if you agree with where this is going.
> > Please point out bugs, but don't bother with a proper code review.
> >
> > The high level idea is to not reimplement what driver core can already
> > handle for us and use it to do some of the work. Instead of trying to
> > resolve supplies from all different code paths and bits and pieces of
> > the tree, we just build it from the root to the leaves by using deferred
> > probing to sequence things in the right order.
> >
> > The last patch is the main one. Rest of them are just setting up for it.
> >
> > I believe there's room for further simplification but this is what I
> > could whip up as a quick first draft that shows the high level idea.
> > I'll probably need some help with getting a better understanding of why
> > things are done in a specific order in regulator_register() before I
> > could attempt simplifying things further.
> >
> > Ideally, regulator_register() would just have DT parsing, init data
> > struct sanity checks and adding the regulator device and then we move
> > everything else to into the probe function that's guaranteed to run only
> > after the supply has been resolved/ready to resolve.
> >
> > fw_devlink/device links should further optimize the flow and also allow
> > us to simplify some of the guarantees and address some of the existing
> > FIXMEs. But this patch series is NOT dependent on fw_devlink or device
> > links.
> >
> > Any thoughts on where this is going?
> >
> > I've tested this on one hardware I have and it works and nothing is
> > broken. But the regulator tree in my hardware isn't that complicated or
> > deep. The regulators are also added mostly in the right order (due to
> > existing fw_devlink). So if you agree with the idea, the next step is to
> > ask people to give it a test.
> >
> > Also, it's based on driver-core-next since that's what I had synced up
> > and had a working baseline. I'll rebase it on the regulator tree when I
> > go from RFC -> PATCH.
>
> I've applied this patchset on top of linux next-20230220 and gave it a
> try on my test farm, as it revealed a few issues related to regulator
> initialization in the past. It looks that handling of some corner cases
> is missing, because this patchset introduced a regression on Samsung
> Snow/Peach-Pit/Peach-Pi Chromebooks, as well as Hardkernel's Odroid-M1
> board. It looks that the issue is common - PHY devices don't probe
> properly. This is an output from Odroid-M1 board
> (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts):
>
> # cat /sys/kernel/debug/devices_deferred 2>/dev/null
> fd8c0000.usb platform: wait for supplier host-port
> fe830000.phy
> fe8a0000.usb2phy rockchip-usb2phy: failed to create phy
> fe8b0000.usb2phy rockchip-usb2phy: failed to create phy
> 3c0800000.pcie rockchip-dw-pcie: failed to get vpcie3v3 regulator
> fcc00000.usb platform: wait for supplier otg-port
> fd000000.usb platform: wait for supplier host-port
> fd800000.usb platform: wait for supplier otg-port
> fd840000.usb platform: wait for supplier otg-port
> fd880000.usb platform: wait for supplier host-port
> fe820000.phy
>
> If you need any additional tests on the mentioned boards, let me know.

Thanks for testing it Marek! I don't want people to spend more time
testing this before I hear Mark/Liam's thoughts. So, let's hold off
for now.

I took a peek at the dts and the logs above. If you go into
/sys/bus/regulator/devices/, I'd expect all of them to have probed
(they'll have a "driver" symlink in their folder). Or at least the
regulator tree used by the phys.

My first guess is that deferred probe handling might be broken
somewhere in the USB/phy framework where they aren't able to handle
regulator_get() returning -EPROBE_DEFER. Looks like my patch is
delaying some reglator_get() from passing. I'll look closer into
avoiding this after Mark/Liam approve the general idea behind this
patch.

-Saravana

>
>
> > Thanks,
> > Saravana
> >
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Geert Uytterhoeven <[email protected]>
> > Cc: Marek Szyprowski <[email protected]>
> > Cc: Bjorn Andersson <[email protected]>
> > Cc: Sudeep Holla <[email protected]>
> > Cc: Tony Lindgren <[email protected]>
> > Cc: Doug Anderson <[email protected]>
> > Cc: Guenter Roeck <[email protected]>
> > Cc: Luca Weiss <[email protected]>
> >
> > Saravana Kannan (4):
> > regulator: core: Add regulator devices to bus instead of class
> > regulator: core: Add sysfs class backward compatibility
> > regulator: core: Probe regulator devices
> > regulator: core: Move regulator supply resolving to the probe function
> >
> > drivers/regulator/core.c | 102 +++++++++++++++++++------------
> > drivers/regulator/internal.h | 2 +-
> > drivers/regulator/of_regulator.c | 2 +-
> > 3 files changed, 64 insertions(+), 42 deletions(-)
> >
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

2023-02-21 22:53:10

by Mark Brown

Subject: Re: [RFC v1 0/4] Simplify regulator supply resolution code by offloading to driver core

On Tue, Feb 21, 2023 at 02:36:52PM -0800, Saravana Kannan wrote:

> Thanks for testing it Marek! I don't want people to spend more time
> testing this before I hear Mark/Liam's thoughts. So, let's hold off
> for now.

My main thought right now is that I'm not going to think about it
too hard if it doesn't work correctly...

Attachments:

2023-02-22 03:14:22

by Saravana Kannan

Subject: Re: [RFC v1 0/4] Simplify regulator supply resolution code by offloading to driver core

On Tue, Feb 21, 2023 at 2:52 PM Mark Brown <[email protected]> wrote:
>
> On Tue, Feb 21, 2023 at 02:36:52PM -0800, Saravana Kannan wrote:
>
> > Thanks for testing it Marek! I don't want people to spend more time
> > testing this before I hear Mark/Liam's thoughts. So, let's hold off
> > for now.
>
> My main thought right now is that I'm not going to think about it
> too hard if it doesn't work correctly...

:( I'm not asking for a thorough code review. Just if you are okay
with the idea/approach of pushing the ordering logic to driver core to
avoid reimplementing what's already available and avoiding some races
in the regulator code (stuff like, checking if some other thread
resolved a supply while you were working on it). The patch at least
works on my device and works for most regulators in Marek's devices.
So, it's not a complete broken mess :)

On a separate note, I have some questions about setting machine
constraints during regulator_register(). Why do we even try to set
machine constraints before a regulator's supply is resolved? None of
the consumers can make any requests anyway. So what else is going to
need those constraints? Wouldn't the regulator just be in whatever
state the bootloader left it at?

The current logic is something like:

1. Try to resolve supply if it's always on or a boot on regulator.
2. Set machine constraints -- this might fail for multiple reasons.
One of them being unresolved supply.
3. If it failed due to unresolved supply, but it wasn't resolved in step 1.
3. a. Try to resolve supply,
3. b. If 3.a. didn't fail, try to set machine constraints.
3. c. If 3.b failed, fail registration.

Why isn't this just:
1. Try to resolve supply (for all regulators).
2. If we are able to resolve supply set machine constraints.
3. If we weren't able to resolve supply, set machine constraints when
we resolve supply in the future?

Or if you need to set machine constraints without waiting for supply,
then why not at least:

1. Try to resolve supply (for all regulators).
2. Set machine constraints.
3. When we resolve supply in the future, do whatever remaining bits
that you need to do.

Thanks,
Saravana

2023-02-22 07:15:42

by Marek Szyprowski

Subject: Re: [RFC v1 0/4] Simplify regulator supply resolution code by offloading to driver core

On 21.02.2023 23:36, Saravana Kannan wrote:
> On Mon, Feb 20, 2023 at 1:02 AM Marek Szyprowski
> <[email protected]> wrote:
>> On 18.02.2023 09:32, Saravana Kannan wrote:
>>> This series is just an RFC to see if you agree with where this is going.
>>> Please point out bugs, but don't bother with a proper code review.
>>>
>>> The high level idea is to not reimplement what driver core can already
>>> handle for us and use it to do some of the work. Instead of trying to
>>> resolve supplies from all different code paths and bits and pieces of
>>> the tree, we just build it from the root to the leaves by using deferred
>>> probing to sequence things in the right order.
>>>
>>> The last patch is the main one. Rest of them are just setting up for it.
>>>
>>> I believe there's room for further simplification but this is what I
>>> could whip up as a quick first draft that shows the high level idea.
>>> I'll probably need some help with getting a better understanding of why
>>> things are done in a specific order in regulator_register() before I
>>> could attempt simplifying things further.
>>>
>>> Ideally, regulator_register() would just have DT parsing, init data
>>> struct sanity checks and adding the regulator device and then we move
>>> everything else to into the probe function that's guaranteed to run only
>>> after the supply has been resolved/ready to resolve.
>>>
>>> fw_devlink/device links should further optimize the flow and also allow
>>> us to simplify some of the guarantees and address some of the existing
>>> FIXMEs. But this patch series is NOT dependent on fw_devlink or device
>>> links.
>>>
>>> Any thoughts on where this is going?
>>>
>>> I've tested this on one hardware I have and it works and nothing is
>>> broken. But the regulator tree in my hardware isn't that complicated or
>>> deep. The regulators are also added mostly in the right order (due to
>>> existing fw_devlink). So if you agree with the idea, the next step is to
>>> ask people to give it a test.
>>>
>>> Also, it's based on driver-core-next since that's what I had synced up
>>> and had a working baseline. I'll rebase it on the regulator tree when I
>>> go from RFC -> PATCH.
>> I've applied this patchset on top of linux next-20230220 and gave it a
>> try on my test farm, as it revealed a few issues related to regulator
>> initialization in the past. It looks that handling of some corner cases
>> is missing, because this patchset introduced a regression on Samsung
>> Snow/Peach-Pit/Peach-Pi Chromebooks, as well as Hardkernel's Odroid-M1
>> board. It looks that the issue is common - PHY devices don't probe
>> properly. This is an output from Odroid-M1 board
>> (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts):
>>
>> # cat /sys/kernel/debug/devices_deferred 2>/dev/null
>> fd8c0000.usb platform: wait for supplier host-port
>> fe830000.phy
>> fe8a0000.usb2phy rockchip-usb2phy: failed to create phy
>> fe8b0000.usb2phy rockchip-usb2phy: failed to create phy
>> 3c0800000.pcie rockchip-dw-pcie: failed to get vpcie3v3 regulator
>> fcc00000.usb platform: wait for supplier otg-port
>> fd000000.usb platform: wait for supplier host-port
>> fd800000.usb platform: wait for supplier otg-port
>> fd840000.usb platform: wait for supplier otg-port
>> fd880000.usb platform: wait for supplier host-port
>> fe820000.phy
>>
>> If you need any additional tests on the mentioned boards, let me know.
> Thanks for testing it Marek! I don't want people to spend more time
> testing this before I hear Mark/Liam's thoughts. So, let's hold off
> for now.
>
> I took a peek at the dts and the logs above. If you go into
> /sys/bus/regulator/devices/, I'd expect all of them to have probed
> (they'll have a "driver" symlink in their folder). Or at least the
> regulator tree used by the phys.

Nope, something is wrong there with those PHY regulators:

# cd /sys/bus/regulator/devices

# ls -l regulator.4
lrwxrwxrwx 1 root root 0 Feb 14  2019 regulator.4 ->
../../../devices/platform/vcc5v0-usb-host-regulator/regulator.4

# ls -l regulator.4/driver
ls: cannot access 'regulator.4/driver': No such file or directory

# ls -l regulator.4/..
total 0
lrwxrwxrwx 1 root root    0 Feb 22 07:06 consumer:platform:fe820000.phy
->
../../virtual/devlink/platform:vcc5v0-usb-host-regulator--platform:fe820000.phy
lrwxrwxrwx 1 root root    0 Feb 22 07:06
consumer:platform:fe8a0000.usb2phy ->
../../virtual/devlink/platform:vcc5v0-usb-host-regulator--platform:fe8a0000.usb2phy
lrwxrwxrwx 1 root root    0 Feb 22 07:06
consumer:platform:fe8b0000.usb2phy ->
../../virtual/devlink/platform:vcc5v0-usb-host-regulator--platform:fe8b0000.usb2phy
lrwxrwxrwx 1 root root    0 Feb 22 07:06 driver ->
../../../bus/platform/drivers/reg-fixed-voltage
-rw-r--r-- 1 root root 4096 Feb 22 07:06 driver_override
-r--r--r-- 1 root root 4096 Feb 22 07:06 modalias
lrwxrwxrwx 1 root root    0 Feb 22 07:06 of_node ->
../../../firmware/devicetree/base/vcc5v0-usb-host-regulator
drwxr-xr-x 2 root root    0 Feb 22 07:06 power
drwxr-xr-x 3 root root    0 Feb 14  2019 regulator.4
lrwxrwxrwx 1 root root    0 Feb 14  2019 subsystem -> ../../../bus/platform
lrwxrwxrwx 1 root root    0 Feb 22 07:06 supplier:platform:fdd60000.gpio
->
../../virtual/devlink/platform:fdd60000.gpio--platform:vcc5v0-usb-host-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:06 supplier:platform:pinctrl ->
../../virtual/devlink/platform:pinctrl--platform:vcc5v0-usb-host-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:06
supplier:platform:vcc5v0-sys-regulator ->
../../virtual/devlink/platform:vcc5v0-sys-regulator--platform:vcc5v0-usb-host-regulator
-rw-r--r-- 1 root root 4096 Feb 14  2019 uevent

# ls -l regulator.5
lrwxrwxrwx 1 root root 0 Feb 14  2019 regulator.5 ->
../../../devices/platform/vcc5v0-usb-otg-regulator/regulator.5

# ls -l regulator.5/driver
ls: cannot access 'regulator.5/driver': No such file or directory

# ls -l regulator.5/..
total 0
lrwxrwxrwx 1 root root    0 Feb 22 07:09 consumer:platform:fe830000.phy
->
../../virtual/devlink/platform:vcc5v0-usb-otg-regulator--platform:fe830000.phy
lrwxrwxrwx 1 root root    0 Feb 22 07:09
consumer:platform:fe8a0000.usb2phy ->
../../virtual/devlink/platform:vcc5v0-usb-otg-regulator--platform:fe8a0000.usb2phy
lrwxrwxrwx 1 root root    0 Feb 22 07:07 driver ->
../../../bus/platform/drivers/reg-fixed-voltage
-rw-r--r-- 1 root root 4096 Feb 22 07:09 driver_override
-r--r--r-- 1 root root 4096 Feb 22 07:09 modalias
lrwxrwxrwx 1 root root    0 Feb 22 07:09 of_node ->
../../../firmware/devicetree/base/vcc5v0-usb-otg-regulator
drwxr-xr-x 2 root root    0 Feb 22 07:09 power
drwxr-xr-x 3 root root    0 Feb 14  2019 regulator.5
lrwxrwxrwx 1 root root    0 Feb 14  2019 subsystem -> ../../../bus/platform
lrwxrwxrwx 1 root root    0 Feb 22 07:09 supplier:platform:fdd60000.gpio
->
../../virtual/devlink/platform:fdd60000.gpio--platform:vcc5v0-usb-otg-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:09 supplier:platform:pinctrl ->
../../virtual/devlink/platform:pinctrl--platform:vcc5v0-usb-otg-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:09
supplier:platform:vcc5v0-sys-regulator ->
../../virtual/devlink/platform:vcc5v0-sys-regulator--platform:vcc5v0-usb-otg-regulator
-rw-r--r-- 1 root root 4096 Feb 14  2019 uevent

# ls -l regulator.23
lrwxrwxrwx 1 root root 0 Feb 14  2019 regulator.23 ->
../../../devices/platform/vcc3v3-pcie-regulator/regulator.23

# ls -l regulator.23/driver
ls: cannot access 'regulator.23/driver': No such file or directory

# ls -l regulator.23/..
total 0
lrwxrwxrwx 1 root root    0 Feb 22 07:10
consumer:platform:3c0800000.pcie ->
../../virtual/devlink/platform:vcc3v3-pcie-regulator--platform:3c0800000.pcie
lrwxrwxrwx 1 root root    0 Feb 22 07:07 driver ->
../../../bus/platform/drivers/reg-fixed-voltage
-rw-r--r-- 1 root root 4096 Feb 22 07:10 driver_override
-r--r--r-- 1 root root 4096 Feb 22 07:10 modalias
lrwxrwxrwx 1 root root    0 Feb 22 07:10 of_node ->
../../../firmware/devicetree/base/vcc3v3-pcie-regulator
drwxr-xr-x 2 root root    0 Feb 22 07:10 power
drwxr-xr-x 3 root root    0 Feb 14  2019 regulator.23
lrwxrwxrwx 1 root root    0 Feb 14  2019 subsystem -> ../../../bus/platform
lrwxrwxrwx 1 root root    0 Feb 22 07:10 supplier:platform:fe770000.gpio
->
../../virtual/devlink/platform:fe770000.gpio--platform:vcc3v3-pcie-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:10 supplier:platform:pinctrl ->
../../virtual/devlink/platform:pinctrl--platform:vcc3v3-pcie-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:10
supplier:platform:vcc3v3-sys-regulator ->
../../virtual/devlink/platform:vcc3v3-sys-regulator--platform:vcc3v3-pcie-regulator
-rw-r--r-- 1 root root 4096 Feb 14  2019 uevent


However other fixed regulator devices have been probed properly:

# ls -l regulator.3/driver
lrwxrwxrwx 1 root root 0 Feb 22 07:10 regulator.3/driver ->
../../../../bus/regulator/drivers/regulator_drv
# ls -l regulator.3/..
total 0
lrwxrwxrwx 1 root root    0 Feb 22 07:10
consumer:platform:vcc5v0-usb-host-regulator ->
../../virtual/devlink/platform:vcc5v0-sys-regulator--platform:vcc5v0-usb-host-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:10
consumer:platform:vcc5v0-usb-otg-regulator ->
../../virtual/devlink/platform:vcc5v0-sys-regulator--platform:vcc5v0-usb-otg-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:10 driver ->
../../../bus/platform/drivers/reg-fixed-voltage
-rw-r--r-- 1 root root 4096 Feb 22 07:10 driver_override
-r--r--r-- 1 root root 4096 Feb 22 07:10 modalias
lrwxrwxrwx 1 root root    0 Feb 22 07:10 of_node ->
../../../firmware/devicetree/base/vcc5v0-sys-regulator
drwxr-xr-x 2 root root    0 Feb 22 07:10 power
drwxr-xr-x 3 root root    0 Feb 14  2019 regulator.3
lrwxrwxrwx 1 root root    0 Feb 14  2019 subsystem -> ../../../bus/platform
lrwxrwxrwx 1 root root    0 Feb 22 07:10
supplier:platform:dc-12v-regulator ->
../../virtual/devlink/platform:dc-12v-regulator--platform:vcc5v0-sys-regulator
-rw-r--r-- 1 root root 4096 Feb 14  2019 uevent

# ls -l regulator.2/driver
lrwxrwxrwx 1 root root 0 Feb 22 07:13 regulator.2/driver ->
../../../../bus/regulator/drivers/regulator_drv
# ls -l regulator.2/..
total 0
lrwxrwxrwx 1 root root    0 Feb 22 07:13 consumer:i2c:3-001c ->
../../virtual/devlink/platform:vcc3v3-sys-regulator--i2c:3-001c
lrwxrwxrwx 1 root root    0 Feb 22 07:13 consumer:i2c:3-0020 ->
../../virtual/devlink/platform:vcc3v3-sys-regulator--i2c:3-0020
lrwxrwxrwx 1 root root    0 Feb 22 07:13
consumer:platform:fe2a0000.ethernet ->
../../virtual/devlink/platform:vcc3v3-sys-regulator--platform:fe2a0000.ethernet
lrwxrwxrwx 1 root root    0 Feb 22 07:13
consumer:platform:vcc3v3-pcie-regulator ->
../../virtual/devlink/platform:vcc3v3-sys-regulator--platform:vcc3v3-pcie-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:13 driver ->
../../../bus/platform/drivers/reg-fixed-voltage
-rw-r--r-- 1 root root 4096 Feb 22 07:13 driver_override
-r--r--r-- 1 root root 4096 Feb 22 07:13 modalias
lrwxrwxrwx 1 root root    0 Feb 22 07:13 of_node ->
../../../firmware/devicetree/base/vcc3v3-sys-regulator
drwxr-xr-x 2 root root    0 Feb 22 07:13 power
drwxr-xr-x 3 root root    0 Feb 14  2019 regulator.2
lrwxrwxrwx 1 root root    0 Feb 14  2019 subsystem -> ../../../bus/platform
lrwxrwxrwx 1 root root    0 Feb 22 07:13
supplier:platform:dc-12v-regulator ->
../../virtual/devlink/platform:dc-12v-regulator--platform:vcc3v3-sys-regulator
-rw-r--r-- 1 root root 4096 Feb 14  2019 uevent

# ls -l regulator.1/driver
lrwxrwxrwx 1 root root 0 Feb 22 07:14 regulator.1/driver ->
../../../../bus/regulator/drivers/regulator_drv
# ls -l regulator.1/..
total 0
lrwxrwxrwx 1 root root    0 Feb 22 07:14
consumer:platform:vcc3v3-sys-regulator ->
../../virtual/devlink/platform:dc-12v-regulator--platform:vcc3v3-sys-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:14
consumer:platform:vcc5v0-sys-regulator ->
../../virtual/devlink/platform:dc-12v-regulator--platform:vcc5v0-sys-regulator
lrwxrwxrwx 1 root root    0 Feb 22 07:14 driver ->
../../../bus/platform/drivers/reg-fixed-voltage
-rw-r--r-- 1 root root 4096 Feb 22 07:14 driver_override
-r--r--r-- 1 root root 4096 Feb 22 07:14 modalias
lrwxrwxrwx 1 root root    0 Feb 22 07:14 of_node ->
../../../firmware/devicetree/base/dc-12v-regulator
drwxr-xr-x 2 root root    0 Feb 22 07:14 power
drwxr-xr-x 3 root root    0 Feb 14  2019 regulator.1
lrwxrwxrwx 1 root root    0 Feb 14  2019 subsystem -> ../../../bus/platform
-rw-r--r-- 1 root root 4096 Feb 14  2019 uevent


I hope the above logs will help identifying the issue.


Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

2023-02-22 14:54:31

by Mark Brown

Subject: Re: [RFC v1 0/4] Simplify regulator supply resolution code by offloading to driver core

On Tue, Feb 21, 2023 at 07:13:39PM -0800, Saravana Kannan wrote:
> On Tue, Feb 21, 2023 at 2:52 PM Mark Brown <[email protected]> wrote:

> > My main thought right now is that I'm not going to think about it
> > too hard if it doesn't work correctly...

> :( I'm not asking for a thorough code review. Just if you are okay
> with the idea/approach of pushing the ordering logic to driver core to
> avoid reimplementing what's already available and avoiding some races
> in the regulator code (stuff like, checking if some other thread
> resolved a supply while you were working on it). The patch at least
> works on my device and works for most regulators in Marek's devices.
> So, it's not a complete broken mess :)

Well, there's the fact that it's clearly not a bus (not even a virtual
one like virtio) which will doubtless cause problems down the line.
Otherwise the fact that you're so concerned is making me think there's
landmines in here that need a really detailed look.

> On a separate note, I have some questions about setting machine
> constraints during regulator_register(). Why do we even try to set
> machine constraints before a regulator's supply is resolved? None of
> the consumers can make any requests anyway. So what else is going to
> need those constraints? Wouldn't the regulator just be in whatever
> state the bootloader left it at?

If the state we inherit is somehow bad then we want to try to correct
problems as fast as possible, to the extent we can. The firmware may
not be making any effort to configure the hardware, we can end up with
hard coded defaults from the silicon which might need some fixup so we
want to minimise the amount of time we spend operating out of spec.

> The current logic is something like:

> 1. Try to resolve supply if it's always on or a boot on regulator.
> 2. Set machine constraints -- this might fail for multiple reasons.
> One of them being unresolved supply.
> 3. If it failed due to unresolved supply, but it wasn't resolved in step 1.
> 3. a. Try to resolve supply,
> 3. b. If 3.a. didn't fail, try to set machine constraints.
> 3. c. If 3.b failed, fail registration.

IIRC the goal is to only configure the supply if we really need to so it
doesn't get in the way of anything else.

> Why isn't this just:
> 1. Try to resolve supply (for all regulators).
> 2. If we are able to resolve supply set machine constraints.

Most constraint setting doesn't need the supply.

> 3. If we weren't able to resolve supply, set machine constraints when
> we resolve supply in the future?

This may never happen.

> Or if you need to set machine constraints without waiting for supply,
> then why not at least:

> 1. Try to resolve supply (for all regulators).
> 2. Set machine constraints.
> 3. When we resolve supply in the future, do whatever remaining bits
> that you need to do.

There's also the coupling to deal with. It's mainly that we don't even
bother trying to resolve the supply until we need it to cut down on
noise from reporting transient errors that'll sort themsleves out later.

Attachments:

2023-02-22 17:47:50

by Mark Brown

Subject: Re: [RFC v1 2/4] regulator: core: Add sysfs class backward compatibility

On Sat, Feb 18, 2023 at 12:32:49AM -0800, Saravana Kannan wrote:
> A regulator device's sysfs directory used to be created under
> /sys/class/regulator when it is added to a class. Since the device is
> now moved to be under a bus, add symlinks from /sys/class/regulator to
> the real device sysfs directory.

Shouldn't this be squashed into the previous patch given that it creates
a bisection issue?

Attachments:

2023-02-22 17:50:57

by Mark Brown

Subject: Re: [RFC v1 3/4] regulator: core: Probe regulator devices

On Sat, Feb 18, 2023 at 12:32:50AM -0800, Saravana Kannan wrote:
> Since devices added to a bus can be probed, add a stub probe function
> for regulator devices.

Why?

> +static int regulator_drv_probe(struct device *dev)
> +{
> + return 0;
> +}

This is just an empty function - why do we need it? Looking at
call_driver_probe() it doesn't appear to be required.

Attachments:

2023-02-22 22:51:26

by Mark Brown

Subject: Re: [RFC v1 4/4] regulator: core: Move regulator supply resolving to the probe function

On Sat, Feb 18, 2023 at 12:32:51AM -0800, Saravana Kannan wrote:
> We can simplify the regulator's supply resolving code if we resolve the
> supply in the regulator's probe function. This allows us to:
>
> - Consolidate the supply resolution code to one place.
> - Avoid the need for recursion by allow driver core to take care of
> handling dependencies.
> - Avoid races and simplify locking by reusing the guarantees provided by
> driver core.
> - Avoid last minute/lazy resolving during regulator_get().
> - Simplify error handling because we can assume the supply has been
> resolved once a regulator is probed.
> - Allow driver core to use device links/fw_devlink, where available, to
> resolve the regulator supplies in the optimal order.

It would be good if you had noted the issues with moving the
constraint initialistion that you mentioned elsewhere in the
thread here - from the review I've done thus far that is the
biggest issue this creates - it means that we will not do any
needed hardware configuration until all the parents have sorted
themselves out (which may never even happen), increasing the
amount of time that the system is running out of spec.

> + if (r && r->dev.links.status == DL_DEV_DRIVER_BOUND)
> return r;
>
> r = regulator_lookup_by_name(supply);
> - if (r)
> + if (r && r->dev.links.status == DL_DEV_DRIVER_BOUND)
> return r;
>
> return ERR_PTR(-ENODEV);

This will return -ENODEV in the case where we have found a device
but it didn't probe yet. It's probably more appropriate to
return -EPROBE_DEFER like we do when we know about a DT link.
This is also adding a leak of the get_device() from the looks of
it.

Shouldn't this be using device_is_bound() rather than peering at
the links? Or alternatively if device_is_bound() does something
different to peering at the links isn't that really confusing?

> @@ -2050,13 +2050,6 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
> }
> }
>
> - /* Recursively resolve the supply of the supply */
> - ret = regulator_resolve_supply(r);
> - if (ret < 0) {
> - put_device(&r->dev);
> - goto out;
> - }
> -
> /*
> * Recheck rdev->supply with rdev->mutex lock held to avoid a race
> * between rdev->supply null check and setting rdev->supply in

Your commit message says this should avoid the need to worry
about locking so much so do we still need to recheck things? We
still seem to be doing all the same things we were doing before.

Attachments: