[PATCH] PM / Domains: If an OF node is found but no device probed yet, defer.

[PATCH] PM / Domains: If an OF node is found but no device probed yet, defer.

[Eric Anholt]

If we've declared a power domain in the OF, and the OF node is found
but the requested domain hasn't been registered on it yet, then we
probably have just tried to probe before the power domain driver has.
Defer our device's probe until it shows up.

Signed-off-by: Eric Anholt <eric@anholt.net>
---

I ran into this when turning my ad-hoc code for BCM2835 (Raspberry Pi)
USB poweron support in the DWC2 controller to an OF-based power domain
declaration.

drivers/base/power/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index ba4abbe..2b93c98 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2064,7 +2064,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
 struct generic_pm_domain *of_genpd_get_from_provider(
 					struct of_phandle_args *genpdspec)
 {
-	struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
+	struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
 	struct of_genpd_provider *provider;
 
 	mutex_lock(&of_genpd_mutex);
-- 
2.1.4

Re: [PATCH] PM / Domains: If an OF node is found but no device probed yet, defer.

[Rafael J. Wysocki]

More CCes.

On Wednesday, March 11, 2015 08:27:28 AM Eric Anholt wrote:
> If we've declared a power domain in the OF, and the OF node is found
> but the requested domain hasn't been registered on it yet, then we
> probably have just tried to probe before the power domain driver has.
> Defer our device's probe until it shows up.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>

Kevin, Ulf, any chance to have a look at this, please?

> ---
> 
> I ran into this when turning my ad-hoc code for BCM2835 (Raspberry Pi)
> USB poweron support in the DWC2 controller to an OF-based power domain
> declaration.
> 
> drivers/base/power/domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index ba4abbe..2b93c98 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2064,7 +2064,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>  struct generic_pm_domain *of_genpd_get_from_provider(
>  					struct of_phandle_args *genpdspec)
>  {
> -	struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
> +	struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
>  	struct of_genpd_provider *provider;
>  
>  	mutex_lock(&of_genpd_mutex);
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] PM / Domains: If an OF node is found but no device probed yet, defer.

[Geert Uytterhoeven]

On Wed, Mar 11, 2015 at 11:08 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> More CCes.
>
> On Wednesday, March 11, 2015 08:27:28 AM Eric Anholt wrote:
>> If we've declared a power domain in the OF, and the OF node is found
>> but the requested domain hasn't been registered on it yet, then we
>> probably have just tried to probe before the power domain driver has.
>> Defer our device's probe until it shows up.
>>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>
> Kevin, Ulf, any chance to have a look at this, please?
>
>> ---
>>
>> I ran into this when turning my ad-hoc code for BCM2835 (Raspberry Pi)
>> USB poweron support in the DWC2 controller to an OF-based power domain
>> declaration.

I guess you are initializing the PM domains from module_init()?

I use core_initcall() in arch/arm/mach-shmobile/pm-rmobile.c to make sure it's
initialized earlier, as e.g. the interrupt controller uses postcore_initcall().

>> drivers/base/power/domain.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index ba4abbe..2b93c98 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -2064,7 +2064,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>>  struct generic_pm_domain *of_genpd_get_from_provider(
>>                                       struct of_phandle_args *genpdspec)
>>  {
>> -     struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
>> +     struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);

Currently platform_drv_probe() just continues if dev_pm_domain_attach() returns
a different error than -EPROBE_DEFER, which is what you are seeing.

Your change does have the side effect that a new DT with PM domains won't
work on an older kernel that doesn't have the PM domain driver yet.

Whether this is a good or a bad thing depends on your bootloader. If all PM
domains are powered when Linux boots, it can work without PM domain driver.
Since DT PM domains are quite recent, I guess this is the case for most
existing SoCs.

>>       struct of_genpd_provider *provider;
>>
>>       mutex_lock(&of_genpd_mutex);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] PM / Domains: If an OF node is found but no device probed yet, defer.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 3188 bytes --]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> On Wed, Mar 11, 2015 at 11:08 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> More CCes.
>>
>> On Wednesday, March 11, 2015 08:27:28 AM Eric Anholt wrote:
>>> If we've declared a power domain in the OF, and the OF node is found
>>> but the requested domain hasn't been registered on it yet, then we
>>> probably have just tried to probe before the power domain driver has.
>>> Defer our device's probe until it shows up.
>>>
>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>
>> Kevin, Ulf, any chance to have a look at this, please?
>>
>>> ---
>>>
>>> I ran into this when turning my ad-hoc code for BCM2835 (Raspberry Pi)
>>> USB poweron support in the DWC2 controller to an OF-based power domain
>>> declaration.
>
> I guess you are initializing the PM domains from module_init()?
>
> I use core_initcall() in arch/arm/mach-shmobile/pm-rmobile.c to make sure it's
> initialized earlier, as e.g. the interrupt controller uses postcore_initcall().

