| Message ID | 20200901103208.440316-2-seanga2@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Andes |
| Headers | show |
| Series | riscv: Clean up timer drivers | expand |
> > The riscv-timer driver currently serves as a shim for several riscv timer > drivers. This is not too desirable because it bypasses the usual timer > selection via the driver model. There is no easy way to specify an > alternate timing driver, or have the tick rate depend on the cpu's > configured frequency. The timer drivers also do not have device structs, > and so have to rely on storing parameters in gd_t. Lastly, there is no > initialization call, so driver init is done in the same function which > reads the time. This can result in confusing error messages. To a user, it > looks like the driver failed when trying to read the time, whereas it may > have failed while initializing. > > This patch removes the shim functionality from the riscv-timer driver, and > has it instead implement the former rdtime.c timer driver. This is because > existing u-boot users who pass in a device tree (e.g. qemu) do not create a > timer device for S-mode u-boot. The existing behavior of creating the > riscv-timer device in the riscv cpu driver must be kept. The actual reading > of the CSRs has been redone in the style of Linux's get_cycles64. > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > Reviewed-by: Bin Meng <bin.meng@windriver.com> > --- > > (no changes since v2) > > Changes in v2: > - Remove RISCV_RDTIME KConfig option > > arch/riscv/Kconfig | 8 -------- > arch/riscv/lib/Makefile | 1 - > arch/riscv/lib/rdtime.c | 38 ------------------------------------ > drivers/timer/Kconfig | 6 +++--- > drivers/timer/riscv_timer.c | 39 +++++++++++++++++++------------------ > 5 files changed, 23 insertions(+), 69 deletions(-) > delete mode 100644 arch/riscv/lib/rdtime.c > Reviewed-by: Rick Chen <rick@andestech.com>
Hi Sean, >-----Original Message----- >From: Sean Anderson <seanga2@gmail.com> >Sent: 01 September 2020 16:02 >To: u-boot@lists.denx.de >Cc: Rick Chen <rickchen36@gmail.com>; Bin Meng <bmeng.cn@gmail.com>; >Pragnesh Patel <pragnesh.patel@openfive.com>; Sean Anderson ><seanga2@gmail.com>; Bin Meng <bin.meng@windriver.com>; Anup Patel ><anup@brainfault.org> >Subject: [PATCH v3 1/7] riscv: Rework riscv timer driver to only support S-mode > >[External Email] Do not click links or attachments unless you recognize the >sender and know the content is safe > >The riscv-timer driver currently serves as a shim for several riscv timer drivers. >This is not too desirable because it bypasses the usual timer selection via the >driver model. There is no easy way to specify an alternate timing driver, or have >the tick rate depend on the cpu's configured frequency. The timer drivers also do >not have device structs, and so have to rely on storing parameters in gd_t. Lastly, >there is no initialization call, so driver init is done in the same function which >reads the time. This can result in confusing error messages. To a user, it looks like >the driver failed when trying to read the time, whereas it may have failed while >initializing. > >This patch removes the shim functionality from the riscv-timer driver, and has it >instead implement the former rdtime.c timer driver. This is because existing u- >boot users who pass in a device tree (e.g. qemu) do not create a timer device for >S-mode u-boot. The existing behavior of creating the riscv-timer device in the >riscv cpu driver must be kept. The actual reading of the CSRs has been redone in >the style of Linux's get_cycles64. > >Signed-off-by: Sean Anderson <seanga2@gmail.com> >Reviewed-by: Bin Meng <bin.meng@windriver.com> >--- > >(no changes since v2) > >Changes in v2: >- Remove RISCV_RDTIME KConfig option > > arch/riscv/Kconfig | 8 -------- > arch/riscv/lib/Makefile | 1 - > arch/riscv/lib/rdtime.c | 38 ------------------------------------ > drivers/timer/Kconfig | 6 +++--- > drivers/timer/riscv_timer.c | 39 +++++++++++++++++++------------------ > 5 files changed, 23 insertions(+), 69 deletions(-) delete mode 100644 >arch/riscv/lib/rdtime.c > >diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 009a545fcf..21e6690f4d >100644 >--- a/arch/riscv/Kconfig >+++ b/arch/riscv/Kconfig >@@ -185,14 +185,6 @@ config ANDES_PLMT > The Andes PLMT block holds memory-mapped mtime register > associated with timer tick. > >-config RISCV_RDTIME >- bool >- default y if RISCV_SMODE || SPL_RISCV_SMODE >- help >- The provides the riscv_get_time() API that is implemented using the >- standard rdtime instruction. This is the case for S-mode U-Boot, and >- is useful for processors that support rdtime in M-mode too. >- > config SYS_MALLOC_F_LEN > default 0x1000 > >diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index >6c503ff2b2..10ac5b06d3 100644 >--- a/arch/riscv/lib/Makefile >+++ b/arch/riscv/lib/Makefile >@@ -15,7 +15,6 @@ obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o > obj-$(CONFIG_ANDES_PLIC) += andes_plic.o > obj-$(CONFIG_ANDES_PLMT) += andes_plmt.o else >-obj-$(CONFIG_RISCV_RDTIME) += rdtime.o > obj-$(CONFIG_SBI) += sbi.o > obj-$(CONFIG_SBI_IPI) += sbi_ipi.o > endif >diff --git a/arch/riscv/lib/rdtime.c b/arch/riscv/lib/rdtime.c deleted file mode >100644 index e128d7fce6..0000000000 >--- a/arch/riscv/lib/rdtime.c >+++ /dev/null >@@ -1,38 +0,0 @@ >-// SPDX-License-Identifier: GPL-2.0+ >-/* >- * Copyright (C) 2018, Anup Patel <anup@brainfault.org> >- * Copyright (C) 2018, Bin Meng <bmeng.cn@gmail.com> >- * >- * The riscv_get_time() API implementation that is using the >- * standard rdtime instruction. >- */ >- >-#include <common.h> >- >-/* Implement the API required by RISC-V timer driver */ -int riscv_get_time(u64 >*time) -{ -#ifdef CONFIG_64BIT >- u64 n; >- >- __asm__ __volatile__ ( >- "rdtime %0" >- : "=r" (n)); >- >- *time = n; >-#else >- u32 lo, hi, tmp; >- >- __asm__ __volatile__ ( >- "1:\n" >- "rdtimeh %0\n" >- "rdtime %1\n" >- "rdtimeh %2\n" >- "bne %0, %2, 1b" >- : "=&r" (hi), "=&r" (lo), "=&r" (tmp)); >- >- *time = ((u64)hi << 32) | lo; >-#endif >- >- return 0; >-} >diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig index >637024445c..b85fa33e47 100644 >--- a/drivers/timer/Kconfig >+++ b/drivers/timer/Kconfig >@@ -144,10 +144,10 @@ config OMAP_TIMER > > config RISCV_TIMER > bool "RISC-V timer support" >- depends on TIMER && RISCV >+ depends on TIMER && RISCV_SMODE What about SPL_RISCV_SMODE ? > help >- Select this to enable support for the timer as defined >- by the RISC-V privileged architecture spec. >+ Select this to enable support for a generic RISC-V S-Mode timer >+ driver. > > config ROCKCHIP_TIMER > bool "Rockchip timer support" >diff --git a/drivers/timer/riscv_timer.c b/drivers/timer/riscv_timer.c index >9f9f070e0b..449fcfcfd5 100644 >--- a/drivers/timer/riscv_timer.c >+++ b/drivers/timer/riscv_timer.c >@@ -1,36 +1,37 @@ > // SPDX-License-Identifier: GPL-2.0+ > /* >+ * Copyright (C) 2020, Sean Anderson <seanga2@gmail.com> > * Copyright (C) 2018, Bin Meng <bmeng.cn@gmail.com> >+ * Copyright (C) 2018, Anup Patel <anup@brainfault.org> >+ * Copyright (C) 2012 Regents of the University of California > * >- * RISC-V privileged architecture defined generic timer driver >+ * RISC-V architecturally-defined generic timer driver > * >- * This driver relies on RISC-V platform codes to provide the essential API >- * riscv_get_time() which is supposed to return the timer counter as defined >- * by the RISC-V privileged architecture spec. >- * >- * This driver can be used in both M-mode and S-mode U-Boot. >+ * This driver provides generic timer support for S-mode U-Boot. > */ > > #include <common.h> > #include <dm.h> > #include <errno.h> > #include <timer.h> >-#include <asm/io.h> >- >-/** >- * riscv_get_time() - get the timer counter >- * >- * Platform codes should provide this API in order to make this driver function. >- * >- * @time: the 64-bit timer count as defined by the RISC-V privileged >- * architecture spec. >- * @return: 0 on success, -ve on error. >- */ >-extern int riscv_get_time(u64 *time); >+#include <asm/csr.h> > > static int riscv_timer_get_count(struct udevice *dev, u64 *count) { >- return riscv_get_time(count); >+ if (IS_ENABLED(CONFIG_64BIT)) { >+ *count = csr_read(CSR_TIME); >+ } else { >+ u32 hi, lo; >+ >+ do { >+ hi = csr_read(CSR_TIMEH); >+ lo = csr_read(CSR_TIME); >+ } while (hi != csr_read(CSR_TIMEH)); >+ >+ *count = ((u64)hi << 32) | lo; >+ } >+ >+ return 0; > } > > static int riscv_timer_probe(struct udevice *dev) >-- >2.28.0
On 9/8/20 3:57 AM, Pragnesh Patel wrote: > Hi Sean, > >> -----Original Message----- >> From: Sean Anderson <seanga2@gmail.com> >> Sent: 01 September 2020 16:02 >> To: u-boot@lists.denx.de >> Cc: Rick Chen <rickchen36@gmail.com>; Bin Meng <bmeng.cn@gmail.com>; >> Pragnesh Patel <pragnesh.patel@openfive.com>; Sean Anderson >> <seanga2@gmail.com>; Bin Meng <bin.meng@windriver.com>; Anup Patel >> <anup@brainfault.org> >> Subject: [PATCH v3 1/7] riscv: Rework riscv timer driver to only support S-mode >> >> [External Email] Do not click links or attachments unless you recognize the >> sender and know the content is safe >> >> The riscv-timer driver currently serves as a shim for several riscv timer drivers. >> This is not too desirable because it bypasses the usual timer selection via the >> driver model. There is no easy way to specify an alternate timing driver, or have >> the tick rate depend on the cpu's configured frequency. The timer drivers also do >> not have device structs, and so have to rely on storing parameters in gd_t. Lastly, >> there is no initialization call, so driver init is done in the same function which >> reads the time. This can result in confusing error messages. To a user, it looks like >> the driver failed when trying to read the time, whereas it may have failed while >> initializing. >> >> This patch removes the shim functionality from the riscv-timer driver, and has it >> instead implement the former rdtime.c timer driver. This is because existing u- >> boot users who pass in a device tree (e.g. qemu) do not create a timer device for >> S-mode u-boot. The existing behavior of creating the riscv-timer device in the >> riscv cpu driver must be kept. The actual reading of the CSRs has been redone in >> the style of Linux's get_cycles64. >> >> Signed-off-by: Sean Anderson <seanga2@gmail.com> >> Reviewed-by: Bin Meng <bin.meng@windriver.com> >> --- >> >> (no changes since v2) >> >> Changes in v2: >> - Remove RISCV_RDTIME KConfig option >> >> arch/riscv/Kconfig | 8 -------- >> arch/riscv/lib/Makefile | 1 - >> arch/riscv/lib/rdtime.c | 38 ------------------------------------ >> drivers/timer/Kconfig | 6 +++--- >> drivers/timer/riscv_timer.c | 39 +++++++++++++++++++------------------ >> 5 files changed, 23 insertions(+), 69 deletions(-) delete mode 100644 >> arch/riscv/lib/rdtime.c >> >> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 009a545fcf..21e6690f4d >> 100644 >> --- a/arch/riscv/Kconfig >> +++ b/arch/riscv/Kconfig >> @@ -185,14 +185,6 @@ config ANDES_PLMT >> The Andes PLMT block holds memory-mapped mtime register >> associated with timer tick. >> >> -config RISCV_RDTIME >> - bool >> - default y if RISCV_SMODE || SPL_RISCV_SMODE >> - help >> - The provides the riscv_get_time() API that is implemented using the >> - standard rdtime instruction. This is the case for S-mode U-Boot, and >> - is useful for processors that support rdtime in M-mode too. >> - >> config SYS_MALLOC_F_LEN >> default 0x1000 >> >> diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index >> 6c503ff2b2..10ac5b06d3 100644 >> --- a/arch/riscv/lib/Makefile >> +++ b/arch/riscv/lib/Makefile >> @@ -15,7 +15,6 @@ obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o >> obj-$(CONFIG_ANDES_PLIC) += andes_plic.o >> obj-$(CONFIG_ANDES_PLMT) += andes_plmt.o else >> -obj-$(CONFIG_RISCV_RDTIME) += rdtime.o >> obj-$(CONFIG_SBI) += sbi.o >> obj-$(CONFIG_SBI_IPI) += sbi_ipi.o >> endif >> diff --git a/arch/riscv/lib/rdtime.c b/arch/riscv/lib/rdtime.c deleted file mode >> 100644 index e128d7fce6..0000000000 >> --- a/arch/riscv/lib/rdtime.c >> +++ /dev/null >> @@ -1,38 +0,0 @@ >> -// SPDX-License-Identifier: GPL-2.0+ >> -/* >> - * Copyright (C) 2018, Anup Patel <anup@brainfault.org> >> - * Copyright (C) 2018, Bin Meng <bmeng.cn@gmail.com> >> - * >> - * The riscv_get_time() API implementation that is using the >> - * standard rdtime instruction. >> - */ >> - >> -#include <common.h> >> - >> -/* Implement the API required by RISC-V timer driver */ -int riscv_get_time(u64 >> *time) -{ -#ifdef CONFIG_64BIT >> - u64 n; >> - >> - __asm__ __volatile__ ( >> - "rdtime %0" >> - : "=r" (n)); >> - >> - *time = n; >> -#else >> - u32 lo, hi, tmp; >> - >> - __asm__ __volatile__ ( >> - "1:\n" >> - "rdtimeh %0\n" >> - "rdtime %1\n" >> - "rdtimeh %2\n" >> - "bne %0, %2, 1b" >> - : "=&r" (hi), "=&r" (lo), "=&r" (tmp)); >> - >> - *time = ((u64)hi << 32) | lo; >> -#endif >> - >> - return 0; >> -} >> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig index >> 637024445c..b85fa33e47 100644 >> --- a/drivers/timer/Kconfig >> +++ b/drivers/timer/Kconfig >> @@ -144,10 +144,10 @@ config OMAP_TIMER >> >> config RISCV_TIMER >> bool "RISC-V timer support" >> - depends on TIMER && RISCV >> + depends on TIMER && RISCV_SMODE > > What about SPL_RISCV_SMODE ? That should be added. >> help >> - Select this to enable support for the timer as defined >> - by the RISC-V privileged architecture spec. >> + Select this to enable support for a generic RISC-V S-Mode timer >> + driver. >> >> config ROCKCHIP_TIMER >> bool "Rockchip timer support" >> diff --git a/drivers/timer/riscv_timer.c b/drivers/timer/riscv_timer.c index >> 9f9f070e0b..449fcfcfd5 100644 >> --- a/drivers/timer/riscv_timer.c >> +++ b/drivers/timer/riscv_timer.c >> @@ -1,36 +1,37 @@ >> // SPDX-License-Identifier: GPL-2.0+ >> /* >> + * Copyright (C) 2020, Sean Anderson <seanga2@gmail.com> >> * Copyright (C) 2018, Bin Meng <bmeng.cn@gmail.com> >> + * Copyright (C) 2018, Anup Patel <anup@brainfault.org> >> + * Copyright (C) 2012 Regents of the University of California >> * >> - * RISC-V privileged architecture defined generic timer driver >> + * RISC-V architecturally-defined generic timer driver >> * >> - * This driver relies on RISC-V platform codes to provide the essential API >> - * riscv_get_time() which is supposed to return the timer counter as defined >> - * by the RISC-V privileged architecture spec. >> - * >> - * This driver can be used in both M-mode and S-mode U-Boot. >> + * This driver provides generic timer support for S-mode U-Boot. >> */ >> >> #include <common.h> >> #include <dm.h> >> #include <errno.h> >> #include <timer.h> >> -#include <asm/io.h> >> - >> -/** >> - * riscv_get_time() - get the timer counter >> - * >> - * Platform codes should provide this API in order to make this driver function. >> - * >> - * @time: the 64-bit timer count as defined by the RISC-V privileged >> - * architecture spec. >> - * @return: 0 on success, -ve on error. >> - */ >> -extern int riscv_get_time(u64 *time); >> +#include <asm/csr.h> >> >> static int riscv_timer_get_count(struct udevice *dev, u64 *count) { >> - return riscv_get_time(count); >> + if (IS_ENABLED(CONFIG_64BIT)) { >> + *count = csr_read(CSR_TIME); >> + } else { >> + u32 hi, lo; >> + >> + do { >> + hi = csr_read(CSR_TIMEH); >> + lo = csr_read(CSR_TIME); >> + } while (hi != csr_read(CSR_TIMEH)); >> + >> + *count = ((u64)hi << 32) | lo; >> + } >> + >> + return 0; >> } >> >> static int riscv_timer_probe(struct udevice *dev) >> -- >> 2.28.0 >
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 009a545fcf..21e6690f4d 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -185,14 +185,6 @@ config ANDES_PLMT The Andes PLMT block holds memory-mapped mtime register associated with timer tick. -config RISCV_RDTIME - bool - default y if RISCV_SMODE || SPL_RISCV_SMODE - help - The provides the riscv_get_time() API that is implemented using the - standard rdtime instruction. This is the case for S-mode U-Boot, and - is useful for processors that support rdtime in M-mode too. - config SYS_MALLOC_F_LEN default 0x1000 diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index 6c503ff2b2..10ac5b06d3 100644 --- a/arch/riscv/lib/Makefile +++ b/arch/riscv/lib/Makefile @@ -15,7 +15,6 @@ obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o obj-$(CONFIG_ANDES_PLIC) += andes_plic.o obj-$(CONFIG_ANDES_PLMT) += andes_plmt.o else -obj-$(CONFIG_RISCV_RDTIME) += rdtime.o obj-$(CONFIG_SBI) += sbi.o obj-$(CONFIG_SBI_IPI) += sbi_ipi.o endif diff --git a/arch/riscv/lib/rdtime.c b/arch/riscv/lib/rdtime.c deleted file mode 100644 index e128d7fce6..0000000000 --- a/arch/riscv/lib/rdtime.c +++ /dev/null @@ -1,38 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0+ -/* - * Copyright (C) 2018, Anup Patel <anup@brainfault.org> - * Copyright (C) 2018, Bin Meng <bmeng.cn@gmail.com> - * - * The riscv_get_time() API implementation that is using the - * standard rdtime instruction. - */ - -#include <common.h> - -/* Implement the API required by RISC-V timer driver */ -int riscv_get_time(u64 *time) -{ -#ifdef CONFIG_64BIT - u64 n; - - __asm__ __volatile__ ( - "rdtime %0" - : "=r" (n)); - - *time = n; -#else - u32 lo, hi, tmp; - - __asm__ __volatile__ ( - "1:\n" - "rdtimeh %0\n" - "rdtime %1\n" - "rdtimeh %2\n" - "bne %0, %2, 1b" - : "=&r" (hi), "=&r" (lo), "=&r" (tmp)); - - *time = ((u64)hi << 32) | lo; -#endif - - return 0; -} diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig index 637024445c..b85fa33e47 100644 --- a/drivers/timer/Kconfig +++ b/drivers/timer/Kconfig @@ -144,10 +144,10 @@ config OMAP_TIMER config RISCV_TIMER bool "RISC-V timer support" - depends on TIMER && RISCV + depends on TIMER && RISCV_SMODE help - Select this to enable support for the timer as defined - by the RISC-V privileged architecture spec. + Select this to enable support for a generic RISC-V S-Mode timer + driver. config ROCKCHIP_TIMER bool "Rockchip timer support" diff --git a/drivers/timer/riscv_timer.c b/drivers/timer/riscv_timer.c index 9f9f070e0b..449fcfcfd5 100644 --- a/drivers/timer/riscv_timer.c +++ b/drivers/timer/riscv_timer.c @@ -1,36 +1,37 @@ // SPDX-License-Identifier: GPL-2.0+ /* + * Copyright (C) 2020, Sean Anderson <seanga2@gmail.com> * Copyright (C) 2018, Bin Meng <bmeng.cn@gmail.com> + * Copyright (C) 2018, Anup Patel <anup@brainfault.org> + * Copyright (C) 2012 Regents of the University of California * - * RISC-V privileged architecture defined generic timer driver + * RISC-V architecturally-defined generic timer driver * - * This driver relies on RISC-V platform codes to provide the essential API - * riscv_get_time() which is supposed to return the timer counter as defined - * by the RISC-V privileged architecture spec. - * - * This driver can be used in both M-mode and S-mode U-Boot. + * This driver provides generic timer support for S-mode U-Boot. */ #include <common.h> #include <dm.h> #include <errno.h> #include <timer.h> -#include <asm/io.h> - -/** - * riscv_get_time() - get the timer counter - * - * Platform codes should provide this API in order to make this driver function. - * - * @time: the 64-bit timer count as defined by the RISC-V privileged - * architecture spec. - * @return: 0 on success, -ve on error. - */ -extern int riscv_get_time(u64 *time); +#include <asm/csr.h> static int riscv_timer_get_count(struct udevice *dev, u64 *count) { - return riscv_get_time(count); + if (IS_ENABLED(CONFIG_64BIT)) { + *count = csr_read(CSR_TIME); + } else { + u32 hi, lo; + + do { + hi = csr_read(CSR_TIMEH); + lo = csr_read(CSR_TIME); + } while (hi != csr_read(CSR_TIMEH)); + + *count = ((u64)hi << 32) | lo; + } + + return 0; } static int riscv_timer_probe(struct udevice *dev)