ARM: tegra: support LP1 suspend mode

The common LP1 resuming procedures of LP1 on Tegra was checking the LP1 mask first. The LP1 mask indicated that the Tegra device was in LP1 then we need to resume the Tegra from the LP1 reset handler.

And the LP1 was putting the SDRAM to self-refresh mode, the SDRAM wasn't accessible when resuming from LP1. We need to copy the LP1 reset handler to IRAM before suspending. That's why you can see the address of LP1 reset handler was located in IRAM.

arch/arm/mach-tegra/reset-handler.S +0 -0

@@ -182,6 +182,19 @@ after_errata:
 1:
 #endif
 

When suspending to LP1 mode, the SYSCLK will be clock gated. And different board may have different polarity of the request of SYSCLK, this patch configure the polarity from the DT for the board.

arch/arm/mach-tegra/pmc.c +0 -0

@@ -238,6 +240,20 @@ void tegra_pmc_suspend_init(void)
 	reg = tegra_pmc_readl(PMC_CTRL);
 	reg |= TEGRA_POWER_CPU_PWRREQ_OE;
 	tegra_pmc_writel(reg, PMC_CTRL);

arch/arm/mach-tegra/pmc.c +3 -0

+	/* now enable the request */
+	reg |= TEGRA_POWER_SYSCLK_OE;
+	tegra_pmc_writel(reg, PMC_CTRL);

Quoted 5 lines · Joseph Lo

When suspending to LP1 mode, the SYSCLK will be clock gated. And different board may have different polarity of the request of SYSCLK, this patch configure the polarity from the DT for the board.

arch/arm/mach-tegra/pmc.c +0 -0

Quoted 4 lines

arch/arm/mach-tegra/pmc.c +0 -0

@@ -238,6 +240,20 @@ void tegra_pmc_suspend_init(void)
 	reg = tegra_pmc_readl(PMC_CTRL);
 	reg |= TEGRA_POWER_CPU_PWRREQ_OE;
 	tegra_pmc_writel(reg, PMC_CTRL);

arch/arm/mach-tegra/pmc.c void tegra_pmc_suspend_init(void) · line 252

Quoted 10 lines

arch/arm/mach-tegra/pmc.c +10 -0

+
+	reg = tegra_pmc_readl(PMC_CTRL);
+
+	if (!pmc_pm_data.sysclkreq_high)
+		reg |= TEGRA_POWER_SYSCLK_POLARITY;
+	else
+		reg &= ~TEGRA_POWER_SYSCLK_POLARITY;
+
+	/* configure the output inverts while the request is tristated */
+	tegra_pmc_writel(reg, PMC_CTRL);

I think s/inverts/polarity/ in that comment would make a lot more sense.

Must _OE be disabled for the code to work correctly? If so, should the code explicitly clear _OE first, since who knows what state it was originally in? Can't we just set the new polarity and enable _OE in a single register write?

Quoted 3 lines

arch/arm/mach-tegra/pmc.c +3 -0

+	/* now enable the request */
+	reg |= TEGRA_POWER_SYSCLK_OE;
+	tegra_pmc_writel(reg, PMC_CTRL);

Quoted 7 lines · Stephen Warren

Quoted 5 lines · Joseph Lo

When suspending to LP1 mode, the SYSCLK will be clock gated. And different board may have different polarity of the request of SYSCLK, this patch configure the polarity from the DT for the board.

arch/arm/mach-tegra/pmc.c +0 -0

Quoted 17 lines

Quoted 4 lines

arch/arm/mach-tegra/pmc.c +0 -0

@@ -238,6 +240,20 @@ void tegra_pmc_suspend_init(void)
 	reg = tegra_pmc_readl(PMC_CTRL);
 	reg |= TEGRA_POWER_CPU_PWRREQ_OE;
 	tegra_pmc_writel(reg, PMC_CTRL);

arch/arm/mach-tegra/pmc.c void tegra_pmc_suspend_init(void) · line 252

Quoted 10 lines

arch/arm/mach-tegra/pmc.c +10 -0

+
+	reg = tegra_pmc_readl(PMC_CTRL);
+
+	if (!pmc_pm_data.sysclkreq_high)
+		reg |= TEGRA_POWER_SYSCLK_POLARITY;
+	else
+		reg &= ~TEGRA_POWER_SYSCLK_POLARITY;
+
+	/* configure the output inverts while the request is tristated */
+	tegra_pmc_writel(reg, PMC_CTRL);

I think s/inverts/polarity/ in that comment would make a lot more sense.

Yes, thanks.

Quoted 5 lines

Must _OE be disabled for the code to work correctly? If so, should the code explicitly clear _OE first, since who knows what state it was originally in? Can't we just set the new polarity and enable _OE in a single register write?

The SYSCLK is super clock that was connected to COP subsystem. It can't be disabled when system is running. The boot loader had initialized it and brought it to kernel. We follow the HW description in DT of the polarity of SCLK to re-configure and re-init again. Then the PMC can clock gate it when system go into suspend state.

Quoted 32 lines · Joseph Lo

Quoted 7 lines · Stephen Warren

Quoted 5 lines · Joseph Lo

When suspending to LP1 mode, the SYSCLK will be clock gated. And different board may have different polarity of the request of SYSCLK, this patch configure the polarity from the DT for the board.

arch/arm/mach-tegra/pmc.c +0 -0

Quoted 17 lines

Quoted 4 lines

arch/arm/mach-tegra/pmc.c +0 -0

@@ -238,6 +240,20 @@ void tegra_pmc_suspend_init(void)
 	reg = tegra_pmc_readl(PMC_CTRL);
 	reg |= TEGRA_POWER_CPU_PWRREQ_OE;
 	tegra_pmc_writel(reg, PMC_CTRL);

arch/arm/mach-tegra/pmc.c void tegra_pmc_suspend_init(void) · line 252

Quoted 10 lines

arch/arm/mach-tegra/pmc.c +10 -0

+
+	reg = tegra_pmc_readl(PMC_CTRL);
+
+	if (!pmc_pm_data.sysclkreq_high)
+		reg |= TEGRA_POWER_SYSCLK_POLARITY;
+	else
+		reg &= ~TEGRA_POWER_SYSCLK_POLARITY;
+
+	/* configure the output inverts while the request is tristated */
+	tegra_pmc_writel(reg, PMC_CTRL);

I think s/inverts/polarity/ in that comment would make a lot more sense.

Yes, thanks.

Quoted 5 lines

Must _OE be disabled for the code to work correctly? If so, should the code explicitly clear _OE first, since who knows what state it was originally in? Can't we just set the new polarity and enable _OE in a single register write?

Quoted 5 lines

The SYSCLK is super clock that was connected to COP subsystem. It can't be disabled when system is running. The boot loader had initialized it and brought it to kernel. We follow the HW description in DT of the polarity of SCLK to re-configure and re-init again. Then the PMC can clock gate it when system go into suspend state.

So it sounds like the bootloader has already configured the clock to a certain polarity. If so, why do we need to reconfigure it again?

Or, is this code not duplicating something the bootloader must have done, but simply informing some HW in the PMC that's receiving SYSCLK how the clock is configured elsewhere?

When the system suspends to LP1, the clock of the CPU would be switched to CLK_M (12MHz Oscillator) during suspend/resume flow. The clock driver needs to restore the clock of CPU after LP1 resume.

Quoted 3 lines · Joseph Lo

When the system suspends to LP1, the clock of the CPU would be switched to CLK_M (12MHz Oscillator) during suspend/resume flow. The clock driver needs to restore the clock of CPU after LP1 resume.

It's unclear to me how the code change implements "restore the clock of the CPU". A register name of CCLKG_BURST_POLICY doesn't sound like it's anything to do with enabled/disabling the CPU clock, nor configuring its rate. What exactly does this register do, and hence what does this new code actually restore?

Why don't Tegra20/30 need a similar change?

Quoted 11 lines · Stephen Warren

Quoted 3 lines · Joseph Lo

When the system suspends to LP1, the clock of the CPU would be switched to CLK_M (12MHz Oscillator) during suspend/resume flow. The clock driver needs to restore the clock of CPU after LP1 resume.

It's unclear to me how the code change implements "restore the clock of the CPU". A register name of CCLKG_BURST_POLICY doesn't sound like it's anything to do with enabled/disabling the CPU clock, nor configuring its rate. What exactly does this register do, and hence what does this new code actually restore?

When system suspend to LP1, most of the PLLs was clock gated. Because we didn't cut off the core power, the settings of PLL still keep there. But we switch the clock source of CPU to CLK_M before shut off PLLs by CCLKG_BURSY_POLICY register. So we need to resume it back to original clock source by CCLKG_BURST_POLICY register. Or it would be keep in low rate (CLK_M) after resume.

Quoted 1 line

Why don't Tegra20/30 need a similar change?

For Tegra20/30, the same code had been implemented in the suspend/resume function of tegra_cpu_car_ops. It restores the CPU clock ASAP when CPU resume from a suspend state to get quick performance I believe.

