[libcpu/rv64] feat: unify tick.c #9164

polarvid · 2024-07-09T12:17:19Z

拉取/合并请求描述：(PR description)

[

为什么提交这份PR (why to submit this PR)

The changes unify the tick.c implementations for all risc-v64 architectures, leveraging the CPUTIME feature. This refactoring was necessary to streamline the codebase, and ensure consistent timer handling across different platforms.

你的解决方案是什么 (what is your solution)

Changes:

Updated Kconfig in bsp/cvitek/cv18xx_risc-v to fix formatting issues.
Added default values for CPUTIME_TIMER_FREQ in components/drivers/cputime/Kconfig.
Refactored tick.c and tick.h in libcpu/risc-v/t-head/c906 and libcpu/risc-v/virt64:
- Replaced direct use of rdtime with clock_cpu_gettime.
- Removed redundant timer frequency definitions.
- Added static assertions to check the value of CPUTIME_TIMER_FREQ.
- Initialized tick_cycles based on CPUTIME_TIMER_FREQ.
- Integrated ktime support for tick initialization.

请提供验证的bsp和config (provide the config and bsp)

BSP: QEMU virt64, Persim M7, milkv duo256

.config:

action: sending a byte to uart with rt_thread_delay() in 1 seconds.

milkv duo

QEMU: the simulation is not very reliable on real world timing. So the test is roughly done with a real stopwatch, and it works : )

Persim M7

]

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality：

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

已经仔细查看过代码改动的对比 Already check the difference between PR and old code
代码风格正确，包括缩进空格，命名及其他风格 Style guide is adhered to, including spacing, naming and other styles
没有垃圾代码，代码尽量精简，不包含#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up
所有变更均有原因及合理的，并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
代码是高质量的 Code in this PR is of high quality
已经使用formatting 等源码格式化工具确保格式符合RT-Thread代码规范 This PR complies with RT-Thread code specification

components/drivers/cputime/Kconfig

libcpu/risc-v/virt64/tick.c

unicornx

review 完了，感觉还有不少问题。请看一下。

BTW：重新 commit 时请加上我的 review 签名, thanks：

Reviewed-by: Chen Wang [email protected]

unicornx · 2024-07-12T09:02:07Z

补充一个问题，我发现 RT_USING_CPUTIME 这个配置并没有在 bsp/cvitek 中开启，你那边是怎么编译链接进 components/drivers/cputime/cputime_riscv.c 这个文件的？是不是需要给 bsp/cvitek 加一下？我看 bsp/qemu-virt64-riscv 里是开了的。

如果要加的话，同样建议新建一个 commit 在 bsp 层级做修改。

unicornx · 2024-07-12T09:02:55Z

请提供验证的bsp和config (provide the config and bsp)

BSP: QEMU virt64, Persim M7, milkv duo256

Persim M7 是哪个 bsp ？

unicornx · 2024-07-12T09:26:27Z

走读代码中我有一个疑问：

目前这个改动，tick.c 中会将调用 clock_cpu_gettime() 来获取硬件 tick 数，而这个 API 依赖于 cputime 中 rt_clock_cputime_ops 的回调需要设置好。而 cputime 回调的设置是通过 INIT_BOARD_EXPORT 机制实现的，见 components/drivers/cputime/cputime_riscv.c

int riscv_cputime_init(void)
{
    clock_cpu_setops(&_riscv_ops);
    return 0;
}
INIT_BOARD_EXPORT(riscv_cputime_init);

可是根据启动流程，以 virt64 为例：

rt_hw_board_init
{
......
rt_hw_tick_init // 这里已经开始调用 `clock_cpu_gettime()`
rt_components_board_init // 这里才开始初始化 board init 函数，即调用 riscv_cputime_init
......
}

我感觉这里时序上存在问题的呀，rt_hw_tick_init 中调用 clock_cpu_gettime() 会取不到的。原来 tick.c 中自己实现了 get_ticks() 是不是就是避免了这个原因啊？

