[U-Boot] [PATCH v3] dm: core: device: switch off power domain after device removal

[U-Boot] [PATCH v3] dm: core: device: switch off power domain after device removal

[Anatolij Gustschin]

Hi Lokesh,

On Thu, 1 Aug 2019 09:43:39 +0530
Lokesh Vutla lokeshvutla at ti.com wrote:

>On 01/08/19 2:55 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 v3:
>>  - remove 'power-domain' device to fix dm power-domain test (make qcheck)
>>  - don't switch off the power domain for current console device
>> 
>> Changes in v2:
>>  - use CONFIG_IS_ENABLED(POWER_DOMAIN) to reduce code size
>> 
>>  drivers/core/device-remove.c | 18 ++++++++++++++++++
>>  include/dm/device.h          |  6 ++++++
>>  2 files changed, 24 insertions(+)
>> 
>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
>> index 586fadee0a..5a0e48e182 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)
>>  {
>> @@ -155,6 +156,7 @@ static bool flags_remove(uint flags, uint drv_flags)
>>  int device_remove(struct udevice *dev, uint flags)
>>  {
>>  	const struct driver *drv;
>> +	struct power_domain pd;
>>  	int ret;
>>  
>>  	if (!dev)
>> @@ -192,6 +194,22 @@ 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 != gd->cur_serial_dev &&
>> +	    !(dev->flags & DM_FLAG_REMOVE_WITH_PD_ON)) {  
>
>I did not realize before but this statement is too hard to read :). Can we drop
>the following two conditions:
>	dev->parent && device_get_uclass_id(dev) != UCLASS_POWER_DOMAIN
>
>power_domain_get() will indirectly take care of above two cases.

I'll test it.

>> +		if (!power_domain_get(dev, &pd)) {
>> +			power_domain_off(&pd);
>> +			/*
>> +			 * power_domain_get() bound the device, thus
>> +			 * we must remove it again to prevent unbinding
>> +			 * active devices (which would result in unbind
>> +			 * error).
>> +			 */
>> +			device_remove(pd.dev, DM_REMOVE_NORMAL);  
>
>Unless I am mistaken, this looks like this is an extra work. Why can't we store
>a PD in struct device, something like below:
>
>diff --git a/drivers/core/device.c b/drivers/core/device.c
>index cded457e2b..efa780a02f 100644
>--- a/drivers/core/device.c
>+++ b/drivers/core/device.c
>@@ -307,7 +307,6 @@ static void *alloc_priv(int size, uint flags)
>
> int device_probe(struct udevice *dev)
> {
>-	struct power_domain pd;
> 	const struct driver *drv;
> 	int size = 0;
> 	int ret;
>@@ -388,10 +387,11 @@ int device_probe(struct udevice *dev)
> 	if (dev->parent && device_get_uclass_id(dev) != UCLASS_PINCTRL)
> 		pinctrl_select_state(dev, "default");
>
>+	dev->pd.dev = NULL;
> 	if (dev->parent && device_get_uclass_id(dev) != UCLASS_POWER_DOMAIN &&
> 	    !(drv->flags & DM_FLAG_DEFAULT_PD_EN_OFF)) {
>-		if (!power_domain_get(dev, &pd))
>-			power_domain_on(&pd);
>+		if (!power_domain_get(dev, &dev->pd))
>+			power_domain_on(&dev->pd);
> 	}
>
> 	ret = uclass_pre_probe_device(dev);
>diff --git a/include/dm/device.h b/include/dm/device.h
>index b892386dc4..26699ca5cc 100644
>--- a/include/dm/device.h
>+++ b/include/dm/device.h
>@@ -12,12 +12,14 @@
>
> #include <dm/ofnode.h>
> #include <dm/uclass-id.h>
>+#include <errno.h>
> #include <fdtdec.h>
> #include <linker_lists.h>
> #include <linux/compat.h>
> #include <linux/kernel.h>
> #include <linux/list.h>
> #include <linux/printk.h>
>+#include <power-domain.h>
>
> struct driver_info;
>
>@@ -126,6 +128,7 @@ enum {
>  *		When CONFIG_DEVRES is enabled, devm_kmalloc() and friends will
>  *		add to this list. Memory so-allocated will be freed
>  *		automatically when the device is removed / unbound
>+ * @pd: Pointer to power_domain controlling this device.
>  */
> struct udevice {
> 	const struct driver *driver;
>@@ -149,6 +152,7 @@ struct udevice {
> #ifdef CONFIG_DEVRES
> 	struct list_head devres_head;
> #endif
>+	struct power_domain pd;
> };
>
> /* Maximum sequence number supported */

I tried similar change, this didn't work yet. When booting Linux I see
infinite recursion in device_remove() for 'lsio_gpio0' on i.mx8qxp.

--
Anatolij

[U-Boot] [PATCH v3] dm: core: device: switch off power domain after device removal

[Simon Glass]

Hi Anatolij,

On Thu, 1 Aug 2019 at 00:44, Anatolij Gustschin <agust@denx.de> wrote:
>
> Hi Lokesh,
>
> On Thu, 1 Aug 2019 09:43:39 +0530
> Lokesh Vutla lokeshvutla at ti.com wrote:
>
> >On 01/08/19 2:55 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 v3:
> >>  - remove 'power-domain' device to fix dm power-domain test (make qcheck)
> >>  - don't switch off the power domain for current console device

This still breaks 'make qcheck' for me. It doesn't build sandbox_spl:

crosfw -b sandbox_spl
cmd: 'make -j32 'ARCH=sandbox' 'CROSS_COMPILE=' --no-print-directory
'HOSTSTRIP=true' 'DEV_TREE_SRC=--default' 'QEMU_ARCH=' -s
'CROS_FULL=1' 'O=/tmp/b/sandbox_spl' 'BUILD_ROM=1'
'DEV_TREE_SEPARATE=1' all'
output: 'drivers/core/device-remove.o: In function `dev_power_domain_on':
/scratch/sglass/cosarm/src/third_party/u-boot/files/include/power-domain.h:172:
multiple definition of `dev_power_domain_on'
drivers/core/device.o:/scratch/sglass/cosarm/src/third_party/u-boot/files/include/power-domain.h:172:
first defined here
/scratch/sglass/cosarm/src/third_party/u-boot/files/scripts/Makefile.build:355:
recipe for target 'drivers/core/built-in.o' failed
make[3]: *** [drivers/core/built-in.o] Error 1
/scratch/sglass/cosarm/src/third_party/u-boot/files/scripts/Makefile.build:432:
recipe for target 'drivers/core' failed
make[2]: *** [drivers/core] Error 2
make[2]: *** Waiting for unfinished jobs....
/scratch/sglass/cosarm/src/third_party/u-boot/files/Makefile:1633:
recipe for target 'drivers' failed
make[1]: *** [drivers] Error 2
make[1]: *** Waiting for unfinished jobs....
Makefile:148: recipe for target 'sub-make' failed
make: *** [sub-make] Error 2

Regards,
SImon

[U-Boot] [PATCH v3] dm: core: device: switch off power domain after device removal

[Lokesh Vutla]

On 27/09/19 7:18 AM, Simon Glass wrote:
> Hi Anatolij,
> 
> On Thu, 1 Aug 2019 at 00:44, Anatolij Gustschin <agust@denx.de> wrote:
>>
>> Hi Lokesh,
>>
>> On Thu, 1 Aug 2019 09:43:39 +0530
>> Lokesh Vutla lokeshvutla at ti.com wrote:
>>
>>> On 01/08/19 2:55 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 v3:
>>>>  - remove 'power-domain' device to fix dm power-domain test (make qcheck)
>>>>  - don't switch off the power domain for current console device
> 
> This still breaks 'make qcheck' for me. It doesn't build sandbox_spl:
> 
> crosfw -b sandbox_spl
> cmd: 'make -j32 'ARCH=sandbox' 'CROSS_COMPILE=' --no-print-directory
> 'HOSTSTRIP=true' 'DEV_TREE_SRC=--default' 'QEMU_ARCH=' -s
> 'CROS_FULL=1' 'O=/tmp/b/sandbox_spl' 'BUILD_ROM=1'
> 'DEV_TREE_SEPARATE=1' all'
> output: 'drivers/core/device-remove.o: In function `dev_power_domain_on':
> /scratch/sglass/cosarm/src/third_party/u-boot/files/include/power-domain.h:172:
> multiple definition of `dev_power_domain_on'

dev_power_domain_on is not yet in the mainline. I guess this is caused by v2 of
dev_power_domain_on() series. Looks like that is fixed by [0]. So, I rebased
this patch on top of [0] and posted it[1]. Anatolij, I hope you don't mind :)

[0] https://patchwork.ozlabs.org/project/uboot/list/?series=132841
[1] https://patchwork.ozlabs.org/project/uboot/list/?series=132880

Thanks and regards,
Lokesh

> drivers/core/device.o:/scratch/sglass/cosarm/src/third_party/u-boot/files/include/power-domain.h:172:
> first defined here
> /scratch/sglass/cosarm/src/third_party/u-boot/files/scripts/Makefile.build:355:
> recipe for target 'drivers/core/built-in.o' failed
> make[3]: *** [drivers/core/built-in.o] Error 1
> /scratch/sglass/cosarm/src/third_party/u-boot/files/scripts/Makefile.build:432:
> recipe for target 'drivers/core' failed
> make[2]: *** [drivers/core] Error 2
> make[2]: *** Waiting for unfinished jobs....
> /scratch/sglass/cosarm/src/third_party/u-boot/files/Makefile:1633:
> recipe for target 'drivers' failed
> make[1]: *** [drivers] Error 2
> make[1]: *** Waiting for unfinished jobs....
> Makefile:148: recipe for target 'sub-make' failed
> make: *** [sub-make] Error 2
> 
> Regards,
> SImon
> 

[U-Boot] [PATCH v3] 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 v3:
 - remove 'power-domain' device to fix dm power-domain test (make qcheck)
 - don't switch off the power domain for current console device

Changes in v2:
 - use CONFIG_IS_ENABLED(POWER_DOMAIN) to reduce code size

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

diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
index 586fadee0a..5a0e48e182 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)
 {
@@ -155,6 +156,7 @@ static bool flags_remove(uint flags, uint drv_flags)
 int device_remove(struct udevice *dev, uint flags)
 {
 	const struct driver *drv;
+	struct power_domain pd;
 	int ret;
 
 	if (!dev)
@@ -192,6 +194,22 @@ 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 != gd->cur_serial_dev &&
+	    !(dev->flags & DM_FLAG_REMOVE_WITH_PD_ON)) {
+		if (!power_domain_get(dev, &pd)) {
+			power_domain_off(&pd);
+			/*
+			 * power_domain_get() bound the device, thus
+			 * we must remove it again to prevent unbinding
+			 * active devices (which would result in unbind
+			 * error).
+			 */
+			device_remove(pd.dev, DM_REMOVE_NORMAL);
+		}
+	}
+
 	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