diff mbox series

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

Message ID 20200502100628.24809-17-pragnesh.patel@sifive.com
State New
Headers show
Series RISC-V SiFive FU540 support SPL | expand

Commit Message

Pragnesh Patel May 2, 2020, 10:06 a.m. UTC
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>
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(+)

Comments

Bin Meng May 2, 2020, 12:55 p.m. UTC | #1
On Sat, May 2, 2020 at 6:08 PM Pragnesh Patel <pragnesh.patel at sifive.com> 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>
> 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(+)
>

Tested-by: Bin Meng <bmeng.cn at gmail.com>
Sean Anderson May 2, 2020, 6:15 p.m. UTC | #2
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.

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

> +	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
Pragnesh Patel May 3, 2020, 7:12 a.m. UTC | #3
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 at 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 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.

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
Sean Anderson May 3, 2020, 5:17 p.m. UTC | #4
On 5/3/20 3:12 AM, Pragnesh Patel wrote:
> 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.

That's fine, just please make a note when you make substantial changes.
For example, you could write

Signed-off-by: Sean Anderson <seanga2 at gmail.com>
Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
[set clock frequency from clock-frequency property]
Signed-off-by: Pragnesh Patel <pragnesh.patel at sifive.com>

so it's more clear what changes have been made since this patch was
posted last.

--Sean
Pragnesh Patel May 4, 2020, 5:20 a.m. UTC | #5
Hi Sean,

>-----Original Message-----
>From: Sean Anderson <seanga2 at gmail.com>
>Sent: 03 May 2020 22:47
>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/3/20 3:12 AM, Pragnesh Patel wrote:
>> 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.
>
>That's fine, just please make a note when you make substantial changes.
>For example, you could write
>
>Signed-off-by: Sean Anderson <seanga2 at gmail.com>
>Reviewed-by: Bin Meng <bmeng.cn at gmail.com> [set clock frequency from
>clock-frequency property]
>Signed-off-by: Pragnesh Patel <pragnesh.patel at sifive.com>
>
>so it's more clear what changes have been made since this patch was posted
>last.

Will take care in future.

>
>--Sean
diff mbox series

Patch

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);
+	if (ret) {
+		debug("clock-frequency not found in dt %d\n", ret);
+		return ret;
+	} else {
+		ret = clk_set_rate(&clk, clock);
+		if (ret < 0) {
+			debug("Could not set CPU clock\n");
+			return ret;
+		}
+	}
+
+	ret = clk_enable(&clk);
+	clk_free(&clk);
+	if (ret == -ENOSYS || ret == -ENOTSUPP)
+		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,
 };