请再确认一下。

其实我还有个疑问，可能是代码设计上的问题，
我们为啥要在 components/drivers/cputime 里放一个 cputime_riscv.c, 我感觉这些都是 ARCH 相关实现，应该放到 libcpu 下去为好。你说呢？

polarvid · 2024-07-12T11:43:10Z

其实我还有个疑问，可能是代码设计上的问题，
我们为啥要在 components/drivers/cputime 里放一个 cputime_riscv.c, 我感觉这些都是 ARCH 相关实现，应该放到 libcpu 下去为好

tick 来自于 timer，本质上是一个设备。同时它的配置也属于设备管理的一部分，因此会放在 driver，由驱动框架实现支持。而 libcpu 里面一般就是纯粹内核依赖的移植代码。从这一点上看，调度管理对于 tick 的依赖也不是强制的，因此放在 libcpu 也不是很有必要。综上，tick.c 其实后续会从 libcpu 移除出去。

随着这个思路，这个 PR 的修改原则是以迭代的方式逐步把 tick 的实现移出 libcpu。但是当前 rv 的 timer 驱动是在 cputime 框架实现的。它存在很多怪异的地方，这里不展开了。同样领域完整度更高的框架是在 components 目录下的 hwtimer 框架。所以从我的角度来说，这个 cputime 仅仅是个历史遗留问题，后续应该是统一到 hwtimer 框架中去。

这里时序上存在问题的呀，rt_hw_tick_init 中调用 clock_cpu_gettime() 会取不到的

时序问题需要结合上面所说来解释。因为后续演进方向是驱动化 timer，因此 rt_hw_tick_init() 操作本身不应该是 rt_hw_board_init 直接去调用的操作。因为逻辑上这是某种驱动初始化，会在驱动框架实现。

但又因为这份修改不倾向于再去改变 cputime 的实现（因为它后续将完全放弃，转向更现代化的 hwtimer 框架），所以在 cputime 框架保持不变的前提下，这里的实现其实是取巧的。

首先 clock_cpu_gettime() 没有 ops 时返回 0，然后把它与 tick_cycles 的和写入 cmp 寄存器。而 clint timer 的原理是每个时钟周期比较绝对时间跟 cmp 寄存器的值。超过则触发一个 timer irq。所以这个代码从功能上是不存在问题的。

不过现在来看逻辑上确实容易让人误会。所以我会补加一个逻辑，在 rt_hw_tick_init 中初始化 cputime 框架。

polarvid · 2024-07-12T12:26:04Z

请提供验证的bsp和config (provide the config and bsp)

BSP: QEMU virt64, Persim M7, milkv duo256

Persim M7 是哪个 bsp ？

请参考 https://github.com/RT-Thread/rt-thread/blob/master/bsp/allwinner/d1s/README-M7.md 。

unicornx · 2024-07-12T12:30:00Z

其实我还有个疑问，可能是代码设计上的问题，
我们为啥要在 components/drivers/cputime 里放一个 cputime_riscv.c, 我感觉这些都是 ARCH 相关实现，应该放到 libcpu 下去为好

tick 来自于 timer，本质上是一个设备。同时它的配置也属于设备管理的一部分，因此会放在 driver，由驱动框架实现支持。而 libcpu 里面一般就是纯粹内核依赖的移植代码。从这一点上看，调度管理对于 tick 的依赖也不是强制的，因此放在 libcpu 也不是很有必要。综上，tick.c 其实后续会从 libcpu 移除出去。

随着这个思路，这个 PR 的修改原则是以迭代的方式逐步把 tick 的实现移出 libcpu。但是当前 rv 的 timer 驱动是在 cputime 框架实现的。它存在很多怪异的地方，这里不展开了。同样领域完整度更高的框架是在 components 目录下的 hwtimer 框架。所以从我的角度来说，这个 cputime 仅仅是个历史遗留问题，后续应该是统一到 hwtimer 框架中去。

