From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vincent Guittot Subject: Re: [PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency Date: Thu, 3 Jul 2014 09:51:27 +0200 Message-ID: References: <1404123143-13041-1-git-send-email-maxime.ripard@free-electrons.com> <1404123143-13041-2-git-send-email-maxime.ripard@free-electrons.com> <20140630102934.GV32514@n2100.arm.linux.org.uk> <20140630124919.GC28647@lukather> <20140630140146.GD28647@lukather> <20140630145847.GE28647@lukather> <20140703071016.GB31996@lukather> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Return-path: In-Reply-To: <20140703071016.GB31996@lukather> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Maxime Ripard Cc: Russell King - ARM Linux , Arnd Bergmann , LAK , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Rob Herring , Nicolas Pitre List-Id: devicetree@vger.kernel.org On 3 July 2014 09:10, Maxime Ripard wrote: > On Mon, Jun 30, 2014 at 05:06:16PM +0200, Vincent Guittot wrote: >> On 30 June 2014 16:58, Maxime Ripard wrote: >> > On Mon, Jun 30, 2014 at 04:48:35PM +0200, Vincent Guittot wrote: >> >> On 30 June 2014 16:01, Maxime Ripard 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. Could you give us more details about why you nee to use current clock ? AFAICT, your patch only makes sense for HMP system which don't have DVFS, is it your case ? Vincent > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: vincent.guittot@linaro.org (Vincent Guittot) Date: Thu, 3 Jul 2014 09:51:27 +0200 Subject: [PATCH v2 1/2] ARM: topology: Use a clock if possible to get the CPU frequency In-Reply-To: <20140703071016.GB31996@lukather> References: <1404123143-13041-1-git-send-email-maxime.ripard@free-electrons.com> <1404123143-13041-2-git-send-email-maxime.ripard@free-electrons.com> <20140630102934.GV32514@n2100.arm.linux.org.uk> <20140630124919.GC28647@lukather> <20140630140146.GD28647@lukather> <20140630145847.GE28647@lukather> <20140703071016.GB31996@lukather> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 3 July 2014 09:10, Maxime Ripard wrote: > On Mon, Jun 30, 2014 at 05:06:16PM +0200, Vincent Guittot wrote: >> On 30 June 2014 16:58, Maxime Ripard wrote: >> > On Mon, Jun 30, 2014 at 04:48:35PM +0200, Vincent Guittot wrote: >> >> On 30 June 2014 16:01, Maxime Ripard 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. Could you give us more details about why you nee to use current clock ? AFAICT, your patch only makes sense for HMP system which don't have DVFS, is it your case ? Vincent > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com