linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: andrew@lunn.ch (Andrew Lunn)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v7 3/8] cpufreq: kirkwood: Remove use of the clk provider API
Date: Fri, 22 Aug 2014 22:11:12 +0200	[thread overview]
Message-ID: <20140822201112.GH17277@lunn.ch> (raw)
In-Reply-To: <20140822192933.5251.53733@quantum>

> Thanks for reviewing. I think my patch is equivalent to the old way of
> doing things, with one exception that I will address later below.
> 
> struct cpufreq_frequency_table kirkwood_freq_table has clock rates
> initialized to zero, so there is no regression compared to my unsigned
> long cpu_frequency variable used for tracking the clock rate. Both
> implementations start with unknown rates in hardware and initialize a
> variable to zero until that rate can be discovered later on in
> kirkwood_cpufreq_probe().
> 
> kirkwood_cpufreq_get_cpu_frequency() returns the frequency based on the
> state of the clock. As best I can tell, this clock is only touched by
> this cpufreq driver and nowhere else

Not quite true. u-boot might of touch the clock. Weird things happen
with some kirkwood boards. Some don't have the ability to control
there power supplies. So some boards implement "power off" by
rebooting, and letting u-boot spin until a button is pressed. I hope
such a u-boot powers off as much as possible, and e.g. drops the CPU
clock to the lower frequency. One would also hope it puts it back to
high speed before calling the kernel.

There is also the somewhat unlikely case that somebody uses kexec to
go from one kernel to another. You have no idea what state the
previous kernel left the clock in.

> , so the driver knows the state of
> the clock implicitly and doesn't need to read any hardware registers to
> see if it is enabled or not. Every time we enable or disable the clock
> we can know the cpu frequency.
> 
> > 
> > However, if you don't cause an actual state change, the WFI never
> > returns. If this assumption is wrong, your box is dead the first time
> > it tries to change cpu frequency.
> 
> So if a state change in hardware never occurs, the cpu will not wake up?

Correct. I had that situation a few times while developing this
driver. And it is not obvious what has happened, since it does not
happen immediately, but when the governor decides the CPU load passes
a threshold and the frequency can be changed. I had it stop dead while
i assume it executed some sleep in an /etc/init.d script.

> That sounds like a bad situation but I do not understand how it relates
> to the changes I made in the driver. Could you explain how tracking
> cpu_frequency in the driver would result in the cpu not waking up from
> wfi?

It was clearer in earlier versions of the driver, but code has been
refactored into the cpufreq core. The core should call
kirkwood_cpufreq_get_cpu_frequency() in order to get the current
frequency, and only perform a change if the requested frequency is
different. In the current code, kirkwood_cpufreq_get_cpu_frequency()
reads from the hardware what the current frequency is. So we are
guaranteed to only call kirkwood_cpufreq_target() when there is a real
change.

[snip]

> Can the driver's
> view of the clock status be out of sync with the actual hardware?

Yes, at startup, as explained above, we have no idea what the current
state of the hardware is. Once we do know the real state, we can track
it within the driver, but we need to get that initial state correct,
or we WFI forever on the first frequency change.

   Andrew

  reply	other threads:[~2014-08-22 20:11 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-08-18 15:30 [PATCH v7 0/8] Per-user clock constraints Tomeu Vizoso
2014-08-18 15:30 ` [PATCH v7 1/8] clk: Add temporary mapping to the existing API Tomeu Vizoso
2014-08-20 14:50   ` Mike Turquette
2014-08-21 18:04     ` Tony Lindgren
2014-08-21 18:10       ` Jason Cooper
2014-08-22  3:49         ` Simon Horman
2014-08-25  9:18         ` Sebastian Hesselbarth
2014-08-18 15:30 ` [PATCH v7 2/8] clk: provide public clk_is_enabled function Tomeu Vizoso
2014-08-18 15:30 ` [PATCH v7 3/8] cpufreq: kirkwood: Remove use of the clk provider API Tomeu Vizoso
     [not found]   ` <20140820225513.5251.284@quantum>
2014-08-21  7:53     ` Tomeu Vizoso
2014-08-21 13:38       ` Andrew Lunn
2014-08-22 19:29         ` Mike Turquette
2014-08-22 20:11           ` Andrew Lunn [this message]
2014-08-22 20:27             ` Andrew Lunn
2014-08-26 21:46             ` Mike Turquette
2014-08-26 22:36               ` Andrew Lunn
2014-08-26 23:30                 ` Mike Turquette
2014-08-27  0:35                   ` Andrew Lunn
2014-08-27  5:04                     ` Mike Turquette
2014-08-27 15:58                   ` Jason Gunthorpe
2014-08-18 15:30 ` [PATCH v7 4/8] ASoC: mxs-saif: fix mixed use of public and provider clk API Tomeu Vizoso
2014-08-18 15:30 ` [PATCH v7 6/8] clk: use struct clk only for external API Tomeu Vizoso
2014-08-18 15:30 ` [PATCH v7 7/8] clk: per-user clock accounting for debug Tomeu Vizoso
2014-08-27 20:54   ` Mike Turquette
2014-08-18 15:30 ` [PATCH v7 8/8] clk: Add floor and ceiling constraints to clock rates Tomeu Vizoso
2014-08-21  2:12 ` [PATCH v7 0/8] Per-user clock constraints Andrew Lunn
2014-08-21  7:10   ` Tomeu Vizoso
2014-08-26 13:20 ` Heiko Stübner

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=20140822201112.GH17277@lunn.ch \
    --to=andrew@lunn.ch \
    --cc=linux-arm-kernel@lists.infradead.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: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).