On 11/18/20 3:37 PM, Simon Glass wrote:
Hi,
[...]
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.
I don't want the word 'late'. Then we'll end up with 'later' and
'fairly late', etc. and it will get out of hand. We need a word that
describes what is special about the devices, not when to do stuff with
them.
If there is a need for "later" , then we will need some more complex
code. I suggest we cross that bridge when we come to it.
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.
What is it about the device that requires it to be torn down 'late'?
For example clock driver needs to be turn down after mmc controller,
since the mmc controller might need to reconfigure the card in .remove
callback, for which it still needs to clock to be active. That's the
usecase I have on real hardware.
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.
Do you understand what I am saying about inverting the flag?
No, please elaborate.
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.
Yes I understand that. All of my comments are about the implementation
rather than the problem you are solving. See the existing DM_REMOVE_
flags for examples.
If you like, I could have a try at this. Perhaps it is not as
straightforward as I imagine.
I think it would be a good idea if you looked at the use case for this
first.
