[U-Boot] [PATCH v2 1/2] dm: core: device: switch off power domain after device removal

[U-Boot] [PATCH v2 1/2] dm: core: device: switch off power domain after device removal

[Anatolij Gustschin]

The power domain associated with a device is enabled when probing,
but currently the domain remains enabled when the device is removed.
Some boards started to disable power domains for selected devices
via custom board_quiesce_devices(), but it doesn't work in many
cases, i. e. because devices still can be accessed later in
.remove() callback on behalf of dm_remove_devices_flags().

Utilize the DM core to power off the device power domain, but add a
device flag to be able to selectively let the power domain enabled
after device removal. This might be required for devices that must
remain enabled when booting OS, i. e. serial console for debug
output, etc.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
Changes in v2:
 - use CONFIG_IS_ENABLED(POWER_DOMAIN) to reduce code size

 drivers/core/device-remove.c | 9 +++++++++
 include/dm/device.h          | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
index 586fadee0a..abed84a652 100644
--- a/drivers/core/device-remove.c
+++ b/drivers/core/device-remove.c
@@ -16,6 +16,7 @@
 #include <dm/uclass.h>
 #include <dm/uclass-internal.h>
 #include <dm/util.h>
+#include <power-domain.h>
 
 int device_chld_unbind(struct udevice *dev, struct driver *drv)
 {
@@ -154,6 +155,7 @@ static bool flags_remove(uint flags, uint drv_flags)
 
 int device_remove(struct udevice *dev, uint flags)
 {
+	struct power_domain pd;
 	const struct driver *drv;
 	int ret;
 
@@ -192,6 +194,13 @@ int device_remove(struct udevice *dev, uint flags)
 		}
 	}
 
