diff mbox series

[v3,05/11] clk: k210: Move the clint clock to under aclk

Message ID 20210409021313.433558-6-seanga2@gmail.com
State Accepted
Delegated to: Andes
Headers show
Series riscv: k210: Enable use of AI ram bank | expand

Commit Message

Sean Anderson April 9, 2021, 2:13 a.m. UTC
No other (real) clocks have the cpu clock as their parent; instead they are
children of aclk. Move the clint clock under aclk to match them.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v2)

Changes in v2:
- New

 drivers/clk/kendryte/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Damien Le Moal April 9, 2021, 2:54 a.m. UTC | #1
On 2021/04/09 11:13, Sean Anderson wrote:
> No other (real) clocks have the cpu clock as their parent; instead they are
> children of aclk. Move the clint clock under aclk to match them.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
> 
> (no changes since v2)
> 
> Changes in v2:
> - New
> 
>  drivers/clk/kendryte/clk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
> index ab86533bb4..5091f07eb0 100644
> --- a/drivers/clk/kendryte/clk.c
> +++ b/drivers/clk/kendryte/clk.c
> @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev)
>  
>  	/* The MTIME register in CLINT runs at one 50th the CPU clock speed */
>  	clk_dm(K210_CLK_CLINT,
> -	       clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50));
> +	       clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50));
>  
>  	return 0;
>  }
> 

Looks good, but representing this clock is in fact useless, at least in Linux,
since the clint driver does not use it directly and derives its rate from
riscv_timebase which is set from the timebase-frequency DT property.

Not sure how u-boot handles that though. Since your code allows changing the
PLLs frequency, the timebase-frequency property may end up being buggy if it is
not changed too.
Sean Anderson April 9, 2021, 2:57 a.m. UTC | #2
On 4/8/21 10:54 PM, Damien Le Moal wrote:
> On 2021/04/09 11:13, Sean Anderson wrote:
>> No other (real) clocks have the cpu clock as their parent; instead they are
>> children of aclk. Move the clint clock under aclk to match them.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> (no changes since v2)
>>
>> Changes in v2:
>> - New
>>
>>   drivers/clk/kendryte/clk.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
>> index ab86533bb4..5091f07eb0 100644
>> --- a/drivers/clk/kendryte/clk.c
>> +++ b/drivers/clk/kendryte/clk.c
>> @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev)
>>   
>>   	/* The MTIME register in CLINT runs at one 50th the CPU clock speed */
>>   	clk_dm(K210_CLK_CLINT,
>> -	       clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50));
>> +	       clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50));
>>   
>>   	return 0;
>>   }
>>
> 
> Looks good, but representing this clock is in fact useless, at least in Linux,
> since the clint driver does not use it directly and derives its rate from
> riscv_timebase which is set from the timebase-frequency DT property.
> 
> Not sure how u-boot handles that though. Since your code allows changing the
> PLLs frequency, the timebase-frequency property may end up being buggy if it is
> not changed too.
> 

U-Boot uses the clocks property since 47d7e3b5eb ("riscv: Move timer
portions of SiFive CLINT to drivers/timer") :)

--Sean
Damien Le Moal April 9, 2021, 2:58 a.m. UTC | #3
On 2021/04/09 11:54, Damien Le Moal wrote:
> On 2021/04/09 11:13, Sean Anderson wrote:
>> No other (real) clocks have the cpu clock as their parent; instead they are
>> children of aclk. Move the clint clock under aclk to match them.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> (no changes since v2)
>>
>> Changes in v2:
>> - New
>>
>>  drivers/clk/kendryte/clk.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
>> index ab86533bb4..5091f07eb0 100644
>> --- a/drivers/clk/kendryte/clk.c
>> +++ b/drivers/clk/kendryte/clk.c
>> @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev)
>>  
>>  	/* The MTIME register in CLINT runs at one 50th the CPU clock speed */
>>  	clk_dm(K210_CLK_CLINT,
>> -	       clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50));
>> +	       clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50));
>>  
>>  	return 0;
>>  }
>>
> 
> Looks good, but representing this clock is in fact useless, at least in Linux,
> since the clint driver does not use it directly and derives its rate from
> riscv_timebase which is set from the timebase-frequency DT property.
> 
> Not sure how u-boot handles that though. Since your code allows changing the
> PLLs frequency, the timebase-frequency property may end up being buggy if it is
> not changed too.

