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

[Sagar Kadam]

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