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
