diff mbox series

[RFC,2/2] Revert "dm: core: device: switch off power domain after device removal"

Message ID 20200203135619.10180-3-oliver.graute@kococonnector.com
State New
Headers show
Series [v2] imx: support for conga-QMX8 board | expand

Commit Message

Oliver Graute Feb. 3, 2020, 1:59 p.m. UTC 
The conga-imx8 board isn't booting without this revert. Can someone tell me
what I need to Do so that this revert is not necessary?

This reverts commit 52edfed65de967a86983a55c51ba0727090efc43.
---
 drivers/core/device-remove.c | 5 -----
 1 file changed, 5 deletions(-)

Comments

Neil Armstrong Feb. 13, 2020, 5:12 p.m. UTC | #1 
Hi,

On 03/02/2020 14:59, Oliver Graute wrote:
> The conga-imx8 board isn't booting without this revert. Can someone tell me
> what I need to Do so that this revert is not necessary?
> 
> This reverts commit 52edfed65de967a86983a55c51ba0727090efc43.
> ---
>  drivers/core/device-remove.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
> index 5c8dc4ad70..586fadee0a 100644
> --- a/drivers/core/device-remove.c
> +++ b/drivers/core/device-remove.c
> @@ -16,7 +16,6 @@
>  #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)
>  {
> @@ -193,10 +192,6 @@ int device_remove(struct udevice *dev, uint flags)
>  		}
>  	}
>  
> -	if (!(drv->flags & DM_FLAG_DEFAULT_PD_CTRL_OFF) &&
> -	    (dev != gd->cur_serial_dev))
> -		dev_power_domain_off(dev);
> -
>  	if (flags_remove(flags, drv->flags)) {
>  		device_free(dev);
>  
> 

It also breaks amlogic boards with video, since we can't tell we want
to keep the power domains up after boot, it cuts the power domains and
thus defeats all the goal of u-boot video...

The problem is DM_FLAG_DEFAULT_PD_CTRL_OFF disables auto power domain on
aswell, reverting to manual power domain enable.

Neil
Lokesh Vutla Feb. 14, 2020, 4:20 a.m. UTC | #2 
On 13/02/20 10:42 PM, Neil Armstrong wrote:
> Hi,
> 
> On 03/02/2020 14:59, Oliver Graute wrote:
>> The conga-imx8 board isn't booting without this revert. Can someone tell me
>> what I need to Do so that this revert is not necessary?

Can you give more details on where the failure is happening?(logs would really
help).

>>
>> This reverts commit 52edfed65de967a86983a55c51ba0727090efc43.
>> ---
>>  drivers/core/device-remove.c | 5 -----
>>  1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
>> index 5c8dc4ad70..586fadee0a 100644
>> --- a/drivers/core/device-remove.c
>> +++ b/drivers/core/device-remove.c
>> @@ -16,7 +16,6 @@
>>  #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)
>>  {
>> @@ -193,10 +192,6 @@ int device_remove(struct udevice *dev, uint flags)
>>  		}
>>  	}
>>  
>> -	if (!(drv->flags & DM_FLAG_DEFAULT_PD_CTRL_OFF) &&
>> -	    (dev != gd->cur_serial_dev))
>> -		dev_power_domain_off(dev);
>> -
>>  	if (flags_remove(flags, drv->flags)) {
>>  		device_free(dev);
>>  
>>
> 
> It also breaks amlogic boards with video, since we can't tell we want
> to keep the power domains up after boot, it cuts the power domains and
> thus defeats all the goal of u-boot video..
> The problem is DM_FLAG_DEFAULT_PD_CTRL_OFF disables auto power domain on
> aswell, reverting to manual power domain enable.

This doesn't mean U-boot must leave the all power-domains un handled before
jumping to kernel. Please use DM_FLAG_DEFAULT_PD_CTRL_OFF in you driver and
enable power_domain in your probe. Your case is exactly the reason why
DM_FLAG_DEFAULT_PD_CTRL_OFF is introduced.

Thanks and regards,
Lokesh
Oliver Graute Feb. 14, 2020, 9:18 a.m. UTC | #3 
On 14/02/20, Lokesh Vutla wrote:
> 
> 
> On 13/02/20 10:42 PM, Neil Armstrong wrote:
> > Hi,
> > 
> > On 03/02/2020 14:59, Oliver Graute wrote:
> >> The conga-imx8 board isn't booting without this revert. Can someone tell me
> >> what I need to Do so that this revert is not necessary?
> 
> Can you give more details on where the failure is happening?(logs would really
> help).

without that revert patch u-boot is stuck at "Starting kernel ..."
Is there a way to get more details here? currently I don't know howto
debug that power-domain stuff deeper.

Bytes transferred = 27030016 (19c7200 hex)
Using ethernet at 5b040000 device
TFTP from server 10.200.134.119; our IP address is 10.200.134.121
Filename 'imx8qm-cgt-qmx8.dtb'.
Load address: 0x83000000
Loading: #####
         2.5 MiB/s
done
Bytes transferred = 21224 (52e8 hex)
## Flattened Device Tree blob at 83000000
   Booting using the fdt blob at 0x83000000
   Using Device Tree in place at 0000000083000000, end 00000000830082e7

Starting kernel ...

Best regards,

Oliver
Lokesh Vutla Feb. 14, 2020, 9:30 a.m. UTC | #4 
On 14/02/20 2:48 PM, Oliver Graute wrote:
> On 14/02/20, Lokesh Vutla wrote:
>>
>>
>> On 13/02/20 10:42 PM, Neil Armstrong wrote:
>>> Hi,
>>>
>>> On 03/02/2020 14:59, Oliver Graute wrote:
>>>> The conga-imx8 board isn't booting without this revert. Can someone tell me
>>>> what I need to Do so that this revert is not necessary?
>>
>> Can you give more details on where the failure is happening?(logs would really
>> help).
> 
> without that revert patch u-boot is stuck at "Starting kernel ..."
> Is there a way to get more details here? currently I don't know howto
> debug that power-domain stuff deeper.

Can you enable debug prints in:
- drivers/power/domain/power-domain-uclass.c
- drivers/power/domain/imx8-power-domain*.c

and provide the logs? this will help which power-domain is causing the issue.

Thanks and regards,
Lokesh

> 
> Bytes transferred = 27030016 (19c7200 hex)
> Using ethernet at 5b040000 device
> TFTP from server 10.200.134.119; our IP address is 10.200.134.121
> Filename 'imx8qm-cgt-qmx8.dtb'.
> Load address: 0x83000000
> Loading: #####
>          2.5 MiB/s
> done
> Bytes transferred = 21224 (52e8 hex)
> ## Flattened Device Tree blob at 83000000
>    Booting using the fdt blob at 0x83000000
>    Using Device Tree in place at 0000000083000000, end 00000000830082e7
> 
> Starting kernel ...
> 
> Best regards,
> 
> Oliver
>
Oliver Graute Feb. 14, 2020, 10:30 a.m. UTC | #5 
On 14/02/20, Lokesh Vutla wrote:
> 
> 
> On 14/02/20 2:48 PM, Oliver Graute wrote:
> > On 14/02/20, Lokesh Vutla wrote:
> >>
> >>
> >> On 13/02/20 10:42 PM, Neil Armstrong wrote:
> >>> Hi,
> >>>
> >>> On 03/02/2020 14:59, Oliver Graute wrote:
> >>>> The conga-imx8 board isn't booting without this revert. Can someone tell me
> >>>> what I need to Do so that this revert is not necessary?
> >>
> >> Can you give more details on where the failure is happening?(logs would really
> >> help).
> > 
> > without that revert patch u-boot is stuck at "Starting kernel ..."
> > Is there a way to get more details here? currently I don't know howto
> > debug that power-domain stuff deeper.
> 
> Can you enable debug prints in:
> - drivers/power/domain/power-domain-uclass.c
> - drivers/power/domain/imx8-power-domain*.c
> 
> and provide the logs? this will help which power-domain is causing the issue.

thx a lot that helped. So it looks like the board is in some endless
loop with lsio_power_domain.

here the log:

Loading: #####
         2 MiB/s
done
Bytes transferred = 21224 (52e8 hex)
## Flattened Device Tree blob at 83000000
   Booting using the fdt blob at 0x83000000
   Using Device Tree in place at 0000000083000000, end 00000000830082e7

Starting kernel ...

power_domain_get_by_index() power_domain_get_by_index(dev=00000000fdae88f0, power_domain=00000000fdae0058)
imx8_power_domain_of_xlate() imx8_power_domain_of_xlate(power_domain=00000000fdae0058)
imx8_power_domain_request() imx8_power_domain_request(power_domain=00000000fdae0058)
power_domain_off() power_domain_off(power_domain=00000000fdae0058)
imx8_power_domain_off() imx8_power_domain_off(power_domain=00000000fdae0058)
imx8_power_domain_off_node() imx8_power_domain_off_node, lsio_power_domain, state_on 1
imx8_power_domain_off() Can't power off the node of dev lsio_power_domain, ret = -1
power_domain_get_by_index() power_domain_get_by_index(dev=00000000fdae88f0, power_domain=00000000fdadff48)
imx8_power_domain_of_xlate() imx8_power_domain_of_xlate(power_domain=00000000fdadff48)
imx8_power_domain_request() imx8_power_domain_request(power_domain=00000000fdadff48)
power_domain_off() power_domain_off(power_domain=00000000fdadff48)
imx8_power_domain_off() imx8_power_domain_off(power_domain=00000000fdadff48)
imx8_power_domain_off_node() imx8_power_domain_off_node, lsio_power_domain, state_on 1
imx8_power_domain_off() Can't power off the node of dev lsio_power_domain, ret = -1
power_domain_get_by_index() power_domain_get_by_index(dev=00000000fdae88f0, power_domain=00000000fdadfe38)
imx8_power_domain_of_xlate() imx8_power_domain_of_xlate(power_domain=00000000fdadfe38)
imx8_power_domain_request() imx8_power_domain_request(power_domain=00000000fdadfe38)
power_domain_off() power_domain_off(power_domain=00000000fdadfe38)
imx8_power_domain_off() imx8_power_domain_off(power_domain=00000000fdadfe38)
imx8_power_domain_off_node() imx8_power_domain_off_node, lsio_power_domain, state_on 1
imx8_power_domain_off() Can't power off the node of dev lsio_power_domain, ret = -1
power_domain_get_by_index() power_domain_get_by_index(dev=00000000fdae88f0, power_domain=00000000fdadfd28)
imx8_power_domain_of_xlate() imx8_power_domain_of_xlate(power_domain=00000000fdadfd28)
imx8_power_domain_request() imx8_power_domain_request(power_domain=00000000fdadfd28)
power_domain_off() power_domain_off(power_domain=00000000fdadfd28)
imx8_power_domain_off() imx8_power_domain_off(power_domain=00000000fdadfd28)
imx8_power_domain_off_node() imx8_power_domain_off_node, lsio_power_domain, state_on 1
imx8_power_domain_off() Can't power off the node of dev lsio_power_domain, ret = -1
power_domain_get_by_index() power_domain_get_by_index(dev=00000000fdae88f0, power_domain=00000000fdadfc18)
imx8_power_domain_of_xlate() imx8_power_domain_of_xlate(power_domain=00000000fdadfc18)
imx8_power_domain_request() imx8_power_domain_request(power_domain=00000000fdadfc18)
power_domain_off() power_domain_off(power_domain=00000000fdadfc18)
imx8_power_domain_off() imx8_power_domain_off(power_domain=00000000fdadfc18)
imx8_power_domain_off_node() imx8_power_domain_off_node, lsio_power_domain, state_on 1
imx8_power_domain_off() Can't power off the node of dev lsio_power_domain, ret = -1
power_domain_get_by_index() power_domain_get_by_index(dev=00000000fdae88f0, power_domain=00000000fdadfb08)
imx8_power_domain_of_xlate() imx8_power_domain_of_xlate(power_domain=00000000fdadfb08)
imx8_power_domain_request() imx8_power_domain_request(power_domain=00000000fdadfb08)
power_domain_off() power_domain_off(power_domain=00000000fdadfb08)
imx8_power_domain_off() imx8_power_domain_off(power_domain=00000000fdadfb08)
imx8_power_domain_off_node() imx8_power_domain_off_node, lsio_power_domain, state_on 1
imx8_power_domain_off() Can't power off the node of dev lsio_power_domain, ret = -1
power_domain_get_by_index() power_domain_get_by_index(dev=00000000fdae88f0, power_domain=00000000fdadf9f8)
imx8_power_domain_of_xlate() imx8_power_domain_of_xlate(power_domain=00000000fdadf9f8)
imx8_power_domain_request() imx8_power_domain_request(power_domain=00000000fdadf9f8)
power_domain_off() power_domain_off(power_domain=00000000fdadf9f8)
imx8_power_domain_off() imx8_power_domain_off(power_domain=00000000fdadf9f8)
imx8_power_domain_off_node() imx8_power_domain_off_node, lsio_power_domain, state_on 1
imx8_power_domain_off() Can't power off the node of dev lsio_power_domain, ret = -1
power_domain_get_by_index() power_domain_get_by_index(dev=00000000fdae88f0, power_domain=00000000fdadf8e8)
imx8_power_domain_of_xlate() imx8_power_domain_of_xlate(power_domain=00000000fdadf8e8)
imx8_power_domain_request() imx8_power_domain_request(power_domain=00000000fdadf8e8)
power_domain_off() power_domain_off(power_domain=00000000fdadf8e8)
imx8_power_domain_off() imx8_power_domain_off(power_domain=00000000fdadf8e8)
imx8_power_domain_off_node() imx8_power_domain_off_node, lsio_power_domain, state_on 1
imx8_power_domain_off() Can't power off the node of dev lsio_power_domain, ret = -1
power_domain_get_by_index() power_domain_get_by_index(dev=00000000fdae88f0, power_domain=00000000fdadf7d8)
imx8_power_domain_of_xlate() imx8_power_domain_of_xlate(power_domain=00000000fdadf7d8)
imx8_power_domain_request() imx8_power_domain_request(power_domain=00000000fdadf7d8)
power_domain_off() power_domain_off(power_domain=00000000fdadf7d8)
imx8_power_domain_off() imx8_power_domain_off(power_domain=00000000fdadf7d8)
imx8_power_domain_off_node() imx8_power_domain_off_node, lsio_power_domain, state_on 1
imx8_power_domain_off() Can't power off the node of dev lsio_power_domain, ret = -1
power_domain_get_by_index() power_domain_get_by_index(dev=00000000fdae88f0, power_domain=00000000fdadf6c8)
imx8_power_domain_of_xlate() imx8_power_domain_of_xlate(power_domain=00000000fdadf6c8)
imx8_power_domain_request() imx8_power_domain_request(power_domain=00000000fdadf6c8)
power_domain_off() power_domain_off(power_domain=00000000fdadf6c8)
imx8_power_domain_off() imx8_power_domain_off(power_domain=00000000fdadf6c8)
imx8_power_domain_off_node() imx8_power_domain_off_node, lsio_power_domain, state_on 1
imx8_power_domain_off() Can't power off the node of dev lsio_power_domain, ret = -1
power_domain_get_by_index() power_domain_get_by_index(dev=00000000fdae88f0, power_domain=00000000fdadf5b8)
imx8_power_domain_of_xlate() imx8_power_domain_of_xlate(power_domain=00000000fdadf5b8)
imx8_power_domain_request() imx8_power_domain_request(power_domain=00000000fdadf5b8)
power_domain_off() power_domain_off(power_domain=00000000fdadf5b8)
imx8_power_domain_off() imx8_power_domain_off(power_domain=00000000fdadf5b8)
imx8_power_domain_off_node() imx8_power_domain_off_node, lsio_power_domain, state_on 1
imx8_power_domain_off() Can't power off the node of dev lsio_power_domain, ret = -1
power_domain_get_by_index() power_domain_get_by_index(dev=00000000fdae88f0, power_domain=00000000fdadf4a8)
imx8_power_domain_of_xlate() imx8_power_domain_of_xlate(power_domain=00000000fdadf4a8)
imx8_power_domain_request() imx8_power_domain_request(power_domain=00000000fdadf4a8)
power_domain_off() power_domain_off(power_domain=00000000fdadf4a8)
imx8_power_domain_off() imx8_power_domain_off(power_domain=00000000fdadf4a8)
imx8_power_domain_off_node() imx8_power_domain_off_node, lsio_power_domain, state_on 1
imx8_power_domain_off() Can't power off the node of dev lsio_power_domain, ret = -1
power_domain_get_by_index() power_domain_get_by_index(dev=00000000fdae88f0, power_domain=00000000fdadf398)

---snipped off--

Best Regards,

Oliver
Neil Armstrong Feb. 17, 2020, 8:55 a.m. UTC | #6 
On 14/02/2020 05:20, Lokesh Vutla wrote:
> 
> 
> On 13/02/20 10:42 PM, Neil Armstrong wrote:
>> Hi,
>>
>> On 03/02/2020 14:59, Oliver Graute wrote:
>>> The conga-imx8 board isn't booting without this revert. Can someone tell me
>>> what I need to Do so that this revert is not necessary?
> 
> Can you give more details on where the failure is happening?(logs would really
> help).
> 
>>>
>>> This reverts commit 52edfed65de967a86983a55c51ba0727090efc43.
>>> ---
>>>  drivers/core/device-remove.c | 5 -----
>>>  1 file changed, 5 deletions(-)
>>>
>>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
>>> index 5c8dc4ad70..586fadee0a 100644
>>> --- a/drivers/core/device-remove.c
>>> +++ b/drivers/core/device-remove.c
>>> @@ -16,7 +16,6 @@
>>>  #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)
>>>  {
>>> @@ -193,10 +192,6 @@ int device_remove(struct udevice *dev, uint flags)
>>>  		}
>>>  	}
>>>  
>>> -	if (!(drv->flags & DM_FLAG_DEFAULT_PD_CTRL_OFF) &&
>>> -	    (dev != gd->cur_serial_dev))
>>> -		dev_power_domain_off(dev);
>>> -
>>>  	if (flags_remove(flags, drv->flags)) {
>>>  		device_free(dev);
>>>  
>>>
>>
>> It also breaks amlogic boards with video, since we can't tell we want
>> to keep the power domains up after boot, it cuts the power domains and
>> thus defeats all the goal of u-boot video..
>> The problem is DM_FLAG_DEFAULT_PD_CTRL_OFF disables auto power domain on
>> aswell, reverting to manual power domain enable.
> 
> This doesn't mean U-boot must leave the all power-domains un handled before
> jumping to kernel. Please use DM_FLAG_DEFAULT_PD_CTRL_OFF in you driver and
> enable power_domain in your probe. Your case is exactly the reason why
> DM_FLAG_DEFAULT_PD_CTRL_OFF is introduced.

This changed the behavior of the power domain core, and this is wrong.

Reverting to manually handling the power domain in the probe defeats the initial
goal of automatic handling of power domain at probe, and the current
DM_FLAG_DEFAULT_PD_CTRL_OF doesn't mean anything, it's not explicit at all,
and doesn't mean the power domain won't be enabled at probe. We must read
the code to understand it.

Please avoid breaking stuff and adding fuzzy flags without a proper behavior.

The use-case of the flag would be to keep the power domain enabled at linux boot
instead of never probing it.

Neil

> 
> Thanks and regards,
> Lokesh
>
Anatolij Gustschin Feb. 17, 2020, 1:05 p.m. UTC | #7 
On Thu, 13 Feb 2020 18:12:12 +0100
Neil Armstrong narmstrong at baylibre.com wrote:

> Hi,
> 
> On 03/02/2020 14:59, Oliver Graute wrote:
> > The conga-imx8 board isn't booting without this revert. Can someone tell me
> > what I need to Do so that this revert is not necessary?
> > 
> > This reverts commit 52edfed65de967a86983a55c51ba0727090efc43.
> > ---
> >  drivers/core/device-remove.c | 5 -----
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
> > index 5c8dc4ad70..586fadee0a 100644
> > --- a/drivers/core/device-remove.c
> > +++ b/drivers/core/device-remove.c
> > @@ -16,7 +16,6 @@
> >  #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)
> >  {
> > @@ -193,10 +192,6 @@ int device_remove(struct udevice *dev, uint flags)
> >  		}
> >  	}
> >  
> > -	if (!(drv->flags & DM_FLAG_DEFAULT_PD_CTRL_OFF) &&
> > -	    (dev != gd->cur_serial_dev))
> > -		dev_power_domain_off(dev);
> > -
> >  	if (flags_remove(flags, drv->flags)) {
> >  		device_free(dev);
> >  
> >   
> 
> It also breaks amlogic boards with video, since we can't tell we want
> to keep the power domains up after boot, it cuts the power domains and
> thus defeats all the goal of u-boot video...
> 
> The problem is DM_FLAG_DEFAULT_PD_CTRL_OFF disables auto power domain on
> aswell, reverting to manual power domain enable.

Yes, in some special cases manual power domain enable is required,
this is the reason why this flag has been introduced. This flag is
not for controlling power off for auto-enabled domains.

My original patch v3 [1] suggested another flag DM_FLAG_REMOVE_WITH_PD_ON,
but in the subsequent patch (which was then merged) Lokesh replaced this
with another flag DM_FLAG_DEFAULT_PD_CTRL_OFF for different purpose and
dropped DM_FLAG_REMOVE_WITH_PD_ON.

Unfortunately Lokesh copied the commit message from my patch v3 to
the updated patch without modification and so this merged commit doesn't
correctly describe the code changes. Unfortunately I didn't have
time to properly review and test this merged patch back then.

[1] https://patchwork.ozlabs.org/patch/1140138

--
Anatolij
Anatolij Gustschin Feb. 17, 2020, 1:17 p.m. UTC | #8 
On Fri, 14 Feb 2020 09:50:54 +0530
Lokesh Vutla lokeshvutla at ti.com wrote:

> On 13/02/20 10:42 PM, Neil Armstrong wrote:
> > Hi,
> > 
> > On 03/02/2020 14:59, Oliver Graute wrote:  
> >> The conga-imx8 board isn't booting without this revert. Can someone tell me
> >> what I need to Do so that this revert is not necessary?  
> 
> Can you give more details on where the failure is happening?(logs would really
> help).
> 
> >>
> >> This reverts commit 52edfed65de967a86983a55c51ba0727090efc43.
> >> ---
> >>  drivers/core/device-remove.c | 5 -----
> >>  1 file changed, 5 deletions(-)
> >>
> >> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
> >> index 5c8dc4ad70..586fadee0a 100644
> >> --- a/drivers/core/device-remove.c
> >> +++ b/drivers/core/device-remove.c
> >> @@ -16,7 +16,6 @@
> >>  #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)
> >>  {
> >> @@ -193,10 +192,6 @@ int device_remove(struct udevice *dev, uint flags)
> >>  		}
> >>  	}
> >>  
> >> -	if (!(drv->flags & DM_FLAG_DEFAULT_PD_CTRL_OFF) &&
> >> -	    (dev != gd->cur_serial_dev))
> >> -		dev_power_domain_off(dev);
> >> -
> >>  	if (flags_remove(flags, drv->flags)) {
> >>  		device_free(dev);
> >>  
> >>  
> > 
> > It also breaks amlogic boards with video, since we can't tell we want
> > to keep the power domains up after boot, it cuts the power domains and
> > thus defeats all the goal of u-boot video..
> > The problem is DM_FLAG_DEFAULT_PD_CTRL_OFF disables auto power domain on
> > aswell, reverting to manual power domain enable.  
> 
> This doesn't mean U-boot must leave the all power-domains un handled before
> jumping to kernel. Please use DM_FLAG_DEFAULT_PD_CTRL_OFF in you driver and
> enable power_domain in your probe.

No, DM_FLAG_DEFAULT_PD_CTRL_OFF is only for very special cases where
manual power domain handling is required.

> Your case is exactly the reason why
> DM_FLAG_DEFAULT_PD_CTRL_OFF is introduced.

No. For this case the flag DM_FLAG_REMOVE_WITH_PD_ON has been introduced
in my v3 patch, but it was dropped in the merged patch. I've submitted a
new patch for this: http://patchwork.ozlabs.org/patch/1239143

--
Anatolij
Lokesh Vutla Feb. 17, 2020, 1:37 p.m. UTC | #9 
On 17/02/20 6:35 PM, Anatolij Gustschin wrote:
> On Thu, 13 Feb 2020 18:12:12 +0100
> Neil Armstrong narmstrong at baylibre.com wrote:
> 
>> Hi,
>>
>> On 03/02/2020 14:59, Oliver Graute wrote:
>>> The conga-imx8 board isn't booting without this revert. Can someone tell me
>>> what I need to Do so that this revert is not necessary?
>>>
>>> This reverts commit 52edfed65de967a86983a55c51ba0727090efc43.
>>> ---
>>>  drivers/core/device-remove.c | 5 -----
>>>  1 file changed, 5 deletions(-)
>>>
>>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
>>> index 5c8dc4ad70..586fadee0a 100644
>>> --- a/drivers/core/device-remove.c
>>> +++ b/drivers/core/device-remove.c
>>> @@ -16,7 +16,6 @@
>>>  #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)
>>>  {
>>> @@ -193,10 +192,6 @@ int device_remove(struct udevice *dev, uint flags)
>>>  		}
>>>  	}
>>>  
>>> -	if (!(drv->flags & DM_FLAG_DEFAULT_PD_CTRL_OFF) &&
>>> -	    (dev != gd->cur_serial_dev))
>>> -		dev_power_domain_off(dev);
>>> -
>>>  	if (flags_remove(flags, drv->flags)) {
>>>  		device_free(dev);
>>>  
>>>   
>>
>> It also breaks amlogic boards with video, since we can't tell we want
>> to keep the power domains up after boot, it cuts the power domains and
>> thus defeats all the goal of u-boot video...
>>
>> The problem is DM_FLAG_DEFAULT_PD_CTRL_OFF disables auto power domain on
>> aswell, reverting to manual power domain enable.
> 
> Yes, in some special cases manual power domain enable is required,
> this is the reason why this flag has been introduced. This flag is
> not for controlling power off for auto-enabled domains.
> 
> My original patch v3 [1] suggested another flag DM_FLAG_REMOVE_WITH_PD_ON,
> but in the subsequent patch (which was then merged) Lokesh replaced this
> with another flag DM_FLAG_DEFAULT_PD_CTRL_OFF for different purpose and
> dropped DM_FLAG_REMOVE_WITH_PD_ON.

May be you should see the patch that introduces the specific flag[0] :). Flag
DM_FLAG_REMOVE_WITH_PD_ON[1] does not have any users at that point. Why would we
introduce it without any users?. Even with the patch it would be breaking the
above driver unless explicitly enabled.

IMHO, it is dm core controlling the PD or not. Why will it control just the
enabling part. May be I am wrong, but anyways Ill let Simon and Tom to have a
decision on it.

[0] https://patchwork.ozlabs.org/patch/1168343/
[1] https://patchwork.ozlabs.org/patch/1140138/

Thanks and regards,
Lokesh
diff mbox series

Patch

diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
index 5c8dc4ad70..586fadee0a 100644
--- a/drivers/core/device-remove.c
+++ b/drivers/core/device-remove.c
@@ -16,7 +16,6 @@ 
 #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)
 {
@@ -193,10 +192,6 @@  int device_remove(struct udevice *dev, uint flags)
 		}
 	}
 
-	if (!(drv->flags & DM_FLAG_DEFAULT_PD_CTRL_OFF) &&
-	    (dev != gd->cur_serial_dev))
-		dev_power_domain_off(dev);
-
 	if (flags_remove(flags, drv->flags)) {
 		device_free(dev);