On 11/21/20 9:13 PM, Marek Vasut wrote:
On 11/22/20 12:07 AM, Simon Glass wrote: [...]
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'?
I think perhaps the problem isn't that it needs to be "late", it's that it has perhaps not obviously described children. Which gets back to what you just said as well about "later" and "fairly late". It's an ordering problem.
Yes it is.
We currently don't record devices that depend on others. It would be possible to add a refcount to DM to cope with this and implement it for clocks. I wonder if that might be better than what we have here?
This is still a bootloader, not a general-purpose OS, so I would argue we should not complicate this more than is necessary. The DM already addsa lot of bloat to U-Boot, no need to make that worse unless there is a real good reason for that. Also, in V1 of this patch, Simon did suggest that a simple approach is OK if I recall correctly.
FWIW the clock subsystem already does refcounting of clocks (though it is for struct clk and not for the devices themselves). However, I think it would be difficult to use those counts to determine when the clock driver is no longer needed. For example, some devices (such as CPUs) may use clocks, but should never stop those clocks.
A quick-and-dirty method could be to create a linked list of all probed devices, and then remove devices in reverse order. So an example call sequence could be
... 1. mmc's probe() 2. clk_get_*() 3. clk's probe() 4. clk's finishes; clk added to probed list 5. mmc's probe finishes; mmc added to probed list ... 7. mmc's remove() called 8. clk's remove() called
Note that this would be a change to dm_uninit, not device_remove; we would still need the recursive removal logic for when devices are removed via other means. Unfortunately, this method would require yet another list_head in udevice, so perhaps it should just be an option?
--Sean