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

[Vincent Guittot]

On 4 July 2014 10:02, Maxime Ripard <maxime.ripard at free-electrons.com> wrote:
> On Thu, Jul 03, 2014 at 09:51:27AM +0200, Vincent Guittot wrote:
>> On 3 July 2014 09:10, Maxime Ripard <maxime.ripard at free-electrons.com> wrote:
>> > On Mon, Jun 30, 2014 at 05:06:16PM +0200, Vincent Guittot wrote:
>> >> On 30 June 2014 16:58, Maxime Ripard <maxime.ripard at free-electrons.com> wrote:
>> >> > On Mon, Jun 30, 2014 at 04:48:35PM +0200, Vincent Guittot wrote:
>> >> >> On 30 June 2014 16:01, Maxime Ripard <maxime.ripard at 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 :)

so Why not changing the log message ? It's clearly not an error to not
have clock-frequency

>
>> 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