Actually, thinking about this some more, the clint DT node should have an
optional clock entry and use that clock rate to set riscv_timebase if the clock
entry is present. riscv_timebase can be set using timebase-frequency DT property
if the clock is not set.

>
Damien Le Moal April 9, 2021, 3 a.m. UTC | #4
On 2021/04/09 11:58, Sean Anderson wrote:
> 
> On 4/8/21 10:54 PM, Damien Le Moal wrote:
>> On 2021/04/09 11:13, Sean Anderson wrote:
>>> No other (real) clocks have the cpu clock as their parent; instead they are
>>> children of aclk. Move the clint clock under aclk to match them.
>>>
>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>>> ---
>>>
>>> (no changes since v2)
>>>
>>> Changes in v2:
>>> - New
>>>
>>>   drivers/clk/kendryte/clk.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
>>> index ab86533bb4..5091f07eb0 100644
>>> --- a/drivers/clk/kendryte/clk.c
>>> +++ b/drivers/clk/kendryte/clk.c
>>> @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev)
>>>   
>>>   	/* The MTIME register in CLINT runs at one 50th the CPU clock speed */
>>>   	clk_dm(K210_CLK_CLINT,
>>> -	       clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50));
>>> +	       clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50));
>>>   
>>>   	return 0;
>>>   }
>>>
>>
>> Looks good, but representing this clock is in fact useless, at least in Linux,
>> since the clint driver does not use it directly and derives its rate from
>> riscv_timebase which is set from the timebase-frequency DT property.
>>
>> Not sure how u-boot handles that though. Since your code allows changing the
>> PLLs frequency, the timebase-frequency property may end up being buggy if it is
>> not changed too.
>>
> 
> U-Boot uses the clocks property since 47d7e3b5eb ("riscv: Move timer
> portions of SiFive CLINT to drivers/timer") :)

I remember that adding the clock property to Linux DT gave warning with make
dtb_check. Hence I removed it. driver/clocksoure/timer-riscv.c also make the
timebase-frequency DT property mandatory. Time for some more patching on Linux
side I guess :)


> 
> --Sean
>
Sean Anderson April 9, 2021, 3:09 a.m. UTC | #5
On 4/8/21 10:58 PM, Damien Le Moal wrote:
> On 2021/04/09 11:54, Damien Le Moal wrote:
>> On 2021/04/09 11:13, Sean Anderson wrote:
>>> No other (real) clocks have the cpu clock as their parent; instead they are
>>> children of aclk. Move the clint clock under aclk to match them.
>>>
>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>>> ---
>>>
>>> (no changes since v2)
>>>
>>> Changes in v2:
>>> - New
>>>
>>>   drivers/clk/kendryte/clk.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
>>> index ab86533bb4..5091f07eb0 100644
>>> --- a/drivers/clk/kendryte/clk.c
>>> +++ b/drivers/clk/kendryte/clk.c
>>> @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev)
>>>   
>>>   	/* The MTIME register in CLINT runs at one 50th the CPU clock speed */
>>>   	clk_dm(K210_CLK_CLINT,
>>> -	       clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50));
>>> +	       clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50));
>>>   
>>>   	return 0;
>>>   }
>>>
>>
>> Looks good, but representing this clock is in fact useless, at least in Linux,
>> since the clint driver does not use it directly and derives its rate from
>> riscv_timebase which is set from the timebase-frequency DT property.
>>
>> Not sure how u-boot handles that though. Since your code allows changing the
>> PLLs frequency, the timebase-frequency property may end up being buggy if it is
>> not changed too.
> 
> Actually, thinking about this some more, the clint DT node should have an
> optional clock entry and use that clock rate to set riscv_timebase if the clock
> entry is present. riscv_timebase can be set using timebase-frequency DT property
> if the clock is not set.

This is how it is done in U-Boot. See timer_pre_probe and
timer_timebase_fallback for the mechanism.

--Sean
diff mbox series

Patch

diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
index ab86533bb4..5091f07eb0 100644
--- a/drivers/clk/kendryte/clk.c
+++ b/drivers/clk/kendryte/clk.c
@@ -643,7 +643,7 @@  static int k210_clk_probe(struct udevice *dev)
 
 	/* The MTIME register in CLINT runs at one 50th the CPU clock speed */
 	clk_dm(K210_CLK_CLINT,
-	       clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50));
+	       clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50));
 
 	return 0;
 }