RE: [PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk frequency
Hello Sean,
> -Original Message-
> From: Sean Anderson
> Sent: Friday, February 21, 2020 11:48 AM
> To: Sagar Kadam ; Bin Meng
>
> Cc: U-Boot Mailing List ; Lukasz Majewski
> ; Anup Patel ; Paul Walmsley (
> Sifive) ; Vincent Chen
>
> 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, );
> > + if (!ret) {
> > + ret = clk_get_rate();
>
> 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),
> > + );
>
> 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, );
> > + 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
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, );
> + if (!ret) {
> + ret = clk_get_rate();
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),
> + );
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, );
> + 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
RE: [PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk frequency
Hello Bin,
> -Original Message-
> From: Bin Meng
> Sent: Wednesday, February 19, 2020 9:40 PM
> To: Sagar Kadam ; Sean Anderson
>
> Cc: U-Boot Mailing List ; Lukasz Majewski
> ; Anup Patel ; Paul Walmsley (
> Sifive) ; Vincent Chen
>
> 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
> 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
> > Tested-by: Vincent Chen
> > ---
> > 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
> > #include
> > #include
> > +#include
> > +#include
>
> 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),
> > + );
> > + if (ret < 0) {
> > + pr_err("%s: Could not get device driver\n", __func__);
> > + return ret;
> > + }
> > +
> > + clk.id = clk_index;
> > + ret = clk_request(dev, );
> > + if (ret < 0) {
> > + pr_err("%s: request to clock device failed...\n", __func__);
> > + return ret;
> > + }
> > +
> > + rate = clk_get_rate();
> > +
> > + clk_free();
> > +
> > + 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 *)>cpu_freq);
> > + ret = dev_read_u32(dev, "clock-frequency", (u32 *)>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, );
+ if (!ret) {
+ ret = clk_get_rate();
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),
+ );
+ clk.id = 0;
+ ret = clk_request(dev, );
+ 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
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
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
> Tested-by: Vincent Chen
> ---
> 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
> #include
> #include
> +#include
> +#include
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),
> + );
> + if (ret < 0) {
> + pr_err("%s: Could not get device driver\n", __func__);
> + return ret;
> + }
> +
> + clk.id = clk_index;
> + ret = clk_request(dev, );
> + if (ret < 0) {
> + pr_err("%s: request to clock device failed...\n", __func__);
> + return ret;
> + }
> +
> + rate = clk_get_rate();
> +
> + clk_free();
> +
> + 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 *)>cpu_freq);
> + ret = dev_read_u32(dev, "clock-frequency", (u32 *)>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
[PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk frequency
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
Tested-by: Vincent Chen
---
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
#include
#include
+#include
+#include
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),
+ );
+ if (ret < 0) {
+ pr_err("%s: Could not get device driver\n", __func__);
+ return ret;
+ }
+
+ clk.id = clk_index;
+ ret = clk_request(dev, );
+ if (ret < 0) {
+ pr_err("%s: request to clock device failed...\n", __func__);
+ return ret;
+ }
+
+ rate = clk_get_rate();
+
+ clk_free();
+
+ 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 *)>cpu_freq);
+ ret = dev_read_u32(dev, "clock-frequency", (u32 *)>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)
--
2.7.4
