From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Anderson Date: Thu, 23 Jul 2020 16:27:49 -0400 Subject: [PATCH 6/6] riscv: Update SiFive device tree for new CLINT driver In-Reply-To: References: <20200722155110.713966-1-seanga2@gmail.com> <20200722155110.713966-7-seanga2@gmail.com> Message-ID: <847c4dab-c5aa-cf89-9441-c589eaf4ba5f@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 7/23/20 12:51 PM, Sagar Kadam wrote: > Hello Sean, > >> -----Original Message----- >> From: Sean Anderson >> Sent: Thursday, July 23, 2020 8:22 PM >> To: Bin Meng >> Cc: Pragnesh Patel ; Sagar Kadam >> ; U-Boot Mailing List ; Rick >> Chen >> 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 >> wrote: >>>> >>>> On 7/23/20 9:50 AM, Bin Meng wrote: >>>>> Hi Sean, >>>>> >>>>> On Wed, Jul 22, 2020 at 11:51 PM Sean Anderson >> wrote: >>>>>> >>>>>> We may need to add a clock-frequency binding like for the K210. >>>>>> >>>>>> Signed-off-by: Sean Anderson >>>>>> --- >>>>>> 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