Yeah, it's just going through normal DT-based module initialization as a
module_platform_driver().  And it depends on another platform driver
(the mailbox to talk to the firmware in the first place), so it's
unlikely to show up early.

The BCM2835 architecture maintainers don't appear to be fans of fixed
init sequence stuff (our sequence is just bcm2835_setup_restart(),
bcm2835_init_clocks(), of_platform_populate()), and I figured
dependencies expressed in DT were supposed to be the ideal way for
things these days.  I can try turning my drivers into fixed init
sequence code, but I don't expect this to go over well.

>>> drivers/base/power/domain.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index ba4abbe..2b93c98 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -2064,7 +2064,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>>>  struct generic_pm_domain *of_genpd_get_from_provider(
>>>                                       struct of_phandle_args *genpdspec)
>>>  {
>>> -     struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
>>> +     struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
>
> Currently platform_drv_probe() just continues if dev_pm_domain_attach() returns
> a different error than -EPROBE_DEFER, which is what you are seeing.
>
> Your change does have the side effect that a new DT with PM domains won't
> work on an older kernel that doesn't have the PM domain driver yet.
>
> Whether this is a good or a bad thing depends on your bootloader. If all PM
> domains are powered when Linux boots, it can work without PM domain driver.
> Since DT PM domains are quite recent, I guess this is the case for most
> existing SoCs.

The main bootloader doesn't turn on these domains, thus why I wrote a
driver for it (so I could get USB and thus netowrking).  You can
chainload U-Boot and it'll turn it on USB for you, though.

Is there a way we could maybe tell if there's a driver due to try
probing on the node but that hasn't had a chance yet?  That could
mitigate the backwards kernel compat for new DT problem.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] PM / Domains: If an OF node is found but no device probed yet, defer.

[Kevin Hilman]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> On Wed, Mar 11, 2015 at 11:08 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> More CCes.
>>
>> On Wednesday, March 11, 2015 08:27:28 AM Eric Anholt wrote:
>>> If we've declared a power domain in the OF, and the OF node is found
>>> but the requested domain hasn't been registered on it yet, then we
>>> probably have just tried to probe before the power domain driver has.
>>> Defer our device's probe until it shows up.
>>>
>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>
>> Kevin, Ulf, any chance to have a look at this, please?
>>
>>> ---
>>>
>>> I ran into this when turning my ad-hoc code for BCM2835 (Raspberry Pi)
>>> USB poweron support in the DWC2 controller to an OF-based power domain
>>> declaration.
>
> I guess you are initializing the PM domains from module_init()?
>
> I use core_initcall() in arch/arm/mach-shmobile/pm-rmobile.c to make sure it's
> initialized earlier, as e.g. the interrupt controller uses postcore_initcall().

Yeah, I think most existing users are initizling PM domains early, but IMO
we should be working towards supporting PM domains that are created
later as well (as this patch does.)

