[RFC][PATCH v2 1/1] ARM: OMAP2+: PM: Register suspend ops even in the presence of DT blob

[RFC][PATCH v2 1/1] ARM: OMAP2+: PM: Register suspend ops even in the presence of DT blob

[Vaibhav Bedia]

As per the comment in omap2_common_late_init() looks like the
original intent of the DT check was to treat only the PMIC
and SR initialization differently. Recent changes to consolidate
the suspend-resume code across OMAP3/4 resulted into the
registration of suspend ops also being dependent on the check
for DT blob. Since the suspend-resume operation should not
really be dependent on the usage of DT remove this dependency
by wrapping the PMIC and SR init under the DT check.

Signed-off-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
---
v2->v1
 - As suggested by Paul, Instead of moving around the suspend ops
   registration just wrap the PMIC and SR init under the DT check.
 - Fixed up Kevin's email address :\

 arch/arm/mach-omap2/pm.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index 9cb5ced..46848f7 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -302,19 +302,18 @@ int __init omap2_common_pm_late_init(void)
 	 * a completely different mechanism.
 	 * Disable this part if a DT blob is available.
 	 */
-	if (of_have_populated_dt())
-		return 0;
+	if (!of_have_populated_dt()) {
+		/* Init the voltage layer */
+		omap_pmic_late_init();
+		omap_voltage_late_init();
 
-	/* Init the voltage layer */
-	omap_pmic_late_init();
-	omap_voltage_late_init();
+		/* Initialize the voltages */
+		omap3_init_voltages();
+		omap4_init_voltages();
 
-	/* Initialize the voltages */
-	omap3_init_voltages();
-	omap4_init_voltages();
-
-	/* Smartreflex device init */
-	omap_devinit_smartreflex();
+		/* Smartreflex device init */
+		omap_devinit_smartreflex();
+	}
 
 #ifdef CONFIG_SUSPEND
 	suspend_set_ops(&omap_pm_ops);
-- 
1.7.0.4

[RFC][PATCH v2 1/1] ARM: OMAP2+: PM: Register suspend ops even in the presence of DT blob

[Rajendra Nayak]

On Thursday 19 July 2012 05:38 PM, Vaibhav Bedia wrote:
> As per the comment in omap2_common_late_init() looks like the
> original intent of the DT check was to treat only the PMIC
> and SR initialization differently. Recent changes to consolidate
> the suspend-resume code across OMAP3/4 resulted into the
> registration of suspend ops also being dependent on the check
> for DT blob. Since the suspend-resume operation should not
> really be dependent on the usage of DT remove this dependency
> by wrapping the PMIC and SR init under the DT check.

So I am guessing you also tested suspend/resume on your hardware
with this patch, when booting with a DT blob, and it passed.

>
> Signed-off-by: Vaibhav Bedia<vaibhav.bedia@ti.com>
> ---
> v2->v1
>   - As suggested by Paul, Instead of moving around the suspend ops
>     registration just wrap the PMIC and SR init under the DT check.
>   - Fixed up Kevin's email address :\
>
>   arch/arm/mach-omap2/pm.c |   21 ++++++++++-----------
>   1 files changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
> index 9cb5ced..46848f7 100644
> --- a/arch/arm/mach-omap2/pm.c
> +++ b/arch/arm/mach-omap2/pm.c
> @@ -302,19 +302,18 @@ int __init omap2_common_pm_late_init(void)
>   	 * a completely different mechanism.
>   	 * Disable this part if a DT blob is available.
>   	 */
> -	if (of_have_populated_dt())
> -		return 0;
> +	if (!of_have_populated_dt()) {
> +		/* Init the voltage layer */
> +		omap_pmic_late_init();
> +		omap_voltage_late_init();
>
> -	/* Init the voltage layer */
> -	omap_pmic_late_init();
> -	omap_voltage_late_init();
> +		/* Initialize the voltages */
> +		omap3_init_voltages();
> +		omap4_init_voltages();
>
> -	/* Initialize the voltages */
> -	omap3_init_voltages();
> -	omap4_init_voltages();
> -
> -	/* Smartreflex device init */
> -	omap_devinit_smartreflex();
> +		/* Smartreflex device init */
> +		omap_devinit_smartreflex();
> +	}
>
>   #ifdef CONFIG_SUSPEND
>   	suspend_set_ops(&omap_pm_ops);

[RFC][PATCH v2 1/1] ARM: OMAP2+: PM: Register suspend ops even in the presence of DT blob

[Bedia, Vaibhav]

