[PATCH 6/6] riscv: Update SiFive device tree for new CLINT driver

[Sean Anderson]

On 7/23/20 12:51 PM, Sagar Kadam wrote:
> Hello Sean,
> 
>> -----Original Message-----
>> From: Sean Anderson <seanga2 at gmail.com>
>> Sent: Thursday, July 23, 2020 8:22 PM
>> To: Bin Meng <bmeng.cn at gmail.com>
>> Cc: Pragnesh Patel <pragnesh.patel at sifive.com>; Sagar Kadam
>> <sagar.kadam at sifive.com>; U-Boot Mailing List <u-boot at lists.denx.de>; Rick
>> Chen <rickchen36 at gmail.com>
>> Subject: Re: [PATCH 6/6] riscv: Update SiFive device tree for new CLINT driver
>>
>> [External Email] Do not click links or attachments unless you recognize the
>> sender and know the content is safe
>>
>> On 7/23/20 10:47 AM, Bin Meng wrote:
>>> Hi Sean,
>>>
>>> On Thu, Jul 23, 2020 at 9:57 PM Sean Anderson <seanga2 at gmail.com>
>> wrote:
>>>>
>>>> On 7/23/20 9:50 AM, Bin Meng wrote:
>>>>> Hi Sean,
>>>>>
>>>>> On Wed, Jul 22, 2020 at 11:51 PM Sean Anderson <seanga2 at gmail.com>
>> wrote:
>>>>>>
>>>>>> We may need to add a clock-frequency binding like for the K210.
>>>>>>
>>>>>> Signed-off-by: Sean Anderson <seanga2 at gmail.com>
>>>>>> ---
>>>>>> This patch builds but has NOT been tested.
>>>>>>
>>>>>>  arch/riscv/dts/fu540-c000-u-boot.dtsi | 7 ++++++-
>>>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/riscv/dts/fu540-c000-u-boot.dtsi
>>>>>> b/arch/riscv/dts/fu540-c000-u-boot.dtsi
>>>>>> index afdb4f4402..e56bfc7595 100644
>>>>>> --- a/arch/riscv/dts/fu540-c000-u-boot.dtsi
>>>>>> +++ b/arch/riscv/dts/fu540-c000-u-boot.dtsi
>>>>>> @@ -55,8 +55,13 @@
>>>>>>                 };
>>>>>>                 clint at 2000000 {
>>>>>>                         compatible = "riscv,clint0";
>>>>>> -                       interrupts-extended = <&cpu0_intc 3 &cpu0_intc 7
>> &cpu1_intc 3 &cpu1_intc 7 &cpu2_intc 3 &cpu2_intc 7 &cpu3_intc 3
>> &cpu3_intc 7 &cpu4_intc 3 &cpu4_intc 7>;
>>>>>> +                       interrupts-extended = <&cpu0_intc 3 &cpu0_intc 7
>>>>>> +                                              &cpu1_intc 3 &cpu1_intc 7
>>>>>> +                                              &cpu2_intc 3 &cpu2_intc 7
>>>>>> +                                              &cpu3_intc 3 &cpu3_intc 7
>>>>>> +                                              &cpu4_intc 3
>>>>>> + &cpu4_intc 7>;
>>>>>>                         reg = <0x0 0x2000000 0x0 0xc0000>;
>>>>>> +                       clocks = <&prci PRCI_CLK_COREPLL>;
>>>>>
>>>>> This looks wrong to me. The CLINT timer frequency should come from the
>> RTC node.
>>>>>
>>>>> +Pragnesh Patel
>>>>>
>>>>> +Sagar Kadam
>>>>
>>>> On further review, I think you are right that this should be RTCCLK_FREQ.
>>>>
>>>> Perhaps the clocks part should be moved into
>>>> arch/riscv/dts/hifive-unleashed-a00-u-boot.dts and changed to
>>>> something like
>>>>
>>>> clocks = <&rtcclk>;
>>>
> 
> Yes, CLINT is driven by RTC clock.  
> 
>>> How does the device tree look like in the upstream Linux to work with
>>> the new CLINT timer driver?
>>>
>>> Regards,
>>> Bin
>>
>> Anup's patch doesn't change the device tree at all so I think they are still using
>> timebase-frequency.
>>
> In that case I was wondering what would be better:
> 1. IMHO we can skip the property as in your patch [4/6] riscv: Rework Sifive CLINT as UCLASS_TIMER driver
>     has a fallback to timebase-frequency, in case clock for clint is not specified and is falling back to timebase-frequency
>     which is again set to RTCCLK_FREQ
> 2. OR Shift the clock part to board -uboot dts file as you are suggesting in order to keep it synonymous with other TIMER
>      changes done as a part of this series.

I prefer the second option for two reasons. First, properties which
affect a device should be located near its binding in the device tree.
Using timebase-frequency only really makes sense when the cpu itself is
the timer device. This is the case when we read the time from a CSR, but
not when there is a separate device. Second, it lets the device use the
clock subsystem which adds flexibility. If the device is configured for
a different clock speed, the timer can adjust itself.

--Sean