>>> drivers/base/power/domain.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index ba4abbe..2b93c98 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -2064,7 +2064,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>>>  struct generic_pm_domain *of_genpd_get_from_provider(
>>>                                       struct of_phandle_args *genpdspec)
>>>  {
>>> -     struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
>>> +     struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
>
> Currently platform_drv_probe() just continues if dev_pm_domain_attach() returns
> a different error than -EPROBE_DEFER, which is what you are seeing.
>
> Your change does have the side effect that a new DT with PM domains won't
> work on an older kernel that doesn't have the PM domain driver yet.

Is that a real problem though?  Using newer DTs on older kernels can
cause many types of problems.

Kevin

Re: [PATCH] PM / Domains: If an OF node is found but no device probed yet, defer.

[Geert Uytterhoeven]

Hi Kevin,

On Fri, Mar 13, 2015 at 7:01 PM, Kevin Hilman <khilman@kernel.org> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> On Wed, Mar 11, 2015 at 11:08 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> More CCes.
>>>
>>> On Wednesday, March 11, 2015 08:27:28 AM Eric Anholt wrote:
>>>> If we've declared a power domain in the OF, and the OF node is found
>>>> but the requested domain hasn't been registered on it yet, then we
>>>> probably have just tried to probe before the power domain driver has.
>>>> Defer our device's probe until it shows up.
>>>>
>>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>>
>>> Kevin, Ulf, any chance to have a look at this, please?
>>>
>>>> ---
>>>>
>>>> I ran into this when turning my ad-hoc code for BCM2835 (Raspberry Pi)
>>>> USB poweron support in the DWC2 controller to an OF-based power domain
>>>> declaration.
>>
>> I guess you are initializing the PM domains from module_init()?
>>
>> I use core_initcall() in arch/arm/mach-shmobile/pm-rmobile.c to make sure it's
>> initialized earlier, as e.g. the interrupt controller uses postcore_initcall().
>
> Yeah, I think most existing users are initizling PM domains early, but IMO
> we should be working towards supporting PM domains that are created
> later as well (as this patch does.)

Sure. When interrupt controllers are involved (yes, they can be in a PM (Clock)
Domain too), there are definitely some complex issues to resolve in the core OF
probing code first. Cfr. the bug mentioned in
http://marc.info/?l=devicetree&m=142325419327559&w=2

>>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>>> index ba4abbe..2b93c98 100644
>>>> --- a/drivers/base/power/domain.c
>>>> +++ b/drivers/base/power/domain.c
>>>> @@ -2064,7 +2064,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>>>>  struct generic_pm_domain *of_genpd_get_from_provider(
>>>>                                       struct of_phandle_args *genpdspec)
>>>>  {
>>>> -     struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
>>>> +     struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
>>
>> Currently platform_drv_probe() just continues if dev_pm_domain_attach() returns
>> a different error than -EPROBE_DEFER, which is what you are seeing.
>>
>> Your change does have the side effect that a new DT with PM domains won't
>> work on an older kernel that doesn't have the PM domain driver yet.
>
> Is that a real problem though?  Using newer DTs on older kernels can
> cause many types of problems.

It means we cannot describe everything in (stable) DT before all support code
has been implemented. Ignoring that, it enforces strict merge order.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] PM / Domains: If an OF node is found but no device probed yet, defer.

[Ulf Hansson]

On 13 March 2015 at 19:01, Kevin Hilman <khilman@kernel.org> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>
>> On Wed, Mar 11, 2015 at 11:08 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> More CCes.
>>>
>>> On Wednesday, March 11, 2015 08:27:28 AM Eric Anholt wrote:
>>>> If we've declared a power domain in the OF, and the OF node is found
>>>> but the requested domain hasn't been registered on it yet, then we
>>>> probably have just tried to probe before the power domain driver has.
>>>> Defer our device's probe until it shows up.
>>>>
>>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>>
>>> Kevin, Ulf, any chance to have a look at this, please?
>>>
>>>> ---
>>>>
>>>> I ran into this when turning my ad-hoc code for BCM2835 (Raspberry Pi)
>>>> USB poweron support in the DWC2 controller to an OF-based power domain
>>>> declaration.
>>
>> I guess you are initializing the PM domains from module_init()?
>>
>> I use core_initcall() in arch/arm/mach-shmobile/pm-rmobile.c to make sure it's
>> initialized earlier, as e.g. the interrupt controller uses postcore_initcall().
>
> Yeah, I think most existing users are initizling PM domains early, but IMO
> we should be working towards supporting PM domains that are created
> later as well (as this patch does.)

I do agree, that we _should_ allow PM domains to be created later/any
time. Unfortunate, that's not going to be a simple one-liner patch.
:-)

To have genpd_dev_pm_attach() return -EPROBE_DEFER, due to that the PM
domain hasn’t been _initialized_ yet, we need to know whether a PM
domain exists at all for the device. In principle we need to split the
work done by genpd_dev_pm_attach() into the two parts described below.

1.
At struct device creation time, done from the "OF core", we also need
to parse for a PM domain node. If such is found, we somehow needs to
assigned it to the device.

Normally we would have assigned the struct dev_pm_domain in the struct
device to deal with this, but that has some implications. Currently
the struct dev_pm_domain is created from SoC specific code and it's
also done at different init levels.

2. At ->probe(), genpd shall return -EPROBE_DEFER, if the device's
assigned PM domain hasn’t been initialized yet.

Implementing 2) should be trivial, but 1) could be a bit harder.
Anyway, if anyone want to have a stab on it, I will gladly review such
patches.

Kind regards
Uffe

