[PATCH] ARM: davinci: enable PM for DT boot

[PATCH] ARM: davinci: enable PM for DT boot

[Kevin Hilman]

Currently system PM is only enabled for legacy (non-DT) boot.  Enable
for DT boot also.

Tested on da850-lcdk using "rtcwake -m mem -s5 -d rtc0".

Signed-off-by: Kevin Hilman <khilman@baylibre.com>
---
 arch/arm/mach-davinci/da8xx-dt.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
index c9f7e9274aa8..a8089fa40d86 100644
--- a/arch/arm/mach-davinci/da8xx-dt.c
+++ b/arch/arm/mach-davinci/da8xx-dt.c
@@ -43,8 +43,26 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
 
 #ifdef CONFIG_ARCH_DAVINCI_DA850
 
+static struct davinci_pm_config da850_pm_pdata = {
+	.sleepcount = 128,
+};
+
+static struct platform_device da850_pm_device = {
+	.name           = "pm-davinci",
+	.dev = {
+		.platform_data	= &da850_pm_pdata,
+	},
+	.id             = -1,
+};
+
 static void __init da850_init_machine(void)
 {
+	int ret;
+
+	ret = da850_register_pm(&da850_pm_device);
+	if (ret)
+		pr_warn("%s: suspend registration failed: %d\n", __func__, ret);
+
 	of_platform_default_populate(NULL, da850_auxdata_lookup, NULL);
 }
 
-- 
2.9.3

[PATCH] ARM: davinci: enable PM for DT boot

[Sekhar Nori]

Hi Kevin,

