Re: [U-Boot] [PATCH 2/4 v3] dm: core: Add dm_remove_devices_flags() and hook it into device_remove()

[Stefan Roese]

Hi Simon,

On 26.03.2017 05:52, Simon Glass wrote:

On 25 March 2017 at 19:17, Simon Glass  wrote:

Hi Stefan,

On 22 March 2017 at 00:28, Stefan Roese  wrote:

The new function dm_remove_devices_flags() is intented for driver specific
last-stage cleanup operations before the OS is started. This patch adds
this functionality and hooks it into the common device_remove()
function.

Drivers wanting to use this feature for some last-stage removal calls,
need to add one of the DM_REMOVE_xx flags to their driver .flags.

Signed-off-by: Stefan Roese 
Reviewed-by: Simon Glass 
---
v3:
- Add conditional compilation to fix compilation breakage on platforms
  without DM and DM_DEVICE_REMOVE support. With this change, Travis
  compiles all targets without any error

v2:
- Added Simons Reviewed-by

 drivers/core/device-remove.c | 15 +++
 drivers/core/root.c  |  9 +
 include/dm/root.h| 16 
 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
index b80bf52320..ca4680f7c2 100644
--- a/drivers/core/device-remove.c
+++ b/drivers/core/device-remove.c
@@ -174,7 +174,12 @@ int device_remove(struct udevice *dev, uint flags)
if (ret)
goto err;

-   if (drv->remove) {
+   /*
+* Remove the device if called with the "normal" remove flag set,
+* or if the remove flag matches the driver flags
+*/
+   if (drv->remove &&
+   ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags))) {


This seems to be comparing different things. The DM_REMOVE_NORMAL flag
is not from the same enum as drv->flags, is it?


I have to admit that this comparison above is a bit "vague". I'll
make it more explicit in the next patch version.

Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 2/4 v3] dm: core: Add dm_remove_devices_flags() and hook it into device_remove()

[Simon Glass]

Hi Stefan,

On 25 March 2017 at 19:17, Simon Glass  wrote:
> Hi Stefan,
>
> On 22 March 2017 at 00:28, Stefan Roese  wrote:
>> The new function dm_remove_devices_flags() is intented for driver specific
>> last-stage cleanup operations before the OS is started. This patch adds
>> this functionality and hooks it into the common device_remove()
>> function.
>>
>> Drivers wanting to use this feature for some last-stage removal calls,
>> need to add one of the DM_REMOVE_xx flags to their driver .flags.
>>
>> Signed-off-by: Stefan Roese 
>> Reviewed-by: Simon Glass 
>> ---
>> v3:
>> - Add conditional compilation to fix compilation breakage on platforms
>>   without DM and DM_DEVICE_REMOVE support. With this change, Travis
>>   compiles all targets without any error
>>
>> v2:
>> - Added Simons Reviewed-by
>>
>>  drivers/core/device-remove.c | 15 +++
>>  drivers/core/root.c  |  9 +
>>  include/dm/root.h| 16 
>>  3 files changed, 36 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
>> index b80bf52320..ca4680f7c2 100644
>> --- a/drivers/core/device-remove.c
>> +++ b/drivers/core/device-remove.c
>> @@ -174,7 +174,12 @@ int device_remove(struct udevice *dev, uint flags)
>> if (ret)
>> goto err;
>>
>> -   if (drv->remove) {
>> +   /*
>> +* Remove the device if called with the "normal" remove flag set,
>> +* or if the remove flag matches the driver flags
>> +*/
>> +   if (drv->remove &&
>> +   ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags))) {

This seems to be comparing different things. The DM_REMOVE_NORMAL flag
is not from the same enum as drv->flags, is it?

>
> You are changing the condition here but still call the post_remove()
> method immediately below. If you decide note to remove the device then
> I think you should not call that function.
>
>> ret = drv->remove(dev);
>> if (ret)
>> goto err_remove;
>> @@ -188,10 +193,12 @@ int device_remove(struct udevice *dev, uint flags)
>> }
>> }
>>
>> -   device_free(dev);
>> +   if ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags)) {
>> +   device_free(dev);
>>
>> -   dev->seq = -1;
>> -   dev->flags &= ~DM_FLAG_ACTIVATED;
>> +   dev->seq = -1;
>> +   dev->flags &= ~DM_FLAG_ACTIVATED;
>> +   }
>>
>> return ret;

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 2/4 v3] dm: core: Add dm_remove_devices_flags() and hook it into device_remove()

[Simon Glass]

Hi Stefan,

