[PATCH v7 16/22] riscv: Enable cpu clock if it is present

[Pragnesh Patel]

Hi Sean,

>-----Original Message-----
>From: Sean Anderson <seanga2 at gmail.com>
>Sent: 02 May 2020 23:46
>To: Pragnesh Patel <pragnesh.patel at sifive.com>; u-boot at lists.denx.de
>Cc: atish.patra at wdc.com; palmerdabbelt at google.com;
>bmeng.cn at gmail.com; Paul Walmsley <paul.walmsley at sifive.com>;
>jagan at amarulasolutions.com; Troy Benjegerdes
><troy.benjegerdes at sifive.com>; anup.patel at wdc.com; Sagar Kadam
><sagar.kadam at sifive.com>; rick at andestech.com; Lukas Auer
><lukas.auer at aisec.fraunhofer.de>
>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 <seanga2 at 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.

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/20200503023550.326791-19-seanga2@gmail.com/

>
>> Signed-off-by: Pragnesh Patel <pragnesh.patel at sifive.com>
>> Reviewed-by: Bin Meng <bmeng.cn at 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 at 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-seanga2 at gmail.com/

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/20200503023550.326791-19-seanga2@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.

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