Re: [PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3420 bytes --]

On Thu, Jul 03, 2014 at 09:51:27AM +0200, Vincent Guittot wrote:
> On 3 July 2014 09:10, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > On Mon, Jun 30, 2014 at 05:06:16PM +0200, Vincent Guittot wrote:
> >> On 30 June 2014 16:58, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >> > On Mon, Jun 30, 2014 at 04:48:35PM +0200, Vincent Guittot wrote:
> >> >> On 30 June 2014 16:01, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> >> >> > On Mon, Jun 30, 2014 at 03:27:21PM +0200, Vincent Guittot wrote:
> >> >> >> >> >> -             rate = of_get_property(cn, "clock-frequency", &len);
> >> >> >> >> >> -             if (!rate || len != 4) {
> >> >> >> >> >> -                     pr_err("%s missing clock-frequency property\n",
> >> >> >> >> >> -                             cn->full_name);
> >> >> >> >> >> +             clk = of_clk_get(cn, 0);
> >> >> >> >> >> +             if (!IS_ERR(clk))
> >> >> >> >> >> +                     rate = clk_get_rate(clk);
> >> >> >> >>
> >> >> >> >> We need the max frequency as it will be used to weight the different
> >> >> >> >> CPUs capacity. How do you ensure that the current clock rate is the
> >> >> >> >> max one ?
> >> >> >> >
> >> >> >> > Hmm, the clock-frequency attribute in the ePAPR is defined at the
> >> >> >> > current CPU frequency, not the max one.
> >> >> >>
> >> >> >> What means current frequency in device tree when DVFS is involved ?
> >> >> >
> >> >> > The ePAPR states that clock-frequency is supposed to be "the current
> >> >> > clock speed of the CPU in Hertz". It's exactly what my patch add.
> >> >> >
> >> >> > Now, you're right, DVFS would be an issue here with clock-frequency,
> >> >> > but this patch actually makes it easier to deal with, since you only
> >> >> > get a reference to a clock, and you can get its rate at any given
> >> >> > time.
> >> >>
> >> >> and what about using clk_round_rate(clk, ULONG_MAX) ?
> >> >
> >> > Well, the clock itself might generate a frequency higher that what the
> >> > CPU can run at, so I'm not sure it's a valid assumption.
> >>
> >> yes, you're right
> >>
> >> >
> >> >> We will not be dependent of when we parse DT
> >> >
> >> > You lost me there. clk_round_rate and clk_get_rate are available at
> >> > the same time in the boot process, aren't they?
> >>
> >> yes, but  clk_round_rate(clk, ULONG_MAX) will return the max frequency
> >> and not the current one. But as you mentioned, it doesn't ensure that
> >> it's a possible frequency for the core
> >
> > Is this an Acked-by ?
> 
>  I still think that using the current rate with clk_get_rate is not a
> good solution.

Well, it's just as good as using clock-frequency, really. If you want
the CPU max frequency, it's not the right property (plus, since the
changed behaviour is not documented in Linux anywhere, the only
reference we have is the ePAPR on this).

> Could you give us more details about why you nee to use current clock ?

Honestly, I just want to remove that big warning at boot on the
A7-based SoCs we have :)

> AFAICT, your patch only makes sense for HMP system which don't have
> DVFS, is it your case ?

Nope, it's just plain simple SMP.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]