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

[Sean Anderson <seanga2@gmail.com>]

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.

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

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

> +		return ret;
> +	} else {
> +		ret = clk_set_rate(&clk, clock);
> +		if (ret < 0) {
> +			debug("Could not set CPU clock\n");

Neither should this be.

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

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