这里时序上存在问题的呀，rt_hw_tick_init 中调用 clock_cpu_gettime() 会取不到的

时序问题需要结合上面所说来解释。因为后续演进方向是驱动化 timer，因此 rt_hw_tick_init() 操作本身不应该是 rt_hw_board_init 直接去调用的操作。因为逻辑上这是某种驱动初始化，会在驱动框架实现。

但又因为这份修改不倾向于再去改变 cputime 的实现（因为它后续将完全放弃，转向更现代化的 hwtimer 框架），所以在 cputime 框架保持不变的前提下，这里的实现其实是取巧的。

首先 clock_cpu_gettime() 没有 ops 时返回 0，然后把它与 tick_cycles 的和写入 cmp 寄存器。而 clint timer 的原理是每个时钟周期比较绝对时间跟 cmp 寄存器的值。超过则触发一个 timer irq。所以这个代码从功能上是不存在问题的。

不过现在来看逻辑上确实容易让人误会。所以我会补加一个逻辑，在 rt_hw_tick_init 中初始化 cputime 框架。

可能我对 RTT 的理解还不够深入，我目前有个感觉就是 RTT 里和 ARCH 相关的代码会散布在很多地方，src/component/libcpu/...。我以前一直以为 ARCH 相关的东西都应该集中在一个地方为好，在 RTT 里感觉似乎就是 libcpu 下, 就像 linux 有个 arch 目录，其他的目录下都是纯 c 的逻辑（算法相关）的。

polarvid · 2024-07-12T12:33:22Z

我个人的理解是这样，除了提供特殊指令实现 MMIO 之外，这些逻辑其实和“架构”关系不大。只是驱动的特殊化例子。

或者参考 Linux 的话，所有 clock-source 也是在 arch 之外独立的目录下维护的。路径是 ./drivers/clocksource/timer-riscv.c 。

The changes unify the tick.c implementations for all risc-v64 architectures, leveraging the CPUTIME feature. This refactoring was necessary to streamline the codebase, and ensure consistent timer handling across different platforms. Changes: - Updated `Kconfig` in `bsp/cvitek/cv18xx_risc-v` to fix formatting issues. - Updated .config for BSPs to update `CPUTIME_TIMER_FREQ` - Updated header of for API `riscv_cputime_init` - Initialized riscv timer on `rt_hw_tick_init` - Refactored `tick.c` and `tick.h` in `libcpu/risc-v/t-head/c906` and `libcpu/risc-v/virt64`: - Replaced direct use of `rdtime` with `clock_cpu_gettime`. - Removed redundant timer frequency definitions. - Added static assertions to check the value of `CPUTIME_TIMER_FREQ`. - Initialized `tick_cycles` based on `CPUTIME_TIMER_FREQ`. - Integrated `ktime` support for tick initialization. Signed-off-by: Shell <[email protected]> Reviewed-on: RT-Thread#9164 Reviewed-by: Chen Wang <[email protected]>

polarvid · 2024-07-15T05:21:51Z

@unicornx

感谢 REVIEW。我参考你的评论做了一些补充和修改。

unicornx · 2024-07-17T01:52:01Z

第二次 review：

其他没有意见，我已经 approve 了。

但是有个问题是：我看更新有关 config，这个主线上代码最近的 merge 也有相关更新（特别是 bsp/cvitek 部分），这里最后 merge 之前是否需要重新 menuconfig 一下？

btw，有个疑问就是对 config 的更新处理，像这里遇到的额问题一样，如果多个 pr 都会更新，冲突该如何保证？

还有就是怎么多个 commit？要不要 squash 一下？

polarvid · 2024-07-17T02:00:17Z

我看更新有关 config，这个主线上代码最近的 merge 也有相关更新（特别是 bsp/cvitek 部分），这里最后 merge 之前是否需要重新 menuconfig 一下？

btw，有个疑问就是对 config 的更新处理，像这里遇到的额问题一样，如果多个 pr 都会更新，冲突该如何保证？