For Tegra114, the resume performance is cool (although we can't see it in upstream kernel now, it still need some other functions.). We can implement all the clock related suspend/resume function in the clock driver.

Quoted 12 lines · Joseph Lo

Quoted 11 lines · Stephen Warren

Quoted 3 lines · Joseph Lo

When the system suspends to LP1, the clock of the CPU would be switched to CLK_M (12MHz Oscillator) during suspend/resume flow. The clock driver needs to restore the clock of CPU after LP1 resume.

It's unclear to me how the code change implements "restore the clock of the CPU". A register name of CCLKG_BURST_POLICY doesn't sound like it's anything to do with enabled/disabling the CPU clock, nor configuring its rate. What exactly does this register do, and hence what does this new code actually restore?

Quoted 6 lines

When system suspend to LP1, most of the PLLs was clock gated. Because we didn't cut off the core power, the settings of PLL still keep there. But we switch the clock source of CPU to CLK_M before shut off PLLs by CCLKG_BURSY_POLICY register. So we need to resume it back to original clock source by CCLKG_BURST_POLICY register. Or it would be keep in low rate (CLK_M) after resume.

OK, I guess the register name was badly chosen by HW. I'd like to see part of your description above in the patch description. How about replacing the patch description with:

----------
When the system suspends to LP1, the CPU clock source is switched to CLK_M (12MHz Oscillator) during suspend/resume flow[1]. The CPU clock source is controlled by the CCLKG_BURST_POLICY register, and hence this register must be restored during LP1 resume.
----------

[1] Question: where does this happen? This patch doesn't make that change. I wonder why the suspend path can't save this register, rather than implementing a separate suspend syscore op in the clock driver. Does the HW auto-switch the value in the register itself?

Quoted 10 lines

Quoted 1 line

Why don't Tegra20/30 need a similar change?

For Tegra20/30, the same code had been implemented in the suspend/resume function of tegra_cpu_car_ops. It restores the CPU clock ASAP when CPU resume from a suspend state to get quick performance I believe.

For Tegra114, the resume performance is cool (although we can't see it in upstream kernel now, it still need some other functions.). We can implement all the clock related suspend/resume function in the clock driver.

OK, I do see something similar in tegra20/30_cpu_clock_suspend/resume. Why can't this new code be part of the equivalent functions; does the Tegra114 suspend/resume code in mach-tegra/ not call tegra_cpu_car_ops.suspend/resume() in the same way it does on Tegra20/30?

Quoted 31 lines · Stephen Warren

Quoted 12 lines · Joseph Lo

Quoted 11 lines · Stephen Warren

Quoted 3 lines · Joseph Lo

When the system suspends to LP1, the clock of the CPU would be switched to CLK_M (12MHz Oscillator) during suspend/resume flow. The clock driver needs to restore the clock of CPU after LP1 resume.

It's unclear to me how the code change implements "restore the clock of the CPU". A register name of CCLKG_BURST_POLICY doesn't sound like it's anything to do with enabled/disabling the CPU clock, nor configuring its rate. What exactly does this register do, and hence what does this new code actually restore?

Quoted 6 lines

When system suspend to LP1, most of the PLLs was clock gated. Because we didn't cut off the core power, the settings of PLL still keep there. But we switch the clock source of CPU to CLK_M before shut off PLLs by CCLKG_BURSY_POLICY register. So we need to resume it back to original clock source by CCLKG_BURST_POLICY register. Or it would be keep in low rate (CLK_M) after resume.

OK, I guess the register name was badly chosen by HW. I'd like to see part of your description above in the patch description. How about replacing the patch description with:

----------
When the system suspends to LP1, the CPU clock source is switched to CLK_M (12MHz Oscillator) during suspend/resume flow[1]. The CPU clock source is controlled by the CCLKG_BURST_POLICY register, and hence this register must be restored during LP1 resume.
----------

Quoted 4 lines

[1] Question: where does this happen? This patch doesn't make that change. I wonder why the suspend path can't save this register, rather than implementing a separate suspend syscore op in the clock driver. Does the HW auto-switch the value in the register itself?

If we switch CPU to CLK_M in clock driver, the system will become slowly during the middle of suspending flow. We do this at the very end of the LP1 suspending flow before the CPU disable all the PLL clocks.

Quoted 11 lines

Quoted 10 lines

Quoted 1 line

Why don't Tegra20/30 need a similar change?

For Tegra20/30, the same code had been implemented in the suspend/resume function of tegra_cpu_car_ops. It restores the CPU clock ASAP when CPU resume from a suspend state to get quick performance I believe.

For Tegra114, the resume performance is cool (although we can't see it in upstream kernel now, it still need some other functions.). We can implement all the clock related suspend/resume function in the clock driver.

Quoted 4 lines

OK, I do see something similar in tegra20/30_cpu_clock_suspend/resume. Why can't this new code be part of the equivalent functions; does the Tegra114 suspend/resume code in mach-tegra/ not call tegra_cpu_car_ops.suspend/resume() in the same way it does on Tegra20/30?

One of the main reasons is due to DFLL clock. The CPU clock of Tegra114 is going to switch to DFLL to a get higher clock rate. But it depends some other HW (i.e. I2C), we can't resume it so early when the CPU just resume like we did in tegra_cpu_car_ops.suspend/resume for Tegra20/30.

Quoted 37 lines · Joseph Lo

Quoted 35 lines · Stephen Warren

Quoted 12 lines · Joseph Lo

Quoted 11 lines · Stephen Warren

Quoted 3 lines · Joseph Lo

When the system suspends to LP1, the clock of the CPU would be switched to CLK_M (12MHz Oscillator) during suspend/resume flow. The clock driver needs to restore the clock of CPU after LP1 resume.

It's unclear to me how the code change implements "restore the clock of the CPU". A register name of CCLKG_BURST_POLICY doesn't sound like it's anything to do with enabled/disabling the CPU clock, nor configuring its rate. What exactly does this register do, and hence what does this new code actually restore?

Quoted 6 lines

When system suspend to LP1, most of the PLLs was clock gated. Because we didn't cut off the core power, the settings of PLL still keep there. But we switch the clock source of CPU to CLK_M before shut off PLLs by CCLKG_BURSY_POLICY register. So we need to resume it back to original clock source by CCLKG_BURST_POLICY register. Or it would be keep in low rate (CLK_M) after resume.

OK, I guess the register name was badly chosen by HW. I'd like to see part of your description above in the patch description. How about replacing the patch description with:

----------
When the system suspends to LP1, the CPU clock source is switched to CLK_M (12MHz Oscillator) during suspend/resume flow[1]. The CPU clock source is controlled by the CCLKG_BURST_POLICY register, and hence this register must be restored during LP1 resume.
----------

[1] Question: where does this happen? This patch doesn't make that change. I wonder why the suspend path can't save this register, rather than implementing a separate suspend syscore op in the clock driver. Does the HW auto-switch the value in the register itself?

Quoted 3 lines

If we switch CPU to CLK_M in clock driver, the system will become slowly during the middle of suspending flow. We do this at the very end of the LP1 suspending flow before the CPU disable all the PLL clocks.

I think you answered "why is the switch to CLK_M performed very late", whereas I asked "where is the code that performs the switch to CLK_M".

Quoted 38 lines · Stephen Warren

Quoted 37 lines · Joseph Lo

Quoted 35 lines · Stephen Warren

Quoted 12 lines · Joseph Lo

Quoted 11 lines · Stephen Warren

Quoted 3 lines · Joseph Lo

When the system suspends to LP1, the clock of the CPU would be switched to CLK_M (12MHz Oscillator) during suspend/resume flow. The clock driver needs to restore the clock of CPU after LP1 resume.

It's unclear to me how the code change implements "restore the clock of the CPU". A register name of CCLKG_BURST_POLICY doesn't sound like it's anything to do with enabled/disabling the CPU clock, nor configuring its rate. What exactly does this register do, and hence what does this new code actually restore?

Quoted 6 lines

When system suspend to LP1, most of the PLLs was clock gated. Because we didn't cut off the core power, the settings of PLL still keep there. But we switch the clock source of CPU to CLK_M before shut off PLLs by CCLKG_BURSY_POLICY register. So we need to resume it back to original clock source by CCLKG_BURST_POLICY register. Or it would be keep in low rate (CLK_M) after resume.

OK, I guess the register name was badly chosen by HW. I'd like to see part of your description above in the patch description. How about replacing the patch description with:

----------
When the system suspends to LP1, the CPU clock source is switched to CLK_M (12MHz Oscillator) during suspend/resume flow[1]. The CPU clock source is controlled by the CCLKG_BURST_POLICY register, and hence this register must be restored during LP1 resume.
----------

[1] Question: where does this happen? This patch doesn't make that change. I wonder why the suspend path can't save this register, rather than implementing a separate suspend syscore op in the clock driver. Does the HW auto-switch the value in the register itself?

Quoted 6 lines

Quoted 3 lines

If we switch CPU to CLK_M in clock driver, the system will become slowly during the middle of suspending flow. We do this at the very end of the LP1 suspending flow before the CPU disable all the PLL clocks.

I think you answered "why is the switch to CLK_M performed very late", whereas I asked "where is the code that performs the switch to CLK_M".

To expand on this a bit more, I can't find any reference to register CCLKG_BURST_POLICY in arch/arm/mach-tegra/ or drivers/clk/tegra/ except for the definition of clock cclk_g, nor any reference to that clock in those two directories. And that CCLKG_BURST_POLICY register is what this patch saves/restores.

I do see function tegra30_switch_cpu_to_clk32k in patch 5/8 appears to do something related to switching to clk_m and touches some other burst-related registers, but not CCLKG_BURST_POLICY...

So, if this syscore_op is attempting to undo some change that happens to CCLKG_BURST_POLICY during suspend, I still can't see what change is happening to that register during suspemd, nor which piece of code causes it.

If the issue is that the value in this register is simply lost during LP1 because the power is turned off, then I wonder what sets up this register during the original boot path?

Quoted 39 lines · Stephen Warren

Quoted 38 lines · Stephen Warren

Quoted 37 lines · Joseph Lo

Quoted 35 lines · Stephen Warren

Quoted 12 lines · Joseph Lo

Quoted 11 lines · Stephen Warren

Quoted 3 lines · Joseph Lo

When the system suspends to LP1, the clock of the CPU would be switched to CLK_M (12MHz Oscillator) during suspend/resume flow. The clock driver needs to restore the clock of CPU after LP1 resume.

It's unclear to me how the code change implements "restore the clock of the CPU". A register name of CCLKG_BURST_POLICY doesn't sound like it's anything to do with enabled/disabling the CPU clock, nor configuring its rate. What exactly does this register do, and hence what does this new code actually restore?

Quoted 6 lines

When system suspend to LP1, most of the PLLs was clock gated. Because we didn't cut off the core power, the settings of PLL still keep there. But we switch the clock source of CPU to CLK_M before shut off PLLs by CCLKG_BURSY_POLICY register. So we need to resume it back to original clock source by CCLKG_BURST_POLICY register. Or it would be keep in low rate (CLK_M) after resume.

OK, I guess the register name was badly chosen by HW. I'd like to see part of your description above in the patch description. How about replacing the patch description with:

----------
When the system suspends to LP1, the CPU clock source is switched to CLK_M (12MHz Oscillator) during suspend/resume flow[1]. The CPU clock source is controlled by the CCLKG_BURST_POLICY register, and hence this register must be restored during LP1 resume.
----------

[1] Question: where does this happen? This patch doesn't make that change. I wonder why the suspend path can't save this register, rather than implementing a separate suspend syscore op in the clock driver. Does the HW auto-switch the value in the register itself?

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

If we switch CPU to CLK_M in clock driver, the system will become slowly during the middle of suspending flow. We do this at the very end of the LP1 suspending flow before the CPU disable all the PLL clocks.

I think you answered "why is the switch to CLK_M performed very late", whereas I asked "where is the code that performs the switch to CLK_M".

You can find to code in the function tegraXX_switch_cpu_to_clk32k of the patch 5/8 and 6/8. It switches the SCLK and CCLK to CLKM before disabling the PLLs.

Quoted 11 lines

To expand on this a bit more, I can't find any reference to register CCLKG_BURST_POLICY in arch/arm/mach-tegra/ or drivers/clk/tegra/ except for the definition of clock cclk_g, nor any reference to that clock in those two directories. And that CCLKG_BURST_POLICY register is what this patch saves/restores.

I do see function tegra30_switch_cpu_to_clk32k in patch 5/8 appears to do something related to switching to clk_m and touches some other burst-related registers, but not CCLKG_BURST_POLICY...

OK. It's a little difference we do in clock driver side and low level code side.

As you knew, we have two CPU clusters (CCLKG & CCLKLP) in Tegra chips. And they have exactly the same clock source for Tegra20/30, but Tegra114. From the low level side, it should be supported when the system going to suspend with CPU_G or CPU_LP. So we switched the clock source of CPU to CLK_M by CCLK_BURST_POLICY. This register can support the running CPU switch to the clock source they want.

On the clock driver side, because the clock source of CCLKG and CCLKLP is not exactly the same for the Tegra114. The CCLKG supports DFLL clock but CCLKLP. We need to restore it separately to avoid the CPU restore to the wrong clock source.

The CPU clock suspend/resume function in the tegra_cpu_car_ops of Tegra20/30 is also using CCLK_BURST_POLICY register, because the definition of the clock source of CCLKG and CCLKLP is the same. We can simply the code for just using CCLK_BURST_POLICY.

I think the code should OK when CPU_G or CPU_LP running with it.

Quoted 7 lines · Joseph Lo

Quoted 6 lines · Stephen Warren

Quoted 5 lines · Stephen Warren

Quoted 4 lines · Joseph Lo

Quoted 3 lines · Stephen Warren

Quoted 2 lines · Joseph Lo

Quoted 1 line · Stephen Warren

On 07/26/2013 03:15 AM, Joseph Lo wrote:

Quoted 20 lines

OK. It's a little difference we do in clock driver side and low level code side.

As you knew, we have two CPU clusters (CCLKG & CCLKLP) in Tegra chips. And they have exactly the same clock source for Tegra20/30, but Tegra114. From the low level side, it should be supported when the system going to suspend with CPU_G or CPU_LP. So we switched the clock source of CPU to CLK_M by CCLK_BURST_POLICY. This register can support the running CPU switch to the clock source they want.

On the clock driver side, because the clock source of CCLKG and CCLKLP is not exactly the same for the Tegra114. The CCLKG supports DFLL clock but CCLKLP. We need to restore it separately to avoid the CPU restore to the wrong clock source.

The CPU clock suspend/resume function in the tegra_cpu_car_ops of Tegra20/30 is also using CCLK_BURST_POLICY register, because the definition of the clock source of CCLKG and CCLKLP is the same. We can simply the code for just using CCLK_BURST_POLICY.

Quoted 1 line

I think the code should OK when CPU_G or CPU_LP running with it.

OK, so just to paraphrase what you've said, to make sure I understand it:

On Tegra114, there are 3 registers:

1) Controls just the clock source of the G cluster.

2) Controls just the clock source of the LP cluster.

3) Controls both at the same time, simulating a write to both (1) and (2)