>
>>>> drivers/base/power/domain.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>>> index ba4abbe..2b93c98 100644
>>>> --- a/drivers/base/power/domain.c
>>>> +++ b/drivers/base/power/domain.c
>>>> @@ -2064,7 +2064,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>>>>  struct generic_pm_domain *of_genpd_get_from_provider(
>>>>                                       struct of_phandle_args *genpdspec)
>>>>  {
>>>> -     struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
>>>> +     struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
>>
>> Currently platform_drv_probe() just continues if dev_pm_domain_attach() returns
>> a different error than -EPROBE_DEFER, which is what you are seeing.
>>
>> Your change does have the side effect that a new DT with PM domains won't
>> work on an older kernel that doesn't have the PM domain driver yet.
>
> Is that a real problem though?  Using newer DTs on older kernels can
> cause many types of problems.
>
> Kevin

Re: [PATCH] PM / Domains: If an OF node is found but no device probed yet, defer.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2571 bytes --]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On 13 March 2015 at 19:01, Kevin Hilman <khilman@kernel.org> wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>>
>>> On Wed, Mar 11, 2015 at 11:08 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> More CCes.
>>>>
>>>> On Wednesday, March 11, 2015 08:27:28 AM Eric Anholt wrote:
>>>>> If we've declared a power domain in the OF, and the OF node is found
>>>>> but the requested domain hasn't been registered on it yet, then we
>>>>> probably have just tried to probe before the power domain driver has.
>>>>> Defer our device's probe until it shows up.
>>>>>
>>>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>>>
>>>> Kevin, Ulf, any chance to have a look at this, please?
>>>>
>>>>> ---
>>>>>
>>>>> I ran into this when turning my ad-hoc code for BCM2835 (Raspberry Pi)
>>>>> USB poweron support in the DWC2 controller to an OF-based power domain
>>>>> declaration.
>>>
>>> I guess you are initializing the PM domains from module_init()?
>>>
>>> I use core_initcall() in arch/arm/mach-shmobile/pm-rmobile.c to make sure it's
>>> initialized earlier, as e.g. the interrupt controller uses postcore_initcall().
>>
>> Yeah, I think most existing users are initizling PM domains early, but IMO
>> we should be working towards supporting PM domains that are created
>> later as well (as this patch does.)
>
> I do agree, that we _should_ allow PM domains to be created later/any
> time. Unfortunate, that's not going to be a simple one-liner patch.
> :-)
>
> To have genpd_dev_pm_attach() return -EPROBE_DEFER, due to that the PM
> domain hasn’t been _initialized_ yet, we need to know whether a PM
> domain exists at all for the device. In principle we need to split the
> work done by genpd_dev_pm_attach() into the two parts described below.
>
> 1.
> At struct device creation time, done from the "OF core", we also need
> to parse for a PM domain node. If such is found, we somehow needs to
> assigned it to the device.
>
> Normally we would have assigned the struct dev_pm_domain in the struct
> device to deal with this, but that has some implications. Currently
> the struct dev_pm_domain is created from SoC specific code and it's
> also done at different init levels.

I haven't made sense of what's actually proposed here, so I'm not
stepping forward to write this.  I think I'd be less confused if the
"struct device" references in the explanation made it clear which ones
were the pm domain controller and which were the pm domain consumer.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] PM / Domains: If an OF node is found but no device probed yet, defer.

[Ulf Hansson]

On 11 March 2015 at 16:27, Eric Anholt <eric@anholt.net> wrote:
> If we've declared a power domain in the OF, and the OF node is found
> but the requested domain hasn't been registered on it yet, then we
> probably have just tried to probe before the power domain driver has.
> Defer our device's probe until it shows up.

As Geert also pointed out in his reply; I think the proper solution is
to move the registration of the PM domain to an earlier init level.

The main reason from my side is that I don't want us to invent yet
another "probe scenario", since we currently already have some
difficulties to handle the existing ones. Like the issue below.
http://marc4.marc.info/?t=142392781500004&r=1&w=2

Kind regards
Uffe

>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>
> I ran into this when turning my ad-hoc code for BCM2835 (Raspberry Pi)
> USB poweron support in the DWC2 controller to an OF-based power domain
> declaration.
>
> drivers/base/power/domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index ba4abbe..2b93c98 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2064,7 +2064,7 @@ EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>  struct generic_pm_domain *of_genpd_get_from_provider(
>                                         struct of_phandle_args *genpdspec)
>  {
> -       struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
> +       struct generic_pm_domain *genpd = ERR_PTR(-EPROBE_DEFER);
>         struct of_genpd_provider *provider;
>
>         mutex_lock(&of_genpd_mutex);
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/