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