In tegra30_switch_cpu_to_clk32k(), we use register (3) above to switch to clk_m then clk_s. This is possible, because in both registers (1) and (2), the values for clk_m and clk_s are identical.

In tegra30_lp1_reset(), we can't use the same technique (a write to register 3) to restore the clock source for both G and LP clusters, since the values we need to write to those registers to re-select their clock source is different.

however, on Tegra30, there really is only one register, so we can use that to both switch to clk_m/clk_s, /and/ to switch back.

That all explains the following part of patch 7/8, which disables the clock register restore path except on Tegra30 where it will work:

Review

Quoted 6 lines

(quoted diff) +4 -2

-	mov32	r4, ((1 << 28) | (0x8))	@ burst policy is PLLX
-	str	r4, [r0, #CLK_RESET_CCLK_BURST]
+	cmp	r10, #TEGRA30
+	movweq	r4, #:lower16:((1 << 28) | (0x8))	@ burst policy is PLLX
+	movteq	r4, #:upper16:((1 << 28) | (0x8))
+	streq	r4, [r0, #CLK_RESET_CCLK_BURST]

However, none of this explains why the CPU clock restore logic on Tegra114 needs to be a syscore_op, rather than simply having tegra30_lp1_reset() execute some Tegra114-specific code.

Re-writing the part of the patch I quoted above in C, it looks like:

if (soc is tegra30)
write to CLK_RESET_CCLK_BURST to select pllx

Instead of making that code do nothing on Tegra114, why can't that code be:

if (soc is tegra30)
write to CLK_RESET_CCLK_BURST to re-select pllx
else
write to G-cluster-specific register to restore saved value write to LP-cluster-specific register to restore saved value

or:

if (soc is tegra30)
write to CLK_RESET_CCLK_BURST to re-select pllx
else
write to G-cluster-specific register to re-select pllx/dfll/... write to LP-cluster-specific register to restore ...

Quoted 8 lines · Stephen Warren

Quoted 7 lines · Joseph Lo

Quoted 6 lines · Stephen Warren

Quoted 5 lines · Stephen Warren

Quoted 4 lines · Joseph Lo

Quoted 3 lines · Stephen Warren

Quoted 2 lines · Joseph Lo

Quoted 1 line · Stephen Warren

On 07/26/2013 03:15 AM, Joseph Lo wrote:

[snip]

Quoted 20 lines

Quoted 20 lines

OK. It's a little difference we do in clock driver side and low level code side.

As you knew, we have two CPU clusters (CCLKG & CCLKLP) in Tegra chips. And they have exactly the same clock source for Tegra20/30, but Tegra114. From the low level side, it should be supported when the system going to suspend with CPU_G or CPU_LP. So we switched the clock source of CPU to CLK_M by CCLK_BURST_POLICY. This register can support the running CPU switch to the clock source they want.

On the clock driver side, because the clock source of CCLKG and CCLKLP is not exactly the same for the Tegra114. The CCLKG supports DFLL clock but CCLKLP. We need to restore it separately to avoid the CPU restore to the wrong clock source.

The CPU clock suspend/resume function in the tegra_cpu_car_ops of Tegra20/30 is also using CCLK_BURST_POLICY register, because the definition of the clock source of CCLKG and CCLKLP is the same. We can simply the code for just using CCLK_BURST_POLICY.

Quoted 5 lines

Quoted 1 line

I think the code should OK when CPU_G or CPU_LP running with it.

OK, so just to paraphrase what you've said, to make sure I understand it:

On Tegra114, there are 3 registers:

It's on Tegra30 too.

Quoted 2 lines

1) Controls just the clock source of the G cluster.

CCLKG_BURST_POLICY at offset 0x368.

Quoted 1 line

2) Controls just the clock source of the LP cluster.

CCLKLP_BURST_POLICY at offset 0x370.

Quoted 1 line

3) Controls both at the same time, simulating a write to both (1) and (2)

CCLK_BURST_POLICY at offset 0x20.
Yes, exactly. And I double confirm the item 3 again with a downstream kernel that support cluster switch today. It's true.

Quoted 4 lines

In tegra30_switch_cpu_to_clk32k(), we use register (3) above to switch to clk_m then clk_s. This is possible, because in both registers (1) and (2), the values for clk_m and clk_s are identical.

Yes.

Quoted 12 lines

In tegra30_lp1_reset(), we can't use the same technique (a write to register 3) to restore the clock source for both G and LP clusters, since the values we need to write to those registers to re-select their clock source is different.

however, on Tegra30, there really is only one register, so we can use that to both switch to clk_m/clk_s, /and/ to switch back.

That all explains the following part of patch 7/8, which disables the clock register restore path except on Tegra30 where it will work:

Quoted 6 lines

Review

Quoted 6 lines

(quoted diff) +4 -2

