xiaoxiang781216 commented on code in PR #5977: URL: https://github.com/apache/incubator-nuttx/pull/5977#discussion_r841835098
########## arch/risc-v/src/common/riscv_mtimer.c: ########## @@ -0,0 +1,119 @@ +/**************************************************************************** + * arch/risc-v/src/common/riscv_mtimer.c + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/config.h> +#include <nuttx/arch.h> + +#include <arch/barriers.h> + +#include <stdint.h> + +#include "riscv_internal.h" + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +#define is_lt_u64(a, b) ((int64_t)((int64_t)(a) - (int64_t)(b)) < 0) +#define is_gtoreq_u64(a, b) !is_lt_u64(a, b) + +#ifdef CONFIG_ARCH_RV32 +# warning "Missing logic... Access to 64-bit registers needs fixing" +#endif + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +/* Memory mapped register base addresses */ + +static volatile uintptr_t g_mtimecmp_base; +static volatile uintptr_t g_mtime; + +/* Amount of M-mode ticks for one OS tick */ + +static volatile uint64_t g_tick; + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +void riscv_init_mtimer(uintptr_t mcmpbase, uintptr_t mtime, uint64_t tick) +{ + /* Set the memory mapped register bases */ + + g_mtimecmp_base = mcmpbase; + g_mtime = mtime; + g_tick = tick; + + /* Make sure they stick */ + + __DMB(); +} + +void riscv_reload_mtimecmp(void) +{ + uintptr_t hartid = READ_CSR(mhartid); + uintptr_t mtimecmp = (uintptr_t)((uintptr_t *) g_mtimecmp_base + hartid); Review Comment: remove space before g_mtimecmp_base ########## arch/risc-v/src/common/riscv_mtimer.c: ########## @@ -0,0 +1,119 @@ +/**************************************************************************** + * arch/risc-v/src/common/riscv_mtimer.c + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/config.h> +#include <nuttx/arch.h> + +#include <arch/barriers.h> + +#include <stdint.h> + +#include "riscv_internal.h" + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +#define is_lt_u64(a, b) ((int64_t)((int64_t)(a) - (int64_t)(b)) < 0) +#define is_gtoreq_u64(a, b) !is_lt_u64(a, b) + +#ifdef CONFIG_ARCH_RV32 +# warning "Missing logic... Access to 64-bit registers needs fixing" +#endif + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +/* Memory mapped register base addresses */ + +static volatile uintptr_t g_mtimecmp_base; +static volatile uintptr_t g_mtime; + +/* Amount of M-mode ticks for one OS tick */ + +static volatile uint64_t g_tick; + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +void riscv_init_mtimer(uintptr_t mcmpbase, uintptr_t mtime, uint64_t tick) +{ + /* Set the memory mapped register bases */ + + g_mtimecmp_base = mcmpbase; + g_mtime = mtime; + g_tick = tick; + + /* Make sure they stick */ + + __DMB(); +} + +void riscv_reload_mtimecmp(void) +{ + uintptr_t hartid = READ_CSR(mhartid); + uintptr_t mtimecmp = (uintptr_t)((uintptr_t *) g_mtimecmp_base + hartid); + uint64_t value = getreg64(mtimecmp); + uint64_t time = getreg64(g_mtime); + uint64_t tick = g_tick; + + /* Reload time and make sure the time is in the future. This is ESSENTIAL + * as the timer will keep interrupting as long as time >= mtimecmp, thus + * if the time is very far in the future, the system will be starved as + * we will be servicing this ISR over and over. + */ + + if (is_lt_u64(time + tick, value)) + { + /* Normal case when booting, mtimecmp can point far into the future */ + + value = time + tick; + } + else if (is_lt_u64(value + tick, time)) + { + /* Handle case where 1 (or more) ticks is missed */ + + value = time + tick; Review Comment: in this case, we should generate the interrupt more quick(or immediately), otherwise we can't catch up the interrupt losing and the timing info will drift bigger and bigger in the long run. ########## arch/risc-v/src/common/riscv_mtimer.c: ########## @@ -0,0 +1,119 @@ +/**************************************************************************** + * arch/risc-v/src/common/riscv_mtimer.c + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/config.h> +#include <nuttx/arch.h> + +#include <arch/barriers.h> + +#include <stdint.h> + +#include "riscv_internal.h" + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +#define is_lt_u64(a, b) ((int64_t)((int64_t)(a) - (int64_t)(b)) < 0) +#define is_gtoreq_u64(a, b) !is_lt_u64(a, b) + +#ifdef CONFIG_ARCH_RV32 +# warning "Missing logic... Access to 64-bit registers needs fixing" +#endif + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +/* Memory mapped register base addresses */ + +static volatile uintptr_t g_mtimecmp_base; +static volatile uintptr_t g_mtime; + +/* Amount of M-mode ticks for one OS tick */ + +static volatile uint64_t g_tick; + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +void riscv_init_mtimer(uintptr_t mcmpbase, uintptr_t mtime, uint64_t tick) +{ + /* Set the memory mapped register bases */ + + g_mtimecmp_base = mcmpbase; + g_mtime = mtime; + g_tick = tick; + + /* Make sure they stick */ + + __DMB(); Review Comment: don't need barrier? ########## arch/risc-v/src/common/riscv_mtimer.c: ########## @@ -0,0 +1,119 @@ +/**************************************************************************** + * arch/risc-v/src/common/riscv_mtimer.c + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/config.h> +#include <nuttx/arch.h> + +#include <arch/barriers.h> + +#include <stdint.h> + +#include "riscv_internal.h" + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +#define is_lt_u64(a, b) ((int64_t)((int64_t)(a) - (int64_t)(b)) < 0) +#define is_gtoreq_u64(a, b) !is_lt_u64(a, b) + +#ifdef CONFIG_ARCH_RV32 +# warning "Missing logic... Access to 64-bit registers needs fixing" +#endif + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +/* Memory mapped register base addresses */ + +static volatile uintptr_t g_mtimecmp_base; +static volatile uintptr_t g_mtime; + +/* Amount of M-mode ticks for one OS tick */ + +static volatile uint64_t g_tick; + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +void riscv_init_mtimer(uintptr_t mcmpbase, uintptr_t mtime, uint64_t tick) Review Comment: do you consider to support the tickless mode or the cpu loading? It's more simple to implement oneshot driver interface: https://github.com/apache/incubator-nuttx/blob/master/include/nuttx/timers/oneshot.h Here is some example: https://github.com/apache/incubator-nuttx/blob/master/arch/xtensa/src/common/xtensa_oneshot.c https://github.com/apache/incubator-nuttx/blob/master/arch/sim/src/sim/up_oneshot.c And than implement arch timer/alarm/tick related api on top of it. Or directly reuse up_alarm_xx wrapper: https://github.com/apache/incubator-nuttx/blob/master/include/nuttx/timers/arch_alarm.h https://github.com/apache/incubator-nuttx/blob/master/drivers/timers/arch_alarm.c -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
