Re: [PATCH v3 1/7] riscv: Rework riscv timer driver to only support S-mode
On 9/8/20 3:57 AM, Pragnesh Patel wrote:
> Hi Sean,
>
>> -Original Message-
>> From: Sean Anderson
>> Sent: 01 September 2020 16:02
>> To: u-boot@lists.denx.de
>> Cc: Rick Chen ; Bin Meng ;
>> Pragnesh Patel ; Sean Anderson
>> ; Bin Meng ; Anup Patel
>>
>> 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
>> Reviewed-by: Bin Meng
>> ---
>>
>> (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..00
>> --- a/arch/riscv/lib/rdtime.c
>> +++ /dev/null
>> @@ -1,38 +0,0 @@
>> -// SPDX-License-Identifier: GPL-2.0+
>> -/*
>> - * Copyright (C) 2018, Anup Patel
>> - * Copyright (C) 2018, Bin Meng
>> - *
>> - * The riscv_get_time() API implementation that is using the
>> - * standard rdtime instruction.
>> - */
>> -
>> -#include
>> -
>> -/* 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 d
RE: [PATCH v3 1/7] riscv: Rework riscv timer driver to only support S-mode
Hi Sean,
>-Original Message-
>From: Sean Anderson
>Sent: 01 September 2020 16:02
>To: u-boot@lists.denx.de
>Cc: Rick Chen ; Bin Meng ;
>Pragnesh Patel ; Sean Anderson
>; Bin Meng ; Anup Patel
>
>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
>Reviewed-by: Bin Meng
>---
>
>(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..00
>--- a/arch/riscv/lib/rdtime.c
>+++ /dev/null
>@@ -1,38 +0,0 @@
>-// SPDX-License-Identifier: GPL-2.0+
>-/*
>- * Copyright (C) 2018, Anup Patel
>- * Copyright (C) 2018, Bin Meng
>- *
>- * The riscv_get_time() API implementation that is using the
>- * standard rdtime instruction.
>- */
>-
>-#include
>-
>-/* 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/time
Re: [PATCH v3 1/7] riscv: Rework riscv timer driver to only support S-mode
> > 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 > Reviewed-by: Bin Meng > --- > > (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
