RE: [PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk frequency

Hello Sean,

-----Original Message----- From: Sean Anderson seanga2@gmail.com Sent: Friday, February 21, 2020 11:48 AM To: Sagar Kadam sagar.kadam@sifive.com; Bin Meng bmeng.cn@gmail.com Cc: U-Boot Mailing List u-boot@lists.denx.de; Lukasz Majewski lukma@denx.de; Anup Patel Anup.Patel@wdc.com; Paul Walmsley ( Sifive) paul.walmsley@sifive.com; Vincent Chen vincent.chen@sifive.com Subject: Re: [PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk frequency

On 2/21/20 12:59 AM, Sagar Kadam wrote:

...

...

What you were trying to do in this patch, I believe the following 2 patches already did it.

http://patchwork.ozlabs.org/patch/1236177/ http://patchwork.ozlabs.org/patch/1236180/

Thanks for sharing the links. Unfortunately I didn’t come across it. I applied these two patches within my repo (assuming there are not extra dependencies) and would like to share my observation: The implementation in the patch links shared here read's clock dt property in clk_get_by_index. If the cpu dt node doesn't contain clock property it return's negative value and so the clk_get_rate here won't be be

executed.

...

• ret = clk_get_by_index(dev, 0, &clk);
• if (!ret) {

• ret = clk_get_rate(&clk);
  

This is working as designed. The idea is to use the clocks property if it exists and to fall back on clock-frequency otherwise.

Thanks for clarifying.

...

Thus when I tested this on hifive unleashed the "cpu detail" showed

frequency as 0 Hz.

This is because the cpu nodes in the hifive/fu540 device tree have neither a clock-frequency property or a clocks property.

Yes, I will add clocks dt property.

...

Please correct me if I am wrong, but IMHO here we can check for negative return code [if (ret < 0)] instead of (!ret) and if "clocks" is missing in dt property then get the clock driver using uclass_get_device_by_driver->request the clock -> and get clock rate, just like below

•   if (!ret) {
  

•   if (ret < 0) {
  

•           ret = uclass_get_device_by_driver(UCLASS_CLK,
  

•                                             DM_GET_DRIVER(sifive_fu540_prci),
  

•                                             &dev);
  

This is again adding board-specific code to a generic driver. Add a UCLASS_CLOCK driver if you want to support clocks. That way there is no need for code like this.

Thanks for suggestion. I will remove board-specific code from generic driver.

...

•           clk.id = 0;
  

•           ret = clk_request(dev, &clk);
  

•           if (ret < 0) {
  

•                   pr_err("%s: request to clock device
  

• failed...\n", __func__);

I belive pr_err is supported for linux compatibility. New code should use log_*. This is also probably debug-level and not err-level.

Ok. I will replace pr_err with log_debug.

...

•                   return ret;
  

•           }
  

Also there is missing "include linux/err.h" which is needed by IS_ERR_VALUE

Yes, I noticed that when rebasing. It will be fixed in the next version of the series.

Thanks for updating.

BR, Sagar Kadam

...

Please let me know if I can rebase and update my patches above the two patch's you pointed to.

...

Regards, Bin

--Sean