On Fri, Jul 20, 2012 at 10:09:43, Nayak, Rajendra wrote:
> On Thursday 19 July 2012 05:38 PM, Vaibhav Bedia wrote:
> > As per the comment in omap2_common_late_init() looks like the
> > original intent of the DT check was to treat only the PMIC
> > and SR initialization differently. Recent changes to consolidate
> > the suspend-resume code across OMAP3/4 resulted into the
> > registration of suspend ops also being dependent on the check
> > for DT blob. Since the suspend-resume operation should not
> > really be dependent on the usage of DT remove this dependency
> > by wrapping the PMIC and SR init under the DT check.
> 
> So I am guessing you also tested suspend/resume on your hardware
> with this patch, when booting with a DT blob, and it passed.
> 

I am getting there ;)

I am in the process of getting a real suspend/resume functional. As part
of this I was just flow flushing the whole suspend process in the mainline
kernel by putting in a dummy implementation for the .enter op and found that
this change was needed.

Regards,
Vaibhav

[RFC][PATCH v2 1/1] ARM: OMAP2+: PM: Register suspend ops even in the presence of DT blob

[Rajendra Nayak]

On Friday 20 July 2012 10:31 AM, Bedia, Vaibhav wrote:
> On Fri, Jul 20, 2012 at 10:09:43, Nayak, Rajendra wrote:
>> On Thursday 19 July 2012 05:38 PM, Vaibhav Bedia wrote:
>>> As per the comment in omap2_common_late_init() looks like the
>>> original intent of the DT check was to treat only the PMIC
>>> and SR initialization differently. Recent changes to consolidate
>>> the suspend-resume code across OMAP3/4 resulted into the
>>> registration of suspend ops also being dependent on the check
>>> for DT blob. Since the suspend-resume operation should not
>>> really be dependent on the usage of DT remove this dependency
>>> by wrapping the PMIC and SR init under the DT check.
>>
>> So I am guessing you also tested suspend/resume on your hardware
>> with this patch, when booting with a DT blob, and it passed.
>>
>
> I am getting there ;)
>
> I am in the process of getting a real suspend/resume functional. As part
> of this I was just flow flushing the whole suspend process in the mainline
> kernel by putting in a dummy implementation for the .enter op and found that
> this change was needed.

Ok, good to know you are working on this. It might then make sense for
for you to also include this patch as part of that series?
It doesn't make much sense to register suspend ops which then break
suspend isn't it? Btw, whats the behavior on current mainline with
just this patch. Does it hang/crash or just prevent the system from
doing down?

>
> Regards,
> Vaibhav

[RFC][PATCH v2 1/1] ARM: OMAP2+: PM: Register suspend ops even in the presence of DT blob

[Bedia, Vaibhav]

On Fri, Jul 20, 2012 at 10:42:53, Nayak, Rajendra wrote:
> On Friday 20 July 2012 10:31 AM, Bedia, Vaibhav wrote:
> > On Fri, Jul 20, 2012 at 10:09:43, Nayak, Rajendra wrote:
> >> On Thursday 19 July 2012 05:38 PM, Vaibhav Bedia wrote:
> >>> As per the comment in omap2_common_late_init() looks like the
> >>> original intent of the DT check was to treat only the PMIC
> >>> and SR initialization differently. Recent changes to consolidate
> >>> the suspend-resume code across OMAP3/4 resulted into the
> >>> registration of suspend ops also being dependent on the check
> >>> for DT blob. Since the suspend-resume operation should not
> >>> really be dependent on the usage of DT remove this dependency
> >>> by wrapping the PMIC and SR init under the DT check.
> >>
> >> So I am guessing you also tested suspend/resume on your hardware
> >> with this patch, when booting with a DT blob, and it passed.
> >>
> >
> > I am getting there ;)
> >
> > I am in the process of getting a real suspend/resume functional. As part
> > of this I was just flow flushing the whole suspend process in the mainline
> > kernel by putting in a dummy implementation for the .enter op and found that
> > this change was needed.
> 
> Ok, good to know you are working on this. It might then make sense for
> for you to also include this patch as part of that series?
> It doesn't make much sense to register suspend ops which then break
> suspend isn't it? Btw, whats the behavior on current mainline with
> just this patch. Does it hang/crash or just prevent the system from
> doing down?
> 

Ok. I can keep this patch as part of the suspend-resume support for AM33XX.

With just this patch on top of the tree being maintained by Vaibhav Hiremath [1]
I found a couple of issues that I am yet to fully root-cause.

1. The PM_SUSPEND_PREPARE handler in drivers/mmc/core/core.c never returns unless
the MMC_UNSAFE_RESUME config option is selected. This might be due to the suspend
and resume in mmc_bus_ops being NULL in the case where the config option is not set.

2. The suspend process hangs somewhere in the .suspend_noirq callback for the UART
being used as the console when no_console_suspend is passed.

I am yet to check the behavior on an OMAP3 EVM so right now I am not sure whether
these are AM33XX specific issues. If you have any pointers that will greatly help.

Regards,
Vaibhav B.

[1] https://github.com/hvaibhav/am335x-linux/tree/am335x-upstream-staging