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