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

[Sean Anderson]

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.

> 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.

> 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.

> +               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.

> +                       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.

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

--Sean