-	mov32	r4, ((1 << 28) | (0x8))	@ burst policy is PLLX
-	str	r4, [r0, #CLK_RESET_CCLK_BURST]
+	cmp	r10, #TEGRA30
+	movweq	r4, #:lower16:((1 << 28) | (0x8))	@ burst policy is PLLX
+	movteq	r4, #:upper16:((1 << 28) | (0x8))
+	streq	r4, [r0, #CLK_RESET_CCLK_BURST]

OK. The burst policy of CPU clock has 5 different states (STDBY, IDLE, RUN, IRQ and FIQ). The SW only can set up the clock source of each state. The switching to different state was controlled by HW.

So the Tegra30 code here was set up the clock source of IDLE state only (the CPU runs in this state after resume) to make the performance better. But It had no idea about the clock sources of the other states. It still needs clock driver to take care of that.

The clock source id (0x8) means PLLX_OUT0 in Tegra30. In Tegra114, it means PLLX_OUT0_LJ (low jitter). It's not available at this moment. I can switch it to PLLX_OUT0 (0xe) for Tegra114 too. The code may like below.

Review

Quoted 7 lines

(quoted diff) +5 -2

-     mov32   r4, ((1 << 28) | (0x8)) @ burst policy is PLLX
-     str     r4, [r0, #CLK_RESET_CCLK_BURST]
+     cmp     r10, #TEGRA30
+     movweq  r4, #:lower16:((1 << 28) | (0x8))
+     movteq  r4, #:upper16:((1 << 28) | (0x8))
+	movwne  r4, #:lower16:((1 << 28) | (0xe))
+	movteq  r4, #:upper16:((1 << 28) | (0xe))

Quoted 1 line

+ str r4, [r0, #CLK_RESET_CCLK_BURST]

I had tested this code before I sent this series. It didn't impact or improve the resuming performance. So I removed them.
Do you want me to add them back?

Quoted 17 lines

However, none of this explains why the CPU clock restore logic on Tegra114 needs to be a syscore_op, rather than simply having tegra30_lp1_reset() execute some Tegra114-specific code.

Re-writing the part of the patch I quoted above in C, it looks like:

if (soc is tegra30)
write to CLK_RESET_CCLK_BURST to select pllx

Instead of making that code do nothing on Tegra114, why can't that code be:

if (soc is tegra30)
write to CLK_RESET_CCLK_BURST to re-select pllx
else
write to G-cluster-specific register to restore saved value write to LP-cluster-specific register to restore saved value

Quoted 8 lines

or:

if (soc is tegra30)
write to CLK_RESET_CCLK_BURST to re-select pllx
else
write to G-cluster-specific register to re-select pllx/dfll/... write to LP-cluster-specific register to restore ...

Hope the explanation above is enough for you. The low level code had no idea of clock source of each CPU state in BURST_POLICY register. It needs to be done by clk driver itself.

Quoted 10 lines · Joseph Lo

Quoted 8 lines · Stephen Warren

Quoted 7 lines · Joseph Lo

Quoted 6 lines · Stephen Warren

Quoted 5 lines · Stephen Warren

Quoted 4 lines · Joseph Lo

Quoted 3 lines · Stephen Warren

Quoted 2 lines · Joseph Lo

Quoted 1 line · Stephen Warren

On 07/26/2013 03:15 AM, Joseph Lo wrote:

[snip]

Quoted 32 lines

Quoted 11 lines

In tegra30_lp1_reset(), we can't use the same technique (a write to register 3) to restore the clock source for both G and LP clusters, since the values we need to write to those registers to re-select their clock source is different.

however, on Tegra30, there really is only one register, so we can use that to both switch to clk_m/clk_s, /and/ to switch back.

That all explains the following part of patch 7/8, which disables the clock register restore path except on Tegra30 where it will work:

Quoted 6 lines

Review

Quoted 6 lines

(quoted diff) +4 -2

-	mov32	r4, ((1 << 28) | (0x8))	@ burst policy is PLLX
-	str	r4, [r0, #CLK_RESET_CCLK_BURST]
+	cmp	r10, #TEGRA30
+	movweq	r4, #:lower16:((1 << 28) | (0x8))	@ burst policy is PLLX
+	movteq	r4, #:upper16:((1 << 28) | (0x8))
+	streq	r4, [r0, #CLK_RESET_CCLK_BURST]

OK. The burst policy of CPU clock has 5 different states (STDBY, IDLE, RUN, IRQ and FIQ). The SW only can set up the clock source of each state. The switching to different state was controlled by HW.

So the Tegra30 code here was set up the clock source of IDLE state only (the CPU runs in this state after resume) to make the performance better. But It had no idea about the clock sources of the other states. It still needs clock driver to take care of that.

The clock source id (0x8) means PLLX_OUT0 in Tegra30. In Tegra114, it means PLLX_OUT0_LJ (low jitter). It's not available at this moment. I can switch it to PLLX_OUT0 (0xe) for Tegra114 too. The code may like below.

Quoted 7 lines

Review

Quoted 7 lines

(quoted diff) +5 -2

-     mov32   r4, ((1 << 28) | (0x8)) @ burst policy is PLLX
-     str     r4, [r0, #CLK_RESET_CCLK_BURST]
+     cmp     r10, #TEGRA30
+     movweq  r4, #:lower16:((1 << 28) | (0x8))
+     movteq  r4, #:upper16:((1 << 28) | (0x8))
+	movwne  r4, #:lower16:((1 << 28) | (0xe))
+	movteq  r4, #:upper16:((1 << 28) | (0xe))

I assume that should be movtne.

Quoted 6 lines

Quoted 2 lines

+ str r4, [r0, #CLK_RESET_CCLK_BURST]

I had tested this code before I sent this series. It didn't impact or improve the resuming performance. So I removed them.
Do you want me to add them back?

I think adding that code back would make sense. It would mean the code works fundamentally the same way for all SoCs.

BTW, you mentioned that LP1 resume takes 10-15 seconds on Dalmore. Are you sure this isn't because the above code is missing, and hence the CPU keeps running at 32KHz or the crystal speed, since nothing restores the CPU clock source until much later, when the syscore op runs?

Quoted 26 lines

Quoted 18 lines

However, none of this explains why the CPU clock restore logic on Tegra114 needs to be a syscore_op, rather than simply having tegra30_lp1_reset() execute some Tegra114-specific code.

Re-writing the part of the patch I quoted above in C, it looks like:

if (soc is tegra30)
write to CLK_RESET_CCLK_BURST to select pllx

Instead of making that code do nothing on Tegra114, why can't that code be:

if (soc is tegra30)
write to CLK_RESET_CCLK_BURST to re-select pllx
else
write to G-cluster-specific register to restore saved value write to LP-cluster-specific register to restore saved value

Quoted 8 lines

or:

if (soc is tegra30)
write to CLK_RESET_CCLK_BURST to re-select pllx
else
write to G-cluster-specific register to re-select pllx/dfll/... write to LP-cluster-specific register to restore ...

Quoted 3 lines

Hope the explanation above is enough for you. The low level code had no idea of clock source of each CPU state in BURST_POLICY register. It needs to be done by clk driver itself.

I still have absolutely no idea why Tegra30 and Tegra114 are different.

You mentioned something about this low-level code only manipulating the IDLE state, and the clock driver needing to restore the other 4 states. This raises yet more questions:

1) Do we not need to restore the other 4 states on Tegra30? If not, why not? If we do, presumably Tegra30 (and Tegra20?) need to the syscore_op this patch series adds to Tegra114 only? If we don't, then why does Tegra114 have to restore them?

2) What triggers the HW to switch from IDLE to RUN state?

In other
words, I think you're saying that the existing Tegra30 code:

	mov32	r4, ((1 << 28) | (0x8))	@ burst policy is PLLX
	str	r4, [r0, #CLK_RESET_CCLK_BURST]

doesn't change the clock rate right away. When does it change?

Related, when tegra30_switch_cpu_to_clk32k() sets the CPU clock source for the IDLE state to clk_s, when does that switch actually take place?

That function appears to assume it happens immediately, since it immediately disables the PLLs that might have been the previous CPU clock source. Perhaps this is what the MSELECT register writes do?

I
guess not since there's only 1 MSELECT write in tegra_lp1_reset() and two changes of CPU clock source.

On Thu, 2013-08-08 at 00:46 +0800, Stephen Warren wrote:

Quoted 10 lines

I still have absolutely no idea why Tegra30 and Tegra114 are different.

You mentioned something about this low-level code only manipulating the IDLE state, and the clock driver needing to restore the other 4 states. This raises yet more questions:

1) Do we not need to restore the other 4 states on Tegra30? If not, why not? If we do, presumably Tegra30 (and Tegra20?) need to the syscore_op this patch series adds to Tegra114 only? If we don't, then why does Tegra114 have to restore them?

We need to restore all of them for all Tegra chips. For Tegra20/30, we had done it in the tegra_cpu_car_ops.suspend/resume. For Tegra114, the patch was here.

The other reason is:
1) The PLLX is the main CPU clock source in Tegra20/30. We can restore it ASAP to get a better performance.
2) For Tegra114, the PLLX is the CPU clock source when CPU runs at low rates. When CPU in high rate, it uses DFLL as clock source. So it depends on what the clock source of the CPU when it goes into suspend. And the DFLL has its own resume code, it needs to be restored before the CPU uses it as clock source again. It makes the CPU clock restore sequence like this.

Quoted 1 line

2) What triggers the HW to switch from IDLE to RUN state?

I also want to know more detail about it. The TRM only said it decided by HW and gave an example about when switching to IRQ or FIQ state.

Quoted 3 lines

In other
words, I think you're saying that the existing Tegra30 code:

Quoted 4 lines

	mov32	r4, ((1 << 28) | (0x8))	@ burst policy is PLLX
	str	r4, [r0, #CLK_RESET_CCLK_BURST]

doesn't change the clock rate right away. When does it change?

The code before this is the PLLX re-enable code. Then it switches to PLLX. The rate is still kept the same when it suspended. The next rate change after resume would be happened in the CPUfreq driver.

Quoted 1 line · Joseph Lo

On Thu, 2013-08-08 at 00:46 +0800, Stephen Warren wrote:

..

Quoted 15 lines

Quoted 10 lines

I still have absolutely no idea why Tegra30 and Tegra114 are different.

You mentioned something about this low-level code only manipulating the IDLE state, and the clock driver needing to restore the other 4 states. This raises yet more questions:

1) Do we not need to restore the other 4 states on Tegra30? If not, why not? If we do, presumably Tegra30 (and Tegra20?) need to the syscore_op this patch series adds to Tegra114 only? If we don't, then why does Tegra114 have to restore them?

We need to restore all of them for all Tegra chips. For Tegra20/30, we had done it in the tegra_cpu_car_ops.suspend/resume. For Tegra114, the patch was here.

Quoted 9 lines

The other reason is:
1) The PLLX is the main CPU clock source in Tegra20/30. We can restore it ASAP to get a better performance.
2) For Tegra114, the PLLX is the CPU clock source when CPU runs at low rates. When CPU in high rate, it uses DFLL as clock source. So it depends on what the clock source of the CPU when it goes into suspend. And the DFLL has its own resume code, it needs to be restored before the CPU uses it as clock source again. It makes the CPU clock restore sequence like this.

So, we don't yet support the DFLL upstream. Presumably, the CPU is always running off PLLX on Tegra114 upstream right now. As such, we can hard-code that into the resume path just like we do on earlier chips, i.e. using tegra_cpu_car_ops.resume().

Once we do get DFLL support, presumably the DFLL resume path can switch the register from PLLX to DFLL, and we still won't need a custom syscore_ops.

Will that work?

Quoted 8 lines · Joseph Lo

Quoted 7 lines · Stephen Warren

Quoted 6 lines · Joseph Lo

Quoted 5 lines · Stephen Warren

Quoted 3 lines · Joseph Lo

When the system suspends to LP1, the clock of the CPU would be switched to CLK_M (12MHz Oscillator) during suspend/resume flow. The clock driver needs to restore the clock of CPU after LP1 resume.

Quoted 16 lines

Quoted 11 lines

Quoted 10 lines

Quoted 1 line

Why don't Tegra20/30 need a similar change?

For Tegra20/30, the same code had been implemented in the suspend/resume function of tegra_cpu_car_ops. It restores the CPU clock ASAP when CPU resume from a suspend state to get quick performance I believe.

For Tegra114, the resume performance is cool (although we can't see it in upstream kernel now, it still need some other functions.). We can implement all the clock related suspend/resume function in the clock driver.

Quoted 4 lines

OK, I do see something similar in tegra20/30_cpu_clock_suspend/resume. Why can't this new code be part of the equivalent functions; does the Tegra114 suspend/resume code in mach-tegra/ not call tegra_cpu_car_ops.suspend/resume() in the same way it does on Tegra20/30?

Quoted 4 lines

One of the main reasons is due to DFLL clock. The CPU clock of Tegra114 is going to switch to DFLL to a get higher clock rate. But it depends some other HW (i.e. I2C), we can't resume it so early when the CPU just resume like we did in tegra_cpu_car_ops.suspend/resume for Tegra20/30.

Is there a guarantee that the syscore_ops are run after the call to tegar_cpu_car_ops.resume() would be? Perhaps the mach-tegra/ code should simply call tegra_cpu_car_ops.resume() later on Tegra114 than it does on earlier chips, so it's run at a suitable time?

The LP1 suspending mode on Tegra means CPU rail off, devices and PLLs are clock gated and SDRAM in self-refresh mode. That means the low level LP1 suspending and resuming code couldn't be run on DRAM and the CPU must switch to the always on clock domain (a.k.a. CLK_M 12MHz oscillator). And the system clock (SCLK) would be switched to CLK_S, a 32KHz oscillator. The LP1 low level handling code need to be moved to IRAM area first. And marking the LP1 mask for indicating the Tegra device is in LP1. The CPU power timer needs to be re-calculated based on 32KHz that was originally based on PCLK.

arch/arm/mach-tegra/pm.c +10 -0

+static void tegra_suspend_enter_lp1(void)
+{
+	tegra_pmc_suspend();
+
+	/* copy the reset vector & SDRAM shutdown code into IRAM */
+	memcpy(iram_save_addr, iram_code, iram_save_size);
+	memcpy(iram_code, tegra_lp1_iram->start_addr, iram_save_size);
+
+	*((u32 *)tegra_cpu_lp1_mask) = 1;
+}

Quoted 11 lines · Joseph Lo

The LP1 suspending mode on Tegra means CPU rail off, devices and PLLs are clock gated and SDRAM in self-refresh mode. That means the low level LP1 suspending and resuming code couldn't be run on DRAM and the CPU must switch to the always on clock domain (a.k.a. CLK_M 12MHz oscillator). And the system clock (SCLK) would be switched to CLK_S, a 32KHz oscillator. The LP1 low level handling code need to be moved to IRAM area first. And marking the LP1 mask for indicating the Tegra device is in LP1. The CPU power timer needs to be re-calculated based on 32KHz that was originally based on PCLK.

arch/arm/mach-tegra/pm.c +0 -0

arch/arm/mach-tegra/pm.c line 51

Quoted 8 lines

#ifdef CONFIG_PM_SLEEP
static DEFINE_SPINLOCK(tegra_lp2_lock);

arch/arm/mach-tegra/pm.c +5 -0

+static void __iomem *iram_code = IO_ADDRESS(TEGRA_IRAM_CODE_AREA);
+static u32 iram_save_size;
+static void *iram_save_addr;
+struct tegra_lp1_iram *tegra_lp1_iram;
 void (*tegra_tear_down_cpu)(void);
+void (*tegra_sleep_core_finish)(unsigned long v2p);

I'm not sure all of those are required to be global variables. For example, iram_code is just a constant, so you could easily just use it directly in code.

arch/arm/mach-tegra/pm.c enum tegra_suspend_mode tegra_pm_validate_suspend_mode( · line 229

Quoted 7 lines

arch/arm/mach-tegra/pm.c +7 -0

+static bool tegra_sleep_core_init(void)
+{
+	if (!tegra_sleep_core_finish)
+		return false;
+
+	return true;
+}

That function seems a little pointless. Why not just check the value of tegra_sleep_core_finish directly in tegra_init_suspend()?

Quoted 16 lines

arch/arm/mach-tegra/pm.c +16 -0

+static void tegra_suspend_enter_lp1(void)
+{
+	tegra_pmc_suspend();
+
+	/* copy the reset vector & SDRAM shutdown code into IRAM */
+	memcpy(iram_save_addr, iram_code, iram_save_size);
+	memcpy(iram_code, tegra_lp1_iram->start_addr, iram_save_size);
+
+	*((u32 *)tegra_cpu_lp1_mask) = 1;
+}
+
+static void tegra_suspend_exit_lp1(void)
+{
+	tegra_pmc_resume();
+
+	/* restore IRAM */

arch/arm/mach-tegra/pm.c enum tegra_suspend_mode tegra_pm_validate_suspend_mode( · line 250

Quoted 4 lines

arch/arm/mach-tegra/pm.c +4 -0

+	memcpy(iram_code, iram_save_addr, iram_save_size);
+
+	*(u32 *)tegra_cpu_lp1_mask = 0;
+}

I'm not really sure I like that, but I suppose it's OK. It sure seems like a performance limiter, but I suppose LP1 is so slow it doesn't matter, due to the need to ramp power rails, PLLs, SDRAM controller, etc.

It'd be nice to simply reserve more IRAM for the kernel's use. Right now, only 1K is reserved, and presumably the code running on the AVP can't use the rest of that page anyway, or can it?

arch/arm/mach-tegra/pm.c static int tegra_suspend_enter(suspend_state_t state) · line 288

Quoted 10 lines

arch/arm/mach-tegra/pm.c +4 -1

@@ -212,9 +282,15 @@ static int tegra_suspend_enter(suspend_state_t state)
 		break;
 	}
 
-	cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu);
+	if (mode == TEGRA_SUSPEND_LP2)
+		cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu);
+	else
+		cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_core);

