On 5/2/20 6:06 AM, Pragnesh Patel wrote:
> The cpu clock is probably already enabled if we are executing code (though
> we could be executing from a different core). This patch prevents the cpu
> clock or its parents from being disabled.
> 
> Signed-off-by: Sean Anderson <sean...@gmail.com>

If you make substantial changes can you please make a note of it in the
commit? I did not sign off on *this* code.

> Signed-off-by: Pragnesh Patel <pragnesh.pa...@sifive.com>
> Reviewed-by: Bin Meng <bmeng...@gmail.com>
> ---
>  drivers/cpu/riscv_cpu.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c
> index 28ad0aa30f..8ebe0341fd 100644
> --- a/drivers/cpu/riscv_cpu.c
> +++ b/drivers/cpu/riscv_cpu.c
> @@ -9,6 +9,7 @@
>  #include <errno.h>
>  #include <dm/device-internal.h>
>  #include <dm/lists.h>
> +#include <clk.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -100,6 +101,37 @@ static int riscv_cpu_bind(struct udevice *dev)
>       return 0;
>  }
>  
> +static int riscv_cpu_probe(struct udevice *dev)
> +{
> +     int ret = 0;
> +     u32 clock = 0;
> +     struct clk clk;
> +
> +     /* Get a clock if it exists */
> +     ret = clk_get_by_index(dev, 0, &clk);
> +     if (ret)
> +             return 0;
> +
> +     ret = dev_read_u32(dev, "clock-frequency", &clock);

Ok, so as I understand it, your goal for your changes this patch is to
have a way to set the cpu frequency on startup. However, I think the
cpu-frequency property is not the correct way to go about this when we
have a clock from the device tree. In this case, one should use the
assigned-clock* properties to have the frequency set automatically when
clk_get_by_index is called. There is no need to add this functionality
here.

With the previous patch in the series you pulled this from [1], one
could easily do something like

cpus {
        assigned-clocks = <&foo FOO_CPU>;
        assigned-clock-frequency = <100000000>;
        cpu@0 {
                /* ... */
                clocks = <&foo FOO_CPU>;
                /* ... */
        };
};

which would use existing code to assign the frequency.

[1] 
https://patchwork.ozlabs.org/project/uboot/patch/20200423023320.1380090-18-sean...@gmail.com/

> +     if (ret) {
> +             debug("clock-frequency not found in dt %d\n", ret);

This should not be an error. You also need to check for ENOSYS and
ENOTSUPP like below.

> +             return ret;
> +     } else {
> +             ret = clk_set_rate(&clk, clock);
> +             if (ret < 0) {
> +                     debug("Could not set CPU clock\n");

Neither should this be.

> +                     return ret;
> +             }
> +     }
> +
> +     ret = clk_enable(&clk);
> +     clk_free(&clk);
> +     if (ret == -ENOSYS || ret == -ENOTSUPP)

All clk_ calls can return ENOSYS and ENOTSUPP. These are returned when
the underlying clock does not support the operation. However, they are
not appropriate errors to return from this function.

> +             return 0;
> +     else
> +             return ret;
> +}
> +
>  static const struct cpu_ops riscv_cpu_ops = {
>       .get_desc       = riscv_cpu_get_desc,
>       .get_info       = riscv_cpu_get_info,
> @@ -116,6 +148,7 @@ U_BOOT_DRIVER(riscv_cpu) = {
>       .id = UCLASS_CPU,
>       .of_match = riscv_cpu_ids,
>       .bind = riscv_cpu_bind,
> +     .probe = riscv_cpu_probe,
>       .ops = &riscv_cpu_ops,
>       .flags = DM_FLAG_PRE_RELOC,
>  };
> 

--Sean

Reply via email to