+	if (CONFIG_IS_ENABLED(POWER_DOMAIN) && dev->parent &&
+	    device_get_uclass_id(dev) != UCLASS_POWER_DOMAIN &&
+	    !(dev->flags & DM_FLAG_REMOVE_WITH_PD_ON)) {
+		if (!power_domain_get(dev, &pd))
+			power_domain_off(&pd);
+	}
+
 	if (flags_remove(flags, drv->flags)) {
 		device_free(dev);
 
diff --git a/include/dm/device.h b/include/dm/device.h
index 27a6d7b9fd..9a98a4a39e 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -61,6 +61,12 @@ struct driver_info;
  */
 #define DM_FLAG_OS_PREPARE		(1 << 10)
 
+/*
+ * Device is removed without switching off its power domain. This might
+ * be required, i. e. for serial console (debug) output when booting OS.
+ */
+#define DM_FLAG_REMOVE_WITH_PD_ON	(1 << 11)
+
 /*
  * 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
-- 
2.17.1

[U-Boot] [PATCH v2 2/2] serial: lpuart: request dm device removal when booting OS

[Anatolij Gustschin]

Extend the driver to allow dm device removal, but always let the
console serial device power domain enabled, so that U-Boot doesn't
crash when i. e. the serial output is enabled for debugging.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
Changes in v2:
 - None

 drivers/serial/serial_lpuart.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/serial/serial_lpuart.c b/drivers/serial/serial_lpuart.c
index 57dd4a72c6..b028f4b35f 100644
--- a/drivers/serial/serial_lpuart.c
+++ b/drivers/serial/serial_lpuart.c
@@ -489,6 +489,13 @@ static int lpuart_serial_probe(struct udevice *dev)
 		return _lpuart_serial_init(dev);
 }
 
+static int lpuart_serial_remove(struct udevice *dev)
+{
+	if (dev == gd->cur_serial_dev)
+		dev->flags |= DM_FLAG_REMOVE_WITH_PD_ON;
+	return 0;
+}
+
 static int lpuart_serial_ofdata_to_platdata(struct udevice *dev)
 {
 	struct lpuart_serial_platdata *plat = dev->platdata;
@@ -543,5 +550,7 @@ U_BOOT_DRIVER(serial_lpuart) = {
 	.ofdata_to_platdata = lpuart_serial_ofdata_to_platdata,
 	.platdata_auto_alloc_size = sizeof(struct lpuart_serial_platdata),
 	.probe = lpuart_serial_probe,
+	.remove = lpuart_serial_remove,
 	.ops	= &lpuart_serial_ops,
+	.flags	= DM_FLAG_OS_PREPARE,
 };
-- 
2.17.1

[U-Boot] [PATCH v2 2/2] serial: lpuart: request dm device removal when booting OS

[Peng Fan]

Hi Anatolij,

> Subject: [PATCH v2 2/2] serial: lpuart: request dm device removal when
> booting OS
> 
> Extend the driver to allow dm device removal, but always let the console
> serial device power domain enabled, so that U-Boot doesn't crash when i. e.
> the serial output is enabled for debugging.
> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
> Changes in v2:
>  - None
> 
>  drivers/serial/serial_lpuart.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/serial/serial_lpuart.c b/drivers/serial/serial_lpuart.c index
> 57dd4a72c6..b028f4b35f 100644
> --- a/drivers/serial/serial_lpuart.c
> +++ b/drivers/serial/serial_lpuart.c
> @@ -489,6 +489,13 @@ static int lpuart_serial_probe(struct udevice *dev)
>  		return _lpuart_serial_init(dev);
>  }
> 
> +static int lpuart_serial_remove(struct udevice *dev) {
> +	if (dev == gd->cur_serial_dev)
> +		dev->flags |= DM_FLAG_REMOVE_WITH_PD_ON;

How about introduce a device tree property for DM? Then
Set the flags in common code, not in specific driver.

Thanks,
Peng

> +	return 0;
> +}
> +
>  static int lpuart_serial_ofdata_to_platdata(struct udevice *dev)  {
>  	struct lpuart_serial_platdata *plat = dev->platdata; @@ -543,5 +550,7
> @@ U_BOOT_DRIVER(serial_lpuart) = {
>  	.ofdata_to_platdata = lpuart_serial_ofdata_to_platdata,
>  	.platdata_auto_alloc_size = sizeof(struct lpuart_serial_platdata),
>  	.probe = lpuart_serial_probe,
> +	.remove = lpuart_serial_remove,
>  	.ops	= &lpuart_serial_ops,
> +	.flags	= DM_FLAG_OS_PREPARE,
>  };
> --
> 2.17.1

[U-Boot] [PATCH v2 2/2] serial: lpuart: request dm device removal when booting OS

[Anatolij Gustschin]

Hi Peng,

On Mon, 15 Jul 2019 03:02:46 +0000
Peng Fan peng.fan at nxp.com wrote:
... 
> > +static int lpuart_serial_remove(struct udevice *dev) {
> > +	if (dev == gd->cur_serial_dev)
> > +		dev->flags |= DM_FLAG_REMOVE_WITH_PD_ON;  
> 
> How about introduce a device tree property for DM? Then
> Set the flags in common code, not in specific driver.

we should not add more DT properties which are not describing the
hardware, I think. Current plan is to drop this patch and do additional
check dev != gd->cur_serial_dev in device_remove(), as Lokesh suggested.

--
Anatolij

[U-Boot] [PATCH v2 1/2] dm: core: device: switch off power domain after device removal

[Peng Fan]

> Subject: [PATCH v2 1/2] dm: core: device: switch off power domain after
> device removal
> 
> The power domain associated with a device is enabled when probing, but
> currently the domain remains enabled when the device is removed.
> Some boards started to disable power domains for selected devices via
> custom board_quiesce_devices(), but it doesn't work in many cases, i. e.
> because devices still can be accessed later in
> .remove() callback on behalf of dm_remove_devices_flags().
> 
> Utilize the DM core to power off the device power domain, but add a device
> flag to be able to selectively let the power domain enabled after device
> removal. This might be required for devices that must remain enabled when
> booting OS, i. e. serial console for debug output, etc.
> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
> Changes in v2:
>  - use CONFIG_IS_ENABLED(POWER_DOMAIN) to reduce code size
> 
>  drivers/core/device-remove.c | 9 +++++++++
>  include/dm/device.h          | 6 ++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
> index 586fadee0a..abed84a652 100644
> --- a/drivers/core/device-remove.c
> +++ b/drivers/core/device-remove.c
> @@ -16,6 +16,7 @@
>  #include <dm/uclass.h>
>  #include <dm/uclass-internal.h>
>  #include <dm/util.h>
> +#include <power-domain.h>
> 
>  int device_chld_unbind(struct udevice *dev, struct driver *drv)  { @@
> -154,6 +155,7 @@ static bool flags_remove(uint flags, uint drv_flags)
> 
>  int device_remove(struct udevice *dev, uint flags)  {
> +	struct power_domain pd;
>  	const struct driver *drv;
>  	int ret;
> 
> @@ -192,6 +194,13 @@ int device_remove(struct udevice *dev, uint flags)
>  		}
>  	}
> 
> +	if (CONFIG_IS_ENABLED(POWER_DOMAIN) && dev->parent &&
> +	    device_get_uclass_id(dev) != UCLASS_POWER_DOMAIN &&
> +	    !(dev->flags & DM_FLAG_REMOVE_WITH_PD_ON)) {
> +		if (!power_domain_get(dev, &pd))
> +			power_domain_off(&pd);
> +	}
> +
>  	if (flags_remove(flags, drv->flags)) {
>  		device_free(dev);
> 
> diff --git a/include/dm/device.h b/include/dm/device.h index
> 27a6d7b9fd..9a98a4a39e 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -61,6 +61,12 @@ struct driver_info;
>   */
>  #define DM_FLAG_OS_PREPARE		(1 << 10)
> 
> +/*
> + * Device is removed without switching off its power domain. This might
> + * be required, i. e. for serial console (debug) output when booting OS.
> + */
> +#define DM_FLAG_REMOVE_WITH_PD_ON	(1 << 11)
> +
>  /*
>   * 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

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> --
> 2.17.1

[U-Boot] [PATCH v2 1/2] dm: core: device: switch off power domain after device removal

[Simon Glass]

Hi Anatolij,

On Sun, 14 Jul 2019 at 20:59, Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: [PATCH v2 1/2] dm: core: device: switch off power domain after
> > device removal
> >
> > The power domain associated with a device is enabled when probing, but
> > currently the domain remains enabled when the device is removed.
> > Some boards started to disable power domains for selected devices via
> > custom board_quiesce_devices(), but it doesn't work in many cases, i. e.
> > because devices still can be accessed later in
> > .remove() callback on behalf of dm_remove_devices_flags().
> >
> > Utilize the DM core to power off the device power domain, but add a device
> > flag to be able to selectively let the power domain enabled after device
> > removal. This might be required for devices that must remain enabled when
> > booting OS, i. e. serial console for debug output, etc.
> >
> > Signed-off-by: Anatolij Gustschin <agust@denx.de>
> > ---
> > Changes in v2:
> >  - use CONFIG_IS_ENABLED(POWER_DOMAIN) to reduce code size
> >
> >  drivers/core/device-remove.c | 9 +++++++++
> >  include/dm/device.h          | 6 ++++++
> >  2 files changed, 15 insertions(+)

Unfortunately this causes a test failure (make qcheck). Can you please
take a look?

Regards,
Simon

[U-Boot] [PATCH v2 1/2] dm: core: device: switch off power domain after device removal

[Anatolij Gustschin]

Hi Simon,

On Thu, 18 Jul 2019 09:22:20 -0600
Simon Glass sjg at chromium.org wrote:
...
> > >  drivers/core/device-remove.c | 9 +++++++++
> > >  include/dm/device.h          | 6 ++++++
> > >  2 files changed, 15 insertions(+)  
> 
> Unfortunately this causes a test failure (make qcheck). Can you please
> take a look?

The dm power_domain test worked, but later when
 dm_test_destroy()
  -> uclass_destroy()
removes devices, first 'power-domain' device is removed, then
the 'power-domain-test' device. When removing the latter, we run

 if (!power_domain_get(dev, &pd))
       power_domain_off(&pd);

and this probes sandbox_power_domain driver for 'power-domain' device
activating this device again. Then 'power-domain-test' device is removed,
but 'power-domain' is active. When unbinding it later, we get this error
in device_unbind():

        if (dev->flags & DM_FLAG_ACTIVATED)
                return -EINVAL;

Following will fix it:

diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
index 586fadee0a..fadb05c944 100644
--- a/drivers/core/device-remove.c
+++ b/drivers/core/device-remove.c
@@ -64,7 +64,8 @@ int device_unbind(struct udevice *dev)
        if (!dev)
                return -EINVAL;
 
-       if (dev->flags & DM_FLAG_ACTIVATED)
+       if (dev->flags & DM_FLAG_ACTIVATED &&
+           device_get_uclass_id(dev) != UCLASS_POWER_DOMAIN)
                return -EINVAL;
 
        if (!(dev->flags & DM_FLAG_BOUND))

But I'm not sure if this it the correct approach. What do you think?

Thanks,
Anatolij

[U-Boot] [PATCH v2 1/2] dm: core: device: switch off power domain after device removal

[Simon Glass]

Hi Anatolij,

On Wed, 31 Jul 2019 at 10:01, Anatolij Gustschin <agust@denx.de> wrote:
>
> Hi Simon,
>
> On Thu, 18 Jul 2019 09:22:20 -0600
> Simon Glass sjg at chromium.org wrote:
> ...
> > > >  drivers/core/device-remove.c | 9 +++++++++
> > > >  include/dm/device.h          | 6 ++++++
> > > >  2 files changed, 15 insertions(+)
> >
> > Unfortunately this causes a test failure (make qcheck). Can you please
> > take a look?
>
> The dm power_domain test worked, but later when
>  dm_test_destroy()
>   -> uclass_destroy()
> removes devices, first 'power-domain' device is removed, then
> the 'power-domain-test' device. When removing the latter, we run
>
>  if (!power_domain_get(dev, &pd))
>        power_domain_off(&pd);
>
> and this probes sandbox_power_domain driver for 'power-domain' device
> activating this device again. Then 'power-domain-test' device is removed,
> but 'power-domain' is active. When unbinding it later, we get this error
> in device_unbind():
>
>         if (dev->flags & DM_FLAG_ACTIVATED)
>                 return -EINVAL;

This is because you are not allowed to unbind an active device. You
must deactivate it (device_remove()) first.

>
> Following will fix it:
>
> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
> index 586fadee0a..fadb05c944 100644
> --- a/drivers/core/device-remove.c
> +++ b/drivers/core/device-remove.c
> @@ -64,7 +64,8 @@ int device_unbind(struct udevice *dev)
>         if (!dev)
>                 return -EINVAL;
>
> -       if (dev->flags & DM_FLAG_ACTIVATED)
> +       if (dev->flags & DM_FLAG_ACTIVATED &&
> +           device_get_uclass_id(dev) != UCLASS_POWER_DOMAIN)
>                 return -EINVAL;
>
>         if (!(dev->flags & DM_FLAG_BOUND))
>
> But I'm not sure if this it the correct approach. What do you think?

That doesn't look right to me. Power supplies should be removed before
being unbound, just like any other device.

Regards,
Simon

[U-Boot] [PATCH v2 1/2] dm: core: device: switch off power domain after device removal

[Anatolij Gustschin]

Hi Simon,

On Wed, 31 Jul 2019 10:29:50 -0600
Simon Glass sjg at chromium.org wrote:
...
>> But I'm not sure if this it the correct approach. What do you think?  
> 
> That doesn't look right to me. Power supplies should be removed before
> being unbound, just like any other device.

OK, I tried to remove the associated power domain device explicitly
and this seems to work, at least with sandbox power domain driver.
Will rework the patch and resend. Thanks!

--
Anatolij

[U-Boot] [PATCH v2 1/2] dm: core: device: switch off power domain after device removal

[Lokesh Vutla]

On 15/07/19 1:27 AM, Anatolij Gustschin wrote:
> The power domain associated with a device is enabled when probing,
> but currently the domain remains enabled when the device is removed.
> Some boards started to disable power domains for selected devices
> via custom board_quiesce_devices(), but it doesn't work in many
> cases, i. e. because devices still can be accessed later in
> .remove() callback on behalf of dm_remove_devices_flags().
> 
> Utilize the DM core to power off the device power domain, but add a
> device flag to be able to selectively let the power domain enabled
> after device removal. This might be required for devices that must
> remain enabled when booting OS, i. e. serial console for debug
> output, etc.
> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
> Changes in v2:
>  - use CONFIG_IS_ENABLED(POWER_DOMAIN) to reduce code size
> 
>  drivers/core/device-remove.c | 9 +++++++++
>  include/dm/device.h          | 6 ++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
> index 586fadee0a..abed84a652 100644
> --- a/drivers/core/device-remove.c
> +++ b/drivers/core/device-remove.c
> @@ -16,6 +16,7 @@
>  #include <dm/uclass.h>
>  #include <dm/uclass-internal.h>
>  #include <dm/util.h>
> +#include <power-domain.h>
>  
>  int device_chld_unbind(struct udevice *dev, struct driver *drv)
>  {
> @@ -154,6 +155,7 @@ static bool flags_remove(uint flags, uint drv_flags)
>  
>  int device_remove(struct udevice *dev, uint flags)
>  {
> +	struct power_domain pd;
>  	const struct driver *drv;
>  	int ret;
>  
> @@ -192,6 +194,13 @@ int device_remove(struct udevice *dev, uint flags)
>  		}
>  	}
>  
> +	if (CONFIG_IS_ENABLED(POWER_DOMAIN) && dev->parent &&
> +	    device_get_uclass_id(dev) != UCLASS_POWER_DOMAIN &&
> +	    !(dev->flags & DM_FLAG_REMOVE_WITH_PD_ON)) {

This is going to hit every board and all serial drivers needs to be updated. Can
we add an extra check here for (dev != gd->cur_serial_dev).

Thanks and regards,
Lokesh

> +		if (!power_domain_get(dev, &pd))
> +			power_domain_off(&pd);
> +	}
> +
>  	if (flags_remove(flags, drv->flags)) {
>  		device_free(dev);
>  
> diff --git a/include/dm/device.h b/include/dm/device.h
> index 27a6d7b9fd..9a98a4a39e 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -61,6 +61,12 @@ struct driver_info;
>   */
>  #define DM_FLAG_OS_PREPARE		(1 << 10)
>  
> +/*
> + * Device is removed without switching off its power domain. This might
> + * be required, i. e. for serial console (debug) output when booting OS.
> + */
> +#define DM_FLAG_REMOVE_WITH_PD_ON	(1 << 11)
> +
>  /*
>   * 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
> 

[U-Boot] [PATCH v2 1/2] dm: core: device: switch off power domain after device removal

[Anatolij Gustschin]

Hi Lokesh,

On Tue, 23 Jul 2019 19:35:43 +0530
Lokesh Vutla lokeshvutla at ti.com wrote:
...  
> > +	if (CONFIG_IS_ENABLED(POWER_DOMAIN) && dev->parent &&
> > +	    device_get_uclass_id(dev) != UCLASS_POWER_DOMAIN &&
> > +	    !(dev->flags & DM_FLAG_REMOVE_WITH_PD_ON)) {  
> 
> This is going to hit every board and all serial drivers needs to be updated. Can
> we add an extra check here for (dev != gd->cur_serial_dev).

Will do it in v3 patch, thanks!

--
Anatolij