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

Hi Sean,

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

...

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

...

• 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