Re: [PATCH] riscv: Try to get cpu frequency from device tree

[Sean Anderson]

On 2/4/20 9:27 AM, Bin Meng wrote:
> Hi Sean,
> 
> On Tue, Feb 4, 2020 at 10:06 PM Sean Anderson  wrote:
>>
>> On 2/4/20 5:36 AM, Bin Meng wrote:
>>> On Mon, Feb 3, 2020 at 1:40 AM Sean Anderson  wrote:
>>> I believe both 2 patches in this series are needed by "riscv: Add
>>> Sipeed Maix support" series?
>>> If yes, I think you can just put 2 patches into the same series, to
>>> give people a good context next time.
>> Hm, I had split them off due to the following comment
>>
>> On 1/31/20 12:46 AM, Rick Chen wrote:
>>> But this patch seem depend on [PATCH v2 06/11] riscv: Fix incorrect
>>> cpu frequency on RV64.
>>> Can you combine those two patches as one patch-set and also modify err
>>> as ret BTW.
>>
>> I guess the intention was to combine them *in* the patch series.
>>
> 
> I meant  you can include these 2 patches in the "riscv: Add Sipeed
> Maix support" series.

Yes, I can do that in the next revision, sorry for the confusion.

Re: [PATCH] riscv: Try to get cpu frequency from device tree

[Bin Meng]

Hi Sean,

On Tue, Feb 4, 2020 at 10:06 PM Sean Anderson  wrote:
>
> On 2/4/20 5:36 AM, Bin Meng wrote:
> > On Mon, Feb 3, 2020 at 1:40 AM Sean Anderson  wrote:
> > I believe both 2 patches in this series are needed by "riscv: Add
> > Sipeed Maix support" series?
> > If yes, I think you can just put 2 patches into the same series, to
> > give people a good context next time.
> Hm, I had split them off due to the following comment
>
> On 1/31/20 12:46 AM, Rick Chen wrote:
> > But this patch seem depend on [PATCH v2 06/11] riscv: Fix incorrect
> > cpu frequency on RV64.
> > Can you combine those two patches as one patch-set and also modify err
> > as ret BTW.
>
> I guess the intention was to combine them *in* the patch series.
>

I meant  you can include these 2 patches in the "riscv: Add Sipeed
Maix support" series.

Regards,
Bin

Re: [PATCH] riscv: Try to get cpu frequency from device tree

[Sean Anderson]

On 2/4/20 5:36 AM, Bin Meng wrote:
> On Mon, Feb 3, 2020 at 1:40 AM Sean Anderson  wrote:
> I believe both 2 patches in this series are needed by "riscv: Add
> Sipeed Maix support" series?
> If yes, I think you can just put 2 patches into the same series, to
> give people a good context next time.
Hm, I had split them off due to the following comment

On 1/31/20 12:46 AM, Rick Chen wrote:
> But this patch seem depend on [PATCH v2 06/11] riscv: Fix incorrect
> cpu frequency on RV64.
> Can you combine those two patches as one patch-set and also modify err
> as ret BTW.

I guess the intention was to combine them *in* the patch series.

--Sean

Re: [PATCH] riscv: Try to get cpu frequency from device tree

[Rick Chen]

Hi Sean

> From: U-Boot [mailto:u-boot-boun...@lists.denx.de] On Behalf Of Lukas Auer
> Sent: Monday, January 27, 2020 6:26 AM
> To: u-boot@lists.denx.de; sean...@gmail.com
> Subject: Re: [PATCH] riscv: Try to get cpu frequency from device tree
>
> On Sun, 2020-01-26 at 13:20 -0500, Sean Anderson wrote:
> > On 1/26/20 11:34 AM, Lukas Auer wrote:
> > > Hi Sean,
> > > Usually, ret is used as a variable name here. I think it would
> > > actually make the code a bit nicer to read here, because the clock
> > > rate is not read from variable err.
> >
> > Hm, I chose err instead of ret since that variable is never the return
> > value of the function. I can change that for v2 if you'd like.
> >
>
> Makes sense. I think it's fine to keep it as is.

But this patch seem depend on [PATCH v2 06/11] riscv: Fix incorrect
cpu frequency on RV64.
Can you combine those two patches as one patch-set and also modify err
as ret BTW.

Thanks
Rick

>
> > > But that's just nit-picking. The patch looks good otherwise!
> > >
> > > Reviewed-by: Lukas Auer 

Re: [PATCH] riscv: Try to get cpu frequency from device tree

[Lukas Auer]

