[PATCH 0/2] Clarify DM_FLAG_PROBE_AFTER_BIND behaviour

[PATCH 0/2] Clarify DM_FLAG_PROBE_AFTER_BIND behaviour

[Caleb Connolly]

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)

[PATCH 1/2] dm: clarify DM_FLAG_PROBE_AFTER_BIND behaviour

[Caleb Connolly]

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

Re: [PATCH 1/2] dm: clarify DM_FLAG_PROBE_AFTER_BIND behaviour

[Simon Glass]

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

[PATCH 2/2] drivers: remove bogus DM_FLAG_PROBE_AFTER_BIND flags

[Caleb Connolly]

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

Re: [PATCH 2/2] drivers: remove bogus DM_FLAG_PROBE_AFTER_BIND flags

[Simon Glass]

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.

Re: [PATCH 2/2] drivers: remove bogus DM_FLAG_PROBE_AFTER_BIND flags

[Caleb Connolly]

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

-- 
// Caleb (they/them)

Re: [PATCH 2/2] drivers: remove bogus DM_FLAG_PROBE_AFTER_BIND flags

[Simon Glass]

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

Re: [PATCH 0/2] Clarify DM_FLAG_PROBE_AFTER_BIND behaviour

[Michal Simek]

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).
> 
> [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)
> 

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

Re: [PATCH 0/2] Clarify DM_FLAG_PROBE_AFTER_BIND behaviour

[Tom Rini]

On Fri, 17 Jan 2025 08:28:55 +0100, 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).
> 
> [...]

Applied to u-boot/next, thanks!

[1/2] dm: clarify DM_FLAG_PROBE_AFTER_BIND behaviour
      commit: 8a0e6212bb5ef547712e922a41555eb7d1aee1ab
[2/2] drivers: remove bogus DM_FLAG_PROBE_AFTER_BIND flags
      commit: 44450434ea9f0f2e0d8ddfd853f4990c1719b4aa
-- 
Tom