This patch series improves fw_devlink in the following ways:
1. It no longer cares about a fwnode having a "compatible" property. It
figures this our more dynamically. The only expectation is that
fwnode that are converted to devices actually get probed by a driver
for the dependencies to be enforced correctly.
2. Finer grained dependency tracking. fw_devlink will now create device
links from the consumer to the actual resource's device (if it has one,
Eg: gpio_device) instead of the parent supplier device. This improves
things like async suspend/resume ordering, potentially remove the need
for frameworks to create device links, more parallelized async probing,
and better sync_state() tracking.
3. Handle hardware/software quirks where a child firmware node gets
populated as a device before its parent firmware node AND actually
supplies a non-optional resource to the parent firmware node's
device.
4. Way more robust at cycle handling (see patch for the insane cases).
5. Stops depending on OF_POPULATED to figure out some corner cases.
6. Simplifies the work that needs to be done by the firmware specific
code.
This took way too long to get done due to typo bugs I had in my rewrite or
corner cases I had to find and handle. But it's fairly well tested at this
point and I expect this to work properly.
Abel & Doug,
This should fix your cyclic dependency issues with your display. Can you
give it a shot please?
Alexander,
This should fix your issue where the power domain device not having a
compatible property. Can you give it a shot please?
Tony,
This should handle the odd case of the child being the supplier of the
parent. Can you please give this a shot? I want to make sure the cycle
detection code handles this properly and treats it like it's NOT a cycle.
Geert,
Can you test the renesas stuff I changed please? They should continue
working like before. Any other sanity test on other hardware would be
great too.
Sudeep,
I don't think there are any unfixed issues you had reported in my other
patches that this series might fix, but it'll be nice if you could give
this a sanity test.
Guenter,
I don't think this will fix the issue you reported in the amba patch, but
it's worth a shot because it improves a bunch of corner case handling. So
it might be better at handling whatever corner cases you might have in the
qemu platforms.
Thanks,
Saravana
Cc: Abel Vesa <[email protected]>
Cc: Alexander Stein <[email protected]>
Cc: Tony Lindgren <[email protected]>
Cc: Sudeep Holla <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: John Stultz <[email protected]>
Cc: Doug Anderson <[email protected]>
Cc: Guenter Roeck <[email protected]>
Saravana Kannan (9):
driver core: fw_devlink: Don't purge child fwnode's consumer links
driver core: fw_devlink: Improve check for fwnode with no
device/driver
soc: renesas: Move away from using OF_POPULATED for fw_devlink
gpiolib: Clear the gpio_device's fwnode initialized flag before adding
driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links
driver core: fw_devlink: Allow marking a fwnode link as being part of
a cycle
driver core: fw_devlink: Consolidate device link flag computation
driver core: fw_devlink: Make cycle detection more robust
of: property: Simplify of_link_to_phandle()
drivers/base/core.c | 437 +++++++++++++++++++++-----------
drivers/gpio/gpiolib.c | 6 +
drivers/of/property.c | 84 +-----
drivers/soc/renesas/rcar-sysc.c | 2 +-
include/linux/device.h | 1 +
include/linux/fwnode.h | 12 +-
6 files changed, 323 insertions(+), 219 deletions(-)
--
2.37.1.559.g78731f0fdb-goog
To improve detection and handling of dependency cycles, we need to be
able to mark fwnode links as being part of cycles. fwnode links marked
as being part of a cycle should not block their consumers from probing.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/base/core.c | 41 +++++++++++++++++++++++++++++++++++------
include/linux/fwnode.h | 11 ++++++++++-
2 files changed, 45 insertions(+), 7 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index afa660d14172..0ec2c4d5ffaa 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -125,6 +125,19 @@ static void __fwnode_link_del(struct fwnode_link *link)
kfree(link);
}
+/**
+ * __fwnode_link_cycle - Mark a fwnode link as being part of a cycle.
+ * @link: the fwnode_link to be marked
+ *
+ * The fwnode_link_lock needs to be held when this function is called.
+ */
+static void __fwnode_link_cycle(struct fwnode_link *link)
+{
+ pr_debug("%pfwf: Relaxing link with %pfwf\n",
+ link->consumer, link->supplier);
+ link->flags |= FWLINK_FLAG_CYCLE;
+}
+
/**
* fwnode_links_purge_suppliers - Delete all supplier links of fwnode_handle.
* @fwnode: fwnode whose supplier links need to be deleted
@@ -1040,6 +1053,23 @@ static bool dev_is_best_effort(struct device *dev)
(dev->fwnode && (dev->fwnode->flags & FWNODE_FLAG_BEST_EFFORT));
}
+static struct fwnode_handle *fwnode_links_check_suppliers(
+ struct fwnode_handle *fwnode)
+{
+ struct fwnode_link *link;
+
+ if (!fwnode || fw_devlink_is_permissive())
+ return NULL;
+
+ list_for_each_entry(link, &fwnode->suppliers, c_hook) {
+ if (link->flags & FWLINK_FLAG_CYCLE)
+ continue;
+ return link->supplier;
+ }
+
+ return NULL;
+}
+
/**
* device_links_check_suppliers - Check presence of supplier drivers.
* @dev: Consumer device.
@@ -1067,11 +1097,8 @@ int device_links_check_suppliers(struct device *dev)
* probe.
*/
mutex_lock(&fwnode_link_lock);
- if (dev->fwnode && !list_empty(&dev->fwnode->suppliers) &&
- !fw_devlink_is_permissive()) {
- sup_fw = list_first_entry(&dev->fwnode->suppliers,
- struct fwnode_link,
- c_hook)->supplier;
+ sup_fw = fwnode_links_check_suppliers(dev->fwnode);
+ if (sup_fw) {
if (!dev_is_best_effort(dev)) {
fwnode_ret = -EPROBE_DEFER;
dev_err_probe(dev, -EPROBE_DEFER,
@@ -1260,7 +1287,9 @@ static ssize_t waiting_for_supplier_show(struct device *dev,
bool val;
device_lock(dev);
- val = !list_empty(&dev->fwnode->suppliers);
+ mutex_lock(&fwnode_link_lock);
+ val = !!fwnode_links_check_suppliers(dev->fwnode);
+ mutex_unlock(&fwnode_link_lock);
device_unlock(dev);
return sysfs_emit(buf, "%u\n", val);
}
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 89b9bdfca925..fdf2ee0285b7 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -18,7 +18,7 @@ struct fwnode_operations;
struct device;
/*
- * fwnode link flags
+ * fwnode flags
*
* LINKS_ADDED: The fwnode has already be parsed to add fwnode links.
* NOT_DEVICE: The fwnode will never be populated as a struct device.
@@ -36,6 +36,7 @@ struct device;
#define FWNODE_FLAG_INITIALIZED BIT(2)
#define FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD BIT(3)
#define FWNODE_FLAG_BEST_EFFORT BIT(4)
+#define FWNODE_FLAG_VISITED BIT(5)
struct fwnode_handle {
struct fwnode_handle *secondary;
@@ -46,11 +47,19 @@ struct fwnode_handle {
u8 flags;
};
+/*
+ * fwnode link flags
+ *
+ * CYCLE: The fwnode link is part of a cycle. Don't defer probe.
+ */
+#define FWLINK_FLAG_CYCLE BIT(0)
+
struct fwnode_link {
struct fwnode_handle *supplier;
struct list_head s_hook;
struct fwnode_handle *consumer;
struct list_head c_hook;
+ u8 flags;
};
/**
--
2.37.1.559.g78731f0fdb-goog
fw_devlink shouldn't defer the probe of a device to wait on a supplier
that'll never have a struct device or will never be probed by a driver.
We currently check if a supplier falls into this category, but don't
check its ancestors. We need to check the ancestors too because if the
ancestor will never probe, then the supplier will never probe either.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/base/core.c | 40 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6f575c2a24ad..8ec2236b1f9c 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1866,6 +1866,35 @@ static int fw_devlink_relax_cycle(struct device *con, void *sup)
return ret;
}
+static bool fwnode_init_without_drv(struct fwnode_handle *fwnode)
+{
+ struct device *dev;
+ bool ret;
+
+ if (!(fwnode->flags & FWNODE_FLAG_INITIALIZED))
+ return false;
+
+ dev = get_dev_from_fwnode(fwnode);
+ ret = !dev || dev->links.status == DL_DEV_NO_DRIVER;
+ put_device(dev);
+
+ return ret;
+}
+
+static bool fwnode_ancestor_init_without_drv(struct fwnode_handle *fwnode)
+{
+ struct fwnode_handle *parent;
+
+ fwnode_for_each_parent_node(fwnode, parent) {
+ if (fwnode_init_without_drv(parent)) {
+ fwnode_handle_put(parent);
+ return true;
+ }
+ }
+
+ return false;
+}
+
/**
* fw_devlink_create_devlink - Create a device link from a consumer to fwnode
* @con: consumer device for the device link
@@ -1943,9 +1972,16 @@ static int fw_devlink_create_devlink(struct device *con,
goto out;
}
- /* Supplier that's already initialized without a struct device. */
- if (sup_handle->flags & FWNODE_FLAG_INITIALIZED)
+ /*
+ * Supplier or supplier's ancestor already initialized without a struct
+ * device or being probed by a driver.
+ */
+ if (fwnode_init_without_drv(sup_handle) ||
+ fwnode_ancestor_init_without_drv(sup_handle)) {
+ dev_dbg(con, "Not linking %pfwP - Might never probe\n",
+ sup_handle);
return -EINVAL;
+ }
/*
* DL_FLAG_SYNC_STATE_ONLY doesn't block probing and supports
--
2.37.1.559.g78731f0fdb-goog
The OF_POPULATED flag was set to let fw_devlink know that the device
tree node will not have a struct device created for it. This information
is used by fw_devlink to avoid deferring the probe of consumers of this
device tree node.
Let's use fwnode_dev_initialized() instead because it achieves the same
effect without using OF specific flags. This allows more generic code to
be written in driver core.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/soc/renesas/rcar-sysc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/renesas/rcar-sysc.c b/drivers/soc/renesas/rcar-sysc.c
index b0a80de34c98..03246ed4a79e 100644
--- a/drivers/soc/renesas/rcar-sysc.c
+++ b/drivers/soc/renesas/rcar-sysc.c
@@ -437,7 +437,7 @@ static int __init rcar_sysc_pd_init(void)
error = of_genpd_add_provider_onecell(np, &domains->onecell_data);
if (!error)
- of_node_set_flag(np, OF_POPULATED);
+ fwnode_dev_initialized(&np->fwnode, true);
out_put:
of_node_put(np);
--
2.37.1.559.g78731f0fdb-goog
Registering an irqdomain sets the flag for the fwnode. But having the
flag set when a device is added is interpreted by fw_devlink to mean the
device has already been initialized and will never probe. This prevents
fw_devlink from creating device links with the gpio_device as a
supplier. So, clear the flag before adding the device.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/gpio/gpiolib.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index cc9c0a12259e..1d57d6f24632 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -522,6 +522,12 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
{
int ret;
+ /*
+ * If fwnode doesn't belong to another device, it's safe to clear its
+ * initialized flag.
+ */
+ if (!gdev->dev.fwnode->dev)
+ fwnode_dev_initialized(gdev->dev.fwnode, false);
ret = gcdev_register(gdev, gpio_devt);
if (ret)
return ret;
--
2.37.1.559.g78731f0fdb-goog
fw_devlink uses DL_FLAG_SYNC_STATE_ONLY device link flag for two
purposes:
1. To allow a parent device to proxy its child device's dependency on a
supplier so that the supplier doesn't get its sync_state() callback
before the child device/consumer can be added and probed. In this
usage scenario, we need to ignore cycles for ensure correctness of
sync_state() callbacks.
2. When there are dependency cycles in firmware, we don't know which of
those dependencies are valid. So, we have to ignore them all wrt
probe ordering while still making sure the sync_state() callbacks
come correctly.
However, when detecting dependency cycles, there can be multiple
dependency cycles between two devices that we need to detect. For
example:
A -> B -> A and A -> C -> B -> A.
To detect multiple cycles correct, we need to be able to differentiate
DL_FLAG_SYNC_STATE_ONLY device links used for (1) vs (2) above.
To allow this differentiation, add a DL_FLAG_CYCLE that can be use to
mark use case (2). We can then use the DL_FLAG_CYCLE to decide which
DL_FLAG_SYNC_STATE_ONLY device links to follow when looking for
dependency cycles.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/base/core.c | 28 ++++++++++++++++++----------
include/linux/device.h | 1 +
2 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 8ec2236b1f9c..afa660d14172 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -321,6 +321,12 @@ static bool device_is_ancestor(struct device *dev, struct device *target)
return false;
}
+static inline bool device_link_flag_is_sync_state_only(u32 flags)
+{
+ return (flags & ~(DL_FLAG_INFERRED | DL_FLAG_CYCLE))
+ == (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED);
+}
+
/**
* device_is_dependent - Check if one device depends on another one
* @dev: Device to check dependencies for.
@@ -347,8 +353,7 @@ int device_is_dependent(struct device *dev, void *target)
return ret;
list_for_each_entry(link, &dev->links.consumers, s_node) {
- if ((link->flags & ~DL_FLAG_INFERRED) ==
- (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED))
+ if (device_link_flag_is_sync_state_only(link->flags))
continue;
if (link->consumer == target)
@@ -421,8 +426,7 @@ static int device_reorder_to_tail(struct device *dev, void *not_used)
device_for_each_child(dev, NULL, device_reorder_to_tail);
list_for_each_entry(link, &dev->links.consumers, s_node) {
- if ((link->flags & ~DL_FLAG_INFERRED) ==
- (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED))
+ if (device_link_flag_is_sync_state_only(link->flags))
continue;
device_reorder_to_tail(link->consumer, NULL);
}
@@ -683,7 +687,8 @@ postcore_initcall(devlink_class_init);
DL_FLAG_AUTOREMOVE_SUPPLIER | \
DL_FLAG_AUTOPROBE_CONSUMER | \
DL_FLAG_SYNC_STATE_ONLY | \
- DL_FLAG_INFERRED)
+ DL_FLAG_INFERRED | \
+ DL_FLAG_CYCLE)
#define DL_ADD_VALID_FLAGS (DL_MANAGED_LINK_FLAGS | DL_FLAG_STATELESS | \
DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE)
@@ -752,8 +757,6 @@ struct device_link *device_link_add(struct device *consumer,
if (!consumer || !supplier || consumer == supplier ||
flags & ~DL_ADD_VALID_FLAGS ||
(flags & DL_FLAG_STATELESS && flags & DL_MANAGED_LINK_FLAGS) ||
- (flags & DL_FLAG_SYNC_STATE_ONLY &&
- (flags & ~DL_FLAG_INFERRED) != DL_FLAG_SYNC_STATE_ONLY) ||
(flags & DL_FLAG_AUTOPROBE_CONSUMER &&
flags & (DL_FLAG_AUTOREMOVE_CONSUMER |
DL_FLAG_AUTOREMOVE_SUPPLIER)))
@@ -769,6 +772,10 @@ struct device_link *device_link_add(struct device *consumer,
if (!(flags & DL_FLAG_STATELESS))
flags |= DL_FLAG_MANAGED;
+ if (flags & DL_FLAG_SYNC_STATE_ONLY &&
+ !device_link_flag_is_sync_state_only(flags))
+ return NULL;
+
device_links_write_lock();
device_pm_lock();
@@ -1728,7 +1735,7 @@ static void fw_devlink_relax_link(struct device_link *link)
if (!(link->flags & DL_FLAG_INFERRED))
return;
- if (link->flags == (DL_FLAG_MANAGED | FW_DEVLINK_FLAGS_PERMISSIVE))
+ if (device_link_flag_is_sync_state_only(link->flags))
return;
pm_runtime_drop_link(link);
@@ -1852,8 +1859,8 @@ static int fw_devlink_relax_cycle(struct device *con, void *sup)
return ret;
list_for_each_entry(link, &con->links.consumers, s_node) {
- if ((link->flags & ~DL_FLAG_INFERRED) ==
- (DL_FLAG_SYNC_STATE_ONLY | DL_FLAG_MANAGED))
+ if (!(link->flags & DL_FLAG_CYCLE) &&
+ device_link_flag_is_sync_state_only(link->flags))
continue;
if (!fw_devlink_relax_cycle(link->consumer, sup))
@@ -1862,6 +1869,7 @@ static int fw_devlink_relax_cycle(struct device *con, void *sup)
ret = 1;
fw_devlink_relax_link(link);
+ link->flags |= DL_FLAG_CYCLE;
}
return ret;
}
diff --git a/include/linux/device.h b/include/linux/device.h
index 424b55df0272..7cf24330d681 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -327,6 +327,7 @@ enum device_link_state {
#define DL_FLAG_MANAGED BIT(6)
#define DL_FLAG_SYNC_STATE_ONLY BIT(7)
#define DL_FLAG_INFERRED BIT(8)
+#define DL_FLAG_CYCLE BIT(9)
/**
* enum dl_dev_state - Device driver presence tracking information.
--
2.37.1.559.g78731f0fdb-goog
Consolidate the code that computes the flags to be used when creating a
device link from a fwnode link.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/base/core.c | 28 +++++++++++++++-------------
include/linux/fwnode.h | 1 -
2 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0ec2c4d5ffaa..296cfb714fe1 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1725,8 +1725,11 @@ static int __init fw_devlink_strict_setup(char *arg)
}
early_param("fw_devlink.strict", fw_devlink_strict_setup);
-u32 fw_devlink_get_flags(void)
+static inline u32 fw_devlink_get_flags(u8 fwlink_flags)
{
+ if (fwlink_flags & FWLINK_FLAG_CYCLE)
+ return FW_DEVLINK_FLAGS_PERMISSIVE | DL_FLAG_CYCLE;
+
return fw_devlink_flags;
}
@@ -1936,7 +1939,7 @@ static bool fwnode_ancestor_init_without_drv(struct fwnode_handle *fwnode)
* fw_devlink_create_devlink - Create a device link from a consumer to fwnode
* @con: consumer device for the device link
* @sup_handle: fwnode handle of supplier
- * @flags: devlink flags
+ * @link: fwnode link that's being converted to a device link
*
* This function will try to create a device link between the consumer device
* @con and the supplier device represented by @sup_handle.
@@ -1953,10 +1956,17 @@ static bool fwnode_ancestor_init_without_drv(struct fwnode_handle *fwnode)
* possible to do that in the future
*/
static int fw_devlink_create_devlink(struct device *con,
- struct fwnode_handle *sup_handle, u32 flags)
+ struct fwnode_handle *sup_handle,
+ struct fwnode_link *link)
{
struct device *sup_dev;
int ret = 0;
+ u32 flags;
+
+ if (con->fwnode == link->consumer)
+ flags = fw_devlink_get_flags(link->flags);
+ else
+ flags = FW_DEVLINK_FLAGS_PERMISSIVE;
/*
* In some cases, a device P might also be a supplier to its child node
@@ -2085,7 +2095,6 @@ static void __fw_devlink_link_to_consumers(struct device *dev)
struct fwnode_link *link, *tmp;
list_for_each_entry_safe(link, tmp, &fwnode->consumers, s_hook) {
- u32 dl_flags = fw_devlink_get_flags();
struct device *con_dev;
bool own_link = true;
int ret;
@@ -2115,14 +2124,13 @@ static void __fw_devlink_link_to_consumers(struct device *dev)
con_dev = NULL;
} else {
own_link = false;
- dl_flags = FW_DEVLINK_FLAGS_PERMISSIVE;
}
}
if (!con_dev)
continue;
- ret = fw_devlink_create_devlink(con_dev, fwnode, dl_flags);
+ ret = fw_devlink_create_devlink(con_dev, fwnode, link);
put_device(con_dev);
if (!own_link || ret == -EAGAIN)
continue;
@@ -2163,19 +2171,13 @@ static void __fw_devlink_link_to_suppliers(struct device *dev,
bool own_link = (dev->fwnode == fwnode);
struct fwnode_link *link, *tmp;
struct fwnode_handle *child = NULL;
- u32 dl_flags;
-
- if (own_link)
- dl_flags = fw_devlink_get_flags();
- else
- dl_flags = FW_DEVLINK_FLAGS_PERMISSIVE;
list_for_each_entry_safe(link, tmp, &fwnode->suppliers, c_hook) {
int ret;
struct device *sup_dev;
struct fwnode_handle *sup = link->supplier;
- ret = fw_devlink_create_devlink(dev, sup, dl_flags);
+ ret = fw_devlink_create_devlink(dev, sup, link);
if (!own_link || ret == -EAGAIN)
continue;
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index fdf2ee0285b7..5700451b300f 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -207,7 +207,6 @@ static inline void fwnode_dev_initialized(struct fwnode_handle *fwnode,
fwnode->flags &= ~FWNODE_FLAG_INITIALIZED;
}
-extern u32 fw_devlink_get_flags(void);
extern bool fw_devlink_is_strict(void);
int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup);
void fwnode_links_purge(struct fwnode_handle *fwnode);
--
2.37.1.559.g78731f0fdb-goog
fw_devlink could only detect a single and simple cycle because it relied
mainly on device link cycle detection code that only checked for cycles
between devices. The expectation was that the firmware wouldn't have
complicated cycles and multiple cycles between devices. That expectation
has been proven to be wrong.
For example, fw_devlink could handle:
+-+ +-+
|A+------> |B+
+-+ +++
^ |
| |
+----------+
But it couldn't handle even something as "simple" as:
+---------------------+
| |
v |
+-+ +-+ +++
|A+------> |B+------> |C|
+-+ +++ +-+
^ |
| |
+----------+
But firmware has even more complicated cycles like:
+---------------------+
| |
v |
+-+ +---+ +++
+--+A+------>| B +-----> |C|<--+
| +-+ ++--+ +++ |
| ^ | ^ | |
| | | | | |
| +---------+ +---------+ |
| |
+------------------------------+
And this is without including parent child dependencies or nodes in the
cycle that are just firmware nodes that'll never have a struct device
created for them.
The proper way to treat these devices it to not force any probe ordering
between them, while still enforce dependencies between node in the
cycles (A, B and C) and their consumers.
So this patch goes all out and just deals with all types of cycles. It
does this by:
1. Following dependencies across device links, parent-child and fwnode
links.
2. When it find cycles, it mark the device links and fwnode links as
such instead of just deleting them or making the indistinguishable
from proxy SYNC_STATE_ONLY device links.
This way, when new nodes get added, we can immediately find and mark any
new cycles whether the new node is a device or firmware node.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/base/core.c | 245 +++++++++++++++++++++++---------------------
1 file changed, 130 insertions(+), 115 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 296cfb714fe1..2f012e826986 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1864,47 +1864,6 @@ static void fw_devlink_unblock_consumers(struct device *dev)
device_links_write_unlock();
}
-/**
- * fw_devlink_relax_cycle - Convert cyclic links to SYNC_STATE_ONLY links
- * @con: Device to check dependencies for.
- * @sup: Device to check against.
- *
- * Check if @sup depends on @con or any device dependent on it (its child or
- * its consumer etc). When such a cyclic dependency is found, convert all
- * device links created solely by fw_devlink into SYNC_STATE_ONLY device links.
- * This is the equivalent of doing fw_devlink=permissive just between the
- * devices in the cycle. We need to do this because, at this point, fw_devlink
- * can't tell which of these dependencies is not a real dependency.
- *
- * Return 1 if a cycle is found. Otherwise, return 0.
- */
-static int fw_devlink_relax_cycle(struct device *con, void *sup)
-{
- struct device_link *link;
- int ret;
-
- if (con == sup)
- return 1;
-
- ret = device_for_each_child(con, sup, fw_devlink_relax_cycle);
- if (ret)
- return ret;
-
- list_for_each_entry(link, &con->links.consumers, s_node) {
- if (!(link->flags & DL_FLAG_CYCLE) &&
- device_link_flag_is_sync_state_only(link->flags))
- continue;
-
- if (!fw_devlink_relax_cycle(link->consumer, sup))
- continue;
-
- ret = 1;
-
- fw_devlink_relax_link(link);
- link->flags |= DL_FLAG_CYCLE;
- }
- return ret;
-}
static bool fwnode_init_without_drv(struct fwnode_handle *fwnode)
{
@@ -1935,6 +1894,113 @@ static bool fwnode_ancestor_init_without_drv(struct fwnode_handle *fwnode)
return false;
}
+/**
+ * __fw_devlink_relax_cycles - Relax and mark dependency cycles.
+ * @con: Potential consumer device.
+ * @sup_handle: Potential supplier's fwnode.
+ *
+ * Needs to be called with fwnode_lock and device link lock held.
+ *
+ * Check if @sup_handle or any of its ancestors or suppliers direct/indirectly
+ * depend on @con. This function can detect multiple cyles between @sup_handle
+ * and @con. When such dependency cycles are found, convert all device links
+ * created solely by fw_devlink into SYNC_STATE_ONLY device links. Also, mark
+ * all fwnode links in the cycle with FWLINK_FLAG_CYCLE so that when they are
+ * converted into a device link in the future, they are created as
+ * SYNC_STATE_ONLY device links. This is the equivalent of doing
+ * fw_devlink=permissive just between the devices in the cycle. We need to do
+ * this because, at this point, fw_devlink can't tell which of these
+ * dependencies is not a real dependency.
+ *
+ * Return true if one or more cycles were found. Otherwise, return false.
+ */
+static bool __fw_devlink_relax_cycles(struct device *con,
+ struct fwnode_handle *sup_handle)
+{
+ struct fwnode_link *link;
+ struct device_link *dev_link;
+ struct device *sup_dev = NULL, *par_dev = NULL;
+ bool ret = false;
+
+ if (!sup_handle)
+ return false;
+
+ /*
+ * We aren't trying to find all cycles. Just a cycle between con and
+ * sup_handle.
+ */
+ if (sup_handle->flags & FWNODE_FLAG_VISITED)
+ return false;
+
+ sup_handle->flags |= FWNODE_FLAG_VISITED;
+
+ sup_dev = get_dev_from_fwnode(sup_handle);
+
+ /* Termination condition. */
+ if (sup_dev == con) {
+ ret = true;
+ goto out;
+ }
+
+ /*
+ * If sup_dev is bound to a driver and @con hasn't started binding to
+ * a driver, @sup_dev can't be a consumer of @con. So, no need to
+ * check further.
+ */
+ if (sup_dev && sup_dev->links.status == DL_DEV_DRIVER_BOUND &&
+ con->links.status == DL_DEV_NO_DRIVER) {
+ ret = false;
+ goto out;
+ }
+
+ list_for_each_entry(link, &sup_handle->suppliers, c_hook) {
+ if (__fw_devlink_relax_cycles(con, link->supplier)) {
+ __fwnode_link_cycle(link);
+ ret = true;
+ }
+ }
+
+ /*
+ * Give priority to device parent over fwnode parent to account for any
+ * quirks in how fwnodes are converted to devices.
+ */
+ if (sup_dev) {
+ par_dev = sup_dev->parent;
+ get_device(par_dev);
+ } else {
+ par_dev = fwnode_get_next_parent_dev(sup_handle);
+ }
+
+ if (par_dev)
+ ret |= __fw_devlink_relax_cycles(con, par_dev->fwnode);
+
+ if (!sup_dev)
+ goto out;
+
+ list_for_each_entry(dev_link, &sup_dev->links.suppliers, c_node) {
+ /*
+ * Ignore a SYNC_STATE_ONLY flag only if it wasn't marked as a
+ * such due to a cycle.
+ */
+ if (device_link_flag_is_sync_state_only(dev_link->flags) &&
+ !(dev_link->flags & DL_FLAG_CYCLE))
+ continue;
+
+ if (__fw_devlink_relax_cycles(con,
+ dev_link->supplier->fwnode)) {
+ fw_devlink_relax_link(dev_link);
+ dev_link->flags |= DL_FLAG_CYCLE;
+ ret = true;
+ }
+ }
+
+out:
+ sup_handle->flags &= ~FWNODE_FLAG_VISITED;
+ put_device(sup_dev);
+ put_device(par_dev);
+ return ret;
+}
+
/**
* fw_devlink_create_devlink - Create a device link from a consumer to fwnode
* @con: consumer device for the device link
@@ -1987,6 +2053,21 @@ static int fw_devlink_create_devlink(struct device *con,
fwnode_is_ancestor_of(sup_handle, con->fwnode))
return -EINVAL;
+ /*
+ * SYNC_STATE_ONLY device links don't block probing and supports cycles.
+ * So cycle detection isn't necessary and shouldn't be done.
+ */
+ if (!(flags & DL_FLAG_SYNC_STATE_ONLY)) {
+ device_links_write_lock();
+ if (__fw_devlink_relax_cycles(con, sup_handle)) {
+ __fwnode_link_cycle(link);
+ flags = fw_devlink_get_flags(link->flags);
+ dev_info(con, "Fixed dependency cycle(s) with %pfwf\n",
+ sup_handle);
+ }
+ device_links_write_unlock();
+ }
+
sup_dev = get_dev_from_fwnode(sup_handle);
if (sup_dev) {
/*
@@ -1996,23 +2077,16 @@ static int fw_devlink_create_devlink(struct device *con,
*/
if (sup_dev->links.status == DL_DEV_NO_DRIVER &&
sup_handle->flags & FWNODE_FLAG_INITIALIZED) {
+ dev_dbg(con,
+ "Not linking %pfwf - dev might never probe\n",
+ sup_handle);
ret = -EINVAL;
goto out;
}
- /*
- * If this fails, it is due to cycles in device links. Just
- * give up on this link and treat it as invalid.
- */
- if (!device_link_add(con, sup_dev, flags) &&
- !(flags & DL_FLAG_SYNC_STATE_ONLY)) {
- dev_info(con, "Fixing up cyclic dependency with %s\n",
- dev_name(sup_dev));
- device_links_write_lock();
- fw_devlink_relax_cycle(con, sup_dev);
- device_links_write_unlock();
- device_link_add(con, sup_dev,
- FW_DEVLINK_FLAGS_PERMISSIVE);
+ if (!device_link_add(con, sup_dev, flags)) {
+ dev_err(con, "Failed to create device link with %s\n",
+ dev_name(sup_dev));
ret = -EINVAL;
}
@@ -2025,49 +2099,12 @@ static int fw_devlink_create_devlink(struct device *con,
*/
if (fwnode_init_without_drv(sup_handle) ||
fwnode_ancestor_init_without_drv(sup_handle)) {
- dev_dbg(con, "Not linking %pfwP - Might never probe\n",
+ dev_dbg(con, "Not linking %pfwf - might never become dev\n",
sup_handle);
return -EINVAL;
}
- /*
- * DL_FLAG_SYNC_STATE_ONLY doesn't block probing and supports
- * cycles. So cycle detection isn't necessary and shouldn't be
- * done.
- */
- if (flags & DL_FLAG_SYNC_STATE_ONLY)
- return -EAGAIN;
-
- /*
- * If we can't find the supplier device from its fwnode, it might be
- * due to a cyclic dependency between fwnodes. Some of these cycles can
- * be broken by applying logic. Check for these types of cycles and
- * break them so that devices in the cycle probe properly.
- *
- * If the supplier's parent is dependent on the consumer, then the
- * consumer and supplier have a cyclic dependency. Since fw_devlink
- * can't tell which of the inferred dependencies are incorrect, don't
- * enforce probe ordering between any of the devices in this cyclic
- * dependency. Do this by relaxing all the fw_devlink device links in
- * this cycle and by treating the fwnode link between the consumer and
- * the supplier as an invalid dependency.
- */
- sup_dev = fwnode_get_next_parent_dev(sup_handle);
- if (sup_dev && device_is_dependent(con, sup_dev)) {
- dev_info(con, "Fixing up cyclic dependency with %pfwP (%s)\n",
- sup_handle, dev_name(sup_dev));
- device_links_write_lock();
- fw_devlink_relax_cycle(con, sup_dev);
- device_links_write_unlock();
- ret = -EINVAL;
- } else {
- /*
- * Can't check for cycles or no cycles. So let's try
- * again later.
- */
- ret = -EAGAIN;
- }
-
+ ret = -EAGAIN;
out:
put_device(sup_dev);
return ret;
@@ -2174,7 +2211,6 @@ static void __fw_devlink_link_to_suppliers(struct device *dev,
list_for_each_entry_safe(link, tmp, &fwnode->suppliers, c_hook) {
int ret;
- struct device *sup_dev;
struct fwnode_handle *sup = link->supplier;
ret = fw_devlink_create_devlink(dev, sup, link);
@@ -2182,27 +2218,6 @@ static void __fw_devlink_link_to_suppliers(struct device *dev,
continue;
__fwnode_link_del(link);
-
- /* If no device link was created, nothing more to do. */
- if (ret)
- continue;
-
- /*
- * If a device link was successfully created to a supplier, we
- * now need to try and link the supplier to all its suppliers.
- *
- * This is needed to detect and delete false dependencies in
- * fwnode links that haven't been converted to a device link
- * yet. See comments in fw_devlink_create_devlink() for more
- * details on the false dependency.
- *
- * Without deleting these false dependencies, some devices will
- * never probe because they'll keep waiting for their false
- * dependency fwnode links to be converted to device links.
- */
- sup_dev = get_dev_from_fwnode(sup);
- __fw_devlink_link_to_suppliers(sup_dev, sup_dev->fwnode);
- put_device(sup_dev);
}
/*
--
2.37.1.559.g78731f0fdb-goog
When a device X is bound successfully to a driver, if it has a child
firmware node Y that doesn't have a struct device created by then, we
delete fwnode links where the child firmware node Y is the supplier. We
did this to avoid blocking the consumers of the child firmware node Y
from deferring probe indefinitely.
While that a step in the right direction, it's better to make the
consumers of the child firmware node Y to be consumers of the device X
because device X is probably implementing whatever functionality is
represented by child firmware node Y. By doing this, we capture the
device dependencies more accurately and ensure better
probe/suspend/resume ordering.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/base/core.c | 91 ++++++++++++++++++++++++++++++++++++---------
1 file changed, 74 insertions(+), 17 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 753e7cca0f40..6f575c2a24ad 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -53,11 +53,12 @@ static LIST_HEAD(deferred_sync);
static unsigned int defer_sync_state_count = 1;
static DEFINE_MUTEX(fwnode_link_lock);
static bool fw_devlink_is_permissive(void);
+static void __fw_devlink_link_to_consumers(struct device *dev);
static bool fw_devlink_drv_reg_done;
static bool fw_devlink_best_effort;
/**
- * fwnode_link_add - Create a link between two fwnode_handles.
+ * __fwnode_link_add - Create a link between two fwnode_handles.
* @con: Consumer end of the link.
* @sup: Supplier end of the link.
*
@@ -73,22 +74,18 @@ static bool fw_devlink_best_effort;
* Attempts to create duplicate links between the same pair of fwnode handles
* are ignored and there is no reference counting.
*/
-int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup)
+static int __fwnode_link_add(struct fwnode_handle *con,
+ struct fwnode_handle *sup)
{
struct fwnode_link *link;
- int ret = 0;
-
- mutex_lock(&fwnode_link_lock);
list_for_each_entry(link, &sup->consumers, s_hook)
if (link->consumer == con)
- goto out;
+ return 0;
link = kzalloc(sizeof(*link), GFP_KERNEL);
- if (!link) {
- ret = -ENOMEM;
- goto out;
- }
+ if (!link)
+ return -ENOMEM;
link->supplier = sup;
INIT_LIST_HEAD(&link->s_hook);
@@ -99,9 +96,17 @@ int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup)
list_add(&link->c_hook, &con->suppliers);
pr_debug("%pfwP Linked as a fwnode consumer to %pfwP\n",
con, sup);
-out:
- mutex_unlock(&fwnode_link_lock);
+ return 0;
+}
+
+int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup)
+{
+ int ret = 0;
+
+ mutex_lock(&fwnode_link_lock);
+ ret = __fwnode_link_add(con, sup);
+ mutex_unlock(&fwnode_link_lock);
return ret;
}
@@ -180,6 +185,51 @@ void fw_devlink_purge_absent_suppliers(struct fwnode_handle *fwnode)
}
EXPORT_SYMBOL_GPL(fw_devlink_purge_absent_suppliers);
+/**
+ * __fwnode_links_move_consumers - Move consumer from @from to @to fwnode_handle
+ * @from: move consumers away from this fwnode
+ * @to: move consumers to this fwnode
+ *
+ * Move all consumer links from @from fwnode to @to fwnode.
+ */
+static void __fwnode_links_move_consumers(struct fwnode_handle *from,
+ struct fwnode_handle *to)
+{
+ struct fwnode_link *link, *tmp;
+
+ list_for_each_entry_safe(link, tmp, &from->consumers, s_hook) {
+ __fwnode_link_add(link->consumer, to);
+ __fwnode_link_del(link);
+ }
+}
+
+/**
+ * __fw_devlink_pickup_dangling_consumers - Pick up dangling consumers
+ * @fwnode: fwnode from which to pick up dangling consumers
+ * @new_sup: fwnode of new supplier
+ *
+ * If the @fwnode has a corresponding struct device and the device supports
+ * probing (that is, added to a bus), then we want to let fw_devlink create
+ * MANAGED device links to this device, so leave @fwnode and its descendant's
+ * fwnode links alone.
+ *
+ * Otherwise, move its consumers to the new supplier @new_sup.
+ */
+static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode,
+ struct fwnode_handle *new_sup)
+{
+ struct fwnode_handle *child;
+
+ if (fwnode->dev && fwnode->dev->bus)
+ return;
+
+ fwnode->flags |= FWNODE_FLAG_NOT_DEVICE;
+ __fwnode_links_move_consumers(fwnode, new_sup);
+
+ fwnode_for_each_available_child_node(fwnode, child)
+ __fw_devlink_pickup_dangling_consumers(child, new_sup);
+}
+
#ifdef CONFIG_SRCU
static DEFINE_MUTEX(device_links_lock);
DEFINE_STATIC_SRCU(device_links_srcu);
@@ -1266,16 +1316,23 @@ void device_links_driver_bound(struct device *dev)
* them. So, fw_devlink no longer needs to create device links to any
* of the device's suppliers.
*
- * Also, if a child firmware node of this bound device is not added as
- * a device by now, assume it is never going to be added and make sure
- * other devices don't defer probe indefinitely by waiting for such a
- * child device.
+ * Also, if a child firmware node of this bound device is not added as a
+ * device by now, assume it is never going to be added. Make this bound
+ * device the fallback supplier to the dangling consumers of the child
+ * firmware node because this bound device is probably implementing the
+ * child firmware node functionality and we don't want the dangling
+ * consumers to defer probe indefinitely waiting for a device for the
+ * child firmware node.
*/
if (dev->fwnode && dev->fwnode->dev == dev) {
struct fwnode_handle *child;
fwnode_links_purge_suppliers(dev->fwnode);
+ mutex_lock(&fwnode_link_lock);
fwnode_for_each_available_child_node(dev->fwnode, child)
- fw_devlink_purge_absent_suppliers(child);
+ __fw_devlink_pickup_dangling_consumers(child,
+ dev->fwnode);
+ __fw_devlink_link_to_consumers(dev);
+ mutex_unlock(&fwnode_link_lock);
}
device_remove_file(dev, &dev_attr_waiting_for_supplier);
--
2.37.1.559.g78731f0fdb-goog
The driver core now:
- Has the parent device of a supplier pick up the consumers if the
supplier never has a device created for it.
- Ignores a supplier if the supplier has no parent device and will never
be probed by a driver
And already prevents creating a device link with the consumer as a
supplier of a parent.
So, we no longer need to find the "compatible" node of the supplier or
do any other checks in of_link_to_phandle(). We simply need to make sure
that the supplier is available in DT.
Signed-off-by: Saravana Kannan <[email protected]>
---
drivers/of/property.c | 84 +++++++------------------------------------
1 file changed, 13 insertions(+), 71 deletions(-)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 967f79b59016..98ca0399a354 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1060,20 +1060,6 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
return of_device_get_match_data(dev);
}
-static bool of_is_ancestor_of(struct device_node *test_ancestor,
- struct device_node *child)
-{
- of_node_get(child);
- while (child) {
- if (child == test_ancestor) {
- of_node_put(child);
- return true;
- }
- child = of_get_next_parent(child);
- }
- return false;
-}
-
static struct device_node *of_get_compat_node(struct device_node *np)
{
of_node_get(np);
@@ -1104,71 +1090,27 @@ static struct device_node *of_get_compat_node_parent(struct device_node *np)
return node;
}
-/**
- * of_link_to_phandle - Add fwnode link to supplier from supplier phandle
- * @con_np: consumer device tree node
- * @sup_np: supplier device tree node
- *
- * Given a phandle to a supplier device tree node (@sup_np), this function
- * finds the device that owns the supplier device tree node and creates a
- * device link from @dev consumer device to the supplier device. This function
- * doesn't create device links for invalid scenarios such as trying to create a
- * link with a parent device as the consumer of its child device. In such
- * cases, it returns an error.
- *
- * Returns:
- * - 0 if fwnode link successfully created to supplier
- * - -EINVAL if the supplier link is invalid and should not be created
- * - -ENODEV if struct device will never be create for supplier
- */
-static int of_link_to_phandle(struct device_node *con_np,
+static void of_link_to_phandle(struct device_node *con_np,
struct device_node *sup_np)
{
- struct device *sup_dev;
- struct device_node *tmp_np = sup_np;
+ struct device_node *tmp_np = of_node_get(sup_np);
- /*
- * Find the device node that contains the supplier phandle. It may be
- * @sup_np or it may be an ancestor of @sup_np.
- */
- sup_np = of_get_compat_node(sup_np);
- if (!sup_np) {
- pr_debug("Not linking %pOFP to %pOFP - No device\n",
- con_np, tmp_np);
- return -ENODEV;
- }
+ /* Check that sup_np and its ancestors are available. */
+ while (tmp_np) {
+ if (of_fwnode_handle(tmp_np)->dev) {
+ of_node_put(tmp_np);
+ break;
+ }
- /*
- * Don't allow linking a device node as a consumer of one of its
- * descendant nodes. By definition, a child node can't be a functional
- * dependency for the parent node.
- */
- if (of_is_ancestor_of(con_np, sup_np)) {
- pr_debug("Not linking %pOFP to %pOFP - is descendant\n",
- con_np, sup_np);
- of_node_put(sup_np);
- return -EINVAL;
- }
+ if (!of_device_is_available(tmp_np)) {
+ of_node_put(tmp_np);
+ return;
+ }
- /*
- * Don't create links to "early devices" that won't have struct devices
- * created for them.
- */
- sup_dev = get_dev_from_fwnode(&sup_np->fwnode);
- if (!sup_dev &&
- (of_node_check_flag(sup_np, OF_POPULATED) ||
- sup_np->fwnode.flags & FWNODE_FLAG_NOT_DEVICE)) {
- pr_debug("Not linking %pOFP to %pOFP - No struct device\n",
- con_np, sup_np);
- of_node_put(sup_np);
- return -ENODEV;
+ tmp_np = of_get_next_parent(tmp_np);
}
- put_device(sup_dev);
fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np));
- of_node_put(sup_np);
-
- return 0;
}
/**
--
2.37.1.559.g78731f0fdb-goog
* Saravana Kannan <[email protected]> [220810 05:54]:
> Tony,
>
> This should handle the odd case of the child being the supplier of the
> parent. Can you please give this a shot? I want to make sure the cycle
> detection code handles this properly and treats it like it's NOT a cycle.
Yup, this series works for me, so feel free to add:
Tested-by: Tony Lindgren <[email protected]>
I have some concerns though on how do we get a working -rc1 with the
earlier series applied? See the comments in the last patch of this
series.
Regards,
Tony
Hi,
* Saravana Kannan <[email protected]> [220810 05:54]:
> The driver core now:
> - Has the parent device of a supplier pick up the consumers if the
> supplier never has a device created for it.
> - Ignores a supplier if the supplier has no parent device and will never
> be probed by a driver
>
> And already prevents creating a device link with the consumer as a
> supplier of a parent.
>
> So, we no longer need to find the "compatible" node of the supplier or
> do any other checks in of_link_to_phandle(). We simply need to make sure
> that the supplier is available in DT.
This patch fixes booting for me, so it should be applied as a fix and
tagged with:
Fixes: 5a46079a9645 ("PM: domains: Delete usage of driver_deferred_probe_check_state()")
If there are dependencies to the other patches in this series, it might
make sense to revert commit 5a46079a9645 instead.
Anyways, thanks for fixing the issue, for this patch:
Reviewed-by: Tony Lindgren <[email protected]>
Tested-by: Tony Lindgren <[email protected]>
For the process, looks like the earlier series got merged despite the
issues reported. And we had non-booting Linux next for at least some SoCs
for weeks. And now we are about to have a non-booting -rc1 unless things
get fixed fast. Annoying glitches, sigh..
Regards,
Tony
On Fri, Aug 12, 2022 at 2:47 AM Tony Lindgren <[email protected]> wrote:
>
> Hi,
>
> * Saravana Kannan <[email protected]> [220810 05:54]:
> > The driver core now:
> > - Has the parent device of a supplier pick up the consumers if the
> > supplier never has a device created for it.
> > - Ignores a supplier if the supplier has no parent device and will never
> > be probed by a driver
> >
> > And already prevents creating a device link with the consumer as a
> > supplier of a parent.
> >
> > So, we no longer need to find the "compatible" node of the supplier or
> > do any other checks in of_link_to_phandle(). We simply need to make sure
> > that the supplier is available in DT.
>
> This patch fixes booting for me, so it should be applied as a fix and
> tagged with:
>
> Fixes: 5a46079a9645 ("PM: domains: Delete usage of driver_deferred_probe_check_state()")
>
> If there are dependencies to the other patches in this series, it might
> make sense to revert commit 5a46079a9645 instead.
Yes, there are dependencies on the rest of the patches in this series.
For linux-next, I think we should pick up this series once we get more
Tested-bys.
So if 5a46079a9645 is causing any regression in stable branches, we
should pick up the revert series [1] instead of this series we are
replying to.
[1] - https://lore.kernel.org/all/[email protected]/
> Anyways, thanks for fixing the issue, for this patch:
>
> Reviewed-by: Tony Lindgren <[email protected]>
> Tested-by: Tony Lindgren <[email protected]>
Thanks!
> For the process, looks like the earlier series got merged despite the
> issues reported.
If I'm not mistaken, the issues were reported after the series got
picked up. And the series got some tested-by s before it was picked
up. And once it's in git history, we obviously can't drop it.
> And we had non-booting Linux next for at least some SoCs
> for weeks. And now we are about to have a non-booting -rc1 unless things
> get fixed fast. Annoying glitches, sigh..
Sorry for breaking some boards -- so mean "creative" corner cases :)
This rewrite is way more flexible (removes a lot of limitations in
fw_devlink) and hopefully this handles all the corner cases. We'll
see.
-Saravana
On Fri, Aug 12, 2022 at 2:49 AM Tony Lindgren <[email protected]> wrote:
>
> * Saravana Kannan <[email protected]> [220810 05:54]:
> > Tony,
> >
> > This should handle the odd case of the child being the supplier of the
> > parent. Can you please give this a shot? I want to make sure the cycle
> > detection code handles this properly and treats it like it's NOT a cycle.
>
> Yup, this series works for me, so feel free to add:
>
> Tested-by: Tony Lindgren <[email protected]>
Thanks for testing!
Btw, out of curiosity, how many different boards did you test this on?
IIRC you had an issue only in one board, right? Not to say I didn't
break anything else, I'm just trying to see how much confidence we
have on this series so far. I'm hoping the rest of the folks I listed
in the email will get around to testing this series.
-Saravana
> I have some concerns though on how do we get a working -rc1 with the
> earlier series applied? See the comments in the last patch of this
> series.
I tried to reply, but not sure if it helps. We'll continue the discussion there.
-Saravana
+Naresh Kamboju
On Tue, Aug 9, 2022 at 11:00 PM Saravana Kannan <[email protected]> wrote:
>
> This patch series improves fw_devlink in the following ways:
>
> 1. It no longer cares about a fwnode having a "compatible" property. It
> figures this our more dynamically. The only expectation is that
> fwnode that are converted to devices actually get probed by a driver
> for the dependencies to be enforced correctly.
>
> 2. Finer grained dependency tracking. fw_devlink will now create device
> links from the consumer to the actual resource's device (if it has one,
> Eg: gpio_device) instead of the parent supplier device. This improves
> things like async suspend/resume ordering, potentially remove the need
> for frameworks to create device links, more parallelized async probing,
> and better sync_state() tracking.
>
> 3. Handle hardware/software quirks where a child firmware node gets
> populated as a device before its parent firmware node AND actually
> supplies a non-optional resource to the parent firmware node's
> device.
>
> 4. Way more robust at cycle handling (see patch for the insane cases).
>
> 5. Stops depending on OF_POPULATED to figure out some corner cases.
>
> 6. Simplifies the work that needs to be done by the firmware specific
> code.
>
> This took way too long to get done due to typo bugs I had in my rewrite or
> corner cases I had to find and handle. But it's fairly well tested at this
> point and I expect this to work properly.
>
> Abel & Doug,
>
> This should fix your cyclic dependency issues with your display. Can you
> give it a shot please?
>
> Alexander,
>
> This should fix your issue where the power domain device not having a
> compatible property. Can you give it a shot please?
>
> Tony,
>
> This should handle the odd case of the child being the supplier of the
> parent. Can you please give this a shot? I want to make sure the cycle
> detection code handles this properly and treats it like it's NOT a cycle.
>
> Geert,
>
> Can you test the renesas stuff I changed please? They should continue
> working like before. Any other sanity test on other hardware would be
> great too.
>
> Sudeep,
>
> I don't think there are any unfixed issues you had reported in my other
> patches that this series might fix, but it'll be nice if you could give
> this a sanity test.
>
> Guenter,
>
> I don't think this will fix the issue you reported in the amba patch, but
> it's worth a shot because it improves a bunch of corner case handling. So
> it might be better at handling whatever corner cases you might have in the
> qemu platforms.
Hi Naresh,
Thanks for testing these patches in the other thread. Mind giving your
tested-by here? I know you tested these patches in X15, but were there
also other boards these patches were tested on as part of the run?
Thanks,
Saravana
>
> Thanks,
> Saravana
>
> Cc: Abel Vesa <[email protected]>
> Cc: Alexander Stein <[email protected]>
> Cc: Tony Lindgren <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: John Stultz <[email protected]>
> Cc: Doug Anderson <[email protected]>
> Cc: Guenter Roeck <[email protected]>
>
> Saravana Kannan (9):
> driver core: fw_devlink: Don't purge child fwnode's consumer links
> driver core: fw_devlink: Improve check for fwnode with no
> device/driver
> soc: renesas: Move away from using OF_POPULATED for fw_devlink
> gpiolib: Clear the gpio_device's fwnode initialized flag before adding
> driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links
> driver core: fw_devlink: Allow marking a fwnode link as being part of
> a cycle
> driver core: fw_devlink: Consolidate device link flag computation
> driver core: fw_devlink: Make cycle detection more robust
> of: property: Simplify of_link_to_phandle()
>
> drivers/base/core.c | 437 +++++++++++++++++++++-----------
> drivers/gpio/gpiolib.c | 6 +
> drivers/of/property.c | 84 +-----
> drivers/soc/renesas/rcar-sysc.c | 2 +-
> include/linux/device.h | 1 +
> include/linux/fwnode.h | 12 +-
> 6 files changed, 323 insertions(+), 219 deletions(-)
>
> --
> 2.37.1.559.g78731f0fdb-goog
>
On Wed, Aug 10, 2022 at 8:00 AM Saravana Kannan <[email protected]> wrote:
>
> Registering an irqdomain sets the flag for the fwnode. But having the
> flag set when a device is added is interpreted by fw_devlink to mean the
> device has already been initialized and will never probe. This prevents
> fw_devlink from creating device links with the gpio_device as a
> supplier. So, clear the flag before adding the device.
>
> Signed-off-by: Saravana Kannan <[email protected]>
> ---
> drivers/gpio/gpiolib.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index cc9c0a12259e..1d57d6f24632 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -522,6 +522,12 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
> {
> int ret;
>
> + /*
> + * If fwnode doesn't belong to another device, it's safe to clear its
> + * initialized flag.
> + */
> + if (!gdev->dev.fwnode->dev)
> + fwnode_dev_initialized(gdev->dev.fwnode, false);
> ret = gcdev_register(gdev, gpio_devt);
> if (ret)
> return ret;
> --
> 2.37.1.559.g78731f0fdb-goog
>
Acked-by: Bartosz Golaszewski <[email protected]>
* Saravana Kannan <[email protected]> [220813 00:45]:
> On Fri, Aug 12, 2022 at 2:49 AM Tony Lindgren <[email protected]> wrote:
> >
> > * Saravana Kannan <[email protected]> [220810 05:54]:
> > > Tony,
> > >
> > > This should handle the odd case of the child being the supplier of the
> > > parent. Can you please give this a shot? I want to make sure the cycle
> > > detection code handles this properly and treats it like it's NOT a cycle.
> >
> > Yup, this series works for me, so feel free to add:
> >
> > Tested-by: Tony Lindgren <[email protected]>
>
> Thanks for testing!
>
> Btw, out of curiosity, how many different boards did you test this on?
> IIRC you had an issue only in one board, right? Not to say I didn't
> break anything else, I'm just trying to see how much confidence we
> have on this series so far. I'm hoping the rest of the folks I listed
> in the email will get around to testing this series.
Sorry if I was not clear earlier. The issue affects several generations
of TI 32-bit SoCs at least, not just one board.
> > I have some concerns though on how do we get a working -rc1 with the
> > earlier series applied? See the comments in the last patch of this
> > series.
>
> I tried to reply, but not sure if it helps. We'll continue the discussion there.
Ack.
Tony
On 22-08-09 23:00:29, Saravana Kannan wrote:
> This patch series improves fw_devlink in the following ways:
>
> 1. It no longer cares about a fwnode having a "compatible" property. It
> figures this our more dynamically. The only expectation is that
> fwnode that are converted to devices actually get probed by a driver
> for the dependencies to be enforced correctly.
>
> 2. Finer grained dependency tracking. fw_devlink will now create device
> links from the consumer to the actual resource's device (if it has one,
> Eg: gpio_device) instead of the parent supplier device. This improves
> things like async suspend/resume ordering, potentially remove the need
> for frameworks to create device links, more parallelized async probing,
> and better sync_state() tracking.
>
> 3. Handle hardware/software quirks where a child firmware node gets
> populated as a device before its parent firmware node AND actually
> supplies a non-optional resource to the parent firmware node's
> device.
>
> 4. Way more robust at cycle handling (see patch for the insane cases).
>
> 5. Stops depending on OF_POPULATED to figure out some corner cases.
>
> 6. Simplifies the work that needs to be done by the firmware specific
> code.
>
> This took way too long to get done due to typo bugs I had in my rewrite or
> corner cases I had to find and handle. But it's fairly well tested at this
> point and I expect this to work properly.
>
> Abel & Doug,
>
> This should fix your cyclic dependency issues with your display. Can you
> give it a shot please?
Tested the specific case we discussed about here:
https://lore.kernel.org/all/CAGETcx8F0wP+RA0KpjOJeZfc=DVG-MbM_=SkRHD4UhD2ReL7Kw@mail.gmail.com/raw
Thanks for fixing this.
Tested-by: Abel Vesa <[email protected]>
>
> Alexander,
>
> This should fix your issue where the power domain device not having a
> compatible property. Can you give it a shot please?
>
> Tony,
>
> This should handle the odd case of the child being the supplier of the
> parent. Can you please give this a shot? I want to make sure the cycle
> detection code handles this properly and treats it like it's NOT a cycle.
>
> Geert,
>
> Can you test the renesas stuff I changed please? They should continue
> working like before. Any other sanity test on other hardware would be
> great too.
>
> Sudeep,
>
> I don't think there are any unfixed issues you had reported in my other
> patches that this series might fix, but it'll be nice if you could give
> this a sanity test.
>
> Guenter,
>
> I don't think this will fix the issue you reported in the amba patch, but
> it's worth a shot because it improves a bunch of corner case handling. So
> it might be better at handling whatever corner cases you might have in the
> qemu platforms.
>
> Thanks,
> Saravana
>
> Cc: Abel Vesa <[email protected]>
> Cc: Alexander Stein <[email protected]>
> Cc: Tony Lindgren <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: John Stultz <[email protected]>
> Cc: Doug Anderson <[email protected]>
> Cc: Guenter Roeck <[email protected]>
>
> Saravana Kannan (9):
> driver core: fw_devlink: Don't purge child fwnode's consumer links
> driver core: fw_devlink: Improve check for fwnode with no
> device/driver
> soc: renesas: Move away from using OF_POPULATED for fw_devlink
> gpiolib: Clear the gpio_device's fwnode initialized flag before adding
> driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links
> driver core: fw_devlink: Allow marking a fwnode link as being part of
> a cycle
> driver core: fw_devlink: Consolidate device link flag computation
> driver core: fw_devlink: Make cycle detection more robust
> of: property: Simplify of_link_to_phandle()
>
> drivers/base/core.c | 437 +++++++++++++++++++++-----------
> drivers/gpio/gpiolib.c | 6 +
> drivers/of/property.c | 84 +-----
> drivers/soc/renesas/rcar-sysc.c | 2 +-
> include/linux/device.h | 1 +
> include/linux/fwnode.h | 12 +-
> 6 files changed, 323 insertions(+), 219 deletions(-)
>
> --
> 2.37.1.559.g78731f0fdb-goog
>
On Sun, 14 Aug 2022 at 11:29, Saravana Kannan <[email protected]> wrote:
>
> +Naresh Kamboju
>
> On Tue, Aug 9, 2022 at 11:00 PM Saravana Kannan <[email protected]> wrote:
> >
> > This patch series improves fw_devlink in the following ways:
> >
> > 1. It no longer cares about a fwnode having a "compatible" property. It
> > figures this our more dynamically. The only expectation is that
> > fwnode that are converted to devices actually get probed by a driver
> > for the dependencies to be enforced correctly.
> >
> > 2. Finer grained dependency tracking. fw_devlink will now create device
> > links from the consumer to the actual resource's device (if it has one,
> > Eg: gpio_device) instead of the parent supplier device. This improves
> > things like async suspend/resume ordering, potentially remove the need
> > for frameworks to create device links, more parallelized async probing,
> > and better sync_state() tracking.
> >
> > 3. Handle hardware/software quirks where a child firmware node gets
> > populated as a device before its parent firmware node AND actually
> > supplies a non-optional resource to the parent firmware node's
> > device.
> >
> > 4. Way more robust at cycle handling (see patch for the insane cases).
> >
> > 5. Stops depending on OF_POPULATED to figure out some corner cases.
> >
> > 6. Simplifies the work that needs to be done by the firmware specific
> > code.
> >
> > This took way too long to get done due to typo bugs I had in my rewrite or
> > corner cases I had to find and handle. But it's fairly well tested at this
> > point and I expect this to work properly.
> >
> > Abel & Doug,
> >
> > This should fix your cyclic dependency issues with your display. Can you
> > give it a shot please?
> >
> > Alexander,
> >
> > This should fix your issue where the power domain device not having a
> > compatible property. Can you give it a shot please?
> >
> > Tony,
> >
> > This should handle the odd case of the child being the supplier of the
> > parent. Can you please give this a shot? I want to make sure the cycle
> > detection code handles this properly and treats it like it's NOT a cycle.
> >
> > Geert,
> >
> > Can you test the renesas stuff I changed please? They should continue
> > working like before. Any other sanity test on other hardware would be
> > great too.
> >
> > Sudeep,
> >
> > I don't think there are any unfixed issues you had reported in my other
> > patches that this series might fix, but it'll be nice if you could give
> > this a sanity test.
> >
> > Guenter,
> >
> > I don't think this will fix the issue you reported in the amba patch, but
> > it's worth a shot because it improves a bunch of corner case handling. So
> > it might be better at handling whatever corner cases you might have in the
> > qemu platforms.
>
> Hi Naresh,
>
> Thanks for testing these patches in the other thread. Mind giving your
> tested-by here? I know you tested these patches in X15, but were there
> also other boards these patches were tested on as part of the run?
I have tested your patches and boot is successful on x15 device and Juno-r2.
Tested-by: Linux Kernel Functional Testing <[email protected]>
Tested-by: Naresh Kamboju <[email protected]>
> > Cc: Abel Vesa <[email protected]>
> > Cc: Alexander Stein <[email protected]>
> > Cc: Tony Lindgren <[email protected]>
> > Cc: Sudeep Holla <[email protected]>
> > Cc: Geert Uytterhoeven <[email protected]>
> > Cc: John Stultz <[email protected]>
> > Cc: Doug Anderson <[email protected]>
> > Cc: Guenter Roeck <[email protected]>
> >
> > Saravana Kannan (9):
> > driver core: fw_devlink: Don't purge child fwnode's consumer links
> > driver core: fw_devlink: Improve check for fwnode with no
> > device/driver
> > soc: renesas: Move away from using OF_POPULATED for fw_devlink
> > gpiolib: Clear the gpio_device's fwnode initialized flag before adding
> > driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links
> > driver core: fw_devlink: Allow marking a fwnode link as being part of
> > a cycle
> > driver core: fw_devlink: Consolidate device link flag computation
> > driver core: fw_devlink: Make cycle detection more robust
> > of: property: Simplify of_link_to_phandle()
> >
> > drivers/base/core.c | 437 +++++++++++++++++++++-----------
> > drivers/gpio/gpiolib.c | 6 +
> > drivers/of/property.c | 84 +-----
> > drivers/soc/renesas/rcar-sysc.c | 2 +-
> > include/linux/device.h | 1 +
> > include/linux/fwnode.h | 12 +-
> > 6 files changed, 323 insertions(+), 219 deletions(-)
> >
> > --
> > 2.37.1.559.g78731f0fdb-goog
- Naresh
* Saravana Kannan <[email protected]> [220813 00:30]:
> On Fri, Aug 12, 2022 at 2:47 AM Tony Lindgren <[email protected]> wrote:
> >
> > Hi,
> >
> > * Saravana Kannan <[email protected]> [220810 05:54]:
> > > The driver core now:
> > > - Has the parent device of a supplier pick up the consumers if the
> > > supplier never has a device created for it.
> > > - Ignores a supplier if the supplier has no parent device and will never
> > > be probed by a driver
> > >
> > > And already prevents creating a device link with the consumer as a
> > > supplier of a parent.
> > >
> > > So, we no longer need to find the "compatible" node of the supplier or
> > > do any other checks in of_link_to_phandle(). We simply need to make sure
> > > that the supplier is available in DT.
> >
> > This patch fixes booting for me, so it should be applied as a fix and
> > tagged with:
> >
> > Fixes: 5a46079a9645 ("PM: domains: Delete usage of driver_deferred_probe_check_state()")
> >
> > If there are dependencies to the other patches in this series, it might
> > make sense to revert commit 5a46079a9645 instead.
>
> Yes, there are dependencies on the rest of the patches in this series.
> For linux-next, I think we should pick up this series once we get more
> Tested-bys.
>
> So if 5a46079a9645 is causing any regression in stable branches, we
> should pick up the revert series [1] instead of this series we are
> replying to.
Agreed we should apply the reverts in [1] for v6.0-rc series. At least
several generations of the TI 32-bit ARM SoCs are failing to boot
otherwise.
Regards,
Tony
Hello Saravana,
Am Mittwoch, 10. August 2022, 08:00:29 CEST schrieb Saravana Kannan:
> Alexander,
>
> This should fix your issue where the power domain device not having a
> compatible property. Can you give it a shot please?
thanks for the update. Unfortunately this does not work:
> [ 0.774838] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@0
> [ 0.775100] imx-pgc imx-pgc-domain.1: __genpd_dev_pm_attach() failed to
find PM domain: -2
> [ 0.775324] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@2
> [ 0.775601] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@3
> [ 0.775842] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@4
> [ 0.776642] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@7
> [ 0.776897] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@8
> [ 0.777158] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@9
> [ 0.777405] PM: Added domain provider from /soc@0/bus@30000000/
gpc@303a0000/pgc/power-domain@a
> [ 0.779342] genpd genpd:0:38320000.blk-ctrl: __genpd_dev_pm_attach()
failed to find PM domain: -2
> [ 0.779422] imx8m-blk-ctrl 38320000.blk-ctrl: error -ENODEV: failed to
attach power domain "bus"
> [ 0.848785] etnaviv-gpu 38000000.gpu: __genpd_dev_pm_attach() failed to
find PM domain: -2
> [ 1.114220] pfuze100-regulator 0-0008: Full layer: 2, Metal layer: 1
> [ 1.122267] pfuze100-regulator 0-0008: FAB: 0, FIN: 0
> [ 1.132970] pfuze100-regulator 0-0008: pfuze100 found.
> [ 1.157011] imx-gpcv2 303a0000.gpc: Failed to create device link with
0-0008
> [ 1.164094] imx-gpcv2 303a0000.gpc: Failed to create device link with
0-0008
The required power-supply for the power domains is still not yet available.
Does this series require some other patches as well?
Whats worse, starting with commit 9/9 [of: property: Simplify
of_link_to_phandle()], other drivers fail to probe waiting for pinctrl to be
available.
> $ cat /sys/kernel/debug/devices_deferred
> gpio-leds platform: wait for supplier gpioledgrp
> extcon-usbotg0 platform: wait for supplier usb0congrp
> gpio-keys platform: wait for supplier gpiobuttongrp
> regulator-otg-vbus platform: wait for supplier reggotgvbusgrp
> regulator-vdd-arm platform: wait for supplier dvfsgrp
Apparently for some reason they are not probed again, once the pinctrl driver
probed.
Best reagrds,
Alexander
On Tue, Aug 09, 2022 at 11:00:29PM -0700, Saravana Kannan wrote:
> This patch series improves fw_devlink in the following ways:
>
> 1. It no longer cares about a fwnode having a "compatible" property. It
> figures this our more dynamically. The only expectation is that
> fwnode that are converted to devices actually get probed by a driver
> for the dependencies to be enforced correctly.
>
> 2. Finer grained dependency tracking. fw_devlink will now create device
> links from the consumer to the actual resource's device (if it has one,
> Eg: gpio_device) instead of the parent supplier device. This improves
> things like async suspend/resume ordering, potentially remove the need
> for frameworks to create device links, more parallelized async probing,
> and better sync_state() tracking.
>
> 3. Handle hardware/software quirks where a child firmware node gets
> populated as a device before its parent firmware node AND actually
> supplies a non-optional resource to the parent firmware node's
> device.
>
> 4. Way more robust at cycle handling (see patch for the insane cases).
>
> 5. Stops depending on OF_POPULATED to figure out some corner cases.
>
> 6. Simplifies the work that needs to be done by the firmware specific
> code.
>
> This took way too long to get done due to typo bugs I had in my rewrite or
> corner cases I had to find and handle. But it's fairly well tested at this
> point and I expect this to work properly.
>
> Abel & Doug,
>
> This should fix your cyclic dependency issues with your display. Can you
> give it a shot please?
>
> Alexander,
>
> This should fix your issue where the power domain device not having a
> compatible property. Can you give it a shot please?
>
> Tony,
>
> This should handle the odd case of the child being the supplier of the
> parent. Can you please give this a shot? I want to make sure the cycle
> detection code handles this properly and treats it like it's NOT a cycle.
>
> Geert,
>
> Can you test the renesas stuff I changed please? They should continue
> working like before. Any other sanity test on other hardware would be
> great too.
>
> Sudeep,
>
> I don't think there are any unfixed issues you had reported in my other
> patches that this series might fix, but it'll be nice if you could give
> this a sanity test.
>
Sure tested this on Juno on top of v6.0-rc1 and found no regressions.
So,
Tested-by: Sudeep Holla <[email protected]>
Just wanted to check if the logs are intentional or do you plan to make
them debug. On Juno with hardly few such dependencies I get below extra
logs during boot, it may add loads on other platforms. I am fine either
way, just thought of checking.
| amba 20040000.funnel: Fixed dependency cycle(s) with /etf@20010000/in-ports/port/endpoint
| amba 20120000.replicator: Fixed dependency cycle(s) with /etr@20070000/in-ports/port/endpoint
| amba 20120000.replicator: Fixed dependency cycle(s) with /tpiu@20030000/in-ports/port/endpoint
| amba 220c0000.funnel: Fixed dependency cycle(s) with /etm@22040000/out-ports/port/endpoint
| amba 220c0000.funnel: Fixed dependency cycle(s) with /funnel@20040000/in-ports/port@0/endpoint
| amba 22140000.etm: Fixed dependency cycle(s) with /funnel@220c0000/in-ports/port@1/endpoint
| amba 230c0000.funnel: Fixed dependency cycle(s) with /etm@23040000/out-ports/port/endpoint
| amba 230c0000.funnel: Fixed dependency cycle(s) with /funnel@20040000/in-ports/port@1/endpoint
| amba 23140000.etm: Fixed dependency cycle(s) with /funnel@230c0000/in-ports/port@1/endpoint
| amba 23240000.etm: Fixed dependency cycle(s) with /funnel@230c0000/in-ports/port@2/endpoint
| amba 23340000.etm: Fixed dependency cycle(s) with /funnel@230c0000/in-ports/port@3/endpoint
| amba 20130000.funnel: Fixed dependency cycle(s) with /stm@20100000/out-ports/port/endpoint
| amba 20140000.etf: Fixed dependency cycle(s) with /funnel@20130000/out-ports/port/endpoint
| amba 20150000.funnel: Fixed dependency cycle(s) with /etf@20140000/out-ports/port/endpoint
| amba 20150000.funnel: Fixed dependency cycle(s) with /etf@20010000/out-ports/port/endpoint
| amba 20150000.funnel: Fixed dependency cycle(s) with /replicator@20120000/in-ports/port/endpoint
| i2c 0-0070: Fixed dependency cycle(s) with /hdlcd@7ff60000/port/endpoint
| i2c 0-0071: Fixed dependency cycle(s) with /hdlcd@7ff50000/port/endpoint
--
Regards,
Sudeep
On Mon, Aug 15, 2022 at 3:33 AM Tony Lindgren <[email protected]> wrote:
>
> * Saravana Kannan <[email protected]> [220813 00:45]:
> > On Fri, Aug 12, 2022 at 2:49 AM Tony Lindgren <[email protected]> wrote:
> > >
> > > * Saravana Kannan <[email protected]> [220810 05:54]:
> > > > Tony,
> > > >
> > > > This should handle the odd case of the child being the supplier of the
> > > > parent. Can you please give this a shot? I want to make sure the cycle
> > > > detection code handles this properly and treats it like it's NOT a cycle.
> > >
> > > Yup, this series works for me, so feel free to add:
> > >
> > > Tested-by: Tony Lindgren <[email protected]>
> >
> > Thanks for testing!
> >
> > Btw, out of curiosity, how many different boards did you test this on?
> > IIRC you had an issue only in one board, right? Not to say I didn't
> > break anything else, I'm just trying to see how much confidence we
> > have on this series so far. I'm hoping the rest of the folks I listed
> > in the email will get around to testing this series.
>
> Sorry if I was not clear earlier. The issue affects several generations
> of TI 32-bit SoCs at least, not just one board.
But this series fixes the issues for all of them or are you still
seeing some broken boot with this series?
-Saravana
>
> > > I have some concerns though on how do we get a working -rc1 with the
> > > earlier series applied? See the comments in the last patch of this
> > > series.
> >
> > I tried to reply, but not sure if it helps. We'll continue the discussion there.
>
> Ack.
>
> Tony
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
>
On Mon, Aug 15, 2022 at 4:01 AM Abel Vesa <[email protected]> wrote:
>
> On 22-08-09 23:00:29, Saravana Kannan wrote:
> > This patch series improves fw_devlink in the following ways:
> >
> > 1. It no longer cares about a fwnode having a "compatible" property. It
> > figures this our more dynamically. The only expectation is that
> > fwnode that are converted to devices actually get probed by a driver
> > for the dependencies to be enforced correctly.
> >
> > 2. Finer grained dependency tracking. fw_devlink will now create device
> > links from the consumer to the actual resource's device (if it has one,
> > Eg: gpio_device) instead of the parent supplier device. This improves
> > things like async suspend/resume ordering, potentially remove the need
> > for frameworks to create device links, more parallelized async probing,
> > and better sync_state() tracking.
> >
> > 3. Handle hardware/software quirks where a child firmware node gets
> > populated as a device before its parent firmware node AND actually
> > supplies a non-optional resource to the parent firmware node's
> > device.
> >
> > 4. Way more robust at cycle handling (see patch for the insane cases).
> >
> > 5. Stops depending on OF_POPULATED to figure out some corner cases.
> >
> > 6. Simplifies the work that needs to be done by the firmware specific
> > code.
> >
> > This took way too long to get done due to typo bugs I had in my rewrite or
> > corner cases I had to find and handle. But it's fairly well tested at this
> > point and I expect this to work properly.
> >
> > Abel & Doug,
> >
> > This should fix your cyclic dependency issues with your display. Can you
> > give it a shot please?
>
> Tested the specific case we discussed about here:
> https://lore.kernel.org/all/CAGETcx8F0wP+RA0KpjOJeZfc=DVG-MbM_=SkRHD4UhD2ReL7Kw@mail.gmail.com/raw
>
> Thanks for fixing this.
>
> Tested-by: Abel Vesa <[email protected]>
Thanks!
-Saravana
>
> >
> > Alexander,
> >
> > This should fix your issue where the power domain device not having a
> > compatible property. Can you give it a shot please?
> >
> > Tony,
> >
> > This should handle the odd case of the child being the supplier of the
> > parent. Can you please give this a shot? I want to make sure the cycle
> > detection code handles this properly and treats it like it's NOT a cycle.
> >
> > Geert,
> >
> > Can you test the renesas stuff I changed please? They should continue
> > working like before. Any other sanity test on other hardware would be
> > great too.
> >
> > Sudeep,
> >
> > I don't think there are any unfixed issues you had reported in my other
> > patches that this series might fix, but it'll be nice if you could give
> > this a sanity test.
> >
> > Guenter,
> >
> > I don't think this will fix the issue you reported in the amba patch, but
> > it's worth a shot because it improves a bunch of corner case handling. So
> > it might be better at handling whatever corner cases you might have in the
> > qemu platforms.
> >
> > Thanks,
> > Saravana
> >
> > Cc: Abel Vesa <[email protected]>
> > Cc: Alexander Stein <[email protected]>
> > Cc: Tony Lindgren <[email protected]>
> > Cc: Sudeep Holla <[email protected]>
> > Cc: Geert Uytterhoeven <[email protected]>
> > Cc: John Stultz <[email protected]>
> > Cc: Doug Anderson <[email protected]>
> > Cc: Guenter Roeck <[email protected]>
> >
> > Saravana Kannan (9):
> > driver core: fw_devlink: Don't purge child fwnode's consumer links
> > driver core: fw_devlink: Improve check for fwnode with no
> > device/driver
> > soc: renesas: Move away from using OF_POPULATED for fw_devlink
> > gpiolib: Clear the gpio_device's fwnode initialized flag before adding
> > driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links
> > driver core: fw_devlink: Allow marking a fwnode link as being part of
> > a cycle
> > driver core: fw_devlink: Consolidate device link flag computation
> > driver core: fw_devlink: Make cycle detection more robust
> > of: property: Simplify of_link_to_phandle()
> >
> > drivers/base/core.c | 437 +++++++++++++++++++++-----------
> > drivers/gpio/gpiolib.c | 6 +
> > drivers/of/property.c | 84 +-----
> > drivers/soc/renesas/rcar-sysc.c | 2 +-
> > include/linux/device.h | 1 +
> > include/linux/fwnode.h | 12 +-
> > 6 files changed, 323 insertions(+), 219 deletions(-)
> >
> > --
> > 2.37.1.559.g78731f0fdb-goog
> >
On Mon, Aug 15, 2022 at 5:39 AM Alexander Stein
<[email protected]> wrote:
>
> Hello Saravana,
>
> Am Mittwoch, 10. August 2022, 08:00:29 CEST schrieb Saravana Kannan:
> > Alexander,
> >
> > This should fix your issue where the power domain device not having a
> > compatible property. Can you give it a shot please?
>
> thanks for the update. Unfortunately this does not work:
>
> > [ 0.774838] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@0
> > [ 0.775100] imx-pgc imx-pgc-domain.1: __genpd_dev_pm_attach() failed to
> find PM domain: -2
> > [ 0.775324] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@2
> > [ 0.775601] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@3
> > [ 0.775842] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@4
> > [ 0.776642] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@7
> > [ 0.776897] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@8
> > [ 0.777158] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@9
> > [ 0.777405] PM: Added domain provider from /soc@0/bus@30000000/
> gpc@303a0000/pgc/power-domain@a
> > [ 0.779342] genpd genpd:0:38320000.blk-ctrl: __genpd_dev_pm_attach()
> failed to find PM domain: -2
> > [ 0.779422] imx8m-blk-ctrl 38320000.blk-ctrl: error -ENODEV: failed to
> attach power domain "bus"
> > [ 0.848785] etnaviv-gpu 38000000.gpu: __genpd_dev_pm_attach() failed to
> find PM domain: -2
> > [ 1.114220] pfuze100-regulator 0-0008: Full layer: 2, Metal layer: 1
> > [ 1.122267] pfuze100-regulator 0-0008: FAB: 0, FIN: 0
> > [ 1.132970] pfuze100-regulator 0-0008: pfuze100 found.
> > [ 1.157011] imx-gpcv2 303a0000.gpc: Failed to create device link with
> 0-0008
> > [ 1.164094] imx-gpcv2 303a0000.gpc: Failed to create device link with
> 0-0008
>
> The required power-supply for the power domains is still not yet available.
> Does this series require some other patches as well?
Ah sorry, yeah, this needs additional patches. The one I gave in the
other thread when I debugged this and I also noticed another issue.
Here's the combined diff of what's needed. Can you add this on top of
the series and test it?
diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
index b9c22f764b4d..8a0e82067924 100644
--- a/drivers/irqchip/irq-imx-gpcv2.c
+++ b/drivers/irqchip/irq-imx-gpcv2.c
@@ -283,6 +283,7 @@ static int __init imx_gpcv2_irqchip_init(struct
device_node *node,
* later the GPC power domain driver will not be skipped.
*/
of_node_clear_flag(node, OF_POPULATED);
+ fwnode_dev_initialized(domain->fwnode, false);
return 0;
}
diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
index 6383a4edc360..181fbfe5bd4d 100644
--- a/drivers/soc/imx/gpcv2.c
+++ b/drivers/soc/imx/gpcv2.c
@@ -1513,6 +1513,7 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
pd_pdev->dev.parent = dev;
pd_pdev->dev.of_node = np;
+ pd_pdev->dev.fwnode = of_fwnode_handle(np);
ret = platform_device_add(pd_pdev);
if (ret) {
With this patch, I'd really expect the power domain dependency to be
handled correctly.
> Whats worse, starting with commit 9/9 [of: property: Simplify
> of_link_to_phandle()], other drivers fail to probe waiting for pinctrl to be
> available.
Heh, Patch 9/9 and all its other dependencies in this series was to
fix your use case. Ironic that it's causing you more issues.
> > $ cat /sys/kernel/debug/devices_deferred
> > gpio-leds platform: wait for supplier gpioledgrp
> > extcon-usbotg0 platform: wait for supplier usb0congrp
> > gpio-keys platform: wait for supplier gpiobuttongrp
> > regulator-otg-vbus platform: wait for supplier reggotgvbusgrp
> > regulator-vdd-arm platform: wait for supplier dvfsgrp
>
> Apparently for some reason they are not probed again, once the pinctrl driver
> probed.
I'm hoping that this is just some issue due to the missing patch
above, but doesn't sound like it if you say that the pinctrl ended up
probing eventually.
So when device_links_driver_bound() calls
__fw_devlink_pickup_dangling_consumers(), it should have picked up the
consumers of node like gpiobuttongrp and moved it to the pinctrl
device. And right after that we call __fw_devlink_link_to_consumers()
that would have created the device links. And then right after that,
we go through all the consumers and add them to the deferred probe
list. After that deferred probe should have run... either because it's
enabled at late_initcall() or because a new device probed
successfully.
Can you check which one of my expectations isn't true in your case?
Thanks,
Saravana
On Mon, Aug 15, 2022 at 12:17 PM Saravana Kannan <[email protected]> wrote:
>
> On Mon, Aug 15, 2022 at 5:39 AM Alexander Stein
> <[email protected]> wrote:
> >
> > Hello Saravana,
> >
> > Am Mittwoch, 10. August 2022, 08:00:29 CEST schrieb Saravana Kannan:
> > > Alexander,
> > >
> > > This should fix your issue where the power domain device not having a
> > > compatible property. Can you give it a shot please?
> >
> > thanks for the update. Unfortunately this does not work:
> >
> > > [ 0.774838] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@0
> > > [ 0.775100] imx-pgc imx-pgc-domain.1: __genpd_dev_pm_attach() failed to
> > find PM domain: -2
> > > [ 0.775324] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@2
> > > [ 0.775601] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@3
> > > [ 0.775842] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@4
> > > [ 0.776642] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@7
> > > [ 0.776897] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@8
> > > [ 0.777158] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@9
> > > [ 0.777405] PM: Added domain provider from /soc@0/bus@30000000/
> > gpc@303a0000/pgc/power-domain@a
> > > [ 0.779342] genpd genpd:0:38320000.blk-ctrl: __genpd_dev_pm_attach()
> > failed to find PM domain: -2
> > > [ 0.779422] imx8m-blk-ctrl 38320000.blk-ctrl: error -ENODEV: failed to
> > attach power domain "bus"
> > > [ 0.848785] etnaviv-gpu 38000000.gpu: __genpd_dev_pm_attach() failed to
> > find PM domain: -2
> > > [ 1.114220] pfuze100-regulator 0-0008: Full layer: 2, Metal layer: 1
> > > [ 1.122267] pfuze100-regulator 0-0008: FAB: 0, FIN: 0
> > > [ 1.132970] pfuze100-regulator 0-0008: pfuze100 found.
> > > [ 1.157011] imx-gpcv2 303a0000.gpc: Failed to create device link with
> > 0-0008
> > > [ 1.164094] imx-gpcv2 303a0000.gpc: Failed to create device link with
> > 0-0008
> >
> > The required power-supply for the power domains is still not yet available.
> > Does this series require some other patches as well?
>
> Ah sorry, yeah, this needs additional patches. The one I gave in the
> other thread when I debugged this and I also noticed another issue.
> Here's the combined diff of what's needed. Can you add this on top of
> the series and test it?
>
> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
> index b9c22f764b4d..8a0e82067924 100644
> --- a/drivers/irqchip/irq-imx-gpcv2.c
> +++ b/drivers/irqchip/irq-imx-gpcv2.c
> @@ -283,6 +283,7 @@ static int __init imx_gpcv2_irqchip_init(struct
> device_node *node,
> * later the GPC power domain driver will not be skipped.
> */
> of_node_clear_flag(node, OF_POPULATED);
> + fwnode_dev_initialized(domain->fwnode, false);
> return 0;
> }
>
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 6383a4edc360..181fbfe5bd4d 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -1513,6 +1513,7 @@ static int imx_gpcv2_probe(struct platform_device *pdev)
>
> pd_pdev->dev.parent = dev;
> pd_pdev->dev.of_node = np;
> + pd_pdev->dev.fwnode = of_fwnode_handle(np);
>
> ret = platform_device_add(pd_pdev);
> if (ret) {
>
> With this patch, I'd really expect the power domain dependency to be
> handled correctly.
>
> > Whats worse, starting with commit 9/9 [of: property: Simplify
> > of_link_to_phandle()], other drivers fail to probe waiting for pinctrl to be
> > available.
>
> Heh, Patch 9/9 and all its other dependencies in this series was to
> fix your use case. Ironic that it's causing you more issues.
>
> > > $ cat /sys/kernel/debug/devices_deferred
> > > gpio-leds platform: wait for supplier gpioledgrp
> > > extcon-usbotg0 platform: wait for supplier usb0congrp
> > > gpio-keys platform: wait for supplier gpiobuttongrp
> > > regulator-otg-vbus platform: wait for supplier reggotgvbusgrp
> > > regulator-vdd-arm platform: wait for supplier dvfsgrp
> >
> > Apparently for some reason they are not probed again, once the pinctrl driver
> > probed.
>
> I'm hoping that this is just some issue due to the missing patch
> above, but doesn't sound like it if you say that the pinctrl ended up
> probing eventually.
>
> So when device_links_driver_bound() calls
> __fw_devlink_pickup_dangling_consumers(), it should have picked up the
> consumers of node like gpiobuttongrp and moved it to the pinctrl
> device. And right after that we call __fw_devlink_link_to_consumers()
> that would have created the device links. And then right after that,
> we go through all the consumers and add them to the deferred probe
> list. After that deferred probe should have run... either because it's
> enabled at late_initcall() or because a new device probed
> successfully.
>
> Can you check which one of my expectations isn't true in your case?
Actually I have a hypothesis on what might be happening. It could be a
case of the consumer device getting added after the supplier has been
initialized.
If the patch above doesn't fix everything, can you add this diff on
top of the patch above and see if that fixes everything? If it fixes
the pinctrl issue, can you check my hypothesis be checking in what
order the devices get added and get probed?
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2f012e826986..866755d8ad95 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2068,7 +2068,11 @@ static int fw_devlink_create_devlink(struct device *con,
device_links_write_unlock();
}
- sup_dev = get_dev_from_fwnode(sup_handle);
+ if (sup_handle->flags & FWNODE_FLAG_NOT_DEVICE)
+ sup_dev = fwnode_get_next_parent_dev(sup_handle);
+ else
+ sup_dev = get_dev_from_fwnode(sup_handle);
+
if (sup_dev) {
/*
* If it's one of those drivers that don't actually bind to
Thanks,
Saravana
On Mon, Aug 15, 2022 at 3:31 AM Tony Lindgren <[email protected]> wrote:
>
> * Saravana Kannan <[email protected]> [220813 00:30]:
> > On Fri, Aug 12, 2022 at 2:47 AM Tony Lindgren <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > * Saravana Kannan <[email protected]> [220810 05:54]:
> > > > The driver core now:
> > > > - Has the parent device of a supplier pick up the consumers if the
> > > > supplier never has a device created for it.
> > > > - Ignores a supplier if the supplier has no parent device and will never
> > > > be probed by a driver
> > > >
> > > > And already prevents creating a device link with the consumer as a
> > > > supplier of a parent.
> > > >
> > > > So, we no longer need to find the "compatible" node of the supplier or
> > > > do any other checks in of_link_to_phandle(). We simply need to make sure
> > > > that the supplier is available in DT.
> > >
> > > This patch fixes booting for me, so it should be applied as a fix and
> > > tagged with:
> > >
> > > Fixes: 5a46079a9645 ("PM: domains: Delete usage of driver_deferred_probe_check_state()")
> > >
> > > If there are dependencies to the other patches in this series, it might
> > > make sense to revert commit 5a46079a9645 instead.
> >
> > Yes, there are dependencies on the rest of the patches in this series.
> > For linux-next, I think we should pick up this series once we get more
> > Tested-bys.
> >
> > So if 5a46079a9645 is causing any regression in stable branches, we
> > should pick up the revert series [1] instead of this series we are
> > replying to.
>
> Agreed we should apply the reverts in [1] for v6.0-rc series. At least
> several generations of the TI 32-bit ARM SoCs are failing to boot
> otherwise.
Actually I wasn't clear in my earlier email. I meant to say "releases
branches", as in 5.19.xxx and not "stable branches". So for 5.19.xxx
we'd pick up these reverts.
And for v6.0-rc if my other patch series [1] fixes the issue, I'd
rather apply [1] than this series. Because this series is meant to be
temporary (I'll be reverting this in the future).
-Saravana
[1] - https://lore.kernel.org/lkml/[email protected]/
Hello Saravana,
Am Montag, 15. August 2022, 22:56:07 CEST schrieb Saravana Kannan:
> On Mon, Aug 15, 2022 at 12:17 PM Saravana Kannan <[email protected]>
wrote:
> > On Mon, Aug 15, 2022 at 5:39 AM Alexander Stein
> >
> > <[email protected]> wrote:
> > > Hello Saravana,
> > >
> > > Am Mittwoch, 10. August 2022, 08:00:29 CEST schrieb Saravana Kannan:
> > > > Alexander,
> > > >
> > > > This should fix your issue where the power domain device not having a
> > > > compatible property. Can you give it a shot please?
> > >
> > > thanks for the update. Unfortunately this does not work:
> > > > [ 0.774838] PM: Added domain provider from /soc@0/bus@30000000/
> > >
> > > gpc@303a0000/pgc/power-domain@0
> > >
> > > > [ 0.775100] imx-pgc imx-pgc-domain.1: __genpd_dev_pm_attach()
> > > > failed to
> > >
> > > find PM domain: -2
> > >
> > > > [ 0.775324] PM: Added domain provider from /soc@0/bus@30000000/
> > >
> > > gpc@303a0000/pgc/power-domain@2
> > >
> > > > [ 0.775601] PM: Added domain provider from /soc@0/bus@30000000/
> > >
> > > gpc@303a0000/pgc/power-domain@3
> > >
> > > > [ 0.775842] PM: Added domain provider from /soc@0/bus@30000000/
> > >
> > > gpc@303a0000/pgc/power-domain@4
> > >
> > > > [ 0.776642] PM: Added domain provider from /soc@0/bus@30000000/
> > >
> > > gpc@303a0000/pgc/power-domain@7
> > >
> > > > [ 0.776897] PM: Added domain provider from /soc@0/bus@30000000/
> > >
> > > gpc@303a0000/pgc/power-domain@8
> > >
> > > > [ 0.777158] PM: Added domain provider from /soc@0/bus@30000000/
> > >
> > > gpc@303a0000/pgc/power-domain@9
> > >
> > > > [ 0.777405] PM: Added domain provider from /soc@0/bus@30000000/
> > >
> > > gpc@303a0000/pgc/power-domain@a
> > >
> > > > [ 0.779342] genpd genpd:0:38320000.blk-ctrl:
> > > > __genpd_dev_pm_attach()
> > >
> > > failed to find PM domain: -2
> > >
> > > > [ 0.779422] imx8m-blk-ctrl 38320000.blk-ctrl: error -ENODEV: failed
> > > > to
> > >
> > > attach power domain "bus"
> > >
> > > > [ 0.848785] etnaviv-gpu 38000000.gpu: __genpd_dev_pm_attach()
> > > > failed to
> > >
> > > find PM domain: -2
> > >
> > > > [ 1.114220] pfuze100-regulator 0-0008: Full layer: 2, Metal layer:
> > > > 1
> > > > [ 1.122267] pfuze100-regulator 0-0008: FAB: 0, FIN: 0
> > > > [ 1.132970] pfuze100-regulator 0-0008: pfuze100 found.
> > > > [ 1.157011] imx-gpcv2 303a0000.gpc: Failed to create device link
> > > > with
> > >
> > > 0-0008
> > >
> > > > [ 1.164094] imx-gpcv2 303a0000.gpc: Failed to create device link
> > > > with
> > >
> > > 0-0008
> > >
> > > The required power-supply for the power domains is still not yet
> > > available.
> > > Does this series require some other patches as well?
> >
> > Ah sorry, yeah, this needs additional patches. The one I gave in the
> > other thread when I debugged this and I also noticed another issue.
> > Here's the combined diff of what's needed. Can you add this on top of
> > the series and test it?
> >
> > diff --git a/drivers/irqchip/irq-imx-gpcv2.c
> > b/drivers/irqchip/irq-imx-gpcv2.c index b9c22f764b4d..8a0e82067924 100644
> > --- a/drivers/irqchip/irq-imx-gpcv2.c
> > +++ b/drivers/irqchip/irq-imx-gpcv2.c
> > @@ -283,6 +283,7 @@ static int __init imx_gpcv2_irqchip_init(struct
> > device_node *node,
> >
> > * later the GPC power domain driver will not be skipped.
> > */
> >
> > of_node_clear_flag(node, OF_POPULATED);
> >
> > + fwnode_dev_initialized(domain->fwnode, false);
> >
> > return 0;
> >
> > }
> >
> > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > index 6383a4edc360..181fbfe5bd4d 100644
> > --- a/drivers/soc/imx/gpcv2.c
> > +++ b/drivers/soc/imx/gpcv2.c
> > @@ -1513,6 +1513,7 @@ static int imx_gpcv2_probe(struct platform_device
> > *pdev)>
> > pd_pdev->dev.parent = dev;
> > pd_pdev->dev.of_node = np;
> >
> > + pd_pdev->dev.fwnode = of_fwnode_handle(np);
> >
> > ret = platform_device_add(pd_pdev);
> > if (ret) {
> >
> > With this patch, I'd really expect the power domain dependency to be
> > handled correctly.
> >
> > > Whats worse, starting with commit 9/9 [of: property: Simplify
> > > of_link_to_phandle()], other drivers fail to probe waiting for pinctrl
> > > to be available.
> >
> > Heh, Patch 9/9 and all its other dependencies in this series was to
> > fix your use case. Ironic that it's causing you more issues.
> >
> > > > $ cat /sys/kernel/debug/devices_deferred
> > > > gpio-leds platform: wait for supplier gpioledgrp
> > > > extcon-usbotg0 platform: wait for supplier usb0congrp
> > > > gpio-keys platform: wait for supplier gpiobuttongrp
> > > > regulator-otg-vbus platform: wait for supplier reggotgvbusgrp
> > > > regulator-vdd-arm platform: wait for supplier dvfsgrp
> > >
> > > Apparently for some reason they are not probed again, once the pinctrl
> > > driver probed.
> >
> > I'm hoping that this is just some issue due to the missing patch
> > above, but doesn't sound like it if you say that the pinctrl ended up
> > probing eventually.
> >
> > So when device_links_driver_bound() calls
> > __fw_devlink_pickup_dangling_consumers(), it should have picked up the
> > consumers of node like gpiobuttongrp and moved it to the pinctrl
> > device. And right after that we call __fw_devlink_link_to_consumers()
> > that would have created the device links. And then right after that,
> > we go through all the consumers and add them to the deferred probe
> > list. After that deferred probe should have run... either because it's
> > enabled at late_initcall() or because a new device probed
> > successfully.
> >
> > Can you check which one of my expectations isn't true in your case?
>
> Actually I have a hypothesis on what might be happening. It could be a
> case of the consumer device getting added after the supplier has been
> initialized.
>
> If the patch above doesn't fix everything, can you add this diff on
> top of the patch above and see if that fixes everything? If it fixes
> the pinctrl issue, can you check my hypothesis be checking in what
> order the devices get added and get probed?
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 2f012e826986..866755d8ad95 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2068,7 +2068,11 @@ static int fw_devlink_create_devlink(struct device
> *con, device_links_write_unlock();
> }
>
> - sup_dev = get_dev_from_fwnode(sup_handle);
> + if (sup_handle->flags & FWNODE_FLAG_NOT_DEVICE)
> + sup_dev = fwnode_get_next_parent_dev(sup_handle);
> + else
> + sup_dev = get_dev_from_fwnode(sup_handle);
> +
> if (sup_dev) {
> /*
> * If it's one of those drivers that don't actually bind to
>
And with this change my pinctrl probing is fixed as well!
Thanks
Alexander
Hello Saravana,
Am Montag, 15. August 2022, 21:17:36 CEST schrieb Saravana Kannan:
> On Mon, Aug 15, 2022 at 5:39 AM Alexander Stein
>
> <[email protected]> wrote:
> > Hello Saravana,
> >
> > Am Mittwoch, 10. August 2022, 08:00:29 CEST schrieb Saravana Kannan:
> > > Alexander,
> > >
> > > This should fix your issue where the power domain device not having a
> > > compatible property. Can you give it a shot please?
> >
> > thanks for the update. Unfortunately this does not work:
> > > [ 0.774838] PM: Added domain provider from /soc@0/bus@30000000/
> >
> > gpc@303a0000/pgc/power-domain@0
> >
> > > [ 0.775100] imx-pgc imx-pgc-domain.1: __genpd_dev_pm_attach() failed
> > > to
> >
> > find PM domain: -2
> >
> > > [ 0.775324] PM: Added domain provider from /soc@0/bus@30000000/
> >
> > gpc@303a0000/pgc/power-domain@2
> >
> > > [ 0.775601] PM: Added domain provider from /soc@0/bus@30000000/
> >
> > gpc@303a0000/pgc/power-domain@3
> >
> > > [ 0.775842] PM: Added domain provider from /soc@0/bus@30000000/
> >
> > gpc@303a0000/pgc/power-domain@4
> >
> > > [ 0.776642] PM: Added domain provider from /soc@0/bus@30000000/
> >
> > gpc@303a0000/pgc/power-domain@7
> >
> > > [ 0.776897] PM: Added domain provider from /soc@0/bus@30000000/
> >
> > gpc@303a0000/pgc/power-domain@8
> >
> > > [ 0.777158] PM: Added domain provider from /soc@0/bus@30000000/
> >
> > gpc@303a0000/pgc/power-domain@9
> >
> > > [ 0.777405] PM: Added domain provider from /soc@0/bus@30000000/
> >
> > gpc@303a0000/pgc/power-domain@a
> >
> > > [ 0.779342] genpd genpd:0:38320000.blk-ctrl: __genpd_dev_pm_attach()
> >
> > failed to find PM domain: -2
> >
> > > [ 0.779422] imx8m-blk-ctrl 38320000.blk-ctrl: error -ENODEV: failed
> > > to
> >
> > attach power domain "bus"
> >
> > > [ 0.848785] etnaviv-gpu 38000000.gpu: __genpd_dev_pm_attach() failed
> > > to
> >
> > find PM domain: -2
> >
> > > [ 1.114220] pfuze100-regulator 0-0008: Full layer: 2, Metal layer: 1
> > > [ 1.122267] pfuze100-regulator 0-0008: FAB: 0, FIN: 0
> > > [ 1.132970] pfuze100-regulator 0-0008: pfuze100 found.
> > > [ 1.157011] imx-gpcv2 303a0000.gpc: Failed to create device link with
> >
> > 0-0008
> >
> > > [ 1.164094] imx-gpcv2 303a0000.gpc: Failed to create device link with
> >
> > 0-0008
> >
> > The required power-supply for the power domains is still not yet
> > available.
> > Does this series require some other patches as well?
>
> Ah sorry, yeah, this needs additional patches. The one I gave in the
> other thread when I debugged this and I also noticed another issue.
> Here's the combined diff of what's needed. Can you add this on top of
> the series and test it?
>
> diff --git a/drivers/irqchip/irq-imx-gpcv2.c
> b/drivers/irqchip/irq-imx-gpcv2.c index b9c22f764b4d..8a0e82067924 100644
> --- a/drivers/irqchip/irq-imx-gpcv2.c
> +++ b/drivers/irqchip/irq-imx-gpcv2.c
> @@ -283,6 +283,7 @@ static int __init imx_gpcv2_irqchip_init(struct
> device_node *node,
> * later the GPC power domain driver will not be skipped.
> */
> of_node_clear_flag(node, OF_POPULATED);
> + fwnode_dev_initialized(domain->fwnode, false);
> return 0;
> }
>
> diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> index 6383a4edc360..181fbfe5bd4d 100644
> --- a/drivers/soc/imx/gpcv2.c
> +++ b/drivers/soc/imx/gpcv2.c
> @@ -1513,6 +1513,7 @@ static int imx_gpcv2_probe(struct platform_device
> *pdev)
>
> pd_pdev->dev.parent = dev;
> pd_pdev->dev.of_node = np;
> + pd_pdev->dev.fwnode = of_fwnode_handle(np);
>
> ret = platform_device_add(pd_pdev);
> if (ret) {
>
> With this patch, I'd really expect the power domain dependency to be
> handled correctly.
I was out of office so I didn't keep track of any dependencies, sorry.
With these 2 changes above my power domain problem is fixed!
Thanks
Alexander
On Tue, Aug 16, 2022 at 12:17 AM Alexander Stein
<[email protected]> wrote:
>
> Hello Saravana,
>
> Am Montag, 15. August 2022, 22:56:07 CEST schrieb Saravana Kannan:
> > On Mon, Aug 15, 2022 at 12:17 PM Saravana Kannan <[email protected]>
> wrote:
> > > On Mon, Aug 15, 2022 at 5:39 AM Alexander Stein
> > >
> > > <[email protected]> wrote:
> > > > Hello Saravana,
> > > >
> > > > Am Mittwoch, 10. August 2022, 08:00:29 CEST schrieb Saravana Kannan:
> > > > > Alexander,
> > > > >
> > > > > This should fix your issue where the power domain device not having a
> > > > > compatible property. Can you give it a shot please?
> > > >
> > > > thanks for the update. Unfortunately this does not work:
> > > > > [ 0.774838] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@0
> > > >
> > > > > [ 0.775100] imx-pgc imx-pgc-domain.1: __genpd_dev_pm_attach()
> > > > > failed to
> > > >
> > > > find PM domain: -2
> > > >
> > > > > [ 0.775324] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@2
> > > >
> > > > > [ 0.775601] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@3
> > > >
> > > > > [ 0.775842] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@4
> > > >
> > > > > [ 0.776642] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@7
> > > >
> > > > > [ 0.776897] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@8
> > > >
> > > > > [ 0.777158] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@9
> > > >
> > > > > [ 0.777405] PM: Added domain provider from /soc@0/bus@30000000/
> > > >
> > > > gpc@303a0000/pgc/power-domain@a
> > > >
> > > > > [ 0.779342] genpd genpd:0:38320000.blk-ctrl:
> > > > > __genpd_dev_pm_attach()
> > > >
> > > > failed to find PM domain: -2
> > > >
> > > > > [ 0.779422] imx8m-blk-ctrl 38320000.blk-ctrl: error -ENODEV: failed
> > > > > to
> > > >
> > > > attach power domain "bus"
> > > >
> > > > > [ 0.848785] etnaviv-gpu 38000000.gpu: __genpd_dev_pm_attach()
> > > > > failed to
> > > >
> > > > find PM domain: -2
> > > >
> > > > > [ 1.114220] pfuze100-regulator 0-0008: Full layer: 2, Metal layer:
> > > > > 1
> > > > > [ 1.122267] pfuze100-regulator 0-0008: FAB: 0, FIN: 0
> > > > > [ 1.132970] pfuze100-regulator 0-0008: pfuze100 found.
> > > > > [ 1.157011] imx-gpcv2 303a0000.gpc: Failed to create device link
> > > > > with
> > > >
> > > > 0-0008
> > > >
> > > > > [ 1.164094] imx-gpcv2 303a0000.gpc: Failed to create device link
> > > > > with
> > > >
> > > > 0-0008
> > > >
> > > > The required power-supply for the power domains is still not yet
> > > > available.
> > > > Does this series require some other patches as well?
> > >
> > > Ah sorry, yeah, this needs additional patches. The one I gave in the
> > > other thread when I debugged this and I also noticed another issue.
> > > Here's the combined diff of what's needed. Can you add this on top of
> > > the series and test it?
> > >
> > > diff --git a/drivers/irqchip/irq-imx-gpcv2.c
> > > b/drivers/irqchip/irq-imx-gpcv2.c index b9c22f764b4d..8a0e82067924 100644
> > > --- a/drivers/irqchip/irq-imx-gpcv2.c
> > > +++ b/drivers/irqchip/irq-imx-gpcv2.c
> > > @@ -283,6 +283,7 @@ static int __init imx_gpcv2_irqchip_init(struct
> > > device_node *node,
> > >
> > > * later the GPC power domain driver will not be skipped.
> > > */
> > >
> > > of_node_clear_flag(node, OF_POPULATED);
> > >
> > > + fwnode_dev_initialized(domain->fwnode, false);
> > >
> > > return 0;
> > >
> > > }
> > >
> > > diff --git a/drivers/soc/imx/gpcv2.c b/drivers/soc/imx/gpcv2.c
> > > index 6383a4edc360..181fbfe5bd4d 100644
> > > --- a/drivers/soc/imx/gpcv2.c
> > > +++ b/drivers/soc/imx/gpcv2.c
> > > @@ -1513,6 +1513,7 @@ static int imx_gpcv2_probe(struct platform_device
> > > *pdev)>
> > > pd_pdev->dev.parent = dev;
> > > pd_pdev->dev.of_node = np;
> > >
> > > + pd_pdev->dev.fwnode = of_fwnode_handle(np);
> > >
> > > ret = platform_device_add(pd_pdev);
> > > if (ret) {
> > >
> > > With this patch, I'd really expect the power domain dependency to be
> > > handled correctly.
> > >
> > > > Whats worse, starting with commit 9/9 [of: property: Simplify
> > > > of_link_to_phandle()], other drivers fail to probe waiting for pinctrl
> > > > to be available.
> > >
> > > Heh, Patch 9/9 and all its other dependencies in this series was to
> > > fix your use case. Ironic that it's causing you more issues.
> > >
> > > > > $ cat /sys/kernel/debug/devices_deferred
> > > > > gpio-leds platform: wait for supplier gpioledgrp
> > > > > extcon-usbotg0 platform: wait for supplier usb0congrp
> > > > > gpio-keys platform: wait for supplier gpiobuttongrp
> > > > > regulator-otg-vbus platform: wait for supplier reggotgvbusgrp
> > > > > regulator-vdd-arm platform: wait for supplier dvfsgrp
> > > >
> > > > Apparently for some reason they are not probed again, once the pinctrl
> > > > driver probed.
> > >
> > > I'm hoping that this is just some issue due to the missing patch
> > > above, but doesn't sound like it if you say that the pinctrl ended up
> > > probing eventually.
> > >
> > > So when device_links_driver_bound() calls
> > > __fw_devlink_pickup_dangling_consumers(), it should have picked up the
> > > consumers of node like gpiobuttongrp and moved it to the pinctrl
> > > device. And right after that we call __fw_devlink_link_to_consumers()
> > > that would have created the device links. And then right after that,
> > > we go through all the consumers and add them to the deferred probe
> > > list. After that deferred probe should have run... either because it's
> > > enabled at late_initcall() or because a new device probed
> > > successfully.
> > >
> > > Can you check which one of my expectations isn't true in your case?
> >
> > Actually I have a hypothesis on what might be happening. It could be a
> > case of the consumer device getting added after the supplier has been
> > initialized.
> >
> > If the patch above doesn't fix everything, can you add this diff on
> > top of the patch above and see if that fixes everything? If it fixes
> > the pinctrl issue, can you check my hypothesis be checking in what
> > order the devices get added and get probed?
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 2f012e826986..866755d8ad95 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -2068,7 +2068,11 @@ static int fw_devlink_create_devlink(struct device
> > *con, device_links_write_unlock();
> > }
> >
> > - sup_dev = get_dev_from_fwnode(sup_handle);
> > + if (sup_handle->flags & FWNODE_FLAG_NOT_DEVICE)
> > + sup_dev = fwnode_get_next_parent_dev(sup_handle);
> > + else
> > + sup_dev = get_dev_from_fwnode(sup_handle);
> > +
> > if (sup_dev) {
> > /*
> > * If it's one of those drivers that don't actually bind to
> >
>
> And with this change my pinctrl probing is fixed as well!
Thanks for testing these! I'll roll these into v2 of the series.
Glad to see I've fixed all the issues I set out to fix. Now to figure
out what other corner cases I've missed.
-Saravana
Hi Saravana,
On Wed, Aug 10, 2022 at 8:00 AM Saravana Kannan <[email protected]> wrote:
> This patch series improves fw_devlink in the following ways:
>
> 1. It no longer cares about a fwnode having a "compatible" property. It
> figures this our more dynamically. The only expectation is that
> fwnode that are converted to devices actually get probed by a driver
> for the dependencies to be enforced correctly.
>
> 2. Finer grained dependency tracking. fw_devlink will now create device
> links from the consumer to the actual resource's device (if it has one,
> Eg: gpio_device) instead of the parent supplier device. This improves
> things like async suspend/resume ordering, potentially remove the need
> for frameworks to create device links, more parallelized async probing,
> and better sync_state() tracking.
>
> 3. Handle hardware/software quirks where a child firmware node gets
> populated as a device before its parent firmware node AND actually
> supplies a non-optional resource to the parent firmware node's
> device.
>
> 4. Way more robust at cycle handling (see patch for the insane cases).
>
> 5. Stops depending on OF_POPULATED to figure out some corner cases.
>
> 6. Simplifies the work that needs to be done by the firmware specific
> code.
>
> This took way too long to get done due to typo bugs I had in my rewrite or
> corner cases I had to find and handle. But it's fairly well tested at this
> point and I expect this to work properly.
>
> Abel & Doug,
>
> This should fix your cyclic dependency issues with your display. Can you
> give it a shot please?
Thanks for to your series!
> Geert,
>
> Can you test the renesas stuff I changed please? They should continue
> working like before. Any other sanity test on other hardware would be
> great too.
At first, this series looked like a total disaster, introducing
several regressions[1].
However, after applying the additional fix from
https://lore.kernel.org/lkml/CAGETcx-JUV1nj8wBJrTPfyvM7=Mre5j_vkVmZojeiumUGG6QZQ@mail.gmail.com
all regressions[1] went away, and /sys/kernel/debug/devices_deferred
is empty again.
Note that the Ethernet PHY interrupt on Koelsch is not fixed, so the issue from
https://lore.kernel.org/all/CAMuHMdWo_wRwV-i_iyTxVnEsf3Th9GBAG+wxUQMQGnw1t2ijTg@mail.gmail.com/
is still present.
Summary: while this series (+ the additional fix) doesn't seem to
introduce any regressions on Renesas ARM platforms, it doesn't seem
to fix anything for me neither.
Tested-by: Geert Uytterhoeven <[email protected]>
[1]
R-Mobile APE6 (ape6evm):
No "deferred probe pending" messages?
# cat /sys/kernel/debug/devices_deferred
ee120000.mmc platform: wait for supplier sd1
ee100000.mmc platform: wait for supplier sd0
ee200000.mmc platform: wait for supplier mmc0
keyboard platform: wait for supplier keyboard
R-Mobile A1 (armadillo-800-eva):
No soundcards found.
platform backlight: deferred probe pending
i2c 0-0030: deferred probe pending (RTC)
platform sound: deferred probe pending
platform fe1f0000.sound: deferred probe pending
platform keyboard: deferred probe pending (gpio-keys)
platform e6850000.mmc: deferred probe pending (SDHI)
platform e6bd0000.mmc: deferred probe pending (SDHI)
# cat /sys/kernel/debug/devices_deferred
backlight platform: wait for supplier backlight
0-0030 i2c: wait for supplier rtc
sound asoc-simple-card: parse error
fe1f0000.sound platform: wait for supplier sounda
keyboard platform: wait for supplier keyboard
e6850000.mmc platform: wait for supplier sd0
e6bd0000.mmc platform: wait for supplier mmc0
SH-Mobile AG5 (kzm9g):
No soundcards found.
platform sound: deferred probe pending
platform ec230000.sound: deferred probe pending
# cat /sys/kernel/debug/devices_deferred
sound asoc-simple-card: parse error
ec230000.sound platform: wait for supplier sounda
Note: This is the only board where gpio-keys still works!
R-Car M2-W (koelsch):
i2c-demux-pinctrl i2c-12: failed to setup demux-adapter 0 (-19)
i2c-demux-pinctrl i2c-13: failed to setup demux-adapter 0 (-19)
i2c-demux-pinctrl i2c-14: failed to setup demux-adapter 0 (-19)
i2c-demux-pinctrl i2c-13: Failed to create device link with hdmi-in
i2c-demux-pinctrl i2c-13: Failed to create device link with hdmi-out
No soundcards found.
Some I2C-busses are missing, but not listed in
/sys/kernel/debug/devices_deferred?
/devices/platform/soc/e6518000.i2c
/devices/platform/soc/e6530000.i2c
/devices/platform/soc/e6520000.i2c
platform keyboard: deferred probe pending
platform sound: deferred probe pending
platform feb00000.display: deferred probe pending
# cat /sys/kernel/debug/devices_deferred
keyboard platform: wait for supplier keyboard
sound asoc-simple-card: parse error
feb00000.display rcar-du: failed to initialize DRM/KMS
R-Car Gen3 (Salvator-X(S), Ebisu)
platform keys: deferred probe pending (gpio-keys)
# cat /sys/kernel/debug/devices_deferred
keys platform: wait for supplier keys
RZ/A (rskrza1, rza2mevb)
No "deferred probe pending" messages?
# cat /sys/kernel/debug/devices_deferred
keyboard platform: wait for supplier keyboard
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi,
* Saravana Kannan <[email protected]> [220815 18:16]:
> On Mon, Aug 15, 2022 at 3:33 AM Tony Lindgren <[email protected]> wrote:
> >
> > * Saravana Kannan <[email protected]> [220813 00:45]:
> > > On Fri, Aug 12, 2022 at 2:49 AM Tony Lindgren <[email protected]> wrote:
> > > >
> > > > * Saravana Kannan <[email protected]> [220810 05:54]:
> > > > > Tony,
> > > > >
> > > > > This should handle the odd case of the child being the supplier of the
> > > > > parent. Can you please give this a shot? I want to make sure the cycle
> > > > > detection code handles this properly and treats it like it's NOT a cycle.
> > > >
> > > > Yup, this series works for me, so feel free to add:
> > > >
> > > > Tested-by: Tony Lindgren <[email protected]>
> > >
> > > Thanks for testing!
> > >
> > > Btw, out of curiosity, how many different boards did you test this on?
> > > IIRC you had an issue only in one board, right? Not to say I didn't
> > > break anything else, I'm just trying to see how much confidence we
> > > have on this series so far. I'm hoping the rest of the folks I listed
> > > in the email will get around to testing this series.
> >
> > Sorry if I was not clear earlier. The issue affects several generations
> > of TI 32-bit SoCs at least, not just one board.
>
> But this series fixes the issues for all of them or are you still
> seeing some broken boot with this series?
Yes. However, I'm now getting confused what exactly you're proposing to fix
the regressions for v6.0-rc series.
I'd like to see just the fixes series for v6.0-rc series. With proper fixes
tags, and possibly reverts.
Then discussing patches for Linux next can be done based on the fixes :)
Regards,
Tony
On Thu, Aug 18, 2022 at 10:05:14AM +0300, Tony Lindgren wrote:
> Hi,
>
> * Saravana Kannan <[email protected]> [220815 18:16]:
> > On Mon, Aug 15, 2022 at 3:33 AM Tony Lindgren <[email protected]> wrote:
> > >
> > > * Saravana Kannan <[email protected]> [220813 00:45]:
> > > > On Fri, Aug 12, 2022 at 2:49 AM Tony Lindgren <[email protected]> wrote:
> > > > >
> > > > > * Saravana Kannan <[email protected]> [220810 05:54]:
> > > > > > Tony,
> > > > > >
> > > > > > This should handle the odd case of the child being the supplier of the
> > > > > > parent. Can you please give this a shot? I want to make sure the cycle
> > > > > > detection code handles this properly and treats it like it's NOT a cycle.
> > > > >
> > > > > Yup, this series works for me, so feel free to add:
> > > > >
> > > > > Tested-by: Tony Lindgren <[email protected]>
> > > >
> > > > Thanks for testing!
> > > >
> > > > Btw, out of curiosity, how many different boards did you test this on?
> > > > IIRC you had an issue only in one board, right? Not to say I didn't
> > > > break anything else, I'm just trying to see how much confidence we
> > > > have on this series so far. I'm hoping the rest of the folks I listed
> > > > in the email will get around to testing this series.
> > >
> > > Sorry if I was not clear earlier. The issue affects several generations
> > > of TI 32-bit SoCs at least, not just one board.
> >
> > But this series fixes the issues for all of them or are you still
> > seeing some broken boot with this series?
>
> Yes. However, I'm now getting confused what exactly you're proposing to fix
> the regressions for v6.0-rc series.
So am I :(
> I'd like to see just the fixes series for v6.0-rc series. With proper fixes
> tags, and possibly reverts.
Agreed, that would help out a lot here.
> Then discussing patches for Linux next can be done based on the fixes :)
Agreed.
I'll drop this whole series from my queue now and wait for a new one.
thanks,
greg k-h