| Message ID | 20200722155110.713966-7-seanga2@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Andes |
| Headers | show |
| Series | riscv: Clean up timer drivers | expand |
Hi Sean, On Wed, Jul 22, 2020 at 11:51 PM Sean Anderson <seanga2@gmail.com> wrote: > > We may need to add a clock-frequency binding like for the K210. > > Signed-off-by: Sean Anderson <seanga2@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 > u-boot,dm-spl; > }; > dmc: dmc@100b0000 { Regards, Bin
On 7/23/20 9:50 AM, Bin Meng wrote: > Hi Sean, > > On Wed, Jul 22, 2020 at 11:51 PM Sean Anderson <seanga2@gmail.com> wrote: >> >> We may need to add a clock-frequency binding like for the K210. >> >> Signed-off-by: Sean Anderson <seanga2@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>; --Sean
Hi Sean, >-----Original Message----- >From: Sean Anderson <seanga2@gmail.com> >Sent: 23 July 2020 19:27 >To: Bin Meng <bmeng.cn@gmail.com>; Pragnesh Patel ><pragnesh.patel@sifive.com>; Sagar Kadam <sagar.kadam@sifive.com> >Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Rick Chen ><rickchen36@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 9:50 AM, Bin Meng wrote: >> Hi Sean, >> >> On Wed, Jul 22, 2020 at 11:51 PM Sean Anderson <seanga2@gmail.com> >wrote: >>> >>> We may need to add a clock-frequency binding like for the K210. >>> >>> Signed-off-by: Sean Anderson <seanga2@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>; I am okay with your suggestion, better to add in "hifive-unleashed-a00-u-boot.dtsi". > >--Sean
Hi Sean, On Thu, Jul 23, 2020 at 9:57 PM Sean Anderson <seanga2@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@gmail.com> wrote: > >> > >> We may need to add a clock-frequency binding like for the K210. > >> > >> Signed-off-by: Sean Anderson <seanga2@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>; How does the device tree look like in the upstream Linux to work with the new CLINT timer driver? Regards, Bin
On 7/23/20 10:47 AM, Bin Meng wrote: > Hi Sean, > > On Thu, Jul 23, 2020 at 9:57 PM Sean Anderson <seanga2@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@gmail.com> wrote: >>>> >>>> We may need to add a clock-frequency binding like for the K210. >>>> >>>> Signed-off-by: Sean Anderson <seanga2@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>; > > 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. --Sean
Hello Sean, > -----Original Message----- > From: Sean Anderson <seanga2@gmail.com> > Sent: Thursday, July 23, 2020 8:22 PM > To: Bin Meng <bmeng.cn@gmail.com> > Cc: Pragnesh Patel <pragnesh.patel@sifive.com>; Sagar Kadam > <sagar.kadam@sifive.com>; U-Boot Mailing List <u-boot@lists.denx.de>; Rick > Chen <rickchen36@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@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@gmail.com> > wrote: > >>>> > >>>> We may need to add a clock-frequency binding like for the K210. > >>>> > >>>> Signed-off-by: Sean Anderson <seanga2@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. Thanks & BR, Sagar > --Sean
On 7/23/20 12:51 PM, Sagar Kadam wrote: > Hello Sean, > >> -----Original Message----- >> From: Sean Anderson <seanga2@gmail.com> >> Sent: Thursday, July 23, 2020 8:22 PM >> To: Bin Meng <bmeng.cn@gmail.com> >> Cc: Pragnesh Patel <pragnesh.patel@sifive.com>; Sagar Kadam >> <sagar.kadam@sifive.com>; U-Boot Mailing List <u-boot@lists.denx.de>; Rick >> Chen <rickchen36@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@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@gmail.com> >> wrote: >>>>>> >>>>>> We may need to add a clock-frequency binding like for the K210. >>>>>> >>>>>> Signed-off-by: Sean Anderson <seanga2@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. --Sean
+Anup Patel On Thu, Jul 23, 2020 at 10:52 PM Sean Anderson <seanga2@gmail.com> wrote: > > On 7/23/20 10:47 AM, Bin Meng wrote: > > Hi Sean, > > > > On Thu, Jul 23, 2020 at 9:57 PM Sean Anderson <seanga2@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@gmail.com> wrote: > >>>> > >>>> We may need to add a clock-frequency binding like for the K210. > >>>> > >>>> Signed-off-by: Sean Anderson <seanga2@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>; > > > > 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. Anup, could you please look at this and suggest whether we should add this in the kernel upstream DTS? Regards, Bin
> -----Original Message----- > From: Sean Anderson <seanga2@gmail.com> > Sent: Friday, July 24, 2020 1:58 AM > To: Sagar Kadam <sagar.kadam@sifive.com>; Bin Meng > <bmeng.cn@gmail.com> > Cc: Pragnesh Patel <pragnesh.patel@sifive.com>; U-Boot Mailing List <u- > boot@lists.denx.de>; Rick Chen <rickchen36@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 <seanga2@gmail.com> > >> Sent: Thursday, July 23, 2020 8:22 PM > >> To: Bin Meng <bmeng.cn@gmail.com> > >> Cc: Pragnesh Patel <pragnesh.patel@sifive.com>; Sagar Kadam > >> <sagar.kadam@sifive.com>; U-Boot Mailing List <u-boot@lists.denx.de>; > >> Rick Chen <rickchen36@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@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@gmail.com> > >> wrote: > >>>>>> > >>>>>> We may need to add a clock-frequency binding like for the K210. > >>>>>> > >>>>>> Signed-off-by: Sean Anderson <seanga2@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
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>; u-boot,dm-spl; }; dmc: dmc@100b0000 {
We may need to add a clock-frequency binding like for the K210. Signed-off-by: Sean Anderson <seanga2@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(-)