> -----Original Message-----
> From: Sean Anderson <sean...@gmail.com>
> Sent: Friday, July 24, 2020 1:58 AM
> To: Sagar Kadam <sagar.ka...@sifive.com>; Bin Meng
> <bmeng...@gmail.com>
> Cc: Pragnesh Patel <pragnesh.pa...@sifive.com>; U-Boot Mailing List <u-
> b...@lists.denx.de>; Rick Chen <rickche...@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 12:51 PM, Sagar Kadam wrote:
> > Hello Sean,
> >
> >> -----Original Message-----
> >> From: Sean Anderson <sean...@gmail.com>
> >> Sent: Thursday, July 23, 2020 8:22 PM
> >> To: Bin Meng <bmeng...@gmail.com>
> >> Cc: Pragnesh Patel <pragnesh.pa...@sifive.com>; Sagar Kadam
> >> <sagar.ka...@sifive.com>; U-Boot Mailing List <u-boot@lists.denx.de>;
> >> Rick Chen <rickche...@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 <sean...@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
> <sean...@gmail.com>
> >> wrote:
> >>>>>>
> >>>>>> We may need to add a clock-frequency binding like for the K210.
> >>>>>>
> >>>>>> Signed-off-by: Sean Anderson <sean...@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@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.
>
Okay, agreed.
Thanks for clarification.
BR,
Sagar
> --Sean