On 22 March 2017 at 00:28, Stefan Roese  wrote:
> The new function dm_remove_devices_flags() is intented for driver specific
> last-stage cleanup operations before the OS is started. This patch adds
> this functionality and hooks it into the common device_remove()
> function.
>
> Drivers wanting to use this feature for some last-stage removal calls,
> need to add one of the DM_REMOVE_xx flags to their driver .flags.
>
> Signed-off-by: Stefan Roese 
> Reviewed-by: Simon Glass 
> ---
> v3:
> - Add conditional compilation to fix compilation breakage on platforms
>   without DM and DM_DEVICE_REMOVE support. With this change, Travis
>   compiles all targets without any error
>
> v2:
> - Added Simons Reviewed-by
>
>  drivers/core/device-remove.c | 15 +++
>  drivers/core/root.c  |  9 +
>  include/dm/root.h| 16 
>  3 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
> index b80bf52320..ca4680f7c2 100644
> --- a/drivers/core/device-remove.c
> +++ b/drivers/core/device-remove.c
> @@ -174,7 +174,12 @@ int device_remove(struct udevice *dev, uint flags)
> if (ret)
> goto err;
>
> -   if (drv->remove) {
> +   /*
> +* Remove the device if called with the "normal" remove flag set,
> +* or if the remove flag matches the driver flags
> +*/
> +   if (drv->remove &&
> +   ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags))) {

You are changing the condition here but still call the post_remove()
method immediately below. If you decide note to remove the device then
I think you should not call that function.

> ret = drv->remove(dev);
> if (ret)
> goto err_remove;
> @@ -188,10 +193,12 @@ int device_remove(struct udevice *dev, uint flags)
> }
> }
>
> -   device_free(dev);
> +   if ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags)) {
> +   device_free(dev);
>
> -   dev->seq = -1;
> -   dev->flags &= ~DM_FLAG_ACTIVATED;
> +   dev->seq = -1;
> +   dev->flags &= ~DM_FLAG_ACTIVATED;
> +   }
>
> return ret;
>

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 2/4 v3] dm: core: Add dm_remove_devices_flags() and hook it into device_remove()

[Stefan Roese]

The new function dm_remove_devices_flags() is intented for driver specific
last-stage cleanup operations before the OS is started. This patch adds
this functionality and hooks it into the common device_remove()
function.

Drivers wanting to use this feature for some last-stage removal calls,
need to add one of the DM_REMOVE_xx flags to their driver .flags.

Signed-off-by: Stefan Roese 
Reviewed-by: Simon Glass 
---
v3:
- Add conditional compilation to fix compilation breakage on platforms
  without DM and DM_DEVICE_REMOVE support. With this change, Travis
  compiles all targets without any error

v2:
- Added Simons Reviewed-by

 drivers/core/device-remove.c | 15 +++
 drivers/core/root.c  |  9 +
 include/dm/root.h| 16 
 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
index b80bf52320..ca4680f7c2 100644
--- a/drivers/core/device-remove.c
+++ b/drivers/core/device-remove.c
@@ -174,7 +174,12 @@ int device_remove(struct udevice *dev, uint flags)
if (ret)
goto err;
 
-   if (drv->remove) {
+   /*
+* Remove the device if called with the "normal" remove flag set,
+* or if the remove flag matches the driver flags
+*/
+   if (drv->remove &&
+   ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags))) {
ret = drv->remove(dev);
if (ret)
goto err_remove;
@@ -188,10 +193,12 @@ int device_remove(struct udevice *dev, uint flags)
}
}
 
-   device_free(dev);
+   if ((flags & DM_REMOVE_NORMAL) || (flags & drv->flags)) {
+   device_free(dev);
 
-   dev->seq = -1;
-   dev->flags &= ~DM_FLAG_ACTIVATED;
+   dev->seq = -1;
+   dev->flags &= ~DM_FLAG_ACTIVATED;
+   }
 
return ret;
 
diff --git a/drivers/core/root.c b/drivers/core/root.c
index 4d2033bfad..deefd00687 100644
--- a/drivers/core/root.c
+++ b/drivers/core/root.c
@@ -182,6 +182,15 @@ int dm_uninit(void)
return 0;
 }
 
+#if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE)
+int dm_remove_devices_flags(uint flags)
+{
+   device_remove(dm_root(), flags);
+
+   return 0;
+}
+#endif
+
 int dm_scan_platdata(bool pre_reloc_only)
 {
int ret;
diff --git a/include/dm/root.h b/include/dm/root.h
index 3cf730dcee..058eb98923 100644
--- a/include/dm/root.h
+++ b/include/dm/root.h
@@ -115,4 +115,20 @@ int dm_init(void);
  */
 int dm_uninit(void);
 
+#if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE)
+/**
+ * dm_remove_devices_flags - Call remove function of all drivers with
+ *   specific removal flags set to selectively
+ *   remove drivers
+ *
+ * All devices with the matching flags set will be removed
+ *
+ * @flags: Flags for selective device removal
+ * @return 0 if OK, -ve on error
+ */
+int dm_remove_devices_flags(uint flags);
+#else
+static inline int dm_remove_devices_flags(uint flags) { return 0; }
+#endif
+
 #endif
-- 
2.12.0

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot