diff mbox series

[v1,2/2] cpu: clk: riscv: populate proper CPU core clk frequency

Message ID 1582042404-27356-3-git-send-email-sagar.kadam@sifive.com
State New
Headers show
Series display proper CPU frequency on hifive-unleashed | expand

Commit Message

Sagar Shrikant Kadam Feb. 18, 2020, 4:13 p.m. UTC
Fetch core clock frequency from prci if clock-frequency for CPU nodes
is missing in device tree, so that the cmd "#cpu detail" will show the
correct CPU frequency.

U-Boot command "#cpu detail" is showing wrong frequency values.
This patch fixes this issue by getting the core clock set in prci driver
if clock-frequency is not added to CPU nodes in device tree.
It is tested on HiFive Unleashed A00 board.

Signed-off-by: Sagar Shrikant Kadam <sagar.kadam at sifive.com>
Tested-by: Vincent Chen <vincent.chen at sifive.com>
---
 drivers/cpu/riscv_cpu.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

Comments

Bin Meng Feb. 19, 2020, 4:09 p.m. UTC | #1
+Sean Anderson

Hi Sagar,

On Wed, Feb 19, 2020 at 12:13 AM Sagar Shrikant Kadam
<sagar.kadam at sifive.com> wrote:
>
> Fetch core clock frequency from prci if clock-frequency for CPU nodes
> is missing in device tree, so that the cmd "#cpu detail" will show the
> correct CPU frequency.
>
> U-Boot command "#cpu detail" is showing wrong frequency values.
> This patch fixes this issue by getting the core clock set in prci driver
> if clock-frequency is not added to CPU nodes in device tree.
> It is tested on HiFive Unleashed A00 board.
>
> Signed-off-by: Sagar Shrikant Kadam <sagar.kadam at sifive.com>
> Tested-by: Vincent Chen <vincent.chen at sifive.com>
> ---
>  drivers/cpu/riscv_cpu.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c
> index 28ad0aa..eb5491f 100644
> --- a/drivers/cpu/riscv_cpu.c
> +++ b/drivers/cpu/riscv_cpu.c
> @@ -9,6 +9,8 @@
>  #include <errno.h>
>  #include <dm/device-internal.h>
>  #include <dm/lists.h>
> +#include <clk-uclass.h>
> +#include <dt-bindings/clock/sifive-fu540-prci.h>

It's wrong to include a SoC specific header file in a generic driver.


>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -25,11 +27,46 @@ static int riscv_cpu_get_desc(struct udevice *dev, char *buf, int size)
>         return 0;
>  }
>
> +static ulong riscv_get_clkrate(int clk_index)
> +{
> +       int ret;
> +       struct udevice *dev;
> +       struct clk clk;
> +       ulong rate;
> +
> +       ret = uclass_get_device_by_driver(UCLASS_CLK,
> +                                         DM_GET_DRIVER(sifive_fu540_prci),
> +                                         &dev);
> +       if (ret < 0) {
> +               pr_err("%s: Could not get device driver\n", __func__);
> +               return ret;
> +       }
> +
> +       clk.id = clk_index;
> +       ret = clk_request(dev, &clk);
> +       if (ret < 0) {
> +               pr_err("%s: request to clock device failed...\n", __func__);
> +               return ret;
> +       }
> +
> +       rate = clk_get_rate(&clk);
> +
> +       clk_free(&clk);
> +
> +       return rate;
> +}
> +
>  static int riscv_cpu_get_info(struct udevice *dev, struct cpu_info *info)
>  {
>         const char *mmu;
> +       int ret;
>
> -       dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq);
> +       ret = dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq);
> +       if (ret) {
> +               /* if clock-frequency is missing in DT, read it from prci */
> +               debug("Fetch core clk configured by prci\n");
> +               info->cpu_freq = riscv_get_clkrate(PRCI_CLK_COREPLL);
> +       }
>
>         mmu = dev_read_string(dev, "mmu-type");
>         if (!mmu)
> --

What you were trying to do in this patch, I believe the following 2
patches already did it.

http://patchwork.ozlabs.org/patch/1236177/
http://patchwork.ozlabs.org/patch/1236180/

Regards,
Bin
Sagar Shrikant Kadam Feb. 21, 2020, 5:59 a.m. UTC | #2
Hello Bin,

> -----Original Message-----
> From: Bin Meng <bmeng.cn at gmail.com>
> Sent: Wednesday, February 19, 2020 9:40 PM
> To: Sagar Kadam <sagar.kadam at sifive.com>; Sean Anderson
> <seanga2 at gmail.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Lukasz Majewski
> <lukma at denx.de>; Anup Patel <Anup.Patel at wdc.com>; Paul Walmsley (
> Sifive) <paul.walmsley at sifive.com>; Vincent Chen
> <vincent.chen at sifive.com>
> Subject: Re: [PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk
> frequency
> 
> +Sean Anderson
> 
> Hi Sagar,
> 
> On Wed, Feb 19, 2020 at 12:13 AM Sagar Shrikant Kadam
> <sagar.kadam at sifive.com> wrote:
> >
> > Fetch core clock frequency from prci if clock-frequency for CPU nodes
> > is missing in device tree, so that the cmd "#cpu detail" will show the
> > correct CPU frequency.
> >
> > U-Boot command "#cpu detail" is showing wrong frequency values.
> > This patch fixes this issue by getting the core clock set in prci driver
> > if clock-frequency is not added to CPU nodes in device tree.
> > It is tested on HiFive Unleashed A00 board.
> >
> > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam at sifive.com>
> > Tested-by: Vincent Chen <vincent.chen at sifive.com>
> > ---
> >  drivers/cpu/riscv_cpu.c | 39
> ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c
> > index 28ad0aa..eb5491f 100644
> > --- a/drivers/cpu/riscv_cpu.c
> > +++ b/drivers/cpu/riscv_cpu.c
> > @@ -9,6 +9,8 @@
> >  #include <errno.h>
> >  #include <dm/device-internal.h>
> >  #include <dm/lists.h>
> > +#include <clk-uclass.h>
> > +#include <dt-bindings/clock/sifive-fu540-prci.h>
> 
> It's wrong to include a SoC specific header file in a generic driver.
>
Thanks for review.
Ok. I will remove this.
> 
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -25,11 +27,46 @@ static int riscv_cpu_get_desc(struct udevice *dev,
> char *buf, int size)
> >         return 0;
> >  }
> >
> > +static ulong riscv_get_clkrate(int clk_index)
> > +{
> > +       int ret;
> > +       struct udevice *dev;
> > +       struct clk clk;
> > +       ulong rate;
> > +
> > +       ret = uclass_get_device_by_driver(UCLASS_CLK,
> > +                                         DM_GET_DRIVER(sifive_fu540_prci),
> > +                                         &dev);
> > +       if (ret < 0) {
> > +               pr_err("%s: Could not get device driver\n", __func__);
> > +               return ret;
> > +       }
> > +
> > +       clk.id = clk_index;
> > +       ret = clk_request(dev, &clk);
> > +       if (ret < 0) {
> > +               pr_err("%s: request to clock device failed...\n", __func__);
> > +               return ret;
> > +       }
> > +
> > +       rate = clk_get_rate(&clk);
> > +
> > +       clk_free(&clk);
> > +
> > +       return rate;
> > +}
> > +
> >  static int riscv_cpu_get_info(struct udevice *dev, struct cpu_info *info)
> >  {
> >         const char *mmu;
> > +       int ret;
> >
> > -       dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq);
> > +       ret = dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq);
> > +       if (ret) {
> > +               /* if clock-frequency is missing in DT, read it from prci */
> > +               debug("Fetch core clk configured by prci\n");
> > +               info->cpu_freq = riscv_get_clkrate(PRCI_CLK_COREPLL);
> > +       }
> >
> >         mmu = dev_read_string(dev, "mmu-type");
> >         if (!mmu)
> > --
> 
> What you were trying to do in this patch, I believe the following 2
> patches already did it.
> 
> http://patchwork.ozlabs.org/patch/1236177/
> http://patchwork.ozlabs.org/patch/1236180/
> 

Thanks for sharing the links. Unfortunately I didn?t come across it.
I applied these two patches within my repo  (assuming there are not
extra dependencies) and would like to share my observation:
The implementation in the patch links shared here read's clock dt property
in clk_get_by_index. If the cpu dt node doesn't contain clock property it return's 
negative value and so the clk_get_rate here won't be be executed.