On Sun, 2020-01-26 at 13:20 -0500, Sean Anderson wrote:
> On 1/26/20 11:34 AM, Lukas Auer wrote:
> > Hi Sean,
> > Usually, ret is used as a variable name here. I think it would actually
> > make the code a bit nicer to read here, because the clock rate is not
> > read from variable err.
> 
> Hm, I chose err instead of ret since that variable is never the return
> value of the function. I can change that for v2 if you'd like.
> 

Makes sense. I think it's fine to keep it as is.

> > But that's just nit-picking. The patch looks good otherwise!
> > 
> > Reviewed-by: Lukas Auer 
> 
> 

Re: [PATCH] riscv: Try to get cpu frequency from device tree

[Sean Anderson]

On 1/26/20 11:34 AM, Lukas Auer wrote:
> Hi Sean,
> Usually, ret is used as a variable name here. I think it would actually
> make the code a bit nicer to read here, because the clock rate is not
> read from variable err.

Hm, I chose err instead of ret since that variable is never the return
value of the function. I can change that for v2 if you'd like.

> But that's just nit-picking. The patch looks good otherwise!
> 
> Reviewed-by: Lukas Auer 

Re: [PATCH] riscv: Try to get cpu frequency from device tree

[Lukas Auer]

Hi Sean,

On Fri, 2020-01-17 at 14:51 -0500, Sean Anderson wrote:
> Instead of always using the "clock-frequency" property to determine cpu
> frequency, try using a clock in "clocks" if it exists.
> 
> Signed-off-by Sean Anderson 
> ---
> This patch depends on ;.
> 
>  drivers/cpu/riscv_cpu.c | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c
> index 1e32bb5678..280c9de376 100644
> --- a/drivers/cpu/riscv_cpu.c
> +++ b/drivers/cpu/riscv_cpu.c
> @@ -3,6 +3,7 @@
>   * Copyright (C) 2018, Bin Meng 
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -27,11 +28,24 @@ static int riscv_cpu_get_desc(struct udevice *dev, char 
> *buf, int size)
>  
>  static int riscv_cpu_get_info(struct udevice *dev, struct cpu_info *info)
>  {
> + int err;
> + struct clk clk;
>   const char *mmu;
>  
>   /* Zero out the frequency, in case sizeof(ulong) != sizeof(u32) */
>   info->cpu_freq = 0;
> - dev_read_u32(dev, "clock-frequency", (u32 *)>cpu_freq);
> +
> + /* First try getting the frequency from the assigned clock */
> + err = clk_get_by_index(dev, 0, );

Usually, ret is used as a variable name here. I think it would actually
make the code a bit nicer to read here, because the clock rate is not
read from variable err.

But that's just nit-picking. The patch looks good otherwise!

Reviewed-by: Lukas Auer 

> + if (!err) {
> + err = clk_get_rate();
> + if (!IS_ERR_VALUE(err))
> + info->cpu_freq = err;
> + clk_free();
> + }
> +
> + if (!info->cpu_freq)
> + dev_read_u32(dev, "clock-frequency", (u32 *)>cpu_freq);
>  
>   mmu = dev_read_string(dev, "mmu-type");
>   if (!mmu)

[PATCH] riscv: Try to get cpu frequency from device tree

[Sean Anderson]

Instead of always using the "clock-frequency" property to determine cpu
frequency, try using a clock in "clocks" if it exists.

Signed-off-by Sean Anderson 
---
This patch depends on .

 drivers/cpu/riscv_cpu.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c
index 1e32bb5678..280c9de376 100644
--- a/drivers/cpu/riscv_cpu.c
+++ b/drivers/cpu/riscv_cpu.c
@@ -3,6 +3,7 @@
  * Copyright (C) 2018, Bin Meng 
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -27,11 +28,24 @@ static int riscv_cpu_get_desc(struct udevice *dev, char 
*buf, int size)
 
 static int riscv_cpu_get_info(struct udevice *dev, struct cpu_info *info)
 {
+   int err;
+   struct clk clk;
const char *mmu;
 
/* Zero out the frequency, in case sizeof(ulong) != sizeof(u32) */
info->cpu_freq = 0;
-   dev_read_u32(dev, "clock-frequency", (u32 *)>cpu_freq);
+
+   /* First try getting the frequency from the assigned clock */
+   err = clk_get_by_index(dev, 0, );
+   if (!err) {
+   err = clk_get_rate();
+   if (!IS_ERR_VALUE(err))
+   info->cpu_freq = err;
+   clk_free();
+   }
+
+   if (!info->cpu_freq)
+   dev_read_u32(dev, "clock-frequency", (u32 *)>cpu_freq);
 
mmu = dev_read_string(dev, "mmu-type");
if (!mmu)
-- 
2.24.1