[PATCH 0/2] Clarify DM_FLAG_PROBE_AFTER_BIND behaviour
In Simons series reworking autoprobe[1], a discussion came up about DM_FLAG_PROBE_AFTER_BIND, specifically that it wasn't very clear where this flag should be used.
This series implements my suggestions made there to clarify the use of this flag, and fixup the two driver which erroneously apply it to their driver struct (this does nothing).
[1]: https://lore.kernel.org/u-boot/20241120153642.861633-1-sjg@chromium.org/
--- Caleb Connolly (2): dm: clarify DM_FLAG_PROBE_AFTER_BIND behaviour drivers: remove bogus DM_FLAG_PROBE_AFTER_BIND flags
doc/develop/driver-model/design.rst | 6 ++++-- drivers/mailbox/zynqmp-ipi.c | 1 - drivers/watchdog/da9063-wdt.c | 1 - include/dm/device.h | 5 ++++- 4 files changed, 8 insertions(+), 5 deletions(-) --- base-commit: e3f716fa2d58aadf53928475ee7e88eb41cb8031
// Caleb (they/them)
The DM_FLAG_PROBE_AFTER_BIND flag only makes sense on a per-device basis, however recently added documentation as well as some confused drivers imply that it might be added to a driver definition, this does nothing.
Clarify the new documentation and expand on the comment by the definition to point people in the right direction.
Signed-off-by: Caleb Connolly caleb.connolly@linaro.org --- Anecdotally, I've spent several hours being confused about this flag, this information would have helped me greatly. --- doc/develop/driver-model/design.rst | 6 ++++-- include/dm/device.h | 5 ++++- 2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/doc/develop/driver-model/design.rst b/doc/develop/driver-model/design.rst index 92f638a02047..30093737200c 100644 --- a/doc/develop/driver-model/design.rst +++ b/doc/develop/driver-model/design.rst @@ -842,10 +842,12 @@ steps (see device_probe()): cause the uclass to do some housekeeping to record the device as activated and 'known' by the uclass.
For some platforms, certain devices must be probed to get the platform into -a working state. To help with this, drivers marked with DM_FLAG_PROBE_AFTER_BIND -will be probed immediately after all devices are bound. For now, this happens in +a working state. To help with this, devices marked with DM_FLAG_PROBE_AFTER_BIND +will be probed immediately after all devices are bound. This flag must be set +on the device in its ``bind()`` function with +``dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND)``. For now, this happens in SPL, before relocation and after relocation. See the call to ``dm_autoprobe()`` for where this is done.
The auto-probe feature is tricky because it bypasses the normal ordering of diff --git a/include/dm/device.h b/include/dm/device.h index add67f9ec06f..678cd83c2716 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -80,9 +80,12 @@ struct driver_info; * e.g. for clock, which need to be active during the device-removal phase. */ #define DM_FLAG_VITAL (1 << 14)
-/* Device must be probed after it was bound */ +/* Device must be probed after it was bound. This flag is per-device and does + * nothing if set on a U_BOOT_DRIVER() definition. Apply it with + * dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND) in the devices bind function. + */ #define DM_FLAG_PROBE_AFTER_BIND (1 << 15)
/* * One or multiple of these flags are passed to device_remove() so that
Hi Caleb,
On Fri, 17 Jan 2025 at 00:29, Caleb Connolly caleb.connolly@linaro.org wrote:
The DM_FLAG_PROBE_AFTER_BIND flag only makes sense on a per-device basis, however recently added documentation as well as some confused drivers imply that it might be added to a driver definition, this does nothing.
Clarify the new documentation and expand on the comment by the definition to point people in the right direction.
Signed-off-by: Caleb Connolly caleb.connolly@linaro.org
Anecdotally, I've spent several hours being confused about this flag, this information would have helped me greatly.
doc/develop/driver-model/design.rst | 6 ++++-- include/dm/device.h | 5 ++++- 2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/doc/develop/driver-model/design.rst
b/doc/develop/driver-model/design.rst
index 92f638a02047..30093737200c 100644 --- a/doc/develop/driver-model/design.rst +++ b/doc/develop/driver-model/design.rst @@ -842,10 +842,12 @@ steps (see device_probe()): cause the uclass to do some housekeeping to record the device as activated and 'known' by the uclass.
For some platforms, certain devices must be probed to get the platform
into
-a working state. To help with this, drivers marked with
DM_FLAG_PROBE_AFTER_BIND
-will be probed immediately after all devices are bound. For now, this
happens in
+a working state. To help with this, devices marked with
DM_FLAG_PROBE_AFTER_BIND
+will be probed immediately after all devices are bound. This flag must
be set
+on the device in its ``bind()`` function with +``dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND)``. For now, this happens in SPL, before relocation and after relocation. See the call to
``dm_autoprobe()``
for where this is done.
The auto-probe feature is tricky because it bypasses the normal ordering
of
diff --git a/include/dm/device.h b/include/dm/device.h index add67f9ec06f..678cd83c2716 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -80,9 +80,12 @@ struct driver_info;
- e.g. for clock, which need to be active during the device-removal
phase.
*/ #define DM_FLAG_VITAL (1 << 14)
-/* Device must be probed after it was bound */ +/* Device must be probed after it was bound. This flag is per-device and
does
- nothing if set on a U_BOOT_DRIVER() definition. Apply it with
- dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND) in the devices bind
function.
- */
#define DM_FLAG_PROBE_AFTER_BIND (1 << 15)
/*
- One or multiple of these flags are passed to device_remove() so that
-- 2.48.0
Reviewed-by: Simon Glass sjg@chromium.org
Any interest in writing a test for this feature? We have already found a few bugs.
Regards, Simon
Some drivers set DM_FLAG_PROBE_AFTER_BIND, this does nothing since it's only every applied on a per-device basis.
Remove the flags.
Signed-off-by: Caleb Connolly caleb.connolly@linaro.org --- drivers/mailbox/zynqmp-ipi.c | 1 - drivers/watchdog/da9063-wdt.c | 1 - 2 files changed, 2 deletions(-)
diff --git a/drivers/mailbox/zynqmp-ipi.c b/drivers/mailbox/zynqmp-ipi.c index 713d93a200c4..851aa737c03e 100644 --- a/drivers/mailbox/zynqmp-ipi.c +++ b/drivers/mailbox/zynqmp-ipi.c @@ -269,6 +269,5 @@ U_BOOT_DRIVER(zynqmp_ipi) = { .name = "zynqmp_ipi", .id = UCLASS_NOP, .of_match = zynqmp_ipi_ids, .probe = zynqmp_ipi_probe, - .flags = DM_FLAG_PROBE_AFTER_BIND, }; diff --git a/drivers/watchdog/da9063-wdt.c b/drivers/watchdog/da9063-wdt.c index b7216b578630..ec9bc0330114 100644 --- a/drivers/watchdog/da9063-wdt.c +++ b/drivers/watchdog/da9063-wdt.c @@ -144,6 +144,5 @@ U_BOOT_DRIVER(da9063_wdt) = { .name = "da9063-wdt", .id = UCLASS_WDT, .of_match = da9063_wdt_ids, .ops = &da9063_wdt_ops, - .flags = DM_FLAG_PROBE_AFTER_BIND, };
On Fri, 17 Jan 2025 at 00:29, Caleb Connolly caleb.connolly@linaro.org wrote:
Some drivers set DM_FLAG_PROBE_AFTER_BIND, this does nothing since it's only every applied on a per-device basis.
Remove the flags.
Signed-off-by: Caleb Connolly caleb.connolly@linaro.org
drivers/mailbox/zynqmp-ipi.c | 1 - drivers/watchdog/da9063-wdt.c | 1 - 2 files changed, 2 deletions(-)
diff --git a/drivers/mailbox/zynqmp-ipi.c b/drivers/mailbox/zynqmp-ipi.c index 713d93a200c4..851aa737c03e 100644 --- a/drivers/mailbox/zynqmp-ipi.c +++ b/drivers/mailbox/zynqmp-ipi.c @@ -269,6 +269,5 @@ U_BOOT_DRIVER(zynqmp_ipi) = { .name = "zynqmp_ipi", .id = UCLASS_NOP, .of_match = zynqmp_ipi_ids, .probe = zynqmp_ipi_probe,
.flags = DM_FLAG_PROBE_AFTER_BIND,}; diff --git a/drivers/watchdog/da9063-wdt.c b/drivers/watchdog/da9063-wdt.c index b7216b578630..ec9bc0330114 100644 --- a/drivers/watchdog/da9063-wdt.c +++ b/drivers/watchdog/da9063-wdt.c @@ -144,6 +144,5 @@ U_BOOT_DRIVER(da9063_wdt) = { .name = "da9063-wdt", .id = UCLASS_WDT, .of_match = da9063_wdt_ids, .ops = &da9063_wdt_ops,
.flags = DM_FLAG_PROBE_AFTER_BIND,};
-- 2.48.0
Reviewed-by: Simon Glass sjg@chromium.org
It is actually a bit unclear, since the comments for struct driver mention DM_FLAG_... for the flags.
On 18/01/2025 05:32, Simon Glass wrote:
On Fri, 17 Jan 2025 at 00:29, Caleb Connolly <caleb.connolly@linaro.org mailto:caleb.connolly@linaro.org> wrote:
Some drivers set DM_FLAG_PROBE_AFTER_BIND, this does nothing since it's only every applied on a per-device basis.
Remove the flags.
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org
mailto:caleb.connolly@linaro.org>
drivers/mailbox/zynqmp-ipi.c | 1 - drivers/watchdog/da9063-wdt.c | 1 - 2 files changed, 2 deletions(-)
diff --git a/drivers/mailbox/zynqmp-ipi.c b/drivers/mailbox/zynqmp-ipi.c index 713d93a200c4..851aa737c03e 100644 --- a/drivers/mailbox/zynqmp-ipi.c +++ b/drivers/mailbox/zynqmp-ipi.c @@ -269,6 +269,5 @@ U_BOOT_DRIVER(zynqmp_ipi) = { .name = "zynqmp_ipi", .id = UCLASS_NOP, .of_match = zynqmp_ipi_ids, .probe = zynqmp_ipi_probe, - .flags = DM_FLAG_PROBE_AFTER_BIND, }; diff --git a/drivers/watchdog/da9063-wdt.c b/drivers/watchdog/da9063-wdt.c index b7216b578630..ec9bc0330114 100644 --- a/drivers/watchdog/da9063-wdt.c +++ b/drivers/watchdog/da9063-wdt.c @@ -144,6 +144,5 @@ U_BOOT_DRIVER(da9063_wdt) = { .name = "da9063-wdt", .id = UCLASS_WDT, .of_match = da9063_wdt_ids, .ops = &da9063_wdt_ops, - .flags = DM_FLAG_PROBE_AFTER_BIND, };
-- 2.48.0
Reviewed-by: Simon Glass <sjg@chromium.org mailto:sjg@chromium.org>
It is actually a bit unclear, since the comments for struct driver mention DM_FLAG_... for the flags.
Yep, some of the flags (most even?) can be applied to drivers and there is no proper distinction between which apply to drivers and which to devices.
DRIVER_FLAG_... and DEV_FLAG_... or something maybe better, something to revisit in the future for sure.
Thx for the review
Hi Caleb,
On Tue, 21 Jan 2025 at 05:00, Caleb Connolly caleb.connolly@linaro.org wrote:
On 18/01/2025 05:32, Simon Glass wrote:
On Fri, 17 Jan 2025 at 00:29, Caleb Connolly <caleb.connolly@linaro.org mailto:caleb.connolly@linaro.org> wrote:
Some drivers set DM_FLAG_PROBE_AFTER_BIND, this does nothing since it's only every applied on a per-device basis.
Remove the flags.
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org
mailto:caleb.connolly@linaro.org>
drivers/mailbox/zynqmp-ipi.c | 1 - drivers/watchdog/da9063-wdt.c | 1 - 2 files changed, 2 deletions(-)
diff --git a/drivers/mailbox/zynqmp-ipi.c b/drivers/mailbox/zynqmp-ipi.c index 713d93a200c4..851aa737c03e 100644 --- a/drivers/mailbox/zynqmp-ipi.c +++ b/drivers/mailbox/zynqmp-ipi.c @@ -269,6 +269,5 @@ U_BOOT_DRIVER(zynqmp_ipi) = { .name = "zynqmp_ipi", .id = UCLASS_NOP, .of_match = zynqmp_ipi_ids, .probe = zynqmp_ipi_probe,
.flags = DM_FLAG_PROBE_AFTER_BIND,}; diff --git a/drivers/watchdog/da9063-wdt.c b/drivers/watchdog/da9063-wdt.c index b7216b578630..ec9bc0330114 100644 --- a/drivers/watchdog/da9063-wdt.c +++ b/drivers/watchdog/da9063-wdt.c @@ -144,6 +144,5 @@ U_BOOT_DRIVER(da9063_wdt) = { .name = "da9063-wdt", .id = UCLASS_WDT, .of_match = da9063_wdt_ids, .ops = &da9063_wdt_ops,
.flags = DM_FLAG_PROBE_AFTER_BIND,};
-- 2.48.0
Reviewed-by: Simon Glass <sjg@chromium.org mailto:sjg@chromium.org>
It is actually a bit unclear, since the comments for struct driver mention DM_FLAG_... for the flags.
Yep, some of the flags (most even?) can be applied to drivers and there is no proper distinction between which apply to drivers and which to devices.
I would rather have one set of flags if we can.
There are flags which only affect binding so perhaps they should be at the start of the enum, with flags which only affect the device after that.
But the original reason for not setting a flag like DM_FLAG_PROBE_AFTER_BIND in the driver was that the driver wanted to control whether to set it in the device.
DRIVER_FLAG_... and DEV_FLAG_... or something maybe better, something to revisit in the future for sure.
Thx for the review
Regards, SImon
On 1/17/25 08:28, Caleb Connolly wrote:
In Simons series reworking autoprobe[1], a discussion came up about DM_FLAG_PROBE_AFTER_BIND, specifically that it wasn't very clear where this flag should be used.
This series implements my suggestions made there to clarify the use of this flag, and fixup the two driver which erroneously apply it to their driver struct (this does nothing).
Caleb Connolly (2): dm: clarify DM_FLAG_PROBE_AFTER_BIND behaviour drivers: remove bogus DM_FLAG_PROBE_AFTER_BIND flags
doc/develop/driver-model/design.rst | 6 ++++-- drivers/mailbox/zynqmp-ipi.c | 1 - drivers/watchdog/da9063-wdt.c | 1 - include/dm/device.h | 5 ++++- 4 files changed, 8 insertions(+), 5 deletions(-)
base-commit: e3f716fa2d58aadf53928475ee7e88eb41cb8031
// Caleb (they/them)
We can't se any issue on our HW when this is applied.
That's why Acked-by: Michal Simek michal.simek@amd.com
Thanks, Michal
participants (3)
-
Caleb Connolly -
Michal Simek -
Simon Glass