[PATCH V2] dm: core: Add late driver remove option

[Marek Vasut]

On 8/7/20 6:23 PM, Simon Glass wrote:

[...]

>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
>> index efdb0f2905..07b241b6bb 100644
>> --- a/drivers/core/device-remove.c
>> +++ b/drivers/core/device-remove.c
>> @@ -172,14 +172,19 @@ int device_remove(struct udevice *dev, uint flags)
>>         drv = dev->driver;
>>         assert(drv);
>>
>> -       ret = uclass_pre_remove_device(dev);
>> -       if (ret)
>> -               return ret;
>> +       if (!(flags & DM_REMOVE_LATE)) {
>> +               ret = uclass_pre_remove_device(dev);
>> +               if (ret)
>> +                       return ret;
>> +       }
>>
>>         ret = device_chld_remove(dev, NULL, flags);
>>         if (ret)
>>                 goto err;
>>
>> +       if (!(flags & DM_REMOVE_LATE) && (drv->flags & DM_FLAG_REMOVE_LATE))
>> +               return 0;
> 
> Why return here?

If the DM isn't in a late-remove phase and the driver has remove-late
flag, then the driver should not be removed yet. This is the case for
e.g. a clock driver.

> Shouldn;'t you use flags_remove() ?

Please explain ?

>>         /*
>>          * Remove the device if called with the "normal" remove flag set,
>>          * or if the remove flag matches any of the drivers remove flags
>> diff --git a/drivers/core/root.c b/drivers/core/root.c
>> index 0726be6b79..21f3054125 100644
>> --- a/drivers/core/root.c
>> +++ b/drivers/core/root.c
>> @@ -168,6 +168,7 @@ int dm_init(bool of_live)
>>  int dm_uninit(void)
>>  {
>>         device_remove(dm_root(), DM_REMOVE_NORMAL);
>> +       device_remove(dm_root(), DM_REMOVE_LATE);
>>         device_unbind(dm_root());
>>         gd->dm_root = NULL;
>>
>> @@ -393,6 +394,7 @@ struct acpi_ops root_acpi_ops = {
>>  U_BOOT_DRIVER(root_driver) = {
>>         .name   = "root_driver",
>>         .id     = UCLASS_ROOT,
>> +       .flags  = DM_FLAG_REMOVE_LATE,
> 
> Why are you changing this flag for the root device?

Because root device should only be removed last.

>>         ACPI_OPS_PTR(&root_acpi_ops)
>>  };
>>
>> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
>> index c3f1b73cd6..ac474d3ff8 100644
>> --- a/drivers/core/uclass.c
>> +++ b/drivers/core/uclass.c
>> @@ -104,10 +104,28 @@ fail_mem:
>>         return ret;
>>  }
>>
>> -int uclass_destroy(struct uclass *uc)
>> +int uclass_find_device_by_drv_flag(struct uclass *uc, const unsigned int flag,
>> +                                  const bool neg, struct udevice **devp)
> 
> Is this intended to be exported? If so, please add a test. If not,
> please make it static. In any case, it needs a full comment as to what
> it does, and args.

Its internal function, should be static.

>> +{
>> +       struct udevice *dev;
>> +
>> +       *devp = NULL;
>> +       uclass_foreach_dev(dev, uc) {
>> +               if ((neg && (dev->driver->flags & flag)) ||
>> +                   (!neg && !(dev->driver->flags & flag))) {
>> +                       *devp = dev;
>> +                       return 0;
>> +               }
>> +       }
>> +
>> +       return -ENODEV;
>> +}
>> +
>> +int uclass_destroy(struct uclass *uc, unsigned int flag)
>>  {
>>         struct uclass_driver *uc_drv;
>>         struct udevice *dev;
>> +       bool late = flag & DM_REMOVE_LATE;
>>         int ret;
>>
>>         /*
>> @@ -116,15 +134,15 @@ int uclass_destroy(struct uclass *uc)
>>          * unbound (by the recursion in the call to device_unbind() below).
>>          * We can loop until the list is empty.
>>          */
>> -       while (!list_empty(&uc->dev_head)) {
>> -               dev = list_first_entry(&uc->dev_head, struct udevice,
>> -                                      uclass_node);
>> -               ret = device_remove(dev, DM_REMOVE_NORMAL | DM_REMOVE_NO_PD);
>> -               if (ret)
>> +       while (!uclass_find_device_by_drv_flag(uc, DM_FLAG_REMOVE_LATE, late, &dev)) {
>> +               ret = device_remove(dev, flag | DM_REMOVE_NO_PD);
> 
> What happened to DM_REMOVE_NORMAL?

See above, bool late, this uclass_destroy() is called twice now. Once
for normal devices, once for late devices, so that the ordering is correct.

>> +               if (ret) {
>>                         return log_msg_ret("remove", ret);
>> +               }
>>                 ret = device_unbind(dev);
>> -               if (ret)
>> +               if (ret) {
>>                         return log_msg_ret("unbind", ret);
>> +               }
> 
> Don't need {}
> 
>>         }
>>
>>         uc_drv = uc->uc_drv;
>> diff --git a/include/dm/device.h b/include/dm/device.h
>> index 953706cf52..7b1db252bf 100644
>> --- a/include/dm/device.h
>> +++ b/include/dm/device.h
>> @@ -73,6 +73,8 @@ struct driver_info;
>>   */
>>  #define DM_FLAG_REMOVE_WITH_PD_ON      (1 << 13)
>>
>> +#define DM_FLAG_REMOVE_LATE            (1 << 14)
> 
> Needs a comment as to what it means.

It means the driver should be removed after all the non-late drivers.

>> +
>>  /*
>>   * One or multiple of these flags are passed to device_remove() so that
>>   * a selective device removal as specified by the remove-stage and the
>> @@ -95,6 +97,8 @@ enum {
>>
>>         /* Don't power down any attached power domains */
>>         DM_REMOVE_NO_PD         = 1 << 1,
>> +
>> +       DM_REMOVE_LATE          = 1 << 2,
> 
> Comment
> 
>>  };
>>
>>  /**
>> diff --git a/include/dm/uclass-internal.h b/include/dm/uclass-internal.h
>> index 6e3f15c2b0..b5926b0f5c 100644
>> --- a/include/dm/uclass-internal.h
>> +++ b/include/dm/uclass-internal.h
>> @@ -247,8 +247,9 @@ struct uclass *uclass_find(enum uclass_id key);
>>   * Destroy a uclass and all its devices
>>   *
>>   * @uc: uclass to destroy
>> + * @flag: driver flags (DM_REMOVE_NORMAL or DM_REMOVE_LATE)
>>   * @return 0 on success, -ve on error
>>   */
>> -int uclass_destroy(struct uclass *uc);
>> +int uclass_destroy(struct uclass *uc, unsigned int flag);
> 
> Why is the flag added here? Does it mean that sometimes it will not
> actually destroy the uclass? It needs a comment.

Yes, because the drivers in the uclass need to be removed in two steps,
first the ones which can be removed early, and then the rest which need
to be removed late (like clock drivers).

[...]