[U-Boot] [RFC PATCH 1/3] dm: core: Add pre-OS remove flag to device_remove()

[Stefan Roese]

Hi Simon,

On 10.03.2017 12:31, Stefan Roese wrote:
> Hi Simon,
> 
> On 08.03.2017 22:01, Simon Glass wrote:
>> Hi Stefan,
>>
>> On 2 March 2017 at 23:24, Stefan Roese <sr at denx.de> wrote:
>>> Hi Simon,
>>>
>>> On 03.03.2017 05:53, Simon Glass wrote:
>>>> On 1 March 2017 at 03:23, Stefan Roese <sr at denx.de> wrote:
>>>>> This patch adds the pre_os_remove boolean flag to device_remove() and
>>>>> changes all calls to this function to provide the default value of
>>>>> "false". This is in preparation for the driver specific pre-OS remove
>>>>> support.
>>>>>
>>>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>>>> Cc: Simon Glass <sjg at chromium.org>
>>>>> ---
>>>>>  arch/x86/cpu/queensbay/tnc.c   |  4 ++--
>>>>>  cmd/cros_ec.c                  |  2 +-
>>>>>  cmd/sf.c                       |  2 +-
>>>>>  drivers/block/blk-uclass.c     |  2 +-
>>>>>  drivers/block/sandbox.c        |  2 +-
>>>>>  drivers/core/device-remove.c   |  9 +++++----
>>>>>  drivers/core/device.c          |  2 +-
>>>>>  drivers/core/root.c            |  2 +-
>>>>>  drivers/core/uclass.c          |  2 +-
>>>>>  drivers/mmc/mmc-uclass.c       |  2 +-
>>>>>  drivers/mtd/spi/sandbox.c      |  2 +-
>>>>>  drivers/mtd/spi/sf-uclass.c    |  2 +-
>>>>>  drivers/spi/spi-uclass.c       |  4 ++--
>>>>>  drivers/usb/emul/sandbox_hub.c |  2 +-
>>>>>  drivers/usb/host/usb-uclass.c  |  4 ++--
>>>>>  include/dm/device-internal.h   |  5 +++--
>>>>>  include/dm/device.h            |  3 +++
>>>>>  test/dm/bus.c                  |  8 ++++----
>>>>>  test/dm/core.c                 | 16 ++++++++--------
>>>>>  test/dm/eth.c                  |  2 +-
>>>>>  test/dm/spi.c                  |  2 +-
>>>>>  21 files changed, 42 insertions(+), 37 deletions(-)
>>>>
>>>> I think adding a parameter to device_remove() makes sense, but how
>>>> about using flags instead? The true/false meaning is not clear here
>>>> and your comment in device.h doesn't really help.
>>>
>>> So you are suggesting something like this:
>>>
>>> int device_remove(struct udevice *dev, uin32_t remove_flags);
>>
>> Yes, or really 'uint remove_flags'
>>
>>>
>>> ?
>>>
>>>> Also I think it is better to name it after the required function
>>>> rather than state related to the caller. IOW instead of 'pre-os' use
>>>> something like 'active_dma_only' or as a flag ONLY_REMOVE_ACTIVE_DMA.
>>>>
>>>> Do you think the presence of DMA should be a device flag?
>>>
>>> The usage of flags instead of this pre-os parameter could make
>>> sense to me, as its much more flexible. But I'm not so sure about
>>> the flag (re-)naming to something specific like DMA. As there
>>> could be multiple reasons other than DMA related for this last-stage
>>> driver cleanup / configuration before the OS is started. E.g.
>>> if a driver needs to stop an internal timer before the OS is started,
>>> it would need to "abuse" this DMA flag to get called at the last
>>> pre-OS stage. Or is your thinking that in such cases (e.g. stopping
>>> of timer) a new flag should get introduced and added to this
>>> "remove_flags" parameter in bootm?
>>
>> Yes, so that it is explicit. Another approach would be:
>>
>> enum {
>>   DM_REMOVE_ACTIVE_ALL   = 1 << 0, /* Remove all devices */
>>   DM_REMOVE_ACTIVE_DMA  = 1 << 1, /* Remove only devices with active
>> DMA */
>>   /* Add more use cases here */
>> };
> 
> Okay, I'll go forward with this suggestion and will generate a new
> patchset version soon. Stay tuned...

Thinking about it, we need a bit for the "normal" remove case as well.
How about this naming:

enum {
	DM_REMOVE_NORMAL      = 1 << 0, /* Remove all devices */
	DM_REMOVE_ACTIVE_ALL  = 1 << 1, /* Remove devices with any active flag set */
	DM_REMOVE_ACTIVE_DMA  = 1 << 2, /* Remove only devices with active DMA */
	/* Add more use cases here */
};

Or do you have another suggestion in mind for this "normal" device
remove case?

Thanks,
Stefan