From: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> To: Joseph Lo <josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> Cc: "linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" <linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>, Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> Subject: Re: [PATCH 3/8] clk: tegra114: add LP1 suspend/resume support Date: Mon, 05 Aug 2013 11:00:16 -0600 [thread overview] Message-ID: <51FFDA20.6050403@wwwdotorg.org> (raw) In-Reply-To: <1375689749.1731.36.camel-yx3yKKdKkHfc7b1ADBJPm0n48jw8i0AO@public.gmane.org> On 08/05/2013 02:02 AM, Joseph Lo wrote: > On Sat, 2013-08-03 at 04:32 +0800, Stephen Warren wrote: >> On 08/02/2013 02:09 AM, Joseph Lo wrote: >>> On Tue, 2013-07-30 at 06:51 +0800, Stephen Warren wrote: >>>> On 07/26/2013 03:15 AM, Joseph Lo wrote: >>>>> 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. >> >> 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? > > 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". >>>> 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? > > 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?
WARNING: multiple messages have this Message-ID (diff)
From: swarren@wwwdotorg.org (Stephen Warren) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 3/8] clk: tegra114: add LP1 suspend/resume support Date: Mon, 05 Aug 2013 11:00:16 -0600 [thread overview] Message-ID: <51FFDA20.6050403@wwwdotorg.org> (raw) In-Reply-To: <1375689749.1731.36.camel@jlo-ubuntu-64.nvidia.com> On 08/05/2013 02:02 AM, Joseph Lo wrote: > On Sat, 2013-08-03 at 04:32 +0800, Stephen Warren wrote: >> On 08/02/2013 02:09 AM, Joseph Lo wrote: >>> On Tue, 2013-07-30 at 06:51 +0800, Stephen Warren wrote: >>>> On 07/26/2013 03:15 AM, Joseph Lo wrote: >>>>> 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. >> >> 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? > > 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". >>>> 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? > > 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?
next prev parent reply other threads:[~2013-08-05 17:00 UTC|newest] Thread overview: 88+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-07-26 9:15 [PATCH 0/8] ARM: tegra: support LP1 suspend mode Joseph Lo 2013-07-26 9:15 ` Joseph Lo [not found] ` <1374830110-30685-1-git-send-email-josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2013-07-26 9:15 ` [PATCH 1/8] ARM: tegra: add common resume handling code for LP1 resuming Joseph Lo 2013-07-26 9:15 ` Joseph Lo [not found] ` <1374830110-30685-2-git-send-email-josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2013-07-29 22:38 ` Stephen Warren 2013-07-29 22:38 ` Stephen Warren 2013-07-26 9:15 ` [PATCH 2/8] ARM: tegra: config the polarity of the request of sys clock Joseph Lo 2013-07-26 9:15 ` Joseph Lo [not found] ` <1374830110-30685-3-git-send-email-josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2013-07-29 22:47 ` Stephen Warren 2013-07-29 22:47 ` Stephen Warren [not found] ` <51F6F109.8010102-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> 2013-08-02 7:48 ` Joseph Lo 2013-08-02 7:48 ` Joseph Lo [not found] ` <1375429739.6761.31.camel-yx3yKKdKkHfc7b1ADBJPm0n48jw8i0AO@public.gmane.org> 2013-08-02 20:24 ` Stephen Warren 2013-08-02 20:24 ` Stephen Warren [not found] ` <51FC1579.50100-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> 2013-08-05 8:42 ` Joseph Lo 2013-08-05 8:42 ` Joseph Lo 2013-07-26 9:15 ` [PATCH 3/8] clk: tegra114: add LP1 suspend/resume support Joseph Lo 2013-07-26 9:15 ` Joseph Lo [not found] ` <1374830110-30685-4-git-send-email-josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2013-07-29 22:51 ` Stephen Warren 2013-07-29 22:51 ` Stephen Warren [not found] ` <51F6F209.2090309-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> 2013-08-02 8:09 ` Joseph Lo 2013-08-02 8:09 ` Joseph Lo [not found] ` <1375430985.6761.51.camel-yx3yKKdKkHfc7b1ADBJPm0n48jw8i0AO@public.gmane.org> 2013-08-02 20:32 ` Stephen Warren 2013-08-02 20:32 ` Stephen Warren [not found] ` <51FC1751.9010401-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> 2013-08-05 8:02 ` Joseph Lo 2013-08-05 8:02 ` Joseph Lo [not found] ` <1375689749.1731.36.camel-yx3yKKdKkHfc7b1ADBJPm0n48jw8i0AO@public.gmane.org> 2013-08-05 17:00 ` Stephen Warren [this message] 2013-08-05 17:00 ` Stephen Warren [not found] ` <51FFDA20.6050403-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> 2013-08-05 17:39 ` Stephen Warren 2013-08-05 17:39 ` Stephen Warren [not found] ` <51FFE363.4080603-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> 2013-08-06 9:10 ` Joseph Lo 2013-08-06 9:10 ` Joseph Lo [not found] ` <1375780249.2261.63.camel-yx3yKKdKkHfc7b1ADBJPm0n48jw8i0AO@public.gmane.org> 2013-08-06 18:37 ` Stephen Warren 2013-08-06 18:37 ` Stephen Warren [not found] ` <52014258.9070602-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> 2013-08-07 9:12 ` Joseph Lo 2013-08-07 9:12 ` Joseph Lo [not found] ` <1375866749.8111.57.camel-yx3yKKdKkHfc7b1ADBJPm0n48jw8i0AO@public.gmane.org> 2013-08-07 16:46 ` Stephen Warren 2013-08-07 16:46 ` Stephen Warren [not found] ` <520279FC.1000006-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> 2013-08-08 2:23 ` Joseph Lo 2013-08-08 2:23 ` Joseph Lo [not found] ` <1375928591.1758.66.camel-yx3yKKdKkHfc7b1ADBJPm0n48jw8i0AO@public.gmane.org> 2013-08-08 19:54 ` Stephen Warren 2013-08-08 19:54 ` Stephen Warren [not found] ` <5203F766.9050100-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> 2013-08-09 9:23 ` Joseph Lo 2013-08-09 9:23 ` Joseph Lo 2013-08-06 9:19 ` Joseph Lo 2013-08-06 9:19 ` Joseph Lo 2013-07-26 9:15 ` [PATCH 4/8] ARM: tegra: add common LP1 suspend support Joseph Lo 2013-07-26 9:15 ` Joseph Lo [not found] ` <1374830110-30685-5-git-send-email-josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2013-07-29 23:13 ` Stephen Warren 2013-07-29 23:13 ` Stephen Warren [not found] ` <51F6F725.6000106-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> 2013-08-02 9:27 ` Joseph Lo 2013-08-02 9:27 ` Joseph Lo [not found] ` <1375435620.6761.120.camel-yx3yKKdKkHfc7b1ADBJPm0n48jw8i0AO@public.gmane.org> 2013-08-02 20:40 ` Stephen Warren 2013-08-02 20:40 ` Stephen Warren 2013-08-05 8:07 ` Joseph Lo 2013-08-05 8:07 ` Joseph Lo 2013-07-26 9:15 ` [PATCH 5/8] ARM: tegra30: add " Joseph Lo 2013-07-26 9:15 ` Joseph Lo [not found] ` <1374830110-30685-6-git-send-email-josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2013-07-29 23:45 ` Stephen Warren 2013-07-29 23:45 ` Stephen Warren [not found] ` <51F6FE89.4060402-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> 2013-08-05 6:46 ` Joseph Lo 2013-08-05 6:46 ` Joseph Lo 2013-07-26 9:15 ` [PATCH 6/8] ARM: tegra20: " Joseph Lo 2013-07-26 9:15 ` Joseph Lo 2013-07-26 9:15 ` [PATCH 7/8] ARM: tegra114: " Joseph Lo 2013-07-26 9:15 ` Joseph Lo [not found] ` <1374830110-30685-8-git-send-email-josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2013-07-29 23:53 ` Stephen Warren 2013-07-29 23:53 ` Stephen Warren [not found] ` <51F7007F.6090200-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> 2013-08-05 6:51 ` Joseph Lo 2013-08-05 6:51 ` Joseph Lo 2013-07-26 9:15 ` [PATCH 8/8] ARM: dts: tegra: enable LP1 suspend mode Joseph Lo 2013-07-26 9:15 ` Joseph Lo 2013-07-27 16:12 ` [PATCH 0/8] ARM: tegra: support " Marc Dietrich 2013-07-27 16:12 ` Marc Dietrich 2013-07-27 16:20 ` Dmitry Osipenko 2013-07-27 16:20 ` Dmitry Osipenko [not found] ` <51F3F356.8080709-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2013-07-27 18:09 ` Marc Dietrich 2013-07-27 18:09 ` Marc Dietrich 2013-07-27 18:26 ` Dmitry Osipenko 2013-07-27 18:26 ` Dmitry Osipenko [not found] ` <51F410BE.3000904-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2013-07-27 18:29 ` Dmitry Osipenko 2013-07-27 18:29 ` Dmitry Osipenko 2013-07-27 19:03 ` Marc Dietrich 2013-07-27 19:03 ` Marc Dietrich 2013-07-27 19:11 ` Dmitry Osipenko 2013-07-27 19:11 ` Dmitry Osipenko 2013-07-30 9:49 ` Joseph Lo 2013-07-30 9:49 ` Joseph Lo
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=51FFDA20.6050403@wwwdotorg.org \ --to=swarren-3lzwwm7+weoh9zmkesr00q@public.gmane.org \ --cc=josephl-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org \ --cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \ --cc=linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.