Nit: It might be slightly simpler to simply calculate the parameter to cpu_suspend(), but then take the same code-path:

if (mode == TEGRA_SUSPEND_LP2)

	sleep_func = tegra_sleep_cpu;
else
	sleep_func = tegra_sleep_core;

cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, sleep_func);

That way, it's a little more obvious that the code always calls cpu_suspend() in the same way, but simply passes a different function to it to power things down at the end.

arch/arm/mach-tegra/pmc.c enum tegra_suspend_mode tegra_pmc_get_suspend_mode(void) · line 220

Quoted 8 lines

arch/arm/mach-tegra/pmc.c +1 -1

 
 void tegra_pmc_pm_set(enum tegra_suspend_mode mode)
 {
 	u32 reg, csr_reg;
-	unsigned long rate = 0;
+	unsigned long rate = 32768;

That's LP1-specific. You should add a "case TEGRA_SUSPEND_LP1:" to the switch statement that calculates rate instead; after all, that's the whole point of having that switch statement.

Quoted 13 lines · Stephen Warren

Quoted 11 lines · Joseph Lo

The LP1 suspending mode on Tegra means CPU rail off, devices and PLLs are clock gated and SDRAM in self-refresh mode. That means the low level LP1 suspending and resuming code couldn't be run on DRAM and the CPU must switch to the always on clock domain (a.k.a. CLK_M 12MHz oscillator). And the system clock (SCLK) would be switched to CLK_S, a 32KHz oscillator. The LP1 low level handling code need to be moved to IRAM area first. And marking the LP1 mask for indicating the Tegra device is in LP1. The CPU power timer needs to be re-calculated based on 32KHz that was originally based on PCLK.

arch/arm/mach-tegra/pm.c +0 -0

Quoted 12 lines

arch/arm/mach-tegra/pm.c line 51

Quoted 8 lines

#ifdef CONFIG_PM_SLEEP
static DEFINE_SPINLOCK(tegra_lp2_lock);

arch/arm/mach-tegra/pm.c +5 -0

+static void __iomem *iram_code = IO_ADDRESS(TEGRA_IRAM_CODE_AREA);
+static u32 iram_save_size;
+static void *iram_save_addr;
+struct tegra_lp1_iram *tegra_lp1_iram;
 void (*tegra_tear_down_cpu)(void);
+void (*tegra_sleep_core_finish)(unsigned long v2p);

I'm not sure all of those are required to be global variables. For example, iram_code is just a constant, so you could easily just use it directly in code.

All of them does not mean the same thing. The LP1 resume code was built in kernel image and store in RAM.
The tegra_lp1_iram hooks the LP1 resume code for different chips. Before LP1 suspend, the original stuffs that in the area of IRAM would be store in the iram_save_addr (RAM). Then copy the LP1 resume code to iram_code area (IRAM).

Quoted 10 lines

arch/arm/mach-tegra/pm.c enum tegra_suspend_mode tegra_pm_validate_suspend_mode( · line 229

Quoted 7 lines

arch/arm/mach-tegra/pm.c +7 -0

+static bool tegra_sleep_core_init(void)
+{
+	if (!tegra_sleep_core_finish)
+		return false;
+
+	return true;
+}

That function seems a little pointless. Why not just check the value of tegra_sleep_core_finish directly in tegra_init_suspend()?

I will add some code that wraps to different SoC later. We add some common code for LP1 support here, but it didn't support by the chip yet. Does that OK or should I move it to the next patch?

Quoted 16 lines

Quoted 16 lines

arch/arm/mach-tegra/pm.c +16 -0

+static void tegra_suspend_enter_lp1(void)
+{
+	tegra_pmc_suspend();
+
+	/* copy the reset vector & SDRAM shutdown code into IRAM */
+	memcpy(iram_save_addr, iram_code, iram_save_size);
+	memcpy(iram_code, tegra_lp1_iram->start_addr, iram_save_size);
+
+	*((u32 *)tegra_cpu_lp1_mask) = 1;
+}
+
+static void tegra_suspend_exit_lp1(void)
+{
+	tegra_pmc_resume();
+
+	/* restore IRAM */

Quoted 8 lines

arch/arm/mach-tegra/pm.c enum tegra_suspend_mode tegra_pm_validate_suspend_mode( · line 250

Quoted 4 lines

arch/arm/mach-tegra/pm.c +4 -0

+	memcpy(iram_code, iram_save_addr, iram_save_size);
+
+	*(u32 *)tegra_cpu_lp1_mask = 0;
+}

I'm not really sure I like that, but I suppose it's OK. It sure seems like a performance limiter, but I suppose LP1 is so slow it doesn't matter, due to the need to ramp power rails, PLLs, SDRAM controller, etc.

That's why we only back up the code size that exactly same with the LP1 resume code of the SoC.

Quoted 3 lines

It'd be nice to simply reserve more IRAM for the kernel's use. Right now, only 1K is reserved, and presumably the code running on the AVP can't use the rest of that page anyway, or can it?

The LP1 resume code still running on the CPU (The LP0 would resume from AVP).

Quoted 27 lines · Joseph Lo

Quoted 13 lines · Stephen Warren

Quoted 11 lines · Joseph Lo

The LP1 suspending mode on Tegra means CPU rail off, devices and PLLs are clock gated and SDRAM in self-refresh mode. That means the low level LP1 suspending and resuming code couldn't be run on DRAM and the CPU must switch to the always on clock domain (a.k.a. CLK_M 12MHz oscillator). And the system clock (SCLK) would be switched to CLK_S, a 32KHz oscillator. The LP1 low level handling code need to be moved to IRAM area first. And marking the LP1 mask for indicating the Tegra device is in LP1. The CPU power timer needs to be re-calculated based on 32KHz that was originally based on PCLK.

arch/arm/mach-tegra/pm.c +0 -0

Quoted 12 lines

arch/arm/mach-tegra/pm.c line 51

Quoted 8 lines

#ifdef CONFIG_PM_SLEEP
static DEFINE_SPINLOCK(tegra_lp2_lock);

arch/arm/mach-tegra/pm.c +5 -0

+static void __iomem *iram_code = IO_ADDRESS(TEGRA_IRAM_CODE_AREA);
+static u32 iram_save_size;
+static void *iram_save_addr;
+struct tegra_lp1_iram *tegra_lp1_iram;
 void (*tegra_tear_down_cpu)(void);
+void (*tegra_sleep_core_finish)(unsigned long v2p);

I'm not sure all of those are required to be global variables. For example, iram_code is just a constant, so you could easily just use it directly in code.

Quoted 6 lines

All of them does not mean the same thing. The LP1 resume code was built in kernel image and store in RAM.
The tegra_lp1_iram hooks the LP1 resume code for different chips. Before LP1 suspend, the original stuffs that in the area of IRAM would be store in the iram_save_addr (RAM). Then copy the LP1 resume code to iram_code area (IRAM).

Sure, some of those variable may differ based on the SoC at runtime etc.

But at least the value of iram_code never changes, right?

Quoted 28 lines

Quoted 16 lines

Quoted 16 lines

arch/arm/mach-tegra/pm.c +16 -0

+static void tegra_suspend_enter_lp1(void)
+{
+	tegra_pmc_suspend();
+
+	/* copy the reset vector & SDRAM shutdown code into IRAM */
+	memcpy(iram_save_addr, iram_code, iram_save_size);
+	memcpy(iram_code, tegra_lp1_iram->start_addr, iram_save_size);
+
+	*((u32 *)tegra_cpu_lp1_mask) = 1;
+}
+
+static void tegra_suspend_exit_lp1(void)
+{
+	tegra_pmc_resume();
+
+	/* restore IRAM */

Quoted 8 lines

arch/arm/mach-tegra/pm.c enum tegra_suspend_mode tegra_pm_validate_suspend_mode( · line 250

Quoted 4 lines

arch/arm/mach-tegra/pm.c +4 -0

+	memcpy(iram_code, iram_save_addr, iram_save_size);
+
+	*(u32 *)tegra_cpu_lp1_mask = 0;
+}

I'm not really sure I like that, but I suppose it's OK. It sure seems like a performance limiter, but I suppose LP1 is so slow it doesn't matter, due to the need to ramp power rails, PLLs, SDRAM controller, etc.

That's why we only back up the code size that exactly same with the LP1 resume code of the SoC.

Quoted 6 lines

Quoted 3 lines

It'd be nice to simply reserve more IRAM for the kernel's use. Right now, only 1K is reserved, and presumably the code running on the AVP can't use the rest of that page anyway, or can it?

The LP1 resume code still running on the CPU (The LP0 would resume from AVP).

Sure. However, if the AVP never touched the IRAM region where the LP1 resume code is placed, we would only need to copy the LP1 resume code to IRAM once at kernel boot time, rather than every time we enter/leave LP1.

I guess the idea is that once we have an AVP driver, we will force the AVP to suspend first, save the IRAM that it might have been using, do the system suspend/resume, then restore the IRAM. And that changing that sequence so that the AVP never ever touched the LP1 IRAM area would require AVP firmware changes that we can't make?

+/*
+ * tegra30_lp1_reset
+ *
+ * reset vector for LP1 restore; copied into IRAM during suspend.

+ * Brings the system back up to a safe staring point (SDRAM out of
+ * self-refresh, PLLC, PLLM and PLLP reenabled, CPU running on PLLX,
+ * system clock running on the same PLL that it suspended at), and
+ * jumps to tegra_resume to restore virtual addressing.
+ * The physical address of tegra_resume expected to be stored in
+ * PMC_SCRATCH41.
+ *
+ * NOTE: THIS *MUST* BE RELOCATED TO TEGRA_IRAM_CODE_AREA AND MUST BE FIRST.

Quoted 5 lines · Joseph Lo

The LP1 suspend mode will power off the CPU, clock gated the PLLs and put SDRAM to self-refresh mode. Any interrupt can wake up device from LP1. The sequence when LP1 suspending:

arch/arm/mach-tegra/pm-tegra30.c +0 -0

arch/arm/mach-tegra/pm-tegra30.c line 28

Quoted 4 lines

arch/arm/mach-tegra/pm-tegra30.c +4 -0

+void tegra30_lp1_iram_hook(void)
+{
+	tegra30_lp1_iram.start_addr = &tegra30_iram_start;
+	tegra30_lp1_iram.end_addr = &tegra30_iram_end;

If you need to fill in the values in that struct dynamically anyway, why not make tegra_lp1_iram be a struct rather than a pointer, and write directly to it?

Quoted 4 lines

+/*
+ * tegra30_lp1_reset
+ *
+ * reset vector for LP1 restore; copied into IRAM during suspend.

Quoted 8 lines

+ * Brings the system back up to a safe staring point (SDRAM out of
+ * self-refresh, PLLC, PLLM and PLLP reenabled, CPU running on PLLX,
+ * system clock running on the same PLL that it suspended at), and
+ * jumps to tegra_resume to restore virtual addressing.
+ * The physical address of tegra_resume expected to be stored in
+ * PMC_SCRATCH41.
+ *
+ * NOTE: THIS *MUST* BE RELOCATED TO TEGRA_IRAM_CODE_AREA AND MUST BE FIRST.

What does "AND MUST BE FIRST" mean?

Quoted 4 lines

arch/arm/mach-tegra/pm-tegra30.c +4 -0

+ENTRY(tegra30_lp1_reset)
+	/*
+	 * The CPU and system bus are running at 32KHz and executing from
+	 * IRAM when this code is executed; immediately switch to CLKM and

arch/arm/mach-tegra/pm-tegra30.c

Quoted 5 lines

arch/arm/mach-tegra/pm-tegra30.c +5 -0

+	 * enable PLLP, PLLM, PLLC, PLLA and PLLX.
+	 */
+	mov32	r0, TEGRA_CLK_RESET_BASE
+
+	mov	r1, #(1 << 28)

Some #defines for the various magic values used in this code would be useful.

arch/arm/mach-tegra/pm-tegra30.c

Quoted 10 lines

arch/arm/mach-tegra/pm-tegra30.c +9 -0

+tegra30_sdram_pad_save:
+	.word	0
+	.word	0
+	.word	0
+	.word	0
+	.word	0
+	.word	0
+	.word	0
+	.word	0

This might be simpler, and easier to validate it's the right length:

.rept 8
.long 0
.endr

arch/arm/mach-tegra/pm-tegra30.c

Quoted 12 lines

arch/arm/mach-tegra/pm-tegra30.c +12 -0

+tegra30_sdram_pad_address:
+	.word	TEGRA_EMC_BASE + EMC_CFG				@0x0
+	.word	TEGRA_EMC_BASE + EMC_ZCAL_INTERVAL			@0x4
+	.word	TEGRA_EMC_BASE + EMC_AUTO_CAL_INTERVAL			@0x8
+	.word	TEGRA_EMC_BASE + EMC_XM2VTTGENPADCTRL			@0xc
+	.word	TEGRA_EMC_BASE + EMC_XM2VTTGENPADCTRL2			@0x10
+	.word	TEGRA_PMC_BASE + PMC_IO_DPD_STATUS			@0x14
+	.word	TEGRA_CLK_RESET_BASE + CLK_RESET_CLK_SOURCE_MSELECT	@0x18
+	.word	TEGRA_CLK_RESET_BASE + CLK_RESET_SCLK_BURST		@0x1c
+
+tegra30_sdram_pad_size:
+	.word	tegra30_sdram_pad_address - tegra30_sdram_pad_save

Perhaps if you swapp the order of declaring tegra30_sdram_pad_save and tegra30_sdram_pad_address, you can even do something like:

	.rept (tegra30_sdram_pad_addr_end - tegra30_sdram_pad_addr) / 4

?

arch/arm/mach-tegra/pm-tegra30.c

Quoted 5 lines

arch/arm/mach-tegra/pm-tegra30.c +4 -0

+	/* 2uS delay delay between changing SCLK and CCLK */
+	wait_for_us r1, r7, r9
+	add	r1, r1, #2
+	wait_until r1, r7, r9

Ah, I see how wait_for_us is used now. Perhaps rename it wait_for_us_boundary or wait_for_us_tick? Alternatively, perhaps wait_until can just incorporate that logic itself?

The LP1 suspend mode will power off the CPU, clock gated the PLLs and put SDRAM to self-refresh mode. Any interrupt can wake up device from LP1. The sequence when LP1 suspending:

arch/arm/mach-tegra/pm.c +0 -0

index 5ae7ee5..36cadaa 100644

arch/arm/mach-tegra/sleep-tegra30.S +0 -0

@@ -96,9 +100,15 @@
 	orreq	\rd, \rd, #(1 << 30)
 	streq	\rd, [\r_car_base, #\pll_base]

Quoted 3 lines · Joseph Lo

The LP1 suspend mode will power off the CPU, clock gated the PLLs and put SDRAM to self-refresh mode. Any interrupt can wake up device from LP1. The sequence when LP1 suspending:

arch/arm/mach-tegra/pm.c static bool tegra_lp1_iram_hook(void) · line 218

Quoted 10 lines

arch/arm/mach-tegra/pm.c +1 -0

index 5ae7ee5..36cadaa 100644
@@ -214,6 +214,7 @@ static bool tegra_lp1_iram_hook(void)
 		tegra20_lp1_iram_hook();
 		break;
 	case TEGRA30:
+	case TEGRA114:
 		tegra30_lp1_iram_hook();

That's in pm-tegra30.c, which is only compiled in if CONFIG_ARCH_TEGRA_3x_SOC is enabled. Similarly, this patch adds a lot of code to sleep-tegra30.S, which also isn't built/linked if !CONFIG_ARCH_TEGRA_3x_SOC.

Does this series built with all 7 combinations of Tegra20/30/114 support enabled?

Quoted 12 lines

arch/arm/mach-tegra/sleep-tegra30.S +5 -0

@@ -96,9 +100,15 @@
 	orreq	\rd, \rd, #(1 << 30)
 	streq	\rd, [\r_car_base, #\pll_base]
 	/* Enable lock detector */
+	.if	\pll_misc
+	ldr	\rd, [\r_car_base, #\pll_misc]
+	bic	\rd, \rd, #(1 << 18)
+	str	\rd, [\r_car_base, #\pll_misc]
+	ldr	\rd, [\r_car_base, #\pll_misc]

arch/arm/mach-tegra/sleep-tegra30.S line 111

Quoted 4 lines

arch/arm/mach-tegra/sleep-tegra30.S +1 -0

 	ldr	\rd, [\r_car_base, #\pll_misc]
 	orr	\rd, \rd, #(1 << 18)
 	str	\rd, [\r_car_base, #\pll_misc]
+	.endif

Hmm. Those last 3 lines that aren't touched by this patch already touched the pll_misc register even if !pll_misc. Was that a bug in a previous patch?

Is git bisect maintained for both compile and run-time across this whole patch series?

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

This series adds the support of LP1 suspend mode for Tegra.

Verified on Seaboard, Cardhu and Dalmore.

And tested with THUMB2_KERNEL as well.

P.S.
A known issue on Dalmore, the LP1 resume may take 10 ~ 15 seconds due to the 792MHz BCT. It can't be reproduced on Seaboard or Cardhu. And the root cause of this is about the default settings of EMC registers that cause the DRAM can't leave self-refresh mode immediately. If you want to test with quick LP1 resume on Dalmore, I can provide another HACK for this.

Joseph Lo (8):
ARM: tegra: add common resume handling code for LP1 resuming
ARM: tegra: config the polarity of the request of sys clock
clk: tegra114: add LP1 suspend/resume support
ARM: tegra: add common LP1 suspend support
ARM: tegra30: add LP1 suspend support
ARM: tegra20: add LP1 suspend support
ARM: tegra114: add LP1 suspend support
ARM: dts: tegra: enable LP1 suspend mode

 arch/arm/boot/dts/tegra114-dalmore.dts     |   2 +-
 arch/arm/boot/dts/tegra20-colibri-512.dtsi |   2 +-
 arch/arm/boot/dts/tegra20-harmony.dts      |   2 +-
 arch/arm/boot/dts/tegra20-paz00.dts        |   2 +-
 arch/arm/boot/dts/tegra20-seaboard.dts     |   2 +-
 arch/arm/boot/dts/tegra20-tamonten.dtsi    |   2 +-
 arch/arm/boot/dts/tegra20-trimslice.dts    |   2 +-
 arch/arm/boot/dts/tegra20-ventana.dts      |   2 +-
 arch/arm/boot/dts/tegra20-whistler.dts     |   2 +-
 arch/arm/boot/dts/tegra30-beaver.dts       |   2 +-
 arch/arm/boot/dts/tegra30-cardhu.dtsi      |   2 +-
 arch/arm/mach-tegra/Makefile               |   3 +
 arch/arm/mach-tegra/iomap.h                |   6 +
 arch/arm/mach-tegra/pm-tegra20.c           |  37 ++
 arch/arm/mach-tegra/pm-tegra30.c           |  37 ++
 arch/arm/mach-tegra/pm.c                   | 121 +++++-
 arch/arm/mach-tegra/pm.h                   |  23 ++
 arch/arm/mach-tegra/pmc.c                  |  36 +-
 arch/arm/mach-tegra/pmc.h                  |   3 +
 arch/arm/mach-tegra/reset-handler.S        |  13 +
 arch/arm/mach-tegra/reset.c                |   2 +
 arch/arm/mach-tegra/reset.h                |   4 +
 arch/arm/mach-tegra/sleep-tegra20.S        | 300 ++++++++++++++
 arch/arm/mach-tegra/sleep-tegra30.S        | 615
+++++++++++++++++++++++++++++ arch/arm/mach-tegra/sleep.S                | 
 8 +-
 arch/arm/mach-tegra/sleep.h                |  18 +
 drivers/clk/tegra/clk-tegra114.c           |  32 ++
 27 files changed, 1259 insertions(+), 21 deletions(-)
 create mode 100644 arch/arm/mach-tegra/pm-tegra20.c
 create mode 100644 arch/arm/mach-tegra/pm-tegra30.c

Quoted 3 lines

This series adds the support of LP1 suspend mode for Tegra.

Verified on Seaboard, Cardhu and Dalmore.

tested on AC100 and worked fine, except

[   68.448007] PM: Syncing filesystems ... done.
[   71.370086] Freezing user space processes ... (elapsed 0.001 seconds) done.
[   71.370088] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) 
done.
[   71.371253] Suspending console(s) (use no_console_suspend to debug)
[   71.638481] PM: suspend of devices complete after 266.604 msecs
[   71.638905] PM: late suspend of devices complete after 0.417 msecs
[   71.639477] PM: noirq suspend of devices complete after 0.568 msecs
[   71.639480] Disabling non-boot CPUs ...
[   71.640242] CPU1: shutdown
[   71.641216] Entering suspend state LP1
[   71.641267] Enabling non-boot CPUs ...
[   71.643125] CPU1: Booted secondary processor
[   71.643449] CPU1 is up
[   71.643786] PM: noirq resume of devices complete after 0.331 msecs
[   71.644300] PM: early resume of devices complete after 0.364 msecs
[   71.648038] ------------[ cut here ]------------
[   71.648061] WARNING: CPU: 0 PID: 1025 at kernel/irq/manage.c:529 
irq_set_irq_wake+0xb8/0xf0()
[   71.648064] Unbalanced IRQ 384 wake disable
[   71.648068] Modules linked in:
[   71.648075] CPU: 0 PID: 1025 Comm: bash Not tainted 3.11.0-rc2-
next-20130726-00008-gef01509-dirty #6
[   71.648107] [<c001551c>] (unwind_backtrace+0x0/0xf8) from [<c001154c>] 
(show_stack+0x10/0x14)
[   71.648128] [<c001154c>] (show_stack+0x10/0x14) from [<c0515288>] 
(dump_stack+0x80/0xc4)
[   71.648142] [<c0515288>] (dump_stack+0x80/0xc4) from [<c0025340>] 
(warn_slowpath_common+0x64/0x88)
[   71.648151] [<c0025340>] (warn_slowpath_common+0x64/0x88) from [<c00253f8>] 
(warn_slowpath_fmt+0x30/0x40)
[   71.648161] [<c00253f8>] (warn_slowpath_fmt+0x30/0x40) from [<c007ff8c>] 
(irq_set_irq_wake+0xb8/0xf0)
[   71.648178] [<c007ff8c>] (irq_set_irq_wake+0xb8/0xf0) from [<c0334d00>] 
(tps6586x_rtc_resume+0x30/0x38)
[   71.648192] [<c0334d00>] (tps6586x_rtc_resume+0x30/0x38) from [<c02951d0>] 
(platform_pm_resume+0x2c/0x4c)
[   71.648202] [<c02951d0>] (platform_pm_resume+0x2c/0x4c) from [<c0299d40>] 
(dpm_run_callback.isra.5+0x18/0x38)
[   71.648211] [<c0299d40>] (dpm_run_callback.isra.5+0x18/0x38) from 
[<c029a044>] (device_resume+0xb0/0x170)
[   71.648219] [<c029a044>] (device_resume+0xb0/0x170) from [<c029aeb4>] 
(dpm_resume+0xe8/0x20c)
[   71.648227] [<c029aeb4>] (dpm_resume+0xe8/0x20c) from [<c029b174>] 
(dpm_resume_end+0xc/0x18)
[   71.648245] [<c029b174>] (dpm_resume_end+0xc/0x18) from [<c005e480>] 
(suspend_devices_and_enter+0xdc/0x2f8)
[   71.648256] [<c005e480>] (suspend_devices_and_enter+0xdc/0x2f8) from 
[<c005e810>] (pm_suspend+0x174/0x1e8)
[   71.648265] [<c005e810>] (pm_suspend+0x174/0x1e8) from [<c005d710>] 
(state_store+0x6c/0xbc)
[   71.648285] [<c005d710>] (state_store+0x6c/0xbc) from [<c01eca78>] 
(kobj_attr_store+0x14/0x20)
[   71.648298] [<c01eca78>] (kobj_attr_store+0x14/0x20) from [<c011e970>] 
(sysfs_write_file+0x168/0x198)
[   71.648312] [<c011e970>] (sysfs_write_file+0x168/0x198) from [<c00cd060>] 
(vfs_write+0xb0/0x188)
[   71.648321] [<c00cd060>] (vfs_write+0xb0/0x188) from [<c00cd410>] 
(SyS_write+0x3c/0x70)
[   71.648330] [<c00cd410>] (SyS_write+0x3c/0x70) from [<c000e560>] 
(ret_fast_syscall+0x0/0x30)
[   71.648334] ---[ end trace e5ed8212ac4db5da ]---
[   72.063578] mmc1: power class selection to bus width 8 failed
[   72.297753] usb 2-1: reset high-speed USB device number 2 using tegra-ehci
[   72.567736] usb 3-1: reset high-speed USB device number 2 using tegra-ehci
[   73.022058] usb 3-1.3: reset high-speed USB device number 4 using tegra-
ehci
[   73.216064] usb 3-1.2: reset high-speed USB device number 3 using tegra-
ehci
[   73.360225] PM: resume of devices complete after 1715.937 msecs
[   73.361008] Restarting tasks ... done.

and sdcard being offline after resume (fixed by CONFIG_MMC_UNSAFE_RESUME=y). Anyway, thanks for your great work!

Tested-by Marc Dietrich <[email protected]>

Quoted 43 lines

And tested with THUMB2_KERNEL as well.

P.S.
A known issue on Dalmore, the LP1 resume may take 10 ~ 15 seconds due to the 792MHz BCT. It can't be reproduced on Seaboard or Cardhu. And the root cause of this is about the default settings of EMC registers that cause the DRAM can't leave self-refresh mode immediately. If you want to test with quick LP1 resume on Dalmore, I can provide another HACK for this.

Joseph Lo (8):
ARM: tegra: add common resume handling code for LP1 resuming
ARM: tegra: config the polarity of the request of sys clock
clk: tegra114: add LP1 suspend/resume support
ARM: tegra: add common LP1 suspend support
ARM: tegra30: add LP1 suspend support
ARM: tegra20: add LP1 suspend support
ARM: tegra114: add LP1 suspend support
ARM: dts: tegra: enable LP1 suspend mode

 arch/arm/boot/dts/tegra114-dalmore.dts     |   2 +-
 arch/arm/boot/dts/tegra20-colibri-512.dtsi |   2 +-
 arch/arm/boot/dts/tegra20-harmony.dts      |   2 +-
 arch/arm/boot/dts/tegra20-paz00.dts        |   2 +-
 arch/arm/boot/dts/tegra20-seaboard.dts     |   2 +-
 arch/arm/boot/dts/tegra20-tamonten.dtsi    |   2 +-
 arch/arm/boot/dts/tegra20-trimslice.dts    |   2 +-
 arch/arm/boot/dts/tegra20-ventana.dts      |   2 +-
 arch/arm/boot/dts/tegra20-whistler.dts     |   2 +-
 arch/arm/boot/dts/tegra30-beaver.dts       |   2 +-
 arch/arm/boot/dts/tegra30-cardhu.dtsi      |   2 +-
 arch/arm/mach-tegra/Makefile               |   3 +
 arch/arm/mach-tegra/iomap.h                |   6 +
 arch/arm/mach-tegra/pm-tegra20.c           |  37 ++
 arch/arm/mach-tegra/pm-tegra30.c           |  37 ++
 arch/arm/mach-tegra/pm.c                   | 121 +++++-
 arch/arm/mach-tegra/pm.h                   |  23 ++
 arch/arm/mach-tegra/pmc.c                  |  36 +-
 arch/arm/mach-tegra/pmc.h                  |   3 +
 arch/arm/mach-tegra/reset-handler.S        |  13 +
 arch/arm/mach-tegra/reset.c                |   2 +
 arch/arm/mach-tegra/reset.h                |   4 +
 arch/arm/mach-tegra/sleep-tegra20.S        | 300 ++++++++++++++

Quoted 8 lines

 arch/arm/mach-tegra/sleep-tegra30.S        | 615
+++++++++++++++++++++++++++++ arch/arm/mach-tegra/sleep.S                | 
 8 +-
 arch/arm/mach-tegra/sleep.h                |  18 +
 drivers/clk/tegra/clk-tegra114.c           |  32 ++
 27 files changed, 1259 insertions(+), 21 deletions(-)
 create mode 100644 arch/arm/mach-tegra/pm-tegra20.c
 create mode 100644 arch/arm/mach-tegra/pm-tegra30.c

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

27.07.2013 20:12, Marc Dietrich ?????:

Quoted 81 lines · On Friday 26 July 2013 17:15:02 Joseph Lo wrote:

Quoted 3 lines

This series adds the support of LP1 suspend mode for Tegra.

Verified on Seaboard, Cardhu and Dalmore.

tested on AC100 and worked fine, except

[   68.448007] PM: Syncing filesystems ... done.
[   71.370086] Freezing user space processes ... (elapsed 0.001 seconds)
done. [   71.370088] Freezing remaining freezable tasks ... (elapsed
0.001 seconds) done.
[   71.371253] Suspending console(s) (use no_console_suspend to debug)
[   71.638481] PM: suspend of devices complete after 266.604 msecs
[   71.638905] PM: late suspend of devices complete after 0.417 msecs
[   71.639477] PM: noirq suspend of devices complete after 0.568 msecs
[   71.639480] Disabling non-boot CPUs ...
[   71.640242] CPU1: shutdown
[   71.641216] Entering suspend state LP1
[   71.641267] Enabling non-boot CPUs ...
[   71.643125] CPU1: Booted secondary processor
[   71.643449] CPU1 is up
[   71.643786] PM: noirq resume of devices complete after 0.331 msecs
[   71.644300] PM: early resume of devices complete after 0.364 msecs
[   71.648038] ------------[ cut here ]------------
[   71.648061] WARNING: CPU: 0 PID: 1025 at kernel/irq/manage.c:529
irq_set_irq_wake+0xb8/0xf0()
[   71.648064] Unbalanced IRQ 384 wake disable
[   71.648068] Modules linked in:
[   71.648075] CPU: 0 PID: 1025 Comm: bash Not tainted 3.11.0-rc2-
next-20130726-00008-gef01509-dirty #6
[   71.648107] [<c001551c>] (unwind_backtrace+0x0/0xf8) from [<c001154c>]
(show_stack+0x10/0x14)
[   71.648128] [<c001154c>] (show_stack+0x10/0x14) from [<c0515288>]
(dump_stack+0x80/0xc4)
[   71.648142] [<c0515288>] (dump_stack+0x80/0xc4) from [<c0025340>]
(warn_slowpath_common+0x64/0x88)
[   71.648151] [<c0025340>] (warn_slowpath_common+0x64/0x88) from
[<c00253f8>] (warn_slowpath_fmt+0x30/0x40)
[   71.648161] [<c00253f8>] (warn_slowpath_fmt+0x30/0x40) from
[<c007ff8c>]
(irq_set_irq_wake+0xb8/0xf0)
[   71.648178] [<c007ff8c>] (irq_set_irq_wake+0xb8/0xf0) from [<c0334d00>]
(tps6586x_rtc_resume+0x30/0x38)
[   71.648192] [<c0334d00>] (tps6586x_rtc_resume+0x30/0x38) from
[<c02951d0>] (platform_pm_resume+0x2c/0x4c)
[   71.648202] [<c02951d0>] (platform_pm_resume+0x2c/0x4c) from
[<c0299d40>] (dpm_run_callback.isra.5+0x18/0x38)
[   71.648211] [<c0299d40>] (dpm_run_callback.isra.5+0x18/0x38) from
[<c029a044>] (device_resume+0xb0/0x170)
[   71.648219] [<c029a044>] (device_resume+0xb0/0x170) from [<c029aeb4>]
(dpm_resume+0xe8/0x20c)
[   71.648227] [<c029aeb4>] (dpm_resume+0xe8/0x20c) from [<c029b174>]
(dpm_resume_end+0xc/0x18)
[   71.648245] [<c029b174>] (dpm_resume_end+0xc/0x18) from [<c005e480>]
(suspend_devices_and_enter+0xdc/0x2f8)
[   71.648256] [<c005e480>] (suspend_devices_and_enter+0xdc/0x2f8) from
[<c005e810>] (pm_suspend+0x174/0x1e8)
[   71.648265] [<c005e810>] (pm_suspend+0x174/0x1e8) from [<c005d710>]
(state_store+0x6c/0xbc)
[   71.648285] [<c005d710>] (state_store+0x6c/0xbc) from [<c01eca78>]
(kobj_attr_store+0x14/0x20)
[   71.648298] [<c01eca78>] (kobj_attr_store+0x14/0x20) from [<c011e970>]
(sysfs_write_file+0x168/0x198)
[   71.648312] [<c011e970>] (sysfs_write_file+0x168/0x198) from
[<c00cd060>] (vfs_write+0xb0/0x188)
[   71.648321] [<c00cd060>] (vfs_write+0xb0/0x188) from [<c00cd410>]
(SyS_write+0x3c/0x70)
[   71.648330] [<c00cd410>] (SyS_write+0x3c/0x70) from [<c000e560>]
(ret_fast_syscall+0x0/0x30)
[   71.648334] ---[ end trace e5ed8212ac4db5da ]---
[   72.063578] mmc1: power class selection to bus width 8 failed
[   72.297753] usb 2-1: reset high-speed USB device number 2 using
tegra-ehci [   72.567736] usb 3-1: reset high-speed USB device number 2
using tegra-ehci [   73.022058] usb 3-1.3: reset high-speed USB device
number 4 using tegra- ehci
[   73.216064] usb 3-1.2: reset high-speed USB device number 3 using
tegra-
ehci
[   73.360225] PM: resume of devices complete after 1715.937 msecs
[   73.361008] Restarting tasks ... done.

Quoted 4 lines

and sdcard being offline after resume (fixed by CONFIG_MMC_UNSAFE_RESUME=y). Anyway, thanks for your great work!

Tested-by Marc Dietrich <[email protected]>

I had same warn on 3.8.
https://bitbucket.org/digetx/picasso-kernel/commits/a86dc95ab48dd77eaa046384
4cdf88a5acf9d842

Quoted 82 lines

27.07.2013 20:12, Marc Dietrich ?????:

Quoted 81 lines · On Friday 26 July 2013 17:15:02 Joseph Lo wrote:

Quoted 3 lines

This series adds the support of LP1 suspend mode for Tegra.

Verified on Seaboard, Cardhu and Dalmore.

tested on AC100 and worked fine, except

[   68.448007] PM: Syncing filesystems ... done.
[   71.370086] Freezing user space processes ... (elapsed 0.001 seconds)
done. [   71.370088] Freezing remaining freezable tasks ... (elapsed
0.001 seconds) done.
[   71.371253] Suspending console(s) (use no_console_suspend to debug)
[   71.638481] PM: suspend of devices complete after 266.604 msecs
[   71.638905] PM: late suspend of devices complete after 0.417 msecs
[   71.639477] PM: noirq suspend of devices complete after 0.568 msecs
[   71.639480] Disabling non-boot CPUs ...
[   71.640242] CPU1: shutdown
[   71.641216] Entering suspend state LP1
[   71.641267] Enabling non-boot CPUs ...
[   71.643125] CPU1: Booted secondary processor
[   71.643449] CPU1 is up
[   71.643786] PM: noirq resume of devices complete after 0.331 msecs
[   71.644300] PM: early resume of devices complete after 0.364 msecs
[   71.648038] ------------[ cut here ]------------
[   71.648061] WARNING: CPU: 0 PID: 1025 at kernel/irq/manage.c:529
irq_set_irq_wake+0xb8/0xf0()
[   71.648064] Unbalanced IRQ 384 wake disable
[   71.648068] Modules linked in:
[   71.648075] CPU: 0 PID: 1025 Comm: bash Not tainted 3.11.0-rc2-
next-20130726-00008-gef01509-dirty #6
[   71.648107] [<c001551c>] (unwind_backtrace+0x0/0xf8) from [<c001154c>]
(show_stack+0x10/0x14)
[   71.648128] [<c001154c>] (show_stack+0x10/0x14) from [<c0515288>]
(dump_stack+0x80/0xc4)
[   71.648142] [<c0515288>] (dump_stack+0x80/0xc4) from [<c0025340>]
(warn_slowpath_common+0x64/0x88)
[   71.648151] [<c0025340>] (warn_slowpath_common+0x64/0x88) from
[<c00253f8>] (warn_slowpath_fmt+0x30/0x40)
[   71.648161] [<c00253f8>] (warn_slowpath_fmt+0x30/0x40) from
[<c007ff8c>]
(irq_set_irq_wake+0xb8/0xf0)
[   71.648178] [<c007ff8c>] (irq_set_irq_wake+0xb8/0xf0) from [<c0334d00>]
(tps6586x_rtc_resume+0x30/0x38)
[   71.648192] [<c0334d00>] (tps6586x_rtc_resume+0x30/0x38) from
[<c02951d0>] (platform_pm_resume+0x2c/0x4c)
[   71.648202] [<c02951d0>] (platform_pm_resume+0x2c/0x4c) from
[<c0299d40>] (dpm_run_callback.isra.5+0x18/0x38)
[   71.648211] [<c0299d40>] (dpm_run_callback.isra.5+0x18/0x38) from
[<c029a044>] (device_resume+0xb0/0x170)
[   71.648219] [<c029a044>] (device_resume+0xb0/0x170) from [<c029aeb4>]
(dpm_resume+0xe8/0x20c)
[   71.648227] [<c029aeb4>] (dpm_resume+0xe8/0x20c) from [<c029b174>]
(dpm_resume_end+0xc/0x18)
[   71.648245] [<c029b174>] (dpm_resume_end+0xc/0x18) from [<c005e480>]
(suspend_devices_and_enter+0xdc/0x2f8)
[   71.648256] [<c005e480>] (suspend_devices_and_enter+0xdc/0x2f8) from
[<c005e810>] (pm_suspend+0x174/0x1e8)
[   71.648265] [<c005e810>] (pm_suspend+0x174/0x1e8) from [<c005d710>]
(state_store+0x6c/0xbc)
[   71.648285] [<c005d710>] (state_store+0x6c/0xbc) from [<c01eca78>]
(kobj_attr_store+0x14/0x20)
[   71.648298] [<c01eca78>] (kobj_attr_store+0x14/0x20) from [<c011e970>]
(sysfs_write_file+0x168/0x198)
[   71.648312] [<c011e970>] (sysfs_write_file+0x168/0x198) from
[<c00cd060>] (vfs_write+0xb0/0x188)
[   71.648321] [<c00cd060>] (vfs_write+0xb0/0x188) from [<c00cd410>]
(SyS_write+0x3c/0x70)
[   71.648330] [<c00cd410>] (SyS_write+0x3c/0x70) from [<c000e560>]
(ret_fast_syscall+0x0/0x30)
[   71.648334] ---[ end trace e5ed8212ac4db5da ]---
[   72.063578] mmc1: power class selection to bus width 8 failed
[   72.297753] usb 2-1: reset high-speed USB device number 2 using
tegra-ehci [   72.567736] usb 3-1: reset high-speed USB device number 2
using tegra-ehci [   73.022058] usb 3-1.3: reset high-speed USB device
number 4 using tegra- ehci
[   73.216064] usb 3-1.2: reset high-speed USB device number 3 using
tegra-
ehci
[   73.360225] PM: resume of devices complete after 1715.937 msecs
[   73.361008] Restarting tasks ... done.

Quoted 8 lines

Quoted 4 lines

and sdcard being offline after resume (fixed by CONFIG_MMC_UNSAFE_RESUME=y). Anyway, thanks for your great work!

Tested-by Marc Dietrich <[email protected]>

I had same warn on 3.8.
https://bitbucket.org/digetx/picasso-kernel/commits/a86dc95ab48dd77eaa046384
4cdf88a5acf9d842

Stephen Warren said on IRC that this is known and on the internal bug tracker already. I just posted it so others are not surprised. Well, I should have added that it's a know problem. Sorry for that.

Marc

It might be a known problem only for those who faced with it. Since it's on their internal irc, it wasn't a known issue.

It also would be interesting to know if suspend with thumb2 works correctly. On 3.8 I've ported lp0 suspend and it works fine with arm compiled kernel, thumb2 also works but only for about 5-10 hours (resuming on each 5-30 minutes, default android behavior) and after I'm getting pmd corruption. I have no idea from where that corruption comes from without ability for hw debugging.

27.07.2013 22:09, Marc Dietrich ?????:

Quoted 33 lines · On Saturday 27 July 2013 20:20:38 Dmitry Osipenko wrote:

Quoted 31 lines

27.07.2013 20:12, Marc Dietrich ?????:

Quoted 30 lines · On Friday 26 July 2013 17:15:02 Joseph Lo wrote:

Quoted 3 lines

This series adds the support of LP1 suspend mode for Tegra.

Verified on Seaboard, Cardhu and Dalmore.

tested on AC100 and worked fine, except

[   68.448007] PM: Syncing filesystems ... done.
[   71.370086] Freezing user space processes ... (elapsed 0.001 seconds)
done. [   71.370088] Freezing remaining freezable tasks ... (elapsed
0.001 seconds) done.
[   71.371253] Suspending console(s) (use no_console_suspend to debug)
[   71.638481] PM: suspend of devices complete after 266.604 msecs
[   71.638905] PM: late suspend of devices complete after 0.417 msecs
[   71.639477] PM: noirq suspend of devices complete after 0.568 msecs
[   71.639480] Disabling non-boot CPUs ...
[   71.640242] CPU1: shutdown
[   71.641216] Entering suspend state LP1
[   71.641267] Enabling non-boot CPUs ...
[   71.643125] CPU1: Booted secondary processor
[   71.643449] CPU1 is up
[   71.643786] PM: noirq resume of devices complete after 0.331 msecs
[   71.644300] PM: early resume of devices complete after 0.364 msecs
[   71.648038] ------------[ cut here ]------------
[   71.648061] WARNING: CPU: 0 PID: 1025 at kernel/irq/manage.c:529
irq_set_irq_wake+0xb8/0xf0()
[   71.648064] Unbalanced IRQ 384 wake disable
[   71.648068] Modules linked in:
[   71.648075] CPU: 0 PID: 1025 Comm: bash Not tainted 3.11.0-rc2-

next-20130726-00008-gef01509-dirty #6

Quoted 3 lines

Stephen Warren said on IRC that this is known and on the internal bug tracker already. I just posted it so others are not surprised. Well, I should have added that it's a know problem. Sorry for that.

It might be a known problem only for those who faced with it. Since it's on their internal irc, it wasn't a known issue.

It also would be interesting to know if suspend with thumb2 works correctly. On 3.8 I've ported lp0 suspend and it works fine with arm compiled kernel, thumb2 also works but only for about 5-10 hours (resuming on each 5-30 minutes, default android behavior) and after I'm getting pmd corruption. I have no idea from where that corruption comes from without ability for hw debugging.

Quoted 41 lines

27.07.2013 22:09, Marc Dietrich ?????:

Quoted 33 lines · On Saturday 27 July 2013 20:20:38 Dmitry Osipenko wrote:

Quoted 31 lines

27.07.2013 20:12, Marc Dietrich ?????:

Quoted 30 lines · On Friday 26 July 2013 17:15:02 Joseph Lo wrote:

Quoted 3 lines

This series adds the support of LP1 suspend mode for Tegra.

Verified on Seaboard, Cardhu and Dalmore.

tested on AC100 and worked fine, except

[   68.448007] PM: Syncing filesystems ... done.
[   71.370086] Freezing user space processes ... (elapsed 0.001 seconds)
done. [   71.370088] Freezing remaining freezable tasks ... (elapsed
0.001 seconds) done.
[   71.371253] Suspending console(s) (use no_console_suspend to debug)
[   71.638481] PM: suspend of devices complete after 266.604 msecs
[   71.638905] PM: late suspend of devices complete after 0.417 msecs
[   71.639477] PM: noirq suspend of devices complete after 0.568 msecs
[   71.639480] Disabling non-boot CPUs ...
[   71.640242] CPU1: shutdown
[   71.641216] Entering suspend state LP1
[   71.641267] Enabling non-boot CPUs ...
[   71.643125] CPU1: Booted secondary processor
[   71.643449] CPU1 is up
[   71.643786] PM: noirq resume of devices complete after 0.331 msecs
[   71.644300] PM: early resume of devices complete after 0.364 msecs
[   71.648038] ------------[ cut here ]------------
[   71.648061] WARNING: CPU: 0 PID: 1025 at kernel/irq/manage.c:529
irq_set_irq_wake+0xb8/0xf0()
[   71.648064] Unbalanced IRQ 384 wake disable
[   71.648068] Modules linked in:
[   71.648075] CPU: 0 PID: 1025 Comm: bash Not tainted 3.11.0-rc2-

next-20130726-00008-gef01509-dirty #6

Quoted 3 lines

Stephen Warren said on IRC that this is known and on the internal bug tracker already. I just posted it so others are not surprised. Well, I should have added that it's a know problem. Sorry for that.

It might be a known problem only for those who faced with it. Since it's on their internal irc, it wasn't a known issue.

Quoted 6 lines

It also would be interesting to know if suspend with thumb2 works correctly. On 3.8 I've ported lp0 suspend and it works fine with arm compiled kernel, thumb2 also works but only for about 5-10 hours (resuming on each 5-30 minutes, default android behavior) and after I'm getting pmd corruption. I have no idea from where that corruption comes from without ability for hw debugging.

I don't think anyone can comment on this because lp0 support for mainline is not published yet. On the other hand, if you have patches for which you want some review, feel free to post them here.

Marc

Quoted 3 lines · On Friday 26 July 2013 17:15:02 Joseph Lo wrote:

This series adds the support of LP1 suspend mode for Tegra.

Verified on Seaboard, Cardhu and Dalmore.

tested on AC100 and worked fine, except

and sdcard being offline after resume (fixed by CONFIG_MMC_UNSAFE_RESUME=y). Anyway, thanks for your great work!

Tested-by Marc Dietrich <[email protected]>