Hi Sean, >-----Original Message----- >From: Sean Anderson <[email protected]> >Sent: 02 May 2020 23:46 >To: Pragnesh Patel <[email protected]>; [email protected] >Cc: [email protected]; [email protected]; >[email protected]; Paul Walmsley <[email protected]>; >[email protected]; Troy Benjegerdes ><[email protected]>; [email protected]; Sagar Kadam ><[email protected]>; [email protected]; Lukas Auer ><[email protected]> >Subject: Re: [PATCH v7 16/22] riscv: Enable cpu clock if it is present > >[External Email] Do not click links or attachments unless you recognize the >sender and know the content is safe > >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 <[email protected]> > >If you make substantial changes can you please make a note of it in the >commit? I did not sign off on *this* code.
This patch is copied from your v9 series [1] and I made some changes, so the idea is to give credit to everyone who contributed. [1] https://patchwork.ozlabs.org/project/uboot/patch/[email protected]/ > >> Signed-off-by: Pragnesh Patel <[email protected]> >> Reviewed-by: Bin Meng <[email protected]> >> --- >> 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 >[email protected]/ Thanks for pointing this out to me, I will give it a try with your patch [1] and drop this patch if not necessary. [1] https://patchwork.ozlabs.org/project/uboot/patch/[email protected]/ > >> + 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. Thanks for the review, will take care in future. > >> + return ret; >> + } else { >> + ret = clk_set_rate(&clk, clock); >> + if (ret < 0) { >> + debug("Could not set CPU clock\n"); > >Neither should this be. will take care in future. > >> + 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. Thanks for the explanation, will take care in future. > >> + 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
