Hi Stefan, On Tue, 18 Aug 2020 at 08:13, Stefan Roese <s...@denx.de> wrote: > > Hi Simon, > > On 18.08.20 15:22, Simon Glass wrote: > > Hi Marek, > > > > +Stefan Roese who wrote the existing code. > > Hmmm, I was not Cc'ed. > > > On Fri, 7 Aug 2020 at 15:40, Marek Vasut <marek.va...@gmail.com> wrote: > >> > >> On 8/7/20 6:23 PM, Simon Glass wrote: > >> > >> [...] > >> > >>>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c > >>>> index efdb0f2905..07b241b6bb 100644 > >>>> --- a/drivers/core/device-remove.c > >>>> +++ b/drivers/core/device-remove.c > >>>> @@ -172,14 +172,19 @@ int device_remove(struct udevice *dev, uint flags) > >>>> drv = dev->driver; > >>>> assert(drv); > >>>> > >>>> - ret = uclass_pre_remove_device(dev); > >>>> - if (ret) > >>>> - return ret; > >>>> + if (!(flags & DM_REMOVE_LATE)) { > >>>> + ret = uclass_pre_remove_device(dev); > >>>> + if (ret) > >>>> + return ret; > >>>> + } > >>>> > >>>> ret = device_chld_remove(dev, NULL, flags); > >>>> if (ret) > >>>> goto err; > >>>> > >>>> + if (!(flags & DM_REMOVE_LATE) && (drv->flags & > >>>> DM_FLAG_REMOVE_LATE)) > > > > 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? > > > > 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) > > > > That way we are describing the property of the device rather than what > > we want to do with it. > > > > 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. > > > > 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. > > > >>>> + return 0; > >>> > >>> Why return here? > >> > >> If the DM isn't in a late-remove phase and the driver has remove-late > >> flag, then the driver should not be removed yet. This is the case for > >> e.g. a clock driver. > >> > >>> Shouldn;'t you use flags_remove() ? > >> > >> Please explain ? > > > > That function checks the flags so I think you should add your check > > there rather than adding new code. > > > >> > >>>> /* > >>>> * Remove the device if called with the "normal" remove flag > >>>> set, > >>>> * or if the remove flag matches any of the drivers remove flags > >>>> diff --git a/drivers/core/root.c b/drivers/core/root.c > >>>> index 0726be6b79..21f3054125 100644 > >>>> --- a/drivers/core/root.c > >>>> +++ b/drivers/core/root.c > >>>> @@ -168,6 +168,7 @@ int dm_init(bool of_live) > >>>> int dm_uninit(void) > >>>> { > >>>> device_remove(dm_root(), DM_REMOVE_NORMAL); > >>>> + device_remove(dm_root(), DM_REMOVE_LATE); > >>>> device_unbind(dm_root()); > >>>> gd->dm_root = NULL; > >>>> > >>>> @@ -393,6 +394,7 @@ struct acpi_ops root_acpi_ops = { > >>>> U_BOOT_DRIVER(root_driver) = { > >>>> .name = "root_driver", > >>>> .id = UCLASS_ROOT, > >>>> + .flags = DM_FLAG_REMOVE_LATE, > >>> > >>> Why are you changing this flag for the root device? > >> > >> Because root device should only be removed last. > > > > OK I see, thanks. > > > >> > >>>> ACPI_OPS_PTR(&root_acpi_ops) > >>>> }; > >>>> > >>>> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c > >>>> index c3f1b73cd6..ac474d3ff8 100644 > >>>> --- a/drivers/core/uclass.c > >>>> +++ b/drivers/core/uclass.c > >>>> @@ -104,10 +104,28 @@ fail_mem: > >>>> return ret; > >>>> } > >>>> > >>>> -int uclass_destroy(struct uclass *uc) > >>>> +int uclass_find_device_by_drv_flag(struct uclass *uc, const unsigned > >>>> int flag, > >>>> + const bool neg, struct udevice **devp) > >>> > >>> Is this intended to be exported? If so, please add a test. If not, > >>> please make it static. In any case, it needs a full comment as to what > >>> it does, and args. > >> > >> Its internal function, should be static. > > > > OK so please add a comment. > > > >> > >>>> +{ > >>>> + struct udevice *dev; > >>>> + > >>>> + *devp = NULL; > >>>> + uclass_foreach_dev(dev, uc) { > >>>> + if ((neg && (dev->driver->flags & flag)) || > >>>> + (!neg && !(dev->driver->flags & flag))) { > >>>> + *devp = dev; > >>>> + return 0; > >>>> + } > >>>> + } > >>>> + > >>>> + return -ENODEV; > >>>> +} > >>>> + > >>>> +int uclass_destroy(struct uclass *uc, unsigned int flag) > >>>> { > >>>> struct uclass_driver *uc_drv; > >>>> struct udevice *dev; > >>>> + bool late = flag & DM_REMOVE_LATE; > >>>> int ret; > >>>> > >>>> /* > >>>> @@ -116,15 +134,15 @@ int uclass_destroy(struct uclass *uc) > >>>> * unbound (by the recursion in the call to device_unbind() > >>>> below). > >>>> * We can loop until the list is empty. > >>>> */ > >>>> - while (!list_empty(&uc->dev_head)) { > >>>> - dev = list_first_entry(&uc->dev_head, struct udevice, > >>>> - uclass_node); > >>>> - ret = device_remove(dev, DM_REMOVE_NORMAL | > >>>> DM_REMOVE_NO_PD); > >>>> - if (ret) > >>>> + while (!uclass_find_device_by_drv_flag(uc, DM_FLAG_REMOVE_LATE, > >>>> late, &dev)) { > >>>> + ret = device_remove(dev, flag | DM_REMOVE_NO_PD); > >>> > >>> What happened to DM_REMOVE_NORMAL? > >> > >> See above, bool late, this uclass_destroy() is called twice now. Once > >> for normal devices, once for late devices, so that the ordering is correct. > > > > Actually DM_REMOVE_NO_PD looks broken to me. We have to describe the > > devices in this function, but it looks like that won't happen as it is > > written. The docs for uclass_destroy() don't say anything about it > > sometimes not destroying the uclass. > > > > Anyway, hopefully with the changes above, you won't need any changes here. > > > >> > >>>> + if (ret) { > >>>> return log_msg_ret("remove", ret); > >>>> + } > >>>> ret = device_unbind(dev); > >>>> - if (ret) > >>>> + if (ret) { > >>>> return log_msg_ret("unbind", ret); > >>>> + } > >>> > >>> Don't need {} > >>> > >>>> } > >>>> > >>>> uc_drv = uc->uc_drv; > >>>> diff --git a/include/dm/device.h b/include/dm/device.h > >>>> index 953706cf52..7b1db252bf 100644 > >>>> --- a/include/dm/device.h > >>>> +++ b/include/dm/device.h > >>>> @@ -73,6 +73,8 @@ struct driver_info; > >>>> */ > >>>> #define DM_FLAG_REMOVE_WITH_PD_ON (1 << 13) > >>>> > >>>> +#define DM_FLAG_REMOVE_LATE (1 << 14) > >>> > >>> Needs a comment as to what it means. > >> > >> It means the driver should be removed after all the non-late drivers. > > > > OK, see above. It still needs a comment. > > > >> > >>>> + > >>>> /* > >>>> * One or multiple of these flags are passed to device_remove() so that > >>>> * a selective device removal as specified by the remove-stage and the > >>>> @@ -95,6 +97,8 @@ enum { > >>>> > >>>> /* Don't power down any attached power domains */ > >>>> DM_REMOVE_NO_PD = 1 << 1, > >>>> + > >>>> + DM_REMOVE_LATE = 1 << 2, > >>> > >>> Comment > >>> > >>>> }; > >>>> > >>>> /** > >>>> diff --git a/include/dm/uclass-internal.h b/include/dm/uclass-internal.h > >>>> index 6e3f15c2b0..b5926b0f5c 100644 > >>>> --- a/include/dm/uclass-internal.h > >>>> +++ b/include/dm/uclass-internal.h > >>>> @@ -247,8 +247,9 @@ struct uclass *uclass_find(enum uclass_id key); > >>>> * Destroy a uclass and all its devices > >>>> * > >>>> * @uc: uclass to destroy > >>>> + * @flag: driver flags (DM_REMOVE_NORMAL or DM_REMOVE_LATE) > >>>> * @return 0 on success, -ve on error > >>>> */ > >>>> -int uclass_destroy(struct uclass *uc); > >>>> +int uclass_destroy(struct uclass *uc, unsigned int flag); > >>> > >>> Why is the flag added here? Does it mean that sometimes it will not > >>> actually destroy the uclass? It needs a comment. > >> > >> Yes, because the drivers in the uclass need to be removed in two steps, > >> first the ones which can be removed early, and then the rest which need > >> to be removed late (like clock drivers). > > > > It is OK to remove the devices from a uclass in stages, but destroying > > the uclass should happen once. So far as I understand it, you should > > not have this flag. > > > > Also we need a sandbox test for this new behaviour as it is getting > > complicated. > > > > Stefan, could you perhaps look at a test for the existing flags? We > > should have some devices with different flags set and check that the > > correct things happen when calling device_remove() with various flags. > > I did send a test at that time. Commit 24f927c527b0 ("dm: test: Add test > for device removal") adds this test. Is something missing in this test?
Ah yes I thought you did. I missed it, sorry. It is DM_REMOVE_NO_PD that lacks a test. Regards, SImon