Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1308653

Re: [PATCH 9/9] clk: tegra210: Initialize PLL_D2 to a sane rate

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

On Wed, Jan 13, 2016 at 11:27:09AM -0500, Rhyland Klein wrote:
> On 1/13/2016 9:03 AM, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Fri, Jan 08, 2016 at 01:45:14PM -0500, Rhyland Klein wrote:
> >> Initialize PLL_D2 to a sane rate at the start of the day.
> >>
> >> Signed-off-by: Rhyland Klein <rklein@nvidia.com>
> >> ---
> >>  drivers/clk/tegra/clk-tegra210.c | 1 +
> >>  1 file changed, 1 insertion(+)
> > 
> > There are a lot of assumptions in this commit message. I'm asking myself
> > why does it need to be initialized to any rate at all? Isn't it up to
> > the user driver to set the PLL to whatever it knows to be a sane rate?
> > Why is 594 MHz a sane rate?
> > 
> > A good commit message should answer those questions.
> > 
> 
> Thanks. This patch came about in response to some work with
> suspend/resume logic in the ChromeOS kernel. In the suspend/resume patch
> there, it reads all clk rates and caches them in suspend, then re-sets
> them on resume. We hadn't been using pll_d2 at all, and its rate (when
> read in suspend()) came back as 0. Then in resume, we would tr to set
> it, clk_set_rate(pll_d2, 0) and would generate a warning/error that 0
> wasn't a valid rate.
> 
> As for the specific rate I chose, I simply took one off the table of
> supported frequencies in the clk-tegra210 code, so that it was non-zero.
> 
> Since we don't support suspend/resume right now in the clk drivers, I
> don't think its urgent to merge this patch. I can drop in in the next
> rev if thats easier.

Yes, that would be my preference as well. Once suspend/resume support
gets added we should run into this again. Setting the rate so some sane
value (the meaning of which is highly dependent on the use-case) sounds
to me more like a workaround rather than a real fix.

pll_d2 is used for display, so I don't quite understand why we'd have
to restore the rate on resume, since DRM/KMS would set it to whatever
it needs at a higher level anyway. Perhaps this will turn out to be a
non-issue upstream.

Thierry

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/9 REPOST] Tegra CLK Fixes Rhyland Klein <rklein@nvidia.com> - 2016-01-08 19:50 +0100
  [PATCH 4/9] clk: tegra: Fix the misnaming of nvenc from msenc Rhyland Klein <rklein@nvidia.com> - 2016-01-08 19:50 +0100
  [PATCH 3/9] clk: tegra210: Fix naming of MISC registers Rhyland Klein <rklein@nvidia.com> - 2016-01-08 19:50 +0100
  [PATCH 2/9] clk: tegra210: Remove improper flags for lock_enable Rhyland Klein <rklein@nvidia.com> - 2016-01-08 19:50 +0100
  [PATCH 6/9] clk: tegra210: fix pllx dyn step calculation Rhyland Klein <rklein@nvidia.com> - 2016-01-08 19:50 +0100
  [PATCH 7/9] clk: tegra: pll: Do not disable PLLE when under HW control Rhyland Klein <rklein@nvidia.com> - 2016-01-08 19:50 +0100
  [PATCH 9/9] clk: tegra210: Initialize PLL_D2 to a sane rate Rhyland Klein <rklein@nvidia.com> - 2016-01-08 19:50 +0100
    Re: [PATCH 9/9] clk: tegra210: Initialize PLL_D2 to a sane rate Thierry Reding <thierry.reding@gmail.com> - 2016-01-13 15:10 +0100
      Re: [PATCH 9/9] clk: tegra210: Initialize PLL_D2 to a sane rate Rhyland Klein <rklein@nvidia.com> - 2016-01-13 17:30 +0100
        Re: [PATCH 9/9] clk: tegra210: Initialize PLL_D2 to a sane rate Thierry Reding <thierry.reding@gmail.com> - 2016-01-13 18:30 +0100
  [PATCH 1/9] clk: tegra: Fix divider on VI_I2C Rhyland Klein <rklein@nvidia.com> - 2016-01-08 19:50 +0100
  Re: [PATCH 0/9 REPOST] Tegra CLK Fixes Thierry Reding <thierry.reding@gmail.com> - 2016-01-13 15:10 +0100

csiph-web