[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 at 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