From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anatolij Gustschin Date: Thu, 1 Aug 2019 09:44:24 +0200 Subject: [U-Boot] [PATCH v3] dm: core: device: switch off power domain after device removal In-Reply-To: References: <20190731212538.9002-1-agust@denx.de> Message-ID: <20190801094424.7d19d3ee@crub> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> --- >> 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 >> #include >> #include >> +#include >> >> 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 > #include >+#include > #include > #include > #include > #include > #include > #include >+#include > > 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