.config 应该不需要维护，特别是注释掉的文本。只要没注释的文本是正确的，用户使用的时候先 menuconfig 后再编译就没有问题。

polarvid mentioned this pull request Jul 9, 2024

[Feature] Remove redundant codes for risc-v libcpu porting #9151

Closed

5 tasks

polarvid marked this pull request as ready for review July 9, 2024 12:29

polarvid added the BSP: Cvitek BSP related with cvitek label Jul 11, 2024

BernardXiong requested a review from unicornx July 12, 2024 01:30

polarvid added Arch: RISC-V BSP related with risc-v and removed BSP: Cvitek BSP related with cvitek labels Jul 12, 2024

unicornx reviewed Jul 12, 2024

View reviewed changes

components/drivers/cputime/Kconfig Outdated Show resolved Hide resolved

unicornx reviewed Jul 12, 2024

View reviewed changes

libcpu/risc-v/virt64/tick.c Show resolved Hide resolved

unicornx suggested changes Jul 12, 2024

View reviewed changes

polarvid force-pushed the shell/rv64-tick branch from 30f3ac4 to 58508fe Compare July 15, 2024 04:03

polarvid force-pushed the shell/rv64-tick branch from 58508fe to 65ffe4e Compare July 15, 2024 04:09

Merge branch 'master' into shell/rv64-tick

f6f570d

unicornx approved these changes Jul 17, 2024

View reviewed changes

BernardXiong approved these changes Jul 17, 2024

View reviewed changes

BernardXiong merged commit 019b8cd into RT-Thread:master Jul 17, 2024
44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libcpu/rv64] feat: unify tick.c #9164

[libcpu/rv64] feat: unify tick.c #9164

polarvid commented Jul 9, 2024 •

edited

Loading

unicornx left a comment •

edited

Loading

unicornx commented Jul 12, 2024

unicornx commented Jul 12, 2024 •

edited

Loading

请提供验证的bsp和config (provide the config and bsp)

unicornx commented Jul 12, 2024

polarvid commented Jul 12, 2024 •

edited

Loading

polarvid commented Jul 12, 2024

请提供验证的bsp和config (provide the config and bsp)

unicornx commented Jul 12, 2024

polarvid commented Jul 12, 2024 •

edited

Loading

polarvid commented Jul 15, 2024

unicornx commented Jul 17, 2024 •

edited

Loading

polarvid commented Jul 17, 2024

[libcpu/rv64] feat: unify tick.c #9164

[libcpu/rv64] feat: unify tick.c #9164

Conversation

polarvid commented Jul 9, 2024 • edited Loading

拉取/合并请求描述：(PR description)

为什么提交这份PR (why to submit this PR)

你的解决方案是什么 (what is your solution)

请提供验证的bsp和config (provide the config and bsp)

当前拉取/合并请求的状态 Intent for your PR

代码质量 Code Quality：

unicornx left a comment • edited Loading

Choose a reason for hiding this comment

unicornx commented Jul 12, 2024

unicornx commented Jul 12, 2024 • edited Loading

请提供验证的bsp和config (provide the config and bsp)

unicornx commented Jul 12, 2024

polarvid commented Jul 12, 2024 • edited Loading

polarvid commented Jul 12, 2024

请提供验证的bsp和config (provide the config and bsp)

unicornx commented Jul 12, 2024

polarvid commented Jul 12, 2024 • edited Loading

polarvid commented Jul 15, 2024

unicornx commented Jul 17, 2024 • edited Loading

polarvid commented Jul 17, 2024

polarvid commented Jul 9, 2024 •

edited

Loading

unicornx left a comment •

edited

Loading

unicornx commented Jul 12, 2024 •

edited

Loading

polarvid commented Jul 12, 2024 •

edited

Loading

polarvid commented Jul 12, 2024 •

edited

Loading

unicornx commented Jul 17, 2024 •

edited

Loading