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

[Simon Glass]

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 */
};

Then, DM_REMOVE_ACTIVE_ALL means everything will be removed, and if
that flag is not set, the other flags can be used.

I am assuming that there actually will be other cases - your email
suggests that could be true.

Regards,
Simon