+	ret = clk_get_by_index(dev, 0, &clk);
+	if (!ret) {
+		ret = clk_get_rate(&clk);

Thus when I tested this on hifive unleashed the "cpu detail" showed frequency as 0 Hz.

Please correct me if I am wrong, but IMHO here we can check for negative return 
code [if (ret < 0)] instead of (!ret) and if "clocks" is missing in dt property then get the clock 
driver using uclass_get_device_by_driver->request the clock -> and get clock rate, just like below

-       if (!ret) {
+       if (ret < 0) {
+               ret = uclass_get_device_by_driver(UCLASS_CLK,
+                                                 DM_GET_DRIVER(sifive_fu540_prci),
+                                                 &dev);
+               clk.id = 0;
+               ret = clk_request(dev, &clk);
+               if (ret < 0) {
+                       pr_err("%s: request to clock device failed...\n", __func__);
+                       return ret;
+               }
+

Also there is missing "include linux/err.h" which is needed by IS_ERR_VALUE
Please let me know if I can rebase and update my patches above the two patch's you
pointed to.

> Regards,
> Bin
Sean Anderson Feb. 21, 2020, 6:17 a.m. UTC | #3
On 2/21/20 12:59 AM, Sagar Kadam wrote:
>> What you were trying to do in this patch, I believe the following 2
>> patches already did it.
>>
>> http://patchwork.ozlabs.org/patch/1236177/
>> http://patchwork.ozlabs.org/patch/1236180/
>>
> 
> Thanks for sharing the links. Unfortunately I didn?t come across it.
> I applied these two patches within my repo  (assuming there are not
> extra dependencies) and would like to share my observation:
> The implementation in the patch links shared here read's clock dt property
> in clk_get_by_index. If the cpu dt node doesn't contain clock property it return's 
> negative value and so the clk_get_rate here won't be be executed.
> 
> +	ret = clk_get_by_index(dev, 0, &clk);
> +	if (!ret) {
> +		ret = clk_get_rate(&clk);

This is working as designed. The idea is to use the clocks property if
it exists and to fall back on clock-frequency otherwise.

> Thus when I tested this on hifive unleashed the "cpu detail" showed frequency as 0 Hz.

This is because the cpu nodes in the hifive/fu540 device tree have
neither a clock-frequency property or a clocks property.

> Please correct me if I am wrong, but IMHO here we can check for negative return 
> code [if (ret < 0)] instead of (!ret) and if "clocks" is missing in dt property then get the clock 
> driver using uclass_get_device_by_driver->request the clock -> and get clock rate, just like below
> 
> -       if (!ret) {
> +       if (ret < 0) {
> +               ret = uclass_get_device_by_driver(UCLASS_CLK,
> +                                                 DM_GET_DRIVER(sifive_fu540_prci),
> +                                                 &dev);

This is again adding board-specific code to a generic driver. Add a
UCLASS_CLOCK driver if you want to support clocks. That way there is no
need for code like this.

> +               clk.id = 0;
> +               ret = clk_request(dev, &clk);
> +               if (ret < 0) {
> +                       pr_err("%s: request to clock device failed...\n", __func__);

I belive pr_err is supported for linux compatibility. New code should
use log_*. This is also probably debug-level and not err-level.

> +                       return ret;
> +               }
> +
> 
> Also there is missing "include linux/err.h" which is needed by IS_ERR_VALUE

Yes, I noticed that when rebasing. It will be fixed in the next version
of the series.

> Please let me know if I can rebase and update my patches above the two patch's you
> pointed to.
> 
>> Regards,
>> Bin

--Sean
Sagar Shrikant Kadam Feb. 24, 2020, 11:31 a.m. UTC | #4
Hello Sean,

> -----Original Message-----
> From: Sean Anderson <seanga2 at gmail.com>
> Sent: Friday, February 21, 2020 11:48 AM
> To: Sagar Kadam <sagar.kadam at sifive.com>; Bin Meng
> <bmeng.cn at gmail.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Lukasz Majewski
> <lukma at denx.de>; Anup Patel <Anup.Patel at wdc.com>; Paul Walmsley (
> Sifive) <paul.walmsley at sifive.com>; Vincent Chen
> <vincent.chen at sifive.com>
> Subject: Re: [PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk
> frequency
> 
> On 2/21/20 12:59 AM, Sagar Kadam wrote:
> >> What you were trying to do in this patch, I believe the following 2
> >> patches already did it.
> >>
> >> http://patchwork.ozlabs.org/patch/1236177/
> >> http://patchwork.ozlabs.org/patch/1236180/
> >>
> >
> > Thanks for sharing the links. Unfortunately I didn?t come across it.
> > I applied these two patches within my repo  (assuming there are not
> > extra dependencies) and would like to share my observation:
> > The implementation in the patch links shared here read's clock dt
> > property in clk_get_by_index. If the cpu dt node doesn't contain clock
> > property it return's negative value and so the clk_get_rate here won't be be
> executed.
> >
> > +	ret = clk_get_by_index(dev, 0, &clk);
> > +	if (!ret) {
> > +		ret = clk_get_rate(&clk);
> 
> This is working as designed. The idea is to use the clocks property if it exists
> and to fall back on clock-frequency otherwise.

Thanks for clarifying. 
> 
> > Thus when I tested this on hifive unleashed the "cpu detail" showed
> frequency as 0 Hz.
> 
> This is because the cpu nodes in the hifive/fu540 device tree have neither a
> clock-frequency property or a clocks property.
> 
Yes, I will add clocks dt property.
> > Please correct me if I am wrong, but IMHO here we can check for
> > negative return code [if (ret < 0)] instead of (!ret) and if "clocks"
> > is missing in dt property then get the clock driver using
> > uclass_get_device_by_driver->request the clock -> and get clock rate,
> > just like below
> >
> > -       if (!ret) {
> > +       if (ret < 0) {
> > +               ret = uclass_get_device_by_driver(UCLASS_CLK,
> > +                                                 DM_GET_DRIVER(sifive_fu540_prci),
> > +                                                 &dev);
> 
> This is again adding board-specific code to a generic driver. Add a
> UCLASS_CLOCK driver if you want to support clocks. That way there is no
> need for code like this.
Thanks for suggestion.
I will remove board-specific code from generic driver.
> 
> > +               clk.id = 0;
> > +               ret = clk_request(dev, &clk);
> > +               if (ret < 0) {
> > +                       pr_err("%s: request to clock device
> > + failed...\n", __func__);
> 
> I belive pr_err is supported for linux compatibility. New code should use
> log_*. This is also probably debug-level and not err-level.
Ok. I will replace pr_err with log_debug.
> 
> > +                       return ret;
> > +               }
> > +
> >
> > Also there is missing "include linux/err.h" which is needed by
> > IS_ERR_VALUE
> 
> Yes, I noticed that when rebasing. It will be fixed in the next version of the
> series.
Thanks for updating.

BR,
Sagar Kadam

> 
> > Please let me know if I can rebase and update my patches above the two
> > patch's you pointed to.
> >
> >> Regards,
> >> Bin
> 
> --Sean
diff mbox series

Patch

diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c
index 28ad0aa..eb5491f 100644
--- a/drivers/cpu/riscv_cpu.c
+++ b/drivers/cpu/riscv_cpu.c
@@ -9,6 +9,8 @@ 
 #include <errno.h>
 #include <dm/device-internal.h>
 #include <dm/lists.h>
+#include <clk-uclass.h>
+#include <dt-bindings/clock/sifive-fu540-prci.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -25,11 +27,46 @@  static int riscv_cpu_get_desc(struct udevice *dev, char *buf, int size)
 	return 0;
 }
 
+static ulong riscv_get_clkrate(int clk_index)
+{
+	int ret;
+	struct udevice *dev;
+	struct clk clk;
+	ulong rate;
+
+	ret = uclass_get_device_by_driver(UCLASS_CLK,
+					  DM_GET_DRIVER(sifive_fu540_prci),
+					  &dev);
+	if (ret < 0) {
+		pr_err("%s: Could not get device driver\n", __func__);
+		return ret;
+	}
+
+	clk.id = clk_index;
+	ret = clk_request(dev, &clk);
+	if (ret < 0) {
+		pr_err("%s: request to clock device failed...\n", __func__);
+		return ret;
+	}
+
+	rate = clk_get_rate(&clk);
+
+	clk_free(&clk);
+
+	return rate;
+}
+
 static int riscv_cpu_get_info(struct udevice *dev, struct cpu_info *info)
 {
 	const char *mmu;
+	int ret;
 
-	dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq);
+	ret = dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq);
+	if (ret) {
+		/* if clock-frequency is missing in DT, read it from prci */
+		debug("Fetch core clk configured by prci\n");
+		info->cpu_freq = riscv_get_clkrate(PRCI_CLK_COREPLL);
+	}
 
 	mmu = dev_read_string(dev, "mmu-type");
 	if (!mmu)