On Wednesday 26 October 2016 03:17 AM, Kevin Hilman wrote:
> Currently system PM is only enabled for legacy (non-DT) boot.  Enable
> for DT boot also.
> 
> Tested on da850-lcdk using "rtcwake -m mem -s5 -d rtc0".
> 
> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> ---
>  arch/arm/mach-davinci/da8xx-dt.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
> index c9f7e9274aa8..a8089fa40d86 100644
> --- a/arch/arm/mach-davinci/da8xx-dt.c
> +++ b/arch/arm/mach-davinci/da8xx-dt.c
> @@ -43,8 +43,26 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
>  
>  #ifdef CONFIG_ARCH_DAVINCI_DA850
>  
> +static struct davinci_pm_config da850_pm_pdata = {
> +	.sleepcount = 128,
> +};
> +
> +static struct platform_device da850_pm_device = {
> +	.name           = "pm-davinci",
> +	.dev = {
> +		.platform_data	= &da850_pm_pdata,
> +	},
> +	.id             = -1,
> +};
> +
>  static void __init da850_init_machine(void)
>  {
> +	int ret;
> +
> +	ret = da850_register_pm(&da850_pm_device);

I am not sure if it makes sense to keep the "pm device" around anymore.
I think for both DT and non-DT boot, we can get rid of the fake PM
device and combine da850_register_pm() and davinci_pm_probe() into a
single davinci_init_suspend() function which can then be called both for
DT and non-DT boot.

This was we can also avoid replication of the platform data and platform
device structures.

Thanks,
Sekhar

[PATCH] ARM: davinci: enable PM for DT boot

[Kevin Hilman]

Sekhar Nori <nsekhar@ti.com> writes:

> Hi Kevin,
>
> On Wednesday 26 October 2016 03:17 AM, Kevin Hilman wrote:
>> Currently system PM is only enabled for legacy (non-DT) boot.  Enable
>> for DT boot also.
>> 
>> Tested on da850-lcdk using "rtcwake -m mem -s5 -d rtc0".
>> 
>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> ---
>>  arch/arm/mach-davinci/da8xx-dt.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>> 
>> diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
>> index c9f7e9274aa8..a8089fa40d86 100644
>> --- a/arch/arm/mach-davinci/da8xx-dt.c
>> +++ b/arch/arm/mach-davinci/da8xx-dt.c
>> @@ -43,8 +43,26 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
>>  
>>  #ifdef CONFIG_ARCH_DAVINCI_DA850
>>  
>> +static struct davinci_pm_config da850_pm_pdata = {
>> +	.sleepcount = 128,
>> +};
>> +
>> +static struct platform_device da850_pm_device = {
>> +	.name           = "pm-davinci",
>> +	.dev = {
>> +		.platform_data	= &da850_pm_pdata,
>> +	},
>> +	.id             = -1,
>> +};
>> +
>>  static void __init da850_init_machine(void)
>>  {
>> +	int ret;
>> +
>> +	ret = da850_register_pm(&da850_pm_device);
>
> I am not sure if it makes sense to keep the "pm device" around anymore.
> I think for both DT and non-DT boot, we can get rid of the fake PM
> device and combine da850_register_pm() and davinci_pm_probe() into a
> single davinci_init_suspend() function which can then be called both for
> DT and non-DT boot.
>
> This was we can also avoid replication of the platform data and platform
> device structures.

Great, that sounds much cleaner.  Will re-spin.

Kevin

[PATCH] ARM: davinci: enable PM for DT boot

[Kevin Hilman]

Hi Sekhar,

Sekhar Nori <nsekhar@ti.com> writes:

> On Wednesday 26 October 2016 03:17 AM, Kevin Hilman wrote:
>> Currently system PM is only enabled for legacy (non-DT) boot.  Enable
>> for DT boot also.
>> 
>> Tested on da850-lcdk using "rtcwake -m mem -s5 -d rtc0".
>> 
>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> ---
>>  arch/arm/mach-davinci/da8xx-dt.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>> 
>> diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
>> index c9f7e9274aa8..a8089fa40d86 100644
>> --- a/arch/arm/mach-davinci/da8xx-dt.c
>> +++ b/arch/arm/mach-davinci/da8xx-dt.c
>> @@ -43,8 +43,26 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
>>  
>>  #ifdef CONFIG_ARCH_DAVINCI_DA850
>>  
>> +static struct davinci_pm_config da850_pm_pdata = {
>> +	.sleepcount = 128,
>> +};
>> +
>> +static struct platform_device da850_pm_device = {
>> +	.name           = "pm-davinci",
>> +	.dev = {
>> +		.platform_data	= &da850_pm_pdata,
>> +	},
>> +	.id             = -1,
>> +};
>> +
>>  static void __init da850_init_machine(void)
>>  {
>> +	int ret;
>> +
>> +	ret = da850_register_pm(&da850_pm_device);
>
> I am not sure if it makes sense to keep the "pm device" around anymore.
> I think for both DT and non-DT boot, we can get rid of the fake PM
> device and combine da850_register_pm() and davinci_pm_probe() into a
> single davinci_init_suspend() function which can then be called both for
> DT and non-DT boot.

Looking closer at this, where do you propose the pdata comes from for
the non-DT boot?

It seems to me that we can't currently remove the pdata dependency
without breaking the non-DT platforms, so the approach proposed here is
the least invasive.

Once other platforms are DT converted, we could clean this up a bit
more.

Kevin

[PATCH] ARM: davinci: enable PM for DT boot

[Sekhar Nori]

On Tuesday 08 November 2016 11:43 PM, Kevin Hilman wrote:
> Hi Sekhar,
> 
> Sekhar Nori <nsekhar@ti.com> writes:
> 
>> On Wednesday 26 October 2016 03:17 AM, Kevin Hilman wrote:
>>> Currently system PM is only enabled for legacy (non-DT) boot.  Enable
>>> for DT boot also.
>>>
>>> Tested on da850-lcdk using "rtcwake -m mem -s5 -d rtc0".
>>>
>>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>>> ---
>>>  arch/arm/mach-davinci/da8xx-dt.c | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
>>> index c9f7e9274aa8..a8089fa40d86 100644
>>> --- a/arch/arm/mach-davinci/da8xx-dt.c
>>> +++ b/arch/arm/mach-davinci/da8xx-dt.c
>>> @@ -43,8 +43,26 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
>>>  
>>>  #ifdef CONFIG_ARCH_DAVINCI_DA850
>>>  
>>> +static struct davinci_pm_config da850_pm_pdata = {
>>> +	.sleepcount = 128,
>>> +};
>>> +
>>> +static struct platform_device da850_pm_device = {
>>> +	.name           = "pm-davinci",
>>> +	.dev = {
>>> +		.platform_data	= &da850_pm_pdata,
>>> +	},
>>> +	.id             = -1,
>>> +};
>>> +
>>>  static void __init da850_init_machine(void)
>>>  {
>>> +	int ret;
>>> +
>>> +	ret = da850_register_pm(&da850_pm_device);
>>
>> I am not sure if it makes sense to keep the "pm device" around anymore.
>> I think for both DT and non-DT boot, we can get rid of the fake PM
>> device and combine da850_register_pm() and davinci_pm_probe() into a
>> single davinci_init_suspend() function which can then be called both for
>> DT and non-DT boot.
> 
> Looking closer at this, where do you propose the pdata comes from for
> the non-DT boot?
> 
> It seems to me that we can't currently remove the pdata dependency
> without breaking the non-DT platforms, so the approach proposed here is
> the least invasive.

There is a single value of sleep count that is used today (128). So I
was thinking we can hardcode that in pm.c. We are not going to add more
board files anyway so there is no risk here.

For future, if a different sleepcount value is needed, it will need to
be a new DT property.

Thanks,
Sekhar

[PATCH] ARM: davinci: enable PM for DT boot

[Kevin Hilman]

Sekhar Nori <nsekhar@ti.com> writes:

> On Tuesday 08 November 2016 11:43 PM, Kevin Hilman wrote:
>> Hi Sekhar,
>> 
>> Sekhar Nori <nsekhar@ti.com> writes:
>> 
>>> On Wednesday 26 October 2016 03:17 AM, Kevin Hilman wrote:
>>>> Currently system PM is only enabled for legacy (non-DT) boot.  Enable
>>>> for DT boot also.
>>>>
>>>> Tested on da850-lcdk using "rtcwake -m mem -s5 -d rtc0".
>>>>
>>>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>>>> ---
>>>>  arch/arm/mach-davinci/da8xx-dt.c | 18 ++++++++++++++++++
>>>>  1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
>>>> index c9f7e9274aa8..a8089fa40d86 100644
>>>> --- a/arch/arm/mach-davinci/da8xx-dt.c
>>>> +++ b/arch/arm/mach-davinci/da8xx-dt.c
>>>> @@ -43,8 +43,26 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
>>>>  
>>>>  #ifdef CONFIG_ARCH_DAVINCI_DA850
>>>>  
>>>> +static struct davinci_pm_config da850_pm_pdata = {
>>>> +	.sleepcount = 128,
>>>> +};
>>>> +
>>>> +static struct platform_device da850_pm_device = {
>>>> +	.name           = "pm-davinci",
>>>> +	.dev = {
>>>> +		.platform_data	= &da850_pm_pdata,
>>>> +	},
>>>> +	.id             = -1,
>>>> +};
>>>> +
>>>>  static void __init da850_init_machine(void)
>>>>  {
>>>> +	int ret;
>>>> +
>>>> +	ret = da850_register_pm(&da850_pm_device);
>>>
>>> I am not sure if it makes sense to keep the "pm device" around anymore.
>>> I think for both DT and non-DT boot, we can get rid of the fake PM
>>> device and combine da850_register_pm() and davinci_pm_probe() into a
>>> single davinci_init_suspend() function which can then be called both for
>>> DT and non-DT boot.
>> 
>> Looking closer at this, where do you propose the pdata comes from for
>> the non-DT boot?
>> 
>> It seems to me that we can't currently remove the pdata dependency
>> without breaking the non-DT platforms, so the approach proposed here is
>> the least invasive.
>
> There is a single value of sleep count that is used today (128). So I
> was thinking we can hardcode that in pm.c. We are not going to add more
> board files anyway so there is no risk here.
>
> For future, if a different sleepcount value is needed, it will need to
> be a new DT property.

Right, but getting rid of the pdata is more than just hard-coding the
sleep count. There are a bunch of other fields in the pdata, which are
filled out to some standard defaults in da850.c.  Are you proposing to
hard-code those in pm.c also?

An intermediate step might be to start by removing the
platform_device/pdata from the board files, but keep it in da850.c for
now.  Then, a follow-up cleanup could be done to either move all of that
into pm.c, or use DT.

Kevin

[PATCH] ARM: davinci: enable PM for DT boot

[Sekhar Nori]

On Friday 11 November 2016 10:06 PM, Kevin Hilman wrote:
> Sekhar Nori <nsekhar@ti.com> writes:
> 
>> On Tuesday 08 November 2016 11:43 PM, Kevin Hilman wrote:
>>> Hi Sekhar,
>>>
>>> Sekhar Nori <nsekhar@ti.com> writes:
>>>
>>>> On Wednesday 26 October 2016 03:17 AM, Kevin Hilman wrote:
>>>>> Currently system PM is only enabled for legacy (non-DT) boot.  Enable
>>>>> for DT boot also.
>>>>>
>>>>> Tested on da850-lcdk using "rtcwake -m mem -s5 -d rtc0".
>>>>>
>>>>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>>>>> ---
>>>>>  arch/arm/mach-davinci/da8xx-dt.c | 18 ++++++++++++++++++
>>>>>  1 file changed, 18 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
>>>>> index c9f7e9274aa8..a8089fa40d86 100644
>>>>> --- a/arch/arm/mach-davinci/da8xx-dt.c
>>>>> +++ b/arch/arm/mach-davinci/da8xx-dt.c
>>>>> @@ -43,8 +43,26 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
>>>>>  
>>>>>  #ifdef CONFIG_ARCH_DAVINCI_DA850
>>>>>  
>>>>> +static struct davinci_pm_config da850_pm_pdata = {
>>>>> +	.sleepcount = 128,
>>>>> +};
>>>>> +
>>>>> +static struct platform_device da850_pm_device = {
>>>>> +	.name           = "pm-davinci",
>>>>> +	.dev = {
>>>>> +		.platform_data	= &da850_pm_pdata,
>>>>> +	},
>>>>> +	.id             = -1,
>>>>> +};
>>>>> +
>>>>>  static void __init da850_init_machine(void)
>>>>>  {
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = da850_register_pm(&da850_pm_device);
>>>>
>>>> I am not sure if it makes sense to keep the "pm device" around anymore.
>>>> I think for both DT and non-DT boot, we can get rid of the fake PM
>>>> device and combine da850_register_pm() and davinci_pm_probe() into a
>>>> single davinci_init_suspend() function which can then be called both for
>>>> DT and non-DT boot.
>>>
>>> Looking closer at this, where do you propose the pdata comes from for
>>> the non-DT boot?
>>>
>>> It seems to me that we can't currently remove the pdata dependency
>>> without breaking the non-DT platforms, so the approach proposed here is
>>> the least invasive.
>>
>> There is a single value of sleep count that is used today (128). So I
>> was thinking we can hardcode that in pm.c. We are not going to add more
>> board files anyway so there is no risk here.
>>
>> For future, if a different sleepcount value is needed, it will need to
>> be a new DT property.
> 
> Right, but getting rid of the pdata is more than just hard-coding the
> sleep count. There are a bunch of other fields in the pdata, which are
> filled out to some standard defaults in da850.c.  Are you proposing to
> hard-code those in pm.c also?

Yeah. When I wrote the code for pm.c, I wrote it hoping that it can be
used on other davinci devices too. But since then, no other DaVinci
device has gained PM support. DM365 could support PM, but its not
supported even on TI's internal releases.

Even if in future PM does get supported on DM365, platform data will not
be used for sure. And besides, the sequence in sleep.S has only ever
been tested on DA850 and pretty sure will need some tweaks for running
on DM365.

> 
> An intermediate step might be to start by removing the
> platform_device/pdata from the board files, but keep it in da850.c for
> now.  Then, a follow-up cleanup could be done to either move all of that
> into pm.c, or use DT.

I think keeping it in pm.c is fine. We could have called pm.c da850-pm.c
but thats not really required I guess since thats the only device in
mach-davinci which supports suspend-to-RAM anyway.

Thanks,
Sekhar