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

[Marek Vasut]

On 11/9/20 1:21 AM, Simon Glass wrote:
> Hi Marek,

[...]

>> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
>> index 1206e306db..f9091a3d41 100644
>> --- a/arch/arm/lib/bootm.c
>> +++ b/arch/arm/lib/bootm.c
>> @@ -120,6 +120,7 @@ static void announce_and_cleanup(int fake)
>>           * of DMA operation or releasing device internal buffers.
>>           */
>>          dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
>> +       dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL | DM_REMOVE_LATE);
> 
> Please see my previous comments about the naming and semantics. I'm
> repeating them here:
> 
> Firstly I think we should use a different name. 'Late' doesn't really
> tell me anything.
> 
> If I understand it correctly, this is supposed to be after OS_PREPARE
> but before booting the OS?

No. The drivers which are marked as remove-late should be removed late, 
after all the normal drivers were removed. The example in the commit 
message explains where this is needed -- first remove mmc driver to set 
eMMC out of HS mode and change the bus clock and after that remove clock 
driver ; the clock driver is still required during the removal of the 
mmc driver.

> I think we need to separate the flag names as they are too similar.
> 
> I think DM_FLAG_BASIC and DM_REMOVE_NON_BASIC would be better (or some
> term that explains that this is a device used by many others)
> 
> If BASIC is too terrible, perhaps CORE, or VITAL, or PRIME or CRITICAL
> or something like that?

This makes no sense to me.

> That way we are describing the property of the device rather than what
> we want to do with it.

The device is not critical or vital, it just needs to be torn down late.

> Note also the semantics of what is going on here. The idea of the
> existing OS_PREPARE and ACTIVE_DMA flags is that the default for
> device_remove() is to remove everything, but if you provide a flag
> then it just removes those things. Your flag is the opposite to that,
> which is why you are changing so much code.

I obviously cannot remove everything, see the example above.

> So I think we could change this to DM_REMOVE_NON_BASIC and make that a
> separate step before we do a final remove with flags of 0.

[...]

>> @@ -82,16 +86,19 @@ void dm_leak_check_start(struct unit_test_state *uts)
>>   int dm_leak_check_end(struct unit_test_state *uts)
>>   {
>>          struct mallinfo end;
>> -       int id, diff;
>> +       int i, id, diff;
>>
>>          /* Don't delete the root class, since we started with that */
>> -       for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) {
>> -               struct uclass *uc;
>> -
>> -               uc = uclass_find(id);
>> -               if (!uc)
>> -                       continue;
>> -               ut_assertok(uclass_destroy(uc));
>> +       for (i = 0; i < 2; i++) {
>> +               for (id = UCLASS_ROOT + 1; id < UCLASS_COUNT; id++) {
>> +                       struct uclass *uc;
>> +
>> +                       uc = uclass_find(id);
>> +                       if (!uc)
>> +                               continue;
>> +                       ut_assertok(uclass_destroy(uc,
>> +                                   i ? DM_REMOVE_LATE : DM_REMOVE_NORMAL));
>> +               }
> 
> Why do we need to destroy the classes in two steps? I understand
> removing devices, but destroying the uclasses seems like it could stay
> as it is?

Because if you destroy clock uclass before mmc uclass, then you cannot 
remove the mmc drivers and reconfigure the bus clock anymore in the 
remove callback. So that needs to be done in two steps.

[...]